Hi Simon, On 12/20/18 10:17 PM, Simon Glass wrote: > On Mon, 17 Dec 2018 at 06:30, Christoph Muellner > <christoph.muell...@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.muell...@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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot