[PATCH 0/2] am335x: set pinmux registers from pins debug file

[PATCH 0/2] am335x: set pinmux registers from pins debug file

[Dario Binacchi]

The patch was born from the need to change the slew rate of the LCD pins
of a custom AM335x board during EMC tests. The AM335x, as described in a
note in section 9.1 of its reference manual [1], is unable to write
pinmux registers from user space. The series now makes it possible to
write these registers from the pins debug file.

[1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf



Dario Binacchi (2):
  pinctrl: core: configure pinmux from pins debug file
  pinctrl: single: set pinmux from pins debug file

 drivers/pinctrl/core.c           | 56 ++++++++++++++++++++++++++++++--
 drivers/pinctrl/pinctrl-single.c | 20 ++++++++++++
 include/linux/pinctrl/pinctrl.h  |  2 ++
 3 files changed, 76 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[Dario Binacchi]

The MPUs of some architectures (e.g AM335x) must be in privileged
operating mode to write on the pinmux 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.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

 drivers/pinctrl/core.c          | 56 +++++++++++++++++++++++++++++++--
 include/linux/pinctrl/pinctrl.h |  2 ++
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index a4ac87c8b4f8..f5c9a7d44039 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1620,6 +1620,46 @@ EXPORT_SYMBOL_GPL(pinctrl_pm_select_idle_state);
 
 #ifdef CONFIG_DEBUG_FS
 
+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;
+	token = strsep(&c, " ");
+	if (kstrtouint(token, 0, &pin))
+		return -EINVAL;
+
+	for (i = 0; i < pctldev->desc->npins; i++) {
+		if (pin != pctldev->desc->pins[i].number)
+			continue;
+
+		ret = ops->pin_dbg_set(pctldev, pin, c);
+		if (ret)
+			return ret;
+
+		return count;
+	}
+
+	return -EINVAL;
+}
+
 static int pinctrl_pins_show(struct seq_file *s, void *what)
 {
 	struct pinctrl_dev *pctldev = s->private;
@@ -1677,7 +1717,11 @@ static int pinctrl_pins_show(struct seq_file *s, void *what)
 
 	return 0;
 }
-DEFINE_SHOW_ATTRIBUTE(pinctrl_pins);
+
+static int pinctrl_pins_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pinctrl_pins_show, inode->i_private);
+}
 
 static int pinctrl_groups_show(struct seq_file *s, void *what)
 {
@@ -1886,6 +1930,14 @@ static int pinctrl_show(struct seq_file *s, void *what)
 }
 DEFINE_SHOW_ATTRIBUTE(pinctrl);
 
+static const struct file_operations pinctrl_pins_fops = {
+	.open		= pinctrl_pins_open,
+	.read		= seq_read,
+	.write		= pinctrl_pins_write,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 static struct dentry *debugfs_root;
 
 static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
@@ -1915,7 +1967,7 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
 			dev_name(pctldev->dev));
 		return;
 	}
-	debugfs_create_file("pins", 0444,
+	debugfs_create_file("pins", 0644,
 			    device_root, pctldev, &pinctrl_pins_fops);
 	debugfs_create_file("pingroups", 0444,
 			    device_root, pctldev, &pinctrl_groups_fops);
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 70b45d28e7a9..6db4a775f549 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -95,6 +95,8 @@ struct pinctrl_ops {
 			       unsigned *num_pins);
 	void (*pin_dbg_show) (struct pinctrl_dev *pctldev, struct seq_file *s,
 			  unsigned offset);
+	int (*pin_dbg_set) (struct pinctrl_dev *pctldev, unsigned int offset,
+			    char *buf);
 	int (*dt_node_to_map) (struct pinctrl_dev *pctldev,
 			       struct device_node *np_config,
 			       struct pinctrl_map **map, unsigned *num_maps);
-- 
2.17.1

Re: [PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[Andy Shevchenko]

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

Re: [PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[Drew Fustini]

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

Re: [PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[Dario Binacchi]

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

Re: [PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[Drew Fustini]

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

Re: [PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[Andy Shevchenko]

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

Re: [PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[Dario Binacchi]

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

Re: [PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[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

Re: [PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[Drew Fustini]

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

Re: [PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[Andy Shevchenko]

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

Re: [PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[Drew Fustini]

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

Re: [PATCH 1/2] pinctrl: core: configure pinmux from pins debug file

[Andy Shevchenko]

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

[PATCH 2/2] pinctrl: single: set pinmux from pins debug file

[Dario Binacchi]

As described in section 9.1 of the TI reference manual for AM335x [1],
"For writing to the control module registers, the MPU will need to be in
privileged mode of operation and writes will not work from user mode".
By adding the pin_dbg_set helper to pcs_pinctrl_ops it will be possible
to write these registers from the pins debug:

cd /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/
echo <pin-number> <reg-value> >pins

[1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

 drivers/pinctrl/pinctrl-single.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 2c9c9835f375..cdbc2298360d 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -313,6 +313,23 @@ static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
 	seq_printf(s, "%zx %08x %s ", pa, val, DRIVER_NAME);
 }
 
+static int pcs_pin_dbg_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			   char *buf)
+{
+	struct pcs_device *pcs;
+	unsigned int val, mux_bytes;
+
+	buf = skip_spaces(buf);
+	if (kstrtouint(buf, 0, &val))
+		return -EINVAL;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+
+	mux_bytes = pcs->width / BITS_PER_BYTE;
+	pcs->write(val, pcs->base + pin * mux_bytes);
+	return 0;
+}
+
 static void pcs_dt_free_map(struct pinctrl_dev *pctldev,
 				struct pinctrl_map *map, unsigned num_maps)
 {
@@ -331,6 +348,9 @@ static const struct pinctrl_ops pcs_pinctrl_ops = {
 	.get_group_name = pinctrl_generic_get_group_name,
 	.get_group_pins = pinctrl_generic_get_group_pins,
 	.pin_dbg_show = pcs_pin_dbg_show,
+#if IS_ENABLED(CONFIG_DEVMEM) && IS_ENABLED(CONFIG_SOC_AM33XX)
+	.pin_dbg_set = pcs_pin_dbg_set,
+#endif
 	.dt_node_to_map = pcs_dt_node_to_map,
 	.dt_free_map = pcs_dt_free_map,
 };
-- 
2.17.1

Re: [PATCH 2/2] pinctrl: single: set pinmux from pins debug file

[Tony Lindgren]

Hi,

* Dario Binacchi <dariobin@libero.it> [210516 13:55]:
> @@ -331,6 +348,9 @@ static const struct pinctrl_ops pcs_pinctrl_ops = {
>  	.get_group_name = pinctrl_generic_get_group_name,
>  	.get_group_pins = pinctrl_generic_get_group_pins,
>  	.pin_dbg_show = pcs_pin_dbg_show,
> +#if IS_ENABLED(CONFIG_DEVMEM) && IS_ENABLED(CONFIG_SOC_AM33XX)
> +	.pin_dbg_set = pcs_pin_dbg_set,
> +#endif
>  	.dt_node_to_map = pcs_dt_node_to_map,
>  	.dt_free_map = pcs_dt_free_map,
>  };

I don't think there should be any CONFIG_SOC_AM33XX dependency here?

Regards,

Tony