[PATCH 0/8] soc: imx: gpc: fixes and clean up

[PATCH 0/8] soc: imx: gpc: fixes and clean up

[Dong Aisheng]

This series mostly fixes a few exist issues and clean up.
1) fix clk error handling to avoid kernel crash
2) fix wrong use of regmap cache
3) fix domain_index sanity check
4) fix mx6sl regression
5) a few clean up

Patch based on shawn/for-next tree.

Dong Aisheng (8):
  soc: imx: gpc: fix gpc clk get error handling
  soc: imx: gpc: fix the wrong using of regmap cache
  soc: imx: gpc: fix domain_index sanity check issue
  soc: imx: gpc: fix imx6sl gpc power domain regression
  soc: imx: gpc: fix comment when power up domain
  soc: imx: gpc: keep PGC_X_CTRL name align with reference manual
  dt-bindings: imx-gpc: correct the DOMAIN_INDEX using
  soc: imx: gpc: remove unnecessary readable_reg callback

 .../devicetree/bindings/power/fsl,imx-gpc.txt      |  4 +--
 drivers/soc/imx/gpc.c                              | 36 ++++------------------
 2 files changed, 8 insertions(+), 32 deletions(-)

-- 
2.7.4

[PATCH 1/8] soc: imx: gpc: fix gpc clk get error handling

[Dong Aisheng]

We got a following kernel crash once supplying one more IPG
clock in GPC node in devicetree. The original error handling of
clocks get is a bit wrong that when reaching the maximum clock
get error, the index 'i' is already GPC_CLK_MAX which can't be used
as the array index for clk_put operations.

[    3.000110] imx-gpc 20dc000.gpc: more than 6 clocks
[    3.005141] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[    3.013487] pgd = c0004000
[    3.016300] [00000000] *pgd=00000000
[    3.020060] Internal error: Oops: 805 [#1] SMP ARM
[    3.024957] Modules linked in:
[    3.028122] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       4.11.0-rc1-00056-g813791b-dirty #1140
[    3.037801] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[    3.044435] task: ef298000 task.stack: ef294000
[    3.049080] PC is at __clk_put+0x38/0xec
[    3.053103] LR is at 0x7f54ce9a
[    3.056345] pc : [<c0537984>]    lr : [<7f54ce9a>]    psr: 60000013
[    3.056345] sp : ef295d48  ip : c8a582b2  fp : ef295d64
[    3.068026] r10: ee9fc400  r9 : 00000000  r8 : ef398c10
[    3.073354] r7 : ef398c10  r6 : c1071264  r5 : c10710f0  r4 : eea5be80
[    3.079986] r3 : 00000000  r2 : 00000000  r1 : 00000100  r0 : 00000001
[    3.086621] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    3.093863] Control: 10c5387d  Table: 1000404a  DAC: 00000051
[    3.099712] Process swapper/0 (pid: 1, stack limit = 0xef294210)
[    3.105823] Stack: (0xef295d48 to 0xef296000)
...
[    3.292660] Backtrace:
[    3.295222] [<c053794c>] (__clk_put) from [<c0531028>] (clk_put+0x18/0x1c)
[    3.302206]  r6:c1071264 r5:c10710f0 r4:c107124c r3:00000001
[    3.307977] [<c0531010>] (clk_put) from [<c0546ba0>] (imx_pgc_get_clocks+0x64/0x78)
[    3.315747] [<c0546b3c>] (imx_pgc_get_clocks) from [<c0547124>] (imx_gpc_probe+0x204/0x31c)
[    3.324209]  r7:00000000 r6:c1070eb0 r5:00000001 r4:ef398c00
[    3.329980] [<c0546f20>] (imx_gpc_probe) from [<c05e65f0>] (platform_drv_probe+0x5c/0xc0)
[    3.338270]  r10:c0f00608 r9:00000000 r8:00000000 r7:fffffdfb r6:c1070f20 r5:ef398c10
[    3.346207]  r4:ef398c10
[    3.348849] [<c05e6594>] (platform_drv_probe) from [<c05e4250>] (driver_probe_device+0x214/0x2ec)
[    3.357835]  r7:c1070f20 r6:00000000 r5:c18cea74 r4:ef398c10
[    3.363607] [<c05e403c>] (driver_probe_device) from [<c05e43ec>] (__driver_attach+0xc4/0xc8)
[    3.372159]  r9:c0f8b858 r8:c0f8b850 r7:00000000 r6:ef398c44 r5:c1070f20 r4:ef398c10
[    3.380017] [<c05e4328>] (__driver_attach) from [<c05e21fc>] (bus_for_each_dev+0x7c/0xb0)
[    3.388304]  r6:c05e4328 r5:c1070f20 r4:00000000 r3:00000000
[    3.394074] [<c05e2180>] (bus_for_each_dev) from [<c05e3bc4>] (driver_attach+0x28/0x30)
[    3.402188]  r6:c107f3e8 r5:eea5be00 r4:c1070f20
[    3.406913] [<c05e3b9c>] (driver_attach) from [<c05e3740>] (bus_add_driver+0x19c/0x224)
[    3.415034] [<c05e35a4>] (bus_add_driver) from [<c05e52fc>] (driver_register+0x88/0x108)
[    3.423235]  r7:c10e1000 r6:00000000 r5:c0f57d2c r4:c1070f20
[    3.429004] [<c05e5274>] (driver_register) from [<c05e6534>] (__platform_driver_register+0x40/0x54)
[    3.438160]  r5:c0f57d2c r4:00000006
[    3.441846] [<c05e64f4>] (__platform_driver_register) from [<c0f57d44>] (imx_gpc_driver_init+0x18/0x20)
[    3.451360] [<c0f57d2c>] (imx_gpc_driver_init) from [<c010200c>] (do_one_initcall+0x4c/0x180)
[    3.460008] [<c0101fc0>] (do_one_initcall) from [<c0f00e40>] (kernel_init_freeable+0x130/0x1f8)
[    3.468820]  r9:c0f8b858 r8:c0f8b850 r6:c0fc2414 r5:c10e1000 r4:00000006
[    3.475637] [<c0f00d10>] (kernel_init_freeable) from [<c0ae6aec>] (kernel_init+0x18/0x124)
[    3.484014]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0ae6ad4
[    3.491951]  r4:00000000
[    3.494590] [<c0ae6ad4>] (kernel_init) from [<c01088d0>] (ret_from_fork+0x14/0x24)
[    3.502267]  r4:00000000 r3:ef294000
[    3.505947] Code: e5943014 e5942018 e3530000 e3a01c01 (e5823000)
[    3.512215] ---[ end trace 375f9f2a5ddeff3c ]---
[    3.517036] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/soc/imx/gpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index 1e9b3b8..c9bfdfd 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -143,7 +143,7 @@ static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain)
 	return 0;
 
 clk_err:
