Re: [RFC v1 0/4] Simplify regulator supply resolution code by offloading to driver core

[Saravana Kannan]

On Mon, Feb 20, 2023 at 1:02 AM Marek Szyprowski
<m.szyprowski@xxxxxxxxxxx> wrote:
>
> Hi Saravana,
>
> On 18.02.2023 09:32, Saravana Kannan wrote:
> > Hi Mark/Liam,
> >
> > This series is just an RFC to see if you agree with where this is going.
> > Please point out bugs, but don't bother with a proper code review.
> >
> > The high level idea is to not reimplement what driver core can already
> > handle for us and use it to do some of the work. Instead of trying to
> > resolve supplies from all different code paths and bits and pieces of
> > the tree, we just build it from the root to the leaves by using deferred
> > probing to sequence things in the right order.
> >
> > The last patch is the main one. Rest of them are just setting up for it.
> >
> > I believe there's room for further simplification but this is what I
> > could whip up as a quick first draft that shows the high level idea.
> > I'll probably need some help with getting a better understanding of why
> > things are done in a specific order in regulator_register() before I
> > could attempt simplifying things further.
> >
> > Ideally, regulator_register() would just have DT parsing, init data
> > struct sanity checks and adding the regulator device and then we move
> > everything else to into the probe function that's guaranteed to run only
> > after the supply has been resolved/ready to resolve.
> >
> > fw_devlink/device links should further optimize the flow and also allow
> > us to simplify some of the guarantees and address some of the existing
> > FIXMEs. But this patch series is NOT dependent on fw_devlink or device
> > links.
> >
> > Any thoughts on where this is going?
> >
> > I've tested this on one hardware I have and it works and nothing is
> > broken. But the regulator tree in my hardware isn't that complicated or
> > deep. The regulators are also added mostly in the right order (due to
> > existing fw_devlink). So if you agree with the idea, the next step is to
> > ask people to give it a test.
> >
> > Also, it's based on driver-core-next since that's what I had synced up
> > and had a working baseline. I'll rebase it on the regulator tree when I
> > go from RFC -> PATCH.
>
> I've applied this patchset on top of linux next-20230220 and gave it a
> try on my test farm, as it revealed a few issues related to regulator
> initialization in the past. It looks that handling of some corner cases
> is missing, because this patchset introduced a regression on Samsung
> Snow/Peach-Pit/Peach-Pi Chromebooks, as well as Hardkernel's Odroid-M1
> board. It looks that the issue is common - PHY devices don't probe
> properly. This is an output from Odroid-M1 board
> (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts):
>
> # cat /sys/kernel/debug/devices_deferred 2>/dev/null
> fd8c0000.usb    platform: wait for supplier host-port
> fe830000.phy
> fe8a0000.usb2phy        rockchip-usb2phy: failed to create phy
> fe8b0000.usb2phy        rockchip-usb2phy: failed to create phy
> 3c0800000.pcie  rockchip-dw-pcie: failed to get vpcie3v3 regulator
> fcc00000.usb    platform: wait for supplier otg-port
> fd000000.usb    platform: wait for supplier host-port
> fd800000.usb    platform: wait for supplier otg-port
> fd840000.usb    platform: wait for supplier otg-port
> fd880000.usb    platform: wait for supplier host-port
> fe820000.phy
>
> If you need any additional tests on the mentioned boards, let me know.

Thanks for testing it Marek! I don't want people to spend more time
testing this before I hear Mark/Liam's thoughts. So, let's hold off
for now.

I took a peek at the dts and the logs above. If you go into
/sys/bus/regulator/devices/, I'd expect all of them to have probed
(they'll have a "driver" symlink in their folder). Or at least the
regulator tree used by the phys.

My first guess is that deferred probe handling might be broken
somewhere in the USB/phy framework where they aren't able to handle
regulator_get() returning -EPROBE_DEFER. Looks like my patch is
delaying some reglator_get() from passing. I'll look closer into
avoiding this after Mark/Liam approve the general idea behind this
patch.

-Saravana

>
>
> > Thanks,
> > Saravana
> >
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > Cc: Bjorn Andersson <andersson@xxxxxxxxxx>
> > Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> > Cc: Tony Lindgren <tony@xxxxxxxxxxx>
> > Cc: Doug Anderson <dianders@xxxxxxxxxxxx>
> > Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
> > Cc: Luca Weiss <luca.weiss@xxxxxxxxxxxxx>
> >
> > Saravana Kannan (4):
> >    regulator: core: Add regulator devices to bus instead of class
> >    regulator: core: Add sysfs class backward compatibility
> >    regulator: core: Probe regulator devices
> >    regulator: core: Move regulator supply resolving to the probe function
> >
> >   drivers/regulator/core.c         | 102 +++++++++++++++++++------------
> >   drivers/regulator/internal.h     |   2 +-
> >   drivers/regulator/of_regulator.c |   2 +-
> >   3 files changed, 64 insertions(+), 42 deletions(-)
> >
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>