[PATCH V3 0/7] drm: bridge: samsung-dsim: Support variable clocking

[PATCH V3 0/7] drm: bridge: samsung-dsim: Support variable clocking

[Adam Ford]

This series fixes the blanking pack size and the PMS calculation.  It then
adds support to allows the DSIM to dynamically DPHY clocks, and support
non-burst mode while allowing the removal of the hard-coded clock values
for the PLL for imx8m mini/nano/plus, and it allows the removal of the
burst-clock device tree entry when burst-mode isn't supported by connected
devices like an HDMI brige.  In that event, the HS clock is set to the value
requested by the bridge chip.

This has been tested on both an i.MX8M Nano and i.MX8M Plus, and should work
on i.MX8M Mini as well.

Adam Ford (6):
  drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]
  drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically
  drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY
  drm: bridge: samsung-dsim: Dynamically configure DPHY timing
  drm: bridge: samsung-dsim: Support non-burst mode
  drm: bridge: samsung-dsim: Let blanking calcuation work in non-burst
    mode

Lucas Stach (1):
  drm: bridge: samsung-dsim: fix blanking packet size calculation

 drivers/gpu/drm/bridge/Kconfig        |   1 +
 drivers/gpu/drm/bridge/samsung-dsim.c | 150 ++++++++++++++++++++++----
 include/drm/bridge/samsung-dsim.h     |   5 +
 3 files changed, 136 insertions(+), 20 deletions(-)

---
V3:  When checking if the bust-clock is present, only check for it
     in the device tree, and don't check the presence of the 
     MIPI_DSI_MODE_VIDEO_BURST flag as it breaks an existing Exynos
     board.
     
     Add a new patch to the series to select GENERIC_PHY_MIPI_DPHY in
     Kconfig otherwise the build breaks on the 32-bit Exynos.
     
     Change vco_min variable name to min_freq

     Added tested-by from Chen-Yu Tsai

V2:  Instead of using my packet blanking calculation, this integrates
     on from Lucas Stach which gets modified later in the series to
     cache the value of the HS-clock instead of having to do the
     calucations again.

     Instead of completely eliminating the PLL clock frequency from
     the device tree, this makes it optional to avoid breaking some
     Samsung devices.  When the samsung,pll-clock-frequency is not
     found, it reads the value of the clock named "sclk_mipi"
     This also maintains backwords compatibility with older device
     trees.

     This also changes the DPHY calcuation from a Look-up table,
     a reverse engineered algorithm which uses
     phy_mipi_dphy_get_default_config to determine the standard
     nominal values and calculates the cycles necessary to update
     the DPHY timings accordingly.
     
-- 
2.39.2

[PATCH V3 1/7] drm: bridge: samsung-dsim: fix blanking packet size calculation

[Adam Ford]

From: Lucas Stach <l.stach@pengutronix.de>

Scale the blanking packet sizes to match the ratio between HS clock
and DPI interface clock. The controller seems to do internal scaling
to the number of active lanes, so we don't take those into account.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Adam Ford <aford173@gmail.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index e0a402a85787..2be3b58624c3 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -874,17 +874,29 @@ 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->burst_clk_rate / 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;
+
+		/* remove packet overhead when possible */
+		hfp = max(hfp - 6, 0);
+		hbp = max(hbp - 6, 0);
+		hsa = max(hsa - 6, 0);
+
+		dev_dbg(dsi->dev, "calculated hfp: %u, hbp: %u, hsa: %u",
+			hfp, hbp, hsa);
+
 		reg = DSIM_CMD_ALLOW(0xf)
 			| DSIM_STABLE_VFP(m->vsync_start - m->vdisplay)
 			| DSIM_MAIN_VBP(m->vtotal - m->vsync_end);
 		samsung_dsim_write(dsi, DSIM_MVPORCH_REG, reg);
 
-		reg = DSIM_MAIN_HFP(m->hsync_start - m->hdisplay)
-			| DSIM_MAIN_HBP(m->htotal - m->hsync_end);
+		reg = DSIM_MAIN_HFP(hfp) | DSIM_MAIN_HBP(hbp);
 		samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
 
 		reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
-			| DSIM_MAIN_HSA(m->hsync_end - m->hsync_start);
+			| DSIM_MAIN_HSA(hsa);
 		samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
 	}
 	reg =  DSIM_MAIN_HRESOL(m->hdisplay, num_bits_resol) |
-- 
2.39.2

Re: [PATCH V3 1/7] drm: bridge: samsung-dsim: fix blanking packet size calculation

[Frieder Schrempf]

On 02.05.23 03:07, Adam Ford wrote:
> From: Lucas Stach <l.stach@pengutronix.de>
> 
> Scale the blanking packet sizes to match the ratio between HS clock
> and DPI interface clock. The controller seems to do internal scaling
> to the number of active lanes, so we don't take those into account.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>

Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>

[PATCH V3 2/7] drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]

[Adam Ford]

According to Table 13-45 of the i.MX8M Mini Reference Manual, the min
and max values for M and the frequency range for the VCO_out
calculator were incorrect.  This information was contradicted in other
parts of the mini, nano and plus manuals.  After reaching out to my
NXP Rep, when confronting him about discrepencies in the Nano manual,
he responded with:
 "Yes it is definitely wrong, the one that is part
  of the NOTE in MIPI_DPHY_M_PLLPMS register table against PMS_P,
  PMS_M and PMS_S is not correct. I will report this to Doc team,
  the one customer should be take into account is the Table 13-40
  DPHY PLL Parameters and the Note above."

These updated values also match what is used in the NXP downstream
kernel.

To fix this, make new variables to hold the min and max values of m
and the minimum value of VCO_out, and update the PMS calculator to
use these new variables instead of using hard-coded values to keep
the backwards compatibility with other parts using this driver.

