Re: [PATCH 17/30] pwm: samsung: repair the worst MMIO abuses
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.red...@avionic-design.de pgpedplha6TqS.pgp Description: PGP signature
Re: [PATCH 17/30] pwm: samsung: repair the worst MMIO abuses
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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 17/30] pwm: samsung: repair the worst MMIO abuses
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 a...@arndb.de
Cc: Tomasz Figa t.f...@samsung.com
Cc: Thierry Reding thierry.red...@avionic-design.de
---
drivers/pwm/pwm-samsung.c | 60 +--
1 file changed, 42 insertions(+), 18 deletions(-)
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(0xf630));
+}
+#else
+#define S3C_VA_TIMER IOMEM(0xf630);
+#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 chartcon_base;
unsigned charpwm_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
