soc: imx: gpc: fixes and clean up
Subsystems (2)
cover soc: imx: gpc: fixes and clean up
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(-)
Signature · 1 line
2.7.4
PATCH 1/8 soc: imx: gpc: fix gpc clk get error handling
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
--- drivers/soc/imx/gpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/soc/imx/gpc.c +1 -1
@@ -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;
Signature · 1 line
2.7.4
PATCH 1/8 soc: imx: gpc: fix gpc clk get error handling
drivers/soc/imx/gpc.c static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain) · line 147
Quoted 11 lines · Dong Aisheng
drivers/soc/imx/gpc.c +1
-1@@ -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
Quoted 2 lines
drivers/soc/imx/gpc.c +0
-0return ret;
PATCH 1/8 soc: imx: gpc: fix gpc clk get error handling
Quoted 8 lines · Shawn Guo
Quoted 7 lines · Dong Aisheng
drivers/soc/imx/gpc.c +0
-0@@ -143,7 +143,7 @@ static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain)return 0 ;
Quoted 7 lines
drivers/soc/imx/gpc.c static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain) · line 147
Quoted 4 lines
drivers/soc/imx/gpc.c +1
-1clk_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
Quoted 4 lines
Shawn
Quoted 2 lines
drivers/soc/imx/gpc.c +0
-0return ret;
PATCH 1/8 soc: imx: gpc: fix gpc clk get error handling
Quoted 17 lines · Dong Aisheng
Quoted 8 lines · Shawn Guo
Quoted 7 lines · Dong Aisheng
drivers/soc/imx/gpc.c +0
-0@@ -143,7 +143,7 @@ static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain)return 0 ;Quoted 7 lines
drivers/soc/imx/gpc.c static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain) · line 147
Quoted 4 lines
drivers/soc/imx/gpc.c +1
-1clk_err: -for (; i >=0 ; i--) +while (i--)clk_put (domain->clk[i]);Will clk[0] be put then?
Quoted 4 lines
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
In reply to Dong AishengWithout 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.
--- drivers/soc/imx/gpc.c | 10 ---------- 1 file changed, 10 deletions(-)
drivers/soc/imx/gpc.c +0 -10
@@ -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 , };
Signature · 1 line
2.7.4
PATCH 2/8 soc: imx: gpc: fix the wrong using of regmap cache
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 7 lines
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 inimx6_pm_domain_power_on():regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val); ThePGC_PUPSCRregister val is always 0 but it's actually0xf01in HW.
Quoted 5 lines
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
Quoted 35 lines
Cc Lucas Stach <[email protected]>Cc Shawn Guo <[email protected]>Fixes 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")Signed-off-by Dong Aisheng <[email protected]>--- drivers/soc/imx/gpc.c | 10 ---------- 1 file changed, 10 deletions(-)drivers/soc/imx/gpc.c +0
-10@@ -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 2/8 soc: imx: gpc: fix the wrong using of regmap cache
Hi Lucas,
Thanks for the review.
Quoted 12 lines · Lucas Stach
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 11 lines
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 inimx6_pm_domain_power_on():regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val); ThePGC_PUPSCRregister val is always 0 but it's actually0xf01in 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.
Quoted 6 lines
Quoted 1 line
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.
Quoted 3 lines
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
Quoted 40 lines
Regards,
LucasQuoted 35 lines
Cc Lucas Stach <[email protected]>Cc Shawn Guo <[email protected]>Fixes 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")Signed-off-by Dong Aisheng <[email protected]>--- drivers/soc/imx/gpc.c | 10 ---------- 1 file changed, 10 deletions(-)drivers/soc/imx/gpc.c +0
-10@@ -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 2/8 soc: imx: gpc: fix the wrong using of regmap cache
Am Donnerstag, den 23.03.2017, 02:53 +0800 schrieb Dong Aisheng:
Quoted 31 lines
Hi Lucas,
Thanks for the review.
Quoted 12 lines · Lucas Stach
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 11 lines
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 inimx6_pm_domain_power_on():regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val); ThePGC_PUPSCRregister val is always 0 but it's actually0xf01in 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.
Quoted 6 lines
Quoted 1 line
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.
Quoted 3 lines
Please also set the cache type to
REGCACHE_NONE, to make it clear that the cache isn't used.
Quoted 8 lines
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_NONEin V2.
Um, no thanks. I claim lack of coffee for my first review. This patch is
Quoted 38 lines
Quoted 38 lines
Quoted 36 lines
Cc Lucas Stach <[email protected]>Cc Shawn Guo <[email protected]>Fixes 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")Signed-off-by Dong Aisheng <[email protected]>--- drivers/soc/imx/gpc.c | 10 ---------- 1 file changed, 10 deletions(-)drivers/soc/imx/gpc.c +0
-10@@ -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
In reply to Dong AishengARRAY_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.
--- drivers/soc/imx/gpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/soc/imx/gpc.c +1 -1
@@ -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];
Signature · 1 line
2.7.4
PATCH 3/8 soc: imx: gpc: fix domain_index sanity check issue
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 4 lines
ARRAY_SIZE(imx_gpc_domains)represents all power domains supported by different SoCs. Driver should use SoC specificof_id_data->num_domainsinstead 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:
Quoted 21 lines
Cc Lucas Stach <[email protected]>Cc Shawn Guo <[email protected]>Fixes 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")Signed-off-by Dong Aisheng <[email protected]>--- drivers/soc/imx/gpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/soc/imx/gpc.c +1
-1@@ -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 4/8 soc: imx: gpc: fix imx6sl gpc power domain regression
In reply to Dong AishengCommit 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.
--- drivers/soc/imx/gpc.c | 6 ------ 1 file changed, 6 deletions(-)
drivers/soc/imx/gpc.c +0 -6
@@ -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;
Signature · 1 line
2.7.4
PATCH 4/8 soc: imx: gpc: fix imx6sl gpc power domain regression
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 5 lines
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 nodeThis 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.
Quoted 26 lines
Cc Lucas Stach <[email protected]>Cc Shawn Guo <[email protected]>Fixes 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")Signed-off-by Dong Aisheng <[email protected]>--- drivers/soc/imx/gpc.c | 6 ------ 1 file changed, 6 deletions(-)drivers/soc/imx/gpc.c +0
-6@@ -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 4/8 soc: imx: gpc: fix imx6sl gpc power domain regression
Quoted 11 lines · Lucas Stach
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 5 lines
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 nodeThis 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:
drivers/soc/imx/gpc.c +6 -3
@@ -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
Quoted 28 lines
Quoted 26 lines
Cc Lucas Stach <[email protected]>Cc Shawn Guo <[email protected]>Fixes 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")Signed-off-by Dong Aisheng <[email protected]>--- drivers/soc/imx/gpc.c | 6 ------ 1 file changed, 6 deletions(-)drivers/soc/imx/gpc.c +0
-6@@ -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 4/8 soc: imx: gpc: fix imx6sl gpc power domain regression
Am Donnerstag, den 23.03.2017, 03:23 +0800 schrieb Dong Aisheng:
Quoted 60 lines · Lucas Stach
Quoted 11 lines
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 5 lines
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 nodeThis 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 currentimx_gpc_old_dt_init()only supports two domains while the driver formerly supports three for MX6SL.And because the
num_domainsis defined by driver itself, no need check inimx_gpc_old_dt_init()anymore.The patch would be as follows:
drivers/soc/imx/gpc.c +6
-3@@ -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:
Quoted 2 lines
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 4/8 soc: imx: gpc: fix imx6sl gpc power domain regression
Quoted 61 lines · Lucas Stach
Am Donnerstag, den 23.03.2017, 03:23 +0800 schrieb Dong Aisheng:
Quoted 60 lines · Lucas Stach
Quoted 11 lines
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 5 lines
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 nodeThis 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 currentimx_gpc_old_dt_init()only supports two domains while the driver formerly supports three for MX6SL.And because the
num_domainsis defined by driver itself, no need check inimx_gpc_old_dt_init()anymore.The patch would be as follows:
drivers/soc/imx/gpc.c +6
-3@@ -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:
Quoted 5 lines
Quoted 2 lines
Do you think it's ok?
I don't like the redundant
of_match_id. Please just passnum_domainsas a parameter toimx_gpc_old_dt_init(). Otherwise this looks good.
Yes, of course, will update it.
Thanks for the suggestion.
Regards
Dong Aisheng
Quoted 3 lines
Regards,
Lucas
PATCH 5/8 soc: imx: gpc: fix comment when power up domain
In reply to Dong AishengThe correct comment should be power up domain.
--- drivers/soc/imx/gpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/soc/imx/gpc.c +1 -1
@@ -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 ;
Signature · 1 line
2.7.4
PATCH 5/8 soc: imx: gpc: fix comment when power up domain
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 5 lines
The correct comment should be power up domain.
Cc Lucas Stach <[email protected]>Cc Shawn Guo <[email protected]>Signed-off-by Dong Aisheng <[email protected]>
Quoted 17 lines
--- drivers/soc/imx/gpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/soc/imx/gpc.c +1
-1@@ -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
In reply to Dong AishengInstead of GPC_PGC_PDN_OFFS, naming it as GPC_PGC_CTRL_OFFS which is defined in reference manual for better reading.
--- drivers/soc/imx/gpc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/soc/imx/gpc.c +3 -3
@@ -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 */
Signature · 1 line
2.7.4
PATCH 6/8 soc: imx: gpc: keep PGC_X_CTRL name align with reference manual
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 6 lines
Instead of
GPC_PGC_PDN_OFFS, naming it asGPC_PGC_CTRL_OFFSwhich is defined in reference manual for better reading.Cc Lucas Stach <[email protected]>Cc Shawn Guo <[email protected]>Signed-off-by Dong Aisheng <[email protected]>
Quoted 35 lines
--- drivers/soc/imx/gpc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/soc/imx/gpc.c +3
-3@@ -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
In reply to Dong AishengActually 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.
--- Documentation/devicetree/bindings/power/fsl,imx-gpc.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/power/fsl,imx-gpc.txt +2 -2
@@ -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>;
Signature · 1 line
2.7.4
PATCH 7/8 dt-bindings: imx-gpc: correct the DOMAIN_INDEX using
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 7 lines
Actually
DOMAIN_INDEXis 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 <[email protected]>Cc Shawn Guo <[email protected]>Signed-off-by Dong Aisheng <[email protected]>
Thanks for the catch. Must have slipped through when changing the binding.
Quoted 26 lines
--- Documentation/devicetree/bindings/power/fsl,imx-gpc.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)Documentation/devicetree/bindings/power/fsl,imx-gpc.txt +2
-2@@ -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
In reply to Dong AishengIt 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.
--- drivers/soc/imx/gpc.c | 8 -------- 1 file changed, 8 deletions(-)
drivers/soc/imx/gpc.c +0 -8
@@ -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 , };
Signature · 1 line
2.7.4
PATCH 8/8 soc: imx: gpc: remove unnecessary readable_reg callback
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 8 lines
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 <[email protected]>Cc Shawn Guo <[email protected]>Signed-off-by Dong Aisheng <[email protected]>
Quoted 27 lines
--- drivers/soc/imx/gpc.c | 8 -------- 1 file changed, 8 deletions(-)drivers/soc/imx/gpc.c +0
-8@@ -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 8/8 soc: imx: gpc: remove unnecessary readable_reg callback
Quoted 6 lines · Lucas Stach
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
Quoted 5 lines
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.
Quoted 6 lines
Quoted 3 lines
Cc Lucas Stach <[email protected]>Cc Shawn Guo <[email protected]>Signed-off-by Dong Aisheng <[email protected]>Acked-by Lucas Stach <[email protected]>
Thanks for the review and Acks.
Regards
Dong Aisheng
Quoted 29 lines
Quoted 27 lines
--- drivers/soc/imx/gpc.c | 8 -------- 1 file changed, 8 deletions(-)drivers/soc/imx/gpc.c +0
-8@@ -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 , };