Re: [PATCH] arm64: defconfig: enable regulator to fix mt8173 regression

[AngeloGioacchino Del Regno]

Il 19/10/21 16:38, AngeloGioacchino Del Regno ha scritto:
> Il 11/10/21 14:53, Adrian Ratiu ha scritto:
> > A regression was introduced on some mediatek boards starting with
> > v5.15-rc1 in commit 109fd20601e2b ("arm64: dts: mediatek: mt8173:
> > Add domain supply for mfg_async") which effectively changed the
> > regulator from the always-on dummy to DA9211 without explicitely
> > enabling it, causing failures like the these caught by KernelCI
> > on Hana Chromebooks [1]:
> >
> > mtk-power-controller 10006000.syscon:power-controller: supply domain not found, 
> > using dummy regulator
> > mtu3 11271000.usb: supply vbus not found, using dummy regulator
> > xhci-mtk 11270000.usb: supply vbus not found, using dummy regulator
> >
> > There might be another bug linking these power domains in the
> > mediatek PM driver, but that is a separate issue wich needs
> > addressing, for now just fix the obvious regression due to the
> > new regulator requirement.
> >
> > [1] https://github.com/kernelci/kernelci-project/issues/66
> > Reported-by: "kernelci.org bot" <bot@xxxxxxxxxxxx>
> > Cc: Guillaume Tucker <guillaume.tucker@xxxxxxxxxxxxx>
> > Suggested-by: Enric Balletbo Serra <eballetbo@xxxxxxxxx>
> > Signed-off-by: Adrian Ratiu <adrian.ratiu@xxxxxxxxxxxxx>
> > ---
> >   arch/arm64/configs/defconfig | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> > index 156d96afbbfc..4901cc1213bb 100644
> > --- a/arch/arm64/configs/defconfig
> > +++ b/arch/arm64/configs/defconfig
> > @@ -616,6 +616,7 @@ CONFIG_REGULATOR_FIXED_VOLTAGE=y
> >   CONFIG_REGULATOR_AXP20X=y
> >   CONFIG_REGULATOR_BD718XX=y
> >   CONFIG_REGULATOR_BD9571MWV=y
> > +CONFIG_REGULATOR_DA9211=y
> >   CONFIG_REGULATOR_FAN53555=y
> >   CONFIG_REGULATOR_GPIO=y
> >   CONFIG_REGULATOR_HI6421V530=y
>
> Hello,
>
> I've been able to get a working Hana boot, with USB probed as early as possible, as
> well solving that KernelCI failure (as now also the usb network works fine again).
>
> My proposal here, for which I have a patch that's almost ready, would be to enable
> this regulator driver as a module instead (since Hana is the only device that's
> using it), avoiding to increase the kernel image size for everyone.
>
>
> Before pointing out my solution, let's first point out what's going on:
>
> In mt8173.dtsi, we have a power-controller node (mediatek,mt8173-power-controller),
> under which all of the SoC's power domain nodes are defined. In this node, we have
> both SCPD_DOMAIN_SUPPLY domains and "regular" ones.
>
> The difference between SCPD domains and the others is that the first ones require a
> parent regulator, while the latter don't (power is supplied from some .. internal
> supply? - either way, no parent vreg necessary/declared).
> As a note, for now, the only two MediaTek SoCs that have a SCPD supply are MT8173
> and MT8183... and nothing else, as the others, including the newer ones seem to
> have no such supplies (the only newer one upstream is MT8192 and has none).
>
>
> My solution was to split the power-controller node in two:
> 1. spm: power-controller@0 - contains all of the non-SCPD power domains
> 2. spm_scpd: power-controller@1 - contains the SCPD power domains.
>
> This made me able to get a full boot without usb/usb-eth issues while enabling this
> regulator as a module; this also requires us to change the
> mediatek,power-controller.yaml binding to allow multiple instances of that driver,
> which is anyway already permitted by the mtk-pm-domains driver itself.
>
>
> Hence, this question comes up: how should we proceed? should we...
> a. enable this regulator driver as module and split the power-controller in two; or
> b. keep this commit enabling this driver built-in and still split the
>     power-controller nodes; or
> c. just enable this driver as built-in and not care about declaring two power
>     controller nodes?
>
> Can you please give us an advice?
>
> Thank you,
> - Angelo

After a discussion on this topic, we chose to pursue option B, as enabling this
regulator fixes a very bad regression. Splitting the power-controller nodes does
require a bit of time due to some more research that has to be done on that topic.

Adrian will follow with a v2 of this patch, adding a Fixes tag.

Thanks everyone,
- Angelo