More GPIO madness on iMX6 - and the crappy ARM port of Linux

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Russell King - ARM Linux]

So, we have this wonderful GPIO layer which abstracts GPIO stuff and
hides stuff.  It's really wonderful, because you don't have to care
about how the GPIOs are actually accessed in drivers anymore.

However, what about the behaviour of GPIOs?

What about... for example... this sequence:

	gpio_direction_output(gpio, 1);
	val = gpio_get_value(gpio);

What value is "val"?  More importantly, what value is reflected in
/sys/kernel/debug/gpio ?  Would it indicate that it's high or low?

Now, while you can make reasonable assumptions, such as "it'll return
that the output is being driven to the requested state" or "it'll
return the actual state of the pin", what about this instead, which
happens on iMX hardware - "it'll _always_ return zero".

Yes, iMX6 at least has this behaviour.  For any output, val as above
will always be zero, and /proc/sys/kernel/debug/gpio will always
report that an output is zero... unless the SION bit has been set for
that GPIO signal.

The reason is that on hardware such as iMX6, reading the GPIO is done
by reading the pad state register, and this register is _only_ supplied
the state of the pad when the input path is enabled.  The input path
is only enabled when the output is disabled, or the SION bit is set
to force the GPIO input path.

So, this brings up three obvious questions:

1. What should gpio_get_value() return for an output?
2. What should be reported in /sys/kernel/debug/gpio for an output?
3. Should iMX6 (and similar) GPIOs always have the SION bit set in
   their DT descriptions?

Discuss.

Since I've wasted all afternoon trying to chase down why the GPIOs
controlling the regulators for USB appear to be disabled when reading
/sys/kernel/debug/gpio, I'm *FAR* from impressed by the current
confusing behaviour - and it's another nail in the "why the ARM Linux
kernel is turning to shit" coffin.  I'm pleased to know that according
to google+, I'm not the only one with these feelings - so it's about
time we started fixing some of these idiotic and crap behaviours in
these layers of subsystems which make platform support unnecessarily
difficult.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Eric Nelson]

Hi Russell,

On 01/17/2014 11:47 AM, Russell King - ARM Linux wrote:
> So, we have this wonderful GPIO layer which abstracts GPIO stuff and
> hides stuff.  It's really wonderful, because you don't have to care
> about how the GPIOs are actually accessed in drivers anymore.
>
> However, what about the behaviour of GPIOs?
>
> What about... for example... this sequence:
>
> 	gpio_direction_output(gpio, 1);
> 	val = gpio_get_value(gpio);
>
> What value is "val"?  More importantly, what value is reflected in
> /sys/kernel/debug/gpio ?  Would it indicate that it's high or low?
>
> Now, while you can make reasonable assumptions, such as "it'll return
> that the output is being driven to the requested state" or "it'll
> return the actual state of the pin", what about this instead, which
> happens on iMX hardware - "it'll _always_ return zero".
>
> Yes, iMX6 at least has this behaviour.  For any output, val as above
> will always be zero, and /proc/sys/kernel/debug/gpio will always
> report that an output is zero... unless the SION bit has been set for
> that GPIO signal.
>
> The reason is that on hardware such as iMX6, reading the GPIO is done
> by reading the pad state register, and this register is _only_ supplied
> the state of the pad when the input path is enabled.  The input path
> is only enabled when the output is disabled, or the SION bit is set
> to force the GPIO input path.
>
> So, this brings up three obvious questions:
>
> 1. What should gpio_get_value() return for an output?

It seems that this is pretty well specified.

To quote Documentation/gpio/gpio-legacy.txt:
	>> When reading the value of an output pin, the value
	>> returned should be what's seen on the pin ... that
	>> won't always match the specified output value, because
	>> of issues including open-drain signaling and output
	>> latencies.

Documentation/gpio/gpio.txt is a little less clear, but implies
the same:
	>> If you are "driving" the signal high but
	>> gpiod_get_value(gpio) reports a low value (after
	>> the appropriate rise time passes), you know some other
	>> component is driving the shared signal low

> 2. What should be reported in /sys/kernel/debug/gpio for an output?
> 3. Should iMX6 (and similar) GPIOs always have the SION bit set in
>     their DT descriptions?
>
> Discuss.
>

Each signal accessed using the GPIO subsystem **must** have
the SION bit set and the values returned should be the value
from the PSR registers.

Regards,


Eric

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Eric Nelson]

Hi Russell,

On 01/17/2014 12:13 PM, Eric Nelson wrote:
> Hi Russell,
>
> On 01/17/2014 11:47 AM, Russell King - ARM Linux wrote:
>>
 >> <snip>
>>
>> So, this brings up three obvious questions:
>>
>> 1. What should gpio_get_value() return for an output?
>
> It seems that this is pretty well specified.
>
> To quote Documentation/gpio/gpio-legacy.txt:
>      >> When reading the value of an output pin, the value
>      >> returned should be what's seen on the pin ... that
>      >> won't always match the specified output value, because
>      >> of issues including open-drain signaling and output
>      >> latencies.
>
> Documentation/gpio/gpio.txt is a little less clear, but implies
> the same:
>      >> If you are "driving" the signal high but
>      >> gpiod_get_value(gpio) reports a low value (after
>      >> the appropriate rise time passes), you know some other
>      >> component is driving the shared signal low
>
>> 2. What should be reported in /sys/kernel/debug/gpio for an output?
>> 3. Should iMX6 (and similar) GPIOs always have the SION bit set in
>>     their DT descriptions?
>>
>> Discuss.
>>
>
> Each signal accessed using the GPIO subsystem **must** have
> the SION bit set and the values returned should be the value
> from the PSR registers.
>

Because the MUX registers are very consistent, it appears
that a fix for this that sets the SION bit is simple (almost
mechanical) in the imx*pinfunc.h files.

It seems that each of the declarations matching

	#define MX*GPIO_something

should have bit four set in the mux_mode column.

I'd be happy to work up a patch if there's agreement here
(and I can't think of any rationale for not setting these).

Regards,


Eric

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Stephen Warren]

On 01/17/2014 11:47 AM, Russell King - ARM Linux wrote:
> So, we have this wonderful GPIO layer which abstracts GPIO stuff and
> hides stuff.  It's really wonderful, because you don't have to care
> about how the GPIOs are actually accessed in drivers anymore.
...
> 1. What should gpio_get_value() return for an output?

