| Message ID | 6ee982f8eb6e07f9ecbb0cc5093152f4a16b9c31.1523454899.git.michal.simek@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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
>> 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
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
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 */
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(-)