Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Jon Hunter]

Hi Linus,

On 11/10/18 10:29, Linus Walleij wrote:
> On Thu, Oct 11, 2018 at 11:01 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> 
>> I've just noticed that this patch causes regression on Samsung
>> Exynos4412-based Trats2 board. Conversion to GPIO descriptor breaks
>> operation when regulators used shared GPIO:  sii9234 i2c driver
>> is not able to get vcc33mhl regulator (it uses shared GPIO enable
>> line with vsil12 regulator).
> 
> So I guess this means that this physical GPIO line will enable the
> vcc33mhl and the vsil12 regulators at the same time?
> 
>> This issue has been already pointed in case of commits:
>> 37fa23dbccbd97663acc085bd79246f427e603a1
>> d1dae72fab2c377ff463742eefd8ac0f9e99b7b9
>> ab4d11e2c2329cf7cb7be31ff22489aae4dee5dc
> 
> A big sorry for my ignorance, I guess the information overload
> on the mailing list just makes me miss the important points.
> I'll try to be better, sadly I constantly fail to keep everything
> in mind and constantly break things like this.
> 
>> Maybe it would be better to first solve the handling of shared enable
>> GPIO in the descriptor-based interface before converting more regulators
>> and stepping into this issue again?
> 
> I am trying to solve it, but I just don't have systems to reproduce all
> kinds of things. It's a bit stressful since this is one of those runtime
> things that is hard to test when devising a patch for systems I don't
> have.

This also appears to be causing a regression on the Tegra124 Jetson TK1
that also uses a shared GPIO for two regulators. The 2nd regulator that
uses the GPIO now fails to probe [0] ...

[    0.680021] +5V_SATA: supplied by +5V_SYS
[    0.683964] reg-fixed-voltage: probe of regulators:regulator@14 failed with error -16

Not sure if you have one of these, but otherwise I can help test.

Cheers
Jon

[0] https://storage.kernelci.org/next/master/next-20181011/arm/tegra_defconfig/lab-baylibre-seattle/boot-tegra124-jetson-tk1.html 

-- 
nvpublic

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Marcel Ziswiler]

Hi Linus

On Thu, 2018-10-11 at 16:00 +0100, Jon Hunter wrote:
> Hi Linus,
> 
> On 11/10/18 10:29, Linus Walleij wrote:
> > On Thu, Oct 11, 2018 at 11:01 AM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> > 
> > > I've just noticed that this patch causes regression on Samsung
> > > Exynos4412-based Trats2 board. Conversion to GPIO descriptor
> > > breaks
> > > operation when regulators used shared GPIO:  sii9234 i2c driver
> > > is not able to get vcc33mhl regulator (it uses shared GPIO enable
> > > line with vsil12 regulator).
> > 
> > So I guess this means that this physical GPIO line will enable the
> > vcc33mhl and the vsil12 regulators at the same time?
> > 
> > > This issue has been already pointed in case of commits:
> > > 37fa23dbccbd97663acc085bd79246f427e603a1
> > > d1dae72fab2c377ff463742eefd8ac0f9e99b7b9
> > > ab4d11e2c2329cf7cb7be31ff22489aae4dee5dc
> > 
> > A big sorry for my ignorance, I guess the information overload
> > on the mailing list just makes me miss the important points.
> > I'll try to be better, sadly I constantly fail to keep everything
> > in mind and constantly break things like this.
> > 
> > > Maybe it would be better to first solve the handling of shared
> > > enable
> > > GPIO in the descriptor-based interface before converting more
> > > regulators
> > > and stepping into this issue again?
> > 
> > I am trying to solve it, but I just don't have systems to reproduce
> > all
> > kinds of things. It's a bit stressful since this is one of those
> > runtime
> > things that is hard to test when devising a patch for systems I
> > don't
> > have.
> 
> This also appears to be causing a regression on the Tegra124 Jetson
> TK1
> that also uses a shared GPIO for two regulators. The 2nd regulator
> that
> uses the GPIO now fails to probe [0] ...
> 
> [    0.680021] +5V_SATA: supplied by +5V_SYS
> [    0.683964] reg-fixed-voltage: probe of regulators:regulator@14
> failed with error -16
> 
> Not sure if you have one of these, but otherwise I can help test.

I guess that is also what broke HDMI on Apalis/Colibri T30 causing me
to submit a fix [1]. I may also help testing.

