[PATCH 1/5] drm/bridge: tc358762: Split register programming from pre-enable to enable

[PATCH 1/5] drm/bridge: tc358762: Split register programming from pre-enable to enable

[Marek Vasut]

Move the register programming part, which actually enables the bridge and
makes it push data out of its DPI side, into the enable callback. The DSI
host like DSIM may not be able to transmit commands in pre_enable, moving
the register programming into enable assures it can transmit commands.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/tc358762.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
index 5641395fd310e..df9703eacab1f 100644
--- a/drivers/gpu/drm/bridge/tc358762.c
+++ b/drivers/gpu/drm/bridge/tc358762.c
@@ -162,11 +162,17 @@ static void tc358762_pre_enable(struct drm_bridge *bridge)
 		usleep_range(5000, 10000);
 	}
 
+	ctx->pre_enabled = true;
+}
+
+static void tc358762_enable(struct drm_bridge *bridge)
+{
+	struct tc358762 *ctx = bridge_to_tc358762(bridge);
+	int ret;
+
 	ret = tc358762_init(ctx);
 	if (ret < 0)
 		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
-
-	ctx->pre_enabled = true;
 }
 
 static int tc358762_attach(struct drm_bridge *bridge,
@@ -181,6 +187,7 @@ static int tc358762_attach(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs tc358762_bridge_funcs = {
 	.post_disable = tc358762_post_disable,
 	.pre_enable = tc358762_pre_enable,
+	.enable = tc358762_enable,
 	.attach = tc358762_attach,
 };
 
-- 
2.39.2

[PATCH 2/5] drm/bridge: tc358762: Switch to atomic ops

[Marek Vasut]

Switch the bridge driver over to atomic ops. No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/tc358762.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
index df9703eacab1f..5e00c08b99540 100644
--- a/drivers/gpu/drm/bridge/tc358762.c
+++ b/drivers/gpu/drm/bridge/tc358762.c
@@ -126,7 +126,7 @@ static int tc358762_init(struct tc358762 *ctx)
 	return tc358762_clear_error(ctx);
 }
 
-static void tc358762_post_disable(struct drm_bridge *bridge)
+static void tc358762_post_disable(struct drm_bridge *bridge, struct drm_bridge_state *state)
 {
 	struct tc358762 *ctx = bridge_to_tc358762(bridge);
 	int ret;
@@ -148,7 +148,7 @@ static void tc358762_post_disable(struct drm_bridge *bridge)
 		dev_err(ctx->dev, "error disabling regulators (%d)\n", ret);
 }
 
-static void tc358762_pre_enable(struct drm_bridge *bridge)
+static void tc358762_pre_enable(struct drm_bridge *bridge, struct drm_bridge_state *state)
 {
 	struct tc358762 *ctx = bridge_to_tc358762(bridge);
 	int ret;
@@ -165,7 +165,7 @@ static void tc358762_pre_enable(struct drm_bridge *bridge)
 	ctx->pre_enabled = true;
 }
 
-static void tc358762_enable(struct drm_bridge *bridge)
+static void tc358762_enable(struct drm_bridge *bridge, struct drm_bridge_state *state)
 {
 	struct tc358762 *ctx = bridge_to_tc358762(bridge);
 	int ret;
@@ -185,9 +185,12 @@ static int tc358762_attach(struct drm_bridge *bridge,
 }
 
 static const struct drm_bridge_funcs tc358762_bridge_funcs = {
-	.post_disable = tc358762_post_disable,
-	.pre_enable = tc358762_pre_enable,
-	.enable = tc358762_enable,
+	.atomic_post_disable = tc358762_post_disable,
+	.atomic_pre_enable = tc358762_pre_enable,
+	.atomic_enable = tc358762_enable,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
 	.attach = tc358762_attach,
 };
 
-- 
2.39.2

Re: [PATCH 2/5] drm/bridge: tc358762: Switch to atomic ops

[Sam Ravnborg]

On Thu, Jun 15, 2023 at 10:18:59PM +0200, Marek Vasut wrote:
> Switch the bridge driver over to atomic ops. No functional change.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

(I hope one day all bridge drivers are converted to atomic ops...)

> ---
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/bridge/tc358762.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
> index df9703eacab1f..5e00c08b99540 100644
> --- a/drivers/gpu/drm/bridge/tc358762.c
> +++ b/drivers/gpu/drm/bridge/tc358762.c
> @@ -126,7 +126,7 @@ static int tc358762_init(struct tc358762 *ctx)
>  	return tc358762_clear_error(ctx);
>  }
>  
> -static void tc358762_post_disable(struct drm_bridge *bridge)
> +static void tc358762_post_disable(struct drm_bridge *bridge, struct drm_bridge_state *state)
>  {
>  	struct tc358762 *ctx = bridge_to_tc358762(bridge);
>  	int ret;
> @@ -148,7 +148,7 @@ static void tc358762_post_disable(struct drm_bridge *bridge)
>  		dev_err(ctx->dev, "error disabling regulators (%d)\n", ret);
>  }
>  
> -static void tc358762_pre_enable(struct drm_bridge *bridge)
> +static void tc358762_pre_enable(struct drm_bridge *bridge, struct drm_bridge_state *state)
>  {
>  	struct tc358762 *ctx = bridge_to_tc358762(bridge);
>  	int ret;
> @@ -165,7 +165,7 @@ static void tc358762_pre_enable(struct drm_bridge *bridge)
>  	ctx->pre_enabled = true;
>  }
>  
> -static void tc358762_enable(struct drm_bridge *bridge)
> +static void tc358762_enable(struct drm_bridge *bridge, struct drm_bridge_state *state)
>  {
>  	struct tc358762 *ctx = bridge_to_tc358762(bridge);
>  	int ret;
> @@ -185,9 +185,12 @@ static int tc358762_attach(struct drm_bridge *bridge,
>  }
>  
>  static const struct drm_bridge_funcs tc358762_bridge_funcs = {
> -	.post_disable = tc358762_post_disable,
> -	.pre_enable = tc358762_pre_enable,
> -	.enable = tc358762_enable,
> +	.atomic_post_disable = tc358762_post_disable,
> +	.atomic_pre_enable = tc358762_pre_enable,
> +	.atomic_enable = tc358762_enable,
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_reset = drm_atomic_helper_bridge_reset,
>  	.attach = tc358762_attach,
>  };
>  
> -- 
> 2.39.2

[PATCH 3/5] drm/bridge: tc358762: Instruct DSI host to generate HSE packets

[Marek Vasut]

This bridge seems to need the HSE packet, otherwise the image is
shifted up and corrupted at the bottom. This makes the bridge
work with Samsung DSIM on i.MX8MM and i.MX8MP.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/tc358762.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
index 5e00c08b99540..77f2ec9de9e59 100644
--- a/drivers/gpu/drm/bridge/tc358762.c
+++ b/drivers/gpu/drm/bridge/tc358762.c
@@ -241,7 +241,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
 	dsi->lanes = 1;
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
-			  MIPI_DSI_MODE_LPM;
+			  MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_VIDEO_HSE;
 
 	ret = tc358762_parse_dt(ctx);
 	if (ret < 0)
-- 
2.39.2

Re: [PATCH 3/5] drm/bridge: tc358762: Instruct DSI host to generate HSE packets

[Sam Ravnborg]

On Thu, Jun 15, 2023 at 10:19:00PM +0200, Marek Vasut wrote:
> This bridge seems to need the HSE packet, otherwise the image is
> shifted up and corrupted at the bottom. This makes the bridge
> work with Samsung DSIM on i.MX8MM and i.MX8MP.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

(Not that I know the register or such, but the patch looks good).

> ---
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/bridge/tc358762.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
> index 5e00c08b99540..77f2ec9de9e59 100644
> --- a/drivers/gpu/drm/bridge/tc358762.c
> +++ b/drivers/gpu/drm/bridge/tc358762.c
> @@ -241,7 +241,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
>  	dsi->lanes = 1;
>  	dsi->format = MIPI_DSI_FMT_RGB888;
>  	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> -			  MIPI_DSI_MODE_LPM;
> +			  MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_VIDEO_HSE;
>  
>  	ret = tc358762_parse_dt(ctx);
>  	if (ret < 0)
> -- 
> 2.39.2

Re: [PATCH 3/5] drm/bridge: tc358762: Instruct DSI host to generate HSE packets

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3295 bytes --]

Hey Marek,

On 15.06.2023 22:19:00, Marek Vasut wrote:
> This bridge seems to need the HSE packet, otherwise the image is
> shifted up and corrupted at the bottom. This makes the bridge
> work with Samsung DSIM on i.MX8MM and i.MX8MP.

I'm using v6.6 (which includes this series) on an i.MX8MP with the 7inch
Rspi Panel ("powertip,ph800480t013-idf02"), but I cannot get a stable
image.

With an unmodified imx8mp clock tree the lower 1/4 of the image sheers
to the left.

With 24.75 MHz on the media_disp1_pix and media_mipi_phy1_ref and 792
MHz on video_pll1_out, the image is not static, but wobbly and it's
wrapped around half of the image.

    video_pll1_ref_sel                1        1        0    24000000          0     0  50000         Y
       video_pll1                     1        1        0   792000000          0     0  50000         Y
          video_pll1_bypass           1        1        0   792000000          0     0  50000         Y
             video_pll1_out           2        2        0   792000000          0     0  50000         Y
                media_mipi_phy1_ref       1        1        0    24750000          0     0  50000         Y
                   media_mipi_phy1_ref_root       0        0        0    24750000          0     0  50000         Y
                media_disp2_pix       0        0        0   792000000          0     0  50000         N
                   media_disp2_pix_root_clk       0        0        0   792000000          0     0  50000         N
                media_disp1_pix       1        1        0    24750000          0     0  50000         Y
                   media_disp1_pix_root_clk       1        1        0    24750000          0     0  50000         Y

Do you have a working device tree for such a setup? regards, Marc

Relevant DT snipped for my setup:

&{/} {
	panel {
		compatible = "powertip,ph800480t013-idf02";
		power-supply = <&attiny>;
		backlight = <&attiny>;

		port {
			panel_in: endpoint {
				remote-endpoint = <&bridge_out>;
			};
		};
	};
};

&mipi_dsi {
	samsung,esc-clock-frequency = <54000000>;
	assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_800M>,
				 <&clk IMX8MP_VIDEO_PLL1_OUT>;
	assigned-clock-rates = <200000000>, <24750000>;
	status = "okay";

	bridge@0 {
		compatible = "toshiba,tc358762";
		reg = <0>;
		vddc-supply = <&attiny>;
		reset-gpio = <&attiny 0 0>;

		ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				reg = <0>;

				bridge_in: endpoint {
					remote-endpoint = <&dsi_out>;
				};
			};

			port@1 {
				reg = <1>;

				bridge_out: endpoint {
					remote-endpoint = <&panel_in>;
				};
			};
		};
	};

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@1 {
			reg = <1>;

			dsi_out: endpoint {
				data-lanes = <1 2>;
				remote-endpoint = <&bridge_in>;
			};
		};
	};
};

