pinctrl: Add pincontrol driver for ARTPEC-6 SoC

PATCH In review Jesper Nilsson 2 participants 5 messages Mar 30, 2017 – Apr 7, 2017

Subsystems (3)

GPIO Pin control Arm/artpec machine support

lore

Share & tools

lore

Jesper Nilsson

author maintainer

2017-03-30 11:33 UTC #0 lore raw

PATCH 2/3 pinctrl: Add pincontrol driver for ARTPEC-6 SoC

Add pinctrl driver support for the Axis ARTPEC-6 SoC. There are only some pins that actually have different functions available, but all can control bias (pull-up/-down) and drive strength.
Code originally written by Chris Paterson.

Signed-off-by Jesper Nilsson <[email protected]>

---
 MAINTAINERS                       |    1 +
 drivers/pinctrl/Kconfig           |   11 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/pinctrl-artpec6.c | 1279 +++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-artpec6.h |  151 +++++
 5 files changed, 1443 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-artpec6.c
 create mode 100644 drivers/pinctrl/pinctrl-artpec6.h

MAINTAINERS +1 -0

index 7563bd6..d2b2449 100644
@@ -1088,6 +1088,7 @@ L:	[email protected]
 F:	arch/arm/mach-artpec
 F:	arch/arm/boot/dts/artpec6*
 F:	drivers/clk/axis
+F:	drivers/pinctrl/pinctrl-artpec*
 F:	Documentation/devicetree/bindings/pinctrl/axis,artpec6-pinctrl.txt
 
 ARM/ASPEED MACHINE SUPPORT

drivers/pinctrl/Kconfig +11 -0

index 8f8c2af..37af5e3 100644
@@ -41,6 +41,17 @@ config PINCTRL_ADI2
 	  future processors. This option is selected automatically when specific
 	  machine and arch are selected to build.
 
+config PINCTRL_ARTPEC6
+        bool "Axis ARTPEC-6 pin controller driver"
+        depends on MACH_ARTPEC6
+        select PINMUX
+        select GENERIC_PINCONF
+        help
+          This is the driver for the Axis ARTPEC-6 pin controller. This driver
+          supports pin function multiplexing as well as pin bias and drive
+          strength configuration. Device tree integration instructions can be
+          found in Documentation/devicetree/bindings/pinctrl/axis,artpec6-pinctrl.txt
+
 config PINCTRL_AS3722
 	tristate "Pinctrl and GPIO driver for ams AS3722 PMIC"
 	depends on MFD_AS3722 && GPIOLIB

drivers/pinctrl/Makefile +1 -0

index a251f43..193dbba 100644
@@ -8,6 +8,7 @@ obj-$(CONFIG_PINCONF)		+= pinconf.o
 obj-$(CONFIG_OF)		+= devicetree.o
 obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
 obj-$(CONFIG_PINCTRL_ADI2)	+= pinctrl-adi2.o
+obj-$(CONFIG_PINCTRL_ARTPEC6)   += pinctrl-artpec6.o
 obj-$(CONFIG_PINCTRL_AS3722)	+= pinctrl-as3722.o
 obj-$(CONFIG_PINCTRL_BF54x)	+= pinctrl-adi2-bf54x.o
 obj-$(CONFIG_PINCTRL_BF60x)	+= pinctrl-adi2-bf60x.o

drivers/pinctrl/pinctrl-artpec6.c new file +1279 -0

1,279 lines not on this page · raw message

drivers/pinctrl/pinctrl-artpec6.h new file +151 -0

151 lines not on this page · raw message

Signature · 6 lines

2.1.4


/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- [email protected]

Linus Walleij

2017-03-30 12:07 UTC #1 lore raw

Quoted 7 lines · Jesper Nilsson

Add pinctrl driver support for the Axis ARTPEC-6 SoC. There are only some pins that actually have different functions available, but all can control bias (pull-up/-down) and drive strength.
Code originally written by Chris Paterson.
Signed-off-by Jesper Nilsson <[email protected]>

Very good start, but look at this:

Quoted 6 lines

