Re: [PATCH 4/9] regulator: mt6358: Drop *_SSHUB regulators

From: AngeloGioacchino Del Regno  <angelogioacchino.delregno@collabora.com>
To: Chen-Yu Tsai <wenst@chromium.org>
Cc: Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Gene Chen <gene_chen@richtek.com>,
	Johnson Wang <johnson.wang@mediatek.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/9] regulator: mt6358: Drop *_SSHUB regulators
Date: Wed, 14 Jun 2023 09:39:04 +0200	[thread overview]
Message-ID: <1d4ac52a-4cc8-a346-b005-f5cebc8d9eb8@collabora.com> (raw)
In-Reply-To: <CAGXv+5FskpxjATk5aJA4CG0GyarO55XXYSYkvxWd-40d4ML8RQ@mail.gmail.com>

Il 12/06/23 06:45, Chen-Yu Tsai ha scritto:
> On Fri, Jun 9, 2023 at 5:03 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 09/06/23 10:30, Chen-Yu Tsai ha scritto:
>>> The *_SSHUB regulators are actually alternate configuration interfaces
>>> for their non *_SSHUB counterparts. They are not separate regulator
>>> outputs. These registers are intended for the companion processor to
>>> use to configure the power rails while the main processor is sleeping.
>>> They are not intended for the main operating system to use.
>>>
>>> Since they are not real outputs they shouldn't be modeled separately.
>>> Remove them. Luckily no device tree actually uses them.
>>>
>>
>> I'm not sure that MT6358/6366 are used only on Chromebook SoCs, and that this SSHUB
>> mechanism (companion processor) is the same across all firmwares.
> 
> AFAICT from Internet sources there's also the MT6771 and MT6781, which
> are used on some phones.
> 
> But what part are you concerned about? The upstream regulator driver does
> not actually have any code to switch to/from normal operation and SSHUB
> mode.
> 
> In a downstream kernel I found that the SSHUB mode is only used if SCP is
> doing DVFS [1]. In that same kernel, the regulator driver [2] doesn't even
> list the *_SSHUB versions. So if SCP DVFS is used, the regulator driver
> has no idea what's going on, and can't interfere either, which I think is
> actually a good thing. Only one side should have complete control of one
> output.
> 

Ok, I'm sold! :-P

Jokes apart, thanks for clarifying. At this point, I agree with you in that this
is safe to do, so:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

P.S.: Sorry for the late reply and thank you for the links to that old
downstream kernel.

Cheers,
Angelo

> ChenYu
> 
> [1] https://github.com/nokia-dev/android_kernel_nokia_mt6771/blob/android-9.0/drivers/misc/mediatek/scp/mt6771/scp_dvfs.c
> [2] https://github.com/nokia-dev/android_kernel_nokia_mt6771/blob/android-9.0/drivers/misc/mediatek/pmic/mt6358/v1/regulator_codegen.c
> 
>> I'd like someone from MediaTek to confirm that this is valid for both Chromebook
>> and Smartphone firmwares.
>>
>> Regards,
>> Angelo
>>
>>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>>> ---
>>>    drivers/regulator/mt6358-regulator.c       | 14 --------------
>>>    include/linux/regulator/mt6358-regulator.h |  4 ----
>>>    2 files changed, 18 deletions(-)
>>>
>>> diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
>>> index faf6b0757019..946a251a8b3a 100644
>>> --- a/drivers/regulator/mt6358-regulator.c
>>> +++ b/drivers/regulator/mt6358-regulator.c
>>> @@ -505,9 +505,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
>>>        MT6358_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
>>>                    buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
>>>                    MT6358_VCORE_VGPU_ANA_CON0, 1),
>>> -     MT6358_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
>>> -                 buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
>>> -                 MT6358_VCORE_VGPU_ANA_CON0, 1),
>>>        MT6358_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
>>>                    buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
>>>                    MT6358_VPA_ANA_CON0, 3),
>>> @@ -583,10 +580,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
>>>        MT6358_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
>>>                    buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
>>>                    MT6358_LDO_VSRAM_CON2, 0x7f),
>>> -     MT6358_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
>>> -                 1293750, 6250, buck_volt_range1,
>>> -                 MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
>>> -                 MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
>>>        MT6358_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
>>>                    buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
>>>                    MT6358_LDO_VSRAM_CON3, 0x7f),
>>> @@ -603,9 +596,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
>>>        MT6366_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
>>>                    buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
>>>                    MT6358_VCORE_VGPU_ANA_CON0, 1),
>>> -     MT6366_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
>>> -                 buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
>>> -                 MT6358_VCORE_VGPU_ANA_CON0, 1),
>>>        MT6366_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
>>>                    buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
>>>                    MT6358_VPA_ANA_CON0, 3),
>>> @@ -670,10 +660,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
>>>        MT6366_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
>>>                    buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
>>>                    MT6358_LDO_VSRAM_CON2, 0x7f),
>>> -     MT6366_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
>>> -                 1293750, 6250, buck_volt_range1,
>>> -                 MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
>>> -                 MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
>>>        MT6366_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
>>>                    buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
>>>                    MT6358_LDO_VSRAM_CON3, 0x7f),
>>> diff --git a/include/linux/regulator/mt6358-regulator.h b/include/linux/regulator/mt6358-regulator.h
>>> index a4307cd9edd6..c71a6a9fce7a 100644
>>> --- a/include/linux/regulator/mt6358-regulator.h
>>> +++ b/include/linux/regulator/mt6358-regulator.h
>>> @@ -47,8 +47,6 @@ enum {
>>>        MT6358_ID_VLDO28,
>>>        MT6358_ID_VAUD28,
>>>        MT6358_ID_VSIM2,
>>> -     MT6358_ID_VCORE_SSHUB,
>>> -     MT6358_ID_VSRAM_OTHERS_SSHUB,
>>>        MT6358_ID_RG_MAX,
>>>    };
>>>
>>> @@ -88,8 +86,6 @@ enum {
>>>        MT6366_ID_VMC,
>>>        MT6366_ID_VAUD28,
>>>        MT6366_ID_VSIM2,
>>> -     MT6366_ID_VCORE_SSHUB,
>>> -     MT6366_ID_VSRAM_OTHERS_SSHUB,
>>>        MT6366_ID_RG_MAX,
>>>    };
>>>
>>