[PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge

[PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge

[Michael Tretter]

I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter,
which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few
modes were working, but in many modes my monitor stayed dark.

This series fixes the Samsung DSIM bridge driver to bring up a few more
modes:

The driver read the rate of the PLL ref clock only during probe.
However, if the clock is re-parented to the VIDEO_PLL, changes to the
pixel clock have an effect on the PLL ref clock. Therefore, the driver
must read and potentially update the PLL ref clock on every modeset.

I also found that the rounding mode of the porches and active area has
an effect on the working modes. If the driver rounds up instead of
rounding down and be calculates them in Hz instead of kHz, more modes
start to work.

The following table shows the modes that were working in my test without
this patch set and the modes that are working now:

|            Mode | Before | Now |
| 1920x1080-60.00 | X      | X   |
| 1920x1080-59.94 |        | X   |
| 1920x1080-50.00 |        | X   |
| 1920x1080-30.00 |        | X   |
| 1920x1080-29.97 |        | X   |
| 1920x1080-25.00 |        | X   |
| 1920x1080-24.00 |        |     |
| 1920x1080-23.98 |        |     |
| 1680x1050-59.88 |        | X   |
| 1280x1024-75.03 | X      | X   |
| 1280x1024-60.02 | X      | X   |
|  1200x960-59.99 |        | X   |
|  1152x864-75.00 | X      | X   |
|  1280x720-60.00 |        |     |
|  1280x720-59.94 |        |     |
|  1280x720-50.00 |        | X   |
|  1024x768-75.03 |        | X   |
|  1024x768-60.00 |        | X   |
|   800x600-75.00 | X      | X   |
|   800x600-60.32 | X      | X   |
|   720x576-50.00 | X      | X   |
|   720x480-60.00 |        |     |
|   720x480-59.94 | X      |     |
|   640x480-75.00 | X      | X   |
|   640x480-60.00 |        | X   |
|   640x480-59.94 |        | X   |
|   720x400-70.08 |        |     |

Interestingly, the 720x480-59.94 mode stopped working. However, I am
able to bring up the 720x480 modes by manually hacking the active area
(hsa) to 40 and carefully adjusting the clocks, but something still
seems to be off.

Unfortunately, a few more modes are still not working at all. The NXP
downstream kernel has some quirks to handle some of the modes especially
wrt. to the porches, but I cannot figure out, what the driver should
actually do in these cases. Maybe there is still an error in the
calculation of the porches and someone at NXP can chime in.

Michael

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
Marco Felsch (1):
      drm/bridge: samsung-dsim: add more mipi-dsi device debug information

Michael Tretter (4):
      drm/bridge: samsung-dsim: reread ref clock before configuring PLL
      drm/bridge: samsung-dsim: update PLL reference clock
      drm/bridge: samsung-dsim: adjust porches by rounding up
      drm/bridge: samsung-dsim: calculate porches in Hz

 drivers/gpu/drm/bridge/samsung-dsim.c | 42 +++++++++++++++++++++++++----------
 include/drm/bridge/samsung-dsim.h     |  1 +
 2 files changed, 31 insertions(+), 12 deletions(-)
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230818-samsung-dsim-42346444bce5

Best regards,
-- 
Michael Tretter <m.tretter@pengutronix.de>

[PATCH 1/5] drm/bridge: samsung-dsim: add more mipi-dsi device debug information

[Michael Tretter]

From: Marco Felsch <m.felsch@pengutronix.de>

Since the MIPI configuration can be changed on demand it is very useful
to print more MIPI settings during the MIPI device attach step.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 73ec60757dbc..6778f1751faa 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1711,7 +1711,10 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
 		return ret;
 	}
 
-	DRM_DEV_INFO(dev, "Attached %s device\n", device->name);
+	DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n",
+		     device->name, device->lanes,
+		     mipi_dsi_pixel_format_to_bpp(device->format),
+		     device->mode_flags);
 
 	drm_bridge_add(&dsi->bridge);
 

-- 
2.39.2

Re: [PATCH 1/5] drm/bridge: samsung-dsim: add more mipi-dsi device debug information

[Adam Ford]

On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter
<m.tretter@pengutronix.de> wrote:
>
> From: Marco Felsch <m.felsch@pengutronix.de>
>
> Since the MIPI configuration can be changed on demand it is very useful
> to print more MIPI settings during the MIPI device attach step.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

Reviewed-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon
Tested-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon

> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 73ec60757dbc..6778f1751faa 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1711,7 +1711,10 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
>                 return ret;
>         }
>
> -       DRM_DEV_INFO(dev, "Attached %s device\n", device->name);
> +       DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n",
> +                    device->name, device->lanes,
> +                    mipi_dsi_pixel_format_to_bpp(device->format),
> +                    device->mode_flags);
>
>         drm_bridge_add(&dsi->bridge);
>
>
> --
> 2.39.2
>

Re: [PATCH 1/5] drm/bridge: samsung-dsim: add more mipi-dsi device debug information

[Inki Dae]

2023년 8월 29일 (화) 오전 7:38, Adam Ford <aford173@gmail.com>님이 작성:
>
> On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter
> <m.tretter@pengutronix.de> wrote:
> >
> > From: Marco Felsch <m.felsch@pengutronix.de>
> >
> > Since the MIPI configuration can be changed on demand it is very useful
> > to print more MIPI settings during the MIPI device attach step.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
>
> Reviewed-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon
> Tested-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon

Reviewed-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>

>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 73ec60757dbc..6778f1751faa 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -1711,7 +1711,10 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> >                 return ret;
> >         }
> >
> > -       DRM_DEV_INFO(dev, "Attached %s device\n", device->name);
> > +       DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n",
> > +                    device->name, device->lanes,
> > +                    mipi_dsi_pixel_format_to_bpp(device->format),
> > +                    device->mode_flags);
> >
> >         drm_bridge_add(&dsi->bridge);
> >
> >
> > --
> > 2.39.2
> >

Re: [PATCH 1/5] drm/bridge: samsung-dsim: add more mipi-dsi device debug information

[Adam Ford]

On Sun, Sep 3, 2023 at 8:05 PM Inki Dae <daeinki@gmail.com> wrote:
>
> 2023년 8월 29일 (화) 오전 7:38, Adam Ford <aford173@gmail.com>님이 작성:
> >
> > On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter
> > <m.tretter@pengutronix.de> wrote:
> > >
> > > From: Marco Felsch <m.felsch@pengutronix.de>
> > >
> > > Since the MIPI configuration can be changed on demand it is very useful
> > > to print more MIPI settings during the MIPI device attach step.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> >
> > Reviewed-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon
> > Tested-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon
>
> Reviewed-by: Inki Dae <inki.dae@samsung.com>
> Acked-by: Inki Dae <inki.dae@samsung.com>

What needs to be done in order to get this accepted?  This series is a
very nice improvement in i.MX8M Mini / Nano.

adam
>
> >
> > > ---
> > >  drivers/gpu/drm/bridge/samsung-dsim.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > index 73ec60757dbc..6778f1751faa 100644
> > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > @@ -1711,7 +1711,10 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > >                 return ret;
> > >         }
> > >
> > > -       DRM_DEV_INFO(dev, "Attached %s device\n", device->name);
> > > +       DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n",
> > > +                    device->name, device->lanes,
> > > +                    mipi_dsi_pixel_format_to_bpp(device->format),
> > > +                    device->mode_flags);
> > >
> > >         drm_bridge_add(&dsi->bridge);
> > >
> > >
> > > --
> > > 2.39.2
> > >

Re: [PATCH 1/5] drm/bridge: samsung-dsim: add more mipi-dsi device debug information

[Michael Tretter]

On Wed, 27 Sep 2023 07:47:53 -0500, Adam Ford wrote:
> On Sun, Sep 3, 2023 at 8:05 PM Inki Dae <daeinki@gmail.com> wrote:
> >
> > 2023년 8월 29일 (화) 오전 7:38, Adam Ford <aford173@gmail.com>님이 작성:
> > >
> > > On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter
> > > <m.tretter@pengutronix.de> wrote:
> > > >
> > > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > >
> > > > Since the MIPI configuration can be changed on demand it is very useful
> > > > to print more MIPI settings during the MIPI device attach step.
> > > >
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > >
> > > Reviewed-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon
> > > Tested-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon
> >
> > Reviewed-by: Inki Dae <inki.dae@samsung.com>
> > Acked-by: Inki Dae <inki.dae@samsung.com>
> 
> What needs to be done in order to get this accepted?  This series is a
> very nice improvement in i.MX8M Mini / Nano.

I think it is my turn to send a v2 that addresses the review comments. I'll
try to find time this week.

Michael

> 
> adam
> >
> > >
> > > > ---
> > > >  drivers/gpu/drm/bridge/samsung-dsim.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > index 73ec60757dbc..6778f1751faa 100644
> > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > @@ -1711,7 +1711,10 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > > >                 return ret;
> > > >         }
> > > >
> > > > -       DRM_DEV_INFO(dev, "Attached %s device\n", device->name);
> > > > +       DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n",
> > > > +                    device->name, device->lanes,
> > > > +                    mipi_dsi_pixel_format_to_bpp(device->format),
> > > > +                    device->mode_flags);
> > > >
> > > >         drm_bridge_add(&dsi->bridge);
> > > >
> > > >
> > > > --
> > > > 2.39.2
> > > >
> 

[PATCH 2/5] drm/bridge: samsung-dsim: reread ref clock before configuring PLL

