Re: [PATCH 2/5] drm/bridge: samsung-dsim: reread ref clock before configuring PLL
2023년 8월 29일 (화) 오전 12:59, Michael Tretter 님이 작성:
> The PLL reference clock may change at runtime. Thus, reading the clock
> rate during probe is not sufficient to correctly configure the PLL for
> the expected hs clock.
>
> Read the actual rate of the reference clock before calculating the PLL
> configuration parameters.
>
> Signed-off-by: Michael Tretter
>
Reviewed-by: Inki Dae
Acked-by: Inki Dae
Thanks,
Inki Dae
---
> drivers/gpu/drm/bridge/samsung-dsim.c | 16 +---
> include/drm/bridge/samsung-dsim.h | 1 +
> 2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 6778f1751faa..da90c2038042 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -611,7 +611,12 @@ static unsigned long samsung_dsim_set_pll(struct
> samsung_dsim *dsi,
> u16 m;
> u32 reg;
>
> - fin = dsi->pll_clk_rate;
> + if (dsi->pll_clk)
> + fin = clk_get_rate(dsi->pll_clk);
> + else
> + fin = dsi->pll_clk_rate;
> + dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
> +
> fout = samsung_dsim_pll_find_pms(dsi, fin, freq, &p, &m, &s);
> if (!fout) {
> dev_err(dsi->dev,
> @@ -1821,18 +1826,15 @@ static int samsung_dsim_parse_dt(struct
> samsung_dsim *dsi)
> u32 lane_polarities[5] = { 0 };
> struct device_node *endpoint;
> int i, nr_lanes, ret;
> - struct clk *pll_clk;
>
> ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
>&dsi->pll_clk_rate, 1);
> /* If it doesn't exist, read it from the clock instead of failing
> */
> if (ret < 0) {
> dev_dbg(dev, "Using sclk_mipi for pll clock frequency\n");
> - pll_clk = devm_clk_get(dev, "sclk_mipi");
> - if (!IS_ERR(pll_clk))
> - dsi->pll_clk_rate = clk_get_rate(pll_clk);
> - else
> - return PTR_ERR(pll_clk);
> + dsi->pll_clk = devm_clk_get(dev, "sclk_mipi");
> + if (IS_ERR(dsi->pll_clk))
> + return PTR_ERR(dsi->pll_clk);
> }
>
> /* If it doesn't exist, use pixel clock instead of failing */
> diff --git a/include/drm/bridge/samsung-dsim.h
> b/include/drm/bridge/samsung-dsim.h
> index 05100e91ecb9..31ff88f152fb 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -87,6 +87,7 @@ struct samsung_dsim {
> void __iomem *reg_base;
> struct phy *phy;
> struct clk **clks;
> + struct clk *pll_clk;
> struct regulator_bulk_data supplies[2];
> int irq;
> struct gpio_desc *te_gpio;
>
> --
> 2.39.2
>
>
Re: [PATCH 2/5] drm/bridge: samsung-dsim: reread ref clock before configuring PLL
2023년 9월 4일 (월) 오후 8:05, Michael Tretter 님이 작성:
> On Mon, 04 Sep 2023 13:38:33 +0900, Inki Dae wrote:
> > 2023년 8월 29일 (화) 오전 12:59, Michael Tretter 님이
> 작성:
> >
> > >
> > > The PLL reference clock may change at runtime. Thus, reading the clock
> > > rate during probe is not sufficient to correctly configure the PLL for
> > > the expected hs clock.
> > >
> > > Read the actual rate of the reference clock before calculating the PLL
> > > configuration parameters.
> > >
> > > Signed-off-by: Michael Tretter
> > > ---
> > > drivers/gpu/drm/bridge/samsung-dsim.c | 16 +---
> > > include/drm/bridge/samsung-dsim.h | 1 +
> > > 2 files changed, 10 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > index 6778f1751faa..da90c2038042 100644
> > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > @@ -611,7 +611,12 @@ static unsigned long samsung_dsim_set_pll(struct
> samsung_dsim *dsi,
> > > u16 m;
> > > u32 reg;
> > >
> > > - fin = dsi->pll_clk_rate;
> > > + if (dsi->pll_clk)
> > > + fin = clk_get_rate(dsi->pll_clk);
> > > + else
> > > + fin = dsi->pll_clk_rate;
> > > + dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
> > > +
> >
> > Could you share us the actual use case that in runtime the pll
> > reference clock can be changed?
>
> On i.MX8M Nano, the VIDEO_PLL_CLK drives the DISPLAY_PIXEL_CLK_ROOT, which
> is
> used as pixel clock by the LCDIF. Changes to the pixel clock propagate to
> the
> VIDEO_PLL_CLK and may reconfigure the VIDEO_PLL_CLK. This is done to reduce
> the error on the pixel clock.
>
> As the ADV3575 as MIPI-DSI device reconstructs the pixel clock, it is
> necessary to keep the pixel clock and MIDI-DSI reference clock in
> sync. This can be done by using the VIDEO_PLL_CLK to drive the PLL
> reference
> clock (MIPI_DSI_CORE_CLK_ROOT). Without this, a connected HDMI Monitor will
> occasionally loose sync.
>
> In this setup, a mode change that changes the pixel clock may change the
> VIDEO_PLL, which will change the PLL reference clock.
>
Thanks for sharing.
> >
> > This patch is trying to change clock binding behavior which is
> > described in dt binding[1]
> > [1]
> Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
> >
> > It says,
> > "DISM oscillator clock frequency. If absent, the clock frequency of
> > sclk_mipi will be used instead."
> >
> > However, this patch makes the sclk_mipi to be used first.
>
> No, the behavior, as described in the dt binding, is preserved by the hunk
> below. dsi->pll_clk is only set, if the samsung,pll-clock-frequency
> property
> is absent. If the dt property exists, dsi->pll_clk will be NULL and
> dsi->pll_clk_rate will be used here.
>
It's true. No behavior change. There was my mistake. Thanks. :)
> Michael
>
> >
> > Thanks,
> > Inki Dae
> >
> > > fout = samsung_dsim_pll_find_pms(dsi, fin, freq, &p, &m, &s);
> > > if (!fout) {
> > > dev_err(dsi->dev,
> > > @@ -1821,18 +1826,15 @@ static int samsung_dsim_parse_dt(struct
> samsung_dsim *dsi)
> > > u32 lane_polarities[5] = { 0 };
> > > struct device_node *endpoint;
> > > int i, nr_lanes, ret;
> > > - struct clk *pll_clk;
> > >
> > > ret = samsung_dsim_of_read_u32(node,
> "samsung,pll-clock-frequency",
> > >&dsi->pll_clk_rate, 1);
> > > /* If it doesn't exist, read it from the clock instead of
> failing */
> > > if (ret < 0) {
> > > dev_dbg(dev, "Using sclk_mipi for pll clock
> frequency\n");
> > > - pll_clk = devm_clk_get(dev, "sclk_mipi");
> > > - if (!IS_ERR(pll_clk))
> > > - dsi->pll_clk_rate = clk_get_rate(pll_clk);
> > > - else
> > > - return PTR_ERR(pll_clk);
> > > + dsi->pll_clk = devm_clk_get(dev, "sclk_mipi");
> > > + if (IS_ERR(dsi->pll_clk))
> > > + return PTR_ERR(dsi->pll_clk);
> > > }
> > >
> > > /* If it doesn't exist, use pixel clock instead of failing */
> > > diff --git a/include/drm/bridge/samsung-dsim.h
> b/include/drm/bridge/samsung-dsim.h
> > > index 05100e91ecb9..31ff88f152fb 100644
> > > --- a/include/drm/bridge/samsung-dsim.h
> > > +++ b/include/drm/bridge/samsung-dsim.h
> > > @@ -87,6 +87,7 @@ struct samsung_dsim {
> > > void __iomem *reg_base;
> > > struct phy *phy;
> > > struct clk **clks;
> > > + struct clk *pll_clk;
> > > struct regulator_bulk_data supplies[2];
> > > int irq;
> > > struct gpio_desc *te_gpio;
> > >
> > > --
> > > 2.39.2
> > >
> >
>
Re: [PATCH 2/5] drm/bridge: samsung-dsim: reread ref clock before configuring PLL
On Mon, 04 Sep 2023 13:38:33 +0900, Inki Dae wrote:
> 2023년 8월 29일 (화) 오전 12:59, Michael Tretter 님이 작성:
>
> >
> > The PLL reference clock may change at runtime. Thus, reading the clock
> > rate during probe is not sufficient to correctly configure the PLL for
> > the expected hs clock.
> >
> > Read the actual rate of the reference clock before calculating the PLL
> > configuration parameters.
> >
> > Signed-off-by: Michael Tretter
> > ---
> > drivers/gpu/drm/bridge/samsung-dsim.c | 16 +---
> > include/drm/bridge/samsung-dsim.h | 1 +
> > 2 files changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 6778f1751faa..da90c2038042 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -611,7 +611,12 @@ static unsigned long samsung_dsim_set_pll(struct
> > samsung_dsim *dsi,
> > u16 m;
> > u32 reg;
> >
> > - fin = dsi->pll_clk_rate;
> > + if (dsi->pll_clk)
> > + fin = clk_get_rate(dsi->pll_clk);
> > + else
> > + fin = dsi->pll_clk_rate;
> > + dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
> > +
>
> Could you share us the actual use case that in runtime the pll
> reference clock can be changed?
On i.MX8M Nano, the VIDEO_PLL_CLK drives the DISPLAY_PIXEL_CLK_ROOT, which is
used as pixel clock by the LCDIF. Changes to the pixel clock propagate to the
VIDEO_PLL_CLK and may reconfigure the VIDEO_PLL_CLK. This is done to reduce
the error on the pixel clock.
As the ADV3575 as MIPI-DSI device reconstructs the pixel clock, it is
necessary to keep the pixel clock and MIDI-DSI reference clock in
sync. This can be done by using the VIDEO_PLL_CLK to drive the PLL reference
clock (MIPI_DSI_CORE_CLK_ROOT). Without this, a connected HDMI Monitor will
occasionally loose sync.
In this setup, a mode change that changes the pixel clock may change the
VIDEO_PLL, which will change the PLL reference clock.
>
> This patch is trying to change clock binding behavior which is
> described in dt binding[1]
> [1] Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
>
> It says,
> "DISM oscillator clock frequency. If absent, the clock frequency of
> sclk_mipi will be used instead."
>
> However, this patch makes the sclk_mipi to be used first.
No, the behavior, as described in the dt binding, is preserved by the hunk
below. dsi->pll_clk is only set, if the samsung,pll-clock-frequency property
is absent. If the dt property exists, dsi->pll_clk will be NULL and
dsi->pll_clk_rate will be used here.
Michael
>
> Thanks,
> Inki Dae
>
> > fout = samsung_dsim_pll_find_pms(dsi, fin, freq, &p, &m, &s);
> > if (!fout) {
> > dev_err(dsi->dev,
> > @@ -1821,18 +1826,15 @@ static int samsung_dsim_parse_dt(struct
> > samsung_dsim *dsi)
> > u32 lane_polarities[5] = { 0 };
> > struct device_node *endpoint;
> > int i, nr_lanes, ret;
> > - struct clk *pll_clk;
> >
> > ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
> >&dsi->pll_clk_rate, 1);
> > /* If it doesn't exist, read it from the clock instead of failing */
> > if (ret < 0) {
> > dev_dbg(dev, "Using sclk_mipi for pll clock frequency\n");
> > - pll_clk = devm_clk_get(dev, "sclk_mipi");
> > - if (!IS_ERR(pll_clk))
> > - dsi->pll_clk_rate = clk_get_rate(pll_clk);
> > - else
> > - return PTR_ERR(pll_clk);
> > + dsi->pll_clk = devm_clk_get(dev, "sclk_mipi");
> > + if (IS_ERR(dsi->pll_clk))
> > + return PTR_ERR(dsi->pll_clk);
> > }
> >
> > /* If it doesn't exist, use pixel clock instead of failing */
> > diff --git a/include/drm/bridge/samsung-dsim.h
> > b/include/drm/bridge/samsung-dsim.h
> > index 05100e91ecb9..31ff88f152fb 100644
> > --- a/include/drm/bridge/samsung-dsim.h
> > +++ b/include/drm/bridge/samsung-dsim.h
> > @@ -87,6 +87,7 @@ struct samsung_dsim {
> > void __iomem *reg_base;
> > struct phy *phy;
> > struct clk **clks;
> > + struct clk *pll_clk;
> > struct regulator_bulk_data supplies[2];
> > int irq;
> > struct gpio_desc *te_gpio;
> >
> > --
> > 2.39.2
> >
>
Re: [PATCH 2/5] drm/bridge: samsung-dsim: reread ref clock before configuring PLL
2023년 8월 29일 (화) 오전 12:59, Michael Tretter 님이 작성:
>
> The PLL reference clock may change at runtime. Thus, reading the clock
> rate during probe is not sufficient to correctly configure the PLL for
> the expected hs clock.
>
> Read the actual rate of the reference clock before calculating the PLL
> configuration parameters.
>
> Signed-off-by: Michael Tretter
> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 16 +---
> include/drm/bridge/samsung-dsim.h | 1 +
> 2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 6778f1751faa..da90c2038042 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -611,7 +611,12 @@ static unsigned long samsung_dsim_set_pll(struct
> samsung_dsim *dsi,
> u16 m;
> u32 reg;
>
> - fin = dsi->pll_clk_rate;
> + if (dsi->pll_clk)
> + fin = clk_get_rate(dsi->pll_clk);
> + else
> + fin = dsi->pll_clk_rate;
> + dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
> +
Could you share us the actual use case that in runtime the pll
reference clock can be changed?
This patch is trying to change clock binding behavior which is
described in dt binding[1]
[1] Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
It says,
"DISM oscillator clock frequency. If absent, the clock frequency of
sclk_mipi will be used instead."
However, this patch makes the sclk_mipi to be used first.
Thanks,
Inki Dae
> fout = samsung_dsim_pll_find_pms(dsi, fin, freq, &p, &m, &s);
> if (!fout) {
> dev_err(dsi->dev,
> @@ -1821,18 +1826,15 @@ static int samsung_dsim_parse_dt(struct samsung_dsim
> *dsi)
> u32 lane_polarities[5] = { 0 };
> struct device_node *endpoint;
> int i, nr_lanes, ret;
> - struct clk *pll_clk;
>
> ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
>&dsi->pll_clk_rate, 1);
> /* If it doesn't exist, read it from the clock instead of failing */
> if (ret < 0) {
> dev_dbg(dev, "Using sclk_mipi for pll clock frequency\n");
> - pll_clk = devm_clk_get(dev, "sclk_mipi");
> - if (!IS_ERR(pll_clk))
> - dsi->pll_clk_rate = clk_get_rate(pll_clk);
> - else
> - return PTR_ERR(pll_clk);
> + dsi->pll_clk = devm_clk_get(dev, "sclk_mipi");
> + if (IS_ERR(dsi->pll_clk))
> + return PTR_ERR(dsi->pll_clk);
> }
>
> /* If it doesn't exist, use pixel clock instead of failing */
> diff --git a/include/drm/bridge/samsung-dsim.h
> b/include/drm/bridge/samsung-dsim.h
> index 05100e91ecb9..31ff88f152fb 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -87,6 +87,7 @@ struct samsung_dsim {
> void __iomem *reg_base;
> struct phy *phy;
> struct clk **clks;
> + struct clk *pll_clk;
> struct regulator_bulk_data supplies[2];
> int irq;
> struct gpio_desc *te_gpio;
>
> --
> 2.39.2
>
[PATCH 2/5] drm/bridge: samsung-dsim: reread ref clock before configuring PLL
The PLL reference clock may change at runtime. Thus, reading the clock
rate during probe is not sufficient to correctly configure the PLL for
the expected hs clock.
Read the actual rate of the reference clock before calculating the PLL
configuration parameters.
Signed-off-by: Michael Tretter
---
drivers/gpu/drm/bridge/samsung-dsim.c | 16 +---
include/drm/bridge/samsung-dsim.h | 1 +
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 6778f1751faa..da90c2038042 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -611,7 +611,12 @@ static unsigned long samsung_dsim_set_pll(struct
samsung_dsim *dsi,
u16 m;
u32 reg;
- fin = dsi->pll_clk_rate;
+ if (dsi->pll_clk)
+ fin = clk_get_rate(dsi->pll_clk);
+ else
+ fin = dsi->pll_clk_rate;
+ dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
+
fout = samsung_dsim_pll_find_pms(dsi, fin, freq, &p, &m, &s);
if (!fout) {
dev_err(dsi->dev,
@@ -1821,18 +1826,15 @@ static int samsung_dsim_parse_dt(struct samsung_dsim
*dsi)
u32 lane_polarities[5] = { 0 };
struct device_node *endpoint;
int i, nr_lanes, ret;
- struct clk *pll_clk;
ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
&dsi->pll_clk_rate, 1);
/* If it doesn't exist, read it from the clock instead of failing */
if (ret < 0) {
dev_dbg(dev, "Using sclk_mipi for pll clock frequency\n");
- pll_clk = devm_clk_get(dev, "sclk_mipi");
- if (!IS_ERR(pll_clk))
- dsi->pll_clk_rate = clk_get_rate(pll_clk);
- else
- return PTR_ERR(pll_clk);
+ dsi->pll_clk = devm_clk_get(dev, "sclk_mipi");
+ if (IS_ERR(dsi->pll_clk))
+ return PTR_ERR(dsi->pll_clk);
}
/* If it doesn't exist, use pixel clock instead of failing */
diff --git a/include/drm/bridge/samsung-dsim.h
b/include/drm/bridge/samsung-dsim.h
index 05100e91ecb9..31ff88f152fb 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -87,6 +87,7 @@ struct samsung_dsim {
void __iomem *reg_base;
struct phy *phy;
struct clk **clks;
+ struct clk *pll_clk;
struct regulator_bulk_data supplies[2];
int irq;
struct gpio_desc *te_gpio;
--
2.39.2
