Re: [PATCH RFC 1/2] regmap: add option to disable debugfs

[Dong Aisheng]

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