[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 <m.tretter@pengutronix.de>
---
 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

[Inki Dae]

2023년 8월 29일 (화) 오전 12:59, Michael Tretter <m.tretter@pengutronix.de>님이 작성:

>
> 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 <m.tretter@pengutronix.de>
> ---
>  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
>

Re: [PATCH 2/5] drm/bridge: samsung-dsim: reread ref clock before configuring PLL

[Michael Tretter]

On Mon, 04 Sep 2023 13:38:33 +0900, Inki Dae wrote:
> 2023년 8월 29일 (화) 오전 12:59, Michael Tretter <m.tretter@pengutronix.de>님이 작성:
> 
> >
> > 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 <m.tretter@pengutronix.de>
> > ---
> >  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
> >
> 

[PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock

[Michael Tretter]

The PLL requires a clock between 2 MHz and 30 MHz after the pre-divider.
The reference clock for the PLL may change due to changes to it's parent
clock. Thus, the frequency may be out of range or unsuited for
generating the high speed clock for MIPI DSI.

Try to keep the pre-devider small, and set the reference clock close to
30 MHz before recalculating the PLL configuration. Use a divider with a
power of two for the reference clock as this seems to work best in
my tests.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index da90c2038042..4de6e4f116db 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
 	u16 m;
 	u32 reg;
 
-	if (dsi->pll_clk)
+	if (dsi->pll_clk) {
+		/*
+		 * Ensure that the reference clock is generated with a power of
+		 * two divider from its parent, but close to the PLLs upper
+		 * limit of the valid range of 2 MHz to 30 MHz.
+		 */
+		fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
+		while (fin > 30 * MHZ)
+			fin = fin / 2;
+		clk_set_rate(dsi->pll_clk, fin);
+
 		fin = clk_get_rate(dsi->pll_clk);
-	else
+	} 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);

-- 
2.39.2

Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock

[Marco Felsch]

On 23-08-28, Michael Tretter wrote:
> The PLL requires a clock between 2 MHz and 30 MHz after the pre-divider.
> The reference clock for the PLL may change due to changes to it's parent
> clock. Thus, the frequency may be out of range or unsuited for
> generating the high speed clock for MIPI DSI.
> 
> Try to keep the pre-devider small, and set the reference clock close to
> 30 MHz before recalculating the PLL configuration. Use a divider with a
> power of two for the reference clock as this seems to work best in
> my tests.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index da90c2038042..4de6e4f116db 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>  	u16 m;
>  	u32 reg;
>  
> -	if (dsi->pll_clk)
> +	if (dsi->pll_clk) {
> +		/*
> +		 * Ensure that the reference clock is generated with a power of
> +		 * two divider from its parent, but close to the PLLs upper
> +		 * limit of the valid range of 2 MHz to 30 MHz.
> +		 */
> +		fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> +		while (fin > 30 * MHZ)
> +			fin = fin / 2;

Really just a cosmetic nit: fin /= 2;

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

> +		clk_set_rate(dsi->pll_clk, fin);
> +
>  		fin = clk_get_rate(dsi->pll_clk);
> -	else
> +	} 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);
> 
> -- 
> 2.39.2
> 
> 
> 

Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock

[Adam Ford]

On Mon, Aug 28, 2023 at 11:42 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> On 23-08-28, Michael Tretter wrote:
> > The PLL requires a clock between 2 MHz and 30 MHz after the pre-divider.
> > The reference clock for the PLL may change due to changes to it's parent
> > clock. Thus, the frequency may be out of range or unsuited for
> > generating the high speed clock for MIPI DSI.
> >
> > Try to keep the pre-devider small, and set the reference clock close to
> > 30 MHz before recalculating the PLL configuration. Use a divider with a
> > power of two for the reference clock as this seems to work best in
> > my tests.
> >
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index da90c2038042..4de6e4f116db 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> >       u16 m;
> >       u32 reg;
> >
> > -     if (dsi->pll_clk)
> > +     if (dsi->pll_clk) {
> > +             /*
> > +              * Ensure that the reference clock is generated with a power of
> > +              * two divider from its parent, but close to the PLLs upper
> > +              * limit of the valid range of 2 MHz to 30 MHz.
> > +              */
> > +             fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> > +             while (fin > 30 * MHZ)
> > +                     fin = fin / 2;
>
> Really just a cosmetic nit: fin /= 2;
>
> Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
>
> > +             clk_set_rate(dsi->pll_clk, fin);
> > +
> >               fin = clk_get_rate(dsi->pll_clk);
> > -     else
> > +     } else {
> >               fin = dsi->pll_clk_rate;
> > +     }

I don't have all the code style rules memorized.  Did you run it
through checkpatch?  I wonder if the braces here are appropriate for a
1-line after the else.  If checkpatch is happy, I am fine with it.

adam
> >       dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
> >
> >       fout = samsung_dsim_pll_find_pms(dsi, fin, freq, &p, &m, &s);
> >
> > --
> > 2.39.2
> >
> >
> >

Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock

[Michael Tretter]

On Mon, 28 Aug 2023 13:17:44 -0500, Adam Ford wrote:
> On Mon, Aug 28, 2023 at 11:42 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
> >
> > On 23-08-28, Michael Tretter wrote:
> > > The PLL requires a clock between 2 MHz and 30 MHz after the pre-divider.
> > > The reference clock for the PLL may change due to changes to it's parent
> > > clock. Thus, the frequency may be out of range or unsuited for
> > > generating the high speed clock for MIPI DSI.
> > >
> > > Try to keep the pre-devider small, and set the reference clock close to
> > > 30 MHz before recalculating the PLL configuration. Use a divider with a
> > > power of two for the reference clock as this seems to work best in
> > > my tests.
> > >
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > index da90c2038042..4de6e4f116db 100644
> > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > @@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> > >       u16 m;
> > >       u32 reg;
> > >
> > > -     if (dsi->pll_clk)
> > > +     if (dsi->pll_clk) {
> > > +             /*
> > > +              * Ensure that the reference clock is generated with a power of
> > > +              * two divider from its parent, but close to the PLLs upper
> > > +              * limit of the valid range of 2 MHz to 30 MHz.
> > > +              */
> > > +             fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> > > +             while (fin > 30 * MHZ)
> > > +                     fin = fin / 2;
> >
> > Really just a cosmetic nit: fin /= 2;
> >
> > Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> >
> > > +             clk_set_rate(dsi->pll_clk, fin);
> > > +
> > >               fin = clk_get_rate(dsi->pll_clk);
> > > -     else
> > > +     } else {
> > >               fin = dsi->pll_clk_rate;
> > > +     }
> 
> I don't have all the code style rules memorized.  Did you run it
> through checkpatch?  I wonder if the braces here are appropriate for a
> 1-line after the else.  If checkpatch is happy, I am fine with it.

checkpatch is happy: The coding style states that if one of the branches needs
braces because it has multiple statements, all branches should have braces.

Michael

> 
> adam
> > >       dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
> > >
> > >       fout = samsung_dsim_pll_find_pms(dsi, fin, freq, &p, &m, &s);
> > >
> > > --
> > > 2.39.2
> > >
> > >
> > >
> 

Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock

[Inki Dae]

2023년 8월 29일 (화) 오전 12:59, Michael Tretter <m.tretter@pengutronix.de>님이 작성:
>
> The PLL requires a clock between 2 MHz and 30 MHz after the pre-divider.
> The reference clock for the PLL may change due to changes to it's parent
> clock. Thus, the frequency may be out of range or unsuited for
> generating the high speed clock for MIPI DSI.
>
> Try to keep the pre-devider small, and set the reference clock close to
> 30 MHz before recalculating the PLL configuration. Use a divider with a
> power of two for the reference clock as this seems to work best in
> my tests.

Clock frequency is specific to SoC architecture so we have to handle
it carefully because samsung-dsim.c is a common module for I.MX and
Exynos series.
You mentioned "The PLL requires a clock between 2 MHz and 3MHz after
pre-divider", and the clock means that fin_pll - PFD input frequency -
which can be calculated with oscillator clock frequency / P value?
According to Exynos datasheet, the fin_pll should be 6 ~ 12Mhz.

For example,
In case of Exyhos, we use 24MHz as oscillator clock frequency, so
fin_pll frequency, 8MHz = 24MHz / P(3).

Can you tell me the source of the constraint that clocks must be
between 2MHz and 30MHz?

To other I.MX and Exynos engineers, please do not merge this patch
until two SoC platforms are tested correctly.

Thanks,
Inki Dae

>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index da90c2038042..4de6e4f116db 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>         u16 m;
>         u32 reg;
>
> -       if (dsi->pll_clk)
> +       if (dsi->pll_clk) {
> +               /*
> +                * Ensure that the reference clock is generated with a power of
> +                * two divider from its parent, but close to the PLLs upper
> +                * limit of the valid range of 2 MHz to 30 MHz.
> +                */
> +               fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> +               while (fin > 30 * MHZ)
> +                       fin = fin / 2;
> +               clk_set_rate(dsi->pll_clk, fin);
> +
>                 fin = clk_get_rate(dsi->pll_clk);
> -       else
> +       } 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);
>
> --
> 2.39.2
>

Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock

[Michael Tretter]

On Mon, 04 Sep 2023 14:44:41 +0900, Inki Dae wrote:
> 2023년 8월 29일 (화) 오전 12:59, Michael Tretter <m.tretter@pengutronix.de>님이 작성:
> >
> > The PLL requires a clock between 2 MHz and 30 MHz after the pre-divider.
> > The reference clock for the PLL may change due to changes to it's parent
> > clock. Thus, the frequency may be out of range or unsuited for
> > generating the high speed clock for MIPI DSI.
> >
> > Try to keep the pre-devider small, and set the reference clock close to
> > 30 MHz before recalculating the PLL configuration. Use a divider with a
> > power of two for the reference clock as this seems to work best in
> > my tests.
> 
> Clock frequency is specific to SoC architecture so we have to handle
> it carefully because samsung-dsim.c is a common module for I.MX and
> Exynos series.
> You mentioned "The PLL requires a clock between 2 MHz and 3MHz after
> pre-divider", and the clock means that fin_pll - PFD input frequency -
> which can be calculated with oscillator clock frequency / P value?
> According to Exynos datasheet, the fin_pll should be 6 ~ 12Mhz.
> 
> For example,
> In case of Exyhos, we use 24MHz as oscillator clock frequency, so
> fin_pll frequency, 8MHz = 24MHz / P(3).
> 
> Can you tell me the source of the constraint that clocks must be
> between 2MHz and 30MHz?

The source is the i.MX8M Nano reference manual, Table 13-40. DPHY PLL
Parameters. It documents that the P divider frequency (fin_pll) has a
frequency range of 2 MHz to 30 MHz. According to the same table, the input
frequency (fin) has a range of 6 MHz to 300 MHz.

Is the table incorrect?

I also tried to always set the reference clock to 24 MHz, but depending on the
display clock this isn't always possible.

Michael

> 
> To other I.MX and Exynos engineers, please do not merge this patch
> until two SoC platforms are tested correctly.
> 
> Thanks,
> Inki Dae
> 
> >
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index da90c2038042..4de6e4f116db 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> >         u16 m;
> >         u32 reg;
> >
> > -       if (dsi->pll_clk)
> > +       if (dsi->pll_clk) {
> > +               /*
> > +                * Ensure that the reference clock is generated with a power of
> > +                * two divider from its parent, but close to the PLLs upper
> > +                * limit of the valid range of 2 MHz to 30 MHz.
> > +                */
> > +               fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> > +               while (fin > 30 * MHZ)
> > +                       fin = fin / 2;
> > +               clk_set_rate(dsi->pll_clk, fin);
> > +
> >                 fin = clk_get_rate(dsi->pll_clk);
> > -       else
> > +       } 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);
> >
> > --
> > 2.39.2
> >
> 

