[PATCH v3 0/2] Fix imx6sl display power domain

[PATCH v3 0/2] Fix imx6sl display power domain

[Leonard Crestez]

Changes since v2:
 * Don't mistakenly include IMX6SL_CLK_IPG in pd_disp
 * Use GENPD_FLAG_ALWAYS_ON, as suggested by Ulf

Errata workaround could be refined later, however the power savings are
small and it would require a lot of testing. So push the safest fix
first to get basic hardware support.

Link to v2: https://lkml.org/lkml/2018/7/9/447

Leonard Crestez (2):
  soc: imx: gpc: Disable 6sl display power gating for ERR006287
  ARM: dts: imx6sl: Convert gpc to new bindings

 arch/arm/boot/dts/imx6sl.dtsi | 35 +++++++++++++++++++++++++++++++----
 drivers/soc/imx/gpc.c         | 10 ++++++++++
 2 files changed, 41 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH v3 1/2] soc: imx: gpc: Disable 6sl display power gating for ERR006287

[Leonard Crestez]

The imx6sl chip errata document describes ERR006287 like this:

> Upon resuming from power gating, the modules in the display power domain
(eLCDIF, EPDC, PXP and SPDC) might fail to perform register reads
correctly.

> When the modules listed above are used, do not use power gating on the
display power domain.

Link: https://www.nxp.com/docs/en/errata/IMX6SLCE.pdf#page=62

