[PATCH] gpio: zynq: Setup chip->base based on alias ID

[PATCH] gpio: zynq: Setup chip->base based on alias ID

[Michal Simek]

In past Xilinx gpio-zynq driver was setting up gpio chip->base as 0
which was chagned to autodetection when driver was upstreamed. Older
systems, which were using this old version, setup SW stack which expects
zynq gpio base as 0 and right now there is no way how to set this up.

The patch is adding an option to setup chip->base based on aliases which
is something what some other drivers are doing too.
It means when gpio0 alias is setup then chip->base is 0. When gpio alias
is not setup gpiochip_find_base() set it up properly which is current
behavior.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 drivers/gpio/gpio-zynq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 23bb10576def..412cb5f31a9b 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -831,7 +831,7 @@ static int zynq_gpio_probe(struct platform_device *pdev)
 	chip->free = zynq_gpio_free;
 	chip->direction_input = zynq_gpio_dir_in;
 	chip->direction_output = zynq_gpio_dir_out;
-	chip->base = -1;
+	chip->base = of_alias_get_id(pdev->dev.of_node, "gpio");
 	chip->ngpio = gpio->p_data->ngpio;
 
 	/* Retrieve GPIO clock */
-- 
2.17.0

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Linus Walleij]

On Wed, Apr 11, 2018 at 3:55 PM, Michal Simek <michal.simek@xilinx.com> wrote:

Thanks for your patch!

> In past Xilinx gpio-zynq driver was setting up gpio chip->base as 0
> which was chagned to autodetection when driver was upstreamed. Older
> systems, which were using this old version, setup SW stack which expects
> zynq gpio base as 0 and right now there is no way how to set this up.
>
> The patch is adding an option to setup chip->base based on aliases which
> is something what some other drivers are doing too.
> It means when gpio0 alias is setup then chip->base is 0. When gpio alias
> is not setup gpiochip_find_base() set it up properly which is current
> behavior.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

In general, we stopped any controlling of the GPIO base.

Also this use would have to be OK with the DT maintainers
as I never saw this use of alias before.

Please describe the use case for this.

The only use case which I can think about is userspace sysfs
and then I would really like to know why these userspace
users cannot use the character device that is nowadays
supported by libgpiod and there is even patches for some
IoT libraries to use it. The character device makes the
GPIO Linux "base" irrelevant for userspace.

GPIO sysfs is deprecated and moved to the obsolete ABI.

If there are legacy applications that use this I would have
to consider it, but since this has been -1 since the driver
was merged I find that unlikely.

Yours,
Linus Walleij

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Michal Simek]

Hi Linus,

On 26.4.2018 15:08, Linus Walleij wrote:
> On Wed, Apr 11, 2018 at 3:55 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> 
> Thanks for your patch!
> 
>> In past Xilinx gpio-zynq driver was setting up gpio chip->base as 0
>> which was chagned to autodetection when driver was upstreamed. Older
>> systems, which were using this old version, setup SW stack which expects
>> zynq gpio base as 0 and right now there is no way how to set this up.
>>
>> The patch is adding an option to setup chip->base based on aliases which
>> is something what some other drivers are doing too.
>> It means when gpio0 alias is setup then chip->base is 0. When gpio alias
>> is not setup gpiochip_find_base() set it up properly which is current
>> behavior.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> 
> In general, we stopped any controlling of the GPIO base.

I know.

> 
> Also this use would have to be OK with the DT maintainers
> as I never saw this use of alias before.

If you grep gpio drivers you will see that these gpio-zx, gpio-mvebu,
gpio-mxc, gpio-mxs, gpio-vf610 are using that.

> 
> Please describe the use case for this.
> 
> The only use case which I can think about is userspace sysfs
> and then I would really like to know why these userspace
> users cannot use the character device that is nowadays
> supported by libgpiod and there is even patches for some
> IoT libraries to use it. The character device makes the
> GPIO Linux "base" irrelevant for userspace.
> 
> GPIO sysfs is deprecated and moved to the obsolete ABI.
> 
> If there are legacy applications that use this I would have
> to consider it, but since this has been -1 since the driver
> was merged I find that unlikely.

Yes, it is about legacy application which I have seen recently and there
is no source code for application calls it because board vendor doesn't
provide it.

You are right that -1 was used from the beginning in mainline but
unfortunately this driver was in vendor tree for a while and it uses 0
there.

In upstreaming this was changed to -1 but customers have a lot of code
which developed against vendor tree and they want to use
latest&greatest. And without this they are not able to run that
applications.

I found that this logic is already in 5 drivers in mainline that's why I
send this patch to be +1.

Thanks,
Michal

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Linus Walleij]

On Thu, Apr 26, 2018 at 3:35 PM, Michal Simek <michal.simek@xilinx.com> wrote:

>> The only use case which I can think about is userspace sysfs
>> and then I would really like to know why these userspace
>> users cannot use the character device that is nowadays
>> supported by libgpiod and there is even patches for some
>> IoT libraries to use it. The character device makes the
>> GPIO Linux "base" irrelevant for userspace.
>>
>> GPIO sysfs is deprecated and moved to the obsolete ABI.
>>
>> If there are legacy applications that use this I would have
>> to consider it, but since this has been -1 since the driver
>> was merged I find that unlikely.
>
> Yes, it is about legacy application which I have seen recently and there
> is no source code for application calls it because board vendor doesn't
> provide it.
>
> You are right that -1 was used from the beginning in mainline but
> unfortunately this driver was in vendor tree for a while and it uses 0
> there.
>
> In upstreaming this was changed to -1 but customers have a lot of code
> which developed against vendor tree and they want to use
> latest&greatest. And without this they are not able to run that
> applications.
>
> I found that this logic is already in 5 drivers in mainline that's why I
> send this patch to be +1.

I see.

Sadly comaptibility with out-of-tree driver code is none of our
(community) business.

We do pay a lot of effort not to break the ABI to userspace, but
it needs to be an ABI coming from the mainline kernel, not from
a vendor tree.

So to the mainline kernel this is no regression.

Yours,
Linus Walleij

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Michal Simek]

On 2.5.2018 12:10, Linus Walleij wrote:
> On Thu, Apr 26, 2018 at 3:35 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> 
>>> The only use case which I can think about is userspace sysfs
>>> and then I would really like to know why these userspace
>>> users cannot use the character device that is nowadays
>>> supported by libgpiod and there is even patches for some
>>> IoT libraries to use it. The character device makes the
>>> GPIO Linux "base" irrelevant for userspace.
>>>
>>> GPIO sysfs is deprecated and moved to the obsolete ABI.
>>>
>>> If there are legacy applications that use this I would have
>>> to consider it, but since this has been -1 since the driver
>>> was merged I find that unlikely.
>>
>> Yes, it is about legacy application which I have seen recently and there
>> is no source code for application calls it because board vendor doesn't
>> provide it.
>>
>> You are right that -1 was used from the beginning in mainline but
>> unfortunately this driver was in vendor tree for a while and it uses 0
>> there.
>>
>> In upstreaming this was changed to -1 but customers have a lot of code
>> which developed against vendor tree and they want to use
>> latest&greatest. And without this they are not able to run that
>> applications.
>>
>> I found that this logic is already in 5 drivers in mainline that's why I
>> send this patch to be +1.
> 
> I see.
> 
> Sadly comaptibility with out-of-tree driver code is none of our
> (community) business.
> 
> We do pay a lot of effort not to break the ABI to userspace, but
> it needs to be an ABI coming from the mainline kernel, not from
> a vendor tree.
> 
> So to the mainline kernel this is no regression.

I understand your statement. On the other hand it is feature which was
permitted in past for some drivers and this is +1.

Thanks,
Michal

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Linus Walleij]

On Wed, May 2, 2018 at 12:15 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 2.5.2018 12:10, Linus Walleij wrote:
>> On Thu, Apr 26, 2018 at 3:35 PM, Michal Simek <michal.simek@xilinx.com> wrote:

>>> Yes, it is about legacy application which I have seen recently and there
>>> is no source code for application calls it because board vendor doesn't
>>> provide it.
>>>
>>> You are right that -1 was used from the beginning in mainline but
>>> unfortunately this driver was in vendor tree for a while and it uses 0
>>> there.
>>>
>>> In upstreaming this was changed to -1 but customers have a lot of code
>>> which developed against vendor tree and they want to use
>>> latest&greatest. And without this they are not able to run that
>>> applications.
>>>
>>> I found that this logic is already in 5 drivers in mainline that's why I
>>> send this patch to be +1.
>>
>> I see.
>>
>> Sadly comaptibility with out-of-tree driver code is none of our
>> (community) business.
>>
>> We do pay a lot of effort not to break the ABI to userspace, but
>> it needs to be an ABI coming from the mainline kernel, not from
>> a vendor tree.
>>
>> So to the mainline kernel this is no regression.
>
> I understand your statement. On the other hand it is feature which was
> permitted in past for some drivers and this is +1.

But was it permitted because of breaking out of tree code?

I know people have done questionable things in the past, but
two wrongs does not make one right.

Yours,
Linus Walleij

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Michal Simek]

On 2.5.2018 15:01, Linus Walleij wrote:
> On Wed, May 2, 2018 at 12:15 PM, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 2.5.2018 12:10, Linus Walleij wrote:
>>> On Thu, Apr 26, 2018 at 3:35 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> 
>>>> Yes, it is about legacy application which I have seen recently and there
>>>> is no source code for application calls it because board vendor doesn't
>>>> provide it.
>>>>
>>>> You are right that -1 was used from the beginning in mainline but
>>>> unfortunately this driver was in vendor tree for a while and it uses 0
>>>> there.
>>>>
>>>> In upstreaming this was changed to -1 but customers have a lot of code
>>>> which developed against vendor tree and they want to use
>>>> latest&greatest. And without this they are not able to run that
>>>> applications.
>>>>
>>>> I found that this logic is already in 5 drivers in mainline that's why I
>>>> send this patch to be +1.
>>>
>>> I see.
>>>
>>> Sadly comaptibility with out-of-tree driver code is none of our
>>> (community) business.
>>>
>>> We do pay a lot of effort not to break the ABI to userspace, but
>>> it needs to be an ABI coming from the mainline kernel, not from
>>> a vendor tree.
>>>
>>> So to the mainline kernel this is no regression.
>>
>> I understand your statement. On the other hand it is feature which was
>> permitted in past for some drivers and this is +1.
> 
> But was it permitted because of breaking out of tree code?

You know how soc vendors are working. We need to develop code out of
mainline for certain time because we can't share details before.

When we get permission to upstream code a lot of application code is
done already with certain expectation.

Soren has sent this driver with using -1 for autodetection even at that
time Xilinx gpio driver had 0 there.

This patch is not breaking any current functionality and there is no any
gpio alias in zynq/zynqmp too. And from my point of view the usage of
gpio alias is similar to others aliases present in DTS file.

If you don't want this patch I understand that and it will become just
another soc vendor patch out of mainline.

Thanks,
Michal

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Linus Walleij]

On Wed, May 2, 2018 at 3:41 PM, Michal Simek <michal.simek@xilinx.com> wrote:

> If you don't want this patch I understand that and it will become just
> another soc vendor patch out of mainline.

I don't really know what to do, so that is why I'm discussing.

It's one of those gray areas.

>From one point of view there is the purist stance that we should
only support what the mainline tree does, and be strict and
consistent so we don't accumulate to many nasty hacks.

On the other hand, it is completely possible that all users of this
particular driver actually must have this patch, and then I just
push them to use a deviant vendor tree for no good reason.

Would it be possible that I apply the patch, and somehow also
establish some understanding with all users of the Xilinx
platform that whatever legacy applications are out there
must start to migrate towards using the character device so
this reliance on the numberspace doesn't stick around forever?

For example can we make a patch to some systems like
arch/arm/boot/dts/zynq-*.dts
adding proper GPIO line names to these device trees, such
as was made in e.g. commit f6b1674d570aa1
"arm64: dts: qcom: sbc: Name GPIO lines"

After all that is what I strive for as maintainer, as the IETF
motto says:
"rough consensus and running code"

Yours,
Linus Walleij

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Michal Simek]

On 2.5.2018 15:56, Linus Walleij wrote:
> On Wed, May 2, 2018 at 3:41 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> 
>> If you don't want this patch I understand that and it will become just
>> another soc vendor patch out of mainline.
> 
> I don't really know what to do, so that is why I'm discussing.

me too. It is also interesting that I have met with the case with
zynq/zynqmp gpio driver and not gpio-xilinx.c which can have a lot of
instances.

> 
> It's one of those gray areas.
> 
> From one point of view there is the purist stance that we should
> only support what the mainline tree does, and be strict and
> consistent so we don't accumulate to many nasty hacks.

Also this expect that the first patch does everything right which is not
truth all the time.

> 
> On the other hand, it is completely possible that all users of this
> particular driver actually must have this patch, and then I just
> push them to use a deviant vendor tree for no good reason.
> 
> Would it be possible that I apply the patch, and somehow also
> establish some understanding with all users of the Xilinx
> platform that whatever legacy applications are out there
> must start to migrate towards using the character device so
> this reliance on the numberspace doesn't stick around forever?

When someone contacts me for asking guidance for gpio I am telling them
not to use legacy sysfs interface and use libgpiod. Last one was a week
ago in connection to Ultra96 and libmraa.

But even chardev is not supported there now.
https://github.com/intel-iot-devkit/mraa/issues/713

> 
> For example can we make a patch to some systems like
> arch/arm/boot/dts/zynq-*.dts
> adding proper GPIO line names to these device trees, such
> as was made in e.g. commit f6b1674d570aa1
> "arm64: dts: qcom: sbc: Name GPIO lines"

If you take a look at
arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
which is Ultra96 board gpio-line-names are filled there for the whole PS
part. Definitely take a look and let know if you find out any issue there.

zynq/zynqmp gpio controller contains PS pins (hard part) and PL pins
coming to logic.

I can't describe PL gpio pins because it can be whatever even I have
done that for one fixed hw design.

Interesting part on that sha1 you shared is how "NC" pin is described.

gpio pin 35 I have on zcu100 as "" but it should be maybe TP_PAD which
is really just a pad on real board. And the rest of "" gpio names are
connected to PL.

I am happy to take a look at existing platforms and use gpio-line-names
there. For example arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts
I use in tca6416_u97 and u61 comments instead of this property.

> 
> After all that is what I strive for as maintainer, as the IETF
> motto says:
> "rough consensus and running code"

Thanks,
Michal

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Michal Simek]

Hi Linus,

On 2.5.2018 16:19, Michal Simek wrote:
> On 2.5.2018 15:56, Linus Walleij wrote:
>> On Wed, May 2, 2018 at 3:41 PM, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>>> If you don't want this patch I understand that and it will become just
>>> another soc vendor patch out of mainline.
>>
>> I don't really know what to do, so that is why I'm discussing.
> 
> me too. It is also interesting that I have met with the case with
> zynq/zynqmp gpio driver and not gpio-xilinx.c which can have a lot of
> instances.
> 
>>
>> It's one of those gray areas.
>>
>> From one point of view there is the purist stance that we should
>> only support what the mainline tree does, and be strict and
>> consistent so we don't accumulate to many nasty hacks.
> 
> Also this expect that the first patch does everything right which is not
> truth all the time.
> 
>>
>> On the other hand, it is completely possible that all users of this
>> particular driver actually must have this patch, and then I just
>> push them to use a deviant vendor tree for no good reason.
>>
>> Would it be possible that I apply the patch, and somehow also
>> establish some understanding with all users of the Xilinx
>> platform that whatever legacy applications are out there
>> must start to migrate towards using the character device so
>> this reliance on the numberspace doesn't stick around forever?
> 
> When someone contacts me for asking guidance for gpio I am telling them
> not to use legacy sysfs interface and use libgpiod. Last one was a week
> ago in connection to Ultra96 and libmraa.
> 
> But even chardev is not supported there now.
> https://github.com/intel-iot-devkit/mraa/issues/713
> 
>>
>> For example can we make a patch to some systems like
>> arch/arm/boot/dts/zynq-*.dts
>> adding proper GPIO line names to these device trees, such
>> as was made in e.g. commit f6b1674d570aa1
>> "arm64: dts: qcom: sbc: Name GPIO lines"
> 
> If you take a look at
> arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> which is Ultra96 board gpio-line-names are filled there for the whole PS
> part. Definitely take a look and let know if you find out any issue there.
> 
> zynq/zynqmp gpio controller contains PS pins (hard part) and PL pins
> coming to logic.
> 
> I can't describe PL gpio pins because it can be whatever even I have
> done that for one fixed hw design.
> 
> Interesting part on that sha1 you shared is how "NC" pin is described.
> 
> gpio pin 35 I have on zcu100 as "" but it should be maybe TP_PAD which
> is really just a pad on real board. And the rest of "" gpio names are
> connected to PL.
> 
> I am happy to take a look at existing platforms and use gpio-line-names
> there. For example arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts
> I use in tca6416_u97 and u61 comments instead of this property.
> 