RE: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock

[대인기/Tizen Platform Lab(SR)/삼성전자]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> Michael Tretter
> Sent: Monday, September 4, 2023 8:15 PM
> To: Inki Dae <daeinki@gmail.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>; Robert Foss
> <rfoss@kernel.org>; Jonas Karlman <jonas@kwiboo.se>; dri-
> devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Jernej Skrabec
> <jernej.skrabec@gmail.com>; Laurent Pinchart
> <Laurent.pinchart@ideasonboard.com>; Andrzej Hajda
> <andrzej.hajda@intel.com>; Marek Szyprowski <m.szyprowski@samsung.com>;
> kernel@pengutronix.de; Jagan Teki <jagan@amarulasolutions.com>
> Subject: Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference
> clock
> 
> On Mon, 04 Sep 2023 14:44:41 +0900, Inki Dae wrote:
> > 2023년 8월 29일 (화) 오전 12:59, Michael Tretter <m.tretter@pengutronix.de>
> 님이 작성:
> > >
> > > The PLL requires a clock between 2 MHz and 30 MHz after the pre-
> divider.
> > > The reference clock for the PLL may change due to changes to it's
> parent
> > > clock. Thus, the frequency may be out of range or unsuited for
> > > generating the high speed clock for MIPI DSI.
> > >
> > > Try to keep the pre-devider small, and set the reference clock close
> to
> > > 30 MHz before recalculating the PLL configuration. Use a divider with
> a
> > > power of two for the reference clock as this seems to work best in
> > > my tests.
> >
> > Clock frequency is specific to SoC architecture so we have to handle
> > it carefully because samsung-dsim.c is a common module for I.MX and
> > Exynos series.
> > You mentioned "The PLL requires a clock between 2 MHz and 3MHz after
> > pre-divider", and the clock means that fin_pll - PFD input frequency -
> > which can be calculated with oscillator clock frequency / P value?
> > According to Exynos datasheet, the fin_pll should be 6 ~ 12Mhz.
> >
> > For example,
> > In case of Exyhos, we use 24MHz as oscillator clock frequency, so
> > fin_pll frequency, 8MHz = 24MHz / P(3).
> >
> > Can you tell me the source of the constraint that clocks must be
> > between 2MHz and 30MHz?
> 
> The source is the i.MX8M Nano reference manual, Table 13-40. DPHY PLL
> Parameters. It documents that the P divider frequency (fin_pll) has a
> frequency range of 2 MHz to 30 MHz. According to the same table, the input

In case of Exynos, the range is from 6MHz to 12MHz according to Exynos4212 reference manual, Table 1-5.

> frequency (fin) has a range of 6 MHz to 300 MHz.

In case of Exynos, the range is from 6MHz to 200MHz.

> 
> Is the table incorrect?

It's correct for I.MX but incorrect for Exynos. I think it would mean that the valid range depends on SoC architecture. So I'd say that this patch is specific to I.MX. This was one of what I concerted about when trying to move samsung-dsim.c module to bridge directory for common use between I.MX and Exynos Platforms, and this will be what we have to solve together - I.MX and Exynos engineers. How about using platform specific data - samsung_dsim_driver_data structure?

I.e,

static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
    ...
    .min_fin_pll = 2,
    .max_fin_pll = 30,
    ...
};

static const struct samsung_dsim_driver_data exynosxxxx_dsi_driver_data = {
    ...
    .min_fin_pll = 6,
    .max_fin_pll = 12,
    ...
};

And then,

while (fin > driver_data->max_fin_pll * MHZ)
    fin = fin / 2;

> 
> I also tried to always set the reference clock to 24 MHz, but depending on
> the
> display clock this isn't always possible.

According to dt binding, if samsung,pll-clock-frequency exists then we have to use it first. I'm not sure but could we check if the pll_clk_rate is valid or not depending on the given display clock in advance? If so then maybe we could update the pll_clk_rate correctly by reading the pll_clk frequency again.

Thanks,
Inki Dae

> 
> Michael
> 
> >
> > To other I.MX and Exynos engineers, please do not merge this patch
> > until two SoC platforms are tested correctly.
> >
> > Thanks,
> > Inki Dae
> >
> > >
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > index da90c2038042..4de6e4f116db 100644
> > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > @@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct
> samsung_dsim *dsi,
> > >         u16 m;
> > >         u32 reg;
> > >
> > > -       if (dsi->pll_clk)
> > > +       if (dsi->pll_clk) {
> > > +               /*
> > > +                * Ensure that the reference clock is generated with a
> power of
> > > +                * two divider from its parent, but close to the PLLs
> upper
> > > +                * limit of the valid range of 2 MHz to 30 MHz.
> > > +                */
> > > +               fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> > > +               while (fin > 30 * MHZ)
> > > +                       fin = fin / 2;
> > > +               clk_set_rate(dsi->pll_clk, fin);
> > > +
> > >                 fin = clk_get_rate(dsi->pll_clk);
> > > -       else
> > > +       } 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);
> > >
> > > --
> > > 2.39.2
> > >
> >

Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock

[Michael Tretter]

On Tue, 05 Sep 2023 18:06:06 +0900, 대인기/Tizen Platform Lab(SR)/삼성전자 wrote:
> 
> 
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> > Michael Tretter
> > Sent: Monday, September 4, 2023 8:15 PM
> > To: Inki Dae <daeinki@gmail.com>
> > Cc: Neil Armstrong <neil.armstrong@linaro.org>; Robert Foss
> > <rfoss@kernel.org>; Jonas Karlman <jonas@kwiboo.se>; dri-
> > devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Jernej Skrabec
> > <jernej.skrabec@gmail.com>; Laurent Pinchart
> > <Laurent.pinchart@ideasonboard.com>; Andrzej Hajda
> > <andrzej.hajda@intel.com>; Marek Szyprowski <m.szyprowski@samsung.com>;
> > kernel@pengutronix.de; Jagan Teki <jagan@amarulasolutions.com>
> > Subject: Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference
> > clock
> > 
> > On Mon, 04 Sep 2023 14:44:41 +0900, Inki Dae wrote:
> > > 2023년 8월 29일 (화) 오전 12:59, Michael Tretter <m.tretter@pengutronix.de>
> > 님이 작성:
> > > >
> > > > The PLL requires a clock between 2 MHz and 30 MHz after the pre-
> > divider.
> > > > The reference clock for the PLL may change due to changes to it's
> > parent
> > > > clock. Thus, the frequency may be out of range or unsuited for
> > > > generating the high speed clock for MIPI DSI.
> > > >
> > > > Try to keep the pre-devider small, and set the reference clock close
> > to
> > > > 30 MHz before recalculating the PLL configuration. Use a divider with
> > a
> > > > power of two for the reference clock as this seems to work best in
> > > > my tests.
> > >
> > > Clock frequency is specific to SoC architecture so we have to handle
> > > it carefully because samsung-dsim.c is a common module for I.MX and
> > > Exynos series.
> > > You mentioned "The PLL requires a clock between 2 MHz and 3MHz after
> > > pre-divider", and the clock means that fin_pll - PFD input frequency -
> > > which can be calculated with oscillator clock frequency / P value?
> > > According to Exynos datasheet, the fin_pll should be 6 ~ 12Mhz.
> > >
> > > For example,
> > > In case of Exyhos, we use 24MHz as oscillator clock frequency, so
> > > fin_pll frequency, 8MHz = 24MHz / P(3).
> > >
> > > Can you tell me the source of the constraint that clocks must be
> > > between 2MHz and 30MHz?
> > 
> > The source is the i.MX8M Nano reference manual, Table 13-40. DPHY PLL
> > Parameters. It documents that the P divider frequency (fin_pll) has a
> > frequency range of 2 MHz to 30 MHz. According to the same table, the input

Small clarification: These are the corrected limits of TRM rev. 1. In TRM rev.
0 the limits are incorrectly specified as 6 MHz to 12 MHz.

> 
> In case of Exynos, the range is from 6MHz to 12MHz according to Exynos4212 reference manual, Table 1-5.
> 
> > frequency (fin) has a range of 6 MHz to 300 MHz.
> 
> In case of Exynos, the range is from 6MHz to 200MHz.
> 
> > 
> > Is the table incorrect?
> 

> It's correct for I.MX but incorrect for Exynos. I think it would mean that
> the valid range depends on SoC architecture. So I'd say that this patch is
> specific to I.MX.

I understand.

> This was one of what I concerted about when trying to move
> samsung-dsim.c module to bridge directory for common use between I.MX and
> Exynos Platforms, and this will be what we have to solve together - I.MX and
> Exynos engineers. How about using platform specific data -
> samsung_dsim_driver_data structure?
> 
> I.e,
> 
> static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
>     ...
>     .min_fin_pll = 2,
>     .max_fin_pll = 30,
>     ...
> };
> 
> static const struct samsung_dsim_driver_data exynosxxxx_dsi_driver_data = {
>     ...
>     .min_fin_pll = 6,
>     .max_fin_pll = 12,
>     ...
> };

Extending the samsung_dsim_driver_data sounds reasonable. There are already
other frequency limits specified. I will implement this in v2.

> 
> And then,
> 
> while (fin > driver_data->max_fin_pll * MHZ)
>     fin = fin / 2;
> 
> > 
> > I also tried to always set the reference clock to 24 MHz, but depending on
> > the
> > display clock this isn't always possible.
> 

> According to dt binding, if samsung,pll-clock-frequency exists then we have
> to use it first. I'm not sure but could we check if the pll_clk_rate is
> valid or not depending on the given display clock in advance? If so then
> maybe we could update the pll_clk_rate correctly by reading the pll_clk
> frequency again.

If samsung,pll-clock-frequency is set, the driver will neither check nor
update the pll_clk, but assume that the clocks are configured correctly. Thus,
the author of the device tree is responsible for selecting and configuring
valid clocks.

I observed that if I set pll_clk to any fixed value for different modes, the
clock framework may use a different clock rate depending on what is possible.
This may result in a reference clock that uses a fractional divider to get the
pll_clk_rate or cannot exactly produce the hs_clock. These situations cause
sync issues on the display.

Checking, if the current pll_clk_rate would involve the pix_clk, the hs_clock,
and the parent of the pll_clk. It may be possible, but I don't see a problem
with calculating a suitable pll_clk_rate, updating the pll_clk, and then
configuring the PLL to generate the hs_clock.

Michael

