[PATCH 0/4] drm/rcar-du: dsi: Implement DSI command support

[PATCH 0/4] drm/rcar-du: dsi: Implement DSI command support

[Marek Vasut]

Use BIT() macro. Clean up lane count handling for non-4-lane panels.

Implement support for DSI command transfer mode using register based
access, with maximum payload length of 16 Bytes in Long Packet.

Marek Vasut (4):
  drm/rcar-du: dsi: Convert register bits to BIT() macro
  drm/rcar-du: dsi: Remove fixed PPI lane count setup
  drm/rcar-du: dsi: Configure TXSETR register to match PPI lane count
  drm/rcar-du: dsi: Implement DSI command support

 .../gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c   | 220 ++++++++++++++++-
 .../drm/renesas/rcar-du/rcar_mipi_dsi_regs.h  | 226 ++++++++++++++----
 2 files changed, 395 insertions(+), 51 deletions(-)
---
Cc: David Airlie <airlied@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-renesas-soc@vger.kernel.org

-- 
2.47.2

[PATCH 1/4] drm/rcar-du: dsi: Convert register bits to BIT() macro

[Marek Vasut]

Convert register bits to BIT() macro where applicable. This is done
automatically using regex 's@(1 << \([0-9]\+\))@BIT(\1)', except for
two BPP_18 macros which are not bits, but bitfields, and which are
not modified.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: David Airlie <airlied@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-renesas-soc@vger.kernel.org
---
 .../drm/renesas/rcar-du/rcar_mipi_dsi_regs.h  | 92 +++++++++----------
 1 file changed, 46 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
index a6b276f1d6ee..b3e57217ae63 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
@@ -9,38 +9,38 @@
 #define __RCAR_MIPI_DSI_REGS_H__
 
 #define LINKSR				0x010
-#define LINKSR_LPBUSY			(1 << 1)
-#define LINKSR_HSBUSY			(1 << 0)
+#define LINKSR_LPBUSY			BIT(1)
+#define LINKSR_HSBUSY			BIT(0)
 
 /*
  * Video Mode Register
  */
 #define TXVMSETR			0x180
 #define TXVMSETR_SYNSEQ_PULSES		(0 << 16)
-#define TXVMSETR_SYNSEQ_EVENTS		(1 << 16)
-#define TXVMSETR_VSTPM			(1 << 15)
-#define TXVMSETR_PIXWDTH		(1 << 8)
-#define TXVMSETR_VSEN_EN		(1 << 4)
+#define TXVMSETR_SYNSEQ_EVENTS		BIT(16)
+#define TXVMSETR_VSTPM			BIT(15)
+#define TXVMSETR_PIXWDTH		BIT(8)
+#define TXVMSETR_VSEN_EN		BIT(4)
 #define TXVMSETR_VSEN_DIS		(0 << 4)
-#define TXVMSETR_HFPBPEN_EN		(1 << 2)
+#define TXVMSETR_HFPBPEN_EN		BIT(2)
 #define TXVMSETR_HFPBPEN_DIS		(0 << 2)
-#define TXVMSETR_HBPBPEN_EN		(1 << 1)
+#define TXVMSETR_HBPBPEN_EN		BIT(1)
 #define TXVMSETR_HBPBPEN_DIS		(0 << 1)
-#define TXVMSETR_HSABPEN_EN		(1 << 0)
+#define TXVMSETR_HSABPEN_EN		BIT(0)
 #define TXVMSETR_HSABPEN_DIS		(0 << 0)
 
 #define TXVMCR				0x190
-#define TXVMCR_VFCLR			(1 << 12)
-#define TXVMCR_EN_VIDEO			(1 << 0)
+#define TXVMCR_VFCLR			BIT(12)
+#define TXVMCR_EN_VIDEO			BIT(0)
 
 #define TXVMSR				0x1a0
-#define TXVMSR_STR			(1 << 16)
-#define TXVMSR_VFRDY			(1 << 12)
-#define TXVMSR_ACT			(1 << 8)
-#define TXVMSR_RDY			(1 << 0)
+#define TXVMSR_STR			BIT(16)
+#define TXVMSR_VFRDY			BIT(12)
+#define TXVMSR_ACT			BIT(8)
+#define TXVMSR_RDY			BIT(0)
 
 #define TXVMSCR				0x1a4
-#define TXVMSCR_STR			(1 << 16)
+#define TXVMSCR_STR			BIT(16)
 
 #define TXVMPSPHSETR			0x1c0
 #define TXVMPSPHSETR_DT_RGB16		(0x0e << 16)
@@ -51,11 +51,11 @@
 
 #define TXVMVPRMSET0R			0x1d0
 #define TXVMVPRMSET0R_HSPOL_HIG		(0 << 17)
-#define TXVMVPRMSET0R_HSPOL_LOW		(1 << 17)
+#define TXVMVPRMSET0R_HSPOL_LOW		BIT(17)
 #define TXVMVPRMSET0R_VSPOL_HIG		(0 << 16)
-#define TXVMVPRMSET0R_VSPOL_LOW		(1 << 16)
+#define TXVMVPRMSET0R_VSPOL_LOW		BIT(16)
 #define TXVMVPRMSET0R_CSPC_RGB		(0 << 4)
-#define TXVMVPRMSET0R_CSPC_YCbCr	(1 << 4)
+#define TXVMVPRMSET0R_CSPC_YCbCr	BIT(4)
 #define TXVMVPRMSET0R_BPP_16		(0 << 0)
 #define TXVMVPRMSET0R_BPP_18		(1 << 0)
 #define TXVMVPRMSET0R_BPP_24		(2 << 0)
@@ -84,21 +84,21 @@
 #define PPISETR_DLEN_1			(0x3 << 0)
 #define PPISETR_DLEN_2			(0x7 << 0)
 #define PPISETR_DLEN_3			(0xf << 0)
-#define PPISETR_CLEN			(1 << 8)
+#define PPISETR_CLEN			BIT(8)
 
 #define PPICLCR				0x710
-#define PPICLCR_TXREQHS			(1 << 8)
-#define PPICLCR_TXULPSEXT		(1 << 1)
-#define PPICLCR_TXULPSCLK		(1 << 0)
+#define PPICLCR_TXREQHS			BIT(8)
+#define PPICLCR_TXULPSEXT		BIT(1)
+#define PPICLCR_TXULPSCLK		BIT(0)
 
 #define PPICLSR				0x720
-#define PPICLSR_HSTOLP			(1 << 27)
-#define PPICLSR_TOHS			(1 << 26)
-#define PPICLSR_STPST			(1 << 0)
+#define PPICLSR_HSTOLP			BIT(27)
+#define PPICLSR_TOHS			BIT(26)
+#define PPICLSR_STPST			BIT(0)
 
 #define PPICLSCR			0x724
-#define PPICLSCR_HSTOLP			(1 << 27)
-#define PPICLSCR_TOHS			(1 << 26)
+#define PPICLSCR_HSTOLP			BIT(27)
+#define PPICLSCR_TOHS			BIT(26)
 
 #define PPIDLSR				0x760
 #define PPIDLSR_STPST			(0xf << 0)
@@ -107,21 +107,21 @@
  * Clocks registers
  */
 #define LPCLKSET			0x1000
-#define LPCLKSET_CKEN			(1 << 8)
+#define LPCLKSET_CKEN			BIT(8)
 #define LPCLKSET_LPCLKDIV(x)		(((x) & 0x3f) << 0)
 
 #define CFGCLKSET			0x1004
-#define CFGCLKSET_CKEN			(1 << 8)
+#define CFGCLKSET_CKEN			BIT(8)
 #define CFGCLKSET_CFGCLKDIV(x)		(((x) & 0x3f) << 0)
 
 #define DOTCLKDIV			0x1008
-#define DOTCLKDIV_CKEN			(1 << 8)
+#define DOTCLKDIV_CKEN			BIT(8)
 #define DOTCLKDIV_DOTCLKDIV(x)		(((x) & 0x3f) << 0)
 
 #define VCLKSET				0x100c
-#define VCLKSET_CKEN			(1 << 16)
+#define VCLKSET_CKEN			BIT(16)
 #define VCLKSET_COLOR_RGB		(0 << 8)
-#define VCLKSET_COLOR_YCC		(1 << 8)
+#define VCLKSET_COLOR_YCC		BIT(8)
 #define VCLKSET_DIV_V3U(x)		(((x) & 0x3) << 4)
 #define VCLKSET_DIV_V4H(x)		(((x) & 0x7) << 4)
 #define VCLKSET_BPP_16			(0 << 2)
@@ -131,23 +131,23 @@
 #define VCLKSET_LANE(x)			(((x) & 0x3) << 0)
 
 #define VCLKEN				0x1010
-#define VCLKEN_CKEN			(1 << 0)
+#define VCLKEN_CKEN			BIT(0)
 
 #define PHYSETUP			0x1014
 #define PHYSETUP_HSFREQRANGE(x)		(((x) & 0x7f) << 16)
 #define PHYSETUP_HSFREQRANGE_MASK	(0x7f << 16)
 #define PHYSETUP_CFGCLKFREQRANGE(x)	(((x) & 0x3f) << 8)
-#define PHYSETUP_SHUTDOWNZ		(1 << 1)
-#define PHYSETUP_RSTZ			(1 << 0)
+#define PHYSETUP_SHUTDOWNZ		BIT(1)
+#define PHYSETUP_RSTZ			BIT(0)
 
 #define CLOCKSET1			0x101c
-#define CLOCKSET1_LOCK_PHY		(1 << 17)
-#define CLOCKSET1_CLKSEL		(1 << 8)
+#define CLOCKSET1_LOCK_PHY		BIT(17)
+#define CLOCKSET1_CLKSEL		BIT(8)
 #define CLOCKSET1_CLKINSEL_EXTAL	(0 << 2)
-#define CLOCKSET1_CLKINSEL_DIG		(1 << 2)
-#define CLOCKSET1_CLKINSEL_DU		(1 << 3)
-#define CLOCKSET1_SHADOW_CLEAR		(1 << 1)
-#define CLOCKSET1_UPDATEPLL		(1 << 0)
+#define CLOCKSET1_CLKINSEL_DIG		BIT(2)
+#define CLOCKSET1_CLKINSEL_DU		BIT(3)
+#define CLOCKSET1_SHADOW_CLEAR		BIT(1)
+#define CLOCKSET1_UPDATEPLL		BIT(0)
 
 #define CLOCKSET2			0x1020
 #define CLOCKSET2_M(x)			(((x) & 0xfff) << 16)
@@ -161,15 +161,15 @@
 #define CLOCKSET3_GMP_CNTRL(x)		(((x) & 0x3) << 0)
 
 #define PHTW				0x1034
-#define PHTW_DWEN			(1 << 24)
+#define PHTW_DWEN			BIT(24)
 #define PHTW_TESTDIN_DATA(x)		(((x) & 0xff) << 16)
-#define PHTW_CWEN			(1 << 8)
+#define PHTW_CWEN			BIT(8)
 #define PHTW_TESTDIN_CODE(x)		(((x) & 0xff) << 0)
 
 #define PHTR				0x1038
-#define PHTR_TEST			(1 << 16)
+#define PHTR_TEST			BIT(16)
 
 #define PHTC				0x103c
-#define PHTC_TESTCLR			(1 << 0)
+#define PHTC_TESTCLR			BIT(0)
 
 #endif /* __RCAR_MIPI_DSI_REGS_H__ */
-- 
2.47.2

Re: [PATCH 1/4] drm/rcar-du: dsi: Convert register bits to BIT() macro

[Tomi Valkeinen]

Hi,

