Re: [PATCH v5 2/5] drm: bridge: dw_mipi_dsi: abstract register access using reg_fields

2020-04-10 Thread Adrian Ratiu

Hi Andrzej,

Thank you for the feedback, I really appreciate it, replies are 
below.


On Mon, 06 Apr 2020, Andrzej Hajda  wrote:
W dniu 30.03.2020 o 13:35, Adrian Ratiu pisze: 
Register existence, address/offsets, field layouts, reserved 
bits and so on differ between MIPI-DSI versions and between SoC 
vendor boards.  Despite these differences the hw IP and 
protocols are mostly the same so the generic bridge can be made 
to compensate these differences. 

The current Rockchip and STM drivers hardcoded a lot of their 
common definitions in the bridge code because they're based on 
DSI v1.30 and 1.31 which are relatively close, but in order to 
support older/future versions with more diverging layouts like 
the v1.01 present on imx6, we abstract some of the register 
accesses via the regmap field APIs. 

The bridge detects the DSI core version and initializes the 
required regmap register layout. Other DSI versions / register 
layouts can easily be added in the future by only changing the 
bridge code. 

The platform drivers don't require any changes, DSI register 
layout versioning will be handled transparently by the bridge, 
but if in the future the regmap or layouts needs to be exposed 
to the drivres, it could easily be done via plat_data or a new 
API in dw_mipi_dsi.h. 

Suggested-by: Boris Brezillon  
Reviewed-by: Emil Velikov  
Signed-off-by: Adrian Ratiu  --- 
Changes since v4: 
   - Move regmap infrastructure logic to a separate commit 
   (Ezequiel) - Consolidate field infrastructure in this commit 
   (Ezequiel) - Move the dsi v1.01 layout logic to a separate 
   commit (Ezequiel) 

Changes since v2: 
   - Added const declarations to dw_mipi_dsi structs (Emil) - 
   Fixed commit tags (Emil) 

Changes since v1: 
   - Moved register definitions & regmap initialization into 
   bridge module. Platform drivers get the regmap via plat_data 
   after calling the bridge probe (Emil). 
--- 
  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 510 
  -- 1 file changed, 352 insertions(+), 158 
  deletions(-) 

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 
6d9e2f21c9cc..5b78ff925af0 100644 --- 
a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ 
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -31,6 +31,7 
@@ 
  #include   #define HWVER_131 
  0x31333100	/* IP version 1.31 */ 
+#define HWVER_130			0x31333000	/* IP 
version 1.30 */ 
   #define DSI_VERSION			0x00 #define 
  VERSIONGENMASK(31, 8) 
@@ -47,7 +48,6 @@ 
  #define DPI_VCID(vcid)			((vcid) & 0x3) 
  #define DSI_DPI_COLOR_CODING		0x10 
-#define LOOSELY18_EN			BIT(8) 
  #define DPI_COLOR_CODING_16BIT_1	0x0 #define 
  DPI_COLOR_CODING_16BIT_2	0x1 #define 
  DPI_COLOR_CODING_16BIT_3	0x2 
@@ -56,11 +56,6 @@ 
  #define DPI_COLOR_CODING_24BIT		0x5  #define 
  DSI_DPI_CFG_POL			0x14 
-#define COLORM_ACTIVE_LOW		BIT(4) -#define 
SHUTD_ACTIVE_LOW		BIT(3) -#define HSYNC_ACTIVE_LOW 
BIT(2) -#define VSYNC_ACTIVE_LOW		BIT(1) -#define 
DATAEN_ACTIVE_LOW		BIT(0) 
   #define DSI_DPI_LP_CMD_TIM		0x18 #define 
  OUTVACT_LPCMD_TIME(p)		(((p) & 0xff) << 16) 
@@ -81,27 +76,19 @@ 
  #define DSI_GEN_VCID			0x30  #define 
  DSI_MODE_CFG			0x34 
-#define ENABLE_VIDEO_MODE		0 -#define ENABLE_CMD_MODE 
BIT(0) 
   #define DSI_VID_MODE_CFG		0x38 
-#define ENABLE_LOW_POWER		(0x3f << 8) -#define 
ENABLE_LOW_POWER_MASK		(0x3f << 8) +#define 
ENABLE_LOW_POWER		0x3f + 
  #define VID_MODE_TYPE_NON_BURST_SYNC_PULSES	0x0 
  #define VID_MODE_TYPE_NON_BURST_SYNC_EVENTS	0x1 
  #define VID_MODE_TYPE_BURST			0x2 
-#define VID_MODE_TYPE_MASK			0x3 -#define 
VID_MODE_VPG_ENABLE		BIT(16) -#define 
VID_MODE_VPG_HORIZONTAL		BIT(24) 
   #define DSI_VID_PKT_SIZE		0x3c 
-#define VID_PKT_SIZE(p)			((p) & 0x3fff) 
   #define DSI_VID_NUM_CHUNKS		0x40 
-#define VID_NUM_CHUNKS(c)		((c) & 0x1fff) 
   #define DSI_VID_NULL_SIZE		0x44 
-#define VID_NULL_SIZE(b)		((b) & 0x1fff) 
   #define DSI_VID_HSA_TIME		0x48 #define 
  DSI_VID_HBP_TIME		0x4c 
@@ -125,7 +112,6 @@ 
  #define GEN_SW_2P_TX_LP			BIT(10) #define 
  GEN_SW_1P_TX_LP			BIT(9) #define 
  GEN_SW_0P_TX_LP			BIT(8) 
-#define ACK_RQST_EN			BIT(1) 
  #define TEAR_FX_EN			BIT(0)  #define 
  CMD_MODE_ALL_LP			(MAX_RD_PKT_SIZE_LP | \ 
@@ -154,8 +140,6 @@ 
  #define GEN_CMD_EMPTY			BIT(0)  #define 
  DSI_TO_CNT_CFG			0x78 
-#define HSTX_TO_CNT(p)			(((p) & 0x) << 
16) -#define LPRX_TO_CNT(p)			((p) & 0x) 
   #define DSI_HS_RD_TO_CNT		0x7c #define 
  DSI_LP_RD_TO_CNT		0x80 
@@ -164,52 +148,17 @@ 
  #define DSI_BTA_TO_CNT			0x8c  #define 
  DSI_LPCLK_CTRL			0x94 
-#define AUTO_CLKLANE_CTRL		BIT(1) -#define 
PHY_TXREQUESTCLKHS		BIT(0) - 
  #define DSI_PHY_TMR_LPCLK_CFG		0x98 
-#define PHY_CLKHS2LP_TIME(lbcc)		(((lbcc) & 0x3ff) 
<< 16) -#define PHY_CLKLP2HS_TIME(lbcc)		((lbcc) & 
0x3ff) - 
  #define DSI_PHY_TMR_CFG			0x9c 
-#define PHY_HS2LP_TIME(lbcc)		(((lbcc) & 0xff) 
<< 24) -#define PHY_LP2HS_TIME(lbcc)		(((lbcc) & 0xff) 
<< 16) -#define MAX_RD_TIME(lbcc)		

Re: [PATCH v5 2/5] drm: bridge: dw_mipi_dsi: abstract register access using reg_fields

2020-04-06 Thread Andrzej Hajda

W dniu 30.03.2020 o 13:35, Adrian Ratiu pisze:
> Register existence, address/offsets, field layouts, reserved bits and
> so on differ between MIPI-DSI versions and between SoC vendor boards.
> Despite these differences the hw IP and protocols are mostly the same
> so the generic bridge can be made to compensate these differences.
>
> The current Rockchip and STM drivers hardcoded a lot of their common
> definitions in the bridge code because they're based on DSI v1.30 and
> 1.31 which are relatively close, but in order to support older/future
> versions with more diverging layouts like the v1.01 present on imx6,
> we abstract some of the register accesses via the regmap field APIs.
>
> The bridge detects the DSI core version and initializes the required
> regmap register layout. Other DSI versions / register layouts can
> easily be added in the future by only changing the bridge code.
>
> The platform drivers don't require any changes, DSI register layout
> versioning will be handled transparently by the bridge, but if in
> the future the regmap or layouts needs to be exposed to the drivres,
> it could easily be done via plat_data or a new API in dw_mipi_dsi.h.
>
> Suggested-by: Boris Brezillon 
> Reviewed-by: Emil Velikov 
> Signed-off-by: Adrian Ratiu 
> ---
> Changes since v4:
>- Move regmap infrastructure logic to a separate commit (Ezequiel)
>- Consolidate field infrastructure in this commit (Ezequiel)
>- Move the dsi v1.01 layout logic to a separate commit (Ezequiel)
>
> Changes since v2:
>- Added const declarations to dw_mipi_dsi structs (Emil)
>- Fixed commit tags (Emil)
>
> Changes since v1:
>- Moved register definitions & regmap initialization into bridge
>module. Platform drivers get the regmap via plat_data after calling
>the bridge probe (Emil).
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 510 --
>   1 file changed, 352 insertions(+), 158 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 6d9e2f21c9cc..5b78ff925af0 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -31,6 +31,7 @@
>   #include 
>   
>   #define HWVER_131   0x31333100  /* IP version 1.31 */
> +#define HWVER_1300x31333000  /* IP version 1.30 */
>   
>   #define DSI_VERSION 0x00
>   #define VERSION GENMASK(31, 8)
> @@ -47,7 +48,6 @@
>   #define DPI_VCID(vcid)  ((vcid) & 0x3)
>   
>   #define DSI_DPI_COLOR_CODING0x10
> -#define LOOSELY18_EN BIT(8)
>   #define DPI_COLOR_CODING_16BIT_10x0
>   #define DPI_COLOR_CODING_16BIT_20x1
>   #define DPI_COLOR_CODING_16BIT_30x2
> @@ -56,11 +56,6 @@
>   #define DPI_COLOR_CODING_24BIT  0x5
>   
>   #define DSI_DPI_CFG_POL 0x14
> -#define COLORM_ACTIVE_LOWBIT(4)
> -#define SHUTD_ACTIVE_LOW BIT(3)
> -#define HSYNC_ACTIVE_LOW BIT(2)
> -#define VSYNC_ACTIVE_LOW BIT(1)
> -#define DATAEN_ACTIVE_LOWBIT(0)
>   
>   #define DSI_DPI_LP_CMD_TIM  0x18
>   #define OUTVACT_LPCMD_TIME(p)   (((p) & 0xff) << 16)
> @@ -81,27 +76,19 @@
>   #define DSI_GEN_VCID0x30
>   
>   #define DSI_MODE_CFG0x34
> -#define ENABLE_VIDEO_MODE0
> -#define ENABLE_CMD_MODE  BIT(0)
>   
>   #define DSI_VID_MODE_CFG0x38
> -#define ENABLE_LOW_POWER (0x3f << 8)
> -#define ENABLE_LOW_POWER_MASK(0x3f << 8)
> +#define ENABLE_LOW_POWER 0x3f
> +
>   #define VID_MODE_TYPE_NON_BURST_SYNC_PULSES 0x0
>   #define VID_MODE_TYPE_NON_BURST_SYNC_EVENTS 0x1
>   #define VID_MODE_TYPE_BURST 0x2
> -#define VID_MODE_TYPE_MASK   0x3
> -#define VID_MODE_VPG_ENABLE  BIT(16)
> -#define VID_MODE_VPG_HORIZONTAL  BIT(24)
>   
>   #define DSI_VID_PKT_SIZE0x3c
> -#define VID_PKT_SIZE(p)  ((p) & 0x3fff)
>   
>   #define DSI_VID_NUM_CHUNKS  0x40
> -#define VID_NUM_CHUNKS(c)((c) & 0x1fff)
>   
>   #define DSI_VID_NULL_SIZE   0x44
> -#define VID_NULL_SIZE(b) ((b) & 0x1fff)
>   
>   #define DSI_VID_HSA_TIME0x48
>   #define DSI_VID_HBP_TIME0x4c
> @@ -125,7 +112,6 @@
>   #define GEN_SW_2P_TX_LP BIT(10)
>   #define GEN_SW_1P_TX_LP BIT(9)
>   #define GEN_SW_0P_TX_LP BIT(8)
> -#define ACK_RQST_EN  BIT(1)
>   #define TEAR_FX_EN  BIT(0)
>   
>   #define CMD_MODE_ALL_LP (MAX_RD_PKT_SIZE_LP | \
> @@ -154,8 +140,6 @@
>   #define GEN_CMD_EMPTY   BIT(0)
>   
>   #define DSI_TO_CNT_CFG