> 
> Thanks,
> Inki Dae
> 
> > 
> > Michael
> > 
> > >
> > > To other I.MX and Exynos engineers, please do not merge this patch
> > > until two SoC platforms are tested correctly.
> > >
> > > Thanks,
> > > Inki Dae
> > >
> > > >
> > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > ---
> > > >  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++++++--
> > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > index da90c2038042..4de6e4f116db 100644
> > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > @@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct
> > samsung_dsim *dsi,
> > > >         u16 m;
> > > >         u32 reg;
> > > >
> > > > -       if (dsi->pll_clk)
> > > > +       if (dsi->pll_clk) {
> > > > +               /*
> > > > +                * Ensure that the reference clock is generated with a
> > power of
> > > > +                * two divider from its parent, but close to the PLLs
> > upper
> > > > +                * limit of the valid range of 2 MHz to 30 MHz.
> > > > +                */
> > > > +               fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> > > > +               while (fin > 30 * MHZ)
> > > > +                       fin = fin / 2;
> > > > +               clk_set_rate(dsi->pll_clk, fin);
> > > > +
> > > >                 fin = clk_get_rate(dsi->pll_clk);
> > > > -       else
> > > > +       } 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);
> > > >
> > > > --
> > > > 2.39.2
> > > >
> > >
> 
> 
> 

RE: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock

[대인기/Tizen Platform Lab(SR)/삼성전자]

> -----Original Message-----
> From: Michael Tretter <m.tretter@pengutronix.de>
> Sent: Friday, September 8, 2023 6:48 PM
> To: 대인기/Tizen Platform Lab(SR)/삼성전자 <inki.dae@samsung.com>
> Cc: 'Inki Dae' <daeinki@gmail.com>; 'Neil Armstrong'
> <neil.armstrong@linaro.org>; 'Robert Foss' <rfoss@kernel.org>; 'Jonas
> Karlman' <jonas@kwiboo.se>; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org; 'Jernej Skrabec' <jernej.skrabec@gmail.com>;
> 'Laurent Pinchart' <Laurent.pinchart@ideasonboard.com>; 'Andrzej Hajda'
> <andrzej.hajda@intel.com>; 'Marek Szyprowski' <m.szyprowski@samsung.com>;
> kernel@pengutronix.de; 'Jagan Teki' <jagan@amarulasolutions.com>
> Subject: Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference
> clock
> 
> On Tue, 05 Sep 2023 18:06:06 +0900, 대인기/Tizen Platform Lab(SR)/삼성전자
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> > > Michael Tretter
> > > Sent: Monday, September 4, 2023 8:15 PM
> > > To: Inki Dae <daeinki@gmail.com>
> > > Cc: Neil Armstrong <neil.armstrong@linaro.org>; Robert Foss
> > > <rfoss@kernel.org>; Jonas Karlman <jonas@kwiboo.se>; dri-
> > > devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Jernej
> Skrabec
> > > <jernej.skrabec@gmail.com>; Laurent Pinchart
> > > <Laurent.pinchart@ideasonboard.com>; Andrzej Hajda
> > > <andrzej.hajda@intel.com>; Marek Szyprowski <m.szyprowski@samsung.com>;
> > > kernel@pengutronix.de; Jagan Teki <jagan@amarulasolutions.com>
> > > Subject: Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL
> reference
> > > clock
> > >
> > > On Mon, 04 Sep 2023 14:44:41 +0900, Inki Dae wrote:
> > > > 2023년 8월 29일 (화) 오전 12:59, Michael Tretter
> <m.tretter@pengutronix.de>
> > > 님이 작성:
> > > > >
> > > > > The PLL requires a clock between 2 MHz and 30 MHz after the pre-
> > > divider.
> > > > > The reference clock for the PLL may change due to changes to it's
> > > parent
> > > > > clock. Thus, the frequency may be out of range or unsuited for
> > > > > generating the high speed clock for MIPI DSI.
> > > > >
> > > > > Try to keep the pre-devider small, and set the reference clock
> close
> > > to
> > > > > 30 MHz before recalculating the PLL configuration. Use a divider
> with
> > > a
> > > > > power of two for the reference clock as this seems to work best in
> > > > > my tests.
> > > >
> > > > Clock frequency is specific to SoC architecture so we have to handle
> > > > it carefully because samsung-dsim.c is a common module for I.MX and
> > > > Exynos series.
> > > > You mentioned "The PLL requires a clock between 2 MHz and 3MHz after
> > > > pre-divider", and the clock means that fin_pll - PFD input frequency
> -
> > > > which can be calculated with oscillator clock frequency / P value?
> > > > According to Exynos datasheet, the fin_pll should be 6 ~ 12Mhz.
> > > >
> > > > For example,
> > > > In case of Exyhos, we use 24MHz as oscillator clock frequency, so
> > > > fin_pll frequency, 8MHz = 24MHz / P(3).
> > > >
> > > > Can you tell me the source of the constraint that clocks must be
> > > > between 2MHz and 30MHz?
> > >
> > > The source is the i.MX8M Nano reference manual, Table 13-40. DPHY PLL
> > > Parameters. It documents that the P divider frequency (fin_pll) has a
> > > frequency range of 2 MHz to 30 MHz. According to the same table, the
> input
> 
> Small clarification: These are the corrected limits of TRM rev. 1. In TRM
> rev.
> 0 the limits are incorrectly specified as 6 MHz to 12 MHz.
> 
> >
> > In case of Exynos, the range is from 6MHz to 12MHz according to
> Exynos4212 reference manual, Table 1-5.
> >
> > > frequency (fin) has a range of 6 MHz to 300 MHz.
> >
> > In case of Exynos, the range is from 6MHz to 200MHz.
> >
> > >
> > > Is the table incorrect?
> >
> 
> > It's correct for I.MX but incorrect for Exynos. I think it would mean
> that
> > the valid range depends on SoC architecture. So I'd say that this patch
> is
> > specific to I.MX.
> 
> I understand.
> 
> > This was one of what I concerted about when trying to move
> > samsung-dsim.c module to bridge directory for common use between I.MX
> and
> > Exynos Platforms, and this will be what we have to solve together - I.MX
> and
> > Exynos engineers. How about using platform specific data -
> > samsung_dsim_driver_data structure?
> >
> > I.e,
> >
> > static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
> >     ...
> >     .min_fin_pll = 2,
> >     .max_fin_pll = 30,
> >     ...
> > };
> >
> > static const struct samsung_dsim_driver_data exynosxxxx_dsi_driver_data
> = {
> >     ...
> >     .min_fin_pll = 6,
> >     .max_fin_pll = 12,
> >     ...
> > };
> 
> Extending the samsung_dsim_driver_data sounds reasonable. There are
> already
> other frequency limits specified. I will implement this in v2.
> 
> >
> > And then,
> >
> > while (fin > driver_data->max_fin_pll * MHZ)
> >     fin = fin / 2;
> >
> > >
> > > I also tried to always set the reference clock to 24 MHz, but
> depending on
> > > the
> > > display clock this isn't always possible.
> >
> 
> > According to dt binding, if samsung,pll-clock-frequency exists then we
> have
> > to use it first. I'm not sure but could we check if the pll_clk_rate is
> > valid or not depending on the given display clock in advance? If so then
> > maybe we could update the pll_clk_rate correctly by reading the pll_clk
> > frequency again.
> 
> If samsung,pll-clock-frequency is set, the driver will neither check nor
> update the pll_clk, but assume that the clocks are configured correctly.
> Thus,
> the author of the device tree is responsible for selecting and configuring
> valid clocks.
> 
> I observed that if I set pll_clk to any fixed value for different modes,
> the
> clock framework may use a different clock rate depending on what is
> possible.
> This may result in a reference clock that uses a fractional divider to get
> the
> pll_clk_rate or cannot exactly produce the hs_clock. These situations
> cause
> sync issues on the display.

Yes, it does. The actual clock can vary according to clock configuration - which child clocks are used and divider values of the clock sources - of the pll_clk source. It would mean that the clock configuration depends on the machine, not the SoC, because the display panel may vary depending on the machine.

In my opinion, a role of the device driver would be to configure the clock sources properly, such as the current driver does, and we may need to find a proper divider value of each clock source and set it in the respective machine device tree so that the actual clock frequency can be close to the given pixel clock frequency provided by the machine device tree as "samsung,pll-clock-frequency".

As a result, even if the type of display attached to the machine varies I think the clock configuration method in the device driver code should not change.

Thanks,
Inki Dae

> 
> Checking, if the current pll_clk_rate would involve the pix_clk, the
> hs_clock,
> and the parent of the pll_clk. It may be possible, but I don't see a
> problem
> with calculating a suitable pll_clk_rate, updating the pll_clk, and then
> configuring the PLL to generate the hs_clock.
> 
> Michael
> 
> >
> > Thanks,
> > Inki Dae
> >
> > >
> > > Michael
> > >
> > > >
> > > > To other I.MX and Exynos engineers, please do not merge this patch
> > > > until two SoC platforms are tested correctly.
> > > >
> > > > Thanks,
> > > > Inki Dae
> > > >
> > > > >
> > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > > ---
> > > > >  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++++++--
> > > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > index da90c2038042..4de6e4f116db 100644
> > > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > @@ -611,10 +611,21 @@ static unsigned long
> samsung_dsim_set_pll(struct
> > > samsung_dsim *dsi,
> > > > >         u16 m;
> > > > >         u32 reg;
> > > > >
> > > > > -       if (dsi->pll_clk)
> > > > > +       if (dsi->pll_clk) {
> > > > > +               /*
> > > > > +                * Ensure that the reference clock is generated with a
> > > power of
> > > > > +                * two divider from its parent, but close to the PLLs
> > > upper
> > > > > +                * limit of the valid range of 2 MHz to 30 MHz.
> > > > > +                */
> > > > > +               fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> > > > > +               while (fin > 30 * MHZ)
> > > > > +                       fin = fin / 2;
> > > > > +               clk_set_rate(dsi->pll_clk, fin);
> > > > > +
> > > > >                 fin = clk_get_rate(dsi->pll_clk);
> > > > > -       else
> > > > > +       } 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);
> > > > >
> > > > > --
> > > > > 2.39.2
> > > > >
> > > >
> >
> >
> >

[PATCH 4/5] drm/bridge: samsung-dsim: adjust porches by rounding up

[Michael Tretter]

The porches must be rounded up to make the samsung-dsim work.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 4de6e4f116db..459be953be55 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -974,9 +974,9 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
 
 	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
 		int byte_clk_khz = dsi->hs_clock / 1000 / 8;
