diff mbox

[17/30] pwm: samsung: repair the worst MMIO abuses

Message ID 1365638712-1028578-18-git-send-email-arnd@arndb.de (mailing list archive)
State New, archived
Headers show

Commit Message

Arnd Bergmann April 11, 2013, 12:04 a.m. UTC
The Samsung PWM driver uses "magic" pointers that are mapped
at boot time to point its MMIO registers. This fails horribly
with a multiplatform kernel, which can not rely on platform
specific header files to contain the right values, aside from
this being a really bad idea in general.

This changes the driver to at least pass an __iomem token
around in the device structure to dereference that. Fixing
the platform code is much harder, so we'll leave that
until we have a DT binding for pwm-samsung, which may require
other changes in this area. Since we are already touching
every MMIO accessor in this driver, let's also use the
proper readl_relaxed variant rather than __raw_readl.

Tomasz Figa has a set of patches to clean this up in a proper
way, but that might be too late for 3.10.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Tomasz Figa <t.figa@samsung.com>
Cc: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/pwm/pwm-samsung.c | 60 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 18 deletions(-)

Comments

Thierry Reding April 12, 2013, 7:06 a.m. UTC | #1
On Thu, Apr 11, 2013 at 02:04:59AM +0200, Arnd Bergmann wrote:
> The Samsung PWM driver uses "magic" pointers that are mapped
> at boot time to point its MMIO registers. This fails horribly
> with a multiplatform kernel, which can not rely on platform
> specific header files to contain the right values, aside from
> this being a really bad idea in general.
> 
> This changes the driver to at least pass an __iomem token
> around in the device structure to dereference that. Fixing
> the platform code is much harder, so we'll leave that
> until we have a DT binding for pwm-samsung, which may require
> other changes in this area. Since we are already touching
> every MMIO accessor in this driver, let's also use the
> proper readl_relaxed variant rather than __raw_readl.
> 
> Tomasz Figa has a set of patches to clean this up in a proper
> way, but that might be too late for 3.10.