Some HW can't ever read back the value of an output pin, so isn't
calling gpio_get_value() undefined for output pins?

> 2. What should be reported in /sys/kernel/debug/gpio for an output?

Shouldn't it indicate that the pin is an output, and say nothing about
the input value?

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Arnaud Patard (Rtp)]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> So, we have this wonderful GPIO layer which abstracts GPIO stuff and
> hides stuff.  It's really wonderful, because you don't have to care
> about how the GPIOs are actually accessed in drivers anymore.
>
> However, what about the behaviour of GPIOs?
>
> What about... for example... this sequence:
>
> 	gpio_direction_output(gpio, 1);
> 	val = gpio_get_value(gpio);
>
> What value is "val"?  More importantly, what value is reflected in
> /sys/kernel/debug/gpio ?  Would it indicate that it's high or low?
>
> Now, while you can make reasonable assumptions, such as "it'll return
> that the output is being driven to the requested state" or "it'll
> return the actual state of the pin", what about this instead, which
> happens on iMX hardware - "it'll _always_ return zero".
>

this is "expected". gpio layer docs are saying that in output case, the
value may be wrong. Not intuitive but documented.

> Yes, iMX6 at least has this behaviour.  For any output, val as above
> will always be zero, and /proc/sys/kernel/debug/gpio will always
> report that an output is zero... unless the SION bit has been set for
> that GPIO signal.

afaik at least imx51/53 have some behaviour.

>
> The reason is that on hardware such as iMX6, reading the GPIO is done
> by reading the pad state register, and this register is _only_ supplied
> the state of the pad when the input path is enabled.  The input path
> is only enabled when the output is disabled, or the SION bit is set
> to force the GPIO input path.

I sent mails about this same issue for imx51 in Dec 2010 and answer were
that the SION bit should not be set for all gpios:
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/100875


Arnaud

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Eric Nelson]

On 01/17/2014 12:57 PM, Arnaud Patard (Rtp) wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
>
>> So, we have this wonderful GPIO layer which abstracts GPIO stuff and
>> hides stuff.  It's really wonderful, because you don't have to care
>> about how the GPIOs are actually accessed in drivers anymore.
>>
>> However, what about the behaviour of GPIOs?
>>
>> What about... for example... this sequence:
>>
>> 	gpio_direction_output(gpio, 1);
>> 	val = gpio_get_value(gpio);
>>
>> What value is "val"?  More importantly, what value is reflected in
>> /sys/kernel/debug/gpio ?  Would it indicate that it's high or low?
>>
>> Now, while you can make reasonable assumptions, such as "it'll return
>> that the output is being driven to the requested state" or "it'll
>> return the actual state of the pin", what about this instead, which
>> happens on iMX hardware - "it'll _always_ return zero".
>>
>
> this is "expected". gpio layer docs are saying that in output case, the
> value may be wrong. Not intuitive but documented.
>
>> Yes, iMX6 at least has this behaviour.  For any output, val as above
>> will always be zero, and /proc/sys/kernel/debug/gpio will always
>> report that an output is zero... unless the SION bit has been set for
>> that GPIO signal.
>
> afaik at least imx51/53 have some behaviour.
>
>>
>> The reason is that on hardware such as iMX6, reading the GPIO is done
>> by reading the pad state register, and this register is _only_ supplied
>> the state of the pad when the input path is enabled.  The input path
>> is only enabled when the output is disabled, or the SION bit is set
>> to force the GPIO input path.
>
> I sent mails about this same issue for imx51 in Dec 2010 and answer were
> that the SION bit should not be set for all gpios:
> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/100875
>

Thanks Arnaud,

This bit from the 2010 chain really needs some explanation:

 >> Arnaud Patard (Rtp) writes:
 >>
 >> <snip>
 >>
 >> I had done the same, but had some trouble with this.
 >> E.g. on our board GPIO1_7 is used as a generic GPIO to enable an
 >> external clock oscillator for the USBH1 ULPI PHY. When the SION bit
 >> for this pad was set, I got strange errors on the USBH1 port
 >> (disconnecting low speed devices behind a hub would stall the
 >> bus). When I removed the SION bit for that pin everything worked
 >> well.
 >>

Did you ever chase down the symptom here? Was the GPIO output not
holding a constant value such that the oscillator wasn't functioning?

Please advise,


Eric

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Uwe Kleine-König]

On Fri, Jan 17, 2014 at 12:40:02PM -0700, Stephen Warren wrote:
> On 01/17/2014 11:47 AM, Russell King - ARM Linux wrote:
> > So, we have this wonderful GPIO layer which abstracts GPIO stuff and
> > hides stuff.  It's really wonderful, because you don't have to care
> > about how the GPIOs are actually accessed in drivers anymore.
> ...
> > 1. What should gpio_get_value() return for an output?
> 
> Some HW can't ever read back the value of an output pin, so isn't
> calling gpio_get_value() undefined for output pins?
I remember something like: "If possible it should return what is seen on
the pad, if not, return 0." being the requirement.

Documentation/gpio/gpio-legacy.txt still says that:
When reading the value of an output pin, the value returned should be
what's seen on the pin ... that won't always match the specified output
value, because of issues including open-drain signaling and output
latencies.

[...] However, note that not all platforms can read the value of output
pins; those that can't should always return zero.

Something similar can be found in Documentation/gpio/consumer about
gpiod_get_value.

So you might consider i.MX6 (and also the earlier i.MX SoCs) to violate
a "should" in the requirements. If (not necessarily iff) adding the SION
bit doesn't have other side effects than being able to read out the gpio
value (like increased latencies or power consumption) I'm all for
setting it in every case assuming it is available for all pins.

Other than that I see five possibilities:

 a) keep everything as is. Seems to imply surprises which is bad. Maybe
    at least improve the docs to have the information that the return
    value of gpio_get_value might not be usefull in the same paragraph
    as what should be reported.
 b) check if for the requested gpio pad the SION bit is set and read the
    pad value if it is, return 0 otherwise. (But note, after thinking
    again I don't believe this to be possible, because there is usually >1
    pad that can output a given gpio. Moreover AFAIK the information to
    which pads a given gpio can be routed is missing in the kernel. That
    could be fixed, that would result in a big table though. (And the
    first problem is still unfixed.))
 c) When gpio_get_value is requested for a gpio, set SION temporarily.
    This has the same implementation problems as b) tough.
 d) Read out the pad value unconditionally and return that.
    I didn't check if it always returns 0 if SION is unset though. That
    should be asserted first (or otherwise check if there are
    possibilities to find out if the pad value is valid).
 e) add a flag to all gpio-chips signalling which case gpio_get_value
    implements (i.e: return
      - the actual value on the pad; or
      - zero.
    ). This is not orthogonal to b) - d)

