Re: [PATCH v3 15/16] soc: mediatek: pm-domains: Add default power off flag

From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
To: Matthias Brugger <matthias.bgg@gmail.com>,
	Weiyi Lu <weiyi.lu@mediatek.com>
Cc: linux-kernel@vger.kernel.org, drinkcat@chromium.org,
	hsinyi@chromium.org, Collabora Kernel ML <kernel@collabora.com>,
	fparent@baylibre.com, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v3 15/16] soc: mediatek: pm-domains: Add default power off flag
Date: Fri, 30 Oct 2020 12:17:29 +0100	[thread overview]
Message-ID: <51cfbada-0ced-56d1-2836-8446fa8957b9@collabora.com> (raw)
In-Reply-To: <83197162-6bfc-b861-653e-4fd21035b5ae@gmail.com>

Hi Weilyi and Matthias,

On 29/10/20 15:51, Matthias Brugger wrote:
> 
> 
> On 27/10/2020 12:18, Weiyi Lu wrote:
>> On Tue, 2020-10-27 at 11:53 +0100, Matthias Brugger wrote:
>>>
>>> On 26/10/2020 18:55, Enric Balletbo i Serra wrote:
>>>> From: Weiyi Lu <weiyi.lu@mediatek.com>
>>>>
>>>> For some power domain, like conn on MT8192, it should be default OFF.
>>>> Because the power on/off control relies the function of connectivity chip
>>>> and its firmware. And if project choose other chip vendor solution,
>>>> those necessary connectivity functions will not provided.
>>>>
>>>> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>> ---
>>>>
>>>> Changes in v3: None
>>>> Changes in v2: None
>>>>
>>>>    drivers/soc/mediatek/mtk-pm-domains.c | 23 +++++++++++++++++------
>>>>    drivers/soc/mediatek/mtk-pm-domains.h |  1 +
>>>>    2 files changed, 18 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c
>>>> b/drivers/soc/mediatek/mtk-pm-domains.c
>>>> index 63993076a544..fe0e955076a0 100644
>>>> --- a/drivers/soc/mediatek/mtk-pm-domains.c
>>>> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
>>>> @@ -378,10 +378,16 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys
>>>> *scpsys, struct device_no
>>>>         * software.  The unused domains will be switched off during
>>>>         * late_init time.
>>>>         */
>>>> -    ret = scpsys_power_on(&pd->genpd);
>>>> -    if (ret < 0) {
>>>> -        dev_err(scpsys->dev, "%pOF: failed to power on domain: %d\n", node,
>>>> ret);
>>>> -        goto err_unprepare_clocks;
>>>> +    if (MTK_SCPD_CAPS(pd, MTK_SCPD_KEEP_DEFAULT_OFF)) {
>>>> +        if (scpsys_domain_is_on(pd))
>>>> +            dev_warn(scpsys->dev,
>>>> +                 "%pOF: A default off power domain has been ON\n", node);
>>>> +    } else {
>>>> +        ret = scpsys_power_on(&pd->genpd);
>>>> +        if (ret < 0) {
>>>> +            dev_err(scpsys->dev, "%pOF: failed to power on domain: %d\n",
>>>> node, ret);
>>>> +            goto err_unprepare_clocks;
>>>> +        }
>>>>        }
>>>>           if (scpsys->domains[id]) {
>>>> @@ -395,7 +401,11 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys
>>>> *scpsys, struct device_no
>>>>        pd->genpd.power_off = scpsys_power_off;
>>>>        pd->genpd.power_on = scpsys_power_on;
>>>>    -    pm_genpd_init(&pd->genpd, NULL, false);
>>>> +    if (MTK_SCPD_CAPS(pd, MTK_SCPD_KEEP_DEFAULT_OFF))
>>>> +        pm_genpd_init(&pd->genpd, NULL, true);
>>>> +    else
>>>> +        pm_genpd_init(&pd->genpd, NULL, false);
>>>> +
>>>>        scpsys->domains[id] = &pd->genpd;
>>>>           return scpsys->pd_data.domains[id];
>>>> @@ -478,7 +488,8 @@ static void scpsys_remove_one_domain(struct
>>>> scpsys_domain *pd)
>>>>                "failed to remove domain '%s' : %d - state may be
>>>> inconsistent\n",
>>>>                pd->genpd.name, ret);
>>>>    -    scpsys_power_off(&pd->genpd);
>>>> +    if (!MTK_SCPD_CAPS(pd, MTK_SCPD_KEEP_DEFAULT_OFF))
>>>> +        scpsys_power_off(&pd->genpd);
>>>
>>> OK, so you merged Weiyi's patches in this series :)
>>>
>>> So same comment here: Does it really hurt if we turn-off a already turned-off
>>> power domain? Or can we get rid of this check?
>>>
>>
>> We do need this check here. If you try to turn-off this power domain,
>> you might make the clock or regulator reference count unbalanced.
>>
> 
> Correct. Could we check via scpsys_domain_is_on() instead? I'd like to avoid
> MTK_SCPD_KEEP_DEFAULT_OFF use here.
> 

So, I'm using scpsys_domain_is_on() now for this check and moved before
pm_genpd_remove call. I'll let you both to decide if is fine or not.

> Regards,
> Matthias
> 
>>> Regards,
>>> Matthias
>>>
>>>>           clk_bulk_unprepare(pd->num_clks, pd->clks);
>>>>        clk_bulk_put(pd->num_clks, pd->clks);
>>>> diff --git a/drivers/soc/mediatek/mtk-pm-domains.h
>>>> b/drivers/soc/mediatek/mtk-pm-domains.h
>>>> index 2ad213be84a5..0fa6a938b40c 100644
>>>> --- a/drivers/soc/mediatek/mtk-pm-domains.h
>>>> +++ b/drivers/soc/mediatek/mtk-pm-domains.h
>>>> @@ -6,6 +6,7 @@
>>>>    #define MTK_SCPD_ACTIVE_WAKEUP        BIT(0)
>>>>    #define MTK_SCPD_FWAIT_SRAM        BIT(1)
>>>>    #define MTK_SCPD_SRAM_ISO        BIT(2)
>>>> +#define MTK_SCPD_KEEP_DEFAULT_OFF    BIT(3)
>>>>    #define MTK_SCPD_CAPS(_scpd, _x)    ((_scpd)->data->caps & (_x))
>>>>       #define SPM_VDE_PWR_CON            0x0210
>>>>
>>