Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220620134758.1286480-1-aisheng.dong@nxp.com>
 <20220620134758.1286480-2-aisheng.dong@nxp.com> <YrCM0reni+x/KWsG@sirena.org.uk>
 <DB9PR04MB84779EF2842D789FA66094C380B09@DB9PR04MB8477.eurprd04.prod.outlook.com>
 <YrCXILblKsp6DuN3@sirena.org.uk> <DB9PR04MB8477CD99D5847291A629994180B09@DB9PR04MB8477.eurprd04.prod.outlook.com>
 <YrCznap77OyHu4bO@sirena.org.uk> <DB9PR04MB847785E1861525FC1E4AD97280B39@DB9PR04MB8477.eurprd04.prod.outlook.com>
 <YrHkXH1M4NydBfQT@sirena.org.uk> <DB9PR04MB84772F73D4AA1B49F87BECCF80B39@DB9PR04MB8477.eurprd04.prod.outlook.com>
In-Reply-To: <DB9PR04MB84772F73D4AA1B49F87BECCF80B39@DB9PR04MB8477.eurprd04.prod.outlook.com>
From:   Dong Aisheng <dongas86@gmail.com>
Date:   Wed, 22 Jun 2022 18:12:49 +0800
Message-ID: <CAA+hA=RQVRHzdy_ZpwcC-ZB8mwUYuMYu_iLNAfuPcMCS1G9WXA@mail.gmail.com>
Subject: Re: [PATCH RFC 1/2] regmap: add option to disable debugfs
To:     Aisheng Dong <aisheng.dong@nxp.com>
Cc:     Mark Brown <broonie@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "l.stach@pengutronix.de" <l.stach@pengutronix.de>,
        Peng Fan <peng.fan@nxp.com>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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