Re: Active low GPIOs (was [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver)

Re: Active low GPIOs (was [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver)

[Doug Anderson]

Linus,

On Thu, Feb 14, 2013 at 2:01 AM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Feb 14, 2013 at 1:41 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 02/13/2013 05:34 PM, Doug Anderson wrote:
>
>>> A little torn here.  It adds a bunch of complexity to the code to
>>> handle this case and there are no known or anticipated users.  I only
>>> wish that the GPIO polarity could be more hidden from drivers (add
>>> functions like gpio_assert, gpio_deassert, etc)...
>>
>> Yes, that would be nice. Alex, LinusW?
>
> OK so good point since Alex is rewriting the way the external
> API to GPIOs is done.
>
> So this is one of the evolutionary artifacts of the GPIO subsystem:
> that it has this concept of clients having to know if they want to
> drive the line low or high.
>
> Either way somewhere in the system the knowledge of whether
> the low or high state counts as asserted must be stored.
>
> The same goes for inputs really: for example we have a
> platform data flag for drivers/mmc/host/mmci.c that tells
> whether a card is inserted when the line goes low or high. And
> this varies between platforms.
>
> So that would lead to gpio_get_value() being rewritten
> as gpio_is_asserted() as well.

I had a very brief hallway conversation with Olof yesterday and he
proposed that introducing a parallel API might make sense.  AKA:
support both gpio_get_value() and gpio_is_asserted().  One would get
the raw value and one would handle the invert (as needed).  Old code
would keep working and new code could be written better.  There are
also times where you care about the actual logic level of a signal.


> We all agree that the meaning of a certain GPIO pins
> high vs low state as asserter or deasserted is a machine-
> specific thing, so it need to come from device tree or platform
> data.

Yes.  The device tree does have this knowledge today, at least on
Samsung SOCs.  See:
    http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=f447ed8b31da7b24c7c75c2d4624598135b41217

...and there's a generic way to access this with of_get_named_gpio_flags().


> So what this is really all about is whether that knowledge
> should be part of the consumer DT node/platform data
> or the producer DT node/platform data.
>
> I.e. in the MMCI case whether we encode that into the
> DT node/pdata for the GPIO controller or the MMCI
> controller.

This is actually kind of a screwed up case today.  See below.


> A bit like whether to eat eggs from the big or little end
> you could say :-)

The big end.  Definitely the big end.  ;)  ...and I think the bike
shed should be blue.


> But changing it would have very drastic effects.
> Consider this snippet from
> arch/arm/boot/dts/snowball.dts:
>
> // External Micro SD slot
> sdi0_per1@80126000 {
>       arm,primecell-periphid = <0x10480180>;
>       max-frequency = <50000000>;
>       bus-width = <4>;
>       mmc-cap-mmc-highspeed;
>       vmmc-supply = <&ab8500_ldo_aux3_reg>;
>
>       cd-gpios  = <&gpio6 26 0x4>; // 218
>       cd-inverted;

We're actually kinda screwed today.  What happens if you're on a
Samsung SoC and you have:
  cd-gpios = <&gpc3 2 2 0x10003 3>;
  cd-inverted;

What _should_ we do?  I'd argue that we ought to be using
"gpio_is_asserted" and then keeping the existing invert logic.  This
is much better than today where you need to do one of:
* ignore the active low on this GPIO
* ignore the cd-inverted property
* manually apply _both_ of these properties.

We could deprecate cd-inverted and at probably should at least
strongly discourage it.  One argument for keeping "cd-inverted" too is
for controllers that don't use a GPIO for card detect.  In this case
you could imagine a MMC controller that has a "card detect" on
special-purpose pin and accessible via a status register.  You might
have a board that needs to invert the card detect signal for whatever
reason.  Assuming that the controller itself didn't care, it might be
nice to be able to handle the invert in software.  That's a stretch,
but might happen...


> Another issue would be with things like bit-banged buses,
> I don't thing an I2C bus with inverted semantics is that
> very useful, and it makes things hard to debug for the
> user, it's much more clear if the bitbang is driving the
> line high or low than if it's asserting or deasserting it.

Yes, bit-banged i2c is a great example where active low doesn't make
sense.  Assuming we're not going to remove the OF_GPIO_ACTIVE_LOW
flag, though, we'd at least need an assertion fail if someone did set
it.  ...otherwise we end up with flags in the device tree that are
silently ignored (which is the case much of the time today).

In all honesty, though, I've seen some hardware engineers come up with
some pretty darn wacky things to save a few cents.  Would it be
completely absurd for someone to run i2c "backward" if they were
running it through a level translator that had the side effect of
inverting the signal?  That would certainly be a reason to use the
bit-bang mode instead of a normal i2c controller...


-Doug

Re: Active low GPIOs (was [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver)

[Stephen Warren]

On 02/14/2013 10:05 AM, Doug Anderson wrote:
> Linus,
> 
> On Thu, Feb 14, 2013 at 2:01 AM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Thu, Feb 14, 2013 at 1:41 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>> On 02/13/2013 05:34 PM, Doug Anderson wrote:
>>
>>>> A little torn here.  It adds a bunch of complexity to the code to
>>>> handle this case and there are no known or anticipated users.  I only
>>>> wish that the GPIO polarity could be more hidden from drivers (add
>>>> functions like gpio_assert, gpio_deassert, etc)...
>>>
>>> Yes, that would be nice. Alex, LinusW?
>>
>> OK so good point since Alex is rewriting the way the external
>> API to GPIOs is done.
>>
>> So this is one of the evolutionary artifacts of the GPIO subsystem:
>> that it has this concept of clients having to know if they want to
>> drive the line low or high.
...
>> But changing it would have very drastic effects.
>> Consider this snippet from
>> arch/arm/boot/dts/snowball.dts:
>>
>> // External Micro SD slot
>> sdi0_per1@80126000 {
>>       arm,primecell-periphid = <0x10480180>;
>>       max-frequency = <50000000>;
>>       bus-width = <4>;
>>       mmc-cap-mmc-highspeed;
>>       vmmc-supply = <&ab8500_ldo_aux3_reg>;
>>
>>       cd-gpios  = <&gpio6 26 0x4>; // 218
>>       cd-inverted;
> 
> We're actually kinda screwed today.  What happens if you're on a
> Samsung SoC and you have:
>   cd-gpios = <&gpc3 2 2 0x10003 3>;
>   cd-inverted;
> 
> What _should_ we do?  I'd argue that we ought to be using
> "gpio_is_asserted" and then keeping the existing invert logic.  This
> is much better than today where you need to do one of:
> * ignore the active low on this GPIO
> * ignore the cd-inverted property
> * manually apply _both_ of these properties.

This is actually why I rather dislike the idea that device tree has an
immutable bindings. To some exten, I imagine that much of
Documentation/stable_api_nonsense.txt could apply against device tree!
We can't fix problems like this by changing the definition of that
binding to remove the cd-inverted property and rely solely on the GPIO
specifier flags.

And I think it's ironic that you linked to the commit that adds the
inverted flag to the Samsung GPIO specifier; IIRC, Samsung's previous
lack of support for that flags was one of the reasons that a bunch of
bindings added their own foo-inverted or foo-active-high flags, since
not all GPIO specifiers could be relied upon to provide that information:-(

Re: Active low GPIOs (was [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver)

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 527 bytes --]

On Thu, Feb 14, 2013 at 10:48:45AM -0700, Stephen Warren wrote:

> This is actually why I rather dislike the idea that device tree has an
> immutable bindings. To some exten, I imagine that much of
> Documentation/stable_api_nonsense.txt could apply against device tree!
> We can't fix problems like this by changing the definition of that
> binding to remove the cd-inverted property and rely solely on the GPIO
> specifier flags.

OTOH if you do that you've not really changed things from where we were
with platform data...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Active low GPIOs (was [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver)

[Linus Walleij]

On Thu, Feb 14, 2013 at 6:48 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> This is actually why I rather dislike the idea that device tree has an
> immutable bindings. To some exten, I imagine that much of
> Documentation/stable_api_nonsense.txt could apply against device tree!
> We can't fix problems like this by changing the definition of that
> binding to remove the cd-inverted property and rely solely on the GPIO
> specifier flags.

Atleast it could use an "unstable, testing" phase I think.
But there are others who think breaking support for an old
device trees is like breaking userspace.

If we shall set DT bindings in stone they need to go through as
much scrutiny as these IEEE committee products that are
over at openfirmware.org, so we can at least mitigate the risk
of horrid mistakes.

I think it was Wolfram who mentioned some DT binding work
being rushed by engineers constantly keeping a finger
on the fast-forward button and I agree with that observation. :-(

Yours,
Linus Walleij

Re: Active low GPIOs (was [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver)

[Linus Walleij]

On Thu, Feb 14, 2013 at 6:05 PM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Linus,

>> But changing it would have very drastic effects.
>> Consider this snippet from
>> arch/arm/boot/dts/snowball.dts:
>>
>> // External Micro SD slot
>> sdi0_per1@80126000 {
>>       arm,primecell-periphid = <0x10480180>;
>>       max-frequency = <50000000>;
>>       bus-width = <4>;
>>       mmc-cap-mmc-highspeed;
>>       vmmc-supply = <&ab8500_ldo_aux3_reg>;
>>
>>       cd-gpios  = <&gpio6 26 0x4>; // 218
>>       cd-inverted;
>
> We're actually kinda screwed today.  What happens if you're on a
> Samsung SoC and you have:
>   cd-gpios = <&gpc3 2 2 0x10003 3>;
>   cd-inverted;

Hm I thought this was a local binding for the MMCI controller,
but no. Ohwell, it's to be kept around forever then.

> What _should_ we do?  I'd argue that we ought to be using
> "gpio_is_asserted" and then keeping the existing invert logic.  This
> is much better than today where you need to do one of:
> * ignore the active low on this GPIO
> * ignore the cd-inverted property
> * manually apply _both_ of these properties.

So if the GPIO is active low and cd-inverted is specified, we
will take the signal and double-negate it so we end up with
an uninverted signal.

So there is suddenly two ways to express "passthru".
well, we usally type !! double-invert in code so why not.
I'm more worrying about the maintainability of this
going forward.

> We could deprecate cd-inverted and at probably should at least
> strongly discourage it.

We also have wp-inverted.

These were added a while back, I guess Arnd and Chris will just
love to discuss this :-)

>  One argument for keeping "cd-inverted" too is
> for controllers that don't use a GPIO for card detect.
>  In this case
> you could imagine a MMC controller that has a "card detect" on
> special-purpose pin and accessible via a status register.

This is actually the case with Integrator/CP and Versatile/AB.

>> Another issue would be with things like bit-banged buses,
>> I don't thing an I2C bus with inverted semantics is that
>> very useful, and it makes things hard to debug for the
>> user, it's much more clear if the bitbang is driving the
>> line high or low than if it's asserting or deasserting it.
>
> Yes, bit-banged i2c is a great example where active low doesn't make
> sense.  Assuming we're not going to remove the OF_GPIO_ACTIVE_LOW
> flag, though, we'd at least need an assertion fail if someone did set
> it.  ...otherwise we end up with flags in the device tree that are
> silently ignored (which is the case much of the time today).
>
> In all honesty, though, I've seen some hardware engineers come up with
> some pretty darn wacky things to save a few cents.  Would it be
> completely absurd for someone to run i2c "backward" if they were
> running it through a level translator that had the side effect of
> inverting the signal?  That would certainly be a reason to use the
> bit-bang mode instead of a normal i2c controller...

We'll handle that as it comes around I guess, we cannot
upfront-design our way throughout the entire thinkable
universe :-)

Yours,
Linus Walleij

Re: Active low GPIOs (was [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver)

[Stephen Warren]

On 02/14/2013 05:02 PM, Linus Walleij wrote:
> On Thu, Feb 14, 2013 at 6:05 PM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
...
>>  One argument for keeping "cd-inverted" too is
>> for controllers that don't use a GPIO for card detect.
>>  In this case
>> you could imagine a MMC controller that has a "card detect" on
>> special-purpose pin and accessible via a status register.
> 
> This is actually the case with Integrator/CP and Versatile/AB.

In this case, I assume that the driver for the HW has custom code to
read the MMC controller's CD register bit, and hence it knows whether
the HW inverts it, and hence we don't need a property in DT to say so;
the driver will simply read the bit, invert it, and return it all
transparently? After all, the inversion isn't board-specific but IP
block specific.

Re: Active low GPIOs (was [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver)

[Linus Walleij]

On Fri, Feb 15, 2013 at 1:19 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 02/14/2013 05:02 PM, Linus Walleij wrote:
>> On Thu, Feb 14, 2013 at 6:05 PM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> ...
>>>  One argument for keeping "cd-inverted" too is
>>> for controllers that don't use a GPIO for card detect.
>>>  In this case
>>> you could imagine a MMC controller that has a "card detect" on
>>> special-purpose pin and accessible via a status register.
>>
>> This is actually the case with Integrator/CP and Versatile/AB.
>
> In this case, I assume that the driver for the HW has custom code to
> read the MMC controller's CD register bit, and hence it knows whether
> the HW inverts it, and hence we don't need a property in DT to say so;
> the driver will simply read the bit, invert it, and return it all
> transparently? After all, the inversion isn't board-specific but IP
> block specific.

No that's not how it works ... heh.

It calls back to the platform:

include/linux/amba/mmci.h
 * @status: if no GPIO read function was given to the block in
 * gpio_wp (below) this function will be called to determine
 * whether a card is present in the MMC slot or not
(...)
struct mmci_platform_data {
(...)
        unsigned int (*status)(struct device *);
(...)
        bool    cd_invert;

So the flag tells whether that signal should be interpreted
as inverted or not. The same flag is used for GPIO
insertion detection.

The code reading the status will read a certain register
like this (arch/arm/mach-integrator_cp.c):

static unsigned int mmc_status(struct device *dev)
{
        unsigned int status = readl(__io_address(0xca000000 + 4));
        writel(8, intcp_con_base + 8);

        return status & 8;
}

0xca000000 + 4 is just the second 32bit word in the system
controller. This address range and that very word is used
for various stuff, so it's not like a general-purpose GPIO
or anything, it's just that one pin being readable throgh that
very bit.

Typical prototype-glue-ASIC-to-this-very-board design
pattern. Generic GPIO was not thought of at the time,
it was introduced in later ARM reference designs.

Yours,
Linus Walleij

Re: Active low GPIOs (was [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver)

[Stephen Warren]

On 02/15/2013 01:34 PM, Linus Walleij wrote:
> On Fri, Feb 15, 2013 at 1:19 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 02/14/2013 05:02 PM, Linus Walleij wrote:
>>> On Thu, Feb 14, 2013 at 6:05 PM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> ...
>>>>  One argument for keeping "cd-inverted" too is
>>>> for controllers that don't use a GPIO for card detect.
>>>>  In this case
>>>> you could imagine a MMC controller that has a "card detect" on
>>>> special-purpose pin and accessible via a status register.
>>>
>>> This is actually the case with Integrator/CP and Versatile/AB.
>>
>> In this case, I assume that the driver for the HW has custom code to
>> read the MMC controller's CD register bit, and hence it knows whether
>> the HW inverts it, and hence we don't need a property in DT to say so;
>> the driver will simply read the bit, invert it, and return it all
>> transparently? After all, the inversion isn't board-specific but IP
>> block specific.
> 
> No that's not how it works ... heh.
> 
> It calls back to the platform:
> 
> include/linux/amba/mmci.h
>  * @status: if no GPIO read function was given to the block in
>  * gpio_wp (below) this function will be called to determine
>  * whether a card is present in the MMC slot or not
> (...)
> struct mmci_platform_data {
> (...)
>         unsigned int (*status)(struct device *);
> (...)
>         bool    cd_invert;
> 
> So the flag tells whether that signal should be interpreted
> as inverted or not. The same flag is used for GPIO
> insertion detection.
> 
> The code reading the status will read a certain register
> like this (arch/arm/mach-integrator_cp.c):
> 
> static unsigned int mmc_status(struct device *dev)
> {
>         unsigned int status = readl(__io_address(0xca000000 + 4));
>         writel(8, intcp_con_base + 8);
> 
>         return status & 8;
> }
> 
> 0xca000000 + 4 is just the second 32bit word in the system
> controller. This address range and that very word is used
> for various stuff, so it's not like a general-purpose GPIO
> or anything, it's just that one pin being readable throgh that
> very bit.

Surely a GPIO is exactly what that is...

It might be annoying to create a GPIO controller driver for just that,
but that seems like the simplest way to implement that HW from a device
tree perspective. With DT, it'd be painful to plug in that board
callback into the platform data there.

Re: Active low GPIOs (was [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver)

[Linus Walleij]

On Fri, Feb 15, 2013 at 9:42 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 02/15/2013 01:34 PM, Linus Walleij wrote:

>> 0xca000000 + 4 is just the second 32bit word in the system
>> controller. This address range and that very word is used
>> for various stuff, so it's not like a general-purpose GPIO
>> or anything, it's just that one pin being readable throgh that
>> very bit.
>
> Surely a GPIO is exactly what that is...
>
> It might be annoying to create a GPIO controller driver for just that,
> but that seems like the simplest way to implement that HW from a device
> tree perspective. With DT, it'd be painful to plug in that board
> callback into the platform data there.

Yeah well, I'll get to it sooner or later.

I have seen cases of cherry-picked registers being routed
to one driver etc (like an MFD device) but this is the first
instance where we need to cherry-pick individual bits over to
a separate driver.

This will be a 1-bit, input-only gpio pin, npins = 1....

But hey, they buils spaceships for just one person so why
not :-)

Yours,
Linus Walleij

Re: Active low GPIOs (was [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver)

[Alex Courbot]

> On Thu, Feb 14, 2013 at 2:01 AM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Thu, Feb 14, 2013 at 1:41 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>> On 02/13/2013 05:34 PM, Doug Anderson wrote:
>>
>>>> A little torn here.  It adds a bunch of complexity to the code to
>>>> handle this case and there are no known or anticipated users.  I only
>>>> wish that the GPIO polarity could be more hidden from drivers (add
>>>> functions like gpio_assert, gpio_deassert, etc)...
>>>
>>> Yes, that would be nice. Alex, LinusW?
>>
>> OK so good point since Alex is rewriting the way the external
>> API to GPIOs is done.
>>
>> So this is one of the evolutionary artifacts of the GPIO subsystem:
>> that it has this concept of clients having to know if they want to
>> drive the line low or high.
>>
>> Either way somewhere in the system the knowledge of whether
>> the low or high state counts as asserted must be stored.

Interestingly this knowledge is already more or less stored in GPIOlib - 
all GPIOs descriptors have a FLAG_ACTIVE_LOW, but it can only be set 
through the sysfs interface does not affect the in-kernel APIs. It does 
affect what is returned by the "value" sysfs node, though.

If we are to introduce something similar for the in-kernel interface, it 
would be nice to keep it consistent with the sysfs behavior, otherwise 
it will become a nightmare to maintain two different polarity settings 
for in-kernel and sysfs interfaces. The introduction of the gpiod 
interface could be helpful here. Since it would work with descriptors 
obtained through gpiod_get(device, consumer), the polarity could be set 
transparently from either the DT or platform data (which would then have 
a "flags" member). Then gpiod_set_value/get_value would take it into 
account, and behave just like the sysfs interface.

So, in short:
- old gpio_* interface would *not* be polarity-aware
- new gpiod_* interface *would* be polarity-aware (and get the settings 
from DT/platform data)

This looks simple but I don't have enough altitude to be sure we won't 
hit some walls by doing so.

This would make the new gpiod and legacy gpio interfaces behave 
differently, but at the same time is a good opportunity to have the 
behavior as sysfs.

An undesirable side-effect would be that the legacy gpio interface could 
not be built as a simple wrapper around gpiod anymore. That's sad, but 
presenting interfaces that make sense is more important.

As a side-note, there is also this gpio_sysfs_set_active_low() public 
interface that only mach-at91 seems to use (once!). Maybe we could try 
to get rid of it?

Alex.