[U-Boot] [PATCH v1 0/5] Add pinmux command

[U-Boot] [PATCH v1 0/5] Add pinmux command

[Patrice Chotard]

For debug purpose, it's useful to know the pins muxing
to check is a pin is configured as a GPIO or as an alternate
function. For this purpose a new command pinmux is implemented.

This series adds:
  _ Add new pinmux_show ops to UCLASS pinctrl
  - Add new pinmux command
  - Add get_function() support to stm32 gpio driver
  - Add pinmux_show() support to stm32 pinctrl driver
  - Enable CMD_PINMUX flag to stm32mp15 defconfig


Patrice Chotard (5):
  dm: pinctrl: Add pinmux_show() ops
  cmd: pinmux: Add pinmux command
  configs: stm32mp15: Enable CMD_PINMUX
  pinctrl: stm32: Add pinmux_show() ops
  gpio: stm32f7: Add ops get_function

 cmd/Kconfig                       |   8 +++
 cmd/Makefile                      |   3 +
 cmd/pinmux.c                      | 115 ++++++++++++++++++++++++++++++++++++++
 configs/stm32mp15_basic_defconfig |   1 +
 drivers/gpio/stm32f7_gpio.c       |  20 +++++++
 drivers/pinctrl/pinctrl-uclass.c  |  10 ++++
 drivers/pinctrl/pinctrl_stm32.c   |  79 ++++++++++++++++++++++++++
 include/dm/pinctrl.h              |  29 ++++++++++
 8 files changed, 265 insertions(+)
 create mode 100644 cmd/pinmux.c

-- 
1.9.1

[U-Boot] [PATCH v1 1/5] dm: pinctrl: Add pinmux_show() ops

[Patrice Chotard]

pinmux_show() allows to display the muxing of all pins
belonging to a pin-controller

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

 drivers/pinctrl/pinctrl-uclass.c | 10 ++++++++++
 include/dm/pinctrl.h             | 29 +++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
index c38bb212ed74..24de7cc09d63 100644
--- a/drivers/pinctrl/pinctrl-uclass.c
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -249,6 +249,16 @@ int pinctrl_get_gpio_mux(struct udevice *dev, int banknum, int index)
 	return ops->get_gpio_mux(dev, banknum, index);
 }
 
+int pinctrl_pinmux_show(struct udevice *dev)
+{
+	struct pinctrl_ops *ops = pinctrl_get_ops(dev);
+
+	if (!ops->pinmux_show)
+		return -ENOSYS;
+
+	return ops->pinmux_show(dev);
+}
+
 /**
  * pinconfig_post_bind() - post binding for PINCTRL uclass
  * Recursively bind child nodes as pinconfig devices in case of full pinctrl.
diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
index 80de3f3fed2b..cbae7e1a640b 100644
--- a/include/dm/pinctrl.h
+++ b/include/dm/pinctrl.h
@@ -66,6 +66,7 @@ struct pinconf_param {
  *	pointing a config node. (necessary for pinctrl_full)
  * @set_state_simple: do needed pinctrl operations for a peripherl @periph.
  *	(necessary for pinctrl_simple)
+ * @pinmux_show: display pin-controller's pin muxing.
  */
 struct pinctrl_ops {
 	int (*get_pins_count)(struct udevice *dev);
@@ -129,6 +130,20 @@ struct pinctrl_ops {
 	* @return mux value (SoC-specific, e.g. 0 for input, 1 for output)
 	 */
 	int (*get_gpio_mux)(struct udevice *dev, int banknum, int index);
+
+	/**
+	 * pinmux_show() - show pin-controller's pins muxing
+	 *
+	 * This allows to display all pin muxing configuration of a
+	 * pin-controller. It's useful for debug purpose to know if a pin is
+	 * configured as GPIO or as an alternate function and which one.
+	 * Typically it is used by a PINCTRL driver with knowledge of the SoC
+	 * pinctrl setup.
+	 *
+	 * @dev:	Pinctrl device to use
+	 * @return 0 if OK, -ve on error
+	 */
+	int (*pinmux_show)(struct udevice *dev);
 };
 
 #define pinctrl_get_ops(dev)	((struct pinctrl_ops *)(dev)->driver->ops)
@@ -348,4 +363,18 @@ int pinctrl_decode_pin_config(const void *blob, int node);
 */
 int pinctrl_get_gpio_mux(struct udevice *dev, int banknum, int index);
 
+/**
+ * pinctrl_pinmux_show() - display pin-controller pins muxing
+ *
+ * This allows the raw mux value for a GPIO to be obtained. It is
+ * useful for displaying the function being used by that GPIO, such
+ * as with the 'gpio' command. This function is internal to the GPIO
+ * subsystem and should not be used by generic code. Typically it is
+ * used by a GPIO driver with knowledge of the SoC pinctrl setup.
+ *
+ * @dev:	Pinctrl device to use
+ * @return 0 if OK, -ve on error
+ */
+int pinctrl_pinmux_show(struct udevice *dev);
+
 #endif /* __PINCTRL_H */
-- 
1.9.1

[U-Boot] [PATCH v1 2/5] cmd: pinmux: Add pinmux command

[Patrice Chotard]

pinmux command allows to :
 - list all pin-controllers available on platforms
 - select a pin-controller
 - display the muxing of all pins of the current pin-controller
   or all pin-controllers depending of given options

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

 cmd/Kconfig  |   8 +++++
 cmd/Makefile |   3 ++
 cmd/pinmux.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+)
 create mode 100644 cmd/pinmux.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 13d4c991bf8b..2c687ceecf49 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -937,6 +937,14 @@ config CMD_PCMCIA
 	  about 1990. These devices are typically removable memory or network
 	  cards using a standard 68-pin connector.
 
+config CMD_PINMUX
+	bool "pinmux - show pins muxing"
+	depends on PINCTRL
+	help
+	  Parse all available pin-controllers and show pins muxing. This
+	  is useful for debug purpoer to check the pin muxing and to know if
+	  a pin is configured as a GPIO or as an alternate function.
+
 config CMD_READ
 	bool "read - Read binary data from a partition"
 	help
diff --git a/cmd/Makefile b/cmd/Makefile
index 3487c80455c4..9e899c44bdef 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -101,6 +101,9 @@ ifdef CONFIG_PCI
 obj-$(CONFIG_CMD_PCI) += pci.o
 endif
 obj-y += pcmcia.o
+ifdef CONFIG_PINCTRL
+obj-$(CONFIG_CMD_PINMUX) += pinmux.o
+endif
 obj-$(CONFIG_CMD_PXE) += pxe.o
 obj-$(CONFIG_CMD_WOL) += wol.o
 obj-$(CONFIG_CMD_QFW) += qfw.o
diff --git a/cmd/pinmux.c b/cmd/pinmux.c
new file mode 100644
index 000000000000..1442d6ef63d2
--- /dev/null
+++ b/cmd/pinmux.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018, STMicroelectronics - All Rights Reserved
+ */
+
+#include <command.h>
+#include <dm.h>
+#include <errno.h>
+#include <dm/pinctrl.h>
+#include <dm/uclass-internal.h>
+
+#ifdef CONFIG_PINCTRL
+
+#define LIMIT_DEVNAME	30
+#define LIMIT_OFNAME	32
+
+static struct udevice *currdev;
+
+static int do_dev(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	const char *name;
+	int ret;
+
+	switch (argc) {
+	case 2:
+		name = argv[1];
+		ret = uclass_get_device_by_name(UCLASS_PINCTRL, name, &currdev);
+		if (ret) {
+			printf("Can't get the pin-controller: %s!\n", name);
+			return CMD_RET_FAILURE;
+		}
+	case 1:
+		if (!currdev) {
+			printf("Pin-controller device is not set!\n\n");
+			return CMD_RET_USAGE;
+		}
+
+		printf("dev: %s\n", currdev->name);
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static int do_status(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	struct udevice *dev;
+	int ret;
+
+	if (currdev && (argc < 2 || strcmp(argv[1], "-a")))
+		return pinctrl_pinmux_show(currdev);
+
+	for (uclass_first_device(UCLASS_PINCTRL, &dev); dev;
+	     uclass_next_device(&dev)) {
+		/* insert a separator between each pin-controller display */
+		printf("--------------------------\n");
+		printf("%s:\n", dev->name);
+		ret = pinctrl_pinmux_show(dev);
+		if (ret)
+			return ret;
+	}
+	return CMD_RET_SUCCESS;
+}
+
+static int do_list(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	struct udevice *dev;
+	int ret;
+
+	printf("| %-*.*s| %-*.*s| %s\n",
+	       LIMIT_DEVNAME, LIMIT_DEVNAME, "Device",
+	       LIMIT_DEVNAME, LIMIT_DEVNAME, "Driver",
+	       "Parent");
+
+	for (ret = uclass_find_first_device(UCLASS_PINCTRL, &dev); dev;
+	     ret = uclass_find_next_device(&dev)) {
+		if (ret)
+			continue;
+
+		printf("| %-*.*s| %-*.*s| %s\n",
+		       LIMIT_DEVNAME, LIMIT_DEVNAME, dev->name,
+		       LIMIT_DEVNAME, LIMIT_DEVNAME, dev->driver->name,
+		       dev->parent->name);
+	}
+
+	return ret;
+}
+
+static cmd_tbl_t pinmux_subcmd[] = {
+	U_BOOT_CMD_MKENT(dev, 2, 1, do_dev, "", ""),
+	U_BOOT_CMD_MKENT(list, 1, 1, do_list, "", ""),
+	U_BOOT_CMD_MKENT(status, 2, 1, do_status, "", ""),
+};
+
+static int do_pinmux(cmd_tbl_t *cmdtp, int flag, int argc,
+		     char * const argv[])
+{
+	cmd_tbl_t *cmd;
+
+	argc--;
+	argv++;
+
+	cmd = find_cmd_tbl(argv[0], pinmux_subcmd, ARRAY_SIZE(pinmux_subcmd));
+	if (!cmd || argc > cmd->maxargs)
+		return CMD_RET_USAGE;
+
+	return cmd->cmd(cmdtp, flag, argc, argv);
+}
+
+U_BOOT_CMD(pinmux, CONFIG_SYS_MAXARGS, 1, do_pinmux,
+	   "show pin-controller muxing",
+	   "list                     - list UCLASS_PINCTRL devices\n"
+	   "pinmux dev [pincontroller-name] - select pin-controller device\n"
+	   "pinmux status [-a]              - print pin-controller muxing [for all]\n"
+)
+#endif
-- 
1.9.1

[U-Boot] [PATCH v1 3/5] configs: stm32mp15: Enable CMD_PINMUX

[Patrice Chotard]

This allows to use the pinmux command to display pins muxing.

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

 configs/stm32mp15_basic_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/stm32mp15_basic_defconfig b/configs/stm32mp15_basic_defconfig
index e134a66fb105..cd8ec387a5f7 100644
--- a/configs/stm32mp15_basic_defconfig
+++ b/configs/stm32mp15_basic_defconfig
@@ -24,6 +24,7 @@ CONFIG_CMD_GPIO=y
 CONFIG_CMD_GPT=y
 CONFIG_CMD_I2C=y
 CONFIG_CMD_MMC=y
+CONFIG_CMD_PINMUX=y
 CONFIG_CMD_PMIC=y
 CONFIG_CMD_REGULATOR=y
 CONFIG_CMD_EXT4_WRITE=y
-- 
1.9.1

[U-Boot] [PATCH v1 4/5] pinctrl: stm32: Add pinmux_show() ops

[Patrice Chotard]

pinmux_show allows to display the muxing of all pins
belonging to pin-controller.

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

 drivers/pinctrl/pinctrl_stm32.c | 79 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/pinctrl/pinctrl_stm32.c b/drivers/pinctrl/pinctrl_stm32.c
index 31285cdd5784..a477035bf420 100644
--- a/drivers/pinctrl/pinctrl_stm32.c
+++ b/drivers/pinctrl/pinctrl_stm32.c
@@ -14,6 +14,16 @@ DECLARE_GLOBAL_DATA_PTR;
 #define OTYPE_MSK			1
 #define AFR_MASK			0xF
 
+#define PINMUX_MODE_COUNT		5
+
+static const char * const pinmux_mode[PINMUX_MODE_COUNT] = {
+	"gpio input",
+	"gpio output",
+	"analog",
+	"unknown",
+	"alt function",
+};
+
 static int stm32_gpio_config(struct gpio_desc *desc,
 			     const struct stm32_gpio_ctl *ctl)
 {
@@ -176,12 +186,81 @@ static int stm32_pinctrl_set_state_simple(struct udevice *dev,
 }
 #endif /* PINCTRL_FULL */
 
+static int stm32_pinctrl_get_af(struct udevice *dev,  unsigned int offset)
+{
+	struct stm32_gpio_priv *priv = dev_get_priv(dev);
+	struct stm32_gpio_regs *regs = priv->regs;
+	u32 af;
+	u32 alt_shift = (offset % 8) * 4;
+	u32 alt_index =  offset / 8;
+
+	af = (readl(&regs->afr[alt_index]) &
+	      GENMASK(alt_shift + 3, alt_shift)) >> alt_shift;
+
+	return af;
+}
+
+static int stm32_pinmux_show(struct udevice *dev)
+{
+	struct udevice *child;
+	struct udevice *dev_gpio;
+	const char *bank_name;
+	const char *label;
+	int offset;
+	int ret;
+	int num_bits;
+	int mode;
+	int af_num;
+
+	/* parse pin-controller sub-nodes, ie gpio bank nodes */
+	list_for_each_entry(child, &dev->child_head, sibling_node) {
+		ret = uclass_get_device_by_name(UCLASS_GPIO, child->name,
+						&dev_gpio);
+
+		if (ret < 0 && ret != -ENODEV) {
+			dev_err(dev, "Failed to find %s device ret = %d\n",
+				child->name, ret);
+			return ret;
+		}
+
+		if (!ret) {
+			bank_name = gpio_get_bank_info(dev_gpio, &num_bits);
+
+			printf("\nBank %s:\n", bank_name);
+			for (offset = 0; offset < num_bits; offset++) {
+				mode = gpio_get_raw_function(dev_gpio,
+							     offset, &label);
+				printf("%s%d: %s", bank_name, offset,
+				       pinmux_mode[mode]);
+				switch (mode) {
+				case GPIOF_FUNC:
+					af_num = stm32_pinctrl_get_af(dev_gpio,
+								      offset);
+					printf(" %d", af_num);
+					break;
+				case STM32_GPIO_MODE_OUT:
+				case STM32_GPIO_MODE_IN:
+					printf(" %s", label ? label : "");
+					break;
+				}
+				printf("\n");
+			}
+		}
+
+		if (!child)
+			break;
+	}
+
+	return 0;
+}
+
 static struct pinctrl_ops stm32_pinctrl_ops = {
 #if CONFIG_IS_ENABLED(PINCTRL_FULL)
 	.set_state		= stm32_pinctrl_set_state,
 #else /* PINCTRL_FULL */
 	.set_state_simple	= stm32_pinctrl_set_state_simple,
 #endif /* PINCTRL_FULL */
+	.pinmux_show		= stm32_pinmux_show,
 };
 
 static const struct udevice_id stm32_pinctrl_ids[] = {
-- 
1.9.1

[U-Boot] [PATCH v1 5/5] gpio: stm32f7: Add ops get_function

[Patrice Chotard]

This patch adds gpio get_function ops support.
This function reports the state of a gpio.

Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

 drivers/gpio/stm32f7_gpio.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpio/stm32f7_gpio.c b/drivers/gpio/stm32f7_gpio.c
index 4c0786fff88d..82c8b8d23ae6 100644
--- a/drivers/gpio/stm32f7_gpio.c
+++ b/drivers/gpio/stm32f7_gpio.c
@@ -65,11 +65,31 @@ static int stm32_gpio_set_value(struct udevice *dev, unsigned offset, int value)
 	return 0;
 }
 
+static int stm32_gpio_get_function(struct udevice *dev, unsigned int offset)
+{
+	struct stm32_gpio_priv *priv = dev_get_priv(dev);
+	struct stm32_gpio_regs *regs = priv->regs;
+	int bits_index = MODE_BITS(offset);
+	int mask = MODE_BITS_MASK << bits_index;
+	u32 mode;
+
+	mode = (readl(&regs->moder) & mask) >> bits_index;
+	if (mode == STM32_GPIO_MODE_OUT)
+		return GPIOF_OUTPUT;
+	if (mode == STM32_GPIO_MODE_IN)
+		return GPIOF_INPUT;
+	if (mode == STM32_GPIO_MODE_AN)
+		return GPIOF_UNUSED;
+
+	return GPIOF_FUNC;
+}
+
 static const struct dm_gpio_ops gpio_stm32_ops = {
 	.direction_input	= stm32_gpio_direction_input,
 	.direction_output	= stm32_gpio_direction_output,
 	.get_value		= stm32_gpio_get_value,
 	.set_value		= stm32_gpio_set_value,
+	.get_function		= stm32_gpio_get_function,
 };
 
 static int gpio_stm32_probe(struct udevice *dev)
-- 
1.9.1

[U-Boot] [PATCH v1 2/5] cmd: pinmux: Add pinmux command

[Michal Simek]

Hi,

On 20.9.2018 15:37, Patrice Chotard wrote:
> pinmux command allows to :
>  - list all pin-controllers available on platforms
>  - select a pin-controller
>  - display the muxing of all pins of the current pin-controller
>    or all pin-controllers depending of given options
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> 
>  cmd/Kconfig  |   8 +++++
>  cmd/Makefile |   3 ++
>  cmd/pinmux.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 126 insertions(+)
>  create mode 100644 cmd/pinmux.c
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 13d4c991bf8b..2c687ceecf49 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -937,6 +937,14 @@ config CMD_PCMCIA
>  	  about 1990. These devices are typically removable memory or network
>  	  cards using a standard 68-pin connector.
>  
> +config CMD_PINMUX
> +	bool "pinmux - show pins muxing"
> +	depends on PINCTRL
> +	help
> +	  Parse all available pin-controllers and show pins muxing. This
> +	  is useful for debug purpoer to check the pin muxing and to know if
> +	  a pin is configured as a GPIO or as an alternate function.
> +
>  config CMD_READ
>  	bool "read - Read binary data from a partition"
>  	help
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 3487c80455c4..9e899c44bdef 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -101,6 +101,9 @@ ifdef CONFIG_PCI
>  obj-$(CONFIG_CMD_PCI) += pci.o
>  endif
>  obj-y += pcmcia.o
> +ifdef CONFIG_PINCTRL
> +obj-$(CONFIG_CMD_PINMUX) += pinmux.o
> +endif

you have dependency already setup in Kconfig that's why no reason to
have it here again.

>  obj-$(CONFIG_CMD_PXE) += pxe.o
>  obj-$(CONFIG_CMD_WOL) += wol.o
>  obj-$(CONFIG_CMD_QFW) += qfw.o
> diff --git a/cmd/pinmux.c b/cmd/pinmux.c
> new file mode 100644
> index 000000000000..1442d6ef63d2
> --- /dev/null
> +++ b/cmd/pinmux.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018, STMicroelectronics - All Rights Reserved
> + */
> +
> +#include <command.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <dm/pinctrl.h>
> +#include <dm/uclass-internal.h>
> +
> +#ifdef CONFIG_PINCTRL

and here again.

The rest looks quite standard from quick look.

Thanks,
Michal

[U-Boot] [Uboot-stm32] [PATCH v1 2/5] cmd: pinmux: Add pinmux command

[Patrice CHOTARD]

Hi Michal

On 09/20/2018 03:53 PM, Michal Simek wrote:
> Hi,
> 
> On 20.9.2018 15:37, Patrice Chotard wrote:
>> pinmux command allows to :
>>  - list all pin-controllers available on platforms
>>  - select a pin-controller
>>  - display the muxing of all pins of the current pin-controller
>>    or all pin-controllers depending of given options
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>>  cmd/Kconfig  |   8 +++++
>>  cmd/Makefile |   3 ++
>>  cmd/pinmux.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 126 insertions(+)
>>  create mode 100644 cmd/pinmux.c
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 13d4c991bf8b..2c687ceecf49 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -937,6 +937,14 @@ config CMD_PCMCIA
>>  	  about 1990. These devices are typically removable memory or network
>>  	  cards using a standard 68-pin connector.
>>  
>> +config CMD_PINMUX
>> +	bool "pinmux - show pins muxing"
>> +	depends on PINCTRL
>> +	help
>> +	  Parse all available pin-controllers and show pins muxing. This
>> +	  is useful for debug purpoer to check the pin muxing and to know if
>> +	  a pin is configured as a GPIO or as an alternate function.
>> +
>>  config CMD_READ
>>  	bool "read - Read binary data from a partition"
>>  	help
>> diff --git a/cmd/Makefile b/cmd/Makefile
>> index 3487c80455c4..9e899c44bdef 100644
>> --- a/cmd/Makefile
>> +++ b/cmd/Makefile
>> @@ -101,6 +101,9 @@ ifdef CONFIG_PCI
>>  obj-$(CONFIG_CMD_PCI) += pci.o
>>  endif
>>  obj-y += pcmcia.o
>> +ifdef CONFIG_PINCTRL
>> +obj-$(CONFIG_CMD_PINMUX) += pinmux.o
>> +endif
> 
> you have dependency already setup in Kconfig that's why no reason to
> have it here again.

Yes, i will remove it

> 
>>  obj-$(CONFIG_CMD_PXE) += pxe.o
>>  obj-$(CONFIG_CMD_WOL) += wol.o
>>  obj-$(CONFIG_CMD_QFW) += qfw.o
>> diff --git a/cmd/pinmux.c b/cmd/pinmux.c
>> new file mode 100644
>> index 000000000000..1442d6ef63d2
>> --- /dev/null
>> +++ b/cmd/pinmux.c
>> @@ -0,0 +1,115 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2018, STMicroelectronics - All Rights Reserved
>> + */
>> +
>> +#include <command.h>
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <dm/pinctrl.h>
>> +#include <dm/uclass-internal.h>
>> +
>> +#ifdef CONFIG_PINCTRL
> 
> and here again.

ok

Thanks

Patrice

> 
> The rest looks quite standard from quick look.
> 
> Thanks,
> Michal
> _______________________________________________
> Uboot-stm32 mailing list
> Uboot-stm32 at st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/uboot-stm32
> 

[U-Boot] [PATCH v1 2/5] cmd: pinmux: Add pinmux command

[Simon Glass]

Hi Patrice,

On 20 September 2018 at 07:37, Patrice Chotard <patrice.chotard@st.com> wrote:
> pinmux command allows to :
>  - list all pin-controllers available on platforms
>  - select a pin-controller
>  - display the muxing of all pins of the current pin-controller
>    or all pin-controllers depending of given options
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
>  cmd/Kconfig  |   8 +++++
>  cmd/Makefile |   3 ++
>  cmd/pinmux.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 126 insertions(+)
>  create mode 100644 cmd/pinmux.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 13d4c991bf8b..2c687ceecf49 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -937,6 +937,14 @@ config CMD_PCMCIA
>           about 1990. These devices are typically removable memory or network
>           cards using a standard 68-pin connector.
>
> +config CMD_PINMUX
> +       bool "pinmux - show pins muxing"
> +       depends on PINCTRL

Should this be default y? It seems that we should normally enable
commands like this which provide useful info.

[..]

> diff --git a/cmd/pinmux.c b/cmd/pinmux.c
> new file mode 100644
> index 000000000000..1442d6ef63d2
> --- /dev/null
> +++ b/cmd/pinmux.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018, STMicroelectronics - All Rights Reserved
> + */
> +
> +#include <command.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <dm/pinctrl.h>
> +#include <dm/uclass-internal.h>
> +
> +#ifdef CONFIG_PINCTRL
> +
> +#define LIMIT_DEVNAME  30
> +#define LIMIT_OFNAME   32
> +
> +static struct udevice *currdev;
> +
> +static int do_dev(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       const char *name;
> +       int ret;
> +
> +       switch (argc) {
> +       case 2:
> +               name = argv[1];
> +               ret = uclass_get_device_by_name(UCLASS_PINCTRL, name, &currdev);
> +               if (ret) {
> +                       printf("Can't get the pin-controller: %s!\n", name);
> +                       return CMD_RET_FAILURE;
> +               }
> +       case 1:
> +               if (!currdev) {
> +                       printf("Pin-controller device is not set!\n\n");

Did you intend to have two \n ?

[..]

> +
> +       for (uclass_first_device(UCLASS_PINCTRL, &dev); dev;
> +            uclass_next_device(&dev)) {

Can you please add a macro a bit like uclass_foreach_dev() for this
pattern? It seems to come up a lot.

I'm not sure what to call it. Perhaps uclass_foreach_dev_probe()?

> +               /* insert a separator between each pin-controller display */
> +               printf("--------------------------\n");
> +               printf("%s:\n", dev->name);
> +               ret = pinctrl_pinmux_show(dev);
> +               if (ret)
> +                       return ret;
> +       }
> +       return CMD_RET_SUCCESS;

Or just 0. It will always be 0.


Regards,
Simon

[U-Boot] [PATCH v1 4/5] pinctrl: stm32: Add pinmux_show() ops

[Simon Glass]

Hi Patrice,

On 20 September 2018 at 07:37, Patrice Chotard <patrice.chotard@st.com> wrote:
> pinmux_show allows to display the muxing of all pins
> belonging to pin-controller.
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
>  drivers/pinctrl/pinctrl_stm32.c | 79 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
>

The only thing I don't like about this is that it is quite
SoC-specific. I am hoping we can make it more generic.

> diff --git a/drivers/pinctrl/pinctrl_stm32.c b/drivers/pinctrl/pinctrl_stm32.c
> index 31285cdd5784..a477035bf420 100644
> --- a/drivers/pinctrl/pinctrl_stm32.c
> +++ b/drivers/pinctrl/pinctrl_stm32.c
> @@ -14,6 +14,16 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define OTYPE_MSK                      1
>  #define AFR_MASK                       0xF
>
> +#define PINMUX_MODE_COUNT              5
> +
> +static const char * const pinmux_mode[PINMUX_MODE_COUNT] = {
> +       "gpio input",
> +       "gpio output",
> +       "analog",
> +       "unknown",
> +       "alt function",
> +};
> +
>  static int stm32_gpio_config(struct gpio_desc *desc,
>                              const struct stm32_gpio_ctl *ctl)
>  {
> @@ -176,12 +186,81 @@ static int stm32_pinctrl_set_state_simple(struct udevice *dev,
>  }
>  #endif /* PINCTRL_FULL */
>
> +static int stm32_pinctrl_get_af(struct udevice *dev,  unsigned int offset)
> +{
> +       struct stm32_gpio_priv *priv = dev_get_priv(dev);
> +       struct stm32_gpio_regs *regs = priv->regs;
> +       u32 af;
> +       u32 alt_shift = (offset % 8) * 4;
> +       u32 alt_index =  offset / 8;
> +
> +       af = (readl(&regs->afr[alt_index]) &
> +             GENMASK(alt_shift + 3, alt_shift)) >> alt_shift;
> +
> +       return af;
> +}
> +
> +static int stm32_pinmux_show(struct udevice *dev)
> +{
> +       struct udevice *child;
> +       struct udevice *dev_gpio;
> +       const char *bank_name;
> +       const char *label;
> +       int offset;
> +       int ret;
> +       int num_bits;
> +       int mode;
> +       int af_num;
> +
> +       /* parse pin-controller sub-nodes, ie gpio bank nodes */
> +       list_for_each_entry(child, &dev->child_head, sibling_node) {
> +               ret = uclass_get_device_by_name(UCLASS_GPIO, child->name,
> +                                               &dev_gpio);

I wonder to what extend this is actually different from 'gpio status'?

> +
> +               if (ret < 0 && ret != -ENODEV) {
> +                       dev_err(dev, "Failed to find %s device ret = %d\n",
> +                               child->name, ret);
> +                       return ret;
> +               }
> +
> +               if (!ret) {
> +                       bank_name = gpio_get_bank_info(dev_gpio, &num_bits);
> +
> +                       printf("\nBank %s:\n", bank_name);
> +                       for (offset = 0; offset < num_bits; offset++) {
> +                               mode = gpio_get_raw_function(dev_gpio,
> +                                                            offset, &label);
> +                               printf("%s%d: %s", bank_name, offset,
> +                                      pinmux_mode[mode]);
> +                               switch (mode) {
> +                               case GPIOF_FUNC:
> +                                       af_num = stm32_pinctrl_get_af(dev_gpio,
> +                                                                     offset);
> +                                       printf(" %d", af_num);
> +                                       break;
> +                               case STM32_GPIO_MODE_OUT:

How come you cannot use GPIOF_OUTPUT here?

> +                               case STM32_GPIO_MODE_IN:
> +                                       printf(" %s", label ? label : "");
> +                                       break;
> +                               }
> +                               printf("\n");
> +                       }
> +               }
> +
> +               if (!child)
> +                       break;
> +       }
> +
> +       return 0;
> +}
> +
>  static struct pinctrl_ops stm32_pinctrl_ops = {
>  #if CONFIG_IS_ENABLED(PINCTRL_FULL)
>         .set_state              = stm32_pinctrl_set_state,
>  #else /* PINCTRL_FULL */
>         .set_state_simple       = stm32_pinctrl_set_state_simple,
>  #endif /* PINCTRL_FULL */
> +       .pinmux_show            = stm32_pinmux_show,
>  };
>
>  static const struct udevice_id stm32_pinctrl_ids[] = {
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH v1 1/5] dm: pinctrl: Add pinmux_show() ops

[Simon Glass]

Hi Patrice,

On 20 September 2018 at 07:37, Patrice Chotard <patrice.chotard@st.com> wrote:
> pinmux_show() allows to display the muxing of all pins
> belonging to a pin-controller
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
>  drivers/pinctrl/pinctrl-uclass.c | 10 ++++++++++
>  include/dm/pinctrl.h             | 29 +++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
>

If you look at this pinctrl method:

const char *(*get_pin_name)(struct udevice *dev, unsigned selector);

you can see that we can specify particular pins. Can we have the
command discover what pins are available and iterate over them? Then
we can have a function which returns the status of a particular pin as
a string, and the command can output it. Perhaps like cpu_get_desc()?

Regards,
Simon

[U-Boot] [PATCH v1 2/5] cmd: pinmux: Add pinmux command

[Patrice CHOTARD]

Hi Simon

On 09/26/2018 07:41 AM, Simon Glass wrote:
> Hi Patrice,
> 
> On 20 September 2018 at 07:37, Patrice Chotard <patrice.chotard@st.com> wrote:
>> pinmux command allows to :
>>  - list all pin-controllers available on platforms
>>  - select a pin-controller
>>  - display the muxing of all pins of the current pin-controller
>>    or all pin-controllers depending of given options
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>>  cmd/Kconfig  |   8 +++++
>>  cmd/Makefile |   3 ++
>>  cmd/pinmux.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 126 insertions(+)
>>  create mode 100644 cmd/pinmux.c
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 13d4c991bf8b..2c687ceecf49 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -937,6 +937,14 @@ config CMD_PCMCIA
>>           about 1990. These devices are typically removable memory or network
>>           cards using a standard 68-pin connector.
>>
>> +config CMD_PINMUX
>> +       bool "pinmux - show pins muxing"
>> +       depends on PINCTRL
> 
> Should this be default y? It seems that we should normally enable
> commands like this which provide useful info.

Yes, i will make it default y if PINCTRL is set

> 
> [..]
> 
>> diff --git a/cmd/pinmux.c b/cmd/pinmux.c
>> new file mode 100644
>> index 000000000000..1442d6ef63d2
>> --- /dev/null
>> +++ b/cmd/pinmux.c
>> @@ -0,0 +1,115 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2018, STMicroelectronics - All Rights Reserved
>> + */
>> +
>> +#include <command.h>
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <dm/pinctrl.h>
>> +#include <dm/uclass-internal.h>
>> +
>> +#ifdef CONFIG_PINCTRL
>> +
>> +#define LIMIT_DEVNAME  30
>> +#define LIMIT_OFNAME   32
>> +
>> +static struct udevice *currdev;
>> +
>> +static int do_dev(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +       const char *name;
>> +       int ret;
>> +
>> +       switch (argc) {
>> +       case 2:
>> +               name = argv[1];
>> +               ret = uclass_get_device_by_name(UCLASS_PINCTRL, name, &currdev);
>> +               if (ret) {
>> +                       printf("Can't get the pin-controller: %s!\n", name);
>> +                       return CMD_RET_FAILURE;
>> +               }
>> +       case 1:
>> +               if (!currdev) {
>> +                       printf("Pin-controller device is not set!\n\n");
> 
> Did you intend to have two \n ?

No, it's a typo issue

> 
> [..]
> 
>> +
>> +       for (uclass_first_device(UCLASS_PINCTRL, &dev); dev;
>> +            uclass_next_device(&dev)) {
> 
> Can you please add a macro a bit like uclass_foreach_dev() for this
> pattern? It seems to come up a lot.
> 
> I'm not sure what to call it. Perhaps uclass_foreach_dev_probe()?

Yes, no problem, i will add it

> 
>> +               /* insert a separator between each pin-controller display */
>> +               printf("--------------------------\n");
>> +               printf("%s:\n", dev->name);
>> +               ret = pinctrl_pinmux_show(dev);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +       return CMD_RET_SUCCESS;
> 
> Or just 0. It will always be 0.

CMD_RET_SUCCESS value is already 0

Thanks

Patrice

> 
> 
> Regards,
> Simon
> 

[U-Boot] [Uboot-stm32] [PATCH v1 4/5] pinctrl: stm32: Add pinmux_show() ops

[Patrice CHOTARD]

Hi Simon

On 09/26/2018 07:41 AM, Simon Glass wrote:
> Hi Patrice,
> 
> On 20 September 2018 at 07:37, Patrice Chotard <patrice.chotard@st.com> wrote:
>> pinmux_show allows to display the muxing of all pins
>> belonging to pin-controller.
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>>  drivers/pinctrl/pinctrl_stm32.c | 79 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 79 insertions(+)
>>
> 
> The only thing I don't like about this is that it is quite
> SoC-specific. I am hoping we can make it more generic.

I will rework it to make it more independant.

> 
>> diff --git a/drivers/pinctrl/pinctrl_stm32.c b/drivers/pinctrl/pinctrl_stm32.c
>> index 31285cdd5784..a477035bf420 100644
>> --- a/drivers/pinctrl/pinctrl_stm32.c
>> +++ b/drivers/pinctrl/pinctrl_stm32.c
>> @@ -14,6 +14,16 @@ DECLARE_GLOBAL_DATA_PTR;
>>  #define OTYPE_MSK                      1
>>  #define AFR_MASK                       0xF
>>
>> +#define PINMUX_MODE_COUNT              5
>> +
>> +static const char * const pinmux_mode[PINMUX_MODE_COUNT] = {
>> +       "gpio input",
>> +       "gpio output",
>> +       "analog",
>> +       "unknown",
>> +       "alt function",
>> +};
>> +
>>  static int stm32_gpio_config(struct gpio_desc *desc,
>>                              const struct stm32_gpio_ctl *ctl)
>>  {
>> @@ -176,12 +186,81 @@ static int stm32_pinctrl_set_state_simple(struct udevice *dev,
>>  }
>>  #endif /* PINCTRL_FULL */
>>
>> +static int stm32_pinctrl_get_af(struct udevice *dev,  unsigned int offset)
>> +{
>> +       struct stm32_gpio_priv *priv = dev_get_priv(dev);
>> +       struct stm32_gpio_regs *regs = priv->regs;
>> +       u32 af;
>> +       u32 alt_shift = (offset % 8) * 4;
>> +       u32 alt_index =  offset / 8;
>> +
>> +       af = (readl(&regs->afr[alt_index]) &
>> +             GENMASK(alt_shift + 3, alt_shift)) >> alt_shift;
>> +
>> +       return af;
>> +}
>> +
>> +static int stm32_pinmux_show(struct udevice *dev)
>> +{
>> +       struct udevice *child;
>> +       struct udevice *dev_gpio;
>> +       const char *bank_name;
>> +       const char *label;
>> +       int offset;
>> +       int ret;
>> +       int num_bits;
>> +       int mode;
>> +       int af_num;
>> +
>> +       /* parse pin-controller sub-nodes, ie gpio bank nodes */
>> +       list_for_each_entry(child, &dev->child_head, sibling_node) {
>> +               ret = uclass_get_device_by_name(UCLASS_GPIO, child->name,
>> +                                               &dev_gpio);
> 
> I wonder to what extend this is actually different from 'gpio status'?

I agree, currently, "pinmux show" and "gpio status" comments are
overlapping..
At the end, as on kernel side, only pinmux should be used to know the
pin state (configured as a GPIO or as alternate function).

pinmux command indicates if pins is configured as an alternate function
and indicates which one, what "gpio status" can't do.

> 
>> +
>> +               if (ret < 0 && ret != -ENODEV) {
>> +                       dev_err(dev, "Failed to find %s device ret = %d\n",
>> +                               child->name, ret);
>> +                       return ret;
>> +               }
>> +
>> +               if (!ret) {
>> +                       bank_name = gpio_get_bank_info(dev_gpio, &num_bits);
>> +
>> +                       printf("\nBank %s:\n", bank_name);
>> +                       for (offset = 0; offset < num_bits; offset++) {
>> +                               mode = gpio_get_raw_function(dev_gpio,
>> +                                                            offset, &label);
>> +                               printf("%s%d: %s", bank_name, offset,
>> +                                      pinmux_mode[mode]);
>> +                               switch (mode) {
>> +                               case GPIOF_FUNC:
>> +                                       af_num = stm32_pinctrl_get_af(dev_gpio,
>> +                                                                     offset);
>> +                                       printf(" %d", af_num);
>> +                                       break;
>> +                               case STM32_GPIO_MODE_OUT:
> 
> How come you cannot use GPIOF_OUTPUT here?

Good catch, GPIOF_OUPUT/INPUT must be used instead of
STM32_GPIO_MODE_OUT/IN.

> 
>> +                               case STM32_GPIO_MODE_IN:
>> +                                       printf(" %s", label ? label : "");
>> +                                       break;
>> +                               }
>> +                               printf("\n");
>> +                       }
>> +               }
>> +
>> +               if (!child)
>> +                       break;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  static struct pinctrl_ops stm32_pinctrl_ops = {
>>  #if CONFIG_IS_ENABLED(PINCTRL_FULL)
>>         .set_state              = stm32_pinctrl_set_state,
>>  #else /* PINCTRL_FULL */
>>         .set_state_simple       = stm32_pinctrl_set_state_simple,
>>  #endif /* PINCTRL_FULL */
>> +       .pinmux_show            = stm32_pinmux_show,
>>  };
>>
>>  static const struct udevice_id stm32_pinctrl_ids[] = {
>> --
>> 1.9.1
>>
> 
> Regards,
> Simon
> _______________________________________________
> Uboot-stm32 mailing list
> Uboot-stm32 at st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/uboot-stm32
> 

[U-Boot] [PATCH v1 1/5] dm: pinctrl: Add pinmux_show() ops

[Patrice CHOTARD]

Hi Simon

On 09/26/2018 07:42 AM, Simon Glass wrote:
> Hi Patrice,
> 
> On 20 September 2018 at 07:37, Patrice Chotard <patrice.chotard@st.com> wrote:
>> pinmux_show() allows to display the muxing of all pins
>> belonging to a pin-controller
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>>  drivers/pinctrl/pinctrl-uclass.c | 10 ++++++++++
>>  include/dm/pinctrl.h             | 29 +++++++++++++++++++++++++++++
>>  2 files changed, 39 insertions(+)
>>
> 
> If you look at this pinctrl method:
> 
> const char *(*get_pin_name)(struct udevice *dev, unsigned selector);
> 
> you can see that we can specify particular pins. Can we have the
> command discover what pins are available and iterate over them? Then
> we can have a function which returns the status of a particular pin as
> a string, and the command can output it. Perhaps like cpu_get_desc()?

Understand, i will rework this series following this way of doing.

Thanks

Patrice

> 
> Regards,
> Simon
>