Hi Andreas, All accepted except the output path for gpio.
I don't think we need to configure the output path for gpio from pinconf. We can use GPIO driver to handle the gpio output. Are you agree? > -----Original Message----- > From: Andreas Bießmann [mailto:[email protected]] > Sent: 2016年5月3日 18:48 > To: Yang, Wenyou <[email protected]> > Cc: U-Boot Mailing List <[email protected]> > Subject: Re: [U-Boot] [PATCH 1/2] pinctrl: at91-pio4: add pinctrl driver > > Dear Wenyou, > > On 2016-04-07 04:15, Wenyou Yang wrote: > > AT91 PIO4 controller is a combined gpio-controller, pin-mux and > > pin-config module. The peripherals are assigned pins through per-pin > > based muxing logic. And the pin configuration are performed on > > specific registers which are shared along with the gpio controller. > > > > Signed-off-by: Wenyou Yang <[email protected]> > > --- > > > > arch/arm/mach-at91/include/mach/atmel_pio4.h | 35 ++++++ > > drivers/pinctrl/Kconfig | 7 ++ > > drivers/pinctrl/Makefile | 1 + > > drivers/pinctrl/pinctrl-at91-pio4.c | 164 > > +++++++++++++++++++++++++++ > > 4 files changed, 207 insertions(+) > > create mode 100644 drivers/pinctrl/pinctrl-at91-pio4.c > > > > diff --git a/arch/arm/mach-at91/include/mach/atmel_pio4.h > > b/arch/arm/mach-at91/include/mach/atmel_pio4.h > > index 8bb4b12..6760bec 100644 > > --- a/arch/arm/mach-at91/include/mach/atmel_pio4.h > > +++ b/arch/arm/mach-at91/include/mach/atmel_pio4.h > > @@ -29,6 +29,41 @@ struct atmel_pio4_port { > > > > #endif > > > > +/* > > + * PIO Configuration Register Fields > > + */ > > +#define ATMEL_PIO_CFGR_FUNC_MASK GENMASK(2, 0) > > +#define ATMEL_PIO_CFGR_FUNC_GPIO (0x0 << 0) > > +#define ATMEL_PIO_CFGR_FUNC_PERIPH_A (0x1 << 0) > > +#define ATMEL_PIO_CFGR_FUNC_PERIPH_B (0x2 << 0) > > +#define ATMEL_PIO_CFGR_FUNC_PERIPH_C (0x3 << 0) > > +#define ATMEL_PIO_CFGR_FUNC_PERIPH_D (0x4 << 0) > > +#define ATMEL_PIO_CFGR_FUNC_PERIPH_E (0x5 << 0) > > +#define ATMEL_PIO_CFGR_FUNC_PERIPH_F (0x6 << 0) > > +#define ATMEL_PIO_CFGR_FUNC_PERIPH_G (0x7 << 0) > > +#define ATMEL_PIO_DIR_MASK BIT(8) > > +#define ATMEL_PIO_PUEN_MASK BIT(9) > > +#define ATMEL_PIO_PDEN_MASK BIT(10) > > +#define ATMEL_PIO_IFEN_MASK BIT(12) > > +#define ATMEL_PIO_IFSCEN_MASK BIT(13) > > +#define ATMEL_PIO_OPD_MASK BIT(14) > > +#define ATMEL_PIO_SCHMITT_MASK BIT(15) > > +#define ATMEL_PIO_CFGR_EVTSEL_MASK GENMASK(26, 24) > > +#define ATMEL_PIO_CFGR_EVTSEL_FALLING (0 << 24) > > +#define ATMEL_PIO_CFGR_EVTSEL_RISING (1 << 24) > > +#define ATMEL_PIO_CFGR_EVTSEL_BOTH (2 << 24) > > +#define ATMEL_PIO_CFGR_EVTSEL_LOW (3 << 24) > > +#define ATMEL_PIO_CFGR_EVTSEL_HIGH (4 << 24) > > + > > +#define ATMEL_PIO_NPINS_PER_BANK 32 > > +#define ATMEL_PIO_BANK(pin_id) (pin_id / > ATMEL_PIO_NPINS_PER_BANK) > > +#define ATMEL_PIO_LINE(pin_id) (pin_id % > ATMEL_PIO_NPINS_PER_BANK) > > +#define ATMEL_PIO_BANK_OFFSET 0x40 > > + > > +#define ATMEL_GET_PIN_NO(pinfunc) ((pinfunc) & 0xff) > > +#define ATMEL_GET_PIN_FUNC(pinfunc) ((pinfunc >> 16) & 0xf) > > +#define ATMEL_GET_PIN_IOSET(pinfunc) ((pinfunc >> 20) & 0xf) > > + > > #define AT91_PIO_PORTA 0x0 > > #define AT91_PIO_PORTB 0x1 > > #define AT91_PIO_PORTC 0x2 > > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index > > 2a69bab..e71f298 100644 > > --- a/drivers/pinctrl/Kconfig > > +++ b/drivers/pinctrl/Kconfig > > @@ -105,6 +105,13 @@ config SPL_PINCONF > > > > if PINCTRL || SPL_PINCTRL > > > > +config PINCTRL_AT91PIO4 > > + bool "AT91 PIO4 pinctrl driver" > > + depends on DM > > + help > > + This option is to enable the AT91 pinctrl driver for AT91 PIO4 > > + controller which is available on SAMA5D2 SoC. > > + > > config ROCKCHIP_PINCTRL > > bool "Rockchip pin control driver" > > depends on DM > > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index > > 37dc904..bd0dea2 100644 > > --- a/drivers/pinctrl/Makefile > > +++ b/drivers/pinctrl/Makefile > > @@ -5,6 +5,7 @@ > > obj-y += pinctrl-uclass.o > > obj-$(CONFIG_$(SPL_)PINCTRL_GENERIC) += pinctrl-generic.o > > > > +obj-$(CONFIG_PINCTRL_AT91PIO4) += pinctrl-at91-pio4.o > > obj-y += nxp/ > > obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip/ > > obj-$(CONFIG_PINCTRL_SANDBOX) += pinctrl-sandbox.o > > diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c > > b/drivers/pinctrl/pinctrl-at91-pio4.c > > new file mode 100644 > > index 0000000..e4ca5dc > > --- /dev/null > > +++ b/drivers/pinctrl/pinctrl-at91-pio4.c > > @@ -0,0 +1,164 @@ > > +/* > > + * Atmel PIO4 pinctrl driver > > + * > > + * Copyright (C) 2016 Atmel Corporation > > + * Wenyou.Yang <[email protected]> > > + * > > + * SPDX-License-Identifier: GPL-2.0+ > > + */ > > + > > +#include <common.h> > > +#include <dm/device.h> > > +#include <dm/pinctrl.h> > > +#include <linux/io.h> > > +#include <linux/err.h> > > +#include <mach/atmel_pio4.h> > > + > > +DECLARE_GLOBAL_DATA_PTR; > > + > > +/* > > + * Warning: > > + * In order to not introduce confusion between Atmel PIO groups and > > pinctrl > > + * framework groups, Atmel PIO groups will be called banks, line is > > kept to > > + * designed the pin id into this bank. > > full stop after 'will be called banks'. What does the rest of the sentence > mean? > > > + */ > > + > > +struct atmel_pio4_platdata { > > + struct atmel_pio4_port *reg_base; > > +}; > > + > > +static const struct pinconf_param conf_params[] = { > > + { "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 }, > > + { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 }, > > + { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 }, > > + { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, > > + { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, > > + { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, > > + { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, }; > > + > > +static u32 atmel_pinctrl_get_pinconf(const void *blob, int node) { > > + const struct pinconf_param *params; > > + u32 param, arg, conf = 0; > > + u32 i; > > + > > + for (i = 0; i < ARRAY_SIZE(conf_params); i++) { > > + params = &conf_params[i]; > > + if (!fdt_get_property(blob, node, params->property, NULL)) > > + continue; > > + > > + param = params->param; > > + arg = params->default_value; > > + > > + switch (param) { > > + case PIN_CONFIG_BIAS_DISABLE: > > + conf &= (~ATMEL_PIO_PUEN_MASK); > > + conf &= (~ATMEL_PIO_PDEN_MASK); > > + break; > > + case PIN_CONFIG_BIAS_PULL_UP: > > + conf |= ATMEL_PIO_PUEN_MASK; > > + break; > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > + conf |= ATMEL_PIO_PDEN_MASK; > > + break; > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > + conf |= ATMEL_PIO_OPD_MASK; > > + break; > > + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: > > + if (arg == 0) > > + conf |= ATMEL_PIO_SCHMITT_MASK; > > I think we should also add the else path for arg != 0. You could argue that > this is > setup by conf = 0 above, but why should we disable the bias setup above then? > > > + break; > > + case PIN_CONFIG_INPUT_DEBOUNCE: > > + conf |= ATMEL_PIO_IFEN_MASK; > > + conf |= ATMEL_PIO_IFSCEN_MASK; > > For this case too, we should use the linux kernel way here too. > > > + break; > > How about the output path for gpio? > > > + default: > > + printf("%s: Unsupported configuration parameter: %u\n", > > + __func__, param); > > + break; > > + } > > + } > > + > > + return conf; > > +} > > + > > +static struct atmel_pio4_port *atmel_pio4_bank_base(struct udevice > > *dev, > > + u32 bank) > > I'd make this an inline function. > > > +{ > > + struct atmel_pio4_platdata *platdata = dev_get_platdata(dev); > > + struct atmel_pio4_port *bank_base = > > + (struct atmel_pio4_port *)((u32)platdata->reg_base + > > + ATMEL_PIO_BANK_OFFSET * bank); > > + > > + return bank_base; > > +} > > + > > +static int atmel_pinctrl_set_state(struct udevice *dev, struct > > +udevice > > *config) > > +{ > > + struct atmel_pio4_port *bank_base; > > + const void *blob = gd->fdt_blob; > > + int node = config->of_offset; > > + u32 offset, func, bank, line; > > + u32 cells[40]; > > + u32 i, conf; > > + int count; > > + > > + conf = atmel_pinctrl_get_pinconf(blob, node); > > + > > + count = fdtdec_get_int_array_count(blob, node, "pinmux", > > + cells, ARRAY_SIZE(cells)); > > + if (count < 0) { > > + printf("%s: bad pinmux array %d\n", __func__, count); > > + return -EINVAL; > > + } > > + > > + for (i = 0 ; i < count; i++) { > > How about more than 40 entries? Could this be handled in any way? > > > + offset = ATMEL_GET_PIN_NO(cells[i]); > > + func = ATMEL_GET_PIN_FUNC(cells[i]); > > + > > + bank = ATMEL_PIO_BANK(offset); > > + line = ATMEL_PIO_LINE(offset); > > + > > + bank_base = atmel_pio4_bank_base(dev, bank); > > + > > + writel(BIT(line), &bank_base->mskr); > > + conf &= (~ATMEL_PIO_CFGR_FUNC_MASK); > > + conf |= (func & ATMEL_PIO_CFGR_FUNC_MASK); > > + writel(conf, &bank_base->cfgr); > > + } > > + > > + return 0; > > +} > > + > > +const struct pinctrl_ops atmel_pinctrl_ops = { > > + .set_state = atmel_pinctrl_set_state, }; > > + > > +static int atmel_pinctrl_probe(struct udevice *dev) { > > + struct atmel_pio4_platdata *platdata = dev_get_platdata(dev); > > + fdt_addr_t addr_base; > > + > > + addr_base = dev_get_addr(dev); > > + if (addr_base == FDT_ADDR_T_NONE) > > + return -ENODEV; > > + > > + platdata->reg_base = (struct atmel_pio4_port *)addr_base; > > + > > + return 0; > > +} > > + > > +static const struct udevice_id atmel_pinctrl_match[] = { > > + { .compatible = "atmel,sama5d2-pinctrl" }, > > + {} > > +}; > > + > > +U_BOOT_DRIVER(atmel_pinctrl) = { > > + .name = "pinctrl_atmel_pio4", > > + .id = UCLASS_PINCTRL, > > + .of_match = atmel_pinctrl_match, > > + .probe = atmel_pinctrl_probe, > > + .platdata_auto_alloc_size = sizeof(struct atmel_pio4_platdata), > > + .ops = &atmel_pinctrl_ops, > > +}; > > Best regards > > Andreas Best Regards, Wenyou Yang _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