Fixes: 4d562c70c4dc ("drm: bridge: samsung-dsim: Add i.MX8M Mini/Nano support")
Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 22 ++++++++++++++++++++--
 include/drm/bridge/samsung-dsim.h     |  3 +++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 2be3b58624c3..bf4b33d2de76 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -405,6 +405,9 @@ static const struct samsung_dsim_driver_data exynos3_dsi_driver_data = {
 	.num_bits_resol = 11,
 	.pll_p_offset = 13,
 	.reg_values = reg_values,
+	.m_min = 41,
+	.m_max = 125,
+	.min_freq = 500,
 };
 
 static const struct samsung_dsim_driver_data exynos4_dsi_driver_data = {
@@ -418,6 +421,9 @@ static const struct samsung_dsim_driver_data exynos4_dsi_driver_data = {
 	.num_bits_resol = 11,
 	.pll_p_offset = 13,
 	.reg_values = reg_values,
+	.m_min = 41,
+	.m_max = 125,
+	.min_freq = 500,
 };
 
 static const struct samsung_dsim_driver_data exynos5_dsi_driver_data = {
@@ -429,6 +435,9 @@ static const struct samsung_dsim_driver_data exynos5_dsi_driver_data = {
 	.num_bits_resol = 11,
 	.pll_p_offset = 13,
 	.reg_values = reg_values,
+	.m_min = 41,
+	.m_max = 125,
+	.min_freq = 500,
 };
 
 static const struct samsung_dsim_driver_data exynos5433_dsi_driver_data = {
@@ -441,6 +450,9 @@ static const struct samsung_dsim_driver_data exynos5433_dsi_driver_data = {
 	.num_bits_resol = 12,
 	.pll_p_offset = 13,
 	.reg_values = exynos5433_reg_values,
+	.m_min = 41,
+	.m_max = 125,
+	.min_freq = 500,
 };
 
 static const struct samsung_dsim_driver_data exynos5422_dsi_driver_data = {
@@ -453,6 +465,9 @@ static const struct samsung_dsim_driver_data exynos5422_dsi_driver_data = {
 	.num_bits_resol = 12,
 	.pll_p_offset = 13,
 	.reg_values = exynos5422_reg_values,
+	.m_min = 41,
+	.m_max = 125,
+	.min_freq = 500,
 };
 
 static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
@@ -469,6 +484,9 @@ static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
 	 */
 	.pll_p_offset = 14,
 	.reg_values = imx8mm_dsim_reg_values,
+	.m_min = 64,
+	.m_max = 1023,
+	.min_freq = 1050,
 };
 
 static const struct samsung_dsim_driver_data *
@@ -547,12 +565,12 @@ static unsigned long samsung_dsim_pll_find_pms(struct samsung_dsim *dsi,
 			tmp = (u64)fout * (_p << _s);
 			do_div(tmp, fin);
 			_m = tmp;
-			if (_m < 41 || _m > 125)
+			if (_m < driver_data->m_min || _m > driver_data->m_max)
 				continue;
 
 			tmp = (u64)_m * fin;
 			do_div(tmp, _p);
-			if (tmp < 500 * MHZ ||
+			if (tmp < driver_data->min_freq  * MHZ ||
 			    tmp > driver_data->max_freq * MHZ)
 				continue;
 
diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
index ba5484de2b30..a1a5b2b89a7a 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -54,11 +54,14 @@ struct samsung_dsim_driver_data {
 	unsigned int has_freqband:1;
 	unsigned int has_clklane_stop:1;
 	unsigned int num_clks;
+	unsigned int min_freq;
 	unsigned int max_freq;
 	unsigned int wait_for_reset;
 	unsigned int num_bits_resol;
 	unsigned int pll_p_offset;
 	const unsigned int *reg_values;
+	u16 m_min;
+	u16 m_max;
 };
 
 struct samsung_dsim_host_ops {
-- 
2.39.2

Re: [PATCH V3 2/7] drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]

[Frieder Schrempf]

On 02.05.23 03:07, Adam Ford wrote:
> According to Table 13-45 of the i.MX8M Mini Reference Manual, the min
> and max values for M and the frequency range for the VCO_out
> calculator were incorrect.  This information was contradicted in other
> parts of the mini, nano and plus manuals.  After reaching out to my
> NXP Rep, when confronting him about discrepencies in the Nano manual,
> he responded with:
>  "Yes it is definitely wrong, the one that is part
>   of the NOTE in MIPI_DPHY_M_PLLPMS register table against PMS_P,
>   PMS_M and PMS_S is not correct. I will report this to Doc team,
>   the one customer should be take into account is the Table 13-40
>   DPHY PLL Parameters and the Note above."
> 
> These updated values also match what is used in the NXP downstream
> kernel.
> 
> To fix this, make new variables to hold the min and max values of m
> and the minimum value of VCO_out, and update the PMS calculator to
> use these new variables instead of using hard-coded values to keep
> the backwards compatibility with other parts using this driver.
> 
> Fixes: 4d562c70c4dc ("drm: bridge: samsung-dsim: Add i.MX8M Mini/Nano support")
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>

Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges.

Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

[PATCH V3 3/7] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Adam Ford]

Make the pll-clock-frequency optional.  If it's present, use it
to maintain backwards compatibility with existing hardware.  If it
is absent, read clock rate of "sclk_mipi" to determine the rate.

Signed-off-by: Adam Ford <aford173@gmail.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index bf4b33d2de76..2dc02a9e37c0 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1726,12 +1726,20 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi)
 {
 	struct device *dev = dsi->dev;
 	struct device_node *node = dev->of_node;
+	struct clk *pll_clk;
 	int ret;
 
 	ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
 				       &dsi->pll_clk_rate);
-	if (ret < 0)
-		return ret;
+
+	/* If it doesn't exist, read it from the clock instead of failing */
+	if (ret < 0) {
+		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);
+	}
 
 	ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency",
 				       &dsi->burst_clk_rate);
-- 
2.39.2

Re: [PATCH V3 3/7] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Frieder Schrempf]

On 02.05.23 03:07, Adam Ford wrote:
> Make the pll-clock-frequency optional.  If it's present, use it
> to maintain backwards compatibility with existing hardware.  If it
> is absent, read clock rate of "sclk_mipi" to determine the rate.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>

Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges.

Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Re: [PATCH V3 3/7] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Alexander Stein]

Am Dienstag, 2. Mai 2023, 03:07:55 CEST schrieb Adam Ford:
> Make the pll-clock-frequency optional.  If it's present, use it
> to maintain backwards compatibility with existing hardware.  If it
> is absent, read clock rate of "sclk_mipi" to determine the rate.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c index bf4b33d2de76..2dc02a9e37c0
> 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1726,12 +1726,20 @@ static int samsung_dsim_parse_dt(struct samsung_dsim
> *dsi) {
>  	struct device *dev = dsi->dev;
>  	struct device_node *node = dev->of_node;
> +	struct clk *pll_clk;
>  	int ret;
> 
>  	ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
>  				       &dsi->pll_clk_rate);
> -	if (ret < 0)
> -		return ret;
> +
> +	/* If it doesn't exist, read it from the clock instead of failing */
> +	if (ret < 0) {
> +		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);
> +	}
> 

Now that 'samsung,pll-clock-frequency' is optional the error in 
samsung_dsim_of_read_u32() should be changed. Otherwise you will get
> /soc@0/bus@32c00000/dsi@32e10000: failed to get 'samsung,pll-clock-
frequency' property

Best regards,
Alexander

>  	ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-
frequency",
>  				       &dsi->burst_clk_rate);


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH V3 3/7] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Adam Ford]

On Thu, May 4, 2023 at 4:21 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Am Dienstag, 2. Mai 2023, 03:07:55 CEST schrieb Adam Ford:
> > Make the pll-clock-frequency optional.  If it's present, use it
> > to maintain backwards compatibility with existing hardware.  If it
> > is absent, read clock rate of "sclk_mipi" to determine the rate.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > b/drivers/gpu/drm/bridge/samsung-dsim.c index bf4b33d2de76..2dc02a9e37c0
> > 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -1726,12 +1726,20 @@ static int samsung_dsim_parse_dt(struct samsung_dsim
> > *dsi) {
> >       struct device *dev = dsi->dev;
> >       struct device_node *node = dev->of_node;
> > +     struct clk *pll_clk;
> >       int ret;
> >
> >       ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
> >                                      &dsi->pll_clk_rate);
> > -     if (ret < 0)
> > -             return ret;
> > +
> > +     /* If it doesn't exist, read it from the clock instead of failing */
> > +     if (ret < 0) {
> > +             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);
> > +     }
> >
>
> Now that 'samsung,pll-clock-frequency' is optional the error in
> samsung_dsim_of_read_u32() should be changed. Otherwise you will get
> > /soc@0/bus@32c00000/dsi@32e10000: failed to get 'samsung,pll-clock-
> frequency' property

I'll change the message from err to info with a message that reads "no
samsung,pll-clock-frequency, using pixel clock"

Does that work?

adam
>
> Best regards,
> Alexander
>
> >       ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-
> frequency",
> >                                      &dsi->burst_clk_rate);
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>

Re: [PATCH V3 3/7] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Alexander Stein]

Hi Adam,

Am Donnerstag, 4. Mai 2023, 14:00:08 CEST schrieb Adam Ford:
> On Thu, May 4, 2023 at 4:21 AM Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Am Dienstag, 2. Mai 2023, 03:07:55 CEST schrieb Adam Ford:
> > > Make the pll-clock-frequency optional.  If it's present, use it
> > > to maintain backwards compatibility with existing hardware.  If it
> > > is absent, read clock rate of "sclk_mipi" to determine the rate.
> > > 
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > 
> > >  drivers/gpu/drm/bridge/samsung-dsim.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > b/drivers/gpu/drm/bridge/samsung-dsim.c index bf4b33d2de76..2dc02a9e37c0
> > > 100644
> > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > @@ -1726,12 +1726,20 @@ static int samsung_dsim_parse_dt(struct
> > > samsung_dsim *dsi) {
> > > 
> > >       struct device *dev = dsi->dev;
> > >       struct device_node *node = dev->of_node;
> > > 
> > > +     struct clk *pll_clk;
> > > 
> > >       int ret;
> > >       
> > >       ret = samsung_dsim_of_read_u32(node,
> > >       "samsung,pll-clock-frequency",
> > >       
> > >                                      &dsi->pll_clk_rate);
> > > 
> > > -     if (ret < 0)
> > > -             return ret;
> > > +
> > > +     /* If it doesn't exist, read it from the clock instead of failing
> > > */
> > > +     if (ret < 0) {
> > > +             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);
> > > +     }
> > 
> > Now that 'samsung,pll-clock-frequency' is optional the error in
> > samsung_dsim_of_read_u32() should be changed. Otherwise you will get
> > 
> > > /soc@0/bus@32c00000/dsi@32e10000: failed to get 'samsung,pll-clock-
> > 
> > frequency' property
> 
> I'll change the message from err to info with a message that reads "no
> samsung,pll-clock-frequency, using pixel clock"
> 
> Does that work?