On 08/06/2025 17:24, Marek Vasut wrote:
> Convert register bits to BIT() macro where applicable. This is done
> automatically using regex 's@(1 << \([0-9]\+\))@BIT(\1)', except for
> two BPP_18 macros which are not bits, but bitfields, and which are
> not modified.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: David Airlie <airlied@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  .../drm/renesas/rcar-du/rcar_mipi_dsi_regs.h  | 92 +++++++++----------
>  1 file changed, 46 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> index a6b276f1d6ee..b3e57217ae63 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> @@ -9,38 +9,38 @@
>  #define __RCAR_MIPI_DSI_REGS_H__
>  
>  #define LINKSR				0x010
> -#define LINKSR_LPBUSY			(1 << 1)
> -#define LINKSR_HSBUSY			(1 << 0)
> +#define LINKSR_LPBUSY			BIT(1)
> +#define LINKSR_HSBUSY			BIT(0)
>  
>  /*
>   * Video Mode Register
>   */
>  #define TXVMSETR			0x180
>  #define TXVMSETR_SYNSEQ_PULSES		(0 << 16)
> -#define TXVMSETR_SYNSEQ_EVENTS		(1 << 16)
> -#define TXVMSETR_VSTPM			(1 << 15)
> -#define TXVMSETR_PIXWDTH		(1 << 8)
> -#define TXVMSETR_VSEN_EN		(1 << 4)
> +#define TXVMSETR_SYNSEQ_EVENTS		BIT(16)
> +#define TXVMSETR_VSTPM			BIT(15)
> +#define TXVMSETR_PIXWDTH		BIT(8)
> +#define TXVMSETR_VSEN_EN		BIT(4)
>  #define TXVMSETR_VSEN_DIS		(0 << 4)
> -#define TXVMSETR_HFPBPEN_EN		(1 << 2)
> +#define TXVMSETR_HFPBPEN_EN		BIT(2)
>  #define TXVMSETR_HFPBPEN_DIS		(0 << 2)
> -#define TXVMSETR_HBPBPEN_EN		(1 << 1)
> +#define TXVMSETR_HBPBPEN_EN		BIT(1)
>  #define TXVMSETR_HBPBPEN_DIS		(0 << 1)
> -#define TXVMSETR_HSABPEN_EN		(1 << 0)
> +#define TXVMSETR_HSABPEN_EN		BIT(0)
>  #define TXVMSETR_HSABPEN_DIS		(0 << 0)
>  
>  #define TXVMCR				0x190
> -#define TXVMCR_VFCLR			(1 << 12)
> -#define TXVMCR_EN_VIDEO			(1 << 0)
> +#define TXVMCR_VFCLR			BIT(12)
> +#define TXVMCR_EN_VIDEO			BIT(0)
>  
>  #define TXVMSR				0x1a0
> -#define TXVMSR_STR			(1 << 16)
> -#define TXVMSR_VFRDY			(1 << 12)
> -#define TXVMSR_ACT			(1 << 8)
> -#define TXVMSR_RDY			(1 << 0)
> +#define TXVMSR_STR			BIT(16)
> +#define TXVMSR_VFRDY			BIT(12)
> +#define TXVMSR_ACT			BIT(8)
> +#define TXVMSR_RDY			BIT(0)
>  
>  #define TXVMSCR				0x1a4
> -#define TXVMSCR_STR			(1 << 16)
> +#define TXVMSCR_STR			BIT(16)
>  
>  #define TXVMPSPHSETR			0x1c0
>  #define TXVMPSPHSETR_DT_RGB16		(0x0e << 16)
> @@ -51,11 +51,11 @@
>  
>  #define TXVMVPRMSET0R			0x1d0
>  #define TXVMVPRMSET0R_HSPOL_HIG		(0 << 17)
> -#define TXVMVPRMSET0R_HSPOL_LOW		(1 << 17)
> +#define TXVMVPRMSET0R_HSPOL_LOW		BIT(17)

I'm not sure about this (and below). We have two defines for the HSPOL,
high and low. If one of them is (x << y), shouldn't the other one be of
that style too?

>  #define TXVMVPRMSET0R_VSPOL_HIG		(0 << 16)
> -#define TXVMVPRMSET0R_VSPOL_LOW		(1 << 16)
> +#define TXVMVPRMSET0R_VSPOL_LOW		BIT(16)
>  #define TXVMVPRMSET0R_CSPC_RGB		(0 << 4)
> -#define TXVMVPRMSET0R_CSPC_YCbCr	(1 << 4)
> +#define TXVMVPRMSET0R_CSPC_YCbCr	BIT(4)

This looks like a value, not a bit.

>  #define TXVMVPRMSET0R_BPP_16		(0 << 0)
>  #define TXVMVPRMSET0R_BPP_18		(1 << 0)
>  #define TXVMVPRMSET0R_BPP_24		(2 << 0)
> @@ -84,21 +84,21 @@
>  #define PPISETR_DLEN_1			(0x3 << 0)
>  #define PPISETR_DLEN_2			(0x7 << 0)
>  #define PPISETR_DLEN_3			(0xf << 0)
> -#define PPISETR_CLEN			(1 << 8)
> +#define PPISETR_CLEN			BIT(8)
>  
>  #define PPICLCR				0x710
> -#define PPICLCR_TXREQHS			(1 << 8)
> -#define PPICLCR_TXULPSEXT		(1 << 1)
> -#define PPICLCR_TXULPSCLK		(1 << 0)
> +#define PPICLCR_TXREQHS			BIT(8)
> +#define PPICLCR_TXULPSEXT		BIT(1)
> +#define PPICLCR_TXULPSCLK		BIT(0)
>  
>  #define PPICLSR				0x720
> -#define PPICLSR_HSTOLP			(1 << 27)
> -#define PPICLSR_TOHS			(1 << 26)
> -#define PPICLSR_STPST			(1 << 0)
> +#define PPICLSR_HSTOLP			BIT(27)
> +#define PPICLSR_TOHS			BIT(26)
> +#define PPICLSR_STPST			BIT(0)
>  
>  #define PPICLSCR			0x724
> -#define PPICLSCR_HSTOLP			(1 << 27)
> -#define PPICLSCR_TOHS			(1 << 26)
> +#define PPICLSCR_HSTOLP			BIT(27)
> +#define PPICLSCR_TOHS			BIT(26)
>  
>  #define PPIDLSR				0x760
>  #define PPIDLSR_STPST			(0xf << 0)
> @@ -107,21 +107,21 @@
>   * Clocks registers
>   */
>  #define LPCLKSET			0x1000
> -#define LPCLKSET_CKEN			(1 << 8)
> +#define LPCLKSET_CKEN			BIT(8)
>  #define LPCLKSET_LPCLKDIV(x)		(((x) & 0x3f) << 0)
>  
>  #define CFGCLKSET			0x1004
> -#define CFGCLKSET_CKEN			(1 << 8)
> +#define CFGCLKSET_CKEN			BIT(8)
>  #define CFGCLKSET_CFGCLKDIV(x)		(((x) & 0x3f) << 0)
>  
>  #define DOTCLKDIV			0x1008
> -#define DOTCLKDIV_CKEN			(1 << 8)
> +#define DOTCLKDIV_CKEN			BIT(8)
>  #define DOTCLKDIV_DOTCLKDIV(x)		(((x) & 0x3f) << 0)
>  
>  #define VCLKSET				0x100c
> -#define VCLKSET_CKEN			(1 << 16)
> +#define VCLKSET_CKEN			BIT(16)
>  #define VCLKSET_COLOR_RGB		(0 << 8)
> -#define VCLKSET_COLOR_YCC		(1 << 8)
> +#define VCLKSET_COLOR_YCC		BIT(8)

And this.

>  #define VCLKSET_DIV_V3U(x)		(((x) & 0x3) << 4)
>  #define VCLKSET_DIV_V4H(x)		(((x) & 0x7) << 4)
>  #define VCLKSET_BPP_16			(0 << 2)
> @@ -131,23 +131,23 @@
>  #define VCLKSET_LANE(x)			(((x) & 0x3) << 0)
>  
>  #define VCLKEN				0x1010
> -#define VCLKEN_CKEN			(1 << 0)
> +#define VCLKEN_CKEN			BIT(0)
>  
>  #define PHYSETUP			0x1014
>  #define PHYSETUP_HSFREQRANGE(x)		(((x) & 0x7f) << 16)
>  #define PHYSETUP_HSFREQRANGE_MASK	(0x7f << 16)
>  #define PHYSETUP_CFGCLKFREQRANGE(x)	(((x) & 0x3f) << 8)
> -#define PHYSETUP_SHUTDOWNZ		(1 << 1)
> -#define PHYSETUP_RSTZ			(1 << 0)
> +#define PHYSETUP_SHUTDOWNZ		BIT(1)
> +#define PHYSETUP_RSTZ			BIT(0)
>  
>  #define CLOCKSET1			0x101c
> -#define CLOCKSET1_LOCK_PHY		(1 << 17)
> -#define CLOCKSET1_CLKSEL		(1 << 8)
> +#define CLOCKSET1_LOCK_PHY		BIT(17)
> +#define CLOCKSET1_CLKSEL		BIT(8)
>  #define CLOCKSET1_CLKINSEL_EXTAL	(0 << 2)
> -#define CLOCKSET1_CLKINSEL_DIG		(1 << 2)
> -#define CLOCKSET1_CLKINSEL_DU		(1 << 3)

And this (although the shift by 3 is confusing with CLOCKSET1_CLKINSEL_DU).

> -#define CLOCKSET1_SHADOW_CLEAR		(1 << 1)
> -#define CLOCKSET1_UPDATEPLL		(1 << 0)
> +#define CLOCKSET1_CLKINSEL_DIG		BIT(2)
> +#define CLOCKSET1_CLKINSEL_DU		BIT(3)
> +#define CLOCKSET1_SHADOW_CLEAR		BIT(1)
> +#define CLOCKSET1_UPDATEPLL		BIT(0)
>  
>  #define CLOCKSET2			0x1020
>  #define CLOCKSET2_M(x)			(((x) & 0xfff) << 16)
> @@ -161,15 +161,15 @@
>  #define CLOCKSET3_GMP_CNTRL(x)		(((x) & 0x3) << 0)
>  
>  #define PHTW				0x1034
> -#define PHTW_DWEN			(1 << 24)
> +#define PHTW_DWEN			BIT(24)
>  #define PHTW_TESTDIN_DATA(x)		(((x) & 0xff) << 16)
> -#define PHTW_CWEN			(1 << 8)
> +#define PHTW_CWEN			BIT(8)
>  #define PHTW_TESTDIN_CODE(x)		(((x) & 0xff) << 0)
>  
>  #define PHTR				0x1038
> -#define PHTR_TEST			(1 << 16)
> +#define PHTR_TEST			BIT(16)
>  
>  #define PHTC				0x103c
> -#define PHTC_TESTCLR			(1 << 0)
> +#define PHTC_TESTCLR			BIT(0)
>  
>  #endif /* __RCAR_MIPI_DSI_REGS_H__ */

Re: [PATCH 1/4] drm/rcar-du: dsi: Convert register bits to BIT() macro

[Marek Vasut]

On 8/12/25 3:26 PM, Tomi Valkeinen wrote:

Hi,

>> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>> index a6b276f1d6ee..b3e57217ae63 100644
>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h

[...]

>> @@ -51,11 +51,11 @@
>>   
>>   #define TXVMVPRMSET0R			0x1d0
>>   #define TXVMVPRMSET0R_HSPOL_HIG		(0 << 17)
>> -#define TXVMVPRMSET0R_HSPOL_LOW		(1 << 17)
>> +#define TXVMVPRMSET0R_HSPOL_LOW		BIT(17)
> 
> I'm not sure about this (and below). We have two defines for the HSPOL,
> high and low. If one of them is (x << y), shouldn't the other one be of
> that style too?
It is inconsistent, but one macro describes bit set to 0 and the other 
bit set to 1 (i.e. the actual bit) which is converted to BIT(n) macro. I 
would be tempted to remove the bits set to 0, that's probably the real 
discussion that should happen here. But that would also be a much bigger 
patch. What do you think ?

Re: [PATCH 1/4] drm/rcar-du: dsi: Convert register bits to BIT() macro

[Laurent Pinchart]

On Tue, Aug 12, 2025 at 09:32:36PM +0200, Marek Vasut wrote:
> On 8/12/25 3:26 PM, Tomi Valkeinen wrote:
> 
> Hi,
> 
> >> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> >> index a6b276f1d6ee..b3e57217ae63 100644
> >> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> >> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> 
> [...]
> 
> >> @@ -51,11 +51,11 @@
> >>   
> >>   #define TXVMVPRMSET0R			0x1d0
> >>   #define TXVMVPRMSET0R_HSPOL_HIG		(0 << 17)
> >> -#define TXVMVPRMSET0R_HSPOL_LOW		(1 << 17)
> >> +#define TXVMVPRMSET0R_HSPOL_LOW		BIT(17)
> > 
> > I'm not sure about this (and below). We have two defines for the HSPOL,
> > high and low. If one of them is (x << y), shouldn't the other one be of
> > that style too?
> 
> It is inconsistent, but one macro describes bit set to 0 and the other 
> bit set to 1 (i.e. the actual bit) which is converted to BIT(n) macro. I 
> would be tempted to remove the bits set to 0, that's probably the real 
> discussion that should happen here. But that would also be a much bigger 
> patch. What do you think ?