-	for (; i >= 0; i--)
+	while (i--)
 		clk_put(domain->clk[i]);
 
 	return ret;
-- 
2.7.4

[PATCH 2/8] soc: imx: gpc: fix the wrong using of regmap cache

[Dong Aisheng]

Without providing the proper reg_defaults, the regmap registers first
read out may be always 0 if enabling cache, which results in the
following issue we met.
e.g. During driver probe in imx6_pm_domain_power_on():
regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val);
The PGC_PUPSCR register val is always 0 but it's actually 0xf01 in HW.

Since GPC registers are tightly related to CPU bring up and may be
changed in bootloader, we don't want to provide defaults.
And the cache really does not save too much for GPC module.

Therefore, simply disable cache to fix the issue and make life easy.

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/soc/imx/gpc.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index c9bfdfd..7e6a672 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -289,22 +289,12 @@ static bool imx_gpc_readable_reg(struct device *dev, unsigned int reg)
 	return (reg % 4 == 0) && (reg <= 0x2ac);
 }
 
-static bool imx_gpc_volatile_reg(struct device *dev, unsigned int reg)
-{
-	if (reg == GPC_CNTR)
-		return true;
-
-	return false;
-}
-
 static const struct regmap_config imx_gpc_regmap_config = {
-	.cache_type = REGCACHE_FLAT,
 	.reg_bits = 32,
 	.val_bits = 32,
 	.reg_stride = 4,
 
 	.readable_reg = imx_gpc_readable_reg,
-	.volatile_reg = imx_gpc_volatile_reg,
 
 	.max_register = 0x2ac,
 };
-- 
2.7.4

[PATCH 3/8] soc: imx: gpc: fix domain_index sanity check issue

[Dong Aisheng]

ARRAY_SIZE(imx_gpc_domains) represents all power domains supported
by different SoCs. Driver should use SoC specific of_id_data->num_domains
instead to do power domain index sanity check.
e.g. MX6Q supports two power domains while MX6SL supports three.

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/soc/imx/gpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index 7e6a672..ba6e7ab 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -413,7 +413,7 @@ static int imx_gpc_probe(struct platform_device *pdev)
 				of_node_put(np);
 				return ret;
 			}
-			if (domain_index >= ARRAY_SIZE(imx_gpc_domains))
+			if (domain_index >= of_id_data->num_domains)
 				continue;
 
 			domain = &imx_gpc_domains[domain_index];
-- 
2.7.4

[PATCH 4/8] soc: imx: gpc: fix imx6sl gpc power domain regression

[Dong Aisheng]

Commit 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
broke the MX6SL GPC power domain support.
It always got the following error:
[    1.248364] imx-gpc 20dc000.gpc: could not find pgc DT node
This patch adds back the legecy support.

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/soc/imx/gpc.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index ba6e7ab..c1d0e67 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -385,12 +385,6 @@ static int imx_gpc_probe(struct platform_device *pdev)
 	}
 
 	if (!pgc_node) {
-		/* old DT layout is only supported for mx6q aka 2 domains */
-		if (of_id_data->num_domains != 2) {
-			dev_err(&pdev->dev, "could not find pgc DT node\n");
-			return -ENODEV;
-		}
-
 		ret = imx_gpc_old_dt_init(&pdev->dev, regmap);
 		if (ret)
 			return ret;
-- 
2.7.4

[PATCH 5/8] soc: imx: gpc: fix comment when power up domain

[Dong Aisheng]

The correct comment should be power up domain.

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/soc/imx/gpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index c1d0e67..19f4e6b 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -104,7 +104,7 @@ static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
 	regmap_update_bits(pd->regmap, pd->reg_offs + GPC_PGC_PDN_OFFS,
 			   0x1, 0x1);
 
-	/* Read ISO and ISO2SW power down delays */
+	/* Read ISO and ISO2SW power up delays */
 	regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val);
 	sw = val & 0x3f;
 	sw2iso = (val >> 8) & 0x3f;