-		int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz / m->clock;
-		int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / m->clock;
-		int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz / m->clock;
+		int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk_khz, m->clock);
+		int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) * byte_clk_khz, m->clock);
+		int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk_khz, m->clock);
 
 		/* remove packet overhead when possible */
 		hfp = max(hfp - 6, 0);

-- 
2.39.2

Re: [PATCH 4/5] drm/bridge: samsung-dsim: adjust porches by rounding up

[Fabio Estevam]

Hi Michael,

On Mon, Aug 28, 2023 at 12:59 PM Michael Tretter
<m.tretter@pengutronix.de> wrote:
>
> The porches must be rounded up to make the samsung-dsim work.

The commit log could be improved here.

The way it is written gives the impression that samsung-dsim does not
work currently.

Re: [PATCH 4/5] drm/bridge: samsung-dsim: adjust porches by rounding up

[Adam Ford]

On Mon, Aug 28, 2023 at 1:26 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Michael,
>
> On Mon, Aug 28, 2023 at 12:59 PM Michael Tretter
> <m.tretter@pengutronix.de> wrote:
> >
> > The porches must be rounded up to make the samsung-dsim work.

...at some resolutions and refresh rates.

>
> The commit log could be improved here.
>
> The way it is written gives the impression that samsung-dsim does not
> work currently.

This is a big improvement in the number of resolutions and refresh rates.

Reviewed-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon
Tested-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon

Re: [PATCH 4/5] drm/bridge: samsung-dsim: adjust porches by rounding up

[Michael Tretter]

On Mon, 28 Aug 2023 17:39:03 -0500, Adam Ford wrote:
> On Mon, Aug 28, 2023 at 1:26 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Michael,
> >
> > On Mon, Aug 28, 2023 at 12:59 PM Michael Tretter
> > <m.tretter@pengutronix.de> wrote:
> > >
> > > The porches must be rounded up to make the samsung-dsim work.
> 
> ...at some resolutions and refresh rates.

I will rephrase the commit message in a v2 and list the resolutions and
refresh rates that were fixed in my test.

Michael

> 
> >
> > The commit log could be improved here.
> >
> > The way it is written gives the impression that samsung-dsim does not
> > work currently.
> 
> This is a big improvement in the number of resolutions and refresh rates.
> 
> Reviewed-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon
> Tested-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon
> 

[PATCH 5/5] drm/bridge: samsung-dsim: calculate porches in Hz

[Michael Tretter]

Calculating the byte_clk in kHz is imprecise for a hs_clock of 55687500
Hz, which may be used with a pixel clock of 74.25 MHz with mode
1920x1080-30.

Fix the calculation by using HZ instead of kHZ.

This requires to change the type to u64 to prevent overflows of the
integer type.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 459be953be55..eb7aca2b9ab7 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -973,10 +973,12 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
 	u32 reg;
 
 	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
-		int byte_clk_khz = dsi->hs_clock / 1000 / 8;
-		int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk_khz, m->clock);
-		int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) * byte_clk_khz, m->clock);
-		int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk_khz, m->clock);
+		u64 byte_clk = dsi->hs_clock / 8;
+		u64 pix_clk = m->clock * 1000;
+
+		int hfp = DIV64_U64_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk, pix_clk);
+		int hbp = DIV64_U64_ROUND_UP((m->htotal - m->hsync_end) * byte_clk, pix_clk);
+		int hsa = DIV64_U64_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk, pix_clk);
 
 		/* remove packet overhead when possible */
 		hfp = max(hfp - 6, 0);

-- 
2.39.2

Re: [PATCH 5/5] drm/bridge: samsung-dsim: calculate porches in Hz

[Adam Ford]

On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter
<m.tretter@pengutronix.de> wrote:
>
> Calculating the byte_clk in kHz is imprecise for a hs_clock of 55687500
> Hz, which may be used with a pixel clock of 74.25 MHz with mode
> 1920x1080-30.
>
> Fix the calculation by using HZ instead of kHZ.
>
> This requires to change the type to u64 to prevent overflows of the
> integer type.
>
I would argue this needs a fixes tag since the previous calculations
were not accurately generated for many resolutions and refresh rates.

Reviewed-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon
Tested-by: Adam Ford <aford173@gmail.com>  #imx8mm-beacon

> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 459be953be55..eb7aca2b9ab7 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -973,10 +973,12 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
>         u32 reg;
>
>         if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> -               int byte_clk_khz = dsi->hs_clock / 1000 / 8;
> -               int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk_khz, m->clock);
> -               int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) * byte_clk_khz, m->clock);
> -               int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk_khz, m->clock);
> +               u64 byte_clk = dsi->hs_clock / 8;
> +               u64 pix_clk = m->clock * 1000;
> +
> +               int hfp = DIV64_U64_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk, pix_clk);
> +               int hbp = DIV64_U64_ROUND_UP((m->htotal - m->hsync_end) * byte_clk, pix_clk);
> +               int hsa = DIV64_U64_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk, pix_clk);
>
>                 /* remove packet overhead when possible */
>                 hfp = max(hfp - 6, 0);
>
> --
> 2.39.2
>

Re: [PATCH 5/5] drm/bridge: samsung-dsim: calculate porches in Hz

[Maxim Schwalm]

Hi,

On 28.08.23 17:59, Michael Tretter wrote:
> Calculating the byte_clk in kHz is imprecise for a hs_clock of 55687500
> Hz, which may be used with a pixel clock of 74.25 MHz with mode
> 1920x1080-30.
> 
> Fix the calculation by using HZ instead of kHZ.
> 
> This requires to change the type to u64 to prevent overflows of the
> integer type.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 459be953be55..eb7aca2b9ab7 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -973,10 +973,12 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
>  	u32 reg;
>  
>  	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> -		int byte_clk_khz = dsi->hs_clock / 1000 / 8;
> -		int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk_khz, m->clock);
> -		int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) * byte_clk_khz, m->clock);
> -		int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk_khz, m->clock);
> +		u64 byte_clk = dsi->hs_clock / 8;
> +		u64 pix_clk = m->clock * 1000;
> +
> +		int hfp = DIV64_U64_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk, pix_clk);
> +		int hbp = DIV64_U64_ROUND_UP((m->htotal - m->hsync_end) * byte_clk, pix_clk);
> +		int hsa = DIV64_U64_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk, pix_clk);

Wouldn't it make sense to use the videomode structure here?

>  
>  		/* remove packet overhead when possible */
>  		hfp = max(hfp - 6, 0);
> 

Best regards,
Maxim

Re: [PATCH 5/5] drm/bridge: samsung-dsim: calculate porches in Hz

[Michael Tretter]

On Mon, 04 Sep 2023 16:28:37 +0200, Maxim Schwalm wrote:
> On 28.08.23 17:59, Michael Tretter wrote:
> > Calculating the byte_clk in kHz is imprecise for a hs_clock of 55687500
> > Hz, which may be used with a pixel clock of 74.25 MHz with mode
> > 1920x1080-30.
> > 
> > Fix the calculation by using HZ instead of kHZ.
> > 
> > This requires to change the type to u64 to prevent overflows of the
> > integer type.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 459be953be55..eb7aca2b9ab7 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -973,10 +973,12 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
> >  	u32 reg;
> >  
> >  	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> > -		int byte_clk_khz = dsi->hs_clock / 1000 / 8;
> > -		int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk_khz, m->clock);
> > -		int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) * byte_clk_khz, m->clock);
> > -		int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk_khz, m->clock);
> > +		u64 byte_clk = dsi->hs_clock / 8;
> > +		u64 pix_clk = m->clock * 1000;
> > +
> > +		int hfp = DIV64_U64_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk, pix_clk);
> > +		int hbp = DIV64_U64_ROUND_UP((m->htotal - m->hsync_end) * byte_clk, pix_clk);
> > +		int hsa = DIV64_U64_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk, pix_clk);
> 
> Wouldn't it make sense to use the videomode structure here?

That would introduce a dependency on VIDEOMODE_HELPERS to avoid four explicit
calculations of the pixel clock in Hz and the porches in pixels. I don't think
that's really worth it.

Michael

> 
> >  
> >  		/* remove packet overhead when possible */
> >  		hfp = max(hfp - 6, 0);
> > 
> 
> Best regards,
> Maxim
> 

Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge

[Adam Ford]

On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter
<m.tretter@pengutronix.de> wrote:
>
> I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter,
> which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few
> modes were working, but in many modes my monitor stayed dark.
>
> This series fixes the Samsung DSIM bridge driver to bring up a few more
> modes:
>
> The driver read the rate of the PLL ref clock only during probe.
> However, if the clock is re-parented to the VIDEO_PLL, changes to the
> pixel clock have an effect on the PLL ref clock. Therefore, the driver
> must read and potentially update the PLL ref clock on every modeset.
>
> I also found that the rounding mode of the porches and active area has
> an effect on the working modes. If the driver rounds up instead of
> rounding down and be calculates them in Hz instead of kHz, more modes
> start to work.
>
> The following table shows the modes that were working in my test without
> this patch set and the modes that are working now:
>
> |            Mode | Before | Now |
> | 1920x1080-60.00 | X      | X   |
> | 1920x1080-59.94 |        | X   |
> | 1920x1080-50.00 |        | X   |
> | 1920x1080-30.00 |        | X   |
> | 1920x1080-29.97 |        | X   |
> | 1920x1080-25.00 |        | X   |
> | 1920x1080-24.00 |        |     |
> | 1920x1080-23.98 |        |     |
> | 1680x1050-59.88 |        | X   |
> | 1280x1024-75.03 | X      | X   |
> | 1280x1024-60.02 | X      | X   |
> |  1200x960-59.99 |        | X   |
> |  1152x864-75.00 | X      | X   |
> |  1280x720-60.00 |        |     |
> |  1280x720-59.94 |        |     |
> |  1280x720-50.00 |        | X   |
> |  1024x768-75.03 |        | X   |
> |  1024x768-60.00 |        | X   |
> |   800x600-75.00 | X      | X   |
> |   800x600-60.32 | X      | X   |
> |   720x576-50.00 | X      | X   |
> |   720x480-60.00 |        |     |
> |   720x480-59.94 | X      |     |
> |   640x480-75.00 | X      | X   |
> |   640x480-60.00 |        | X   |
> |   640x480-59.94 |        | X   |
> |   720x400-70.08 |        |     |
>

Nice!

> Interestingly, the 720x480-59.94 mode stopped working. However, I am
> able to bring up the 720x480 modes by manually hacking the active area
> (hsa) to 40 and carefully adjusting the clocks, but something still
> seems to be off.

Checkout my

branch: https://github.com/aford173/linux/commit/183cf6d154afeb9b0300500b09d7b8ec53047a12