For what it's worth, for single-bit register fields, I usually define a
single macro. I understand it's usually a coding style preference.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] drm/rcar-du: dsi: Convert register bits to BIT() macro

[Geert Uytterhoeven]

On Tue, 12 Aug 2025 at 22:05, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, Aug 12, 2025 at 09:32:36PM +0200, Marek Vasut wrote:
> > On 8/12/25 3:26 PM, Tomi Valkeinen wrote:
> > >> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> > >> index a6b276f1d6ee..b3e57217ae63 100644
> > >> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> > >> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> >
> > [...]
> >
> > >> @@ -51,11 +51,11 @@
> > >>
> > >>   #define TXVMVPRMSET0R                    0x1d0
> > >>   #define TXVMVPRMSET0R_HSPOL_HIG          (0 << 17)
> > >> -#define TXVMVPRMSET0R_HSPOL_LOW           (1 << 17)
> > >> +#define TXVMVPRMSET0R_HSPOL_LOW           BIT(17)
> > >
> > > I'm not sure about this (and below). We have two defines for the HSPOL,
> > > high and low. If one of them is (x << y), shouldn't the other one be of
> > > that style too?
> >
> > It is inconsistent, but one macro describes bit set to 0 and the other
> > bit set to 1 (i.e. the actual bit) which is converted to BIT(n) macro. I
> > would be tempted to remove the bits set to 0, that's probably the real
> > discussion that should happen here. But that would also be a much bigger
> > patch. What do you think ?
>
> For what it's worth, for single-bit register fields, I usually define a
> single macro. I understand it's usually a coding style preference.

An alternative would be to define 3 macros:

    #define TXVMVPRMSET0R_HSPOL        BIT(17)
    #define TXVMVPRMSET0R_HSPOL_HIG    0
    #define TXVMVPRMSET0R_HSPOL_LOW    1

and use FIELD_PREP(TXVMVPRMSET0R_HSPOL, TXVMVPRMSET0R_HSPOL_{HIG,LOW}).
But I agree a single definition is fine for a single-bit register field.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] drm/rcar-du: dsi: Convert register bits to BIT() macro

[Marek Vasut]

On 8/13/25 8:59 AM, Geert Uytterhoeven wrote:
> On Tue, 12 Aug 2025 at 22:05, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> On Tue, Aug 12, 2025 at 09:32:36PM +0200, Marek Vasut wrote:
>>> On 8/12/25 3:26 PM, Tomi Valkeinen wrote:
>>>>> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>>> index a6b276f1d6ee..b3e57217ae63 100644
>>>>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>
>>> [...]
>>>
>>>>> @@ -51,11 +51,11 @@
>>>>>
>>>>>    #define TXVMVPRMSET0R                    0x1d0
>>>>>    #define TXVMVPRMSET0R_HSPOL_HIG          (0 << 17)
>>>>> -#define TXVMVPRMSET0R_HSPOL_LOW           (1 << 17)
>>>>> +#define TXVMVPRMSET0R_HSPOL_LOW           BIT(17)
>>>>
>>>> I'm not sure about this (and below). We have two defines for the HSPOL,
>>>> high and low. If one of them is (x << y), shouldn't the other one be of
>>>> that style too?
>>>
>>> It is inconsistent, but one macro describes bit set to 0 and the other
>>> bit set to 1 (i.e. the actual bit) which is converted to BIT(n) macro. I
>>> would be tempted to remove the bits set to 0, that's probably the real
>>> discussion that should happen here. But that would also be a much bigger
>>> patch. What do you think ?
>>
>> For what it's worth, for single-bit register fields, I usually define a
>> single macro. I understand it's usually a coding style preference.
> 
> An alternative would be to define 3 macros:
> 
>      #define TXVMVPRMSET0R_HSPOL        BIT(17)
>      #define TXVMVPRMSET0R_HSPOL_HIG    0
>      #define TXVMVPRMSET0R_HSPOL_LOW    1
> 
> and use FIELD_PREP(TXVMVPRMSET0R_HSPOL, TXVMVPRMSET0R_HSPOL_{HIG,LOW}).
> But I agree a single definition is fine for a single-bit register field.

I think single bit macro for single register bit is just about the right 
amount of complexity .

Re: [PATCH 1/4] drm/rcar-du: dsi: Convert register bits to BIT() macro

[Tomi Valkeinen]

Hi,

On 12/08/2025 22:32, Marek Vasut wrote:
> On 8/12/25 3:26 PM, Tomi Valkeinen wrote:
> 
> Hi,
> 
>>> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/
>>> drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>> index a6b276f1d6ee..b3e57217ae63 100644
>>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> 
> [...]
> 
>>> @@ -51,11 +51,11 @@
>>>     #define TXVMVPRMSET0R            0x1d0
>>>   #define TXVMVPRMSET0R_HSPOL_HIG        (0 << 17)
>>> -#define TXVMVPRMSET0R_HSPOL_LOW        (1 << 17)
>>> +#define TXVMVPRMSET0R_HSPOL_LOW        BIT(17)
>>
>> I'm not sure about this (and below). We have two defines for the HSPOL,
>> high and low. If one of them is (x << y), shouldn't the other one be of
>> that style too?
> It is inconsistent, but one macro describes bit set to 0 and the other
> bit set to 1 (i.e. the actual bit) which is converted to BIT(n) macro. I
> would be tempted to remove the bits set to 0, that's probably the real
> discussion that should happen here. But that would also be a much bigger
> patch. What do you think ?

In my mind if you have defines for both HIGH and LOW, you have a
bitfield with a value, the values being 0 and 1, and for values you use
(0 << 17) and (1 << 17). It just happens here that the bitfield value is
only one bit long.

But I'm also fine with having only "TXVMVPRMSET0R_HSPOL_LOW
BIT(17)", and then the interpretation is that we have a enable/disable
style bit.

In the end, I'm fine with any of these, or the current one in the patch.
Although the current one does rub me the wrong way just enough for me to
comment about it =).

 Tomi

Re: [PATCH 1/4] drm/rcar-du: dsi: Convert register bits to BIT() macro

[Marek Vasut]

On 8/13/25 9:42 AM, Tomi Valkeinen wrote:

Hi,

>>>> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/
>>>> drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>> index a6b276f1d6ee..b3e57217ae63 100644
>>>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>
>> [...]
>>
>>>> @@ -51,11 +51,11 @@
>>>>      #define TXVMVPRMSET0R            0x1d0
>>>>    #define TXVMVPRMSET0R_HSPOL_HIG        (0 << 17)
>>>> -#define TXVMVPRMSET0R_HSPOL_LOW        (1 << 17)
>>>> +#define TXVMVPRMSET0R_HSPOL_LOW        BIT(17)
>>>
>>> I'm not sure about this (and below). We have two defines for the HSPOL,
>>> high and low. If one of them is (x << y), shouldn't the other one be of
>>> that style too?
>> It is inconsistent, but one macro describes bit set to 0 and the other
>> bit set to 1 (i.e. the actual bit) which is converted to BIT(n) macro. I
>> would be tempted to remove the bits set to 0, that's probably the real
>> discussion that should happen here. But that would also be a much bigger
>> patch. What do you think ?
> 
> In my mind if you have defines for both HIGH and LOW, you have a
> bitfield with a value, the values being 0 and 1, and for values you use
> (0 << 17) and (1 << 17). It just happens here that the bitfield value is
> only one bit long.

I am not a big fan of that, it seems overcomplicated, hence this clean up.

> But I'm also fine with having only "TXVMVPRMSET0R_HSPOL_LOW
> BIT(17)", and then the interpretation is that we have a enable/disable
> style bit.

I think this would work, yes.

> In the end, I'm fine with any of these, or the current one in the patch.
> Although the current one does rub me the wrong way just enough for me to
> comment about it =).
I can also drop this patch from the series and do full conversion of the 
driver to TXVMVPRMSET0R_HSPOL_LOW BIT(17) style afterward. This patch is 
not strictly necessary for the follow up patches.

[PATCH 2/4] drm/rcar-du: dsi: Remove fixed PPI lane count setup

[Marek Vasut]

The R-Car DSI host is capable of operating in 1..4 DSI lane mode.
Remove hard-coded 4-lane configuration from PPI register settings
and instead configure the PPI lane count according to lane count
information already obtained by this driver instance.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: David Airlie <airlied@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c      | 2 +-
 drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h | 5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
index 7ab8be46c7f6..373bd0040a46 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
@@ -576,7 +576,7 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
 	udelay(10);
 	rcar_mipi_dsi_clr(dsi, CLOCKSET1, CLOCKSET1_UPDATEPLL);
 
-	ppisetr = PPISETR_DLEN_3 | PPISETR_CLEN;
+	ppisetr = ((BIT(dsi->lanes) - 1) & PPISETR_DLEN_MASK) | PPISETR_CLEN;
 	rcar_mipi_dsi_write(dsi, PPISETR, ppisetr);
 
 	rcar_mipi_dsi_set(dsi, PHYSETUP, PHYSETUP_SHUTDOWNZ);
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
index b3e57217ae63..cefa7e92b5b8 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
@@ -80,10 +80,7 @@
  * PHY-Protocol Interface (PPI) Registers
  */
 #define PPISETR				0x700
-#define PPISETR_DLEN_0			(0x1 << 0)
-#define PPISETR_DLEN_1			(0x3 << 0)
-#define PPISETR_DLEN_2			(0x7 << 0)
-#define PPISETR_DLEN_3			(0xf << 0)
+#define PPISETR_DLEN_MASK		(0xf << 0)
 #define PPISETR_CLEN			BIT(8)
 
 #define PPICLCR				0x710
-- 
2.47.2

Re: [PATCH 2/4] drm/rcar-du: dsi: Remove fixed PPI lane count setup

[Tomi Valkeinen]

Hi,

On 08/06/2025 17:24, Marek Vasut wrote:
> The R-Car DSI host is capable of operating in 1..4 DSI lane mode.
> Remove hard-coded 4-lane configuration from PPI register settings
> and instead configure the PPI lane count according to lane count
> information already obtained by this driver instance.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: David Airlie <airlied@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c      | 2 +-
>  drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h | 5 +----
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> index 7ab8be46c7f6..373bd0040a46 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> @@ -576,7 +576,7 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>  	udelay(10);
>  	rcar_mipi_dsi_clr(dsi, CLOCKSET1, CLOCKSET1_UPDATEPLL);
>  
> -	ppisetr = PPISETR_DLEN_3 | PPISETR_CLEN;
> +	ppisetr = ((BIT(dsi->lanes) - 1) & PPISETR_DLEN_MASK) | PPISETR_CLEN;
>  	rcar_mipi_dsi_write(dsi, PPISETR, ppisetr);
>  
>  	rcar_mipi_dsi_set(dsi, PHYSETUP, PHYSETUP_SHUTDOWNZ);
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> index b3e57217ae63..cefa7e92b5b8 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> @@ -80,10 +80,7 @@
>   * PHY-Protocol Interface (PPI) Registers
>   */
>  #define PPISETR				0x700
> -#define PPISETR_DLEN_0			(0x1 << 0)
> -#define PPISETR_DLEN_1			(0x3 << 0)
> -#define PPISETR_DLEN_2			(0x7 << 0)
> -#define PPISETR_DLEN_3			(0xf << 0)
> +#define PPISETR_DLEN_MASK		(0xf << 0)
>  #define PPISETR_CLEN			BIT(8)

Looks fine, but do you know what the TXSETR register does? It also has
LANECNT, but I don't see the driver touching that register at all.
TXSETR:LANECNT default value is 3 (4 lanes), which matches with the old
hardcoded behavior for PPISETR... So I wonder if that register should
also be set?

 Tomi

Re: [PATCH 2/4] drm/rcar-du: dsi: Remove fixed PPI lane count setup

[Tomi Valkeinen]

Hi,