Having just a info is totally fine with me. Thanks.
Although your suggested message somehow implies (to me) using pixel clock is 
just a fallback. I'm a bit concerned some might think "samsung,pll-clock-
frequency" should be provided in DT. But this might just be me.

Best regards,
Alexander

> adam
> 
> > Best regards,
> > Alexander
> > 
> > >       ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-
> > 
> > frequency",
> > 
> > >                                      &dsi->burst_clk_rate);
> > 
> > --
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > http://www.tq-group.com/


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH V3 3/7] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Adam Ford]

On Thu, May 4, 2023 at 7:40 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi Adam,
>
> Am Donnerstag, 4. Mai 2023, 14:00:08 CEST schrieb Adam Ford:
> > On Thu, May 4, 2023 at 4:21 AM Alexander Stein
> >
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Am Dienstag, 2. Mai 2023, 03:07:55 CEST schrieb Adam Ford:
> > > > Make the pll-clock-frequency optional.  If it's present, use it
> > > > to maintain backwards compatibility with existing hardware.  If it
> > > > is absent, read clock rate of "sclk_mipi" to determine the rate.
> > > >
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > > > ---
> > > >
> > > >  drivers/gpu/drm/bridge/samsung-dsim.c | 12 ++++++++++--
> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > b/drivers/gpu/drm/bridge/samsung-dsim.c index bf4b33d2de76..2dc02a9e37c0
> > > > 100644
> > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > @@ -1726,12 +1726,20 @@ static int samsung_dsim_parse_dt(struct
> > > > samsung_dsim *dsi) {
> > > >
> > > >       struct device *dev = dsi->dev;
> > > >       struct device_node *node = dev->of_node;
> > > >
> > > > +     struct clk *pll_clk;
> > > >
> > > >       int ret;
> > > >
> > > >       ret = samsung_dsim_of_read_u32(node,
> > > >       "samsung,pll-clock-frequency",
> > > >
> > > >                                      &dsi->pll_clk_rate);
> > > >
> > > > -     if (ret < 0)
> > > > -             return ret;
> > > > +
> > > > +     /* If it doesn't exist, read it from the clock instead of failing
> > > > */
> > > > +     if (ret < 0) {
> > > > +             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);
> > > > +     }
> > >
> > > Now that 'samsung,pll-clock-frequency' is optional the error in
> > > samsung_dsim_of_read_u32() should be changed. Otherwise you will get
> > >
> > > > /soc@0/bus@32c00000/dsi@32e10000: failed to get 'samsung,pll-clock-
> > >
> > > frequency' property
> >
> > I'll change the message from err to info with a message that reads "no
> > samsung,pll-clock-frequency, using pixel clock"
> >
> > Does that work?
>
> Having just a info is totally fine with me. Thanks.
> Although your suggested message somehow implies (to me) using pixel clock is
> just a fallback. I'm a bit concerned some might think "samsung,pll-clock-
> frequency" should be provided in DT. But this might just be me.

Oops, I got the PLL and burst burst clock confused.  I think both
burst-clock and pll clock messages should get updates.

The pll clock should say something like "samsung,pll-clock-frequency
not defined, using sclk_mipi"

The imx8m n/m/p have the sclk_mipi defined in the device tree, and
this patch allows them to not have
to manually set the pll clock since it can be read.  This allows to
people to change the frequency of the PLL in
in one place and let the driver read it instead of having to set the
value in two places for the same clock.

For the burst clock, I'd like to propose
"samsung,burst-clock-frequency not defined, using pixel clock"

Does that work for you?

adam

> frequency

>
> Best regards,
> Alexander
>
> > adam
> >
> > > Best regards,
> > > Alexander
> > >
> > > >       ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-
> > >
> > > frequency",
> > >
> > > >                                      &dsi->burst_clk_rate);
> > >
> > > --
> > > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > > Amtsgericht München, HRB 105018
> > > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > > http://www.tq-group.com/
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>

Re: [PATCH V3 3/7] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Alexander Stein]

Am Donnerstag, 4. Mai 2023, 14:57:01 CEST schrieb Adam Ford:
> On Thu, May 4, 2023 at 7:40 AM Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hi Adam,
> > 
> > Am Donnerstag, 4. Mai 2023, 14:00:08 CEST schrieb Adam Ford:
> > > On Thu, May 4, 2023 at 4:21 AM Alexander Stein
> > > 
> > > <alexander.stein@ew.tq-group.com> wrote:
> > > > Am Dienstag, 2. Mai 2023, 03:07:55 CEST schrieb Adam Ford:
> > > > > Make the pll-clock-frequency optional.  If it's present, use it
> > > > > to maintain backwards compatibility with existing hardware.  If it
> > > > > is absent, read clock rate of "sclk_mipi" to determine the rate.
> > > > > 
> > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > > > > ---
> > > > > 
> > > > >  drivers/gpu/drm/bridge/samsung-dsim.c | 12 ++++++++++--
> > > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > b/drivers/gpu/drm/bridge/samsung-dsim.c index
> > > > > bf4b33d2de76..2dc02a9e37c0
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > @@ -1726,12 +1726,20 @@ static int samsung_dsim_parse_dt(struct
> > > > > samsung_dsim *dsi) {
> > > > > 
> > > > >       struct device *dev = dsi->dev;
> > > > >       struct device_node *node = dev->of_node;
> > > > > 
> > > > > +     struct clk *pll_clk;
> > > > > 
> > > > >       int ret;
> > > > >       
> > > > >       ret = samsung_dsim_of_read_u32(node,
> > > > >       "samsung,pll-clock-frequency",
> > > > >       
> > > > >                                      &dsi->pll_clk_rate);
> > > > > 
> > > > > -     if (ret < 0)
> > > > > -             return ret;
> > > > > +
> > > > > +     /* If it doesn't exist, read it from the clock instead of
> > > > > failing
> > > > > */
> > > > > +     if (ret < 0) {
> > > > > +             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);
> > > > > +     }
> > > > 
> > > > Now that 'samsung,pll-clock-frequency' is optional the error in
> > > > samsung_dsim_of_read_u32() should be changed. Otherwise you will get
> > > > 
> > > > > /soc@0/bus@32c00000/dsi@32e10000: failed to get 'samsung,pll-clock-
> > > > 
> > > > frequency' property
> > > 
> > > I'll change the message from err to info with a message that reads "no
> > > samsung,pll-clock-frequency, using pixel clock"
> > > 
> > > Does that work?
> > 
> > Having just a info is totally fine with me. Thanks.
> > Although your suggested message somehow implies (to me) using pixel clock
> > is just a fallback. I'm a bit concerned some might think
> > "samsung,pll-clock- frequency" should be provided in DT. But this might
> > just be me.
> 
> Oops, I got the PLL and burst burst clock confused.  I think both
> burst-clock and pll clock messages should get updates.
> 
> The pll clock should say something like "samsung,pll-clock-frequency
> not defined, using sclk_mipi"
> 
> The imx8m n/m/p have the sclk_mipi defined in the device tree, and
> this patch allows them to not have
> to manually set the pll clock since it can be read.  This allows to
> people to change the frequency of the PLL in
> in one place and let the driver read it instead of having to set the
> value in two places for the same clock.

That's why I would like to make it sound less error-like.
How about "Using sclk_mipi for pll clock frequency"?

> For the burst clock, I'd like to propose
> "samsung,burst-clock-frequency not defined, using pixel clock"

Similar to above how about "Using pixel clock for burst clock frequency"?

