[PATCH V2 0/6] drm: bridge: samsung-dsim: Support variable clocking

[PATCH V2 0/6] 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 (5):
  drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]
  drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically
  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/samsung-dsim.c | 150 ++++++++++++++++++++++----
 include/drm/bridge/samsung-dsim.h     |   5 +
 2 files changed, 135 insertions(+), 20 deletions(-)

---
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 V2 1/6] 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>
---
 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 V2 1/6] drm: bridge: samsung-dsim: fix blanking packet size calculation

[Jagan Teki]

On Sun, Apr 23, 2023 at 5:42 PM Adam Ford <aford173@gmail.com> 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>
> ---
>  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;

I do not quite understand why it depends on burst_clk_rate, would you
please explain? does it depends on bpp something like this

mipi_dsi_pixel_format_to_bpp(format) / 8

> +               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);

6 blanking packet overhead here means, 4 bytes + payload + 2 bytes
format? does this packet overhead depends on the respective porch's
like hpf, hbp and hsa has different packet overheads?

Jagan.

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

[Adam Ford]

On Mon, Apr 24, 2023 at 4:03 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Sun, Apr 23, 2023 at 5:42 PM Adam Ford <aford173@gmail.com> 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>
> > ---
> >  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;
>
> I do not quite understand why it depends on burst_clk_rate, would you
> please explain? does it depends on bpp something like this
>
> mipi_dsi_pixel_format_to_bpp(format) / 8

The pixel clock is currently set to the burst clock rate.  Dividing
the clock by 1000 gets the pixel clock in KHz, and dividing by 8
converts bits to bytes.
Later in the series, I change the clock from the burst clock to the
cached value returned from samsung_dsim_set_pll.

>
> > +               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);
>
> 6 blanking packet overhead here means, 4 bytes + payload + 2 bytes
> format? does this packet overhead depends on the respective porch's
> like hpf, hbp and hsa has different packet overheads?

Lucas might be able to explain this better.  However, it does match
the values of the downstream NXP kernel, and I tried playing with
these values manually, and 6 appeared to be the only number that
seemed to work for me too.  I abandoned my approach for Lucas'
implementation, because it seemed more clear than mine.
Maybe Lucas can chime in, since this is really his patch.

adam
>
> Jagan.

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

[Jagan Teki]

On Mon, Apr 24, 2023 at 3:17 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Apr 24, 2023 at 4:03 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Sun, Apr 23, 2023 at 5:42 PM Adam Ford <aford173@gmail.com> 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>
> > > ---
> > >  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;
> >
> > I do not quite understand why it depends on burst_clk_rate, would you
> > please explain? does it depends on bpp something like this
> >
> > mipi_dsi_pixel_format_to_bpp(format) / 8
>
> The pixel clock is currently set to the burst clock rate.  Dividing
> the clock by 1000 gets the pixel clock in KHz, and dividing by 8
> converts bits to bytes.
> Later in the series, I change the clock from the burst clock to the
> cached value returned from samsung_dsim_set_pll.

Okay.

>
> >
> > > +               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);
> >
> > 6 blanking packet overhead here means, 4 bytes + payload + 2 bytes
> > format? does this packet overhead depends on the respective porch's
> > like hpf, hbp and hsa has different packet overheads?
>
> Lucas might be able to explain this better.  However, it does match
> the values of the downstream NXP kernel, and I tried playing with
> these values manually, and 6 appeared to be the only number that
> seemed to work for me too.  I abandoned my approach for Lucas'
> implementation, because it seemed more clear than mine.
> Maybe Lucas can chime in, since this is really his patch.

Lucan, any inputs?

Jagan.

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

[Lucas Stach]