BTW: Is it only me or is today's -next completely broken now?

[    0.691258] Unable to handle kernel NULL pointer dereference at
virtual address 000001f8
[    0.699704] pgd = (ptrval)
[    0.702515] [000001f8] *pgd=00000000
[    0.706236] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[    0.711749] Modules linked in:
[    0.714930] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc7-
next-20181011-dirty #3
[    0.723245] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[    0.729765] PC is at gpiod_hog+0x2c/0x150
[    0.733933] LR is at of_gpiochip_add+0x34c/0x510

This has been observed on Apalis TK1.

> Cheers
> Jon
> 
> [0] https://storage.kernelci.org/next/master/next-20181011/arm/tegra_
> defconfig/lab-baylibre-seattle/boot-tegra124-jetson-tk1.html 

Cheers

Marcel

[1] https://lore.kernel.org/lkml/20181009152523.3771-4-marcel@ziswiler.com

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Linus Walleij]

Hi Jon,

On Thu, Oct 11, 2018 at 5:00 PM Jon Hunter <jonathanh@nvidia.com> wrote:

> This also appears to be causing a regression on the Tegra124 Jetson TK1
> that also uses a shared GPIO for two regulators. The 2nd regulator that
> uses the GPIO now fails to probe [0] ...
>
> [    0.680021] +5V_SATA: supplied by +5V_SYS
> [    0.683964] reg-fixed-voltage: probe of regulators:regulator@14 failed with error -16
>
> Not sure if you have one of these, but otherwise I can help test.

Would be great if you could test the patch I came up with for Marek:
https://marc.info/?l=linux-kernel&m=153926854327176&w=2

FWIW what makes it so confusing for the GPIO maintainer with
multiple consumers is that unless there is some mechanism
(as in the regulator core) to pair them up and avoid them shaking
the GPIO from two ends, it makes little sense.

So I guess in the long run I should pull this into the gpiolib somehow.

Linus

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Linus Walleij]

On Thu, Oct 11, 2018 at 5:34 PM Marcel Ziswiler
<marcel.ziswiler@toradex.com> wrote:

> I guess that is also what broke HDMI on Apalis/Colibri T30 causing me
> to submit a fix [1]. I may also help testing.

I see there are many ways to skin this cat.

Does this patch fix things for you as well?
https://marc.info/?l=linux-kernel&m=153926854327176&w=2

Yours,
Linus Walleij

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Marcel Ziswiler]

On Thu, 2018-10-11 at 19:47 +0200, Linus Walleij wrote:
> On Thu, Oct 11, 2018 at 5:34 PM Marcel Ziswiler
> <marcel.ziswiler@toradex.com> wrote:
> 
> > I guess that is also what broke HDMI on Apalis/Colibri T30 causing
> > me
> > to submit a fix [1]. I may also help testing.
> 
> I see there are many ways to skin this cat.

Yes, as a matter of fact I screened the kernel concerning this multi
gpio stuff but could not quite find many examples and no mentioning
anywhere whether or not this is actually allowed. So I kind of assumed
that this may just not really be allowed and cooked up my patch which
is anyway kind of a cleaner solution. I mean explicitly modelling the
GPIO into some intermediate regulator supplying the others.

> Does this patch fix things for you as well?
> https://marc.info/?l=linux-kernel&m=153926854327176&w=2

Yes, da-da and HDMI works again. Thanks!

I will still try to get my other patch in as like mentioned above I
feel it is a cleaner solution to our regulator setup.

BTW: Have you heard of lore as well? I believe it is the better mailing
list archive as of today:

https://lore.kernel.org/lkml/20181011143531.7195-1-linus.walleij@linaro
.org/

> Yours,
> Linus Walleij

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Jon Hunter]

Hi Linus,

On 11/10/18 18:45, Linus Walleij wrote:
> Hi Jon,
> 
> On Thu, Oct 11, 2018 at 5:00 PM Jon Hunter <jonathanh@nvidia.com> wrote:
> 
>> This also appears to be causing a regression on the Tegra124 Jetson TK1
>> that also uses a shared GPIO for two regulators. The 2nd regulator that
>> uses the GPIO now fails to probe [0] ...
>>
>> [    0.680021] +5V_SATA: supplied by +5V_SYS
>> [    0.683964] reg-fixed-voltage: probe of regulators:regulator@14 failed with error -16
>>
>> Not sure if you have one of these, but otherwise I can help test.
> 
> Would be great if you could test the patch I came up with for Marek:
> https://marc.info/?l=linux-kernel&m=153926854327176&w=2

