[PATCH v1 0/4] imx8mm display controller power sequence

[PATCH v1 0/4] imx8mm display controller power sequence

[Viraj Shah]

This patch queue addresses the power sequence of the display controller
of the imx8mm SoC. The sequence mentioned in example code 5 in section
5.2.9.5 of reference manual imx-8MMini-yhsc.pdf was not being performed.
This meant that the display controller was not coming up.

Viraj Shah (4):
  soc: imx: gpcv2: Power sequence for DISP
  soc: imx: imx8m-blk-ctrl: Display Power ON sequence
  soc: imx: imx8m-blk-ctrl: Add reset bits for mipi dsi phy
  arm64: dts: imx8mm.dtsi: Add resets for dispmix power domain.

 arch/arm64/boot/dts/freescale/imx8mm.dtsi |  1 +
 drivers/soc/imx/gpcv2.c                   | 36 +++++++++++++++++++----
 drivers/soc/imx/imx8m-blk-ctrl.c          |  9 ++++--
 3 files changed, 38 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH v1 1/4] soc: imx8mm: gpcv2: Power sequence for DISP

[Viraj Shah]

As per the imx8mm reference manual, read bit 25(GPC_DISPMIX_
PWRDNACKN) of the power handshake register and wait for ack during
power on/off.

Signed-off-by: Viraj Shah <viraj.shah@linutronix.de>
---
 drivers/soc/imx/gpcv2.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 3cb123016b3e..8ee70c30964f 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -254,11 +254,24 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
 		/*
 		 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
 		 * for PUP_REQ/PDN_REQ bit to be cleared
+		 *
+		 * As per "5.2.9.5 Example Code 5" in i.MX-8MMini-yhsc.pdf
+		 * Display power on section checks for bit 25 of
+		 * Power handshake register to be cleared.
 		 */