On 12/08/2025 16:18, Tomi Valkeinen wrote:
> Hi,
> 
> On 08/06/2025 17:24, Marek Vasut wrote:
>> The R-Car DSI host is capable of operating in 1..4 DSI lane mode.
>> Remove hard-coded 4-lane configuration from PPI register settings
>> and instead configure the PPI lane count according to lane count
>> information already obtained by this driver instance.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>> ---
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Magnus Damm <magnus.damm@gmail.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Simona Vetter <simona@ffwll.ch>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linux-renesas-soc@vger.kernel.org
>> ---
>>  drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c      | 2 +-
>>  drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h | 5 +----
>>  2 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
>> index 7ab8be46c7f6..373bd0040a46 100644
>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
>> @@ -576,7 +576,7 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>>  	udelay(10);
>>  	rcar_mipi_dsi_clr(dsi, CLOCKSET1, CLOCKSET1_UPDATEPLL);
>>  
>> -	ppisetr = PPISETR_DLEN_3 | PPISETR_CLEN;
>> +	ppisetr = ((BIT(dsi->lanes) - 1) & PPISETR_DLEN_MASK) | PPISETR_CLEN;
>>  	rcar_mipi_dsi_write(dsi, PPISETR, ppisetr);
>>  
>>  	rcar_mipi_dsi_set(dsi, PHYSETUP, PHYSETUP_SHUTDOWNZ);
>> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>> index b3e57217ae63..cefa7e92b5b8 100644
>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>> @@ -80,10 +80,7 @@
>>   * PHY-Protocol Interface (PPI) Registers
>>   */
>>  #define PPISETR				0x700
>> -#define PPISETR_DLEN_0			(0x1 << 0)
>> -#define PPISETR_DLEN_1			(0x3 << 0)
>> -#define PPISETR_DLEN_2			(0x7 << 0)
>> -#define PPISETR_DLEN_3			(0xf << 0)
>> +#define PPISETR_DLEN_MASK		(0xf << 0)
>>  #define PPISETR_CLEN			BIT(8)
> 
> Looks fine, but do you know what the TXSETR register does? It also has
> LANECNT, but I don't see the driver touching that register at all.
> TXSETR:LANECNT default value is 3 (4 lanes), which matches with the old
> hardcoded behavior for PPISETR... So I wonder if that register should
> also be set?

Ah, never mind, I now saw the patch 3 =). But should it be before patch
2? Hmm, I guess that ordering is no better. Should they be combined into
"support 1,2,3 datalanes" patch?

 Tomi

Re: [PATCH 2/4] drm/rcar-du: dsi: Remove fixed PPI lane count setup

[Marek Vasut]

On 8/12/25 3:30 PM, Tomi Valkeinen wrote:

Hi,

>>> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>> index b3e57217ae63..cefa7e92b5b8 100644
>>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>> @@ -80,10 +80,7 @@
>>>    * PHY-Protocol Interface (PPI) Registers
>>>    */
>>>   #define PPISETR				0x700
>>> -#define PPISETR_DLEN_0			(0x1 << 0)
>>> -#define PPISETR_DLEN_1			(0x3 << 0)
>>> -#define PPISETR_DLEN_2			(0x7 << 0)
>>> -#define PPISETR_DLEN_3			(0xf << 0)
>>> +#define PPISETR_DLEN_MASK		(0xf << 0)
>>>   #define PPISETR_CLEN			BIT(8)
>>
>> Looks fine, but do you know what the TXSETR register does? It also has
>> LANECNT, but I don't see the driver touching that register at all.
>> TXSETR:LANECNT default value is 3 (4 lanes), which matches with the old
>> hardcoded behavior for PPISETR... So I wonder if that register should
>> also be set?
> 
> Ah, never mind, I now saw the patch 3 =). But should it be before patch
> 2? Hmm, I guess that ordering is no better. Should they be combined into
> "support 1,2,3 datalanes" patch?
I think each patch fixes slighly different issue, even if the issues are 
related. I tried to keep the issue description in each patch commit 
message for posterity. I can squash them if you think that's better, I 
don't mind either way.

Re: [PATCH 2/4] drm/rcar-du: dsi: Remove fixed PPI lane count setup

[Tomi Valkeinen]

Hi,

On 12/08/2025 22:35, Marek Vasut wrote:
> On 8/12/25 3:30 PM, Tomi Valkeinen wrote:
> 
> Hi,
> 
>>>> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/
>>>> drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>> index b3e57217ae63..cefa7e92b5b8 100644
>>>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>> @@ -80,10 +80,7 @@
>>>>    * PHY-Protocol Interface (PPI) Registers
>>>>    */
>>>>   #define PPISETR                0x700
>>>> -#define PPISETR_DLEN_0            (0x1 << 0)
>>>> -#define PPISETR_DLEN_1            (0x3 << 0)
>>>> -#define PPISETR_DLEN_2            (0x7 << 0)
>>>> -#define PPISETR_DLEN_3            (0xf << 0)
>>>> +#define PPISETR_DLEN_MASK        (0xf << 0)
>>>>   #define PPISETR_CLEN            BIT(8)
>>>
>>> Looks fine, but do you know what the TXSETR register does? It also has
>>> LANECNT, but I don't see the driver touching that register at all.
>>> TXSETR:LANECNT default value is 3 (4 lanes), which matches with the old
>>> hardcoded behavior for PPISETR... So I wonder if that register should
>>> also be set?
>>
>> Ah, never mind, I now saw the patch 3 =). But should it be before patch
>> 2? Hmm, I guess that ordering is no better. Should they be combined into
>> "support 1,2,3 datalanes" patch?
> I think each patch fixes slighly different issue, even if the issues are
> related. I tried to keep the issue description in each patch commit
> message for posterity. I can squash them if you think that's better, I
> don't mind either way.

I was thinking about this the user's or backporting point of view.
Neither of the commits (clearly) say that they add support for 1/2/3
lane modes. You say they "fix", but they're not quite fixes either. The
patch 3 could be considered a fix, but at the moment it just writes the
default value to the register, so no point in marking it as a fix to be
backported.

So... I don't have a strong opinion, but I think a single patch that
adds support to 1, 2,3 lanes makes most sense.

 Tomi

Re: [PATCH 2/4] drm/rcar-du: dsi: Remove fixed PPI lane count setup

[Marek Vasut]

On 8/13/25 9:34 AM, Tomi Valkeinen wrote:

Hi,

>>>>> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/
>>>>> drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>>> index b3e57217ae63..cefa7e92b5b8 100644
>>>>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>>> @@ -80,10 +80,7 @@
>>>>>     * PHY-Protocol Interface (PPI) Registers
>>>>>     */
>>>>>    #define PPISETR                0x700
>>>>> -#define PPISETR_DLEN_0            (0x1 << 0)
>>>>> -#define PPISETR_DLEN_1            (0x3 << 0)
>>>>> -#define PPISETR_DLEN_2            (0x7 << 0)
>>>>> -#define PPISETR_DLEN_3            (0xf << 0)
>>>>> +#define PPISETR_DLEN_MASK        (0xf << 0)
>>>>>    #define PPISETR_CLEN            BIT(8)
>>>>
>>>> Looks fine, but do you know what the TXSETR register does? It also has
>>>> LANECNT, but I don't see the driver touching that register at all.
>>>> TXSETR:LANECNT default value is 3 (4 lanes), which matches with the old
>>>> hardcoded behavior for PPISETR... So I wonder if that register should
>>>> also be set?
>>>
>>> Ah, never mind, I now saw the patch 3 =). But should it be before patch
>>> 2? Hmm, I guess that ordering is no better. Should they be combined into
>>> "support 1,2,3 datalanes" patch?
>> I think each patch fixes slighly different issue, even if the issues are
>> related. I tried to keep the issue description in each patch commit
>> message for posterity. I can squash them if you think that's better, I
>> don't mind either way.
> 
> I was thinking about this the user's or backporting point of view.
> Neither of the commits (clearly) say that they add support for 1/2/3
> lane modes.

The 1/2/3 lane mode was already implemented in the driver, except it was 
broken.

> You say they "fix", but they're not quite fixes either. The
> patch 3 could be considered a fix, but at the moment it just writes the
> default value to the register, so no point in marking it as a fix to be
> backported.

3/4 does write the DSI lane count into TXSETR , not the default value.

> So... I don't have a strong opinion, but I think a single patch that
> adds support to 1, 2,3 lanes makes most sense.

Lemme send a single patch with Fixes tag then. The combined patch does 
not look too great though.

Re: [PATCH 2/4] drm/rcar-du: dsi: Remove fixed PPI lane count setup

[Tomi Valkeinen]

Hi,

On 14/08/2025 00:06, Marek Vasut wrote:
> On 8/13/25 9:34 AM, Tomi Valkeinen wrote:
> 
> Hi,
> 
>>>>>> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/
>>>>>> drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>>>> index b3e57217ae63..cefa7e92b5b8 100644
>>>>>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>>>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>>>> @@ -80,10 +80,7 @@
>>>>>>     * PHY-Protocol Interface (PPI) Registers
>>>>>>     */
>>>>>>    #define PPISETR                0x700
>>>>>> -#define PPISETR_DLEN_0            (0x1 << 0)
>>>>>> -#define PPISETR_DLEN_1            (0x3 << 0)
>>>>>> -#define PPISETR_DLEN_2            (0x7 << 0)
>>>>>> -#define PPISETR_DLEN_3            (0xf << 0)
>>>>>> +#define PPISETR_DLEN_MASK        (0xf << 0)
>>>>>>    #define PPISETR_CLEN            BIT(8)
>>>>>
>>>>> Looks fine, but do you know what the TXSETR register does? It also has
>>>>> LANECNT, but I don't see the driver touching that register at all.
>>>>> TXSETR:LANECNT default value is 3 (4 lanes), which matches with the
>>>>> old
>>>>> hardcoded behavior for PPISETR... So I wonder if that register should
>>>>> also be set?
>>>>
>>>> Ah, never mind, I now saw the patch 3 =). But should it be before patch
>>>> 2? Hmm, I guess that ordering is no better. Should they be combined
>>>> into
>>>> "support 1,2,3 datalanes" patch?
>>> I think each patch fixes slighly different issue, even if the issues are
>>> related. I tried to keep the issue description in each patch commit
>>> message for posterity. I can squash them if you think that's better, I
>>> don't mind either way.
>>
>> I was thinking about this the user's or backporting point of view.
>> Neither of the commits (clearly) say that they add support for 1/2/3
>> lane modes.
> 
> The 1/2/3 lane mode was already implemented in the driver, except it was
> broken.