Yes works for me! I will respond to your actual patch as well with
tested-by.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Jon Hunter]

On 12/10/18 10:43, Marcel Ziswiler wrote:
> On Thu, 2018-10-11 at 19:47 +0200, Linus Walleij wrote:
>> On Thu, Oct 11, 2018 at 5:34 PM Marcel Ziswiler
>> <marcel.ziswiler@toradex.com> wrote:
>>
>>> I guess that is also what broke HDMI on Apalis/Colibri T30 causing
>>> me
>>> to submit a fix [1]. I may also help testing.
>>
>> I see there are many ways to skin this cat.
> 
> Yes, as a matter of fact I screened the kernel concerning this multi
> gpio stuff but could not quite find many examples and no mentioning
> anywhere whether or not this is actually allowed. So I kind of assumed
> that this may just not really be allowed and cooked up my patch which
> is anyway kind of a cleaner solution. I mean explicitly modelling the
> GPIO into some intermediate regulator supplying the others.

See the function 'regulator_ena_gpio_request()', it states that the same
GPIO pin can be shared among regulators.

We had the same situation for Tegra124 Jetson TK1 but I don't think that
adding a pseudo intermediate regulator is cleaner. If the GPIO controls
more than one regulator, I don't see why is it necessary to change the
DT. There are several other people reporting the same problem with
various different boards. So this does seem to be a common usage.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Russell King - ARM Linux]

On Fri, Oct 12, 2018 at 11:39:15AM +0100, Jon Hunter wrote:
> We had the same situation for Tegra124 Jetson TK1 but I don't think that
> adding a pseudo intermediate regulator is cleaner. If the GPIO controls
> more than one regulator, I don't see why is it necessary to change the
> DT. There are several other people reporting the same problem with
> various different boards. So this does seem to be a common usage.

Given that DT describes the hardware, not the software implementation,
it must not change just because we move from GPIO numbers to GPIO
descriptors.

The existing DT description is reasonable, and introducing ficticious
regulators in DT to work around the implementation is not reasonable.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Linus Walleij]

On Fri, Oct 12, 2018 at 12:43 PM Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:

> Given that DT describes the hardware, not the software implementation,
> it must not change just because we move from GPIO numbers to GPIO
> descriptors.
>
> The existing DT description is reasonable, and introducing ficticious
> regulators in DT to work around the implementation is not reasonable.

You're right. In the electronics and the device tree it
makes perfect sense for the same line to enable/disable several
regulators.

The patch I made is a quick hack to allow multiple users of the
same descriptor, I think the long term fix is simply allow multiple
users where applicable and maintain a reference count just like
the regulator core is doing, assert the GPIO when the first
consumer asserts it and de-assert it when the last consumer
de-asserts it.

What the old and current gpiolib does us just
call the callback to enable/disable the line immediately as
response to the callback asserting/deasserting the GPIO,
which is just too simplified.

Yours,
Linus Walleij

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Marcel Ziswiler]

On Fri, 2018-10-12 at 11:43 +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 12, 2018 at 11:39:15AM +0100, Jon Hunter wrote:
> > We had the same situation for Tegra124 Jetson TK1 but I don't think
> > that
> > adding a pseudo intermediate regulator is cleaner. If the GPIO
> > controls
> > more than one regulator, I don't see why is it necessary to change
> > the
> > DT. There are several other people reporting the same problem with
> > various different boards. So this does seem to be a common usage.
> 
> Given that DT describes the hardware, not the software
> implementation,
> it must not change just because we move from GPIO numbers to GPIO
> descriptors.

Yes, that I do agree. However, like mentioned before on quick glance I
really could not find any documentation about this "GPIO sharing" being
allowed or not.

> The existing DT description is reasonable, and introducing ficticious
> regulators in DT to work around the implementation is not reasonable.

I don't think it is that fictitious as it makes it crystal clear that
there is something shared with all its pros and cons. E.g. what happens
if one of them regulators wants to turn off while the other one still
needs power? The regular regulator dependency tree would nicely make
this all clear.

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Russell King - ARM Linux]

On Fri, Oct 12, 2018 at 11:43:13AM +0000, Marcel Ziswiler wrote:
> I don't think it is that fictitious as it makes it crystal clear that
> there is something shared with all its pros and cons. E.g. what happens
> if one of them regulators wants to turn off while the other one still
> needs power? The regular regulator dependency tree would nicely make
> this all clear.

