[U-Boot] [PATCH v3 5/8] rockchip: rk3399: Add improved pinctrl driver.
Christoph Müllner
christoph.muellner at theobroma-systems.com
Tue Dec 25 22:17:09 UTC 2018
Hi Simon,
On 12/20/18 10:17 PM, Simon Glass wrote:
> 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
Done for v4.
>
>> +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.
Done for v4.
>
>> + */
>> +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?
No, it is two bits in the configuration register per pin.
For v4: documented also shift and mask.
>> + *
>> + * @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
Done for v4.
>> + *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.
Done for v4.
>
>> +
>> + 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.
Can you please explain the reason for that?
The implemented error handling was inspired by the similar loop
of pinctrl-generic.c (pinctrl_generic_set_state_subnode), where
no such rolling-back is performed. Also the Linux driver does
not have such mechanism (see rockchip_pinctrl_parse_groups()).
Most (all?) other pinctrl drivers also don't do any roll-back.
Actual question:
What's the point of undoing pinmuxing of working (and assumed
to be correct) pinmux configurations in case of a typo somewhere
later on in an unrelated pinctrl setting?
In worst case we might break pinmuxing for the UART pins...
>> + 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
Done for v4 (pr_debug()).
Also addressed the other printf() calls in the patch
and converted them to pr_warn().
Thank's a lot for the review,
Christoph
>
>> + 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