Messages in this thread Patch in this message | | | From | Dong Aisheng <> | Date | Wed, 22 Jun 2022 18:12:49 +0800 | Subject | Re: [PATCH RFC 1/2] regmap: add option to disable debugfs |
| |
Hi Mark & Lucas,
[...]
> > > Furthermore, current imx blkctl is a common driver used by many devices > > [1]. > > > Introducing volatile registers and cache may bloat the driver a lot > > unnecessarily. > > > > You don't actually need to have a cache to use cache only mode, if there are > > no cached registers then you'll just get -EBUSY on any access to the registers > > but that's hopefully fine since at the minute things will just fall over anyway. > > You shouldn't even need to flag registers as volatile if there's no cache since it's > > not really relevant without a cache. > > > > After a quick try initially, I found two issues: > 1. There's a warning dump if using cache_only without cache > void regcache_cache_only(struct regmap *map, bool enable) > { > map->lock(map->lock_arg); > WARN_ON(map->cache_bypass && enable); > ... > } > 2. It seems _regmap_write() did not handle cache_only case well without cache. > It may still writes HW even for cache_only mode? > > I guess we may need fix those two issues first before we can safely use it? >
Below is a quick try fix which seems to work at my side by using cache_only mode suggested by Mark. I set cache_only mode in the bus power notifier rather than in blkctl power on/off callback in order to avoid possible cache mode setting contention.
NOTE: i didn't fix _regmap_write() as i.MX controls regmap write well in driver with power enabled first, so don't have issues in reality. It can be fixed in a separate patch later if needed. You may check if it's as your expected solution.
For syscon, I still have no idea how to fix it if I can't disable it.
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c index 2eaffd3224c9..da1702fd57cc 100644 --- a/drivers/base/regmap/regcache.c +++ b/drivers/base/regmap/regcache.c @@ -495,7 +495,7 @@ EXPORT_SYMBOL_GPL(regcache_drop_region); void regcache_cache_only(struct regmap *map, bool enable) { map->lock(map->lock_arg); - WARN_ON(map->cache_bypass && enable); +// WARN_ON(map->cache_bypass && enable); map->cache_only = enable; trace_regmap_cache_only(map, enable); map->unlock(map->lock_arg); diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c index 7ebc28709e94..12f0f9a24fad 100644 --- a/drivers/soc/imx/imx8m-blk-ctrl.c +++ b/drivers/soc/imx/imx8m-blk-ctrl.c @@ -199,6 +199,8 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev) return dev_err_probe(dev, PTR_ERR(bc->regmap), "failed to init regmap\n");
+ regcache_cache_only(bc->regmap, true); + bc->domains = devm_kcalloc(dev, bc_data->num_domains, sizeof(struct imx8m_blk_ctrl_domain), GFP_KERNEL); @@ -408,6 +410,8 @@ static int imx8mm_vpu_power_notifier(struct notifier_block *nb, */ udelay(5);
+ regcache_cache_only(bc->regmap, false); + /* set "fuse" bits to enable the VPUs */ regmap_set_bits(bc->regmap, 0x8, 0xffffffff); regmap_set_bits(bc->regmap, 0xc, 0xffffffff); @@ -415,6 +419,9 @@ static int imx8mm_vpu_power_notifier(struct notifier_block *nb, regmap_set_bits(bc->regmap, 0x14, 0xffffffff); }
+ if (action == GENPD_NOTIFY_PRE_OFF) + regcache_cache_only(bc->regmap, true); + return NOTIFY_OK; }
@@ -461,6 +468,9 @@ static int imx8mm_disp_power_notifier(struct notifier_block *nb, if (action != GENPD_NOTIFY_ON && action != GENPD_NOTIFY_PRE_OFF) return NOTIFY_OK;
+ if (action == GENPD_NOTIFY_ON) + regcache_cache_only(bc->regmap, false); + /* Enable bus clock and deassert bus reset */ regmap_set_bits(bc->regmap, BLK_CLK_EN, BIT(12)); regmap_set_bits(bc->regmap, BLK_SFT_RSTN, BIT(6)); @@ -473,6 +483,8 @@ static int imx8mm_disp_power_notifier(struct notifier_block *nb, if (action == GENPD_NOTIFY_ON) udelay(5);
+ if (action == GENPD_NOTIFY_PRE_OFF) + regcache_cache_only(bc->regmap, true);
return NOTIFY_OK; } @@ -531,6 +543,9 @@ static int imx8mn_disp_power_notifier(struct notifier_block *nb, if (action != GENPD_NOTIFY_ON && action != GENPD_NOTIFY_PRE_OFF) return NOTIFY_OK;
+ if (action == GENPD_NOTIFY_ON) + regcache_cache_only(bc->regmap, false); + /* Enable bus clock and deassert bus reset */ regmap_set_bits(bc->regmap, BLK_CLK_EN, BIT(8)); regmap_set_bits(bc->regmap, BLK_SFT_RSTN, BIT(8)); @@ -543,6 +558,8 @@ static int imx8mn_disp_power_notifier(struct notifier_block *nb, if (action == GENPD_NOTIFY_ON) udelay(5);
+ if (action == GENPD_NOTIFY_PRE_OFF) + regcache_cache_only(bc->regmap, true);
return NOTIFY_OK; } @@ -716,6 +733,9 @@ static int imx8mq_vpu_power_notifier(struct notifier_block *nb, if (action != GENPD_NOTIFY_ON && action != GENPD_NOTIFY_PRE_OFF) return NOTIFY_OK;
+ if (action == GENPD_NOTIFY_ON) + regcache_cache_only(bc->regmap, false); + /* * The ADB in the VPUMIX domain has no separate reset and clock * enable bits, but is ungated and reset together with the VPUs. The @@ -740,6 +760,9 @@ static int imx8mq_vpu_power_notifier(struct notifier_block *nb, regmap_set_bits(bc->regmap, 0x10, 0xffffffff); }
+ if (action == GENPD_NOTIFY_PRE_OFF) + regcache_cache_only(bc->regmap, true); + return NOTIFY_OK; }
Regards Aisheng
| |