| Message ID | 1553135724-38331-1-git-send-email-zhuchangchun@cvte.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: intel: Implements gpio free function | expand |
On Thu, Mar 21, 2019 at 10:35:24AM +0800, zhuchangchun wrote: > When we use the gpio to control some peripheral devices,and try to > export the gpio first,then unexport the gpio, we test the signal with > oscilloscope,and find the signal can't meet the requirements,because > after we unexported the gpio,the gpio's register(tx and rx)value can't > be recovered,and this will infruence the device work flow. After you unexport GPIO it can go back to any previous mode it was. If you need to use it as GPIO then why unexport it in the first place? > We check the gpio's unexport code work flow, then find the gpio's free > hook function has not been implemented, After we add pinmux_ops' free > function to set exported gpio to recover its original value,the problem > is fixed. I don't think this is what ->free callback should do (assuming we decide to implement it since we don't implement ->release either). It is supposed to reverse effects of ->request which is what it currently does ;-)
On Thu, Mar 21, 2019 at 10:44:20AM +0200, Mika Westerberg wrote: > On Thu, Mar 21, 2019 at 10:35:24AM +0800, zhuchangchun wrote: > > When we use the gpio to control some peripheral devices,and try to > > export the gpio first,then unexport the gpio, we test the signal with > > oscilloscope,and find the signal can't meet the requirements,because > > after we unexported the gpio,the gpio's register(tx and rx)value can't > > be recovered,and this will infruence the device work flow. > > After you unexport GPIO it can go back to any previous mode it was. If > you need to use it as GPIO then why unexport it in the first place? ...and on top of that GPIO sysfs interface is deprecated. > > We check the gpio's unexport code work flow, then find the gpio's free > > hook function has not been implemented, After we add pinmux_ops' free > > function to set exported gpio to recover its original value,the problem > > is fixed. > > I don't think this is what ->free callback should do (assuming we decide > to implement it since we don't implement ->release either). It is > supposed to reverse effects of ->request which is what it currently does ;-) Exactly!
On Thu, Mar 21, 2019 at 07:20:56PM +0800, zhuchangchun@cvte.com wrote: > After you unexport GPIO it can go back to any previous mode it was. If > you need to use it as GPIO then why unexport it in the first place? > --> because we need use the GPIO for device reset, we want to recover > the GPIO value after reset done.If we don't unexport it, then reboot > the > intel SOC(not power off), the GPIO's value will stay until the GPIO be > reinit. > I tested the GPIO signal with oscilloscope, if power off the SOC, > then > power on, the GPIO value can recover its original value. This will > infruence > the peripheral device working condition. If I understand correctly you have some peripheral connected to Intel based board and one GPIO is used to reset the peripheral. You say it works fine if you power cycle the board but it does not when you issue 'reboot' command. Did I understood the problem correctly?
On Thu, Mar 21, 2019 at 08:22:40PM +0800, zhuchangchun@cvte.com wrote: > On Thu, Mar 21, 2019 at 07:20:56PM +0800, zhuchangchun@cvte.com wrote: > > After you unexport GPIO it can go back to any previous mode it > was. If > > you need to use it as GPIO then why unexport it in the first > place? > > --> because we need use the GPIO for device reset, we want to > recover > > the GPIO value after reset done.If we don't unexport it, then > reboot > > the > > intel SOC(not power off), the GPIO's value will stay until the > GPIO be > > reinit. > > I tested the GPIO signal with oscilloscope, if power off the SOC, > > then > > power on, the GPIO value can recover its original value. This will > > infruence > > the peripheral device working condition. > > If I understand correctly you have some peripheral connected to Intel > based board and one GPIO is used to reset the peripheral. > > You say it works fine if you power cycle the board but it does not when > you issue 'reboot' command. Did I understood the problem correctly? > > --> Yes , so I add the -> free callback, it fixes the problem. OK, so you then write 1 to the sysfs to assert reset, right? And in your patch you change the mode to input effectively deasserting the reset. Why not simply do following instead of unexport? # echo 0 > /sys/class/gpio/gpioXX/value or even # echo 0 > /sys/class/gpio/gpioXX/value # echo in > /sys/class/gpio/gpioXX/direction Am I missing something?
On Thu, Mar 21, 2019 at 09:35:26PM +0800, zhuchangchun@cvte.com wrote: > --> I know your meaning, even though I've did following actions > instead of unexport, > > # echo 0 > /sys/class/gpio/gpioXX/value > # echo in > /sys/class/gpio/gpioXX/direction > > the padcfg0 value still can not roll back,because after export the > gpio,the following > > action as padcfg0 settings was done. > > Exactly TX and RX register have been set as below in GPIO request: > > /* Disable TX buffer and enable RX (this will be input) */ > > value &= ~PADCFG0_GPIORXDIS; > > value |= PADCFG0_GPIOTXDIS; > > this will infruence next reboot gpio signal. > > And could you pls see my ->free function implementation? I checked it and you do this: + value |= PADCFG0_GPIORXDIS; + value &= ~PADCFG0_GPIOTXDIS; which pretty much turns the pin GPIO output unconditionally. I don't really think it is good idea. May work in your case but may cause problems with others. If you want to keep it as output and leave the value 1 (high) before you issue reboot, I don't think you need to do anything via sysfs (as the pin is already in that state and issuing reboot command should not change anything because the driver does not have ->shutdown callback. In other words, your peripheral expects reset GPIO to be asserted (high) during reboot so unless the boot firmware resets the pin I don't see why leaving it as is does not work for you.
On Fri, Mar 22, 2019 at 11:14:14AM +0800, zhuchangchun@cvte.com wrote: > static int intel_gpio_set_direction(struct pinctrl_dev *pctldev, > struct pinctrl_gpio_range *range, > unsigned pin, bool input) > { > struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > void __iomem *padcfg0; > unsigned long flags; > u32 value; > spin_lock_irqsave(&pctrl->lock, flags); > padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0); > value = readl(padcfg0); > if (input) > value |= PADCFG0_GPIOTXDIS; > else > value &= ~PADCFG0_GPIOTXDIS; > writel(value, padcfg0); > spin_unlock_irqrestore(&pctrl->lock, flags); > return 0; > } > > From above,you can kown when you export a GPIO ,it will do request, > > and there will set TX and RX register at the time same time. > > when you try to set direction in and set value, TX register value can > roll back > > the value,but RX register was not set, so who will set RX value back?? I think you are looking at some older code. There is now function __intel_gpio_set_direction() that is supposed to set both buffers depending on the direction. It was introduced with commit 17fab473693e ("pinctrl: intel: Set pin direction properly").
On 21.03.19 10:23, Andy Shevchenko wrote:
> ...and on top of that GPIO sysfs interface is deprecated.
I don't like the idea of deprecating this. It might not be enough for
all usecases, but for a lot of usecases, it's a very easy and simple
interfaces.
--mtx
On 21.03.19 14:56, Mika Westerberg wrote:
> I checked it and you do this:> > + value |= PADCFG0_GPIORXDIS;> + value &=
~PADCFG0_GPIOTXDIS;> > which pretty much turns the pin GPIO output
unconditionally. I don't> really think it is good idea. May work in your
case but may cause> problems with others.
Uh, please don't do this. In some cases it could be even dangerous.
In general, I'd question whether such an gpio should be userland-
controlled in the first place. Better write a driver for it, which
also handles the reset properly.
--mtx
On Fri, Mar 22, 2019 at 07:32:28PM +0100, Enrico Weigelt, metux IT consult wrote: > On 21.03.19 10:23, Andy Shevchenko wrote: > > > ...and on top of that GPIO sysfs interface is deprecated. > > I don't like the idea of deprecating this. It might not be enough for > all usecases, but for a lot of usecases, it's a very easy and simple > interfaces. So, you probably late for more than year. Linus W. and others discussed that a lot and the points of the choice are listed in documentation IIRC.
On Sat, Mar 23, 2019 at 02:51:52PM +0800, zhuchangchun@cvte.com wrote: > On Fri, Mar 22, 2019 at 11:14:14AM +0800, zhuchangchun@cvte.com wrote: > > From above,you can kown when you export a GPIO ,it will do request, > > > > and there will set TX and RX register at the time same time. > > > > when you try to set direction in and set value, TX register value can > > roll back > > > > the value,but RX register was not set, so who will set RX value back?? > > I think you are looking at some older code. There is now function > __intel_gpio_set_direction() that is supposed to set both buffers > depending on the direction. It was introduced with commit 17fab473693e > ("pinctrl: intel: Set pin direction properly"). > > > --> Yes ,I see the latest master branch, the __intel_gpio_set_direction will > set RX and TX, but I still think we need to implement free function,cause > it will help many other engineers,especailly for some manufactories use some > module,and this modules embeded its own driver but not can be modified, > if they want use the gpio control the module, they may meet the same problem. What problem? Is it reproducible on latest vanilla kernel?
On 22.03.19 20:06, Andy Shevchenko wrote: > On Fri, Mar 22, 2019 at 07:32:28PM +0100, Enrico Weigelt, metux IT consult wrote: >> On 21.03.19 10:23, Andy Shevchenko wrote: >> >>> ...and on top of that GPIO sysfs interface is deprecated. >> >> I don't like the idea of deprecating this. It might not be enough for >> all usecases, but for a lot of usecases, it's a very easy and simple >> interfaces. > > So, you probably late for more than year. Linus W. and others discussed that > a lot and the points of the choice are listed in documentation IIRC. If "deprecated" means there just won't be any new features, but everything remains as it is, I can live w/ that. But But having to rewrite lots of applications in the field for the new interface would be really bad. Note that the dev interface is *much* more complex than the sysfs one. For example, it needs ioctl()s, so this can't be done just w/ a few lines of shellscript anymore. (which is very common in many embedded devices) --mtx
On Mon, Mar 25, 2019 at 10:36:26AM +0100, Enrico Weigelt, metux IT consult wrote: > On 22.03.19 20:06, Andy Shevchenko wrote: > > On Fri, Mar 22, 2019 at 07:32:28PM +0100, Enrico Weigelt, metux IT consult wrote: > >> On 21.03.19 10:23, Andy Shevchenko wrote: > >> > >>> ...and on top of that GPIO sysfs interface is deprecated. > >> > >> I don't like the idea of deprecating this. It might not be enough for > >> all usecases, but for a lot of usecases, it's a very easy and simple > >> interfaces. > > > > So, you probably late for more than year. Linus W. and others discussed that > > a lot and the points of the choice are listed in documentation IIRC. > > If "deprecated" means there just won't be any new features, but > everything remains as it is, I can live w/ that. But But having to > rewrite lots of applications in the field for the new interface would > be really bad. If you don't want to have new features and OK with broken behaviour, yes, that's fine. > Note that the dev interface is *much* more complex than the sysfs one. > For example, it needs ioctl()s, so this can't be done just w/ a few > lines of shellscript anymore. (which is very common in many embedded > devices) I do it with a shell script, hint: libgpiod is a part of almost all alive Linux distributions. P.S. I don't think I would continue wasting time on the topic, since there is nothing proposed how to improve the case.
On Mon, Mar 25, 2019 at 05:52:10PM +0800, zhuchangchun@cvte.com wrote: > On Sat, Mar 23, 2019 at 02:51:52PM +0800, zhuchangchun@cvte.com wrote: > > On Fri, Mar 22, 2019 at 11:14:14AM +0800, zhuchangchun@cvte.com wrote: > > > > From above,you can kown when you export a GPIO ,it will do request, > > > > > > and there will set TX and RX register at the time same time. > > > > > > when you try to set direction in and set value, TX register value can > > > roll back > > > > > > the value,but RX register was not set, so who will set RX value back?? > > > > I think you are looking at some older code. There is now function > > __intel_gpio_set_direction() that is supposed to set both buffers > > depending on the direction. It was introduced with commit 17fab473693e > > ("pinctrl: intel: Set pin direction properly"). > > > > > > --> Yes ,I see the latest master branch, the __intel_gpio_set_direction will > > set RX and TX, but I still think we need to implement free function,cause > > it will help many other engineers,especailly for some manufactories use some > > module,and this modules embeded its own driver but not can be modified, > > if they want use the gpio control the module, they may meet the same problem. > > What problem? > Is it reproducible on latest vanilla kernel? > > --> Not yet, I mean if someone use export GPIO ,then forget to set direction in, > and then set GPIO unexport directly, the GPIO buffer status may influence > the device work flow. If you would like to return pin back to the previous state, it should be done in generic way in the pin control subsystem. Since there is no problem, nothing to fix then.
On Sat, Mar 23, 2019 at 1:32 AM Enrico Weigelt, metux IT consult <lkml@metux.net> wrote: > On 21.03.19 10:23, Andy Shevchenko wrote: > > > ...and on top of that GPIO sysfs interface is deprecated. > > I don't like the idea of deprecating this. It might not be enough for > all usecases, but for a lot of usecases, it's a very easy and simple > interfaces. Yeah it is deprecated for years. It is not like I don't see the simpleness and utility. The reason why it is not good for anyone is simple: Export some GPIOs in sysfs. You script crashes. Who is going to unexport them now? (Answer: noone. Restart machine?) Two scripts at the same time try to use the same GPIO. Such as happen to start the same script twice. What happens now? (Answer: both, with interesting results.) Sure you can make more elaborate scripts that check for stuff already being exported etc. But the chardev on the other hand will protect you from all this. If the program crashes, the lines will be free:ed. If two scripts try to access the same GPIO, they will be denied. And to spice things up, I only add new stuff like open drain and driving several lines at the same time only to the chardev. Yours, Linus Walleij
On 03.04.19 06:13, Linus Walleij wrote:
> But the chardev on the other hand will protect you from all this.> > If the program crashes, the lines will be free:ed.> > If two scripts
try to access the same GPIO, they will be denied.
Right, when you want this concurrency protection and cleanup stiff
the chardev is the better choice. But I've already had several cases
where the simplicity of the sysfs interface is a big win - all you need
few trivial fs operations.
That's also nice for exporting in a grid, eg. via 9P (eg. nice for
quickly building up HIL environments)
ioctls have the bad side effect that they can't be exported via
network in a generic way - your remote fs protocol must support all of
them - even worse: it needs to cope with overlapping ioctl-nr's that
can have entirely different data structures depending on the actual
device. And now try to do that w/ reasonable effort and w/o creating
a shared memory between server and client :p
Another interesting usecase is permission handling:
a) some ioctls need special privileges (oh, how I hate all these "if
(!capable(CAP_SYS_ADMIN)) ..." lines in the drivers), but you wanna
give some unprivileged user access to them
b) you wanna give an unprivileged user access to specific gpio's,
but not to all at once.
With pure filesystem based approach, you can easly define permissions
for each filesystem object.
Yes, these usecases aren't so common for average Jon Doe, but the gpio
subsystem is used that way, out there in the field, and it would be bad
if that functionality would go away someday.
--mtx
On Thu, Apr 04, 2019 at 12:51:35PM +0200, Enrico Weigelt, metux IT consult wrote: > On 03.04.19 06:13, Linus Walleij wrote: > > > But the chardev on the other hand will protect you from all this.> > If the program crashes, the lines will be free:ed.> > If two scripts > try to access the same GPIO, they will be denied. > Right, when you want this concurrency protection and cleanup stiff > the chardev is the better choice. But I've already had several cases > where the simplicity of the sysfs interface is a big win - all you need > few trivial fs operations. > > That's also nice for exporting in a grid, eg. via 9P (eg. nice for > quickly building up HIL environments) > > ioctls have the bad side effect that they can't be exported via > network in a generic way - your remote fs protocol must support all of > them - even worse: it needs to cope with overlapping ioctl-nr's that > can have entirely different data structures depending on the actual > device. And now try to do that w/ reasonable effort and w/o creating > a shared memory between server and client :p > > Another interesting usecase is permission handling: > > a) some ioctls need special privileges (oh, how I hate all these "if > (!capable(CAP_SYS_ADMIN)) ..." lines in the drivers), but you wanna > give some unprivileged user access to them > b) you wanna give an unprivileged user access to specific gpio's, > but not to all at once. > > With pure filesystem based approach, you can easly define permissions > for each filesystem object. Also you may consider gpiod daemon and it's socket interface, for example. Yes, complicated, but solves above problems AFAICT. I guess the best person, missed in Cc, Bartosz, can tell more about user space interaction. And btw gpiod still a good to have for other even local cases: https://github.com/brgl/libgpiod/issues/29 https://github.com/brgl/libgpiod/issues/40 ... > Yes, these usecases aren't so common for average Jon Doe, but the gpio > subsystem is used that way, out there in the field, and it would be bad > if that functionality would go away someday.
On Thu, Apr 4, 2019 at 5:51 PM Enrico Weigelt, metux IT consult <lkml@metux.net> wrote: > On 03.04.19 06:13, Linus Walleij wrote: > > > But the chardev on the other hand will protect you from all this. > > If the program crashes, the lines will be free:ed. > > If two scripts try to access the same GPIO, they will be denied. > Right, when you want this concurrency protection and cleanup stiff > the chardev is the better choice. But I've already had several cases > where the simplicity of the sysfs interface is a big win - all you need > few trivial fs operations. It is admittedly a tradeoff. But if we want something that is hacky and dangerous it should be moved over to debugfs not sitting around in sysfs. In debugfs we give users enough rope all the time. > Another interesting usecase is permission handling: > > a) some ioctls need special privileges (oh, how I hate all these "if > (!capable(CAP_SYS_ADMIN)) ..." lines in the drivers), but you wanna > give some unprivileged user access to them > b) you wanna give an unprivileged user access to specific gpio's, > but not to all at once. > > With pure filesystem based approach, you can easly define permissions > for each filesystem object. This was a comment when we designed the chardev as well, but noone really could give a good usecase for this. Often GPIOs they want to protect should not have been made available to userspace or anyone else in the first place and we now have ways inside the kernel to take GPIOs aside (valid_mask) we can also use hogs (at least in the device tree) to set up GPIOs so that they are held perpetually by the kernel (until reboot) so that userspace cannot try to use them. I can't see that any more access control granularity than the whole gpiochip chardev is really used, not anymore than you want to have access control on every indidual pixel on a framebuffer or each individual block on a block device. Things like requesting and flipping several GPIOs at once could become explosively complex to implement with individual access permissions on each GPIO line, and this is on the other hand a very real and important use case. Yours, Linus Walleij
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index 3b18181..002b9d3 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -480,6 +480,33 @@ static int intel_gpio_set_direction(struct pinctrl_dev *pctldev, return 0; } +static int intel_gpio_free(struct pinctrl_dev *pctldev, unsigned int pin) +{ + struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + void __iomem *padcfg0; + unsigned long flags; + u32 value; + + spin_lock_irqsave(&pctrl->lock, flags); + + if (!intel_pad_usable(pctrl, pin)) { + spin_unlock_irqrestore(&pctrl->lock, flags); + return -EBUSY; + } + padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0); + + /* Put the pad into GPIO mode */ + value = readl(padcfg0) & ~PADCFG0_PMODE_MASK; + /* enable TX buffer and disable RX (this will be input) */ + value |= PADCFG0_GPIORXDIS; + value &= ~PADCFG0_GPIOTXDIS; + writel(value, padcfg0); + + spin_unlock_irqrestore(&pctrl->lock, flags); + + return 0; +} + static const struct pinmux_ops intel_pinmux_ops = { .get_functions_count = intel_get_functions_count, .get_function_name = intel_get_function_name, @@ -487,6 +514,7 @@ static const struct pinmux_ops intel_pinmux_ops = { .set_mux = intel_pinmux_set_mux, .gpio_request_enable = intel_gpio_request_enable, .gpio_set_direction = intel_gpio_set_direction, + .free = intel_gpio_free, }; static int intel_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
When we use the gpio to control some peripheral devices,and try to export the gpio first,then unexport the gpio, we test the signal with oscilloscope,and find the signal can't meet the requirements,because after we unexported the gpio,the gpio's register(tx and rx)value can't be recovered,and this will infruence the device work flow. We check the gpio's unexport code work flow, then find the gpio's free hook function has not been implemented, After we add pinmux_ops' free function to set exported gpio to recover its original value,the problem is fixed. Signed-off-by: zhuchangchun <zhuchangchun@cvte.com> --- drivers/pinctrl/intel/pinctrl-intel.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)