> > 2. What should be reported in /sys/kernel/debug/gpio for an output?
It should report the same thing as gpio_get_value in 1.

> Shouldn't it indicate that the pin is an output, and say nothing about
> the input value?
It's usefull at times to be able to read an output pin. So I'm against
discarding the value on all platforms because some platforms are unable
to provide it.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Russell King - ARM Linux]

On Fri, Jan 17, 2014 at 12:40:02PM -0700, Stephen Warren wrote:
> On 01/17/2014 11:47 AM, Russell King - ARM Linux wrote:
> > So, we have this wonderful GPIO layer which abstracts GPIO stuff and
> > hides stuff.  It's really wonderful, because you don't have to care
> > about how the GPIOs are actually accessed in drivers anymore.
> ...
> > 1. What should gpio_get_value() return for an output?
> 
> Some HW can't ever read back the value of an output pin, so isn't
> calling gpio_get_value() undefined for output pins?

As has been pointed out, that's not how gpio_get_valie() is documented.
It's documented to return the value of the pin where possible.  In my
case, it _is_ possible to read back the value of the pin - it just
needs the appropriate chip configuration to make it happen.

Now to the crunch point of my email: where subsystems differ completely
_unnecessarily_ from what is expected from them - such as returning the
current state of the output where it's possible to do so - then this
kind of difference *reduces* the portability of that subsystem, and
makes being able to move from one SoC to another _unnecessarily_ more
difficult.

This is the issue: if everyone has to re-learn how subsystem X behaves
on hardware Y because it has unnecessarily different hardware, then
we're just making stuff much harder for no reason what so ever, and I'd
say there's no point to having that subsystem in the first place.  It's
nothing more than a waste of space.

Let's put it a different way - what if every hard disk you bought had
a completely different connector on it just because the manufacturer
could put a different connector on it, and you had to also buy their
special cable... oh, and the motherboard end also had a motherboard
specific connector, so you had to get the right connector at both ends
of the cable.  Electrically, the thing is the same, it's just that the
manufacturer is being perverse and changing the connector for the shere
hell of it.  Would that annoy you?

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Arnaud Patard (Rtp)]

Eric Nelson <eric.nelson@boundarydevices.com> writes:

> On 01/17/2014 12:57 PM, Arnaud Patard (Rtp) wrote:
>> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
>>
>>> So, we have this wonderful GPIO layer which abstracts GPIO stuff and
>>> hides stuff.  It's really wonderful, because you don't have to care
>>> about how the GPIOs are actually accessed in drivers anymore.
>>>
>>> However, what about the behaviour of GPIOs?
>>>
>>> What about... for example... this sequence:
>>>
>>> 	gpio_direction_output(gpio, 1);
>>> 	val = gpio_get_value(gpio);
>>>
>>> What value is "val"?  More importantly, what value is reflected in
>>> /sys/kernel/debug/gpio ?  Would it indicate that it's high or low?
>>>
>>> Now, while you can make reasonable assumptions, such as "it'll return
>>> that the output is being driven to the requested state" or "it'll
>>> return the actual state of the pin", what about this instead, which
>>> happens on iMX hardware - "it'll _always_ return zero".
>>>
>>
>> this is "expected". gpio layer docs are saying that in output case, the
>> value may be wrong. Not intuitive but documented.
>>
>>> Yes, iMX6 at least has this behaviour.  For any output, val as above
>>> will always be zero, and /proc/sys/kernel/debug/gpio will always
>>> report that an output is zero... unless the SION bit has been set for
>>> that GPIO signal.
>>
>> afaik at least imx51/53 have some behaviour.
>>
>>>
>>> The reason is that on hardware such as iMX6, reading the GPIO is done
>>> by reading the pad state register, and this register is _only_ supplied
>>> the state of the pad when the input path is enabled.  The input path
>>> is only enabled when the output is disabled, or the SION bit is set
>>> to force the GPIO input path.
>>
>> I sent mails about this same issue for imx51 in Dec 2010 and answer were
>> that the SION bit should not be set for all gpios:
>> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/100875
>>
>
> Thanks Arnaud,
>
> This bit from the 2010 chain really needs some explanation:
>
>>> Arnaud Patard (Rtp) writes:
>>>
>>> <snip>
>>>
>>> I had done the same, but had some trouble with this.
>>> E.g. on our board GPIO1_7 is used as a generic GPIO to enable an
>>> external clock oscillator for the USBH1 ULPI PHY. When the SION bit
>>> for this pad was set, I got strange errors on the USBH1 port
>>> (disconnecting low speed devices behind a hub would stall the
>>> bus). When I removed the SION bit for that pin everything worked
>>> well.
>>>
>
> Did you ever chase down the symptom here? Was the GPIO output not
> holding a constant value such that the oscillator wasn't functioning?
>

It was not me who got this issue. The issue I had was not being able to
read GPIO value if set as output. This link may be clearer:
http://archive.arm.linux.org.uk/lurker/message/20101215.151016.ea731aa7.en.html


Arnaud

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Stephen Warren]

On 01/17/2014 01:24 PM, Russell King - ARM Linux wrote:
> On Fri, Jan 17, 2014 at 12:40:02PM -0700, Stephen Warren wrote:
>> On 01/17/2014 11:47 AM, Russell King - ARM Linux wrote:
>>> So, we have this wonderful GPIO layer which abstracts GPIO stuff and
>>> hides stuff.  It's really wonderful, because you don't have to care
>>> about how the GPIOs are actually accessed in drivers anymore.
>> ...
>>> 1. What should gpio_get_value() return for an output?
>>
>> Some HW can't ever read back the value of an output pin, so isn't
>> calling gpio_get_value() undefined for output pins?
> 
> As has been pointed out, that's not how gpio_get_valie() is documented.
> It's documented to return the value of the pin where possible.  In my
> case, it _is_ possible to read back the value of the pin - it just
> needs the appropriate chip configuration to make it happen.
> 
> Now to the crunch point of my email: where subsystems differ completely
> _unnecessarily_ from what is expected from them - such as returning the
> current state of the output where it's possible to do so - then this
> kind of difference *reduces* the portability of that subsystem, and
> makes being able to move from one SoC to another _unnecessarily_ more
> difficult.