&media_blk_ctrl {
	assigned-clock-rates = <500000000>, <200000000>,
			       <0>, <0>, <792000000>;
};

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/5] drm/bridge: tc358762: Instruct DSI host to generate HSE packets

[Marek Vasut]

On 11/16/23 17:06, Marc Kleine-Budde wrote:
> Hey Marek,
> 
> On 15.06.2023 22:19:00, Marek Vasut wrote:
>> This bridge seems to need the HSE packet, otherwise the image is
>> shifted up and corrupted at the bottom. This makes the bridge
>> work with Samsung DSIM on i.MX8MM and i.MX8MP.
> 
> I'm using v6.6 (which includes this series) on an i.MX8MP with the 7inch
> Rspi Panel ("powertip,ph800480t013-idf02"), but I cannot get a stable
> image.
> 
> With an unmodified imx8mp clock tree the lower 1/4 of the image sheers
> to the left.
> 
> With 24.75 MHz on the media_disp1_pix and media_mipi_phy1_ref and 792
> MHz on video_pll1_out, the image is not static, but wobbly and it's
> wrapped around half of the image.
> 
>      video_pll1_ref_sel                1        1        0    24000000          0     0  50000         Y
>         video_pll1                     1        1        0   792000000          0     0  50000         Y
>            video_pll1_bypass           1        1        0   792000000          0     0  50000         Y
>               video_pll1_out           2        2        0   792000000          0     0  50000         Y
>                  media_mipi_phy1_ref       1        1        0    24750000          0     0  50000         Y
>                     media_mipi_phy1_ref_root       0        0        0    24750000          0     0  50000         Y
>                  media_disp2_pix       0        0        0   792000000          0     0  50000         N
>                     media_disp2_pix_root_clk       0        0        0   792000000          0     0  50000         N
>                  media_disp1_pix       1        1        0    24750000          0     0  50000         Y
>                     media_disp1_pix_root_clk       1        1        0    24750000          0     0  50000         Y
> 
> Do you have a working device tree for such a setup? regards, Marc

This seems to be what I used last time I tried (notice the 
samsung,burst-clock-frequency = <720000000>; ), but this was a few 
months ago:

/ {
         panel {
                 compatible = "powertip,ph800480t013-idf02";
                 backlight = <&attiny>;
                 enable-gpio = <&attiny 0 GPIO_ACTIVE_HIGH>;
                 power-supply = <&attiny>;

                 port {
                         panel_in: endpoint {
                                 remote-endpoint = <&bridge_out>;
                         };
                 };
         };
};

&mipi_dsi {
         /*
          * This is DSIM PLL frequency, DSI HS clock lane frequency
          * is half of the "samsung,burst-clock-frequency" value.
          */
         samsung,burst-clock-frequency = <720000000>;
         status = "okay";

         bridge@0 {
                 compatible = "toshiba,tc358762";
                 reg = <0>;
                 vddc-supply = <&attiny>;
                 #address-cells = <1>;
                 #size-cells = <0>;
                 status = "okay";

                 port@0 {
                         reg = <0>;
                         bridge_in: endpoint {
                                 data-lanes = <1>;
                                 remote-endpoint = <&dsi_out>;
                         };
                 };

                 port@1 {
                         reg = <1>;
                         bridge_out: endpoint {
                                 remote-endpoint = <&panel_in>;
                         };
                 };
         };
};