> Does that work for you?

But I'm okay with both ways. Up to you.

Thanks and best regards,
Alexander


> > frequency
> > 
> > 
> > Best regards,
> > Alexander
> > 
> > > adam
> > > 
> > > > Best regards,
> > > > Alexander
> > > > 
> > > > >       ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-
> > > > 
> > > > frequency",
> > > > 
> > > > >                                      &dsi->burst_clk_rate);
> > > > 
> > > > --
> > > > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > > > Amtsgericht München, HRB 105018
> > > > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > > > http://www.tq-group.com/
> > 
> > --
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > http://www.tq-group.com/


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH V3 3/7] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Adam Ford]

On Thu, May 4, 2023 at 8:18 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Am Donnerstag, 4. Mai 2023, 14:57:01 CEST schrieb Adam Ford:
> > On Thu, May 4, 2023 at 7:40 AM Alexander Stein
> >
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Hi Adam,
> > >
> > > Am Donnerstag, 4. Mai 2023, 14:00:08 CEST schrieb Adam Ford:
> > > > On Thu, May 4, 2023 at 4:21 AM Alexander Stein
> > > >
> > > > <alexander.stein@ew.tq-group.com> wrote:
> > > > > Am Dienstag, 2. Mai 2023, 03:07:55 CEST schrieb Adam Ford:
> > > > > > Make the pll-clock-frequency optional.  If it's present, use it
> > > > > > to maintain backwards compatibility with existing hardware.  If it
> > > > > > is absent, read clock rate of "sclk_mipi" to determine the rate.
> > > > > >
> > > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > > > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > > > > > ---
> > > > > >
> > > > > >  drivers/gpu/drm/bridge/samsung-dsim.c | 12 ++++++++++--
> > > > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > > b/drivers/gpu/drm/bridge/samsung-dsim.c index
> > > > > > bf4b33d2de76..2dc02a9e37c0
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > > @@ -1726,12 +1726,20 @@ static int samsung_dsim_parse_dt(struct
> > > > > > samsung_dsim *dsi) {
> > > > > >
> > > > > >       struct device *dev = dsi->dev;
> > > > > >       struct device_node *node = dev->of_node;
> > > > > >
> > > > > > +     struct clk *pll_clk;
> > > > > >
> > > > > >       int ret;
> > > > > >
> > > > > >       ret = samsung_dsim_of_read_u32(node,
> > > > > >       "samsung,pll-clock-frequency",
> > > > > >
> > > > > >                                      &dsi->pll_clk_rate);
> > > > > >
> > > > > > -     if (ret < 0)
> > > > > > -             return ret;
> > > > > > +
> > > > > > +     /* If it doesn't exist, read it from the clock instead of
> > > > > > failing
> > > > > > */
> > > > > > +     if (ret < 0) {
> > > > > > +             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);
> > > > > > +     }
> > > > >
> > > > > Now that 'samsung,pll-clock-frequency' is optional the error in
> > > > > samsung_dsim_of_read_u32() should be changed. Otherwise you will get
> > > > >
> > > > > > /soc@0/bus@32c00000/dsi@32e10000: failed to get 'samsung,pll-clock-
> > > > >
> > > > > frequency' property
> > > >
> > > > I'll change the message from err to info with a message that reads "no
> > > > samsung,pll-clock-frequency, using pixel clock"
> > > >
> > > > Does that work?
> > >
> > > Having just a info is totally fine with me. Thanks.
> > > Although your suggested message somehow implies (to me) using pixel clock
> > > is just a fallback. I'm a bit concerned some might think
> > > "samsung,pll-clock- frequency" should be provided in DT. But this might
> > > just be me.
> >
> > Oops, I got the PLL and burst burst clock confused.  I think both
> > burst-clock and pll clock messages should get updates.
> >
> > The pll clock should say something like "samsung,pll-clock-frequency
> > not defined, using sclk_mipi"
> >
> > The imx8m n/m/p have the sclk_mipi defined in the device tree, and
> > this patch allows them to not have
> > to manually set the pll clock since it can be read.  This allows to
> > people to change the frequency of the PLL in
> > in one place and let the driver read it instead of having to set the
> > value in two places for the same clock.
>
> That's why I would like to make it sound less error-like.
> How about "Using sclk_mipi for pll clock frequency"?
>
> > For the burst clock, I'd like to propose
> > "samsung,burst-clock-frequency not defined, using pixel clock"
>
> Similar to above how about "Using pixel clock for burst clock frequency"?

I like that.

>
> > Does that work for you?
>
> But I'm okay with both ways. Up to you.

 I'll wait another day or two to see if anyone else has any feedback,
and I'll submit V4 with some other items addressed too.

Thank you for your review!

adam

>
> Thanks and best regards,
> Alexander
>
>
> > > frequency
> > >
> > >
> > > Best regards,
> > > Alexander
> > >
> > > > adam
> > > >
> > > > > Best regards,
> > > > > Alexander
> > > > >
> > > > > >       ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-
> > > > >
> > > > > frequency",
> > > > >
> > > > > >                                      &dsi->burst_clk_rate);
> > > > >
> > > > > --
> > > > > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > > > > Amtsgericht München, HRB 105018
> > > > > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > > > > http://www.tq-group.com/
> > >
> > > --
> > > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > > Amtsgericht München, HRB 105018
> > > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > > http://www.tq-group.com/
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>

[PATCH V3 4/7] drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY

[Adam Ford]

In order to support variable DPHY timings, it's necessary
to enable GENERIC_PHY_MIPI_DPHY so phy_mipi_dphy_get_default_config
can be used to determine the nominal values for a given resolution
and refresh rate.

Signed-off-by: Adam Ford <aford173@gmail.com>
---
 drivers/gpu/drm/bridge/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index f076a09afac0..82c68b042444 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -227,6 +227,7 @@ config DRM_SAMSUNG_DSIM
 	select DRM_KMS_HELPER
 	select DRM_MIPI_DSI
 	select DRM_PANEL_BRIDGE
+	select GENERIC_PHY_MIPI_DPHY
 	help
 	  The Samsung MIPI DSIM bridge controller driver.
 	  This MIPI DSIM bridge can be found it on Exynos SoCs and
-- 
2.39.2

Re: [PATCH V3 4/7] drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY

[Frieder Schrempf]

On 02.05.23 03:07, Adam Ford wrote:
> In order to support variable DPHY timings, it's necessary
> to enable GENERIC_PHY_MIPI_DPHY so phy_mipi_dphy_get_default_config
> can be used to determine the nominal values for a given resolution
> and refresh rate.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>

This fixes the build error which existed in v2!

Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges.

Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

[PATCH V3 5/7] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Adam Ford]

The DPHY timings are currently hard coded. Since the input
clock can be variable, the phy timings need to be variable
too.  Add an additional variable to the driver data to enable
this feature to prevent breaking boards that don't support it.

The phy_mipi_dphy_get_default_config function configures the
DPHY timings in pico-seconds, and a small macro converts those
timings into clock cycles based on the pixel clock rate.

Signed-off-by: Adam Ford <aford173@gmail.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 79 +++++++++++++++++++++++----
 include/drm/bridge/samsung-dsim.h     |  1 +
 2 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 2dc02a9e37c0..99642230a54a 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -18,9 +18,7 @@
 #include <linux/media-bus-format.h>
 #include <linux/of_device.h>
 #include <linux/phy/phy.h>
-
 #include <video/mipi_display.h>
-
 #include <drm/bridge/samsung-dsim.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
@@ -218,6 +216,8 @@
 
 #define OLD_SCLK_MIPI_CLK_NAME		"pll_clk"
 
+#define PS_TO_CYCLE(PS, MHz) DIV64_U64_ROUND_CLOSEST(((PS) * (MHz)), 1000000000000ULL)
+
 static const char *const clk_names[5] = {
 	"bus_clk",
 	"sclk_mipi",
@@ -487,6 +487,7 @@ static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
 	.m_min = 64,
 	.m_max = 1023,
 	.min_freq = 1050,
+	.dynamic_dphy = 1,
 };
 
 static const struct samsung_dsim_driver_data *
