[U-Boot] [PATCH v3 0/8] rk3399-puma: Enable PWM regulator for RK3399-Q7

[U-Boot] [PATCH v3 0/8] rk3399-puma: Enable PWM regulator for RK3399-Q7

[Christoph Muellner]

This patch series allows to tune VDD_LOG on RK3399-Q7 Puma boards
to a voltage level defined in the DTS using a PWM adjustable regulator.

To do so a reimplemenation of the RK3399 pinctrl driver has been done,
which uses .set_state() instead of .set_state_simple().
This means, that the pinctrl driver operates based on the pinctrl settings
in the DTB (contrary to the current implemenation, which has these
settings hard-coded).

Although the new pinctrl driver is written in a way, that we could
factor it out into a generic rockchip pinctrl driver and have SoC-specific
mini drivers, this is not part of this series. This is planned to be
done for at least RK3399 in the next merge window.

The effect of the series is, that VDD_LOG will be set to about 950 mV
on Puma. This is required to address stability issues on Puma.

This series has no intended impact on any board except the RK3399-Q7.

Changes in v3:
 - Fix message verbosity in pinctrl driver.
 - Changed patches according to review feedback.
 - Add patches to enable the full pinctrl driver only for Puma boards.

Changes in v2:
 - Changed patches according to review feedback.
 - Fix pinctrl infrastructure instead of hacking board_init() code.

Christoph Muellner (8):
  rockchip: rk3399-puma: Cleanup of vdd_log DTS entry.
  power: regulator: Allow PWM regulator to be omitted from SPL.
  rockchip: rk3399-puma: enable PWM regulator in Puma defconfig.
  dm: pinctrl: Add pinctrl_decode_pin_config_dm().
  rockchip: rk3399: Add improved pinctrl driver.
  rockchip: rk3399: Add Kconfig option for full pinctrl driver
  rockchip: rk3399-puma: enable full pinctrl driver in Puma defconfig.
  rockchip: rk3399-puma: Set VDD_LOG to 950 mV.

 arch/arm/dts/rk3399-puma.dtsi             |   5 +-
 configs/puma-rk3399_defconfig             |   2 +
 drivers/pinctrl/Kconfig                   |  10 ++
 drivers/pinctrl/pinctrl-uclass.c          |  15 ++
 drivers/pinctrl/rockchip/pinctrl_rk3399.c | 235 ++++++++++++++++++++++++++++++
 drivers/power/regulator/Kconfig           |   7 +
 drivers/power/regulator/Makefile          |   2 +-
 include/dm/pinctrl.h                      |  12 ++
 8 files changed, 283 insertions(+), 5 deletions(-)

-- 
2.11.0

[U-Boot] [PATCH v3 1/8] rockchip: rk3399-puma: Cleanup of vdd_log DTS entry.

[Christoph Muellner]

This patch eliminates the non-standard entries "rockchip,pwm_id"
and "rockchip,pwm_voltage". They are neither documented nor
read out by any driver.

Additionally it introduces the entry regulator-init-microvolt
and sets it to 900 mV, which is the default target value
for VDD_LOG.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---

Changes in v3: None
Changes in v2: None

 arch/arm/dts/rk3399-puma.dtsi | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm/dts/rk3399-puma.dtsi b/arch/arm/dts/rk3399-puma.dtsi
index 9a61fbb453..09f7992f65 100644
--- a/arch/arm/dts/rk3399-puma.dtsi
+++ b/arch/arm/dts/rk3399-puma.dtsi
@@ -172,10 +172,7 @@
 		regulator-max-microvolt = <1400000>;
 		regulator-always-on;
 		regulator-boot-on;
-
-		/* for rockchip boot on */
-		rockchip,pwm_id= <2>;
-		rockchip,pwm_voltage = <1000000>;
+		regulator-init-microvolt = <900000>;
 	};
 };
 
-- 
2.11.0

[U-Boot] [PATCH v3 2/8] power: regulator: Allow PWM regulator to be omitted from SPL.

[Christoph Muellner]

This patch allows to enable the PWM regulator driver
independent for U-Boot and SPL.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---

