| Message ID | 20210516135531.2203-1-dariobin@libero.it |
|---|---|
| Headers | show |
| Series | am335x: set pinmux registers from pins debug file | expand |
On Sun, May 16, 2021 at 7:43 PM Dario Binacchi <dariobin@libero.it> wrote: > > The MPUs of some architectures (e.g AM335x) must be in privileged > operating mode to write on the pinmux pinmux is not pin configuration. You need to rethink the approach. > registers. In such cases, where > writes will not work from user space, now it can be done from the pins > debug file if the platform driver exports the pin_dbg_set() helper among > the registered operations. Drew, is it similar to what you are trying to achieve? ... > +static ssize_t pinctrl_pins_write(struct file *file, > + const char __user *user_buf, size_t count, > + loff_t *ppos) > +{ > + struct seq_file *s = file->private_data; > + struct pinctrl_dev *pctldev = s->private; > + const struct pinctrl_ops *ops = pctldev->desc->pctlops; > + char buf[32]; > + char *c = &buf[0]; > + char *token; > + int ret, buf_size; > + unsigned int i, pin; > + > + if (!ops->pin_dbg_set) > + return -EFAULT; > + > + /* Get userspace string and assure termination */ > + buf_size = min(count, sizeof(buf) - 1); > + if (copy_from_user(buf, user_buf, buf_size)) > + return -EFAULT; > + > + buf[buf_size] = 0; Can't you use strncpy_from_user() ? > + token = strsep(&c, " "); > + if (kstrtouint(token, 0, &pin)) > + return -EINVAL; Don't shadow an error code. > + for (i = 0; i < pctldev->desc->npins; i++) { > + if (pin != pctldev->desc->pins[i].number) > + continue; Hmm... I don't get this. Why is it needed? > + ret = ops->pin_dbg_set(pctldev, pin, c); > + if (ret) > + return ret; > + > + return count; > + } > + > + return -EINVAL; > +} ... > - debugfs_create_file("pins", 0444, > + debugfs_create_file("pins", 0644, > device_root, pctldev, &pinctrl_pins_fops); Why is it in this file? -- With Best Regards, Andy Shevchenko
On Mon, May 17, 2021 at 11:02:00PM +0300, Andy Shevchenko wrote: > On Sun, May 16, 2021 at 7:43 PM Dario Binacchi <dariobin@libero.it> wrote: > > > > The MPUs of some architectures (e.g AM335x) must be in privileged > > operating mode to write on the pinmux > > pinmux is not pin configuration. You need to rethink the approach. > > > registers. In such cases, where > > writes will not work from user space, now it can be done from the pins > > debug file if the platform driver exports the pin_dbg_set() helper among > > the registered operations. > > Drew, is it similar to what you are trying to achieve? Yes, I would say this similar to what I was trying to accomplish: being able to change contents of conf_<module>_<pin> register [table 9-60] from userspace. However, I was specifically looking to change bits 2:0 which is mux mode. My motivation was to allow BeagleBone users to easily switch between pin functions on the expansion headers during runtime to make rapid prototyping with a breadboard easier (such as changing header pin from GPIO to SPI mode). Most of the header pins have 7 different modes. Ultimately, the solution I settled on with feedback from this list was to create pinmux-select debugfs file that can activate desired fucntion: 6199f6becc86 ("pinctrl: pinmux: Add pinmux-select debugfs file") Bits 6:3 are related to what this subsystem would refer to as pin conf such as slew, input enable and bias. Thus it might make sense to expose something like a select-pinconf file to activate pin conf settings from userspace. This would require using 'pinconf-single' compatible. I fixed pinctrl-single bug regarding pinconf last year so it should be possible to use 'pinconf-single' compatible for the am33xx_pinmux node: f46fe79ff1b6 ("pinctrl-single: fix pcs_parse_pinconf() return value") Thanks, Drew
Hi, > Il 18/05/2021 00:57 Drew Fustini <drew@beagleboard.org> ha scritto: > > > On Mon, May 17, 2021 at 11:02:00PM +0300, Andy Shevchenko wrote: > > On Sun, May 16, 2021 at 7:43 PM Dario Binacchi <dariobin@libero.it> wrote: > > > > > > The MPUs of some architectures (e.g AM335x) must be in privileged > > > operating mode to write on the pinmux > > > > pinmux is not pin configuration. You need to rethink the approach. > > > > > registers. In such cases, where > > > writes will not work from user space, now it can be done from the pins > > > debug file if the platform driver exports the pin_dbg_set() helper among > > > the registered operations. > > > > Drew, is it similar to what you are trying to achieve? > > Yes, I would say this similar to what I was trying to accomplish: being > able to change contents of conf_<module>_<pin> register [table 9-60] > from userspace. > > However, I was specifically looking to change bits 2:0 which is mux > mode. My motivation was to allow BeagleBone users to easily switch > between pin functions on the expansion headers during runtime to make > rapid prototyping with a breadboard easier (such as changing header pin > from GPIO to SPI mode). Most of the header pins have 7 different modes. > > Ultimately, the solution I settled on with feedback from this list was > to create pinmux-select debugfs file that can activate desired fucntion: > 6199f6becc86 ("pinctrl: pinmux: Add pinmux-select debugfs file") > > Bits 6:3 are related to what this subsystem would refer to as pin conf > such as slew, input enable and bias. Thus it might make sense to expose > something like a select-pinconf file to activate pin conf settings from > userspace. This would require using 'pinconf-single' compatible. > > I fixed pinctrl-single bug regarding pinconf last year so it should be > possible to use 'pinconf-single' compatible for the am33xx_pinmux node: > f46fe79ff1b6 ("pinctrl-single: fix pcs_parse_pinconf() return value") > In the kernel version 4.1.6 that I am using on my custom board, I have fixed the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However, this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/). Do you think it is better to bring that functionality back to life or the submitted patch could be fine too? Thanks and regards, Dario > Thanks, > Drew
On Tue, May 18, 2021 at 2:38 AM Dario Binacchi <dariobin@libero.it> wrote: > > Hi, > > > Il 18/05/2021 00:57 Drew Fustini <drew@beagleboard.org> ha scritto: > > > > > > On Mon, May 17, 2021 at 11:02:00PM +0300, Andy Shevchenko wrote: > > > On Sun, May 16, 2021 at 7:43 PM Dario Binacchi <dariobin@libero.it> wrote: > > > > > > > > The MPUs of some architectures (e.g AM335x) must be in privileged > > > > operating mode to write on the pinmux > > > > > > pinmux is not pin configuration. You need to rethink the approach. > > > > > > > registers. In such cases, where > > > > writes will not work from user space, now it can be done from the pins > > > > debug file if the platform driver exports the pin_dbg_set() helper among > > > > the registered operations. > > > > > > Drew, is it similar to what you are trying to achieve? > > > > Yes, I would say this similar to what I was trying to accomplish: being > > able to change contents of conf_<module>_<pin> register [table 9-60] > > from userspace. > > > > However, I was specifically looking to change bits 2:0 which is mux > > mode. My motivation was to allow BeagleBone users to easily switch > > between pin functions on the expansion headers during runtime to make > > rapid prototyping with a breadboard easier (such as changing header pin > > from GPIO to SPI mode). Most of the header pins have 7 different modes. > > > > Ultimately, the solution I settled on with feedback from this list was > > to create pinmux-select debugfs file that can activate desired fucntion: > > 6199f6becc86 ("pinctrl: pinmux: Add pinmux-select debugfs file") > > > > Bits 6:3 are related to what this subsystem would refer to as pin conf > > such as slew, input enable and bias. Thus it might make sense to expose > > something like a select-pinconf file to activate pin conf settings from > > userspace. This would require using 'pinconf-single' compatible. > > > > I fixed pinctrl-single bug regarding pinconf last year so it should be > > possible to use 'pinconf-single' compatible for the am33xx_pinmux node: > > f46fe79ff1b6 ("pinctrl-single: fix pcs_parse_pinconf() return value") > > > > In the kernel version 4.1.6 that I am using on my custom board, I have fixed > the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However, > this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/). > Do you think it is better to bring that functionality back to life or the submitted > patch could be fine too? Wow, I had no idea there used to be a pinconf-config debugfs file. I would have been interested in using it if it had still existed. Regarding your patch, I think it could be helpful to be able to set the conf_<module>_<pin> registers directly through debugfs as I can imagine situations where it would be useful for testing. It is a bit dangerous as the person using it has to be careful not to change the wrong bits, but they would need to have debugfs mounted and permissions to write to it. I suppose it depends on what others maintainers like Linus and Tony think about whether that is an acceptable solution. thanks, drew
On Tue, May 18, 2021 at 1:22 PM Drew Fustini <drew@beagleboard.org> wrote: > On Tue, May 18, 2021 at 2:38 AM Dario Binacchi <dariobin@libero.it> wrote: > > > Il 18/05/2021 00:57 Drew Fustini <drew@beagleboard.org> ha scritto: > > > On Mon, May 17, 2021 at 11:02:00PM +0300, Andy Shevchenko wrote: > > > > On Sun, May 16, 2021 at 7:43 PM Dario Binacchi <dariobin@libero.it> wrote: ... > > > > Drew, is it similar to what you are trying to achieve? > > > > > > Yes, I would say this similar to what I was trying to accomplish: being > > > able to change contents of conf_<module>_<pin> register [table 9-60] > > > from userspace. > > > > > > However, I was specifically looking to change bits 2:0 which is mux > > > mode. My motivation was to allow BeagleBone users to easily switch > > > between pin functions on the expansion headers during runtime to make > > > rapid prototyping with a breadboard easier (such as changing header pin > > > from GPIO to SPI mode). Most of the header pins have 7 different modes. > > > > > > Ultimately, the solution I settled on with feedback from this list was > > > to create pinmux-select debugfs file that can activate desired fucntion: > > > 6199f6becc86 ("pinctrl: pinmux: Add pinmux-select debugfs file") > > > > > > Bits 6:3 are related to what this subsystem would refer to as pin conf > > > such as slew, input enable and bias. Thus it might make sense to expose > > > something like a select-pinconf file to activate pin conf settings from > > > userspace. This would require using 'pinconf-single' compatible. > > > > > > I fixed pinctrl-single bug regarding pinconf last year so it should be > > > possible to use 'pinconf-single' compatible for the am33xx_pinmux node: > > > f46fe79ff1b6 ("pinctrl-single: fix pcs_parse_pinconf() return value") > > > > > > > In the kernel version 4.1.6 that I am using on my custom board, I have fixed > > the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However, > > this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/). > > Do you think it is better to bring that functionality back to life or the submitted > > patch could be fine too? > > Wow, I had no idea there used to be a pinconf-config debugfs file. I > would have been interested in using it if it had still existed. In Git you may always resurrect the removed feature. > Regarding your patch, I think it could be helpful to be able to set > the conf_<module>_<pin> registers directly through debugfs as I can > imagine situations where it would be useful for testing. It is a bit > dangerous as the person using it has to be careful not to change the > wrong bits, but they would need to have debugfs mounted and > permissions to write to it. I suppose it depends on what others > maintainers like Linus and Tony think about whether that is an > acceptable solution. -- With Best Regards, Andy Shevchenko
Hi, > Il 17/05/2021 22:02 Andy Shevchenko <andy.shevchenko@gmail.com> ha scritto: > > > On Sun, May 16, 2021 at 7:43 PM Dario Binacchi <dariobin@libero.it> wrote: > > > > The MPUs of some architectures (e.g AM335x) must be in privileged > > operating mode to write on the pinmux > > pinmux is not pin configuration. You need to rethink the approach. > > > registers. In such cases, where > > writes will not work from user space, now it can be done from the pins > > debug file if the platform driver exports the pin_dbg_set() helper among > > the registered operations. > > Drew, is it similar to what you are trying to achieve? > > ... > > > +static ssize_t pinctrl_pins_write(struct file *file, > > + const char __user *user_buf, size_t count, > > + loff_t *ppos) > > +{ > > + struct seq_file *s = file->private_data; > > + struct pinctrl_dev *pctldev = s->private; > > + const struct pinctrl_ops *ops = pctldev->desc->pctlops; > > + char buf[32]; > > + char *c = &buf[0]; > > + char *token; > > + int ret, buf_size; > > + unsigned int i, pin; > > + > > + if (!ops->pin_dbg_set) > > + return -EFAULT; > > + > > + /* Get userspace string and assure termination */ > > + buf_size = min(count, sizeof(buf) - 1); > > + if (copy_from_user(buf, user_buf, buf_size)) > > + return -EFAULT; > > + > > + buf[buf_size] = 0; > > Can't you use strncpy_from_user() ? Ok, I'll use strncpy_from_user() in the next version of the patch > > > > + token = strsep(&c, " "); > > > + if (kstrtouint(token, 0, &pin)) > > + return -EINVAL; > > Don't shadow an error code. You are right > > > + for (i = 0; i < pctldev->desc->npins; i++) { > > + if (pin != pctldev->desc->pins[i].number) > > + continue; > > Hmm... I don't get this. Why is it needed? I want to make sure the pin is managed Thanks and regards, Dario > > > + ret = ops->pin_dbg_set(pctldev, pin, c); > > + if (ret) > > + return ret; > > + > > + return count; > > + } > > + > > + return -EINVAL; > > +} > > ... > > > - debugfs_create_file("pins", 0444, > > + debugfs_create_file("pins", 0644, > > device_root, pctldev, &pinctrl_pins_fops); > > Why is it in this file? > > > > -- > With Best Regards, > Andy Shevchenko
On Tue, May 18, 2021 at 4:57 PM Dario Binacchi <dariobin@libero.it> wrote: > > Il 17/05/2021 22:02 Andy Shevchenko <andy.shevchenko@gmail.com> ha scritto: > > On Sun, May 16, 2021 at 7:43 PM Dario Binacchi <dariobin@libero.it> wrote: ... > > Can't you use strncpy_from_user() ? > > Ok, I'll use strncpy_from_user() in the next version of the patch Don't be in a hurry. We need to settle down the interface first. We still haven't heard from the maintainer (Linus?) about it. Neither we have a clear view if we need to revert that patch that dropped pinconf-config (Drew, what do you think?). -- With Best Regards, Andy Shevchenko
On Tue, May 18, 2021 at 05:01:30PM +0300, Andy Shevchenko wrote: > On Tue, May 18, 2021 at 4:57 PM Dario Binacchi <dariobin@libero.it> wrote: > > > Il 17/05/2021 22:02 Andy Shevchenko <andy.shevchenko@gmail.com> ha scritto: > > > On Sun, May 16, 2021 at 7:43 PM Dario Binacchi <dariobin@libero.it> wrote: > > ... > > > > Can't you use strncpy_from_user() ? > > > > Ok, I'll use strncpy_from_user() in the next version of the patch > > Don't be in a hurry. > > We need to settle down the interface first. We still haven't heard > from the maintainer (Linus?) about it. Neither we have a clear view if > we need to revert that patch that dropped pinconf-config (Drew, what > do you think?). Vladimir Zapolskiy wrote in e73339037f6b ("pinctrl: remove unused 'pinconf-config' debugfs interface"): Of course it might be possible to increase MAX_NAME_LEN, and then add .pin_config_dbg_parse_modify callbacks to the drivers, but the whole idea of such a limited debug option looks inviable. A more flexible way to functionally substitute the original approach is to implicitly or explicitly use pinctrl_select_state() function whenever needed. This makes me think it is not a good idea to bring back pinconf-config. The pinmux-select debugfs file that I add added in commit 6199f6becc86 ("pinctrl: pinmux: Add pinmux-select debugfs file") provides a method to activate a pin function and pin group which I think provides the same capability as long as the possible pin functions are described in dts. thanks, drew
On Wed, May 19, 2021 at 1:02 PM Drew Fustini <drew@beagleboard.org> wrote: > On Tue, May 18, 2021 at 05:01:30PM +0300, Andy Shevchenko wrote: ... > Vladimir Zapolskiy wrote in e73339037f6b ("pinctrl: remove unused > 'pinconf-config' debugfs interface"): > > Of course it might be possible to increase MAX_NAME_LEN, and then add > .pin_config_dbg_parse_modify callbacks to the drivers, but the whole > idea of such a limited debug option looks inviable. A more flexible > way to functionally substitute the original approach is to implicitly > or explicitly use pinctrl_select_state() function whenever needed. > > This makes me think it is not a good idea to bring back pinconf-config. > The pinmux-select debugfs file that I add added in commit 6199f6becc86 > ("pinctrl: pinmux: Add pinmux-select debugfs file") provides a method to > activate a pin function and pin group which I think provides the same > capability as long as the possible pin functions are described in dts. The problem is that the pinctrl_select_state() is very limited and has no clear meanings of the states. Only few are defined and still unclear. What does `sleep` or `standby` or whatever mean? It may be quite different to the device in question. Basically what we need is to say we want this device ('function') to appear on this group of pins ('group'). And pinctrl_select_state() can't fulfill this simple task :-( If we look at the ACPI case it makes that API completely out of useful context (it can be used due to above and some kind of layering violations, like PM vs. pin control). Since above is the debugfs interface we may return it for the certain task, i.e. printing current function / group choice(s) (if it's not done by other means) and allow to switch it desired function/group (that's what Dario tries to achieve AFAIU). -- With Best Regards, Andy Shevchenko
On Wed, May 19, 2021 at 02:27:38PM +0300, Andy Shevchenko wrote: > On Wed, May 19, 2021 at 1:02 PM Drew Fustini <drew@beagleboard.org> wrote: > > On Tue, May 18, 2021 at 05:01:30PM +0300, Andy Shevchenko wrote: > > ... > > > Vladimir Zapolskiy wrote in e73339037f6b ("pinctrl: remove unused > > 'pinconf-config' debugfs interface"): > > > > Of course it might be possible to increase MAX_NAME_LEN, and then add > > .pin_config_dbg_parse_modify callbacks to the drivers, but the whole > > idea of such a limited debug option looks inviable. A more flexible > > way to functionally substitute the original approach is to implicitly > > or explicitly use pinctrl_select_state() function whenever needed. > > > > This makes me think it is not a good idea to bring back pinconf-config. > > The pinmux-select debugfs file that I add added in commit 6199f6becc86 > > ("pinctrl: pinmux: Add pinmux-select debugfs file") provides a method to > > activate a pin function and pin group which I think provides the same > > capability as long as the possible pin functions are described in dts. > > The problem is that the pinctrl_select_state() is very limited and has > no clear meanings of the states. Only few are defined and still > unclear. What does `sleep` or `standby` or whatever mean? It may be > quite different to the device in question. Basically what we need is > to say we want this device ('function') to appear on this group of > pins ('group'). And pinctrl_select_state() can't fulfill this simple > task :-( > > If we look at the ACPI case it makes that API completely out of useful > context (it can be used due to above and some kind of layering > violations, like PM vs. pin control). > > Since above is the debugfs interface we may return it for the certain > task, i.e. printing current function / group choice(s) (if it's not > done by other means) and allow to switch it desired function/group > (that's what Dario tries to achieve AFAIU). A write to the pinmux-select debugfs file will call pinmux_select() in drivers/pinctrl/pinmux.c which, after some validation checks, will call pmxops->set_mux() with function selector and group selector as arguments. For pinctrl-single, this will invoke pcs_set_mux() which will ultimately set the mux mode bits in the register for each pin in that function. IS that useful for pin controllers in ACPI systems as well? thanks, drew
On Thu, May 20, 2021 at 7:17 AM Drew Fustini <drew@beagleboard.org> wrote: > On Wed, May 19, 2021 at 02:27:38PM +0300, Andy Shevchenko wrote: > > On Wed, May 19, 2021 at 1:02 PM Drew Fustini <drew@beagleboard.org> wrote: > > > On Tue, May 18, 2021 at 05:01:30PM +0300, Andy Shevchenko wrote: ... > > > Vladimir Zapolskiy wrote in e73339037f6b ("pinctrl: remove unused > > > 'pinconf-config' debugfs interface"): > > > > > > Of course it might be possible to increase MAX_NAME_LEN, and then add > > > .pin_config_dbg_parse_modify callbacks to the drivers, but the whole > > > idea of such a limited debug option looks inviable. A more flexible > > > way to functionally substitute the original approach is to implicitly > > > or explicitly use pinctrl_select_state() function whenever needed. > > > > > > This makes me think it is not a good idea to bring back pinconf-config. > > > The pinmux-select debugfs file that I add added in commit 6199f6becc86 > > > ("pinctrl: pinmux: Add pinmux-select debugfs file") provides a method to > > > activate a pin function and pin group which I think provides the same > > > capability as long as the possible pin functions are described in dts. > > > > The problem is that the pinctrl_select_state() is very limited and has > > no clear meanings of the states. Only few are defined and still > > unclear. What does `sleep` or `standby` or whatever mean? It may be > > quite different to the device in question. Basically what we need is > > to say we want this device ('function') to appear on this group of > > pins ('group'). And pinctrl_select_state() can't fulfill this simple > > task :-( > > > > If we look at the ACPI case it makes that API completely out of useful > > context (it can be used due to above and some kind of layering > > violations, like PM vs. pin control). > > > > Since above is the debugfs interface we may return it for the certain > > task, i.e. printing current function / group choice(s) (if it's not > > done by other means) and allow to switch it desired function/group > > (that's what Dario tries to achieve AFAIU). > > A write to the pinmux-select debugfs file will call pinmux_select() in > drivers/pinctrl/pinmux.c which, after some validation checks, will call > pmxops->set_mux() with function selector and group selector as > arguments. For pinctrl-single, this will invoke pcs_set_mux() which > will ultimately set the mux mode bits in the register for each pin in > that function. > > IS that useful for pin controllers in ACPI systems as well? Yes, the debugfs interface is useful independently of the resource provider. What I was talking about is the boot / driver load time pin muxing and configuration as well as PM transitions. -- With Best Regards, Andy Shevchenko