I found that certain resolutions don't properly round, and it seemed
to be related to the size of HFP.  HFP of 110 when divded between 4
lanes, required a round-up, but then I had to recalculate the rest of
the horizontal timings to compensate.

I was going to push that as an RFC, but I will investigate your patch
series first.  With my small rounding correction, I am able to get
720x480 and 720p on my imx8mp, but not my mini/nano, so I am hoping
your patch series fixes that issue for me.

>
> Unfortunately, a few more modes are still not working at all. The NXP
> downstream kernel has some quirks to handle some of the modes especially
> wrt. to the porches, but I cannot figure out, what the driver should
> actually do in these cases. Maybe there is still an error in the
> calculation of the porches and someone at NXP can chime in.

Hopefully the comment in my above patch explains how the calculation
is corrected and adjusted to get some of the missing resolutions.

> Michael
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

I'll try to reivew this week and respond with my feedback.

adam

> ---
> Marco Felsch (1):
>       drm/bridge: samsung-dsim: add more mipi-dsi device debug information
>
> Michael Tretter (4):
>       drm/bridge: samsung-dsim: reread ref clock before configuring PLL
>       drm/bridge: samsung-dsim: update PLL reference clock
>       drm/bridge: samsung-dsim: adjust porches by rounding up
>       drm/bridge: samsung-dsim: calculate porches in Hz
>
>  drivers/gpu/drm/bridge/samsung-dsim.c | 42 +++++++++++++++++++++++++----------
>  include/drm/bridge/samsung-dsim.h     |  1 +
>  2 files changed, 31 insertions(+), 12 deletions(-)
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230818-samsung-dsim-42346444bce5
>
> Best regards,
> --
> Michael Tretter <m.tretter@pengutronix.de>
>

Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge

[Adam Ford]

On Mon, Aug 28, 2023 at 11:13 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter
> <m.tretter@pengutronix.de> wrote:
> >
> > I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter,
> > which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few
> > modes were working, but in many modes my monitor stayed dark.
> >
> > This series fixes the Samsung DSIM bridge driver to bring up a few more
> > modes:
> >
> > The driver read the rate of the PLL ref clock only during probe.
> > However, if the clock is re-parented to the VIDEO_PLL, changes to the
> > pixel clock have an effect on the PLL ref clock. Therefore, the driver
> > must read and potentially update the PLL ref clock on every modeset.
> >
> > I also found that the rounding mode of the porches and active area has
> > an effect on the working modes. If the driver rounds up instead of
> > rounding down and be calculates them in Hz instead of kHz, more modes
> > start to work.
> >
> > The following table shows the modes that were working in my test without
> > this patch set and the modes that are working now:
> >
> > |            Mode | Before | Now |
> > | 1920x1080-60.00 | X      | X   |
> > | 1920x1080-59.94 |        | X   |
> > | 1920x1080-50.00 |        | X   |
> > | 1920x1080-30.00 |        | X   |
> > | 1920x1080-29.97 |        | X   |
> > | 1920x1080-25.00 |        | X   |
> > | 1920x1080-24.00 |        |     |
> > | 1920x1080-23.98 |        |     |
> > | 1680x1050-59.88 |        | X   |
> > | 1280x1024-75.03 | X      | X   |
> > | 1280x1024-60.02 | X      | X   |
> > |  1200x960-59.99 |        | X   |
> > |  1152x864-75.00 | X      | X   |
> > |  1280x720-60.00 |        |     |
> > |  1280x720-59.94 |        |     |
> > |  1280x720-50.00 |        | X   |
> > |  1024x768-75.03 |        | X   |
> > |  1024x768-60.00 |        | X   |
> > |   800x600-75.00 | X      | X   |
> > |   800x600-60.32 | X      | X   |
> > |   720x576-50.00 | X      | X   |
> > |   720x480-60.00 |        |     |
> > |   720x480-59.94 | X      |     |
> > |   640x480-75.00 | X      | X   |
> > |   640x480-60.00 |        | X   |
> > |   640x480-59.94 |        | X   |
> > |   720x400-70.08 |        |     |
> >
>
> Nice!
>
> > Interestingly, the 720x480-59.94 mode stopped working. However, I am
> > able to bring up the 720x480 modes by manually hacking the active area
> > (hsa) to 40 and carefully adjusting the clocks, but something still
> > seems to be off.
>
> Checkout my
>
> branch: https://github.com/aford173/linux/commit/183cf6d154afeb9b0300500b09d7b8ec53047a12
>
> I found that certain resolutions don't properly round, and it seemed
> to be related to the size of HFP.  HFP of 110 when divded between 4
> lanes, required a round-up, but then I had to recalculate the rest of
> the horizontal timings to compensate.
>
> I was going to push that as an RFC, but I will investigate your patch
> series first.  With my small rounding correction, I am able to get
> 720x480 and 720p on my imx8mp, but not my mini/nano, so I am hoping
> your patch series fixes that issue for me.
>
> >
> > Unfortunately, a few more modes are still not working at all. The NXP
> > downstream kernel has some quirks to handle some of the modes especially
> > wrt. to the porches, but I cannot figure out, what the driver should
> > actually do in these cases. Maybe there is still an error in the
> > calculation of the porches and someone at NXP can chime in.
>
> Hopefully the comment in my above patch explains how the calculation
> is corrected and adjusted to get some of the missing resolutions.

I tested my above patch and it works to sync 720p-60 on my imx8mp, but
it doesn't seem to sync on my imx8mm using the same monitor and HDMI
cable.  I don't have a DSI analyzer, so I might still send out a
modified version of my patch as an RFC once this gets approved.

adam
>
> > Michael
> >
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
>
> I'll try to reivew this week and respond with my feedback.
>
> adam
>
> > ---
> > Marco Felsch (1):
> >       drm/bridge: samsung-dsim: add more mipi-dsi device debug information
> >
> > Michael Tretter (4):
> >       drm/bridge: samsung-dsim: reread ref clock before configuring PLL
> >       drm/bridge: samsung-dsim: update PLL reference clock
> >       drm/bridge: samsung-dsim: adjust porches by rounding up
> >       drm/bridge: samsung-dsim: calculate porches in Hz
> >
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 42 +++++++++++++++++++++++++----------
> >  include/drm/bridge/samsung-dsim.h     |  1 +
> >  2 files changed, 31 insertions(+), 12 deletions(-)
> > ---
> > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > change-id: 20230818-samsung-dsim-42346444bce5
> >
> > Best regards,
> > --
> > Michael Tretter <m.tretter@pengutronix.de>
> >

Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge

[Michael Tretter]

On Mon, 28 Aug 2023 18:07:08 -0500, Adam Ford wrote:
> On Mon, Aug 28, 2023 at 11:13 AM Adam Ford <aford173@gmail.com> wrote:
> >
> > On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter
> > <m.tretter@pengutronix.de> wrote:
> > >
> > > I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter,
> > > which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few
> > > modes were working, but in many modes my monitor stayed dark.
> > >
> > > This series fixes the Samsung DSIM bridge driver to bring up a few more
> > > modes:
> > >
> > > The driver read the rate of the PLL ref clock only during probe.
> > > However, if the clock is re-parented to the VIDEO_PLL, changes to the
> > > pixel clock have an effect on the PLL ref clock. Therefore, the driver
> > > must read and potentially update the PLL ref clock on every modeset.
> > >
> > > I also found that the rounding mode of the porches and active area has
> > > an effect on the working modes. If the driver rounds up instead of
> > > rounding down and be calculates them in Hz instead of kHz, more modes
> > > start to work.
> > >
> > > The following table shows the modes that were working in my test without
> > > this patch set and the modes that are working now:
> > >
> > > |            Mode | Before | Now |
> > > | 1920x1080-60.00 | X      | X   |
> > > | 1920x1080-59.94 |        | X   |
> > > | 1920x1080-50.00 |        | X   |
> > > | 1920x1080-30.00 |        | X   |
> > > | 1920x1080-29.97 |        | X   |
> > > | 1920x1080-25.00 |        | X   |
> > > | 1920x1080-24.00 |        |     |
> > > | 1920x1080-23.98 |        |     |
> > > | 1680x1050-59.88 |        | X   |
> > > | 1280x1024-75.03 | X      | X   |
> > > | 1280x1024-60.02 | X      | X   |
> > > |  1200x960-59.99 |        | X   |
> > > |  1152x864-75.00 | X      | X   |
> > > |  1280x720-60.00 |        |     |
> > > |  1280x720-59.94 |        |     |
> > > |  1280x720-50.00 |        | X   |
> > > |  1024x768-75.03 |        | X   |
> > > |  1024x768-60.00 |        | X   |
> > > |   800x600-75.00 | X      | X   |
> > > |   800x600-60.32 | X      | X   |
> > > |   720x576-50.00 | X      | X   |
> > > |   720x480-60.00 |        |     |
> > > |   720x480-59.94 | X      |     |
> > > |   640x480-75.00 | X      | X   |
> > > |   640x480-60.00 |        | X   |
> > > |   640x480-59.94 |        | X   |
> > > |   720x400-70.08 |        |     |
> > >
> >
> > Nice!
> >
> > > Interestingly, the 720x480-59.94 mode stopped working. However, I am
> > > able to bring up the 720x480 modes by manually hacking the active area
> > > (hsa) to 40 and carefully adjusting the clocks, but something still
> > > seems to be off.
> >
> > Checkout my
> >
> > branch: https://github.com/aford173/linux/commit/183cf6d154afeb9b0300500b09d7b8ec53047a12

Thanks for the pointer.

> >
> > I found that certain resolutions don't properly round, and it seemed
> > to be related to the size of HFP.  HFP of 110 when divded between 4
> > lanes, required a round-up, but then I had to recalculate the rest of
> > the horizontal timings to compensate.
> >
> > I was going to push that as an RFC, but I will investigate your patch
> > series first.  With my small rounding correction, I am able to get
> > 720x480 and 720p on my imx8mp, but not my mini/nano, so I am hoping
> > your patch series fixes that issue for me.
> >
> > >
> > > Unfortunately, a few more modes are still not working at all. The NXP
> > > downstream kernel has some quirks to handle some of the modes especially
> > > wrt. to the porches, but I cannot figure out, what the driver should
> > > actually do in these cases. Maybe there is still an error in the
> > > calculation of the porches and someone at NXP can chime in.
> >
> > Hopefully the comment in my above patch explains how the calculation
> > is corrected and adjusted to get some of the missing resolutions.
> 
> I tested my above patch and it works to sync 720p-60 on my imx8mp, but
> it doesn't seem to sync on my imx8mm using the same monitor and HDMI
> cable.  I don't have a DSI analyzer, so I might still send out a
> modified version of my patch as an RFC once this gets approved.

