Hi David, On 12/27/18 1:49 PM, David Wu wrote: > Hi Christoph, > > I once submitted a series of patches that they can support all Socs' > Pinctrl and how do you feel about using them.
Thank's for pointing to that. Your driver looks good, but I don't like the huge amount of duplication in it (you have a function rkNNNN_calc_pull_reg_and_bit() for each SoC variant, which more or less do all the same). Also I prefer to have a generic core driver and SoC specific parts in their own C files (to have a slim driver for each SoC, but a maximum of code reuse). Also Kconfig entries like PINCTRL_ROCKCHIP_RK3188 don't seem to do anything in your driver. Since this is from Feb 2018: May I ask, why you did not continue to bring that mainline? My plan is to get the driver in for RK3399 asap and enable it only for the RK3399-Q7 board for now (to not mess with other boards). During the next merge window I want to move the generic parts into their own C files. Other SoC-specific drivers can follow then with their own mini-drivers (no code just configuration). Thanks, Christoph > > http://patchwork.ozlabs.org/patch/868849/ > > 在 2018/12/27 上午9:11, Kever Yang 写道: >> >> Add David to review the pinctrl driver. >> >> Thanks, >> - Kever >> On 12/17/2018 09:30 PM, Christoph Muellner 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. >>> + */ >>> +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) >>> +{ >>> + /* >>> + * 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. >>> + * >>> + * @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. */ >>> + *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; >>> + >>> + 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++) { >>> + 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"); >>> + 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; >>> } >> >> >> >> > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