Joonyoung Shim (Cc'ed) posted a driver for Exynos back in December. I
had a few comments on it but never saw an updated version. Is Tomasz'
work based on that patch?

Given that this patch solves a real problem and doesn't make things any
worse, I think it's okay to use it temporarily until Tomasz has had time
to finish up the proper driver, so:

Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
Tomasz Figa April 12, 2013, 7:46 a.m. UTC | #2
On Friday 12 of April 2013 09:06:35 Thierry Reding wrote:
> On Thu, Apr 11, 2013 at 02:04:59AM +0200, Arnd Bergmann wrote:
> > The Samsung PWM driver uses "magic" pointers that are mapped
> > at boot time to point its MMIO registers. This fails horribly
> > with a multiplatform kernel, which can not rely on platform
> > specific header files to contain the right values, aside from
> > this being a really bad idea in general.
> > 
> > This changes the driver to at least pass an __iomem token
> > around in the device structure to dereference that. Fixing
> > the platform code is much harder, so we'll leave that
> > until we have a DT binding for pwm-samsung, which may require
> > other changes in this area. Since we are already touching
> > every MMIO accessor in this driver, let's also use the
> > proper readl_relaxed variant rather than __raw_readl.
> > 
> > Tomasz Figa has a set of patches to clean this up in a proper
> > way, but that might be too late for 3.10.
> 
> Joonyoung Shim (Cc'ed) posted a driver for Exynos back in December. I
> had a few comments on it but never saw an updated version. Is Tomasz'
> work based on that patch?

Is this the patch you are talking about?

https://lkml.org/lkml/2012/12/13/84

If yes, it solves the problems only for Exynos SoCs (excluding Exynos 4210 
rev0), which don't need to share the PWM hardware between two drivers.

On older platforms (S3C24xx, S3C64xx, S5P64x0, S5PC100, S5PV210 and Exynos 
4210 rev0) the same hardware block is used for two drivers - clocksource 
driver and PWM driver.

You can get more information from these two threads:

- http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=16510
(starting from the linked post)

- http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464
 

> Given that this patch solves a real problem and doesn't make things any
> worse, I think it's okay to use it temporarily until Tomasz has had time
> to finish up the proper driver, so:

Well, from what I've seen, the PWM might have been already broken on non-
DT platforms using channels higher than 0, because the driver registers 
each channel passing -1 as base, which makes the id being assigned 
dynamically (usually to 0, since it's the only PWM channel used), while in 
all platform data, the real (hardware) PWM channel id is used and no PWM 
lookup is registered.

You can use the following grep to find all platforms with broken PWM:
git grep pwm_id arch/arm/mach-s[35]* | grep -v "= 0"

I have a set of patches to fix everything up in the PWM driver, but they 
depend on my patches providing the infrastructure to share the PWM 
hardware, on next version of which I'm currently working.

So, for now, Arnd's patch is fine to me as well.

Best regards,
Tomasz
diff mbox

Patch

diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index 5207e6c..9d7234d 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -22,9 +22,25 @@ 
 #include <linux/io.h>
 #include <linux/pwm.h>
 
-#include <mach/map.h>
+#ifndef CONFIG_ARCH_MULTIPLATFORM
+/*
+ * This is gross: the platform maps the timer at a fixed
+ * virtual address and expects us to use that address
+ * rather than a proper resource. This should get done properly
+ * when we get a DT binding for this driver.
+ */
+#include <plat/map-base.h>
+static inline void dummy(void)
+{
+	BUILD_BUG_ON(S3C_VA_TIMER != IOMEM(0xf6300000));
+}
+#else
+#define S3C_VA_TIMER IOMEM(0xf6300000);
+#endif
 
-#include <plat/regs-timer.h>
+#define S3C2410_TCON		8
+#define S3C2410_TCNTB(tmr)	(0x0c + (tmr)*0x0c + 0x00)
+#define S3C2410_TCMPB(tmr)	(0x0c + (tmr)*0x0c + 0x04)
 
 struct s3c_chip {
 	struct platform_device	*pdev;
@@ -38,6 +54,7 @@  struct s3c_chip {
 
 	unsigned char		 tcon_base;
 	unsigned char		 pwm_id;
+	void __iomem		*base;
 	struct pwm_chip		 chip;
 };
 
@@ -65,9 +82,9 @@  static int s3c_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	local_irq_save(flags);
 
-	tcon = __raw_readl(S3C2410_TCON);
+	tcon = readl_relaxed(s3c->base + S3C2410_TCON);
 	tcon |= pwm_tcon_start(s3c);
-	__raw_writel(tcon, S3C2410_TCON);
+	writel_relaxed(tcon, s3c->base + S3C2410_TCON);
 
 	local_irq_restore(flags);
 
@@ -82,9 +99,9 @@  static void s3c_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	local_irq_save(flags);
 
-	tcon = __raw_readl(S3C2410_TCON);
+	tcon = readl_relaxed(s3c->base + S3C2410_TCON);
 	tcon &= ~pwm_tcon_start(s3c);
-	__raw_writel(tcon, S3C2410_TCON);
+	writel_relaxed(tcon, s3c->base + S3C2410_TCON);
 
 	local_irq_restore(flags);
 }
@@ -133,8 +150,8 @@  static int s3c_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	/* The TCMP and TCNT can be read without a lock, they're not
 	 * shared between the timers. */
 
-	tcmp = __raw_readl(S3C2410_TCMPB(s3c->pwm_id));
-	tcnt = __raw_readl(S3C2410_TCNTB(s3c->pwm_id));
+	tcmp = readl_relaxed(s3c->base + S3C2410_TCMPB(s3c->pwm_id));
+	tcnt = readl_relaxed(s3c->base + S3C2410_TCNTB(s3c->pwm_id));
 
 	period = NS_IN_HZ / period_ns;
 
@@ -177,16 +194,16 @@  static int s3c_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	local_irq_save(flags);
 
-	__raw_writel(tcmp, S3C2410_TCMPB(s3c->pwm_id));
-	__raw_writel(tcnt, S3C2410_TCNTB(s3c->pwm_id));
+	writel_relaxed(tcmp, s3c->base + S3C2410_TCMPB(s3c->pwm_id));
+	writel_relaxed(tcnt, s3c->base + S3C2410_TCNTB(s3c->pwm_id));
 
-	tcon = __raw_readl(S3C2410_TCON);
+	tcon = readl_relaxed(s3c->base + S3C2410_TCON);
 	tcon |= pwm_tcon_manulupdate(s3c);
 	tcon |= pwm_tcon_autoreload(s3c);
-	__raw_writel(tcon, S3C2410_TCON);
+	writel_relaxed(tcon, s3c->base + S3C2410_TCON);
 
 	tcon &= ~pwm_tcon_manulupdate(s3c);
-	__raw_writel(tcon, S3C2410_TCON);
+	writel_relaxed(tcon, s3c->base + S3C2410_TCON);
 
 	local_irq_restore(flags);
 
@@ -207,6 +224,7 @@  static int s3c_pwm_probe(struct platform_device *pdev)
 	unsigned long flags;
 	unsigned long tcon;
 	unsigned int id = pdev->id;
+	struct resource *res;
 	int ret;
 
 	if (id == 4) {
@@ -220,6 +238,12 @@  static int s3c_pwm_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	/* try to get a proper base address, fall back to S3C_VA_TIMER */
+	s3c->base = S3C_VA_TIMER;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res)
+		s3c->base = devm_ioremap(dev, res->start, resource_size(res));
+
 	/* calculate base of control bits in TCON */
 	s3c->tcon_base = id == 0 ? 0 : (id * 4) + 4;
 	s3c->pwm_id = id;
@@ -245,9 +269,9 @@  static int s3c_pwm_probe(struct platform_device *pdev)
 
 	local_irq_save(flags);
 
-	tcon = __raw_readl(S3C2410_TCON);
+	tcon = readl_relaxed(s3c->base + S3C2410_TCON);
 	tcon |= pwm_tcon_invert(s3c);
-	__raw_writel(tcon, S3C2410_TCON);
+	writel_relaxed(tcon, s3c->base + S3C2410_TCON);
 
 	local_irq_restore(flags);
 
@@ -258,7 +282,7 @@  static int s3c_pwm_probe(struct platform_device *pdev)
 	}
 
 	pwm_dbg(s3c, "config bits %02x\n",
-		(__raw_readl(S3C2410_TCON) >> s3c->tcon_base) & 0x0f);
+		(readl_relaxed(s3c->base + S3C2410_TCON) >> s3c->tcon_base) & 0x0f);
 
 	dev_info(dev, "tin at %lu, tdiv at %lu, tin=%sclk, base %d\n",
 		 clk_get_rate(s3c->clk),
@@ -310,9 +334,9 @@  static int s3c_pwm_resume(struct platform_device *pdev)
 	unsigned long tcon;
 
 	/* Restore invertion */
-	tcon = __raw_readl(S3C2410_TCON);
+	tcon = readl_relaxed(s3c->base + S3C2410_TCON);
 	tcon |= pwm_tcon_invert(s3c);
-	__raw_writel(tcon, S3C2410_TCON);
+	writel_relaxed(tcon, s3c->base + S3C2410_TCON);
 
 	return 0;
 }