I tested your patch with all modes of my monitor. 720p-60 doesn't work on my
imx8mn either. The results are the same for DIV_ROUND_CLOSEST and
DIV64_U64_ROUND_UP, but I didn't check for differences in the actually
calculated HFP, yet.

Michael

> 
> adam
> >
> > > Michael
> > >
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> >
> > I'll try to reivew this week and respond with my feedback.
> >
> > adam
> >
> > > ---
> > > Marco Felsch (1):
> > >       drm/bridge: samsung-dsim: add more mipi-dsi device debug information
> > >
> > > Michael Tretter (4):
> > >       drm/bridge: samsung-dsim: reread ref clock before configuring PLL
> > >       drm/bridge: samsung-dsim: update PLL reference clock
> > >       drm/bridge: samsung-dsim: adjust porches by rounding up
> > >       drm/bridge: samsung-dsim: calculate porches in Hz
> > >
> > >  drivers/gpu/drm/bridge/samsung-dsim.c | 42 +++++++++++++++++++++++++----------
> > >  include/drm/bridge/samsung-dsim.h     |  1 +
> > >  2 files changed, 31 insertions(+), 12 deletions(-)
> > > ---
> > > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > > change-id: 20230818-samsung-dsim-42346444bce5
> > >
> > > Best regards,
> > > --
> > > Michael Tretter <m.tretter@pengutronix.de>
> > >
> 

Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge

[Marco Felsch]

On 23-08-28, Michael Tretter wrote:
> I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter,
> which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few
> modes were working, but in many modes my monitor stayed dark.
> 
> This series fixes the Samsung DSIM bridge driver to bring up a few more
> modes:
> 
> The driver read the rate of the PLL ref clock only during probe.
> However, if the clock is re-parented to the VIDEO_PLL, changes to the
> pixel clock have an effect on the PLL ref clock. Therefore, the driver
> must read and potentially update the PLL ref clock on every modeset.
> 
> I also found that the rounding mode of the porches and active area has
> an effect on the working modes. If the driver rounds up instead of
> rounding down and be calculates them in Hz instead of kHz, more modes
> start to work.
> 
> The following table shows the modes that were working in my test without
> this patch set and the modes that are working now:
> 
> |            Mode | Before | Now |
> | 1920x1080-60.00 | X      | X   |
> | 1920x1080-59.94 |        | X   |
> | 1920x1080-50.00 |        | X   |
> | 1920x1080-30.00 |        | X   |
> | 1920x1080-29.97 |        | X   |
> | 1920x1080-25.00 |        | X   |
> | 1920x1080-24.00 |        |     |
> | 1920x1080-23.98 |        |     |
> | 1680x1050-59.88 |        | X   |
> | 1280x1024-75.03 | X      | X   |
> | 1280x1024-60.02 | X      | X   |
> |  1200x960-59.99 |        | X   |
> |  1152x864-75.00 | X      | X   |
> |  1280x720-60.00 |        |     |
> |  1280x720-59.94 |        |     |
> |  1280x720-50.00 |        | X   |
> |  1024x768-75.03 |        | X   |
> |  1024x768-60.00 |        | X   |
> |   800x600-75.00 | X      | X   |
> |   800x600-60.32 | X      | X   |
> |   720x576-50.00 | X      | X   |
> |   720x480-60.00 |        |     |
> |   720x480-59.94 | X      |     |
> |   640x480-75.00 | X      | X   |
> |   640x480-60.00 |        | X   |
> |   640x480-59.94 |        | X   |
> |   720x400-70.08 |        |     |
> 
> Interestingly, the 720x480-59.94 mode stopped working. However, I am
> able to bring up the 720x480 modes by manually hacking the active area
> (hsa) to 40 and carefully adjusting the clocks, but something still
> seems to be off.
> 
> Unfortunately, a few more modes are still not working at all. The NXP
> downstream kernel has some quirks to handle some of the modes especially
> wrt. to the porches, but I cannot figure out, what the driver should
> actually do in these cases. Maybe there is still an error in the
> calculation of the porches and someone at NXP can chime in.
> 
> Michael
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> Marco Felsch (1):
>       drm/bridge: samsung-dsim: add more mipi-dsi device debug information
> 
> Michael Tretter (4):
>       drm/bridge: samsung-dsim: reread ref clock before configuring PLL
>       drm/bridge: samsung-dsim: update PLL reference clock
>       drm/bridge: samsung-dsim: adjust porches by rounding up
>       drm/bridge: samsung-dsim: calculate porches in Hz

Feel free to add my r-b for your patches.

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge

[Frieder Schrempf]

Hi Michael,

On 28.08.23 17:59, Michael Tretter wrote:
> I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter,
> which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few
> modes were working, but in many modes my monitor stayed dark.
> 
> This series fixes the Samsung DSIM bridge driver to bring up a few more
> modes:
> 
> The driver read the rate of the PLL ref clock only during probe.
> However, if the clock is re-parented to the VIDEO_PLL, changes to the
> pixel clock have an effect on the PLL ref clock. Therefore, the driver
> must read and potentially update the PLL ref clock on every modeset.
> 
> I also found that the rounding mode of the porches and active area has
> an effect on the working modes. If the driver rounds up instead of
> rounding down and be calculates them in Hz instead of kHz, more modes
> start to work.
> 
> The following table shows the modes that were working in my test without
> this patch set and the modes that are working now:
> 
> |            Mode | Before | Now |
> | 1920x1080-60.00 | X      | X   |
> | 1920x1080-59.94 |        | X   |
> | 1920x1080-50.00 |        | X   |
> | 1920x1080-30.00 |        | X   |
> | 1920x1080-29.97 |        | X   |
> | 1920x1080-25.00 |        | X   |
> | 1920x1080-24.00 |        |     |
> | 1920x1080-23.98 |        |     |
> | 1680x1050-59.88 |        | X   |
> | 1280x1024-75.03 | X      | X   |
> | 1280x1024-60.02 | X      | X   |
> |  1200x960-59.99 |        | X   |
> |  1152x864-75.00 | X      | X   |
> |  1280x720-60.00 |        |     |
> |  1280x720-59.94 |        |     |
> |  1280x720-50.00 |        | X   |
> |  1024x768-75.03 |        | X   |
> |  1024x768-60.00 |        | X   |
> |   800x600-75.00 | X      | X   |
> |   800x600-60.32 | X      | X   |
> |   720x576-50.00 | X      | X   |
> |   720x480-60.00 |        |     |
> |   720x480-59.94 | X      |     |
> |   640x480-75.00 | X      | X   |
> |   640x480-60.00 |        | X   |
> |   640x480-59.94 |        | X   |
> |   720x400-70.08 |        |     |
> 
> Interestingly, the 720x480-59.94 mode stopped working. However, I am
> able to bring up the 720x480 modes by manually hacking the active area
> (hsa) to 40 and carefully adjusting the clocks, but something still
> seems to be off.
> 
> Unfortunately, a few more modes are still not working at all. The NXP
> downstream kernel has some quirks to handle some of the modes especially
> wrt. to the porches, but I cannot figure out, what the driver should
> actually do in these cases. Maybe there is still an error in the
> calculation of the porches and someone at NXP can chime in.

Thanks for working on this! We tested these patches with our Kontron BL
i.MX8MM board and a "10.1inch HDMI LCD (E)" display from Waveshare  [1].

Without this series we don't get an image with the default mode of the
display (1024x600). With this series applied, it's now working.

For the whole series:

Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de> # Kontron BL
i.MX8MM + Waveshare 10.1inch HDMI LCD (E)

Thanks
Frieder

[1] https://www.waveshare.com/10.1inch-hdmi-lcd-e.htm

Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge

[Frieder Schrempf]

On 04.09.23 16:02, Frieder Schrempf wrote:
> Hi Michael,
> 
> On 28.08.23 17:59, Michael Tretter wrote:
>> I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter,
>> which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few
>> modes were working, but in many modes my monitor stayed dark.
>>
>> This series fixes the Samsung DSIM bridge driver to bring up a few more
>> modes:
>>
>> The driver read the rate of the PLL ref clock only during probe.
>> However, if the clock is re-parented to the VIDEO_PLL, changes to the
>> pixel clock have an effect on the PLL ref clock. Therefore, the driver
>> must read and potentially update the PLL ref clock on every modeset.
>>
>> I also found that the rounding mode of the porches and active area has
>> an effect on the working modes. If the driver rounds up instead of
>> rounding down and be calculates them in Hz instead of kHz, more modes
>> start to work.
>>
>> The following table shows the modes that were working in my test without
>> this patch set and the modes that are working now:
>>
>> |            Mode | Before | Now |
>> | 1920x1080-60.00 | X      | X   |
>> | 1920x1080-59.94 |        | X   |
>> | 1920x1080-50.00 |        | X   |
>> | 1920x1080-30.00 |        | X   |
>> | 1920x1080-29.97 |        | X   |
>> | 1920x1080-25.00 |        | X   |
>> | 1920x1080-24.00 |        |     |
>> | 1920x1080-23.98 |        |     |
>> | 1680x1050-59.88 |        | X   |
>> | 1280x1024-75.03 | X      | X   |
>> | 1280x1024-60.02 | X      | X   |
>> |  1200x960-59.99 |        | X   |
>> |  1152x864-75.00 | X      | X   |
>> |  1280x720-60.00 |        |     |
>> |  1280x720-59.94 |        |     |
>> |  1280x720-50.00 |        | X   |
>> |  1024x768-75.03 |        | X   |
>> |  1024x768-60.00 |        | X   |
>> |   800x600-75.00 | X      | X   |
>> |   800x600-60.32 | X      | X   |
>> |   720x576-50.00 | X      | X   |
>> |   720x480-60.00 |        |     |
>> |   720x480-59.94 | X      |     |
>> |   640x480-75.00 | X      | X   |
>> |   640x480-60.00 |        | X   |
>> |   640x480-59.94 |        | X   |
>> |   720x400-70.08 |        |     |
>>
>> Interestingly, the 720x480-59.94 mode stopped working. However, I am
>> able to bring up the 720x480 modes by manually hacking the active area
>> (hsa) to 40 and carefully adjusting the clocks, but something still
>> seems to be off.
>>
>> Unfortunately, a few more modes are still not working at all. The NXP
>> downstream kernel has some quirks to handle some of the modes especially
>> wrt. to the porches, but I cannot figure out, what the driver should
>> actually do in these cases. Maybe there is still an error in the
>> calculation of the porches and someone at NXP can chime in.
> 
> Thanks for working on this! We tested these patches with our Kontron BL
> i.MX8MM board and a "10.1inch HDMI LCD (E)" display from Waveshare  [1].
> 
> Without this series we don't get an image with the default mode of the
> display (1024x600). With this series applied, it's now working.

