Message ID | 1486474223-19444-1-git-send-email-fb@ltec.ch |
---|---|
State | Superseded |
Delegated to: | Simon Glass |
Headers | show |
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 = < .... >; }; };
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
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()
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
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.
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 --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, +};
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