Date:   Fri, 29 Jul 2022 15:35:33 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Kuogee Hsieh <quic_khsieh@...cinc.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Vinod Koul <vkoul@...nel.org>, Mark Brown <broonie@...nel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
        Stephen Boyd <swboyd@...omium.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Andy Gross <agross@...nel.org>,
        "Abhinav Kumar (QUIC)" <quic_abhinavk@...cinc.com>,
        "Aravind Venkateswaran (QUIC)" <quic_aravindh@...cinc.com>,
        Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
        freedreno <freedreno@...ts.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

Sorry about the late follow-up on this. Has been one of those
constant-preemption weeks.

And thanks Doug and Mark for the insightful comments in this and the
dt-load RFC thread.

On Thu, Jul 21, 2022 at 07:49:55AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jul 21, 2022 at 6:25 AM Dmitry Baryshkov
> <dmitry.baryshkov@...aro.org> wrote:
> >
> > > This series breaks USB and PCIe for some SC8280XP and SA540P machines
> > > where the DP PHY regulators are shared with other PHYs whose drivers do
> > > not request a load.
> >
> > I'm trying to understand, why does this series cause the regression.
> > Previously it would be the DP controller voting on the regulators
> > load, now it has been moved to the DP/eDP PHYs.
> 
> I think in the past not many device trees actually hooked up the
> regulators to the DP/eDP but did hook up the regulators to the PHYs?
> That means they didn't used to get a regulator_set_load() on them and
> now they do?

This was the case for us, but this could probably also partly be blamed
on the DP stuff being work-in-progress and the regulators were only ever
specified in the PHY nodes.

> It should also be noted that we're now setting the load for a bunch of
> USB PHYs that we didn't used to set a load on...

That was my concern. But after skimming the mainline dts it seems that
no users of these drivers are currently allowing the regulator mode to
be changed for those particular regulators.

Well, except for the sc8280xp (derivate) machines in -next that I'm
working on and that regressed of course.

I guess we just need to drop all those regulator-allow-set-load
properties for now even if using DT for power-management configuration
this way does seem to run against the whole DT-as-hardware-description
idea (e.g. we may want to add them back when/if active- and idle loads
are specified by the corresponding Linux drivers).

> > > It seems quite likely that changes like this one affects other systems
> > > too, and the effects may be hard to debug. So a more general solution
> > > than playing whack-a-mole using DT would be good to have.
> >
> > I think enabling a regulator which supports set_load() and without
> > load being set should cause a warning, at least with
> > CONFIG_REGULATOR_DEBUG. WDYT?
> 
> I'm not a total fan. I'd love to see evidence to the contrary, but I'm
> a believer that the amount of extra cruft involved with all these
> regulator_set_load() calls is overkill for most cases. All the extra
> code / per-SoC tables added to drivers isn't ideal.
> 
> Every single LDO on Qualcomm's PMICs seems to be able to be set in
> "high power mode" and "low power mode", but I think the majority of
> clients only really care about two things: on and in high power mode
> vs. off. I think the amount of stuff peripherals can do in low power
> mode is super limited, so you have to be _really_ sure that the
> peripheral won't draw too much current without you having a chance to
> reconfigure the regulator.
> 
> Of course, if the load could be easily specified in the device tree
> and we could get rid of all the cruft in the drivers then maybe that
> would be OK.

It seems a decision was made against this (which I tend to agree with),
and that instead it is now easier for drivers to specify loads using
your updated bulk-regulator implementation.

But that doesn't address the problem that was trying to highlight here,
and that you had noticed years ago, namely that using set_load only
works reliably if *all* consumers use it.

Otherwise we may end up with a couple of drivers adding some 10 mA each to
the load, while other driver do not specify a load so that while the
actual load is really above the 30 mA HPM threshold, we still end up in
LPM.

Shouldn't an enabled regulator from a consumer that didn't specify a
load somehow result in HPM always being selected (e.g. count as INT_MAX
load as Doug suggested some years ago)?

Note that LPM may be ok until that last consumer that didn't specify a
load pushes it over the HPM threshold (so specifying a regulator base
load in DT would also prevent ever selecting LPM).

This case of mixing consumers that specify and don't specify loads might
at least warrant a warning otherwise to avoid ending up in a
hard-to-debug situation with failing USB and PCIe due to a seemingly
unrelated DP PHY update.

At some point in the discussion I thought Mark suggested removing
set_load from drivers that don't actually manage active and idle loads.
That would also work, at least until the day one of the drivers adds
support for idle loads.

Johan

Powered by blists - more mailing lists