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

[U-Boot] [PATCH v2 0/6] 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.
Although the new pinctrl driver is written in a way, that we could
merge it with other rockchip pinctrl drivers, this is has not been done
as part of this series.

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.
As a side effect, the pinctrl settings of all RK3399 boards will
be configured according to the description in the DTS.

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

Christoph Muellner (6):
  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-puma: Set VDD_LOG to 950 mV.

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

-- 
2.11.0

[U-Boot] [PATCH v2 1/6] 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 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 9a61fbb4536..09f7992f65c 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 v2 2/6] 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 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 09b311de8bb..3ed0dd2264a 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 8017045d542..f617ce723a9 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 v2 3/6] 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 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 a45a34be31c..1afa5a75f9a 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 v2 4/6] 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 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 6db04450670..cebba12b4a4 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 63a7d55b888..ff2b82e7c25 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 v2 5/6] 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 v2: None

 drivers/pinctrl/rockchip/pinctrl_rk3399.c | 189 ++++++++++++++++++++++++++++++
 1 file changed, 189 insertions(+)

diff --git a/drivers/pinctrl/rockchip/pinctrl_rk3399.c b/drivers/pinctrl/rockchip/pinctrl_rk3399.c
index bc92dd7c062..b6651c92d11 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,197 @@
 #include <asm/arch/clock.h>
 #include <dm/pinctrl.h>
 
