[PATCH v1 0/2] drm/bridge/synopsys: dsi: Various cleanups

[PATCH v1 0/2] drm/bridge/synopsys: dsi: Various cleanups

[Philippe CORNU]

Version 1:
- Initial commit

The purpose of this set of patches is to clean up the mipi dsi dw Synopsys
drm bridge.

Philippe CORNU (2):
  drm/bridge/synopsys: dsi: Constify funcs structures
  drm/bridge/synopsys: dsi: Register list clean up

 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 79 ++++++++++++---------------
 1 file changed, 34 insertions(+), 45 deletions(-)

-- 
1.9.1

[PATCH v1 1/2] drm/bridge/synopsys: dsi: Constify funcs structures

[Philippe CORNU]

Signed-off-by: Philippe CORNU <philippe.cornu@st.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 36f5ccb..63c7a01 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -811,7 +811,7 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
 	return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge);
 }
 
-static struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = {
+static const struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = {
 	.mode_set     = dw_mipi_dsi_bridge_mode_set,
 	.enable	      = dw_mipi_dsi_bridge_enable,
 	.post_disable = dw_mipi_dsi_bridge_post_disable,
-- 
1.9.1

Re: [PATCH v1 1/2] drm/bridge/synopsys: dsi: Constify funcs structures

[Laurent Pinchart]

Hi Philippe,

Thank you for the patch.

On Tuesday 18 Jul 2017 13:43:51 Philippe CORNU wrote:
> Signed-off-by: Philippe CORNU <philippe.cornu@st.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 36f5ccb..63c7a01
> 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -811,7 +811,7 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge
> *bridge) return drm_bridge_attach(bridge->encoder, dsi->panel_bridge,
> bridge); }
> 
> -static struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = {
> +static const struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = {
>  	.mode_set     = dw_mipi_dsi_bridge_mode_set,
>  	.enable	      = dw_mipi_dsi_bridge_enable,
>  	.post_disable = dw_mipi_dsi_bridge_post_disable,

-- 
Regards,

Laurent Pinchart

[PATCH v1 2/2] drm/bridge/synopsys: dsi: Register list clean up

[Philippe CORNU]

This patch cleans up the Synopsys mipi dsi register list:
- remove unused registers
- rename registers according to the Synopsys documentation
  (1.30 & 1.31)
- fix typos
- re-order registers for a better coherency

Signed-off-by: Philippe CORNU <philippe.cornu@st.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 77 ++++++++++++---------------
 1 file changed, 33 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 63c7a01..c777ee3 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -35,14 +35,14 @@
 #define POWERUP				BIT(0)
 
 #define DSI_CLKMGR_CFG			0x08
-#define TO_CLK_DIVIDSION(div)		(((div) & 0xff) << 8)
-#define TX_ESC_CLK_DIVIDSION(div)	(((div) & 0xff) << 0)
+#define TO_CLK_DIVISION(div)		(((div) & 0xff) << 8)
+#define TX_ESC_CLK_DIVISION(div)	((div) & 0xff)
 
 #define DSI_DPI_VCID			0x0c
-#define DPI_VID(vid)			(((vid) & 0x3) << 0)
+#define DPI_VCID(vid)			((vid) & 0x3)
 
 #define DSI_DPI_COLOR_CODING		0x10
-#define EN18_LOOSELY			BIT(8)
+#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
@@ -61,22 +61,18 @@
 #define OUTVACT_LPCMD_TIME(p)		(((p) & 0xff) << 16)
 #define INVACT_LPCMD_TIME(p)		((p) & 0xff)
 
-#define DSI_DBI_CFG			0x20
-#define DSI_DBI_CMDSIZE			0x28
-
 #define DSI_PCKHDL_CFG			0x2c
-#define EN_CRC_RX			BIT(4)
-#define EN_ECC_RX			BIT(3)
-#define EN_BTA				BIT(2)
-#define EN_EOTP_RX			BIT(1)
-#define EN_EOTP_TX			BIT(0)
+#define CRC_RX_EN			BIT(4)
+#define ECC_RX_EN			BIT(3)
+#define BTA_EN				BIT(2)
+#define EOTP_RX_EN			BIT(1)
+#define EOTP_TX_EN			BIT(0)
 
 #define DSI_MODE_CFG			0x34
 #define ENABLE_VIDEO_MODE		0
 #define ENABLE_CMD_MODE			BIT(0)
 
 #define DSI_VID_MODE_CFG		0x38
-#define FRAME_BTA_ACK			BIT(14)
 #define ENABLE_LOW_POWER		(0x3f << 8)
 #define ENABLE_LOW_POWER_MASK		(0x3f << 8)
 #define VID_MODE_TYPE_NON_BURST_SYNC_PULSES	0x0
@@ -85,8 +81,7 @@
 #define VID_MODE_TYPE_MASK			0x3
 
 #define DSI_VID_PKT_SIZE		0x3c
-#define VID_PKT_SIZE(p)			(((p) & 0x3fff) << 0)
-#define VID_PKT_MAX_SIZE		0x3fff
+#define VID_PKT_SIZE(p)			((p) & 0x3fff)
 
 #define DSI_VID_HSA_TIME		0x48
 #define DSI_VID_HBP_TIME		0x4c
@@ -108,8 +103,8 @@
 #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 EN_ACK_RQST			BIT(1)
-#define EN_TEAR_FX			BIT(0)
+#define ACK_RQST_EN			BIT(1)
+#define TEAR_FX_EN			BIT(0)
 
 #define CMD_MODE_ALL_LP			(MAX_RD_PKT_SIZE_LP | \
 					 DCS_LW_TX_LP | \
@@ -125,21 +120,20 @@
 					 GEN_SW_0P_TX_LP)
 
 #define DSI_GEN_HDR			0x6c
+/* TODO These 2 defines will be reworked thanks to mipi_dsi_create_packet() */
 #define GEN_HDATA(data)			(((data) & 0xffff) << 8)
-#define GEN_HDATA_MASK			(0xffff << 8)
 #define GEN_HTYPE(type)			(((type) & 0xff) << 0)
-#define GEN_HTYPE_MASK			0xff
 
 #define DSI_GEN_PLD_DATA		0x70
 
 #define DSI_CMD_PKT_STATUS		0x74
-#define GEN_CMD_EMPTY			BIT(0)
-#define GEN_CMD_FULL			BIT(1)
-#define GEN_PLD_W_EMPTY			BIT(2)
-#define GEN_PLD_W_FULL			BIT(3)
-#define GEN_PLD_R_EMPTY			BIT(4)
-#define GEN_PLD_R_FULL			BIT(5)
 #define GEN_RD_CMD_BUSY			BIT(6)
+#define GEN_PLD_R_FULL			BIT(5)
+#define GEN_PLD_R_EMPTY			BIT(4)
+#define GEN_PLD_W_FULL			BIT(3)
+#define GEN_PLD_W_EMPTY			BIT(2)
+#define GEN_CMD_FULL			BIT(1)
+#define GEN_CMD_EMPTY			BIT(0)
 
 #define DSI_TO_CNT_CFG			0x78
 #define HSTX_TO_CNT(p)			(((p) & 0xffff) << 16)
@@ -154,6 +148,7 @@
 #define PHY_CLKHS2LP_TIME(lbcc)		(((lbcc) & 0x3ff) << 16)
 #define PHY_CLKLP2HS_TIME(lbcc)		((lbcc) & 0x3ff)
 
+/* TODO Next register is slightly different between 1.30 & 1.31 IP version */
 #define DSI_PHY_TMR_CFG			0x9c
 #define PHY_HS2LP_TIME(lbcc)		(((lbcc) & 0xff) << 24)
 #define PHY_LP2HS_TIME(lbcc)		(((lbcc) & 0xff) << 16)
@@ -170,12 +165,12 @@
 #define PHY_UNSHUTDOWNZ			BIT(0)
 
 #define DSI_PHY_IF_CFG			0xa4
-#define N_LANES(n)			((((n) - 1) & 0x3) << 0)
 #define PHY_STOP_WAIT_TIME(cycle)	(((cycle) & 0xff) << 8)
+#define N_LANES(n)			(((n) - 1) & 0x3)
 
 #define DSI_PHY_STATUS			0xb0
-#define LOCK				BIT(0)
-#define STOP_STATE_CLK_LANE		BIT(2)
+#define PHY_STOP_STATE_CLK_LANE		BIT(2)
+#define PHY_LOCK			BIT(0)
 
 #define DSI_PHY_TST_CTRL0		0xb4
 #define PHY_TESTCLK			BIT(1)
@@ -183,12 +178,6 @@
 #define PHY_TESTCLR			BIT(0)
 #define PHY_UNTESTCLR			0
 
-#define DSI_PHY_TST_CTRL1		0xb8
-#define PHY_TESTEN			BIT(16)
-#define PHY_UNTESTEN			0
-#define PHY_TESTDOUT(n)			(((n) & 0xff) << 8)
-#define PHY_TESTDIN(n)			(((n) & 0xff) << 0)
-
 #define DSI_INT_ST0			0xbc
 #define DSI_INT_ST1			0xc0
 #define DSI_INT_MSK0			0xc4
@@ -307,7 +296,7 @@ static void dw_mipi_message_config(struct dw_mipi_dsi *dsi,
 	u32 val = 0;
 
 	if (msg->flags & MIPI_DSI_MSG_REQ_ACK)
-		val |= EN_ACK_RQST;
+		val |= ACK_RQST_EN;
 	if (lpm)
 		val |= CMD_MODE_ALL_LP;
 
@@ -506,8 +495,8 @@ static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
 	 * timeout clock division should be computed with the
 	 * high speed transmission counter timeout and byte lane...
 	 */
-	dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVIDSION(10) |
-		  TX_ESC_CLK_DIVIDSION(esc_clk_division));
+	dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) |
+		  TX_ESC_CLK_DIVISION(esc_clk_division));
 }
 
 static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,
@@ -520,7 +509,7 @@ static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,
 		color = DPI_COLOR_CODING_24BIT;
 		break;
 	case MIPI_DSI_FMT_RGB666:
-		color = DPI_COLOR_CODING_18BIT_2 | EN18_LOOSELY;
+		color = DPI_COLOR_CODING_18BIT_2 | LOOSELY18_EN;
 		break;
 	case MIPI_DSI_FMT_RGB666_PACKED:
 		color = DPI_COLOR_CODING_18BIT_1;
@@ -535,7 +524,7 @@ static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,
 	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
 		val |= HSYNC_ACTIVE_LOW;
 
-	dsi_write(dsi, DSI_DPI_VCID, DPI_VID(dsi->channel));
+	dsi_write(dsi, DSI_DPI_VCID, DPI_VCID(dsi->channel));
 	dsi_write(dsi, DSI_DPI_COLOR_CODING, color);
 	dsi_write(dsi, DSI_DPI_CFG_POL, val);
 	/*
@@ -550,7 +539,7 @@ static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,
 
 static void dw_mipi_dsi_packet_handler_config(struct dw_mipi_dsi *dsi)
 {
-	dsi_write(dsi, DSI_PCKHDL_CFG, EN_CRC_RX | EN_ECC_RX | EN_BTA);
+	dsi_write(dsi, DSI_PCKHDL_CFG, CRC_RX_EN | ECC_RX_EN | BTA_EN);
 }
 
 static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi,
@@ -571,7 +560,7 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
 	/*
 	 * TODO dw drv improvements
 	 * compute high speed transmission counter timeout according
-	 * to the timeout clock division (TO_CLK_DIVIDSION) and byte lane...
+	 * to the timeout clock division (TO_CLK_DIVISION) and byte lane...
 	 */
 	dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(1000) | LPRX_TO_CNT(1000));
 	/*
@@ -684,13 +673,13 @@ static void dw_mipi_dsi_dphy_enable(struct dw_mipi_dsi *dsi)
 	dsi_write(dsi, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK |
 		  PHY_UNRSTZ | PHY_UNSHUTDOWNZ);
 
-	ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS,
-				 val, val & LOCK, 1000, PHY_STATUS_TIMEOUT_US);
+	ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS, val,
+				 val & PHY_LOCK, 1000, PHY_STATUS_TIMEOUT_US);
 	if (ret < 0)
 		DRM_DEBUG_DRIVER("failed to wait phy lock state\n");
 
 	ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS,
-				 val, val & STOP_STATE_CLK_LANE, 1000,
+				 val, val & PHY_STOP_STATE_CLK_LANE, 1000,
 				 PHY_STATUS_TIMEOUT_US);
 	if (ret < 0)
 		DRM_DEBUG_DRIVER("failed to wait phy clk lane stop state\n");
-- 
1.9.1

Re: [PATCH v1 2/2] drm/bridge/synopsys: dsi: Register list clean up

[Laurent Pinchart]

Hi Philippe,

Thank you for the patch.

On Tuesday 18 Jul 2017 13:43:52 Philippe CORNU wrote:
> This patch cleans up the Synopsys mipi dsi register list:
> - remove unused registers

Is the documentation for the DSI transmitter core public ? If not, it could be 
useful to keep unused registers for reference. That's nine lines only.

> - rename registers according to the Synopsys documentation
>   (1.30 & 1.31)
> - fix typos
> - re-order registers for a better coherency

For the rest,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Philippe CORNU <philippe.cornu@st.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 77 +++++++++---------------
>  1 file changed, 33 insertions(+), 44 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 2/2] drm/bridge/synopsys: dsi: Register list clean up

[Philippe CORNU]

On 07/18/2017 03:39 PM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Tuesday 18 Jul 2017 13:43:52 Philippe CORNU wrote:
>> This patch cleans up the Synopsys mipi dsi register list:
>> - remove unused registers
> 
> Is the documentation for the DSI transmitter core public ? If not, it could be
> useful to keep unused registers for reference. That's nine lines only.
> 

Hi Laurent,
And many thanks for the code review.

Unfortunately the Synopsys Documentation seems not public (I may google 
more), so I will put back these registers :)

Nevertheless, the stm32 documentation is public and it is probably the 
same for rockchip (and hisilicon and...), and in these public 
documentations, register and bitfield names are slightly different but 
register addresses and bitfield descriptions are the same...
I do not know if it makes sense or not but we may ask Synopsys to 
publicly release the mipi dsi documentation...

Did you manage to get it "publicly" for the Synopsys hdmi?

>> - rename registers according to the Synopsys documentation
>>    (1.30 & 1.31)
>> - fix typos
>> - re-order registers for a better coherency
> 
> For the rest,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 

Thank you
Philippe

>> Signed-off-by: Philippe CORNU <philippe.cornu@st.com>
>> ---
>>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 77 +++++++++---------------
>>   1 file changed, 33 insertions(+), 44 deletions(-)
> 

Re: [PATCH v1 2/2] drm/bridge/synopsys: dsi: Register list clean up

[Laurent Pinchart]

Hi Philippe,

On Wednesday 19 Jul 2017 09:11:44 Philippe CORNU wrote:
> On 07/18/2017 03:39 PM, Laurent Pinchart wrote:
> > On Tuesday 18 Jul 2017 13:43:52 Philippe CORNU wrote:
> > 
> >> This patch cleans up the Synopsys mipi dsi register list:
> >> - remove unused registers
> > 
> > Is the documentation for the DSI transmitter core public ? If not, it
> > could be useful to keep unused registers for reference. That's nine
> > lines only. 
> 
> Hi Laurent,
> And many thanks for the code review.
> 
> Unfortunately the Synopsys Documentation seems not public (I may google 
> more), so I will put back these registers :)
> 
> Nevertheless, the stm32 documentation is public and it is probably the 
> same for rockchip (and hisilicon and...), and in these public 
> documentations, register and bitfield names are slightly different but 
> register addresses and bitfield descriptions are the same...

That's good to know, thanks.

> I do not know if it makes sense or not but we may ask Synopsys to 
> publicly release the mipi dsi documentation...

I'd love that. There's little chance it will happen, but it certainly won't if 
we don't ask.

> Did you manage to get it "publicly" for the Synopsys hdmi?

Unfortunately not :-( I don't even have access to Synopsys documentation under 
NDA.

-- 
Regards,

Laurent Pinchart