| Message ID | 20181107182733.62155-2-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pinctrl: Add RZ/A2 pin and gpio driver | expand |
Hi Chris, On Wed, Nov 7, 2018 at 7:27 PM Chris Brandt <chris.brandt@renesas.com> wrote: > Adds support for the pin and gpio controller found in R7S9210 (RZ/A2) SoCs. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Thanks for your patch! > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -195,6 +195,17 @@ config PINCTRL_RZA1 > help > This selects pinctrl driver for Renesas RZ/A1 platforms. > > +config PINCTRL_RZA2 > + bool "Renesas RZ/A2 gpio and pinctrl driver" > + depends on OF > + depends on ARCH_R7S9210 || COMPILE_TEST > + select GPIOLIB > + select GENERIC_PINCTRL_GROUPS > + select GENERIC_PINMUX_FUNCTIONS > + select GENERIC_PINCONF Given the PFS_ISEL bit, I think you can select GPIOLIB_IRQCHIP, and implement interrupt support on GPIOs. Of course that can be added later... > + help > + This selects pinctrl driver for Renesas RZ/A2 platforms. GPIO and pinctrl? > --- /dev/null > +++ b/drivers/pinctrl/pinctrl-rza2.c > @@ -0,0 +1,530 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Combined GPIO and pin controller support for Renesas RZ/A2 (R7S9210) SoC > + * > + * Copyright (C) 2018 Chris Brandt > + */ > + > +/* > + * This pin controller/gpio combined driver supports Renesas devices of RZ/A2 > + * family. > + */ > + > +#include <linux/bitops.h> > +#include <linux/gpio.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/pinctrl/pinmux.h> > + > +#include "core.h" > +#include "pinmux.h" > + > +#define DRIVER_NAME "pinctrl-rza2" > + > +#define RZA2_PINS_PER_PORT 8 > +#define RZA2_NPINS (RZA2_PINS_PER_PORT * RZA2_NPORTS) > +#define RZA2_PIN_ID_TO_PORT(id) ((id) / RZA2_PINS_PER_PORT) > +#define RZA2_PIN_ID_TO_PIN(id) ((id) % RZA2_PINS_PER_PORT) > + > +/* > + * Use 16 lower bits [15:0] for pin identifier > + * Use 16 higher bits [31:16] for pin mux function > + */ > +#define MUX_PIN_ID_MASK GENMASK(15, 0) > +#define MUX_FUNC_MASK GENMASK(31, 16) > +#define MUX_FUNC_OFFS 16 > +#define MUX_FUNC(pinconf) ((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS) > + > +enum pfc_pin_port_name {PORT0, PORT1, PORT2, PORT3, PORT4, PORT5, PORT6, PORT7, > + PORT8, PORT9, PORTA, PORTB, PORTC, PORTD, PORTE, PORTF, > + PORTG, PORTH, PORTJ, PORTK, PORTL, PORTM, RZA2_NPORTS}; The only reason for this enum is to fix the value of RZA2_NPORTS, right? > +#define RZA2_PDR_BASE(_b) ((_b) + 0x0000) /* 16-bit, 2 bytes apart */ > +#define RZA2_PODR_BASE(_b) ((_b) + 0x0040) /* 8-bit, 1 byte apart */ > +#define RZA2_PIDR_BASE(_b) ((_b) + 0x0060) /* 8-bit, 1 byte apart */ > +#define RZA2_PMR_BASE(_b) ((_b) + 0x0080) /* 8-bit, 1 byte apart */ > +#define RZA2_DSCR_BASE(_b) ((_b) + 0x0140) /* 16-bit, 2 bytes apart */ > +#define RZA2_PFS_BASE(_b) ((_b) + 0x0200) /* 8-bit, 8 bytes apart */ > +#define RZA2_PWPR(_b) ((_b) + 0x02FF) /* 8-bit */ > +#define RZA2_PFENET(_b) ((_b) + 0x0820) /* 8-bit */ > +#define RZA2_PPOC(_b) ((_b) + 0x0900) /* 32-bit */ > +#define RZA2_PHMOMO(_b) ((_b) + 0x0980) /* 32-bit */ > +#define RZA2_PCKIO(_b) ((_b) + 0x09D0) /* 8-bit */ > + > +#define RZA2_PDR(_b, _port) (RZA2_PDR_BASE(_b) + ((_port) * 2)) > +#define RZA2_PODR(_b, _port) (RZA2_PODR_BASE(_b) + (_port)) > +#define RZA2_PIDR(_b, _port) (RZA2_PIDR_BASE(_b) + (_port)) > +#define RZA2_PMR(_b, _port) (RZA2_PMR_BASE(_b) + (_port)) > +#define RZA2_DSCR(_b, _port) (RZA2_DSCR_BASE(_b) + ((_port) * 2)) > +#define RZA2_PFS(_b, _port, _pin) (RZA2_PFS_BASE(_b) + ((_port) * 8) \ > + + (_pin)) The above two sets of macros split information in two locations, and partially duplicates it. I think it becomes easier to read if you combine them, and factor out the addition of the base address. E.g. #define RZA2_PDR(port) (0x0000 + (port) * 2) /* Port Direction, 16-bit */ Then you can write: reg16 = readw(pfc_base + RZA2_PDR(port)); mask16 = RZA2_PDR_MASK << (pin * 2); reg16 &= ~mask16; writew(reg16, pfc_base + RZA2_PDR(port)); > +static void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin, > + u8 func) > +{ > + u8 reg8; > + u16 reg16; > + u16 mask16; > + > + /* Set pin to 'Non-use (Hi-z input protection)' */ > + reg16 = readw(RZA2_PDR(pfc_base, port)); > + mask16 = 0x03 << (pin * 2); mask16 = RZA2_PDR_MASK << (pin * 2); > + reg16 &= ~mask16; > + writew(reg16, RZA2_PDR(pfc_base, port)); > + > + /* Temporarily switch to GPIO */ > + reg8 = readb(RZA2_PMR(pfc_base, port)); > + reg8 &= ~BIT(pin); > + writeb(reg8, RZA2_PMR(pfc_base, port)); > + > + /* PFS Register Write Protect : OFF */ > + writeb(0x00, RZA2_PWPR(pfc_base)); /* B0WI=0, PFSWE=0 */ > + writeb(0x40, RZA2_PWPR(pfc_base)); /* B0WI=0, PFSWE=1 */ #define PWPR_B0WI BIT(7) /* Bit Write Disable */ #define PWPR_PFSWE BIT(6) /* PFS Register Write Enable */ > + > + /* Set Pin function (interrupt disabled, ISEL=0) */ > + writeb(func, RZA2_PFS(pfc_base, port, pin)); #define PFS_ISEL BIT(6) /* Interrupt Select */ > + > + /* PFS Register Write Protect : ON */ > + writeb(0x00, RZA2_PWPR(pfc_base)); /* B0WI=0, PFSWE=0 */ > + writeb(0x80, RZA2_PWPR(pfc_base)); /* B0WI=1, PFSWE=0 */ > + > + /* Port Mode : Peripheral module pin functions */ > + reg8 = readb(RZA2_PMR(pfc_base, port)); > + reg8 |= BIT(pin); > + writeb(reg8, RZA2_PMR(pfc_base, port)); > +} > + > +static void rza2_pin_to_gpio(void __iomem *pfc_base, u8 port, u8 pin, u8 dir) > +{ > + u16 reg16; > + u16 mask16; > + > + reg16 = readw(RZA2_PDR(pfc_base, port)); > + mask16 = RZA2_PDR_MASK << (pin * 2); > + reg16 &= ~mask16; > + > + if (dir == GPIOF_DIR_IN) > + reg16 |= RZA2_PDR_INPUT << (pin * 2); /* pin as input */ > + else > + reg16 |= RZA2_PDR_OUTPUT << (pin * 2); /* pin as output */ > + > + writew(reg16, RZA2_PDR(pfc_base, port)); > +} > + > +static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int offset) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; GPIO offset numbers are identical to PIN numbers, right? So you can use RZA2_PIN_ID_TO_PORT(), RZA2_PIN_ID_TO_PIN() for consistency. > + u16 reg16; > + > + reg16 = readw(RZA2_PDR(priv->base, port)); > + reg16 = (reg16 >> (pin * 2)) & RZA2_PDR_MASK; > + > + if (reg16 == RZA2_PDR_OUTPUT) > + return GPIOF_DIR_OUT; > + > + if (reg16 == RZA2_PDR_INPUT) > + return GPIOF_DIR_IN; > + > + /* > + * This GPIO controller has a default Hi-Z state that is not input or > + * output, so force the pin to input now. > + */ > + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN); > + > + return GPIOF_DIR_IN; > +} > + > +static int rza2_chip_direction_input(struct gpio_chip *chip, > + unsigned int offset) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + > + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN); Perhaps pass offset to rza2_pin_to_gpio() directly? > + > + return 0; > +} > + > +static int rza2_chip_direction_output(struct gpio_chip *chip, > + unsigned int offset, int val) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + > + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_OUT); > + > + return 0; > +} > + > +static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + > + return (readb(RZA2_PIDR(priv->base, port)) >> pin) & 1; Might be easier to read if you maintain symmetry with below: return !!(readb(RZA2_PIDR(priv->base, port) & BIT(pin)); > +} > + > +static void rza2_chip_set(struct gpio_chip *chip, unsigned int offset, > + int value) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + u8 new_value = readb(RZA2_PODR(priv->base, port)); > + > + if (value) > + new_value |= (1 << pin); new_value |= BIT(pin); > + else > + new_value &= ~(1 << pin); new_value &= ~BIT(pin); > + > + writeb(new_value, RZA2_PODR(priv->base, port)); > +} > + > +static const char * const rza2_gpio_names[] = { > + "P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7", > + "P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7", > + "P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7", > + "P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7", > + "P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7", > + "P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7", > + "P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7", > + "P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7", > + "P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7", > + "P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7", > + "PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7", > + "PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7", > + "PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7", > + "PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7", > + "PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7", > + "PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7", > + "PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7", > + "PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7", > + /* port I does not exist */ Hence shouldn't you have 8 NULL entries here? * @names: if set, must be an array of strings to use as alternative * names for the GPIOs in this chip. Any entry in the array * may be NULL if there is no alias for the GPIO, however the * array must be @ngpio entries long. A name can include a single printk * format specifier for an unsigned int. It is substituted by the actual * number of the gpio. > + "PJ_0", "PJ_1", "PJ_2", "PJ_3", "PJ_4", "PJ_5", "PJ_6", "PJ_7", > + "PK_0", "PK_1", "PK_2", "PK_3", "PK_4", "PK_5", "PK_6", "PK_7", > + "PL_0", "PL_1", "PL_2", "PL_3", "PL_4", "PL_5", "PL_6", "PL_7", > + "PM_0", "PM_1", "PM_2", "PM_3", "PM_4", "PM_5", "PM_6", "PM_7", > +}; > + > +static struct gpio_chip chip = { > + .names = rza2_gpio_names, BTW, is their much value in filling gpio_chip.names[]? I had never seen that before. > + .base = -1, > + .ngpio = RZA2_NPINS, > + .get_direction = rza2_chip_get_direction, > + .direction_input = rza2_chip_direction_input, > + .direction_output = rza2_chip_direction_output, > + .get = rza2_chip_get, > + .set = rza2_chip_set, You may want to implement .[gs]et_multiple(), too. > +}; > + > +struct pinctrl_gpio_range gpio_range; Please make this static. Or even better, store it in rza2_pinctrl_priv? > +static int rza2_pinctrl_register(struct rza2_pinctrl_priv *priv) > +{ > + struct pinctrl_pin_desc *pins; > + unsigned int i; > + int ret; > + > + pins = devm_kcalloc(priv->dev, RZA2_NPINS, sizeof(*pins), GFP_KERNEL); > + if (!pins) > + return -ENOMEM; > + > + priv->pins = pins; > + priv->desc.pins = pins; > + priv->desc.npins = RZA2_NPINS; > + > + for (i = 0; i < RZA2_NPINS; i++) { > + unsigned int pin = RZA2_PIN_ID_TO_PIN(i); > + unsigned int port = RZA2_PIN_ID_TO_PORT(i); > + > + pins[i].number = i; > + pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL, "P%c_%u", > + port_names[port], pin); These are identical to rza2_gpio_names[], so can't you use those? > +/* > + * For each DT node, create a single pin mapping. That pin mapping will only > + * contain a single group of pins, and that group of pins will only have a > + * single function that can be selected. > + */ > +static int rza2_dt_node_to_map(struct pinctrl_dev *pctldev, > + struct device_node *np, > + struct pinctrl_map **map, > + unsigned int *num_maps) > +{ > + struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev); > + struct property *of_pins; > + int i; > + unsigned int *pins; > + unsigned int *psel_val; > + const char **pin_fn; > + int ret, npins; > + int gsel, fsel; Some people prefer "reverse Xmas tree ordering" i.e. sorted by decreasing declaration length. [...] > + dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins); %pOF ... np > + > + /* Create map where to retrieve function and mux settings from */ > + *num_maps = 0; > + *map = kzalloc(sizeof(**map), GFP_KERNEL); > + if (!*map) { > + ret = -ENOMEM; > + goto remove_function; > + } > + > + (*map)->type = PIN_MAP_TYPE_MUX_GROUP; > + (*map)->data.mux.group = np->name; > + (*map)->data.mux.function = np->name; > + *num_maps = 1; > + > + return 0; > + > +remove_function: > + pinmux_generic_remove_function(pctldev, fsel); > + > +remove_group: > + pinctrl_generic_remove_group(pctldev, gsel); > + > + dev_info(priv->dev, "Unable to parse DT node %s\n", np->name); dev_err? %pOF ... np > + > + return ret; > +} > +static int rza2_set_mux(struct pinctrl_dev *pctldev, unsigned int selector, > + unsigned int group) > +{ > + struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev); > + struct function_desc *func; > + struct group_desc *grp; > + int i; > + unsigned int *psel_val; Reverse Xmas tree ordering? Gr{oetje,eeting}s, Geert
Hi Geert, As always, thank you for your review! On Monday, November 12, 2018, Geert Uytterhoeven wrote: > > +config PINCTRL_RZA2 > > + bool "Renesas RZ/A2 gpio and pinctrl driver" > > + depends on OF > > + depends on ARCH_R7S9210 || COMPILE_TEST > > + select GPIOLIB > > + select GENERIC_PINCTRL_GROUPS > > + select GENERIC_PINMUX_FUNCTIONS > > + select GENERIC_PINCONF > > Given the PFS_ISEL bit, I think you can select GPIOLIB_IRQCHIP, and > implement interrupt support on GPIOs. > Of course that can be added later... Yes, I was thinking that too. I have had user ask about that before, so maybe that will be a 2019 task. > > + help > > + This selects pinctrl driver for Renesas RZ/A2 platforms. > > GPIO and pinctrl? True. > > +enum pfc_pin_port_name {PORT0, PORT1, PORT2, PORT3, PORT4, PORT5, PORT6, > PORT7, > > + PORT8, PORT9, PORTA, PORTB, PORTC, PORTD, PORTE, > PORTF, > > + PORTG, PORTH, PORTJ, PORTK, PORTL, PORTM, > RZA2_NPORTS}; > > The only reason for this enum is to fix the value of RZA2_NPORTS, right? At the moment, yes. Originally I thought I would need these. But in the end, I did not. So I can just define RZA2_NPORTS for now. Maybe for future SoCs, That value will change depending on the SoC. > The above two sets of macros split information in two locations, and > partially > duplicates it. I think it becomes easier to read if you combine them, and > factor > out the addition of the base address. E.g. > > #define RZA2_PDR(port) (0x0000 + (port) * 2) /* Port > Direction, 16-bit */ > > Then you can write: > > reg16 = readw(pfc_base + RZA2_PDR(port)); > mask16 = RZA2_PDR_MASK << (pin * 2); > reg16 &= ~mask16; > writew(reg16, pfc_base + RZA2_PDR(port)); > > > +static void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 > pin, OK. I'm all for 'easier to read'! > > + mask16 = 0x03 << (pin * 2); > > mask16 = RZA2_PDR_MASK << (pin * 2); OK. > > + /* PFS Register Write Protect : OFF */ > > + writeb(0x00, RZA2_PWPR(pfc_base)); /* B0WI=0, PFSWE=0 */ > > + writeb(0x40, RZA2_PWPR(pfc_base)); /* B0WI=0, PFSWE=1 */ > > #define PWPR_B0WI BIT(7) /* Bit Write Disable */ > #define PWPR_PFSWE BIT(6) /* PFS Register Write Enable */ OK. > > + /* Set Pin function (interrupt disabled, ISEL=0) */ > > + writeb(func, RZA2_PFS(pfc_base, port, pin)); > > #define PFS_ISEL BIT(6) /* Interrupt Select */ OK. > > +static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int > offset) > > +{ > > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > > + u8 port = offset / 8; > > + u8 pin = offset % 8; > > GPIO offset numbers are identical to PIN numbers, right? > So you can use RZA2_PIN_ID_TO_PORT(), RZA2_PIN_ID_TO_PIN() > for consistency. Good point! > > +static int rza2_chip_direction_input(struct gpio_chip *chip, > > + unsigned int offset) > > +{ > > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > > + u8 port = offset / 8; > > + u8 pin = offset % 8; > > + > > + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN); > > Perhaps pass offset to rza2_pin_to_gpio() directly? OK. I can do that. Saves a couple of lines ;) > > +static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset) > > +{ > > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > > + u8 port = offset / 8; > > + u8 pin = offset % 8; > > + > > + return (readb(RZA2_PIDR(priv->base, port)) >> pin) & 1; > > Might be easier to read if you maintain symmetry with below: > > return !!(readb(RZA2_PIDR(priv->base, port) & BIT(pin)); Cool. > > + if (value) > > + new_value |= (1 << pin); > > new_value |= BIT(pin); > > > + else > > + new_value &= ~(1 << pin); > > new_value &= ~BIT(pin); I wondered about using BIT more often in the code. Thanks. > > +static const char * const rza2_gpio_names[] = { > > + "P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7", > > + "P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7", > > + "P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7", > > + "P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7", > > + "P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7", > > + "P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7", > > + "P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7", > > + "P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7", > > + "P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7", > > + "P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7", > > + "PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7", > > + "PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7", > > + "PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7", > > + "PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7", > > + "PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7", > > + "PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7", > > + "PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7", > > + "PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7", > > + /* port I does not exist */ > > Hence shouldn't you have 8 NULL entries here? But there is no such port named "I". And even in the dt-bindings and other documentation, the global pin ID is based off and a world where "I" is not in the alphabet. So if I put 8 NULLs here, wouldn't that screw up all the indexing?? > > +static struct gpio_chip chip = { > > + .names = rza2_gpio_names, > > BTW, is their much value in filling gpio_chip.names[]? > I had never seen that before. It makes the files show up under /sys look nice. For example, P5_6 is button SW4: $ echo 912 > /sys/class/gpio/export Then you end up with "/sys/class/gpio/P5_6/" $ echo in > /sys/class/gpio/P5_6/direction $ cat /sys/class/gpio/P5_6/direction $ cat /sys/class/gpio/P5_6/value > > + .get = rza2_chip_get, > > + .set = rza2_chip_set, > > You may want to implement .[gs]et_multiple(), too. OK, I will have a look. > > +struct pinctrl_gpio_range gpio_range; > > Please make this static. > Or even better, store it in rza2_pinctrl_priv? OK. > > + for (i = 0; i < RZA2_NPINS; i++) { > > + unsigned int pin = RZA2_PIN_ID_TO_PIN(i); > > + unsigned int port = RZA2_PIN_ID_TO_PORT(i); > > + > > + pins[i].number = i; > > + pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL, > "P%c_%u", > > + port_names[port], pin); > > These are identical to rza2_gpio_names[], so can't you use those? Good point! I'll try that. > > +{ > > + struct rza2_pinctrl_priv *priv = > pinctrl_dev_get_drvdata(pctldev); > > + struct property *of_pins; > > + int i; > > + unsigned int *pins; > > + unsigned int *psel_val; > > + const char **pin_fn; > > + int ret, npins; > > + int gsel, fsel; > > Some people prefer "reverse Xmas tree ordering" i.e. sorted by decreasing > declaration length. https://lwn.net/Articles/758552/ "only a few maintainers insist on that, while most really do not care or think that it's actively silly." So are you one of those maintainers????? :) > > + dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins); > > %pOF ... np OK. > > +remove_group: > > + pinctrl_generic_remove_group(pctldev, gsel); > > + > > + dev_info(priv->dev, "Unable to parse DT node %s\n", np->name); > > dev_err? > > %pOF ... np Thanks. Chris
Hi Chris, On Tue, Nov 13, 2018 at 7:43 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Monday, November 12, 2018, Geert Uytterhoeven wrote: > > > +static const char * const rza2_gpio_names[] = { > > > + "P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7", > > > + "P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7", > > > + "P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7", > > > + "P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7", > > > + "P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7", > > > + "P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7", > > > + "P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7", > > > + "P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7", > > > + "P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7", > > > + "P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7", > > > + "PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7", > > > + "PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7", > > > + "PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7", > > > + "PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7", > > > + "PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7", > > > + "PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7", > > > + "PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7", > > > + "PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7", > > > + /* port I does not exist */ > > > > Hence shouldn't you have 8 NULL entries here? > > But there is no such port named "I". And even in the dt-bindings and other > documentation, the global pin ID is based off and a world where "I" is not > in the alphabet. So if I put 8 NULLs here, wouldn't that screw up all the > indexing?? I'd swear there's an "I" in port_names[], but upon checking again, it must have been my eyes that mislead me. Sorry for that. Hence please forget my comment above. > > > +static struct gpio_chip chip = { > > > + .names = rza2_gpio_names, > > > > BTW, is their much value in filling gpio_chip.names[]? > > I had never seen that before. > > It makes the files show up under /sys look nice. > > For example, P5_6 is button SW4: > > $ echo 912 > /sys/class/gpio/export > > Then you end up with "/sys/class/gpio/P5_6/" > > $ echo in > /sys/class/gpio/P5_6/direction > $ cat /sys/class/gpio/P5_6/direction > $ cat /sys/class/gpio/P5_6/value (Ah, the legacy and deprecated sysfs GPIO interface, being replaced by /dev/gpiochip[0-9]+ and https://github.com/brgl/libgpiod) Cool, I didn't know that. But you still need to know which number to write to the export file in the first place? > > > + .get = rza2_chip_get, > > > + .set = rza2_chip_set, > > > > You may want to implement .[gs]et_multiple(), too. > > OK, I will have a look. You can add that later. It doesn't add functionality, but may improve performance for bitbanging multiple pins. > > > +{ > > > + struct rza2_pinctrl_priv *priv = > > pinctrl_dev_get_drvdata(pctldev); > > > + struct property *of_pins; > > > + int i; > > > + unsigned int *pins; > > > + unsigned int *psel_val; > > > + const char **pin_fn; > > > + int ret, npins; > > > + int gsel, fsel; > > > > Some people prefer "reverse Xmas tree ordering" i.e. sorted by decreasing > > declaration length. > > https://lwn.net/Articles/758552/ > > "only a few maintainers insist on that, while most really do not care or > think that it's actively silly." > > So are you one of those maintainers????? :) Sorry, got baptised by Laurent... Gr{oetje,eeting}s, Geert
Hi Geert, On Tuesday, November 13, 2018, Geert Uytterhoeven wrote: > > It makes the files show up under /sys look nice. > > > > For example, P5_6 is button SW4: > > > > $ echo 912 > /sys/class/gpio/export > > > > Then you end up with "/sys/class/gpio/P5_6/" > > > > $ echo in > /sys/class/gpio/P5_6/direction > > $ cat /sys/class/gpio/P5_6/direction > > $ cat /sys/class/gpio/P5_6/value > > (Ah, the legacy and deprecated sysfs GPIO interface, being replaced > by /dev/gpiochip[0-9]+ and https://github.com/brgl/libgpiod) > > Cool, I didn't know that. > But you still need to know which number to write to the export file > in the first place? True, meaning the table does not help you as much as you want. Jacopo also mentioned the new libgpiod. So, I think I might just drop this table in the next revision. What I really want to do is just say "make P5_6 an input" and not have to convert to a global ID number. But, I'm not sure how libgpiod is going to know what "P5_6" is. > > > Some people prefer "reverse Xmas tree ordering" i.e. sorted by > decreasing > > > declaration length. > > > > https://lwn.net/Articles/758552/ > > > > "only a few maintainers insist on that, while most really do not care or > > think that it's actively silly." > > > > So are you one of those maintainers????? :) > > Sorry, got baptised by Laurent... (insert picture of Laurent handing out Kool-Aid here) Chris
Hi Chris, a few more notes on top of what Geert said. Thanks for addressing comments on the previous version. On Wed, Nov 07, 2018 at 01:27:32PM -0500, Chris Brandt wrote: > Adds support for the pin and gpio controller found in R7S9210 (RZ/A2) SoCs. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > v3: > - Changed names from Px to PORTx because "PC" is already defined > v2: > - fixed SOC part number in comments > - sorted #includes > - removed spaces in pfc_pin_port_name enum > - put RZA2_NPORTS at the end of pfc_pin_port_name enum > - added RZA2_ to the beginning of all #define macros > - put ( ) around all passed arguments in #define macros > - made helper macros to get register address easier > - use defines for pin direction bit settings > --- > drivers/pinctrl/Kconfig | 11 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/pinctrl-rza2.c | 530 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 542 insertions(+) > create mode 100644 drivers/pinctrl/pinctrl-rza2.c > > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index 4d8c00eac742..3e4d890d4bd9 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -195,6 +195,17 @@ config PINCTRL_RZA1 > help > This selects pinctrl driver for Renesas RZ/A1 platforms. > > +config PINCTRL_RZA2 > + bool "Renesas RZ/A2 gpio and pinctrl driver" > + depends on OF > + depends on ARCH_R7S9210 || COMPILE_TEST > + select GPIOLIB > + select GENERIC_PINCTRL_GROUPS > + select GENERIC_PINMUX_FUNCTIONS > + select GENERIC_PINCONF > + help > + This selects pinctrl driver for Renesas RZ/A2 platforms. > + > config PINCTRL_RZN1 > bool "Renesas RZ/N1 pinctrl driver" > depends on OF > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile > index 18a13c1e2c21..712184b74a5c 100644 > --- a/drivers/pinctrl/Makefile > +++ b/drivers/pinctrl/Makefile > @@ -27,6 +27,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o > obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o > obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o > obj-$(CONFIG_PINCTRL_RZA1) += pinctrl-rza1.o > +obj-$(CONFIG_PINCTRL_RZA2) += pinctrl-rza2.o > obj-$(CONFIG_PINCTRL_RZN1) += pinctrl-rzn1.o > obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o > obj-$(CONFIG_PINCTRL_SIRF) += sirf/ > diff --git a/drivers/pinctrl/pinctrl-rza2.c b/drivers/pinctrl/pinctrl-rza2.c > new file mode 100644 > index 000000000000..3781c3f693e8 > --- /dev/null > +++ b/drivers/pinctrl/pinctrl-rza2.c > @@ -0,0 +1,530 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Combined GPIO and pin controller support for Renesas RZ/A2 (R7S9210) SoC > + * > + * Copyright (C) 2018 Chris Brandt > + */ > + > +/* > + * This pin controller/gpio combined driver supports Renesas devices of RZ/A2 > + * family. > + */ > + > +#include <linux/bitops.h> > +#include <linux/gpio.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/pinctrl/pinmux.h> > + > +#include "core.h" > +#include "pinmux.h" > + > +#define DRIVER_NAME "pinctrl-rza2" > + > +#define RZA2_PINS_PER_PORT 8 > +#define RZA2_NPINS (RZA2_PINS_PER_PORT * RZA2_NPORTS) > +#define RZA2_PIN_ID_TO_PORT(id) ((id) / RZA2_PINS_PER_PORT) > +#define RZA2_PIN_ID_TO_PIN(id) ((id) % RZA2_PINS_PER_PORT) > + > +/* > + * Use 16 lower bits [15:0] for pin identifier > + * Use 16 higher bits [31:16] for pin mux function > + */ > +#define MUX_PIN_ID_MASK GENMASK(15, 0) > +#define MUX_FUNC_MASK GENMASK(31, 16) > +#define MUX_FUNC_OFFS 16 > +#define MUX_FUNC(pinconf) ((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS) > + > +enum pfc_pin_port_name {PORT0, PORT1, PORT2, PORT3, PORT4, PORT5, PORT6, PORT7, > + PORT8, PORT9, PORTA, PORTB, PORTC, PORTD, PORTE, PORTF, > + PORTG, PORTH, PORTJ, PORTK, PORTL, PORTM, RZA2_NPORTS}; > +static const char port_names[] = "0123456789ABCDEFGHJKLM"; > + > +struct rza2_pinctrl_priv { > + struct device *dev; > + void __iomem *base; > + > + struct pinctrl_pin_desc *pins; > + struct pinctrl_desc desc; > + struct pinctrl_dev *pctl; > +}; > + > +#define RZA2_PDR_BASE(_b) ((_b) + 0x0000) /* 16-bit, 2 bytes apart */ > +#define RZA2_PODR_BASE(_b) ((_b) + 0x0040) /* 8-bit, 1 byte apart */ > +#define RZA2_PIDR_BASE(_b) ((_b) + 0x0060) /* 8-bit, 1 byte apart */ > +#define RZA2_PMR_BASE(_b) ((_b) + 0x0080) /* 8-bit, 1 byte apart */ > +#define RZA2_DSCR_BASE(_b) ((_b) + 0x0140) /* 16-bit, 2 bytes apart */ > +#define RZA2_PFS_BASE(_b) ((_b) + 0x0200) /* 8-bit, 8 bytes apart */ > +#define RZA2_PWPR(_b) ((_b) + 0x02FF) /* 8-bit */ 0x2ff > +#define RZA2_PFENET(_b) ((_b) + 0x0820) /* 8-bit */ > +#define RZA2_PPOC(_b) ((_b) + 0x0900) /* 32-bit */ > +#define RZA2_PHMOMO(_b) ((_b) + 0x0980) /* 32-bit */ > +#define RZA2_PCKIO(_b) ((_b) + 0x09D0) /* 8-bit */ > + 0x09d0 > +#define RZA2_PDR(_b, _port) (RZA2_PDR_BASE(_b) + ((_port) * 2)) > +#define RZA2_PODR(_b, _port) (RZA2_PODR_BASE(_b) + (_port)) > +#define RZA2_PIDR(_b, _port) (RZA2_PIDR_BASE(_b) + (_port)) > +#define RZA2_PMR(_b, _port) (RZA2_PMR_BASE(_b) + (_port)) > +#define RZA2_DSCR(_b, _port) (RZA2_DSCR_BASE(_b) + ((_port) * 2)) > +#define RZA2_PFS(_b, _port, _pin) (RZA2_PFS_BASE(_b) + ((_port) * 8) \ > + + (_pin)) > +#define RZA2_PDR_HIGHZ 0x00 This is not used (I know I've suggested that, sorry :) > +#define RZA2_PDR_INPUT 0x02 > +#define RZA2_PDR_OUTPUT 0x03 > +#define RZA2_PDR_MASK 0x03 > + > +static void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin, > + u8 func) > +{ > + u8 reg8; > + u16 reg16; > + u16 mask16; > + I would prefer the reverse xmas order too, but I'm not saying out loud as I understand is something hard to enforce, as it's a very minor issue :) > + /* Set pin to 'Non-use (Hi-z input protection)' */ > + reg16 = readw(RZA2_PDR(pfc_base, port)); > + mask16 = 0x03 << (pin * 2); > + reg16 &= ~mask16; > + writew(reg16, RZA2_PDR(pfc_base, port)); > + > + /* Temporarily switch to GPIO */ > + reg8 = readb(RZA2_PMR(pfc_base, port)); > + reg8 &= ~BIT(pin); > + writeb(reg8, RZA2_PMR(pfc_base, port)); > + > + /* PFS Register Write Protect : OFF */ > + writeb(0x00, RZA2_PWPR(pfc_base)); /* B0WI=0, PFSWE=0 */ > + writeb(0x40, RZA2_PWPR(pfc_base)); /* B0WI=0, PFSWE=1 */ > + > + /* Set Pin function (interrupt disabled, ISEL=0) */ > + writeb(func, RZA2_PFS(pfc_base, port, pin)); > + > + /* PFS Register Write Protect : ON */ > + writeb(0x00, RZA2_PWPR(pfc_base)); /* B0WI=0, PFSWE=0 */ > + writeb(0x80, RZA2_PWPR(pfc_base)); /* B0WI=1, PFSWE=0 */ > + > + /* Port Mode : Peripheral module pin functions */ > + reg8 = readb(RZA2_PMR(pfc_base, port)); > + reg8 |= BIT(pin); > + writeb(reg8, RZA2_PMR(pfc_base, port)); > +} > + > +static void rza2_pin_to_gpio(void __iomem *pfc_base, u8 port, u8 pin, u8 dir) > +{ > + u16 reg16; > + u16 mask16; > + > + reg16 = readw(RZA2_PDR(pfc_base, port)); > + mask16 = RZA2_PDR_MASK << (pin * 2); > + reg16 &= ~mask16; > + > + if (dir == GPIOF_DIR_IN) > + reg16 |= RZA2_PDR_INPUT << (pin * 2); /* pin as input */ > + else > + reg16 |= RZA2_PDR_OUTPUT << (pin * 2); /* pin as output */ > + > + writew(reg16, RZA2_PDR(pfc_base, port)); > +} > + > +static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int offset) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + u16 reg16; > + > + reg16 = readw(RZA2_PDR(priv->base, port)); > + reg16 = (reg16 >> (pin * 2)) & RZA2_PDR_MASK; > + > + if (reg16 == RZA2_PDR_OUTPUT) > + return GPIOF_DIR_OUT; > + > + if (reg16 == RZA2_PDR_INPUT) > + return GPIOF_DIR_IN; > + > + /* > + * This GPIO controller has a default Hi-Z state that is not input or > + * output, so force the pin to input now. > + */ > + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN); > + > + return GPIOF_DIR_IN; > +} > + > +static int rza2_chip_direction_input(struct gpio_chip *chip, > + unsigned int offset) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + > + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN); > + > + return 0; > +} > + > +static int rza2_chip_direction_output(struct gpio_chip *chip, > + unsigned int offset, int val) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + > + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_OUT); > + > + return 0; > +} > + > +static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + > + return (readb(RZA2_PIDR(priv->base, port)) >> pin) & 1; > +} > + > +static void rza2_chip_set(struct gpio_chip *chip, unsigned int offset, > + int value) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + u8 new_value = readb(RZA2_PODR(priv->base, port)); > + > + if (value) > + new_value |= (1 << pin); > + else > + new_value &= ~(1 << pin); As Geert suggested, use BIT() ? > + > + writeb(new_value, RZA2_PODR(priv->base, port)); > +} > + > +static const char * const rza2_gpio_names[] = { > + "P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7", > + "P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7", > + "P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7", > + "P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7", > + "P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7", > + "P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7", > + "P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7", > + "P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7", > + "P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7", > + "P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7", > + "PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7", > + "PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7", > + "PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7", > + "PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7", > + "PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7", > + "PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7", > + "PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7", > + "PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7", > + /* port I does not exist */ > + "PJ_0", "PJ_1", "PJ_2", "PJ_3", "PJ_4", "PJ_5", "PJ_6", "PJ_7", > + "PK_0", "PK_1", "PK_2", "PK_3", "PK_4", "PK_5", "PK_6", "PK_7", > + "PL_0", "PL_1", "PL_2", "PL_3", "PL_4", "PL_5", "PL_6", "PL_7", > + "PM_0", "PM_1", "PM_2", "PM_3", "PM_4", "PM_5", "PM_6", "PM_7", > +}; > + > +static struct gpio_chip chip = { > + .names = rza2_gpio_names, > + .base = -1, > + .ngpio = RZA2_NPINS, > + .get_direction = rza2_chip_get_direction, > + .direction_input = rza2_chip_direction_input, > + .direction_output = rza2_chip_direction_output, > + .get = rza2_chip_get, > + .set = rza2_chip_set, > +}; > + > +struct pinctrl_gpio_range gpio_range; > + > +static int rza2_gpio_register(struct rza2_pinctrl_priv *priv) > +{ > + struct device_node *np = priv->dev->of_node; > + struct of_phandle_args of_args; > + int ret; > + > + if (!of_property_read_bool(np, "gpio-controller")) { > + dev_info(priv->dev, "No gpio controller registered\n"); > + return 0; > + } The bindings say this is mandatory... What do you think, would that make sense to have no gpio-controller support for this driver (testing apart :) > + > + chip.label = devm_kasprintf(priv->dev, GFP_KERNEL, "%pOFn", np); > + chip.of_node = np; > + chip.parent = priv->dev; > + > + ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, > + &of_args); > + if (ret) { > + dev_err(priv->dev, "Unable to parse gpio-ranges\n"); > + return ret; > + } > + > + gpio_range.id = of_args.args[0]; Do you think of_args need to be validated? As I understand them, id and pin_base are fixed at 0, while the pin number depends on the soc version/revision. Now I wonder if you would need a gpio-ranges property at all, but that's fine, it doesn't hurt... > + gpio_range.name = chip.label; > + gpio_range.pin_base = gpio_range.base = of_args.args[1]; > + gpio_range.npins = of_args.args[2]; > + gpio_range.gc = &chip; > + > + /* Register our gpio chip with gpiolib */ > + ret = devm_gpiochip_add_data(priv->dev, &chip, priv); > + if (ret) > + return ret; > + > + /* Register pin range with pinctrl core */ > + pinctrl_add_gpio_range(priv->pctl, &gpio_range); > + > + dev_dbg(priv->dev, "Registered gpio controller\n"); > + > + return 0; > +} > + > +static int rza2_pinctrl_register(struct rza2_pinctrl_priv *priv) > +{ > + struct pinctrl_pin_desc *pins; > + unsigned int i; > + int ret; > + > + pins = devm_kcalloc(priv->dev, RZA2_NPINS, sizeof(*pins), GFP_KERNEL); > + if (!pins) > + return -ENOMEM; > + > + priv->pins = pins; > + priv->desc.pins = pins; > + priv->desc.npins = RZA2_NPINS; > + > + for (i = 0; i < RZA2_NPINS; i++) { > + unsigned int pin = RZA2_PIN_ID_TO_PIN(i); > + unsigned int port = RZA2_PIN_ID_TO_PORT(i); > + > + pins[i].number = i; > + pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL, "P%c_%u", > + port_names[port], pin); > + } > + > + ret = devm_pinctrl_register_and_init(priv->dev, &priv->desc, priv, > + &priv->pctl); > + if (ret) { > + dev_err(priv->dev, "pinctrl registration failed\n"); > + return ret; > + } > + > + ret = pinctrl_enable(priv->pctl); > + if (ret) { > + dev_err(priv->dev, "pinctrl enable failed\n"); > + return ret; > + } > + > + ret = rza2_gpio_register(priv); > + if (ret) { > + dev_err(priv->dev, "GPIO registration failed\n"); > + return ret; > + } > + > + return 0; > +} > + > +/* > + * For each DT node, create a single pin mapping. That pin mapping will only > + * contain a single group of pins, and that group of pins will only have a > + * single function that can be selected. > + */ > +static int rza2_dt_node_to_map(struct pinctrl_dev *pctldev, > + struct device_node *np, > + struct pinctrl_map **map, > + unsigned int *num_maps) > +{ > + struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev); > + struct property *of_pins; > + int i; > + unsigned int *pins; > + unsigned int *psel_val; > + const char **pin_fn; > + int ret, npins; > + int gsel, fsel; > + > + /* Find out how many pins to map */ > + of_pins = of_find_property(np, "pinmux", NULL); > + if (!of_pins) { > + dev_info(priv->dev, "Missing pinmux property\n"); > + return -ENOENT; > + } > + npins = of_pins->length / sizeof(u32); > + > + pins = devm_kcalloc(priv->dev, npins, sizeof(*pins), GFP_KERNEL); > + psel_val = devm_kcalloc(priv->dev, npins, sizeof(*psel_val), > + GFP_KERNEL); > + pin_fn = devm_kzalloc(priv->dev, sizeof(*pin_fn), GFP_KERNEL); > + if (!pins || !psel_val || !pin_fn) > + return -ENOMEM; > + > + /* Collect pin locations and mux settings from DT properties */ > + for (i = 0; i < npins; ++i) { > + u32 value; > + > + ret = of_property_read_u32_index(np, "pinmux", i, &value); > + if (ret) > + return ret; > + pins[i] = value & MUX_PIN_ID_MASK; > + psel_val[i] = MUX_FUNC(value); > + } > + > + /* Register a single pin group listing all the pins we read from DT */ > + gsel = pinctrl_generic_add_group(pctldev, np->name, pins, npins, NULL); > + if (gsel < 0) > + return gsel; > + > + /* > + * Register a single group function where the 'data' is an array PSEL > + * register values read from DT. > + */ > + pin_fn[0] = np->name; > + fsel = pinmux_generic_add_function(pctldev, np->name, pin_fn, 1, > + psel_val); > + if (fsel < 0) { > + ret = fsel; > + goto remove_group; > + } > + > + dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins); > + > + /* Create map where to retrieve function and mux settings from */ > + *num_maps = 0; > + *map = kzalloc(sizeof(**map), GFP_KERNEL); > + if (!*map) { > + ret = -ENOMEM; > + goto remove_function; > + } > + > + (*map)->type = PIN_MAP_TYPE_MUX_GROUP; > + (*map)->data.mux.group = np->name; > + (*map)->data.mux.function = np->name; > + *num_maps = 1; > + > + return 0; > + > +remove_function: > + pinmux_generic_remove_function(pctldev, fsel); > + > +remove_group: > + pinctrl_generic_remove_group(pctldev, gsel); > + > + dev_info(priv->dev, "Unable to parse DT node %s\n", np->name); > + > + return ret; > +} > + > +static void rza2_dt_free_map(struct pinctrl_dev *pctldev, > + struct pinctrl_map *map, unsigned int num_maps) > +{ > + kfree(map); > +} > + > +static const struct pinctrl_ops rza2_pinctrl_ops = { > + .get_groups_count = pinctrl_generic_get_group_count, > + .get_group_name = pinctrl_generic_get_group_name, > + .get_group_pins = pinctrl_generic_get_group_pins, > + .dt_node_to_map = rza2_dt_node_to_map, > + .dt_free_map = rza2_dt_free_map, > +}; > + > +static int rza2_set_mux(struct pinctrl_dev *pctldev, unsigned int selector, > + unsigned int group) > +{ > + struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev); > + struct function_desc *func; > + struct group_desc *grp; > + int i; > + unsigned int *psel_val; > + > + grp = pinctrl_generic_get_group(pctldev, group); > + if (!grp) > + return -EINVAL; > + > + func = pinmux_generic_get_function(pctldev, selector); > + if (!func) > + return -EINVAL; > + > + psel_val = func->data; > + > + for (i = 0; i < grp->num_pins; ++i) { > + dev_dbg(priv->dev, "Setting P%c_%d to PSEL=%d\n", > + port_names[RZA2_PIN_ID_TO_PORT(grp->pins[i])], > + RZA2_PIN_ID_TO_PIN(grp->pins[i]), > + psel_val[i]); > + rza2_set_pin_function( > + priv->base, > + RZA2_PIN_ID_TO_PORT(grp->pins[i]), > + RZA2_PIN_ID_TO_PIN(grp->pins[i]), > + psel_val[i]); > + } > + > + return 0; > +} > + > +static const struct pinmux_ops rza2_pinmux_ops = { > + .get_functions_count = pinmux_generic_get_function_count, > + .get_function_name = pinmux_generic_get_function_name, > + .get_function_groups = pinmux_generic_get_function_groups, > + .set_mux = rza2_set_mux, > + .strict = true, > +}; > + > +static int rza2_pinctrl_probe(struct platform_device *pdev) > +{ > + struct rza2_pinctrl_priv *priv; > + struct resource *res; > + int ret; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv->base)) > + return PTR_ERR(priv->base); > + > + platform_set_drvdata(pdev, priv); > + > + priv->desc.name = DRIVER_NAME; > + priv->desc.pctlops = &rza2_pinctrl_ops; > + priv->desc.pmxops = &rza2_pinmux_ops; > + priv->desc.owner = THIS_MODULE; > + > + ret = rza2_pinctrl_register(priv); > + if (ret) > + return ret; > + > + pr_info("RZ/A2 pin controller registered\n"); nit: dev_info > + > + return 0; > +} > + > +static const struct of_device_id rza2_pinctrl_of_match[] = { > + { > + .compatible = "renesas,r7s9210-pinctrl", > + }, > + { /* sentinel */ } > +}; > + > +static struct platform_driver rza2_pinctrl_driver = { > + .driver = { > + .name = DRIVER_NAME, > + .of_match_table = rza2_pinctrl_of_match, > + }, > + .probe = rza2_pinctrl_probe, > +}; > + > +static int __init rza2_pinctrl_init(void) > +{ > + return platform_driver_register(&rza2_pinctrl_driver); > +} > +core_initcall(rza2_pinctrl_init); > + > + nit: multiple empty lines With this minor fixed, please add my Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > +MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>"); > +MODULE_DESCRIPTION("Pin and gpio controller driver for RZ/A2 SoC"); > +MODULE_LICENSE("GPL v2"); > -- > 2.16.1 >
Hi Chris, On Tue, Nov 13, 2018 at 8:26 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Tuesday, November 13, 2018, Geert Uytterhoeven wrote: > > > It makes the files show up under /sys look nice. > > > > > > For example, P5_6 is button SW4: > > > > > > $ echo 912 > /sys/class/gpio/export > > > > > > Then you end up with "/sys/class/gpio/P5_6/" > > > > > > $ echo in > /sys/class/gpio/P5_6/direction > > > $ cat /sys/class/gpio/P5_6/direction > > > $ cat /sys/class/gpio/P5_6/value > > > > (Ah, the legacy and deprecated sysfs GPIO interface, being replaced > > by /dev/gpiochip[0-9]+ and https://github.com/brgl/libgpiod) > > > > Cool, I didn't know that. > > But you still need to know which number to write to the export file > > in the first place? > > True, meaning the table does not help you as much as you want. > Jacopo also mentioned the new libgpiod. > So, I think I might just drop this table in the next revision. > > What I really want to do is just say "make P5_6 an input" and > not have to convert to a global ID number. But, I'm not sure how > libgpiod is going to know what "P5_6" is. There are two parts: 1. New kernel /dev/gpiochip[0-9]+ interface Sample code comes with the kernel under tools/gpio/. E.g. root@koelsch:~# lsgpio -n gpiochip7 GPIO chip: gpiochip7, "e6055800.gpio", 26 GPIO lines line 0: unnamed "SW30" [kernel active-low] line 1: unnamed "SW31" [kernel active-low] line 2: unnamed "SW32" [kernel active-low] line 3: unnamed "SW33" [kernel active-low] line 4: unnamed "SW34" [kernel active-low] line 5: unnamed "SW35" [kernel active-low] line 6: unnamed "SW36" [kernel active-low] line 7: unnamed unused line 8: unnamed unused line 9: unnamed unused line 10: unnamed unused line 11: unnamed unused line 12: unnamed unused line 13: unnamed unused line 14: unnamed unused line 15: unnamed unused line 16: unnamed unused line 17: unnamed "regulator-vcc-sdhi0" [kernel output] line 18: unnamed "regulator-vcc-sdhi1" [kernel output] line 19: unnamed "regulator-vcc-sdhi2" [kernel output] line 20: unnamed unused line 21: unnamed unused line 22: unnamed unused line 23: unnamed unused line 24: unnamed unused line 25: unnamed unused 2. Userspace library libgpiod, incl. a few tools like gpioinfo/gpioset/gpioget. These accept whatever reference to identify a GPIO. As your driver fills in pins[i].name, I expect you can pass names like P5_6. Have fun! ;-) Gr{oetje,eeting}s, Geert
Hi Geert, Just in case you were wondering... On Monday, November 12, 2018, Geert Uytterhoeven wrote: > > + "PJ_0", "PJ_1", "PJ_2", "PJ_3", "PJ_4", "PJ_5", "PJ_6", "PJ_7", > > + "PK_0", "PK_1", "PK_2", "PK_3", "PK_4", "PK_5", "PK_6", "PK_7", > > + "PL_0", "PL_1", "PL_2", "PL_3", "PL_4", "PL_5", "PL_6", "PL_7", > > + "PM_0", "PM_1", "PM_2", "PM_3", "PM_4", "PM_5", "PM_6", "PM_7", > > +}; > > + > > +static struct gpio_chip chip = { > > + .names = rza2_gpio_names, > > BTW, is their much value in filling gpio_chip.names[]? > I had never seen that before. On Tuesday, November 13, 2018, Geert Uytterhoeven wrote: > 2. Userspace library libgpiod, incl. a few tools like > gpioinfo/gpioset/gpioget. > These accept whatever reference to identify a GPIO. > As your driver fills in pins[i].name, I expect you can pass names > like P5_6. Actually, just filling in pins[i].name is not enough. You really need to pass an array of strings to gpio_chip.name. When struct gpio_chip.names = NULL, you get this: $ gpioinfo gpiochip0 - 176 lines: line 0: unnamed unused input active-high line 1: unnamed unused input active-high line 2: unnamed unused input active-high line 3: unnamed unused input active-high line 4: unnamed unused input active-high line 5: unnamed unused input active-high line 6: unnamed unused input active-high line 7: unnamed unused input active-high line 8: unnamed unused input active-high line 9: unnamed unused input active-high line 10: unnamed unused input active-high line 11: unnamed unused input active-high line 12: unnamed unused input active-high When struct gpio_chip.names = rza2_gpio_names, you get this: $ gpioinfo gpiochip0 - 176 lines: line 0: "P0_0" unused input active-high line 1: "P0_1" unused input active-high line 2: "P0_2" unused input active-high line 3: "P0_3" unused input active-high line 4: "P0_4" unused input active-high line 5: "P0_5" unused input active-high line 6: "P0_6" unused input active-high line 7: "P0_7" unused input active-high line 8: "P1_0" unused input active-high line 9: "P1_1" unused input active-high line 10: "P1_2" unused input active-high line 11: "P1_3" unused input active-high line 12: "P1_4" unused input active-high Then you can use gpiofind to convert a name into an ID. $ gpiofind P6_0 gpiochip0 48 But to your point, libgpiod is way better than using /sys directly. Chris
Hi Jacopo, Thank you for your review....again ;) On Tuesday, November 13, 2018, jacopo mondi wrote: > I would prefer the reverse xmas order too, but I'm not saying out loud > as I understand is something hard to enforce, as it's a very minor > issue :) The next version will be very festive! # # .#%###%##%##. .##%###%##. .%##%###. .#%##%. .###. .#. t Christmas in Australia > > + if (!of_property_read_bool(np, "gpio-controller")) { > > + dev_info(priv->dev, "No gpio controller registered\n"); > > + return 0; > > + } > > The bindings say this is mandatory... What do you think, would that > make sense to have no gpio-controller support for this driver (testing > apart :) Good point. I don't need to check for this. If it doesn't work..."RTFM" > > + gpio_range.id = of_args.args[0]; > > Do you think of_args need to be validated? > As I understand them, id and pin_base are fixed at 0, while the pin > number depends on the soc version/revision. > > Now I wonder if you would need a gpio-ranges property at all, but > that's fine, it doesn't hurt... My idea was that if another RZ/A2 with a different package size is created, or even a RZ/A3 with the same pin controller, nothing in the driver will have to change because the gpio-ranges is passed in and will tell you how many ports you have. As for validating the values, the only thing I can really check is that: of_args.args[2] == RZA2_NPINS Of course, now that I say that, I realize that if/when it does come time to expand this driver beyond the 1 SOC that exists today, I will have to stop using that hard coded RZA2_NPINS value...but I'll deal with that when the time comes. > > + ret = rza2_pinctrl_register(priv); > > + if (ret) > > + return ret; > > + > > + pr_info("RZ/A2 pin controller registered\n"); > nit: dev_info Actually, I changed it to this to anticipate the day when more than one SOC is supported: dev_info(&pdev->dev, "Registered ports P0 - P%c\n", port_names[priv->desc.npins / RZA2_PINS_PER_PORT - 1]); > With this minor fixed, please add my > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thank you. Chris
Hi Chris, On Thu, Nov 15, 2018 at 12:41 AM Chris Brandt <Chris.Brandt@renesas.com> wrote: > > > + gpio_range.id = of_args.args[0]; > > > > Do you think of_args need to be validated? > > As I understand them, id and pin_base are fixed at 0, while the pin > > number depends on the soc version/revision. > > > > Now I wonder if you would need a gpio-ranges property at all, but > > that's fine, it doesn't hurt... > > My idea was that if another RZ/A2 with a different package size is > created, or even a RZ/A3 with the same pin controller, nothing in the driver > will have to change because the gpio-ranges is passed in and will tell > you how many ports you have. > > As for validating the values, the only thing I can really check is that: > of_args.args[2] == RZA2_NPINS > > Of course, now that I say that, I realize that if/when it does come time > to expand this driver beyond the 1 SOC that exists today, I will have > to stop using that hard coded RZA2_NPINS value...but I'll deal with that > when the time comes. Not that there is an inherent danger in taking of_args.args[2] without validation to support future SoCs without driver changes. port_names[] is a fixed size array, so an SoC with more pins will cause memory access beyond the end of the array. Possibly there are other locations that need to be changed. So IMHO it's better to have explicit support for different SoCs using different compatible values, so you can store the number of pins in of_device_id.data, and validate against that. > > > + ret = rza2_pinctrl_register(priv); > > > + if (ret) > > > + return ret; > > > + > > > + pr_info("RZ/A2 pin controller registered\n"); > > nit: dev_info > > Actually, I changed it to this to anticipate the day when more than one > SOC is supported: > > dev_info(&pdev->dev, "Registered ports P0 - P%c\n", > port_names[priv->desc.npins / RZA2_PINS_PER_PORT - 1]); See, if npins becomes larger, you'll overflow port_names[]. Gr{oetje,eeting}s, Geert
Hi Geert, On Thursday, November 15, 2018, Geert Uytterhoeven wrote: > > As for validating the values, the only thing I can really check is that: > > of_args.args[2] == RZA2_NPINS > > > > Of course, now that I say that, I realize that if/when it does come time > > to expand this driver beyond the 1 SOC that exists today, I will have > > to stop using that hard coded RZA2_NPINS value...but I'll deal with that > > when the time comes. > > Not that there is an inherent danger in taking of_args.args[2] without > validation to support future SoCs without driver changes. > port_names[] is a fixed size array, so an SoC with more pins will > cause memory access beyond the end of the array. Possibly there are > other locations that need to be changed. > > So IMHO it's better to have explicit support for different SoCs using > different compatible values, so you can store the number of pins in > of_device_id.data, and validate against that. I will fill in of_device_id.data with the number of ports for each SOC (only 1 SOC today) and then use that number throughout the driver instead of the hard coded number. I'll also check that of_args.args[2] matches of_device_id.data. Then, it should be real easy to add support for another SOC when it comes along. RZ/T1 which has the same controller has ports up to Port "U" for example. Chris
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 4d8c00eac742..3e4d890d4bd9 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -195,6 +195,17 @@ config PINCTRL_RZA1 help This selects pinctrl driver for Renesas RZ/A1 platforms. +config PINCTRL_RZA2 + bool "Renesas RZ/A2 gpio and pinctrl driver" + depends on OF + depends on ARCH_R7S9210 || COMPILE_TEST + select GPIOLIB + select GENERIC_PINCTRL_GROUPS + select GENERIC_PINMUX_FUNCTIONS + select GENERIC_PINCONF + help + This selects pinctrl driver for Renesas RZ/A2 platforms. + config PINCTRL_RZN1 bool "Renesas RZ/N1 pinctrl driver" depends on OF diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index 18a13c1e2c21..712184b74a5c 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -27,6 +27,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o obj-$(CONFIG_PINCTRL_RZA1) += pinctrl-rza1.o +obj-$(CONFIG_PINCTRL_RZA2) += pinctrl-rza2.o obj-$(CONFIG_PINCTRL_RZN1) += pinctrl-rzn1.o obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o obj-$(CONFIG_PINCTRL_SIRF) += sirf/ diff --git a/drivers/pinctrl/pinctrl-rza2.c b/drivers/pinctrl/pinctrl-rza2.c new file mode 100644 index 000000000000..3781c3f693e8 --- /dev/null +++ b/drivers/pinctrl/pinctrl-rza2.c @@ -0,0 +1,530 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Combined GPIO and pin controller support for Renesas RZ/A2 (R7S9210) SoC + * + * Copyright (C) 2018 Chris Brandt + */ + +/* + * This pin controller/gpio combined driver supports Renesas devices of RZ/A2 + * family. + */ + +#include <linux/bitops.h> +#include <linux/gpio.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/pinctrl/pinmux.h> + +#include "core.h" +#include "pinmux.h" + +#define DRIVER_NAME "pinctrl-rza2" + +#define RZA2_PINS_PER_PORT 8 +#define RZA2_NPINS (RZA2_PINS_PER_PORT * RZA2_NPORTS) +#define RZA2_PIN_ID_TO_PORT(id) ((id) / RZA2_PINS_PER_PORT) +#define RZA2_PIN_ID_TO_PIN(id) ((id) % RZA2_PINS_PER_PORT) + +/* + * Use 16 lower bits [15:0] for pin identifier + * Use 16 higher bits [31:16] for pin mux function + */ +#define MUX_PIN_ID_MASK GENMASK(15, 0) +#define MUX_FUNC_MASK GENMASK(31, 16) +#define MUX_FUNC_OFFS 16 +#define MUX_FUNC(pinconf) ((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS) + +enum pfc_pin_port_name {PORT0, PORT1, PORT2, PORT3, PORT4, PORT5, PORT6, PORT7, + PORT8, PORT9, PORTA, PORTB, PORTC, PORTD, PORTE, PORTF, + PORTG, PORTH, PORTJ, PORTK, PORTL, PORTM, RZA2_NPORTS}; +static const char port_names[] = "0123456789ABCDEFGHJKLM"; + +struct rza2_pinctrl_priv { + struct device *dev; + void __iomem *base; + + struct pinctrl_pin_desc *pins; + struct pinctrl_desc desc; + struct pinctrl_dev *pctl; +}; + +#define RZA2_PDR_BASE(_b) ((_b) + 0x0000) /* 16-bit, 2 bytes apart */ +#define RZA2_PODR_BASE(_b) ((_b) + 0x0040) /* 8-bit, 1 byte apart */ +#define RZA2_PIDR_BASE(_b) ((_b) + 0x0060) /* 8-bit, 1 byte apart */ +#define RZA2_PMR_BASE(_b) ((_b) + 0x0080) /* 8-bit, 1 byte apart */ +#define RZA2_DSCR_BASE(_b) ((_b) + 0x0140) /* 16-bit, 2 bytes apart */ +#define RZA2_PFS_BASE(_b) ((_b) + 0x0200) /* 8-bit, 8 bytes apart */ +#define RZA2_PWPR(_b) ((_b) + 0x02FF) /* 8-bit */ +#define RZA2_PFENET(_b) ((_b) + 0x0820) /* 8-bit */ +#define RZA2_PPOC(_b) ((_b) + 0x0900) /* 32-bit */ +#define RZA2_PHMOMO(_b) ((_b) + 0x0980) /* 32-bit */ +#define RZA2_PCKIO(_b) ((_b) + 0x09D0) /* 8-bit */ + +#define RZA2_PDR(_b, _port) (RZA2_PDR_BASE(_b) + ((_port) * 2)) +#define RZA2_PODR(_b, _port) (RZA2_PODR_BASE(_b) + (_port)) +#define RZA2_PIDR(_b, _port) (RZA2_PIDR_BASE(_b) + (_port)) +#define RZA2_PMR(_b, _port) (RZA2_PMR_BASE(_b) + (_port)) +#define RZA2_DSCR(_b, _port) (RZA2_DSCR_BASE(_b) + ((_port) * 2)) +#define RZA2_PFS(_b, _port, _pin) (RZA2_PFS_BASE(_b) + ((_port) * 8) \ + + (_pin)) +#define RZA2_PDR_HIGHZ 0x00 +#define RZA2_PDR_INPUT 0x02 +#define RZA2_PDR_OUTPUT 0x03 +#define RZA2_PDR_MASK 0x03 + +static void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin, + u8 func) +{ + u8 reg8; + u16 reg16; + u16 mask16; + + /* Set pin to 'Non-use (Hi-z input protection)' */ + reg16 = readw(RZA2_PDR(pfc_base, port)); + mask16 = 0x03 << (pin * 2); + reg16 &= ~mask16; + writew(reg16, RZA2_PDR(pfc_base, port)); + + /* Temporarily switch to GPIO */ + reg8 = readb(RZA2_PMR(pfc_base, port)); + reg8 &= ~BIT(pin); + writeb(reg8, RZA2_PMR(pfc_base, port)); + + /* PFS Register Write Protect : OFF */ + writeb(0x00, RZA2_PWPR(pfc_base)); /* B0WI=0, PFSWE=0 */ + writeb(0x40, RZA2_PWPR(pfc_base)); /* B0WI=0, PFSWE=1 */ + + /* Set Pin function (interrupt disabled, ISEL=0) */ + writeb(func, RZA2_PFS(pfc_base, port, pin)); + + /* PFS Register Write Protect : ON */ + writeb(0x00, RZA2_PWPR(pfc_base)); /* B0WI=0, PFSWE=0 */ + writeb(0x80, RZA2_PWPR(pfc_base)); /* B0WI=1, PFSWE=0 */ + + /* Port Mode : Peripheral module pin functions */ + reg8 = readb(RZA2_PMR(pfc_base, port)); + reg8 |= BIT(pin); + writeb(reg8, RZA2_PMR(pfc_base, port)); +} + +static void rza2_pin_to_gpio(void __iomem *pfc_base, u8 port, u8 pin, u8 dir) +{ + u16 reg16; + u16 mask16; + + reg16 = readw(RZA2_PDR(pfc_base, port)); + mask16 = RZA2_PDR_MASK << (pin * 2); + reg16 &= ~mask16; + + if (dir == GPIOF_DIR_IN) + reg16 |= RZA2_PDR_INPUT << (pin * 2); /* pin as input */ + else + reg16 |= RZA2_PDR_OUTPUT << (pin * 2); /* pin as output */ + + writew(reg16, RZA2_PDR(pfc_base, port)); +} + +static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int offset) +{ + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); + u8 port = offset / 8; + u8 pin = offset % 8; + u16 reg16; + + reg16 = readw(RZA2_PDR(priv->base, port)); + reg16 = (reg16 >> (pin * 2)) & RZA2_PDR_MASK; + + if (reg16 == RZA2_PDR_OUTPUT) + return GPIOF_DIR_OUT; + + if (reg16 == RZA2_PDR_INPUT) + return GPIOF_DIR_IN; + + /* + * This GPIO controller has a default Hi-Z state that is not input or + * output, so force the pin to input now. + */ + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN); + + return GPIOF_DIR_IN; +} + +static int rza2_chip_direction_input(struct gpio_chip *chip, + unsigned int offset) +{ + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); + u8 port = offset / 8; + u8 pin = offset % 8; + + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN); + + return 0; +} + +static int rza2_chip_direction_output(struct gpio_chip *chip, + unsigned int offset, int val) +{ + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); + u8 port = offset / 8; + u8 pin = offset % 8; + + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_OUT); + + return 0; +} + +static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset) +{ + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); + u8 port = offset / 8; + u8 pin = offset % 8; + + return (readb(RZA2_PIDR(priv->base, port)) >> pin) & 1; +} + +static void rza2_chip_set(struct gpio_chip *chip, unsigned int offset, + int value) +{ + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); + u8 port = offset / 8; + u8 pin = offset % 8; + u8 new_value = readb(RZA2_PODR(priv->base, port)); + + if (value) + new_value |= (1 << pin); + else + new_value &= ~(1 << pin); + + writeb(new_value, RZA2_PODR(priv->base, port)); +} + +static const char * const rza2_gpio_names[] = { + "P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7", + "P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7", + "P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7", + "P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7", + "P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7", + "P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7", + "P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7", + "P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7", + "P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7", + "P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7", + "PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7", + "PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7", + "PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7", + "PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7", + "PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7", + "PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7", + "PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7", + "PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7", + /* port I does not exist */ + "PJ_0", "PJ_1", "PJ_2", "PJ_3", "PJ_4", "PJ_5", "PJ_6", "PJ_7", + "PK_0", "PK_1", "PK_2", "PK_3", "PK_4", "PK_5", "PK_6", "PK_7", + "PL_0", "PL_1", "PL_2", "PL_3", "PL_4", "PL_5", "PL_6", "PL_7", + "PM_0", "PM_1", "PM_2", "PM_3", "PM_4", "PM_5", "PM_6", "PM_7", +}; + +static struct gpio_chip chip = { + .names = rza2_gpio_names, + .base = -1, + .ngpio = RZA2_NPINS, + .get_direction = rza2_chip_get_direction, + .direction_input = rza2_chip_direction_input, + .direction_output = rza2_chip_direction_output, + .get = rza2_chip_get, + .set = rza2_chip_set, +}; + +struct pinctrl_gpio_range gpio_range; + +static int rza2_gpio_register(struct rza2_pinctrl_priv *priv) +{ + struct device_node *np = priv->dev->of_node; + struct of_phandle_args of_args; + int ret; + + if (!of_property_read_bool(np, "gpio-controller")) { + dev_info(priv->dev, "No gpio controller registered\n"); + return 0; + } + + chip.label = devm_kasprintf(priv->dev, GFP_KERNEL, "%pOFn", np); + chip.of_node = np; + chip.parent = priv->dev; + + ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, + &of_args); + if (ret) { + dev_err(priv->dev, "Unable to parse gpio-ranges\n"); + return ret; + } + + gpio_range.id = of_args.args[0]; + gpio_range.name = chip.label; + gpio_range.pin_base = gpio_range.base = of_args.args[1]; + gpio_range.npins = of_args.args[2]; + gpio_range.gc = &chip; + + /* Register our gpio chip with gpiolib */ + ret = devm_gpiochip_add_data(priv->dev, &chip, priv); + if (ret) + return ret; + + /* Register pin range with pinctrl core */ + pinctrl_add_gpio_range(priv->pctl, &gpio_range); + + dev_dbg(priv->dev, "Registered gpio controller\n"); + + return 0; +} + +static int rza2_pinctrl_register(struct rza2_pinctrl_priv *priv) +{ + struct pinctrl_pin_desc *pins; + unsigned int i; + int ret; + + pins = devm_kcalloc(priv->dev, RZA2_NPINS, sizeof(*pins), GFP_KERNEL); + if (!pins) + return -ENOMEM; + + priv->pins = pins; + priv->desc.pins = pins; + priv->desc.npins = RZA2_NPINS; + + for (i = 0; i < RZA2_NPINS; i++) { + unsigned int pin = RZA2_PIN_ID_TO_PIN(i); + unsigned int port = RZA2_PIN_ID_TO_PORT(i); + + pins[i].number = i; + pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL, "P%c_%u", + port_names[port], pin); + } + + ret = devm_pinctrl_register_and_init(priv->dev, &priv->desc, priv, + &priv->pctl); + if (ret) { + dev_err(priv->dev, "pinctrl registration failed\n"); + return ret; + } + + ret = pinctrl_enable(priv->pctl); + if (ret) { + dev_err(priv->dev, "pinctrl enable failed\n"); + return ret; + } + + ret = rza2_gpio_register(priv); + if (ret) { + dev_err(priv->dev, "GPIO registration failed\n"); + return ret; + } + + return 0; +} + +/* + * For each DT node, create a single pin mapping. That pin mapping will only + * contain a single group of pins, and that group of pins will only have a + * single function that can be selected. + */ +static int rza2_dt_node_to_map(struct pinctrl_dev *pctldev, + struct device_node *np, + struct pinctrl_map **map, + unsigned int *num_maps) +{ + struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev); + struct property *of_pins; + int i; + unsigned int *pins; + unsigned int *psel_val; + const char **pin_fn; + int ret, npins; + int gsel, fsel; + + /* Find out how many pins to map */ + of_pins = of_find_property(np, "pinmux", NULL); + if (!of_pins) { + dev_info(priv->dev, "Missing pinmux property\n"); + return -ENOENT; + } + npins = of_pins->length / sizeof(u32); + + pins = devm_kcalloc(priv->dev, npins, sizeof(*pins), GFP_KERNEL); + psel_val = devm_kcalloc(priv->dev, npins, sizeof(*psel_val), + GFP_KERNEL); + pin_fn = devm_kzalloc(priv->dev, sizeof(*pin_fn), GFP_KERNEL); + if (!pins || !psel_val || !pin_fn) + return -ENOMEM; + + /* Collect pin locations and mux settings from DT properties */ + for (i = 0; i < npins; ++i) { + u32 value; + + ret = of_property_read_u32_index(np, "pinmux", i, &value); + if (ret) + return ret; + pins[i] = value & MUX_PIN_ID_MASK; + psel_val[i] = MUX_FUNC(value); + } + + /* Register a single pin group listing all the pins we read from DT */ + gsel = pinctrl_generic_add_group(pctldev, np->name, pins, npins, NULL); + if (gsel < 0) + return gsel; + + /* + * Register a single group function where the 'data' is an array PSEL + * register values read from DT. + */ + pin_fn[0] = np->name; + fsel = pinmux_generic_add_function(pctldev, np->name, pin_fn, 1, + psel_val); + if (fsel < 0) { + ret = fsel; + goto remove_group; + } + + dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins); + + /* Create map where to retrieve function and mux settings from */ + *num_maps = 0; + *map = kzalloc(sizeof(**map), GFP_KERNEL); + if (!*map) { + ret = -ENOMEM; + goto remove_function; + } + + (*map)->type = PIN_MAP_TYPE_MUX_GROUP; + (*map)->data.mux.group = np->name; + (*map)->data.mux.function = np->name; + *num_maps = 1; + + return 0; + +remove_function: + pinmux_generic_remove_function(pctldev, fsel); + +remove_group: + pinctrl_generic_remove_group(pctldev, gsel); + + dev_info(priv->dev, "Unable to parse DT node %s\n", np->name); + + return ret; +} + +static void rza2_dt_free_map(struct pinctrl_dev *pctldev, + struct pinctrl_map *map, unsigned int num_maps) +{ + kfree(map); +} + +static const struct pinctrl_ops rza2_pinctrl_ops = { + .get_groups_count = pinctrl_generic_get_group_count, + .get_group_name = pinctrl_generic_get_group_name, + .get_group_pins = pinctrl_generic_get_group_pins, + .dt_node_to_map = rza2_dt_node_to_map, + .dt_free_map = rza2_dt_free_map, +}; + +static int rza2_set_mux(struct pinctrl_dev *pctldev, unsigned int selector, + unsigned int group) +{ + struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev); + struct function_desc *func; + struct group_desc *grp; + int i; + unsigned int *psel_val; + + grp = pinctrl_generic_get_group(pctldev, group); + if (!grp) + return -EINVAL; + + func = pinmux_generic_get_function(pctldev, selector); + if (!func) + return -EINVAL; + + psel_val = func->data; + + for (i = 0; i < grp->num_pins; ++i) { + dev_dbg(priv->dev, "Setting P%c_%d to PSEL=%d\n", + port_names[RZA2_PIN_ID_TO_PORT(grp->pins[i])], + RZA2_PIN_ID_TO_PIN(grp->pins[i]), + psel_val[i]); + rza2_set_pin_function( + priv->base, + RZA2_PIN_ID_TO_PORT(grp->pins[i]), + RZA2_PIN_ID_TO_PIN(grp->pins[i]), + psel_val[i]); + } + + return 0; +} + +static const struct pinmux_ops rza2_pinmux_ops = { + .get_functions_count = pinmux_generic_get_function_count, + .get_function_name = pinmux_generic_get_function_name, + .get_function_groups = pinmux_generic_get_function_groups, + .set_mux = rza2_set_mux, + .strict = true, +}; + +static int rza2_pinctrl_probe(struct platform_device *pdev) +{ + struct rza2_pinctrl_priv *priv; + struct resource *res; + int ret; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->dev = &pdev->dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + priv->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(priv->base)) + return PTR_ERR(priv->base); + + platform_set_drvdata(pdev, priv); + + priv->desc.name = DRIVER_NAME; + priv->desc.pctlops = &rza2_pinctrl_ops; + priv->desc.pmxops = &rza2_pinmux_ops; + priv->desc.owner = THIS_MODULE; + + ret = rza2_pinctrl_register(priv); + if (ret) + return ret; + + pr_info("RZ/A2 pin controller registered\n"); + + return 0; +} + +static const struct of_device_id rza2_pinctrl_of_match[] = { + { + .compatible = "renesas,r7s9210-pinctrl", + }, + { /* sentinel */ } +}; + +static struct platform_driver rza2_pinctrl_driver = { + .driver = { + .name = DRIVER_NAME, + .of_match_table = rza2_pinctrl_of_match, + }, + .probe = rza2_pinctrl_probe, +}; + +static int __init rza2_pinctrl_init(void) +{ + return platform_driver_register(&rza2_pinctrl_driver); +} +core_initcall(rza2_pinctrl_init); + + +MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>"); +MODULE_DESCRIPTION("Pin and gpio controller driver for RZ/A2 SoC"); +MODULE_LICENSE("GPL v2");
Adds support for the pin and gpio controller found in R7S9210 (RZ/A2) SoCs. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- v3: - Changed names from Px to PORTx because "PC" is already defined v2: - fixed SOC part number in comments - sorted #includes - removed spaces in pfc_pin_port_name enum - put RZA2_NPORTS at the end of pfc_pin_port_name enum - added RZA2_ to the beginning of all #define macros - put ( ) around all passed arguments in #define macros - made helper macros to get register address easier - use defines for pin direction bit settings --- drivers/pinctrl/Kconfig | 11 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-rza2.c | 530 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 542 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-rza2.c