Changes in v3: None
Changes in v2: None

 drivers/power/regulator/Kconfig  | 7 +++++++
 drivers/power/regulator/Makefile | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
index 09b311de8b..3ed0dd2264 100644
--- a/drivers/power/regulator/Kconfig
+++ b/drivers/power/regulator/Kconfig
@@ -61,6 +61,13 @@ config REGULATOR_PWM
 	This driver is controlled by a device tree node
 	which includes voltage limits.
 
+config SPL_REGULATOR_PWM
+	bool "Enable Driver for PWM regulators in SPL"
+	depends on REGULATOR_PWM
+	help
+	  This config enables implementation of driver-model regulator uclass
+	  features for PWM regulators in SPL.
+
 config DM_REGULATOR_MAX77686
 	bool "Enable Driver Model for REGULATOR MAX77686"
 	depends on DM_REGULATOR && DM_PMIC_MAX77686
diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
index 8017045d54..f617ce723a 100644
--- a/drivers/power/regulator/Makefile
+++ b/drivers/power/regulator/Makefile
@@ -9,7 +9,7 @@ obj-$(CONFIG_REGULATOR_ACT8846) += act8846.o
 obj-$(CONFIG_REGULATOR_AS3722)	+= as3722_regulator.o
 obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o
 obj-$(CONFIG_$(SPL_)DM_PMIC_PFUZE100) += pfuze100.o
-obj-$(CONFIG_REGULATOR_PWM) += pwm_regulator.o
+obj-$(CONFIG_$(SPL_)REGULATOR_PWM) += pwm_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_GPIO) += gpio-regulator.o
-- 
2.11.0

[U-Boot] [PATCH v3 3/8] rockchip: rk3399-puma: enable PWM regulator in Puma defconfig.

[Christoph Muellner]

This patch enables the PWM regulator driver in the defconfig
for the RK3399-Q7.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---

Changes in v3: None
Changes in v2: None

 configs/puma-rk3399_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig
index a45a34be31..1afa5a75f9 100644
--- a/configs/puma-rk3399_defconfig
+++ b/configs/puma-rk3399_defconfig
@@ -79,6 +79,7 @@ CONFIG_DM_PMIC=y
 CONFIG_DM_PMIC_FAN53555=y
 CONFIG_PMIC_RK8XX=y
 CONFIG_SPL_DM_REGULATOR=y
+CONFIG_REGULATOR_PWM=y
 CONFIG_DM_REGULATOR_FIXED=y
 CONFIG_SPL_DM_REGULATOR_FIXED=y
 CONFIG_DM_REGULATOR_GPIO=y
-- 
2.11.0

[U-Boot] [PATCH v3 4/8] dm: pinctrl: Add pinctrl_decode_pin_config_dm().

[Christoph Muellner]

pinctrl_decode_pin_config_dm() is basically a feature-equivalent
implementation of pinctrl_decode_pin_config(), which operates
on struct udevice devices and uses the dev_read_*() API.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---

Changes in v3: None
Changes in v2: None

 drivers/pinctrl/pinctrl-uclass.c | 15 +++++++++++++++
 include/dm/pinctrl.h             | 12 ++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
index 6db0445067..cebba12b4a 100644
--- a/drivers/pinctrl/pinctrl-uclass.c
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -27,6 +27,21 @@ int pinctrl_decode_pin_config(const void *blob, int node)
 	return flags;
 }
 