drivers/pinctrl/pinctrl-artpec6.h new file +6 -0

+#define ARTPEC6_PINMUX_P0_0_CTRL       0x000
+#define ARTPEC6_PINMUX_P0_1_CTRL       0x004
+#define ARTPEC6_PINMUX_P0_2_CTRL       0x008
+#define ARTPEC6_PINMUX_P0_3_CTRL       0x00C
+#define ARTPEC6_PINMUX_P0_4_CTRL       0x010
+#define ARTPEC6_PINMUX_P0_5_CTRL       0x014

drivers/pinctrl/pinctrl-artpec6.h line 30

Quoted 10 lines

drivers/pinctrl/pinctrl-artpec6.h new file +10 -0

+#define ARTPEC6_PINMUX_P0_6_CTRL       0x018
+#define ARTPEC6_PINMUX_P0_7_CTRL       0x01C
+#define ARTPEC6_PINMUX_P0_8_CTRL       0x020
+#define ARTPEC6_PINMUX_P0_9_CTRL       0x024
+#define ARTPEC6_PINMUX_P0_10_CTRL      0x028
+#define ARTPEC6_PINMUX_P0_11_CTRL      0x02C
+#define ARTPEC6_PINMUX_P0_12_CTRL      0x030
+#define ARTPEC6_PINMUX_P0_13_CTRL      0x034
+#define ARTPEC6_PINMUX_P0_14_CTRL      0x038
+#define ARTPEC6_PINMUX_P0_15_CTRL      0x03C

It's pretty clear that the stride is always 4 bytes is it not.

Quoted 7 lines

drivers/pinctrl/pinctrl-artpec6.c new file +7 -0

+static const unsigned int pin_regs[] = {
+       ARTPEC6_PINMUX_P0_0_CTRL,       /* Pin 0 */
+       ARTPEC6_PINMUX_P0_1_CTRL,
+       ARTPEC6_PINMUX_P0_2_CTRL,
+       ARTPEC6_PINMUX_P0_3_CTRL,
+       ARTPEC6_PINMUX_P0_4_CTRL,
+       ARTPEC6_PINMUX_P0_5_CTRL,

drivers/pinctrl/pinctrl-artpec6.c line 172

Quoted 10 lines

drivers/pinctrl/pinctrl-artpec6.c new file +10 -0

+       ARTPEC6_PINMUX_P0_6_CTRL,
+       ARTPEC6_PINMUX_P0_7_CTRL,
+       ARTPEC6_PINMUX_P0_8_CTRL,
+       ARTPEC6_PINMUX_P0_9_CTRL,
+       ARTPEC6_PINMUX_P0_10_CTRL,
+       ARTPEC6_PINMUX_P0_11_CTRL,
+       ARTPEC6_PINMUX_P0_12_CTRL,
+       ARTPEC6_PINMUX_P0_13_CTRL,
+       ARTPEC6_PINMUX_P0_14_CTRL,
+       ARTPEC6_PINMUX_P0_15_CTRL,

The oceans of redundant information are rising ;)

drivers/pinctrl/pinctrl-artpec6.c line 329

Quoted 12 lines

drivers/pinctrl/pinctrl-artpec6.c new file +12 -0

+static const unsigned int uart0_regs0[] = {
+       ARTPEC6_PINMUX_P1_0_CTRL,
+       ARTPEC6_PINMUX_P1_1_CTRL,
+       ARTPEC6_PINMUX_P1_2_CTRL,
+       ARTPEC6_PINMUX_P1_3_CTRL,
+       ARTPEC6_PINMUX_P1_4_CTRL,
+       ARTPEC6_PINMUX_P1_5_CTRL,
+       ARTPEC6_PINMUX_P1_6_CTRL,
+       ARTPEC6_PINMUX_P1_7_CTRL,
+       ARTPEC6_PINMUX_P1_8_CTRL,
+       ARTPEC6_PINMUX_P1_9_CTRL,
+};

And rising.

drivers/pinctrl/pinctrl-artpec6.c line 586

Quoted 8 lines

drivers/pinctrl/pinctrl-artpec6.c new file +8 -0