If the gpio_get_value() was guaranteed never to be valid for an output
pin, that would actually be *more* portable, not less; it would work the
same way everywhere.

I believe you want gpio_get_value() to return either the driven or
actual pin value where it can on the current HW, but just e.g. hard-code
0 on other HW. That would introduce a core feature that works some
places but not others, and hence make drivers that relied on the feature
less portable between HW with different actual features.

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Russell King - ARM Linux]

On Fri, Jan 17, 2014 at 09:20:15PM +0100, Uwe Kleine-K?nig wrote:
> On Fri, Jan 17, 2014 at 12:40:02PM -0700, Stephen Warren wrote:
> > On 01/17/2014 11:47 AM, Russell King - ARM Linux wrote:
> > > So, we have this wonderful GPIO layer which abstracts GPIO stuff and
> > > hides stuff.  It's really wonderful, because you don't have to care
> > > about how the GPIOs are actually accessed in drivers anymore.
> > ...
> > > 1. What should gpio_get_value() return for an output?
> > 
> > Some HW can't ever read back the value of an output pin, so isn't
> > calling gpio_get_value() undefined for output pins?
> I remember something like: "If possible it should return what is seen on
> the pad, if not, return 0." being the requirement.

Yes, these have already been quoted. :)

> Documentation/gpio/gpio-legacy.txt still says that:
> When reading the value of an output pin, the value returned should be
> what's seen on the pin ... that won't always match the specified output
> value, because of issues including open-drain signaling and output
> latencies.
> 
> [...] However, note that not all platforms can read the value of output
> pins; those that can't should always return zero.
> 
> Something similar can be found in Documentation/gpio/consumer about
> gpiod_get_value.

Right - and I've no problem with this wording.  If it's _impossible_ to
read back the output because the hardware has no support for doing so,
then that's absolutely fine - we can't ask the impossible from the
hardware.

My issue here is that it _is_ possible to read back the state of an
output pin on i.MX6 hardware provided stuff is configured correctly.

> Other than that I see five possibilities:
> 
>  a) keep everything as is. Seems to imply surprises which is bad. Maybe
>     at least improve the docs to have the information that the return
>     value of gpio_get_value might not be usefull in the same paragraph
>     as what should be reported.

This I'm not in favour of as it makes the board porter's job
unnecessarily harder.

>  b) check if for the requested gpio pad the SION bit is set and read the
>     pad value if it is, return 0 otherwise. (But note, after thinking
>     again I don't believe this to be possible, because there is usually >1
>     pad that can output a given gpio. Moreover AFAIK the information to
>     which pads a given gpio can be routed is missing in the kernel. That
>     could be fixed, that would result in a big table though. (And the
>     first problem is still unfixed.))

There's no input muxing on the GPIOs as you have for the non-GPIO
functions in iMX6, so I'd be surprised if you could assign a particular
GPIO to more than one pin.  As I understand it as well, the function of
the SION bit is to force the input path to be enabled, and direct the
state of that pin to the GPIO corresponding with that pin.

Note that the hardware returns zero from the pad value if the input path
is not enabled, so this already happens.  Hence why, when I was checking
the state of the GPIOs enabling the regulators, they were showing that
the GPIOs were low, hence regulators disabled, and hence why I've been
looking at the regulator and gpio code all afternoon.

>  c) When gpio_get_value is requested for a gpio, set SION temporarily.
>     This has the same implementation problems as b) tough.
>  d) Read out the pad value unconditionally and return that.
>     I didn't check if it always returns 0 if SION is unset though. That
>     should be asserted first (or otherwise check if there are
>     possibilities to find out if the pad value is valid).

This is what currently happens.

>  e) add a flag to all gpio-chips signalling which case gpio_get_value
>     implements (i.e: return
>       - the actual value on the pad; or
>       - zero.
>     ). This is not orthogonal to b) - d)
> 
> > > 2. What should be reported in /sys/kernel/debug/gpio for an output?
> It should report the same thing as gpio_get_value in 1.

There's another solution here.  /sys/kernel/debug/gpio is there to allow
us to see the state of the GPIOs, right?  Well, if the asked-for output
value can be different from the read-back output value, how about fixing
this so that the _debug_ can report back what the desired output state
as well as the current input state.

This would mean that this file becomes something like:

 gpio-86  (usb_otg_vbus        ) dir:out out:hi in:lo

which makes it clear that either the pad is being asked to output a high
level, but for some reason reading the input side is returning low state.
It also lets you see what was asked of the output, and what was (in
theory) written to the output.  We already have gpio-generic.c tracking
the output state so it doesn't have to read-modify-write the output
register.

It may _also_ be a good idea to do (e) - have a per-gpiochip flag which
indicates the behaviour here, and omit the "out:" or "in:" as appropriate.

Finally, consider that some drivers should be provided this information.
For example, the bitbanging I2C driver needs this for:

        scllo(adap);
        sda = getsda(adap);
        scl = (adap->getscl == NULL) ? 0 : getscl(adap);
        if (scl) {
                printk(KERN_WARNING "%s: SCL stuck high!\n", name);
                goto bailout;
        }
        if (!sda) {
                printk(KERN_WARNING "%s: SDA unexpected low "
                       "while pulling SCL low!\n", name);
                goto bailout;
        }

        sclhi(adap);
        sda = getsda(adap);
        scl = (adap->getscl == NULL) ? 1 : getscl(adap);
        if (!scl) {
                printk(KERN_WARNING "%s: SCL stuck low!\n", name);
                goto bailout;
        }
        if (!sda) {
                printk(KERN_WARNING "%s: SDA unexpected low "
                       "while pulling SCL high!\n", name);
                goto bailout;
        }

Now, if getscl() always returns zero when scllo() is called (because the
pin is set as an output) the test is pretty useless - it turns into a
verification that yes, the hardware does return zero from the pad register
when the pin is set as an output.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Russell King - ARM Linux]

