Re: [PATCH v3 2/3] dt-bindings: lcdif: Expand the imx6sl/imx6sll fallbacks

[Marek Vasut]

On 10/29/24 8:16 PM, Fabio Estevam wrote:

From: Fabio Estevam 

mx6sl.dtsi and imx6sll.dtsi have the following lcdif entries:

compatible = "fsl,imx6sl-lcdif", "fsl,imx28-lcdif";

This causes dt-schema warnings as the current binding only
allow 'fsl,imx6sx-lcdif' as fallback.

['fsl,imx6sl-lcdif', 'fsl,imx28-lcdif'] is too long
['fsl,imx6sll-lcdif', 'fsl,imx28-lcdif'] is too long

The imx6sx-lcdif programming model has more advanced features, such
as overlay plane and the CRC32 support than the imx28-lcdif IP.

Expand the imx6sl/imx6sll lcdif fallbacks to accept a less specific
fsl,imx28-lcdif fallback:

compatible = "fsl,imx6sl-lcdif", "fsl,imx6sx-lcdif", "fsl,imx28-lcdif";

This helps keeping DT compatibility as well as using the more advanced
lcdif features found on imx6sl and imx6sll.

Signed-off-by: Fabio Estevam 
---
Changes since v2:
- Make the three compatible entres the only valid combination
for imx6sl and imx6sll (Andreas).

  Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml 
b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
index ad0cca562463..72e509bc975b 100644
--- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
+++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
@@ -23,14 +23,18 @@ properties:
- fsl,imx93-lcdif
- items:
- enum:
-  - fsl,imx6sl-lcdif
-  - fsl,imx6sll-lcdif
- fsl,imx6ul-lcdif
- fsl,imx7d-lcdif
- fsl,imx8mm-lcdif
- fsl,imx8mn-lcdif
- fsl,imx8mq-lcdif
- const: fsl,imx6sx-lcdif
+  - items:
+  - enum:
+  - fsl,imx6sl-lcdif
+  - fsl,imx6sll-lcdif
+  - const: fsl,imx6sx-lcdif
+  - const: fsl,imx28-lcdif

Shouldn't this be

- enum:
  - fsl,imx6sl-lcdif
  - fsl,imx6sll-lcdif
  - fsl,imx6sx-lcdif
- const: fsl,imx28-lcdif

So you wouldn't have to write three compatible strings for the 6sl/sll , 
but only two ? I.e. this:


compatible = "fsl,imx6sl-lcdif", "fsl,imx28-lcdif";
compatible = "fsl,imx6sll-lcdif", "fsl,imx28-lcdif";
compatible = "fsl,imx6sx-lcdif", "fsl,imx28-lcdif";

?

Re: [PATCH 2/2] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Marek Vasut]

On 10/28/24 2:52 PM, Herve Codina wrote:

Hi Marek,


Hi,


On Sat, 26 Oct 2024 00:53:51 +0200
Marek Vasut  wrote:
   

On 10/24/24 11:55 AM, Herve Codina wrote:

In some cases observed during ESD tests, the TI SN65DSI83 cannot recover
from errors by itself. A full restart of the bridge is needed in those
cases to have the bridge output LVDS signals again.


I have seen the bridge being flaky sometimes, do you have any more
details of what is going on when this irrecoverable error occurs ?


The panel attached to the bridge goes and stays black. That's the behavior.
A full reset brings the panel back displaying frames.

Is there some noticeable change in 0xe0/0xe1/0xe5 registers, esp. 0xe5,
do they indicate the error occurred somehow ?


0xe5 register can signal any DSI errors (depending on when the ESD affects
the DSI bus) even PLL unlock bit was observed set but we didn't see any
relationship between the bits set in 0xe5 register and the recoverable or
unrecoverable behavior.

Also, in some cases, reading the register was not even possible (i2c
transaction nacked).
Oh, wow, I haven't seen that one before. But this is really useful 
information, can you please add it into the commit message for V2 ?


Thank you

Re: [PATCH] drm/bridge: tc358767: Fix odd pixel alignment

[Marek Vasut]

On 10/28/24 2:52 PM, Maxime Ripard wrote:

On Mon, Oct 28, 2024 at 01:36:58PM +0100, Marek Vasut wrote:

On 10/28/24 10:25 AM, Maxime Ripard wrote:

On Sat, Oct 26, 2024 at 06:10:01AM +0200, Marek Vasut wrote:

Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
bitfields description state "These bits must be multiple of even
pixel". It is not possible to simply align every bitfield to the
nearest even pixel, because that would unalign the line width and
cause visible distortion. Instead, attempt to re-align the timings
such that the hardware requirement is fulfilled without changing
the line width if at all possible.

