Hi Simon,
On 12/20/18 10:17 PM, Simon Glass wrote:
On Mon, 17 Dec 2018 at 06:30, Christoph Muellner christoph.muellner@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@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 bitsDo 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;elsereturn -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