Re: [RFT PATCH v2 2/2] regulator: core: Don't err if allow-set-load but no allowed-modes

From: Doug Anderson <dianders@chromium.org>
To: Andrew Halaney <ahalaney@redhat.com>
Cc: Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [RFT PATCH v2 2/2] regulator: core: Don't err if allow-set-load but no allowed-modes
Date: Thu, 25 Aug 2022 09:43:45 -0700	[thread overview]
Message-ID: <CAD=FV=Wf90u_DdczAeOqP8aXTO-CSei+9SGxytwS=M0LA9+w-g@mail.gmail.com> (raw)
In-Reply-To: <20220825151427.dux3luxs6sxx5tno@halaneylaptop>

Hi,

On Thu, Aug 25, 2022 at 8:14 AM Andrew Halaney <ahalaney@redhat.com> wrote:
>
> On Wed, Aug 24, 2022 at 02:22:57PM -0700, Douglas Anderson wrote:
> > Apparently the device trees of some boards have the property
> > "regulator-allow-set-load" for some of their regulators but then they
> > don't specify anything for "regulator-allowed-modes". That's not
> > really legit, but...
> >
> > ...before commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> > get_optimum_mode(), not set_load()") they used to get away with it, at
> > least on boards using RPMH regulators. That's because when a regulator
> > driver implements set_load() then the core doesn't look at
> > "regulator-allowed-modes" when trying to automatically adjust things
> > in response to the regulator's load. The core doesn't know what mode
> > we'll end up in, so how could it validate it?
> >
> > Said another way: before commit efb0cb50c427 ("regulator: qcom-rpmh:
> > Implement get_optimum_mode(), not set_load()") some boards _were_
> > having the regulator mode adjusted despite listing no allowed
> > modes. After commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> > get_optimum_mode(), not set_load()") these same boards were now
> > getting an error returned when trying to use their regulators, since
> > simply enabling a regulator tries to update its load and that was
> > failing.
> >
> > We don't really want to go back to the behavior from before commit
> > efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not
> > set_load()"). Boards shouldn't have been changing modes if no allowed
> > modes were listed. However, the behavior after commit efb0cb50c427
> > ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()")
> > isn't the best because now boards can't even turn their regulators on.
> >
> > Let's choose to detect this case and return "no error" from
> > drms_uA_update(). The net-result will be _different_ behavior than we
> > had before commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> > get_optimum_mode(), not set_load()"), but this new behavior seems more
> > correct. If a board truly needed the mode switched then its device
> > tree should be updated to list the allowed modes.
> >
> > Reported-by: Andrew Halaney <ahalaney@redhat.com>
> > Fixes: efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Tested-by: Andrew Halaney <ahalaney@redhat.com>
>
> As you made clear in the commit message, a good number of boards will
> have a change in behavior since efb0cb50c427 ("regulator: qcom-rpmh: Implement
> get_optimum_mode(), not set_load()") and associated fixes. I agree that
> these devices are not properly described. Is there any sort of heads up we
> should give? Just looking at the Qualcomm devicetrees for aarch64, I see all
> of these are affected:
>
>     apq8016-sbc.dts
>     apq8096-db820c.dts
>     apq8096-ifc6640.dts
>     msm8916-alcatel-idol347.dts
>     msm8916-asus-z00l.dts
>     msm8916-huawei-g7.dts
>     msm8916-longcheer-l8150.dts
>     msm8916-longcheer-l8910.dts
>     msm8916-samsung-a2015-common.dtsi
>     msm8916-samsung-j5.dts
>     msm8916-samsung-serranove.dts
>     msm8916-wingtech-wt88047.dts
>     msm8992-lg-bullhead.dtsi
>     msm8992-xiaomi-libra.dts
>     msm8994-msft-lumia-octagon.dtsi
>     msm8994-sony-xperia-kitakami.dtsi
>     msm8996-sony-xperia-tone.dtsi
>     msm8996-xiaomi-common.dtsi
>     msm8998-clamshell.dtsi
>     msm8998-fxtec-pro1.dts
>     msm8998-mtp.dts
>     msm8998-oneplus-common.dtsi
>     msm8998-sony-xperia-yoshino.dtsi
>     sa8155p-adp.dts
>     sa8xxxp-auto-adp.dtsi
>     sc8280xp-crd.dts
>     sc8280xp-lenovo-thinkpad-x13s.dts
>     sda660-inforce-ifc6560.dts
>     sdm630-sony-xperia-nile.dtsi
>     sdm660-xiaomi-lavender.dts
>     sm8150-sony-xperia-kumano.dtsi
>     sm8250-sony-xperia-edo.dtsi
>     sm8350-hdk.dts

True, it would be a good idea to send out fixes. OK, so let's see. We
can probably get fairly close to seeing who is affected with these
greps:

rpmh_users=$(git grep -l -i rpmh -- arch/arm*/boot/dts/qcom)
set_modes=$(grep -l regulator-allow-set-load ${rpmh_users})
but_no_allowed_modes=$(grep -l -v regulator-allowed-modes ${set_modes})

That actually gives a (much)shorter list than yours. Why?

Ah. Your list includes not just RPMH users but also RPM users. RPM
users _won't_ be affected. In RPM regulators we don't actually track
the modes in the kernel--we actually pass the load directly to the
remote processor and it handles translating that into loads. RPM
regulators don't even have a way to directly set the mode.

...so we only need to fix a small number (7) boards.

Posting up patches now. OK, the cover letter should show up shortly at:

https://lore.kernel.org/r/20220825164205.4060647-1-dianders@chromium.org

-Doug