Re: [PATCH v9 09/11] drm: bridge: dw-mipi-dsi: split low power cfg register into fields

From: Yannick FERTRE <yannick.fertre-qxv4g6HH51o@public.gmane.org>
To: Adrian Ratiu
	<adrian.ratiu-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>,
	"linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org"
	<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
	"devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org"
	<linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
	Laurent Pinchart
	<Laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Cc: Jernej Skrabec <jernej.skrabec-gGgVlfcn5nU@public.gmane.org>,
	Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>,
	Adrian Pop <pop.adrian61-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	Jonas Karlman <jonas-uIzNG4q0ceqzQB+pC5nmwQ@public.gmane.org>,
	Philippe CORNU <philippe.cornu-qxv4g6HH51o@public.gmane.org>,
	"dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org"
	<dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>,
	"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>,
	"linux-imx-3arQi8VN3Tc@public.gmane.org"
	<linux-imx-3arQi8VN3Tc@public.gmane.org>,
	"kernel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org"
	<kernel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>,
	"linux-stm32-XDFAJ8BFU24N7RejjzZ/Li2xQDfSxrLKVpNB7YpNyf8@public.gmane.org"
	<linux-stm32-XDFAJ8BFU24N7RejjzZ/Li2xQDfSxrLKVpNB7YpNyf8@public.gmane.org>,
	Arnaud Ferraris
	<arnaud.ferraris-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
Subject: Re: [PATCH v9 09/11] drm: bridge: dw-mipi-dsi: split low power cfg register into fields
Date: Wed, 10 Jun 2020 12:21:55 +0000	[thread overview]
Message-ID: <81f04df5-218c-7f1e-98eb-9dad9cdcfa48@st.com> (raw)
In-Reply-To: <20200609174959.955926-10-adrian.ratiu-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>

Hi Adrian,

thanks for the pach: tested on stm32mp1.

Tested-by: Yannick Fertré <yannick.fertre@st.com>