If it never worked, was it broken or not implemented? How much code the
original driver must have for the feature to have the feature
"implemented, just broken"? If it reads the num lanes from the DT, and
allows the driver to probe with 1-4 lanes, is it then "implemented, but
broken"? Or does the driver have to have a clear intent on having the
feature (even if it doesn't work) for it to be implemented?

I'm not trying to be annoying here, and I'm fine with the new patch you
sent. I bring this topic up as I just had a similar discussion in a
thread for another patch series, and the answer is not clear to me.

stable-kernel-rules.rst doesn't really cover this case, so, afaics,
someone could argue that this (well, the new one you sent) is not valid
stable patch: it doesn't fix a crash/hang/etc, it adds support for more
lane setups.

In this particular case I'm fine calling this a fix, and backporting to
stable, as the patch is such a small one and "obviously correct" (as
stable-kernel-rules.rst says) because with the only working lane setup,
4-lanes, we can easily see that the values written to registers are
identical to default/old values.

Still, if someone has feedback on how to approach this question, I
wouldn't mind hearing the thoughts =).

>> You say they "fix", but they're not quite fixes either. The
>> patch 3 could be considered a fix, but at the moment it just writes the
>> default value to the register, so no point in marking it as a fix to be
>> backported.
> 
> 3/4 does write the DSI lane count into TXSETR , not the default value.

I meant that as only the 4-lane mode works, I must assume all the users
use 4-lane mode. Thus with patch 3, a value identical to the default
value gets written, as everyone uses 4 lanes. So, if it's backported to
kernels where everyone uses 4 lanes, it won't change anything.

 Tomi

Re: [PATCH 2/4] drm/rcar-du: dsi: Remove fixed PPI lane count setup

[Marek Vasut]

On 8/14/25 7:39 AM, Tomi Valkeinen wrote:

Hello Tomi,

>> The 1/2/3 lane mode was already implemented in the driver, except it was
>> broken.
> 
> If it never worked, was it broken or not implemented? How much code the
> original driver must have for the feature to have the feature
> "implemented, just broken"?

I believe the code was all there, and the lane count could be 
configured, it simply never worked. That's why I think the lane count 
was implemented, but broken.

> If it reads the num lanes from the DT, and
> allows the driver to probe with 1-4 lanes, is it then "implemented, but
> broken"? Or does the driver have to have a clear intent on having the
> feature (even if it doesn't work) for it to be implemented?

If it was not implemented, then the lane count from DT should have been 
checked and if it was != 4, such configuration should have been rejected.

> I'm not trying to be annoying here, and I'm fine with the new patch you
> sent. I bring this topic up as I just had a similar discussion in a
> thread for another patch series, and the answer is not clear to me.
> 
> stable-kernel-rules.rst doesn't really cover this case, so, afaics,
> someone could argue that this (well, the new one you sent) is not valid
> stable patch: it doesn't fix a crash/hang/etc, it adds support for more
> lane setups.
> 
> In this particular case I'm fine calling this a fix, and backporting to
> stable, as the patch is such a small one and "obviously correct" (as
> stable-kernel-rules.rst says) because with the only working lane setup,
> 4-lanes, we can easily see that the values written to registers are
> identical to default/old values.
> 
> Still, if someone has feedback on how to approach this question, I
> wouldn't mind hearing the thoughts =).

See above why I think this is a fix, maybe it helps.

>>> You say they "fix", but they're not quite fixes either. The
>>> patch 3 could be considered a fix, but at the moment it just writes the
>>> default value to the register, so no point in marking it as a fix to be
>>> backported.
>>
>> 3/4 does write the DSI lane count into TXSETR , not the default value.
> 
> I meant that as only the 4-lane mode works, I must assume all the users
> use 4-lane mode.

Yes, that is currently the case.

> Thus with patch 3, a value identical to the default
> value gets written, as everyone uses 4 lanes. So, if it's backported to
> kernels where everyone uses 4 lanes, it won't change anything.
Correct.

[PATCH 3/4] drm/rcar-du: dsi: Configure TXSETR register to match PPI lane count

[Marek Vasut]

The R-Car V4H Reference Manual R19UH0186EJ0121 Rev.1.21 section
67.2.2.3 Tx Set Register (TXSETR), field LANECNT description
indicates that the TXSETR register LANECNT bitfield lane count
must be configured such, that it matches lane count configuration
in PPISETR register DLEN bitfield. Make sure the LANECNT and DLEN
bitfields are configured to match.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: David Airlie <airlied@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c      | 3 +++
 drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
index 373bd0040a46..c31e0d8f3ff9 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
@@ -576,6 +576,9 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
 	udelay(10);
 	rcar_mipi_dsi_clr(dsi, CLOCKSET1, CLOCKSET1_UPDATEPLL);
 
+	rcar_mipi_dsi_clr(dsi, TXSETR, TXSETR_LANECNT_MASK);
+	rcar_mipi_dsi_set(dsi, TXSETR, dsi->lanes - 1);
+
 	ppisetr = ((BIT(dsi->lanes) - 1) & PPISETR_DLEN_MASK) | PPISETR_CLEN;
 	rcar_mipi_dsi_write(dsi, PPISETR, ppisetr);
 
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
index cefa7e92b5b8..b018037e116d 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
@@ -12,6 +12,10 @@
 #define LINKSR_LPBUSY			BIT(1)
 #define LINKSR_HSBUSY			BIT(0)
 
+#define TXSETR				0x100
+#define TXSETR_EOTPEN			BIT(12)
+#define TXSETR_LANECNT_MASK		(0x3 << 0)
+
 /*
  * Video Mode Register
  */
-- 
2.47.2

[PATCH 4/4] drm/rcar-du: dsi: Implement DSI command support

[Marek Vasut]

Implement support for DSI command transfer mode. Transmission of both Short
Packet and Long Packet is implemented, so is command transmission to request
response from peripheral device and transmission of non-read command with BTA.

The AXI memory access mode is currently not implemented, each transfer is
performed purely using controller register interface. Short Packet transfer
can transfer up to 2 Bytes of data, Long Packet transfer can transfer up to
16 Bytes of data.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: David Airlie <airlied@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-renesas-soc@vger.kernel.org
---
 .../gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c   | 215 ++++++++++++++++++
 .../drm/renesas/rcar-du/rcar_mipi_dsi_regs.h  | 125 ++++++++++
 2 files changed, 340 insertions(+)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
index c31e0d8f3ff9..bc1151c3ce90 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
@@ -938,9 +938,224 @@ static int rcar_mipi_dsi_host_detach(struct mipi_dsi_host *host,
 	return 0;
 }
 
+static ssize_t rcar_mipi_dsi_host_tx_transfer(struct mipi_dsi_host *host,
+					      const struct mipi_dsi_msg *msg,
+					      bool is_rx_xfer)
+{
+	const bool is_tx_long = mipi_dsi_packet_format_is_long(msg->type);
+	struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host);
+	struct mipi_dsi_packet packet;
+	u8 payload[16] = { 0 };
+	u32 status;
+	int ret;
+
+	ret = mipi_dsi_create_packet(&packet, msg);
+	if (ret)
+		return ret;
+
+	/* Configure LP or HS command transfer. */
+	rcar_mipi_dsi_write(dsi, TXCMSETR, (msg->flags & MIPI_DSI_MSG_USE_LPM) ?
+					   TXCMSETR_SPDTYP : 0);
+
+	/* Register access mode for RX transfer. */
+	if (is_rx_xfer)
+		rcar_mipi_dsi_write(dsi, RXPSETR, 0);
+
+	/* Do not use IRQ, poll for completion, the completion is quick. */
+	rcar_mipi_dsi_write(dsi, TXCMIER, 0);
+
+	/*
+	 * Send the header:
+	 * header[0] = Virtual Channel + Data Type
+	 * header[1] = Word Count LSB (LP) or first param (SP)
+	 * header[2] = Word Count MSB (LP) or second param (SP)
+	 */
+	rcar_mipi_dsi_write(dsi, TXCMPHDR,
+			    (is_tx_long ? TXCMPHDR_FMT : 0) |
+			    TXCMPHDR_VC(msg->channel) |
+			    TXCMPHDR_DT(msg->type) |
+			    TXCMPHDR_DATA1(packet.header[2]) |
+			    TXCMPHDR_DATA0(packet.header[1]));
+
+	if (is_tx_long) {
+		memcpy(payload, packet.payload,
+		       min(msg->tx_len, sizeof(payload)));
+
+		rcar_mipi_dsi_write(dsi, TXCMPPD0R,
+				    (payload[3] << 24) | (payload[2] << 16) |
+				    (payload[1] << 8) | payload[0]);
+		rcar_mipi_dsi_write(dsi, TXCMPPD1R,
+				    (payload[7] << 24) | (payload[6] << 16) |
+				    (payload[5] << 8) | payload[4]);
+		rcar_mipi_dsi_write(dsi, TXCMPPD2R,
+				    (payload[11] << 24) | (payload[10] << 16) |
+				    (payload[9] << 8) | payload[8]);
+		rcar_mipi_dsi_write(dsi, TXCMPPD3R,
+				    (payload[15] << 24) | (payload[14] << 16) |
+				    (payload[13] << 8) | payload[12]);
+	}
+
+	/* Start the transfer, RX with BTA, TX without BTA */
+	if (is_rx_xfer) {
+		rcar_mipi_dsi_write(dsi, TXCMCR, TXCMCR_BTAREQ);
+
+		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
+					(status & RXPSR_BTAREQEND),
+					2000, 10000, false, dsi, RXPSR);
+	} else {
+		rcar_mipi_dsi_write(dsi, TXCMCR, TXCMCR_TXREQ);
+
+		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
+					(status & TXCMSR_TXREQEND),
+					2000, 10000, false, dsi, TXCMSR);
+	}
+
+	if (ret < 0) {
+		dev_err(dsi->dev, "Command transfer timeout (0x%08x)\n",
+			status);
+		return ret;
+	}
+
+	return packet.size;
+}
+
+static ssize_t rcar_mipi_dsi_host_rx_transfer(struct mipi_dsi_host *host,
+					      const struct mipi_dsi_msg *msg)
+{
+	struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host);
+	u8 *rx_buf = (u8 *)(msg->rx_buf);
+	u32 reg, data, status, wc;
+	int i, ret;
+
+	/* RX transfer received data validation and parsing starts here. */
+	reg = rcar_mipi_dsi_read(dsi, TOSR);
+	if (reg & TOSR_TATO) {	/* Turn-Around TimeOut. */
+		/* Clear TATO Turn-Around TimeOut bit. */
+		rcar_mipi_dsi_write(dsi, TOSR, TOSR_TATO);
+		return -ETIMEDOUT;
+	}
+
+	reg = rcar_mipi_dsi_read(dsi, RXPSR);
+
+	if (msg->flags & MIPI_DSI_MSG_REQ_ACK) {
+		/* Transfer with zero-length RX */
+		if (!(reg & RXPSR_RCVACK)) {
+			/* No ACK on RX response received */
+			return -EINVAL;
+		}
+	} else {
+		/* Transfer with non-zero-length RX */
+		if (!(reg & RXPSR_RCVRESP)) {
+			/* No packet header of RX response received */
+			return -EINVAL;
+		}
+
+		if (reg & (RXPSR_CRCERR | RXPSR_WCERR | RXPSR_AXIERR | RXPSR_OVRERR)) {
+			/* Incorrect response payload */
+			return -ENODATA;
+		}
+
+		data = rcar_mipi_dsi_read(dsi, RXPHDR);
+		if (data & RXPHDR_FMT) {	/* Long Packet Response */
+			/* Read Long Packet Response length from packet header. */
+			wc = data & 0xffff;
+			if (wc > msg->rx_len) {
+				dev_warn(dsi->dev,
+					 "Long Packet Response longer than RX buffer (%d), limited to %ld Bytes\n",
+					 wc, msg->rx_len);
+				wc = msg->rx_len;
+			}
+
+			if (wc > 16) {
+				dev_warn(dsi->dev,
+					 "Long Packet Response too long (%d), limited to 16 Bytes\n",
+					 wc);
+				wc = 16;
+			}
+
+			for (i = 0; i < msg->rx_len; i++) {
+				if (!(i % 4))
+					data = rcar_mipi_dsi_read(dsi, RXPPD0R + i);
+
+				rx_buf[i] = data & 0xff;
+				data >>= 8;
+			}
+		} else {	/* Short Packet Response */
+			if (msg->rx_len >= 1)
+				rx_buf[0] = data & 0xff;
+			if (msg->rx_len >= 2)
+				rx_buf[1] = (data >> 8) & 0xff;
+			if (msg->rx_len >= 3) {
+				dev_warn(dsi->dev,
+					 "Expected Short Packet Response too long (%ld), limited to 2 Bytes\n",
+					 msg->rx_len);
+			}
+		}
+	}
+
+	if (reg & RXPSR_RCVAKE) {
+		/* Acknowledge and Error report received */
+		return -EFAULT;
+	}
+
+	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
+				!(status & PPIDL0SR_DIR),
+				2000, 10000, false, dsi, PPIDL0SR);
+	if (ret < 0) {
+		dev_err(dsi->dev, "Command RX DIR timeout (0x%08x)\n", status);
+		return ret;
+	}
+
+	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
+				status & PPIDL0SR_STPST,
+				2000, 10000, false, dsi, PPIDL0SR);
+	if (ret < 0) {
+		dev_err(dsi->dev, "Command RX STPST timeout (0x%08x)\n", status);
+		return ret;
+	}
+
+	return 0;
+}
+
+static ssize_t rcar_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
+					   const struct mipi_dsi_msg *msg)
+{
+	const bool is_rx_xfer = (msg->flags & MIPI_DSI_MSG_REQ_ACK) || msg->rx_len;
+	struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host);
+	int ret;
+
+	if (msg->tx_len > 16 || msg->rx_len > 16) {
+		/* ToDo: Implement Memory on AXI bus command mode. */
+		dev_warn(dsi->dev,
+			 "Register-based command mode supports only up to 16 Bytes long payload\n");
+		return -EOPNOTSUPP;
+	}
+
+	ret = rcar_mipi_dsi_host_tx_transfer(host, msg, is_rx_xfer);
+
+	/* If TX transfer succeeded and this transfer has RX part. */
+	if (ret >= 0 && is_rx_xfer) {
+		ret = rcar_mipi_dsi_host_rx_transfer(host, msg);
+		if (ret)
+			return ret;
+
+		ret = msg->rx_len;
+	}
+
+	/* Wait a bit between commands */
+	usleep_range(1000, 2000);
+
+	/* Clear the completion interrupt */
+	if (!msg->rx_len)
+		rcar_mipi_dsi_write(dsi, TXCMSR, TXCMSR_TXREQEND);
+
+	return ret;
+}
+
 static const struct mipi_dsi_host_ops rcar_mipi_dsi_host_ops = {
 	.attach = rcar_mipi_dsi_host_attach,
 	.detach = rcar_mipi_dsi_host_detach,
+	.transfer = rcar_mipi_dsi_host_transfer
 };
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
index b018037e116d..8c5c4fc8a55c 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
@@ -16,6 +16,127 @@
 #define TXSETR_EOTPEN			BIT(12)
 #define TXSETR_LANECNT_MASK		(0x3 << 0)
 
+/*
+ * DSI Command Transfer Registers
+ */
+#define TXCMSETR			0x110
+#define TXCMSETR_SPDTYP			BIT(8)	/* 0:HS 1:LP */
+#define TXCMSETR_LPPDACC		BIT(0)
+#define TXCMCR				0x120
+#define TXCMCR_BTATYP			BIT(2)
+#define TXCMCR_BTAREQ			BIT(1)
+#define TXCMCR_TXREQ			BIT(0)
+#define TXCMSR				0x130
+#define TXCMSR_CLSNERR			BIT(18)
+#define TXCMSR_AXIERR			BIT(16)
+#define TXCMSR_TXREQEND			BIT(0)
+#define TXCMSCR				0x134
+#define TXCMSCR_CLSNERR			BIT(18)
+#define TXCMSCR_AXIERR			BIT(16)
+#define TXCMSCR_TXREQEND		BIT(0)
+#define TXCMIER				0x138
+#define TXCMIER_CLSNERR			BIT(18)
+#define TXCMIER_AXIERR			BIT(16)
+#define TXCMIER_TXREQEND		BIT(0)
+#define TXCMADDRSET0R			0x140
+#define TXCMPHDR			0x150
+#define TXCMPHDR_FMT			BIT(24)	/* 0:SP 1:LP */
+#define TXCMPHDR_VC(n)			(((n) & 0x3) << 22)
+#define TXCMPHDR_DT(n)			(((n) & 0x3f) << 16)
+#define TXCMPHDR_DATA1(n)		(((n) & 0xff) << 8)
+#define TXCMPHDR_DATA0(n)		(((n) & 0xff) << 0)
+#define TXCMPPD0R			0x160
+#define TXCMPPD1R			0x164
+#define TXCMPPD2R			0x168
+#define TXCMPPD3R			0x16c
+
+#define RXSETR				0x200
+#define RXSETR_CRCEN			(((n) & 0xf) << 24)
+#define RXSETR_ECCEN			(((n) & 0xf) << 16)
+#define RXPSETR				0x210
+#define RXPSETR_LPPDACC			BIT(0)
+#define RXPSR				0x220
+#define RXPSR_ECCERR1B			BIT(28)
+#define RXPSR_UEXTRGERR			BIT(25)
+#define RXPSR_RESPTOERR			BIT(24)
+#define RXPSR_OVRERR			BIT(23)
+#define RXPSR_AXIERR			BIT(22)
+#define RXPSR_CRCERR			BIT(21)
+#define RXPSR_WCERR			BIT(20)
+#define RXPSR_UEXDTERR			BIT(19)
+#define RXPSR_UEXPKTERR			BIT(18)
+#define RXPSR_ECCERR			BIT(17)
+#define RXPSR_MLFERR			BIT(16)
+#define RXPSR_RCVACK			BIT(14)
+#define RXPSR_RCVEOT			BIT(10)
+#define RXPSR_RCVAKE			BIT(9)
+#define RXPSR_RCVRESP			BIT(8)
+#define RXPSR_BTAREQEND			BIT(0)
+#define RXPSCR				0x224
+#define RXPSCR_ECCERR1B			BIT(28)
+#define RXPSCR_UEXTRGERR		BIT(25)
+#define RXPSCR_RESPTOERR		BIT(24)
+#define RXPSCR_OVRERR			BIT(23)
+#define RXPSCR_AXIERR			BIT(22)
+#define RXPSCR_CRCERR			BIT(21)
+#define RXPSCR_WCERR			BIT(20)
+#define RXPSCR_UEXDTERR			BIT(19)
+#define RXPSCR_UEXPKTERR		BIT(18)
+#define RXPSCR_ECCERR			BIT(17)
+#define RXPSCR_MLFERR			BIT(16)
+#define RXPSCR_RCVACK			BIT(14)
+#define RXPSCR_RCVEOT			BIT(10)
+#define RXPSCR_RCVAKE			BIT(9)
+#define RXPSCR_RCVRESP			BIT(8)
+#define RXPSCR_BTAREQEND		BIT(0)
+#define RXPIER				0x228
+#define RXPIER_ECCERR1B			BIT(28)
+#define RXPIER_UEXTRGERR		BIT(25)
+#define RXPIER_RESPTOERR		BIT(24)
+#define RXPIER_OVRERR			BIT(23)
+#define RXPIER_AXIERR			BIT(22)
+#define RXPIER_CRCERR			BIT(21)
+#define RXPIER_WCERR			BIT(20)
+#define RXPIER_UEXDTERR			BIT(19)
+#define RXPIER_UEXPKTERR		BIT(18)
+#define RXPIER_ECCERR			BIT(17)
+#define RXPIER_MLFERR			BIT(16)
+#define RXPIER_RCVACK			BIT(14)
+#define RXPIER_RCVEOT			BIT(10)
+#define RXPIER_RCVAKE			BIT(9)
+#define RXPIER_RCVRESP			BIT(8)
+#define RXPIER_BTAREQEND		BIT(0)
+#define RXPADDRSET0R			0x230
+#define RXPSIZESETR			0x238
+#define RXPSIZESETR_SIZE(n)		(((n) & 0xf) << 3)
+#define RXPHDR				0x240
+#define RXPHDR_FMT			BIT(24)	/* 0:SP 1:LP */
+#define RXPHDR_VC(n)			(((n) & 0x3) << 22)
+#define RXPHDR_DT(n)			(((n) & 0x3f) << 16)
+#define RXPHDR_DATA1(n)			(((n) & 0xff) << 8)
+#define RXPHDR_DATA0(n)			(((n) & 0xff) << 0)
+#define RXPPD0R				0x250
+#define RXPPD1R				0x254
+#define RXPPD2R				0x258
+#define RXPPD3R				0x25c
+#define AKEPR				0x300
+#define AKEPR_VC(n)			(((n) & 0x3) << 22)
+#define AKEPR_DT(n)			(((n) & 0x3f) << 16)
+#define AKEPR_ERRRPT(n)			(((n) & 0xffff) << 0)
+#define RXRESPTOSETR			0x400
+#define TACR				0x500
+#define TASR				0x510
+#define TASCR				0x514
+#define TAIER				0x518
+#define TOSR				0x610
+#define TOSR_TATO			BIT(2)
+#define TOSR_LRXHTO			BIT(1)
+#define TOSR_HRXTO			BIT(0)
+#define TOSCR				0x614
+#define TOSCR_TATO			BIT(2)
+#define TOSCR_LRXHTO			BIT(1)
+#define TOSCR_HRXTO			BIT(0)
+
 /*
  * Video Mode Register
  */
@@ -101,6 +222,10 @@
 #define PPICLSCR_HSTOLP			BIT(27)
 #define PPICLSCR_TOHS			BIT(26)
 
+#define PPIDL0SR			0x740
+#define PPIDL0SR_DIR			BIT(10)
+#define PPIDL0SR_STPST			BIT(6)
+
 #define PPIDLSR				0x760
 #define PPIDLSR_STPST			(0xf << 0)
 
-- 
2.47.2

Re: [PATCH 4/4] drm/rcar-du: dsi: Implement DSI command support

[kernel test robot]

Hi Marek,

kernel test robot noticed the following build warnings:

[auto build test WARNING on geert-renesas-devel/next]
[also build test WARNING on drm-exynos/exynos-drm-next linus/master v6.16-rc1 next-20250606]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Marek-Vasut/drm-rcar-du-dsi-Convert-register-bits-to-BIT-macro/20250609-054641
base:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next
patch link:    https://lore.kernel.org/r/20250608142636.54033-5-marek.vasut%2Brenesas%40mailbox.org
patch subject: [PATCH 4/4] drm/rcar-du: dsi: Implement DSI command support
config: i386-buildonly-randconfig-005-20250609 (https://download.01.org/0day-ci/archive/20250609/202506090832.Vo4IJeD2-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250609/202506090832.Vo4IJeD2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506090832.Vo4IJeD2-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c:1064:11: warning: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
    1063 |                                          "Long Packet Response longer than RX buffer (%d), limited to %ld Bytes\n",
         |                                                                                                       ~~~
         |                                                                                                       %zu
    1064 |                                          wc, msg->rx_len);
         |                                              ^~~~~~~~~~~
   include/linux/dev_printk.h:156:70: note: expanded from macro 'dev_warn'
     156 |         dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                     ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c:1090:7: warning: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
    1089 |                                          "Expected Short Packet Response too long (%ld), limited to 2 Bytes\n",
         |                                                                                    ~~~
         |                                                                                    %zu
    1090 |                                          msg->rx_len);
         |                                          ^~~~~~~~~~~
   include/linux/dev_printk.h:156:70: note: expanded from macro 'dev_warn'
     156 |         dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                     ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   2 warnings generated.


vim +1064 drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c

  1020	
  1021	static ssize_t rcar_mipi_dsi_host_rx_transfer(struct mipi_dsi_host *host,
  1022						      const struct mipi_dsi_msg *msg)
  1023	{
  1024		struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host);
  1025		u8 *rx_buf = (u8 *)(msg->rx_buf);
  1026		u32 reg, data, status, wc;
  1027		int i, ret;
  1028	
  1029		/* RX transfer received data validation and parsing starts here. */
  1030		reg = rcar_mipi_dsi_read(dsi, TOSR);
  1031		if (reg & TOSR_TATO) {	/* Turn-Around TimeOut. */
  1032			/* Clear TATO Turn-Around TimeOut bit. */
  1033			rcar_mipi_dsi_write(dsi, TOSR, TOSR_TATO);
  1034			return -ETIMEDOUT;
  1035		}
  1036	
  1037		reg = rcar_mipi_dsi_read(dsi, RXPSR);
  1038	
  1039		if (msg->flags & MIPI_DSI_MSG_REQ_ACK) {
  1040			/* Transfer with zero-length RX */
  1041			if (!(reg & RXPSR_RCVACK)) {
  1042				/* No ACK on RX response received */
  1043				return -EINVAL;
  1044			}
  1045		} else {
  1046			/* Transfer with non-zero-length RX */
  1047			if (!(reg & RXPSR_RCVRESP)) {
  1048				/* No packet header of RX response received */
  1049				return -EINVAL;
  1050			}
  1051	
  1052			if (reg & (RXPSR_CRCERR | RXPSR_WCERR | RXPSR_AXIERR | RXPSR_OVRERR)) {
  1053				/* Incorrect response payload */
  1054				return -ENODATA;
  1055			}
  1056	
  1057			data = rcar_mipi_dsi_read(dsi, RXPHDR);
  1058			if (data & RXPHDR_FMT) {	/* Long Packet Response */
  1059				/* Read Long Packet Response length from packet header. */
  1060				wc = data & 0xffff;
  1061				if (wc > msg->rx_len) {
  1062					dev_warn(dsi->dev,
  1063						 "Long Packet Response longer than RX buffer (%d), limited to %ld Bytes\n",
> 1064						 wc, msg->rx_len);
  1065					wc = msg->rx_len;
  1066				}
  1067	
  1068				if (wc > 16) {
  1069					dev_warn(dsi->dev,
  1070						 "Long Packet Response too long (%d), limited to 16 Bytes\n",
  1071						 wc);
  1072					wc = 16;
  1073				}
  1074	
  1075				for (i = 0; i < msg->rx_len; i++) {
  1076					if (!(i % 4))
  1077						data = rcar_mipi_dsi_read(dsi, RXPPD0R + i);
  1078	
  1079					rx_buf[i] = data & 0xff;
  1080					data >>= 8;
  1081				}
  1082			} else {	/* Short Packet Response */
  1083				if (msg->rx_len >= 1)
  1084					rx_buf[0] = data & 0xff;
  1085				if (msg->rx_len >= 2)
  1086					rx_buf[1] = (data >> 8) & 0xff;
  1087				if (msg->rx_len >= 3) {
  1088					dev_warn(dsi->dev,
  1089						 "Expected Short Packet Response too long (%ld), limited to 2 Bytes\n",
  1090						 msg->rx_len);
  1091				}
  1092			}
  1093		}
  1094	
  1095		if (reg & RXPSR_RCVAKE) {
  1096			/* Acknowledge and Error report received */
  1097			return -EFAULT;
  1098		}
  1099	
  1100		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
  1101					!(status & PPIDL0SR_DIR),
  1102					2000, 10000, false, dsi, PPIDL0SR);
  1103		if (ret < 0) {
  1104			dev_err(dsi->dev, "Command RX DIR timeout (0x%08x)\n", status);
  1105			return ret;
  1106		}
  1107	
  1108		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
  1109					status & PPIDL0SR_STPST,
  1110					2000, 10000, false, dsi, PPIDL0SR);
  1111		if (ret < 0) {
  1112			dev_err(dsi->dev, "Command RX STPST timeout (0x%08x)\n", status);
  1113			return ret;
  1114		}
  1115	
  1116		return 0;
  1117	}
  1118	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/4] drm/rcar-du: dsi: Implement DSI command support

