Re: [U-Boot] [PATCH 1/2] pinctrl: at91-pio4: add pinctrl driver

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@biessmann.org] Sent: 2016年5月3日 18:48 To: Yang, Wenyou Wenyou.Yang@atmel.com Cc: U-Boot Mailing List u-boot@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@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@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