[v2] pinctrl/nomadik: make independent of prcmu driver

Commit Message

Linus Walleij Nov. 9, 2012, 10:28 a.m. UTC

From: Jonas Aaberg <[e-mail]>

Currently there are some unnecessary criss-cross
dependencies between the PRCMU driver in MFD and a lot of
other drivers, mainly because other drivers need to poke
around in the PRCM register range.

In cases like this there are actually just a few select
registers that the pinctrl driver need to read/modify/write,
and it turns out that no other driver is actually using
these registers, so there are no concurrency issues
whatsoever.

So: don't let the location of the register range complicate
things, just poke into these registers directly and skip
a layer of indirection.

Take this opportunity to add kerneldoc to the pinctrl
state container.

Cc: Loic Pallardy <[e-mail]>
Signed-off-by: Jonas Aaberg <[e-mail]>
Signed-off-by: Linus Walleij <[e-mail]>
---
ChangeLog v1->v2:
- Have the pointer in the pin controller state container
  instead of as part of SoC data.
---
 drivers/pinctrl/pinctrl-nomadik.c | 59 ++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 20 deletions(-)

Comments

Stephen Warren Nov. 9, 2012, 5:16 p.m. UTC | #1

On 11/09/2012 03:28 AM, Linus Walleij wrote:
> From: Jonas Aaberg <[e-mail]>
> 
> Currently there are some unnecessary criss-cross
> dependencies between the PRCMU driver in MFD and a lot of
> other drivers, mainly because other drivers need to poke
> around in the PRCM register range.
> 
> In cases like this there are actually just a few select
> registers that the pinctrl driver need to read/modify/write,
> and it turns out that no other driver is actually using
> these registers, so there are no concurrency issues
> whatsoever.
> 
> So: don't let the location of the register range complicate
> things, just poke into these registers directly and skip
> a layer of indirection.
> 
> Take this opportunity to add kerneldoc to the pinctrl
> state container.

> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (res) {
> +		npct->prcm_base = devm_ioremap(&pdev->dev, res->start,
> +					       resource_size(res));
> +		if (!npct->prcm_base) {
> +			dev_err(&pdev->dev,
> +				"failed to ioremap PRCM registers\n");
> +			return -ENOMEM;
> +		}
> +	} else {
> +		dev_info(&pdev->dev,
> +			 "No PRCM base, assume no ALT-Cx control is available\n");
> +	}

Where is "assume no ALT-Cx control is available" implemented; I don't
see anything that uses npct->prcm_base to conditionally enable/block any
features. Is it just assumed that the DT won't contain any entries that
trigger writes to the PRCM registers? That seems fragile; it could cause
a "user"-triggered kernel crash.

Aside from that, this seems fine. Much smaller than V1:-)

Linus Walleij Nov. 11, 2012, 6:40 p.m. UTC | #2

On Fri, Nov 9, 2012 at 6:16 PM, Stephen Warren <[e-mail]> wrote:

> [Me]
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     if (res) {
>> +             npct->prcm_base = devm_ioremap(&pdev->dev, res->start,
>> +                                            resource_size(res));
>> +             if (!npct->prcm_base) {
>> +                     dev_err(&pdev->dev,
>> +                             "failed to ioremap PRCM registers\n");
>> +                     return -ENOMEM;
>> +             }
>> +     } else {
>> +             dev_info(&pdev->dev,
>> +                      "No PRCM base, assume no ALT-Cx control is available\n");
>> +     }
>
> Where is "assume no ALT-Cx control is available" implemented; I don't
> see anything that uses npct->prcm_base to conditionally enable/block any
> features. Is it just assumed that the DT won't contain any entries that
> trigger writes to the PRCM registers? That seems fragile; it could cause
> a "user"-triggered kernel crash.

Yeah. That's been in for a while, so this patch in itself is not making
things more fragile, it came with the first Alt-Cx patch.
So it is indeed based on good faith in the maps.

I'm poking Jean-Nicolas to have a look at hardening
this with some check, ping!

> Aside from that, this seems fine. Much smaller than V1:-)

Yeah, thanks for poking us...

Yours,
Linus Walleij

Patch

diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
index 22f6937..6a95d04 100644
--- a/drivers/pinctrl/pinctrl-nomadik.c
+++ b/drivers/pinctrl/pinctrl-nomadik.c
@@ -30,20 +30,6 @@ 
 #include <linux/pinctrl/pinconf.h>
 /* Since we request GPIOs from ourself */
 #include <linux/pinctrl/consumer.h>
-/*
- * For the U8500 archs, use the PRCMU register interface, for the older
- * Nomadik, provide some stubs. The functions using these will only be
- * called on the U8500 series.
- */
-#ifdef CONFIG_ARCH_U8500
-#include <linux/mfd/dbx500-prcmu.h>
-#else
-static inline u32 prcmu_read(unsigned int reg) {
-	return 0;
-}
-static inline void prcmu_write(unsigned int reg, u32 value) {}
-static inline void prcmu_write_masked(unsigned int reg, u32 mask, u32 value) {}
-#endif
 #include <linux/platform_data/pinctrl-nomadik.h>
 
 #include <asm/mach/irq.h>
@@ -82,10 +68,18 @@  struct nmk_gpio_chip {
 	u32 lowemi;
 };
 