Have you done any decision about this patch?

Thanks,
Michal

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Linus Walleij]

On Tue, May 15, 2018 at 3:26 PM, Michal Simek <michal.simek@xilinx.com> wrote:

> Have you done any decision about this patch?

Yeah I applied the patch. It's a bit of "choose the lesser evil" approach
but what can I do...

Yours,
Linus Walleij

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Michal Simek]

On 23.5.2018 11:44, Linus Walleij wrote:
> On Tue, May 15, 2018 at 3:26 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> 
>> Have you done any decision about this patch?
> 
> Yeah I applied the patch. It's a bit of "choose the lesser evil" approach
> but what can I do...

Thanks,
Michal

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Linus Walleij]

On Wed, May 2, 2018 at 4:19 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 2.5.2018 15:56, Linus Walleij wrote:

>> Would it be possible that I apply the patch, and somehow also
>> establish some understanding with all users of the Xilinx
>> platform that whatever legacy applications are out there
>> must start to migrate towards using the character device so
>> this reliance on the numberspace doesn't stick around forever?
>
> When someone contacts me for asking guidance for gpio I am telling them
> not to use legacy sysfs interface and use libgpiod.

Awesome, thanks for your support :)

> Last one was a week
> ago in connection to Ultra96 and libmraa.
>
> But even chardev is not supported there now.
> https://github.com/intel-iot-devkit/mraa/issues/713

Hm I hope this happens soon. Manavanninan is doing a great
job in switching this over, I just have to accept that the pace
isn't always what it should be in all upstream projects, and
deal with an imperfect world.

> If you take a look at
> arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> which is Ultra96 board gpio-line-names are filled there for the whole PS
> part. Definitely take a look and let know if you find out any issue there.

It looks good. I even managed to boot my Ultra96 board which
was sent over as price in some competition (great hardware!)

> zynq/zynqmp gpio controller contains PS pins (hard part) and PL pins
> coming to logic.
>
> I can't describe PL gpio pins because it can be whatever even I have
> done that for one fixed hw design.
>
> Interesting part on that sha1 you shared is how "NC" pin is described.
>
> gpio pin 35 I have on zcu100 as "" but it should be maybe TP_PAD which
> is really just a pad on real board. And the rest of "" gpio names are
> connected to PL.

How to handle anything routed to/from programmable logic is
in a bit of mess right now, I understand this work isn't the
easiest :/

Let's go for this patch and try to improve the situation gradually
moving forward.

Yours,
Linus Walleij

Re: [PATCH] gpio: zynq: Setup chip->base based on alias ID

[Michal Simek]

>> If you take a look at
>> arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
>> which is Ultra96 board gpio-line-names are filled there for the whole PS
>> part. Definitely take a look and let know if you find out any issue there.
> 
> It looks good. I even managed to boot my Ultra96 board which
> was sent over as price in some competition (great hardware!)

Wonderful that you get hw available to be closer to issues.
Maybe it would be worth to give you some guidance what to do with PL and
how to add more stuff there especially in connection to GPIO.
We have pinctrl driver in vendor tree but my colleague needs to upstream
firmware interface first.


>> zynq/zynqmp gpio controller contains PS pins (hard part) and PL pins
>> coming to logic.
>>
>> I can't describe PL gpio pins because it can be whatever even I have
>> done that for one fixed hw design.
>>
>> Interesting part on that sha1 you shared is how "NC" pin is described.
>>
>> gpio pin 35 I have on zcu100 as "" but it should be maybe TP_PAD which
>> is really just a pad on real board. And the rest of "" gpio names are
>> connected to PL.
> 
> How to handle anything routed to/from programmable logic is
> in a bit of mess right now, I understand this work isn't the
> easiest :/

I am happy to discuss this to find out a way how this can be handled.
There are several things together.

On arm64 how to disable/enable access for NS software.

Maybe also separate pins PS pins from PL pins. Right now all are
together and not sure if this is ideal in sense of DT overal for
example. Separate nodes would enable better flexibility. Also if you
look at partial reconfiguration you can just route just part of pins
which suggest list of gpios used there.

Thanks,
Michal