&dsi_out {
         data-lanes = <1>;
         remote-endpoint = <&bridge_in>;
};

&i2cmuxed0 {
         #address-cells = <1>;
         #size-cells = <0>;

         touchscreen: touchscreen@38 {
                 compatible = "edt,edt-ft5406";
                 reg = <0x38>;
                 reset-gpios = <&attiny 1 GPIO_ACTIVE_LOW>;
                 /*
                  * Disabled, since the IRQ line is not on
                  * the FPC cable, so we cannot get touch
                  * IRQs unless its connected otherwise. In
                  * that case, add entry like this one and
                  * enable below.
                  *
                  * interrupt-parent = <&gpiog>;
                  * interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
                  */
                 status = "disabled";
         };

         attiny: regulator@45 {
                 compatible = 
"raspberrypi,7inch-touchscreen-panel-regulator";
                 gpio-controller;
                 #gpio-cells = <2>;
                 reg = <0x45>;
         };
};

&pwm1 {
         status = "okay";
};

&lcdif1 {
         status = "okay";
};

[PATCH 4/5] drm/bridge: tc358762: Guess the meaning of LCDCTRL bits

[Marek Vasut]

The register content and behavior is very similar to TC358764 VP_CTRL.
All the bits except for unknown bit 6 also seem to match, even though
the datasheet is just not available. Add a comment and reuse the bit
definitions.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/tc358762.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
index 77f2ec9de9e59..a092e2096074f 100644
--- a/drivers/gpu/drm/bridge/tc358762.c
+++ b/drivers/gpu/drm/bridge/tc358762.c
@@ -41,8 +41,17 @@
 #define DSI_LANEENABLE		0x0210 /* Enables each lane */
 #define DSI_RX_START		1
 
