[U-Boot] [PATCH v3 5/8] rockchip: rk3399: Add improved pinctrl driver.

Simon Glass sjg at chromium.org
Thu Dec 20 21:17:03 UTC 2018


On Mon, 17 Dec 2018 at 06:30, Christoph Muellner
<christoph.muellner at theobroma-systems.com> wrote:
>
> The current pinctrl driver for the RK3399 has a range of qulity issues.
> E.g. it only implements the .set_state_simple() callback, it
> does not parse the available pinctrl information from the DTS
> (instead uses hardcoded values), is not flexible enough to cover
> devices without 'interrupt' field in the DTS (e.g. PWM),
> is not written generic enough to make code reusable among other
> rockchip SoCs...
>
> This patch addresses these issues by reimplementing the whole driver
> from scratch using the .set_state() callback.
> The new implementation covers all featurese of the old code
> (i.e. it supports pinmuxing and pullup/pulldown configuration).
>
> This patch has been tested on a RK3399-Q7 SoM (Puma).
>
> Signed-off-by: Christoph Muellner <christoph.muellner at theobroma-systems.com>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/pinctrl/rockchip/pinctrl_rk3399.c | 226 ++++++++++++++++++++++++++++++
>  1 file changed, 226 insertions(+)
>
> diff --git a/drivers/pinctrl/rockchip/pinctrl_rk3399.c b/drivers/pinctrl/rockchip/pinctrl_rk3399.c
> index bc92dd7c06..ed9828989f 100644
> --- a/drivers/pinctrl/rockchip/pinctrl_rk3399.c
> +++ b/drivers/pinctrl/rockchip/pinctrl_rk3399.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
>   * (C) Copyright 2016 Rockchip Electronics Co., Ltd
> + * (C) 2018 Theobroma Systems Design und Consulting GmbH
>   */
>
>  #include <common.h>
> @@ -14,11 +15,234 @@
>  #include <asm/arch/clock.h>
>  #include <dm/pinctrl.h>
>
> +static const u32 RK_GRF_P_PULLUP = 1;
> +static const u32 RK_GRF_P_PULLDOWN = 2;
> +
>  struct rk3399_pinctrl_priv {
>         struct rk3399_grf_regs *grf;
>         struct rk3399_pmugrf_regs *pmugrf;
> +       struct rockchip_pin_bank *banks;
> +};
> +
> +/**
> + * Location of pinctrl/pinconf registers.
> + */

Comment should be on one line