-		ret = regmap_read_poll_timeout(domain->regmap,
-					       GPC_PU_PGC_SW_PUP_REQ, reg_val,
-					       !(reg_val & domain->bits.pxx),
-					       0, USEC_PER_MSEC);
+		if (domain->bits.pxx == IMX8MM_DISPMIX_SW_Pxx_REQ) {
+			regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
+				   BIT(7), BIT(7));
+			ret = regmap_read_poll_timeout(domain->regmap,
+						GPC_PU_PWRHSK, reg_val,
+						!(reg_val & IMX8MM_DISPMIX_HSK_PWRDNACKN),
+						0, USEC_PER_MSEC);
+		} else
+			ret = regmap_read_poll_timeout(domain->regmap,
+						GPC_PU_PGC_SW_PUP_REQ, reg_val,
+						!(reg_val & domain->bits.pxx),
+						0, USEC_PER_MSEC);
+
 		if (ret) {
 			dev_err(domain->dev, "failed to command PGC\n");
 			goto out_clk_disable;
@@ -355,11 +368,24 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
 		/*
 		 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
 		 * for PUP_REQ/PDN_REQ bit to be cleared
+		 *
+		 * As per "5.2.9.5 Example Code 5" in i.MX-8MMini-yhsc.pdf
+		 * Display power on section checks for bit 25 of
+		 * Power handshake register to be set.
 		 */
-		ret = regmap_read_poll_timeout(domain->regmap,
+		if (domain->bits.pxx == IMX8MM_DISPMIX_SW_Pxx_REQ) {
+			regmap_clear_bits(domain->regmap, GPC_PU_PWRHSK,
+				   BIT(7));
+			ret = regmap_read_poll_timeout(domain->regmap,
+						GPC_PU_PWRHSK, reg_val,
+						!(reg_val & IMX8MM_DISPMIX_HSK_PWRDNACKN),
+						0, USEC_PER_MSEC);
+		} else {
+			ret = regmap_read_poll_timeout(domain->regmap,
 					       GPC_PU_PGC_SW_PDN_REQ, reg_val,
 					       !(reg_val & domain->bits.pxx),
 					       0, USEC_PER_MSEC);
+		}
 		if (ret) {
 			dev_err(domain->dev, "failed to command PGC\n");
 			goto out_clk_disable;
-- 
2.20.1

Re: [PATCH v1 1/4] soc: imx8mm: gpcv2: Power sequence for DISP

[Lucas Stach]

Hi Viraj,

Am Montag, dem 02.05.2022 um 12:02 +0200 schrieb Viraj Shah:
> As per the imx8mm reference manual, read bit 25(GPC_DISPMIX_
> PWRDNACKN) of the power handshake register and wait for ack during
> power on/off.
> 
> Signed-off-by: Viraj Shah <viraj.shah@linutronix.de>
> ---
>  drivers/soc/imx/gpcv2.c | 36 +++++++++++++++++++++++++++++++-----
>  1 file changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 3cb123016b3e..8ee70c30964f 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -254,11 +254,24 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>  		/*
>  		 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
>  		 * for PUP_REQ/PDN_REQ bit to be cleared
> +		 *
> +		 * As per "5.2.9.5 Example Code 5" in i.MX-8MMini-yhsc.pdf
> +		 * Display power on section checks for bit 25 of
> +		 * Power handshake register to be cleared.
>  		 */
> -		ret = regmap_read_poll_timeout(domain->regmap,
> -					       GPC_PU_PGC_SW_PUP_REQ, reg_val,
> -					       !(reg_val & domain->bits.pxx),
> -					       0, USEC_PER_MSEC);
> +		if (domain->bits.pxx == IMX8MM_DISPMIX_SW_Pxx_REQ) {
> +			regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
> +				   BIT(7), BIT(7));
> +			ret = regmap_read_poll_timeout(domain->regmap,
> +						GPC_PU_PWRHSK, reg_val,
> +						!(reg_val & IMX8MM_DISPMIX_HSK_PWRDNACKN),
> +						0, USEC_PER_MSEC);
> +		} else
> +			ret = regmap_read_poll_timeout(domain->regmap,
> +						GPC_PU_PGC_SW_PUP_REQ, reg_val,
> +						!(reg_val & domain->bits.pxx),
> +						0, USEC_PER_MSEC);
> +
The driver already handles the PWRHSK bits at the appropriate places.
Please do not hack in random sequences from the reference manual for
specific domains. Also you can not wait for the handshake ack in the
power up sequence, as the blk-ctrl driver only enables the ADB clock,
_after_ the GPC power domain has been powered up, so there is no way
for it to so the handshake here. See the comments in the blk-ctrl
driver.

Regards,
Lucas

>  		if (ret) {
>  			dev_err(domain->dev, "failed to command PGC\n");
>  			goto out_clk_disable;
> @@ -355,11 +368,24 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
>  		/*
>  		 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
>  		 * for PUP_REQ/PDN_REQ bit to be cleared
> +		 *
> +		 * As per "5.2.9.5 Example Code 5" in i.MX-8MMini-yhsc.pdf
> +		 * Display power on section checks for bit 25 of
> +		 * Power handshake register to be set.
>  		 */
> -		ret = regmap_read_poll_timeout(domain->regmap,
> +		if (domain->bits.pxx == IMX8MM_DISPMIX_SW_Pxx_REQ) {
> +			regmap_clear_bits(domain->regmap, GPC_PU_PWRHSK,
> +				   BIT(7));
> +			ret = regmap_read_poll_timeout(domain->regmap,
> +						GPC_PU_PWRHSK, reg_val,
> +						!(reg_val & IMX8MM_DISPMIX_HSK_PWRDNACKN),
> +						0, USEC_PER_MSEC);
> +		} else {
> +			ret = regmap_read_poll_timeout(domain->regmap,
>  					       GPC_PU_PGC_SW_PDN_REQ, reg_val,
>  					       !(reg_val & domain->bits.pxx),
>  					       0, USEC_PER_MSEC);
> +		}
>  		if (ret) {
>  			dev_err(domain->dev, "failed to command PGC\n");
>  			goto out_clk_disable;

[PATCH v1 2/4] soc: imx: imx8m-blk-ctrl: Display Power ON sequence

[Viraj Shah]

Enable the dispmix software clock and release the resets
as shown in the 5.2.9.5 section of reference manual.

Signed-off-by: Viraj Shah <viraj.shah@linutronix.de>
---
 drivers/soc/imx/imx8m-blk-ctrl.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
index 122f9c884b38..ca63fd30e70a 100644
--- a/drivers/soc/imx/imx8m-blk-ctrl.c
+++ b/drivers/soc/imx/imx8m-blk-ctrl.c
@@ -99,7 +99,10 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
 		dev_err(bc->dev, "failed to enable clocks\n");
 		goto bus_put;
 	}
-	regmap_set_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
+	/* As per section 5.2.9.5 of reference manual imx-8MMini-yhsc.pdf,
+	 * enable dispmix sft clock to power on the display
+	 */
+	regmap_write(bc->regmap, BLK_CLK_EN, 0x1FFF);
 
 	/* power up upstream GPC domain */
 	ret = pm_runtime_get_sync(domain->power_dev);
@@ -112,7 +115,7 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
 	udelay(5);
 
 	/* release reset */
-	regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
+	regmap_write(bc->regmap, BLK_SFT_RSTN, 0x7F);
 	if (data->mipi_phy_rst_mask)
 		regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
 
-- 
2.20.1

Re: [PATCH v1 2/4] soc: imx: imx8m-blk-ctrl: Display Power ON sequence

[Lucas Stach]

Am Montag, dem 02.05.2022 um 12:02 +0200 schrieb Viraj Shah:
> Enable the dispmix software clock and release the resets
> as shown in the 5.2.9.5 section of reference manual.
> 
> Signed-off-by: Viraj Shah <viraj.shah@linutronix.de>
> ---
>  drivers/soc/imx/imx8m-blk-ctrl.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> index 122f9c884b38..ca63fd30e70a 100644
> --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> @@ -99,7 +99,10 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>  		dev_err(bc->dev, "failed to enable clocks\n");
>  		goto bus_put;
>  	}
> -	regmap_set_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> +	/* As per section 5.2.9.5 of reference manual imx-8MMini-yhsc.pdf,
> +	 * enable dispmix sft clock to power on the display
> +	 */
> +	regmap_write(bc->regmap, BLK_CLK_EN, 0x1FFF);

Nack. The only clock that needs to be handled here is the ADB clock for
the handshake. All other clocks are enabled by the virtual power
domains provided by this blk-ctrl driver when the peripheral power up.

Regards,
Lucas

>  
>  	/* power up upstream GPC domain */
>  	ret = pm_runtime_get_sync(domain->power_dev);
> @@ -112,7 +115,7 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>  	udelay(5);
>  
>  	/* release reset */
> -	regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> +	regmap_write(bc->regmap, BLK_SFT_RSTN, 0x7F);
>  	if (data->mipi_phy_rst_mask)
>  		regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
>  

[PATCH v1 3/4] soc: imx: imx8m-blk-ctrl: Add reset bits for mipi dsi phy

[Viraj Shah]

As per reference manual page 3903, bit 16 (GPR_MIPI_S_RESETN)
as well as 17 (GPR_MIPI_M_RESETN) are the reset masks for mipi phy reset
mask.

Signed-off-by: Viraj Shah <viraj.shah@linutronix.de>
---
 drivers/soc/imx/imx8m-blk-ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
index ca63fd30e70a..d7638b7fa99d 100644
--- a/drivers/soc/imx/imx8m-blk-ctrl.c
+++ b/drivers/soc/imx/imx8m-blk-ctrl.c
@@ -502,7 +502,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
 		.gpc_name = "mipi-dsi",
 		.rst_mask = BIT(5),
 		.clk_mask = BIT(8) | BIT(9),
-		.mipi_phy_rst_mask = BIT(17),
+		.mipi_phy_rst_mask = BIT(17) | BIT(16),
 	},
 	[IMX8MM_DISPBLK_PD_MIPI_CSI] = {
 		.name = "dispblk-mipi-csi",
-- 
2.20.1

Re: [PATCH v1 3/4] soc: imx: imx8m-blk-ctrl: Add reset bits for mipi dsi phy

[Lucas Stach]

Am Montag, dem 02.05.2022 um 12:02 +0200 schrieb Viraj Shah:
> As per reference manual page 3903, bit 16 (GPR_MIPI_S_RESETN)
> as well as 17 (GPR_MIPI_M_RESETN) are the reset masks for mipi phy reset
> mask.

Nack. The MIPI-S reset is used for the CSI receiver and is already
handled by the appropriate power domain.

Regards,
Lucas

> 
> Signed-off-by: Viraj Shah <viraj.shah@linutronix.de>
> ---
>  drivers/soc/imx/imx8m-blk-ctrl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> index ca63fd30e70a..d7638b7fa99d 100644
> --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> @@ -502,7 +502,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
>  		.gpc_name = "mipi-dsi",
>  		.rst_mask = BIT(5),
>  		.clk_mask = BIT(8) | BIT(9),
> -		.mipi_phy_rst_mask = BIT(17),
> +		.mipi_phy_rst_mask = BIT(17) | BIT(16),
>  	},
>  	[IMX8MM_DISPBLK_PD_MIPI_CSI] = {
>  		.name = "dispblk-mipi-csi",

[PATCH v1 4/4] arm64: dts: imx8mm.dtsi: Add resets for dispmix power domain.

[Viraj Shah]

The resets are controlled from src. From reference manual page
959, display controller needs DISP_RESET bit to be set to reset
dispmix.

Signed-off-by: Viraj Shah <viraj.shah@linutronix.de>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index 1ee05677c2dd..11a6cae5bb99 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -734,6 +734,7 @@
 						assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>,
 									 <&clk IMX8MM_SYS_PLL1_800M>;
 						assigned-clock-rates = <500000000>, <200000000>;
+						resets = <&src IMX8MQ_RESET_DISP_RESET>;
 					};
 
 					pgc_mipi: power-domain@11 {
-- 
2.20.1

Re: [PATCH v1 4/4] arm64: dts: imx8mm.dtsi: Add resets for dispmix power domain.

[Lucas Stach]

Am Montag, dem 02.05.2022 um 12:02 +0200 schrieb Viraj Shah:
> The resets are controlled from src. From reference manual page
> 959, display controller needs DISP_RESET bit to be set to reset
> dispmix.
> 
This reset is driven by the GPC hardware logic. Only if you are 100%
sure that this is not the case should a reset be added to the DT, as
handling it both from the GPC HW sequencing and software has proven to
be problematic and result in sporadic hangs.

Regards,
Lucas

> Signed-off-by: Viraj Shah <viraj.shah@linutronix.de>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index 1ee05677c2dd..11a6cae5bb99 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -734,6 +734,7 @@
>  						assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>,
>  									 <&clk IMX8MM_SYS_PLL1_800M>;
>  						assigned-clock-rates = <500000000>, <200000000>;
> +						resets = <&src IMX8MQ_RESET_DISP_RESET>;
>  					};
>  
>  					pgc_mipi: power-domain@11 {

Re: [PATCH v1 0/4] imx8mm display controller power sequence

[Lucas Stach]

Hi Viraj,

Am Montag, dem 02.05.2022 um 12:02 +0200 schrieb Viraj Shah:
> This patch queue addresses the power sequence of the display controller
> of the imx8mm SoC. The sequence mentioned in example code 5 in section
> 5.2.9.5 of reference manual imx-8MMini-yhsc.pdf was not being performed.
> This meant that the display controller was not coming up.

I don't know where you got the idea that the current implementation
doesn't work. Numerous people are using this to get the i.MX8MM display
light up. All you need to do is add the various display pipeline
peripherals into the virtual power-domains provided by the blk-ctrl
driver. The only thing that prevents upstream from having a working
display pipeline is the conversion of the exynos-dsi into a proper
bridge driver, which has just landed upstream and the addition of a few
DT nodes, now that the drivers are getting ready.

Regards,
Lucas

> 
> Viraj Shah (4):
>   soc: imx: gpcv2: Power sequence for DISP
>   soc: imx: imx8m-blk-ctrl: Display Power ON sequence
>   soc: imx: imx8m-blk-ctrl: Add reset bits for mipi dsi phy
>   arm64: dts: imx8mm.dtsi: Add resets for dispmix power domain.
> 
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi |  1 +
>  drivers/soc/imx/gpcv2.c                   | 36 +++++++++++++++++++----
>  drivers/soc/imx/imx8m-blk-ctrl.c          |  9 ++++--
>  3 files changed, 38 insertions(+), 8 deletions(-)
>