On 6/9/20 7:49 PM, Adrian Ratiu wrote:
> According to the Host Registers documentation for IMX, STM and RK
> the LP cfg register should not be written entirely in one go because
> some bits are reserved and should be kept to reset values, for eg.
> BIT(15) which is reserved in all versions.
>
> This also cleans up the code by removing the the ugly defines
> and making field ranges & values written more explicit.
>
> Tested-by: Adrian Pop <pop.adrian61@gmail.com>
> Tested-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
> New in v6.
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 105 ++++++------------
>   1 file changed, 33 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 70df0578cbe7b..1e47d40b5becb 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -120,60 +120,6 @@
>   #define DSI_TO_CNT_CFG_V101		0x40
>   #define DSI_PCKHDL_CFG_V101		0x18
>   
> -#define MAX_RD_PKT_SIZE_LP		BIT(24)
> -#define DCS_LW_TX_LP			BIT(19)
> -#define DCS_SR_0P_TX_LP			BIT(18)
> -#define DCS_SW_1P_TX_LP			BIT(17)
> -#define DCS_SW_0P_TX_LP			BIT(16)
> -#define GEN_LW_TX_LP			BIT(14)
> -#define GEN_SR_2P_TX_LP			BIT(13)
> -#define GEN_SR_1P_TX_LP			BIT(12)
> -#define GEN_SR_0P_TX_LP			BIT(11)
> -#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 TEAR_FX_EN			BIT(0)
> -
> -#define CMD_MODE_ALL_LP			(MAX_RD_PKT_SIZE_LP | \
> -					 DCS_LW_TX_LP | \
> -					 DCS_SR_0P_TX_LP | \
> -					 DCS_SW_1P_TX_LP | \
> -					 DCS_SW_0P_TX_LP | \
> -					 GEN_LW_TX_LP | \
> -					 GEN_SR_2P_TX_LP | \
> -					 GEN_SR_1P_TX_LP | \
> -					 GEN_SR_0P_TX_LP | \
> -					 GEN_SW_2P_TX_LP | \
> -					 GEN_SW_1P_TX_LP | \
> -					 GEN_SW_0P_TX_LP)
> -
> -#define EN_TEAR_FX_V101			BIT(14)
> -#define DCS_LW_TX_LP_V101		BIT(12)
> -#define GEN_LW_TX_LP_V101		BIT(11)
> -#define MAX_RD_PKT_SIZE_LP_V101		BIT(10)
> -#define DCS_SW_2P_TX_LP_V101		BIT(9)
> -#define DCS_SW_1P_TX_LP_V101		BIT(8)
> -#define DCS_SW_0P_TX_LP_V101		BIT(7)
> -#define GEN_SR_2P_TX_LP_V101		BIT(6)
> -#define GEN_SR_1P_TX_LP_V101		BIT(5)
> -#define GEN_SR_0P_TX_LP_V101		BIT(4)
> -#define GEN_SW_2P_TX_LP_V101		BIT(3)
> -#define GEN_SW_1P_TX_LP_V101		BIT(2)
> -#define GEN_SW_0P_TX_LP_V101		BIT(1)
> -
> -#define CMD_MODE_ALL_LP_V101		(DCS_LW_TX_LP_V101 | \
> -					 GEN_LW_TX_LP_V101 | \
> -					 MAX_RD_PKT_SIZE_LP_V101 | \
> -					 DCS_SW_2P_TX_LP_V101 | \
> -					 DCS_SW_1P_TX_LP_V101 | \
> -					 DCS_SW_0P_TX_LP_V101 | \
> -					 GEN_SR_2P_TX_LP_V101 | \
> -					 GEN_SR_1P_TX_LP_V101 | \
> -					 GEN_SR_0P_TX_LP_V101 | \
> -					 GEN_SW_2P_TX_LP_V101 | \
> -					 GEN_SW_1P_TX_LP_V101 | \
> -					 GEN_SW_0P_TX_LP_V101)
> -
>   #define DSI_GEN_HDR			0x6c
>   #define DSI_GEN_PLD_DATA		0x70
>   
> @@ -257,7 +203,11 @@ struct dw_mipi_dsi {
>   	struct regmap_field	*field_dpi_vsync_active_low;
>   	struct regmap_field	*field_dpi_hsync_active_low;
>   	struct regmap_field	*field_cmd_mode_ack_rqst_en;
> -	struct regmap_field	*field_cmd_mode_all_lp_en;
> +	struct regmap_field	*field_cmd_mode_gen_sw_sr_en;
> +	struct regmap_field	*field_cmd_mode_dcs_sw_sr_en;
> +	struct regmap_field	*field_cmd_mode_gen_lw_en;
> +	struct regmap_field	*field_cmd_mode_dcs_lw_en;
> +	struct regmap_field	*field_cmd_mode_max_rd_pkt_size;
>   	struct regmap_field	*field_cmd_mode_en;
>   	struct regmap_field	*field_cmd_pkt_status;
>   	struct regmap_field	*field_vid_mode_en;
> @@ -315,7 +265,11 @@ struct dw_mipi_dsi_variant {
>   	struct reg_field	cfg_dpi_hsync_active_low;
>   	struct reg_field	cfg_cmd_mode_en;
>   	struct reg_field	cfg_cmd_mode_ack_rqst_en;
> -	struct reg_field	cfg_cmd_mode_all_lp_en;
> +	struct reg_field	cfg_cmd_mode_gen_sw_sr_en;
> +	struct reg_field	cfg_cmd_mode_dcs_sw_sr_en;
> +	struct reg_field	cfg_cmd_mode_gen_lw_en;
> +	struct reg_field	cfg_cmd_mode_dcs_lw_en;
> +	struct reg_field	cfg_cmd_mode_max_rd_pkt_size;
>   	struct reg_field	cfg_cmd_pkt_status;
>   	struct reg_field	cfg_vid_mode_en;
>   	struct reg_field	cfg_vid_mode_type;
> @@ -366,7 +320,11 @@ static const struct dw_mipi_dsi_variant dw_mipi_dsi_v130_v131_layout = {
>   	.cfg_dpi_vsync_active_low =	REG_FIELD(DSI_DPI_CFG_POL, 1, 1),
>   	.cfg_dpi_hsync_active_low =	REG_FIELD(DSI_DPI_CFG_POL, 2, 2),
>   	.cfg_cmd_mode_ack_rqst_en =	REG_FIELD(DSI_CMD_MODE_CFG, 1, 1),
> -	.cfg_cmd_mode_all_lp_en =	REG_FIELD(DSI_CMD_MODE_CFG, 8, 24),
> +	.cfg_cmd_mode_gen_sw_sr_en =	REG_FIELD(DSI_CMD_MODE_CFG, 8, 13),
> +	.cfg_cmd_mode_gen_lw_en =	REG_FIELD(DSI_CMD_MODE_CFG, 14, 14),
> +	.cfg_cmd_mode_dcs_sw_sr_en =	REG_FIELD(DSI_CMD_MODE_CFG, 16, 18),
> +	.cfg_cmd_mode_dcs_lw_en =	REG_FIELD(DSI_CMD_MODE_CFG, 19, 19),
> +	.cfg_cmd_mode_max_rd_pkt_size =	REG_FIELD(DSI_CMD_MODE_CFG, 24, 24),
>   	.cfg_cmd_mode_en =		REG_FIELD(DSI_MODE_CFG, 0, 31),
>   	.cfg_cmd_pkt_status =		REG_FIELD(DSI_CMD_PKT_STATUS, 0, 31),
>   	.cfg_vid_mode_en =		REG_FIELD(DSI_MODE_CFG, 0, 31),
> @@ -418,7 +376,11 @@ static const struct dw_mipi_dsi_variant dw_mipi_dsi_v101_layout = {
>   	.cfg_dpi_vsync_active_low =	REG_FIELD(DSI_DPI_CFG, 6, 6),
>   	.cfg_dpi_hsync_active_low =	REG_FIELD(DSI_DPI_CFG, 7, 7),
>   	.cfg_cmd_mode_en =		REG_FIELD(DSI_CMD_MODE_CFG_V101, 0, 0),
> -	.cfg_cmd_mode_all_lp_en =	REG_FIELD(DSI_CMD_MODE_CFG_V101, 1, 12),
> +	.cfg_cmd_mode_gen_sw_sr_en =	REG_FIELD(DSI_CMD_MODE_CFG, 1, 6),
> +	.cfg_cmd_mode_dcs_sw_sr_en =	REG_FIELD(DSI_CMD_MODE_CFG, 7, 9),
> +	.cfg_cmd_mode_max_rd_pkt_size =	REG_FIELD(DSI_CMD_MODE_CFG, 10, 10),
> +	.cfg_cmd_mode_gen_lw_en =	REG_FIELD(DSI_CMD_MODE_CFG, 11, 11),
> +	.cfg_cmd_mode_dcs_lw_en =	REG_FIELD(DSI_CMD_MODE_CFG, 12, 12),
>   	.cfg_cmd_mode_ack_rqst_en =	REG_FIELD(DSI_CMD_MODE_CFG_V101, 13, 13),
>   	.cfg_cmd_pkt_status =		REG_FIELD(DSI_CMD_PKT_STATUS_V101, 0, 14),
>   	.cfg_vid_mode_en =		REG_FIELD(DSI_VID_MODE_CFG_V101, 0, 0),
> @@ -554,23 +516,18 @@ static void dw_mipi_message_config(struct dw_mipi_dsi *dsi,
>   				   const struct mipi_dsi_msg *msg)
>   {
>   	bool lpm = msg->flags & MIPI_DSI_MSG_USE_LPM;
> -	u32 cmd_mode_lp = 0;
> -
> -	switch (dsi->hw_version) {
> -	case HWVER_130:
> -	case HWVER_131:
> -		cmd_mode_lp = CMD_MODE_ALL_LP;
> -		break;
> -	case HWVER_101:
> -		cmd_mode_lp = CMD_MODE_ALL_LP_V101;
> -		break;
> -	}
>   
>   	if (msg->flags & MIPI_DSI_MSG_REQ_ACK)
>   		regmap_field_write(dsi->field_cmd_mode_ack_rqst_en, 1);
>   
> -	if (lpm)
> -		regmap_field_write(dsi->field_cmd_mode_all_lp_en, cmd_mode_lp);
> +	if (lpm) {
> +		regmap_field_write(dsi->field_cmd_mode_gen_sw_sr_en,
> +				   ENABLE_LOW_POWER);
> +		regmap_field_write(dsi->field_cmd_mode_dcs_sw_sr_en, 7);
> +		regmap_field_write(dsi->field_cmd_mode_gen_lw_en, 1);
> +		regmap_field_write(dsi->field_cmd_mode_dcs_lw_en, 1);
> +		regmap_field_write(dsi->field_cmd_mode_max_rd_pkt_size, 1);
> +	}
>   
>   	regmap_field_write(dsi->field_phy_txrequestclkhs, lpm ? 0 : 1);
>   }
> @@ -1256,7 +1213,11 @@ static int dw_mipi_dsi_regmap_fields_init(struct dw_mipi_dsi *dsi)
>   	INIT_FIELD(dpi_vsync_active_low);
>   	INIT_FIELD(dpi_hsync_active_low);
>   	INIT_FIELD(cmd_mode_ack_rqst_en);
> -	INIT_FIELD(cmd_mode_all_lp_en);
> +	INIT_FIELD(cmd_mode_gen_sw_sr_en);
> +	INIT_FIELD(cmd_mode_dcs_sw_sr_en);
> +	INIT_FIELD(cmd_mode_gen_lw_en);
> +	INIT_FIELD(cmd_mode_dcs_lw_en);
> +	INIT_FIELD(cmd_mode_max_rd_pkt_size);
>   	INIT_FIELD(cmd_mode_en);
>   	INIT_FIELD(cmd_pkt_status);
>   	INIT_FIELD(vid_mode_en);
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip