| Message ID | 1386925027-16288-2-git-send-email-Li.Xiubo@freescale.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote: [...] > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 8b754e4..9029a12 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o > obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o > obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o > obj-$(CONFIG_PWM_IMX) += pwm-imx.o > +obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o This needs to move up one line to preserve the alphabetical ordering. > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c [...] > +struct fsl_pwm_chip { [...] > + int big_endian; This should be of type bool. > +}; > + > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, > + const void __iomem *addr) > +{ > + u32 val; > + > + val = __raw_readl(addr); > + > + if (likely(fpc->big_endian)) The likely() probably isn't very useful in this case. But if you want to keep it, it should at least be reversed, since little-endian is actually the default (you have to specify the big-endian property to activate the big endian mode). > + val = be32_to_cpu(val); > + else > + val = le32_to_cpu(val); > + rmb(); I'd prefer the rmb() to follow the __raw_readl() immediately to make the relationship more explicit. > + > + return val; > +} > + > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, > + u32 val, void __iomem *addr) > +{ > + wmb(); > + if (likely(fpc->big_endian)) > + val = cpu_to_be32(val); > + else > + val = cpu_to_le32(val); > + > + __raw_writel(val, addr); Same here. wmb() should precede __raw_writel() immediately. > +static inline int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc, > + int index) Please align arguments on subsequent lines with the first argument on the first line. There are a few more of those in this file, please check all other functions as well. Also I think it's better to turn the FSL_PWM_CLK_* enum into a named enum and reuse the type here. That way you get proper type-checking. > + switch (index) { [...] > + default: > + return -EINVAL; And with proper type checking you don't need this default case anymore. > +static unsigned long fsl_pwm_calculate_period_cycles(struct fsl_pwm_chip *fpc, > + unsigned long period_ns, int index) > +{ > + int ret; > + unsigned long cycles; > + > + fpc->counter_clk_select = FTM_SC_CLK(index); > + > + ret = fsl_pwm_calculate_default_ps(fpc, index); > + if (ret) { > + dev_err(fpc->chip.dev, "failed to calculate default ps.\n"); No need for the terminating fullstop (.) here, but perhaps you could write out what "ps" actually stands for. Also including the error code in the error message would be good since you're not propagating the error to the caller. > + cycles = fsl_pwm_calculate_cycles(fpc, period_ns); > + > + return cycles; This can simply be "return fsl_pwm_calculate_cycles(fpc, period_ns);", in which case you can get rid of the cycles variable. > +static unsigned long fsl_pwm_calculate_period(struct fsl_pwm_chip *fpc, > + unsigned long period_ns) > +{ [...] > + if (fix_rate > ext_rate) { > + m0 = FSL_PWM_CLK_FIX; > + m1 = FSL_PWM_CLK_EXT; > + } else { > + m0 = FSL_PWM_CLK_EXT; > + m1 = FSL_PWM_CLK_FIX; > + > + } There's a gratuitous blank line above this one. > +static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + u32 val, period, duty; > + struct fsl_pwm_chip *fpc = to_fsl_chip(chip); > + > + mutex_lock(&fpc->lock); > + if (fpc->period_ns && fpc->period_ns != period_ns) { There should be a blank line between the above two. > + dev_err(fpc->chip.dev, "fail to config pwm%d, the period " > + "time should be the same with the current " > + "running pwm(s).\n", pwm->hwpwm); I think I'd make this error message more terse and put more information into a comment. Perhaps something like this: /* * The FSL FTM controller supports only a single period for all PWM * channels, therefore incompatible changes need to be refused. */ if (fpc->period_ns && fpc->period_ns != period_ns) { dev_err(fpc->chip.dev, "conflicting period requested for PWM %u\n", pwm->hwpwm); ... } > + mutex_unlock(&fpc->lock); > + return -EINVAL; I think something like -EBUSY would be more appropriate here. > + if (!fpc->period_ns && duty_ns) { > + period = fsl_pwm_calculate_period(fpc, period_ns); > + if (!period) { > + dev_err(fpc->chip.dev, "fail to calculate the " "failed" > + "period cycles.\n"); And no fullstop either. Also I don't think you need "cycles" here. In fact something equally accurate like this: dev_err(fpc->chip.dev, "failed to calculate period\n"); Still fits within 80 characters. > + fpc->period_ns = period_ns; > + } > + mutex_unlock(&fpc->lock); This could use a blank line as well. > +static int fsl_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, > + enum pwm_polarity polarity) > +{ > + u32 val; > + struct fsl_pwm_chip *fpc = to_fsl_chip(chip); > + > + val = fsl_pwm_readl(fpc, fpc->base + FTM_POL); > + if (polarity == PWM_POLARITY_INVERSED) Blank line between the above. > + val |= BIT(pwm->hwpwm); > + else > + val &= ~BIT(pwm->hwpwm); > + fsl_pwm_writel(fpc, val, fpc->base + FTM_POL); And here. > +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + u32 val; > + struct fsl_pwm_chip *fpc = to_fsl_chip(chip); > + > + val = fsl_pwm_readl(fpc, fpc->base + FTM_OUTMASK); > + val |= BIT(pwm->hwpwm); > + fsl_pwm_writel(fpc, val, fpc->base + FTM_OUTMASK); > + > + mutex_lock(&fpc->lock); > + fsl_counter_clock_disable(fpc); > + > + val = fsl_pwm_readl(fpc, fpc->base + FTM_OUTMASK); > + if ((val & 0xFF) == 0xFF) { > + fpc->period_ns = 0; > + fpc->counter_clk_en = NULL; > + } > + mutex_unlock(&fpc->lock); Same here. > +static int fsl_pwm_probe(struct platform_device *pdev) > +{ > + int ret; > + struct fsl_pwm_chip *fpc; > + struct resource *res; > + struct device_node *np = pdev->dev.of_node; > + > + fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL); > + if (!fpc) > + return -ENOMEM; > + > + mutex_init(&fpc->lock); > + > + fpc->chip.dev = &pdev->dev; > + > + fpc->big_endian = of_property_read_bool(np, "big-endian"); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + fpc->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(fpc->base)) > + return PTR_ERR(fpc->base); > + > + fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm_sys"); For consistency I'd use &pdev->dev here... > + if (IS_ERR(fpc->sys_clk)) { > + dev_err(fpc->chip.dev, ... and here. > + ret = pwmchip_add(&fpc->chip); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret); Perhaps "failed to add PWM chip: %d\n", so that %d isn't confused to mean the PWM chip number? > +static int fsl_pwm_remove(struct platform_device *pdev) > +{ > + struct fsl_pwm_chip *fpc = platform_get_drvdata(pdev); > + > + mutex_destroy(&fpc->lock); I think you can drop this. Once fsl_pwm_remove() exists, the memory pointed to by fpc will be freed, so you won't be able to access the mutex anyway. Thierry
On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote: > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote: > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, > > + const void __iomem *addr) > > +{ > > + u32 val; > > + > > + val = __raw_readl(addr); > > + > > + if (likely(fpc->big_endian)) > > The likely() probably isn't very useful in this case. But if you want to > keep it, it should at least be reversed, since little-endian is actually > the default (you have to specify the big-endian property to activate the > big endian mode). > > > + val = be32_to_cpu(val); > > + else > > + val = le32_to_cpu(val); This will also cause sparse errors, because when sparse is enabled, these expect __le32 or __be32 arguments, not u32. > > + rmb(); > > I'd prefer the rmb() to follow the __raw_readl() immediately to make the > relationship more explicit. A better question to ask is: why is this barrier here? What memory ordering operations is it trying to serialise? > > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, > > + u32 val, void __iomem *addr) > > +{ > > + wmb(); > > + if (likely(fpc->big_endian)) > > + val = cpu_to_be32(val); > > + else > > + val = cpu_to_le32(val); > > + > > + __raw_writel(val, addr); > > Same here. wmb() should precede __raw_writel() immediately. Same comments here - what memory operations is the wmb() trying to serialise? Does this PWM driver somehow end up doing DMA? -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 17 of December 2013 11:51:36 Russell King - ARM Linux wrote: > On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote: > > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote: > > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, > > > + const void __iomem *addr) > > > +{ > > > + u32 val; > > > + > > > + val = __raw_readl(addr); > > > + > > > + if (likely(fpc->big_endian)) > > > > The likely() probably isn't very useful in this case. But if you want to > > keep it, it should at least be reversed, since little-endian is actually > > the default (you have to specify the big-endian property to activate the > > big endian mode). > > > > > + val = be32_to_cpu(val); > > > + else > > > + val = le32_to_cpu(val); > > This will also cause sparse errors, because when sparse is enabled, these > expect __le32 or __be32 arguments, not u32. My question is why can't you just create two sets of accessors, one big endian and one little endian, add two function pointers to your fsl_pwm_chip struct and let the driver set the to correct accessors in probe? This would eliminate the problem with types Russell mentioned and IMHO make the code cleaner. Best regards, Tomasz > > > > + rmb(); > > > > I'd prefer the rmb() to follow the __raw_readl() immediately to make the > > relationship more explicit. > > A better question to ask is: why is this barrier here? What memory > ordering operations is it trying to serialise? I'd also add a question why __raw accessors are used here. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 17, 2013 at 11:51:36AM +0000, Russell King - ARM Linux wrote: > On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote: > > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote: > > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, > > > + const void __iomem *addr) > > > +{ > > > + u32 val; > > > + > > > + val = __raw_readl(addr); > > > + > > > + if (likely(fpc->big_endian)) > > > > The likely() probably isn't very useful in this case. But if you want to > > keep it, it should at least be reversed, since little-endian is actually > > the default (you have to specify the big-endian property to activate the > > big endian mode). > > > > > + val = be32_to_cpu(val); > > > + else > > > + val = le32_to_cpu(val); > > This will also cause sparse errors, because when sparse is enabled, these > expect __le32 or __be32 arguments, not u32. > > > > + rmb(); > > > > I'd prefer the rmb() to follow the __raw_readl() immediately to make the > > relationship more explicit. > > A better question to ask is: why is this barrier here? What memory > ordering operations is it trying to serialise? I suppose that this was done so that the accesses would essentially remain the same as those performed by readl() and writel(). > > > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, > > > + u32 val, void __iomem *addr) > > > +{ > > > + wmb(); > > > + if (likely(fpc->big_endian)) > > > + val = cpu_to_be32(val); > > > + else > > > + val = cpu_to_le32(val); > > > + > > > + __raw_writel(val, addr); > > > > Same here. wmb() should precede __raw_writel() immediately. > > Same comments here - what memory operations is the wmb() trying to > serialise? Does this PWM driver somehow end up doing DMA? Not that I can see. But if my understanding is correct, not using the barriers would allow the compiler and CPU to reorder accesses, and by that cause the register accesses to potentially happen in the wrong order. Thierry
On Tue, Dec 17, 2013 at 01:00:10PM +0100, Tomasz Figa wrote: > On Tuesday 17 of December 2013 11:51:36 Russell King - ARM Linux wrote: > > On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote: > > > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote: > > > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, > > > > + const void __iomem *addr) > > > > +{ > > > > + u32 val; > > > > + > > > > + val = __raw_readl(addr); > > > > + > > > > + if (likely(fpc->big_endian)) > > > > > > The likely() probably isn't very useful in this case. But if you want to > > > keep it, it should at least be reversed, since little-endian is actually > > > the default (you have to specify the big-endian property to activate the > > > big endian mode). > > > > > > > + val = be32_to_cpu(val); > > > > + else > > > > + val = le32_to_cpu(val); > > > > This will also cause sparse errors, because when sparse is enabled, these > > expect __le32 or __be32 arguments, not u32. > > My question is why can't you just create two sets of accessors, one big > endian and one little endian, add two function pointers to your > fsl_pwm_chip struct and let the driver set the to correct accessors in > probe? I guess that would be one possibility. > This would eliminate the problem with types Russell mentioned and IMHO > make the code cleaner. I fail to see how that would eliminate the problem with the types. That said I don't actually see sparse complaining about any type mismatches. That's probably because the various macros implicitly cast to u32. > > > > + rmb(); > > > > > > I'd prefer the rmb() to follow the __raw_readl() immediately to make the > > > relationship more explicit. > > > > A better question to ask is: why is this barrier here? What memory > > ordering operations is it trying to serialise? > > I'd also add a question why __raw accessors are used here. Because both readl() and writel() explicitly perform little endian accesses. Thierry
On Tuesday 17 of December 2013 13:45:06 Thierry Reding wrote: > On Tue, Dec 17, 2013 at 01:00:10PM +0100, Tomasz Figa wrote: > > On Tuesday 17 of December 2013 11:51:36 Russell King - ARM Linux wrote: > > > On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote: > > > > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote: > > > > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, > > > > > + const void __iomem *addr) > > > > > +{ > > > > > + u32 val; > > > > > + > > > > > + val = __raw_readl(addr); > > > > > + > > > > > + if (likely(fpc->big_endian)) > > > > > > > > The likely() probably isn't very useful in this case. But if you want to > > > > keep it, it should at least be reversed, since little-endian is actually > > > > the default (you have to specify the big-endian property to activate the > > > > big endian mode). > > > > > > > > > + val = be32_to_cpu(val); > > > > > + else > > > > > + val = le32_to_cpu(val); > > > > > > This will also cause sparse errors, because when sparse is enabled, these > > > expect __le32 or __be32 arguments, not u32. > > > > My question is why can't you just create two sets of accessors, one big > > endian and one little endian, add two function pointers to your > > fsl_pwm_chip struct and let the driver set the to correct accessors in > > probe? > > I guess that would be one possibility. > > > This would eliminate the problem with types Russell mentioned and IMHO > > make the code cleaner. > > I fail to see how that would eliminate the problem with the types. That > said I don't actually see sparse complaining about any type mismatches. > That's probably because the various macros implicitly cast to u32. Well, in BE variant you would read the register using __raw_readl() into a __be32 and then get an u32 from be32_to_cpu() and return it. Similarly for writes In LE variant, readl()/writel() could be used directly. > > > > > > + rmb(); > > > > > > > > I'd prefer the rmb() to follow the __raw_readl() immediately to make the > > > > relationship more explicit. > > > > > > A better question to ask is: why is this barrier here? What memory > > > ordering operations is it trying to serialise? > > > > I'd also add a question why __raw accessors are used here. > > Because both readl() and writel() explicitly perform little endian > accesses. Right. Thanks for pointing this out. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 17, 2013 at 01:24:33PM +0100, Thierry Reding wrote: > On Tue, Dec 17, 2013 at 11:51:36AM +0000, Russell King - ARM Linux wrote: > > Same comments here - what memory operations is the wmb() trying to > > serialise? Does this PWM driver somehow end up doing DMA? > > Not that I can see. But if my understanding is correct, not using the > barriers would allow the compiler and CPU to reorder accesses, and by > that cause the register accesses to potentially happen in the wrong > order. The compiler won't reorder them, but the CPU may if it meets certain criteria. The architecture guarantees that accesses to device memory within a (minimum of) 1KB block will be ordered. The ARM ARM is slightly ambiguous in how this is applied - in one place it says that "Accesses must arrive at any particular memory-mapped peripheral or block of memory in program order" and another part it says "The size of a memory mapped peripheral, or a block of memory, is IMPLEMENTATION DEFINED, but is not smaller than 1KByte. Note This implies that the maximum memory-mapped peripheral size for which the architecture guarantees order for all implementations is 1KB." See page A3-148. What this means (to me at least) is that on any SoC, the architecture guarantees that accesses _within_ a 1KB device memory block will always be ordered, but two accesses outside of a 1KB block _to the same device_ is implementation defined whether it is ordered or not. The interesting point here though is that the "note" contradicts the first definition if you have (eg) AMBA Primecell peripherals which are generally 4KB in size, since if the architecture only guarantees 1KB, then accesses _may_ _not_ arrive at one primecell in program order. Hence, the note is a direct contradiction of the first definition. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 17, 2013 at 01:54:35PM +0100, Tomasz Figa wrote: > On Tuesday 17 of December 2013 13:45:06 Thierry Reding wrote: > > I fail to see how that would eliminate the problem with the types. That > > said I don't actually see sparse complaining about any type mismatches. > > That's probably because the various macros implicitly cast to u32. > > Well, in BE variant you would read the register using __raw_readl() into > a __be32 and then get an u32 from be32_to_cpu() and return it. Similarly > for writes __raw_readl() returns a u32, so you'll get a warning trying to assign a u32 to a __be32. We do have ioread32() and ioread32be() which do the appropriate conversion, as well as the write versions too. They both include the barrier if you're overly concerned about that. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 17 of December 2013 13:04:35 Russell King - ARM Linux wrote: > On Tue, Dec 17, 2013 at 01:54:35PM +0100, Tomasz Figa wrote: > > On Tuesday 17 of December 2013 13:45:06 Thierry Reding wrote: > > > I fail to see how that would eliminate the problem with the types. That > > > said I don't actually see sparse complaining about any type mismatches. > > > That's probably because the various macros implicitly cast to u32. > > > > Well, in BE variant you would read the register using __raw_readl() into > > a __be32 and then get an u32 from be32_to_cpu() and return it. Similarly > > for writes > > __raw_readl() returns a u32, so you'll get a warning trying to assign a > u32 to a __be32. > > We do have ioread32() and ioread32be() which do the appropriate conversion, > as well as the write versions too. They both include the barrier if you're > overly concerned about that. Nice. I wasn't aware of their existence. Thanks. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 17, 2013 at 12:58:32PM +0000, Russell King - ARM Linux wrote: > On Tue, Dec 17, 2013 at 01:24:33PM +0100, Thierry Reding wrote: > > On Tue, Dec 17, 2013 at 11:51:36AM +0000, Russell King - ARM Linux wrote: > > > Same comments here - what memory operations is the wmb() trying to > > > serialise? Does this PWM driver somehow end up doing DMA? > > > > Not that I can see. But if my understanding is correct, not using the > > barriers would allow the compiler and CPU to reorder accesses, and by > > that cause the register accesses to potentially happen in the wrong > > order. > > The compiler won't reorder them, but the CPU may if it meets certain > criteria. The architecture guarantees that accesses to device memory > within a (minimum of) 1KB block will be ordered. > > The ARM ARM is slightly ambiguous in how this is applied - in one > place it says that "Accesses must arrive at any particular memory-mapped > peripheral or block of memory in program order" and another part it > says "The size of a memory mapped peripheral, or a block of memory, > is IMPLEMENTATION DEFINED, but is not smaller than 1KByte. Note > This implies that the maximum memory-mapped peripheral size for which > the architecture guarantees order for all implementations is 1KB." See > page A3-148. None of the ARM ARM versions that I have seem to have page A3-148. Which version should I be looking at? Not that I'm in any way doubting what you're saying, I'd just like to make sure to have the correct reference to look at in the future. > What this means (to me at least) is that on any SoC, the architecture > guarantees that accesses _within_ a 1KB device memory block will always > be ordered, but two accesses outside of a 1KB block _to the same device_ > is implementation defined whether it is ordered or not. This means at least every ARM SoC would behave that way. Since this driver doesn't have an explicit dependency on ARM I assume it could eventually be used on a different architecture. Even more so since there's Freescale in the name. > The interesting point here though is that the "note" contradicts the > first definition if you have (eg) AMBA Primecell peripherals which are > generally 4KB in size, since if the architecture only guarantees 1KB, > then accesses _may_ _not_ arrive at one primecell in program order. > Hence, the note is a direct contradiction of the first definition. Interesting indeed. Perhaps implementation defined in this case means that an implementation would have to adjust the size of a memory mapped peripheral or block of memory accordingly, depending on the largest block within the SoC. I suppose, though, that if the architecture doesn't give any guarantees about it, we can't safely assume that the implementation will. Thierry
On Tue, Dec 17, 2013 at 01:04:35PM +0000, Russell King - ARM Linux wrote: > On Tue, Dec 17, 2013 at 01:54:35PM +0100, Tomasz Figa wrote: > > On Tuesday 17 of December 2013 13:45:06 Thierry Reding wrote: > > > I fail to see how that would eliminate the problem with the types. That > > > said I don't actually see sparse complaining about any type mismatches. > > > That's probably because the various macros implicitly cast to u32. > > > > Well, in BE variant you would read the register using __raw_readl() into > > a __be32 and then get an u32 from be32_to_cpu() and return it. Similarly > > for writes > > __raw_readl() returns a u32, so you'll get a warning trying to assign a > u32 to a __be32. If sparse doesn't complain about the original code here, does that mean we have a bug that should be fixed? > We do have ioread32() and ioread32be() which do the appropriate conversion, > as well as the write versions too. They both include the barrier if you're > overly concerned about that. A few years ago GregKH commented in response to a patch that ioread*() weren't supposed to be used for memory-mapped only devices. The original purpose apparently was to allow drivers to work with both I/O and memory-mapped devices. Thierry
> On Tue, Dec 17, 2013 at 01:00:10PM +0100, Tomasz Figa wrote: > > On Tuesday 17 of December 2013 11:51:36 Russell King - ARM Linux wrote: > > > On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote: > > > > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote: > > > > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, > > > > > + const void __iomem *addr) > > > > > +{ > > > > > + u32 val; > > > > > + > > > > > + val = __raw_readl(addr); > > > > > + > > > > > + if (likely(fpc->big_endian)) > > > > > > > > The likely() probably isn't very useful in this case. But if you > > > > want to keep it, it should at least be reversed, since > > > > little-endian is actually the default (you have to specify the > > > > big-endian property to activate the big endian mode). > > > > > > > > > + val = be32_to_cpu(val); > > > > > + else > > > > > + val = le32_to_cpu(val); > > > > > > This will also cause sparse errors, because when sparse is enabled, > > > these expect __le32 or __be32 arguments, not u32. > > > > My question is why can't you just create two sets of accessors, one > > big endian and one little endian, add two function pointers to your > > fsl_pwm_chip struct and let the driver set the to correct accessors in > > probe? > > I guess that would be one possibility. > Yes, that's one possibility. If so, it must deference the function pointers and do the C stack push/pop stuff every time when doing the accesses. For instance, but for some devices(USB, NET, DMA..), we need to do many accesses every time in the frequent interrupt handler, and I think the inline type functions will be more efficiency. In LS-1 series platforms, there are many devices that need to do the same work like this, and could these be moved to some global files ? -- Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Tue, Dec 17, 2013 at 01:54:35PM +0100, Tomasz Figa wrote: > > On Tuesday 17 of December 2013 13:45:06 Thierry Reding wrote: > > > I fail to see how that would eliminate the problem with the types. > > > That said I don't actually see sparse complaining about any type > mismatches. > > > That's probably because the various macros implicitly cast to u32. > > > > Well, in BE variant you would read the register using __raw_readl() > > into a __be32 and then get an u32 from be32_to_cpu() and return it. > > Similarly for writes > > __raw_readl() returns a u32, so you'll get a warning trying to assign a > u32 to a __be32. > If all are ok about this method. How about the following: +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, + const void __iomem *addr) +{ + u32 val; + + val = __raw_readl(addr); + + if (likely(fpc->big_endian)) + val = be32_to_cpu((__force __be32)val); + else + val = le32_to_cpu((__force __le32)val); + rmb(); + + return val; +} + +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, + u32 val, void __iomem *addr) +{ + wmb(); + if (likely(fpc->big_endian)) + val = (__force u32)cpu_to_be32(val); + else + val = (__force u32)cpu_to_le32(val); + + __raw_writel(val, addr); +} Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" 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/Kconfig b/drivers/pwm/Kconfig index eece329..c77c571 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -71,6 +71,16 @@ config PWM_EP93XX To compile this driver as a module, choose M here: the module will be called pwm-ep93xx. +config PWM_FSL_FTM + tristate "Freescale FlexTimer Module (FTM) PWM support" + depends on OF + help + Generic FTM PWM framework driver for Freescale VF610 and + Layerscape LS-1 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-fsl-ftm. + config PWM_IMX tristate "i.MX PWM support" depends on ARCH_MXC diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 8b754e4..9029a12 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o obj-$(CONFIG_PWM_IMX) += pwm-imx.o +obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o obj-$(CONFIG_PWM_MXS) += pwm-mxs.o diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c new file mode 100644 index 0000000..22715e7 --- /dev/null +++ b/drivers/pwm/pwm-fsl-ftm.c @@ -0,0 +1,508 @@ +/* + * Freescale FlexTimer Module (FTM) PWM Driver + * + * Copyright 2012-2013 Freescale Semiconductor, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> + +#define FTM_SC 0x00 +#define FTM_SC_CLK_MASK 0x3 +#define FTM_SC_CLK_SHIFT 3 +#define FTM_SC_CLK(c) (((c) + 1) << FTM_SC_CLK_SHIFT) +#define FTM_SC_PS_MASK 0x7 +#define FTM_SC_PS_SHIFT 0 + +#define FTM_CNT 0x04 +#define FTM_MOD 0x08 + +#define FTM_CSC_BASE 0x0C +#define FTM_CSC_MSB BIT(5) +#define FTM_CSC_MSA BIT(4) +#define FTM_CSC_ELSB BIT(3) +#define FTM_CSC_ELSA BIT(2) +#define FTM_CSC(_channel) (FTM_CSC_BASE + ((_channel) * 8)) + +#define FTM_CV_BASE 0x10 +#define FTM_CV(_channel) (FTM_CV_BASE + ((_channel) * 8)) + +#define FTM_CNTIN 0x4C +#define FTM_STATUS 0x50 + +#define FTM_MODE 0x54 +#define FTM_MODE_FTMEN BIT(0) +#define FTM_MODE_INIT BIT(2) +#define FTM_MODE_PWMSYNC BIT(3) + +#define FTM_SYNC 0x58 +#define FTM_OUTINIT 0x5C +#define FTM_OUTMASK 0x60 +#define FTM_COMBINE 0x64 +#define FTM_DEADTIME 0x68 +#define FTM_EXTTRIG 0x6C +#define FTM_POL 0x70 +#define FTM_FMS 0x74 +#define FTM_FILTER 0x78 +#define FTM_FLTCTRL 0x7C +#define FTM_QDCTRL 0x80 +#define FTM_CONF 0x84 +#define FTM_FLTPOL 0x88 +#define FTM_SYNCONF 0x8C +#define FTM_INVCTRL 0x90 +#define FTM_SWOCTRL 0x94 +#define FTM_PWMLOAD 0x98 + +enum { + FSL_PWM_CLK_SYS, + FSL_PWM_CLK_FIX, + FSL_PWM_CLK_EXT, + FSL_PWM_CLK_MAX, +}; + +struct fsl_pwm_chip { + struct pwm_chip chip; + + struct mutex lock; + + struct clk *sys_clk; + struct clk *counter_clk; + struct clk *counter_clk_en; + unsigned int counter_clk_select; + unsigned int counter_clk_enable; + unsigned int clk_ps; + + void __iomem *base; + + int period_ns; + int big_endian; +}; + +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, + const void __iomem *addr) +{ + u32 val; + + val = __raw_readl(addr); + + if (likely(fpc->big_endian)) + val = be32_to_cpu(val); + else + val = le32_to_cpu(val); + rmb(); + + return val; +} + +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, + u32 val, void __iomem *addr) +{ + wmb(); + if (likely(fpc->big_endian)) + val = cpu_to_be32(val); + else + val = cpu_to_le32(val); + + __raw_writel(val, addr); +} + +static inline struct fsl_pwm_chip *to_fsl_chip(struct pwm_chip *chip) +{ + return container_of(chip, struct fsl_pwm_chip, chip); +} + +static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct fsl_pwm_chip *fpc = to_fsl_chip(chip); + + return clk_prepare_enable(fpc->sys_clk); +} + +static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct fsl_pwm_chip *fpc = to_fsl_chip(chip); + + clk_disable_unprepare(fpc->sys_clk); +} + +static inline int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc, + int index) +{ + unsigned long sys_rate, cnt_rate; + unsigned long long ratio; + + sys_rate = clk_get_rate(fpc->sys_clk); + if (!sys_rate) + return -EINVAL; + + cnt_rate = clk_get_rate(fpc->counter_clk); + if (!cnt_rate) + return -EINVAL; + + switch (index) { + case FSL_PWM_CLK_SYS: + fpc->clk_ps = 1; + break; + case FSL_PWM_CLK_FIX: + ratio = 2 * cnt_rate - 1; + do_div(ratio, sys_rate); + fpc->clk_ps = ratio; + break; + case FSL_PWM_CLK_EXT: + ratio = 4 * cnt_rate - 1; + do_div(ratio, sys_rate); + fpc->clk_ps = ratio; + break; + default: + return -EINVAL; + } + + return 0; +} + +static inline unsigned long fsl_pwm_calculate_cycles(struct fsl_pwm_chip *fpc, + unsigned long period_ns) +{ + unsigned long long c, c0; + + c = clk_get_rate(fpc->counter_clk); + c = c * period_ns; + do_div(c, 1000000000UL); + + do { + c0 = c; + do_div(c0, (1 << fpc->clk_ps)); + if (c0 <= 0xFFFF) + return (unsigned long)c0; + } while (++fpc->clk_ps < 8); + + return 0; +} + +static unsigned long fsl_pwm_calculate_period_cycles(struct fsl_pwm_chip *fpc, + unsigned long period_ns, int index) +{ + int ret; + unsigned long cycles; + + fpc->counter_clk_select = FTM_SC_CLK(index); + + ret = fsl_pwm_calculate_default_ps(fpc, index); + if (ret) { + dev_err(fpc->chip.dev, "failed to calculate default ps.\n"); + return 0; + } + + cycles = fsl_pwm_calculate_cycles(fpc, period_ns); + + return cycles; +} + +static unsigned long fsl_pwm_calculate_period(struct fsl_pwm_chip *fpc, + unsigned long period_ns) +{ + unsigned long cycles; + int m0, m1; + unsigned long fix_rate, ext_rate; + struct clk *cnt_clk[FSL_PWM_CLK_MAX]; + + fpc->counter_clk = fpc->sys_clk; + cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns, + FSL_PWM_CLK_SYS); + if (cycles) + return cycles; + + cnt_clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix"); + if (IS_ERR(cnt_clk[FSL_PWM_CLK_FIX])) + return PTR_ERR(cnt_clk[FSL_PWM_CLK_FIX]); + + cnt_clk[FSL_PWM_CLK_EXT] = devm_clk_get(fpc->chip.dev, "ftm_ext"); + if (IS_ERR(cnt_clk[FSL_PWM_CLK_EXT])) + return PTR_ERR(cnt_clk[FSL_PWM_CLK_EXT]); + + fpc->counter_clk_en = devm_clk_get(fpc->chip.dev, "ftm_cnt_clk_en"); + if (IS_ERR(fpc->counter_clk_en)) + return PTR_ERR(fpc->counter_clk_en); + + fix_rate = clk_get_rate(cnt_clk[FSL_PWM_CLK_FIX]); + ext_rate = clk_get_rate(cnt_clk[FSL_PWM_CLK_EXT]); + + if (fix_rate > ext_rate) { + m0 = FSL_PWM_CLK_FIX; + m1 = FSL_PWM_CLK_EXT; + } else { + m0 = FSL_PWM_CLK_EXT; + m1 = FSL_PWM_CLK_FIX; + + } + + fpc->counter_clk = cnt_clk[m0]; + cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns, m0); + if (cycles) + return cycles; + + fpc->counter_clk = cnt_clk[m1]; + cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns, m0); + + return cycles; +} + +static unsigned long fsl_pwm_calculate_duty(struct fsl_pwm_chip *fpc, + unsigned long period_ns, unsigned long duty_ns) +{ + unsigned long long val, duty; + + val = fsl_pwm_readl(fpc, fpc->base + FTM_MOD); + duty = duty_ns * (val + 1); + do_div(duty, period_ns); + + return (unsigned long)duty; +} + +static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + u32 val, period, duty; + struct fsl_pwm_chip *fpc = to_fsl_chip(chip); + + mutex_lock(&fpc->lock); + if (fpc->period_ns && fpc->period_ns != period_ns) { + dev_err(fpc->chip.dev, "fail to config pwm%d, the period " + "time should be the same with the current " + "running pwm(s).\n", pwm->hwpwm); + mutex_unlock(&fpc->lock); + return -EINVAL; + } + + if (!fpc->period_ns && duty_ns) { + period = fsl_pwm_calculate_period(fpc, period_ns); + if (!period) { + dev_err(fpc->chip.dev, "fail to calculate the " + "period cycles.\n"); + mutex_unlock(&fpc->lock); + return -EINVAL; + } + + val = fsl_pwm_readl(fpc, fpc->base + FTM_SC); + val &= ~(FTM_SC_PS_MASK << FTM_SC_PS_SHIFT); + val |= fpc->clk_ps; + fsl_pwm_writel(fpc, val, fpc->base + FTM_SC); + fsl_pwm_writel(fpc, period - 1, fpc->base + FTM_MOD); + + fpc->period_ns = period_ns; + } + mutex_unlock(&fpc->lock); + + duty = fsl_pwm_calculate_duty(fpc, period_ns, duty_ns); + + fsl_pwm_writel(fpc, FTM_CSC_MSB | FTM_CSC_ELSB, + fpc->base + FTM_CSC(pwm->hwpwm)); + fsl_pwm_writel(fpc, duty, fpc->base + FTM_CV(pwm->hwpwm)); + + return 0; +} + +static int fsl_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, + enum pwm_polarity polarity) +{ + u32 val; + struct fsl_pwm_chip *fpc = to_fsl_chip(chip); + + val = fsl_pwm_readl(fpc, fpc->base + FTM_POL); + if (polarity == PWM_POLARITY_INVERSED) + val |= BIT(pwm->hwpwm); + else + val &= ~BIT(pwm->hwpwm); + fsl_pwm_writel(fpc, val, fpc->base + FTM_POL); + + return 0; +} + +static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc) +{ + int ret; + u32 val; + + if (fpc->counter_clk_enable++) + return 0; + + ret = clk_prepare_enable(fpc->counter_clk); + if (ret) { + fpc->counter_clk_enable--; + return ret; + } + + ret = clk_prepare_enable(fpc->counter_clk_en); + if (ret) { + fpc->counter_clk_enable--; + return ret; + } + + /* select counter clock source */ + val = fsl_pwm_readl(fpc, fpc->base + FTM_SC); + val &= ~(FTM_SC_CLK_MASK << FTM_SC_CLK_SHIFT); + val |= fpc->counter_clk_select; + fsl_pwm_writel(fpc, val, fpc->base + FTM_SC); + + return 0; +} + +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + int ret; + u32 val; + struct fsl_pwm_chip *fpc = to_fsl_chip(chip); + + val = fsl_pwm_readl(fpc, fpc->base + FTM_OUTMASK); + val &= ~BIT(pwm->hwpwm); + fsl_pwm_writel(fpc, val, fpc->base + FTM_OUTMASK); + + mutex_lock(&fpc->lock); + ret = fsl_counter_clock_enable(fpc); + mutex_unlock(&fpc->lock); + + return ret; +} + +static inline void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) +{ + u32 val; + + if (--fpc->counter_clk_enable) + return; + + val = fsl_pwm_readl(fpc, fpc->base + FTM_SC); + val &= ~(FTM_SC_CLK_MASK << FTM_SC_CLK_SHIFT); + fsl_pwm_writel(fpc, val, fpc->base + FTM_SC); + + clk_disable_unprepare(fpc->counter_clk_en); + clk_disable_unprepare(fpc->counter_clk); +} + +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + u32 val; + struct fsl_pwm_chip *fpc = to_fsl_chip(chip); + + val = fsl_pwm_readl(fpc, fpc->base + FTM_OUTMASK); + val |= BIT(pwm->hwpwm); + fsl_pwm_writel(fpc, val, fpc->base + FTM_OUTMASK); + + mutex_lock(&fpc->lock); + fsl_counter_clock_disable(fpc); + + val = fsl_pwm_readl(fpc, fpc->base + FTM_OUTMASK); + if ((val & 0xFF) == 0xFF) { + fpc->period_ns = 0; + fpc->counter_clk_en = NULL; + } + mutex_unlock(&fpc->lock); +} + +static const struct pwm_ops fsl_pwm_ops = { + .request = fsl_pwm_request, + .free = fsl_pwm_free, + .config = fsl_pwm_config, + .set_polarity = fsl_pwm_set_polarity, + .enable = fsl_pwm_enable, + .disable = fsl_pwm_disable, + .owner = THIS_MODULE, +}; + +static int fsl_pwm_probe(struct platform_device *pdev) +{ + int ret; + struct fsl_pwm_chip *fpc; + struct resource *res; + struct device_node *np = pdev->dev.of_node; + + fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL); + if (!fpc) + return -ENOMEM; + + mutex_init(&fpc->lock); + + fpc->chip.dev = &pdev->dev; + + fpc->big_endian = of_property_read_bool(np, "big-endian"); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + fpc->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(fpc->base)) + return PTR_ERR(fpc->base); + + fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm_sys"); + if (IS_ERR(fpc->sys_clk)) { + dev_err(fpc->chip.dev, + "failed to get \"ftm_sys\" clock\n"); + return PTR_ERR(fpc->sys_clk); + } + + ret = clk_prepare_enable(fpc->sys_clk); + if (ret) + return ret; + + fsl_pwm_writel(fpc, 0x00, fpc->base + FTM_CNTIN); + fsl_pwm_writel(fpc, 0x00, fpc->base + FTM_OUTINIT); + fsl_pwm_writel(fpc, 0xFF, fpc->base + FTM_OUTMASK); + clk_disable_unprepare(fpc->sys_clk); + + fpc->chip.ops = &fsl_pwm_ops; + fpc->chip.of_xlate = of_pwm_xlate_with_flags; + fpc->chip.of_pwm_n_cells = 3; + fpc->chip.base = -1; + fpc->chip.npwm = 8; + + ret = pwmchip_add(&fpc->chip); + if (ret < 0) { + dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret); + return ret; + } + + platform_set_drvdata(pdev, fpc); + + return 0; +} + +static int fsl_pwm_remove(struct platform_device *pdev) +{ + struct fsl_pwm_chip *fpc = platform_get_drvdata(pdev); + + mutex_destroy(&fpc->lock); + + return pwmchip_remove(&fpc->chip); +} + +static const struct of_device_id fsl_pwm_dt_ids[] = { + { .compatible = "fsl,vf610-ftm-pwm", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, fsl_pwm_dt_ids); + +static struct platform_driver fsl_pwm_driver = { + .driver = { + .name = "fsl-ftm-pwm", + .of_match_table = fsl_pwm_dt_ids, + }, + .probe = fsl_pwm_probe, + .remove = fsl_pwm_remove, +}; +module_platform_driver(fsl_pwm_driver); + +MODULE_DESCRIPTION("Freescale FlexTimer Module PWM Driver"); +MODULE_AUTHOR("Xiubo Li <Li.Xiubo@freescale.com>"); +MODULE_ALIAS("platform:fsl-ftm-pwm"); +MODULE_LICENSE("GPL");