-- 
2.7.4

[PATCH 6/8] soc: imx: gpc: keep PGC_X_CTRL name align with reference manual

[Dong Aisheng]

Instead of GPC_PGC_PDN_OFFS, naming it as GPC_PGC_CTRL_OFFS which is
defined in reference manual for better reading.

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/soc/imx/gpc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index 19f4e6b..a744f7e 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -21,7 +21,7 @@
 
 #define GPC_CNTR		0x000
 
-#define GPC_PGC_PDN_OFFS	0x0
+#define GPC_PGC_CTRL_OFFS	0x0
 #define GPC_PGC_PUPSCR_OFFS	0x4
 #define GPC_PGC_PDNSCR_OFFS	0x8
 #define GPC_PGC_SW2ISO_SHIFT	0x8
@@ -65,7 +65,7 @@ static int imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
 	iso2sw = (val >> 8) & 0x3f;
 
 	/* Gate off domain when powered down */
-	regmap_update_bits(pd->regmap, pd->reg_offs + GPC_PGC_PDN_OFFS,
+	regmap_update_bits(pd->regmap, pd->reg_offs + GPC_PGC_CTRL_OFFS,
 			   0x1, 0x1);
 
 	/* Request GPC to power down domain */
@@ -101,7 +101,7 @@ static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
 		clk_prepare_enable(pd->clk[i]);
 
 	/* Gate off domain when powered down */
-	regmap_update_bits(pd->regmap, pd->reg_offs + GPC_PGC_PDN_OFFS,
+	regmap_update_bits(pd->regmap, pd->reg_offs + GPC_PGC_CTRL_OFFS,
 			   0x1, 0x1);
 
 	/* Read ISO and ISO2SW power up delays */
-- 
2.7.4

[PATCH 7/8] dt-bindings: imx-gpc: correct the DOMAIN_INDEX using

[Dong Aisheng]

Actually DOMAIN_INDEX is not used by the client devices to refer to
the power domain, it uses phandle. Corrent the binding doc a bit
to avoid confusing.

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 Documentation/devicetree/bindings/power/fsl,imx-gpc.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
index 06040a4..58d323c 100644
--- a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
+++ b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
@@ -20,8 +20,7 @@ subnodes of the power gating controller 'pgc' node of the GPC and should
 contain the following:
 
 Required properties:
-- reg: the DOMAIN_INDEX as used by the client devices to refer to this
-  power domain
+- reg: Must contain the DOMAIN_INDEX of this power domain
   The following DOMAIN_INDEX values are valid for i.MX6Q:
   ARM_DOMAIN     0
   PU_DOMAIN      1
@@ -54,6 +53,7 @@ Example:
 				reg = <0>;
 				#power-domain-cells = <0>;
 			};
+
 			pd_pu: power-domain at 1 {
 				reg = <1>;
 				#power-domain-cells = <0>;
-- 
2.7.4

[PATCH 8/8] soc: imx: gpc: remove unnecessary readable_reg callback

[Dong Aisheng]

It is not really necessary to provide the current .readable_reg
implementation as we know what we're doing in our driver
and the regmap core has already done the partial check for
available maximum regs.

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/soc/imx/gpc.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index a744f7e..4885ec2 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -284,18 +284,10 @@ static const struct of_device_id imx_gpc_dt_ids[] = {
 	{ }
 };
 
-static bool imx_gpc_readable_reg(struct device *dev, unsigned int reg)
-{
-	return (reg % 4 == 0) && (reg <= 0x2ac);
-}
-
 static const struct regmap_config imx_gpc_regmap_config = {
 	.reg_bits = 32,
 	.val_bits = 32,
 	.reg_stride = 4,
-
-	.readable_reg = imx_gpc_readable_reg,
-
 	.max_register = 0x2ac,
 };
 
-- 
2.7.4

[PATCH 1/8] soc: imx: gpc: fix gpc clk get error handling

[Shawn Guo]

On Mon, Mar 20, 2017 at 02:15:40PM +0800, Dong Aisheng wrote:
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index 1e9b3b8..c9bfdfd 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -143,7 +143,7 @@ static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain)
>  	return 0;
>  
>  clk_err:
> -	for (; i >= 0; i--)
> +	while (i--)
>  		clk_put(domain->clk[i]);

Will clk[0] be put then?

Shawn

>  
>  	return ret;

[PATCH 4/8] soc: imx: gpc: fix imx6sl gpc power domain regression

[Lucas Stach]

Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> Commit 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> broke the MX6SL GPC power domain support.
> It always got the following error:
> [    1.248364] imx-gpc 20dc000.gpc: could not find pgc DT node
> This patch adds back the legecy support.

This patch removes a safeguard against abusing the old binding for new
SoCs. Please leave this check in place and instead loosen the check to
also allow the old i.MX6SL binding.

> 
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/soc/imx/gpc.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index ba6e7ab..c1d0e67 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -385,12 +385,6 @@ static int imx_gpc_probe(struct platform_device *pdev)
>  	}
>  
>  	if (!pgc_node) {
> -		/* old DT layout is only supported for mx6q aka 2 domains */
> -		if (of_id_data->num_domains != 2) {
> -			dev_err(&pdev->dev, "could not find pgc DT node\n");
> -			return -ENODEV;
> -		}
> -
>  		ret = imx_gpc_old_dt_init(&pdev->dev, regmap);
>  		if (ret)
>  			return ret;

[PATCH 2/8] soc: imx: gpc: fix the wrong using of regmap cache

[Lucas Stach]

Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> Without providing the proper reg_defaults, the regmap registers first
> read out may be always 0 if enabling cache, which results in the
> following issue we met.
> e.g. During driver probe in imx6_pm_domain_power_on():
> regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val);
> The PGC_PUPSCR register val is always 0 but it's actually 0xf01 in HW.
> 
> Since GPC registers are tightly related to CPU bring up and may be
> changed in bootloader, we don't want to provide defaults.
> And the cache really does not save too much for GPC module.
> 
> Therefore, simply disable cache to fix the issue and make life easy.