-/* LCDC/DPI Host Registers */
-#define LCDCTRL			0x0420
+/* LCDC/DPI Host Registers, based on guesswork that this matches TC358764 */
+#define LCDCTRL			0x0420 /* Video Path Control */
+#define LCDCTRL_MSF		BIT(0) /* Magic square in RGB666 */
+#define LCDCTRL_VTGEN		BIT(4)/* Use chip clock for timing */
+#define LCDCTRL_UNK6		BIT(6) /* Unknown */
+#define LCDCTRL_EVTMODE		BIT(5) /* Event mode */
+#define LCDCTRL_RGB888		BIT(8) /* RGB888 mode */
+#define LCDCTRL_HSPOL		BIT(17) /* Polarity of HSYNC signal */
+#define LCDCTRL_DEPOL		BIT(18) /* Polarity of DE signal */
+#define LCDCTRL_VSPOL		BIT(19) /* Polarity of VSYNC signal */
+#define LCDCTRL_VSDELAY(v)	(((v) & 0xfff) << 20) /* VSYNC delay */
 
 /* SPI Master Registers */
 #define SPICMR			0x0450
@@ -114,7 +123,8 @@ static int tc358762_init(struct tc358762 *ctx)
 	tc358762_write(ctx, PPI_LPTXTIMECNT, LPX_PERIOD);
 
 	tc358762_write(ctx, SPICMR, 0x00);
-	tc358762_write(ctx, LCDCTRL, 0x00100150);
+	tc358762_write(ctx, LCDCTRL, LCDCTRL_VSDELAY(1) | LCDCTRL_RGB888 |
+				     LCDCTRL_UNK6 | LCDCTRL_VTGEN);
 	tc358762_write(ctx, SYSCTRL, 0x040f);
 	msleep(100);
 
-- 
2.39.2

Re: [PATCH 4/5] drm/bridge: tc358762: Guess the meaning of LCDCTRL bits

[Sam Ravnborg]

On Thu, Jun 15, 2023 at 10:19:01PM +0200, Marek Vasut wrote:
> The register content and behavior is very similar to TC358764 VP_CTRL.
> All the bits except for unknown bit 6 also seem to match, even though
> the datasheet is just not available. Add a comment and reuse the bit
> definitions.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

[PATCH 5/5] drm/bridge: tc358762: Handle HS/VS polarity

[Marek Vasut]

Add support for handling the HS/VS sync signals polarity in the bridge
driver, otherwise e.g. DSIM bridge feeds the TC358762 inverted polarity
sync signals and the image is shifted to the left, up, and wobbly.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/tc358762.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
index a092e2096074f..46198af9eebbf 100644
--- a/drivers/gpu/drm/bridge/tc358762.c
+++ b/drivers/gpu/drm/bridge/tc358762.c
@@ -74,6 +74,7 @@ struct tc358762 {
 	struct regulator *regulator;
 	struct drm_bridge *panel_bridge;
 	struct gpio_desc *reset_gpio;
+	struct drm_display_mode mode;
 	bool pre_enabled;
 	int error;
 };
@@ -114,6 +115,8 @@ static inline struct tc358762 *bridge_to_tc358762(struct drm_bridge *bridge)
 
 static int tc358762_init(struct tc358762 *ctx)
 {
+	u32 lcdctrl;
+
 	tc358762_write(ctx, DSI_LANEENABLE,
 		       LANEENABLE_L0EN | LANEENABLE_CLEN);
 	tc358762_write(ctx, PPI_D0S_CLRSIPOCOUNT, 5);
@@ -123,8 +126,18 @@ static int tc358762_init(struct tc358762 *ctx)
 	tc358762_write(ctx, PPI_LPTXTIMECNT, LPX_PERIOD);
 
 	tc358762_write(ctx, SPICMR, 0x00);
-	tc358762_write(ctx, LCDCTRL, LCDCTRL_VSDELAY(1) | LCDCTRL_RGB888 |
-				     LCDCTRL_UNK6 | LCDCTRL_VTGEN);
+
+	lcdctrl = LCDCTRL_VSDELAY(1) | LCDCTRL_RGB888 |
+		  LCDCTRL_UNK6 | LCDCTRL_VTGEN;
+
+	if (ctx->mode.flags & DRM_MODE_FLAG_NHSYNC)
+		lcdctrl |= LCDCTRL_HSPOL;
+
+	if (ctx->mode.flags & DRM_MODE_FLAG_NVSYNC)
+		lcdctrl |= LCDCTRL_VSPOL;
+
+	tc358762_write(ctx, LCDCTRL, lcdctrl);
+
 	tc358762_write(ctx, SYSCTRL, 0x040f);
 	msleep(100);
 
@@ -194,6 +207,15 @@ static int tc358762_attach(struct drm_bridge *bridge,
 				 bridge, flags);
 }
 