@@ -698,13 +699,50 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 	const unsigned int *reg_values = driver_data->reg_values;
 	u32 reg;
+	struct drm_display_mode *m = &dsi->mode;
+	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+	struct phy_configure_opts_mipi_dphy cfg;
+	int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
+	int hs_exit, hs_prepare, hs_zero, hs_trail;
+	unsigned long long clock_in_hz = m->clock * 1000;
 
 	if (driver_data->has_freqband)
 		return;
 
+	/* The dynamic_phy has the ability to adjust PHY Timing settings */
+	if (driver_data->dynamic_dphy) {
+		phy_mipi_dphy_get_default_config(clock_in_hz, bpp, dsi->lanes, &cfg);
+
+		/*
+		 * TODO:
+		 * The tech reference manual for i.MX8M Mini/Nano/Plus
+		 * doesn't state what the definition of the PHYTIMING
+		 * bits are beyond their address and bit position.
+		 * After reviewing NXP's downstream code, it appears
+		 * that the various PHYTIMING registers take the number
+		 * of cycles and use various dividers on them.  This
+		 * calculation does not result in an exact match to the
+		 * downstream code, but it is very close, and it appears
+		 * to sync at a variety of resolutions. If someone
+		 * can get a more accurate mathematical equation needed
+		 * for these registers, this should be updated.
+		 */
+
+		lpx = PS_TO_CYCLE(cfg.lpx, clock_in_hz);
+		hs_exit = PS_TO_CYCLE(cfg.hs_exit, clock_in_hz);
+		clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, clock_in_hz);
+		clk_zero = PS_TO_CYCLE(cfg.clk_zero, clock_in_hz);
+		clk_post = PS_TO_CYCLE(cfg.clk_post, clock_in_hz);
+		clk_trail = PS_TO_CYCLE(cfg.clk_trail, clock_in_hz);
+		hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, clock_in_hz);
+		hs_zero = PS_TO_CYCLE(cfg.hs_zero, clock_in_hz);
+		hs_trail = PS_TO_CYCLE(cfg.hs_trail, clock_in_hz);
+	}
+
 	/* B D-PHY: D-PHY Master & Slave Analog Block control */
 	reg = reg_values[PHYCTRL_ULPS_EXIT] | reg_values[PHYCTRL_VREG_LP] |
 		reg_values[PHYCTRL_SLEW_UP];
+
 	samsung_dsim_write(dsi, DSIM_PHYCTRL_REG, reg);
 
 	/*
@@ -712,7 +750,11 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	 * T HS-EXIT: Time that the transmitter drives LP-11 following a HS
 	 *	burst
 	 */
-	reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
+	if (driver_data->dynamic_dphy)
+		reg  = DSIM_PHYTIMING_LPX(lpx) | DSIM_PHYTIMING_HS_EXIT(hs_exit);
+	else
+		reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
+
 	samsung_dsim_write(dsi, DSIM_PHYTIMING_REG, reg);
 
 	/*
@@ -728,10 +770,17 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	 * T CLK-TRAIL: Time that the transmitter drives the HS-0 state after
 	 *	the last payload clock bit of a HS transmission burst
 	 */
-	reg = reg_values[PHYTIMING_CLK_PREPARE] |
-		reg_values[PHYTIMING_CLK_ZERO] |
-		reg_values[PHYTIMING_CLK_POST] |
-		reg_values[PHYTIMING_CLK_TRAIL];
+	if (driver_data->dynamic_dphy) {
+		reg = DSIM_PHYTIMING1_CLK_PREPARE(clk_prepare)	|
+		      DSIM_PHYTIMING1_CLK_ZERO(clk_zero)	|
+		      DSIM_PHYTIMING1_CLK_POST(clk_post)	|
+		      DSIM_PHYTIMING1_CLK_TRAIL(clk_trail);
+	} else {
+		reg = reg_values[PHYTIMING_CLK_PREPARE] |
+		      reg_values[PHYTIMING_CLK_ZERO] |
+		      reg_values[PHYTIMING_CLK_POST] |
+		      reg_values[PHYTIMING_CLK_TRAIL];
+	}
 
 	samsung_dsim_write(dsi, DSIM_PHYTIMING1_REG, reg);
 
@@ -744,8 +793,17 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	 * T HS-TRAIL: Time that the transmitter drives the flipped differential
 	 *	state after last payload data bit of a HS transmission burst
 	 */
-	reg = reg_values[PHYTIMING_HS_PREPARE] | reg_values[PHYTIMING_HS_ZERO] |
-		reg_values[PHYTIMING_HS_TRAIL];
+
+	if (driver_data->dynamic_dphy) {
+		reg = DSIM_PHYTIMING2_HS_PREPARE(hs_prepare) |
+		      DSIM_PHYTIMING2_HS_ZERO(hs_zero) |
+		      DSIM_PHYTIMING2_HS_TRAIL(hs_trail);
+	} else {
+		reg = reg_values[PHYTIMING_HS_PREPARE] |
+		      reg_values[PHYTIMING_HS_ZERO] |
+		      reg_values[PHYTIMING_HS_TRAIL];
+	}
+
 	samsung_dsim_write(dsi, DSIM_PHYTIMING2_REG, reg);
 }
 
@@ -1337,7 +1395,8 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
 	samsung_dsim_enable_clock(dsi);
 	if (driver_data->wait_for_reset)
 		samsung_dsim_wait_for_reset(dsi);
-	samsung_dsim_set_phy_ctrl(dsi);
+	if (!driver_data->has_freqband)
+		samsung_dsim_set_phy_ctrl(dsi);
 	samsung_dsim_init_link(dsi);
 
 	dsi->state |= DSIM_STATE_INITIALIZED;
diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
index a1a5b2b89a7a..76ea8a1720cc 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -62,6 +62,7 @@ struct samsung_dsim_driver_data {
 	const unsigned int *reg_values;
 	u16 m_min;
 	u16 m_max;
+	bool dynamic_dphy;
 };
 
 struct samsung_dsim_host_ops {
-- 
2.39.2

Re: [PATCH V3 5/7] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Frieder Schrempf]

On 02.05.23 03:07, Adam Ford wrote:
> The DPHY timings are currently hard coded. Since the input
> clock can be variable, the phy timings need to be variable
> too.  Add an additional variable to the driver data to enable
> this feature to prevent breaking boards that don't support it.
> 
> The phy_mipi_dphy_get_default_config function configures the
> DPHY timings in pico-seconds, and a small macro converts those
> timings into clock cycles based on the pixel clock rate.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>

A few nitpicks below, otherwise:

Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges.

Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 79 +++++++++++++++++++++++----
>  include/drm/bridge/samsung-dsim.h     |  1 +
>  2 files changed, 70 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 2dc02a9e37c0..99642230a54a 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -18,9 +18,7 @@
>  #include <linux/media-bus-format.h>
>  #include <linux/of_device.h>
>  #include <linux/phy/phy.h>
> -
>  #include <video/mipi_display.h>
> -

Unrelated blank lines removed above!?

>  #include <drm/bridge/samsung-dsim.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_print.h>
> @@ -218,6 +216,8 @@
>  
>  #define OLD_SCLK_MIPI_CLK_NAME		"pll_clk"
>  
> +#define PS_TO_CYCLE(PS, MHz) DIV64_U64_ROUND_CLOSEST(((PS) * (MHz)), 1000000000000ULL)

Should macro arguments PS and MHz better be all lower-case?
Also, MHz is actually in Hz, right? So it should be renamed.