[Tomi Valkeinen]

Hi,

On 08/06/2025 17:24, Marek Vasut wrote:
> Implement support for DSI command transfer mode. Transmission of both Short

I constantly kept reading "DSI command mode support". So I was quite
confused for a bit =). Maybe avoid the use of "mode" with "DSI command".

> Packet and Long Packet is implemented, so is command transmission to request
> response from peripheral device and transmission of non-read command with BTA.
> 
> The AXI memory access mode is currently not implemented, each transfer is
> performed purely using controller register interface. Short Packet transfer
> can transfer up to 2 Bytes of data, Long Packet transfer can transfer up to
> 16 Bytes of data.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: David Airlie <airlied@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  .../gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c   | 215 ++++++++++++++++++
>  .../drm/renesas/rcar-du/rcar_mipi_dsi_regs.h  | 125 ++++++++++
>  2 files changed, 340 insertions(+)
> 
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> index c31e0d8f3ff9..bc1151c3ce90 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> @@ -938,9 +938,224 @@ static int rcar_mipi_dsi_host_detach(struct mipi_dsi_host *host,
>  	return 0;
>  }
>  
> +static ssize_t rcar_mipi_dsi_host_tx_transfer(struct mipi_dsi_host *host,
> +					      const struct mipi_dsi_msg *msg,
> +					      bool is_rx_xfer)
> +{
> +	const bool is_tx_long = mipi_dsi_packet_format_is_long(msg->type);
> +	struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host);
> +	struct mipi_dsi_packet packet;
> +	u8 payload[16] = { 0 };
> +	u32 status;
> +	int ret;
> +
> +	ret = mipi_dsi_create_packet(&packet, msg);
> +	if (ret)
> +		return ret;
> +
> +	/* Configure LP or HS command transfer. */
> +	rcar_mipi_dsi_write(dsi, TXCMSETR, (msg->flags & MIPI_DSI_MSG_USE_LPM) ?
> +					   TXCMSETR_SPDTYP : 0);

There's no runtime PM in the driver, and the clocks are enabled
externally... So I think we just assume that the IP is running here?
It's not a few times (on other platforms) I've hit an issue with
dsi_host tx being called when the video output is not yet enabled, and
thus the whole IP is off. So, maybe this works, but just a note.

> +	/* Register access mode for RX transfer. */
> +	if (is_rx_xfer)
> +		rcar_mipi_dsi_write(dsi, RXPSETR, 0);
> +
> +	/* Do not use IRQ, poll for completion, the completion is quick. */
> +	rcar_mipi_dsi_write(dsi, TXCMIER, 0);
> +
> +	/*
> +	 * Send the header:
> +	 * header[0] = Virtual Channel + Data Type
> +	 * header[1] = Word Count LSB (LP) or first param (SP)
> +	 * header[2] = Word Count MSB (LP) or second param (SP)
> +	 */
> +	rcar_mipi_dsi_write(dsi, TXCMPHDR,
> +			    (is_tx_long ? TXCMPHDR_FMT : 0) |
> +			    TXCMPHDR_VC(msg->channel) |
> +			    TXCMPHDR_DT(msg->type) |
> +			    TXCMPHDR_DATA1(packet.header[2]) |
> +			    TXCMPHDR_DATA0(packet.header[1]));
> +
> +	if (is_tx_long) {
> +		memcpy(payload, packet.payload,
> +		       min(msg->tx_len, sizeof(payload)));
> +
> +		rcar_mipi_dsi_write(dsi, TXCMPPD0R,
> +				    (payload[3] << 24) | (payload[2] << 16) |
> +				    (payload[1] << 8) | payload[0]);
> +		rcar_mipi_dsi_write(dsi, TXCMPPD1R,
> +				    (payload[7] << 24) | (payload[6] << 16) |
> +				    (payload[5] << 8) | payload[4]);
> +		rcar_mipi_dsi_write(dsi, TXCMPPD2R,
> +				    (payload[11] << 24) | (payload[10] << 16) |
> +				    (payload[9] << 8) | payload[8]);
> +		rcar_mipi_dsi_write(dsi, TXCMPPD3R,
> +				    (payload[15] << 24) | (payload[14] << 16) |
> +				    (payload[13] << 8) | payload[12]);
> +	}
> +
> +	/* Start the transfer, RX with BTA, TX without BTA */
> +	if (is_rx_xfer) {
> +		rcar_mipi_dsi_write(dsi, TXCMCR, TXCMCR_BTAREQ);
> +
> +		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> +					(status & RXPSR_BTAREQEND),
> +					2000, 10000, false, dsi, RXPSR);
> +	} else {
> +		rcar_mipi_dsi_write(dsi, TXCMCR, TXCMCR_TXREQ);
> +
> +		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> +					(status & TXCMSR_TXREQEND),
> +					2000, 10000, false, dsi, TXCMSR);
> +	}

Did you check the timeout is big enough? With LP and BTA... Well, it's
only 16 bytes at max. Maybe it's fine. Again, just a note. =)

Does this work when the video stream is on? If yes, then it might take
much longer until the command can be transferred. If not maybe the
function should return an error if the video stream is enabled.

What do these read_poll_timeouts wait, exactly? The first one waits
until the data is sent, and BTA has been done? And the latter waits only
for the data to be sent? Hmm, no... The first must wait until the
peripheral has replied, as there's no wait in the
rcar_mipi_dsi_host_rx_transfer()...

It would be nice to have a short comment what the wait is for.

> +
> +	if (ret < 0) {
> +		dev_err(dsi->dev, "Command transfer timeout (0x%08x)\n",
> +			status);
> +		return ret;
> +	}
> +
> +	return packet.size;
> +}
> +
> +static ssize_t rcar_mipi_dsi_host_rx_transfer(struct mipi_dsi_host *host,
> +					      const struct mipi_dsi_msg *msg)
> +{
> +	struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host);
> +	u8 *rx_buf = (u8 *)(msg->rx_buf);
> +	u32 reg, data, status, wc;
> +	int i, ret;
> +
> +	/* RX transfer received data validation and parsing starts here. */
> +	reg = rcar_mipi_dsi_read(dsi, TOSR);
> +	if (reg & TOSR_TATO) {	/* Turn-Around TimeOut. */
> +		/* Clear TATO Turn-Around TimeOut bit. */
> +		rcar_mipi_dsi_write(dsi, TOSR, TOSR_TATO);
> +		return -ETIMEDOUT;
> +	}
> +
> +	reg = rcar_mipi_dsi_read(dsi, RXPSR);
> +
> +	if (msg->flags & MIPI_DSI_MSG_REQ_ACK) {
> +		/* Transfer with zero-length RX */
> +		if (!(reg & RXPSR_RCVACK)) {
> +			/* No ACK on RX response received */
> +			return -EINVAL;
> +		}
> +	} else {
> +		/* Transfer with non-zero-length RX */
> +		if (!(reg & RXPSR_RCVRESP)) {
> +			/* No packet header of RX response received */
> +			return -EINVAL;
> +		}
> +
> +		if (reg & (RXPSR_CRCERR | RXPSR_WCERR | RXPSR_AXIERR | RXPSR_OVRERR)) {
> +			/* Incorrect response payload */
> +			return -ENODATA;
> +		}
> +
> +		data = rcar_mipi_dsi_read(dsi, RXPHDR);
> +		if (data & RXPHDR_FMT) {	/* Long Packet Response */
> +			/* Read Long Packet Response length from packet header. */
> +			wc = data & 0xffff;
> +			if (wc > msg->rx_len) {
> +				dev_warn(dsi->dev,
> +					 "Long Packet Response longer than RX buffer (%d), limited to %ld Bytes\n",
> +					 wc, msg->rx_len);
> +				wc = msg->rx_len;
> +			}
> +
> +			if (wc > 16) {
> +				dev_warn(dsi->dev,
> +					 "Long Packet Response too long (%d), limited to 16 Bytes\n",
> +					 wc);
> +				wc = 16;
> +			}
> +
> +			for (i = 0; i < msg->rx_len; i++) {
> +				if (!(i % 4))
> +					data = rcar_mipi_dsi_read(dsi, RXPPD0R + i);
> +
> +				rx_buf[i] = data & 0xff;
> +				data >>= 8;
> +			}
> +		} else {	/* Short Packet Response */
> +			if (msg->rx_len >= 1)
> +				rx_buf[0] = data & 0xff;
> +			if (msg->rx_len >= 2)
> +				rx_buf[1] = (data >> 8) & 0xff;
> +			if (msg->rx_len >= 3) {
> +				dev_warn(dsi->dev,
> +					 "Expected Short Packet Response too long (%ld), limited to 2 Bytes\n",
> +					 msg->rx_len);
> +			}
> +		}
> +	}
> +
> +	if (reg & RXPSR_RCVAKE) {
> +		/* Acknowledge and Error report received */
> +		return -EFAULT;
> +	}
> +
> +	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> +				!(status & PPIDL0SR_DIR),
> +				2000, 10000, false, dsi, PPIDL0SR);
> +	if (ret < 0) {
> +		dev_err(dsi->dev, "Command RX DIR timeout (0x%08x)\n", status);
> +		return ret;
> +	}
> +
> +	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> +				status & PPIDL0SR_STPST,
> +				2000, 10000, false, dsi, PPIDL0SR);
> +	if (ret < 0) {
> +		dev_err(dsi->dev, "Command RX STPST timeout (0x%08x)\n", status);
> +		return ret;
> +	}

Same here, it's not very clear what the waits are for. Aren't we done
already after the tx function finished?

> +
> +	return 0;
> +}
> +
> +static ssize_t rcar_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
> +					   const struct mipi_dsi_msg *msg)
> +{
> +	const bool is_rx_xfer = (msg->flags & MIPI_DSI_MSG_REQ_ACK) || msg->rx_len;
> +	struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host);
> +	int ret;
> +
> +	if (msg->tx_len > 16 || msg->rx_len > 16) {
> +		/* ToDo: Implement Memory on AXI bus command mode. */
> +		dev_warn(dsi->dev,
> +			 "Register-based command mode supports only up to 16 Bytes long payload\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = rcar_mipi_dsi_host_tx_transfer(host, msg, is_rx_xfer);
> +
> +	/* If TX transfer succeeded and this transfer has RX part. */
> +	if (ret >= 0 && is_rx_xfer) {
> +		ret = rcar_mipi_dsi_host_rx_transfer(host, msg);
> +		if (ret)
> +			return ret;
> +
> +		ret = msg->rx_len;
> +	}
> +
> +	/* Wait a bit between commands */
> +	usleep_range(1000, 2000);

Why wait and wait a bit between what?

 Tomi

Re: [PATCH 4/4] drm/rcar-du: dsi: Implement DSI command support

[Marek Vasut]

On 8/12/25 4:36 PM, Tomi Valkeinen wrote:

Hello Tomi,

> On 08/06/2025 17:24, Marek Vasut wrote:
>> Implement support for DSI command transfer mode. Transmission of both Short
> 
> I constantly kept reading "DSI command mode support". So I was quite
> confused for a bit =). Maybe avoid the use of "mode" with "DSI command".

I dropped the "mode" in V3.

[...]

>> +static ssize_t rcar_mipi_dsi_host_tx_transfer(struct mipi_dsi_host *host,
>> +					      const struct mipi_dsi_msg *msg,
>> +					      bool is_rx_xfer)
>> +{
>> +	const bool is_tx_long = mipi_dsi_packet_format_is_long(msg->type);
>> +	struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host);
>> +	struct mipi_dsi_packet packet;
>> +	u8 payload[16] = { 0 };
>> +	u32 status;
>> +	int ret;
>> +
>> +	ret = mipi_dsi_create_packet(&packet, msg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Configure LP or HS command transfer. */
>> +	rcar_mipi_dsi_write(dsi, TXCMSETR, (msg->flags & MIPI_DSI_MSG_USE_LPM) ?
>> +					   TXCMSETR_SPDTYP : 0);
> 
> There's no runtime PM in the driver, and the clocks are enabled
> externally... So I think we just assume that the IP is running here?

Correct.

[...]

>> +	/* Start the transfer, RX with BTA, TX without BTA */
>> +	if (is_rx_xfer) {
>> +		rcar_mipi_dsi_write(dsi, TXCMCR, TXCMCR_BTAREQ);
>> +
>> +		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
>> +					(status & RXPSR_BTAREQEND),
>> +					2000, 10000, false, dsi, RXPSR);
>> +	} else {
>> +		rcar_mipi_dsi_write(dsi, TXCMCR, TXCMCR_TXREQ);
>> +
>> +		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
>> +					(status & TXCMSR_TXREQEND),
>> +					2000, 10000, false, dsi, TXCMSR);
>> +	}
> 
> Did you check the timeout is big enough? With LP and BTA... Well, it's
> only 16 bytes at max. Maybe it's fine. Again, just a note. =)