+static void tc358762_bridge_mode_set(struct drm_bridge *bridge,
+				     const struct drm_display_mode *mode,
+				     const struct drm_display_mode *adj)
+{
+	struct tc358762 *ctx = bridge_to_tc358762(bridge);
+
+	drm_mode_copy(&ctx->mode, mode);
+}
+
 static const struct drm_bridge_funcs tc358762_bridge_funcs = {
 	.atomic_post_disable = tc358762_post_disable,
 	.atomic_pre_enable = tc358762_pre_enable,
@@ -202,6 +224,7 @@ static const struct drm_bridge_funcs tc358762_bridge_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
 	.attach = tc358762_attach,
+	.mode_set = tc358762_bridge_mode_set,
 };
 
 static int tc358762_parse_dt(struct tc358762 *ctx)
-- 
2.39.2

Re: [PATCH 5/5] drm/bridge: tc358762: Handle HS/VS polarity

[Sam Ravnborg]

On Thu, Jun 15, 2023 at 10:19:02PM +0200, Marek Vasut wrote:
> Add support for handling the HS/VS sync signals polarity in the bridge
> driver, otherwise e.g. DSIM bridge feeds the TC358762 inverted polarity
> sync signals and the image is shifted to the left, up, and wobbly.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Re: [PATCH 1/5] drm/bridge: tc358762: Split register programming from pre-enable to enable

[Sam Ravnborg]

Hi Marek,

On Thu, Jun 15, 2023 at 10:18:58PM +0200, Marek Vasut wrote:
> Move the register programming part, which actually enables the bridge and
> makes it push data out of its DPI side, into the enable callback. The DSI
> host like DSIM may not be able to transmit commands in pre_enable, moving
> the register programming into enable assures it can transmit commands.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

I was about to complain that the use of .enable is deprecated, but the
following patch fixes this.
So:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>


> ---
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/bridge/tc358762.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
> index 5641395fd310e..df9703eacab1f 100644
> --- a/drivers/gpu/drm/bridge/tc358762.c
> +++ b/drivers/gpu/drm/bridge/tc358762.c
> @@ -162,11 +162,17 @@ static void tc358762_pre_enable(struct drm_bridge *bridge)
>  		usleep_range(5000, 10000);
>  	}
>  
> +	ctx->pre_enabled = true;
> +}
> +
> +static void tc358762_enable(struct drm_bridge *bridge)
> +{
> +	struct tc358762 *ctx = bridge_to_tc358762(bridge);
> +	int ret;
> +
>  	ret = tc358762_init(ctx);
>  	if (ret < 0)
>  		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
> -
> -	ctx->pre_enabled = true;
>  }
>  
>  static int tc358762_attach(struct drm_bridge *bridge,
> @@ -181,6 +187,7 @@ static int tc358762_attach(struct drm_bridge *bridge,
>  static const struct drm_bridge_funcs tc358762_bridge_funcs = {
>  	.post_disable = tc358762_post_disable,
>  	.pre_enable = tc358762_pre_enable,
> +	.enable = tc358762_enable,
>  	.attach = tc358762_attach,
>  };
>  
> -- 
> 2.39.2

Re: [PATCH 1/5] drm/bridge: tc358762: Split register programming from pre-enable to enable

[Dmitry Baryshkov]

On 16/06/2023 22:02, Sam Ravnborg wrote:
> Hi Marek,
> 
> On Thu, Jun 15, 2023 at 10:18:58PM +0200, Marek Vasut wrote:
>> Move the register programming part, which actually enables the bridge and
>> makes it push data out of its DPI side, into the enable callback. The DSI
>> host like DSIM may not be able to transmit commands in pre_enable, moving
>> the register programming into enable assures it can transmit commands.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> I was about to complain that the use of .enable is deprecated, but the
> following patch fixes this.
> So:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Dear drm-misc / bridge maintainers. About two years ago nearly the same 
patch was rejected ([1]) on the grounds of some hosts (sunxi) being 
unable to send DCS commands in HS state. Later Dave created and landed 
patches that potentially enable all peripherals to send DSI commands 
from the pre_enable callback ([2]) and tc358762 picked up usage of these 
flags ([3]).

And then we land this patch, which contradicts all previous steps.
I think I fail to understand your actions.

[1] 
https://lore.kernel.org/linux-arm-msm/CAPY8ntBrhYAmsraDqJGuTrSL6VjGXBAMVoN7xweV7E4qZv+v3Q@mail.gmail.com/

[2] 
https://lore.kernel.org/r/20221205173328.1395350-5-dave.stevenson@raspberrypi.com

[3] 
https://lore.kernel.org/dri-devel/20230131141756.RFT.v2.1.I723a3761d57ea60c5dd754c144aed6c3b2ea6f5a@changeid/

> 
> 
>> ---
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>> Cc: Jonas Karlman <jonas@kwiboo.se>
>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>> Cc: Robert Foss <rfoss@kernel.org>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/bridge/tc358762.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
>> index 5641395fd310e..df9703eacab1f 100644
>> --- a/drivers/gpu/drm/bridge/tc358762.c
>> +++ b/drivers/gpu/drm/bridge/tc358762.c
>> @@ -162,11 +162,17 @@ static void tc358762_pre_enable(struct drm_bridge *bridge)
>>   		usleep_range(5000, 10000);
>>   	}
>>   
>> +	ctx->pre_enabled = true;
>> +}
>> +
>> +static void tc358762_enable(struct drm_bridge *bridge)
>> +{
>> +	struct tc358762 *ctx = bridge_to_tc358762(bridge);
>> +	int ret;
>> +
>>   	ret = tc358762_init(ctx);
>>   	if (ret < 0)
>>   		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
>> -
>> -	ctx->pre_enabled = true;
>>   }
>>   
>>   static int tc358762_attach(struct drm_bridge *bridge,
>> @@ -181,6 +187,7 @@ static int tc358762_attach(struct drm_bridge *bridge,
>>   static const struct drm_bridge_funcs tc358762_bridge_funcs = {
>>   	.post_disable = tc358762_post_disable,
>>   	.pre_enable = tc358762_pre_enable,
>> +	.enable = tc358762_enable,
>>   	.attach = tc358762_attach,
>>   };
>>   
>> -- 
>> 2.39.2

-- 
With best wishes
Dmitry

Re: [PATCH 1/5] drm/bridge: tc358762: Split register programming from pre-enable to enable

[rfoss]

From: Robert Foss <rfoss@kernel.org>

On Thu, 15 Jun 2023 22:18:58 +0200, Marek Vasut wrote:
> Move the register programming part, which actually enables the bridge and
> makes it push data out of its DPI side, into the enable callback. The DSI
> host like DSIM may not be able to transmit commands in pre_enable, moving
> the register programming into enable assures it can transmit commands.
> 
> 

Applied, thanks!

[1/5] drm/bridge: tc358762: Split register programming from pre-enable to enable
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=8a4b2fc9c91a
[2/5] drm/bridge: tc358762: Switch to atomic ops
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=404643859a4f
[3/5] drm/bridge: tc358762: Instruct DSI host to generate HSE packets
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=362fa8f6e6a0
[4/5] drm/bridge: tc358762: Guess the meaning of LCDCTRL bits
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=80382226ef6f
[5/5] drm/bridge: tc358762: Handle HS/VS polarity
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=7f4e171f9d05



Rob