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

  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.

Thanks,
Saravana

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Bjorn Andersson <andersson@kernel.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Luca Weiss <luca.weiss@fairphone.com>

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(-)
  

On Sat, Feb 18, 2023 at 12:32 AM Saravana Kannan <saravanak@google.com> 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.
>
> Thanks,
> Saravana
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Luca Weiss <luca.weiss@fairphone.com>

Christian, I was meaning to add you but I forgot. I'll add you to
future versions.

-Saravana

>
> 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(-)
>
> --
> 2.39.2.637.g21b0678d19-goog
>
  

On Sat, Feb 18, 2023 at 12:32:47AM -0800, 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.

At first glance, this looks sane to me, thanks for doing this work!

greg k-h
  

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,
> Saravana
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Luca Weiss <luca.weiss@fairphone.com>
>
> 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
  

On Mon, Feb 20, 2023 at 1:02 AM Marek Szyprowski
<m.szyprowski@samsung.com> 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@linuxfoundation.org>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > Cc: Bjorn Andersson <andersson@kernel.org>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Doug Anderson <dianders@chromium.org>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: Luca Weiss <luca.weiss@fairphone.com>
> >
> > 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
>
  

On Tue, Feb 21, 2023 at 02:36:52PM -0800, Saravana Kannan wrote:

> 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.

My main thought right now is that I'm not going to think about it
too hard if it doesn't work correctly...
  

On Tue, Feb 21, 2023 at 2:52 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Feb 21, 2023 at 02:36:52PM -0800, Saravana Kannan wrote:
>
> > 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.
>
> My main thought right now is that I'm not going to think about it
> too hard if it doesn't work correctly...

