[U-Boot] [PATCH 1/2] pinctrl: at91-pio4: add pinctrl driver
Yang, Wenyou
Wenyou.Yang at atmel.com
Wed May 4 07:14:55 CEST 2016
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:andreas at biessmann.org]
> Sent: 2016年5月3日 18:48
> To: Yang, Wenyou <Wenyou.Yang at atmel.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>
> 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 <wenyou.yang at atmel.com>
> > ---
> >
> > 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 <wenyou.yang at atmel.com>
> > + *
> > + * 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
More information about the U-Boot
mailing list