> +
>  static const char *const clk_names[5] = {
>  	"bus_clk",
>  	"sclk_mipi",
> @@ -487,6 +487,7 @@ static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
>  	.m_min = 64,
>  	.m_max = 1023,
>  	.min_freq = 1050,
> +	.dynamic_dphy = 1,
>  };
>  
>  static const struct samsung_dsim_driver_data *
> @@ -698,13 +699,50 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
>  	const unsigned int *reg_values = driver_data->reg_values;
>  	u32 reg;
> +	struct drm_display_mode *m = &dsi->mode;
> +	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +	struct phy_configure_opts_mipi_dphy cfg;
> +	int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
> +	int hs_exit, hs_prepare, hs_zero, hs_trail;
> +	unsigned long long clock_in_hz = m->clock * 1000;
>  
>  	if (driver_data->has_freqband)
>  		return;
>  
> +	/* The dynamic_phy has the ability to adjust PHY Timing settings */
> +	if (driver_data->dynamic_dphy) {
> +		phy_mipi_dphy_get_default_config(clock_in_hz, bpp, dsi->lanes, &cfg);
> +
> +		/*
> +		 * TODO:
> +		 * The tech reference manual for i.MX8M Mini/Nano/Plus
> +		 * doesn't state what the definition of the PHYTIMING
> +		 * bits are beyond their address and bit position.
> +		 * After reviewing NXP's downstream code, it appears
> +		 * that the various PHYTIMING registers take the number
> +		 * of cycles and use various dividers on them.  This
> +		 * calculation does not result in an exact match to the
> +		 * downstream code, but it is very close, and it appears
> +		 * to sync at a variety of resolutions. If someone
> +		 * can get a more accurate mathematical equation needed
> +		 * for these registers, this should be updated.
> +		 */
> +
> +		lpx = PS_TO_CYCLE(cfg.lpx, clock_in_hz);
> +		hs_exit = PS_TO_CYCLE(cfg.hs_exit, clock_in_hz);
> +		clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, clock_in_hz);
> +		clk_zero = PS_TO_CYCLE(cfg.clk_zero, clock_in_hz);
> +		clk_post = PS_TO_CYCLE(cfg.clk_post, clock_in_hz);
> +		clk_trail = PS_TO_CYCLE(cfg.clk_trail, clock_in_hz);
> +		hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, clock_in_hz);
> +		hs_zero = PS_TO_CYCLE(cfg.hs_zero, clock_in_hz);
> +		hs_trail = PS_TO_CYCLE(cfg.hs_trail, clock_in_hz);
> +	}
> +
>  	/* B D-PHY: D-PHY Master & Slave Analog Block control */
>  	reg = reg_values[PHYCTRL_ULPS_EXIT] | reg_values[PHYCTRL_VREG_LP] |
>  		reg_values[PHYCTRL_SLEW_UP];
> +
>  	samsung_dsim_write(dsi, DSIM_PHYCTRL_REG, reg);
>  
>  	/*
> @@ -712,7 +750,11 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	 * T HS-EXIT: Time that the transmitter drives LP-11 following a HS
>  	 *	burst
>  	 */
> -	reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
> +	if (driver_data->dynamic_dphy)
> +		reg  = DSIM_PHYTIMING_LPX(lpx) | DSIM_PHYTIMING_HS_EXIT(hs_exit);
> +	else
> +		reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
> +
>  	samsung_dsim_write(dsi, DSIM_PHYTIMING_REG, reg);
>  
>  	/*
> @@ -728,10 +770,17 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	 * T CLK-TRAIL: Time that the transmitter drives the HS-0 state after
>  	 *	the last payload clock bit of a HS transmission burst
>  	 */
> -	reg = reg_values[PHYTIMING_CLK_PREPARE] |
> -		reg_values[PHYTIMING_CLK_ZERO] |
> -		reg_values[PHYTIMING_CLK_POST] |
> -		reg_values[PHYTIMING_CLK_TRAIL];
> +	if (driver_data->dynamic_dphy) {
> +		reg = DSIM_PHYTIMING1_CLK_PREPARE(clk_prepare)	|
> +		      DSIM_PHYTIMING1_CLK_ZERO(clk_zero)	|
> +		      DSIM_PHYTIMING1_CLK_POST(clk_post)	|
> +		      DSIM_PHYTIMING1_CLK_TRAIL(clk_trail);
> +	} else {
> +		reg = reg_values[PHYTIMING_CLK_PREPARE] |
> +		      reg_values[PHYTIMING_CLK_ZERO] |
> +		      reg_values[PHYTIMING_CLK_POST] |
> +		      reg_values[PHYTIMING_CLK_TRAIL];
> +	}
>  
>  	samsung_dsim_write(dsi, DSIM_PHYTIMING1_REG, reg);
>  
> @@ -744,8 +793,17 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	 * T HS-TRAIL: Time that the transmitter drives the flipped differential
>  	 *	state after last payload data bit of a HS transmission burst
>  	 */
> -	reg = reg_values[PHYTIMING_HS_PREPARE] | reg_values[PHYTIMING_HS_ZERO] |
> -		reg_values[PHYTIMING_HS_TRAIL];
> +
> +	if (driver_data->dynamic_dphy) {
> +		reg = DSIM_PHYTIMING2_HS_PREPARE(hs_prepare) |
> +		      DSIM_PHYTIMING2_HS_ZERO(hs_zero) |
> +		      DSIM_PHYTIMING2_HS_TRAIL(hs_trail);
> +	} else {
> +		reg = reg_values[PHYTIMING_HS_PREPARE] |
> +		      reg_values[PHYTIMING_HS_ZERO] |
> +		      reg_values[PHYTIMING_HS_TRAIL];
> +	}
> +
>  	samsung_dsim_write(dsi, DSIM_PHYTIMING2_REG, reg);
>  }
>  
> @@ -1337,7 +1395,8 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
>  	samsung_dsim_enable_clock(dsi);
>  	if (driver_data->wait_for_reset)
>  		samsung_dsim_wait_for_reset(dsi);
> -	samsung_dsim_set_phy_ctrl(dsi);
> +	if (!driver_data->has_freqband)

samsung_dsim_set_phy_ctrl() already contains a check for
driver_data->has_freqband

> +		samsung_dsim_set_phy_ctrl(dsi);
>  	samsung_dsim_init_link(dsi);
>  
>  	dsi->state |= DSIM_STATE_INITIALIZED;
> diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> index a1a5b2b89a7a..76ea8a1720cc 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -62,6 +62,7 @@ struct samsung_dsim_driver_data {
>  	const unsigned int *reg_values;
>  	u16 m_min;
>  	u16 m_max;
> +	bool dynamic_dphy;
>  };
>  
>  struct samsung_dsim_host_ops {

[PATCH V3 5/7] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Michael Walle]

> The DPHY timings are currently hard coded. Since the input
> clock can be variable, the phy timings need to be variable
> too.  Add an additional variable to the driver data to enable
> this feature to prevent breaking boards that don't support it.
> 
> The phy_mipi_dphy_get_default_config function configures the
> DPHY timings in pico-seconds, and a small macro converts those
> timings into clock cycles based on the pixel clock rate.

This actually fixes a bug with the DSI84 bridge on our boards. The
hardcoded settings will violate the D-PHY spec timings for lower
frequencies, esp. the Ths_prepare+Ths_zero timing. Thus, the bridge
will read a wrong HS sync sequence and set it's internal SoT error
bit (and don't generate any RGB signals on the LVDS side).

Tested-by: Michael Walle <michael@walle.cc>

Thanks!
-michael

[PATCH V3 6/7] drm: bridge: samsung-dsim: Support non-burst mode

[Adam Ford]

The high-speed clock is hard-coded to the burst-clock
frequency specified in the device tree.  However, when
using devices like certain bridge chips without burst mode
and varying resolutions and refresh rates, it may be
necessary to set the high-speed clock dynamically based
on the desired pixel clock for the connected device.

This also removes the need to set a clock speed from
the device tree for non-burst mode operation, since the
pixel clock rate is the rate requested from the attached
device like an HDMI bridge chip.  This should have no
impact for people using burst-mode and setting the burst
clock rate is still required for those users.

Signed-off-by: Adam Ford <aford173@gmail.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 99642230a54a..53099461cdc2 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -657,11 +657,21 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
 
 static int samsung_dsim_enable_clock(struct samsung_dsim *dsi)
 {
-	unsigned long hs_clk, byte_clk, esc_clk;
+	unsigned long hs_clk, byte_clk, esc_clk, pix_clk;
 	unsigned long esc_div;
 	u32 reg;
+	struct drm_display_mode *m = &dsi->mode;
+	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+
+	/* m->clock is in KHz */
+	pix_clk = m->clock * 1000;
+
+	/* Use burst_clk_rate if available, otherwise use the pix_clk */
+	if (dsi->burst_clk_rate)
+		hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
+	else
+		hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes));
 
-	hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
 	if (!hs_clk) {
 		dev_err(dsi->dev, "failed to configure DSI PLL\n");
 		return -EFAULT;
@@ -1800,10 +1810,11 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi)
 			return PTR_ERR(pll_clk);
 	}
 
+	/* If it doesn't exist, use pixel clock instead of failing */
 	ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency",
 				       &dsi->burst_clk_rate);
 	if (ret < 0)
-		return ret;
+		dsi->burst_clk_rate = 0;
 
 	ret = samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequency",
 				       &dsi->esc_clk_rate);
-- 
2.39.2

Re: [PATCH V3 6/7] drm: bridge: samsung-dsim: Support non-burst mode

[Frieder Schrempf]

On 02.05.23 03:07, Adam Ford wrote:
> The high-speed clock is hard-coded to the burst-clock
> frequency specified in the device tree.  However, when
> using devices like certain bridge chips without burst mode
> and varying resolutions and refresh rates, it may be
> necessary to set the high-speed clock dynamically based
> on the desired pixel clock for the connected device.
> 
> This also removes the need to set a clock speed from
> the device tree for non-burst mode operation, since the
> pixel clock rate is the rate requested from the attached
> device like an HDMI bridge chip.  This should have no
> impact for people using burst-mode and setting the burst
> clock rate is still required for those users.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>

Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges.

Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

[PATCH V3 7/7] drm: bridge: samsung-dsim: Let blanking calcuation work in non-burst mode

[Adam Ford]

The blanking calculation currently uses burst_clk_rate for calculating
the settings. Since it's possible to use this in non-burst mode, it's
possible that where won't be burst_clk_rate.  Instead, cache the
clock rate configured from of samsung_dsim_set_pll and use it instead.

Signed-off-by: Adam Ford <aford173@gmail.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 4 +++-
 include/drm/bridge/samsung-dsim.h     | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 53099461cdc2..1dc913db2cb3 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -652,6 +652,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
 		reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
 	} while ((reg & DSIM_PLL_STABLE) == 0);
 
+	dsi->hs_clock = fout;
+
 	return fout;
 }
 
@@ -960,7 +962,7 @@ 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->burst_clk_rate / 1000 / 8;
+		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;
diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
index 76ea8a1720cc..14176e6e9040 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -94,6 +94,7 @@ struct samsung_dsim {
 
 	u32 pll_clk_rate;
 	u32 burst_clk_rate;
+	u32 hs_clock;
 	u32 esc_clk_rate;
 	u32 lanes;
 	u32 mode_flags;
-- 
2.39.2

Re: [PATCH V3 7/7] drm: bridge: samsung-dsim: Let blanking calcuation work in non-burst mode

[Frieder Schrempf]

On 02.05.23 03:07, Adam Ford wrote:
> The blanking calculation currently uses burst_clk_rate for calculating
> the settings. Since it's possible to use this in non-burst mode, it's
> possible that where won't be burst_clk_rate.  Instead, cache the

"possible that burst_clk_rate is 0"

> clock rate configured from of samsung_dsim_set_pll and use it instead.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com> Tested-by: Chen-Yu Tsai <wenst@chromium.org>

Maybe this patch should be squashed into patch 6/7 as otherwise
burst_clk_rate could be 0 here causing bisection issues?

Apart from that:

Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges.

Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 4 +++-
>  include/drm/bridge/samsung-dsim.h     | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 53099461cdc2..1dc913db2cb3 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -652,6 +652,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>  		reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
>  	} while ((reg & DSIM_PLL_STABLE) == 0);
>  
> +	dsi->hs_clock = fout;
> +
>  	return fout;
>  }
>  
> @@ -960,7 +962,7 @@ 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->burst_clk_rate / 1000 / 8;
> +		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;
> diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> index 76ea8a1720cc..14176e6e9040 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -94,6 +94,7 @@ struct samsung_dsim {
>  
>  	u32 pll_clk_rate;
>  	u32 burst_clk_rate;
> +	u32 hs_clock;
>  	u32 esc_clk_rate;
>  	u32 lanes;
>  	u32 mode_flags;

Re: [PATCH V3 7/7] drm: bridge: samsung-dsim: Let blanking calcuation work in non-burst mode

[Adam Ford]

On Wed, May 3, 2023 at 10:52 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> On 02.05.23 03:07, Adam Ford wrote:
> > The blanking calculation currently uses burst_clk_rate for calculating
> > the settings. Since it's possible to use this in non-burst mode, it's
> > possible that where won't be burst_clk_rate.  Instead, cache the
>
> "possible that burst_clk_rate is 0"
>
> > clock rate configured from of samsung_dsim_set_pll and use it instead.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
>
> Maybe this patch should be squashed into patch 6/7 as otherwise
> burst_clk_rate could be 0 here causing bisection issues?

I thought about squashing them and I went back and forth on that.
Since there are some other minor edits in this series, I can push a V4
with these squashed.

>
> Apart from that:
>
> Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges.
>

Thank you for testing this series.

> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>

adam
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 4 +++-
> >  include/drm/bridge/samsung-dsim.h     | 1 +
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 53099461cdc2..1dc913db2cb3 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -652,6 +652,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> >               reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
> >       } while ((reg & DSIM_PLL_STABLE) == 0);
> >
> > +     dsi->hs_clock = fout;
> > +
> >       return fout;
> >  }
> >
> > @@ -960,7 +962,7 @@ 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->burst_clk_rate / 1000 / 8;
> > +             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;
> > diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> > index 76ea8a1720cc..14176e6e9040 100644
> > --- a/include/drm/bridge/samsung-dsim.h
> > +++ b/include/drm/bridge/samsung-dsim.h
> > @@ -94,6 +94,7 @@ struct samsung_dsim {
> >
> >       u32 pll_clk_rate;
> >       u32 burst_clk_rate;
> > +     u32 hs_clock;
> >       u32 esc_clk_rate;
> >       u32 lanes;
> >       u32 mode_flags;

Re: [PATCH V3 0/7] drm: bridge: samsung-dsim: Support variable clocking

[Chen-Yu Tsai]

On Tue, May 2, 2023 at 9:08 AM Adam Ford <aford173@gmail.com> wrote:
>
> This series fixes the blanking pack size and the PMS calculation.  It then
> adds support to allows the DSIM to dynamically DPHY clocks, and support
> non-burst mode while allowing the removal of the hard-coded clock values
> for the PLL for imx8m mini/nano/plus, and it allows the removal of the
> burst-clock device tree entry when burst-mode isn't supported by connected
> devices like an HDMI brige.  In that event, the HS clock is set to the value
> requested by the bridge chip.
>
> This has been tested on both an i.MX8M Nano and i.MX8M Plus, and should work
> on i.MX8M Mini as well.
>
> Adam Ford (6):
>   drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]
>   drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically
>   drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY
>   drm: bridge: samsung-dsim: Dynamically configure DPHY timing
>   drm: bridge: samsung-dsim: Support non-burst mode
>   drm: bridge: samsung-dsim: Let blanking calcuation work in non-burst
>     mode
>
> Lucas Stach (1):
>   drm: bridge: samsung-dsim: fix blanking packet size calculation

Still works, so whole series is

Tested-by: Chen-Yu Tsai <wenst@chromium.org>

on i.MX8M Mini based Hummingboard Pulse.

Re: [PATCH V3 0/7] drm: bridge: samsung-dsim: Support variable clocking

[Marek Szyprowski]