> +enum rk_grf_location {
> +       RK_GRF,
> +       RK_PMUGRF,
> +};
> +
> +/**
> + * @nr_pins: number of pins in this bank
> + * @bank_num: number of the bank, to account for holes
> + * @iomux: array describing the 4 iomux sources of the bank

This comment seems to refer to something else.

> + */
> +struct rockchip_pin_bank {
> +       u8 nr_pins;
> +       enum rk_grf_location grf_location;
> +       size_t iomux_offset;
> +       size_t pupd_offset;
>  };
>
> +#define PIN_BANK(pins, grf, iomux, pupd)               \
> +       {                                               \
> +               .nr_pins = pins,                        \
> +               .grf_location = grf,                    \
> +               .iomux_offset = iomux,                  \
> +               .pupd_offset = pupd,                    \
> +       }
> +
> +static struct rockchip_pin_bank rk3399_pin_banks[] = {
> +       PIN_BANK(16, RK_PMUGRF,
> +                offsetof(struct rk3399_pmugrf_regs, gpio0a_iomux),
> +                offsetof(struct rk3399_pmugrf_regs, gpio0_p)),
> +       PIN_BANK(32, RK_PMUGRF,
> +                offsetof(struct rk3399_pmugrf_regs, gpio1a_iomux),
> +                offsetof(struct rk3399_pmugrf_regs, gpio1_p)),
> +       PIN_BANK(32, RK_GRF,
> +                offsetof(struct rk3399_grf_regs, gpio2a_iomux),
> +                offsetof(struct rk3399_grf_regs, gpio2_p)),
> +       PIN_BANK(32, RK_GRF,
> +                offsetof(struct rk3399_grf_regs, gpio3a_iomux),
> +                offsetof(struct rk3399_grf_regs, gpio3_p)),
> +       PIN_BANK(32, RK_GRF,
> +                offsetof(struct rk3399_grf_regs, gpio4a_iomux),
> +                offsetof(struct rk3399_grf_regs, gpio4_p)),
> +};
> +
> +static void rk_pinctrl_get_info(uintptr_t base, u32 index, uintptr_t *addr,
> +                               u32 *shift, u32 *mask)
> +{
> +       /*
> +        * In general we four subsequent 32-bit configuration registers
> +        * per bank (e.g. GPIO2A_P, GPIO2B_P, GPIO2C_P, GPIO2D_P).
> +        * The configuration for each pin has two bits

Do you mean three?

> +        *
> +        * @base...contains the address to the first register.
> +        * @index...defines the pin within the bank (0..31).
> +        * @addr...will be the address of the actual register to use
> +        */
> +
> +       const u32 pins_per_register = 8;
> +       const u32 config_bits_per_pin = 2;
> +
> +       /* Get the address of the configuration register. */
> +       *addr = base + (index / pins_per_register) * sizeof(u32);
> +
> +       /* Get the bit offset within the configruation register. */

configuration

> +       *shift = (index & (pins_per_register - 1)) * config_bits_per_pin;
> +
> +       /* Get the (unshifted) mask for the configuration pins. */
> +       *mask = ((1 << config_bits_per_pin) - 1);
> +
> +       pr_debug("%s: addr=0x%lx, mask=0x%x, shift=0x%x\n",
> +                __func__, *addr, *mask, *shift);
> +}
> +
> +static void rk3399_pinctrl_set_pin_iomux(uintptr_t grf_addr,
> +                                        struct rockchip_pin_bank *bank,
> +                                        u32 index, u32 muxval)
> +{
> +       uintptr_t iomux_base, addr;
> +       u32 shift, mask;
> +
> +       iomux_base = grf_addr + bank->iomux_offset;
> +       rk_pinctrl_get_info(iomux_base, index, &addr, &shift, &mask);
> +
> +       /* Set pinmux register */
> +       rk_clrsetreg(addr, mask << shift, muxval << shift);
> +}
> +
> +static void rk3399_pinctrl_set_pin_pupd(uintptr_t grf_addr,
> +                                       struct rockchip_pin_bank *bank,
> +                                       u32 index, int pinconfig)
> +{
> +       uintptr_t pupd_base, addr;
> +       u32 shift, mask, pupdval;
> +
> +       /* Fast path in case there's nothing to do. */
> +       if (!pinconfig)
> +               return;
> +
> +       if (pinconfig & (1 << PIN_CONFIG_BIAS_PULL_UP))
> +               pupdval = RK_GRF_P_PULLUP;
> +       else if (pinconfig & (1 << PIN_CONFIG_BIAS_PULL_DOWN))
> +               pupdval = RK_GRF_P_PULLDOWN;
> +       else
> +               /* Flag not supported. */
> +               pr_warn("%s: Unsupported pinconfig flag: 0x%x\n", __func__,
> +                       pinconfig);
> +               return;
> +
> +       pupd_base = grf_addr + (uintptr_t)bank->pupd_offset;
> +       rk_pinctrl_get_info(pupd_base, index, &addr, &shift, &mask);
> +
> +       /* Set pull-up/pull-down regisrer */
> +       rk_clrsetreg(addr, mask << shift, pupdval << shift);
> +}
> +
> +static int rk3399_pinctrl_set_pin(struct udevice *dev, u32 banknum, u32 index,
> +                                 u32 muxval, int pinconfig)
> +{
> +       struct rk3399_pinctrl_priv *priv = dev_get_priv(dev);
> +       struct rockchip_pin_bank *bank = &priv->banks[banknum];
> +       uintptr_t grf_addr;
> +
> +       pr_debug("%s: 0x%x 0x%x 0x%x 0x%x\n", __func__, banknum, index, muxval,
> +                pinconfig);
> +
> +       if (bank->grf_location == RK_GRF)
> +               grf_addr = (uintptr_t)priv->grf;
> +       else if (bank->grf_location == RK_PMUGRF)
> +               grf_addr = (uintptr_t)priv->pmugrf;
> +       else
> +               return -EINVAL;
> +
> +       rk3399_pinctrl_set_pin_iomux(grf_addr, bank, index, muxval);
> +
> +       rk3399_pinctrl_set_pin_pupd(grf_addr, bank, index, pinconfig);
> +       return 0;
> +}
> +
> +static int rk3399_pinctrl_set_state(struct udevice *dev, struct udevice *config)
> +{
> +       /*
> +        * The order of the fields in this struct must match the order of
> +        * the fields in the "rockchip,pins" property.
> +        */
> +       struct rk_pin {
> +               u32 banknum;
> +               u32 index;
> +               u32 muxval;
> +               u32 phandle;
> +       } __packed;
> +
> +       u32 *fields = NULL;
> +       const int fields_per_pin = 4;
> +       int num_fields, num_pins;
> +       int ret;
> +       int size;
> +       int i;
> +       struct rk_pin *pin;
> +
> +       pr_debug("%s: %s\n", __func__, config->name);
> +
> +       size = dev_read_size(config, "rockchip,pins");
> +       if (size < 0)
> +               return -ENODEV;

EINVAL? There is a device...it's just that we have an invalid DT.

> +
> +       num_fields = size / sizeof(u32);
> +       num_pins = num_fields / fields_per_pin;
> +
> +       if (num_fields * sizeof(u32) != size ||
> +           num_pins * fields_per_pin != num_fields) {
> +               printf("Sanity check failed!\n");
> +               return -EINVAL;
> +       }
> +
> +       fields = calloc(num_fields, sizeof(u32));
> +       if (!fields)
> +               return -ENOMEM;
> +
> +       ret = dev_read_u32_array(config, "rockchip,pins", fields, num_fields);
> +       if (ret) {
> +               pr_warn("%s: Failed to read rockchip,pins fields.\n",
> +                       config->name);
> +               goto end;
> +       }
> +
> +       pin = (struct rk_pin *)fields;
> +       for (i = 0; i < num_pins; i++, pin++) {

If this fails in a particular iteration, it should really undo what
has been done in earlier loop iterations.

> +               struct udevice *dev_pinconfig;
> +               int pinconfig;
> +
> +               ret = uclass_get_device_by_phandle_id(UCLASS_PINCONFIG,
> +                                                     pin->phandle,
> +                                                     &dev_pinconfig);
> +               if (ret) {
> +                       printf("Could not get pinconfig device\n");

debug() for these to avoid bloating the code

> +                       goto end;
> +               }
> +
> +               pinconfig = pinctrl_decode_pin_config_dm(dev_pinconfig);
> +               if (pinconfig < 0) {
> +                       printf("Could not parse pinconfig\n");
> +                       goto end;
> +               }
> +
> +               ret = rk3399_pinctrl_set_pin(dev, pin->banknum, pin->index,
> +                                            pin->muxval, pinconfig);
> +               if (ret) {
> +                       printf("Could not set pinctrl settings\n");
> +                       goto end;
> +               }
> +       }
> +
> +end:
> +       free(fields);
> +       return ret;
> +}
> +
>  static void pinctrl_rk3399_pwm_config(struct rk3399_grf_regs *grf,
>                 struct rk3399_pmugrf_regs *pmugrf, int pwm_id)
>  {
> @@ -468,6 +692,7 @@ static int rk3399_pinctrl_set_state_simple(struct udevice *dev,
>  }
>
>  static struct pinctrl_ops rk3399_pinctrl_ops = {
> +       .set_state      = rk3399_pinctrl_set_state,
>         .set_state_simple       = rk3399_pinctrl_set_state_simple,
>         .request        = rk3399_pinctrl_request,
>         .get_periph_id  = rk3399_pinctrl_get_periph_id,
> @@ -481,6 +706,7 @@ static int rk3399_pinctrl_probe(struct udevice *dev)
>         priv->grf = syscon_get_first_range(ROCKCHIP_SYSCON_GRF);
>         priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
>         debug("%s: grf=%p, pmugrf=%p\n", __func__, priv->grf, priv->pmugrf);
> +       priv->banks = rk3399_pin_banks;
>
>         return ret;
>  }
> --
> 2.11.0
>

Regards,
SImon


More information about the U-Boot mailing list