+#define RK_PINCONFIG_PULLUP 1
+#define RK_PINCONFIG_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)
+{
+	*addr = base + (index / 8) * 4;
+	*shift = index % 8;
+	*shift *= 2;
+	*mask = 3;
+
+	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 + (uintptr_t)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;
+
+	if (pinconfig & (1 << PIN_CONFIG_BIAS_PULL_UP))
+		pupdval = RK_PINCONFIG_PULLUP;
+	else if (pinconfig & (1 << PIN_CONFIG_BIAS_PULL_DOWN))
+		pupdval = RK_PINCONFIG_PULLDOWN;
+	else
+		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)
+{
+	u32 *fields = NULL;
+	const int fields_per_pin = 4;
+	int num_fields, num_pins;
+	int ret;
+	int size;
+	int i;
+
+	pr_err("%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)
+		goto end;
+
+	for (i = 0; i < num_pins; i++) {
+		u32 banknum = fields[i * fields_per_pin];
+		u32 index = fields[i * fields_per_pin + 1];
+		u32 muxval = fields[i * fields_per_pin + 2];
+		u32 phandle = fields[i * fields_per_pin + 3];
+		struct udevice *dev_pinconfig;
+		int pinconfig;
+
+		ret = uclass_get_device_by_phandle_id(UCLASS_PINCONFIG, 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, banknum, index, 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 +655,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 +669,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 v2 6/6] 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 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 09f7992f65c..8304f67192c 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 v2 1/6] rockchip: rk3399-puma: Cleanup of vdd_log DTS entry.

[Philipp Tomsich]

> On 12.12.2018, at 01:57, Christoph Muellner <christoph.muellner@theobroma-systems.com> wrote:
> 
> 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>

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

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

[Philipp Tomsich]

Christoph,

> On 12.12.2018, at 01:57, Christoph Muellner <christoph.muellner@theobroma-systems.com> wrote:
> 
> 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.
> Although the new pinctrl driver is written in a way, that we could
> merge it with other rockchip pinctrl drivers, this is has not been done
> as part of this series.

As discussed today, when I came by your desk: thanks for taking this
on; it had been on my wish-list for a long time.

Given the fact that this may have unintended side-effects for some of
the other RK3399 boards, I am considering to hold this back until the
next merge window (even though I know that we’d like to change the
vdd_log setting as soon as possible to properly support the one use-case
that triggered this entire effort).

> 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.
> As a side effect, the pinctrl settings of all RK3399 boards will
> be configured according to the description in the DTS.

I wonder if we should add a transitional Kconfig entry to enable this only
for boards that opt-in for the current release… this would allow us to pull
this in for 2019.1 and then make it an unconditional feature starting early
in the next merge-window.

Philipp.

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

[Philipp Tomsich]

> On 12.12.2018, at 01:57, Christoph Muellner <christoph.muellner@theobroma-systems.com> wrote:
> 
> This patch enables the PWM regulator driver in the defconfig
> for the RK3399-Q7.
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

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

[Philipp Tomsich]

> On 12.12.2018, at 01:57, 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).

Given that the original version was sent before the close of the MW and that
and that I want to give the maximum possible time to others to evaluate and
fix their DTS (if required), please do the following (in a seperate patch)
(a)	make this conditional to a transitory Kconfig option 
(b)	select this for our boards, as needed
to ensure that the behaviour doesn’t get picked up unknowingly.

I’ll then revert this additional patch as one of the first changes in the next
merge-window to make this common behaviour.

> This patch has been tested on a RK3399-Q7 SoM (Puma).
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

See below for requested changes (and also above).

> ---
> 
> Changes in v2: None
> 
> drivers/pinctrl/rockchip/pinctrl_rk3399.c | 189 ++++++++++++++++++++++++++++++
> 1 file changed, 189 insertions(+)
> 
> diff --git a/drivers/pinctrl/rockchip/pinctrl_rk3399.c b/drivers/pinctrl/rockchip/pinctrl_rk3399.c
> index bc92dd7c062..b6651c92d11 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,197 @@
> #include <asm/arch/clock.h>
> #include <dm/pinctrl.h>
> 
> +#define RK_PINCONFIG_PULLUP 1
> +#define RK_PINCONFIG_PULLDOWN 2

Please make these 'const u32’ or use an enum.
I prefer the const u32, whereas Simon prefers the enum.

> +
> 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)
> +{
> +	*addr = base + (index / 8) * 4;

Shudder. Can you write this much more cleanly.

> +	*shift = index % 8;

I don’t like % (as you know). Please use &.

> +	*shift *= 2;
> +	*mask = 3;

Generally, I don’t like this calculation, as it’s not obvious what happens and
why this works.  Please comment and rewrite in a more readable way.

> +
> +	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 + (uintptr_t)bank->iomux_offset;

The case of uintptr_t seems unnecessary. AFAIR, a uintptr_t + size_t
should be a well-defined combination… but I may remember wrong
and couldn’t be bothered to read my C99 standard.

> +	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;
> +
> +	if (pinconfig & (1 << PIN_CONFIG_BIAS_PULL_UP))

Is this defined in one of the header-files included in the DTSI?
This isn’t exactly intuitive when just reading it.

> +		pupdval = RK_PINCONFIG_PULLUP;
> +	else if (pinconfig & (1 << PIN_CONFIG_BIAS_PULL_DOWN))
> +		pupdval = RK_PINCONFIG_PULLDOWN;
> +	else
> +		return;

No error in the case where we don’t know what to do?

> +
> +	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)
> +{
> +	u32 *fields = NULL;
> +	const int fields_per_pin = 4;
> +	int num_fields, num_pins;
> +	int ret;
> +	int size;
> +	int i;
> +
> +	pr_err("%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)
> +		goto end;
> +
> +	for (i = 0; i < num_pins; i++) {
> +		u32 banknum = fields[i * fields_per_pin];
> +		u32 index = fields[i * fields_per_pin + 1];
> +		u32 muxval = fields[i * fields_per_pin + 2];
> +		u32 phandle = fields[i * fields_per_pin + 3];

I’d prefer a struct foo { u32 bank; u32 idx; u32 mux; u32 phandle }; and then
a cast of fields to struct foo, so we can do something along the line of
fields_as_struct[i].bank.

> +		struct udevice *dev_pinconfig;
> +		int pinconfig;
> +
> +		ret = uclass_get_device_by_phandle_id(UCLASS_PINCONFIG, phandle,
> +						      &dev_pinconfig);

I wonder if it would be more efficient to iterate over the fields-array multiple
times for each different phandle (IIRC, uclass_get_device_by_phandle_id
can be inefficient).

Then again, it probably doesn’t matter...

> +		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, banknum, index, 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 +655,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 +669,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 v2 4/6] dm: pinctrl: Add pinctrl_decode_pin_config_dm().

[Philipp Tomsich]

Christoph,

> On 12.12.2018, at 01:57, 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.

Can’t we use pinctrl_generic_set_state and then simply pass a 
struct pinconf_param?  I am not convinced that we need to have
yet-another mechanism in addition to what’s already there...

Thanks,
Philipp.

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

[Christoph Müllner]

> On 13.12.2018, at 23:33, Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
>> On 12.12.2018, at 01:57, 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.
> 
> Can’t we use pinctrl_generic_set_state and then simply pass a 
> struct pinconf_param?  I am not convinced that we need to have
> yet-another mechanism in addition to what’s already there...

Hi Philipp,

the rockchip pinctrl syntax differs from the generic one:
doc/device-tree-bindings/pinctrl/pinctrl-bindings.txt
doc/device-tree-bindings/pinctrl/rockchip,pinctrl.txt

Therefore we can't simply use pinctrl_generic_set_state().

Instead we need to parse the "rockchip,pins" property and
evaluate banknum, index, muxval, and the pinconf phandle.
Afterwards we need to extract the pinconf settings (i.e. iterate
over pinconf properties and do strcmp()).

We could in theory reuse pinconf_prop_name_to_param() from
the generic driver, but that would mean, that we need two
pinctrl drivers enabled in our config. Also using the generic driver's static functions
is not possible. We either have to make them non-static or call the code via
pinctrl_generic_set_state().

I could move pinctrl_decode_pin_config_dm() into
pinctrl_rk3399.c, but I thought it is valuable enough
to be shared with others.

In other words:
I think there are enough good reasons to keep the patch like it is.

BR
Christoph

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

[Christoph Müllner]

> On 13.12.2018, at 23:26, Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
> 
>> On 12.12.2018, at 01:57, 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).
> 
> Given that the original version was sent before the close of the MW and that
> and that I want to give the maximum possible time to others to evaluate and
> fix their DTS (if required), please do the following (in a seperate patch)
> (a)	make this conditional to a transitory Kconfig option 
> (b)	select this for our boards, as needed
> to ensure that the behaviour doesn’t get picked up unknowingly.
> 
> I’ll then revert this additional patch as one of the first changes in the next
> merge-window to make this common behaviour.

Will do.

> 
>> This patch has been tested on a RK3399-Q7 SoM (Puma).
>> 
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> 
> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> 
> See below for requested changes (and also above).
> 
>> ---
>> 
>> Changes in v2: None
>> 
>> drivers/pinctrl/rockchip/pinctrl_rk3399.c | 189 ++++++++++++++++++++++++++++++
>> 1 file changed, 189 insertions(+)
>> 
>> diff --git a/drivers/pinctrl/rockchip/pinctrl_rk3399.c b/drivers/pinctrl/rockchip/pinctrl_rk3399.c
>> index bc92dd7c062..b6651c92d11 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,197 @@
>> #include <asm/arch/clock.h>
>> #include <dm/pinctrl.h>
>> 
>> +#define RK_PINCONFIG_PULLUP 1
>> +#define RK_PINCONFIG_PULLDOWN 2
> 
> Please make these 'const u32’ or use an enum.
> I prefer the const u32, whereas Simon prefers the enum.

Will do.

> 
>> +
>> 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)
>> +{
>> +	*addr = base + (index / 8) * 4;
> 
> Shudder. Can you write this much more cleanly.
> 
>> +	*shift = index % 8;
> 
> I don’t like % (as you know). Please use &.
> 
>> +	*shift *= 2;
>> +	*mask = 3;
> 
> Generally, I don’t like this calculation, as it’s not obvious what happens and
> why this works.  Please comment and rewrite in a more readable way.

Will do.

> 
>> +
>> +	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 + (uintptr_t)bank->iomux_offset;
> 
> The case of uintptr_t seems unnecessary. AFAIR, a uintptr_t + size_t
> should be a well-defined combination… but I may remember wrong
> and couldn’t be bothered to read my C99 standard.

Correct.
The cast was actually a left-over from an earlier patch, where iomux_offset was a void*.
And I changed that to uintptr_t to get rid of exactly this cast...

> 
>> +	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;
>> +
>> +	if (pinconfig & (1 << PIN_CONFIG_BIAS_PULL_UP))
> 
> Is this defined in one of the header-files included in the DTSI?
> This isn’t exactly intuitive when just reading it.

PIN_CONFIG_* is defined in include/dm/pinctrl.h.
Unfortunately just the enum values and not the shifted values.

>> +		pupdval = RK_PINCONFIG_PULLUP;
>> +	else if (pinconfig & (1 << PIN_CONFIG_BIAS_PULL_DOWN))
>> +		pupdval = RK_PINCONFIG_PULLDOWN;
>> +	else
>> +		return;
> 
> No error in the case where we don’t know what to do?

We just set, what is defined in the DTS.
If nothing is defined, we leave it as it was (most likely default values apply).

I've moved the if() statement up and added a comment above.


> 
>> +
>> +	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)
>> +{
>> +	u32 *fields = NULL;
>> +	const int fields_per_pin = 4;
>> +	int num_fields, num_pins;
>> +	int ret;
>> +	int size;
>> +	int i;
>> +
>> +	pr_err("%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)
>> +		goto end;
>> +
>> +	for (i = 0; i < num_pins; i++) {
>> +		u32 banknum = fields[i * fields_per_pin];
>> +		u32 index = fields[i * fields_per_pin + 1];
>> +		u32 muxval = fields[i * fields_per_pin + 2];
>> +		u32 phandle = fields[i * fields_per_pin + 3];
> 
> I’d prefer a struct foo { u32 bank; u32 idx; u32 mux; u32 phandle }; and then
> a cast of fields to struct foo, so we can do something along the line of
> fields_as_struct[i].bank.

Will do.

Thanks,
Christoph



>> +		struct udevice *dev_pinconfig;
>> +		int pinconfig;
>> +
>> +		ret = uclass_get_device_by_phandle_id(UCLASS_PINCONFIG, phandle,
>> +						      &dev_pinconfig);
> 
> I wonder if it would be more efficient to iterate over the fields-array multiple
> times for each different phandle (IIRC, uclass_get_device_by_phandle_id
> can be inefficient).
> 
> Then again, it probably doesn’t matter...
> 
>> +		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, banknum, index, 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 +655,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 +669,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 v2 2/6] power: regulator: Allow PWM regulator to be omitted from SPL.

[Simon Glass]

On Tue, 11 Dec 2018 at 17:58, Christoph Muellner
<christoph.muellner@theobroma-systems.com> wrote:
>
> 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 v2: None
>
>  drivers/power/regulator/Kconfig  | 7 +++++++
>  drivers/power/regulator/Makefile | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

On Tue, 11 Dec 2018 at 17:58, Christoph Muellner
<christoph.muellner@theobroma-systems.com> wrote:
>
> 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 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(-)

Reviewed-by: Simon Glass <sjg@chromium.org>