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

[PATCH v2 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


Changes in v2:
- Remove CONFIG_SOC_AM33XX dependency.

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 v2 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>
---

(no changes since v1)

 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

[PATCH v2 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>

---

Changes in v2:
- Remove CONFIG_SOC_AM33XX dependency.

 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..8497414e3384 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)
+	.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 v2 2/2] pinctrl: single: set pinmux from pins debug file

[Tony Lindgren]

Hi,

I noticed few more things I started to wonder about after
looking at this again.

* Dario Binacchi <dariobin@libero.it> [210517 20:00]:
> +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;
> +}

Since you're adding a new interface, how about pass unsigned
int val instead of char *buf?

>  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)
> +	.pin_dbg_set = pcs_pin_dbg_set,
> +#endif
>  	.dt_node_to_map = pcs_dt_node_to_map,
>  	.dt_free_map = pcs_dt_free_map,
>  };

It might be better to always have the .pin_dbg_set around to
avoid the IS_ENABLED(CONFIG_DEVMEM).

Does the new interface need something under Documentation too?

Regards,

Tony

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

[Dario Binacchi]

Hi Tony,

> Il 18/05/2021 08:05 Tony Lindgren <tony@atomide.com> ha scritto:
> 
>  
> Hi,
> 
> I noticed few more things I started to wonder about after
> looking at this again.
> 
> * Dario Binacchi <dariobin@libero.it> [210517 20:00]:
> > +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;
> > +}
> 
> Since you're adding a new interface, how about pass unsigned
> int val instead of char *buf?

I thought about passing char *buf because it seemed more generic 
to me. As the output of pin_dbg_show() depends on the platform 
driver, perhaps pin_dbg_set() may need driver-dependent data.
Is it possible that only the value to be set in the register 
(unsigned int) is required?

> 
> >  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)
> > +	.pin_dbg_set = pcs_pin_dbg_set,
> > +#endif
> >  	.dt_node_to_map = pcs_dt_node_to_map,
> >  	.dt_free_map = pcs_dt_free_map,
> >  };
> 
> It might be better to always have the .pin_dbg_set around to
> avoid the IS_ENABLED(CONFIG_DEVMEM).

Ok, I'll remove the CONFIG_DEVMEM dependency

> 
> Does the new interface need something under Documentation too?

Yes, the description of `pins` in Documentation/driver-api/pin-control.rst 
needs to be updated. I'll add another patch to the series.

Thanks and regards,
Dario

> 
> Regards,
> 
> Tony

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

[Andy Shevchenko]

On Wed, May 19, 2021 at 3:58 AM 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 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.

NAK. Please get into discussion and encourage maintainers to participate.

(Esp. taking into account that v2 of this patch is the same as v1)

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, May 19, 2021 at 3:58 AM Dario Binacchi <dariobin@libero.it> wrote:
>
>
> 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.

Even for debugfs it would be nice to have a piece of documentation.
Because pin control is a quite sensitive area and if something goes
wrong, it may damage the hardware.

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


-- 
With Best Regards,
Andy Shevchenko

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

[Tony Lindgren]

* Dario Binacchi <dariobin@libero.it> [210518 08:57]:
> I thought about passing char *buf because it seemed more generic 
> to me. As the output of pin_dbg_show() depends on the platform 
> driver, perhaps pin_dbg_set() may need driver-dependent data.
> Is it possible that only the value to be set in the register 
> (unsigned int) is required?

Maybe Linus W can answer this one.

Regards,

Tony