While I agree that not using the cache may be the right thing to do
here, bypassing the cache by not providing the volatile function is only
half the fix.

Please also set the cache type to REGCACHE_NONE, to make it clear that
the cache isn't used.

Regards,
Lucas

> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/soc/imx/gpc.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index c9bfdfd..7e6a672 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -289,22 +289,12 @@ static bool imx_gpc_readable_reg(struct device *dev, unsigned int reg)
>  	return (reg % 4 == 0) && (reg <= 0x2ac);
>  }
>  
> -static bool imx_gpc_volatile_reg(struct device *dev, unsigned int reg)
> -{
> -	if (reg == GPC_CNTR)
> -		return true;
> -
> -	return false;
> -}
> -
>  static const struct regmap_config imx_gpc_regmap_config = {
> -	.cache_type = REGCACHE_FLAT,
>  	.reg_bits = 32,
>  	.val_bits = 32,
>  	.reg_stride = 4,
>  
>  	.readable_reg = imx_gpc_readable_reg,
> -	.volatile_reg = imx_gpc_volatile_reg,
>  
>  	.max_register = 0x2ac,
>  };

[PATCH 3/8] soc: imx: gpc: fix domain_index sanity check issue

[Lucas Stach]

Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> ARRAY_SIZE(imx_gpc_domains) represents all power domains supported
> by different SoCs. Driver should use SoC specific of_id_data->num_domains
> instead to do power domain index sanity check.
> e.g. MX6Q supports two power domains while MX6SL supports three.

It's a sanity check against writing into memory outside the array. We
don't really need to guard more against wrong DTs, but as this patch is
also equally correct:

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

> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/soc/imx/gpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index 7e6a672..ba6e7ab 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -413,7 +413,7 @@ static int imx_gpc_probe(struct platform_device *pdev)
>  				of_node_put(np);
>  				return ret;
>  			}
> -			if (domain_index >= ARRAY_SIZE(imx_gpc_domains))
> +			if (domain_index >= of_id_data->num_domains)
>  				continue;
>  
>  			domain = &imx_gpc_domains[domain_index];

[PATCH 5/8] soc: imx: gpc: fix comment when power up domain

[Lucas Stach]

Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> The correct comment should be power up domain.
> 
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

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

> ---
>  drivers/soc/imx/gpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index c1d0e67..19f4e6b 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -104,7 +104,7 @@ static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
>  	regmap_update_bits(pd->regmap, pd->reg_offs + GPC_PGC_PDN_OFFS,
>  			   0x1, 0x1);
>  
> -	/* Read ISO and ISO2SW power down delays */
> +	/* Read ISO and ISO2SW power up delays */
>  	regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val);
>  	sw = val & 0x3f;
>  	sw2iso = (val >> 8) & 0x3f;

[PATCH 6/8] soc: imx: gpc: keep PGC_X_CTRL name align with reference manual

[Lucas Stach]

Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> Instead of GPC_PGC_PDN_OFFS, naming it as GPC_PGC_CTRL_OFFS which is
> defined in reference manual for better reading.
> 
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

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