I did check it with the only hardware I had available which needs this 
command mode so far, the RPi Display 2 using ILI9881 DSI-to-TCON .

> Does this work when the video stream is on?

That is untested, the ILI9881 only uses command mode during 
initialization, before it switches video mode on.

> If yes, then it might take
> much longer until the command can be transferred.

Do you know the upper limit , is that one or two frame times ?

> If not maybe the
> function should return an error if the video stream is enabled.

I haven't seen any drivers special casing that, I'd prefer to increase 
the timeouts. For V3, I'll update the timeout to 50ms , which is about 3 
frame times.

> What do these read_poll_timeouts wait, exactly? The first one waits
> until the data is sent, and BTA has been done? And the latter waits only
> for the data to be sent? Hmm, no... The first must wait until the
> peripheral has replied, as there's no wait in the
> rcar_mipi_dsi_host_rx_transfer()...

First one is transmit+BTA+receive , second one is only transmit .

> It would be nice to have a short comment what the wait is for.

Will do in V3.

[...]

>> +	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
>> +				status & PPIDL0SR_STPST,
>> +				2000, 10000, false, dsi, PPIDL0SR);
>> +	if (ret < 0) {
>> +		dev_err(dsi->dev, "Command RX STPST timeout (0x%08x)\n", status);
>> +		return ret;
>> +	}
> 
> Same here, it's not very clear what the waits are for. Aren't we done
> already after the tx function finished?

First one waits for bus handover to host processor to complete, the 
second one (STPST) waits for data lane to enter LP11 stop state .

>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t rcar_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>> +					   const struct mipi_dsi_msg *msg)
>> +{
>> +	const bool is_rx_xfer = (msg->flags & MIPI_DSI_MSG_REQ_ACK) || msg->rx_len;
>> +	struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host);
>> +	int ret;
>> +
>> +	if (msg->tx_len > 16 || msg->rx_len > 16) {
>> +		/* ToDo: Implement Memory on AXI bus command mode. */
>> +		dev_warn(dsi->dev,
>> +			 "Register-based command mode supports only up to 16 Bytes long payload\n");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	ret = rcar_mipi_dsi_host_tx_transfer(host, msg, is_rx_xfer);
>> +
>> +	/* If TX transfer succeeded and this transfer has RX part. */
>> +	if (ret >= 0 && is_rx_xfer) {
>> +		ret = rcar_mipi_dsi_host_rx_transfer(host, msg);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = msg->rx_len;
>> +	}
>> +
>> +	/* Wait a bit between commands */
>> +	usleep_range(1000, 2000);
> 
> Why wait and wait a bit between what?
The consecutive command transmission was unreliable unless there was a 
slight delay between the consecutive commands. Hence this delay.

Re: [PATCH 4/4] drm/rcar-du: dsi: Implement DSI command support

[Tomi Valkeinen]

Hi,

On 18/08/2025 02:40, Marek Vasut wrote:
> On 8/12/25 4:36 PM, Tomi Valkeinen wrote:
> 
> Hello Tomi,
> 
>> On 08/06/2025 17:24, Marek Vasut wrote:
>>> Implement support for DSI command transfer mode. Transmission of both
>>> Short
>>
>> I constantly kept reading "DSI command mode support". So I was quite
>> confused for a bit =). Maybe avoid the use of "mode" with "DSI command".
> 
> I dropped the "mode" in V3.
> 
> [...]
> 
>>> +static ssize_t rcar_mipi_dsi_host_tx_transfer(struct mipi_dsi_host
>>> *host,
>>> +                          const struct mipi_dsi_msg *msg,
>>> +                          bool is_rx_xfer)
>>> +{
>>> +    const bool is_tx_long = mipi_dsi_packet_format_is_long(msg->type);
>>> +    struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host);
>>> +    struct mipi_dsi_packet packet;
>>> +    u8 payload[16] = { 0 };
>>> +    u32 status;
>>> +    int ret;
>>> +
>>> +    ret = mipi_dsi_create_packet(&packet, msg);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    /* Configure LP or HS command transfer. */
>>> +    rcar_mipi_dsi_write(dsi, TXCMSETR, (msg->flags &
>>> MIPI_DSI_MSG_USE_LPM) ?
>>> +                       TXCMSETR_SPDTYP : 0);
>>
>> There's no runtime PM in the driver, and the clocks are enabled
>> externally... So I think we just assume that the IP is running here?
> 
> Correct.
> 
> [...]
> 
>>> +    /* Start the transfer, RX with BTA, TX without BTA */
>>> +    if (is_rx_xfer) {
>>> +        rcar_mipi_dsi_write(dsi, TXCMCR, TXCMCR_BTAREQ);
>>> +
>>> +        ret = read_poll_timeout(rcar_mipi_dsi_read, status,
>>> +                    (status & RXPSR_BTAREQEND),
>>> +                    2000, 10000, false, dsi, RXPSR);
>>> +    } else {
>>> +        rcar_mipi_dsi_write(dsi, TXCMCR, TXCMCR_TXREQ);
>>> +
>>> +        ret = read_poll_timeout(rcar_mipi_dsi_read, status,
>>> +                    (status & TXCMSR_TXREQEND),
>>> +                    2000, 10000, false, dsi, TXCMSR);
>>> +    }
>>
>> Did you check the timeout is big enough? With LP and BTA... Well, it's
>> only 16 bytes at max. Maybe it's fine. Again, just a note. =)
> 
> I did check it with the only hardware I had available which needs this
> command mode so far, the RPi Display 2 using ILI9881 DSI-to-TCON .

Don't call it "command mode" ;).

>> Does this work when the video stream is on?
> 
> That is untested, the ILI9881 only uses command mode during
> initialization, before it switches video mode on.
> 
>> If yes, then it might take
>> much longer until the command can be transferred.
> 
> Do you know the upper limit , is that one or two frame times ?

If using DSI video mode and the stream is on, the DSI TX has to
interleave the commands either to the line blanking or frame blanking.
Usually this is configurable in the DSI TX (if the chip can do
interleaving).

A read command will be the most difficult to interleave, as it takes
more time with the BTA and reply.

I think the worst case is one frame delay (next vblank).

>> If not maybe the
>> function should return an error if the video stream is enabled.
> 
> I haven't seen any drivers special casing that, I'd prefer to increase
> the timeouts. For V3, I'll update the timeout to 50ms , which is about 3
> frame times.

If the docs have no word about interleaving the commands and there are
no related registers, I would guess that it's not supported. If this
can't be tested, I suggest just returning an error if a command is sent
while the video is on.

You should be able to test this, though. E.g. just add a debugfs/sysfs
file to the panel or dsi tx driver, which does a DSI command, possibly a
read. See what happens when the video is enabled.

>> What do these read_poll_timeouts wait, exactly? The first one waits
>> until the data is sent, and BTA has been done? And the latter waits only
>> for the data to be sent? Hmm, no... The first must wait until the
>> peripheral has replied, as there's no wait in the
>> rcar_mipi_dsi_host_rx_transfer()...
> 
> First one is transmit+BTA+receive , second one is only transmit .
> 
>> It would be nice to have a short comment what the wait is for.
> 
> Will do in V3.
> 
> [...]
> 
>>> +    ret = read_poll_timeout(rcar_mipi_dsi_read, status,
>>> +                status & PPIDL0SR_STPST,
>>> +                2000, 10000, false, dsi, PPIDL0SR);
>>> +    if (ret < 0) {
>>> +        dev_err(dsi->dev, "Command RX STPST timeout (0x%08x)\n",
>>> status);
>>> +        return ret;
>>> +    }
>>
>> Same here, it's not very clear what the waits are for. Aren't we done
>> already after the tx function finished?
> 
> First one waits for bus handover to host processor to complete, the
> second one (STPST) waits for data lane to enter LP11 stop state .
> 
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static ssize_t rcar_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>>> +                       const struct mipi_dsi_msg *msg)
>>> +{
>>> +    const bool is_rx_xfer = (msg->flags & MIPI_DSI_MSG_REQ_ACK) ||
>>> msg->rx_len;
>>> +    struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host);
>>> +    int ret;
>>> +
>>> +    if (msg->tx_len > 16 || msg->rx_len > 16) {
>>> +        /* ToDo: Implement Memory on AXI bus command mode. */
>>> +        dev_warn(dsi->dev,
>>> +             "Register-based command mode supports only up to 16
>>> Bytes long payload\n");
>>> +        return -EOPNOTSUPP;
>>> +    }
>>> +
>>> +    ret = rcar_mipi_dsi_host_tx_transfer(host, msg, is_rx_xfer);
>>> +
>>> +    /* If TX transfer succeeded and this transfer has RX part. */
>>> +    if (ret >= 0 && is_rx_xfer) {
>>> +        ret = rcar_mipi_dsi_host_rx_transfer(host, msg);
>>> +        if (ret)
>>> +            return ret;
>>> +
>>> +        ret = msg->rx_len;
>>> +    }
>>> +
>>> +    /* Wait a bit between commands */
>>> +    usleep_range(1000, 2000);
>>
>> Why wait and wait a bit between what?
> The consecutive command transmission was unreliable unless there was a
> slight delay between the consecutive commands. Hence this delay.

This sounds like there's a bug in the driver, or the TX or RX hardware.
Please document the sleep clearly in the comment.

 Tomi

Re: [PATCH 4/4] drm/rcar-du: dsi: Implement DSI command support

[Marek Vasut]

On 8/18/25 9:20 AM, Tomi Valkeinen wrote:

Hello Tomi,

>>> If yes, then it might take
>>> much longer until the command can be transferred.
>>
>> Do you know the upper limit , is that one or two frame times ?
> 
> If using DSI video mode and the stream is on, the DSI TX has to
> interleave the commands either to the line blanking or frame blanking.
> Usually this is configurable in the DSI TX (if the chip can do
> interleaving).
> 
> A read command will be the most difficult to interleave, as it takes
> more time with the BTA and reply.
> 
> I think the worst case is one frame delay (next vblank).

I set the timeouts to 50ms, which at 60 Hz refresh should be a bit over 
three frame delays, which should be safe.

>>> If not maybe the
>>> function should return an error if the video stream is enabled.
>>
>> I haven't seen any drivers special casing that, I'd prefer to increase
>> the timeouts. For V3, I'll update the timeout to 50ms , which is about 3
>> frame times.
> 
> If the docs have no word about interleaving the commands and there are
> no related registers, I would guess that it's not supported. If this
> can't be tested, I suggest just returning an error if a command is sent
> while the video is on.
> 
> You should be able to test this, though. E.g. just add a debugfs/sysfs
> file to the panel or dsi tx driver, which does a DSI command, possibly a
> read. See what happens when the video is enabled.

I patched the ili9881c panel and triggered DCS read of its ID via sysfs 
attribute , I could always get valid ID, so I think we are safe here.

[...]

>>>> +    /* Wait a bit between commands */
>>>> +    usleep_range(1000, 2000);
>>>
>>> Why wait and wait a bit between what?
>> The consecutive command transmission was unreliable unless there was a
>> slight delay between the consecutive commands. Hence this delay.
> 
> This sounds like there's a bug in the driver, or the TX or RX hardware.
> Please document the sleep clearly in the comment.
Done.