:( I'm not asking for a thorough code review. Just if you are okay
with the idea/approach of pushing the ordering logic to driver core to
avoid reimplementing what's already available and avoiding some races
in the regulator code (stuff like, checking if some other thread
resolved a supply while you were working on it). The patch at least
works on my device and works for most regulators in Marek's devices.
So, it's not a complete broken mess :)

On a separate note, I have some questions about setting machine
constraints during regulator_register(). Why do we even try to set
machine constraints before a regulator's supply is resolved? None of
the consumers can make any requests anyway. So what else is going to
need those constraints? Wouldn't the regulator just be in whatever
state the bootloader left it at?

The current logic is something like:

1. Try to resolve supply if it's always on or a boot on regulator.
2. Set machine constraints -- this might fail for multiple reasons.
One of them being unresolved supply.
3. If it failed due to unresolved supply, but it wasn't resolved in step 1.
3. a. Try to resolve supply,
3. b. If 3.a. didn't fail, try to set machine constraints.
3. c. If 3.b failed, fail registration.

Why isn't this just:
1. Try to resolve supply (for all regulators).
2. If we are able to resolve supply set machine constraints.
3. If we weren't able to resolve supply, set machine constraints when
we resolve supply in the future?

Or if you need to set machine constraints without waiting for supply,
then why not at least:

1. Try to resolve supply (for all regulators).
2. Set machine constraints.
3. When we resolve supply in the future, do whatever remaining bits
that you need to do.

Thanks,
Saravana
  

On 21.02.2023 23:36, Saravana Kannan wrote:
> On Mon, Feb 20, 2023 at 1:02 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 18.02.2023 09:32, Saravana Kannan wrote:
>>> 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.

Nope, something is wrong there with those PHY regulators:

# cd /sys/bus/regulator/devices

# ls -l regulator.4
lrwxrwxrwx 1 root root 0 Feb 14  2019 regulator.4 -> 
../../../devices/platform/vcc5v0-usb-host-regulator/regulator.4

# ls -l regulator.4/driver
ls: cannot access 'regulator.4/driver': No such file or directory

# ls -l regulator.4/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:06 consumer:platform:fe820000.phy 
-> 
../../virtual/devlink/platform:vcc5v0-usb-host-regulator--platform:fe820000.phy
lrwxrwxrwx 1 root root    0 Feb 22 07:06 
consumer:platform:fe8a0000.usb2phy -> 
../../virtual/devlink/platform:vcc5v0-usb-host-regulator--platform:fe8a0000.usb2phy
lrwxrwxrwx 1 root root    0 Feb 22 07:06 
consumer:platform:fe8b0000.usb2phy -> 
../../virtual/devlink/platform:vcc5v0-usb-host-regulator--platform:fe8b0000.usb2phy
lrwxrwxrwx 1 root root    0 Feb 22 07:06 driver -> 
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:06 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:06 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:06 of_node -> 
../../../firmware/devicetree/base/vcc5v0-usb-host-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:06 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.4
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
lrwxrwxrwx 1 root root    0 Feb 22 07:06 supplier:platform:fdd60000.gpio 
-> 
../../virtual/devlink/platform:fdd60000.gpio--platform:vcc5v0-usb-host-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:06 supplier:platform:pinctrl -> 
../../virtual/devlink/platform:pinctrl--platform:vcc5v0-usb-host-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:06 
supplier:platform:vcc5v0-sys-regulator -> 
../../virtual/devlink/platform:vcc5v0-sys-regulator--platform:vcc5v0-usb-host-regulator
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent

# ls -l regulator.5
lrwxrwxrwx 1 root root 0 Feb 14  2019 regulator.5 -> 
../../../devices/platform/vcc5v0-usb-otg-regulator/regulator.5

# ls -l regulator.5/driver
ls: cannot access 'regulator.5/driver': No such file or directory

# ls -l regulator.5/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:09 consumer:platform:fe830000.phy 
-> 
../../virtual/devlink/platform:vcc5v0-usb-otg-regulator--platform:fe830000.phy
lrwxrwxrwx 1 root root    0 Feb 22 07:09 
consumer:platform:fe8a0000.usb2phy -> 
../../virtual/devlink/platform:vcc5v0-usb-otg-regulator--platform:fe8a0000.usb2phy
lrwxrwxrwx 1 root root    0 Feb 22 07:07 driver -> 
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:09 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:09 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:09 of_node -> 
../../../firmware/devicetree/base/vcc5v0-usb-otg-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:09 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.5
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
lrwxrwxrwx 1 root root    0 Feb 22 07:09 supplier:platform:fdd60000.gpio 
-> 
../../virtual/devlink/platform:fdd60000.gpio--platform:vcc5v0-usb-otg-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:09 supplier:platform:pinctrl -> 
../../virtual/devlink/platform:pinctrl--platform:vcc5v0-usb-otg-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:09 
supplier:platform:vcc5v0-sys-regulator -> 
../../virtual/devlink/platform:vcc5v0-sys-regulator--platform:vcc5v0-usb-otg-regulator
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent

# ls -l regulator.23
lrwxrwxrwx 1 root root 0 Feb 14  2019 regulator.23 -> 
../../../devices/platform/vcc3v3-pcie-regulator/regulator.23

# ls -l regulator.23/driver
ls: cannot access 'regulator.23/driver': No such file or directory

# ls -l regulator.23/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:10 
consumer:platform:3c0800000.pcie -> 
../../virtual/devlink/platform:vcc3v3-pcie-regulator--platform:3c0800000.pcie
lrwxrwxrwx 1 root root    0 Feb 22 07:07 driver -> 
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:10 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:10 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:10 of_node -> 
../../../firmware/devicetree/base/vcc3v3-pcie-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:10 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.23
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
lrwxrwxrwx 1 root root    0 Feb 22 07:10 supplier:platform:fe770000.gpio 
-> 
../../virtual/devlink/platform:fe770000.gpio--platform:vcc3v3-pcie-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:10 supplier:platform:pinctrl -> 
../../virtual/devlink/platform:pinctrl--platform:vcc3v3-pcie-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:10 
supplier:platform:vcc3v3-sys-regulator -> 
../../virtual/devlink/platform:vcc3v3-sys-regulator--platform:vcc3v3-pcie-regulator
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent


However other fixed regulator devices have been probed properly:

# ls -l regulator.3/driver
lrwxrwxrwx 1 root root 0 Feb 22 07:10 regulator.3/driver -> 
../../../../bus/regulator/drivers/regulator_drv
# ls -l regulator.3/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:10 
consumer:platform:vcc5v0-usb-host-regulator -> 
../../virtual/devlink/platform:vcc5v0-sys-regulator--platform:vcc5v0-usb-host-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:10 
consumer:platform:vcc5v0-usb-otg-regulator -> 
../../virtual/devlink/platform:vcc5v0-sys-regulator--platform:vcc5v0-usb-otg-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:10 driver -> 
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:10 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:10 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:10 of_node -> 
../../../firmware/devicetree/base/vcc5v0-sys-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:10 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.3
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
lrwxrwxrwx 1 root root    0 Feb 22 07:10 
supplier:platform:dc-12v-regulator -> 
../../virtual/devlink/platform:dc-12v-regulator--platform:vcc5v0-sys-regulator
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent

# ls -l regulator.2/driver
lrwxrwxrwx 1 root root 0 Feb 22 07:13 regulator.2/driver -> 
../../../../bus/regulator/drivers/regulator_drv
# ls -l regulator.2/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:13 consumer:i2c:3-001c -> 
../../virtual/devlink/platform:vcc3v3-sys-regulator--i2c:3-001c
lrwxrwxrwx 1 root root    0 Feb 22 07:13 consumer:i2c:3-0020 -> 
../../virtual/devlink/platform:vcc3v3-sys-regulator--i2c:3-0020
lrwxrwxrwx 1 root root    0 Feb 22 07:13 
consumer:platform:fe2a0000.ethernet -> 
../../virtual/devlink/platform:vcc3v3-sys-regulator--platform:fe2a0000.ethernet
lrwxrwxrwx 1 root root    0 Feb 22 07:13 
consumer:platform:vcc3v3-pcie-regulator -> 
../../virtual/devlink/platform:vcc3v3-sys-regulator--platform:vcc3v3-pcie-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:13 driver -> 
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:13 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:13 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:13 of_node -> 
../../../firmware/devicetree/base/vcc3v3-sys-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:13 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.2
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
lrwxrwxrwx 1 root root    0 Feb 22 07:13 
supplier:platform:dc-12v-regulator -> 
../../virtual/devlink/platform:dc-12v-regulator--platform:vcc3v3-sys-regulator
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent

# ls -l regulator.1/driver
lrwxrwxrwx 1 root root 0 Feb 22 07:14 regulator.1/driver -> 
../../../../bus/regulator/drivers/regulator_drv
# ls -l regulator.1/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:14 
consumer:platform:vcc3v3-sys-regulator -> 
../../virtual/devlink/platform:dc-12v-regulator--platform:vcc3v3-sys-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:14 
consumer:platform:vcc5v0-sys-regulator -> 
../../virtual/devlink/platform:dc-12v-regulator--platform:vcc5v0-sys-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:14 driver -> 
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:14 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:14 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:14 of_node -> 
../../../firmware/devicetree/base/dc-12v-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:14 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.1
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent


I hope the above logs will help identifying the issue.


Best regards
  

On Tue, Feb 21, 2023 at 07:13:39PM -0800, Saravana Kannan wrote:
> On Tue, Feb 21, 2023 at 2:52 PM Mark Brown <broonie@kernel.org> wrote:

> > My main thought right now is that I'm not going to think about it
> > too hard if it doesn't work correctly...

> :( I'm not asking for a thorough code review. Just if you are okay
> with the idea/approach of pushing the ordering logic to driver core to
> avoid reimplementing what's already available and avoiding some races
> in the regulator code (stuff like, checking if some other thread
> resolved a supply while you were working on it). The patch at least
> works on my device and works for most regulators in Marek's devices.
> So, it's not a complete broken mess :)

Well, there's the fact that it's clearly not a bus (not even a virtual
one like virtio) which will doubtless cause problems down the line.
Otherwise the fact that you're so concerned is making me think there's
landmines in here that need a really detailed look.

> On a separate note, I have some questions about setting machine
> constraints during regulator_register(). Why do we even try to set
> machine constraints before a regulator's supply is resolved? None of
> the consumers can make any requests anyway. So what else is going to
> need those constraints? Wouldn't the regulator just be in whatever
> state the bootloader left it at?

If the state we inherit is somehow bad then we want to try to correct
problems as fast as possible, to the extent we can.  The firmware may
not be making any effort to configure the hardware, we can end up with
hard coded defaults from the silicon which might need some fixup so we
want to minimise the amount of time we spend operating out of spec.

> The current logic is something like:

> 1. Try to resolve supply if it's always on or a boot on regulator.
> 2. Set machine constraints -- this might fail for multiple reasons.
> One of them being unresolved supply.
> 3. If it failed due to unresolved supply, but it wasn't resolved in step 1.
> 3. a. Try to resolve supply,
> 3. b. If 3.a. didn't fail, try to set machine constraints.
> 3. c. If 3.b failed, fail registration.

IIRC the goal is to only configure the supply if we really need to so it
doesn't get in the way of anything else.

> Why isn't this just:
> 1. Try to resolve supply (for all regulators).
> 2. If we are able to resolve supply set machine constraints.

Most constraint setting doesn't need the supply.

> 3. If we weren't able to resolve supply, set machine constraints when
> we resolve supply in the future?

This may never happen.

> Or if you need to set machine constraints without waiting for supply,
> then why not at least:

> 1. Try to resolve supply (for all regulators).
> 2. Set machine constraints.
> 3. When we resolve supply in the future, do whatever remaining bits
> that you need to do.

There's also the coupling to deal with.  It's mainly that we don't even
bother trying to resolve the supply until we need it to cut down on
noise from reporting transient errors that'll sort themsleves out later.