diff mbox

[U-Boot,v2] Add single register pin controller driver

Message ID 1486474223-19444-1-git-send-email-fb@ltec.ch
State Superseded
Delegated to: Simon Glass
Headers show

Commit Message

Felix Brack Feb. 7, 2017, 1:30 p.m. UTC
This patch adds a pin controller driver supporting devices
using a single configuration register per pin.
Signed-off-by: Felix Brack <fb@ltec.ch>
---

Changes in v2:
- add a comment on function single_configure_pins() explaining
  the function and its parameters
- use fdt_getprop() to simplify retrieval of property
  'pinctrl-single,pins' from FDT
- change the return value of single_ofdata_to_platdata()
  to -EINVAL in case of failure
- change the driver name from pcs_pinctrl to single_pinctrl
- minor reformatting to comply with coding style

 drivers/pinctrl/Kconfig          |  10 +++
 drivers/pinctrl/Makefile         |   1 +
 drivers/pinctrl/pinctrl-single.c | 143 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-single.c

Comments

Masahiro Yamada Feb. 8, 2017, 3:02 a.m. UTC | #1
Hi.



2017-02-07 22:30 GMT+09:00 Felix Brack <fb@ltec.ch>:

> +
> +static int single_set_state_simple(struct udevice *dev,
> +                                  struct udevice *periph)
> +{
> +       const void *fdt = gd->fdt_blob;
> +       const struct single_fdt_pin_cfg *prop;
> +       int len;
> +
> +       prop = fdt_getprop(fdt, periph->of_offset, "pinctrl-single,pins", &len);


This seems wrong to me.


The "periph" is a peripheral device (like UART, eMMC, USB, etc.).

So, you are parsing the "pinctrl-single,pins" property
in the peripheral device, like

    uart: uart {

                  pinctrl-single,pins = <
                          .....
                  >;

    };




In pinctrl, peripheral nodes should have a phandle to a child of the
pinctrl device.
As you see Linux, the DT should look like this:


         uart: uart {

               pinctrl-0 = <&uart_pins>;

        };


    pinctrl {

                               uart_pins: uart_pins {
                                                  pinctrl-single,pins = <
                                                               ....
                                                  >;
                               };

         };
Felix Brack Feb. 8, 2017, 2:26 p.m. UTC | #2
Hello Masahiro,

This is my understanding of the parameters 'dev' and 'perihp' passed to
the function implementing set_state_simple().

On 08.02.2017 04:02, Masahiro Yamada wrote:
> Hi.
> 
> 
> 
> 2017-02-07 22:30 GMT+09:00 Felix Brack <fb@ltec.ch>:
> 
>> +
>> +static int single_set_state_simple(struct udevice *dev,
>> +                                  struct udevice *periph)
>> +{
>> +       const void *fdt = gd->fdt_blob;
>> +       const struct single_fdt_pin_cfg *prop;
>> +       int len;
>> +
>> +       prop = fdt_getprop(fdt, periph->of_offset, "pinctrl-single,pins", &len);
> 
> 
> This seems wrong to me.
> 
> 
> The "periph" is a peripheral device (like UART, eMMC, USB, etc.).
> 

In the case above 'dev' represents the pin controller node of the DT and
'periph' represents the DT node holding the pin configuration
information for a specific peripheral like i2c0, not the peripheral
itself. I know that neither 'dev' nor 'periph' _are_ device tree nodes
objects. This is why I use the word 'represent'.

> So, you are parsing the "pinctrl-single,pins" property
> in the peripheral device, like
> 
>     uart: uart {
> 
>                   pinctrl-single,pins = <
>                           .....
>                   >;
> 
>     };
> 
> 

I don't think so (see below).

> 
> 
> In pinctrl, peripheral nodes should have a phandle to a child of the
> pinctrl device.
> As you see Linux, the DT should look like this:
> 
> 
>          uart: uart {
> 
>                pinctrl-0 = <&uart_pins>;
> 
>         };
> 
> 
>     pinctrl {
> 
>                                uart_pins: uart_pins {
>                                                   pinctrl-single,pins = <
>                                                                ....
>                                                   >;
>                                };
> 
>          };
> 
> 

This is how it works. single_set_state_simple() would be called with
'dev' representing 'pinctrl' node and 'periph' representing 'uart_pins'
node. I would have to parse 'periph' for 'pinctrl-single,pins'.

> 
> 

I use this pin controller on a AM335x based board. Here are the relevant
parts from the device tree.

From 'am33xx.dtsi', representing the pin controller itself:

am33xx_pinmux: pinmux@800 {
	compatible = "pinctrl-single";
	reg = <0x800 0x238>;
	...
}

From a a device tree source file, representing the configuration of the
pins for the peripheral i2c0:

&am33xx_pinmux {
	pinctrl-names = "default";
	pinctrl-0 = <&i2c0_pins>;

	i2c0_pins: pinmux_i2c0_pins {
		pinctrl-single,pins = <
			AM33XX_IOPAD(0x988, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c0_sda.i2c0_sda */
			AM33XX_IOPAD(0x98c, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c0_scl.i2c0_scl */
		>;
	};
};

With this, single_set_state_simple() gets called with 'dev' representing
'pinmux@800' and 'periph' representing 'pinmux_i2c0_pins' which makes
perfect sense to me.
Again I would have to parse 'periph' for 'pinctrl-single,pins'.


regards Felix
Masahiro Yamada Feb. 8, 2017, 3:27 p.m. UTC | #3
Hi.


>> 2017-02-07 22:30 GMT+09:00 Felix Brack <fb@ltec.ch>:
>>
>>> +
>>> +static int single_set_state_simple(struct udevice *dev,
>>> +                                  struct udevice *periph)
>>> +{
>>> +       const void *fdt = gd->fdt_blob;
>>> +       const struct single_fdt_pin_cfg *prop;
>>> +       int len;
>>> +
>>> +       prop = fdt_getprop(fdt, periph->of_offset, "pinctrl-single,pins", &len);
>>
>>
>> This seems wrong to me.
>>
>>
>> The "periph" is a peripheral device (like UART, eMMC, USB, etc.).
>>
>
> In the case above 'dev' represents the pin controller node of the DT and
> 'periph' represents the DT node holding the pin configuration
> information for a specific peripheral like i2c0, not the peripheral
> itself.


I recommend you to read pinctrl-uclass.c once again.


Did you track how "periph" was passed across the following function calls?

device_probe()
  -> pinctrl_select_state()
      -> pinctrl_select_state_simple()
          -> ops->set_state_simple()
Felix Brack Feb. 9, 2017, 9:47 a.m. UTC | #4
Hello Masahiro,

On 08.02.2017 16:27, Masahiro Yamada wrote:
> Hi.
> 
> 
>>> 2017-02-07 22:30 GMT+09:00 Felix Brack <fb@ltec.ch>:
>>>
>>>> +
>>>> +static int single_set_state_simple(struct udevice *dev,
>>>> +                                  struct udevice *periph)
>>>> +{
>>>> +       const void *fdt = gd->fdt_blob;
>>>> +       const struct single_fdt_pin_cfg *prop;
>>>> +       int len;
>>>> +
>>>> +       prop = fdt_getprop(fdt, periph->of_offset, "pinctrl-single,pins", &len);
>>>
>>>
>>> This seems wrong to me.
>>>
>>>
>>> The "periph" is a peripheral device (like UART, eMMC, USB, etc.).
>>>
>>
>> In the case above 'dev' represents the pin controller node of the DT and
>> 'periph' represents the DT node holding the pin configuration
>> information for a specific peripheral like i2c0, not the peripheral
>> itself.
> 
> 
> I recommend you to read pinctrl-uclass.c once again.
>

I did.

> 
> Did you track how "periph" was passed across the following function calls?
> 

I did.

> device_probe()

From 'pinctrl-uclass.c::device_probe()':

---
/*
 * Process pinctrl for everything except the root device, and
 * continue regardless of the result of pinctrl. Don't process pinctrl
 * settings for pinctrl devices since the device may not yet be
 * probed.
 */
if (dev->parent && device_get_uclass_id(dev) != UCLASS_PINCTRL)
	pinctrl_select_state(dev, "default");
---

The comment says it all. This is where 'periph' originates.

>   -> pinctrl_select_state()
>       -> pinctrl_select_state_simple()
>           -> ops->set_state_simple()
> 
> 

Also the outcome reported by 'dm tree' makes sense to me:

 Class       Probed   Name
----------------------------------------
 root        [ + ]    root_driver
 simple_bus  [ + ]    `-- ocp
 simple_bus  [ + ]        |-- l4_wkup@44c00000
 simple_bus  [ + ]        |   `-- scm@210000
 pinctrl     [ + ]        |       `-- pinmux@800
 pinconfig   [ + ]        |           |-- pinmux_i2c0_pins
 pinconfig   [ + ]        |           |-- pinmux_i2c1_pins
 pinconfig   [   ]        |           |-- pinmux_i2c2_pins
 pinconfig   [ + ]        |           |-- pinmux_spi1_pins
 pinconfig   [   ]        |           |-- pinmux_uart0_pins
 pinconfig   [   ]        |           |-- pinmux_uart1_pins
 pinconfig   [ + ]        |           |-- pinmux_uart3_pins
 pinconfig   [ + ]        |           |-- pinmux_clkout2_pin
 pinconfig   [   ]        |           |-- cpsw_default
 pinconfig   [   ]        |           |-- davinci_mdio_default
 pinconfig   [ + ]        |           |-- pinmux_mmc1_pins
 pinconfig   [ + ]        |           |-- pinmux_mmc2_pins
 pinconfig   [   ]        |           |-- lcd_pins_s0
 pinconfig   [   ]        |           `-- pinmux_dcan0_pins
 serial      [   ]        |-- serial@44e09000
 serial      [   ]        |-- serial@48022000
 serial      [ + ]        |-- serial@481a6000
 i2c         [   ]        |-- i2c@44e0b000
 i2c         [ + ]        |-- i2c@4802a000
 i2c_generic [ + ]        |   |-- generic_60
 i2c_generic [ + ]        |   `-- generic_50
 i2c         [   ]        `-- i2c@4819c000

regards Felix
Masahiro Yamada Feb. 10, 2017, 4:35 a.m. UTC | #5
Hi Felix,

2017-02-09 18:47 GMT+09:00 Felix Brack <fb@ltec.ch>:
>
>> device_probe()
>
> From 'pinctrl-uclass.c::device_probe()':
>
> ---
> /*
>  * Process pinctrl for everything except the root device, and
>  * continue regardless of the result of pinctrl. Don't process pinctrl
>  * settings for pinctrl devices since the device may not yet be
>  * probed.
>  */
> if (dev->parent && device_get_uclass_id(dev) != UCLASS_PINCTRL)
>         pinctrl_select_state(dev, "default");
> ---
>
> The comment says it all. This is where 'periph' originates.


That's right.
Then, the "dev" in device_probe() represents
the peripheral, such as i2c.

Not a pin-config node, like i2c0_pins.
James Balean Feb. 18, 2017, 12:16 p.m. UTC | #6
Hi All,

2017-02-10 13:35 GMT+09:00 Masahiro Yamada <yamada.masahiro at socionext.com>:
> That's right.
> Then, the "dev" in device_probe() represents
> the peripheral, such as i2c.
> 
> Not a pin-config node, like i2c0_pins.

Masahiro, have a look at the call to set_state_simple from pinctrl_select_state_simple (in drivers/pinctrl/pinctrl-uclass.c). It is at this point that the pinctrl node becomes the 'dev' parameter, and the former 'dev' then becomes the second 'periph' parameter.

---
/*
 * For simplicity, assume the first device of PINCTRL uclass
 * is the correct one.  This is most likely OK as there is
 * usually only one pinctrl device on the system.
 */
ret = uclass_get_device(UCLASS_PINCTRL, 0, &pctldev);
if (ret)
	return ret;

ops = pinctrl_get_ops(pctldev);
if (!ops->set_state_simple) {
	dev_dbg(dev, "set_state_simple op missing\n");
	return -ENOSYS;
}

return ops->set_state_simple(pctldev, dev);
---

Reviewed-by: James Balean <james@balean.com.au>

--
Kind regards,
James Balean
diff mbox

Patch

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index efcb4c0..32bda65 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -175,6 +175,16 @@  config PIC32_PINCTRL
 	  by a device tree node which contains both GPIO defintion and pin control
 	  functions.
 
+config PINCTRL_SINGLE
+	bool "Single register pin-control and pin-multiplex driver"
+	depends on DM
+	help
+	  This enables pinctrl driver for systems using a single register for
+	  pin configuration and multiplexing. TI's AM335X SoCs are examples of
+	  such systems.
+	  Depending on the platform make sure to also enable OF_TRANSLATE and
+	  eventually SPL_OF_TRANSLATE to get correct address translations.
+
 endif
 
 source "drivers/pinctrl/meson/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 512112a..f148f94 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -16,3 +16,4 @@  obj-$(CONFIG_PIC32_PINCTRL)	+= pinctrl_pic32.o
 obj-$(CONFIG_PINCTRL_EXYNOS)	+= exynos/
 obj-$(CONFIG_PINCTRL_MESON)	+= meson/
 obj-$(CONFIG_PINCTRL_MVEBU)	+= mvebu/
+obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
new file mode 100644
index 0000000..e85a4d7
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -0,0 +1,143 @@ 
+/*
+ * Copyright (C) EETS GmbH, 2017, Felix Brack <f.brack@eets.ch>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm/device.h>
+#include <dm/pinctrl.h>
+#include <libfdt.h>
+#include <asm/io.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct single_pdata {
+	fdt_addr_t base;	/* first configuration register */
+	int offset;		/* index of last configuration register */
+	u32 mask;		/* configuration-value mask bits */
+	int width;		/* configuration register bit width */
+};
+
+struct single_fdt_pin_cfg {
+	fdt32_t reg;		/* configuration register offset */
+	fdt32_t val;		/* configuration register value */
+};
+
+/**
+ * single_configure_pins() - Configure pins based on FDT data
+ *
+ * @dev: Pointer to single pin configuration device which is the parent of
+ *       the pins node holding the pin configuration data.
+ * @pins: Pointer to the first element of an array of register/value pairs
+ *        of type 'struct single_fdt_pin_cfg'. Each such pair describes the
+ *        the pin to be configured and the value to be used for configuration.
+ *        This pointer points to a 'pinctrl-single,pins' property in the
+ *        device-tree.
+ * @size: Size of the 'pins' array in bytes.
+ *        The number of register/value pairs in the 'pins' array therefore
+ *        equals to 'size / sizeof(struct single_fdt_pin_cfg)'.
+ */
+static int single_configure_pins(struct udevice *dev,
+				 const struct single_fdt_pin_cfg *pins,
+				 int size)
+{
+	struct single_pdata *pdata = dev->platdata;
+	int count = size / sizeof(struct single_fdt_pin_cfg);
+	int n, reg;
+	u32 val;
+
+	for (n = 0; n < count; n++) {
+		reg = fdt32_to_cpu(pins->reg);
+		if ((reg < 0) || (reg > pdata->offset)) {
+			dev_dbg(dev, "  invalid register offset 0x%08x\n", reg);
+			pins++;
+			continue;
+		}
+		reg += pdata->base;
+		switch (pdata->width) {
+		case 32:
+			val = readl(reg) & ~pdata->mask;
+			val |= fdt32_to_cpu(pins->val) & pdata->mask;
+			writel(val, reg);
+			dev_dbg(dev, "  reg/val 0x%08x/0x%08x\n",
+				reg, val);
+			break;
+		default:
+			dev_warn(dev, "unsupported register width %i\n",
+				 pdata->width);
+		}
+		pins++;
+	}
+	return 0;
+}
+
+static int single_set_state_simple(struct udevice *dev,
+				   struct udevice *periph)
+{
+	const void *fdt = gd->fdt_blob;
+	const struct single_fdt_pin_cfg *prop;
+	int len;
+
+	prop = fdt_getprop(fdt, periph->of_offset, "pinctrl-single,pins", &len);
+	if (prop) {
+		dev_dbg(dev, "configuring pins for %s\n", periph->name);
+		if (len % sizeof(struct single_fdt_pin_cfg)) {
+			dev_dbg(dev, "  invalid pin configuration in fdt\n");
+			return -FDT_ERR_BADSTRUCTURE;
+		}
+		single_configure_pins(dev, prop, len);
+		len = 0;
+	}
+
+	return len;
+}
+
+static int single_ofdata_to_platdata(struct udevice *dev)
+{
+	fdt_addr_t addr;
+	u32 of_reg[2];
+	int res;
+	struct single_pdata *pdata = dev->platdata;
+
+	pdata->width = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
+				      "pinctrl-single,register-width", 0);
+
+	res = fdtdec_get_int_array(gd->fdt_blob, dev->of_offset,
+				   "reg", of_reg, 2);
+	if (res)
+		return res;
+	pdata->offset = of_reg[1] - pdata->width / 8;
+
+	addr = dev_get_addr(dev);
+	if (addr == FDT_ADDR_T_NONE) {
+		dev_dbg(dev, "no valid base register address\n");
+		return -EINVAL;
+	}
+	pdata->base = addr;
+
+	pdata->mask = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
+				     "pinctrl-single,function-mask",
+				     0xffffffff);
+	return 0;
+}
+
+const struct pinctrl_ops single_pinctrl_ops = {
+	.set_state_simple = single_set_state_simple,
+	.set_state = pinctrl_generic_set_state,
+};
+
+static const struct udevice_id single_pinctrl_match[] = {
+	{ .compatible = "pinctrl-single" },
+	{ /* sentinel */ }
+};
+
+U_BOOT_DRIVER(single_pinctrl) = {
+	.name = "single-pinctrl",
+	.id = UCLASS_PINCTRL,
+	.of_match = single_pinctrl_match,
+	.ops = &single_pinctrl_ops,
+	.flags = DM_FLAG_PRE_RELOC,
+	.platdata_auto_alloc_size = sizeof(struct single_pdata),
+	.ofdata_to_platdata = single_ofdata_to_platdata,
+};