Am Mittwoch, dem 03.05.2023 um 22:05 +0530 schrieb Jagan Teki:
> On Mon, Apr 24, 2023 at 3:17 PM Adam Ford <aford173@gmail.com> wrote:
> > 
> > On Mon, Apr 24, 2023 at 4:03 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > 
> > > On Sun, Apr 23, 2023 at 5:42 PM Adam Ford <aford173@gmail.com> 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>
> > > > ---
> > > >  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;
> > > 
> > > I do not quite understand why it depends on burst_clk_rate, would you
> > > please explain? does it depends on bpp something like this
> > > 
> > > mipi_dsi_pixel_format_to_bpp(format) / 8
> > 
> > The pixel clock is currently set to the burst clock rate.  Dividing
> > the clock by 1000 gets the pixel clock in KHz, and dividing by 8
> > converts bits to bytes.
> > Later in the series, I change the clock from the burst clock to the
> > cached value returned from samsung_dsim_set_pll.
> 
> Okay.
> 
> > 
> > > 
> > > > +               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);
> > > 
> > > 6 blanking packet overhead here means, 4 bytes + payload + 2 bytes
> > > format? does this packet overhead depends on the respective porch's
> > > like hpf, hbp and hsa has different packet overheads?
> > 
> > Lucas might be able to explain this better.  However, it does match
> > the values of the downstream NXP kernel, and I tried playing with
> > these values manually, and 6 appeared to be the only number that
> > seemed to work for me too.  I abandoned my approach for Lucas'
> > implementation, because it seemed more clear than mine.
> > Maybe Lucas can chime in, since this is really his patch.
> 
> Lucan, any inputs?
> 
The blanking packets are are MIPI long packets, so 4 byte header,
payload, 2 bytes footer. I experimented with different blanking sizes
and haven't found that it would make any difference. I don't think any
real-world horizontal blanking sizes would require multiple packets to
be sent.

Regards,
Lucas

[PATCH V2 2/6] 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>
---
 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..adb9c13c5f7f 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,
+	.vco_min = 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,
+	.vco_min = 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,
+	.vco_min = 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,
+	.vco_min = 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,
+	.vco_min = 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,
+	.vco_min = 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->vco_min  * 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..a088d84579bc 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -59,6 +59,9 @@ struct samsung_dsim_driver_data {
 	unsigned int num_bits_resol;
 	unsigned int pll_p_offset;
 	const unsigned int *reg_values;
+	u16 m_min;
+	u16 m_max;
+	u64 vco_min;
 };
 
 struct samsung_dsim_host_ops {
-- 
2.39.2

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

[Lucas Stach]

Am Sonntag, dem 23.04.2023 um 07:12 -0500 schrieb 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>

> ---
>  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..adb9c13c5f7f 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,
> +	.vco_min = 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,
> +	.vco_min = 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,
> +	.vco_min = 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,
> +	.vco_min = 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,
> +	.vco_min = 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,
> +	.vco_min = 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->vco_min  * 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..a088d84579bc 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -59,6 +59,9 @@ struct samsung_dsim_driver_data {
>  	unsigned int num_bits_resol;
>  	unsigned int pll_p_offset;
>  	const unsigned int *reg_values;
> +	u16 m_min;
> +	u16 m_max;
> +	u64 vco_min;
>  };
>  
>  struct samsung_dsim_host_ops {

[PATCH V2 3/6] 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>
---
 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 adb9c13c5f7f..5b6e7825b92f 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

[PATCH V2 4/6] 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>
---
 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 5b6e7825b92f..f165483d5044 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,
 	.vco_min = 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 a088d84579bc..25475d78adb3 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -62,6 +62,7 @@ struct samsung_dsim_driver_data {
 	u16 m_min;
 	u16 m_max;
 	u64 vco_min;
+	bool dynamic_dphy;
 };
 
 struct samsung_dsim_host_ops {
-- 
2.39.2

Re: [PATCH V2 4/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Chen-Yu Tsai]

On Sun, Apr 23, 2023 at 8:13 PM Adam Ford <aford173@gmail.com> 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>
> ---
>  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 5b6e7825b92f..f165483d5044 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,
>         .vco_min = 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);

This requires adding "select GENERIC_PHY_MIPI_DPHY" to DRM_SAMSUNG_DSIM,
otherwise with CONFIG_DRM_SAMSUNG_DSIM=m:

ERROR: modpost: "phy_mipi_dphy_get_default_config"
[drivers/gpu/drm/bridge/samsung-dsim.ko] undefined!
make[5]: *** [scripts/Makefile.modpost:136: Module.symvers] Error 1
make[4]: *** [Makefile:1978: modpost] Error 2
make[3]: *** [Makefile:357: __build_one_by_one] Error 2

I'm sure there'll be a similar error if CONFIG_DRM_SAMSUNG_DSIM=y.

> +
> +               /*
> +                * 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 a088d84579bc..25475d78adb3 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -62,6 +62,7 @@ struct samsung_dsim_driver_data {
>         u16 m_min;
>         u16 m_max;
>         u64 vco_min;
> +       bool dynamic_dphy;
>  };
>
>  struct samsung_dsim_host_ops {
> --
> 2.39.2
>

Re: [PATCH V2 4/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Adam Ford]

On Mon, Apr 24, 2023 at 1:12 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Sun, Apr 23, 2023 at 8:13 PM Adam Ford <aford173@gmail.com> 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>
> > ---
> >  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 5b6e7825b92f..f165483d5044 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,
> >         .vco_min = 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);
>
> This requires adding "select GENERIC_PHY_MIPI_DPHY" to DRM_SAMSUNG_DSIM,
> otherwise with CONFIG_DRM_SAMSUNG_DSIM=m:
>
> ERROR: modpost: "phy_mipi_dphy_get_default_config"
> [drivers/gpu/drm/bridge/samsung-dsim.ko] undefined!
> make[5]: *** [scripts/Makefile.modpost:136: Module.symvers] Error 1
> make[4]: *** [Makefile:1978: modpost] Error 2
> make[3]: *** [Makefile:357: __build_one_by_one] Error 2
>
> I'm sure there'll be a similar error if CONFIG_DRM_SAMSUNG_DSIM=y.

That's interesting, I didn't come across that.
What did you use for a starting point when you applied the patches?
I want to see if I can replicate it.

adam
>
> > +
> > +               /*
> > +                * 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 a088d84579bc..25475d78adb3 100644
> > --- a/include/drm/bridge/samsung-dsim.h
> > +++ b/include/drm/bridge/samsung-dsim.h
> > @@ -62,6 +62,7 @@ struct samsung_dsim_driver_data {
> >         u16 m_min;
> >         u16 m_max;
> >         u64 vco_min;
> > +       bool dynamic_dphy;
> >  };
> >
> >  struct samsung_dsim_host_ops {
> > --
> > 2.39.2
> >

Re: [PATCH V2 4/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Chen-Yu Tsai]

On Mon, Apr 24, 2023 at 5:31 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Apr 24, 2023 at 1:12 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > On Sun, Apr 23, 2023 at 8:13 PM Adam Ford <aford173@gmail.com> 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>
> > > ---
> > >  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 5b6e7825b92f..f165483d5044 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,
> > >         .vco_min = 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);
> >
> > This requires adding "select GENERIC_PHY_MIPI_DPHY" to DRM_SAMSUNG_DSIM,
> > otherwise with CONFIG_DRM_SAMSUNG_DSIM=m:
> >
> > ERROR: modpost: "phy_mipi_dphy_get_default_config"
> > [drivers/gpu/drm/bridge/samsung-dsim.ko] undefined!
> > make[5]: *** [scripts/Makefile.modpost:136: Module.symvers] Error 1
> > make[4]: *** [Makefile:1978: modpost] Error 2
> > make[3]: *** [Makefile:357: __build_one_by_one] Error 2
> >
> > I'm sure there'll be a similar error if CONFIG_DRM_SAMSUNG_DSIM=y.
>
> That's interesting, I didn't come across that.
> What did you use for a starting point when you applied the patches?
> I want to see if I can replicate it.

next-20230421. My config is pretty much tailored to the Hummingbird Pulse.
Device drivers for other hardware or things that I can't enable are all
disabled. For example I don't have PHY_MIXEL_MIPI_DPHY enabled.

Maybe you have some other bridge or phy that selects it enabled?

ChenYu

Re: [PATCH V2 4/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Marek Szyprowski]

On 24.04.2023 11:44, Chen-Yu Tsai wrote:
> On Mon, Apr 24, 2023 at 5:31 PM Adam Ford <aford173@gmail.com> wrote:
>> On Mon, Apr 24, 2023 at 1:12 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
>>> On Sun, Apr 23, 2023 at 8:13 PM Adam Ford <aford173@gmail.com> 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>
>>>> ---
>>>>   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 5b6e7825b92f..f165483d5044 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,
>>>>          .vco_min = 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);
>>> This requires adding "select GENERIC_PHY_MIPI_DPHY" to DRM_SAMSUNG_DSIM,
>>> otherwise with CONFIG_DRM_SAMSUNG_DSIM=m:
>>>
>>> ERROR: modpost: "phy_mipi_dphy_get_default_config"
>>> [drivers/gpu/drm/bridge/samsung-dsim.ko] undefined!
>>> make[5]: *** [scripts/Makefile.modpost:136: Module.symvers] Error 1
>>> make[4]: *** [Makefile:1978: modpost] Error 2
>>> make[3]: *** [Makefile:357: __build_one_by_one] Error 2
>>>
>>> I'm sure there'll be a similar error if CONFIG_DRM_SAMSUNG_DSIM=y.
>> That's interesting, I didn't come across that.
>> What did you use for a starting point when you applied the patches?
>> I want to see if I can replicate it.
> next-20230421. My config is pretty much tailored to the Hummingbird Pulse.
> Device drivers for other hardware or things that I can't enable are all
> disabled. For example I don't have PHY_MIXEL_MIPI_DPHY enabled.
>
> Maybe you have some other bridge or phy that selects it enabled?


I've observed similar issue while building exynos_defconfig for arm 32bit.


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

Re: [PATCH V2 4/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Adam Ford]

On Mon, Apr 24, 2023 at 4:47 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 24.04.2023 11:44, Chen-Yu Tsai wrote:
> > On Mon, Apr 24, 2023 at 5:31 PM Adam Ford <aford173@gmail.com> wrote:
> >> On Mon, Apr 24, 2023 at 1:12 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >>> On Sun, Apr 23, 2023 at 8:13 PM Adam Ford <aford173@gmail.com> 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>
> >>>> ---
> >>>>   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 5b6e7825b92f..f165483d5044 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,
> >>>>          .vco_min = 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);
> >>> This requires adding "select GENERIC_PHY_MIPI_DPHY" to DRM_SAMSUNG_DSIM,
> >>> otherwise with CONFIG_DRM_SAMSUNG_DSIM=m:
> >>>
> >>> ERROR: modpost: "phy_mipi_dphy_get_default_config"
> >>> [drivers/gpu/drm/bridge/samsung-dsim.ko] undefined!
> >>> make[5]: *** [scripts/Makefile.modpost:136: Module.symvers] Error 1
> >>> make[4]: *** [Makefile:1978: modpost] Error 2
> >>> make[3]: *** [Makefile:357: __build_one_by_one] Error 2
> >>>
> >>> I'm sure there'll be a similar error if CONFIG_DRM_SAMSUNG_DSIM=y.
> >> That's interesting, I didn't come across that.
> >> What did you use for a starting point when you applied the patches?
> >> I want to see if I can replicate it.
> > next-20230421. My config is pretty much tailored to the Hummingbird Pulse.
> > Device drivers for other hardware or things that I can't enable are all
> > disabled. For example I don't have PHY_MIXEL_MIPI_DPHY enabled.
> >
> > Maybe you have some other bridge or phy that selects it enabled?
>
>
> I've observed similar issue while building exynos_defconfig for arm 32bit.

Thanks to both of you for the head's up.  I'll add a patch to update
Kconfig to explicitly select that when I do V3.  I was just using the
base arm64 'defconfig' option without any customization.

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

Re: [PATCH V2 4/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Adam Ford]

On Mon, Apr 24, 2023 at 4:50 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Apr 24, 2023 at 4:47 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> >
> > On 24.04.2023 11:44, Chen-Yu Tsai wrote:
> > > On Mon, Apr 24, 2023 at 5:31 PM Adam Ford <aford173@gmail.com> wrote:
> > >> On Mon, Apr 24, 2023 at 1:12 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> > >>> On Sun, Apr 23, 2023 at 8:13 PM Adam Ford <aford173@gmail.com> 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>
> > >>>> ---
> > >>>>   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 5b6e7825b92f..f165483d5044 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,
> > >>>>          .vco_min = 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);
> > >>> This requires adding "select GENERIC_PHY_MIPI_DPHY" to DRM_SAMSUNG_DSIM,
> > >>> otherwise with CONFIG_DRM_SAMSUNG_DSIM=m:
> > >>>
> > >>> ERROR: modpost: "phy_mipi_dphy_get_default_config"
> > >>> [drivers/gpu/drm/bridge/samsung-dsim.ko] undefined!
> > >>> make[5]: *** [scripts/Makefile.modpost:136: Module.symvers] Error 1
> > >>> make[4]: *** [Makefile:1978: modpost] Error 2
> > >>> make[3]: *** [Makefile:357: __build_one_by_one] Error 2
> > >>>
> > >>> I'm sure there'll be a similar error if CONFIG_DRM_SAMSUNG_DSIM=y.
> > >> That's interesting, I didn't come across that.
> > >> What did you use for a starting point when you applied the patches?
> > >> I want to see if I can replicate it.
> > > next-20230421. My config is pretty much tailored to the Hummingbird Pulse.
> > > Device drivers for other hardware or things that I can't enable are all
> > > disabled. For example I don't have PHY_MIXEL_MIPI_DPHY enabled.
> > >
> > > Maybe you have some other bridge or phy that selects it enabled?
> >
> >
> > I've observed similar issue while building exynos_defconfig for arm 32bit.
>
> Thanks to both of you for the head's up.  I'll add a patch to update
> Kconfig to explicitly select that when I do V3.  I was just using the
> base arm64 'defconfig' option without any customization.

Marek S & Chen-Yu,

I added a patch into the series to make this select
GENERIC_PHY_MIPI_DPHY, and I tested it with the 32-bit arm config,
exynos_defconfig and it now builds successfully.

I'm going to wait a few more hours to see if anyone else has any
feedback before I send V3.

adam



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

[PATCH V2 5/6] 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>
---
 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 f165483d5044..cea847b8e23c 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 for burst mode, otherwise use the pix_clk */
+	if ((dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && 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 V2 5/6] drm: bridge: samsung-dsim: Support non-burst mode

[Marek Szyprowski]

On 23.04.2023 14:12, 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>

This one breaks Exynos-5433 based TM2e board with a DSI panel.

> ---
>   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 f165483d5044..cea847b8e23c 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 for burst mode, otherwise use the pix_clk */
> +	if ((dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && 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);

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

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

[Adam Ford]

On Mon, Apr 24, 2023 at 3:25 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 23.04.2023 14:12, 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>
>
> This one breaks Exynos-5433 based TM2e board with a DSI panel.

Marek S,

Thank you for testing!  I knoiw there are several of us who appreciate
your testing this since it's hard to know if something broke without
hardware.  Is there any way you can tell me if the flag is set to
enable MIPI_DSI_MODE_VIDEO_BURST?
I was trying to be diligent about not breaking your boards, but
without your boards, it's difficult.  The theory was that if
MIPI_DSI_MODE_VIDEO_BURST is set and there is a burst clock set in the
device tree, it would use the burst clock.

As a fall-back I could just simply check for the presence of the
burst_clock_rate instead of both MIPI_DSI_MODE_VIDEO_BURST and
burst_clock_rate.


>
> > ---
> >   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 f165483d5044..cea847b8e23c 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 for burst mode, otherwise use the pix_clk */
> > +     if ((dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && dsi->burst_clk_rate)

Would you be willing to test this if this line just read:

              if (dsi->burst_clk_rate)

That would tell me if my fallback idea works.

Thank you,

adam

> > +             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);
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

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

[Marek Szyprowski]

On 24.04.2023 12:00, Adam Ford wrote:
> On Mon, Apr 24, 2023 at 3:25 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 23.04.2023 14:12, 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>
>> This one breaks Exynos-5433 based TM2e board with a DSI panel.
> Marek S,
>
> Thank you for testing!  I knoiw there are several of us who appreciate
> your testing this since it's hard to know if something broke without
> hardware.  Is there any way you can tell me if the flag is set to
> enable MIPI_DSI_MODE_VIDEO_BURST?

TM2e board uses the DSI panel operated in command mode and handled by 
panel-samsung-s6e3ha2.c driver. The MIPI_DSI_MODE_VIDEO_BURST flag is 
not set by the driver. However, the MIPI_DSI_CLOCK_NON_CONTINUOUS flags 
is set there. I really have no idea if setting VIDEO_BURST would make 
sense together with CLOCK_NON_CONTINUOUS or not. Maybe the driver lacks 
setting it?


> I was trying to be diligent about not breaking your boards, but
> without your boards, it's difficult.  The theory was that if
> MIPI_DSI_MODE_VIDEO_BURST is set and there is a burst clock set in the
> device tree, it would use the burst clock.
>
> As a fall-back I could just simply check for the presence of the
> burst_clock_rate instead of both MIPI_DSI_MODE_VIDEO_BURST and
> burst_clock_rate.

Maybe you should extend your check also for the 
MIPI_DSI_CLOCK_NON_CONTINUOUS flag? Does it make sense?

 > ...

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

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

[Adam Ford]

On Fri, Apr 28, 2023 at 7:31 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 24.04.2023 12:00, Adam Ford wrote:
> > On Mon, Apr 24, 2023 at 3:25 AM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 23.04.2023 14:12, 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>
> >> This one breaks Exynos-5433 based TM2e board with a DSI panel.
> > Marek S,
> >
> > Thank you for testing!  I knoiw there are several of us who appreciate
> > your testing this since it's hard to know if something broke without
> > hardware.  Is there any way you can tell me if the flag is set to
> > enable MIPI_DSI_MODE_VIDEO_BURST?
>
> TM2e board uses the DSI panel operated in command mode and handled by
> panel-samsung-s6e3ha2.c driver. The MIPI_DSI_MODE_VIDEO_BURST flag is
> not set by the driver. However, the MIPI_DSI_CLOCK_NON_CONTINUOUS flags
> is set there. I really have no idea if setting VIDEO_BURST would make
> sense together with CLOCK_NON_CONTINUOUS or not. Maybe the driver lacks
> setting it?
>
>
> > I was trying to be diligent about not breaking your boards, but
> > without your boards, it's difficult.  The theory was that if
> > MIPI_DSI_MODE_VIDEO_BURST is set and there is a burst clock set in the
> > device tree, it would use the burst clock.
> >
> > As a fall-back I could just simply check for the presence of the
> > burst_clock_rate instead of both MIPI_DSI_MODE_VIDEO_BURST and
> > burst_clock_rate.
>
> Maybe you should extend your check also for the
> MIPI_DSI_CLOCK_NON_CONTINUOUS flag? Does it make sense?

Looking at some of the devices that might attach in the future, It
appears that ti-sn65dsi86.c sets this flag.  It's a display port
bridge, so I would expect it to need a variable clock rate similar to
how the HDMI bridge that I need works.  I am concerned that I make the
burst clock dependent on MIPI_DSI_CLOCK_NON_CONTINUOUS, it might break
the Display Port bridge.

I think it's better to just check if the samsung,burst-clock-frequency
is present in the device tree and use it when present.  If it's not
present, then fall back to the pixel clock of the connected device.

I looked at a bunch of Exynos parts, and it looks like they all use
the samsung,burst-clock-frequency device tree setting.  Is that true,
or did I miss one?

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

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

[Marek Szyprowski]

On 28.04.2023 15:35, Adam Ford wrote:
> On Fri, Apr 28, 2023 at 7:31 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 24.04.2023 12:00, Adam Ford wrote:
>>> On Mon, Apr 24, 2023 at 3:25 AM Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>> On 23.04.2023 14:12, 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>
>>>> This one breaks Exynos-5433 based TM2e board with a DSI panel.
>>> Marek S,
>>>
>>> Thank you for testing!  I knoiw there are several of us who appreciate
>>> your testing this since it's hard to know if something broke without
>>> hardware.  Is there any way you can tell me if the flag is set to
>>> enable MIPI_DSI_MODE_VIDEO_BURST?
>> TM2e board uses the DSI panel operated in command mode and handled by
>> panel-samsung-s6e3ha2.c driver. The MIPI_DSI_MODE_VIDEO_BURST flag is
>> not set by the driver. However, the MIPI_DSI_CLOCK_NON_CONTINUOUS flags
>> is set there. I really have no idea if setting VIDEO_BURST would make
>> sense together with CLOCK_NON_CONTINUOUS or not. Maybe the driver lacks
>> setting it?
>>
>>
>>> I was trying to be diligent about not breaking your boards, but
>>> without your boards, it's difficult.  The theory was that if
>>> MIPI_DSI_MODE_VIDEO_BURST is set and there is a burst clock set in the
>>> device tree, it would use the burst clock.
>>>
>>> As a fall-back I could just simply check for the presence of the
>>> burst_clock_rate instead of both MIPI_DSI_MODE_VIDEO_BURST and
>>> burst_clock_rate.
>> Maybe you should extend your check also for the
>> MIPI_DSI_CLOCK_NON_CONTINUOUS flag? Does it make sense?
> Looking at some of the devices that might attach in the future, It
> appears that ti-sn65dsi86.c sets this flag.  It's a display port
> bridge, so I would expect it to need a variable clock rate similar to
> how the HDMI bridge that I need works.  I am concerned that I make the
> burst clock dependent on MIPI_DSI_CLOCK_NON_CONTINUOUS, it might break
> the Display Port bridge.
>
> I think it's better to just check if the samsung,burst-clock-frequency
> is present in the device tree and use it when present.  If it's not
> present, then fall back to the pixel clock of the connected device.

Right, this sounds rational.

> I looked at a bunch of Exynos parts, and it looks like they all use
> the samsung,burst-clock-frequency device tree setting.  Is that true,
> or did I miss one?

That true. All Exynos based boards with DSI panels use constant DSI 
burst frequency defined in the device tree.


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

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

[Adam Ford]

On Fri, Apr 28, 2023 at 9:04 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 28.04.2023 15:35, Adam Ford wrote:
> > On Fri, Apr 28, 2023 at 7:31 AM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 24.04.2023 12:00, Adam Ford wrote:
> >>> On Mon, Apr 24, 2023 at 3:25 AM Marek Szyprowski
> >>> <m.szyprowski@samsung.com> wrote:
> >>>> On 23.04.2023 14:12, 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>
> >>>> This one breaks Exynos-5433 based TM2e board with a DSI panel.
> >>> Marek S,
> >>>
> >>> Thank you for testing!  I knoiw there are several of us who appreciate
> >>> your testing this since it's hard to know if something broke without
> >>> hardware.  Is there any way you can tell me if the flag is set to
> >>> enable MIPI_DSI_MODE_VIDEO_BURST?
> >> TM2e board uses the DSI panel operated in command mode and handled by
> >> panel-samsung-s6e3ha2.c driver. The MIPI_DSI_MODE_VIDEO_BURST flag is
> >> not set by the driver. However, the MIPI_DSI_CLOCK_NON_CONTINUOUS flags
> >> is set there. I really have no idea if setting VIDEO_BURST would make
> >> sense together with CLOCK_NON_CONTINUOUS or not. Maybe the driver lacks
> >> setting it?
> >>
> >>
> >>> I was trying to be diligent about not breaking your boards, but
> >>> without your boards, it's difficult.  The theory was that if
> >>> MIPI_DSI_MODE_VIDEO_BURST is set and there is a burst clock set in the
> >>> device tree, it would use the burst clock.
> >>>
> >>> As a fall-back I could just simply check for the presence of the
> >>> burst_clock_rate instead of both MIPI_DSI_MODE_VIDEO_BURST and
> >>> burst_clock_rate.
> >> Maybe you should extend your check also for the
> >> MIPI_DSI_CLOCK_NON_CONTINUOUS flag? Does it make sense?
> > Looking at some of the devices that might attach in the future, It
> > appears that ti-sn65dsi86.c sets this flag.  It's a display port
> > bridge, so I would expect it to need a variable clock rate similar to
> > how the HDMI bridge that I need works.  I am concerned that I make the
> > burst clock dependent on MIPI_DSI_CLOCK_NON_CONTINUOUS, it might break
> > the Display Port bridge.
> >
> > I think it's better to just check if the samsung,burst-clock-frequency
> > is present in the device tree and use it when present.  If it's not
> > present, then fall back to the pixel clock of the connected device.
>
> Right, this sounds rational.
>
> > I looked at a bunch of Exynos parts, and it looks like they all use
> > the samsung,burst-clock-frequency device tree setting.  Is that true,
> > or did I miss one?
>
> That true. All Exynos based boards with DSI panels use constant DSI
> burst frequency defined in the device tree.

Thanks for the feedback.  I'll try to get another rev of this series
pushed later today or Monday.

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

[PATCH V2 6/6] 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>
---
 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 cea847b8e23c..8c69d22a57b7 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 25475d78adb3..41cbae00cd7e 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 V2 0/6] drm: bridge: samsung-dsim: Support variable clocking

[Chen-Yu Tsai]

On Sun, Apr 23, 2023 at 8:12 PM 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 (5):
>   drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]
>   drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically
>   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

This makes the micro-HDMI port on my Hummingboard Pulse (w/ i.MX8M Mini SOM)
work properly, so the whole series is

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

Thanks!