+/**
+ * struct nmk_pinctrl - state container for the Nomadik pin controller
+ * @dev: containing device pointer
+ * @pctl: corresponding pin controller device
+ * @soc: SoC data for this specific chip
+ * @prcm_base: PRCM register range virtual base
+ */
 struct nmk_pinctrl {
 	struct device *dev;
 	struct pinctrl_dev *pctl;
 	const struct nmk_pinctrl_soc_data *soc;
+	void __iomem *prcm_base;
 };
 
 static struct nmk_gpio_chip *
@@ -247,6 +241,15 @@  nmk_gpio_disable_lazy_irq(struct nmk_gpio_chip *nmk_chip, unsigned offset)
 	dev_dbg(nmk_chip->chip.dev, "%d: clearing interrupt mask\n", gpio);
 }
 
+static void nmk_write_masked(void __iomem *reg, u32 mask, u32 value)
+{
+	u32 val;
+
+	val = readl(reg);
+	val = ((val & ~mask) | (value & mask));
+	writel(val, reg);
+}
+
 static void nmk_prcm_altcx_set_mode(struct nmk_pinctrl *npct,
 	unsigned offset, unsigned alt_num)
 {
@@ -285,8 +288,8 @@  static void nmk_prcm_altcx_set_mode(struct nmk_pinctrl *npct,
 			if (pin_desc->altcx[i].used == true) {
 				reg = gpiocr_regs[pin_desc->altcx[i].reg_index];
 				bit = pin_desc->altcx[i].control_bit;
-				if (prcmu_read(reg) & BIT(bit)) {
-					prcmu_write_masked(reg, BIT(bit), 0);
+				if (readl(npct->prcm_base + reg) & BIT(bit)) {
+					nmk_write_masked(npct->prcm_base + reg, BIT(bit), 0);
 					dev_dbg(npct->dev,
 						"PRCM GPIOCR: pin %i: alternate-C%i has been disabled\n",
 						offset, i+1);
@@ -314,8 +317,8 @@  static void nmk_prcm_altcx_set_mode(struct nmk_pinctrl *npct,
 		if (pin_desc->altcx[i].used == true) {
 			reg = gpiocr_regs[pin_desc->altcx[i].reg_index];
 			bit = pin_desc->altcx[i].control_bit;
-			if (prcmu_read(reg) & BIT(bit)) {
-				prcmu_write_masked(reg, BIT(bit), 0);
+			if (readl(npct->prcm_base + reg) & BIT(bit)) {
+				nmk_write_masked(npct->prcm_base + reg, BIT(bit), 0);
 				dev_dbg(npct->dev,
 					"PRCM GPIOCR: pin %i: alternate-C%i has been disabled\n",
 					offset, i+1);
@@ -327,7 +330,7 @@  static void nmk_prcm_altcx_set_mode(struct nmk_pinctrl *npct,
 	bit = pin_desc->altcx[alt_index].control_bit;
 	dev_dbg(npct->dev, "PRCM GPIOCR: pin %i: alternate-C%i has been selected\n",
 		offset, alt_index+1);
-	prcmu_write_masked(reg, BIT(bit), BIT(bit));
+	nmk_write_masked(npct->prcm_base + reg, BIT(bit), BIT(bit));
 }
 
 static void __nmk_config_pin(struct nmk_gpio_chip *nmk_chip, unsigned offset,
@@ -693,7 +696,7 @@  static int nmk_prcm_gpiocr_get_mode(struct pinctrl_dev *pctldev, int gpio)
 		if (pin_desc->altcx[i].used == true) {
 			reg = gpiocr_regs[pin_desc->altcx[i].reg_index];
 			bit = pin_desc->altcx[i].control_bit;
-			if (prcmu_read(reg) & BIT(bit))
+			if (readl(npct->prcm_base + reg) & BIT(bit))
 				return NMK_GPIO_ALT_C+i+1;
 		}
 	}
@@ -1851,6 +1854,7 @@  static int __devinit nmk_pinctrl_probe(struct platform_device *pdev)
 	const struct platform_device_id *platid = platform_get_device_id(pdev);
 	struct device_node *np = pdev->dev.of_node;
 	struct nmk_pinctrl *npct;
+	struct resource *res;
 	unsigned int version = 0;
 	int i;
 
@@ -1872,6 +1876,20 @@  static int __devinit nmk_pinctrl_probe(struct platform_device *pdev)
 	if (version == PINCTRL_NMK_DB8540)
 		nmk_pinctrl_db8540_init(&npct->soc);
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res) {
+		npct->prcm_base = devm_ioremap(&pdev->dev, res->start,
+					       resource_size(res));
+		if (!npct->prcm_base) {
+			dev_err(&pdev->dev,
+				"failed to ioremap PRCM registers\n");
+			return -ENOMEM;
+		}
+	} else {
+		dev_info(&pdev->dev,
+			 "No PRCM base, assume no ALT-Cx control is available\n");
+	}
+
 	/*
 	 * We need all the GPIO drivers to probe FIRST, or we will not be able
 	 * to obtain references to the struct gpio_chip * for them, and we
@@ -1888,6 +1906,7 @@  static int __devinit nmk_pinctrl_probe(struct platform_device *pdev)
 	nmk_pinctrl_desc.pins = npct->soc->pins;
 	nmk_pinctrl_desc.npins = npct->soc->npins;
 	npct->dev = &pdev->dev;
+
 	npct->pctl = pinctrl_register(&nmk_pinctrl_desc, &pdev->dev, npct);
 	if (!npct->pctl) {
 		dev_err(&pdev->dev, "could not register Nomadik pinctrl driver\n");