Hi Wenyou,

On 11 October 2016 at 23:22, Wenyou Yang <wenyou.y...@atmel.com> wrote:
> AT91 PIO controller is a combined gpio-controller, pin-mux and
> pin-config module. The peripheral's pins are assigned through
> per-pin based muxing logic.
>
> Each soc will have to describe the SoC limitation and pin
> configuration via DT. This will allow to do not need to touch
> the C code when adding new SoC if the IP version is supported.
>
> Signed-off-by: Wenyou Yang <wenyou.y...@atmel.com>
> ---
>
>  arch/arm/mach-at91/include/mach/at91_pio.h |   6 +-
>  drivers/pinctrl/Kconfig                    |   7 +
>  drivers/pinctrl/Makefile                   |   1 +
>  drivers/pinctrl/pinctrl-at91.c             | 451 
> +++++++++++++++++++++++++++++
>  4 files changed, 464 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pinctrl/pinctrl-at91.c

Reviewed-by: Simon Glass <s...@chromium.org>

Some comments below.

>
> diff --git a/arch/arm/mach-at91/include/mach/at91_pio.h 
> b/arch/arm/mach-at91/include/mach/at91_pio.h
> index 393a163..f195a7d 100644
> --- a/arch/arm/mach-at91/include/mach/at91_pio.h
> +++ b/arch/arm/mach-at91/include/mach/at91_pio.h
> @@ -107,7 +107,11 @@ typedef struct at91_port {
>         u32     wpsr;           /* 0xE8 Write Protect Status Register */
>         u32     reserved11[5];  /* */
>         u32     schmitt;        /* 0x100 Schmitt Trigger Register */
> -       u32     reserved12[63];
> +       u32     reserved12[4];  /* 0x104 ~ 0x110 */
> +       u32     driver1;        /* 0x114 I/O Driver Register1(AT91SAM9x5's 
> driver1) */
> +       u32     driver12;       /* 0x118 I/O Driver Register12(AT91SAM9x5's 
> driver2 or SAMA5D3x's driver1 ) */
> +       u32     driver2;        /* 0x11C I/O Driver Register2(SAMA5D3x's 
> driver2) */
> +       u32     reserved13[12]; /* 0x120 ~ 0x14C */
>  } at91_port_t;
>
>  typedef union at91_pio {
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 12be3cf..87a7ff0 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -141,6 +141,13 @@ config ROCKCHIP_RK3288_PINCTRL
>           definitions and pin control functions for each available multiplex
>           function.
>
> +config PINCTRL_AT91
> +       bool "AT91 pinctrl driver"
> +       depends on DM
> +       help
> +         This option is to enable the AT91 pinctrl driver for AT91 PIO
> +         controller.

Can you add a bit more info? What features does it support? How does
it relate to the pio4 driver?

> +
>  config PINCTRL_AT91PIO4
>         bool "AT91 PIO4 pinctrl driver"
>         depends on DM
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index f28b5c1..a9535fa 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_AT91)             += pinctrl-at91.o
>  obj-$(CONFIG_PINCTRL_AT91PIO4)         += pinctrl-at91-pio4.o
>  obj-y                                  += nxp/
>  obj-$(CONFIG_ARCH_ATH79) += ath79/
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> new file mode 100644
> index 0000000..c7f75cb
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -0,0 +1,451 @@
> +/*
> + * Atmel PIO pinctrl driver
> + *
> + * Copyright (C) 2016 Atmel Corporation
> + *               Wenyou.Yang <wenyou.y...@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/at91_pio.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define MAX_GPIO_BANKS         5
> +#define MAX_NB_GPIO_PER_BANK   32
> +
> +#define MAX_PINMUX_ENTRIES     200
> +
> +struct at91_pinctrl_priv {
> +       struct at91_port *reg_base[MAX_GPIO_BANKS];
> +       u32 nbanks;
> +};
> +
> +#define PULL_UP                        BIT(0)
> +#define MULTI_DRIVE            BIT(1)
> +#define DEGLITCH               BIT(2)
> +#define PULL_DOWN              BIT(3)
> +#define DIS_SCHMIT             BIT(4)
> +#define DRIVE_STRENGTH_SHIFT   5
> +#define DRIVE_STRENGTH_MASK    0x3
> +#define DRIVE_STRENGTH         (DRIVE_STRENGTH_MASK << DRIVE_STRENGTH_SHIFT)
> +#define DEBOUNCE               BIT(16)
> +#define DEBOUNCE_VAL_SHIFT     17
> +#define DEBOUNCE_VAL           (0x3fff << DEBOUNCE_VAL_SHIFT)
> +
> +/**
> + * These defines will translated the dt binding settings to our internal
> + * settings. They are not necessarily the same value as the register setting.
> + * The actual drive strength current of low, medium and high must be looked 
> up
> + * from the corresponding device datasheet. This value is different for pins
> + * that are even in the same banks. It is also dependent on VCC.
> + * DRIVE_STRENGTH_DEFAULT is just a placeholder to avoid changing the drive
> + * strength when there is no dt config for it.
> + */
> +#define DRIVE_STRENGTH_DEFAULT (0 << DRIVE_STRENGTH_SHIFT)
> +#define DRIVE_STRENGTH_LOW     (1 << DRIVE_STRENGTH_SHIFT)
> +#define DRIVE_STRENGTH_MED     (2 << DRIVE_STRENGTH_SHIFT)
> +#define DRIVE_STRENGTH_HI      (3 << DRIVE_STRENGTH_SHIFT)
> +
> +enum at91_mux {
> +       AT91_MUX_GPIO = 0,
> +       AT91_MUX_PERIPH_A = 1,
> +       AT91_MUX_PERIPH_B = 2,
> +       AT91_MUX_PERIPH_C = 3,
> +       AT91_MUX_PERIPH_D = 4,
> +};
> +
> +/**
> + * struct at91_pinctrl_mux_ops - describes an AT91 mux ops group
> + * on new IP with support for periph C and D the way to mux in
> + * periph A and B has changed
> + * So provide the right callbacks
> + * if not present means the IP does not support it
> + * @mux_A_periph: mux as periph A
> + * @mux_B_periph: mux as periph B
> + * @mux_C_periph: mux as periph C
> + * @mux_D_periph: mux as periph D
> + * @set_deglitch: enable/disable deglitch
> + * @set_debounce: enable/disable debounce
> + * @set_pulldown: enable/disable pulldown
> + * @disable_schmitt_trig: disable schmitt trigger
> + */
> +struct at91_pinctrl_mux_ops {
> +       void (*mux_A_periph)(struct at91_port *pio, u32 mask);
> +       void (*mux_B_periph)(struct at91_port *pio, u32 mask);
> +       void (*mux_C_periph)(struct at91_port *pio, u32 mask);
> +       void (*mux_D_periph)(struct at91_port *pio, u32 mask);
> +       void (*set_deglitch)(struct at91_port *pio, u32 mask, bool is_on);
> +       void (*set_debounce)(struct at91_port *pio, u32 mask, bool is_on,
> +                            u32 div);
> +       void (*set_pulldown)(struct at91_port *pio, u32 mask, bool is_on);
> +       void (*disable_schmitt_trig)(struct at91_port *pio, u32 mask);
> +       void (*set_drivestrength)(struct at91_port *pio, u32 pin,
> +                                 u32 strength);

Can you add comments for these functions? It isn't clear why they do
or what the arguments mean.
[..]

> +static void set_drive_strength(void *reg, u32 pin, u32 strength)
> +{
> +       u32 tmp = readl(reg);
> +       u32 shift = two_bit_pin_value_shift_amount(pin);
> +
> +       tmp &= ~(DRIVE_STRENGTH_MASK  <<  shift);
> +       tmp |= strength << shift;
> +
> +       writel(tmp, reg);

Can you use clrsetbits_le32() here?

[..]

> +static int at91_pinconf_set(struct at91_pinctrl_mux_ops *ops,
> +                           struct at91_port *pio, u32 pin, u32 config)
> +{
> +       u32 mask = BIT(pin);
> +
> +       if (config & PULL_UP && config & PULL_DOWN)

I'm not sure what that means but I think it needs brackets for readability.

> +               return -EINVAL;
> +
> +       at91_mux_set_pullup(pio, mask, config & PULL_UP);
> +       at91_mux_set_multidrive(pio, mask, config & MULTI_DRIVE);
> +       if (ops->set_deglitch)
> +               ops->set_deglitch(pio, mask, config & DEGLITCH);
> +       if (ops->set_debounce)
> +               ops->set_debounce(pio, mask, config & DEBOUNCE,
> +                       (config & DEBOUNCE_VAL) >> DEBOUNCE_VAL_SHIFT);
> +       if (ops->set_pulldown)
> +               ops->set_pulldown(pio, mask, config & PULL_DOWN);
> +       if (ops->disable_schmitt_trig && config & DIS_SCHMIT)
> +               ops->disable_schmitt_trig(pio, mask);
> +       if (ops->set_drivestrength)
> +               ops->set_drivestrength(pio, pin,
> +                       (config & DRIVE_STRENGTH) >> DRIVE_STRENGTH_SHIFT);
> +
> +       return 0;
> +}
> +
> +static int at91_pin_check_config(struct udevice *dev, u32 bank, u32 pin)
> +{
> +       struct at91_pinctrl_priv *priv = dev_get_priv(dev);
> +
> +       if (bank >= priv->nbanks) {
> +               printf("pin conf bank %d >= nbanks %d\n", bank, priv->nbanks);

debug()

> +               return -EINVAL;
> +       }
> +
> +       if (pin >= MAX_NB_GPIO_PER_BANK) {
> +               printf("pin conf pin %d >= %d\n", pin, MAX_NB_GPIO_PER_BANK);

debug()

> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static struct at91_port *at91_bank_base(struct udevice *dev, u32 bank)

This function doesn't seem very useful - remove it?

Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to