+int pinctrl_decode_pin_config_dm(struct udevice *dev)
+{
+	int pinconfig = 0;
+
+	if (dev->uclass->uc_drv->id != UCLASS_PINCONFIG)
+		return -EINVAL;
+
+	if (dev_read_bool(dev, "bias-pull-up"))
+		pinconfig |= 1 << PIN_CONFIG_BIAS_PULL_UP;
+	else if (dev_read_bool(dev, "bias-pull-down"))
+		pinconfig |= 1 << PIN_CONFIG_BIAS_PULL_DOWN;
+
+	return pinconfig;
+}
+
 #if CONFIG_IS_ENABLED(PINCTRL_FULL)
 /**
  * pinctrl_config_one() - apply pinctrl settings for a single node
diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
index 63a7d55b88..ff2b82e7c2 100644
--- a/include/dm/pinctrl.h
+++ b/include/dm/pinctrl.h
@@ -355,6 +355,18 @@ int pinctrl_get_periph_id(struct udevice *dev, struct udevice *periph);
 int pinctrl_decode_pin_config(const void *blob, int node);
 
 /**
+ * pinctrl_decode_pin_config_dm() - decode pin configuration flags
+ *
+ * This decodes some of the PIN_CONFIG values into flags, with each value
+ * being (1 << pin_cfg). This does not support things with values like the
+ * slew rate.
+ *
+ * @pinconfig:	Pinconfig udevice
+ * @return decoded flag value, or -ve on error
+ */
+int pinctrl_decode_pin_config_dm(struct udevice *dev);
+
+/**
  * pinctrl_get_gpio_mux() - get the mux value for a particular GPIO
  *
  * This allows the raw mux value for a GPIO to be obtained. It is
-- 
2.11.0

[U-Boot] [PATCH v3 5/8] rockchip: rk3399: Add improved pinctrl driver.

[Christoph Muellner]

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.
+ */
+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;
 }
-- 
2.11.0

[U-Boot] [PATCH v3 6/8] rockchip: rk3399: Add Kconfig option for full pinctrl driver

[Christoph Muellner]

This patch adds a Kconfig option to enable the full pinctrl driver
for the RK3399. This flag needs to be enabed in order to get the
features of the full pinctrl driver compiled in (i.e. a .set_state()
callback).

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---

Changes in v3: None
Changes in v2: None

 drivers/pinctrl/Kconfig                   | 10 ++++++++++
 drivers/pinctrl/rockchip/pinctrl_rk3399.c |  9 +++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 7e6fad305a..7ec06f08b9 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -238,6 +238,16 @@ config PINCTRL_ROCKCHIP_RK3399
 	  the GPIO definitions and pin control functions for each available
 	  multiplex function.
 
+config PINCTRL_ROCKCHIP_RK3399_FULL
+	bool "Rockchip rk3399 pin control driver (full)"
+	depends on PINCTRL_FULL && PINCTRL_ROCKCHIP_RK3399
+	help
+	  Support full pin multiplexing control on Rockchip rk3399 SoCs.
+
+	  This enables the full pinctrl driver for the RK3399.
+	  Contrary to the non-full pinctrl driver, this will evaluate
+	  the board DTB to get the pinctrl settings.
+
 config PINCTRL_ROCKCHIP_RV1108
 	bool "Rockchip rv1108 pin control driver"
 	depends on DM
diff --git a/drivers/pinctrl/rockchip/pinctrl_rk3399.c b/drivers/pinctrl/rockchip/pinctrl_rk3399.c
index ed9828989f..19c15bf46c 100644
--- a/drivers/pinctrl/rockchip/pinctrl_rk3399.c
+++ b/drivers/pinctrl/rockchip/pinctrl_rk3399.c
@@ -15,8 +15,10 @@
 #include <asm/arch/clock.h>
 #include <dm/pinctrl.h>
 
+#if CONFIG_IS_ENABLED(PINCTRL_ROCKCHIP_RK3399_FULL)
 static const u32 RK_GRF_P_PULLUP = 1;
 static const u32 RK_GRF_P_PULLDOWN = 2;
+#endif /* PINCTRL_ROCKCHIP_RK3399_FULL */
 
 struct rk3399_pinctrl_priv {
 	struct rk3399_grf_regs *grf;
@@ -24,6 +26,7 @@ struct rk3399_pinctrl_priv {
 	struct rockchip_pin_bank *banks;
 };
 
+#if CONFIG_IS_ENABLED(PINCTRL_ROCKCHIP_RK3399_FULL)
 /**
  * Location of pinctrl/pinconf registers.
  */
@@ -243,6 +246,8 @@ end:
 	return ret;
 }
 
