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: <20220726102024.1.Icc838fe7bf0ef54a014ab2fee8af311654f5342a@changeid>
 <20220822193153.zn2oxljmd76awqot@halaneylaptop>
In-Reply-To: <20220822193153.zn2oxljmd76awqot@halaneylaptop>
From:   Doug Anderson <dianders@chromium.org>
Date:   Mon, 22 Aug 2022 13:13:55 -0700
Message-ID: <CAD=FV=V_V-M1fJmeWH_=wG4GB9GERL9ToAZTwAjX9i-6k6QkWA@mail.gmail.com>
Subject: Re: [PATCH] regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()
To:     Andrew Halaney <ahalaney@redhat.com>
Cc:     Mark Brown <broonie@kernel.org>, Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Mon, Aug 22, 2022 at 12:32 PM Andrew Halaney <ahalaney@redhat.com> wrote:
>
> Hey Douglas,
>
> On Tue, Jul 26, 2022 at 10:20:29AM -0700, Douglas Anderson wrote:
> > Since we don't actually pass the load to the firmware, switch to using
> > get_optimum_mode() instead of open-coding it.
> >
> > This is intended to have no effect other than cleanup.
>
> I hate to post something without looking into it very deeply, but with
> this statement about no effect I figured I'd report what I'm noticing
> before diving deeper.
>
> I'm currently playing with the dts patches in [0], and without this
> patch (and a hack patch applied that is unrelated to this) the board boots
> fine. With this patch applied I get the messages reported in [1] (which I
> think is still a clean up that should be applied) and the board fails to
> boot due to regulator_enable() failing at the first invocation I see.
>
> Is that something you expect?
>
> Here's the ultimate failure message for regulator_enable():
>
>     [    1.213419] ufshcd-qcom 1d84000.ufs: Adding to iommu group 0
>     [    1.219492] ufshcd-qcom 1d84000.ufs: ufshcd_populate_vreg: Unable to find vdd-hba-supply regulator, assuming enabled
>     [    1.230287] ufshcd-qcom 1d84000.ufs: ufshcd_populate_vreg: Unable to find vccq2-supply regulator, assuming enabled
>     [    1.241079] vreg_l17c: invalid input voltage found
>     [    1.246002] ufshcd-qcom 1d84000.ufs: ufshcd_enable_vreg: vcc enable failed, err=-22
>     [    1.253952] ufshcd-qcom 1d84000.ufs: Initialization failed
>     [    1.259622] ufshcd-qcom 1d84000.ufs: ufshcd_pltfrm_init() failed -22
>     [    1.266151] ufshcd-qcom: probe of 1d84000.ufs failed with error -22
>
> [0] https://lore.kernel.org/all/20220722143711.17563-1-quic_ppareek@quicinc.com/
> [1] https://lore.kernel.org/all/166118500944.215002.10320899094541954077.b4-ty@kernel.org/

Ah, I see what's happening.

When we were using set_load() all the regulator core did was call into
RPMH and tell it the load.

...but with the get_optimum_mode() the regulator core needs to also
pass the input and output voltages to RPMH as part of the API call
(even though RPMH ignores them). That means that if the input voltages
or output voltages are not known then it will error out before it even
calls to RPMH. This all might be made worse by the fact that RPMH
regulators boot up not knowing what their voltage is (see
"vreg->voltage_selector = -ENOTRECOVERABLE" in
rpmh_regulator_init_vreg()).

Ah, I guess it's failing to get the "input" voltage though. I guess
officially you could fix that by specifying the input supplies? I know
we never bothered doing that on any trogdor devices, though. If I
remember correctly all of the magic init that the firmware does for
RPMH already magically makes the input supplies get enabled / voltage
tuned properly so we decided that wasn't a benefit to modeling them in
Linux...

...hmmm, so I tried to figure out why I didn't see a failure and I
realized that in trogdor we never actually set
`regulator-allow-set-load` so basically we're not running this code at
all, so when I boot tested it on my hardware it wasn't terribly
useful. :(

I guess at this point I'll wait for Mark to give his suggestion for
what to do. Options I'm aware of:

a) ${SUBJECT} patch was written as a cleanup as per Mark's request and
we could just revert it.

b) We could make it so that failures to get the input/output voltages
doesn't count as an error when going through the get_optimum_mode()
path.


-Doug