On 02.05.2023 03:07, Adam Ford wrote:
> This series fixes the blanking pack size and the PMS calculation.  It then
> adds support to allows the DSIM to dynamically DPHY clocks, and support
> non-burst mode while allowing the removal of the hard-coded clock values
> for the PLL for imx8m mini/nano/plus, and it allows the removal of the
> burst-clock device tree entry when burst-mode isn't supported by connected
> devices like an HDMI brige.  In that event, the HS clock is set to the value
> requested by the bridge chip.
>
> This has been tested on both an i.MX8M Nano and i.MX8M Plus, and should work
> on i.MX8M Mini as well.
>
> Adam Ford (6):
>    drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]
>    drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically
>    drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY
>    drm: bridge: samsung-dsim: Dynamically configure DPHY timing
>    drm: bridge: samsung-dsim: Support non-burst mode
>    drm: bridge: samsung-dsim: Let blanking calcuation work in non-burst
>      mode
>
> Lucas Stach (1):
>    drm: bridge: samsung-dsim: fix blanking packet size calculation
>
>   drivers/gpu/drm/bridge/Kconfig        |   1 +
>   drivers/gpu/drm/bridge/samsung-dsim.c | 150 ++++++++++++++++++++++----
>   include/drm/bridge/samsung-dsim.h     |   5 +
>   3 files changed, 136 insertions(+), 20 deletions(-)

Works fine (= doesn't break) on Exynos.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


>
> ---
> V3:  When checking if the bust-clock is present, only check for it
>       in the device tree, and don't check the presence of the
>       MIPI_DSI_MODE_VIDEO_BURST flag as it breaks an existing Exynos
>       board.
>       
>       Add a new patch to the series to select GENERIC_PHY_MIPI_DPHY in
>       Kconfig otherwise the build breaks on the 32-bit Exynos.
>       
>       Change vco_min variable name to min_freq
>
>       Added tested-by from Chen-Yu Tsai
>
> V2:  Instead of using my packet blanking calculation, this integrates
>       on from Lucas Stach which gets modified later in the series to
>       cache the value of the HS-clock instead of having to do the
>       calucations again.
>
>       Instead of completely eliminating the PLL clock frequency from
>       the device tree, this makes it optional to avoid breaking some
>       Samsung devices.  When the samsung,pll-clock-frequency is not
>       found, it reads the value of the clock named "sclk_mipi"
>       This also maintains backwords compatibility with older device
>       trees.
>
>       This also changes the DPHY calcuation from a Look-up table,
>       a reverse engineered algorithm which uses
>       phy_mipi_dphy_get_default_config to determine the standard
>       nominal values and calculates the cycles necessary to update
>       the DPHY timings accordingly.
>       

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH V3 0/7] drm: bridge: samsung-dsim: Support variable clocking

[Adam Ford]

On Tue, May 2, 2023 at 3:35 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 02.05.2023 03:07, Adam Ford wrote:
> > This series fixes the blanking pack size and the PMS calculation.  It then
> > adds support to allows the DSIM to dynamically DPHY clocks, and support
> > non-burst mode while allowing the removal of the hard-coded clock values
> > for the PLL for imx8m mini/nano/plus, and it allows the removal of the
> > burst-clock device tree entry when burst-mode isn't supported by connected
> > devices like an HDMI brige.  In that event, the HS clock is set to the value
> > requested by the bridge chip.
> >
> > This has been tested on both an i.MX8M Nano and i.MX8M Plus, and should work
> > on i.MX8M Mini as well.
> >
> > Adam Ford (6):
> >    drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]
> >    drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically
> >    drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY
> >    drm: bridge: samsung-dsim: Dynamically configure DPHY timing
> >    drm: bridge: samsung-dsim: Support non-burst mode
> >    drm: bridge: samsung-dsim: Let blanking calcuation work in non-burst
> >      mode
> >
> > Lucas Stach (1):
> >    drm: bridge: samsung-dsim: fix blanking packet size calculation
> >
> >   drivers/gpu/drm/bridge/Kconfig        |   1 +
> >   drivers/gpu/drm/bridge/samsung-dsim.c | 150 ++++++++++++++++++++++----
> >   include/drm/bridge/samsung-dsim.h     |   5 +
> >   3 files changed, 136 insertions(+), 20 deletions(-)
>
> Works fine (= doesn't break) on Exynos.
>
Thank is great news.  Thank you for testing!

> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

adam
>
>
> >
> > ---
> > V3:  When checking if the bust-clock is present, only check for it
> >       in the device tree, and don't check the presence of the
> >       MIPI_DSI_MODE_VIDEO_BURST flag as it breaks an existing Exynos
> >       board.
> >
> >       Add a new patch to the series to select GENERIC_PHY_MIPI_DPHY in
> >       Kconfig otherwise the build breaks on the 32-bit Exynos.
> >
> >       Change vco_min variable name to min_freq
> >
> >       Added tested-by from Chen-Yu Tsai
> >
> > V2:  Instead of using my packet blanking calculation, this integrates
> >       on from Lucas Stach which gets modified later in the series to
> >       cache the value of the HS-clock instead of having to do the
> >       calucations again.
> >
> >       Instead of completely eliminating the PLL clock frequency from
> >       the device tree, this makes it optional to avoid breaking some
> >       Samsung devices.  When the samsung,pll-clock-frequency is not
> >       found, it reads the value of the clock named "sclk_mipi"
> >       This also maintains backwords compatibility with older device
> >       trees.
> >
> >       This also changes the DPHY calcuation from a Look-up table,
> >       a reverse engineered algorithm which uses
> >       phy_mipi_dphy_get_default_config to determine the standard
> >       nominal values and calculates the cycles necessary to update
> >       the DPHY timings accordingly.
> >
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH V3 0/7] drm: bridge: samsung-dsim: Support variable clocking

[Alexander Stein]

Hi Adam,

Am Dienstag, 2. Mai 2023, 03:07:52 CEST schrieb Adam Ford:
> This series fixes the blanking pack size and the PMS calculation.  It then
> adds support to allows the DSIM to dynamically DPHY clocks, and support
> non-burst mode while allowing the removal of the hard-coded clock values
> for the PLL for imx8m mini/nano/plus, and it allows the removal of the
> burst-clock device tree entry when burst-mode isn't supported by connected
> devices like an HDMI brige.  In that event, the HS clock is set to the value
> requested by the bridge chip.
> 
> This has been tested on both an i.MX8M Nano and i.MX8M Plus, and should work
> on i.MX8M Mini as well.
> 
> Adam Ford (6):
>   drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]
>   drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically
>   drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY
>   drm: bridge: samsung-dsim: Dynamically configure DPHY timing
>   drm: bridge: samsung-dsim: Support non-burst mode
>   drm: bridge: samsung-dsim: Let blanking calcuation work in non-burst
>     mode
> 
> Lucas Stach (1):
>   drm: bridge: samsung-dsim: fix blanking packet size calculation
> 
>  drivers/gpu/drm/bridge/Kconfig        |   1 +
>  drivers/gpu/drm/bridge/samsung-dsim.c | 150 ++++++++++++++++++++++----
>  include/drm/bridge/samsung-dsim.h     |   5 +
>  3 files changed, 136 insertions(+), 20 deletions(-)

Whole series tested on TQMa8MxML/MBa8Mx using a SN65DSI84 + Tianma TM070JVHG33 
display.

Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
> V3:  When checking if the bust-clock is present, only check for it
>      in the device tree, and don't check the presence of the
>      MIPI_DSI_MODE_VIDEO_BURST flag as it breaks an existing Exynos
>      board.
> 
>      Add a new patch to the series to select GENERIC_PHY_MIPI_DPHY in
>      Kconfig otherwise the build breaks on the 32-bit Exynos.
> 
>      Change vco_min variable name to min_freq
> 
>      Added tested-by from Chen-Yu Tsai
> 
> V2:  Instead of using my packet blanking calculation, this integrates
>      on from Lucas Stach which gets modified later in the series to
>      cache the value of the HS-clock instead of having to do the
>      calucations again.
> 
>      Instead of completely eliminating the PLL clock frequency from
>      the device tree, this makes it optional to avoid breaking some
>      Samsung devices.  When the samsung,pll-clock-frequency is not
>      found, it reads the value of the clock named "sclk_mipi"
>      This also maintains backwords compatibility with older device
>      trees.
> 
>      This also changes the DPHY calcuation from a Look-up table,
>      a reverse engineered algorithm which uses
>      phy_mipi_dphy_get_default_config to determine the standard
>      nominal values and calculates the cycles necessary to update
>      the DPHY timings accordingly.


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/