[U-Boot] [PATCH v1] Add single register pin controller driver
This patch adds a pin controller driver supporting devices using a single configuration register per pin. Signed-off-by: Felix Brack fb@ltec.ch ---
drivers/pinctrl/Kconfig | 10 +++ drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-single.c | 138 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-single.c
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..f9e04f0 --- /dev/null +++ b/drivers/pinctrl/pinctrl-single.c @@ -0,0 +1,138 @@ +/* + * 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; + fdt32_t val; +}; + +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; + int offset; + const char *name; + + offset = fdt_first_property_offset(fdt, periph->of_offset); + if (offset < 0) + return offset; + prop = fdt_getprop_by_offset(fdt, offset, &name, &len); + if (!prop) + return len; + + if (!strcmp(name, "pinctrl-single,pins")) { + 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); + } + + return 0; +} + +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 -FDT_ERR_BADSTRUCTURE; + } + 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(pcs_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, +};
Hi Felix,
On 3 February 2017 at 05:29, Felix Brack fb@ltec.ch wrote:
This patch adds a pin controller driver supporting devices using a single configuration register per pin. Signed-off-by: Felix Brack fb@ltec.ch
drivers/pinctrl/Kconfig | 10 +++ drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-single.c | 138 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-single.c
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 DMhelpThis enables pinctrl driver for systems using a single register forpin configuration and multiplexing. TI's AM335X SoCs are examples ofsuch systems.Depending on the platform make sure to also enable OF_TRANSLATE andeventually 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..f9e04f0 --- /dev/null +++ b/drivers/pinctrl/pinctrl-single.c @@ -0,0 +1,138 @@ +/*
- 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;fdt32_t val;+};
+static int single_configure_pins(struct udevice *dev,
const struct single_fdt_pin_cfg *pins,int size)
Can you add a comment about this function? In particular, mention that pins is a pointer to a device-tree property.
+{
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: {
case should be at same level as switch
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;int offset;const char *name;offset = fdt_first_property_offset(fdt, periph->of_offset);if (offset < 0)return offset;prop = fdt_getprop_by_offset(fdt, offset, &name, &len);if (!prop)return len;if (!strcmp(name, "pinctrl-single,pins")) {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);}
This seems odd in that it looks at the first property and processes it if it is pinctrl-single,pins. Why not use fdt_getprop()?
return 0;+}
+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 -FDT_ERR_BADSTRUCTURE;
-EINVAL (we can't return FDT error numbers to the driver-model system)
}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(pcs_pinctrl) = {
Can you use a generic name here?
.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,+};
2.7.4
Regards, Simon
Hello Simon,
Thank you for the review! I will submit an improved version of the patch.
On 06.02.2017 16:34, Simon Glass wrote:
Hi Felix,
On 3 February 2017 at 05:29, Felix Brack fb@ltec.ch wrote:
This patch adds a pin controller driver supporting devices using a single configuration register per pin. Signed-off-by: Felix Brack fb@ltec.ch
drivers/pinctrl/Kconfig | 10 +++ drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-single.c | 138 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-single.c
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 DMhelpThis enables pinctrl driver for systems using a single register forpin configuration and multiplexing. TI's AM335X SoCs are examples ofsuch systems.Depending on the platform make sure to also enable OF_TRANSLATE andeventually 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..f9e04f0 --- /dev/null +++ b/drivers/pinctrl/pinctrl-single.c @@ -0,0 +1,138 @@ +/*
- 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;fdt32_t val;+};
+static int single_configure_pins(struct udevice *dev,
const struct single_fdt_pin_cfg *pins,int size)Can you add a comment about this function? In particular, mention that pins is a pointer to a device-tree property.
Yes, I will.
+{
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: {case should be at same level as switch
Right, my bad.
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;int offset;const char *name;offset = fdt_first_property_offset(fdt, periph->of_offset);if (offset < 0)return offset;prop = fdt_getprop_by_offset(fdt, offset, &name, &len);if (!prop)return len;if (!strcmp(name, "pinctrl-single,pins")) {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);}This seems odd in that it looks at the first property and processes it if it is pinctrl-single,pins. Why not use fdt_getprop()?
Agreed. Seems I tried to reinvent the wheel.
return 0;+}
+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 -FDT_ERR_BADSTRUCTURE;-EINVAL (we can't return FDT error numbers to the driver-model system)
Okay.
}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(pcs_pinctrl) = {
Can you use a generic name here?
This is relict. I will use 'single_pinctrl' instead.
.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,+};
2.7.4
Regards, Simon
Regards, Felix
participants (2)
-
Felix Brack -
Simon Glass