On Fri, Jan 17, 2014 at 01:42:44PM -0700, Stephen Warren wrote:
> On 01/17/2014 01:24 PM, Russell King - ARM Linux wrote:
> > On Fri, Jan 17, 2014 at 12:40:02PM -0700, Stephen Warren wrote:
> >> On 01/17/2014 11:47 AM, Russell King - ARM Linux wrote:
> >>> So, we have this wonderful GPIO layer which abstracts GPIO stuff and
> >>> hides stuff.  It's really wonderful, because you don't have to care
> >>> about how the GPIOs are actually accessed in drivers anymore.
> >> ...
> >>> 1. What should gpio_get_value() return for an output?
> >>
> >> Some HW can't ever read back the value of an output pin, so isn't
> >> calling gpio_get_value() undefined for output pins?
> > 
> > As has been pointed out, that's not how gpio_get_valie() is documented.
> > It's documented to return the value of the pin where possible.  In my
> > case, it _is_ possible to read back the value of the pin - it just
> > needs the appropriate chip configuration to make it happen.
> > 
> > Now to the crunch point of my email: where subsystems differ completely
> > _unnecessarily_ from what is expected from them - such as returning the
> > current state of the output where it's possible to do so - then this
> > kind of difference *reduces* the portability of that subsystem, and
> > makes being able to move from one SoC to another _unnecessarily_ more
> > difficult.
> 
> If the gpio_get_value() was guaranteed never to be valid for an output
> pin, that would actually be *more* portable, not less; it would work the
> same way everywhere.
> 
> I believe you want gpio_get_value() to return either the driven or
> actual pin value where it can on the current HW, but just e.g. hard-code
> 0 on other HW. That would introduce a core feature that works some
> places but not others, and hence make drivers that relied on the feature
> less portable between HW with different actual features.

I can buy that argument, but there's an issue which stands squarely in
its way, and that is open-drain GPIOs.

These are modelled just as any other GPIO, mainly so that both
gpio_set_value(gpio, 1) and gpio_direction_input(gpio) both result in
the signal being high.  The only combination which results in the
signal being driven low is outputting zero - and the state of the signal
can aways be read back.

The problem here is that such gpios are implemented in things like the
I2C driver such that they're _always_ outputs, and gpio_set_value() is
used to pull the signal down.  gpio_get_value() is used to read its
current state.

So, if we say that gpio_get_value() is undefined, we force such
subsystems to always jump through the non-open-drain paths (using
gpio_direction_input() to set the line high and
gpio_direction_output(gpio, 0) to drive it low.)

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Linus Walleij]

[CC:ing the comaintainer and linux-gpio for generic questions...]

On Fri, Jan 17, 2014 at 9:43 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jan 17, 2014 at 09:20:15PM +0100, Uwe Kleine-K?nig wrote:

>>  e) add a flag to all gpio-chips signalling which case gpio_get_value
>>     implements (i.e: return
>>       - the actual value on the pad; or
>>       - zero.
>>     ). This is not orthogonal to b) - d)
>>
>> > > 2. What should be reported in /sys/kernel/debug/gpio for an output?
>> It should report the same thing as gpio_get_value in 1.
>
> There's another solution here.  /sys/kernel/debug/gpio is there to allow
> us to see the state of the GPIOs, right?  Well, if the asked-for output
> value can be different from the read-back output value, how about fixing
> this so that the _debug_ can report back what the desired output state
> as well as the current input state.
>
> This would mean that this file becomes something like:
>
>  gpio-86  (usb_otg_vbus        ) dir:out out:hi in:lo
>
> which makes it clear that either the pad is being asked to output a high
> level, but for some reason reading the input side is returning low state.
> It also lets you see what was asked of the output, and what was (in
> theory) written to the output.

I really like the looks of this. That kind of helpful stuff is exactly
what the debugfs files shall be used for.

> It may _also_ be a good idea to do (e) - have a per-gpiochip flag which
> indicates the behaviour here, and omit the "out:" or "in:" as appropriate.

That also seems like a good idea.

> Finally, consider that some drivers should be provided this information.
> For example, the bitbanging I2C driver needs this for:
>
>         scllo(adap);
>         sda = getsda(adap);
>         scl = (adap->getscl == NULL) ? 0 : getscl(adap);
>         if (scl) {
>                 printk(KERN_WARNING "%s: SCL stuck high!\n", name);
>                 goto bailout;
>         }
>         if (!sda) {
>                 printk(KERN_WARNING "%s: SDA unexpected low "
>                        "while pulling SCL low!\n", name);
>                 goto bailout;
>         }
>
>         sclhi(adap);
>         sda = getsda(adap);
>         scl = (adap->getscl == NULL) ? 1 : getscl(adap);
>         if (!scl) {
>                 printk(KERN_WARNING "%s: SCL stuck low!\n", name);
>                 goto bailout;
>         }
>         if (!sda) {
>                 printk(KERN_WARNING "%s: SDA unexpected low "
>                        "while pulling SCL high!\n", name);
>                 goto bailout;
>         }
>
> Now, if getscl() always returns zero when scllo() is called (because the
> pin is set as an output) the test is pretty useless - it turns into a
> verification that yes, the hardware does return zero from the pad register
> when the pin is set as an output.

If we can come up with a good API for it this seems useful as
well. The bit-banged I2C is a perfectly valid usecase.

I guess we could discuss adding that to the new gpiod API so
we get some traction around that and start to clean things up properly.
I'd really prefer not to see it in the legacy API.

Yours,
Linus Walleij

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Eric Nelson]

