[RFC/PATCH 0/8] pinctrl-rockchip: Change wrong initial assumptions

[RFC/PATCH 0/8] pinctrl-rockchip: Change wrong initial assumptions

[Heiko Stübner]

>From the "wet behind the ears" files:

Initially due to lack of documentation and (personal) understanding
I assumed that the area holding the iomux settings would be separate
from everything else, while in fact the grf registers contain not only
pinctrl stuff but also dma, usb-phy and general soc-status settings.
Also things like drive-strength we do not support currently are intermixed.

The same is true for the pmu, which does not only contain power domains
but also the system reset as well as well as general registers surviving
system-resets. Additionally the rk3188 moved parts of the pull-setting
registers into the pmu space.

While this wasn't a problem until now, the upcoming rk3288 introduces
additional changes to both the grf and pmu areas. On it even part of
the pinmux registers move into the pmu space.

For this my current gut-feeling is, that providing both the grf and pmu
as syscons to the pinctrl driver might be more future proof for the next
socs. But as I'm not sure on this, I'd like of course comments :-)

The code in it's current form supports both the old as well as the
changed binding.

The other option would be to leave the grf as it is and create separate
syscons for real small individual parts like the soc-conf and usb-phys.
But apart from creating these real small syscons that would
also make it necessary to introduce another register map for the
drive-strength settings of the pin-controller, which are sitting in the
middle of everything at least on rk3066 and rk3188.


So I'd really like comments here :-)


@Max: sorry to come up with this now, but I feel this should be resolved
(in whatever direction) before we introduce any grf syscon. Because due
to dt being an API we will be tied for a long time to it.


Heiko Stuebner (8):
  pinctrl: rockchip: do not require 2nd register area
  pinctrl: rockchip: use regmaps instead of raw mappings
  pinctrl: rockchip: rockchip_pinctrl in rockchip_get_bank_data
  pinctrl: rockchip: let pmu registers be supplied by a syscon
  pinctrl: rockchip: only map bank0-pull-region when pmu regmap missing
  pinctrl: rockchip: base regmap supplied by a syscon
  dt-bindings: adapt rockchip-pinctrl documentation to changed bindings
  ARM: dts: rockchip: convert pinctrl nodes to new bindings

 .../bindings/pinctrl/rockchip,pinctrl.txt          |  28 +++-
 arch/arm/boot/dts/rk3066a.dtsi                     |   2 +-
 arch/arm/boot/dts/rk3188.dtsi                      |   9 +-
 arch/arm/boot/dts/rk3xxx.dtsi                      |   9 +-
 drivers/pinctrl/pinctrl-rockchip.c                 | 178 +++++++++++++++------
 5 files changed, 164 insertions(+), 62 deletions(-)

-- 
1.9.0


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/8] pinctrl: rockchip: do not require 2nd register area

[Heiko Stübner]

Deprecate secondary register area for rk3188 pulls. Instead use big enough
initial mapping of grf registers to catch all.

The now deprecated register is still supported though.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 .../devicetree/bindings/pinctrl/rockchip,pinctrl.txt      |  2 ++
 drivers/pinctrl/pinctrl-rockchip.c                        | 15 ++++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
index f378d34..78dafc9 100644
--- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
@@ -22,6 +22,8 @@ Required properties for iomux controller:
   - compatible: one of "rockchip,rk2928-pinctrl", "rockchip,rk3066a-pinctrl"
 		       "rockchip,rk3066b-pinctrl", "rockchip,rk3188-pinctrl"
   - reg: first element is the general register space of the iomux controller
+	 It should be large enough to contain also separate pull registers.
+	 Deprecated:
 	 second element is the separate pull register space of the rk3188
 
 Required properties for gpio sub nodes:
diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 2e198a4..ab71de8 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -160,6 +160,7 @@ struct rockchip_pmx_func {
 
 struct rockchip_pinctrl {
 	void __iomem			*reg_base;
+	int				reg_size;
 	void __iomem			*reg_pull;
 	struct device			*dev;
 	struct rockchip_pin_ctrl	*ctrl;
@@ -416,6 +417,7 @@ static void rk2928_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
 	*bit = pin_num % RK2928_PULL_PINS_PER_REG;
 };
 
+#define RK3188_PULL_OFFSET		0x164
 #define RK3188_PULL_BITS_PER_PIN	2
 #define RK3188_PULL_PINS_PER_REG	8
 #define RK3188_PULL_BANK_STRIDE		16
@@ -432,7 +434,10 @@ static void rk3188_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
 		*bit = pin_num % RK3188_PULL_PINS_PER_REG;
 		*bit *= RK3188_PULL_BITS_PER_PIN;
 	} else {
-		*reg = info->reg_pull - 4;
+		*reg = info->reg_pull ? info->reg_pull
+				      : info->reg_base + RK3188_PULL_OFFSET;
+		/* correct the offset, as it is the 2nd pull register */
+		*reg -= 4;
 		*reg += bank->bank_num * RK3188_PULL_BANK_STRIDE;
 		*reg += ((pin_num / RK3188_PULL_PINS_PER_REG) * 4);
 
@@ -1427,6 +1432,7 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank,
 	 */
 	if (of_device_is_compatible(bank->of_node,
 				    "rockchip,rk3188-gpio-bank0")) {
+
 		bank->bank_type = RK3188_BANK0;
 
 		if (of_address_to_resource(bank->of_node, 1, &res)) {
@@ -1525,8 +1531,11 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
 	if (IS_ERR(info->reg_base))
 		return PTR_ERR(info->reg_base);
 
-	/* The RK3188 has its pull registers in a separate place */
-	if (ctrl->type == RK3188) {
+	/* to check for the old dt-bindings */
+	info->reg_size = resource_size(res);
+
+	/* Honor the old binding, with pull registers as 2nd resource */
+	if (ctrl->type == RK3188 &&  info->reg_size < 0x200) {
 		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 		info->reg_pull = devm_ioremap_resource(&pdev->dev, res);
 		if (IS_ERR(info->reg_pull))
-- 
1.9.0


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/8] pinctrl: rockchip: use regmaps instead of raw mappings

[Heiko Stübner]

This allows us to use syscons in the future.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 drivers/pinctrl/pinctrl-rockchip.c | 116 +++++++++++++++++++++++++------------
 1 file changed, 80 insertions(+), 36 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index ab71de8..71d9c99 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -37,6 +37,7 @@
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/clk.h>
+#include <linux/regmap.h>
 #include <dt-bindings/pinctrl/rockchip.h>
 
 #include "core.h"
@@ -86,7 +87,7 @@ enum rockchip_pin_bank_type {
  */
 struct rockchip_pin_bank {
 	void __iomem			*reg_base;
-	void __iomem			*reg_pull;
+	struct regmap			*regmap_pull;
 	struct clk			*clk;
 	int				irq;
 	u32				pin_base;
@@ -120,8 +121,9 @@ struct rockchip_pin_ctrl {
 	char				*label;
 	enum rockchip_pinctrl_type	type;
 	int				mux_offset;
-	void	(*pull_calc_reg)(struct rockchip_pin_bank *bank, int pin_num,
-				 void __iomem **reg, u8 *bit);
+	void	(*pull_calc_reg)(struct rockchip_pin_bank *bank,
+				    int pin_num, struct regmap **regmap,
+				    int *reg, u8 *bit);
 };
 
 struct rockchip_pin_config {
@@ -159,9 +161,9 @@ struct rockchip_pmx_func {
 };
 
 struct rockchip_pinctrl {
-	void __iomem			*reg_base;
+	struct regmap			*regmap_base;
 	int				reg_size;
-	void __iomem			*reg_pull;
+	struct regmap			*regmap_pull;
 	struct device			*dev;
 	struct rockchip_pin_ctrl	*ctrl;
 	struct pinctrl_desc		pctl;
@@ -172,6 +174,12 @@ struct rockchip_pinctrl {
 	unsigned int			nfunctions;
 };
 
+static struct regmap_config rockchip_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
 static inline struct rockchip_pin_bank *gc_to_pin_bank(struct gpio_chip *gc)
 {
 	return container_of(gc, struct rockchip_pin_bank, gpio_chip);
@@ -333,18 +341,24 @@ static const struct pinctrl_ops rockchip_pctrl_ops = {
 static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin)
 {
 	struct rockchip_pinctrl *info = bank->drvdata;
-	void __iomem *reg = info->reg_base + info->ctrl->mux_offset;
+	unsigned int val;
+	int reg, ret;
 	u8 bit;
 
 	if (bank->bank_type == RK3188_BANK0 && pin < 16)
 		return RK_FUNC_GPIO;
 
 	/* get basic quadrupel of mux registers and the correct reg inside */
+	reg = info->ctrl->mux_offset;
 	reg += bank->bank_num * 0x10;
 	reg += (pin / 8) * 4;
 	bit = (pin % 8) * 2;
 
-	return ((readl(reg) >> bit) & 3);
+	ret = regmap_read(info->regmap_base, reg, &val);
+	if (ret)
+		return ret;
+
+	return ((val >> bit) & 3);
 }
 
 /*
@@ -363,7 +377,7 @@ static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin)
 static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 {
 	struct rockchip_pinctrl *info = bank->drvdata;
-	void __iomem *reg = info->reg_base + info->ctrl->mux_offset;
+	int reg, ret;
 	unsigned long flags;
 	u8 bit;
 	u32 data;
@@ -386,6 +400,7 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 						bank->bank_num, pin, mux);
 
 	/* get basic quadrupel of mux registers and the correct reg inside */
+	reg = info->ctrl->mux_offset;
 	reg += bank->bank_num * 0x10;
 	reg += (pin / 8) * 4;
 	bit = (pin % 8) * 2;
@@ -394,11 +409,11 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 
 	data = (3 << (bit + 16));
 	data |= (mux & 3) << bit;
-	writel(data, reg);
+	ret = regmap_write(info->regmap_base, reg, data);
 
 	spin_unlock_irqrestore(&bank->slock, flags);
 
-	return 0;
+	return ret;
 }
 
 #define RK2928_PULL_OFFSET		0x118
@@ -406,11 +421,13 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 #define RK2928_PULL_BANK_STRIDE		8
 
 static void rk2928_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
-				    int pin_num, void __iomem **reg, u8 *bit)
+				    int pin_num, struct regmap **regmap,
+				    int *reg, u8 *bit)
 {
 	struct rockchip_pinctrl *info = bank->drvdata;
 
-	*reg = info->reg_base + RK2928_PULL_OFFSET;
+	*regmap = info->regmap_base;
+	*reg = RK2928_PULL_OFFSET;
 	*reg += bank->bank_num * RK2928_PULL_BANK_STRIDE;
 	*reg += (pin_num / RK2928_PULL_PINS_PER_REG) * 4;
 
@@ -423,19 +440,23 @@ static void rk2928_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
 #define RK3188_PULL_BANK_STRIDE		16
 
 static void rk3188_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
-				    int pin_num, void __iomem **reg, u8 *bit)
+				    int pin_num, struct regmap **regmap,
+				    int *reg, u8 *bit)
 {
 	struct rockchip_pinctrl *info = bank->drvdata;
 
 	/* The first 12 pins of the first bank are located elsewhere */
 	if (bank->bank_type == RK3188_BANK0 && pin_num < 12) {
-		*reg = bank->reg_pull +
-				((pin_num / RK3188_PULL_PINS_PER_REG) * 4);
+		*regmap = bank->regmap_pull;
+		*reg = 0;
+		*reg += ((pin_num / RK3188_PULL_PINS_PER_REG) * 4);
 		*bit = pin_num % RK3188_PULL_PINS_PER_REG;
 		*bit *= RK3188_PULL_BITS_PER_PIN;
 	} else {
-		*reg = info->reg_pull ? info->reg_pull
-				      : info->reg_base + RK3188_PULL_OFFSET;
+		*regmap = info->regmap_pull ? info->regmap_pull
+					    : info->regmap_base;
+		*reg = info->regmap_pull ? 0 : RK3188_PULL_OFFSET;
+
 		/* correct the offset, as it is the 2nd pull register */
 		*reg -= 4;
 		*reg += bank->bank_num * RK3188_PULL_BANK_STRIDE;
@@ -455,7 +476,8 @@ static int rockchip_get_pull(struct rockchip_pin_bank *bank, int pin_num)
 {
 	struct rockchip_pinctrl *info = bank->drvdata;
 	struct rockchip_pin_ctrl *ctrl = info->ctrl;
-	void __iomem *reg;
+	struct regmap *regmap;
+	int reg, ret;
 	u8 bit;
 	u32 data;
 
@@ -463,15 +485,19 @@ static int rockchip_get_pull(struct rockchip_pin_bank *bank, int pin_num)
 	if (ctrl->type == RK3066B)
 		return PIN_CONFIG_BIAS_DISABLE;
 
-	ctrl->pull_calc_reg(bank, pin_num, &reg, &bit);
+	ctrl->pull_calc_reg(bank, pin_num, &regmap, &reg, &bit);
+
+	ret = regmap_read(regmap, reg, &data);
+	if (ret)
+		return ret;
 
 	switch (ctrl->type) {
 	case RK2928:
-		return !(readl_relaxed(reg) & BIT(bit))
+		return !(data & BIT(bit))
 				? PIN_CONFIG_BIAS_PULL_PIN_DEFAULT
 				: PIN_CONFIG_BIAS_DISABLE;
 	case RK3188:
-		data = readl_relaxed(reg) >> bit;
+		data >>= bit;
 		data &= (1 << RK3188_PULL_BITS_PER_PIN) - 1;
 
 		switch (data) {
@@ -498,7 +524,8 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 {
 	struct rockchip_pinctrl *info = bank->drvdata;
 	struct rockchip_pin_ctrl *ctrl = info->ctrl;
-	void __iomem *reg;
+	struct regmap *regmap;
+	int reg, ret;
 	unsigned long flags;
 	u8 bit;
 	u32 data;
@@ -510,7 +537,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 	if (ctrl->type == RK3066B)
 		return pull ? -EINVAL : 0;
 
-	ctrl->pull_calc_reg(bank, pin_num, &reg, &bit);
+	ctrl->pull_calc_reg(bank, pin_num, &regmap, &reg, &bit);
 
 	switch (ctrl->type) {
 	case RK2928:
@@ -519,7 +546,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 		data = BIT(bit + 16);
 		if (pull == PIN_CONFIG_BIAS_DISABLE)
 			data |= BIT(bit);
-		writel(data, reg);
+		ret = regmap_write(regmap, reg, data);
 
 		spin_unlock_irqrestore(&bank->slock, flags);
 		break;
@@ -548,7 +575,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 			return -EINVAL;
 		}
 
-		writel(data, reg);
+		ret = regmap_write(regmap, reg, data);
 
 		spin_unlock_irqrestore(&bank->slock, flags);
 		break;
@@ -557,7 +584,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 		return -EINVAL;
 	}
 
-	return 0;
+	return ret;
 }
 
 /*
@@ -1416,6 +1443,7 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank,
 				  struct device *dev)
 {
 	struct resource res;
+	void __iomem *base;
 
 	if (of_address_to_resource(bank->of_node, 0, &res)) {
 		dev_err(dev, "cannot find IO resource for bank\n");
@@ -1440,9 +1468,14 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank,
 			return -ENOENT;
 		}
 
-		bank->reg_pull = devm_ioremap_resource(dev, &res);
-		if (IS_ERR(bank->reg_pull))
-			return PTR_ERR(bank->reg_pull);
+		base = devm_ioremap_resource(dev, &res);
+		if (IS_ERR(base))
+			return PTR_ERR(base);
+		rockchip_regmap_config.max_register = resource_size(&res) - 4;
+		rockchip_regmap_config.name = "rockchip,rk3188-gpio-bank0-pull";
+		bank->regmap_pull = devm_regmap_init_mmio(dev, base,
+						  &rockchip_regmap_config);
+
 	} else {
 		bank->bank_type = COMMON_BANK;
 	}
@@ -1507,6 +1540,7 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct rockchip_pin_ctrl *ctrl;
 	struct resource *res;
+	void __iomem *base;
 	int ret;
 
 	if (!dev->of_node) {
@@ -1527,19 +1561,29 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
 	info->dev = dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	info->reg_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(info->reg_base))
-		return PTR_ERR(info->reg_base);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	rockchip_regmap_config.max_register = resource_size(res) - 4;
+	rockchip_regmap_config.name = "rockchip,pinctrl";
+	info->regmap_base = devm_regmap_init_mmio(&pdev->dev, base,
+						  &rockchip_regmap_config);
 
 	/* to check for the old dt-bindings */
 	info->reg_size = resource_size(res);
 
 	/* Honor the old binding, with pull registers as 2nd resource */
-	if (ctrl->type == RK3188 &&  info->reg_size < 0x200) {
+	if (ctrl->type == RK3188 && info->reg_size < 0x200) {
 		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-		info->reg_pull = devm_ioremap_resource(&pdev->dev, res);
-		if (IS_ERR(info->reg_pull))
-			return PTR_ERR(info->reg_pull);
+		base = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(base))
+			return PTR_ERR(base);
+
+		rockchip_regmap_config.max_register = resource_size(res) - 4;
+		rockchip_regmap_config.name = "rockchip,pinctrl-pull";
+		info->regmap_pull = devm_regmap_init_mmio(&pdev->dev, base,
+						  &rockchip_regmap_config);
 	}
 
 	ret = rockchip_gpiolib_register(pdev, info);
-- 
1.9.0


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/8] pinctrl: rockchip: rockchip_pinctrl in rockchip_get_bank_data

[Heiko Stübner]

Convert rockchip_get_bank_data to use the struct rockchip_pinctrl because
later on we need to check a value from it when registering the gpio banks.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 drivers/pinctrl/pinctrl-rockchip.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 71d9c99..b67771d 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -1440,17 +1440,17 @@ static int rockchip_gpiolib_unregister(struct platform_device *pdev,
 }
 
 static int rockchip_get_bank_data(struct rockchip_pin_bank *bank,
-				  struct device *dev)
+				  struct rockchip_pinctrl *info)
 {
 	struct resource res;
 	void __iomem *base;
 
 	if (of_address_to_resource(bank->of_node, 0, &res)) {
-		dev_err(dev, "cannot find IO resource for bank\n");
+		dev_err(info->dev, "cannot find IO resource for bank\n");
 		return -ENOENT;
 	}
 
-	bank->reg_base = devm_ioremap_resource(dev, &res);
+	bank->reg_base = devm_ioremap_resource(info->dev, &res);
 	if (IS_ERR(bank->reg_base))
 		return PTR_ERR(bank->reg_base);
 
@@ -1464,16 +1464,16 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank,
 		bank->bank_type = RK3188_BANK0;
 
 		if (of_address_to_resource(bank->of_node, 1, &res)) {
-			dev_err(dev, "cannot find IO resource for bank\n");
+			dev_err(info->dev, "cannot find IO resource for bank\n");
 			return -ENOENT;
 		}
 
-		base = devm_ioremap_resource(dev, &res);
+		base = devm_ioremap_resource(info->dev, &res);
 		if (IS_ERR(base))
 			return PTR_ERR(base);
 		rockchip_regmap_config.max_register = resource_size(&res) - 4;
 		rockchip_regmap_config.name = "rockchip,rk3188-gpio-bank0-pull";
-		bank->regmap_pull = devm_regmap_init_mmio(dev, base,
+		bank->regmap_pull = devm_regmap_init_mmio(info->dev, base,
 						  &rockchip_regmap_config);
 
 	} else {
@@ -1515,7 +1515,7 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data(
 			if (!strcmp(bank->name, np->name)) {
 				bank->of_node = np;
 
-				if (!rockchip_get_bank_data(bank, &pdev->dev))
+				if (!rockchip_get_bank_data(bank, d))
 					bank->valid = true;
 
 				break;
@@ -1552,13 +1552,14 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
 	if (!info)
 		return -ENOMEM;
 
+	info->dev = dev;
+
 	ctrl = rockchip_pinctrl_get_soc_data(info, pdev);
 	if (!ctrl) {
 		dev_err(dev, "driver data not available\n");
 		return -EINVAL;
 	}
 	info->ctrl = ctrl;
-	info->dev = dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, res);
-- 
1.9.0


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/8] pinctrl: rockchip: let pmu registers be supplied by a syscon

[Heiko Stübner]

Currently the pmu registers containing pin pull settings on the rk3188 are mapped
locally when bank0 is instantiated. Add an alternative that can resolve the pmu
from a syscon phandle.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 drivers/pinctrl/pinctrl-rockchip.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index b67771d..11ad643 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -38,6 +38,7 @@
 #include <linux/irqchip/chained_irq.h>
 #include <linux/clk.h>
 #include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
 #include <dt-bindings/pinctrl/rockchip.h>
 
 #include "core.h"
@@ -164,6 +165,7 @@ struct rockchip_pinctrl {
 	struct regmap			*regmap_base;
 	int				reg_size;
 	struct regmap			*regmap_pull;
+	struct regmap			*regmap_pmu;
 	struct device			*dev;
 	struct rockchip_pin_ctrl	*ctrl;
 	struct pinctrl_desc		pctl;
@@ -438,6 +440,7 @@ static void rk2928_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
 #define RK3188_PULL_BITS_PER_PIN	2
 #define RK3188_PULL_PINS_PER_REG	8
 #define RK3188_PULL_BANK_STRIDE		16
+#define RK3188_PULL_PMU_OFFSET		0x64
 
 static void rk3188_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
 				    int pin_num, struct regmap **regmap,
@@ -447,8 +450,9 @@ static void rk3188_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
 
 	/* The first 12 pins of the first bank are located elsewhere */
 	if (bank->bank_type == RK3188_BANK0 && pin_num < 12) {
-		*regmap = bank->regmap_pull;
-		*reg = 0;
+		*regmap = info->regmap_pmu ? info->regmap_pmu
+					   : bank->regmap_pull;
+		*reg = info->regmap_pmu ? RK3188_PULL_PMU_OFFSET : 0;
 		*reg += ((pin_num / RK3188_PULL_PINS_PER_REG) * 4);
 		*bit = pin_num % RK3188_PULL_PINS_PER_REG;
 		*bit *= RK3188_PULL_BITS_PER_PIN;
@@ -1539,6 +1543,7 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
 	struct rockchip_pinctrl *info;
 	struct device *dev = &pdev->dev;
 	struct rockchip_pin_ctrl *ctrl;
+	struct device_node *np = pdev->dev.of_node, *node;
 	struct resource *res;
 	void __iomem *base;
 	int ret;
@@ -1587,6 +1592,14 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
 						  &rockchip_regmap_config);
 	}
 
+	/* try to find the optional reference to the pmu syscon */
+	node = of_parse_phandle(np, "rockchip,pmu", 0);
+	if (node) {
+		info->regmap_pmu = syscon_node_to_regmap(node);
+		if (IS_ERR(info->regmap_pmu))
+			return PTR_ERR(info->regmap_pmu);
+	}
+
 	ret = rockchip_gpiolib_register(pdev, info);
 	if (ret)
 		return ret;
-- 
1.9.0


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/8] pinctrl: rockchip: only map bank0-pull-region when pmu regmap missing

[Heiko Stübner]

When the pmu registers are supplied through a syscon regmap we do not need
to map the registers ourself.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 11ad643..d6e2401 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -1464,21 +1464,29 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank,
 	 */
 	if (of_device_is_compatible(bank->of_node,
 				    "rockchip,rk3188-gpio-bank0")) {
+		struct device_node *node;
 
 		bank->bank_type = RK3188_BANK0;
 
-		if (of_address_to_resource(bank->of_node, 1, &res)) {
-			dev_err(info->dev, "cannot find IO resource for bank\n");
-			return -ENOENT;
-		}
+		node = of_parse_phandle(bank->of_node->parent,
+					"rockchip,pmu", 0);
+		if (!node) {
+			if (of_address_to_resource(bank->of_node, 1, &res)) {
+				dev_err(info->dev, "cannot find IO resource for bank\n");
+				return -ENOENT;
+			}
 
-		base = devm_ioremap_resource(info->dev, &res);
-		if (IS_ERR(base))
-			return PTR_ERR(base);
-		rockchip_regmap_config.max_register = resource_size(&res) - 4;
-		rockchip_regmap_config.name = "rockchip,rk3188-gpio-bank0-pull";
-		bank->regmap_pull = devm_regmap_init_mmio(info->dev, base,
-						  &rockchip_regmap_config);
+			base = devm_ioremap_resource(info->dev, &res);
+			if (IS_ERR(base))
+				return PTR_ERR(base);
+			rockchip_regmap_config.max_register =
+						    resource_size(&res) - 4;
+			rockchip_regmap_config.name =
+					    "rockchip,rk3188-gpio-bank0-pull";
+			bank->regmap_pull = devm_regmap_init_mmio(info->dev,
+						    base,
+						    &rockchip_regmap_config);
+		}
 
 	} else {
 		bank->bank_type = COMMON_BANK;
-- 
1.9.0


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/8] pinctrl: rockchip: base regmap supplied by a syscon

[Heiko Stübner]

This allows the basic registers of the general register files to be supplied
by a syscon instead of being mapped locally.

The GRF registers contain a lot more than pinctrl functions like dma, usb-phy
and general soc control and status registers, intermixed with the iomux, pull
and drive-strength registers.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 drivers/pinctrl/pinctrl-rockchip.c | 47 +++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index d6e2401..bb805d5 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -1574,30 +1574,39 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
 	}
 	info->ctrl = ctrl;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	rockchip_regmap_config.max_register = resource_size(res) - 4;
-	rockchip_regmap_config.name = "rockchip,pinctrl";
-	info->regmap_base = devm_regmap_init_mmio(&pdev->dev, base,
-						  &rockchip_regmap_config);
-
-	/* to check for the old dt-bindings */
-	info->reg_size = resource_size(res);
-
-	/* Honor the old binding, with pull registers as 2nd resource */
-	if (ctrl->type == RK3188 && info->reg_size < 0x200) {
-		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	node = of_parse_phandle(np, "rockchip,grf", 0);
+	if (node) {
+		info->regmap_base = syscon_node_to_regmap(node);
+		if (IS_ERR(info->regmap_base))
+			return PTR_ERR(info->regmap_base);
+	} else {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 		base = devm_ioremap_resource(&pdev->dev, res);
 		if (IS_ERR(base))
 			return PTR_ERR(base);
 
 		rockchip_regmap_config.max_register = resource_size(res) - 4;
-		rockchip_regmap_config.name = "rockchip,pinctrl-pull";
-		info->regmap_pull = devm_regmap_init_mmio(&pdev->dev, base,
-						  &rockchip_regmap_config);
+		rockchip_regmap_config.name = "rockchip,pinctrl";
+		info->regmap_base = devm_regmap_init_mmio(&pdev->dev, base,
+						    &rockchip_regmap_config);
+
+		/* to check for the old dt-bindings */
+		info->reg_size = resource_size(res);
+
+		/* Honor the old binding, with pull registers as 2nd resource */
+		if (ctrl->type == RK3188 && info->reg_size < 0x200) {
+			res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+			base = devm_ioremap_resource(&pdev->dev, res);
+			if (IS_ERR(base))
+				return PTR_ERR(base);
+
+			rockchip_regmap_config.max_register =
+							resource_size(res) - 4;
+			rockchip_regmap_config.name = "rockchip,pinctrl-pull";
+			info->regmap_pull = devm_regmap_init_mmio(&pdev->dev,
+						    base,
+						    &rockchip_regmap_config);
+		}
 	}
 
 	/* try to find the optional reference to the pmu syscon */
-- 
1.9.0


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/8] dt-bindings: adapt rockchip-pinctrl doc to changed bindings

[Heiko Stübner]

Introduce the syscons for grf and pmu and deprecate the previous register
areas.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 .../bindings/pinctrl/rockchip,pinctrl.txt          | 28 +++++++++++++++-------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
index 78dafc9..cefef74 100644
--- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
@@ -21,15 +21,23 @@ defined as gpio sub-nodes of the pinmux controller.
 Required properties for iomux controller:
   - compatible: one of "rockchip,rk2928-pinctrl", "rockchip,rk3066a-pinctrl"
 		       "rockchip,rk3066b-pinctrl", "rockchip,rk3188-pinctrl"
+  - rockchip,grf: phandle referencing a syscon providing the
+	 "general register files"
+
+Optional properties for iomux controller:
+  - rockchip,pmu: phandle referencing a syscon providing the pmu registers
+	 as some SoCs carry parts of the iomux controller registers there.
+	 Required for at least rk3188 and rk3288.
+
+Deprecated properties for iomux controller:
   - reg: first element is the general register space of the iomux controller
 	 It should be large enough to contain also separate pull registers.
-	 Deprecated:
-	 second element is the separate pull register space of the rk3188
+	 second element is the separate pull register space of the rk3188.
+	 Use rockchip,grf and rockchip,pmu described above instead.
 
 Required properties for gpio sub nodes:
   - compatible: "rockchip,gpio-bank", "rockchip,rk3188-gpio-bank0"
   - reg: register of the gpio bank (different than the iomux registerset)
-         second element: separate pull register for rk3188 bank0
   - interrupts: base interrupt of the gpio bank in the interrupt controller
   - clocks: clock that drives this bank
   - gpio-controller: identifies the node as a gpio controller and pin bank.
@@ -41,6 +49,10 @@ Required properties for gpio sub nodes:
     cells should use the standard two-cell scheme described in
     bindings/interrupt-controller/interrupts.txt
 
+Deprecated properties for gpio sub nodes:
+  - reg: second element: separate pull register for rk3188 bank0, use
+	 rockchip,pmu described above instead
+
 Required properties for pin configuration node:
   - rockchip,pins: 3 integers array, represents a group of pins mux and config
     setting. The format is rockchip,pins = <PIN_BANK PIN_BANK_IDX MUX &phandle>.
@@ -56,7 +68,8 @@ Examples:
 
 pinctrl@20008000 {
 	compatible = "rockchip,rk3066a-pinctrl";
-	reg = <0x20008000 0x150>;
+	rockchip,grf = <&grf>;
+
 	#address-cells = <1>;
 	#size-cells = <1>;
 	ranges;
@@ -105,16 +118,15 @@ Example for rk3188:
 
 	pinctrl@20008000 {
 		compatible = "rockchip,rk3188-pinctrl";
-		reg = <0x20008000 0xa0>,
-		      <0x20008164 0x1a0>;
+		rockchip,grf = <&grf>;
+		rockchip,pmu = <&pmu>;
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges;
 
 		gpio0: gpio0@0x2000a000 {
 			compatible = "rockchip,rk3188-gpio-bank0";
-			reg = <0x2000a000 0x100>,
-			      <0x20004064 0x8>;
+			reg = <0x2000a000 0x100>;
 			interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&clk_gates8 9>;
 
-- 
1.9.0


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/8] ARM: dts: rockchip: convert pinctrl nodes to new bindings

[Heiko Stübner]

Introduce the grf syscon and convert the pinctrl drivers for rk3066 and rk3188
to use it, instead of mapping the grf registers themselfs.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 arch/arm/boot/dts/rk3066a.dtsi | 2 +-
 arch/arm/boot/dts/rk3188.dtsi  | 9 ++++-----
 arch/arm/boot/dts/rk3xxx.dtsi  | 9 +++++++--
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/arm/boot/dts/rk3066a.dtsi b/arch/arm/boot/dts/rk3066a.dtsi
index 4d4dfbb..048c5de 100644
--- a/arch/arm/boot/dts/rk3066a.dtsi
+++ b/arch/arm/boot/dts/rk3066a.dtsi
@@ -79,7 +79,7 @@
 
 		pinctrl@20008000 {
 			compatible = "rockchip,rk3066a-pinctrl";
-			reg = <0x20008000 0x150>;
+			rockchip,grf = <&grf>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index ed9a70a..a494fb0 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -75,17 +75,16 @@
 
 		pinctrl@20008000 {
 			compatible = "rockchip,rk3188-pinctrl";
-			reg = <0x20008000 0xa0>,
-			      <0x20008164 0x1a0>;
-			reg-names = "base", "pull";
+			rockchip,grf = <&grf>;
+			rockchip,pmu = <&pmu>;
+
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
 
 			gpio0: gpio0@0x2000a000 {
 				compatible = "rockchip,rk3188-gpio-bank0";
-				reg = <0x2000a000 0x100>,
-				      <0x20004064 0x8>;
+				reg = <0x2000a000 0x100>;
 				interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clk_gates8 9>;
 
diff --git a/arch/arm/boot/dts/rk3xxx.dtsi b/arch/arm/boot/dts/rk3xxx.dtsi
index 26e5a96..2adf1cc9e 100644
--- a/arch/arm/boot/dts/rk3xxx.dtsi
+++ b/arch/arm/boot/dts/rk3xxx.dtsi
@@ -31,11 +31,16 @@
 			reg = <0x1013c000 0x100>;
 		};
 
-		pmu@20004000 {
-			compatible = "rockchip,rk3066-pmu";
+		pmu: pmu@20004000 {
+			compatible = "rockchip,rk3066-pmu", "syscon";
 			reg = <0x20004000 0x100>;
 		};
 
+		grf: grf@20008000 {
+			compatible = "syscon";
+			reg = <0x20008000 0x200>;
+		};
+
 		gic: interrupt-controller@1013d000 {
 			compatible = "arm,cortex-a9-gic";
 			interrupt-controller;
-- 
1.9.0


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 0/8] pinctrl-rockchip: Change wrong initial assumptions

[Max Schwarz]

On Wednesday 30 April 2014 at 00:07:12, Heiko Stübner wrote:
> While this wasn't a problem until now, the upcoming rk3288 introduces
> additional changes to both the grf and pmu areas. On it even part of
> the pinmux registers move into the pmu space.

Could you give us more information on that? I tried to find details on the 
RK3288 but came up with nothing. How are the pinmux registers divided?

Would adding a third reg element for the pinmux register to the gpio subnodes 
suffice to fix your problem?

> For this my current gut-feeling is, that providing both the grf and pmu
> as syscons to the pinctrl driver might be more future proof for the next
> socs. But as I'm not sure on this, I'd like of course comments :-)

I don't see the problem with the current solution. In my mind it's cleaner to 
specify register mappings explicitly in the dt rather than map one large block 
and let the drivers figure out themselves where their registers are.

There are some question marks for me on the syscon solution. Regmap uses 
locking internally, which means separate drivers can't access separate 
registers simultaneously. We have an SMP machine here, so that's not far-
fetched. And that locking is completely unnecessary, as we *know* in most 
cases that the accessed areas do not overlap.

> The other option would be to leave the grf as it is and create separate
> syscons for real small individual parts like the soc-conf and usb-phys.
> But apart from creating these real small syscons that would
> also make it necessary to introduce another register map for the
> drive-strength settings of the pin-controller, which are sitting in the
> middle of everything at least on rk3066 and rk3188.

Wy do we need a syscon for usb-phys? Is it shared by multiple drivers?
My instinctive approach would be two usb-phys devices mapping the GRF_UOC0/1 
spaces directly via reg properties. Or did I miss something?

The only register space I see that is used from many drivers is the GRF_SOC_* 
space. So in my mind that should be the only syscon device.

> @Max: sorry to come up with this now, but I feel this should be resolved
> (in whatever direction) before we introduce any grf syscon. Because due
> to dt being an API we will be tied for a long time to it.

Yes, I agree. If we want to change something, we should change it now. All in 
all I would vote for the current solution. But it seems you have more 
information than me, so my vote is somewhat uneducated ;-)

Cheers,
  Max
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

syscon or memory mappings (was: Re: [RFC/PATCH 0/8] pinctrl-rockchip: Change wrong initial assumptions)

[Heiko Stübner]

[-- Attachment #1: Type: text/plain, Size: 6580 bytes --]

Hi Max,

Am Donnerstag, 1. Mai 2014, 12:43:13 schrieb Max Schwarz:
> On Wednesday 30 April 2014 at 00:07:12, Heiko Stübner wrote:
> > While this wasn't a problem until now, the upcoming rk3288 introduces
> > additional changes to both the grf and pmu areas. On it even part of
> > the pinmux registers move into the pmu space.
> 
> Could you give us more information on that? I tried to find details on the
> RK3288 but came up with nothing. How are the pinmux registers divided?

Some days ago, Rockchip released kernel sources for the rk3288 [0]. They took 
a lot of our current mainline code as base for their kernel. AS you can see in 
the register map below, the pinmux registers for the gpio0 bank are residing 
in the pmu space, while gpio1-8 are residing in the regular "general register 
files"


> Would adding a third reg element for the pinmux register to the gpio
> subnodes suffice to fix your problem?

Rockchip designers do not seem to fear reordering both the GRF and PMU 
register spaces. So my biggest fear is what they'll come up with the next SoC 
;-). Thus I'd like to reduce soc-specific parts instead of adding more.


To elaborate a bit:

On rk3188 it is
GRF: 0x00 - 0x5c: pin suspend control
GRF: 0x60 - 0x9c: pinmux control (0x60 and 0x64 gpio-only)
GRF: 0xa0 - 0xac: soc-control/status
GRF: 0xb0 - 0xc8: dma-control
GRF: 0xcc - 0xe0: "cpu core configuration"
GRF: 0xec - 0xf0: ddr-controller config
GRF: 0xf4 - 0x104: pin drive-strength (what we currently do not support)
GRF: 0x108: soc_status1
GRF: 0x10c - 0x140: USB phy control
GRF: 0x144 - 0x160: "OS register"
GRF: 0x160 - 0x19c: pin pull settings
PMU: 0x00 - 0x38: power-domains and a lot of unknown stuff
PMU: 0x3c: something called GPIO0_CON, what Rockchip does not use at all
PMU: 0x40 - 0x60: "SYS_REGx"
PMU: 0x64 - 0x68: part of GPIO0 pull config

so we would/will in the end need 4 mappings for the rk3188-pinctrl
GRF: 0x00 - 0x9c, GRF: 0xf4 - 0x104, GRF: 0x160 - 0x19c, PMU: 0x64 - 0x68


On rk3288 it is

GRF: 0x00 - 0x84: gpio1-gpio8 iomux settings
GRF: 0x104 - 0x138: unknown GPIOxx_SR registers
GRF: 0x140 - 0x1b4: gpio1-gpio8 pull settings
GRF: 0x1c0 - 0x234: gpio1-gpio8 driver strength settings
GRF: 0x240: unknown/unused GPIO_SMT
GRF: 0x244 - 0x2d4: soc control/status registers
GRF: 0x2e0 - ...: a lot of stuff like dma, usb-phy etc.
PMU: 0x00 - 0x5c: powerdomains and a lot of other stuff
PMU: 0x60: GPIO_SR
PMU: 0x64 - 0x6c: gpio0 pull settings
PMU: 0x70 - 0x78: gpio0 drive-strength settings
PMU: 0x7c: GPIO_OP
PMU: 0x80: GPIO0_SEL18
PMU: 0x84 - 0x8c: gpio0 pinmux settings
PMU: 0x90 - 0xa0: more misc registers (powermode, sys_regX)

so we would essentially need only two mappings here
GRF: 0x00 - 0x240 and PMU: 0x60 - 0x8c


So we'd need additional if(is_rk3188()) conditionals to distinguish between 
these implementations [and possible future ones] to select the correct base 
address, and we don't know what the next SoC will bring and how the stuff will 
be ordered there.


Also leaving the driver behind, devicetree is meant to describe the hardware, 
not the implementation. And hw-wise both PMU and GRF are actual hardware 
blocks even with individual clock gates.

Citing the syscon-devicetree bindings:

	System controller node represents a register region containing a set
	of miscellaneous registers. The registers are not cohesive enough to
	represent as any specific type of device.

So to me both GRF and PMU regions scream "syscon".


> > For this my current gut-feeling is, that providing both the grf and pmu
> > as syscons to the pinctrl driver might be more future proof for the next
> > socs. But as I'm not sure on this, I'd like of course comments :-)
> 
> I don't see the problem with the current solution. In my mind it's cleaner
> to specify register mappings explicitly in the dt rather than map one large
> block and let the drivers figure out themselves where their registers are.

I've attached my current WIP patch to implement rk3288 support (untested, as I 
don't have any hardware), based on this series. As you can see in it, the 
rk3288 has even more peculiarities with gpio-only and 4bit wide iomuxes.

As the patch stands now, rk3288 doesn't even need special handling for its 
iomux registers, as it can be simply described now in the pin-bank declaration 
at the bottom - and even the rk3188-bank0 wouldn't be necessary anymore.


> There are some question marks for me on the syscon solution. Regmap uses
> locking internally, which means separate drivers can't access separate
> registers simultaneously. We have an SMP machine here, so that's not far-
> fetched. And that locking is completely unnecessary, as we *know* in most
> cases that the accessed areas do not overlap.

For locking vs. speed, I do not see this as a big problem. All registers in 
there mainly contain general settings that are not changed often during the 
operation of the device. So there is no high frequency access to them in any 
case.


> > The other option would be to leave the grf as it is and create separate
> > syscons for real small individual parts like the soc-conf and usb-phys.
> > But apart from creating these real small syscons that would
> > also make it necessary to introduce another register map for the
> > drive-strength settings of the pin-controller, which are sitting in the
> > middle of everything at least on rk3066 and rk3188.
> 
> Wy do we need a syscon for usb-phys? Is it shared by multiple drivers?
> My instinctive approach would be two usb-phys devices mapping the GRF_UOC0/1
> spaces directly via reg properties. Or did I miss something?

Of course if we're going to map each part of the GRF individually there is no 
need for a syscon.


> The only register space I see that is used from many drivers is the
> GRF_SOC_* space. So in my mind that should be the only syscon device.
> 
> > @Max: sorry to come up with this now, but I feel this should be resolved
> > (in whatever direction) before we introduce any grf syscon. Because due
> > to dt being an API we will be tied for a long time to it.
> 
> Yes, I agree. If we want to change something, we should change it now. All
> in all I would vote for the current solution. But it seems you have more
> information than me, so my vote is somewhat uneducated ;-)

I'm also hoping for more input so I've changed the title a bit, to maybe 
attract more people :-) .


Heiko


[0] https://github.com/rkchrome/kernel

[-- Attachment #2: 0001-pinctrl-rockchip-WIP-introduce-flexible-iomux-handli.patch --]
[-- Type: text/x-patch, Size: 12142 bytes --]

>From 413669fdf083e5570fc544c08d998876d77b5b55 Mon Sep 17 00:00:00 2001
From: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
Date: Thu, 1 May 2014 00:47:01 +0200
Subject: [PATCH] pinctrl: rockchip: WIP introduce flexible iomux handling and
 rk3288 support

todo: split into two patches, one introducing the flexible iomux handling
and another one adding the actual rk3288 support.
---
 drivers/pinctrl/pinctrl-rockchip.c | 230 ++++++++++++++++++++++++++++++++-----
 1 file changed, 202 insertions(+), 28 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 948a19f..15f2d30 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -70,6 +70,29 @@ enum rockchip_pin_bank_type {
 };
 
 /**
+ * Encode variants of iomux registers into a type variable
+ * bit[0] - iomux is gpio-only
+ * bit[3:1] - bit width of pin iomux settings
+ * bit[6:4] - location of this iomux register
+ */
+#define IOMUX_GPIO_ONLY		(1 << 0)
+#define IOMUX_WIDTH_2BIT	0
+#define IOMUX_WIDTH_4BIT	(1 << 1)
+#define IOMUX_SOURCE_GRF	0
+#define IOMUX_SOURCE_PMU	(1 << 4)
+
+/**
+ * @type: iomux variant using IOMUX_* constants
+ * @offset: if initialized to -1 it will be autocalculated, by specifying
+ *	    an initial offset value the relevant source offset can be reset
+ *	    to a new value for autocalculating the following iomux registers.
+ */
+struct rockchip_iomux {
+	int				type;
+	int				offset;
+};
+
+/**
  * @reg_base: register base of the gpio bank
  * @reg_pull: optional separate register for additional pull settings
  * @clk: clock of the gpio bank
@@ -78,6 +101,7 @@ enum rockchip_pin_bank_type {
  * @nr_pins: number of pins in this bank
  * @name: name of the bank
  * @bank_num: number of the bank, to account for holes
+ * @iomux: array describing the 4 iomux sources of the bank
  * @valid: are all necessary informations present
  * @of_node: dt node of this bank
  * @drvdata: common pinctrl basedata
@@ -96,6 +120,7 @@ struct rockchip_pin_bank {
 	char				*name;
 	u8				bank_num;
 	enum rockchip_pin_bank_type	bank_type;
+	struct rockchip_iomux		iomux[4];
 	bool				valid;
 	struct device_node		*of_node;
 	struct rockchip_pinctrl		*drvdata;
@@ -111,6 +136,25 @@ struct rockchip_pin_bank {
 		.bank_num	= id,			\
 		.nr_pins	= pins,			\
 		.name		= label,		\
+		.iomux		= {			\
+			{ .offset = -1 },		\
+			{ .offset = -1 },		\
+			{ .offset = -1 },		\
+			{ .offset = -1 },		\
+		},					\
+	}
+
+#define PIN_BANK_IOMUX_FLAGS(id, pins, label, iom0, iom1, iom2, iom3)	\
+	{								\
+		.bank_num	= id,					\
+		.nr_pins	= pins,					\
+		.name		= label,				\
+		.iomux		= {					\
+			{ .type = iom0, .offset = -1 },			\
+			{ .type = iom1, .offset = -1 },			\
+			{ .type = iom2, .offset = -1 },			\
+			{ .type = iom3, .offset = -1 },			\
+		},							\
 	}
 
 /**
@@ -121,7 +165,8 @@ struct rockchip_pin_ctrl {
 	u32				nr_pins;
 	char				*label;
 	enum rockchip_pinctrl_type	type;
-	int				mux_offset;
+	int				grf_mux_offset;
+	int				pmu_mux_offset;
 	void	(*pull_calc_reg)(struct rockchip_pin_bank *bank,
 				    int pin_num, struct regmap **regmap,
 				    int *reg, u8 *bit);
@@ -343,24 +388,34 @@ static const struct pinctrl_ops rockchip_pctrl_ops = {
 static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin)
 {
 	struct rockchip_pinctrl *info = bank->drvdata;
+	int iomux_num = (pin / 8);
+	struct regmap *regmap;
 	unsigned int val;
-	int reg, ret;
+	int reg, ret, mask;
 	u8 bit;
 
-	if (bank->bank_type == RK3188_BANK0 && pin < 16)
+	if (bank->iomux[iomux_num].type & IOMUX_GPIO_ONLY)
 		return RK_FUNC_GPIO;
 
+	regmap = (bank->iomux[iomux_num].type & IOMUX_SOURCE_PMU)
+				? info->regmap_pmu : info->regmap_base;
+
 	/* get basic quadrupel of mux registers and the correct reg inside */
-	reg = info->ctrl->mux_offset;
-	reg += bank->bank_num * 0x10;
-	reg += (pin / 8) * 4;
-	bit = (pin % 8) * 2;
+	mask = (bank->iomux[iomux_num].type & IOMUX_WIDTH_4BIT) ? 0xf : 0x3;
+	reg = bank->iomux[iomux_num].offset;
+	if (bank->iomux[iomux_num].type & IOMUX_WIDTH_4BIT) {
+		if ((pin % 8) >= 4)
+			reg += 0x4;
+		bit = (pin % 4) * 4;
+	} else {
+		bit = (pin % 8) * 2;
+	}
 
-	ret = regmap_read(info->regmap_base, reg, &val);
+	ret = regmap_read(regmap, reg, &val);
 	if (ret)
 		return ret;
 
-	return ((val >> bit) & 3);
+	return ((val >> bit) & mask);
 }
 
 /*
@@ -379,16 +434,14 @@ static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin)
 static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 {
 	struct rockchip_pinctrl *info = bank->drvdata;
-	int reg, ret;
+	int iomux_num = (pin / 8);
+	struct regmap *regmap;
+	int reg, ret, mask;
 	unsigned long flags;
 	u8 bit;
 	u32 data;
 
-	/*
-	 * The first 16 pins of rk3188_bank0 are always gpios and do not have
-	 * a mux register at all.
-	 */
-	if (bank->bank_type == RK3188_BANK0 && pin < 16) {
+	if (bank->iomux[iomux_num].type & IOMUX_GPIO_ONLY) {
 		if (mux != RK_FUNC_GPIO) {
 			dev_err(info->dev,
 				"pin %d only supports a gpio mux\n", pin);
@@ -401,17 +454,25 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 	dev_dbg(info->dev, "setting mux of GPIO%d-%d to %d\n",
 						bank->bank_num, pin, mux);
 
+	regmap = (bank->iomux[iomux_num].type & IOMUX_SOURCE_PMU)
+				? info->regmap_pmu : info->regmap_base;
+
 	/* get basic quadrupel of mux registers and the correct reg inside */
-	reg = info->ctrl->mux_offset;
-	reg += bank->bank_num * 0x10;
-	reg += (pin / 8) * 4;
-	bit = (pin % 8) * 2;
+	mask = (bank->iomux[iomux_num].type & IOMUX_WIDTH_4BIT) ? 0xf : 0x3;
+	reg = bank->iomux[iomux_num].offset;
+	if (bank->iomux[iomux_num].type & IOMUX_WIDTH_4BIT) {
+		if ((pin % 8) >= 4)
+			reg += 0x4;
+		bit = (pin % 4) * 4;
+	} else {
+		bit = (pin % 8) * 2;
+	}
 
 	spin_lock_irqsave(&bank->slock, flags);
 
-	data = (3 << (bit + 16));
-	data |= (mux & 3) << bit;
-	ret = regmap_write(info->regmap_base, reg, data);
+	data = (mask << (bit + 16));
+	data |= (mux & mask) << bit;
+	ret = regmap_write(regmap, reg, data);
 
 	spin_unlock_irqrestore(&bank->slock, flags);
 
@@ -475,6 +536,38 @@ static void rk3188_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
 	}
 }
 
+//FIXME: what is the real first pull registers
+//FIXME: GPIO0A_PULL would be at 0x130 of the GRF, if it weren't in the pmu
+//GPIO0A_PULL - GPIO0C_PULL are in the pmu instead
+#define RK3288_PULL_OFFSET		0x140
+static void rk3288_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
+				    int pin_num, struct regmap **regmap,
+				    int *reg, u8 *bit)
+{
+	struct rockchip_pinctrl *info = bank->drvdata;
+
+	/* The first 24 pins of the first bank are located elsewhere */
+	if (bank->bank_num == 0 && pin_num < 24) {
+		*regmap = info->regmap_pmu;
+		*reg = RK3188_PULL_PMU_OFFSET;
+
+		*reg += ((pin_num / RK3188_PULL_PINS_PER_REG) * 4);
+		*bit = pin_num % RK3188_PULL_PINS_PER_REG;
+		*bit *= RK3188_PULL_BITS_PER_PIN;
+	} else {
+		*regmap = info->regmap_base;
+		*reg = RK3288_PULL_OFFSET;
+
+		/* correct the offset, as it is the 4th pull register */
+		*reg -= 0x10;
+		*reg += bank->bank_num * RK3188_PULL_BANK_STRIDE;
+		*reg += ((pin_num / RK3188_PULL_PINS_PER_REG) * 4);
+
+		*bit = (pin_num % RK3188_PULL_PINS_PER_REG);
+		*bit *= RK3188_PULL_BITS_PER_PIN;
+	}
+}
+
 static int rockchip_get_pull(struct rockchip_pin_bank *bank, int pin_num)
 {
 	struct rockchip_pinctrl *info = bank->drvdata;
@@ -1510,7 +1603,7 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data(
 	struct device_node *np;
 	struct rockchip_pin_ctrl *ctrl;
 	struct rockchip_pin_bank *bank;
-	int i;
+	int grf_offs, pmu_offs, i, j;
 
 	match = of_match_node(rockchip_pinctrl_dt_match, node);
 	ctrl = (struct rockchip_pin_ctrl *)match->data;
@@ -1532,12 +1625,45 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data(
 		}
 	}
 
+	grf_offs = ctrl->grf_mux_offset;
+	pmu_offs = ctrl->pmu_mux_offset;
 	bank = ctrl->pin_banks;
 	for (i = 0; i < ctrl->nr_banks; ++i, ++bank) {
 		spin_lock_init(&bank->slock);
 		bank->drvdata = d;
 		bank->pin_base = ctrl->nr_pins;
 		ctrl->nr_pins += bank->nr_pins;
+
+		/* calculate iomux offsets */
+		for (j = 0; j < 4; j++) {
+			struct rockchip_iomux *iom = &bank->iomux[j];
+			int inc;
+
+			/* preset offset value, set new start value */
+			if (iom->offset >= 0) {
+				if (iom->type & IOMUX_SOURCE_PMU)
+					pmu_offs = iom->offset;
+				else
+					grf_offs = iom->offset;
+			} else { /* set current offset */
+				iom->offset = (iom->type & IOMUX_SOURCE_PMU) ?
+							pmu_offs : grf_offs;
+			}
+
+			dev_info(d->dev, "bank %d, iomux %d has offset 0x%x\n",
+				 i, j, iom->offset);
+
+			/*
+			 * Increase offset according to iomux width.
+			 * 4bit iomux'es are spread over two registers.
+			 */
+			inc = (iom->type & IOMUX_WIDTH_4BIT) ? 8 : 4;
+			if (iom->type & IOMUX_SOURCE_PMU)
+				pmu_offs += inc;
+			else
+				grf_offs += inc;
+		}
+
 	}
 
 	return ctrl;
@@ -1641,7 +1767,7 @@ static struct rockchip_pin_ctrl rk2928_pin_ctrl = {
 		.nr_banks		= ARRAY_SIZE(rk2928_pin_banks),
 		.label			= "RK2928-GPIO",
 		.type			= RK2928,
-		.mux_offset		= 0xa8,
+		.grf_mux_offset		= 0xa8,
 		.pull_calc_reg		= rk2928_calc_pull_reg_and_bit,
 };
 
@@ -1659,7 +1785,7 @@ static struct rockchip_pin_ctrl rk3066a_pin_ctrl = {
 		.nr_banks		= ARRAY_SIZE(rk3066a_pin_banks),
 		.label			= "RK3066a-GPIO",
 		.type			= RK2928,
-		.mux_offset		= 0xa8,
+		.grf_mux_offset		= 0xa8,
 		.pull_calc_reg		= rk2928_calc_pull_reg_and_bit,
 };
 
@@ -1675,11 +1801,11 @@ static struct rockchip_pin_ctrl rk3066b_pin_ctrl = {
 		.nr_banks	= ARRAY_SIZE(rk3066b_pin_banks),
 		.label		= "RK3066b-GPIO",
 		.type		= RK3066B,
-		.mux_offset	= 0x60,
+		.grf_mux_offset	= 0x60,
 };
 
 static struct rockchip_pin_bank rk3188_pin_banks[] = {
-	PIN_BANK(0, 32, "gpio0"),
+	PIN_BANK_IOMUX_FLAGS(0, 32, "gpio0", IOMUX_GPIO_ONLY, 0, 0, 0),
 	PIN_BANK(1, 32, "gpio1"),
 	PIN_BANK(2, 32, "gpio2"),
 	PIN_BANK(3, 32, "gpio3"),
@@ -1690,10 +1816,56 @@ static struct rockchip_pin_ctrl rk3188_pin_ctrl = {
 		.nr_banks		= ARRAY_SIZE(rk3188_pin_banks),
 		.label			= "RK3188-GPIO",
 		.type			= RK3188,
-		.mux_offset		= 0x60,
+		.grf_mux_offset		= 0x60,
 		.pull_calc_reg		= rk3188_calc_pull_reg_and_bit,
 };
 
+static struct rockchip_pin_bank rk3288_pin_banks[] = {
+	PIN_BANK_IOMUX_FLAGS(0, 32, "gpio0", IOMUX_SOURCE_PMU,
+					     IOMUX_SOURCE_PMU,
+					     IOMUX_SOURCE_PMU,
+					     IOMUX_SOURCE_PMU | IOMUX_GPIO_ONLY
+			    ),
+	PIN_BANK_IOMUX_FLAGS(1, 32, "gpio1", IOMUX_GPIO_ONLY,
+					     IOMUX_GPIO_ONLY,
+					     IOMUX_GPIO_ONLY,
+					     0
+			    ),
+	PIN_BANK_IOMUX_FLAGS(2, 32, "gpio2", 0, 0, 0, IOMUX_GPIO_ONLY),
+	PIN_BANK_IOMUX_FLAGS(3, 32, "gpio3", 0, 0, 0, IOMUX_WIDTH_4BIT),
+	PIN_BANK_IOMUX_FLAGS(4, 32, "gpio4", IOMUX_WIDTH_4BIT,
+					     IOMUX_WIDTH_4BIT,
+					     0,
+					     0
+			    ),
+	PIN_BANK_IOMUX_FLAGS(5, 32, "gpio5", IOMUX_GPIO_ONLY,
+					     0,
+					     0,
+					     IOMUX_GPIO_ONLY
+			    ),
+	PIN_BANK_IOMUX_FLAGS(6, 32, "gpio6", 0, 0, 0, IOMUX_GPIO_ONLY),
+	PIN_BANK_IOMUX_FLAGS(7, 32, "gpio7", 0,
+					     0,
+					     IOMUX_WIDTH_4BIT,
+					     IOMUX_GPIO_ONLY
+			    ),
+	PIN_BANK_IOMUX_FLAGS(8, 32, "gpio8", 0,
+					     0,
+					     IOMUX_GPIO_ONLY,
+					     IOMUX_GPIO_ONLY
+			    ),
+};
+
+static struct rockchip_pin_ctrl rk3288_pin_ctrl = {
+		.pin_banks		= rk3288_pin_banks,
+		.nr_banks		= ARRAY_SIZE(rk3288_pin_banks),
+		.label			= "RK3288-GPIO",
+		.type			= RK3188,
+		.grf_mux_offset		= 0x0,
+		.pmu_mux_offset		= 0x84,
+		.pull_calc_reg		= rk3288_calc_pull_reg_and_bit,
+};
+
 static const struct of_device_id rockchip_pinctrl_dt_match[] = {
 	{ .compatible = "rockchip,rk2928-pinctrl",
 		.data = (void *)&rk2928_pin_ctrl },
@@ -1703,6 +1875,8 @@ static const struct of_device_id rockchip_pinctrl_dt_match[] = {
 		.data = (void *)&rk3066b_pin_ctrl },
 	{ .compatible = "rockchip,rk3188-pinctrl",
 		.data = (void *)&rk3188_pin_ctrl },
+	{ .compatible = "rockchip,rk3288-pinctrl",
+		.data = (void *)&rk3288_pin_ctrl },
 	{},
 };
 MODULE_DEVICE_TABLE(of, rockchip_pinctrl_dt_match);
-- 
1.9.0

Re: syscon or memory mappings (was: Re: [RFC/PATCH 0/8] pinctrl-rockchip: Change wrong initial assumptions)

[Max Schwarz]

Hello Heiko,

On Thursday 01 May 2014 at 15:21:34, Heiko Stübner wrote:
> Hi Max,
> 
> Am Donnerstag, 1. Mai 2014, 12:43:13 schrieb Max Schwarz:
> > On Wednesday 30 April 2014 at 00:07:12, Heiko Stübner wrote:
> > > While this wasn't a problem until now, the upcoming rk3288 introduces
> > > additional changes to both the grf and pmu areas. On it even part of
> > > the pinmux registers move into the pmu space.
> > 
> > Could you give us more information on that? I tried to find details on the
> > RK3288 but came up with nothing. How are the pinmux registers divided?
> 
> Some days ago, Rockchip released kernel sources for the rk3288 [0]. They
> took a lot of our current mainline code as base for their kernel. AS you
> can see in the register map below, the pinmux registers for the gpio0 bank
> are residing in the pmu space, while gpio1-8 are residing in the regular
> "general register files"

Wow, that's interesting. Seems they invested some real effort to catch up 
instead of simply modifying their old patches. Which is a compliment to your 
contributions!

Maybe we should try to get one of the Rockchip developers on board. They must 
have thought about this kind of thing as well.

> To elaborate a bit:
> 
> On rk3188 it is
> GRF: 0x00 - 0x5c: pin suspend control
> GRF: 0x60 - 0x9c: pinmux control (0x60 and 0x64 gpio-only)
> GRF: 0xa0 - 0xac: soc-control/status
> GRF: 0xb0 - 0xc8: dma-control
> GRF: 0xcc - 0xe0: "cpu core configuration"
> GRF: 0xec - 0xf0: ddr-controller config
> GRF: 0xf4 - 0x104: pin drive-strength (what we currently do not support)
> GRF: 0x108: soc_status1
> GRF: 0x10c - 0x140: USB phy control
> GRF: 0x144 - 0x160: "OS register"
> GRF: 0x160 - 0x19c: pin pull settings
> PMU: 0x00 - 0x38: power-domains and a lot of unknown stuff
> PMU: 0x3c: something called GPIO0_CON, what Rockchip does not use at all
> PMU: 0x40 - 0x60: "SYS_REGx"
> PMU: 0x64 - 0x68: part of GPIO0 pull config
> 
> so we would/will in the end need 4 mappings for the rk3188-pinctrl
> GRF: 0x00 - 0x9c, GRF: 0xf4 - 0x104, GRF: 0x160 - 0x19c, PMU: 0x64 - 0x68
> 
> 
> On rk3288 it is
> 
> GRF: 0x00 - 0x84: gpio1-gpio8 iomux settings
> GRF: 0x104 - 0x138: unknown GPIOxx_SR registers
> GRF: 0x140 - 0x1b4: gpio1-gpio8 pull settings
> GRF: 0x1c0 - 0x234: gpio1-gpio8 driver strength settings
> GRF: 0x240: unknown/unused GPIO_SMT
> GRF: 0x244 - 0x2d4: soc control/status registers
> GRF: 0x2e0 - ...: a lot of stuff like dma, usb-phy etc.
> PMU: 0x00 - 0x5c: powerdomains and a lot of other stuff
> PMU: 0x60: GPIO_SR
> PMU: 0x64 - 0x6c: gpio0 pull settings
> PMU: 0x70 - 0x78: gpio0 drive-strength settings
> PMU: 0x7c: GPIO_OP
> PMU: 0x80: GPIO0_SEL18
> PMU: 0x84 - 0x8c: gpio0 pinmux settings
> PMU: 0x90 - 0xa0: more misc registers (powermode, sys_regX)
> 
> so we would essentially need only two mappings here
> GRF: 0x00 - 0x240 and PMU: 0x60 - 0x8c
>
> So we'd need additional if(is_rk3188()) conditionals to distinguish between
> these implementations [and possible future ones] to select the correct base
> address, and we don't know what the next SoC will bring and how the stuff
> will be ordered there.

Thanks for providing the register mappings.

Yes, if you do specify the mappings as you proposed it would be a nightmare.

However, this sheds light on an underlying issue: Rockchip is not treating the 
whole GPIO block as one cohesive device as we do currently. Instead, it seems 
to me, one GPIO bank is one device. Each has its cohesive mux, bank and pull 
registers - apart from rk3188-bank-0, maybe. But that one is special anyway 
with regards to register ordering (s.b.).

The issues you had with RK3188 and now have with RK3288 seem to stem from 
trying to group all banks together into one pinctrl driver.

So maybe we should promote the GPIO banks to full devices in the dt and make 
smaller mappings for each GPIO bank, i.e. three mappings for each GPIO bank 
(mux, bank, pull). I know we have to stay backwards compatible dt-wise, but 
that should be doable.

Then we are fully flexible and don't need any conditionals or address 
calculation logic. And should a future SoC bring another layout inside the 
banks, we can react with a new "compatible"-name (and maybe a completely 
separate driver, if the change is big enough).

> Also leaving the driver behind, devicetree is meant to describe the
> hardware, not the implementation. And hw-wise both PMU and GRF are actual
> hardware blocks even with individual clock gates.

Whatever a "hardware block" is. n:m mappings between devices and clocks are no 
problems in the dt. So why not describe things a bit more precisely?

> Citing the syscon-devicetree bindings:
> 
> 	System controller node represents a register region containing a set
> 	of miscellaneous registers. The registers are not cohesive enough to
> 	represent as any specific type of device.
> 
> So to me both GRF and PMU regions scream "syscon".

Yes, that sounds a bit like the mess we are dealing with ;-)
I still feel that declaring everything as syscon is somehow circumventing the 
dt. And I feel more comfortable declaring GRF_SOC_* as "miscellaneous 
registers" rather than e.g. the iomux space of GPIO0.

Don't get me wrong please, I'm not completely against the syscon idea. I'm 
just trying to have a full discussion on the issue.

> I've attached my current WIP patch to implement rk3288 support (untested, as
> I don't have any hardware), based on this series. As you can see in it, the
> rk3288 has even more peculiarities with gpio-only and 4bit wide iomuxes.

Nice, but you needed to introduce flags like "SOURCE_PMU/GRF" which would not 
be necessary with the fine-grained mapping. GPIO-Only could be handled by a 
mask specified in the dt.

> As the patch stands now, rk3288 doesn't even need special handling for its
> iomux registers, as it can be simply described now in the pin-bank
> declaration at the bottom - and even the rk3188-bank0 wouldn't be necessary
> anymore.

       /*
		 * The bits in these registers have an inverse ordering
		 * with the lowest pin being in bits 15:14 and the highest
		 * pin in bits 1:0
		 */
		*bit = 7 - (pin_num % RK3188_PULL_PINS_PER_REG);

(from rk3188_calc_pull_reg_and_bit)

That's probably still a peculiarity of rk3188-bank0, isn't it? So we'd still 
need a conditional on rk3188-bank0. That could enable a fourth mapping for the 
split up mux space of bank0 as well.

Compared to your RK3288 patch we'd be moving the information from the table in 
your driver (which is just describing the hw layout & capabilities) into the 
dt.

> > There are some question marks for me on the syscon solution. Regmap uses
> > locking internally, which means separate drivers can't access separate
> > registers simultaneously. We have an SMP machine here, so that's not far-
> > fetched. And that locking is completely unnecessary, as we *know* in most
> > cases that the accessed areas do not overlap.
> 
> For locking vs. speed, I do not see this as a big problem. All registers in
> there mainly contain general settings that are not changed often during the
> operation of the device. So there is no high frequency access to them in any
> case.

Agreed, that's probably not an issue (if no one wants to do high-speed 
concurrent bitbang I/O :D).

> > > The other option would be to leave the grf as it is and create separate
> > > syscons for real small individual parts like the soc-conf and usb-phys.
> > > But apart from creating these real small syscons that would
> > > also make it necessary to introduce another register map for the
> > > drive-strength settings of the pin-controller, which are sitting in the
> > > middle of everything at least on rk3066 and rk3188.
> > 
> > Wy do we need a syscon for usb-phys? Is it shared by multiple drivers?
> > My instinctive approach would be two usb-phys devices mapping the
> > GRF_UOC0/1 spaces directly via reg properties. Or did I miss something?
> 
> Of course if we're going to map each part of the GRF individually there is
> no need for a syscon.

Okay, sorry for misunderstanding.

> I'm also hoping for more input so I've changed the title a bit, to maybe 
> attract more people :-).

Yes, let's hope someone else speaks up. Maybe there has already been a 
precedent in another mach-*? I'll try to find something similar.
In the end you as the maintainer have to make the decision though. And as I 
said I don't have real problems with the syscon solution, it just doesn't feel 
nice.

Cheers,
  Max
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: syscon or memory mappings (was: Re: [RFC/PATCH 0/8] pinctrl-rockchip: Change wrong initial assumptions)

[Heiko Stübner]

Hi Max,

Am Freitag, 2. Mai 2014, 15:59:05 schrieb Max Schwarz:
> On Thursday 01 May 2014 at 15:21:34, Heiko Stübner wrote:
> > On rk3188 it is
> > GRF: 0x00 - 0x5c: pin suspend control
> > GRF: 0x60 - 0x9c: pinmux control (0x60 and 0x64 gpio-only)
> > GRF: 0xa0 - 0xac: soc-control/status
> > GRF: 0xb0 - 0xc8: dma-control
> > GRF: 0xcc - 0xe0: "cpu core configuration"
> > GRF: 0xec - 0xf0: ddr-controller config
> > GRF: 0xf4 - 0x104: pin drive-strength (what we currently do not support)
> > GRF: 0x108: soc_status1
> > GRF: 0x10c - 0x140: USB phy control
> > GRF: 0x144 - 0x160: "OS register"
> > GRF: 0x160 - 0x19c: pin pull settings
> > PMU: 0x00 - 0x38: power-domains and a lot of unknown stuff
> > PMU: 0x3c: something called GPIO0_CON, what Rockchip does not use at all
> > PMU: 0x40 - 0x60: "SYS_REGx"
> > PMU: 0x64 - 0x68: part of GPIO0 pull config
> > 
> > so we would/will in the end need 4 mappings for the rk3188-pinctrl
> > GRF: 0x00 - 0x9c, GRF: 0xf4 - 0x104, GRF: 0x160 - 0x19c, PMU: 0x64 - 0x68
> > 
> > 
> > On rk3288 it is
> > 
> > GRF: 0x00 - 0x84: gpio1-gpio8 iomux settings
> > GRF: 0x104 - 0x138: unknown GPIOxx_SR registers
> > GRF: 0x140 - 0x1b4: gpio1-gpio8 pull settings
> > GRF: 0x1c0 - 0x234: gpio1-gpio8 driver strength settings
> > GRF: 0x240: unknown/unused GPIO_SMT
> > GRF: 0x244 - 0x2d4: soc control/status registers
> > GRF: 0x2e0 - ...: a lot of stuff like dma, usb-phy etc.
> > PMU: 0x00 - 0x5c: powerdomains and a lot of other stuff
> > PMU: 0x60: GPIO_SR
> > PMU: 0x64 - 0x6c: gpio0 pull settings
> > PMU: 0x70 - 0x78: gpio0 drive-strength settings
> > PMU: 0x7c: GPIO_OP
> > PMU: 0x80: GPIO0_SEL18
> > PMU: 0x84 - 0x8c: gpio0 pinmux settings
> > PMU: 0x90 - 0xa0: more misc registers (powermode, sys_regX)
> > 
> > so we would essentially need only two mappings here
> > GRF: 0x00 - 0x240 and PMU: 0x60 - 0x8c
> > 
> > So we'd need additional if(is_rk3188()) conditionals to distinguish
> > between
> > these implementations [and possible future ones] to select the correct
> > base
> > address, and we don't know what the next SoC will bring and how the stuff
> > will be ordered there.
> 
> Thanks for providing the register mappings.
> 
> Yes, if you do specify the mappings as you proposed it would be a nightmare.
> 
> However, this sheds light on an underlying issue: Rockchip is not treating
> the whole GPIO block as one cohesive device as we do currently. Instead, it
> seems to me, one GPIO bank is one device. Each has its cohesive mux, bank
> and pull registers - apart from rk3188-bank-0, maybe. But that one is
> special anyway with regards to register ordering (s.b.).
> 
> The issues you had with RK3188 and now have with RK3288 seem to stem from
> trying to group all banks together into one pinctrl driver.
>
> So maybe we should promote the GPIO banks to full devices in the dt and make
> smaller mappings for each GPIO bank, i.e. three mappings for each GPIO bank
> (mux, bank, pull). I know we have to stay backwards compatible dt-wise, but
> that should be doable.

Yes, that is another miss-conception from the early days. The gpio-controllers 
themself are actually Synopsis Designware IPs - the kernel now even has a 
separate driver for them (gpio-dwapb). Only the real 
pincontrol/muxing/pull/etc is from Rockchip.

Currently I can't think of a way to move over gracefully, without introducing 
crap into the gpio-dwapb driver. As at the moment it parses all information it 
needs from the dt directly - of course with different bindings.

That is also the reason I do not want to introduce more "special-cases" in the 
bank-declaration we're using currently - to not make this move more difficult in 
the future.

As it is, with the patch attached to the last mail, the pinctrl driver 
wouldn't even need the "rockchip,rk3188-gpio-bank0" compatible anymore, as the 
information about its special case is now sitting in the bank declaration 
inside the driver. Which then would enable us to remove the gpio-bank subnodes 
alltogether and use the external gpio driver.


> Then we are fully flexible and don't need any conditionals or address
> calculation logic. And should a future SoC bring another layout inside the
> banks, we can react with a new "compatible"-name (and maybe a completely
> separate driver, if the change is big enough).

Especially the rk3288 seems to introduce a lot more variants. So if we were to 
really split each pin-bank into a separate node, each bank would need to be 
handled differently - as they all have one peculiarity or another.



> > Also leaving the driver behind, devicetree is meant to describe the
> > hardware, not the implementation. And hw-wise both PMU and GRF are actual
> > hardware blocks even with individual clock gates.
> 
> Whatever a "hardware block" is. n:m mappings between devices and clocks are
> no problems in the dt. So why not describe things a bit more precisely?
> > Citing the syscon-devicetree bindings:
> > 	System controller node represents a register region containing a set
> > 	of miscellaneous registers. The registers are not cohesive enough to
> > 	represent as any specific type of device.
> > 
> > So to me both GRF and PMU regions scream "syscon".
> 
> Yes, that sounds a bit like the mess we are dealing with ;-)
> I still feel that declaring everything as syscon is somehow circumventing
> the dt. And I feel more comfortable declaring GRF_SOC_* as "miscellaneous
> registers" rather than e.g. the iomux space of GPIO0.
> 
> Don't get me wrong please, I'm not completely against the syscon idea. I'm
> just trying to have a full discussion on the issue.

Oh, me too :-) . I'm not entirely fixated on this idea, but using a syscon 
feels somehow naturally to me in this case.

While for example syscon cannot handle clocks currently, the underlying regmap 
can - so it would be only a matter of teaching syscon to tell regmap of the 
clock to use (GRF and PMU register-clocks in this case), instead of needing to 
have every user of parts of these registers handle the relevant clock itself 
on register accesses.

Also, as you can see in the grf map, the rk3188 actually has two soc_status 
registers (0xac and 0x108) which have the dmac, cpu and drive strength 
registers in between. So having a syscon only for soc_con and soc_status will 
produce problems too.

Another example, GRF_IO_VSEL at 0x0380 is most likely some sort of pin voltage 
selection. As it only spans 1 register I'd assume it could contain settings 
that span all pin banks.

And of course, splitting the register space into dozens of small individual 
mappings looks messy :-)


> > I've attached my current WIP patch to implement rk3288 support (untested,
> > as I don't have any hardware), based on this series. As you can see in
> > it, the rk3288 has even more peculiarities with gpio-only and 4bit wide
> > iomuxes.
> Nice, but you needed to introduce flags like "SOURCE_PMU/GRF" which would
> not be necessary with the fine-grained mapping. GPIO-Only could be handled
> by a mask specified in the dt.

While it is true, that on the rk3288 all gpio0-iomux settings are residing the 
in the pmu, this is not necessarily true for everything. Like on the rk3188, 
the iomux registers of bank0 are in the GRF space while parts of the pull 
settings are in the PMU space.


> > As the patch stands now, rk3288 doesn't even need special handling for its
> > iomux registers, as it can be simply described now in the pin-bank
> > declaration at the bottom - and even the rk3188-bank0 wouldn't be
> > necessary
> > anymore.
> 
>        /*
> 		 * The bits in these registers have an inverse ordering
> 		 * with the lowest pin being in bits 15:14 and the highest
> 		 * pin in bits 1:0
> 		 */
> 		*bit = 7 - (pin_num % RK3188_PULL_PINS_PER_REG);
> 
> (from rk3188_calc_pull_reg_and_bit)
> 
> That's probably still a peculiarity of rk3188-bank0, isn't it? So we'd still
> need a conditional on rk3188-bank0. That could enable a fourth mapping for
> the split up mux space of bank0 as well.

No, the pull-pins are reversed inside the register on all banks of the rk3188.


> > I'm also hoping for more input so I've changed the title a bit, to maybe
> > attract more people :-).
> 
> Yes, let's hope someone else speaks up. Maybe there has already been a
> precedent in another mach-*? I'll try to find something similar.
> In the end you as the maintainer have to make the decision though. And as I
> said I don't have real problems with the syscon solution, it just doesn't
> feel nice.

Actually Linus Walleij is the pinctrl maintainer, so he'll also needs to be 
happy with what we come up here :-) .


Heiko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: syscon or memory mappings (was: Re: [RFC/PATCH 0/8] pinctrl-rockchip: Change wrong initial assumptions)

[Max Schwarz]

Hello Heiko,

On Saturday 03 May 2014 at 01:45:11, Heiko Stübner wrote:
> > However, this sheds light on an underlying issue: Rockchip is not treating
> > the whole GPIO block as one cohesive device as we do currently. Instead,
> > it
> > seems to me, one GPIO bank is one device. Each has its cohesive mux, bank
> > and pull registers - apart from rk3188-bank-0, maybe. But that one is
> > special anyway with regards to register ordering (s.b.).
> > 
> > The issues you had with RK3188 and now have with RK3288 seem to stem from
> > trying to group all banks together into one pinctrl driver.
> > 
> > So maybe we should promote the GPIO banks to full devices in the dt and
> > make smaller mappings for each GPIO bank, i.e. three mappings for each
> > GPIO bank (mux, bank, pull). I know we have to stay backwards compatible
> > dt-wise, but that should be doable.
> 
> Yes, that is another miss-conception from the early days. The
> gpio-controllers themself are actually Synopsis Designware IPs - the kernel
> now even has a separate driver for them (gpio-dwapb). Only the real
> pincontrol/muxing/pull/etc is from Rockchip.
> 
> Currently I can't think of a way to move over gracefully, without
> introducing crap into the gpio-dwapb driver. As at the moment it parses all
> information it needs from the dt directly - of course with different
> bindings.
> 
> That is also the reason I do not want to introduce more "special-cases" in
> the bank-declaration we're using currently - to not make this move more
> difficult in the future.
> 
> As it is, with the patch attached to the last mail, the pinctrl driver
> wouldn't even need the "rockchip,rk3188-gpio-bank0" compatible anymore, as
> the information about its special case is now sitting in the bank
> declaration inside the driver. Which then would enable us to remove the
> gpio-bank subnodes alltogether and use the external gpio driver.

Okay, you convinced me. That sounds like a good plan to me. Maybe we can 
introduce some compability code into the pinctrl driver to generate the 
correct dt nodes for gpio-dwabp at runtime? I think that would depend on 
something like CONFIG_OF_DYNAMIC though.

> While for example syscon cannot handle clocks currently, the underlying
> regmap can - so it would be only a matter of teaching syscon to tell regmap
> of the clock to use (GRF and PMU register-clocks in this case), instead of
> needing to have every user of parts of these registers handle the relevant
> clock itself on register accesses.
> 
> Also, as you can see in the grf map, the rk3188 actually has two soc_status
> registers (0xac and 0x108) which have the dmac, cpu and drive strength
> registers in between. So having a syscon only for soc_con and soc_status
> will produce problems too.
> 
> Another example, GRF_IO_VSEL at 0x0380 is most likely some sort of pin
> voltage selection. As it only spans 1 register I'd assume it could contain
> settings that span all pin banks.

Okay, we couldn't handle that with individual devices for each bank.

> And of course, splitting the register space into dozens of small individual
> mappings looks messy :-)

Yes, I agree now ;-)

Cheers,
  Max
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/8] pinctrl: rockchip: use regmaps instead of raw mappings

[Max Schwarz]

Hi Heiko,

On Wednesday 30 April 2014 at 00:08:19, Heiko Stübner wrote:
> This allows us to use syscons in the future.
> 
> Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 116
> +++++++++++++++++++++++++------------ 1 file changed, 80 insertions(+), 36
> deletions(-)

You should add a "select REGMAP_MMIO" in drivers/pinctrl/Kconfig to make sure 
it compiles. Or just add MFD_SYSCON since it depends on REGMAP_MMIO and is a 
runtime dependency for the new binding now.

Otherwise I tested your series with my tree on RK3188/Radxa Rock and 
everything works.

Cheers,
  Max

> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index ab71de8..71d9c99 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -37,6 +37,7 @@
>  #include <linux/pinctrl/pinconf-generic.h>
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/clk.h>
> +#include <linux/regmap.h>
>  #include <dt-bindings/pinctrl/rockchip.h>
> 
>  #include "core.h"
> @@ -86,7 +87,7 @@ enum rockchip_pin_bank_type {
>   */
>  struct rockchip_pin_bank {
>  	void __iomem			*reg_base;
> -	void __iomem			*reg_pull;
> +	struct regmap			*regmap_pull;
>  	struct clk			*clk;
>  	int				irq;
>  	u32				pin_base;
> @@ -120,8 +121,9 @@ struct rockchip_pin_ctrl {
>  	char				*label;
>  	enum rockchip_pinctrl_type	type;
>  	int				mux_offset;
> -	void	(*pull_calc_reg)(struct rockchip_pin_bank *bank, int pin_num,
> -				 void __iomem **reg, u8 *bit);
> +	void	(*pull_calc_reg)(struct rockchip_pin_bank *bank,
> +				    int pin_num, struct regmap **regmap,
> +				    int *reg, u8 *bit);
>  };
> 
>  struct rockchip_pin_config {
> @@ -159,9 +161,9 @@ struct rockchip_pmx_func {
>  };
> 
>  struct rockchip_pinctrl {
> -	void __iomem			*reg_base;
> +	struct regmap			*regmap_base;
>  	int				reg_size;
> -	void __iomem			*reg_pull;
> +	struct regmap			*regmap_pull;
>  	struct device			*dev;
>  	struct rockchip_pin_ctrl	*ctrl;
>  	struct pinctrl_desc		pctl;
> @@ -172,6 +174,12 @@ struct rockchip_pinctrl {
>  	unsigned int			nfunctions;
>  };
> 
> +static struct regmap_config rockchip_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +};
> +
>  static inline struct rockchip_pin_bank *gc_to_pin_bank(struct gpio_chip
> *gc) {
>  	return container_of(gc, struct rockchip_pin_bank, gpio_chip);
> @@ -333,18 +341,24 @@ static const struct pinctrl_ops rockchip_pctrl_ops = {
> static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin) {
>  	struct rockchip_pinctrl *info = bank->drvdata;
> -	void __iomem *reg = info->reg_base + info->ctrl->mux_offset;
> +	unsigned int val;
> +	int reg, ret;
>  	u8 bit;
> 
>  	if (bank->bank_type == RK3188_BANK0 && pin < 16)
>  		return RK_FUNC_GPIO;
> 
>  	/* get basic quadrupel of mux registers and the correct reg inside */
> +	reg = info->ctrl->mux_offset;
>  	reg += bank->bank_num * 0x10;
>  	reg += (pin / 8) * 4;
>  	bit = (pin % 8) * 2;
> 
> -	return ((readl(reg) >> bit) & 3);
> +	ret = regmap_read(info->regmap_base, reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	return ((val >> bit) & 3);
>  }
> 
>  /*
> @@ -363,7 +377,7 @@ static int rockchip_get_mux(struct rockchip_pin_bank
> *bank, int pin) static int rockchip_set_mux(struct rockchip_pin_bank *bank,
> int pin, int mux) {
>  	struct rockchip_pinctrl *info = bank->drvdata;
> -	void __iomem *reg = info->reg_base + info->ctrl->mux_offset;
> +	int reg, ret;
>  	unsigned long flags;
>  	u8 bit;
>  	u32 data;
> @@ -386,6 +400,7 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux) bank->bank_num, pin, mux);
> 
>  	/* get basic quadrupel of mux registers and the correct reg inside */
> +	reg = info->ctrl->mux_offset;
>  	reg += bank->bank_num * 0x10;
>  	reg += (pin / 8) * 4;
>  	bit = (pin % 8) * 2;
> @@ -394,11 +409,11 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux)
> 
>  	data = (3 << (bit + 16));
>  	data |= (mux & 3) << bit;
> -	writel(data, reg);
> +	ret = regmap_write(info->regmap_base, reg, data);
> 
>  	spin_unlock_irqrestore(&bank->slock, flags);
> 
> -	return 0;
> +	return ret;
>  }
> 
>  #define RK2928_PULL_OFFSET		0x118
> @@ -406,11 +421,13 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux) #define RK2928_PULL_BANK_STRIDE		8
> 
>  static void rk2928_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
> -				    int pin_num, void __iomem **reg, u8 *bit)
> +				    int pin_num, struct regmap **regmap,
> +				    int *reg, u8 *bit)
>  {
>  	struct rockchip_pinctrl *info = bank->drvdata;
> 
> -	*reg = info->reg_base + RK2928_PULL_OFFSET;
> +	*regmap = info->regmap_base;
> +	*reg = RK2928_PULL_OFFSET;
>  	*reg += bank->bank_num * RK2928_PULL_BANK_STRIDE;
>  	*reg += (pin_num / RK2928_PULL_PINS_PER_REG) * 4;
> 
> @@ -423,19 +440,23 @@ static void rk2928_calc_pull_reg_and_bit(struct
> rockchip_pin_bank *bank, #define RK3188_PULL_BANK_STRIDE		16
> 
>  static void rk3188_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
> -				    int pin_num, void __iomem **reg, u8 *bit)
> +				    int pin_num, struct regmap **regmap,
> +				    int *reg, u8 *bit)
>  {
>  	struct rockchip_pinctrl *info = bank->drvdata;
> 
>  	/* The first 12 pins of the first bank are located elsewhere */
>  	if (bank->bank_type == RK3188_BANK0 && pin_num < 12) {
> -		*reg = bank->reg_pull +
> -				((pin_num / RK3188_PULL_PINS_PER_REG) * 4);
> +		*regmap = bank->regmap_pull;
> +		*reg = 0;
> +		*reg += ((pin_num / RK3188_PULL_PINS_PER_REG) * 4);
>  		*bit = pin_num % RK3188_PULL_PINS_PER_REG;
>  		*bit *= RK3188_PULL_BITS_PER_PIN;
>  	} else {
> -		*reg = info->reg_pull ? info->reg_pull
> -				      : info->reg_base + RK3188_PULL_OFFSET;
> +		*regmap = info->regmap_pull ? info->regmap_pull
> +					    : info->regmap_base;
> +		*reg = info->regmap_pull ? 0 : RK3188_PULL_OFFSET;
> +
>  		/* correct the offset, as it is the 2nd pull register */
>  		*reg -= 4;
>  		*reg += bank->bank_num * RK3188_PULL_BANK_STRIDE;
> @@ -455,7 +476,8 @@ static int rockchip_get_pull(struct rockchip_pin_bank
> *bank, int pin_num) {
>  	struct rockchip_pinctrl *info = bank->drvdata;
>  	struct rockchip_pin_ctrl *ctrl = info->ctrl;
> -	void __iomem *reg;
> +	struct regmap *regmap;
> +	int reg, ret;
>  	u8 bit;
>  	u32 data;
> 
> @@ -463,15 +485,19 @@ static int rockchip_get_pull(struct rockchip_pin_bank
> *bank, int pin_num) if (ctrl->type == RK3066B)
>  		return PIN_CONFIG_BIAS_DISABLE;
> 
> -	ctrl->pull_calc_reg(bank, pin_num, &reg, &bit);
> +	ctrl->pull_calc_reg(bank, pin_num, &regmap, &reg, &bit);
> +
> +	ret = regmap_read(regmap, reg, &data);
> +	if (ret)
> +		return ret;
> 
>  	switch (ctrl->type) {
>  	case RK2928:
> -		return !(readl_relaxed(reg) & BIT(bit))
> +		return !(data & BIT(bit))
>  				? PIN_CONFIG_BIAS_PULL_PIN_DEFAULT
> 
>  				: PIN_CONFIG_BIAS_DISABLE;
> 
>  	case RK3188:
> -		data = readl_relaxed(reg) >> bit;
> +		data >>= bit;
>  		data &= (1 << RK3188_PULL_BITS_PER_PIN) - 1;
> 
>  		switch (data) {
> @@ -498,7 +524,8 @@ static int rockchip_set_pull(struct rockchip_pin_bank
> *bank, {
>  	struct rockchip_pinctrl *info = bank->drvdata;
>  	struct rockchip_pin_ctrl *ctrl = info->ctrl;
> -	void __iomem *reg;
> +	struct regmap *regmap;
> +	int reg, ret;
>  	unsigned long flags;
>  	u8 bit;
>  	u32 data;
> @@ -510,7 +537,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank
> *bank, if (ctrl->type == RK3066B)
>  		return pull ? -EINVAL : 0;
> 
> -	ctrl->pull_calc_reg(bank, pin_num, &reg, &bit);
> +	ctrl->pull_calc_reg(bank, pin_num, &regmap, &reg, &bit);
> 
>  	switch (ctrl->type) {
>  	case RK2928:
> @@ -519,7 +546,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank
> *bank, data = BIT(bit + 16);
>  		if (pull == PIN_CONFIG_BIAS_DISABLE)
>  			data |= BIT(bit);
> -		writel(data, reg);
> +		ret = regmap_write(regmap, reg, data);
> 
>  		spin_unlock_irqrestore(&bank->slock, flags);
>  		break;
> @@ -548,7 +575,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank
> *bank, return -EINVAL;
>  		}
> 
> -		writel(data, reg);
> +		ret = regmap_write(regmap, reg, data);
> 
>  		spin_unlock_irqrestore(&bank->slock, flags);
>  		break;
> @@ -557,7 +584,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank
> *bank, return -EINVAL;
>  	}
> 
> -	return 0;
> +	return ret;
>  }
> 
>  /*
> @@ -1416,6 +1443,7 @@ static int rockchip_get_bank_data(struct
> rockchip_pin_bank *bank, struct device *dev)
>  {
>  	struct resource res;
> +	void __iomem *base;
> 
>  	if (of_address_to_resource(bank->of_node, 0, &res)) {
>  		dev_err(dev, "cannot find IO resource for bank\n");
> @@ -1440,9 +1468,14 @@ static int rockchip_get_bank_data(struct
> rockchip_pin_bank *bank, return -ENOENT;
>  		}
> 
> -		bank->reg_pull = devm_ioremap_resource(dev, &res);
> -		if (IS_ERR(bank->reg_pull))
> -			return PTR_ERR(bank->reg_pull);
> +		base = devm_ioremap_resource(dev, &res);
> +		if (IS_ERR(base))
> +			return PTR_ERR(base);
> +		rockchip_regmap_config.max_register = resource_size(&res) - 4;
> +		rockchip_regmap_config.name = "rockchip,rk3188-gpio-bank0-pull";
> +		bank->regmap_pull = devm_regmap_init_mmio(dev, base,
> +						  &rockchip_regmap_config);
> +
>  	} else {
>  		bank->bank_type = COMMON_BANK;
>  	}
> @@ -1507,6 +1540,7 @@ static int rockchip_pinctrl_probe(struct
> platform_device *pdev) struct device *dev = &pdev->dev;
>  	struct rockchip_pin_ctrl *ctrl;
>  	struct resource *res;
> +	void __iomem *base;
>  	int ret;
> 
>  	if (!dev->of_node) {
> @@ -1527,19 +1561,29 @@ static int rockchip_pinctrl_probe(struct
> platform_device *pdev) info->dev = dev;
> 
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	info->reg_base = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(info->reg_base))
> -		return PTR_ERR(info->reg_base);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	rockchip_regmap_config.max_register = resource_size(res) - 4;
> +	rockchip_regmap_config.name = "rockchip,pinctrl";
> +	info->regmap_base = devm_regmap_init_mmio(&pdev->dev, base,
> +						  &rockchip_regmap_config);
> 
>  	/* to check for the old dt-bindings */
>  	info->reg_size = resource_size(res);
> 
>  	/* Honor the old binding, with pull registers as 2nd resource */
> -	if (ctrl->type == RK3188 &&  info->reg_size < 0x200) {
> +	if (ctrl->type == RK3188 && info->reg_size < 0x200) {
>  		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -		info->reg_pull = devm_ioremap_resource(&pdev->dev, res);
> -		if (IS_ERR(info->reg_pull))
> -			return PTR_ERR(info->reg_pull);
> +		base = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(base))
> +			return PTR_ERR(base);
> +
> +		rockchip_regmap_config.max_register = resource_size(res) - 4;
> +		rockchip_regmap_config.name = "rockchip,pinctrl-pull";
> +		info->regmap_pull = devm_regmap_init_mmio(&pdev->dev, base,
> +						  &rockchip_regmap_config);
>  	}
> 
>  	ret = rockchip_gpiolib_register(pdev, info);

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: syscon or memory mappings (was: Re: [RFC/PATCH 0/8] pinctrl-rockchip: Change wrong initial assumptions)

[Heiko Stübner]

Am Samstag, 3. Mai 2014, 14:40:36 schrieb Max Schwarz:
> Hello Heiko,
> 
> On Saturday 03 May 2014 at 01:45:11, Heiko Stübner wrote:
> > > However, this sheds light on an underlying issue: Rockchip is not
> > > treating
> > > the whole GPIO block as one cohesive device as we do currently. Instead,
> > > it
> > > seems to me, one GPIO bank is one device. Each has its cohesive mux,
> > > bank
> > > and pull registers - apart from rk3188-bank-0, maybe. But that one is
> > > special anyway with regards to register ordering (s.b.).
> > > 
> > > The issues you had with RK3188 and now have with RK3288 seem to stem
> > > from
> > > trying to group all banks together into one pinctrl driver.
> > > 
> > > So maybe we should promote the GPIO banks to full devices in the dt and
> > > make smaller mappings for each GPIO bank, i.e. three mappings for each
> > > GPIO bank (mux, bank, pull). I know we have to stay backwards compatible
> > > dt-wise, but that should be doable.
> > 
> > Yes, that is another miss-conception from the early days. The
> > gpio-controllers themself are actually Synopsis Designware IPs - the
> > kernel
> > now even has a separate driver for them (gpio-dwapb). Only the real
> > pincontrol/muxing/pull/etc is from Rockchip.
> > 
> > Currently I can't think of a way to move over gracefully, without
> > introducing crap into the gpio-dwapb driver. As at the moment it parses
> > all
> > information it needs from the dt directly - of course with different
> > bindings.
> > 
> > That is also the reason I do not want to introduce more "special-cases" in
> > the bank-declaration we're using currently - to not make this move more
> > difficult in the future.
> > 
> > As it is, with the patch attached to the last mail, the pinctrl driver
> > wouldn't even need the "rockchip,rk3188-gpio-bank0" compatible anymore, as
> > the information about its special case is now sitting in the bank
> > declaration inside the driver. Which then would enable us to remove the
> > gpio-bank subnodes alltogether and use the external gpio driver.
> 
> Okay, you convinced me. That sounds like a good plan to me. Maybe we can
> introduce some compability code into the pinctrl driver to generate the
> correct dt nodes for gpio-dwabp at runtime? I think that would depend on
> something like CONFIG_OF_DYNAMIC though.

Yep, that's one option. The other would be to let the driver support both (the 
internal gpio code and the real gpio-dwapb), deprecate the old one 
[of course also disallowing it for rk3288 and following] and simply remove it 
after the appropriate timespan (1 or more years if I remember correctly :-) ].



> > While for example syscon cannot handle clocks currently, the underlying
> > regmap can - so it would be only a matter of teaching syscon to tell
> > regmap
> > of the clock to use (GRF and PMU register-clocks in this case), instead of
> > needing to have every user of parts of these registers handle the relevant
> > clock itself on register accesses.
> > 
> > Also, as you can see in the grf map, the rk3188 actually has two
> > soc_status
> > registers (0xac and 0x108) which have the dmac, cpu and drive strength
> > registers in between. So having a syscon only for soc_con and soc_status
> > will produce problems too.
> > 
> > Another example, GRF_IO_VSEL at 0x0380 is most likely some sort of pin
> > voltage selection. As it only spans 1 register I'd assume it could contain
> > settings that span all pin banks.
> 
> Okay, we couldn't handle that with individual devices for each bank.
> 
> > And of course, splitting the register space into dozens of small
> > individual
> > mappings looks messy :-)
> 
> Yes, I agree now ;-)

cool :-D

I've also just sent a v2, including the missing MFD_SYSCON dependency, thanks 
for the find.



Heiko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html