> ---
>  drivers/soc/imx/gpc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index 19f4e6b..a744f7e 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -21,7 +21,7 @@
>  
>  #define GPC_CNTR		0x000
>  
> -#define GPC_PGC_PDN_OFFS	0x0
> +#define GPC_PGC_CTRL_OFFS	0x0
>  #define GPC_PGC_PUPSCR_OFFS	0x4
>  #define GPC_PGC_PDNSCR_OFFS	0x8
>  #define GPC_PGC_SW2ISO_SHIFT	0x8
> @@ -65,7 +65,7 @@ static int imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
>  	iso2sw = (val >> 8) & 0x3f;
>  
>  	/* Gate off domain when powered down */
> -	regmap_update_bits(pd->regmap, pd->reg_offs + GPC_PGC_PDN_OFFS,
> +	regmap_update_bits(pd->regmap, pd->reg_offs + GPC_PGC_CTRL_OFFS,
>  			   0x1, 0x1);
>  
>  	/* Request GPC to power down domain */
> @@ -101,7 +101,7 @@ static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
>  		clk_prepare_enable(pd->clk[i]);
>  
>  	/* Gate off domain when powered down */
> -	regmap_update_bits(pd->regmap, pd->reg_offs + GPC_PGC_PDN_OFFS,
> +	regmap_update_bits(pd->regmap, pd->reg_offs + GPC_PGC_CTRL_OFFS,
>  			   0x1, 0x1);
>  
>  	/* Read ISO and ISO2SW power up delays */

[PATCH 7/8] dt-bindings: imx-gpc: correct the DOMAIN_INDEX using

[Lucas Stach]

Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> Actually DOMAIN_INDEX is not used by the client devices to refer to
> the power domain, it uses phandle. Corrent the binding doc a bit
> to avoid confusing.
> 
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

Thanks for the catch. Must have slipped through when changing the
binding.

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

> ---
>  Documentation/devicetree/bindings/power/fsl,imx-gpc.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
> index 06040a4..58d323c 100644
> --- a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
> +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
> @@ -20,8 +20,7 @@ subnodes of the power gating controller 'pgc' node of the GPC and should
>  contain the following:
>  
>  Required properties:
> -- reg: the DOMAIN_INDEX as used by the client devices to refer to this
> -  power domain
> +- reg: Must contain the DOMAIN_INDEX of this power domain
>    The following DOMAIN_INDEX values are valid for i.MX6Q:
>    ARM_DOMAIN     0
>    PU_DOMAIN      1
> @@ -54,6 +53,7 @@ Example:
>  				reg = <0>;
>  				#power-domain-cells = <0>;
>  			};
> +
>  			pd_pu: power-domain at 1 {
>  				reg = <1>;
>  				#power-domain-cells = <0>;

[PATCH 8/8] soc: imx: gpc: remove unnecessary readable_reg callback

[Lucas Stach]

Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> It is not really necessary to provide the current .readable_reg
> implementation as we know what we're doing in our driver
> and the regmap core has already done the partial check for
> available maximum regs.
> 
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

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

> ---
>  drivers/soc/imx/gpc.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index a744f7e..4885ec2 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -284,18 +284,10 @@ static const struct of_device_id imx_gpc_dt_ids[] = {
>  	{ }
>  };
>  
> -static bool imx_gpc_readable_reg(struct device *dev, unsigned int reg)
> -{
> -	return (reg % 4 == 0) && (reg <= 0x2ac);
> -}
> -
>  static const struct regmap_config imx_gpc_regmap_config = {
>  	.reg_bits = 32,
>  	.val_bits = 32,
>  	.reg_stride = 4,
> -
> -	.readable_reg = imx_gpc_readable_reg,
> -
>  	.max_register = 0x2ac,
>  };
>  

[PATCH 1/8] soc: imx: gpc: fix gpc clk get error handling

[Shawn Guo]

On Thu, Mar 23, 2017 at 02:33:06AM +0800, Dong Aisheng wrote:
> On Mon, Mar 20, 2017 at 04:05:24PM +0800, Shawn Guo wrote:
> > On Mon, Mar 20, 2017 at 02:15:40PM +0800, Dong Aisheng wrote:
> > > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> > > index 1e9b3b8..c9bfdfd 100644
> > > --- a/drivers/soc/imx/gpc.c
> > > +++ b/drivers/soc/imx/gpc.c
> > > @@ -143,7 +143,7 @@ static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain)
> > >  	return 0;
> > >  
> > >  clk_err:
> > > -	for (; i >= 0; i--)
> > > +	while (i--)
> > >  		clk_put(domain->clk[i]);
> > 
> > Will clk[0] be put then?
> > 
> 
> Yes, it is a bit tricky:
> See as follows:
> while (i--)	<== (i = 1)
> 	clk_put(domain->clk[i]);	<=== (i = 0)

Hah, I was still thinking it in the way how for-loop works.  Sorry for
the noise.

Shawn

[PATCH 2/8] soc: imx: gpc: fix the wrong using of regmap cache

[Lucas Stach]

Am Donnerstag, den 23.03.2017, 02:53 +0800 schrieb Dong Aisheng:
> Hi Lucas,
> 
> Thanks for the review.
> 
> On Mon, Mar 20, 2017 at 10:38:21AM +0100, Lucas Stach wrote:
> > Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> > > Without providing the proper reg_defaults, the regmap registers first
> > > read out may be always 0 if enabling cache, which results in the
> > > following issue we met.
> > > e.g. During driver probe in imx6_pm_domain_power_on():
> > > regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val);
> > > The PGC_PUPSCR register val is always 0 but it's actually 0xf01 in HW.
> > > 
> > > Since GPC registers are tightly related to CPU bring up and may be
> > > changed in bootloader, we don't want to provide defaults.
> > > And the cache really does not save too much for GPC module.
> > > 
> > > Therefore, simply disable cache to fix the issue and make life easy.
> > 
> > While I agree that not using the cache may be the right thing to do
> > here, bypassing the cache by not providing the volatile function is only
> > half the fix.
> > 
> 
> Removing volatile function actually is only due to it becomes meaningless
> after disable cache, not used to bypass or fix the cache issue.
> 
> > Please also set the cache type to REGCACHE_NONE, to make it clear that
> > the cache isn't used.
> > 
> 
> I believe all guys normally may know the regmap cache is disabled by default
> if without specifying cache_type, this way is also commonly used in kernel.
> So i just simply remove it.
> Another bonus is it fully hides the cache bits from drive user.
> No one needs to know it if not use.
> 
> Anyway, if you insist on it, i could explicitly claim the cache type to
> REGCACHE_NONE in V2.

