| Message ID | 1486128571-10391-1-git-send-email-fb@ltec.ch |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
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 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) 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 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) > > 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
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, +};
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