Minor correction: The display does work, but there is some flickering
and occasional black screens if you let it run for some time. So there
is still some sync issue.

Anyway it's better than not working at all.

> 
> For the whole series:
> 
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de> # Kontron BL
> i.MX8MM + Waveshare 10.1inch HDMI LCD (E)
> 
> Thanks
> Frieder
> 
> [1] https://www.waveshare.com/10.1inch-hdmi-lcd-e.htm

Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge

[Michael Tretter]

Hi Frieder,

On Wed, 06 Sep 2023 11:31:45 +0200, Frieder Schrempf wrote:
> On 04.09.23 16:02, Frieder Schrempf wrote:
> > On 28.08.23 17:59, Michael Tretter wrote:
> >> I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter,
> >> which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few
> >> modes were working, but in many modes my monitor stayed dark.
> >>
> >> This series fixes the Samsung DSIM bridge driver to bring up a few more
> >> modes:
> >>
> >> The driver read the rate of the PLL ref clock only during probe.
> >> However, if the clock is re-parented to the VIDEO_PLL, changes to the
> >> pixel clock have an effect on the PLL ref clock. Therefore, the driver
> >> must read and potentially update the PLL ref clock on every modeset.
> >>
> >> I also found that the rounding mode of the porches and active area has
> >> an effect on the working modes. If the driver rounds up instead of
> >> rounding down and be calculates them in Hz instead of kHz, more modes
> >> start to work.
> >>
> >> The following table shows the modes that were working in my test without
> >> this patch set and the modes that are working now:
> >>
> >> |            Mode | Before | Now |
> >> | 1920x1080-60.00 | X      | X   |
> >> | 1920x1080-59.94 |        | X   |
> >> | 1920x1080-50.00 |        | X   |
> >> | 1920x1080-30.00 |        | X   |
> >> | 1920x1080-29.97 |        | X   |
> >> | 1920x1080-25.00 |        | X   |
> >> | 1920x1080-24.00 |        |     |
> >> | 1920x1080-23.98 |        |     |
> >> | 1680x1050-59.88 |        | X   |
> >> | 1280x1024-75.03 | X      | X   |
> >> | 1280x1024-60.02 | X      | X   |
> >> |  1200x960-59.99 |        | X   |
> >> |  1152x864-75.00 | X      | X   |
> >> |  1280x720-60.00 |        |     |
> >> |  1280x720-59.94 |        |     |
> >> |  1280x720-50.00 |        | X   |
> >> |  1024x768-75.03 |        | X   |
> >> |  1024x768-60.00 |        | X   |
> >> |   800x600-75.00 | X      | X   |
> >> |   800x600-60.32 | X      | X   |
> >> |   720x576-50.00 | X      | X   |
> >> |   720x480-60.00 |        |     |
> >> |   720x480-59.94 | X      |     |
> >> |   640x480-75.00 | X      | X   |
> >> |   640x480-60.00 |        | X   |
> >> |   640x480-59.94 |        | X   |
> >> |   720x400-70.08 |        |     |
> >>
> >> Interestingly, the 720x480-59.94 mode stopped working. However, I am
> >> able to bring up the 720x480 modes by manually hacking the active area
> >> (hsa) to 40 and carefully adjusting the clocks, but something still
> >> seems to be off.
> >>
> >> Unfortunately, a few more modes are still not working at all. The NXP
> >> downstream kernel has some quirks to handle some of the modes especially
> >> wrt. to the porches, but I cannot figure out, what the driver should
> >> actually do in these cases. Maybe there is still an error in the
> >> calculation of the porches and someone at NXP can chime in.
> > 
> > Thanks for working on this! We tested these patches with our Kontron BL
> > i.MX8MM board and a "10.1inch HDMI LCD (E)" display from Waveshare  [1].
> > 
> > Without this series we don't get an image with the default mode of the
> > display (1024x600). With this series applied, it's now working.
> 
> Minor correction: The display does work, but there is some flickering
> and occasional black screens if you let it run for some time. So there
> is still some sync issue.

What is the parent of the dsi_phy_ref clock in your test case?  Make sure that
the lcdif_pixel and dsi_phy_ref clocks are driven by the same PLL.

I saw occasional black screens, if the lcdif_pixel and dsi_phy_ref clock had
different parents, and fixed it by changing the assigned-parent of
IMX8MN_CLK_DSI_PHY_REF to IMX8MN_VIDEO_PLL1_OUT in the device tree. If the
clocks are not in sync, it seems that the ADV3575 has issues correctly
reconstructing the pixel clock and HDMI monitors loose sync.

This is something that must be configured in the board device tree, though.

Michael

> 
> Anyway it's better than not working at all.
> 
> > 
> > For the whole series:
> > 
> > Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de> # Kontron BL
> > i.MX8MM + Waveshare 10.1inch HDMI LCD (E)
> > 
> > Thanks
> > Frieder
> > 
> > [1] https://www.waveshare.com/10.1inch-hdmi-lcd-e.htm
> 

Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge

[Frieder Schrempf]

Hi Michael,

On 06.09.23 11:56, Michael Tretter wrote:
> Hi Frieder,
> 
> On Wed, 06 Sep 2023 11:31:45 +0200, Frieder Schrempf wrote:
>> On 04.09.23 16:02, Frieder Schrempf wrote:
>>> On 28.08.23 17:59, Michael Tretter wrote:
>>>> I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter,
>>>> which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few
>>>> modes were working, but in many modes my monitor stayed dark.
>>>>
>>>> This series fixes the Samsung DSIM bridge driver to bring up a few more
>>>> modes:
>>>>
>>>> The driver read the rate of the PLL ref clock only during probe.
>>>> However, if the clock is re-parented to the VIDEO_PLL, changes to the
>>>> pixel clock have an effect on the PLL ref clock. Therefore, the driver
>>>> must read and potentially update the PLL ref clock on every modeset.
>>>>
>>>> I also found that the rounding mode of the porches and active area has
>>>> an effect on the working modes. If the driver rounds up instead of
>>>> rounding down and be calculates them in Hz instead of kHz, more modes
>>>> start to work.
>>>>
>>>> The following table shows the modes that were working in my test without
>>>> this patch set and the modes that are working now:
>>>>
>>>> |            Mode | Before | Now |
>>>> | 1920x1080-60.00 | X      | X   |
>>>> | 1920x1080-59.94 |        | X   |
>>>> | 1920x1080-50.00 |        | X   |
>>>> | 1920x1080-30.00 |        | X   |
>>>> | 1920x1080-29.97 |        | X   |
>>>> | 1920x1080-25.00 |        | X   |
>>>> | 1920x1080-24.00 |        |     |
>>>> | 1920x1080-23.98 |        |     |
>>>> | 1680x1050-59.88 |        | X   |
>>>> | 1280x1024-75.03 | X      | X   |
>>>> | 1280x1024-60.02 | X      | X   |
>>>> |  1200x960-59.99 |        | X   |
>>>> |  1152x864-75.00 | X      | X   |
>>>> |  1280x720-60.00 |        |     |
>>>> |  1280x720-59.94 |        |     |
>>>> |  1280x720-50.00 |        | X   |
>>>> |  1024x768-75.03 |        | X   |
>>>> |  1024x768-60.00 |        | X   |
>>>> |   800x600-75.00 | X      | X   |
>>>> |   800x600-60.32 | X      | X   |
>>>> |   720x576-50.00 | X      | X   |
>>>> |   720x480-60.00 |        |     |
>>>> |   720x480-59.94 | X      |     |
>>>> |   640x480-75.00 | X      | X   |
>>>> |   640x480-60.00 |        | X   |
>>>> |   640x480-59.94 |        | X   |
>>>> |   720x400-70.08 |        |     |
>>>>
>>>> Interestingly, the 720x480-59.94 mode stopped working. However, I am
>>>> able to bring up the 720x480 modes by manually hacking the active area
>>>> (hsa) to 40 and carefully adjusting the clocks, but something still
>>>> seems to be off.
>>>>
>>>> Unfortunately, a few more modes are still not working at all. The NXP
>>>> downstream kernel has some quirks to handle some of the modes especially
>>>> wrt. to the porches, but I cannot figure out, what the driver should
>>>> actually do in these cases. Maybe there is still an error in the
>>>> calculation of the porches and someone at NXP can chime in.
>>>
>>> Thanks for working on this! We tested these patches with our Kontron BL
>>> i.MX8MM board and a "10.1inch HDMI LCD (E)" display from Waveshare  [1].
>>>
>>> Without this series we don't get an image with the default mode of the
>>> display (1024x600). With this series applied, it's now working.
>>
>> Minor correction: The display does work, but there is some flickering
>> and occasional black screens if you let it run for some time. So there
>> is still some sync issue.
> 
> What is the parent of the dsi_phy_ref clock in your test case?

It's the IMX8MM_CLK_24M default from imx8mm.dtsi.

> Make sure that
> the lcdif_pixel and dsi_phy_ref clocks are driven by the same PLL.
> 
> I saw occasional black screens, if the lcdif_pixel and dsi_phy_ref clock had
> different parents, and fixed it by changing the assigned-parent of
> IMX8MN_CLK_DSI_PHY_REF to IMX8MN_VIDEO_PLL1_OUT in the device tree. If the
> clocks are not in sync, it seems that the ADV3575 has issues correctly
> reconstructing the pixel clock and HDMI monitors loose sync.
> 
> This is something that must be configured in the board device tree, though.

Ok, I tried to set the parent of IMX8MM_CLK_DSI_PHY_REF to
IMX8MM_VIDEO_PLL1_OUT.

I also noticed, that I forgot to remove the samsung,pll-clock-frequency
from the mipi_dsi node in imx8mm.dtsi. This is necessary to make use of
your PLL input clock improvements. Otherwise I get 23.76 MHz for the
DSIM PLL input (derived from IMX8MM_VIDEO_PLL1_OUT) which results in a
HS clock of 300.96 MHz while the display requests 301.5 MHz (50.25 MHz
pixel clock). This is too low and the display doesn't work at all.

Unfortunately all of this doesn't result in a more stable image on the
10" Waveshare display. It seems like the display turns black whenever
the Qt app does a lot of framebuffer updates, e. g. during animations, etc.

Anyway, thanks for the help!

Best regards
Frieder