On 01/17/2014 01:33 PM, Arnaud Patard (Rtp) wrote:
> Eric Nelson <eric.nelson@boundarydevices.com> writes:
>
>> On 01/17/2014 12:57 PM, Arnaud Patard (Rtp) wrote:
>>> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
>>>
>>>> So, we have this wonderful GPIO layer which abstracts GPIO stuff and
>>>> hides stuff.  It's really wonderful, because you don't have to care
>>>> about how the GPIOs are actually accessed in drivers anymore.
>>>>
>>>> However, what about the behaviour of GPIOs?
>>>>
>>>> What about... for example... this sequence:
>>>>
>>>> 	gpio_direction_output(gpio, 1);
>>>> 	val = gpio_get_value(gpio);
>>>>
>>>> What value is "val"?  More importantly, what value is reflected in
>>>> /sys/kernel/debug/gpio ?  Would it indicate that it's high or low?
>>>>
>>>> Now, while you can make reasonable assumptions, such as "it'll return
>>>> that the output is being driven to the requested state" or "it'll
>>>> return the actual state of the pin", what about this instead, which
>>>> happens on iMX hardware - "it'll _always_ return zero".
>>>>
>>>
>>> this is "expected". gpio layer docs are saying that in output case, the
>>> value may be wrong. Not intuitive but documented.
>>>
>>>> Yes, iMX6 at least has this behaviour.  For any output, val as above
>>>> will always be zero, and /proc/sys/kernel/debug/gpio will always
>>>> report that an output is zero... unless the SION bit has been set for
>>>> that GPIO signal.
>>>
>>> afaik at least imx51/53 have some behaviour.
>>>
>>>>
>>>> The reason is that on hardware such as iMX6, reading the GPIO is done
>>>> by reading the pad state register, and this register is _only_ supplied
>>>> the state of the pad when the input path is enabled.  The input path
>>>> is only enabled when the output is disabled, or the SION bit is set
>>>> to force the GPIO input path.
>>>
>>> I sent mails about this same issue for imx51 in Dec 2010 and answer were
>>> that the SION bit should not be set for all gpios:
>>> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/100875
>>>
>>
>> Thanks Arnaud,
>>
>> This bit from the 2010 chain really needs some explanation:
>>
>>>> Arnaud Patard (Rtp) writes:
>>>>
>>>> <snip>
>>>>
>>>> I had done the same, but had some trouble with this.
>>>> E.g. on our board GPIO1_7 is used as a generic GPIO to enable an
>>>> external clock oscillator for the USBH1 ULPI PHY. When the SION bit
>>>> for this pad was set, I got strange errors on the USBH1 port
>>>> (disconnecting low speed devices behind a hub would stall the
>>>> bus). When I removed the SION bit for that pin everything worked
>>>> well.
>>>>
>>
>> Did you ever chase down the symptom here? Was the GPIO output not
>> holding a constant value such that the oscillator wasn't functioning?
>>
>
> It was not me who got this issue. The issue I had was not being able to
> read GPIO value if set as output. This link may be clearer:
> http://archive.arm.linux.org.uk/lurker/message/20101215.151016.ea731aa7.en.html
>
Thanks Arnaud. That link is quite a bit easier to follow.

I'm looping in Lothar and Dinh.

Dinh's comment in this thread makes no sense:

	>>
	>> The SION bit is a "Software Input On" bit. Basically, if
	>> you set the GPIO as an output, you cannot set the SION bit.
	>>

If that were the case, there's no point to either the bit or the PSR
registers. Dinh, can you or another Freescaler give a definitive
explanation here?

We also had some discussion about this on the U-Boot list regarding

Patch:      http://lists.denx.de/pipermail/u-boot/2013-September/163805.html
Discussion: 
http://lists.denx.de/pipermail/u-boot/2013-October/thread.html#163916

In the U-Boot case, we decided to flag those pins used as both
input and output specfically with the SION flag, which has the
benefit of pointing out to a reader that there's something unusual
going on.

Regards,


Eric

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Linus Walleij]

On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jan 17, 2014 at 01:42:44PM -0700, Stephen Warren wrote:

>> I believe you want gpio_get_value() to return either the driven or
>> actual pin value where it can on the current HW, but just e.g. hard-code
>> 0 on other HW. That would introduce a core feature that works some
>> places but not others, and hence make drivers that relied on the feature
>> less portable between HW with different actual features.
>
> I can buy that argument, but there's an issue which stands squarely in
> its way, and that is open-drain GPIOs.
>
> These are modelled just as any other GPIO, mainly so that both
> gpio_set_value(gpio, 1) and gpio_direction_input(gpio) both result in
> the signal being high.  The only combination which results in the
> signal being driven low is outputting zero - and the state of the signal
> can aways be read back.
>
> The problem here is that such gpios are implemented in things like the
> I2C driver such that they're _always_ outputs, and gpio_set_value() is
> used to pull the signal down.  gpio_get_value() is used to read its
> current state.
>
> So, if we say that gpio_get_value() is undefined, we force such
> subsystems to always jump through the non-open-drain paths (using
> gpio_direction_input() to set the line high and
> gpio_direction_output(gpio, 0) to drive it low.)

Incidentally that is what gpiolib is doing internally in
gpiod_direction_output().

You're absolutely right that it makes no sense to have open
drain (or open source) unless the signal can be read back from
the hardware.

I'm thinking something like if the driver manages to obtain a
GPIO with

gpio_request_one(gpio, GPIOF_OPEN_DRAIN |
                     GPIOF_OUT_INIT_HIGH);

As the I2C core does, and then when that call succeeds, it can
expect that whatever comes back from gpio_get_value() is
always what is actually on the line. If the driver cannot determine
this it should not have allowed that flag to succeed in the first
place, so this might be something we want to enforce.

There are two white spots on the map here:

1. Today this OPEN_DRAIN flag is not even passed down to
the driver so how could it say anything about it :-( it's a pure gpiolib
internal flag. We don't know if the hardware can actually even
do open drain, we just assume it can.

What it should really do - in the best of worlds - is to check if
it can cross-reference the GPIO line to a pin in the pin control
subsystem, and if that is possible, then ask the pin if it
is supporting open drain and set it. It currently has no such
cross-calls, it is just assumed that the configuration is consistent,
and the actual pin is set up as open drain. But it would make
sense to add more cross-calls here, since GPIO is accepting
these flags (OPEN_DRAIN/OPEN_SOURCE).

Like:
int pinctrl_gpio_set_flags(unsigned gpio, unsigned long flags);

Where the pinctrl subsystem would attempt to cross reference
and set the flag, and the pin controller backend will then have
the option to return an error code.

We could atleast support that for the select pin controllers
that use generic pin config. i.MX is another story, but I'm open
to compromises.

2. In the new descriptor API this open drain setting would
be set from the lookup table and be a property on the line,
meaning this flag is not requested explicitly by the consumer,
and the consumer needs to inspect the obtained descriptor
to figure out if it is set to open drain.

Alexandre: do you have plans for how to handle a dynamic
consumer passing flags to its gpio request in the gpiod API?
I noticed that API missing now, there is exactly one user in the
entire kernel, in drivers/i2c/i2c-core.c but a very important one.

I guess to switch the I2C core over to descriptors I could
think of an API like this:

int gpiod_get_flags(const struct gpio_desc *desc);

If the OPEN_DRAIN flag is set on that descriptor we should
always be able to read the input. But as this is not really what the
I2C core wants to know (it really would prefer not to bother with
such GPIO flag details) so is it better if we add a special call to
figure out if the input can be read? Like:

bool gpiod_input_always_valid(const struct gpio_desc *desc);

And leave it up to the core to look at flags, driver characteristics
etc and determine whether the input can be trusted?

Yours,
Linus Walleij

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Eric Nelson]