Um, no thanks. I claim lack of coffee for my first review. This patch is

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

> > 
> > > Cc: Lucas Stach <l.stach@pengutronix.de>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > >  drivers/soc/imx/gpc.c | 10 ----------
> > >  1 file changed, 10 deletions(-)
> > > 
> > > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> > > index c9bfdfd..7e6a672 100644
> > > --- a/drivers/soc/imx/gpc.c
> > > +++ b/drivers/soc/imx/gpc.c
> > > @@ -289,22 +289,12 @@ static bool imx_gpc_readable_reg(struct device *dev, unsigned int reg)
> > >  	return (reg % 4 == 0) && (reg <= 0x2ac);
> > >  }
> > >  
> > > -static bool imx_gpc_volatile_reg(struct device *dev, unsigned int reg)
> > > -{
> > > -	if (reg == GPC_CNTR)
> > > -		return true;
> > > -
> > > -	return false;
> > > -}
> > > -
> > >  static const struct regmap_config imx_gpc_regmap_config = {
> > > -	.cache_type = REGCACHE_FLAT,
> > >  	.reg_bits = 32,
> > >  	.val_bits = 32,
> > >  	.reg_stride = 4,
> > >  
> > >  	.readable_reg = imx_gpc_readable_reg,
> > > -	.volatile_reg = imx_gpc_volatile_reg,
> > >  
> > >  	.max_register = 0x2ac,
> > >  };
> > 
> > 

[PATCH 4/8] soc: imx: gpc: fix imx6sl gpc power domain regression

[Lucas Stach]

Am Donnerstag, den 23.03.2017, 03:23 +0800 schrieb Dong Aisheng:
> On Mon, Mar 20, 2017 at 10:26:14AM +0100, Lucas Stach wrote:
> > Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> > > Commit 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> > > broke the MX6SL GPC power domain support.
> > > It always got the following error:
> > > [    1.248364] imx-gpc 20dc000.gpc: could not find pgc DT node
> > > This patch adds back the legecy support.
> > 
> > This patch removes a safeguard against abusing the old binding for new
> > SoCs. Please leave this check in place and instead loosen the check to
> > also allow the old i.MX6SL binding.
> > 
> 
> Thanks for the reminder.
> I found beside the checking, we probably also need some extra fix that
> the current imx_gpc_old_dt_init() only supports two domains while the
> driver formerly supports three for MX6SL.
> 
> And because the num_domains is defined by driver itself, no need check
> in imx_gpc_old_dt_init() anymore.
> 
> The patch would be as follows:
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index 4885ec2..6a806a1 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -303,10 +303,13 @@ static struct genpd_onecell_data imx_gpc_onecell_data = {
>  
>  static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
>  {
> +       const struct of_device_id *of_id =
> +                       of_match_device(imx_gpc_dt_ids, dev);
> +       const struct imx_gpc_dt_data *of_id_data = of_id->data;
>         struct imx_pm_domain *domain;
>         int i, ret;
>  
> -       for (i = 0; i < 2; i++) {
> +       for (i = 0; i < of_id_data->num_domains; i++) {
>                 domain = &imx_gpc_domains[i];
>                 domain->regmap = regmap;
>                 domain->ipg_rate_mhz = 66;
> @@ -324,7 +327,7 @@ static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
>                 }
>         }
>  
> -       for (i = 0; i < 2; i++)
> +       for (i = 0; i < of_id_data->num_domains; i++)
>                 pm_genpd_init(&imx_gpc_domains[i].base, NULL, false);
>  
>         if (IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)) {
> @@ -337,7 +340,7 @@ static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
>         return 0;
>  
>  genpd_err:
> -       for (i = 0; i < 2; i++)
> +       for (i = 0; i < of_id_data->num_domains; i++)
>                 pm_genpd_remove(&imx_gpc_domains[i].base);
>         imx_pgc_put_clocks(&imx_gpc_domains[1]);
>  clk_err:
> 
> Do you think it's ok?
> 
I don't like the redundant of_match_id. Please just pass num_domains as
a parameter to imx_gpc_old_dt_init(). Otherwise this looks good.

Regards,
Lucas

[PATCH 1/8] soc: imx: gpc: fix gpc clk get error handling

[Dong Aisheng]

On Mon, Mar 20, 2017 at 04:05:24PM +0800, Shawn Guo wrote:
> On Mon, Mar 20, 2017 at 02:15:40PM +0800, Dong Aisheng wrote:
> > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> > index 1e9b3b8..c9bfdfd 100644
> > --- a/drivers/soc/imx/gpc.c
> > +++ b/drivers/soc/imx/gpc.c
> > @@ -143,7 +143,7 @@ static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain)
> >  	return 0;
> >  
> >  clk_err:
> > -	for (; i >= 0; i--)
> > +	while (i--)
> >  		clk_put(domain->clk[i]);
> 
> Will clk[0] be put then?
> 

Yes, it is a bit tricky:
See as follows:
while (i--)	<== (i = 1)
	clk_put(domain->clk[i]);	<=== (i = 0)

This code actually is used before until the commit below:
721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
So i did not change it.

Regards
Dong Aisheng

> Shawn
> 
> >  
> >  	return ret;

[PATCH 2/8] soc: imx: gpc: fix the wrong using of regmap cache

[Dong Aisheng]

Hi Lucas,

Thanks for the review.

On Mon, Mar 20, 2017 at 10:38:21AM +0100, Lucas Stach wrote:
> Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> > Without providing the proper reg_defaults, the regmap registers first
> > read out may be always 0 if enabling cache, which results in the
> > following issue we met.
> > e.g. During driver probe in imx6_pm_domain_power_on():
> > regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val);
> > The PGC_PUPSCR register val is always 0 but it's actually 0xf01 in HW.
> > 
> > Since GPC registers are tightly related to CPU bring up and may be
> > changed in bootloader, we don't want to provide defaults.
> > And the cache really does not save too much for GPC module.
> > 
> > Therefore, simply disable cache to fix the issue and make life easy.
> 
> While I agree that not using the cache may be the right thing to do
> here, bypassing the cache by not providing the volatile function is only
> half the fix.
> 

Removing volatile function actually is only due to it becomes meaningless
after disable cache, not used to bypass or fix the cache issue.

> Please also set the cache type to REGCACHE_NONE, to make it clear that
> the cache isn't used.
> 

I believe all guys normally may know the regmap cache is disabled by default
if without specifying cache_type, this way is also commonly used in kernel.
So i just simply remove it.
Another bonus is it fully hides the cache bits from drive user.
No one needs to know it if not use.

Anyway, if you insist on it, i could explicitly claim the cache type to
REGCACHE_NONE in V2.

Thanks

Regards
Dong Aisheng

> Regards,
> Lucas
> 
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  drivers/soc/imx/gpc.c | 10 ----------
> >  1 file changed, 10 deletions(-)
> > 
> > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> > index c9bfdfd..7e6a672 100644
> > --- a/drivers/soc/imx/gpc.c
> > +++ b/drivers/soc/imx/gpc.c
> > @@ -289,22 +289,12 @@ static bool imx_gpc_readable_reg(struct device *dev, unsigned int reg)
> >  	return (reg % 4 == 0) && (reg <= 0x2ac);
> >  }
> >  
> > -static bool imx_gpc_volatile_reg(struct device *dev, unsigned int reg)
> > -{
> > -	if (reg == GPC_CNTR)
> > -		return true;
> > -
> > -	return false;
> > -}
> > -
> >  static const struct regmap_config imx_gpc_regmap_config = {
> > -	.cache_type = REGCACHE_FLAT,
> >  	.reg_bits = 32,
> >  	.val_bits = 32,
> >  	.reg_stride = 4,
> >  
> >  	.readable_reg = imx_gpc_readable_reg,
> > -	.volatile_reg = imx_gpc_volatile_reg,
> >  
> >  	.max_register = 0x2ac,
> >  };
> 
> 

[PATCH 4/8] soc: imx: gpc: fix imx6sl gpc power domain regression

[Dong Aisheng]

On Mon, Mar 20, 2017 at 10:26:14AM +0100, Lucas Stach wrote:
> Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> > Commit 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> > broke the MX6SL GPC power domain support.
> > It always got the following error:
> > [    1.248364] imx-gpc 20dc000.gpc: could not find pgc DT node
> > This patch adds back the legecy support.
> 
> This patch removes a safeguard against abusing the old binding for new
> SoCs. Please leave this check in place and instead loosen the check to
> also allow the old i.MX6SL binding.
> 

Thanks for the reminder.
I found beside the checking, we probably also need some extra fix that
the current imx_gpc_old_dt_init() only supports two domains while the
driver formerly supports three for MX6SL.

And because the num_domains is defined by driver itself, no need check
in imx_gpc_old_dt_init() anymore.