Warn the user in case a panel with odd active pixel width or full
line width is used, this is not possible to support with this one
bridge.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Simona Vetter 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
---
   drivers/gpu/drm/bridge/tc358767.c | 63 +--
   1 file changed, 60 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 0a6894498267e..7968183510e63 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -901,6 +901,63 @@ static int tc_set_common_video_mode(struct tc_data *tc,
int vsync_len = mode->vsync_end - mode->vsync_start;
int ret;
+   /*
+* Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
+* bitfields description state "These bits must be multiple of even
+* pixel". It is not possible to simply align every bitfield to the
+* nearest even pixel, because that would unalign the line width.
+* Instead, attempt to re-align the timings.
+*/
+
+   /* Panels with odd active pixel count are not supported by the bridge */
+   if (mode->hdisplay & 1)
+   dev_warn(tc->dev, "Panels with odd pixel count per active line are 
not supported.\n");
+
+   /* HPW is odd */
+   if (hsync_len & 1) {
+   /* Make sure there is some margin left */
+   if (left_margin >= 2) {
+   /* Align HPW up */
+   hsync_len++;
+   left_margin--;
+   } else if (right_margin >= 2) {
+   /* Align HPW up */
+   hsync_len++;
+   right_margin--;
+   } else if (hsync_len > 2) {
+   /* Align HPW down as last-resort option */
+   hsync_len--;
+   left_margin++;
+   } else {
+   dev_warn(tc->dev, "HPW is odd, not enough margins to 
compensate.\n");
+   }
+   }
+
+   /* HBP is odd (HPW is surely even now) */
+   if (left_margin & 1) {
+   /* Make sure there is some margin left */
+   if (right_margin >= 2) {
+   /* Align HBP up */
+   left_margin++;
+   right_margin--;
+   } else if (hsync_len > 2) {
+   /* HPW is surely even and > 2, which means at least 4 */
+   hsync_len -= 2;
+   /*
+* Subtract 2 from sync pulse and distribute it between
+* margins. This aligns HBP and keeps HPW aligned.
+*/
+   left_margin++;
+   right_margin++;
+   } else {
+   dev_warn(tc->dev, "HBP is odd, not enough pixels to 
compensate.\n");
+   }
+   }
+
+   /* HFP is odd (HBP and HPW is surely even now) */
+   if (right_margin & 1)
+   dev_warn(tc->dev, "HFP is odd, panels with odd pixel count per full 
line are not supported.\n");
+


This should all happen in atomic_check, and reject modes that can't
be supported.



No, that would reject panels I need to support and which can be
supported by this bridge.


Then drop the warnings, either you support it or you don't.
These warnings are useful to notify users that something is not right. I 
find them useful when bringing up systems with this bridge.

Re: [PATCH] drm/bridge: tc358767: Fix odd pixel alignment

[Marek Vasut]

On 10/28/24 10:25 AM, Maxime Ripard wrote:

On Sat, Oct 26, 2024 at 06:10:01AM +0200, Marek Vasut wrote:

Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
bitfields description state "These bits must be multiple of even
pixel". It is not possible to simply align every bitfield to the
nearest even pixel, because that would unalign the line width and
cause visible distortion. Instead, attempt to re-align the timings
such that the hardware requirement is fulfilled without changing
the line width if at all possible.

Warn the user in case a panel with odd active pixel width or full
line width is used, this is not possible to support with this one
bridge.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Simona Vetter 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
---
  drivers/gpu/drm/bridge/tc358767.c | 63 +--
  1 file changed, 60 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 0a6894498267e..7968183510e63 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -901,6 +901,63 @@ static int tc_set_common_video_mode(struct tc_data *tc,
int vsync_len = mode->vsync_end - mode->vsync_start;
int ret;
  
+	/*

+* Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
+* bitfields description state "These bits must be multiple of even
+* pixel". It is not possible to simply align every bitfield to the
+* nearest even pixel, because that would unalign the line width.
+* Instead, attempt to re-align the timings.
+*/
+
+   /* Panels with odd active pixel count are not supported by the bridge */
+   if (mode->hdisplay & 1)
+   dev_warn(tc->dev, "Panels with odd pixel count per active line are 
not supported.\n");
+
+   /* HPW is odd */
+   if (hsync_len & 1) {
+   /* Make sure there is some margin left */
+   if (left_margin >= 2) {
+   /* Align HPW up */
+   hsync_len++;
+   left_margin--;
+   } else if (right_margin >= 2) {
+   /* Align HPW up */
+   hsync_len++;
+   right_margin--;
+   } else if (hsync_len > 2) {
+   /* Align HPW down as last-resort option */
+   hsync_len--;
+   left_margin++;
+   } else {
+   dev_warn(tc->dev, "HPW is odd, not enough margins to 
compensate.\n");
+   }
+   }
+
+   /* HBP is odd (HPW is surely even now) */
+   if (left_margin & 1) {
+   /* Make sure there is some margin left */
+   if (right_margin >= 2) {
+   /* Align HBP up */
+   left_margin++;
+   right_margin--;
+   } else if (hsync_len > 2) {
+   /* HPW is surely even and > 2, which means at least 4 */
+   hsync_len -= 2;
+   /*
+* Subtract 2 from sync pulse and distribute it between
+* margins. This aligns HBP and keeps HPW aligned.
+*/
+   left_margin++;
+   right_margin++;
+   } else {
+   dev_warn(tc->dev, "HBP is odd, not enough pixels to 
compensate.\n");
+   }
+   }
+
+   /* HFP is odd (HBP and HPW is surely even now) */
+   if (right_margin & 1)
+   dev_warn(tc->dev, "HFP is odd, panels with odd pixel count per full 
line are not supported.\n");
+


This should all happen in atomic_check, and reject modes that can't be 
supported.
No, that would reject panels I need to support and which can be 
supported by this bridge.

Re: [PATCH 2/2] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Marek Vasut]

On 10/28/24 9:02 AM, Herve Codina wrote:

Hi Marek,


Hi,


On Sat, 26 Oct 2024 00:53:51 +0200
Marek Vasut  wrote:


On 10/24/24 11:55 AM, Herve Codina wrote:

In some cases observed during ESD tests, the TI SN65DSI83 cannot recover
from errors by itself. A full restart of the bridge is needed in those
cases to have the bridge output LVDS signals again.


I have seen the bridge being flaky sometimes, do you have any more
details of what is going on when this irrecoverable error occurs ?


The panel attached to the bridge goes and stays black. That's the behavior.
A full reset brings the panel back displaying frames.
Is there some noticeable change in 0xe0/0xe1/0xe5 registers, esp. 0xe5, 
do they indicate the error occurred somehow ?

[PATCH v2] drm/bridge: tc358767: Improve DPI output pixel clock accuracy

[Marek Vasut]

The Pixel PLL is not very capable and may come up with wildly inaccurate
clock. Since DPI panels are often tolerant to slightly higher pixel clock
without being operated outside of specification, calculate two Pixel PLL
settings for DPI output, one for desired output pixel clock and one for
output pixel clock with 1% increase, and then pick the result which is
closer to the desired pixel clock and use it as the Pixel PLL settings.

For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency
without this patch is 65 MHz which is out of the panel specification of
68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within
the specification and far more accurate.

Keep the change isolated to DPI output.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Simona Vetter 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
---
V2: Isolate the change to DPI only, split tc_bridge_mode_set()
---
 drivers/gpu/drm/bridge/tc358767.c | 47 ++-
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 7968183510e63..84c1429321c43 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1682,15 +1682,39 @@ static int tc_dpi_atomic_check(struct drm_bridge 
*bridge,
   struct drm_connector_state *conn_state)
 {
struct tc_data *tc = bridge_to_tc(bridge);
-   int adjusted_clock = 0;
+   int adjusted_clock_0p = 0;
+   int adjusted_clock_1p = 0;
+   int adjusted_clock;
int ret;
 
+   /*
+* Calculate adjusted clock pixel and find out what the PLL can
+* produce. The PLL is limited, so the clock might be inaccurate.
+*/
ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
  crtc_state->mode.clock * 1000,
- &adjusted_clock, NULL);
+ &adjusted_clock_0p, NULL);
if (ret)
return ret;
 
+   /*
+* Calculate adjusted pixel clock with 1% faster requested pixel
+* clock and find out what the PLL can produce. This may in fact
+* be closer to the expected pixel clock of the output.
+*/
+   ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
+ crtc_state->mode.clock * 1010,
+ &adjusted_clock_1p, NULL);
+   if (ret)
+   return ret;
+
+   /* Pick the more accurate of the two calculated results. */
+   if (crtc_state->mode.clock * 1010 - adjusted_clock_1p <
+   crtc_state->mode.clock * 1000 - adjusted_clock_0p)
+   adjusted_clock = adjusted_clock_1p;
+   else
+   adjusted_clock = adjusted_clock_0p;
+
crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
 
/* DSI->DPI interface clock limitation: upto 100 MHz */
@@ -1758,9 +1782,18 @@ tc_edp_mode_valid(struct drm_bridge *bridge,
return MODE_OK;
 }
 
-static void tc_bridge_mode_set(struct drm_bridge *bridge,
-  const struct drm_display_mode *mode,
-  const struct drm_display_mode *adj)
+static void tc_dpi_bridge_mode_set(struct drm_bridge *bridge,
+  const struct drm_display_mode *mode,
+  const struct drm_display_mode *adj)
+{
+   struct tc_data *tc = bridge_to_tc(bridge);
+
+   drm_mode_copy(&tc->mode, adj);
+}
+
+static void tc_edp_bridge_mode_set(struct drm_bridge *bridge,
+  const struct drm_display_mode *mode,
+  const struct drm_display_mode *adj)
 {
struct tc_data *tc = bridge_to_tc(bridge);
 
@@ -1977,7 +2010,7 @@ tc_edp_atomic_get_output_bus_fmts(struct drm_bridge 
*bridge,
 static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
.attach = tc_dpi_bridge_attach,
.mode_valid = tc_dpi_mode_valid,
-   .mode_set = tc_bridge_mode_set,
+   .mode_set = tc_dpi_bridge_mode_set,
.atomic_check = tc_dpi_atomic_check,
.atomic_enable = tc_dpi_bridge_atomic_enable,
.atomic_disable = tc_dpi_bridge_atomic_disable,
@@ -1991,7 +2024,7 @@ static const struct drm_bridge_funcs tc_edp_bridge_funcs 
= {
.attach = tc_edp_bridge_attach,
.detach = tc_edp_bridge_detach,
.mode_valid = tc_edp_mode_valid,
-   .mode_set = tc_bridge_mode_set,
+   .mode_set = tc_edp_bridge_mode_set,
.atomic_check = tc_edp_atomic_check,
.atomic_enable = tc_edp_bridge_atomic_enable,
.atomic_disable = tc_edp_bridge_atomic_disable,
-- 
2.45.2

[PATCH] drm/bridge: tc358767: Fix odd pixel alignment

[Marek Vasut]

Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
bitfields description state "These bits must be multiple of even
pixel". It is not possible to simply align every bitfield to the
nearest even pixel, because that would unalign the line width and
cause visible distortion. Instead, attempt to re-align the timings
such that the hardware requirement is fulfilled without changing
the line width if at all possible.

Warn the user in case a panel with odd active pixel width or full
line width is used, this is not possible to support with this one
bridge.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Simona Vetter 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/tc358767.c | 63 +--
 1 file changed, 60 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 0a6894498267e..7968183510e63 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -901,6 +901,63 @@ static int tc_set_common_video_mode(struct tc_data *tc,
int vsync_len = mode->vsync_end - mode->vsync_start;
int ret;
 
+   /*
+* Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
+* bitfields description state "These bits must be multiple of even
+* pixel". It is not possible to simply align every bitfield to the
+* nearest even pixel, because that would unalign the line width.
+* Instead, attempt to re-align the timings.
+*/
+
+   /* Panels with odd active pixel count are not supported by the bridge */
+   if (mode->hdisplay & 1)
+   dev_warn(tc->dev, "Panels with odd pixel count per active line 
are not supported.\n");
+
+   /* HPW is odd */
+   if (hsync_len & 1) {
+   /* Make sure there is some margin left */
+   if (left_margin >= 2) {
+   /* Align HPW up */
+   hsync_len++;
+   left_margin--;
+   } else if (right_margin >= 2) {
+   /* Align HPW up */
+   hsync_len++;
+   right_margin--;
+   } else if (hsync_len > 2) {
+   /* Align HPW down as last-resort option */
+   hsync_len--;
+   left_margin++;
+   } else {
+   dev_warn(tc->dev, "HPW is odd, not enough margins to 
compensate.\n");
+   }
+   }
+
+   /* HBP is odd (HPW is surely even now) */
+   if (left_margin & 1) {
+   /* Make sure there is some margin left */
+   if (right_margin >= 2) {
+   /* Align HBP up */
+   left_margin++;
+   right_margin--;
+   } else if (hsync_len > 2) {
+   /* HPW is surely even and > 2, which means at least 4 */
+   hsync_len -= 2;
+   /*
+* Subtract 2 from sync pulse and distribute it between
+* margins. This aligns HBP and keeps HPW aligned.
+*/
+   left_margin++;
+   right_margin++;
+   } else {
+   dev_warn(tc->dev, "HBP is odd, not enough pixels to 
compensate.\n");
+   }
+   }
+
+   /* HFP is odd (HBP and HPW is surely even now) */
+   if (right_margin & 1)
+   dev_warn(tc->dev, "HFP is odd, panels with odd pixel count per 
full line are not supported.\n");
+
dev_dbg(tc->dev, "set mode %dx%d\n",
mode->hdisplay, mode->vdisplay);
dev_dbg(tc->dev, "H margin %d,%d sync %d\n",
@@ -922,14 +979,14 @@ static int tc_set_common_video_mode(struct tc_data *tc,
return ret;
 
ret = regmap_write(tc->regmap, HTIM01,
-  FIELD_PREP(HBPR, ALIGN(left_margin, 2)) |
-  FIELD_PREP(HPW, ALIGN(hsync_len, 2)));
+  FIELD_PREP(HBPR, left_margin) |
+  FIELD_PREP(HPW, hsync_len));
if (ret)
return ret;
 
ret = regmap_write(tc->regmap, HTIM02,
   FIELD_PREP(HDISPR, ALIGN(mode->hdisplay, 2)) |
-  FIELD_PREP(HFPR, ALIGN(right_margin, 2)));
+  FIELD_PREP(HFPR, right_margin));
if (ret)
return ret;
 
-- 
2.45.2

Re: [PATCH 2/2] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Marek Vasut]

On 10/24/24 11:55 AM, Herve Codina wrote:

In some cases observed during ESD tests, the TI SN65DSI83 cannot recover
from errors by itself. A full restart of the bridge is needed in those
cases to have the bridge output LVDS signals again.


I have seen the bridge being flaky sometimes, do you have any more 
details of what is going on when this irrecoverable error occurs ?



The TI SN65DSI83 has some error detection capabilities. Introduce an
error recovery mechanism based on this detection.

The errors detected are signaled through an interrupt. On system where
this interrupt is not available, the driver uses a polling monitoring
fallback to check for errors. When an error is present, the recovery
process is launched.

Restarting the bridge needs to redo the initialization sequence. This
initialization sequence has to be done with the DSI data lanes driven in
LP11 state. In order to do that, the recovery process resets the entire
pipeline.


+CC Michael regarding the LP11 part , I think there was some development 
to switch lanes into LP11 mode on request ?


[...]

[PATCH] drm/bridge: tc358767: Improve DPI output pixel clock accuracy

[Marek Vasut]

The Pixel PLL is not very capable and may come up with wildly inaccurate
clock. Since DPI panels are often tolerant to slightly higher pixel clock
without being operated outside of specification, calculate two Pixel PLL
settings for DPI output, one for desired output pixel clock and one for
output pixel clock with 1% increase, and then pick the result which is
closer to the desired pixel clock and use it as the Pixel PLL settings.

For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency
without this patch is 65 MHz which is out of the panel specification of
68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within
the specification and far more accurate.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Simona Vetter 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/tc358767.c | 28 ++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 0d523322fdd8e..7e1a7322cec70 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1682,15 +1682,39 @@ static int tc_dpi_atomic_check(struct drm_bridge 
*bridge,
   struct drm_connector_state *conn_state)
 {
struct tc_data *tc = bridge_to_tc(bridge);
-   int adjusted_clock = 0;
+   int adjusted_clock_0p = 0;
+   int adjusted_clock_1p = 0;
+   int adjusted_clock;
int ret;
 
+   /*
+* Calculate adjusted clock pixel and find out what the PLL can
+* produce. The PLL is limited, so the clock might be inaccurate.
+*/
ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
  crtc_state->mode.clock * 1000,
- &adjusted_clock, NULL);
+ &adjusted_clock_0p, NULL);
+   if (ret)
+   return ret;
+
+   /*
+* Calculate adjusted pixel clock with 1% faster requested pixel
+* clock and find out what the PLL can produce. This may in fact
+* be closer to the expected pixel clock of the output.
+*/
+   ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
+ crtc_state->mode.clock * 1010,
+ &adjusted_clock_1p, NULL);
if (ret)
return ret;
 
+   /* Pick the more accurate of the two calculated results. */
+   if (crtc_state->mode.clock * 1010 - adjusted_clock_1p <
+   crtc_state->mode.clock * 1000 - adjusted_clock_0p)
+   adjusted_clock = adjusted_clock_1p;
+   else
+   adjusted_clock = adjusted_clock_0p;
+
crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
 
/* DSI->DPI interface clock limitation: upto 100 MHz */
-- 
2.45.2

[PATCH] drm/bridge: tc358767: Fix use of unadjusted mode in the driver

[Marek Vasut]

The driver configures mostly Pixel PLL from the clock cached in
local copy of the mode. Make sure the driver uses adjusted mode
which contains the updated Pixel PLL settings negotiated in
tc_dpi_atomic_check()/tc_edp_atomic_check().

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Simona Vetter 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 7968183510e63..0d523322fdd8e 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1764,7 +1764,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge,
 {
struct tc_data *tc = bridge_to_tc(bridge);
 
-   drm_mode_copy(&tc->mode, mode);
+   drm_mode_copy(&tc->mode, adj);
 }
 
 static const struct drm_edid *tc_edid_read(struct drm_bridge *bridge,
-- 
2.45.2

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Marek Vasut]

On 10/22/24 7:59 AM, Liu Ying wrote:

[...]


Anyway, I don't think it is necessary to manage the clk_set_rate()
function calls between this driver and mxsfb_kms or lcdif_kms
because "video_pll1" clock rate is supposed to be assigned in DT...


I disagree with this part. I believe the assignment of clock in DT is only a 
temporary workaround which should be removed. The drivers should be able to 
figure out and set the clock tree configuration.


I think the clock rate assignment in DT is still needed.
A good reason is that again we need to share one video PLL
between MIPI DSI and LDB display pipelines for i.MX8MP.


You don't really need to share the Video PLL , you can free up e.g. PLL3 
and use it for one video output pipeline, and use the Video PLL for the 
other video pipeline, and then you get accurate pixel clock in both 
pipelines.



The idea is to assign a reasonable PLL clock rate in DT to make
display drivers' life easier, especially for i.MX8MP where LDB,
Samsung MIPI DSI may use a single PLL at the same time.

I would really like to avoid setting arbitrary clock in DT, esp. if it can be 
avoided. And it surely can be avoided for this simple use case.


... just like I said in patch 1/2, "video_pll1" clock rate needs
to be x2 "media_ldb" clock rate for dual LVDS link mode. Without
an assigned "video_pll1" clock rate in DT, this driver cannot
achieve that.


This is something the LDB driver can infer from DT and configure the clock tree 
accordingly.


Well, the LDB driver only controls the "ldb" clock rate. It doesn't
magically set the parent "video_pll1" clock's rate to 2x it's rate,
unless the driver gets "video_pll1_out" clock by calling
clk_get_parent() and directly controls the PLL clock rate which
doesn't look neat.


It isn't nice, but it actually may solve this problem, no ?


And, the i.MX8MP LDB + Samsung MIPI DSI case is
not simple considering using one single PLL and display modes
read from EDID.

You could use separate PLLs for each LCDIF scanout engine in such a deployment, 
I already ran into that, so I am aware of it. That is probably the best way out 
of such a problem, esp. if accurate pixel clock are the requirement.


I cannot use separate PLLs for the i.MX8MP LDB and Samsung MIPI
DSI display pipelines on i.MX8MP EVK, because the PLLs are limited
resources and we are running out of it.  Because LDB needs the pixel
clock and LVDS serial clock to be derived from a same PLL, the only
valid PLLs(see imx8mp_media_disp_pix_sels[] and
imx8mp_media_ldb_sels[]) are "video_pll1_out", "audio_pll2_out",
"sys_pll2_1000m" and "sys_pll1_800m".  All are used as either audio
clock or system clocks on i.MX8MP EVK, except "video_pll1_out".


Could you use Video PLL for LDB and PLL3 for DSI then ?

I think this could still be configurable per board, it shouldn't be such 
that one board which attempts to showcase everything would prevent other 
boards with specific requirements from achieving those.



You probably may use separate PLLs for a particular i.MX8MP platform
with limited features, but not for i.MX8MP EVK which is supposed to
evaluate all SoC features.

Right, that, exactly.

[...]

Re: [PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate

[Marek Vasut]

On 10/22/24 8:13 AM, Liu Ying wrote:

[...]


This patch would cause the below in-flight LDB bridge driver
patch[1] fail to do display mode validation upon display modes
read from LVDS to HDMI converter IT6263's DDC I2C bus.


Why ?


Mode validation is affected only for dual LVDS link mode.
For single LVDS link mode, this patch does open more display
modes read from the DDC I2C bus.  The reason behind is that
LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
link mode, while it's 7 for single LVDS link mode.

In my system, "video_pll1" clock rate is assigned to 1.0395GHz
in imx8mp.dtsi.  For 1920x1080-60.00Hz with 148.5MHz pixel
clock rate, "media_ldb" clock rate is 519.75MHz and
"media_disp2_pix" clock rate is 148.5MHz, which is fine for
dual LVDS link mode(x3.5).  For newly opened up 1920x1080-59.94Hz
with 148.352MHz pixel clock rate, "video_pll1" clock rate will
be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
too because "media_disp2_pix" clock cannot handle the 3.5
division ratio from "video_pll1_out" clock running at
519.232MHz.  See the below clk_summary.


Shouldn't this patch help exactly with that ?


No, it doesn't help but only makes clk_round_rate() called in
LDB driver's .mode_valid() against 148.352MHz return 148.352MHz
which allows the unexpected 1920x1080-59.94Hz display mode.


Why is 1920x1080-59.94Hz mode unexpected in the first place ?
I assume your display device reports that it supports this mode, and now 
the scanout engine and LDB can generate this mode too ? Or does the 
display device NOT support this mode ?



It should allow you to set video_pll1_out to whatever is necessary by LDB 
first, fixate that frequency, and the LCDIFv3 would then be forced to use /7 
divider from faster Video PLL1 , right ?


Yes, it allows that for single-link LVDS use cases.
And, __no__, for dual-link LVDS use cases because the
video_pll1_out clock rate needs to be 2x the LVDS serial clock
rate.


Can't the LDB still set the Video PLL frequency to whatever it needs 
first, fixate it, and only then let the LCDIFv3 divide the frequency 
down ? (sorry, I am a bit tired today, maybe I am missing the obvious)



  video_pll1_ref_sel   1   1    0    2400    0  
    0 5  Y  deviceless  no_connection_id
     video_pll1    1   1    0    519232000   0  
    0 5  Y deviceless  
no_connection_id
    video_pll1_bypass  1   1    0    519232000   0  
    0 5  Y    deviceless  
no_connection_id
   video_pll1_out  2   2    0    519232000   0  
    0 5  Y   deviceless  
no_connection_id
  media_ldb    1   1    0    519232000   0  
    0 5  Y  deviceless  
no_connection_id
     media_ldb_root_clk 1   1    0    519232000   0 
 0 5  Y 32ec.blk-ctrl:bridge@5c 
ldb
    
   deviceless  
no_connection_id
  media_disp1_pix  0   0    0    129808000   0  
    0 5  N  deviceless  
no_connection_id
     media_disp1_pix_root_clk 0   0    0    
129808000   0  0 5  N 
32e8.display-controller pix
    
   32ec.blk-ctrl   
disp1
    
   deviceless  
no_connection_id
  media_disp2_pix  1   1    0    519232000   0  
    0 5  Y  deviceless  
no_connection_id
     media_disp2_pix_root_clk 1   1    0    
519232000   0  0 5  Y 
32e9.display-controller pix
    
   32ec.blk-ctrl   
disp2
    
   deviceless  
no_connection_id

Single LVDS link mode is not affected because "media_disp2_pix"
clock can handle the 7 division ratio.

To support the dual LVDS link mode, "video_pll1" clock rate needs
to be x2 "media_l

Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 10/21/24 1:48 PM, Maxime Ripard wrote:

On Sun, Oct 20, 2024 at 04:49:29AM +0200, Marek Vasut wrote:

On 10/19/24 11:49 PM, Kieran Bingham wrote:

Quoting Marek Vasut (2024-10-12 21:37:59)

On 10/11/24 5:10 AM, Liu Ying wrote:

Hi,


This Video PLL1 configuration since moved to &media_blk_ctrl {} , but it is 
still in the imx8mp.dtsi . Therefore, to make your panel work at the correct 
desired pixel clock frequency instead of some random one inherited from 
imx8mp.dtsi, add the following to the pollux DT, I believe that will fix the 
problem and is the correct fix:

&media_blk_ctrl {
       // 50680 = 7240 * 7 (for single-link LVDS, this is enough)
       // there is no need to multiply the clock by * 2
       assigned-clock-rates = <5>, <2>, <0>, <0>, <5>, 
<50680>;


This assigns "video_pll1" clock rate to 506.8MHz which is currently not
listed in imx_pll1443x_tbl[].


Since commit b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic rates") the 
1443x PLLs can be configured to arbitrary rates which for video PLL is desirable as those 
should produce accurate clock.


Ack.




Does the below patch[1] fix the regression issue? It explicitly sets
the clock frequency of the panel timing to 74.25MHz.

[1] https://patchwork.freedesktop.org/patch/616905/?series=139266&rev=1

That patch is wrong, there is an existing entry for this panel in 
panel-simple.c which is correct and precise, please do not add that kind of 
imprecise duplicate timings into DT.


At least the patch[1] is legitimate now to override the display
timing of the panel because the override mode is something
panel-simple.c supports.


It may be possible to override the mode, but why would this be the
desired if the panel-simple.c already contains valid accurate timings
for this particular panel ?


I'm confused a little here. Why is it that setting the panel timings in
the DT as per [1] make the display work, while the panel timeings in
panel-simple alone are not enough?

Is there some difference in code path for 'how' the panel timings are
set as to whether they will apply fully or not ?

Because [1] sets inaccurate pixel clock of 74.25 MHz, which can be divided
down from random default Video PLL setting of 1039.5 MHz set in imx8mp.dtsi
media_blk_ctrl , 1039.5 / 74.25 = 14 .

The panel-simple pixel clock are 72.4 MHz, to achieve that accurately, it is
necessary to reconfigure the Video PLL frequency to 506.8 MHz , which
LCDIFv3 can do, but LDB can not, hence it has to be done in DT for now.


That the clock driver is broken and thus should be fixed through the DT is a
bit backward, don't you think?


See these two patches, they might address that part for next cycle:

clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate
drm: bridge: ldb: Configure LDB clock in .mode_set

For this cycle, fixing up the frequency that is already incorrectly set 
in imx8mp.dtsi in board DT is the least impact approach, see


arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1 frequency to 506.8 MHz


AFAIU, the clock can't reach the ideal pixel clock panel-simple has. Do
you have the datasheet for that panel?


No


If so, using display_timings and shortening/extending the blanking
timings to match the clock that can be provided seems like a more robust
solution.

The PLL has to be configured correctly, see the two patches listed above.

Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 10/21/24 1:10 PM, Kieran Bingham wrote:

Quoting Marek Vasut (2024-10-20 03:49:29)

On 10/19/24 11:49 PM, Kieran Bingham wrote:

Quoting Marek Vasut (2024-10-12 21:37:59)

On 10/11/24 5:10 AM, Liu Ying wrote:

Hi,


This Video PLL1 configuration since moved to &media_blk_ctrl {} , but it is 
still in the imx8mp.dtsi . Therefore, to make your panel work at the correct 
desired pixel clock frequency instead of some random one inherited from 
imx8mp.dtsi, add the following to the pollux DT, I believe that will fix the 
problem and is the correct fix:

&media_blk_ctrl {
       // 50680 = 7240 * 7 (for single-link LVDS, this is enough)
       // there is no need to multiply the clock by * 2
       assigned-clock-rates = <5>, <2>, <0>, <0>, <5>, 
<50680>;


This assigns "video_pll1" clock rate to 506.8MHz which is currently not
listed in imx_pll1443x_tbl[].


Since commit b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic rates") the 
1443x PLLs can be configured to arbitrary rates which for video PLL is desirable as those 
should produce accurate clock.


Ack.




Does the below patch[1] fix the regression issue? It explicitly sets
the clock frequency of the panel timing to 74.25MHz.

[1] https://patchwork.freedesktop.org/patch/616905/?series=139266&rev=1

That patch is wrong, there is an existing entry for this panel in 
panel-simple.c which is correct and precise, please do not add that kind of 
imprecise duplicate timings into DT.


At least the patch[1] is legitimate now to override the display
timing of the panel because the override mode is something
panel-simple.c supports.


It may be possible to override the mode, but why would this be the
desired if the panel-simple.c already contains valid accurate timings
for this particular panel ?


I'm confused a little here. Why is it that setting the panel timings in
the DT as per [1] make the display work, while the panel timeings in
panel-simple alone are not enough?

Is there some difference in code path for 'how' the panel timings are
set as to whether they will apply fully or not ?

Because [1] sets inaccurate pixel clock of 74.25 MHz, which can be
divided down from random default Video PLL setting of 1039.5 MHz set in
imx8mp.dtsi media_blk_ctrl , 1039.5 / 74.25 = 14 .

The panel-simple pixel clock are 72.4 MHz, to achieve that accurately,
it is necessary to reconfigure the Video PLL frequency to 506.8 MHz ,
which LCDIFv3 can do, but LDB can not, hence it has to be done in DT for
now.


Aha - right - Thanks, I'd missed the part that 74.25 MHz /is not/ the
correct or supported pixel clock for the panel


It is inaccurate, it is still within the spec ...


, so it just becomes
'close enough' and lucky that it works...


... but only by sheer chance, because the Video PLL in imx8mp.dtsi is 
accidentally set to frequency that is just close enough to be divisible 
to the inaccurate 74.25 MHz .



Now I understand how your proposed :


&media_blk_ctrl {
       // 50680 = 7240 * 7 (for single-link LVDS, this is enough)
       // there is no need to multiply the clock by * 2
       assigned-clock-rates = <5>, <2>, <0>, <0>, <5>, 
<50680>;


This assigns "video_pll1" clock rate to 506.8MHz which is currently not
listed in imx_pll1443x_tbl[].


is more accurate. But is that acceptable for DT ? To just hardcode
clocks like that?


I am not happy about it, but it is the best we can do right now.

See these two patches, they might address that part for next cycle:

clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate
drm: bridge: ldb: Configure LDB clock in .mode_set


Or is this something we'll then remove when the additional patches make
it upstream? (I'm curious on the basis that I thought we treat DT as
'firmware' so if we put this in we expect it to be there forever?)


It is already there in imx8mp.dtsi , so we are not making the situation 
any worse, rather the opposite.



All of this seems like perhaps it should be in an overlay for the
display too - but given this board comes with this panel as a kit - I
suspect it's fine to keep it all there.
That's a separate topic, but yes, DTOs for displays are a good thing to 
have.

Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 10/19/24 11:49 PM, Kieran Bingham wrote:

Quoting Marek Vasut (2024-10-12 21:37:59)

On 10/11/24 5:10 AM, Liu Ying wrote:

Hi,


This Video PLL1 configuration since moved to &media_blk_ctrl {} , but it is 
still in the imx8mp.dtsi . Therefore, to make your panel work at the correct 
desired pixel clock frequency instead of some random one inherited from 
imx8mp.dtsi, add the following to the pollux DT, I believe that will fix the 
problem and is the correct fix:

&media_blk_ctrl {
      // 50680 = 7240 * 7 (for single-link LVDS, this is enough)
      // there is no need to multiply the clock by * 2
      assigned-clock-rates = <5>, <2>, <0>, <0>, <5>, 
<50680>;


This assigns "video_pll1" clock rate to 506.8MHz which is currently not
listed in imx_pll1443x_tbl[].


Since commit b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic rates") the 
1443x PLLs can be configured to arbitrary rates which for video PLL is desirable as those 
should produce accurate clock.


Ack.




Does the below patch[1] fix the regression issue? It explicitly sets
the clock frequency of the panel timing to 74.25MHz.

[1] https://patchwork.freedesktop.org/patch/616905/?series=139266&rev=1

That patch is wrong, there is an existing entry for this panel in 
panel-simple.c which is correct and precise, please do not add that kind of 
imprecise duplicate timings into DT.


At least the patch[1] is legitimate now to override the display
timing of the panel because the override mode is something
panel-simple.c supports.


It may be possible to override the mode, but why would this be the
desired if the panel-simple.c already contains valid accurate timings
for this particular panel ?


I'm confused a little here. Why is it that setting the panel timings in
the DT as per [1] make the display work, while the panel timeings in
panel-simple alone are not enough?

Is there some difference in code path for 'how' the panel timings are
set as to whether they will apply fully or not ?
Because [1] sets inaccurate pixel clock of 74.25 MHz, which can be 
divided down from random default Video PLL setting of 1039.5 MHz set in 
imx8mp.dtsi media_blk_ctrl , 1039.5 / 74.25 = 14 .


The panel-simple pixel clock are 72.4 MHz, to achieve that accurately, 
it is necessary to reconfigure the Video PLL frequency to 506.8 MHz , 
which LCDIFv3 can do, but LDB can not, hence it has to be done in DT for 
now.

Re: [PATCH 3/5] arm64: dts: imx8mp-msc-sm2s-ep1: Add HDMI connector

[Marek Vasut]

On 10/18/24 11:00 AM, Liu Ying wrote:

On 10/18/2024, Marek Vasut wrote:

On 10/18/24 8:48 AM, Liu Ying wrote:

Add a HDMI connector to connect with i.MX8MP HDMI TX output.
This is a preparation for making the i.MX8MP LCDIF driver use
drm_bridge_connector which requires the DRM_BRIDGE_ATTACH_NO_CONNECTOR
flag.  With that flag, the DW HDMI bridge core driver would
try to attach the next bridge which is the HDMI connector.

Signed-off-by: Liu Ying 
---
   .../dts/freescale/imx8mp-msc-sm2s-ep1.dts | 19 +++
   1 file changed, 19 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s-ep1.dts 
b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s-ep1.dts
index 83194ea7cb81..b776646a258a 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s-ep1.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s-ep1.dts
@@ -15,6 +15,17 @@ / {
    "avnet,sm2s-imx8mp-14N0600E", "avnet,sm2s-imx8mp",
    "fsl,imx8mp";
   +    hdmi-connector {
+    compatible = "hdmi-connector";
+    type = "a";

Shouldn't this also have a 'label' property ?


'label' property is not required by hdmi-connector.yaml and there
are in-tree hdmi-connector nodes that haven't got it.
I tried to find schematics for the board online, but failed.
I don't have the board to see the label printed in silk layer, either.

If anyone can provide a valid label name, I may add it.

For the Kontron board, Frieder might be able to look it up for you ?

For the MSC one, hmmm, I am not sure. Anyone ?

Re: [PATCH 3/5] arm64: dts: imx8mp-msc-sm2s-ep1: Add HDMI connector

[Marek Vasut]

On 10/18/24 8:48 AM, Liu Ying wrote:

Add a HDMI connector to connect with i.MX8MP HDMI TX output.
This is a preparation for making the i.MX8MP LCDIF driver use
drm_bridge_connector which requires the DRM_BRIDGE_ATTACH_NO_CONNECTOR
flag.  With that flag, the DW HDMI bridge core driver would
try to attach the next bridge which is the HDMI connector.

Signed-off-by: Liu Ying 
---
  .../dts/freescale/imx8mp-msc-sm2s-ep1.dts | 19 +++
  1 file changed, 19 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s-ep1.dts 
b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s-ep1.dts
index 83194ea7cb81..b776646a258a 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s-ep1.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s-ep1.dts
@@ -15,6 +15,17 @@ / {
 "avnet,sm2s-imx8mp-14N0600E", "avnet,sm2s-imx8mp",
 "fsl,imx8mp";
  
+	hdmi-connector {

+   compatible = "hdmi-connector";
+   type = "a";

Shouldn't this also have a 'label' property ?

Re: [PATCH] drm/bridge: tc358767: fix missing of_node_put() in for_each_endpoint_of_node()

[Marek Vasut]

On 10/13/24 8:11 PM, Javier Carrasco wrote:

for_each_endpoint_of_node() requires a call to of_node_put() for every
early exit. A new error path was added to the loop without observing
this requirement.

Add the missing call to of_node_put() in the error path.

Fixes: 1fb4dceeedc5 ("drm/bridge: tc358767: Add configurable default 
preemphasis")
Signed-off-by: Javier Carrasco 
---
  drivers/gpu/drm/bridge/tc358767.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 159c95b26d33..942fbaa1413a 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -2405,6 +2405,7 @@ static int tc_probe_bridge_endpoint(struct tc_data *tc)
if (tc->pre_emphasis[0] < 0 || tc->pre_emphasis[0] > 2 
||
tc->pre_emphasis[1] < 0 || tc->pre_emphasis[1] > 2) 
{
dev_err(dev, "Incorrect Pre-Emphasis setting, use 
either 0=0dB 1=3.5dB 2=6dB\n");
+   of_node_put(node);
return -EINVAL;

Right, thanks!

Reviewed-by: Marek Vasut 

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Marek Vasut]

On 10/11/24 8:49 AM, Liu Ying wrote:

On 10/11/2024, Marek Vasut wrote:

On 10/10/24 9:15 AM, Liu Ying wrote:

On 10/09/2024, Marek Vasut wrote:

The LDB serializer clock operate at either x7 or x14 rate of the input


Isn't it either x7 or 3.5x?


Is it 3.5 for the dual-link LVDS ?


Yes.

static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
{
 if (fsl_ldb_is_dual(fsl_ldb))
 return clock * 3500;
 else
 return clock * 7000;
}


I don't have such a panel right now to test.


You can add a panel DT node for test to see the relationship
between the clocks, without a real dual-link LVDS panel.


I'll take your word for this.


[...]


diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 0e4bac7dd04ff..a3a31467fcc85 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -278,6 +278,16 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
   return MODE_OK;
   }
   +static void fsl_ldb_mode_set(struct drm_bridge *bridge,
+   const struct drm_display_mode *mode,
+   const struct drm_display_mode *adj)
+{
+    struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
+    unsigned long requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, 
mode->clock);
+
+    clk_set_rate(fsl_ldb->clk, requested_link_freq);


The mode_set callback won't be called when only crtc_state->active
is changed from false to true in an atomic commit, e.g., blanking
the emulated fbdev first and then unblanking it.  So, in this case,
the clk_set_rate() in fsl_ldb_atomic_enable() is still called after
those from mxsfb_kms or lcdif_kms.

Also, it doesn't look neat to call clk_set_rate() from both mode_set
callback and atomic_enable callback.


I agree the mode_set callback is not the best place for this.
Do you know of a better callback where to do this ? I couldn't find one.


A wild idea is to change the order between the CRTC atomic_enable
callback and the bridge one by implementing your own
atomic_commit_tail...  I don't think there is any place to do this
other than atomic_enable callback.


I will give that a try, thanks.


Anyway, I don't think it is necessary to manage the clk_set_rate()
function calls between this driver and mxsfb_kms or lcdif_kms
because "video_pll1" clock rate is supposed to be assigned in DT...


I disagree with this part. I believe the assignment of clock in DT is 
only a temporary workaround which should be removed. The drivers should 
be able to figure out and set the clock tree configuration.



The idea is to assign a reasonable PLL clock rate in DT to make
display drivers' life easier, especially for i.MX8MP where LDB,
Samsung MIPI DSI may use a single PLL at the same time.

I would really like to avoid setting arbitrary clock in DT, esp. if it can be 
avoided. And it surely can be avoided for this simple use case.


... just like I said in patch 1/2, "video_pll1" clock rate needs
to be x2 "media_ldb" clock rate for dual LVDS link mode. Without
an assigned "video_pll1" clock rate in DT, this driver cannot
achieve that.


This is something the LDB driver can infer from DT and configure the 
clock tree accordingly.



And, the i.MX8MP LDB + Samsung MIPI DSI case is
not simple considering using one single PLL and display modes
read from EDID.
You could use separate PLLs for each LCDIF scanout engine in such a 
deployment, I already ran into that, so I am aware of it. That is 
probably the best way out of such a problem, esp. if accurate pixel 
clock are the requirement.


[...]

Re: [PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate

[Marek Vasut]

On 10/11/24 8:18 AM, Liu Ying wrote:

On 10/11/2024, Marek Vasut wrote:

On 10/10/24 7:22 AM, Liu Ying wrote:

On 10/09/2024, Marek Vasut wrote:

The media_ldb_root_clk supply LDB serializer. These clock are usually
shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
results in accurate serializer clock.

Signed-off-by: Marek Vasut 
---
Cc: Abel Vesa 
Cc: Andrzej Hajda 
Cc: David Airlie 
Cc: Fabio Estevam 
Cc: Isaac Scott 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Michael Turquette 
Cc: Neil Armstrong 
Cc: Peng Fan 
Cc: Pengutronix Kernel Team 
Cc: Robert Foss 
Cc: Sascha Hauer 
Cc: Shawn Guo 
Cc: Simona Vetter 
Cc: Stephen Boyd 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: i...@lists.linux.dev
Cc: ker...@dh-electronics.com
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-...@vger.kernel.org
---
   drivers/clk/imx/clk-imx8mp.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 516dbd170c8a3..2e61d340b8ab7 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
   hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = 
imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, 
ccm_base + 0xbd80);
   hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = 
imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, 
ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
   hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = 
imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 
0xbe80);
-    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", 
imx8mp_media_ldb_sels, ccm_base + 0xbf00);
+    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", 
imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);


This patch would cause the below in-flight LDB bridge driver
patch[1] fail to do display mode validation upon display modes
read from LVDS to HDMI converter IT6263's DDC I2C bus.


Why ?


Mode validation is affected only for dual LVDS link mode.
For single LVDS link mode, this patch does open more display
modes read from the DDC I2C bus.  The reason behind is that
LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
link mode, while it's 7 for single LVDS link mode.

In my system, "video_pll1" clock rate is assigned to 1.0395GHz
in imx8mp.dtsi.  For 1920x1080-60.00Hz with 148.5MHz pixel
clock rate, "media_ldb" clock rate is 519.75MHz and
"media_disp2_pix" clock rate is 148.5MHz, which is fine for
dual LVDS link mode(x3.5).  For newly opened up 1920x1080-59.94Hz
with 148.352MHz pixel clock rate, "video_pll1" clock rate will
be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
too because "media_disp2_pix" clock cannot handle the 3.5
division ratio from "video_pll1_out" clock running at
519.232MHz.  See the below clk_summary.


Shouldn't this patch help exactly with that ?

It should allow you to set video_pll1_out to whatever is necessary by 
LDB first, fixate that frequency, and the LCDIFv3 would then be forced 
to use /7 divider from faster Video PLL1 , right ?



 video_pll1_ref_sel   1   1024000   
   0 5  Y  deviceless  no_connection_id
video_pll11   10519232000   0   
   0 5  Y deviceless  
no_connection_id
   video_pll1_bypass  1   10519232000   0   
   0 5  Ydeviceless  
no_connection_id
  video_pll1_out  2   20519232000   0   
   0 5  Y   deviceless  
no_connection_id
 media_ldb1   10519232000   0   
   0 5  Y  deviceless  
no_connection_id
media_ldb_root_clk 1   10519232000   0  
0 5  Y 32ec.blk-ctrl:bridge@5c 
ldb

  deviceless  
no_connection_id
 media_disp1_pix  0   00129808000   0   
   0 5  N  deviceless  
no_connection_id
media

Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 10/11/24 5:10 AM, Liu Ying wrote:

Hi,


This Video PLL1 configuration since moved to &media_blk_ctrl {} , but it is 
still in the imx8mp.dtsi . Therefore, to make your panel work at the correct 
desired pixel clock frequency instead of some random one inherited from 
imx8mp.dtsi, add the following to the pollux DT, I believe that will fix the 
problem and is the correct fix:

&media_blk_ctrl {
     // 50680 = 7240 * 7 (for single-link LVDS, this is enough)
     // there is no need to multiply the clock by * 2
     assigned-clock-rates = <5>, <2>, <0>, <0>, <5>, 
<50680>;


This assigns "video_pll1" clock rate to 506.8MHz which is currently not
listed in imx_pll1443x_tbl[].


Since commit b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic rates") the 
1443x PLLs can be configured to arbitrary rates which for video PLL is desirable as those 
should produce accurate clock.


Ack.




Does the below patch[1] fix the regression issue? It explicitly sets
the clock frequency of the panel timing to 74.25MHz.

[1] https://patchwork.freedesktop.org/patch/616905/?series=139266&rev=1

That patch is wrong, there is an existing entry for this panel in 
panel-simple.c which is correct and precise, please do not add that kind of 
imprecise duplicate timings into DT.


At least the patch[1] is legitimate now to override the display
timing of the panel because the override mode is something
panel-simple.c supports.


It may be possible to override the mode, but why would this be the 
desired if the panel-simple.c already contains valid accurate timings 
for this particular panel ?



And, pixel clock @74.25MHz is not out
of the panel specification since edt_etml1010g3dra_timing
indicates the minimum as 66.3MHz and the maximum as 78.9MHz.


The panel-simple.c already contains timings for this panel, which are 
accurate and follow the panel datasheet. If the goal here is to support 
approximate panel timings between the currently available three options 
(min/typ/max) listed in panel-simple, then that is another discussion 
for another patch.



Furthermore, if "PHYTEC phyBOARD-Pollux i.MX8MP" also supports
something like MIPI DSI to HDMI, then 74.25MHz panel pixel clock
rate is more desirable because the LVDS display and the MIPI DSI
display pipeline with typical 148.5MHz/74.25MHz pixel clock rates
may use one single "video_pll1" clock.


I actually do have exactly this use case on one system -- one LVDS panel 
and one MIPI DSI panel -- the solution is really easy, source the LVDS 
pixel clock from Video PLL and the DSI clock from e.g. PLL3 .



Anyway, I think it is ok to use the patch[1] or assigning
"video_pll1" clock rate to 506.8MHz in DT(no things like MIPI
DSI to HDMI in existing DT).
I believe for the current release, it is better to update the Video PLL 
clock in this one board DT, because that is minimum impact change 
isolated to this board and fixes a real issue where the LVDS panel 
worked within specification only by sheer chance.

Re: [PATCH v2 2/9] arm64: dts: imx8mp-phyboard-pollux-rdk: Add panel-timing node to panel-lvds node

[Marek Vasut]

On 10/12/24 9:35 AM, Liu Ying wrote:

Add a panel-timing node to panel-lvds node to override any fixed
display modes written in a panel driver.  This way, 74.25MHz clock
frequency specified in panel-timing node may accommodate 7-fold
519.75MHz "media_ldb" clock which is derived from 1.0395GHz
"video_pll1" clock.

This should suppress this LDB driver warning:
[   17.923709] fsl-ldb 32ec.blk-ctrl:bridge@5c: Configured LDB clock 
(7240 Hz) does not match requested LVDS clock: 50680 Hz

This also makes the display mode used by the panel pass mode validation
against pixel clock rate and "media_ldb" clock rate in a certain display
driver.

Fixes: 326d86e197fc ("arm64: dts: imx8mp-phyboard-pollux-rdk: add etml panel 
support")
Signed-off-by: Liu Ying 
---
v2:
* No change.

  .../dts/freescale/imx8mp-phyboard-pollux-rdk.dts  | 15 +++
  1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts 
b/arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts
index 50debe821c42..20cb5363cccb 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts
@@ -37,6 +37,21 @@ panel1_lvds: panel-lvds {
backlight = <&backlight_lvds>;
power-supply = <®_vcc_3v3_sw>;
  
+		panel-timing {

+   clock-frequency = <7425>;
+   hactive = <1280>;
+   vactive = <800>;
+   hfront-porch = <72>;
+   hback-porch = <86>;
+   hsync-len = <2>;
+   vfront-porch = <15>;
+   vback-porch = <21>;
+   vsync-len = <2>;
+   hsync-active = <0>;
+   vsync-active = <0>;
+   de-active = <1>;
+   };
There is an existing entry for this panel in panel-simple.c , please do 
not duplicate timings in the DT:


drivers/gpu/drm/panel/panel-simple.c:static const struct panel_desc 
edt_etml1010g3dra = {
drivers/gpu/drm/panel/panel-simple.c:   .timings = 
&edt_etml1010g3dra_timing,
drivers/gpu/drm/panel/panel-simple.c:   .compatible = 
"edt,etml1010g3dra",

Re: [PATCH v2 1/9] arm64: dts: imx8mp-skov-revb-mi1010ait-1cp1: Add panel-timing node to panel node

[Marek Vasut]

On 10/12/24 9:35 AM, Liu Ying wrote:

Add a panel-timing node to panel node to override any fixed display
modes written in a panel driver.  This way, 68.9MHz clock frequency
specified in panel-timing node may accommodate 7-fold 482.3MHz
"media_ldb" clock which is derived from 964.6MHz "video_pll1" clock.
The above clock frequencies align to the clock rates assigned in the
lvds_bridge node and media_blk_ctrl node in this DT file.

This should be able to suppress this LDB driver warning:
[   17.206644] fsl-ldb 32ec.blk-ctrl:bridge@5c: Configured LDB clock 
(7000 Hz) does not match requested LVDS clock: 49000 Hz

This also makes the display mode used by the panel pass mode validation
against pixel clock rate and "media_ldb" clock rate in a certain display
driver.

Fixes: 6d382d51d979 ("arm64: dts: freescale: Add SKOV IMX8MP CPU revB board")
Signed-off-by: Liu Ying 
---
v2:
* No change.

  .../freescale/imx8mp-skov-revb-mi1010ait-1cp1.dts | 15 +++
  1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-skov-revb-mi1010ait-1cp1.dts 
b/arch/arm64/boot/dts/freescale/imx8mp-skov-revb-mi1010ait-1cp1.dts
index 3c2efdc59bfa..4e9f76de7462 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-skov-revb-mi1010ait-1cp1.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-skov-revb-mi1010ait-1cp1.dts
@@ -13,6 +13,21 @@ panel {
backlight = <&backlight>;
power-supply = <®_tft_vcom>;
  
+		panel-timing {

+   clock-frequency = <6890>;
+   hactive = <1280>;
+   vactive = <800>;
+   hfront-porch = <30>;
+   hback-porch = <30>;
+   hsync-len = <10>;
+   vfront-porch = <5>;
+   vback-porch = <5>;
+   vsync-len = <5>;
+   hsync-active = <0>;
+   vsync-active = <0>;
+   de-active = <1>;
+   };
There is an existing entry for this panel in panel-simple.c , please do 
not duplicate timings in the DT:


drivers/gpu/drm/panel/panel-simple.c:   .timings = 
&multi_inno_mi1010ait_1cp_timing,
drivers/gpu/drm/panel/panel-simple.c:   .compatible = 
"multi-inno,mi1010ait-1cp",
drivers/gpu/drm/panel/panel-simple.c:   .data = 
&multi_inno_mi1010ait_1cp,

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Marek Vasut]

On 10/10/24 9:15 AM, Liu Ying wrote:

On 10/09/2024, Marek Vasut wrote:

The LDB serializer clock operate at either x7 or x14 rate of the input


Isn't it either x7 or 3.5x?


Is it 3.5 for the dual-link LVDS ?
I don't have such a panel right now to test.

[...]


diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 0e4bac7dd04ff..a3a31467fcc85 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -278,6 +278,16 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
return MODE_OK;
  }
  
+static void fsl_ldb_mode_set(struct drm_bridge *bridge,

+  const struct drm_display_mode *mode,
+  const struct drm_display_mode *adj)
+{
+   struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
+   unsigned long requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, 
mode->clock);
+
+   clk_set_rate(fsl_ldb->clk, requested_link_freq);


The mode_set callback won't be called when only crtc_state->active
is changed from false to true in an atomic commit, e.g., blanking
the emulated fbdev first and then unblanking it.  So, in this case,
the clk_set_rate() in fsl_ldb_atomic_enable() is still called after
those from mxsfb_kms or lcdif_kms.

Also, it doesn't look neat to call clk_set_rate() from both mode_set
callback and atomic_enable callback.


I agree the mode_set callback is not the best place for this.
Do you know of a better callback where to do this ? I couldn't find one.


The idea is to assign a reasonable PLL clock rate in DT to make
display drivers' life easier, especially for i.MX8MP where LDB,
Samsung MIPI DSI may use a single PLL at the same time.
I would really like to avoid setting arbitrary clock in DT, esp. if it 
can be avoided. And it surely can be avoided for this simple use case.

Re: [PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate

[Marek Vasut]

On 10/10/24 7:22 AM, Liu Ying wrote:

On 10/09/2024, Marek Vasut wrote:

The media_ldb_root_clk supply LDB serializer. These clock are usually
shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
results in accurate serializer clock.

Signed-off-by: Marek Vasut 
---
Cc: Abel Vesa 
Cc: Andrzej Hajda 
Cc: David Airlie 
Cc: Fabio Estevam 
Cc: Isaac Scott 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Michael Turquette 
Cc: Neil Armstrong 
Cc: Peng Fan 
Cc: Pengutronix Kernel Team 
Cc: Robert Foss 
Cc: Sascha Hauer 
Cc: Shawn Guo 
Cc: Simona Vetter 
Cc: Stephen Boyd 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: i...@lists.linux.dev
Cc: ker...@dh-electronics.com
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-...@vger.kernel.org
---
  drivers/clk/imx/clk-imx8mp.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 516dbd170c8a3..2e61d340b8ab7 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = 
imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, 
ccm_base + 0xbd80);
hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = 
imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, 
ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = 
imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 
0xbe80);
-   hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", 
imx8mp_media_ldb_sels, ccm_base + 0xbf00);
+   hws[IMX8MP_CLK_MEDIA_LDB] = 
imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 
0xbf00, CLK_SET_RATE_PARENT);


This patch would cause the below in-flight LDB bridge driver
patch[1] fail to do display mode validation upon display modes
read from LVDS to HDMI converter IT6263's DDC I2C bus.


Why ?

Also, please Cc me on fsl-ldb.c patches.


Unsupported display modes cannot be ruled out.  Note that
"media_ldb" is derived from "video_pll1_out" by default as the
parent is set in imx8mp.dtsi.  And, the only 4 rates supported
by "video_pll1" are listed in imx_pll1443x_tbl[] - 1.0395GHz,
650MHz, 594MHz and 519.75MHz.
I disagree with this part, since commit b09c68dc57c9 ("clk: imx: 
pll14xx: Support dynamic rates") the 1443x PLLs can be configured to 
arbitrary rates which for video PLL is desirable as those should produce 
accurate clock.

Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 10/10/24 7:31 AM, Liu Ying wrote:

Hi,


This Video PLL1 configuration since moved to &media_blk_ctrl {} , but it is 
still in the imx8mp.dtsi . Therefore, to make your panel work at the correct 
desired pixel clock frequency instead of some random one inherited from 
imx8mp.dtsi, add the following to the pollux DT, I believe that will fix the 
problem and is the correct fix:

&media_blk_ctrl {
    // 50680 = 7240 * 7 (for single-link LVDS, this is enough)
    // there is no need to multiply the clock by * 2
    assigned-clock-rates = <5>, <2>, <0>, <0>, <5>, 
<50680>;


This assigns "video_pll1" clock rate to 506.8MHz which is currently not
listed in imx_pll1443x_tbl[].


Since commit b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic rates") 
the 1443x PLLs can be configured to arbitrary rates which for video PLL 
is desirable as those should produce accurate clock.



Does the below patch[1] fix the regression issue? It explicitly sets
the clock frequency of the panel timing to 74.25MHz.

[1] https://patchwork.freedesktop.org/patch/616905/?series=139266&rev=1
That patch is wrong, there is an existing entry for this panel in 
panel-simple.c which is correct and precise, please do not add that kind 
of imprecise duplicate timings into DT.


[...]

Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 10/9/24 5:58 PM, Isaac Scott wrote:

[...]


  media_mipi_phy1_ref  0   0    0
23036364
0  0 5  N  deviceless
no_co
     media_mipi_phy1_ref_root 0   0    0
23036364    0  0 5  N
32ec.blk-ctrl

The media_disp2_pix clock now seems to be correct at 72400
after
your changes.


Do you want to submit the DT patch with correct Fixes: tag ? :)


I thought this wasn't needed with the other two patches?
The DT change is not needed with the other patches, however, I believe 
the DT change is the correct fix for current Linux 6.12-rc and the two 
patches are feature development for the Linux 6.13.y cycle.

Re: [PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate

[Marek Vasut]

On 10/9/24 1:40 PM, Abel Vesa wrote:

On 24-10-09 00:38:19, Marek Vasut wrote:

The media_ldb_root_clk supply LDB serializer. These clock are usually
shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
results in accurate serializer clock.

Signed-off-by: Marek Vasut 


Any fixes tag needed ?
I now replied to 2/2 , I think this is feature development and should be 
applied to 6.13 cycle only. I would like to get input from Isaac on 
whether the DT fix I suggested to them in the original bug report also 
works, and if so, that should possibly be the Fixes: patch for 6.12 .

Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 10/9/24 11:55 AM, Isaac Scott wrote:

On Tue, 2024-10-08 at 23:48 +0200, Marek Vasut wrote:

On 10/8/24 12:07 PM, Isaac Scott wrote:

On Mon, 2024-10-07 at 20:06 +0200, Marek Vasut wrote:

On 10/7/24 7:01 PM, Isaac Scott wrote:

Hi Marek,


Hi,


On Sat, 2024-07-06 at 02:16 +0200, Marek Vasut wrote:

On 6/24/24 11:19 AM, Alexander Stein wrote:

Am Freitag, 31. Mai 2024, 22:27:21 CEST schrieb Marek
Vasut:

In case an upstream bridge modified the required clock
frequency
in its .atomic_check callback by setting
adjusted_mode.clock
,
make sure that clock frequency is generated by the
LCDIFv3
block.

This is useful e.g. when LCDIFv3 feeds DSIM which feeds
TC358767
with (e)DP output, where the TC358767 expects precise
timing
on
its input side, the precise timing must be generated by
the
LCDIF.

Signed-off-by: Marek Vasut 


With the other rc358767 patches in place, this does the
trick.
Reviewed-by: Alexander Stein



I'll pick this up next week if there is no objection.


Unfortunately, this has caused a regression that is present in
v6.12-
rc1 on the i.MX8MP PHYTEC Pollux using the
arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts.
The
display is the edt,etml1010g3dra panel, as per the upstream
dts. We
bisected to this commit, and reverting this change fixed the
screen.

We then tried to retest this on top of v6.12-rc2, and found we
also
had
to revert commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155
("clk:
imx:
clk-imx8mp: Allow media_disp pixel clock reconfigure parent
rate")
alongside this. Reverting these two commits makes the display
work
again at -rc2.

Do you have any suggestions on anything we might be missing on
our
end?
Please let me know if there's anything you'd like me to test as
I'm
not
sure what the underlying fault was here.

I believe what is going on is that the LCDIF cannot configure its
upstream clock because something else is already using those
clock
and
it set those clock to a specific frequency. LCDIF is now trying
to
configure those clock to match the LVDS panel, and it fails, so
it
tries
to set some approximate clock and that is not good enough for the
LVDS
panel.

Can you share dump of /sys/kernel/debug/clk/clk_summary on
failing
and
working system ? You might see the difference around the "video"
clock.

(I have seen this behavior before, the fix was usually a matter
of
moving one of the LCDIFs to another upstream clock like PLL3, so
it
can
pick well matching output clock instead of some horrid
approximation
which then drives the panel likely out of specification)


Hi Marek,

Please find attached the clk_summary for v6.12-rc2 before and after
the
reversion (the one after the reversion is 6.12-
rc2_summary_postfix).

Thank you, this helped greatly.

I believe I know why it used to kind-of work for you, but I'm afraid
this used to work by sheer chance and it does not really work
correctly
for the panel you use, even if the panel likely does show the correct
content. But, there is a way to make it work properly for the panel
you use.

First of all, the pixel clock never really matched the panel-simple.c
pixel clock for the edt_etml1010g3dra_timing:

$ grep '\' 6.12-rc2_summary_postfix
    media_disp2_pix  1  1  0  7425 ...
  

$ grep -A 1 edt_etml1010g3dra_timing drivers/gpu/drm/panel/panel-
simple.c
static const struct display_timing edt_etml1010g3dra_timing = {
  .pixelclock = { 6630, 7240, 7890 },
    

The pixel clock are within tolerance, but there is a discrepancy
7425 != 7240 .

Since commit 94e6197dadc9 ("arm64: dts: imx8mp: Add LCDIF2 & LDB
nodes")
the IMX8MP_VIDEO_PLL1_OUT is set to a very specific frequency of
103950 Hz, which tidily divides by 2 to 51975 Hz (which is
your
LVDS serializer frequency) and divides by 7 to 7425 Hz which is
your
LCDIF pixel clock.

This Video PLL1 configuration since moved to &media_blk_ctrl {} , but
it
is still in the imx8mp.dtsi . Therefore, to make your panel work at
the
correct desired pixel clock frequency instead of some random one
inherited from imx8mp.dtsi, add the following to the pollux DT, I
believe that will fix the problem and is the correct fix:

&media_blk_ctrl {
     // 50680 = 7240 * 7 (for single-link LVDS, this is
enough)
     // there is no need to multiply the clock by * 2
     assigned-clock-rates = <5>, <2>, <0>, <0>,
<5>, <50680>;
};

Can you please test whether this works and the pixel clock are
accurate
in /sys/kernel/debug/clk/clk_summary ?


Interestingly, after making the change you suggested to imx8mp-
phyboard-pollux-rdk.dts before the two reversions, the display now
seems to work. Please see below for the relevant section of the new
clk_summary referring to media_disp2_pix:

video_pll1_ref_sel   1   10  

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Marek Vasut]

On 10/9/24 12:27 PM, Isaac Scott wrote:

On Wed, 2024-10-09 at 00:38 +0200, Marek Vasut wrote:

The LDB serializer clock operate at either x7 or x14 rate of the
input
LCDIFv3 scanout engine clock. Make sure the serializer clock and
their
upstream Video PLL are configured early in .mode_set to the x7 or x14
rate of pixel clock, before LCDIFv3 .atomic_enable is called which
would
configure the Video PLL to low x1 rate, which is unusable.

With this patch in place, the clock tree is correctly configured. The
example below is for a 71.1 MHz pixel clock panel, the LDB serializer
clock is then 497.7 MHz:


Awesome! Thank you for this, this seems to fix the regression and the
patches work as expected. I have tested both patches on v6.12-rc2 and
the display works well.

For both patches,

Tested-by: Isaac Scott 
Thank you for testing, but this patch feels too much like a feature 
development to me. Does the DT tweak I suggested also fix your issue? If 
so, I would really like that DT tweak to be the fix for current release 
and these two patches be feature development for 6.13 cycle. What do you 
think ?

[PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate

[Marek Vasut]

The media_ldb_root_clk supply LDB serializer. These clock are usually
shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
results in accurate serializer clock.

Signed-off-by: Marek Vasut 
---
Cc: Abel Vesa 
Cc: Andrzej Hajda 
Cc: David Airlie 
Cc: Fabio Estevam 
Cc: Isaac Scott 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Michael Turquette 
Cc: Neil Armstrong 
Cc: Peng Fan 
Cc: Pengutronix Kernel Team 
Cc: Robert Foss 
Cc: Sascha Hauer 
Cc: Shawn Guo 
Cc: Simona Vetter 
Cc: Stephen Boyd 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: i...@lists.linux.dev
Cc: ker...@dh-electronics.com
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-...@vger.kernel.org
---
 drivers/clk/imx/clk-imx8mp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 516dbd170c8a3..2e61d340b8ab7 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = 
imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, 
ccm_base + 0xbd80);
hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = 
imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, 
ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = 
imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 
0xbe80);
-   hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", 
imx8mp_media_ldb_sels, ccm_base + 0xbf00);
+   hws[IMX8MP_CLK_MEDIA_LDB] = 
imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 
0xbf00, CLK_SET_RATE_PARENT);
hws[IMX8MP_CLK_MEMREPAIR] = 
imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 
0xbf80);
hws[IMX8MP_CLK_MEDIA_MIPI_TEST_BYTE] = 
imx8m_clk_hw_composite("media_mipi_test_byte", 
imx8mp_media_mipi_test_byte_sels, ccm_base + 0xc100);
hws[IMX8MP_CLK_ECSPI3] = imx8m_clk_hw_composite("ecspi3", 
imx8mp_ecspi3_sels, ccm_base + 0xc180);
-- 
2.45.2

[PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Marek Vasut]

The LDB serializer clock operate at either x7 or x14 rate of the input
LCDIFv3 scanout engine clock. Make sure the serializer clock and their
upstream Video PLL are configured early in .mode_set to the x7 or x14
rate of pixel clock, before LCDIFv3 .atomic_enable is called which would
configure the Video PLL to low x1 rate, which is unusable.

With this patch in place, the clock tree is correctly configured. The
example below is for a 71.1 MHz pixel clock panel, the LDB serializer
clock is then 497.7 MHz:

video_pll1_ref_sel  1 1 0  2400 0 0 5
   video_pll1   1 1 0 49770 0 0 5
  video_pll1_bypass 1 1 0 49770 0 0 5
 video_pll1_out 2 2 0 49770 0 0 5
media_ldb   1 1 0 49770 0 0 5
   media_ldb_root_clk   1 1 0 49770 0 0 5
media_disp2_pix 1 1 0  7110 0 0 5
   media_disp2_pix_root_clk 1 1 0  7110 0 0 5

Signed-off-by: Marek Vasut 
---
Cc: Abel Vesa 
Cc: Andrzej Hajda 
Cc: David Airlie 
Cc: Fabio Estevam 
Cc: Isaac Scott 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Michael Turquette 
Cc: Neil Armstrong 
Cc: Peng Fan 
Cc: Pengutronix Kernel Team 
Cc: Robert Foss 
Cc: Sascha Hauer 
Cc: Shawn Guo 
Cc: Simona Vetter 
Cc: Stephen Boyd 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: i...@lists.linux.dev
Cc: ker...@dh-electronics.com
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-...@vger.kernel.org
---
 drivers/gpu/drm/bridge/fsl-ldb.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 0e4bac7dd04ff..a3a31467fcc85 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -278,6 +278,16 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
return MODE_OK;
 }
 
+static void fsl_ldb_mode_set(struct drm_bridge *bridge,
+  const struct drm_display_mode *mode,
+  const struct drm_display_mode *adj)
+{
+   struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
+   unsigned long requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, 
mode->clock);
+
+   clk_set_rate(fsl_ldb->clk, requested_link_freq);
+}
+
 static const struct drm_bridge_funcs funcs = {
.attach = fsl_ldb_attach,
.atomic_enable = fsl_ldb_atomic_enable,
@@ -287,6 +297,7 @@ static const struct drm_bridge_funcs funcs = {
.atomic_get_input_bus_fmts = fsl_ldb_atomic_get_input_bus_fmts,
.atomic_reset = drm_atomic_helper_bridge_reset,
.mode_valid = fsl_ldb_mode_valid,
+   .mode_set = fsl_ldb_mode_set,
 };
 
 static int fsl_ldb_probe(struct platform_device *pdev)
-- 
2.45.2

Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 10/8/24 12:07 PM, Isaac Scott wrote:

On Mon, 2024-10-07 at 20:06 +0200, Marek Vasut wrote:

On 10/7/24 7:01 PM, Isaac Scott wrote:

Hi Marek,


Hi,


On Sat, 2024-07-06 at 02:16 +0200, Marek Vasut wrote:

On 6/24/24 11:19 AM, Alexander Stein wrote:

Am Freitag, 31. Mai 2024, 22:27:21 CEST schrieb Marek Vasut:

In case an upstream bridge modified the required clock
frequency
in its .atomic_check callback by setting adjusted_mode.clock
,
make sure that clock frequency is generated by the LCDIFv3
block.

This is useful e.g. when LCDIFv3 feeds DSIM which feeds
TC358767
with (e)DP output, where the TC358767 expects precise timing
on
its input side, the precise timing must be generated by the
LCDIF.

Signed-off-by: Marek Vasut 


With the other rc358767 patches in place, this does the trick.
Reviewed-by: Alexander Stein 


I'll pick this up next week if there is no objection.


Unfortunately, this has caused a regression that is present in
v6.12-
rc1 on the i.MX8MP PHYTEC Pollux using the
arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts. The
display is the edt,etml1010g3dra panel, as per the upstream dts. We
bisected to this commit, and reverting this change fixed the
screen.

We then tried to retest this on top of v6.12-rc2, and found we also
had
to revert commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155 ("clk:
imx:
clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")
alongside this. Reverting these two commits makes the display work
again at -rc2.

Do you have any suggestions on anything we might be missing on our
end?
Please let me know if there's anything you'd like me to test as I'm
not
sure what the underlying fault was here.

I believe what is going on is that the LCDIF cannot configure its
upstream clock because something else is already using those clock
and
it set those clock to a specific frequency. LCDIF is now trying to
configure those clock to match the LVDS panel, and it fails, so it
tries
to set some approximate clock and that is not good enough for the
LVDS
panel.

Can you share dump of /sys/kernel/debug/clk/clk_summary on failing
and
working system ? You might see the difference around the "video"
clock.

(I have seen this behavior before, the fix was usually a matter of
moving one of the LCDIFs to another upstream clock like PLL3, so it
can
pick well matching output clock instead of some horrid approximation
which then drives the panel likely out of specification)


Hi Marek,

Please find attached the clk_summary for v6.12-rc2 before and after the
reversion (the one after the reversion is 6.12-rc2_summary_postfix).

Thank you, this helped greatly.

I believe I know why it used to kind-of work for you, but I'm afraid 
this used to work by sheer chance and it does not really work correctly 
for the panel you use, even if the panel likely does show the correct 
content. But, there is a way to make it work properly for the panel you use.


First of all, the pixel clock never really matched the panel-simple.c 
pixel clock for the edt_etml1010g3dra_timing:


$ grep '\' 6.12-rc2_summary_postfix
  media_disp2_pix  1  1  0  7425 ...


$ grep -A 1 edt_etml1010g3dra_timing drivers/gpu/drm/panel/panel-simple.c
static const struct display_timing edt_etml1010g3dra_timing = {
.pixelclock = { 6630, 7240, 7890 },
  

The pixel clock are within tolerance, but there is a discrepancy 
7425 != 7240 .


Since commit 94e6197dadc9 ("arm64: dts: imx8mp: Add LCDIF2 & LDB nodes") 
the IMX8MP_VIDEO_PLL1_OUT is set to a very specific frequency of 
103950 Hz, which tidily divides by 2 to 51975 Hz (which is your 
LVDS serializer frequency) and divides by 7 to 7425 Hz which is your 
LCDIF pixel clock.


This Video PLL1 configuration since moved to &media_blk_ctrl {} , but it 
is still in the imx8mp.dtsi . Therefore, to make your panel work at the 
correct desired pixel clock frequency instead of some random one 
inherited from imx8mp.dtsi, add the following to the pollux DT, I 
believe that will fix the problem and is the correct fix:


&media_blk_ctrl {
   // 50680 = 7240 * 7 (for single-link LVDS, this is enough)
   // there is no need to multiply the clock by * 2
   assigned-clock-rates = <5>, <2>, <0>, <0>, 
<5>, <50680>;

};

Can you please test whether this works and the pixel clock are accurate 
in /sys/kernel/debug/clk/clk_summary ?


Now ... as for the LVDS serializer clock ... that is more complicated.

The LCDIF driver does its .atomic_enable first , configures the pixel 
clock (and the Video PLL now) and enables the clock. The LVDS LDB 
serializer driver does its .atomic_enable second and cannot reconfigure 
the clock anymore, the Video PLL is stuck at /7 rate set by LCDIF driver 
and won't budge because the clo

Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 10/8/24 12:07 PM, Isaac Scott wrote:

On Mon, 2024-10-07 at 20:06 +0200, Marek Vasut wrote:

On 10/7/24 7:01 PM, Isaac Scott wrote:

Hi Marek,


Hi,


On Sat, 2024-07-06 at 02:16 +0200, Marek Vasut wrote:

On 6/24/24 11:19 AM, Alexander Stein wrote:

Am Freitag, 31. Mai 2024, 22:27:21 CEST schrieb Marek Vasut:

In case an upstream bridge modified the required clock
frequency
in its .atomic_check callback by setting adjusted_mode.clock
,
make sure that clock frequency is generated by the LCDIFv3
block.

This is useful e.g. when LCDIFv3 feeds DSIM which feeds
TC358767
with (e)DP output, where the TC358767 expects precise timing
on
its input side, the precise timing must be generated by the
LCDIF.

Signed-off-by: Marek Vasut 


With the other rc358767 patches in place, this does the trick.
Reviewed-by: Alexander Stein 


I'll pick this up next week if there is no objection.


Unfortunately, this has caused a regression that is present in
v6.12-
rc1 on the i.MX8MP PHYTEC Pollux using the
arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts. The
display is the edt,etml1010g3dra panel, as per the upstream dts. We
bisected to this commit, and reverting this change fixed the
screen.

We then tried to retest this on top of v6.12-rc2, and found we also
had
to revert commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155 ("clk:
imx:
clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")
alongside this. Reverting these two commits makes the display work
again at -rc2.

Do you have any suggestions on anything we might be missing on our
end?
Please let me know if there's anything you'd like me to test as I'm
not
sure what the underlying fault was here.

I believe what is going on is that the LCDIF cannot configure its
upstream clock because something else is already using those clock
and
it set those clock to a specific frequency. LCDIF is now trying to
configure those clock to match the LVDS panel, and it fails, so it
tries
to set some approximate clock and that is not good enough for the
LVDS
panel.

Can you share dump of /sys/kernel/debug/clk/clk_summary on failing
and
working system ? You might see the difference around the "video"
clock.

(I have seen this behavior before, the fix was usually a matter of
moving one of the LCDIFs to another upstream clock like PLL3, so it
can
pick well matching output clock instead of some horrid approximation
which then drives the panel likely out of specification)


Hi Marek,

Please find attached the clk_summary for v6.12-rc2 before and after the
reversion (the one after the reversion is 6.12-rc2_summary_postfix).

How come "media_mipi_phy1_ref" is on Video PLL1 before the revert ?

Does it start working if you move "media_mipi_phy1_ref" to osc_24m ? 
(probably not)


Also, why is the LDB configured to 74 MHz instead of 519 MHz now ? That 
is really odd. I'll see if I can reproduce this later today.

Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 10/7/24 7:01 PM, Isaac Scott wrote:

Hi Marek,


Hi,


On Sat, 2024-07-06 at 02:16 +0200, Marek Vasut wrote:

On 6/24/24 11:19 AM, Alexander Stein wrote:

Am Freitag, 31. Mai 2024, 22:27:21 CEST schrieb Marek Vasut:

In case an upstream bridge modified the required clock frequency
in its .atomic_check callback by setting adjusted_mode.clock ,
make sure that clock frequency is generated by the LCDIFv3 block.

This is useful e.g. when LCDIFv3 feeds DSIM which feeds TC358767
with (e)DP output, where the TC358767 expects precise timing on
its input side, the precise timing must be generated by the
LCDIF.

Signed-off-by: Marek Vasut 


With the other rc358767 patches in place, this does the trick.
Reviewed-by: Alexander Stein 


I'll pick this up next week if there is no objection.


Unfortunately, this has caused a regression that is present in v6.12-
rc1 on the i.MX8MP PHYTEC Pollux using the
arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts. The
display is the edt,etml1010g3dra panel, as per the upstream dts. We
bisected to this commit, and reverting this change fixed the screen.

We then tried to retest this on top of v6.12-rc2, and found we also had
to revert commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155 ("clk: imx:
clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")
alongside this. Reverting these two commits makes the display work
again at -rc2.

Do you have any suggestions on anything we might be missing on our end?
Please let me know if there's anything you'd like me to test as I'm not
sure what the underlying fault was here.
I believe what is going on is that the LCDIF cannot configure its 
upstream clock because something else is already using those clock and 
it set those clock to a specific frequency. LCDIF is now trying to 
configure those clock to match the LVDS panel, and it fails, so it tries 
to set some approximate clock and that is not good enough for the LVDS 
panel.


Can you share dump of /sys/kernel/debug/clk/clk_summary on failing and 
working system ? You might see the difference around the "video" clock.


(I have seen this behavior before, the fix was usually a matter of 
moving one of the LCDIFs to another upstream clock like PLL3, so it can 
pick well matching output clock instead of some horrid approximation 
which then drives the panel likely out of specification)

Re: [PATCH v2 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

On 9/27/24 9:33 PM, Nicolas Dufresne wrote:

Le mercredi 25 septembre 2024 à 22:45 +0200, Marek Vasut a écrit :

On 9/25/24 7:58 PM, Nicolas Dufresne wrote:




[...]




+static irqreturn_t ipu_mem2mem_vdic_nfb4eof_interrupt(int irq, void *dev_id)
+{
+   struct ipu_mem2mem_vdic_priv *priv = dev_id;
+
+   /* That is about all we can do about it, report it. */
+   dev_warn_ratelimited(priv->dev, "NFB4EOF error interrupt occurred\n");


Not sure this is right. If that means ipu_mem2mem_vdic_eof_interrupt won't fire,
then it means streamoff/close after that will hang forever, leaving a zombie
process behind.

Perhaps mark the buffers as ERROR, and finish the job.


The NFB4EOF interrupt is generated when the VDIC didn't write (all of)
output frame . I think it stands for "New Frame Before EOF" or some
such. Basically the currently written frame will be corrupted and the
next frame(s) are likely going to be OK again.


So the other IRQ will be triggered ? After this one ? Is so, perhaps take a
moment to mark the frames as ERROR (which means corrupted).


OK, fixed in V3.


[...]



The driver is not taking ownership of prev_buf, only curr_buf is guaranteed to
exist until v4l2_m2m_job_finish() is called. Usespace could streamoff, allocate
new buffers, and then an old freed buffer may endup being used.


So, what should I do about this ? Is there some way to ref the buffer to
keep it around ?


Its also unclear to me how userspace can avoid this ugly warning, how can you
have curr_buf set the first time ? (I might be missing something you this one
though).


The warning happens when streaming starts and there is only one input
frame available for the VDIC, which needs three fields to work
correctly. So, if there in only one input frame, VDI uses the input
frame bottom field as PREV field for the prediction, and input frame top
and bottom fields as CURR and NEXT fields for the prediction, the result
may be one sub-optimal deinterlaced output frame (the first one). Once
another input frame gets enqueued, the VDIC uses the previous frame
bottom field as PREV and the newly enqueued frame top and bottom fields
as CURR and NEXT and the prediction works correctly from that point on.


Warnings by default are not acceptable.


This is a workaround so that older gstreamer versions would work, what 
else can I do here ?



Perhaps what you want is a custom job_ready() callback, that ensure you have 2
buffers in the OUTPUT queue ? You also need to ajust the CID
MIN_BUFFERS_FOR_OUTPUT accordingly.


I had that before, but gstreamer didn't enqueue the two frames for me,
so I got back to this variant for maximum compatibility.


Its well known that GStreamer v4l2convert element have no support for
detinterlacing and need to be improved to support any deinterlace drivers out
there.


It seems v4l2convert disable-passthrough=true works with deinterlacers 
just fine , except for this one reused frame at stream start ?



Other drivers will simply holds on output buffers until it has enough to produce
the first valid picture. Holding meaning not marking them done, which keeps then
in the ACTIVE state, which is being tracked by the core for your.


As far as I understand this, when the EOF interrupt happens, 
v4l2_m2m_src_buf_remove() pulls the oldest input buffer from the queue 
and that buffer is then marked as DONE (or ERROR in v3), that is the 
->prev buffer, isn't it ?


Once the next frame deinterlacing starts, the (new) current frame and 
the prev frame are both active, the deinterlacing happens and then in 
the EOF interrupt, the ->prev frame gets marked as DONE again.


What am I missing here ?


[...]


+
+   if (ipu_mem2mem_vdic_format_is_yuv420(f->fmt.pix.pixelformat))
+   f->fmt.pix.bytesperline = f->fmt.pix.width * 3 / 2;
+   else if (ipu_mem2mem_vdic_format_is_yuv422(f->fmt.pix.pixelformat))
+   f->fmt.pix.bytesperline = f->fmt.pix.width * 2;
+   else if (ipu_mem2mem_vdic_format_is_rgb16(f->fmt.pix.pixelformat))
+   f->fmt.pix.bytesperline = f->fmt.pix.width * 2;
+   else if (ipu_mem2mem_vdic_format_is_rgb24(f->fmt.pix.pixelformat))
+   f->fmt.pix.bytesperline = f->fmt.pix.width * 3;
+   else if (ipu_mem2mem_vdic_format_is_rgb32(f->fmt.pix.pixelformat))
+   f->fmt.pix.bytesperline = f->fmt.pix.width * 4;
+   else
+   f->fmt.pix.bytesperline = f->fmt.pix.width;
+
+   f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;


And use v4l2-common ?


I don't really understand, there is nothing in v4l2-common.c that would
be really useful replacement for this ?


Not sure I get your response, v4l2-common is used in many drivers already, and
we intent to keep improving it so that all driver uses it in the long term. It
been created because folks believed they can calculate bytesperline

Re: [PATCH v2 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

On 9/26/24 1:14 PM, Philipp Zabel wrote:

Hi,


Hi,


On Mi, 2024-09-25 at 22:14 +0200, Marek Vasut wrote:

The userspace could distribute the frames between the two devices in an
alternating manner, can it not ?


This doesn't help with latency, or when converting a single large
frame.

For the deinterlacer, this can't be done with the motion-aware
temporally filtering modes. Those need a field from the previous frame.


It is up to the userspace to pass the correct frames to the deinterlacer.


Would the 1280x360 field be split into two tiles vertically and each
tile (newly 1280/2 x 360) be enqueued on each VDIC ? I don't think that
works, because you wouldn't be able to stitch those tiles back together
nicely after the deinterlacing, would you? I would expect to see some
sort of an artifact exactly where the two tiles got stitched back
together, because the VDICs are unaware of each other and how each
deinterlaced the tile.


I was thinking horizontally, two 640x720 tiles side by side. 1280 is
larger than the 968 pixel maximum horizontal resolution of the VDIC.

As you say, splitting vertically (which would be required for 1080i)
should cause artifacts at the seam due to the 4-tap vertical filter.


Can the userspace set some sort of offset/stride in each buffer and 
distribute the task between the two VDIs then ?



[...]


With the rigid V4L2 model though, where memory handling, parameter
calculation, and job scheduling of tiles in a single frame all have to
be hidden behind the V4L2 API, I don't think requiring userspace to
combine multiple mem2mem video devices to work together on a single
frame is feasible.


If your concern is throughput (from what I gathered from the text
above), userspace could schedule frames on either VDIC in alternating
manner.


Both throughput and latency.

Yes, alternating to different devices would help with throughput where
possible, but it's worse for frame pacing, a hassle to implement
generically in userspace, and it's straight up impossible with temporal
filtering.


See above, userspace should be able to pass the correct frames to m2m 
device.



I think this is much better and actually generic approach than trying to
combine two independent devices on kernel level and introduce some sort
of scheduler into kernel driver to distribute jobs between the two
devices. Generic, because this approach works even if either of the two
devices is not VDIC. Independent devices, because yes, the MX6Q IPUs are
two independent blocks, it is only the current design of the IPUv3
driver that makes them look kind-of like they are one single big device,
I am not happy about that design, but rewriting the IPUv3 driver is way
out of scope here. (*)


The IPUs are glued together at the capture and output paths, so yes,
they are independent blocks, but also work together as a big device.


Is limiting different users to the different deinterlacer hardware
units a real usecase? I saw the two ICs, when used as mem2mem devices,
as interchangeable resources.


I do not have that use case, but I can imagine it could come up.
In my case, I schedule different cameras to different VDICs from
userspace as needed.


Is this just because a single VDIC does not have enough throughput to
serve all cameras, or is there some reason for a fixed assignment
between cameras and VDICs?


I want to be able to distribute the bandwidth utilization between the 
two IPUs .



To be fair, we never implemented that for the CSC/scaler mem2mem device
either.


I don't think that is actually a good idea. Instead, it would be better
to have two scaler nodes in userspace.


See above, that would make it impossible (or rather unreasonably
complicated) to distribute work on a single frame to both IPUs.


Is your concern latency instead of throughput ? See my comment in
paragraph (*) .


Either, depending on the use case.

[...]



https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/media/imx/imx-media-vdic.c#n207


That code is unused. The direct hardware path doesn't use
IPUV3_CHANNEL_MEM_VDI_PREV/CUR/NEXT, but is has a similar effect, half
of the incoming fields are dropped. The setup is vdic_setup_direct().


All right, let's drop that unused code then, I'll prepare a patch.


Thanks!


But it seems the bottom line is, the VDI direct mode does not act as a
frame-rate doubler ?


Yes, it can't. In direct mode, VDIC only receives half of the fields.

[...]



Why would adding the (configurable) frame-rate doubling mode break
userspace if this is not the default ?


I'm not sure it would. Maybe there should be a deinterlacer control to
choose between full and half field rate output (aka frame doubling and
1:1 input to output frame rate).

Also, my initial assumption was that currently there is 1:1 input
frames to output frames. But with temporal filtering enabled there's
already one input frame (the first one) tha

Re: [PATCH v2 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

On 9/26/24 1:16 PM, Philipp Zabel wrote:

On Mi, 2024-09-25 at 22:45 +0200, Marek Vasut wrote:
[...]

The driver is not taking ownership of prev_buf, only curr_buf is guaranteed to
exist until v4l2_m2m_job_finish() is called. Usespace could streamoff, allocate
new buffers, and then an old freed buffer may endup being used.


So, what should I do about this ? Is there some way to ref the buffer to
keep it around ?


Have a look how other deinterlacers with temporal filtering do it.
sunxi/sun8i-di or ti/vpe look like candidates.
I don't see exactly what those drivers are doing differently to protect 
the prev buffer during deinterlacing . Can you be more specific ?

Re: [PATCH v2 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

On 9/25/24 7:58 PM, Nicolas Dufresne wrote:

Hi,

[...]


+static struct v4l2_pix_format *
+ipu_mem2mem_vdic_get_format(struct ipu_mem2mem_vdic_priv *priv,
+   enum v4l2_buf_type type)
+{
+   return &priv->fmt[V4L2_TYPE_IS_OUTPUT(type) ? V4L2_M2M_SRC : 
V4L2_M2M_DST];
+}


 From here ...


+
+static bool ipu_mem2mem_vdic_format_is_yuv420(const u32 pixelformat)
+{
+   /* All 4:2:0 subsampled formats supported by this hardware */
+   return pixelformat == V4L2_PIX_FMT_YUV420 ||
+  pixelformat == V4L2_PIX_FMT_YVU420 ||
+  pixelformat == V4L2_PIX_FMT_NV12;
+}
+
+static bool ipu_mem2mem_vdic_format_is_yuv422(const u32 pixelformat)
+{
+   /* All 4:2:2 subsampled formats supported by this hardware */
+   return pixelformat == V4L2_PIX_FMT_UYVY ||
+  pixelformat == V4L2_PIX_FMT_YUYV ||
+  pixelformat == V4L2_PIX_FMT_YUV422P ||
+  pixelformat == V4L2_PIX_FMT_NV16;
+}
+
+static bool ipu_mem2mem_vdic_format_is_yuv(const u32 pixelformat)
+{
+   return ipu_mem2mem_vdic_format_is_yuv420(pixelformat) ||
+  ipu_mem2mem_vdic_format_is_yuv422(pixelformat);
+}
+
+static bool ipu_mem2mem_vdic_format_is_rgb16(const u32 pixelformat)
+{
+   /* All 16-bit RGB formats supported by this hardware */
+   return pixelformat == V4L2_PIX_FMT_RGB565;
+}
+
+static bool ipu_mem2mem_vdic_format_is_rgb24(const u32 pixelformat)
+{
+   /* All 24-bit RGB formats supported by this hardware */
+   return pixelformat == V4L2_PIX_FMT_RGB24 ||
+  pixelformat == V4L2_PIX_FMT_BGR24;
+}
+
+static bool ipu_mem2mem_vdic_format_is_rgb32(const u32 pixelformat)
+{
+   /* All 32-bit RGB formats supported by this hardware */
+   return pixelformat == V4L2_PIX_FMT_XRGB32 ||
+  pixelformat == V4L2_PIX_FMT_XBGR32 ||
+  pixelformat == V4L2_PIX_FMT_BGRX32 ||
+  pixelformat == V4L2_PIX_FMT_RGBX32;
+}


To here, these days, all this information can be derived from v4l2_format_info
in v4l2-common in a way you don't have to create a big barrier to adding more
formats in the future.


I am not sure I quite understand this suggestion, what should I change here?

Note that the IPUv3 seems to be done, it does not seem like there will 
be new SoCs with this block, so the list of formats here is likely final.


[...]


+static irqreturn_t ipu_mem2mem_vdic_nfb4eof_interrupt(int irq, void *dev_id)
+{
+   struct ipu_mem2mem_vdic_priv *priv = dev_id;
+
+   /* That is about all we can do about it, report it. */
+   dev_warn_ratelimited(priv->dev, "NFB4EOF error interrupt occurred\n");


Not sure this is right. If that means ipu_mem2mem_vdic_eof_interrupt won't fire,
then it means streamoff/close after that will hang forever, leaving a zombie
process behind.

Perhaps mark the buffers as ERROR, and finish the job.


The NFB4EOF interrupt is generated when the VDIC didn't write (all of) 
output frame . I think it stands for "New Frame Before EOF" or some 
such. Basically the currently written frame will be corrupted and the 
next frame(s) are likely going to be OK again.



+
+   return IRQ_HANDLED;
+}
+
+static void ipu_mem2mem_vdic_device_run(void *_ctx)
+{
+   struct ipu_mem2mem_vdic_ctx *ctx = _ctx;
+   struct ipu_mem2mem_vdic_priv *priv = ctx->priv;
+   struct vb2_v4l2_buffer *curr_buf, *dst_buf;
+   dma_addr_t prev_phys, curr_phys, out_phys;
+   struct v4l2_pix_format *infmt;
+   u32 phys_offset = 0;
+   unsigned long flags;
+
+   infmt = ipu_mem2mem_vdic_get_format(priv, V4L2_BUF_TYPE_VIDEO_OUTPUT);
+   if (V4L2_FIELD_IS_SEQUENTIAL(infmt->field))
+   phys_offset = infmt->sizeimage / 2;
+   else if (V4L2_FIELD_IS_INTERLACED(infmt->field))
+   phys_offset = infmt->bytesperline;
+   else
+   dev_err(priv->dev, "Invalid field %d\n", infmt->field);
+
+   dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
+   out_phys = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
+
+   curr_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
+   if (!curr_buf) {
+   dev_err(priv->dev, "Not enough buffers\n");
+   return;


Impossible branch, has been checked by __v4l2_m2m_try_queue().


Fixed in V3


+   }
+
+   spin_lock_irqsave(&priv->irqlock, flags);
+
+   if (ctx->curr_buf) {
+   ctx->prev_buf = ctx->curr_buf;
+   ctx->curr_buf = curr_buf;
+   } else {
+   ctx->prev_buf = curr_buf;
+   ctx->curr_buf = curr_buf;
+   dev_warn(priv->dev, "Single-buffer mode, fix your userspace\n");
+   }


The driver is not taking ownership of prev_buf, only curr_buf is guaranteed to
exist until v4l2_m2m_job_finish() is called. Usespace could streamoff, allocate
new buffers, and then an old freed buffer may endup being used.


So, what should I do about this ? Is there some way to ref the buffer to 
keep it ar

Re: [PATCH v2 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

On 9/25/24 5:07 PM, Philipp Zabel wrote:

Hi,


On Di, 2024-09-24 at 17:28 +0200, Marek Vasut wrote:

On 9/6/24 11:01 AM, Philipp Zabel wrote:

[...]

Instead of presenting two devices to userspace, it would be better to
have a single video device that can distribute work to both IPUs.


Why do you think so ?


The scaler/colorspace converter supports frames larger than the
1024x1024 hardware by splitting each frame into multiple tiles. It
currently does so sequentially on a single IC. Speed could be improved
by distributing the tiles to both ICs. This is not an option anymore if
there are two video devices that are fixed to one IC each.


The userspace could distribute the frames between the two devices in an 
alternating manner, can it not ?



The same would be possible for the deinterlacer, e.g. to support 720i
frames split into two tiles each sent to one of the two VDICs.


Would the 1280x360 field be split into two tiles vertically and each 
tile (newly 1280/2 x 360) be enqueued on each VDIC ? I don't think that 
works, because you wouldn't be able to stitch those tiles back together 
nicely after the deinterlacing, would you? I would expect to see some 
sort of an artifact exactly where the two tiles got stitched back 
together, because the VDICs are unaware of each other and how each 
deinterlaced the tile.



I think it is better to keep the kernel code as simple as possible, i.e.
provide the device node for each m2m device to userspace and handle the
m2m device hardware interaction in the kernel driver, but let userspace
take care of policy like job scheduling, access permissions assignment
to each device (e.g. if different user accounts should have access to
different VDICs), or other such topics.


I both agree and disagree with you at the same time.

If the programming model were more similar to DRM, I'd agree in a
heartbeat. If the kernel driver just had to do memory/fence handling
and command submission (and parameter sanitization, because there is no
MMU), and there was some userspace API on top, it would make sense to
me to handle parameter calculation and job scheduling in a hardware
specific userspace driver that can just open one device for each IPU.

With the rigid V4L2 model though, where memory handling, parameter
calculation, and job scheduling of tiles in a single frame all have to
be hidden behind the V4L2 API, I don't think requiring userspace to
combine multiple mem2mem video devices to work together on a single
frame is feasible.


If your concern is throughput (from what I gathered from the text 
above), userspace could schedule frames on either VDIC in alternating 
manner.


I think this is much better and actually generic approach than trying to 
combine two independent devices on kernel level and introduce some sort 
of scheduler into kernel driver to distribute jobs between the two 
devices. Generic, because this approach works even if either of the two 
devices is not VDIC. Independent devices, because yes, the MX6Q IPUs are 
two independent blocks, it is only the current design of the IPUv3 
driver that makes them look kind-of like they are one single big device, 
I am not happy about that design, but rewriting the IPUv3 driver is way 
out of scope here. (*)



Is limiting different users to the different deinterlacer hardware
units a real usecase? I saw the two ICs, when used as mem2mem devices,
as interchangeable resources.


I do not have that use case, but I can imagine it could come up.
In my case, I schedule different cameras to different VDICs from 
userspace as needed.



To be fair, we never implemented that for the CSC/scaler mem2mem device
either.


I don't think that is actually a good idea. Instead, it would be better
to have two scaler nodes in userspace.


See above, that would make it impossible (or rather unreasonably
complicated) to distribute work on a single frame to both IPUs.


Is your concern latency instead of throughput ? See my comment in 
paragraph (*) .




[...]

+   ipu_cpmem_set_buffer(priv->vdi_out_ch,  0, out_phys);
+   ipu_cpmem_set_buffer(priv->vdi_in_ch_p, 0, prev_phys + phys_offset);
+   ipu_cpmem_set_buffer(priv->vdi_in_ch,   0, curr_phys);
+   ipu_cpmem_set_buffer(priv->vdi_in_ch_n, 0, curr_phys + phys_offset);


This always outputs at a frame rate of half the field rate, and only
top fields are ever used as current field, and bottom fields as
previous/next fields, right?


Yes, currently the driver extracts 1 frame from two consecutive incoming
fields (previous Bottom, and current Top and Bottom):

(frame 1 and 3 below is omitted)

  1  2  3  4
...|T |T |T |T |...
...| B| B| B| B|...
   | ||  | ||
   '-''  '-''
||||
||\/
\/  Frame#4
  Frame#2

As far as I understand it, this is how the current VDI implementation
behaves too, right ?


Yes, that is a hardware limitation when using the direct CSI-&g

Re: [PATCH v2 1/2] gpu: ipu-v3: vdic: Simplify ipu_vdi_setup()

[Marek Vasut]

On 9/25/24 6:43 PM, Philipp Zabel wrote:

On Di, 2024-09-24 at 12:47 +0200, Marek Vasut wrote:

On 9/4/24 11:05 AM, Philipp Zabel wrote:

On Mi, 2024-07-24 at 02:19 +0200, Marek Vasut wrote:

The 'code' parameter only ever selects between YUV 4:2:0 and 4:2:2
subsampling, turn it into boolean to select exactly that and update
related code accordingly.

Signed-off-by: Marek Vasut 


I'd prefer this to be an enum ipu_chroma_subsampling or similar,
instead of a boolean. Otherwise,


I'm afraid this introduces unnecessary back and forth conversions
between the boolean and either of the two enum ipu_chroma_subsampling
values in the code.


Fair enough. I dislike the opaque usage in vdic_start() a bit, but with
the in422 variable in ipu_mem2mem_vdic_setup_hardware() it is clear
enough.
I'll keep the rework for V3 and then we can decide whether or not to 
undo it.

Re: [PATCH] dt-bindings: lcdif: Add support for specifying display with timings

[Marek Vasut]

On 9/25/24 12:57 AM, Rob Herring wrote:

On Mon, Sep 23, 2024 at 07:53:57PM +0200, Marek Vasut wrote:

On 9/23/24 3:57 PM, Lukasz Majewski wrote:

Up till now the fsl,lcdif.yaml was requiring the "port" property as a
must have to specify the display interface on iMX devices.

However, it shall also be possible to specify the display only with
passing its timing parameters (h* and v* ones) via "display" property:
(as in
Documentation/devicetree/bindings/display/panel/display-timings.yaml).


Timings should go into panel node, not into scanout engine node.

See e.g. panel-timings in arch/arm64/boot/dts/freescale/imx8mm-phg.dts , in
your case the compatible might be "panel-dpi" .


I agree, but if this is already in use, we should allow it. We can mark
it deprecated though.
I don't think it is in use yet, at least not in upstream, so let's not 
allow this.

Re: [PATCH v2 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

On 9/6/24 11:01 AM, Philipp Zabel wrote:

Hi,


diff --git a/drivers/staging/media/imx/imx-media-dev.c 
b/drivers/staging/media/imx/imx-media-dev.c
index be54dca11465d..a841fdb4c2394 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -57,7 +57,52 @@ static int imx6_media_probe_complete(struct 
v4l2_async_notifier *notifier)
goto unlock;
}
  
+	imxmd->m2m_vdic[0] = imx_media_mem2mem_vdic_init(imxmd, 0);

+   if (IS_ERR(imxmd->m2m_vdic[0])) {
+   ret = PTR_ERR(imxmd->m2m_vdic[0]);
+   imxmd->m2m_vdic[0] = NULL;
+   goto unlock;
+   }
+
+   /* MX6S/DL has one IPUv3, init second VDI only on MX6Q/QP */
+   if (imxmd->ipu[1]) {
+   imxmd->m2m_vdic[1] = imx_media_mem2mem_vdic_init(imxmd, 1);
+   if (IS_ERR(imxmd->m2m_vdic[1])) {
+   ret = PTR_ERR(imxmd->m2m_vdic[1]);
+   imxmd->m2m_vdic[1] = NULL;
+   goto uninit_vdi0;
+   }
+   }


Instead of presenting two devices to userspace, it would be better to
have a single video device that can distribute work to both IPUs.


Why do you think so ?

I think it is better to keep the kernel code as simple as possible, i.e. 
provide the device node for each m2m device to userspace and handle the 
m2m device hardware interaction in the kernel driver, but let userspace 
take care of policy like job scheduling, access permissions assignment 
to each device (e.g. if different user accounts should have access to 
different VDICs), or other such topics.



To be fair, we never implemented that for the CSC/scaler mem2mem device
either.


I don't think that is actually a good idea. Instead, it would be better 
to have two scaler nodes in userspace.


[...]


+++ b/drivers/staging/media/imx/imx-media-mem2mem-vdic.c
@@ -0,0 +1,997 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * i.MX VDIC mem2mem de-interlace driver
+ *
+ * Copyright (c) 2024 Marek Vasut 
+ *
+ * Based on previous VDIC mem2mem work by Steve Longerbeam that is:
+ * Copyright (c) 2018 Mentor Graphics Inc.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "imx-media.h"
+
+#define fh_to_ctx(__fh)container_of(__fh, struct ipu_mem2mem_vdic_ctx, 
fh)
+
+#define to_mem2mem_priv(v) container_of(v, struct ipu_mem2mem_vdic_priv, vdev)


These could be inline functions for added type safety.


Fixed in v3

[...]


+static void ipu_mem2mem_vdic_device_run(void *_ctx)
+{
+   struct ipu_mem2mem_vdic_ctx *ctx = _ctx;
+   struct ipu_mem2mem_vdic_priv *priv = ctx->priv;
+   struct vb2_v4l2_buffer *curr_buf, *dst_buf;
+   dma_addr_t prev_phys, curr_phys, out_phys;
+   struct v4l2_pix_format *infmt;
+   u32 phys_offset = 0;
+   unsigned long flags;
+
+   infmt = ipu_mem2mem_vdic_get_format(priv, V4L2_BUF_TYPE_VIDEO_OUTPUT);
+   if (V4L2_FIELD_IS_SEQUENTIAL(infmt->field))
+   phys_offset = infmt->sizeimage / 2;
+   else if (V4L2_FIELD_IS_INTERLACED(infmt->field))
+   phys_offset = infmt->bytesperline;
+   else
+   dev_err(priv->dev, "Invalid field %d\n", infmt->field);
+
+   dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
+   out_phys = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
+
+   curr_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
+   if (!curr_buf) {
+   dev_err(priv->dev, "Not enough buffers\n");
+   return;
+   }
+
+   spin_lock_irqsave(&priv->irqlock, flags);
+
+   if (ctx->curr_buf) {
+   ctx->prev_buf = ctx->curr_buf;
+   ctx->curr_buf = curr_buf;
+   } else {
+   ctx->prev_buf = curr_buf;
+   ctx->curr_buf = curr_buf;
+   dev_warn(priv->dev, "Single-buffer mode, fix your userspace\n");
+   }
+
+   prev_phys = vb2_dma_contig_plane_dma_addr(&ctx->prev_buf->vb2_buf, 0);
+   curr_phys = vb2_dma_contig_plane_dma_addr(&ctx->curr_buf->vb2_buf, 0);
+
+   priv->curr_ctx = ctx;
+   spin_unlock_irqrestore(&priv->irqlock, flags);
+
+   ipu_cpmem_set_buffer(priv->vdi_out_ch,  0, out_phys);
+   ipu_cpmem_set_buffer(priv->vdi_in_ch_p, 0, prev_phys + phys_offset);
+   ipu_cpmem_set_buffer(priv->vdi_in_ch,   0, curr_phys);
+   ipu_cpmem_set_buffer(priv->vdi_in_ch_n, 0, curr_phys + phys_offset);


This always outputs at a frame rate of half the field rate, and only
top fields are ever used as current field, and bottom fields as
previous/next fields, right?


Yes, currently the driver extracts 1 frame from two consecutive incoming 
fields (previous Bottom, and current Top an

Re: [PATCH v2 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

On 7/30/24 6:05 PM, Nicolas Dufresne wrote:

Hi,

sorry for the abysmal delay.


Le mercredi 24 juillet 2024 à 02:19 +0200, Marek Vasut a écrit :

Introduce dedicated memory-to-memory IPUv3 VDI deinterlacer driver.
Currently the IPUv3 can operate VDI in DIRECT mode, from sensor to
memory. This only works for single stream, that is, one input from
one camera is deinterlaced on the fly with a helper buffer in DRAM
and the result is written into memory.

The i.MX6Q/QP does support up to four analog cameras via two IPUv3
instances, each containing one VDI deinterlacer block. In order to
deinterlace all four streams from all four analog cameras live, it
is necessary to operate VDI in INDIRECT mode, where the interlaced
streams are written to buffers in memory, and then deinterlaced in
memory using VDI in INDIRECT memory-to-memory mode.


Just a quick design question. Is it possible to chain the deinterlacer and the
csc-scaler ?


I think you could do that.


If so, it would be much more efficient if all this could be
combined into the existing m2m driver, since you could save a memory rountrip
when needing to deinterlace, change the colorspace and possibly scale too.


The existing PRP/IC driver is similar to what this driver does, yes, but
it uses a different DMA path , I believe it is IDMAC->PRP->IC->IDMAC .
This driver uses IDMAC->VDI->IC->IDMAC . I am not convinced mixing the
two paths into a single driver would be beneficial, but I am reasonably
sure it would be very convoluted. Instead, this driver could be extended
to do deinterlacing and scaling using the IC if that was needed. I think
that would be the cleaner approach.


Not that I only meant to ask if there was a path to combine
CSC/Scaling/Deinterlacing without a memory rountrip. If a rountrip is needed
anyway, I would rather make separate video nodes, and leave it to userspace to
deal with. Though, if we can avoid it, a combined driver should be highly
beneficial.
The VDI mem2mem driver already uses the IC as an output path from the 
deinterlacer, IC is capable of scaling and it could be configured to do 
scaling. The IC configuration in the VDI mem2mem driver is some 10 lines 
of code (select input and output colorspace, and input and output image 
resolution), the rest of the VDI mem2mem driver is interaction with the 
VDI itself.


Since the IC configuration (i.e. color space conversion and scaling) is 
already well factored out, I think mixing the VDI and CSC drivers 
wouldn't bring any real benefit, it would only make the code more 
complicated.


[...]

Re: [PATCH v2 1/2] gpu: ipu-v3: vdic: Simplify ipu_vdi_setup()

[Marek Vasut]

On 9/4/24 11:05 AM, Philipp Zabel wrote:

On Mi, 2024-07-24 at 02:19 +0200, Marek Vasut wrote:

The 'code' parameter only ever selects between YUV 4:2:0 and 4:2:2
subsampling, turn it into boolean to select exactly that and update
related code accordingly.

Signed-off-by: Marek Vasut 


I'd prefer this to be an enum ipu_chroma_subsampling or similar,
instead of a boolean. Otherwise,


I'm afraid this introduces unnecessary back and forth conversions 
between the boolean and either of the two enum ipu_chroma_subsampling 
values in the code.

Re: [PATCH] dt-bindings: lcdif: Add support for specifying display with timings

[Marek Vasut]

On 9/23/24 3:57 PM, Lukasz Majewski wrote:

Up till now the fsl,lcdif.yaml was requiring the "port" property as a
must have to specify the display interface on iMX devices.

However, it shall also be possible to specify the display only with
passing its timing parameters (h* and v* ones) via "display" property:
(as in
Documentation/devicetree/bindings/display/panel/display-timings.yaml).


Timings should go into panel node, not into scanout engine node.

See e.g. panel-timings in arch/arm64/boot/dts/freescale/imx8mm-phg.dts , 
in your case the compatible might be "panel-dpi" .

Re: [PATCH] dt-bindings: lcdif: Document the dmas/dma-names properties

[Marek Vasut]

On 9/3/24 6:27 PM, Fabio Estevam wrote:

From: Fabio Estevam 

i.MX28 has an RX DMA channel associated with the LCDIF controller.

Document the 'dmas' and 'dma-names' properties to fix the following
dt-schema warnings:

lcdif@8003: 'dma-names', 'dmas' do not match any of the regexes: 
'pinctrl-[0-9]+'

Signed-off-by: Fabio Estevam 
---
  .../bindings/display/fsl,lcdif.yaml   | 19 +++
  1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml 
b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
index 0681fc49aa1b..dd462abd61f8 100644
--- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
+++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
@@ -50,6 +50,14 @@ properties:
- const: disp_axi
  minItems: 1
  
+  dmas:

+items:
+  - description: DMA specifier for the RX DMA channel.
+
+  dma-names:
+items:
+  - const: rx
+
interrupts:
  items:
- description: LCDIF DMA interrupt
@@ -156,6 +164,17 @@ allOf:
  interrupts:
maxItems: 1
  
+  - if:

+  not:
+properties:
+  compatible:
+contains:
+  enum:
+- fsl,imx28-lcdif


This also applies to MX23 , that one also has the support for 
command-mode LCDs which are then driven by pumping commands via DMA. I 
don't think Linux actually supports this mode of operation, but I do 
recall using it some long time ago on MX23.

Re: [PATCH v2 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

On 7/24/24 6:16 PM, Dan Carpenter wrote:

On Wed, Jul 24, 2024 at 02:19:38AM +0200, Marek Vasut wrote:

diff --git a/drivers/staging/media/imx/imx-media-dev.c 
b/drivers/staging/media/imx/imx-media-dev.c
index be54dca11465d..a841fdb4c2394 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -57,7 +57,52 @@ static int imx6_media_probe_complete(struct 
v4l2_async_notifier *notifier)
goto unlock;
}
  
+	imxmd->m2m_vdic[0] = imx_media_mem2mem_vdic_init(imxmd, 0);

+   if (IS_ERR(imxmd->m2m_vdic[0])) {
+   ret = PTR_ERR(imxmd->m2m_vdic[0]);
+   imxmd->m2m_vdic[0] = NULL;
+   goto unlock;
+   }
+
+   /* MX6S/DL has one IPUv3, init second VDI only on MX6Q/QP */
+   if (imxmd->ipu[1]) {
+   imxmd->m2m_vdic[1] = imx_media_mem2mem_vdic_init(imxmd, 1);
+   if (IS_ERR(imxmd->m2m_vdic[1])) {
+   ret = PTR_ERR(imxmd->m2m_vdic[1]);
+   imxmd->m2m_vdic[1] = NULL;
+   goto uninit_vdi0;
+   }
+   }
+
ret = imx_media_csc_scaler_device_register(imxmd->m2m_vdev);
+   if (ret)
+   goto uninit_vdi1;
+
+   ret = imx_media_mem2mem_vdic_register(imxmd->m2m_vdic[0]);
+   if (ret)
+   goto unreg_csc;
+
+   /* MX6S/DL has one IPUv3, init second VDI only on MX6Q/QP */
+   if (imxmd->ipu[1]) {
+   ret = imx_media_mem2mem_vdic_register(imxmd->m2m_vdic[1]);
+   if (ret)
+   goto unreg_vdic;
+   }
+
+   mutex_unlock(&imxmd->mutex);
+   return ret;


Since it looks like you're going to do another version of this, could
you change this to return 0;


Fixed up both for V3, thanks .

Re: [PATCH v2 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

On 7/24/24 6:08 PM, Nicolas Dufresne wrote:

Hi Marek,


Hi,


Le mercredi 24 juillet 2024 à 02:19 +0200, Marek Vasut a écrit :

Introduce dedicated memory-to-memory IPUv3 VDI deinterlacer driver.
Currently the IPUv3 can operate VDI in DIRECT mode, from sensor to
memory. This only works for single stream, that is, one input from
one camera is deinterlaced on the fly with a helper buffer in DRAM
and the result is written into memory.

The i.MX6Q/QP does support up to four analog cameras via two IPUv3
instances, each containing one VDI deinterlacer block. In order to
deinterlace all four streams from all four analog cameras live, it
is necessary to operate VDI in INDIRECT mode, where the interlaced
streams are written to buffers in memory, and then deinterlaced in
memory using VDI in INDIRECT memory-to-memory mode.


Just a quick design question. Is it possible to chain the deinterlacer and the
csc-scaler ?


I think you could do that.


If so, it would be much more efficient if all this could be
combined into the existing m2m driver, since you could save a memory rountrip
when needing to deinterlace, change the colorspace and possibly scale too.


The existing PRP/IC driver is similar to what this driver does, yes, but 
it uses a different DMA path , I believe it is IDMAC->PRP->IC->IDMAC . 
This driver uses IDMAC->VDI->IC->IDMAC . I am not convinced mixing the 
two paths into a single driver would be beneficial, but I am reasonably 
sure it would be very convoluted. Instead, this driver could be extended 
to do deinterlacing and scaling using the IC if that was needed. I think 
that would be the cleaner approach.

[PATCH 2/2] drm/panel/panel-ilitek-ili9806e: Add Densitron DMT028VGHMCMI-1D TFT to ILI9806E DSI TCON driver

[Marek Vasut]

Add Densitron DMT028VGHMCMI-1D 480x640 TFT matrix 2.83 inch panel
attached to Ilitek ILI9806E DSI TCON into the ILI9806E driver.

Note that the Densitron panels use different TCONs, this driver is for
the later panel, use panel-ilitek-st7701.c for the former panel:
DMT028VGHMCMI-1A - ST7701
DMT028VGHMCMI-1D - ILI9806E

Signed-off-by: Marek Vasut 
---
Cc: Conor Dooley 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jessica Zhang 
Cc: Krzysztof Kozlowski 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Michael Walle 
Cc: Neil Armstrong 
Cc: Rob Herring 
Cc: Thomas Zimmermann 
Cc: devicet...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/panel/panel-ilitek-ili9806e.c | 165 ++
 1 file changed, 165 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c 
b/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c
index e4a44cd26c4dc..a3c79ad99d0bd 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c
@@ -380,7 +380,172 @@ static const struct panel_desc com35h3p70ulc_desc = {
.lanes = 2,
 };
 
+static void dmt028vghmcmi_1d_init(struct mipi_dsi_multi_context *ctx)
+{
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x01);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x08, 0x10);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x21, 0x01);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x30, 0x03);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x31, 0x00);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x60, 0x06);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x61, 0x00);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x62, 0x07);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x63, 0x00);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x40, 0x16);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x41, 0x44);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x42, 0x00);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x43, 0x83);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x44, 0x89);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x45, 0x8a);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x46, 0x44);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x47, 0x44);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x50, 0x78);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x51, 0x78);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x52, 0x00);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x53, 0x6c);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x54, 0x00);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x55, 0x6c);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x56, 0x00);
+   /* Gamma settings */
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xa0, 0x00);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xa1, 0x09);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xa2, 0x14);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xa3, 0x09);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xa4, 0x05);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xa5, 0x0a);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xa6, 0x07);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xa7, 0x07);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xa8, 0x08);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xa9, 0x0b);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xaa, 0x0c);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xab, 0x05);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xac, 0x0a);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xad, 0x19);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xae, 0x0b);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xaf, 0x00);
+
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xc0, 0x00);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xc1, 0x0c);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xc2, 0x14);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xc3, 0x11);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xc4, 0x05);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xc5, 0x0c);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xc6, 0x08);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xc7, 0x03);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xc8, 0x06);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xc9, 0x0a);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xca, 0x10);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xcb, 0x05);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xcc, 0x0d);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xcd, 0x15);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xce, 0x13);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xcf, 0x00);
+
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x07);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x17, 0x22);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x18, 0x1d);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x02, 0x77);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xe1, 0x79);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x06, 0x13);
+
+   mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x06);
+   /* GIP 0 */
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x00, 0x21);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x01, 0x0a);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x02, 0x00);
+   mipi_dsi_dcs_write_seq_multi(ctx, 0x03, 0x05

[PATCH 1/2] dt-bindings: display: panel: Document Densitron DMT028VGHMCMI-1D TFT on ILI9806E DSI TCON

[Marek Vasut]

Document Densitron DMT028VGHMCMI-1D 480x640 TFT matrix 2.83 inch panel
attached to Ilitek ILI9806E DSI TCON in the ILI9806E bindings.

Signed-off-by: Marek Vasut 
---
Cc: Conor Dooley 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jessica Zhang 
Cc: Krzysztof Kozlowski 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Michael Walle 
Cc: Neil Armstrong 
Cc: Rob Herring 
Cc: Thomas Zimmermann 
Cc: devicet...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
---
 .../devicetree/bindings/display/panel/ilitek,ili9806e.yaml   | 1 +
 1 file changed, 1 insertion(+)

diff --git 
a/Documentation/devicetree/bindings/display/panel/ilitek,ili9806e.yaml 
b/Documentation/devicetree/bindings/display/panel/ilitek,ili9806e.yaml
index cfd7cc9c87257..f803075794854 100644
--- a/Documentation/devicetree/bindings/display/panel/ilitek,ili9806e.yaml
+++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9806e.yaml
@@ -16,6 +16,7 @@ properties:
   compatible:
 items:
   - enum:
+  - densitron,dmt028vghmcmi-1d
   - ortustech,com35h3p70ulc
   - const: ilitek,ili9806e
 
-- 
2.43.0

[PATCH v2 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

Introduce dedicated memory-to-memory IPUv3 VDI deinterlacer driver.
Currently the IPUv3 can operate VDI in DIRECT mode, from sensor to
memory. This only works for single stream, that is, one input from
one camera is deinterlaced on the fly with a helper buffer in DRAM
and the result is written into memory.

The i.MX6Q/QP does support up to four analog cameras via two IPUv3
instances, each containing one VDI deinterlacer block. In order to
deinterlace all four streams from all four analog cameras live, it
is necessary to operate VDI in INDIRECT mode, where the interlaced
streams are written to buffers in memory, and then deinterlaced in
memory using VDI in INDIRECT memory-to-memory mode.

This driver also makes use of the IDMAC->VDI->IC->IDMAC data path
to provide pixel format conversion from input YUV formats to both
output YUV or RGB formats. The later is useful in case the data
are imported into the GPU, which on this platform cannot directly
sample YUV buffers.

This is derived from previous work by Steve Longerbeam and from the
IPUv3 CSC Scaler mem2mem driver.

Signed-off-by: Marek Vasut 
---
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Fabio Estevam 
Cc: Greg Kroah-Hartman 
Cc: Helge Deller 
Cc: Mauro Carvalho Chehab 
Cc: Pengutronix Kernel Team 
Cc: Philipp Zabel 
Cc: Sascha Hauer 
Cc: Shawn Guo 
Cc: Steve Longerbeam 
Cc: dri-devel@lists.freedesktop.org
Cc: i...@lists.linux.dev
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-fb...@vger.kernel.org
Cc: linux-me...@vger.kernel.org
Cc: linux-stag...@lists.linux.dev
---
V2: - Add complementary imx_media_mem2mem_vdic_uninit()
- Drop uninitiaized ret from ipu_mem2mem_vdic_device_run()
- Drop duplicate nbuffers assignment in ipu_mem2mem_vdic_queue_setup()
- Fix %u formatting string in ipu_mem2mem_vdic_queue_setup()
- Drop devm_*free from ipu_mem2mem_vdic_get_ipu_resources() fail path
  and ipu_mem2mem_vdic_put_ipu_resources()
- Add missing video_device_release()
---
 drivers/staging/media/imx/Makefile|   2 +-
 drivers/staging/media/imx/imx-media-dev.c |  55 +
 .../media/imx/imx-media-mem2mem-vdic.c| 997 ++
 drivers/staging/media/imx/imx-media.h |  10 +
 4 files changed, 1063 insertions(+), 1 deletion(-)
 create mode 100644 drivers/staging/media/imx/imx-media-mem2mem-vdic.c

diff --git a/drivers/staging/media/imx/Makefile 
b/drivers/staging/media/imx/Makefile
index 330e0825f506b..0cad87123b590 100644
--- a/drivers/staging/media/imx/Makefile
+++ b/drivers/staging/media/imx/Makefile
@@ -4,7 +4,7 @@ imx-media-common-objs := imx-media-capture.o 
imx-media-dev-common.o \
 
 imx6-media-objs := imx-media-dev.o imx-media-internal-sd.o \
imx-ic-common.o imx-ic-prp.o imx-ic-prpencvf.o imx-media-vdic.o \
-   imx-media-csc-scaler.o
+   imx-media-mem2mem-vdic.o imx-media-csc-scaler.o
 
 imx6-media-csi-objs := imx-media-csi.o imx-media-fim.o
 
diff --git a/drivers/staging/media/imx/imx-media-dev.c 
b/drivers/staging/media/imx/imx-media-dev.c
index be54dca11465d..a841fdb4c2394 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -57,7 +57,52 @@ static int imx6_media_probe_complete(struct 
v4l2_async_notifier *notifier)
goto unlock;
}
 
+   imxmd->m2m_vdic[0] = imx_media_mem2mem_vdic_init(imxmd, 0);
+   if (IS_ERR(imxmd->m2m_vdic[0])) {
+   ret = PTR_ERR(imxmd->m2m_vdic[0]);
+   imxmd->m2m_vdic[0] = NULL;
+   goto unlock;
+   }
+
+   /* MX6S/DL has one IPUv3, init second VDI only on MX6Q/QP */
+   if (imxmd->ipu[1]) {
+   imxmd->m2m_vdic[1] = imx_media_mem2mem_vdic_init(imxmd, 1);
+   if (IS_ERR(imxmd->m2m_vdic[1])) {
+   ret = PTR_ERR(imxmd->m2m_vdic[1]);
+   imxmd->m2m_vdic[1] = NULL;
+   goto uninit_vdi0;
+   }
+   }
+
ret = imx_media_csc_scaler_device_register(imxmd->m2m_vdev);
+   if (ret)
+   goto uninit_vdi1;
+
+   ret = imx_media_mem2mem_vdic_register(imxmd->m2m_vdic[0]);
+   if (ret)
+   goto unreg_csc;
+
+   /* MX6S/DL has one IPUv3, init second VDI only on MX6Q/QP */
+   if (imxmd->ipu[1]) {
+   ret = imx_media_mem2mem_vdic_register(imxmd->m2m_vdic[1]);
+   if (ret)
+   goto unreg_vdic;
+   }
+
+   mutex_unlock(&imxmd->mutex);
+   return ret;
+
+unreg_vdic:
+   imx_media_mem2mem_vdic_unregister(imxmd->m2m_vdic[0]);
+   imxmd->m2m_vdic[0] = NULL;
+unreg_csc:
+   imx_media_csc_scaler_device_unregister(imxmd->m2m_vdev);
+   imxmd->m2m_vdev = NULL;
+uninit_vdi1:
+   if (imxmd->ipu[1])
+   imx_media_mem2mem_vdic_uninit(imxmd->m2m_vdic[1]);
+uninit_vdi0:
+   imx_media_mem2mem_vdic_uninit(imxmd->m2m_vdic[0]);
 unlock:
mutex

[PATCH v2 1/2] gpu: ipu-v3: vdic: Simplify ipu_vdi_setup()

[Marek Vasut]

The 'code' parameter only ever selects between YUV 4:2:0 and 4:2:2
subsampling, turn it into boolean to select exactly that and update
related code accordingly.

Signed-off-by: Marek Vasut 
---
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Fabio Estevam 
Cc: Greg Kroah-Hartman 
Cc: Helge Deller 
Cc: Mauro Carvalho Chehab 
Cc: Pengutronix Kernel Team 
Cc: Philipp Zabel 
Cc: Sascha Hauer 
Cc: Shawn Guo 
Cc: Steve Longerbeam 
Cc: dri-devel@lists.freedesktop.org
Cc: i...@lists.linux.dev
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-fb...@vger.kernel.org
Cc: linux-me...@vger.kernel.org
Cc: linux-stag...@lists.linux.dev
--
V2: No change
---
 drivers/gpu/ipu-v3/ipu-vdi.c   | 14 +++---
 drivers/staging/media/imx/imx-media-vdic.c |  3 +--
 include/video/imx-ipu-v3.h |  2 +-
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-vdi.c b/drivers/gpu/ipu-v3/ipu-vdi.c
index a593b232b6d3e..4df2821977c0c 100644
--- a/drivers/gpu/ipu-v3/ipu-vdi.c
+++ b/drivers/gpu/ipu-v3/ipu-vdi.c
@@ -117,10 +117,10 @@ void ipu_vdi_set_motion(struct ipu_vdi *vdi, enum 
ipu_motion_sel motion_sel)
 }
 EXPORT_SYMBOL_GPL(ipu_vdi_set_motion);
 