Handle this in the safest possible way by keeping the DISP domain
always-on.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/soc/imx/gpc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index 526f2d02dc78..f7960f773019 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -339,30 +339,35 @@ static struct imx_pm_domain imx_gpc_domains[] = {
 };
 
 struct imx_gpc_dt_data {
 	int num_domains;
 	bool err009619_present;
+	bool err006287_present;
 };
 
 static const struct imx_gpc_dt_data imx6q_dt_data = {
 	.num_domains = 2,
 	.err009619_present = false,
+	.err006287_present = false,
 };
 
 static const struct imx_gpc_dt_data imx6qp_dt_data = {
 	.num_domains = 2,
 	.err009619_present = true,
+	.err006287_present = false,
 };
 
 static const struct imx_gpc_dt_data imx6sl_dt_data = {
 	.num_domains = 3,
 	.err009619_present = false,
+	.err006287_present = true,
 };
 
 static const struct imx_gpc_dt_data imx6sx_dt_data = {
 	.num_domains = 4,
 	.err009619_present = false,
+	.err006287_present = false,
 };
 
 static const struct of_device_id imx_gpc_dt_ids[] = {
 	{ .compatible = "fsl,imx6q-gpc", .data = &imx6q_dt_data },
 	{ .compatible = "fsl,imx6qp-gpc", .data = &imx6qp_dt_data },
@@ -467,10 +472,15 @@ static int imx_gpc_probe(struct platform_device *pdev)
 	/* Disable PU power down in normal operation if ERR009619 is present */
 	if (of_id_data->err009619_present)
 		imx_gpc_domains[GPC_PGC_DOMAIN_PU].flags |=
 				PGC_DOMAIN_FLAG_NO_PD;
 
+	/* Keep DISP always on if ERR006287 is present */
+	if (of_id_data->err006287_present)
+		imx_gpc_domains[GPC_PGC_DOMAIN_DISPLAY].base.flags |=
+				GENPD_FLAG_ALWAYS_ON;
+
 	if (!pgc_node) {
 		ret = imx_gpc_old_dt_init(&pdev->dev, regmap,
 					  of_id_data->num_domains);
 		if (ret)
 			return ret;
-- 
2.17.1

Re: [PATCH v3 1/2] soc: imx: gpc: Disable 6sl display power gating for ERR006287

[Lucas Stach]

Am Mittwoch, den 11.07.2018, 15:11 +0300 schrieb Leonard Crestez:
> The imx6sl chip errata document describes ERR006287 like this:
> 
> > Upon resuming from power gating, the modules in the display power domain
> 
> (eLCDIF, EPDC, PXP and SPDC) might fail to perform register reads
> correctly.
> 
> > When the modules listed above are used, do not use power gating on the
> 
> display power domain.
> 
> Link: https://www.nxp.com/docs/en/errata/IMX6SLCE.pdf#page=62
> 
> Handle this in the safest possible way by keeping the DISP domain
> always-on.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

Can you send a follow on patch to switch the i.MX6QP errata workaround
to use GENPD_FLAG_ALWAYS_ON and remove the -EBUSY stuff in the power
down path?

Regards,
Lucas

> ---
>  drivers/soc/imx/gpc.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index 526f2d02dc78..f7960f773019 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -339,30 +339,35 @@ static struct imx_pm_domain imx_gpc_domains[] = {
>  };
>  
>  struct imx_gpc_dt_data {
> >  	int num_domains;
> >  	bool err009619_present;
> > +	bool err006287_present;
>  };
>  
>  static const struct imx_gpc_dt_data imx6q_dt_data = {
> >  	.num_domains = 2,
> >  	.err009619_present = false,
> > +	.err006287_present = false,
>  };
>  
>  static const struct imx_gpc_dt_data imx6qp_dt_data = {
> >  	.num_domains = 2,
> >  	.err009619_present = true,
> > +	.err006287_present = false,
>  };
>  
>  static const struct imx_gpc_dt_data imx6sl_dt_data = {
> >  	.num_domains = 3,
> >  	.err009619_present = false,
> > +	.err006287_present = true,
>  };
>  
>  static const struct imx_gpc_dt_data imx6sx_dt_data = {
> >  	.num_domains = 4,
> >  	.err009619_present = false,
> > +	.err006287_present = false,
>  };
>  
>  static const struct of_device_id imx_gpc_dt_ids[] = {
> >  	{ .compatible = "fsl,imx6q-gpc", .data = &imx6q_dt_data },
> >  	{ .compatible = "fsl,imx6qp-gpc", .data = &imx6qp_dt_data },
> @@ -467,10 +472,15 @@ static int imx_gpc_probe(struct platform_device *pdev)
> >  	/* Disable PU power down in normal operation if ERR009619 is present */
> >  	if (of_id_data->err009619_present)
> >  		imx_gpc_domains[GPC_PGC_DOMAIN_PU].flags |=
> >  				PGC_DOMAIN_FLAG_NO_PD;
>  
> > +	/* Keep DISP always on if ERR006287 is present */
> > +	if (of_id_data->err006287_present)
> > +		imx_gpc_domains[GPC_PGC_DOMAIN_DISPLAY].base.flags |=
> > +				GENPD_FLAG_ALWAYS_ON;
> +
> >  	if (!pgc_node) {
> >  		ret = imx_gpc_old_dt_init(&pdev->dev, regmap,
> >  					  of_id_data->num_domains);
> >  		if (ret)
> >  			return ret;

Re: [PATCH v3 1/2] soc: imx: gpc: Disable 6sl display power gating for ERR006287

[Leonard Crestez]

On Wed, 2018-07-11 at 14:16 +0200, Lucas Stach wrote:
> Am Mittwoch, den 11.07.2018, 15:11 +0300 schrieb Leonard Crestez:
> > Handle this in the safest possible way by keeping the DISP domain
> > always-on.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> 
> Can you send a follow on patch to switch the i.MX6QP errata workaround
> to use GENPD_FLAG_ALWAYS_ON and remove the -EBUSY stuff in the power
> down path?

Sure.

I was thinking of converting it to a new GENPD_FLAG which allows
power_off in suspend (as suggested by Ulf) but switching to
GENPD_FLAG_ALWAYS_ON in order to simplify the code could be done first.

The -EBUSY stuff is not very harmful.

--
Regards,
Leonard

Re: [PATCH v3 1/2] soc: imx: gpc: Disable 6sl display power gating for ERR006287

[Lucas Stach]

Am Mittwoch, den 11.07.2018, 15:21 +0300 schrieb Leonard Crestez:
> On Wed, 2018-07-11 at 14:16 +0200, Lucas Stach wrote:
> > Am Mittwoch, den 11.07.2018, 15:11 +0300 schrieb Leonard Crestez:
> > > Handle this in the safest possible way by keeping the DISP domain
> > > always-on.
> > > 
> > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > 
> > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> > 
> > Can you send a follow on patch to switch the i.MX6QP errata
> > workaround
> > to use GENPD_FLAG_ALWAYS_ON and remove the -EBUSY stuff in the
> > power
> > down path?
> 
> Sure.
> 
> I was thinking of converting it to a new GENPD_FLAG which allows
> power_off in suspend (as suggested by Ulf) but switching to
> GENPD_FLAG_ALWAYS_ON in order to simplify the code could be done
> first.

Sure, if you are going to work on this I'm fine with converting it over
to this without first going for GENPD_FLAG_ALWAYS_ON. Just wanted to
make sure that things are consistent.

> The -EBUSY stuff is not very harmful.

It's dead code once the appropriate flags have been added to the
domain, so should be removed.

Regards,
Lucas

Re: [PATCH v3 1/2] soc: imx: gpc: Disable 6sl display power gating for ERR006287

[Ulf Hansson]

On 11 July 2018 at 14:11, Leonard Crestez <leonard.crestez@nxp.com> wrote:
> The imx6sl chip errata document describes ERR006287 like this:
>
>> Upon resuming from power gating, the modules in the display power domain
> (eLCDIF, EPDC, PXP and SPDC) might fail to perform register reads
> correctly.
>
>> When the modules listed above are used, do not use power gating on the
> display power domain.
>
> Link: https://www.nxp.com/docs/en/errata/IMX6SLCE.pdf#page=62
>
> Handle this in the safest possible way by keeping the DISP domain
> always-on.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/soc/imx/gpc.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index 526f2d02dc78..f7960f773019 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -339,30 +339,35 @@ static struct imx_pm_domain imx_gpc_domains[] = {
>  };
>
>  struct imx_gpc_dt_data {
>         int num_domains;
>         bool err009619_present;
> +       bool err006287_present;
>  };
>
>  static const struct imx_gpc_dt_data imx6q_dt_data = {
>         .num_domains = 2,
>         .err009619_present = false,
> +       .err006287_present = false,
>  };
>
>  static const struct imx_gpc_dt_data imx6qp_dt_data = {
>         .num_domains = 2,
>         .err009619_present = true,
> +       .err006287_present = false,
>  };
>
>  static const struct imx_gpc_dt_data imx6sl_dt_data = {
>         .num_domains = 3,
>         .err009619_present = false,
> +       .err006287_present = true,
>  };
>
>  static const struct imx_gpc_dt_data imx6sx_dt_data = {
>         .num_domains = 4,
>         .err009619_present = false,
> +       .err006287_present = false,
>  };
>
>  static const struct of_device_id imx_gpc_dt_ids[] = {
>         { .compatible = "fsl,imx6q-gpc", .data = &imx6q_dt_data },
>         { .compatible = "fsl,imx6qp-gpc", .data = &imx6qp_dt_data },
> @@ -467,10 +472,15 @@ static int imx_gpc_probe(struct platform_device *pdev)
>         /* Disable PU power down in normal operation if ERR009619 is present */
>         if (of_id_data->err009619_present)
>                 imx_gpc_domains[GPC_PGC_DOMAIN_PU].flags |=
>                                 PGC_DOMAIN_FLAG_NO_PD;
>
> +       /* Keep DISP always on if ERR006287 is present */
> +       if (of_id_data->err006287_present)
> +               imx_gpc_domains[GPC_PGC_DOMAIN_DISPLAY].base.flags |=
> +                               GENPD_FLAG_ALWAYS_ON;
> +
>         if (!pgc_node) {
>                 ret = imx_gpc_old_dt_init(&pdev->dev, regmap,
>                                           of_id_data->num_domains);
>                 if (ret)
>                         return ret;
> --
> 2.17.1
>

[PATCH v3 2/2] ARM: dts: imx6sl: Convert gpc to new bindings

[Leonard Crestez]

With old bindings imx_gpc_onecell_data always sets num_domains to 2 so
the DISPMIX domain can't actually be referenced. The pd is still defined
and pm core shuts it down as "unused" so display can't work.

Fix this by converting to new gpc bindings by adding pgc nodes and
referencing the newly-defined &pu_disp domain from &lcdif.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm/boot/dts/imx6sl.dtsi | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
index 102c575a6025..49a56b4fd393 100644
--- a/arch/arm/boot/dts/imx6sl.dtsi
+++ b/arch/arm/boot/dts/imx6sl.dtsi
@@ -698,14 +698,40 @@
 				reg = <0x020dc000 0x4000>;
 				interrupt-controller;
 				#interrupt-cells = <3>;
 				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-parent = <&intc>;
-				pu-supply = <&reg_pu>;
-				clocks = <&clks IMX6SL_CLK_GPU2D_OVG>,
-					 <&clks IMX6SL_CLK_GPU2D_PODF>;
-				#power-domain-cells = <1>;
+				clocks = <&clks IMX6SL_CLK_IPG>;
+				clock-names = "ipg";
+
+				pgc {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					power-domain@0 {
+						reg = <0>;
+						#power-domain-cells = <0>;
+					};
+
+					pd_pu: power-domain@1 {
+						reg = <1>;
+						#power-domain-cells = <0>;
+						power-supply = <&reg_pu>;
+						clocks = <&clks IMX6SL_CLK_GPU2D_OVG>,
+						         <&clks IMX6SL_CLK_GPU2D_PODF>;
+					};
+
+					pd_disp: power-domain@2 {
+						reg = <2>;
+						#power-domain-cells = <0>;
+						clocks = <&clks IMX6SL_CLK_LCDIF_AXI>,
+							 <&clks IMX6SL_CLK_LCDIF_PIX>,
+							 <&clks IMX6SL_CLK_EPDC_AXI>,
+							 <&clks IMX6SL_CLK_EPDC_PIX>,
+							 <&clks IMX6SL_CLK_PXP_AXI>;
+					};
+				};
 			};
 
 			gpr: iomuxc-gpr@20e0000 {
 				compatible = "fsl,imx6sl-iomuxc-gpr",
 					     "fsl,imx6q-iomuxc-gpr", "syscon";
@@ -756,10 +782,11 @@
 				clocks = <&clks IMX6SL_CLK_LCDIF_PIX>,
 					 <&clks IMX6SL_CLK_LCDIF_AXI>,
 					 <&clks IMX6SL_CLK_DUMMY>;
 				clock-names = "pix", "axi", "disp_axi";
 				status = "disabled";
+				power-domains = <&pd_disp>;
 			};
 
 			dcp: dcp@20fc000 {
 				compatible = "fsl,imx6sl-dcp", "fsl,imx28-dcp";
 				reg = <0x020fc000 0x4000>;
-- 
2.17.1

Re: [PATCH v3 2/2] ARM: dts: imx6sl: Convert gpc to new bindings

[Lucas Stach]

Am Mittwoch, den 11.07.2018, 15:11 +0300 schrieb Leonard Crestez:
> With old bindings imx_gpc_onecell_data always sets num_domains to 2 so
> the DISPMIX domain can't actually be referenced. The pd is still defined
> and pm core shuts it down as "unused" so display can't work.
> 
> Fix this by converting to new gpc bindings by adding pgc nodes and
> referencing the newly-defined &pu_disp domain from &lcdif.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  arch/arm/boot/dts/imx6sl.dtsi | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
> index 102c575a6025..49a56b4fd393 100644
> --- a/arch/arm/boot/dts/imx6sl.dtsi
> +++ b/arch/arm/boot/dts/imx6sl.dtsi
> @@ -698,14 +698,40 @@
> >  				reg = <0x020dc000 0x4000>;
> >  				interrupt-controller;
> >  				#interrupt-cells = <3>;
> >  				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>;
> >  				interrupt-parent = <&intc>;
> > -				pu-supply = <&reg_pu>;
> > -				clocks = <&clks IMX6SL_CLK_GPU2D_OVG>,
> > -					 <&clks IMX6SL_CLK_GPU2D_PODF>;
> > -				#power-domain-cells = <1>;
> > +				clocks = <&clks IMX6SL_CLK_IPG>;
> > +				clock-names = "ipg";
> +
> > +				pgc {
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> +
> > +					power-domain@0 {
> > +						reg = <0>;
> > +						#power-domain-cells = <0>;
> > +					};
> +
> > > +					pd_pu: power-domain@1 {
> > +						reg = <1>;
> > +						#power-domain-cells = <0>;
> > +						power-supply = <&reg_pu>;
> > +						clocks = <&clks IMX6SL_CLK_GPU2D_OVG>,
> > +						         <&clks IMX6SL_CLK_GPU2D_PODF>;
> > +					};
> +
> > > +					pd_disp: power-domain@2 {
> > +						reg = <2>;
> > +						#power-domain-cells = <0>;
> > +						clocks = <&clks IMX6SL_CLK_LCDIF_AXI>,
> > +							 <&clks IMX6SL_CLK_LCDIF_PIX>,
> > +							 <&clks IMX6SL_CLK_EPDC_AXI>,
> > +							 <&clks IMX6SL_CLK_EPDC_PIX>,
> > +							 <&clks IMX6SL_CLK_PXP_AXI>;
> > +					};
> > +				};
> >  			};
>  
> > >  			gpr: iomuxc-gpr@20e0000 {
> >  				compatible = "fsl,imx6sl-iomuxc-gpr",
> >  					     "fsl,imx6q-iomuxc-gpr", "syscon";
> @@ -756,10 +782,11 @@
> >  				clocks = <&clks IMX6SL_CLK_LCDIF_PIX>,
> >  					 <&clks IMX6SL_CLK_LCDIF_AXI>,
> >  					 <&clks IMX6SL_CLK_DUMMY>;
> >  				clock-names = "pix", "axi", "disp_axi";
> >  				status = "disabled";
> > +				power-domains = <&pd_disp>;
> >  			};
>  
> > >  			dcp: dcp@20fc000 {
> >  				compatible = "fsl,imx6sl-dcp", "fsl,imx28-dcp";
> >  				reg = <0x020fc000 0x4000>;

Re: [PATCH v3 0/2] Fix imx6sl display power domain

[Shawn Guo]

On Wed, Jul 11, 2018 at 03:11:15PM +0300, Leonard Crestez wrote:
> Leonard Crestez (2):
>   soc: imx: gpc: Disable 6sl display power gating for ERR006287
>   ARM: dts: imx6sl: Convert gpc to new bindings

Applied both, thanks.