The patch would be as follows:
diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index 4885ec2..6a806a1 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -303,10 +303,13 @@ static struct genpd_onecell_data imx_gpc_onecell_data = {
 
 static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
 {
+       const struct of_device_id *of_id =
+                       of_match_device(imx_gpc_dt_ids, dev);
+       const struct imx_gpc_dt_data *of_id_data = of_id->data;
        struct imx_pm_domain *domain;
        int i, ret;
 
-       for (i = 0; i < 2; i++) {
+       for (i = 0; i < of_id_data->num_domains; i++) {
                domain = &imx_gpc_domains[i];
                domain->regmap = regmap;
                domain->ipg_rate_mhz = 66;
@@ -324,7 +327,7 @@ static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
                }
        }
 
-       for (i = 0; i < 2; i++)
+       for (i = 0; i < of_id_data->num_domains; i++)
                pm_genpd_init(&imx_gpc_domains[i].base, NULL, false);
 
        if (IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)) {
@@ -337,7 +340,7 @@ static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
        return 0;
 
 genpd_err:
-       for (i = 0; i < 2; i++)
+       for (i = 0; i < of_id_data->num_domains; i++)
                pm_genpd_remove(&imx_gpc_domains[i].base);
        imx_pgc_put_clocks(&imx_gpc_domains[1]);
 clk_err:

Do you think it's ok?

Regards
Dong Aisheng

> > 
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  drivers/soc/imx/gpc.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> > index ba6e7ab..c1d0e67 100644
> > --- a/drivers/soc/imx/gpc.c
> > +++ b/drivers/soc/imx/gpc.c
> > @@ -385,12 +385,6 @@ static int imx_gpc_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	if (!pgc_node) {
> > -		/* old DT layout is only supported for mx6q aka 2 domains */
> > -		if (of_id_data->num_domains != 2) {
> > -			dev_err(&pdev->dev, "could not find pgc DT node\n");
> > -			return -ENODEV;
> > -		}
> > -
> >  		ret = imx_gpc_old_dt_init(&pdev->dev, regmap);
> >  		if (ret)
> >  			return ret;
> 
> 

[PATCH 8/8] soc: imx: gpc: remove unnecessary readable_reg callback

[Dong Aisheng]

On Mon, Mar 20, 2017 at 10:43:47AM +0100, Lucas Stach wrote:
> Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> > It is not really necessary to provide the current .readable_reg
> > implementation as we know what we're doing in our driver
> > and the regmap core has already done the partial check for
> > available maximum regs.
> > 
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> 
> Acked-by: Lucas Stach <l.stach@pengutronix.de>
> 

Thanks for the review and Acks.

Regards
Dong Aisheng

> > ---
> >  drivers/soc/imx/gpc.c | 8 --------
> >  1 file changed, 8 deletions(-)
> > 
> > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> > index a744f7e..4885ec2 100644
> > --- a/drivers/soc/imx/gpc.c
> > +++ b/drivers/soc/imx/gpc.c
> > @@ -284,18 +284,10 @@ static const struct of_device_id imx_gpc_dt_ids[] = {
> >  	{ }
> >  };
> >  
> > -static bool imx_gpc_readable_reg(struct device *dev, unsigned int reg)
> > -{
> > -	return (reg % 4 == 0) && (reg <= 0x2ac);
> > -}
> > -
> >  static const struct regmap_config imx_gpc_regmap_config = {
> >  	.reg_bits = 32,
> >  	.val_bits = 32,
> >  	.reg_stride = 4,
> > -
> > -	.readable_reg = imx_gpc_readable_reg,
> > -
> >  	.max_register = 0x2ac,
> >  };
> >  
> 
> 

[PATCH 4/8] soc: imx: gpc: fix imx6sl gpc power domain regression

[Dong Aisheng]

On Wed, Mar 22, 2017 at 09:46:10AM +0100, Lucas Stach wrote:
> Am Donnerstag, den 23.03.2017, 03:23 +0800 schrieb Dong Aisheng:
> > On Mon, Mar 20, 2017 at 10:26:14AM +0100, Lucas Stach wrote:
> > > Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> > > > Commit 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> > > > broke the MX6SL GPC power domain support.
> > > > It always got the following error:
> > > > [    1.248364] imx-gpc 20dc000.gpc: could not find pgc DT node
> > > > This patch adds back the legecy support.
> > > 
> > > This patch removes a safeguard against abusing the old binding for new
> > > SoCs. Please leave this check in place and instead loosen the check to
> > > also allow the old i.MX6SL binding.
> > > 
> > 
> > Thanks for the reminder.
> > I found beside the checking, we probably also need some extra fix that
> > the current imx_gpc_old_dt_init() only supports two domains while the
> > driver formerly supports three for MX6SL.
> > 
> > And because the num_domains is defined by driver itself, no need check
> > in imx_gpc_old_dt_init() anymore.
> > 
> > The patch would be as follows:
> > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> > index 4885ec2..6a806a1 100644
> > --- a/drivers/soc/imx/gpc.c
> > +++ b/drivers/soc/imx/gpc.c
> > @@ -303,10 +303,13 @@ static struct genpd_onecell_data imx_gpc_onecell_data = {
> >  
> >  static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
> >  {
> > +       const struct of_device_id *of_id =
> > +                       of_match_device(imx_gpc_dt_ids, dev);
> > +       const struct imx_gpc_dt_data *of_id_data = of_id->data;
> >         struct imx_pm_domain *domain;
> >         int i, ret;
> >  
> > -       for (i = 0; i < 2; i++) {
> > +       for (i = 0; i < of_id_data->num_domains; i++) {
> >                 domain = &imx_gpc_domains[i];
> >                 domain->regmap = regmap;
> >                 domain->ipg_rate_mhz = 66;
> > @@ -324,7 +327,7 @@ static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
> >                 }
> >         }
> >  
> > -       for (i = 0; i < 2; i++)
> > +       for (i = 0; i < of_id_data->num_domains; i++)
> >                 pm_genpd_init(&imx_gpc_domains[i].base, NULL, false);
> >  
> >         if (IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)) {
> > @@ -337,7 +340,7 @@ static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
> >         return 0;
> >  
> >  genpd_err:
> > -       for (i = 0; i < 2; i++)
> > +       for (i = 0; i < of_id_data->num_domains; i++)
> >                 pm_genpd_remove(&imx_gpc_domains[i].base);
> >         imx_pgc_put_clocks(&imx_gpc_domains[1]);
> >  clk_err:
> > 
> > Do you think it's ok?
> > 
> I don't like the redundant of_match_id. Please just pass num_domains as
> a parameter to imx_gpc_old_dt_init(). Otherwise this looks good.
> 

Yes, of course, will update it.

Thanks for the suggestion.

Regards
Dong Aisheng

> Regards,
> Lucas
>