+#endif /* PINCTRL_ROCKCHIP_RK3399_FULL */
+
 static void pinctrl_rk3399_pwm_config(struct rk3399_grf_regs *grf,
 		struct rk3399_pmugrf_regs *pmugrf, int pwm_id)
 {
@@ -692,7 +697,9 @@ static int rk3399_pinctrl_set_state_simple(struct udevice *dev,
 }
 
 static struct pinctrl_ops rk3399_pinctrl_ops = {
+#if CONFIG_IS_ENABLED(PINCTRL_ROCKCHIP_RK3399_FULL)
 	.set_state	= rk3399_pinctrl_set_state,
+#endif
 	.set_state_simple	= rk3399_pinctrl_set_state_simple,
 	.request	= rk3399_pinctrl_request,
 	.get_periph_id	= rk3399_pinctrl_get_periph_id,
@@ -706,7 +713,9 @@ 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);
+#if CONFIG_IS_ENABLED(PINCTRL_ROCKCHIP_RK3399_FULL)
 	priv->banks = rk3399_pin_banks;
+#endif /* PINCTRL_ROCKCHIP_RK3399_FULL */
 
 	return ret;
 }
-- 
2.11.0

[U-Boot] [PATCH v3 7/8] rockchip: rk3399-puma: enable full pinctrl driver in Puma defconfig.

[Christoph Muellner]

This patch enables the full pinctrl driver in the defconfig
for the RK3399-Q7.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---

Changes in v3: None
Changes in v2: None

 configs/puma-rk3399_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig
index 1afa5a75f9..3c293b69e4 100644
--- a/configs/puma-rk3399_defconfig
+++ b/configs/puma-rk3399_defconfig
@@ -75,6 +75,7 @@ CONFIG_GMAC_ROCKCHIP=y
 CONFIG_PINCTRL=y
 CONFIG_SPL_PINCTRL=y
 CONFIG_PINCTRL_ROCKCHIP_RK3399=y
+CONFIG_PINCTRL_ROCKCHIP_RK3399_FULL=y
 CONFIG_DM_PMIC=y
 CONFIG_DM_PMIC_FAN53555=y
 CONFIG_PMIC_RK8XX=y
-- 
2.11.0

[U-Boot] [PATCH v3 8/8] rockchip: rk3399-puma: Set VDD_LOG to 950 mV.

[Christoph Muellner]

This patch sets VDD_LOG to 950 mV on RK3399-Q7.
This is required to address stability issues on Puma
in heavy-load use-cases.

Reported-by: Assaf Agmon <assaf@r-go.io>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>

---

Changes in v3:
 - Fix message verbosity in pinctrl driver.
 - Changed patches according to review feedback.
 - Add patch to enable the full pinctrl driver only for Puma boards.

Changes in v2:
 - Changed patches according to review feedback.
 - Fix pinctrl infrastructure instead of hacking board_init() code.

 arch/arm/dts/rk3399-puma.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/dts/rk3399-puma.dtsi b/arch/arm/dts/rk3399-puma.dtsi
index 09f7992f65..8304f67192 100644
--- a/arch/arm/dts/rk3399-puma.dtsi
+++ b/arch/arm/dts/rk3399-puma.dtsi
@@ -172,7 +172,7 @@
 		regulator-max-microvolt = <1400000>;
 		regulator-always-on;
 		regulator-boot-on;
-		regulator-init-microvolt = <900000>;
+		regulator-init-microvolt = <950000>;
 	};
 };
 
-- 
2.11.0

[U-Boot] [PATCH v3 4/8] dm: pinctrl: Add pinctrl_decode_pin_config_dm().

[Simon Glass]

Hi Christoph,

On Mon, 17 Dec 2018 at 06:30, Christoph Muellner
<christoph.muellner@theobroma-systems.com> wrote:
>
> pinctrl_decode_pin_config_dm() is basically a feature-equivalent
> implementation of pinctrl_decode_pin_config(), which operates
> on struct udevice devices and uses the dev_read_*() API.

So is it possible to remove the old function and avoid the _dm suffix here?
>

> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/pinctrl/pinctrl-uclass.c | 15 +++++++++++++++
>  include/dm/pinctrl.h             | 12 ++++++++++++
>  2 files changed, 27 insertions(+)

Regards,
Simon

[U-Boot] [PATCH v3 5/8] rockchip: rk3399: Add improved pinctrl driver.

[Simon Glass]

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

> +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.

> + */
> +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?

> +        *
> +        * @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

> +       *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.

> +
> +       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.

> +               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

> +                       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] [PATCH v3 4/8] dm: pinctrl: Add pinctrl_decode_pin_config_dm().

[Philipp Tomsich]

Simon,

> On 20.12.2018, at 22:16, Simon Glass <sjg@chromium.org> wrote:
> 
> Hi Christoph,
> 
> On Mon, 17 Dec 2018 at 06:30, Christoph Muellner
> <christoph.muellner@theobroma-systems.com> wrote:
>> 
>> pinctrl_decode_pin_config_dm() is basically a feature-equivalent
>> implementation of pinctrl_decode_pin_config(), which operates
>> on struct udevice devices and uses the dev_read_*() API.
> 
> So is it possible to remove the old function and avoid the _dm suffix here?

Assuming we get this sorted this week or over the weekend: would it be ok
with you if I pull this into the current cycle?

I would like to see the vdd_log fix for the RK3399-Q7 merged into 2019.1
before we go to the longer release cycles (as this will have to wait until 2019.4
otherwise) … especially, as my own insistence of doing it the right way
(i.e. using autoset for the regulator) has delayed this since before the 
merge-window closed.

Thanks,
Philipp.

[U-Boot] [PATCH v3 4/8] dm: pinctrl: Add pinctrl_decode_pin_config_dm().

[Simon Glass]

Hi Philipp,

On Thu, 20 Dec 2018 at 14:22, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
>
> Simon,
>
> > On 20.12.2018, at 22:16, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Christoph,
> >
> > On Mon, 17 Dec 2018 at 06:30, Christoph Muellner
> > <christoph.muellner@theobroma-systems.com> wrote:
> >>
> >> pinctrl_decode_pin_config_dm() is basically a feature-equivalent
> >> implementation of pinctrl_decode_pin_config(), which operates
> >> on struct udevice devices and uses the dev_read_*() API.
> >
> > So is it possible to remove the old function and avoid the _dm suffix here?
>
> Assuming we get this sorted this week or over the weekend: would it be ok
> with you if I pull this into the current cycle?

Yes, fine with me.

>
> I would like to see the vdd_log fix for the RK3399-Q7 merged into 2019.1
> before we go to the longer release cycles (as this will have to wait until 2019.4
> otherwise) … especially, as my own insistence of doing it the right way
> (i.e. using autoset for the regulator) has delayed this since before the
> merge-window closed.

Yes, I am not looking forward to the longer cycles.

>
> Thanks,
> Philipp.
>

Regards,
Simon

[U-Boot] [PATCH v3 4/8] dm: pinctrl: Add pinctrl_decode_pin_config_dm().

[Christoph Müllner]

On 12/20/18 10:16 PM, Simon Glass wrote:
> Hi Christoph,
> 
> On Mon, 17 Dec 2018 at 06:30, Christoph Muellner
> <christoph.muellner@theobroma-systems.com> wrote:
>>
>> pinctrl_decode_pin_config_dm() is basically a feature-equivalent
>> implementation of pinctrl_decode_pin_config(), which operates
>> on struct udevice devices and uses the dev_read_*() API.
> 
> So is it possible to remove the old function and avoid the _dm suffix here?

Seems like the right way to go for me.
But I don't have the option to test on devices, which
need this function (RK3188 and RK3288) and I don't want
to risk touching drivers, which I cannot test outside
of the regular merge window.

My plan was to do this early during the next merge window
(together with dropping the old RK3399 pinctrl driver and
the refactoring of the RK3399 pinctrl driver into a generic
RK pinctrl driver and a RK3399 mini-driver).

Acceptable?

Thank's,
Christoph


>>
> 
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>  drivers/pinctrl/pinctrl-uclass.c | 15 +++++++++++++++
>>  include/dm/pinctrl.h             | 12 ++++++++++++
>>  2 files changed, 27 insertions(+)
> 
> Regards,
> Simon
> 

[U-Boot] [PATCH v3 5/8] rockchip: rk3399: Add improved pinctrl driver.

[Christoph Müllner]

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 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] [PATCH v3 5/8] rockchip: rk3399: Add improved pinctrl driver.

[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.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.
> + */
> +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] [PATCH v3 5/8] rockchip: rk3399: Add improved pinctrl driver.

[David Wu]

Hi Christoph,

I once submitted a series of patches that they can support all Socs' 
Pinctrl and how do you feel about using them.

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.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.
>> + */
>> +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] [PATCH v3 5/8] rockchip: rk3399: Add improved pinctrl driver.

[Christoph Müllner]

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.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.
>>> + */
>>> +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] [PATCH v3 5/8] rockchip: rk3399: Add improved pinctrl driver.

[Philipp Tomsich]

David,

> On 27.12.2018, at 13:49, David Wu <david.wu@rock-chips.com> 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.
> 
> http://patchwork.ozlabs.org/patch/868849/

Which reminds me that I am still waiting for a newer revision that addresses the various comments (e.g. breaking up into a driver and mini-drivers/driver-data, not including support for yet-unsupported SoCs, etc.)...

Are you planning to do an updatted and rebased version in the near future?

Thanks,
Philipp.

[U-Boot] [PATCH v3 5/8] rockchip: rk3399: Add improved pinctrl driver.

[David Wu]

Hi Christoph,

This patch seems is less of code about drive strength, for some modules, 
like LCD, Ethernet is still needed.

在 2018/12/27 下午9:13, Christoph Müllner 写道:
> 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?

It seems i have lost them in my mailbox. I'm going to update a new 
version in the near future.

> 
> 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.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.
>>>> + */
>>>> +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] [PATCH v3 5/8] rockchip: rk3399: Add improved pinctrl driver.

[David Wu]

Hi Philipp,

在 2018/12/27 下午10:31, Philipp Tomsich 写道:
> David,
> 
>> On 27.12.2018, at 13:49, David Wu <david.wu@rock-chips.com> 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.
>>
>> http://patchwork.ozlabs.org/patch/868849/
> 
> Which reminds me that I am still waiting for a newer revision that addresses the various comments (e.g. breaking up into a driver and mini-drivers/driver-data, not including support for yet-unsupported SoCs, etc.)...
> 
> Are you planning to do an updatted and rebased version in the near future?

Yes, i will pick them up, and update.

> 
> Thanks,
> Philipp.
> 

[U-Boot] [PATCH v3 4/8] dm: pinctrl: Add pinctrl_decode_pin_config_dm().

[Simon Glass]

Hi Christoph,

On Tue, 25 Dec 2018 at 14:49, Christoph Müllner
<christoph.muellner@theobroma-systems.com> wrote:
>
> On 12/20/18 10:16 PM, Simon Glass wrote:
> > Hi Christoph,
> >
> > On Mon, 17 Dec 2018 at 06:30, Christoph Muellner
> > <christoph.muellner@theobroma-systems.com> wrote:
> >>
> >> pinctrl_decode_pin_config_dm() is basically a feature-equivalent
> >> implementation of pinctrl_decode_pin_config(), which operates
> >> on struct udevice devices and uses the dev_read_*() API.
> >
> > So is it possible to remove the old function and avoid the _dm suffix here?
>
> Seems like the right way to go for me.
> But I don't have the option to test on devices, which
> need this function (RK3188 and RK3288) and I don't want
> to risk touching drivers, which I cannot test outside
> of the regular merge window.
>
> My plan was to do this early during the next merge window
> (together with dropping the old RK3399 pinctrl driver and
> the refactoring of the RK3399 pinctrl driver into a generic
> RK pinctrl driver and a RK3399 mini-driver).
>
> Acceptable?

Yes that's fine with me. Could you add a comment saying that this
function is temporary for this release and will be removed in patches
by February?

Regards,
Simon