[U-Boot] [PATCH v2 5/6] rockchip: rk3399: Add improved pinctrl driver.
Philipp Tomsich
philipp.tomsich at theobroma-systems.com
Thu Dec 13 22:26:38 UTC 2018
> On 12.12.2018, at 01:57, 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).
Given that the original version was sent before the close of the MW and that
and that I want to give the maximum possible time to others to evaluate and
fix their DTS (if required), please do the following (in a seperate patch)
(a) make this conditional to a transitory Kconfig option
(b) select this for our boards, as needed
to ensure that the behaviour doesn’t get picked up unknowingly.
I’ll then revert this additional patch as one of the first changes in the next
merge-window to make this common behaviour.
> This patch has been tested on a RK3399-Q7 SoM (Puma).
>
> Signed-off-by: Christoph Muellner <christoph.muellner at theobroma-systems.com>
Reviewed-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
See below for requested changes (and also above).
> ---
>
> Changes in v2: None
>
> drivers/pinctrl/rockchip/pinctrl_rk3399.c | 189 ++++++++++++++++++++++++++++++
> 1 file changed, 189 insertions(+)
>
> diff --git a/drivers/pinctrl/rockchip/pinctrl_rk3399.c b/drivers/pinctrl/rockchip/pinctrl_rk3399.c
> index bc92dd7c062..b6651c92d11 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,197 @@
> #include <asm/arch/clock.h>
> #include <dm/pinctrl.h>
>
> +#define RK_PINCONFIG_PULLUP 1
> +#define RK_PINCONFIG_PULLDOWN 2
Please make these 'const u32’ or use an enum.
I prefer the const u32, whereas Simon prefers the enum.
> +
> struct rk3399_pinctrl_priv {
> struct rk3399_grf_regs *grf;
> struct rk3399_pmugrf_regs *pmugrf;
> + struct rockchip_pin_bank *banks;
> +};
> +
> +/**
> + * Location of pinctrl/pinconf registers.
> + */
> +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
> + */
> +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)
> +{
> + *addr = base + (index / 8) * 4;
Shudder. Can you write this much more cleanly.
> + *shift = index % 8;
I don’t like % (as you know). Please use &.
> + *shift *= 2;
> + *mask = 3;
Generally, I don’t like this calculation, as it’s not obvious what happens and
why this works. Please comment and rewrite in a more readable way.
> +
> + 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 + (uintptr_t)bank->iomux_offset;
The case of uintptr_t seems unnecessary. AFAIR, a uintptr_t + size_t
should be a well-defined combination… but I may remember wrong
and couldn’t be bothered to read my C99 standard.
> + 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;
> +
> + if (pinconfig & (1 << PIN_CONFIG_BIAS_PULL_UP))
Is this defined in one of the header-files included in the DTSI?
This isn’t exactly intuitive when just reading it.
> + pupdval = RK_PINCONFIG_PULLUP;
> + else if (pinconfig & (1 << PIN_CONFIG_BIAS_PULL_DOWN))
> + pupdval = RK_PINCONFIG_PULLDOWN;
> + else
> + return;
No error in the case where we don’t know what to do?
> +
> + 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)
> +{
> + u32 *fields = NULL;
> + const int fields_per_pin = 4;
> + int num_fields, num_pins;
> + int ret;
> + int size;
> + int i;
> +
> + pr_err("%s: %s\n", __func__, config->name);
> +
> + size = dev_read_size(config, "rockchip,pins");
> + if (size < 0)
> + return -ENODEV;
> +
> + 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)
> + goto end;
> +
> + for (i = 0; i < num_pins; i++) {
> + u32 banknum = fields[i * fields_per_pin];
> + u32 index = fields[i * fields_per_pin + 1];
> + u32 muxval = fields[i * fields_per_pin + 2];
> + u32 phandle = fields[i * fields_per_pin + 3];
I’d prefer a struct foo { u32 bank; u32 idx; u32 mux; u32 phandle }; and then
a cast of fields to struct foo, so we can do something along the line of
fields_as_struct[i].bank.
> + struct udevice *dev_pinconfig;
> + int pinconfig;
> +
> + ret = uclass_get_device_by_phandle_id(UCLASS_PINCONFIG, phandle,
> + &dev_pinconfig);
I wonder if it would be more efficient to iterate over the fields-array multiple
times for each different phandle (IIRC, uclass_get_device_by_phandle_id
can be inefficient).
Then again, it probably doesn’t matter...
> + if (ret) {
> + printf("Could not get pinconfig device\n");
> + 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, banknum, index, 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 +655,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 +669,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
>
More information about the U-Boot
mailing list