+       {
+               .name = "uart0grp0",
+               .pins = uart0_pins0,
+               .num_pins = ARRAY_SIZE(uart0_pins0),
+               .reg_offsets = uart0_regs0,
+               .num_regs = ARRAY_SIZE(uart0_regs0),
+               .config = ARTPEC6_CONFIG_1,
+       },

So what if the struct artpec6_pin_group...

drivers/pinctrl/pinctrl-artpec6.c line 47

Quoted 8 lines

drivers/pinctrl/pinctrl-artpec6.c new file +8 -0

+struct artpec6_pin_group {
+       const char         *name;
+       const unsigned int *pins;
+       const unsigned int num_pins;
+       const unsigned int *reg_offsets;
+       const unsigned int num_regs;
+       unsigned char      config;
+};

Instead of reg_offsets had reg_offset_base, and you just calculate it with base + pin * 4 when you need it?

I also highly suspect that num_pins and num_regs above are exactly the same number in all cases, correct? You probably only need num_pins.

Quoted 1 line

+static struct artpec6_pmx_func artpec6_pmx_functions[] = {

Needs const

Quoted 23 lines

drivers/pinctrl/pinctrl-artpec6.c new file +23 -0

+static void artpec6_pmx_select_func(struct pinctrl_dev *pctldev,
+                                   unsigned int function, unsigned int group,
+                                   bool enable)
+{
+       unsigned int regval, val;
+       int i;
+       const unsigned int *pmx_regs;
+       struct artpec6_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+       pmx_regs = artpec6_pin_groups[group].reg_offsets;
+
+       for (i = 0; i < artpec6_pin_groups[group].num_regs; i++) {
+               /* Ports 4-8 do not have a SEL field and are always selected */
+               if (pmx_regs[i] >= ARTPEC6_PINMUX_P4_0_CTRL)
+                       continue;
+
+               if (!strcmp(artpec6_pmx_get_fname(pctldev, function), "gpio")) {
+                       /* GPIO is always config 0 */
+                       val = ARTPEC6_CONFIG_0 << ARTPEC6_PINMUX_SEL_SHIFT;
+               } else {
+                       if (enable)
+                               val = artpec6_pin_groups[group].config
+                                       << ARTPEC6_PINMUX_SEL_SHIFT;

drivers/pinctrl/pinctrl-artpec6.c line 942

Quoted 10 lines

drivers/pinctrl/pinctrl-artpec6.c new file +10 -0

+                       else
+                               val = ARTPEC6_CONFIG_0
+                                       << ARTPEC6_PINMUX_SEL_SHIFT;
+               }
+
+               regval = readl(pmx->base + pmx_regs[i]);
+               regval &= ~ARTPEC6_PINMUX_SEL_MASK;
+               regval |= val;
+               writel(regval, pmx->base + pmx_regs[i]);
+       }

So thus look can be different and include something like +=4 for the registers for each iteration.

drivers/pinctrl/pinctrl-artpec6.h

Quoted 2 lines

drivers/pinctrl/pinctrl-artpec6.h new file +0 -0

You don't need to have these defines in their own file, just copy it into the top of the driver file.

drivers/pinctrl/pinctrl-artpec6.h line 33

Quoted 4 lines

drivers/pinctrl/pinctrl-artpec6.h new file +4 -0

+/* Pinmux control register offset definitions */
+
+#define ARTPEC6_PINMUX_P1_0_CTRL       0x040
+#define ARTPEC6_PINMUX_P1_1_CTRL       0x044

(...)

So for these defines you only need the first one.

With these things fixes I'm pretty sure it is close to finished.

Yours,
Linus Walleij

Jesper Nilsson

author maintainer

2017-03-30 14:23 UTC #2 lore raw

Quoted 22 lines · Linus Walleij

Quoted 7 lines · Jesper Nilsson
Add pinctrl driver support for the Axis ARTPEC-6 SoC. There are only some pins that actually have different functions available, but all can control bias (pull-up/-down) and drive strength.
Code originally written by Chris Paterson.
Signed-off-by Jesper Nilsson <[email protected]>
Very good start, but look at this:
Quoted 10 lines
drivers/pinctrl/pinctrl-artpec6.h new file +10 -0
+#define ARTPEC6_PINMUX_P0_0_CTRL       0x000
+#define ARTPEC6_PINMUX_P0_1_CTRL       0x004
+#define ARTPEC6_PINMUX_P0_2_CTRL       0x008
+#define ARTPEC6_PINMUX_P0_3_CTRL       0x00C
+#define ARTPEC6_PINMUX_P0_4_CTRL       0x010
+#define ARTPEC6_PINMUX_P0_5_CTRL       0x014
+#define ARTPEC6_PINMUX_P0_6_CTRL       0x018
+#define ARTPEC6_PINMUX_P0_7_CTRL       0x01C
+#define ARTPEC6_PINMUX_P0_8_CTRL       0x020
+#define ARTPEC6_PINMUX_P0_9_CTRL       0x024

Quoted 8 lines

drivers/pinctrl/pinctrl-artpec6.h line 30

Quoted 6 lines

drivers/pinctrl/pinctrl-artpec6.h new file +6 -0

+#define ARTPEC6_PINMUX_P0_10_CTRL      0x028
+#define ARTPEC6_PINMUX_P0_11_CTRL      0x02C
+#define ARTPEC6_PINMUX_P0_12_CTRL      0x030
+#define ARTPEC6_PINMUX_P0_13_CTRL      0x034
+#define ARTPEC6_PINMUX_P0_14_CTRL      0x038
+#define ARTPEC6_PINMUX_P0_15_CTRL      0x03C

It's pretty clear that the stride is always 4 bytes is it not.

Agreed.

Quoted 20 lines

Quoted 7 lines

drivers/pinctrl/pinctrl-artpec6.c new file +7 -0

+static const unsigned int pin_regs[] = {
+       ARTPEC6_PINMUX_P0_0_CTRL,       /* Pin 0 */
+       ARTPEC6_PINMUX_P0_1_CTRL,
+       ARTPEC6_PINMUX_P0_2_CTRL,
+       ARTPEC6_PINMUX_P0_3_CTRL,
+       ARTPEC6_PINMUX_P0_4_CTRL,
+       ARTPEC6_PINMUX_P0_5_CTRL,

drivers/pinctrl/pinctrl-artpec6.c line 172

Quoted 10 lines

drivers/pinctrl/pinctrl-artpec6.c new file +10 -0

+       ARTPEC6_PINMUX_P0_6_CTRL,
+       ARTPEC6_PINMUX_P0_7_CTRL,
+       ARTPEC6_PINMUX_P0_8_CTRL,
+       ARTPEC6_PINMUX_P0_9_CTRL,
+       ARTPEC6_PINMUX_P0_10_CTRL,
+       ARTPEC6_PINMUX_P0_11_CTRL,
+       ARTPEC6_PINMUX_P0_12_CTRL,
+       ARTPEC6_PINMUX_P0_13_CTRL,
+       ARTPEC6_PINMUX_P0_14_CTRL,
+       ARTPEC6_PINMUX_P0_15_CTRL,

The oceans of redundant information are rising ;)

Quoted 14 lines

drivers/pinctrl/pinctrl-artpec6.c line 329

Quoted 12 lines

drivers/pinctrl/pinctrl-artpec6.c new file +12 -0

+static const unsigned int uart0_regs0[] = {
+       ARTPEC6_PINMUX_P1_0_CTRL,
+       ARTPEC6_PINMUX_P1_1_CTRL,
+       ARTPEC6_PINMUX_P1_2_CTRL,
+       ARTPEC6_PINMUX_P1_3_CTRL,
+       ARTPEC6_PINMUX_P1_4_CTRL,
+       ARTPEC6_PINMUX_P1_5_CTRL,
+       ARTPEC6_PINMUX_P1_6_CTRL,
+       ARTPEC6_PINMUX_P1_7_CTRL,
+       ARTPEC6_PINMUX_P1_8_CTRL,
+       ARTPEC6_PINMUX_P1_9_CTRL,
+};

And rising.

:-)

Quoted 11 lines

drivers/pinctrl/pinctrl-artpec6.c line 586
Quoted 8 lines
drivers/pinctrl/pinctrl-artpec6.c new file +8 -0
+       {
+               .name = "uart0grp0",
+               .pins = uart0_pins0,
+               .num_pins = ARRAY_SIZE(uart0_pins0),
+               .reg_offsets = uart0_regs0,
+               .num_regs = ARRAY_SIZE(uart0_regs0),
+               .config = ARTPEC6_CONFIG_1,
+       },
So what if the struct artpec6_pin_group...

Quoted 11 lines

drivers/pinctrl/pinctrl-artpec6.c line 47
Quoted 8 lines
drivers/pinctrl/pinctrl-artpec6.c new file +8 -0
+struct artpec6_pin_group {
+       const char         *name;
+       const unsigned int *pins;
+       const unsigned int num_pins;
+       const unsigned int *reg_offsets;
+       const unsigned int num_regs;
+       unsigned char      config;
+};
Instead of reg_offsets had reg_offset_base, and you just calculate it with base + pin * 4 when you need it?

Yes, that would be much clearer.

Quoted 3 lines

I also highly suspect that num_pins and num_regs above are exactly the same number in all cases, correct? You probably only need num_pins.

Agreed.

Quoted 41 lines

Quoted 1 line

+static struct artpec6_pmx_func artpec6_pmx_functions[] = {

Needs const

Quoted 23 lines

drivers/pinctrl/pinctrl-artpec6.c new file +23 -0

+static void artpec6_pmx_select_func(struct pinctrl_dev *pctldev,
+                                   unsigned int function, unsigned int group,
+                                   bool enable)
+{
+       unsigned int regval, val;
+       int i;
+       const unsigned int *pmx_regs;
+       struct artpec6_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+       pmx_regs = artpec6_pin_groups[group].reg_offsets;
+
+       for (i = 0; i < artpec6_pin_groups[group].num_regs; i++) {
+               /* Ports 4-8 do not have a SEL field and are always selected */
+               if (pmx_regs[i] >= ARTPEC6_PINMUX_P4_0_CTRL)
+                       continue;
+
+               if (!strcmp(artpec6_pmx_get_fname(pctldev, function), "gpio")) {
+                       /* GPIO is always config 0 */
+                       val = ARTPEC6_CONFIG_0 << ARTPEC6_PINMUX_SEL_SHIFT;
+               } else {
+                       if (enable)
+                               val = artpec6_pin_groups[group].config
+                                       << ARTPEC6_PINMUX_SEL_SHIFT;

drivers/pinctrl/pinctrl-artpec6.c line 942

Quoted 10 lines

drivers/pinctrl/pinctrl-artpec6.c new file +10 -0

+                       else
+                               val = ARTPEC6_CONFIG_0
+                                       << ARTPEC6_PINMUX_SEL_SHIFT;
+               }
+
+               regval = readl(pmx->base + pmx_regs[i]);
+               regval &= ~ARTPEC6_PINMUX_SEL_MASK;
+               regval |= val;
+               writel(regval, pmx->base + pmx_regs[i]);
+       }

So thus look can be different and include something like +=4 for the registers for each iteration.

Quoted 5 lines

drivers/pinctrl/pinctrl-artpec6.h
Quoted 2 lines
drivers/pinctrl/pinctrl-artpec6.h new file +0 -0
You don't need to have these defines in their own file, just copy it into the top of the driver file.

Ok.

Quoted 9 lines

drivers/pinctrl/pinctrl-artpec6.h line 33
Quoted 4 lines
drivers/pinctrl/pinctrl-artpec6.h new file +4 -0
+/* Pinmux control register offset definitions */
+
+#define ARTPEC6_PINMUX_P1_0_CTRL       0x040
+#define ARTPEC6_PINMUX_P1_1_CTRL       0x044
(...)
So for these defines you only need the first one.
With these things fixes I'm pretty sure it is close to finished.

Thanks for the feedback, will rework and resend. :-)

Quoted 2 lines

Yours,
Linus Walleij

/^JN - Jesper Nilsson

Signature · 1 line

               Jesper Nilsson -- [email protected]

Jesper Nilsson

author maintainer

2017-04-03 12:47 UTC #3 lore raw

PATCH v2 2/3 pinctrl: Add pincontrol driver for ARTPEC-6 SoC

In reply to Linus Walleij

Signed-off-by Jesper Nilsson <[email protected]>

---
Changes since v1:
- Get rid of the artpec-6 pinctrl header
- Replace the register arrays with a function that maps from pin number to the register offset.
- Cleanup duplicate information in artpec6_pin_groups array

 MAINTAINERS                       |   1 +
 drivers/pinctrl/Kconfig           |  11 +
 drivers/pinctrl/Makefile          |   1 +
 drivers/pinctrl/pinctrl-artpec6.c | 992 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 1005 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-artpec6.c

MAINTAINERS +1 -0

index 7563bd6fdfbe..d2b2449c3f4f 100644
@@ -1088,6 +1088,7 @@ L:	[email protected]
 F:	arch/arm/mach-artpec
 F:	arch/arm/boot/dts/artpec6*
 F:	drivers/clk/axis
+F:	drivers/pinctrl/pinctrl-artpec*
 F:	Documentation/devicetree/bindings/pinctrl/axis,artpec6-pinctrl.txt
 
 ARM/ASPEED MACHINE SUPPORT

drivers/pinctrl/Kconfig +11 -0

index 8f8c2af45781..37af5e3029d5 100644
@@ -41,6 +41,17 @@ config PINCTRL_ADI2
 	  future processors. This option is selected automatically when specific
 	  machine and arch are selected to build.
 
+config PINCTRL_ARTPEC6
+        bool "Axis ARTPEC-6 pin controller driver"
+        depends on MACH_ARTPEC6
+        select PINMUX
+        select GENERIC_PINCONF
+        help
+          This is the driver for the Axis ARTPEC-6 pin controller. This driver
+          supports pin function multiplexing as well as pin bias and drive
+          strength configuration. Device tree integration instructions can be
+          found in Documentation/devicetree/bindings/pinctrl/axis,artpec6-pinctrl.txt
+
 config PINCTRL_AS3722
 	tristate "Pinctrl and GPIO driver for ams AS3722 PMIC"
 	depends on MFD_AS3722 && GPIOLIB

drivers/pinctrl/Makefile +1 -0

index a251f439626f..5fc3c1ddd86f 100644
@@ -8,6 +8,7 @@ obj-$(CONFIG_PINCONF)		+= pinconf.o
 obj-$(CONFIG_OF)		+= devicetree.o
 obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
 obj-$(CONFIG_PINCTRL_ADI2)	+= pinctrl-adi2.o
+obj-$(CONFIG_PINCTRL_ARTPEC6)	+= pinctrl-artpec6.o
 obj-$(CONFIG_PINCTRL_AS3722)	+= pinctrl-as3722.o
 obj-$(CONFIG_PINCTRL_BF54x)	+= pinctrl-adi2-bf54x.o
 obj-$(CONFIG_PINCTRL_BF60x)	+= pinctrl-adi2-bf60x.o

drivers/pinctrl/pinctrl-artpec6.c new file +992 -0

992 lines not on this page · raw message

Signature · 6 lines

2.1.4


/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- [email protected]

Linus Walleij

2017-04-07 09:50 UTC #4 lore raw

Quoted 6 lines · Jesper Nilsson

Add pinctrl driver support for the Axis ARTPEC-6 SoC. There are only some pins that actually have different functions available, but all can control bias (pull-up/-down) and drive strength.
Code originally written by Chris Paterson.

Quoted 7 lines

Signed-off-by Jesper Nilsson <[email protected]>
---
Changes since v1:
- Get rid of the artpec-6 pinctrl header
- Replace the register arrays with a function that maps from pin number to the register offset.
- Cleanup duplicate information in artpec6_pin_groups array

Nice, patch applied for v4.12.

Yours,
Linus Walleij