-void ipu_vdi_setup(struct ipu_vdi *vdi, u32 code, int xres, int yres)
+void ipu_vdi_setup(struct ipu_vdi *vdi, bool yuv422not420, int xres, int yres)
 {
unsigned long flags;
-   u32 pixel_fmt, reg;
+   u32 reg;
 
spin_lock_irqsave(&vdi->lock, flags);
 
@@ -131,16 +131,8 @@ void ipu_vdi_setup(struct ipu_vdi *vdi, u32 code, int 
xres, int yres)
 * Full motion, only vertical filter is used.
 * Burst size is 4 accesses
 */
-   if (code == MEDIA_BUS_FMT_UYVY8_2X8 ||
-   code == MEDIA_BUS_FMT_UYVY8_1X16 ||
-   code == MEDIA_BUS_FMT_YUYV8_2X8 ||
-   code == MEDIA_BUS_FMT_YUYV8_1X16)
-   pixel_fmt = VDI_C_CH_422;
-   else
-   pixel_fmt = VDI_C_CH_420;
-
reg = ipu_vdi_read(vdi, VDI_C);
-   reg |= pixel_fmt;
+   reg |= yuv422not420 ? VDI_C_CH_422 : VDI_C_CH_420;
reg |= VDI_C_BURST_SIZE2_4;
reg |= VDI_C_BURST_SIZE1_4 | VDI_C_VWM1_CLR_2;
reg |= VDI_C_BURST_SIZE3_4 | VDI_C_VWM3_CLR_2;
diff --git a/drivers/staging/media/imx/imx-media-vdic.c 
b/drivers/staging/media/imx/imx-media-vdic.c
index 09da4103a8dbe..ea5b4ef3573de 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -376,8 +376,7 @@ static int vdic_start(struct vdic_priv *priv)
 * only supports 4:2:2 or 4:2:0, and this subdev will only
 * negotiate 4:2:2 at its sink pads.
 */
-   ipu_vdi_setup(priv->vdi, MEDIA_BUS_FMT_UYVY8_2X8,
- infmt->width, infmt->height);
+   ipu_vdi_setup(priv->vdi, true, infmt->width, infmt->height);
ipu_vdi_set_field_order(priv->vdi, V4L2_STD_UNKNOWN, infmt->field);
ipu_vdi_set_motion(priv->vdi, priv->motion);
 
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index c422a403c0990..75f435d024895 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -466,7 +466,7 @@ void ipu_ic_dump(struct ipu_ic *ic);
 struct ipu_vdi;
 void ipu_vdi_set_field_order(struct ipu_vdi *vdi, v4l2_std_id std, u32 field);
 void ipu_vdi_set_motion(struct ipu_vdi *vdi, enum ipu_motion_sel motion_sel);
-void ipu_vdi_setup(struct ipu_vdi *vdi, u32 code, int xres, int yres);
+void ipu_vdi_setup(struct ipu_vdi *vdi, bool yuv422not420, int xres, int yres);
 void ipu_vdi_unsetup(struct ipu_vdi *vdi);
 int ipu_vdi_enable(struct ipu_vdi *vdi);
 int ipu_vdi_disable(struct ipu_vdi *vdi);
-- 
2.43.0

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 7/16/24 9:50 PM, Michael Walle wrote:

Thank you for testing and keeping up with this. I will wait for more
feedback if there is any (Frieder? Lucas? Michael?). If there are no
objections, then I can merge it in a week or two ?


I'll try to use your approach on the tc358775. Hopefully, I'll find
some time this week.


So ... I wonder ... shall I apply these patches or not ?


As mentioned on IRC, I tried it to port it for the mediatek DSI
host, but I gave up and got doubts that this is the way to go. I
think this is too invasive (in a sense that it changes behavior)


I would argue it makes the behavior well defined. If that breaks some
drivers that depended on the undefined behavior before, those should be
fixed too.


Then this behavior should be documented (and accepted) in the
corresponding section:
https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation


I think so too.


This will help DSI host driver developers and we can point all the
host DSI driver maintainers to that document along with the proper
implementation :)


and not that easy to implement on other drivers.


How so ? At least the DSIM and STM32 DW DSI host can switch lanes to
LP11 state. Is the mediatek host not capable of that ?


The controller is certainly capable to do that. But the changes
seems pretty invasive to me and I fear some fallout. Although it's
basically just one line for the DSIM, you seem to be moving the init
of the DSIM to an earlier point(?). I'm no expert with all the DRM
stuff, so I might be wrong here.


I am moving some of the initialization to an earlier point, but only 
enough of it to configure the lanes to LP11 state before the next 
bridge(s) start to (pre)enable themselves. And the DSIM controller is 
RPM suspended again after the lanes are in LP11.



Given that this requirement is far more common across DSI bridges,
I'd favor a more general solution which isn't a workaround.


I think we only had a look at the TI DSI83 / ICN6211 / Toshiba TC358767
bridges, but we did not look at many panels, did we ? Do panels require
lanes in non-LP11 state on start up ?


I'm not talking about panels, just bridges. It's not just one bridge
with a weird behavior but most bridges.


What do you refer to by "weird behavior" ? It seems the DSI bridges we 
looked at all required data lanes in LP11 state on start up one way or 
the other, didn't they ?



Was there any progress on the generic LP11 solution, I think you did
mention something was in progress ? How would that even look like ?


I had a new callback implemented:
https://lore.kernel.org/r/20240506-tc358775-fix-powerup-v1-1-545dcf00b...@kernel.org/

Not sure if that's any better though.


Wouldn't that callback be called by various controllers at various 
stages of initialization , without consistency on when that callback 
will be called ? That would be my concern.


At least here, the expectation is that the controller would put data 
lanes into LP11 before the next bridge can even pre_enable itself , 
which is not perfect though, because if a bridge needs DSI clock to 
probe() itself and then data lanes in LP11 to probe itself, that is a 
really bad situation (and the TC358767 is capable of being used that 
way, although this is currently not supported by the kernel driver and 
there is no real interest in supporting it).

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 7/12/24 9:16 AM, Michael Walle wrote:

Hi Marek,


Hi,


Thank you for testing and keeping up with this. I will wait for more
feedback if there is any (Frieder? Lucas? Michael?). If there are no
objections, then I can merge it in a week or two ?


I'll try to use your approach on the tc358775. Hopefully, I'll find
some time this week.


So ... I wonder ... shall I apply these patches or not ?


As mentioned on IRC, I tried it to port it for the mediatek DSI
host, but I gave up and got doubts that this is the way to go. I
think this is too invasive (in a sense that it changes behavior)


I would argue it makes the behavior well defined. If that breaks some 
drivers that depended on the undefined behavior before, those should be 
fixed too.



and not that easy to implement on other drivers.


How so ? At least the DSIM and STM32 DW DSI host can switch lanes to 
LP11 state. Is the mediatek host not capable of that ?



Given that this requirement is far more common across DSI bridges,
I'd favor a more general solution which isn't a workaround.


I think we only had a look at the TI DSI83 / ICN6211 / Toshiba TC358767 
bridges, but we did not look at many panels, did we ? Do panels require 
lanes in non-LP11 state on start up ?


Was there any progress on the generic LP11 solution, I think you did 
mention something was in progress ? How would that even look like ?

Re: [PATCH v4 1/2] dt-bindings: display: bridge: tc358867: Document default DP preemphasis

[Marek Vasut]

On 7/10/24 5:33 PM, Rob Herring (Arm) wrote:


On Mon, 08 Jul 2024 17:01:13 +0200, Marek Vasut wrote:

Document default DP port preemphasis configurable via new DT property
"toshiba,pre-emphasis". This is useful in case the DP link properties
are known and starting link training from preemphasis setting of 0 dB
is not useful. The preemphasis can be set separately for both DP lanes
in range 0=0dB, 1=3.5dB, 2=6dB .

This is an endpoint property, not a port property, because the TC9595
datasheet does mention that the DP might operate in some sort of split
mode, where each DP lane is used to feed one display, so in that case
there might be two endpoints.

Signed-off-by: Marek Vasut 
---
Cc: Alexander Stein 
Cc: Andrzej Hajda 
Cc: Conor Dooley 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Krzysztof Kozlowski 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Rob Herring 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: devicet...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: - Fix the type to u8 array
 - Fix the enum items to match what they represent
V3: - Update commit message, expand on this being an endpoint property
V4: - Fix ref: /schemas/graph.yaml#/$defs/port-base and add 
unevaluatedProperties
---
  .../display/bridge/toshiba,tc358767.yaml  | 21 ++-
  1 file changed, 20 insertions(+), 1 deletion(-)



Reviewed-by: Rob Herring (Arm) 


If there are no objections, I will apply these two patches to drm-misc 
soon ?

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 7/11/24 5:57 PM, Marek Szyprowski wrote:

On 11.07.2024 17:38, Marek Vasut wrote:

On 6/26/24 10:02 AM, Michael Walle wrote:

On Wed Jun 26, 2024 at 5:21 AM CEST, Marek Vasut wrote:

Thank you for testing and keeping up with this. I will wait for more
feedback if there is any (Frieder? Lucas? Michael?). If there are no
objections, then I can merge it in a week or two ?


I'll try to use your approach on the tc358775. Hopefully, I'll find
some time this week.


So ... I wonder ... shall I apply these patches or not ?

I'll wait about a week or two before applying them, to get some input.


I've pointed that they break current users of Samsung DSIM: Exynos-DSI
and Samsung s6e3ha2/s6e3hf2 panels, but unfortunately I'm not able to
provide datasheet nor any other documentation. Due to other tasks and
holidays I'm not able to debug it further too, at least till the end of
August. Maybe we could keep old behavior for "exynos-dsi" compatible device?


Nope, let's not pile up workarounds. It would be good to figure out why 
does this display misbehave when the data lanes are in LP11 on start up. 
It seems most sinks do require data lanes in LP11 on start up, so what 
is special about this panel ? Do you have access to the datasheet and 
can you check once you're back ?

Re: [PATCH 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

On 7/14/24 3:37 AM, Dan Carpenter wrote:

[...]


+err_irq_nfb4eof:
+   ipu_idmac_put(priv->vdi_out_ch);
+err_out:
+   ipu_idmac_put(priv->vdi_in_ch_n);
+err_next:
+   ipu_idmac_put(priv->vdi_in_ch);
+err_curr:
+   ipu_idmac_put(priv->vdi_in_ch_p);
+err_prev:
+   ipu_ic_put(priv->ic);
+err_ic:
+   ipu_vdi_put(priv->vdi);
+err_vdi:
+   devm_kfree(priv->dev, eofname);

 ^^


+err_eof:
+   devm_kfree(priv->dev, nfbname);

 ^^
Any time we call devm_kfree() it's a red flag.  Sometimes it makes sense
but I haven't looked at it closely enough to see how it makes sense
here.  Is it an ordering issue where we had to do devm_free_irq() and
then we just freed oefname and nfbname for consistency and because why
not?


I think in this case, the devm_*free() can be dropped, yes.

The rest is addressed in V2. I'll wait a bit for more feedback before 
sending it.


Thanks !

[PATCH 1/2] gpu: ipu-v3: vdic: Simplify ipu_vdi_setup()

[Marek Vasut]

The 'code' parameter only ever selects between YUV 4:2:0 and 4:2:2
subsampling, turn it into boolean to select exactly that and update
related code accordingly.

Signed-off-by: Marek Vasut 
---
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Fabio Estevam 
Cc: Greg Kroah-Hartman 
Cc: Helge Deller 
Cc: Mauro Carvalho Chehab 
Cc: Pengutronix Kernel Team 
Cc: Philipp Zabel 
Cc: Sascha Hauer 
Cc: Shawn Guo 
Cc: Steve Longerbeam 
Cc: dri-devel@lists.freedesktop.org
Cc: i...@lists.linux.dev
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-fb...@vger.kernel.org
Cc: linux-me...@vger.kernel.org
Cc: linux-stag...@lists.linux.dev
---
 drivers/gpu/ipu-v3/ipu-vdi.c   | 14 +++---
 drivers/staging/media/imx/imx-media-vdic.c |  3 +--
 include/video/imx-ipu-v3.h |  2 +-
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-vdi.c b/drivers/gpu/ipu-v3/ipu-vdi.c
index a593b232b6d3e..4df2821977c0c 100644
--- a/drivers/gpu/ipu-v3/ipu-vdi.c
+++ b/drivers/gpu/ipu-v3/ipu-vdi.c
@@ -117,10 +117,10 @@ void ipu_vdi_set_motion(struct ipu_vdi *vdi, enum 
ipu_motion_sel motion_sel)
 }
 EXPORT_SYMBOL_GPL(ipu_vdi_set_motion);
 
-void ipu_vdi_setup(struct ipu_vdi *vdi, u32 code, int xres, int yres)
+void ipu_vdi_setup(struct ipu_vdi *vdi, bool yuv422not420, int xres, int yres)
 {
unsigned long flags;
-   u32 pixel_fmt, reg;
+   u32 reg;
 
spin_lock_irqsave(&vdi->lock, flags);
 
@@ -131,16 +131,8 @@ void ipu_vdi_setup(struct ipu_vdi *vdi, u32 code, int 
xres, int yres)
 * Full motion, only vertical filter is used.
 * Burst size is 4 accesses
 */
-   if (code == MEDIA_BUS_FMT_UYVY8_2X8 ||
-   code == MEDIA_BUS_FMT_UYVY8_1X16 ||
-   code == MEDIA_BUS_FMT_YUYV8_2X8 ||
-   code == MEDIA_BUS_FMT_YUYV8_1X16)
-   pixel_fmt = VDI_C_CH_422;
-   else
-   pixel_fmt = VDI_C_CH_420;
-
reg = ipu_vdi_read(vdi, VDI_C);
-   reg |= pixel_fmt;
+   reg |= yuv422not420 ? VDI_C_CH_422 : VDI_C_CH_420;
reg |= VDI_C_BURST_SIZE2_4;
reg |= VDI_C_BURST_SIZE1_4 | VDI_C_VWM1_CLR_2;
reg |= VDI_C_BURST_SIZE3_4 | VDI_C_VWM3_CLR_2;
diff --git a/drivers/staging/media/imx/imx-media-vdic.c 
b/drivers/staging/media/imx/imx-media-vdic.c
index 09da4103a8dbe..ea5b4ef3573de 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -376,8 +376,7 @@ static int vdic_start(struct vdic_priv *priv)
 * only supports 4:2:2 or 4:2:0, and this subdev will only
 * negotiate 4:2:2 at its sink pads.
 */
-   ipu_vdi_setup(priv->vdi, MEDIA_BUS_FMT_UYVY8_2X8,
- infmt->width, infmt->height);
+   ipu_vdi_setup(priv->vdi, true, infmt->width, infmt->height);
ipu_vdi_set_field_order(priv->vdi, V4L2_STD_UNKNOWN, infmt->field);
ipu_vdi_set_motion(priv->vdi, priv->motion);
 
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index c422a403c0990..75f435d024895 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -466,7 +466,7 @@ void ipu_ic_dump(struct ipu_ic *ic);
 struct ipu_vdi;
 void ipu_vdi_set_field_order(struct ipu_vdi *vdi, v4l2_std_id std, u32 field);
 void ipu_vdi_set_motion(struct ipu_vdi *vdi, enum ipu_motion_sel motion_sel);
-void ipu_vdi_setup(struct ipu_vdi *vdi, u32 code, int xres, int yres);
+void ipu_vdi_setup(struct ipu_vdi *vdi, bool yuv422not420, int xres, int yres);
 void ipu_vdi_unsetup(struct ipu_vdi *vdi);
 int ipu_vdi_enable(struct ipu_vdi *vdi);
 int ipu_vdi_disable(struct ipu_vdi *vdi);
-- 
2.43.0

[PATCH 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

Introduce dedicated memory-to-memory IPUv3 VDI deinterlacer driver.
Currently the IPUv3 can operate VDI in DIRECT mode, from sensor to
memory. This only works for single stream, that is, one input from
one camera is deinterlaced on the fly with a helper buffer in DRAM
and the result is written into memory.

The i.MX6Q/QP does support up to four analog cameras via two IPUv3
instances, each containing one VDI deinterlacer block. In order to
deinterlace all four streams from all four analog cameras live, it
is necessary to operate VDI in INDIRECT mode, where the interlaced
streams are written to buffers in memory, and then deinterlaced in
memory using VDI in INDIRECT memory-to-memory mode.

This driver also makes use of the IDMAC->VDI->IC->IDMAC data path
to provide pixel format conversion from input YUV formats to both
output YUV or RGB formats. The later is useful in case the data
are imported into the GPU, which on this platform cannot directly
sample YUV buffers.

This is derived from previous work by Steve Longerbeam and from the
IPUv3 CSC Scaler mem2mem driver.

Signed-off-by: Marek Vasut 
---
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Fabio Estevam 
Cc: Greg Kroah-Hartman 
Cc: Helge Deller 
Cc: Mauro Carvalho Chehab 
Cc: Pengutronix Kernel Team 
Cc: Philipp Zabel 
Cc: Sascha Hauer 
Cc: Shawn Guo 
Cc: Steve Longerbeam 
Cc: dri-devel@lists.freedesktop.org
Cc: i...@lists.linux.dev
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-fb...@vger.kernel.org
Cc: linux-me...@vger.kernel.org
Cc: linux-stag...@lists.linux.dev
---
 drivers/staging/media/imx/Makefile|   2 +-
 drivers/staging/media/imx/imx-media-dev.c |  50 +
 .../media/imx/imx-media-mem2mem-vdic.c| 992 ++
 drivers/staging/media/imx/imx-media.h |   9 +
 4 files changed, 1052 insertions(+), 1 deletion(-)
 create mode 100644 drivers/staging/media/imx/imx-media-mem2mem-vdic.c

diff --git a/drivers/staging/media/imx/Makefile 
b/drivers/staging/media/imx/Makefile
index 330e0825f506b..0cad87123b590 100644
--- a/drivers/staging/media/imx/Makefile
+++ b/drivers/staging/media/imx/Makefile
@@ -4,7 +4,7 @@ imx-media-common-objs := imx-media-capture.o 
imx-media-dev-common.o \
 
 imx6-media-objs := imx-media-dev.o imx-media-internal-sd.o \
imx-ic-common.o imx-ic-prp.o imx-ic-prpencvf.o imx-media-vdic.o \
-   imx-media-csc-scaler.o
+   imx-media-mem2mem-vdic.o imx-media-csc-scaler.o
 
 imx6-media-csi-objs := imx-media-csi.o imx-media-fim.o
 
diff --git a/drivers/staging/media/imx/imx-media-dev.c 
b/drivers/staging/media/imx/imx-media-dev.c
index be54dca11465d..b75eec4513eab 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -57,7 +57,47 @@ static int imx6_media_probe_complete(struct 
v4l2_async_notifier *notifier)
goto unlock;
}
 
+   imxmd->m2m_vdic[0] = imx_media_mem2mem_vdic_init(imxmd, 0);
+   if (IS_ERR(imxmd->m2m_vdic[0])) {
+   ret = PTR_ERR(imxmd->m2m_vdic[0]);
+   imxmd->m2m_vdic[0] = NULL;
+   goto unlock;
+   }
+
+   /* MX6S/DL has one IPUv3, init second VDI only on MX6Q/QP */
+   if (imxmd->ipu[1]) {
+   imxmd->m2m_vdic[1] = imx_media_mem2mem_vdic_init(imxmd, 1);
+   if (IS_ERR(imxmd->m2m_vdic[1])) {
+   ret = PTR_ERR(imxmd->m2m_vdic[1]);
+   imxmd->m2m_vdic[1] = NULL;
+   goto unlock;
+   }
+   }
+
ret = imx_media_csc_scaler_device_register(imxmd->m2m_vdev);
+   if (ret)
+   goto unlock;
+
+   ret = imx_media_mem2mem_vdic_register(imxmd->m2m_vdic[0]);
+   if (ret)
+   goto unreg_csc;
+
+   /* MX6S/DL has one IPUv3, init second VDI only on MX6Q/QP */
+   if (imxmd->ipu[1]) {
+   ret = imx_media_mem2mem_vdic_register(imxmd->m2m_vdic[1]);
+   if (ret)
+   goto unreg_vdic;
+   }
+
+   mutex_unlock(&imxmd->mutex);
+   return ret;
+
+unreg_vdic:
+   imx_media_mem2mem_vdic_unregister(imxmd->m2m_vdic[0]);
+   imxmd->m2m_vdic[0] = NULL;
+unreg_csc:
+   imx_media_csc_scaler_device_unregister(imxmd->m2m_vdev);
+   imxmd->m2m_vdev = NULL;
 unlock:
mutex_unlock(&imxmd->mutex);
return ret;
@@ -108,6 +148,16 @@ static void imx_media_remove(struct platform_device *pdev)
 
v4l2_info(&imxmd->v4l2_dev, "Removing imx-media\n");
 
+   if (imxmd->m2m_vdic[1]) {   /* MX6Q/QP only */
+   imx_media_mem2mem_vdic_unregister(imxmd->m2m_vdic[1]);
+   imxmd->m2m_vdic[1] = NULL;
+   }
+
+   if (imxmd->m2m_vdic[0]) {
+   imx_media_mem2mem_vdic_unregister(imxmd->m2m_vdic[0]);
+   imxmd->m2m_vdic[0] = NULL;
+   }
+
if (imxmd->m2m_vdev) {
imx_m

[PATCH] gpu: ipu-v3: image-convert: Drop unused single conversion request code

[Marek Vasut]

Neither ipu_image_convert_sync() nor ipu_image_convert() is used or call
from anywhere. Remove this unused code.

Signed-off-by: Marek Vasut 
---
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Fabio Estevam 
Cc: Greg Kroah-Hartman 
Cc: Helge Deller 
Cc: Mauro Carvalho Chehab 
Cc: Pengutronix Kernel Team 
Cc: Philipp Zabel 
Cc: Sascha Hauer 
Cc: Shawn Guo 
Cc: Steve Longerbeam 
Cc: dri-devel@lists.freedesktop.org
Cc: i...@lists.linux.dev
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-fb...@vger.kernel.org
Cc: linux-me...@vger.kernel.org
Cc: linux-stag...@lists.linux.dev
---
 drivers/gpu/ipu-v3/ipu-image-convert.c | 76 --
 include/video/imx-ipu-image-convert.h  | 46 
 2 files changed, 122 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c 
b/drivers/gpu/ipu-v3/ipu-image-convert.c
index 841316582ea9d..c87866253eee9 100644
--- a/drivers/gpu/ipu-v3/ipu-image-convert.c
+++ b/drivers/gpu/ipu-v3/ipu-image-convert.c
@@ -2395,82 +2395,6 @@ void ipu_image_convert_unprepare(struct 
ipu_image_convert_ctx *ctx)
 }
 EXPORT_SYMBOL_GPL(ipu_image_convert_unprepare);
 
-/*
- * "Canned" asynchronous single image conversion. Allocates and returns
- * a new conversion run.  On successful return the caller must free the
- * run and call ipu_image_convert_unprepare() after conversion completes.
- */
-struct ipu_image_convert_run *
-ipu_image_convert(struct ipu_soc *ipu, enum ipu_ic_task ic_task,
- struct ipu_image *in, struct ipu_image *out,
- enum ipu_rotate_mode rot_mode,
- ipu_image_convert_cb_t complete,
- void *complete_context)
-{
-   struct ipu_image_convert_ctx *ctx;
-   struct ipu_image_convert_run *run;
-   int ret;
-
-   ctx = ipu_image_convert_prepare(ipu, ic_task, in, out, rot_mode,
-   complete, complete_context);
-   if (IS_ERR(ctx))
-   return ERR_CAST(ctx);
-
-   run = kzalloc(sizeof(*run), GFP_KERNEL);
-   if (!run) {
-   ipu_image_convert_unprepare(ctx);
-   return ERR_PTR(-ENOMEM);
-   }
-
-   run->ctx = ctx;
-   run->in_phys = in->phys0;
-   run->out_phys = out->phys0;
-
-   ret = ipu_image_convert_queue(run);
-   if (ret) {
-   ipu_image_convert_unprepare(ctx);
-   kfree(run);
-   return ERR_PTR(ret);
-   }
-
-   return run;
-}
-EXPORT_SYMBOL_GPL(ipu_image_convert);
-
-/* "Canned" synchronous single image conversion */
-static void image_convert_sync_complete(struct ipu_image_convert_run *run,
-   void *data)
-{
-   struct completion *comp = data;
-
-   complete(comp);
-}
-
-int ipu_image_convert_sync(struct ipu_soc *ipu, enum ipu_ic_task ic_task,
-  struct ipu_image *in, struct ipu_image *out,
-  enum ipu_rotate_mode rot_mode)
-{
-   struct ipu_image_convert_run *run;
-   struct completion comp;
-   int ret;
-
-   init_completion(&comp);
-
-   run = ipu_image_convert(ipu, ic_task, in, out, rot_mode,
-   image_convert_sync_complete, &comp);
-   if (IS_ERR(run))
-   return PTR_ERR(run);
-
-   ret = wait_for_completion_timeout(&comp, msecs_to_jiffies(1));
-   ret = (ret == 0) ? -ETIMEDOUT : 0;
-
-   ipu_image_convert_unprepare(run->ctx);
-   kfree(run);
-
-   return ret;
-}
-EXPORT_SYMBOL_GPL(ipu_image_convert_sync);
-
 int ipu_image_convert_init(struct ipu_soc *ipu, struct device *dev)
 {
struct ipu_image_convert_priv *priv;
diff --git a/include/video/imx-ipu-image-convert.h 
b/include/video/imx-ipu-image-convert.h
index 3c71b8b94b33a..39906b0cbf2d8 100644
--- a/include/video/imx-ipu-image-convert.h
+++ b/include/video/imx-ipu-image-convert.h
@@ -149,50 +149,4 @@ int ipu_image_convert_queue(struct ipu_image_convert_run 
*run);
  */
 void ipu_image_convert_abort(struct ipu_image_convert_ctx *ctx);
 
-/**
- * ipu_image_convert() - asynchronous image conversion request
- *
- * @ipu:   the IPU handle to use for the conversion
- * @ic_task:   the IC task to use for the conversion
- * @in:input image format
- * @out:   output image format
- * @rot_mode:  rotation mode
- * @complete:  run completion callback
- * @complete_context:  a context pointer for the completion callback
- *
- * Request a single image conversion. Returns the run that has been queued.
- * A conversion context is automatically created and is available in run->ctx.
- * As with ipu_image_convert_prepare(), the input/output formats and rotation
- * mode must already meet IPU retrictions.
- *
- * On successful return the caller can queue more run requests if needed, using
- * the prepared context in run->ctx. The caller is responsible for unpreparing
- * the context when no more conversion

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 6/26/24 10:02 AM, Michael Walle wrote:

On Wed Jun 26, 2024 at 5:21 AM CEST, Marek Vasut wrote:

Thank you for testing and keeping up with this. I will wait for more
feedback if there is any (Frieder? Lucas? Michael?). If there are no
objections, then I can merge it in a week or two ?


I'll try to use your approach on the tc358775. Hopefully, I'll find
some time this week.


So ... I wonder ... shall I apply these patches or not ?

I'll wait about a week or two before applying them, to get some input.

Re: [PATCH] dt-bindings: display: bridge: ti,sn65dsi83: add burst-mode-disabled

[Marek Vasut]

On 7/9/24 7:30 PM, Stefano Radaelli wrote:

Okay, I get it.

So if you think this mode shouldn't be implemented within this driver, we
can close the thread.
Just for information, this driver has been implemented from the work done
by Compulab (as it says in the driver's initial comments), and they do not
put the burst mode by default, not even giving the possibility to activate
it by dts:
https://github.com/compulab-yokneam/imx8-android/blob/master/o8/vendor/nxp-opensource/kernel_imx/0055-sn65dsi83-Add-ti-sn65dsi83-dsi-to-lvds-bridge-driver.patch


This is not the mainline Linux driver.


The panels that I've had these problems with are some of JuTouch's
1920x1200, for example JT101TM015 , and I solved it by giving the option to
remove this mode.
I have also heard from other colleagues who have had the same problem on
some dual-channel displays.


Does that problem happen with the aforementioned driver or the mainline 
Linux driver ?

Re: [PATCH] dt-bindings: display: bridge: ti,sn65dsi83: add burst-mode-disabled

[Marek Vasut]

On 7/9/24 4:44 PM, Stefano Radaelli wrote:

Hi Marek,


Hi,


Actually this property is specific also to DSI8x bridge, as you can see
from the screenshot below taken from official datasheet:

[image: image.png]

And it's the sn65dsi8x driver that tells MIPI driver which flags to use
during attachment.


There are other bridges and panels which support both DSI burst and 
sync-pulse/sync-events modes, so a property which selects the mode is 
generic, not specific to this particular bridge . The bridge driver 
could parse such generic property, although it would be better if the 
core code parsed it instead.



So, for example, this bridge can work also for MIPI interfaces which don't
support burst-mode.
Also, as a value-added benefit, I found non-burst mode better for some
1920x1200 LVDS panels I'm testing (Of course with more energy consumption).
That's why I though it could be useful have this option, since SN65DSI8x
supports both modes.


Can you share which panel model this is ?

Re: [PATCH] dt-bindings: display: bridge: ti,sn65dsi83: add burst-mode-disabled

[Marek Vasut]

On 7/9/24 2:45 PM, Stefano Radaelli wrote:

Hello everyone,


Hi,


Thank you a lot for your prompt feedbacks.
I'm really sorry for all the mistakes, it is the first time that I try to
submit a patch and i thought I followed the guideline but clearly that was
not the case.

  @Marek Vasut  About your question to why disabling
burst-mode:
- I agree with you that Burst Mode is the preferred way to send data. For
that reason I created the new flag in a way that, if not used in dts, burst
mode remains active by default.
   However, I decide to introduced this property because I have noticed that
some dual-channel panels work better in non-burst mode (even if less
efficient), and since the sn65dsi84 datasheet allows this setting, I
thought to give this opportunity to users.
   What do you think about it?


Are there any further details, which panels behave this way ? Does your 
DSI host generate correct HS clock, ones which the DSI84 expects to 
receive on the DSI side ?


Such link mode properties would have to be generic properties placed in 
some dsi-client.yaml file in any case, such properties are not specific 
to this DSI8x bridge.

Re: [PATCH v3 1/2] dt-bindings: display: bridge: tc358867: Document default DP preemphasis

[Marek Vasut]

On 6/26/24 9:29 AM, Alexander Stein wrote:

Hi,


+
  oneOf:
- required:
- port@0



I get this warning:

mx8mp-tqma8mpql-mba8mpxl.dtb: bridge@f: ports:port@2:endpoint: Unevaluated
properties are not allowed ('toshiba,pre-emphasis' was unexpected)


DT node looks like this:

port@2 {
reg = <2>;

endpoint {
toshiba,pre-emphasis = /bits/ 8 <1 1>;
};
};

I think you are missing this change as well:
-- 8< --
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
@@ -92,7 +92,8 @@ properties:
  reference to a valid DPI output or input endpoint node.
  
port@2:

-$ref: /schemas/graph.yaml#/properties/port
+$ref: /schemas/graph.yaml#/$defs/port-base
+unevaluatedProperties: false
  description: |
  eDP/DP output port. The remote endpoint phandle should be a
  reference to a valid eDP panel input endpoint node. This port is
-- 8< --


Picked for V4, thank you !


How would you determine the values to be set here? I suspect it's the value
from register DP0_SnkLTChReq (0x6d4) after link training. Are they dependent
on the actual display to be attached?


In my case, I only did trial-and-error, because the test hardware I have 
available right now ... well, it is a test hardware and won't work 
reliably with pre-emphasis 0, so I had to up it a bit.

Re: [PATCH] dt-bindings: display: bridge: ti,sn65dsi83: add burst-mode-disabled

[Marek Vasut]

On 7/8/24 5:18 PM, stefano.radaell...@gmail.com wrote:

From: Stefano Radaelli 

It allows to disable Burst video mode


Why would you want to disable burst mode ? This is the energy efficient 
and recommended mode, why disable it ? Details please ?

Re: [PATCH v3 2/2] drm/bridge: tc358767: Add configurable default preemphasis

[Marek Vasut]

On 6/26/24 9:36 AM, Alexander Stein wrote:

Hi,

sorry for the late reply.


@@ -2435,6 +2454,18 @@ static int tc_probe_bridge_endpoint(struct tc_data *tc)
return -EINVAL;
}
mode |= BIT(endpoint.port);
+   if (endpoint.port != 2)
+   continue;
+


Mh, I know currently there are not other port-specific properties. But
maybe it's easier to read if 'if (endpoint.port == 2) {' is used.


Fixed in V4, thanks.

[PATCH v4 2/2] drm/bridge: tc358767: Add configurable default preemphasis

[Marek Vasut]

Make the default DP port preemphasis configurable via new DT property
"toshiba,pre-emphasis". This is useful in case the DP link properties
are known and starting link training from preemphasis setting of 0 dB
is not useful. The preemphasis can be set separately for both DP lanes
in range 0=0dB, 1=3.5dB, 2=6dB .

Acked-by: Alexander Stein 
Signed-off-by: Marek Vasut 
---
Cc: Alexander Stein 
Cc: Andrzej Hajda 
Cc: Conor Dooley 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Krzysztof Kozlowski 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Rob Herring 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: devicet...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: - Parse toshiba,pre-emphasis property out of an endpoint of port 2 (the DP 
port)
V3: - No change
V4: - Invert the if (endpoint.port == 2) conditional in 
tc_probe_bridge_endpoint()
- Add AB from Alexander
---
 drivers/gpu/drm/bridge/tc358767.c | 45 ++-
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index dde1b2734c98a..a13e4898a210c 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -241,6 +241,10 @@
 
 /* Link Training */
 #define DP0_SRCCTRL0x06a0
+#define DP0_SRCCTRL_PRE1   GENMASK(29, 28)
+#define DP0_SRCCTRL_SWG1   GENMASK(25, 24)
+#define DP0_SRCCTRL_PRE0   GENMASK(21, 20)
+#define DP0_SRCCTRL_SWG0   GENMASK(17, 16)
 #define DP0_SRCCTRL_SCRMBLDIS  BIT(13)
 #define DP0_SRCCTRL_EN810B BIT(12)
 #define DP0_SRCCTRL_NOTP   (0 << 8)
@@ -278,6 +282,8 @@
 #define AUDIFDATA6 0x0720  /* DP0 Audio Info Frame Bytes 27 to 24 
*/
 
 #define DP1_SRCCTRL0x07a0  /* DP1 Control Register */
+#define DP1_SRCCTRL_PREGENMASK(21, 20)
+#define DP1_SRCCTRL_SWGGENMASK(17, 16)
 
 /* PHY */
 #define DP_PHY_CTRL0x0800
@@ -369,6 +375,7 @@ struct tc_data {
 
u32 rev;
u8  assr;
+   u8  pre_emphasis[2];
 
struct gpio_desc*sd_gpio;
struct gpio_desc*reset_gpio;
@@ -1090,13 +1097,17 @@ static int tc_main_link_enable(struct tc_data *tc)
return ret;
}
 
-   ret = regmap_write(tc->regmap, DP0_SRCCTRL, tc_srcctrl(tc));
+   ret = regmap_write(tc->regmap, DP0_SRCCTRL,
+  tc_srcctrl(tc) |
+  FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_emphasis[0]) |
+  FIELD_PREP(DP0_SRCCTRL_PRE1, tc->pre_emphasis[1]));
if (ret)
return ret;
/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
ret = regmap_write(tc->regmap, DP1_SRCCTRL,
 (tc->link.spread ? DP0_SRCCTRL_SSCG : 0) |
-((tc->link.rate != 162000) ? DP0_SRCCTRL_BW27 : 0));
+((tc->link.rate != 162000) ? DP0_SRCCTRL_BW27 : 0) |
+FIELD_PREP(DP1_SRCCTRL_PRE, tc->pre_emphasis[1]));
if (ret)
return ret;
 
@@ -1188,8 +1199,10 @@ static int tc_main_link_enable(struct tc_data *tc)
goto err_dpcd_write;
 
/* Reset voltage-swing & pre-emphasis */
-   tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
- DP_TRAIN_PRE_EMPH_LEVEL_0;
+   tmp[0] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
+FIELD_PREP(DP_TRAIN_PRE_EMPHASIS_MASK, tc->pre_emphasis[0]);
+   tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
+FIELD_PREP(DP_TRAIN_PRE_EMPHASIS_MASK, tc->pre_emphasis[1]);
ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2);
if (ret < 0)
goto err_dpcd_write;
@@ -1213,7 +1226,9 @@ static int tc_main_link_enable(struct tc_data *tc)
ret = regmap_write(tc->regmap, DP0_SRCCTRL,
   tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
   DP0_SRCCTRL_AUTOCORRECT |
-  DP0_SRCCTRL_TP1);
+  DP0_SRCCTRL_TP1 |
+  FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_emphasis[0]) |
+  FIELD_PREP(DP0_SRCCTRL_PRE1, tc->pre_emphasis[1]));
if (ret)
return ret;
 
@@ -1248,7 +1263,9 @@ static int tc_main_link_enable(struct tc_data *tc)
ret = regmap_write(tc->regmap, DP0_SRCCTRL,
   tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
   DP0_SRCCTRL_AUTOCORRECT |
-  DP0_SRCCTRL_TP2);
+  DP0_SRCCTRL_TP2 |
+  FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_em

[PATCH v4 1/2] dt-bindings: display: bridge: tc358867: Document default DP preemphasis

[Marek Vasut]

Document default DP port preemphasis configurable via new DT property
"toshiba,pre-emphasis". This is useful in case the DP link properties
are known and starting link training from preemphasis setting of 0 dB
is not useful. The preemphasis can be set separately for both DP lanes
in range 0=0dB, 1=3.5dB, 2=6dB .

This is an endpoint property, not a port property, because the TC9595
datasheet does mention that the DP might operate in some sort of split
mode, where each DP lane is used to feed one display, so in that case
there might be two endpoints.

Signed-off-by: Marek Vasut 
---
Cc: Alexander Stein 
Cc: Andrzej Hajda 
Cc: Conor Dooley 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Krzysztof Kozlowski 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Rob Herring 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: devicet...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: - Fix the type to u8 array
- Fix the enum items to match what they represent
V3: - Update commit message, expand on this being an endpoint property
V4: - Fix ref: /schemas/graph.yaml#/$defs/port-base and add 
unevaluatedProperties
---
 .../display/bridge/toshiba,tc358767.yaml  | 21 ++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml 
b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
index 2ad0cd6dd49e0..b78f64c9c5f44 100644
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
@@ -92,12 +92,31 @@ properties:
 reference to a valid DPI output or input endpoint node.
 
   port@2:
-$ref: /schemas/graph.yaml#/properties/port
+$ref: /schemas/graph.yaml#/$defs/port-base
+unevaluatedProperties: false
 description: |
 eDP/DP output port. The remote endpoint phandle should be a
 reference to a valid eDP panel input endpoint node. This port is
 optional, treated as DP panel if not defined
 
+properties:
+  endpoint:
+$ref: /schemas/media/video-interfaces.yaml#
+unevaluatedProperties: false
+
+properties:
+  toshiba,pre-emphasis:
+description:
+  Display port output Pre-Emphasis settings for both DP lanes.
+$ref: /schemas/types.yaml#/definitions/uint8-array
+minItems: 2
+maxItems: 2
+items:
+  enum:
+- 0 # No pre-emphasis
+- 1 # 3.5dB pre-emphasis
+- 2 # 6dB pre-emphasis
+
 oneOf:
   - required:
   - port@0
-- 
2.43.0

Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 6/24/24 11:19 AM, Alexander Stein wrote:

Am Freitag, 31. Mai 2024, 22:27:21 CEST schrieb Marek Vasut:

In case an upstream bridge modified the required clock frequency
in its .atomic_check callback by setting adjusted_mode.clock ,
make sure that clock frequency is generated by the LCDIFv3 block.

This is useful e.g. when LCDIFv3 feeds DSIM which feeds TC358767
with (e)DP output, where the TC358767 expects precise timing on
its input side, the precise timing must be generated by the LCDIF.

Signed-off-by: Marek Vasut 


With the other rc358767 patches in place, this does the trick.
Reviewed-by: Alexander Stein 


I'll pick this up next week if there is no objection.

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 6/25/24 4:37 PM, Alexander Stein wrote:

Hi Marek,


Hi,


Am Dienstag, 25. Juni 2024, 14:26:10 CEST schrieb Marek Vasut:

Initialize the bridge on attach already, to force lanes into LP11
state, since attach does trigger attach of downstream bridges which
may trigger (e)DP AUX channel mode read.

This fixes a corner case where DSIM with TC9595 attached to it fails
to operate the DP AUX channel, because the TC9595 enters some debug
mode when it is released from reset without lanes in LP11 mode. By
ensuring the DSIM lanes are in LP11, the TC9595 (tc358767.c driver)
can be reset in its attach callback called from DSIM attach callback,
and recovered out of the debug mode just before TC9595 performs first
AUX channel access later in its attach callback.

Signed-off-by: Marek Vasut 
---
Cc: Adam Ford 
Cc: Alexander Stein 
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Frieder Schrempf 
Cc: Inki Dae 
Cc: Jagan Teki 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Marek Szyprowski 
Cc: Maxime Ripard 
Cc: Michael Walle 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: Handle case where mode is not set yet
---
  drivers/gpu/drm/bridge/samsung-dsim.c | 32 ---
  1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
b/drivers/gpu/drm/bridge/samsung-dsim.c
index e7e53a9e42afb..22d3bbd866d97 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -699,20 +699,24 @@ 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, pix_clk;
+   unsigned long hs_clk, byte_clk, esc_clk;
unsigned long esc_div;
u32 reg;
struct drm_display_mode *m = &dsi->mode;
int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
  
-	/* m->clock is in KHz */

-   pix_clk = m->clock * 1000;
-
-   /* Use burst_clk_rate if available, otherwise use the pix_clk */
+   /*
+* Use burst_clk_rate if available, otherwise use the mode clock
+* if mode is already set and available, otherwise fall back to
+* PLL input clock and operate in 1:1 lowest frequency mode until
+* a mode is set.
+*/
if (dsi->burst_clk_rate)
hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
+   else if (m) /* m->clock is in KHz */
+   hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(m->clock * 1000 * 
bpp, dsi->lanes));
else
-   hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, 
dsi->lanes));
+   hs_clk = dsi->pll_clk_rate;
  


I can't reproduce that mentioned corner case and presumably this problem
does not exist otherwise. If samsung,burst-clock-frequency is unset I get
a sluggish image on the monitor.

It seems the calculation is using a adjusted clock frequency:
samsung-dsim 32e6.dsi: Using pixel clock for HS clock frequency
samsung-dsim 32e6.dsi: [drm:samsung_dsim_host_attach [samsung_dsim]] 
Attached tc358767 device (lanes:4 bpp:24 mode-flags:0xc03)
samsung-dsim 32e6.dsi: PLL ref clock freq 2400
samsung-dsim 32e6.dsi: failed to find PLL PMS for requested frequency
samsung-dsim 32e6.dsi: failed to configure DSI PLL
samsung-dsim 32e6.dsi: PLL ref clock freq 2400
samsung-dsim 32e6.dsi: PLL freq 883636363, (p 11, m 810, s 1)
samsung-dsim 32e6.dsi: hs_clk = 883636363, byte_clk = 110454545, esc_clk = 
9204545
samsung-dsim 32e6.dsi: calculated hfp: 60, hbp: 105, hsa: 27
samsung-dsim 32e6.dsi: LCD size = 1920x1080

891 MHz is the nominal one for 148.5 MHz pixelclock. But even setting
samsung,burst-clock-frequency to 891 MHz results in a sluggish image.
Maybe this usecase is nothing I need to consider while using DSI-DP
with EDID timings available.

As the burst clock needs to provide more bandwidth than actually needed,
I consider this a different usecase not providing
samsung,burst-clock-frequency for DSI->DP usage.

But the initialization upon attach is working intended, so:
Reviewed-by: Alexander Stein 


Thank you for testing and keeping up with this. I will wait for more 
feedback if there is any (Frieder? Lucas? Michael?). If there are no 
objections, then I can merge it in a week or two ?