Hi Uwe,

On 01/17/2014 01:20 PM, Uwe Kleine-K?nig wrote:
> On Fri, Jan 17, 2014 at 12:40:02PM -0700, Stephen Warren wrote:
>> On 01/17/2014 11:47 AM, Russell King - ARM Linux wrote:
>
 > <snip>
>
> Other than that I see five possibilities:
>
 > ...
>   b) check if for the requested gpio pad the SION bit is set and read the
>      pad value if it is, return 0 otherwise. (But note, after thinking
>      again I don't believe this to be possible, because there is usually >1
>      pad that can output a given gpio. Moreover AFAIK the information to
>      which pads a given gpio can be routed is missing in the kernel. That
>      could be fixed, that would result in a big table though. (And the
>      first problem is still unfixed.))

It would be a hassle to collect the data, but it seems that a table
of 2-byte offsets would be < 500 bytes for the 7 banks of GPIOs
on each platform (MX51,53,6DQ,6DLS).

Using this, it would be possible to set SION if/when a GPIO is
configured as an output, which would be much less intrusive than
setting the bit for all GPIO pads.

The current dts files show less than 10 pads being set up as GPIOs of 
any sort for current board files. I suspect that this is because many
are incomplete and leave some pad setup to the boot loader though.

Regards,


Eric

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Alexandre Courbot]

On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Fri, Jan 17, 2014 at 01:42:44PM -0700, Stephen Warren wrote:
>
>>> I believe you want gpio_get_value() to return either the driven or
>>> actual pin value where it can on the current HW, but just e.g. hard-code
>>> 0 on other HW. That would introduce a core feature that works some
>>> places but not others, and hence make drivers that relied on the feature
>>> less portable between HW with different actual features.
>>
>> I can buy that argument, but there's an issue which stands squarely in
>> its way, and that is open-drain GPIOs.
>>
>> These are modelled just as any other GPIO, mainly so that both
>> gpio_set_value(gpio, 1) and gpio_direction_input(gpio) both result in
>> the signal being high.  The only combination which results in the
>> signal being driven low is outputting zero - and the state of the signal
>> can aways be read back.
>>
>> The problem here is that such gpios are implemented in things like the
>> I2C driver such that they're _always_ outputs, and gpio_set_value() is
>> used to pull the signal down.  gpio_get_value() is used to read its
>> current state.
>>
>> So, if we say that gpio_get_value() is undefined, we force such
>> subsystems to always jump through the non-open-drain paths (using
>> gpio_direction_input() to set the line high and
>> gpio_direction_output(gpio, 0) to drive it low.)
>
> Incidentally that is what gpiolib is doing internally in
> gpiod_direction_output().
>
> You're absolutely right that it makes no sense to have open
> drain (or open source) unless the signal can be read back from
> the hardware.
>
> I'm thinking something like if the driver manages to obtain a
> GPIO with
>
> gpio_request_one(gpio, GPIOF_OPEN_DRAIN |
>                      GPIOF_OUT_INIT_HIGH);
>
> As the I2C core does, and then when that call succeeds, it can
> expect that whatever comes back from gpio_get_value() is
> always what is actually on the line. If the driver cannot determine
> this it should not have allowed that flag to succeed in the first
> place, so this might be something we want to enforce.
>
> There are two white spots on the map here:
>
> 1. Today this OPEN_DRAIN flag is not even passed down to
> the driver so how could it say anything about it :-( it's a pure gpiolib
> internal flag. We don't know if the hardware can actually even
> do open drain, we just assume it can.
>
> What it should really do - in the best of worlds - is to check if
> it can cross-reference the GPIO line to a pin in the pin control
> subsystem, and if that is possible, then ask the pin if it
> is supporting open drain and set it. It currently has no such
> cross-calls, it is just assumed that the configuration is consistent,
> and the actual pin is set up as open drain. But it would make
> sense to add more cross-calls here, since GPIO is accepting
> these flags (OPEN_DRAIN/OPEN_SOURCE).

This would definitely work in the case of pinctrl-backed GPIOs, but
would not cover all GPIO chips. If we want to cover all cases we
should give drivers a way to way to report or enforce this capability,
and make the pinctrl cross-reference one of its implementations where
it can be done.

>
> Like:
> int pinctrl_gpio_set_flags(unsigned gpio, unsigned long flags);
>
> Where the pinctrl subsystem would attempt to cross reference
> and set the flag, and the pin controller backend will then have
> the option to return an error code.
>
> We could atleast support that for the select pin controllers
> that use generic pin config. i.MX is another story, but I'm open
> to compromises.
>
> 2. In the new descriptor API this open drain setting would
> be set from the lookup table and be a property on the line,
> meaning this flag is not requested explicitly by the consumer,
> and the consumer needs to inspect the obtained descriptor
> to figure out if it is set to open drain.
>
> Alexandre: do you have plans for how to handle a dynamic
> consumer passing flags to its gpio request in the gpiod API?

Do you mean like passing OPEN_DRAIN or OPEN_SOURCE flags to
gpiod_get(), similarly to what is done for e.g. gpio_request_one()?

In the case of the gpiod API I would rather see these flags defined in
the GPIO mapping if possible. For platform data it is already possible
to specify open drain/open source, for DT this is trivial to add. ACPI
would be more of a problem here, but I'm not sure whether the problem
is relevant for ACPI GPIOs.

So the way I see it coming into shape would be something like:

1) GPIO drivers' request() function get an extra flags argument that
is passed by the GPIO core with the flags of the mapping. There we can
define all the range of properties that gpio_request_one() supported.
The driver's request() will fail it if cannot satisfy these
properties. That's where the pinctrl cross-reference would take place.

2) All properties accepted by gpio_request_one() can also be passed
through GPIO mappings. That is, probably platform_data and DT. I don't
know if we ever get to use open drain GPIOs provided by ACPI, if we do
then this might be a problem.

This does not address the initial problem, which is the uncertainty of
values returned by input GPIOs. For this either we enforce some more
strict rules, or we provide a function to allow drivers to check how
the value is reported, which is what you proposed below:

> I noticed that API missing now, there is exactly one user in the
> entire kernel, in drivers/i2c/i2c-core.c but a very important one.
>
> I guess to switch the I2C core over to descriptors I could
> think of an API like this:
>
> int gpiod_get_flags(const struct gpio_desc *desc);
>
> If the OPEN_DRAIN flag is set on that descriptor we should
> always be able to read the input. But as this is not really what the
> I2C core wants to know (it really would prefer not to bother with
> such GPIO flag details) so is it better if we add a special call to
> figure out if the input can be read? Like:
>
> bool gpiod_input_always_valid(const struct gpio_desc *desc);
>
> And leave it up to the core to look at flags, driver characteristics
> etc and determine whether the input can be trusted?