If you're introducing a regulator that doesn't exist in reality
just to be able to share a GPIO line that is wired to several
real regulators, then it _is_ ficticious.  You're not describing
the hardware, you're describing something else to work around the
shortcomings of the implementation that can't cope with how stuff
is wired up in the real world.  You're making the DT description
fit the software implementation, rather than the software
implementation fit the real world hardware.

Having a single GPIO that controls multiple separate regulators
which have entirely separate supplies of their own is very common
in electronics.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Marcel Ziswiler]

On Fri, 2018-10-12 at 13:59 +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 12, 2018 at 11:43:13AM +0000, Marcel Ziswiler wrote:
> > I don't think it is that fictitious as it makes it crystal clear
> > that
> > there is something shared with all its pros and cons. E.g. what
> > happens
> > if one of them regulators wants to turn off while the other one
> > still
> > needs power? The regular regulator dependency tree would nicely
> > make
> > this all clear.
> 
> If you're introducing a regulator that doesn't exist in reality
> just to be able to share a GPIO line that is wired to several
> real regulators, then it _is_ ficticious.  You're not describing
> the hardware, you're describing something else to work around the
> shortcomings of the implementation that can't cope with how stuff
> is wired up in the real world.  You're making the DT description
> fit the software implementation, rather than the software
> implementation fit the real world hardware.
> 
> Having a single GPIO that controls multiple separate regulators
> which have entirely separate supplies of their own is very common
> in electronics.

Sure, fine. I will drop it. Thanks!

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Andy Shevchenko]

On Fri, Oct 12, 2018 at 1:45 PM Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
>
> On Fri, Oct 12, 2018 at 11:39:15AM +0100, Jon Hunter wrote:
> > We had the same situation for Tegra124 Jetson TK1 but I don't think that
> > adding a pseudo intermediate regulator is cleaner. If the GPIO controls
> > more than one regulator, I don't see why is it necessary to change the
> > DT. There are several other people reporting the same problem with
> > various different boards. So this does seem to be a common usage.
>
> Given that DT describes the hardware, not the software implementation,
> it must not change just because we move from GPIO numbers to GPIO
> descriptors.
>
> The existing DT description is reasonable, and introducing ficticious
> regulators in DT to work around the implementation is not reasonable.

If there is no way to detect shared use of GPIO line for regulators
(*) from current DT description, DT description should be updated to
reflect, yes, hardware.

(*) Not familiar with the guts of DT descriptive language, don't know
if there are some ways to do a such without additional flags or so.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Fri, Oct 12, 2018 at 04:58:38PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 12, 2018 at 1:45 PM Russell King - ARM Linux

> > Given that DT describes the hardware, not the software implementation,
> > it must not change just because we move from GPIO numbers to GPIO
> > descriptors.

> > The existing DT description is reasonable, and introducing ficticious
> > regulators in DT to work around the implementation is not reasonable.

> If there is no way to detect shared use of GPIO line for regulators
> (*) from current DT description, DT description should be updated to
> reflect, yes, hardware.

> (*) Not familiar with the guts of DT descriptive language, don't know
> if there are some ways to do a such without additional flags or so.

You can detect this via resolving the GPIOs and seeing if it points back
to something that's already in use for an enable.  This isn't ideal
especially if you want to do it up front but it is doable.  You could
also just assume anything might end up being shared and rather than
doing it up front which is easier and probably about as good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

On Fri, Oct 12, 2018 at 11:43:13AM +0000, Marcel Ziswiler wrote:
> On Fri, 2018-10-12 at 11:43 +0100, Russell King - ARM Linux wrote:

> > The existing DT description is reasonable, and introducing ficticious
> > regulators in DT to work around the implementation is not reasonable.

> I don't think it is that fictitious as it makes it crystal clear that
> there is something shared with all its pros and cons. E.g. what happens
> if one of them regulators wants to turn off while the other one still
> needs power? The regular regulator dependency tree would nicely make
> this all clear.

We already have code to handle that via refcounting on the GPIO once we
identify that it's the same GPIO.  If we make a shared virtual parent
regulator that'll break other things where we're tracking what the
actual physical parent for voltage reasons like adjusting parent
voltages up and down to improve efficiency or handling things that are
just dumb power switches rather than actual regulators.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]