Re: [PATCH 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 5/16/24 8:51 AM, Alexander Stein wrote:

Hi Marek,

thanks for the patch.

Am Montag, 13. Mai 2024, 04:16:27 CEST schrieb Marek Vasut:

Initialize the bridge on attach already, to force lanes into LP11
state, since attach does trigger attach of downstream bridges which
may trigger (e)DP AUX channel mode read.

This fixes a corner case where DSIM with TC9595 attached to it fails
to operate the DP AUX channel, because the TC9595 enters some debug
mode when it is released from reset without lanes in LP11 mode. By
ensuring the DSIM lanes are in LP11, the TC9595 (tc358767.c driver)
can be reset in its attach callback called from DSIM attach callback,
and recovered out of the debug mode just before TC9595 performs first
AUX channel access later in its attach callback.

Signed-off-by: Marek Vasut 
---
Cc: Adam Ford 
Cc: Alexander Stein 
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Frieder Schrempf 
Cc: Inki Dae 
Cc: Jagan Teki 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Marek Szyprowski 
Cc: Maxime Ripard 
Cc: Michael Walle 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
  drivers/gpu/drm/bridge/samsung-dsim.c | 16 ++--
  1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 95fedc68b0ae5..56093fc3d62cc 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1622,9 +1622,21 @@ static int samsung_dsim_attach(struct drm_bridge *bridge,
   enum drm_bridge_attach_flags flags)
  {
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+   int ret;
+
+   ret = pm_runtime_resume_and_get(dsi->dev);
+   if (ret < 0)
+   return ret;
  
-	return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,

-flags);
+   ret = samsung_dsim_init(dsi);
+   if (ret < 0)
+   goto err;
+
+   ret = drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
+   flags);
+err:
+   pm_runtime_put_sync(dsi->dev);
+   return ret;
  }
  
  static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {




It seems the right thing to do. But if 'samsung,burst-clock-frequency' is
not specified for DSIM I get a DSI PLL configuration failure. There is no
mode yet, thus samsung_dsim_enable_clock() tries to configure the PLL for
0Hz. There is another reconfiguration while pre_enabling the chain targeting
the correct clock (89100Hz for 1920x1080), so this seems fine.
But I'm not sure if the 1st config error has any negative side effects.


Will be fixed in V2, thanks for pointing this out.

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

On 6/25/24 8:11 AM, Alexander Stein wrote:

Hi Marek,

Am Dienstag, 25. Juni 2024, 02:33:53 CEST schrieb Marek Vasut:

On 6/24/24 11:26 AM, Alexander Stein wrote:

Hi Marek,


Hi,


Am Freitag, 21. Juni 2024, 16:54:51 CEST schrieb Marek Vasut:

On 6/21/24 12:32 PM, Alexander Stein wrote:

Hi,

skipping the parts where I would simply write "OK" ...


As FVUEN is cleared at the next VSYNC event I suspect the DSI timings
are (slightly) off, but unfortunately I don't have equipment to check
DSI signal quality/timings.


As long as the LCDIFv3 pixel clock are equal or slightly slower than
what the TC9595 PixelPLL generates, AND, DSIM serializer has enough
bandwidth on the DSI bus (i.e. set the bus to 1 GHz, the TC9595 DSI RX
cannot go any faster), you should have no issues on that end.


I'm using samsung,burst-clock-frequency = <10> so this should be
okay. That is 1080p resolution.


Yes, correct.


When in doubt, try and use i2ctransfer to read out register 0x300
repeatedly, that's DSI RX error counter register. See if the DSI error
count increments.


If the bridge is not working the registers look like this:
300: c080
464: 0001

they are not changing and stay like that.

If the bridge is actually running they are like
300: c08000d3
464: 

and are also not changing.


Uh ... that looks like the whole chip clock tree somehow locked up .

Thinking about this, I once did force the DSIM into 24 MHz mode (there
is PLL bypass setting, where the DSIM uses 24 MHz serializer clock
directly for the DSI HS clock) or something close, it was enough to
drive a low resolution panel. But the upside was, with a 200 MHz 5Gsps
scope set to AC-coupling and 10x probe, I could discern the traffic on
DSI data lane and decode it by hand. The nice thing is, you could
trigger on 1V2 LP mode, so you know where the packet starts. The
downside is, if you have multiple data lanes, the packet is spread
across them.

You could also tweak tc_edp_atomic_check()/tc_edp_mode_valid() and force
only low(er) resolution modes of your DP panel right from the start, so
you wouldn't need that much DSI bandwidth. Maybe you could reach some
mode where your equipment is enough to analyze the traffic by hand ?


I think I got it running now. Apparently there were different, independent
problems which you addressed by your series.


Oh, glad I could help.


Unfortunately the patch
'tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS' introduced a new problem
(at least for me). For the record I'm running the following patch stack based
on next-20240621:


Thanks for tracking it down. I can drop that one
MIPI_DSI_CLOCK_NON_CONTINUOUS patch from the series and do a V4. Would
that work for you ? At least there would be some improvement to the
driver and I can analyze the MIPI_DSI_CLOCK_NON_CONTINUOUS issue in
detail separately.


Sure, thanks to your patches this bridge does its job now.
Sure, now that I have a reference system I can easily try a V4 without
the MIPI_DSI_CLOCK_NON_CONTINUOUS patch.


V4 is now out, thanks !

[PATCH v2 2/2] drm/bridge: tc358767: Reset chip again on attach

[Marek Vasut]

In case the chip is released from reset using the RESX signal while the
DSI lanes are in non-LP11 mode, the chip may enter some sort of debug
mode, where its internal clock run at 1/6th of expected clock rate. In
this mode, the AUX channel also operates at 1/6th of the 10 MHz mandated
by DP specification, which breaks DPCD communication.

There is no known software way of bringing the chip out of this state
once the chip enters it, except for toggling the RESX signal and
performing full reset.

The chip may enter this mode when the chip was released from reset in
probe(), because at that point the DSI lane mode is undefined.

When the .attach callback is called, the DSI link is surely in LP11 mode.
Toggle the RESX signal here and reconfigure the AUX channel. That way,
the AUX channel communication from this point on does surely run at
10 MHz as it should.

Reviewed-by: Alexander Stein 
Signed-off-by: Marek Vasut 
---
Cc: Adam Ford 
Cc: Alexander Stein 
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Frieder Schrempf 
Cc: Inki Dae 
Cc: Jagan Teki 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Marek Szyprowski 
Cc: Maxime Ripard 
Cc: Michael Walle 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: Add RB from Alexander
---
 drivers/gpu/drm/bridge/tc358767.c | 50 +++
 1 file changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 6bda26ca6b917..7d8797b3d8579 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1749,10 +1749,30 @@ static const struct drm_connector_funcs 
tc_connector_funcs = {
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+static void tc_bridge_reset(struct tc_data *tc)
+{
+   if (!tc->reset_gpio)
+   return;
+
+   gpiod_set_value_cansleep(tc->reset_gpio, 0);
+   usleep_range(1, 11000);
+   gpiod_set_value_cansleep(tc->reset_gpio, 1);
+   usleep_range(5000, 1);
+}
+
 static int tc_dpi_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags)
 {
struct tc_data *tc = bridge_to_tc(bridge);
+   int ret;
+
+   if (tc->reset_gpio) {
+   tc_bridge_reset(tc);
+
+   ret = tc_set_syspllparam(tc);
+   if (ret)
+   return ret;
+   }
 
if (!tc->panel_bridge)
return 0;
@@ -1769,6 +1789,36 @@ static int tc_edp_bridge_attach(struct drm_bridge 
*bridge,
struct drm_device *drm = bridge->dev;
int ret;
 
+   if (tc->reset_gpio) {
+   /*
+* In case the chip is released from reset using the RESX
+* signal while the DSI lanes are in non-LP11 mode, the chip
+* may enter some sort of debug mode, where its internal
+* clock run at 1/6th of expected clock rate. In this mode,
+* the AUX channel also operates at 1/6th of the 10 MHz
+* mandated by DP specification, which breaks DPCD
+* communication.
+*
+* There is no known software way of bringing the chip out of
+* this state once the chip enters it, except for toggling
+* the RESX signal and performing full reset.
+*
+* The chip may enter this mode when the chip was released
+* from reset in probe(), because at that point the DSI lane
+* mode is undefined.
+*
+* At this point, the DSI link is surely in LP11 mode. Toggle
+* the RESX signal here and reconfigure the AUX channel. That
+* way, the AUX channel communication from this point on does
+* surely run at 10 MHz as it should.
+*/
+   tc_bridge_reset(tc);
+
+   ret = tc_aux_link_setup(tc);
+   if (ret)
+   return ret;
+   }
+
if (tc->panel_bridge) {
/* If a connector is required then this driver shall create it 
*/
ret = drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
-- 
2.43.0

[PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

Initialize the bridge on attach already, to force lanes into LP11
state, since attach does trigger attach of downstream bridges which
may trigger (e)DP AUX channel mode read.

This fixes a corner case where DSIM with TC9595 attached to it fails
to operate the DP AUX channel, because the TC9595 enters some debug
mode when it is released from reset without lanes in LP11 mode. By
ensuring the DSIM lanes are in LP11, the TC9595 (tc358767.c driver)
can be reset in its attach callback called from DSIM attach callback,
and recovered out of the debug mode just before TC9595 performs first
AUX channel access later in its attach callback.

Signed-off-by: Marek Vasut 
---
Cc: Adam Ford 
Cc: Alexander Stein 
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Frieder Schrempf 
Cc: Inki Dae 
Cc: Jagan Teki 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Marek Szyprowski 
Cc: Maxime Ripard 
Cc: Michael Walle 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: Handle case where mode is not set yet
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 32 ---
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
b/drivers/gpu/drm/bridge/samsung-dsim.c
index e7e53a9e42afb..22d3bbd866d97 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -699,20 +699,24 @@ 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, pix_clk;
+   unsigned long hs_clk, byte_clk, esc_clk;
unsigned long esc_div;
u32 reg;
struct drm_display_mode *m = &dsi->mode;
int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
 
-   /* m->clock is in KHz */
-   pix_clk = m->clock * 1000;
-
-   /* Use burst_clk_rate if available, otherwise use the pix_clk */
+   /*
+* Use burst_clk_rate if available, otherwise use the mode clock
+* if mode is already set and available, otherwise fall back to
+* PLL input clock and operate in 1:1 lowest frequency mode until
+* a mode is set.
+*/
if (dsi->burst_clk_rate)
hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
+   else if (m) /* m->clock is in KHz */
+   hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(m->clock * 1000 
* bpp, dsi->lanes));
else
-   hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, 
dsi->lanes));
+   hs_clk = dsi->pll_clk_rate;
 
if (!hs_clk) {
dev_err(dsi->dev, "failed to configure DSI PLL\n");
@@ -1643,9 +1647,21 @@ static int samsung_dsim_attach(struct drm_bridge *bridge,
   enum drm_bridge_attach_flags flags)
 {
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+   int ret;
 
-   return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
-flags);
+   ret = pm_runtime_resume_and_get(dsi->dev);
+   if (ret < 0)
+   return ret;
+
+   ret = samsung_dsim_init(dsi);
+   if (ret < 0)
+   goto err;
+
+   ret = drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
+   flags);
+err:
+   pm_runtime_put_sync(dsi->dev);
+   return ret;
 }
 
 static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
-- 
2.43.0

[PATCH v3 2/2] drm/bridge: tc358767: Add configurable default preemphasis

[Marek Vasut]

Make the default DP port preemphasis configurable via new DT property
"toshiba,pre-emphasis". This is useful in case the DP link properties
are known and starting link training from preemphasis setting of 0 dB
is not useful. The preemphasis can be set separately for both DP lanes
in range 0=0dB, 1=3.5dB, 2=6dB .

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Conor Dooley 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Krzysztof Kozlowski 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Rob Herring 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: devicet...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: - Parse toshiba,pre-emphasis property out of an endpoint of port 2 (the DP 
port)
V3: - No change
---
 drivers/gpu/drm/bridge/tc358767.c | 45 ++-
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index dde1b2734c98a..257fe15080099 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -241,6 +241,10 @@
 
 /* Link Training */
 #define DP0_SRCCTRL0x06a0
+#define DP0_SRCCTRL_PRE1   GENMASK(29, 28)
+#define DP0_SRCCTRL_SWG1   GENMASK(25, 24)
+#define DP0_SRCCTRL_PRE0   GENMASK(21, 20)
+#define DP0_SRCCTRL_SWG0   GENMASK(17, 16)
 #define DP0_SRCCTRL_SCRMBLDIS  BIT(13)
 #define DP0_SRCCTRL_EN810B BIT(12)
 #define DP0_SRCCTRL_NOTP   (0 << 8)
@@ -278,6 +282,8 @@
 #define AUDIFDATA6 0x0720  /* DP0 Audio Info Frame Bytes 27 to 24 
*/
 
 #define DP1_SRCCTRL0x07a0  /* DP1 Control Register */
+#define DP1_SRCCTRL_PREGENMASK(21, 20)
+#define DP1_SRCCTRL_SWGGENMASK(17, 16)
 
 /* PHY */
 #define DP_PHY_CTRL0x0800
@@ -369,6 +375,7 @@ struct tc_data {
 
u32 rev;
u8  assr;
+   u8  pre_emphasis[2];
 
struct gpio_desc*sd_gpio;
struct gpio_desc*reset_gpio;
@@ -1090,13 +1097,17 @@ static int tc_main_link_enable(struct tc_data *tc)
return ret;
}
 
-   ret = regmap_write(tc->regmap, DP0_SRCCTRL, tc_srcctrl(tc));
+   ret = regmap_write(tc->regmap, DP0_SRCCTRL,
+  tc_srcctrl(tc) |
+  FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_emphasis[0]) |
+  FIELD_PREP(DP0_SRCCTRL_PRE1, tc->pre_emphasis[1]));
if (ret)
return ret;
/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
ret = regmap_write(tc->regmap, DP1_SRCCTRL,
 (tc->link.spread ? DP0_SRCCTRL_SSCG : 0) |
-((tc->link.rate != 162000) ? DP0_SRCCTRL_BW27 : 0));
+((tc->link.rate != 162000) ? DP0_SRCCTRL_BW27 : 0) |
+FIELD_PREP(DP1_SRCCTRL_PRE, tc->pre_emphasis[1]));
if (ret)
return ret;
 
@@ -1188,8 +1199,10 @@ static int tc_main_link_enable(struct tc_data *tc)
goto err_dpcd_write;
 
/* Reset voltage-swing & pre-emphasis */
-   tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
- DP_TRAIN_PRE_EMPH_LEVEL_0;
+   tmp[0] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
+FIELD_PREP(DP_TRAIN_PRE_EMPHASIS_MASK, tc->pre_emphasis[0]);
+   tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
+FIELD_PREP(DP_TRAIN_PRE_EMPHASIS_MASK, tc->pre_emphasis[1]);
ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2);
if (ret < 0)
goto err_dpcd_write;
@@ -1213,7 +1226,9 @@ static int tc_main_link_enable(struct tc_data *tc)
ret = regmap_write(tc->regmap, DP0_SRCCTRL,
   tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
   DP0_SRCCTRL_AUTOCORRECT |
-  DP0_SRCCTRL_TP1);
+  DP0_SRCCTRL_TP1 |
+  FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_emphasis[0]) |
+  FIELD_PREP(DP0_SRCCTRL_PRE1, tc->pre_emphasis[1]));
if (ret)
return ret;
 
@@ -1248,7 +1263,9 @@ static int tc_main_link_enable(struct tc_data *tc)
ret = regmap_write(tc->regmap, DP0_SRCCTRL,
   tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
   DP0_SRCCTRL_AUTOCORRECT |
-  DP0_SRCCTRL_TP2);
+  DP0_SRCCTRL_TP2 |
+  FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_emphasis[0]) |
+  FIELD_PREP(DP0_SRCCTRL_PRE1, tc->pre_emphasis[1]));
if (ret)
return ret;
 
@@ -1274,7 +1291,

[PATCH v3 1/2] dt-bindings: display: bridge: tc358867: Document default DP preemphasis

[Marek Vasut]

Document default DP port preemphasis configurable via new DT property
"toshiba,pre-emphasis". This is useful in case the DP link properties
are known and starting link training from preemphasis setting of 0 dB
is not useful. The preemphasis can be set separately for both DP lanes
in range 0=0dB, 1=3.5dB, 2=6dB .

This is an endpoint property, not a port property, because the TC9595
datasheet does mention that the DP might operate in some sort of split
mode, where each DP lane is used to feed one display, so in that case
there might be two endpoints.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Conor Dooley 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Krzysztof Kozlowski 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Rob Herring 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: devicet...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: - Fix the type to u8 array
- Fix the enum items to match what they represent
V3: - Update commit message, expand on this being an endpoint property
---
 .../display/bridge/toshiba,tc358767.yaml   | 18 ++
 1 file changed, 18 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml 
b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
index 2ad0cd6dd49e0..9490854c22f3b 100644
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
@@ -98,6 +98,24 @@ properties:
 reference to a valid eDP panel input endpoint node. This port is
 optional, treated as DP panel if not defined
 
+properties:
+  endpoint:
+$ref: /schemas/media/video-interfaces.yaml#
+unevaluatedProperties: false
+
+properties:
+  toshiba,pre-emphasis:
+description:
+  Display port output Pre-Emphasis settings for both DP lanes.
+$ref: /schemas/types.yaml#/definitions/uint8-array
+minItems: 2
+maxItems: 2
+items:
+  enum:
+- 0 # No pre-emphasis
+- 1 # 3.5dB pre-emphasis
+- 2 # 6dB pre-emphasis
+
 oneOf:
   - required:
   - port@0
-- 
2.43.0

[PATCH v4 5/5] Revert "drm/bridge: tc358767: Set default CLRSIPO count"

[Marek Vasut]

This reverts commit 01338bb82fed40a6a234c2b36a92367c8671adf0.

With clock improvements in place, this seems to be no longer
necessary. Set the CLRSIPO to default setting recommended by
manufacturer.

Reviewed-by: Alexander Stein 
Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
V3: No change
V4: - Add RB from Alexander
---
 drivers/gpu/drm/bridge/tc358767.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 0c6912bd5ec9e..cc8bf9416b661 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1356,10 +1356,10 @@ static int tc_dsi_rx_enable(struct tc_data *tc)
u32 value;
int ret;
 
-   regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 25);
-   regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 25);
-   regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 25);
-   regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 25);
+   regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 5);
+   regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 5);
+   regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 5);
+   regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 5);
regmap_write(tc->regmap, PPI_D0S_ATMR, 0);
regmap_write(tc->regmap, PPI_D1S_ATMR, 0);
regmap_write(tc->regmap, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
-- 
2.43.0

[PATCH v4 3/5] drm/bridge: tc358767: Drop line_pixel_subtract

[Marek Vasut]

This line_pixel_subtract is no longer needed now that the bridge can
request and obtain specific pixel clock on input to the bridge, with
clock frequency that matches the Pixel PLL frequency.

The line_pixel_subtract is now always 0, so drop it entirely.

The line_pixel_subtract was not reliable as it never worked when the
Pixel PLL and input clock were off just so that the required amount
of pixels to subtract would not be whole integer.

Reviewed-by: Alexander Stein 
Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
V3: Fix up rebase artifact
V4: - Fix another rebase mishap
- Add RB from Alexander
---
 drivers/gpu/drm/bridge/tc358767.c | 21 +
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index a77be11769791..19684b8400bef 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -382,9 +382,6 @@ struct tc_data {
 
/* HPD pin number (0 or 1) or -ENODEV */
int hpd_pin;
-
-   /* Number of pixels to subtract from a line due to pixel clock delta */
-   u32 line_pixel_subtract;
 };
 
 static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
@@ -580,11 +577,6 @@ static int tc_pllupdate(struct tc_data *tc, unsigned int 
pllctrl)
return 0;
 }
 
-static u32 div64_round_up(u64 v, u32 d)
-{
-   return div_u64(v + d - 1, d);
-}
-
 static int tc_pxl_pll_calc(struct tc_data *tc, u32 refclk, u32 pixelclock,
   int *out_best_pixelclock, u32 *out_pxl_pllparam)
 {
@@ -666,11 +658,7 @@ static int tc_pxl_pll_calc(struct tc_data *tc, u32 refclk, 
u32 pixelclock,
return -EINVAL;
}
 
-   tc->line_pixel_subtract = tc->mode.htotal -
-   div64_round_up(tc->mode.htotal * (u64)best_pixelclock, 
pixelclock);
-
-   dev_dbg(tc->dev, "PLL: got %d, delta %d (subtract %d px)\n", 
best_pixelclock,
-   best_delta, tc->line_pixel_subtract);
+   dev_dbg(tc->dev, "PLL: got %d, delta %d\n", best_pixelclock, 
best_delta);
dev_dbg(tc->dev, "PLL: %d / %d / %d * %d / %d\n", refclk,
ext_div[best_pre], best_div, best_mul, ext_div[best_post]);
 
@@ -914,13 +902,6 @@ static int tc_set_common_video_mode(struct tc_data *tc,
upper_margin, lower_margin, vsync_len);
dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
 
-   if (right_margin > tc->line_pixel_subtract) {
-   right_margin -= tc->line_pixel_subtract;
-   } else {
-   dev_err(tc->dev, "Bridge pixel clock too slow for mode\n");
-   right_margin = 0;
-   }
-
/*
 * LCD Ctl Frame Size
 * datasheet is not clear of vsdelay in case of DPI
-- 
2.43.0

[PATCH v4 4/5] drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1

[Marek Vasut]

The only information in the datasheet regarding this divider is a note
in SYS_PLLPARAM register documentation which states that when LSCLK is
270 MHz, LSCLK_DIV should be 1. What should LSCLK_DIV be set to when
LSCLK is 162 MHz (for DP 1.62G mode) is unclear, but empirical test
confirms using LSCLK_DIV 1 has no adverse effects either. In the worst
case, the internal TC358767 clock would run faster.

Reviewed-by: Alexander Stein 
Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
V3: No change
V4: - Add RB from Alexander
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 19684b8400bef..0c6912bd5ec9e 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -738,7 +738,7 @@ static int tc_stream_clock_calc(struct tc_data *tc)
 static int tc_set_syspllparam(struct tc_data *tc)
 {
unsigned long rate;
-   u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
+   u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_1;
 
rate = clk_get_rate(tc->refclk);
switch (rate) {
-- 
2.43.0

[PATCH v4 1/5] drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation and enablement

[Marek Vasut]

Split tc_pxl_pll_en() into tc_pxl_pll_calc() which does only Pixel PLL
parameter calculation and tc_pxl_pll_en() which calls tc_pxl_pll_calc()
and then configures the Pixel PLL register.

This is a preparatory patch for further rework, where tc_pxl_pll_calc()
will also be used to find out the exact clock frequency generated by the
Pixel PLL. This frequency will be used as adjusted_mode clock frequency
and passed down the display pipeline to obtain exactly this frequency
on input into this bridge.

The precise input frequency that matches the Pixel PLL frequency is
important for this bridge, as if the frequencies do not match, the
bridge does suffer VFIFO overruns or underruns.

Reviewed-by: Alexander Stein 
Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
V3: No change
V4: - Fix another rebase mishap
- Add RB from Alexander
---
 drivers/gpu/drm/bridge/tc358767.c | 32 ---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 3a2a93cfc17da..b2f0175d95420 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -585,9 +585,9 @@ static u32 div64_round_up(u64 v, u32 d)
return div_u64(v + d - 1, d);
 }
 
-static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
+static int tc_pxl_pll_calc(struct tc_data *tc, u32 refclk, u32 pixelclock,
+  int *out_best_pixelclock, u32 *out_pxl_pllparam)
 {
-   int ret;
int i_pre, best_pre = 1;
int i_post, best_post = 1;
int div, best_div = 1;
@@ -683,11 +683,6 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, 
u32 pixelclock)
if (best_mul == 128)
best_mul = 0;
 
-   /* Power up PLL and switch to bypass */
-   ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLBYP | PLLEN);
-   if (ret)
-   return ret;
-
pxl_pllparam  = vco_hi << 24; /* For PLL VCO >= 300 MHz = 1 */
pxl_pllparam |= ext_div[best_pre] << 20; /* External Pre-divider */
pxl_pllparam |= ext_div[best_post] << 16; /* External Post-divider */
@@ -695,6 +690,29 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, 
u32 pixelclock)
pxl_pllparam |= best_div << 8; /* Divider for PLL RefClk */
pxl_pllparam |= best_mul; /* Multiplier for PLL */
 
+   if (out_best_pixelclock)
+   *out_best_pixelclock = best_pixelclock;
+
+   if (out_pxl_pllparam)
+   *out_pxl_pllparam = pxl_pllparam;
+
+   return 0;
+}
+
+static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
+{
+   u32 pxl_pllparam = 0;
+   int ret;
+
+   ret = tc_pxl_pll_calc(tc, refclk, pixelclock, NULL, &pxl_pllparam);
+   if (ret)
+   return ret;
+
+   /* Power up PLL and switch to bypass */
+   ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLBYP | PLLEN);
+   if (ret)
+   return ret;
+
ret = regmap_write(tc->regmap, PXL_PLLPARAM, pxl_pllparam);
if (ret)
return ret;
-- 
2.43.0

[PATCH v4 2/5] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

Use tc_pxl_pll_calc() to find out the exact clock frequency generated by the
Pixel PLL. Use the Pixel PLL frequency as adjusted_mode clock frequency and
pass it down the display pipeline to obtain exactly this frequency on input
into this bridge.

The precise input frequency that matches the Pixel PLL frequency is
important for this bridge, as if the frequencies do not match, the
bridge does suffer VFIFO overruns or underruns.

Reviewed-by: Alexander Stein 
Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: - Use mode clock as input into tc_pxl_pll_calc() to avoid
  accumulating rounding error
V3: No change
V4: - Add RB from Alexander
---
 drivers/gpu/drm/bridge/tc358767.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index b2f0175d95420..a77be11769791 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1624,6 +1624,18 @@ static int tc_dpi_atomic_check(struct drm_bridge *bridge,
   struct drm_crtc_state *crtc_state,
   struct drm_connector_state *conn_state)
 {
+   struct tc_data *tc = bridge_to_tc(bridge);
+   int adjusted_clock = 0;
+   int ret;
+
+   ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
+ crtc_state->mode.clock * 1000,
+ &adjusted_clock, NULL);
+   if (ret)
+   return ret;
+
+   crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
+
/* DSI->DPI interface clock limitation: upto 100 MHz */
if (crtc_state->adjusted_mode.clock > 10)
return -EINVAL;
@@ -1636,6 +1648,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
   struct drm_crtc_state *crtc_state,
   struct drm_connector_state *conn_state)
 {
+   struct tc_data *tc = bridge_to_tc(bridge);
+   int adjusted_clock = 0;
+   int ret;
+
+   ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
+ crtc_state->mode.clock * 1000,
+ &adjusted_clock, NULL);
+   if (ret)
+   return ret;
+
+   crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
+
/* DPI->(e)DP interface clock limitation: upto 154 MHz */
if (crtc_state->adjusted_mode.clock > 154000)
return -EINVAL;
-- 
2.43.0

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

On 6/24/24 11:26 AM, Alexander Stein wrote:

Hi Marek,


Hi,


Am Freitag, 21. Juni 2024, 16:54:51 CEST schrieb Marek Vasut:

On 6/21/24 12:32 PM, Alexander Stein wrote:

Hi,

skipping the parts where I would simply write "OK" ...


As FVUEN is cleared at the next VSYNC event I suspect the DSI timings
are (slightly) off, but unfortunately I don't have equipment to check
DSI signal quality/timings.


As long as the LCDIFv3 pixel clock are equal or slightly slower than
what the TC9595 PixelPLL generates, AND, DSIM serializer has enough
bandwidth on the DSI bus (i.e. set the bus to 1 GHz, the TC9595 DSI RX
cannot go any faster), you should have no issues on that end.


I'm using samsung,burst-clock-frequency = <10> so this should be
okay. That is 1080p resolution.


Yes, correct.


When in doubt, try and use i2ctransfer to read out register 0x300
repeatedly, that's DSI RX error counter register. See if the DSI error
count increments.


If the bridge is not working the registers look like this:
300: c080
464: 0001

they are not changing and stay like that.

If the bridge is actually running they are like
300: c08000d3
464: 

and are also not changing.


Uh ... that looks like the whole chip clock tree somehow locked up .

Thinking about this, I once did force the DSIM into 24 MHz mode (there
is PLL bypass setting, where the DSIM uses 24 MHz serializer clock
directly for the DSI HS clock) or something close, it was enough to
drive a low resolution panel. But the upside was, with a 200 MHz 5Gsps
scope set to AC-coupling and 10x probe, I could discern the traffic on
DSI data lane and decode it by hand. The nice thing is, you could
trigger on 1V2 LP mode, so you know where the packet starts. The
downside is, if you have multiple data lanes, the packet is spread
across them.

You could also tweak tc_edp_atomic_check()/tc_edp_mode_valid() and force
only low(er) resolution modes of your DP panel right from the start, so
you wouldn't need that much DSI bandwidth. Maybe you could reach some
mode where your equipment is enough to analyze the traffic by hand ?


I think I got it running now. Apparently there were different, independent
problems which you addressed by your series.


Oh, glad I could help.


Unfortunately the patch
'tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS' introduced a new problem
(at least for me). For the record I'm running the following patch stack based
on next-20240621:


Thanks for tracking it down. I can drop that one 
MIPI_DSI_CLOCK_NON_CONTINUOUS patch from the series and do a V4. Would 
that work for you ? At least there would be some improvement to the 
driver and I can analyze the MIPI_DSI_CLOCK_NON_CONTINUOUS issue in 
detail separately.



* Add linux-next specific files for 20240621
* arm64: dts: imx8mp: Add TC9595 DSI-DP bridge on TQMa8MPxL/MBa8MPxL
* drm: bridge: samsung-dsim: Initialize bridge on attach
* drm/bridge: tc358767: Reset chip again on attach
* drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
* drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation and 
enablement
* drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
* drm/bridge: tc358767: Drop line_pixel_subtract
* drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
* Revert "drm/bridge: tc358767: Set default CLRSIPO count"

All of them are needed, reverting/dropping a single one results in a
non-functioning bridge.


Thank you for testing !

Re: [1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 6/24/24 11:26 AM, Alexander Stein wrote:

Hello Alexander,


Am Montag, 13. Mai 2024, 04:16:27 CEST schrieb Marek Vasut:

Initialize the bridge on attach already, to force lanes into LP11
state, since attach does trigger attach of downstream bridges which
may trigger (e)DP AUX channel mode read.

This fixes a corner case where DSIM with TC9595 attached to it fails
to operate the DP AUX channel, because the TC9595 enters some debug
mode when it is released from reset without lanes in LP11 mode. By
ensuring the DSIM lanes are in LP11, the TC9595 (tc358767.c driver)
can be reset in its attach callback called from DSIM attach callback,
and recovered out of the debug mode just before TC9595 performs first
AUX channel access later in its attach callback.

Signed-off-by: Marek Vasut 


This does the trick on my hardware as well.
Reviewed-by: Alexander Stein 


Thank you. I still have to address your previous comment on 1/2, I 
didn't get to that one yet, sorry.


Also, I am not sure if this should be applied as-is, it is a borderline 
workaround and there was some discussion about fixing the LP11 lane mode 
switch properly. Michael ?

Re: [PATCH v3 4/6] drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS

[Marek Vasut]

On 6/24/24 11:06 AM, Alexander Stein wrote:

Am Montag, 24. Juni 2024, 09:45:13 CEST schrieb Alexander Stein:

Hi,

Am Sonntag, 23. Juni 2024, 16:38:36 CEST schrieb Marek Vasut:

The MIPI_DSI_CLOCK_NON_CONTINUOUS causes visible artifacts in high
resolution modes, disable it. Namely, in DSI->DP mode 1920x1200 24
bpp 59.95 Hz, with DSI bus at maximum 1 Gbps per lane setting, the
image contains jittering empty lines.

Signed-off-by: Marek Vasut 


I can't see these artifacts in 1920x1200 24bpp, but still looks good to me
Acked-by: Alexander Stein 


I have to retract that. After checking for those mentioned artifacts
I noticed that the DP output was running without any issues.
There is something more going on here. Reverting this patch there wasn't
a single output problem.
This changes actually breaks my DSI connection randomly.
Sometimes it works, sometimes not. I also noticed that there wasn't even
a single DP link training failure, so I assume the DSI clock somehow
affected the internal state machine which even affected DP link training.
Until we know what's going on, NAK form me.


I can temporarily drop this patch and keep the remaining five if that's 
OK with you ?

Re: [PATCH v2 1/2] dt-bindings: display: bridge: tc358867: Document default DP preemphasis

[Marek Vasut]

On 6/23/24 7:20 PM, Conor Dooley wrote:

On Sun, Jun 23, 2024 at 04:48:47PM +0200, Marek Vasut wrote:

On 6/22/24 1:56 PM, Conor Dooley wrote:

On Fri, Jun 21, 2024 at 05:53:53PM +0200, Marek Vasut wrote:

Document default DP port preemphasis configurable via new DT property
"toshiba,pre-emphasis". This is useful in case the DP link properties
are known and starting link training from preemphasis setting of 0 dB
is not useful. The preemphasis can be set separately for both DP lanes
in range 0=0dB, 1=3.5dB, 2=6dB .

Signed-off-by: Marek Vasut 
---
V2: - Fix the type to u8 array
  - Fix the enum items to match what they represent
---
   .../display/bridge/toshiba,tc358767.yaml   | 18 ++
   1 file changed, 18 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml 
b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
index 2ad0cd6dd49e0..6287eb2b40908 100644
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
@@ -98,6 +98,24 @@ properties:
   reference to a valid eDP panel input endpoint node. This port is
   optional, treated as DP panel if not defined
+properties:
+  endpoint:
+$ref: /schemas/media/video-interfaces.yaml#
+unevaluatedProperties: false
+
+properties:
+  toshiba,pre-emphasis:
+description:
+  Display port output Pre-Emphasis settings for both ports.


Why here and not in the port nodes?


There was a short discussion about that in V1:

https://lore.kernel.org/all/00e9ef90-3bbe-4556-8da9-462f65928...@denx.de/

"
Let's keep it in the endpoint node.

There is some mention in the TC9595 datasheet that the DP might operate
in some split mode, where each DP lane is used to feed one display (?),
so I assume in that case there might be two endpoints (?), but that is
not supported right now.

If that is ever needed, I guess this array would have minItems 1 and
maxItems 2 and another endpoint would be added to the schema for this
port 2.
"


Can this be put in the commit message please?


Will do.


+$ref: /schemas/types.yaml#/definitions/uint8-array
+minItems: 2
+maxItems: 2
+items:
+  enum:
+- 0 # No pre-emphasis
+- 1 # 3.5dB pre-emphasis
+- 2 # 6dB pre-emphasis


I'd love to say please make this -bB and put this in units, but that'd
require it being a string..


I can do that. Do you think that's worth it ?


I dunno, I'd advocate for it for any other unit cos I would ask for the
unit to be changed into something that didn't require fractions. But for
decibels, that just going to be confusing given how it works. I think
for dB it's just not worth it.


All right then.

Re: [PATCH v2 1/2] dt-bindings: display: bridge: tc358867: Document default DP preemphasis

[Marek Vasut]

On 6/22/24 1:56 PM, Conor Dooley wrote:

On Fri, Jun 21, 2024 at 05:53:53PM +0200, Marek Vasut wrote:

Document default DP port preemphasis configurable via new DT property
"toshiba,pre-emphasis". This is useful in case the DP link properties
are known and starting link training from preemphasis setting of 0 dB
is not useful. The preemphasis can be set separately for both DP lanes
in range 0=0dB, 1=3.5dB, 2=6dB .

Signed-off-by: Marek Vasut 
---
V2: - Fix the type to u8 array
 - Fix the enum items to match what they represent
---
  .../display/bridge/toshiba,tc358767.yaml   | 18 ++
  1 file changed, 18 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml 
b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
index 2ad0cd6dd49e0..6287eb2b40908 100644
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
@@ -98,6 +98,24 @@ properties:
  reference to a valid eDP panel input endpoint node. This port is
  optional, treated as DP panel if not defined
  
+properties:

+  endpoint:
+$ref: /schemas/media/video-interfaces.yaml#
+unevaluatedProperties: false
+
+properties:
+  toshiba,pre-emphasis:
+description:
+  Display port output Pre-Emphasis settings for both ports.


Why here and not in the port nodes?


There was a short discussion about that in V1:

https://lore.kernel.org/all/00e9ef90-3bbe-4556-8da9-462f65928...@denx.de/

"
Let's keep it in the endpoint node.

There is some mention in the TC9595 datasheet that the DP might operate
in some split mode, where each DP lane is used to feed one display (?),
so I assume in that case there might be two endpoints (?), but that is
not supported right now.

If that is ever needed, I guess this array would have minItems 1 and
maxItems 2 and another endpoint would be added to the schema for this
port 2.
"


+$ref: /schemas/types.yaml#/definitions/uint8-array
+minItems: 2
+maxItems: 2
+items:
+  enum:
+- 0 # No pre-emphasis
+- 1 # 3.5dB pre-emphasis
+- 2 # 6dB pre-emphasis


I'd love to say please make this -bB and put this in units, but that'd
require it being a string..


I can do that. Do you think that's worth it ?

[PATCH v3 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

Use tc_pxl_pll_calc() to find out the exact clock frequency generated by the
Pixel PLL. Use the Pixel PLL frequency as adjusted_mode clock frequency and
pass it down the display pipeline to obtain exactly this frequency on input
into this bridge.

The precise input frequency that matches the Pixel PLL frequency is
important for this bridge, as if the frequencies do not match, the
bridge does suffer VFIFO overruns or underruns.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: - Use mode clock as input into tc_pxl_pll_calc() to avoid
  accumulating rounding error
V3: No change
---
 drivers/gpu/drm/bridge/tc358767.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index cbb342d811ac3..20be21660ba76 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1619,6 +1619,18 @@ static int tc_dpi_atomic_check(struct drm_bridge *bridge,
   struct drm_crtc_state *crtc_state,
   struct drm_connector_state *conn_state)
 {
+   struct tc_data *tc = bridge_to_tc(bridge);
+   int adjusted_clock = 0;
+   int ret;
+
+   ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
+ crtc_state->mode.clock * 1000,
+ &adjusted_clock, NULL);
+   if (ret)
+   return ret;
+
+   crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
+
/* DSI->DPI interface clock limitation: upto 100 MHz */
if (crtc_state->adjusted_mode.clock > 10)
return -EINVAL;
@@ -1631,6 +1643,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
   struct drm_crtc_state *crtc_state,
   struct drm_connector_state *conn_state)
 {
+   struct tc_data *tc = bridge_to_tc(bridge);
+   int adjusted_clock = 0;
+   int ret;
+
+   ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
+ crtc_state->mode.clock * 1000,
+ &adjusted_clock, NULL);
+   if (ret)
+   return ret;
+
+   crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
+
/* DPI->(e)DP interface clock limitation: upto 154 MHz */
if (crtc_state->adjusted_mode.clock > 154000)
return -EINVAL;
-- 
2.43.0

[PATCH v3 6/6] Revert "drm/bridge: tc358767: Set default CLRSIPO count"

[Marek Vasut]

This reverts commit 01338bb82fed40a6a234c2b36a92367c8671adf0.

With clock improvements in place, this seems to be no longer
necessary. Set the CLRSIPO to default setting recommended by
manufacturer.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
V3: No change
---
 drivers/gpu/drm/bridge/tc358767.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 743bf1334923d..2b035a136a6e5 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1356,10 +1356,10 @@ static int tc_dsi_rx_enable(struct tc_data *tc)
u32 value;
int ret;
 
-   regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 25);
-   regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 25);
-   regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 25);
-   regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 25);
+   regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 5);
+   regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 5);
+   regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 5);
+   regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 5);
regmap_write(tc->regmap, PPI_D0S_ATMR, 0);
regmap_write(tc->regmap, PPI_D1S_ATMR, 0);
regmap_write(tc->regmap, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
-- 
2.43.0

[PATCH v3 4/6] drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS

[Marek Vasut]

The MIPI_DSI_CLOCK_NON_CONTINUOUS causes visible artifacts in high
resolution modes, disable it. Namely, in DSI->DP mode 1920x1200 24
bpp 59.95 Hz, with DSI bus at maximum 1 Gbps per lane setting, the
image contains jittering empty lines.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
V3: No change
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index c4e2455ad95e4..a48454fe2f634 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -2303,7 +2303,7 @@ static int tc_mipi_dsi_host_attach(struct tc_data *tc)
dsi->lanes = dsi_lanes;
dsi->format = MIPI_DSI_FMT_RGB888;
dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
- MIPI_DSI_MODE_LPM | MIPI_DSI_CLOCK_NON_CONTINUOUS;
+ MIPI_DSI_MODE_LPM;
 
ret = devm_mipi_dsi_attach(dev, dsi);
if (ret < 0) {
-- 
2.43.0

[PATCH v3 3/6] drm/bridge: tc358767: Drop line_pixel_subtract

[Marek Vasut]

This line_pixel_subtract is no longer needed now that the bridge can
request and obtain specific pixel clock on input to the bridge, with
clock frequency that matches the Pixel PLL frequency.

The line_pixel_subtract is now always 0, so drop it entirely.

The line_pixel_subtract was not reliable as it never worked when the
Pixel PLL and input clock were off just so that the required amount
of pixels to subtract would not be whole integer.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
V3: Fix up rebase artifact
---
 drivers/gpu/drm/bridge/tc358767.c | 16 +---
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 20be21660ba76..c4e2455ad95e4 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -382,9 +382,6 @@ struct tc_data {
 
/* HPD pin number (0 or 1) or -ENODEV */
int hpd_pin;
-
-   /* Number of pixels to subtract from a line due to pixel clock delta */
-   u32 line_pixel_subtract;
 };
 
 static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
@@ -661,11 +658,7 @@ static int tc_pxl_pll_calc(struct tc_data *tc, u32 refclk, 
u32 pixelclock,
return -EINVAL;
}
 
-   tc->line_pixel_subtract = tc->mode.htotal -
-   div64_round_up(tc->mode.htotal * (u64)best_pixelclock, 
pixelclock);
-
-   dev_dbg(tc->dev, "PLL: got %d, delta %d (subtract %d px)\n", 
best_pixelclock,
-   best_delta, tc->line_pixel_subtract);
+   dev_dbg(tc->dev, "PLL: got %d, delta %d\n", best_pixelclock, 
best_delta);
dev_dbg(tc->dev, "PLL: %d / %d / %d * %d / %d\n", refclk,
ext_div[best_pre], best_div, best_mul, ext_div[best_post]);
 
@@ -909,13 +902,6 @@ static int tc_set_common_video_mode(struct tc_data *tc,
upper_margin, lower_margin, vsync_len);
dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
 
-   if (right_margin > tc->line_pixel_subtract) {
-   right_margin -= tc->line_pixel_subtract;
-   } else {
-   dev_err(tc->dev, "Bridge pixel clock too slow for mode\n");
-   right_margin = 0;
-   }
-
/*
 * LCD Ctl Frame Size
 * datasheet is not clear of vsdelay in case of DPI
-- 
2.43.0

[PATCH v3 5/6] drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1

[Marek Vasut]

The only information in the datasheet regarding this divider is a note
in SYS_PLLPARAM register documentation which states that when LSCLK is
270 MHz, LSCLK_DIV should be 1. What should LSCLK_DIV be set to when
LSCLK is 162 MHz (for DP 1.62G mode) is unclear, but empirical test
confirms using LSCLK_DIV 1 has no adverse effects either. In the worst
case, the internal TC358767 clock would run faster.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
V3: No change
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index a48454fe2f634..743bf1334923d 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -738,7 +738,7 @@ static int tc_stream_clock_calc(struct tc_data *tc)
 static int tc_set_syspllparam(struct tc_data *tc)
 {
unsigned long rate;
-   u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
+   u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_1;
 
rate = clk_get_rate(tc->refclk);
switch (rate) {
-- 
2.43.0

[PATCH v3 1/6] drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation and enablement

[Marek Vasut]

Split tc_pxl_pll_en() into tc_pxl_pll_calc() which does only Pixel PLL
parameter calculation and tc_pxl_pll_en() which calls tc_pxl_pll_calc()
and then configures the Pixel PLL register.

This is a preparatory patch for further rework, where tc_pxl_pll_calc()
will also be used to find out the exact clock frequency generated by the
Pixel PLL. This frequency will be used as adjusted_mode clock frequency
and passed down the display pipeline to obtain exactly this frequency
on input into this bridge.

The precise input frequency that matches the Pixel PLL frequency is
important for this bridge, as if the frequencies do not match, the
bridge does suffer VFIFO overruns or underruns.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
V3: No change
---
 drivers/gpu/drm/bridge/tc358767.c | 37 +--
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index b0435c8b754b4..cbb342d811ac3 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -580,14 +580,9 @@ static int tc_pllupdate(struct tc_data *tc, unsigned int 
pllctrl)
return 0;
 }
 
-static u32 div64_round_up(u64 v, u32 d)
+static int tc_pxl_pll_calc(struct tc_data *tc, u32 refclk, u32 pixelclock,
+  int *out_best_pixelclock, u32 *out_pxl_pllparam)
 {
-   return div_u64(v + d - 1, d);
-}
-
-static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
-{
-   int ret;
int i_pre, best_pre = 1;
int i_post, best_post = 1;
int div, best_div = 1;
@@ -683,11 +678,6 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, 
u32 pixelclock)
if (best_mul == 128)
best_mul = 0;
 
-   /* Power up PLL and switch to bypass */
-   ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLBYP | PLLEN);
-   if (ret)
-   return ret;
-
pxl_pllparam  = vco_hi << 24; /* For PLL VCO >= 300 MHz = 1 */
pxl_pllparam |= ext_div[best_pre] << 20; /* External Pre-divider */
pxl_pllparam |= ext_div[best_post] << 16; /* External Post-divider */
@@ -695,6 +685,29 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, 
u32 pixelclock)
pxl_pllparam |= best_div << 8; /* Divider for PLL RefClk */
pxl_pllparam |= best_mul; /* Multiplier for PLL */
 
+   if (out_best_pixelclock)
+   *out_best_pixelclock = best_pixelclock;
+
+   if (out_pxl_pllparam)
+   *out_pxl_pllparam = pxl_pllparam;
+
+   return 0;
+}
+
+static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
+{
+   u32 pxl_pllparam = 0;
+   int ret;
+
+   ret = tc_pxl_pll_calc(tc, refclk, pixelclock, NULL, &pxl_pllparam);
+   if (ret)
+   return ret;
+
+   /* Power up PLL and switch to bypass */
+   ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLBYP | PLLEN);
+   if (ret)
+   return ret;
+
ret = regmap_write(tc->regmap, PXL_PLLPARAM, pxl_pllparam);
if (ret)
return ret;
-- 
2.43.0

[PATCH v2 2/2] drm/bridge: tc358767: Add configurable default preemphasis

[Marek Vasut]

Make the default DP port preemphasis configurable via new DT property
"toshiba,pre-emphasis". This is useful in case the DP link properties
are known and starting link training from preemphasis setting of 0 dB
is not useful. The preemphasis can be set separately for both DP lanes
in range 0=0dB, 1=3.5dB, 2=6dB .

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Conor Dooley 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Krzysztof Kozlowski 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Rob Herring 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: devicet...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: - Parse toshiba,pre-emphasis property out of an endpoint of port 2 (the DP 
port)
---
 drivers/gpu/drm/bridge/tc358767.c | 45 ++-
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 2b035a136a6e5..8f81fc960d0fa 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -241,6 +241,10 @@
 
 /* Link Training */
 #define DP0_SRCCTRL0x06a0
+#define DP0_SRCCTRL_PRE1   GENMASK(29, 28)
+#define DP0_SRCCTRL_SWG1   GENMASK(25, 24)
+#define DP0_SRCCTRL_PRE0   GENMASK(21, 20)
+#define DP0_SRCCTRL_SWG0   GENMASK(17, 16)
 #define DP0_SRCCTRL_SCRMBLDIS  BIT(13)
 #define DP0_SRCCTRL_EN810B BIT(12)
 #define DP0_SRCCTRL_NOTP   (0 << 8)
@@ -278,6 +282,8 @@
 #define AUDIFDATA6 0x0720  /* DP0 Audio Info Frame Bytes 27 to 24 
*/
 
 #define DP1_SRCCTRL0x07a0  /* DP1 Control Register */
+#define DP1_SRCCTRL_PREGENMASK(21, 20)
+#define DP1_SRCCTRL_SWGGENMASK(17, 16)
 
 /* PHY */
 #define DP_PHY_CTRL0x0800
@@ -369,6 +375,7 @@ struct tc_data {
 
u32 rev;
u8  assr;
+   u8  pre_emphasis[2];
 
struct gpio_desc*sd_gpio;
struct gpio_desc*reset_gpio;
@@ -1090,13 +1097,17 @@ static int tc_main_link_enable(struct tc_data *tc)
return ret;
}
 
-   ret = regmap_write(tc->regmap, DP0_SRCCTRL, tc_srcctrl(tc));
+   ret = regmap_write(tc->regmap, DP0_SRCCTRL,
+  tc_srcctrl(tc) |
+  FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_emphasis[0]) |
+  FIELD_PREP(DP0_SRCCTRL_PRE1, tc->pre_emphasis[1]));
if (ret)
return ret;
/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
ret = regmap_write(tc->regmap, DP1_SRCCTRL,
 (tc->link.spread ? DP0_SRCCTRL_SSCG : 0) |
-((tc->link.rate != 162000) ? DP0_SRCCTRL_BW27 : 0));
+((tc->link.rate != 162000) ? DP0_SRCCTRL_BW27 : 0) |
+FIELD_PREP(DP1_SRCCTRL_PRE, tc->pre_emphasis[1]));
if (ret)
return ret;
 
@@ -1188,8 +1199,10 @@ static int tc_main_link_enable(struct tc_data *tc)
goto err_dpcd_write;
 
/* Reset voltage-swing & pre-emphasis */
-   tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
- DP_TRAIN_PRE_EMPH_LEVEL_0;
+   tmp[0] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
+FIELD_PREP(DP_TRAIN_PRE_EMPHASIS_MASK, tc->pre_emphasis[0]);
+   tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
+FIELD_PREP(DP_TRAIN_PRE_EMPHASIS_MASK, tc->pre_emphasis[1]);
ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2);
if (ret < 0)
goto err_dpcd_write;
@@ -1213,7 +1226,9 @@ static int tc_main_link_enable(struct tc_data *tc)
ret = regmap_write(tc->regmap, DP0_SRCCTRL,
   tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
   DP0_SRCCTRL_AUTOCORRECT |
-  DP0_SRCCTRL_TP1);
+  DP0_SRCCTRL_TP1 |
+  FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_emphasis[0]) |
+  FIELD_PREP(DP0_SRCCTRL_PRE1, tc->pre_emphasis[1]));
if (ret)
return ret;
 
@@ -1248,7 +1263,9 @@ static int tc_main_link_enable(struct tc_data *tc)
ret = regmap_write(tc->regmap, DP0_SRCCTRL,
   tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
   DP0_SRCCTRL_AUTOCORRECT |
-  DP0_SRCCTRL_TP2);
+  DP0_SRCCTRL_TP2 |
+  FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_emphasis[0]) |
+  FIELD_PREP(DP0_SRCCTRL_PRE1, tc->pre_emphasis[1]));
if (ret)
return ret;
 
@@ -1274,7 +1291,9 @@ static int tc_mai

[PATCH v2 1/2] dt-bindings: display: bridge: tc358867: Document default DP preemphasis

[Marek Vasut]

Document default DP port preemphasis configurable via new DT property
"toshiba,pre-emphasis". This is useful in case the DP link properties
are known and starting link training from preemphasis setting of 0 dB
is not useful. The preemphasis can be set separately for both DP lanes
in range 0=0dB, 1=3.5dB, 2=6dB .

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Conor Dooley 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Krzysztof Kozlowski 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Rob Herring 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: devicet...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: - Fix the type to u8 array
- Fix the enum items to match what they represent
---
 .../display/bridge/toshiba,tc358767.yaml   | 18 ++
 1 file changed, 18 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml 
b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
index 2ad0cd6dd49e0..6287eb2b40908 100644
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
@@ -98,6 +98,24 @@ properties:
 reference to a valid eDP panel input endpoint node. This port is
 optional, treated as DP panel if not defined
 
+properties:
+  endpoint:
+$ref: /schemas/media/video-interfaces.yaml#
+unevaluatedProperties: false
+
+properties:
+  toshiba,pre-emphasis:
+description:
+  Display port output Pre-Emphasis settings for both ports.
+$ref: /schemas/types.yaml#/definitions/uint8-array
+minItems: 2
+maxItems: 2
+items:
+  enum:
+- 0 # No pre-emphasis
+- 1 # 3.5dB pre-emphasis
+- 2 # 6dB pre-emphasis
+
 oneOf:
   - required:
   - port@0
-- 
2.43.0

[PATCH v2 4/6] drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS

[Marek Vasut]

The MIPI_DSI_CLOCK_NON_CONTINUOUS causes visible artifacts in high
resolution modes, disable it. Namely, in DSI->DP mode 1920x1200 24
bpp 59.95 Hz, with DSI bus at maximum 1 Gbps per lane setting, the
image contains jittering empty lines.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index c4e2455ad95e4..a48454fe2f634 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -2303,7 +2303,7 @@ static int tc_mipi_dsi_host_attach(struct tc_data *tc)
dsi->lanes = dsi_lanes;
dsi->format = MIPI_DSI_FMT_RGB888;
dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
- MIPI_DSI_MODE_LPM | MIPI_DSI_CLOCK_NON_CONTINUOUS;
+ MIPI_DSI_MODE_LPM;
 
ret = devm_mipi_dsi_attach(dev, dsi);
if (ret < 0) {
-- 
2.43.0

[PATCH v2 5/6] drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1

[Marek Vasut]

The only information in the datasheet regarding this divider is a note
in SYS_PLLPARAM register documentation which states that when LSCLK is
270 MHz, LSCLK_DIV should be 1. What should LSCLK_DIV be set to when
LSCLK is 162 MHz (for DP 1.62G mode) is unclear, but empirical test
confirms using LSCLK_DIV 1 has no adverse effects either. In the worst
case, the internal TC358767 clock would run faster.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index a48454fe2f634..743bf1334923d 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -738,7 +738,7 @@ static int tc_stream_clock_calc(struct tc_data *tc)
 static int tc_set_syspllparam(struct tc_data *tc)
 {
unsigned long rate;
-   u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
+   u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_1;
 
rate = clk_get_rate(tc->refclk);
switch (rate) {
-- 
2.43.0

[PATCH v2 3/6] drm/bridge: tc358767: Drop line_pixel_subtract

[Marek Vasut]

This line_pixel_subtract is no longer needed now that the bridge can
request and obtain specific pixel clock on input to the bridge, with
clock frequency that matches the Pixel PLL frequency.

The line_pixel_subtract is now always 0, so drop it entirely.

The line_pixel_subtract was not reliable as it never worked when the
Pixel PLL and input clock were off just so that the required amount
of pixels to subtract would not be whole integer.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
---
 drivers/gpu/drm/bridge/tc358767.c | 16 +---
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 4f1f4383d3cf0..c4e2455ad95e4 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -382,9 +382,6 @@ struct tc_data {
 
/* HPD pin number (0 or 1) or -ENODEV */
int hpd_pin;
-
-   /* Number of pixels to subtract from a line due to pixel clock delta */
-   u32 line_pixel_subtract;
 };
 
 static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
@@ -661,11 +658,7 @@ static int tc_pxl_pll_calc(struct tc_data *tc, u32 refclk, 
u32 pixelclock,
return -EINVAL;
}
 
-   tc->line_pixel_subtract = tc->mode.htotal -
-   DIV_ROUND_UP(tc->mode.htotal * (u64)best_pixelclock, 
(u64)pixelclock);
-
-   dev_dbg(tc->dev, "PLL: got %d, delta %d (subtract %d px)\n", 
best_pixelclock,
-   best_delta, tc->line_pixel_subtract);
+   dev_dbg(tc->dev, "PLL: got %d, delta %d\n", best_pixelclock, 
best_delta);
dev_dbg(tc->dev, "PLL: %d / %d / %d * %d / %d\n", refclk,
ext_div[best_pre], best_div, best_mul, ext_div[best_post]);
 
@@ -909,13 +902,6 @@ static int tc_set_common_video_mode(struct tc_data *tc,
upper_margin, lower_margin, vsync_len);
dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
 
-   if (right_margin > tc->line_pixel_subtract) {
-   right_margin -= tc->line_pixel_subtract;
-   } else {
-   dev_err(tc->dev, "Bridge pixel clock too slow for mode\n");
-   right_margin = 0;
-   }
-
/*
 * LCD Ctl Frame Size
 * datasheet is not clear of vsdelay in case of DPI
-- 
2.43.0

[PATCH v2 6/6] Revert "drm/bridge: tc358767: Set default CLRSIPO count"

[Marek Vasut]

This reverts commit 01338bb82fed40a6a234c2b36a92367c8671adf0.

With clock improvements in place, this seems to be no longer
necessary. Set the CLRSIPO to default setting recommended by
manufacturer.

Signed-off-by: Marek Vasut 
---
Cc: Andrzej Hajda 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Jernej Skrabec 
Cc: Jonas Karlman 
Cc: Laurent Pinchart 
Cc: Lucas Stach 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Neil Armstrong 
Cc: Robert Foss 
Cc: Thomas Zimmermann 
Cc: dri-devel@lists.freedesktop.org
Cc: ker...@dh-electronics.com
---
V2: No change
---
 drivers/gpu/drm/bridge/tc358767.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 743bf1334923d..2b035a136a6e5 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1356,10 +1356,10 @@ static int tc_dsi_rx_enable(struct tc_data *tc)
u32 value;
int ret;
 
-   regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 25);
-   regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 25);
-   regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 25);
-   regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 25);
+   regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 5);
+   regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 5);
+   regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 5);
+   regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 5);
regmap_write(tc->regmap, PPI_D0S_ATMR, 0);
regmap_write(tc->regmap, PPI_D1S_ATMR, 0);
regmap_write(tc->regmap, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
-- 
2.43.0