I am personally a little bit scared by the number of exported
functions in the GPIO framework. It is a pretty large number for
something that is supposed to be simple, so I'd like to avoid adding
more. :) How about the following:

1) GPIOs configured as output without the open drain or open source
flag either return -EINVAL on gpiod_get_value(), or a cached value
tracked by gpiolib for consistency (probably the latter).
2) GPIOs configured as open drain or open source report the actual
value read on the pin, like i2c-core needs. This requires that, for
each GPIO that can be set open drain or open source,
gpiod_input_always_valid() == true.

This is probably naive and needs to be refined, but I wonder if we
could not come with a relatively simple behavior that would lift
ambiguities without complexifying the API. Whatever we come with, we
will also need to think about how we can make the change without
breaking too many users.

Alex.

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Lothar Waßmann]

Hi,

Alexandre Courbot wrote:
> On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> >> On Fri, Jan 17, 2014 at 01:42:44PM -0700, Stephen Warren wrote:
> >
[...]
> > If the OPEN_DRAIN flag is set on that descriptor we should
> > always be able to read the input. But as this is not really what the
> > I2C core wants to know (it really would prefer not to bother with
> > such GPIO flag details) so is it better if we add a special call to
> > figure out if the input can be read? Like:
> >
> > bool gpiod_input_always_valid(const struct gpio_desc *desc);
> >
> > And leave it up to the core to look at flags, driver characteristics
> > etc and determine whether the input can be trusted?
> 
> I am personally a little bit scared by the number of exported
> functions in the GPIO framework. It is a pretty large number for
> something that is supposed to be simple, so I'd like to avoid adding
> more. :) How about the following:
> 
> 1) GPIOs configured as output without the open drain or open source
> flag either return -EINVAL on gpiod_get_value(), or a cached value
> tracked by gpiolib for consistency (probably the latter).
> 2) GPIOs configured as open drain or open source report the actual
> value read on the pin, like i2c-core needs. This requires that, for
> each GPIO that can be set open drain or open source,
> gpiod_input_always_valid() == true.
> 
I would not bind this to the open drain configuration. Any GPIO output
pin may actually be in a different state than programmed when the
output is forcefully driven by another source (shortcut).
So it makes sense to be able to read back the real state of the pad
even for push pull outputs.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

More GPIO madness on iMX6 - and the crappy ARM port of Linux

[Linus Walleij]

On Tue, Jan 21, 2014 at 3:55 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> 1. Today this OPEN_DRAIN flag is not even passed down to
>> the driver so how could it say anything about it :-( it's a pure gpiolib
>> internal flag. We don't know if the hardware can actually even
>> do open drain, we just assume it can.
>>
>> What it should really do - in the best of worlds - is to check if
>> it can cross-reference the GPIO line to a pin in the pin control
>> subsystem, and if that is possible, then ask the pin if it
>> is supporting open drain and set it. It currently has no such
>> cross-calls, it is just assumed that the configuration is consistent,
>> and the actual pin is set up as open drain. But it would make
>> sense to add more cross-calls here, since GPIO is accepting
>> these flags (OPEN_DRAIN/OPEN_SOURCE).
>
> This would definitely work in the case of pinctrl-backed GPIOs, but
> would not cover all GPIO chips. If we want to cover all cases we
> should give drivers a way to way to report or enforce this capability,
> and make the pinctrl cross-reference one of its implementations where
> it can be done.

It can never be done in all cases, since in some cases the
open drain config is just a property of the line and not controlled
by software at all, and the datasheet just says this line is open
drain.

But we should cover the cases where we deal with pure
SW-controlled configs as far as possible.

>> Alexandre: do you have plans for how to handle a dynamic
>> consumer passing flags to its gpio request in the gpiod API?
>
> Do you mean like passing OPEN_DRAIN or OPEN_SOURCE flags to
> gpiod_get(), similarly to what is done for e.g. gpio_request_one()?

Yes...

> In the case of the gpiod API I would rather see these flags defined in
> the GPIO mapping if possible. For platform data it is already possible
> to specify open drain/open source, for DT this is trivial to add.

I figured so much. But we have a consumer in i2c-core.c doing
this for a valid reason.

> ACPI
> would be more of a problem here, but I'm not sure whether the problem
> is relevant for ACPI GPIOs.

ACPI has an open drain/source flag for some GPIO lines IIRC.

> 1) GPIO drivers' request() function get an extra flags argument that
> is passed by the GPIO core with the flags of the mapping. There we can
> define all the range of properties that gpio_request_one() supported.
> The driver's request() will fail it if cannot satisfy these
> properties. That's where the pinctrl cross-reference would take place.

I think this doesn't need to go all the way down into the driver
actually. We can call to pinctrl in the gpiolib core and
keep the gpiochip API simple. The GPIO driver doesn't even need
to know this.

> 2) All properties accepted by gpio_request_one() can also be passed
> through GPIO mappings. That is, probably platform_data and DT. I don't
> know if we ever get to use open drain GPIOs provided by ACPI, if we do
> then this might be a problem.

I think we need that.

>> Like:
>>
>> bool gpiod_input_always_valid(const struct gpio_desc *desc);
>>
>> And leave it up to the core to look at flags, driver characteristics
>> etc and determine whether the input can be trusted?
>
> I am personally a little bit scared by the number of exported
> functions in the GPIO framework. It is a pretty large number for
> something that is supposed to be simple, so I'd like to avoid adding
> more. :)

I objected already when the OPEN_DRAIN et al were added,
but to no avail...

> How about the following:
>
> 1) GPIOs configured as output without the open drain or open source
> flag either return -EINVAL on gpiod_get_value(), or a cached value
> tracked by gpiolib for consistency (probably the latter).

Make sense.

> 2) GPIOs configured as open drain or open source report the actual
> value read on the pin, like i2c-core needs. This requires that, for
> each GPIO that can be set open drain or open source,
> gpiod_input_always_valid() == true.

Yeah, but as seen from the I2C core, the algorithm there needs
to know if the input is always valid or not, and take different
execution paths depending on that. :-/

Yours,
Linus Walleij