| Message ID | 1365638712-1028578-18-git-send-email-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)