Message ID | 1385979309-10505-1-git-send-email-Li.Xiubo@freescale.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Mon, Dec 02, 2013 at 10:15:09AM +0000, Xiubo Li wrote: > The FTM PWM device can be found on Vybrid VF610 Tower and > Layerscape LS-1 SoCs. > > Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com> > Signed-off-by: Alison Wang <b18965@freescale.com> > Signed-off-by: Jingchang Lu <b35083@freescale.com> > --- > > I'm sending the RFC patch about the FTM IP block registers read and write > endian fix for comments and more parcticed ideas. > > In Vybird VF610 Tower, all the IP blocks expect LE data. In the LS-1, some of > the IP blocks expect LE data, while others expect BE data. And the CPU always > operates in LE mode in these two platforms. Could you elaborate on "expect BE data" please? Is this all registers within a given device, or a subset thereof? Are there data structrues involved (i.e. buffers)? > > So now I must take care of all these two cases. I'm not very sure if there is > other better ways to resolve this problem. In this patch I have implemented > two functions fsl_pwm_readl() and fsl_pwm_writel() to replace readl() and > writel(). At the same time there should add one "endianess" property in the DT > file. If you're adding DT properties, binding updates are helpful to describe their intended use. [...] > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, void __iomem *reg) > +{ > + u32 val; > + > + val = __raw_readl(reg); > + > + if (fpc->endianess == FTM_BIG) > + return be32_to_cpu(val); > + else > + return le32_to_cpu(val); > +} > + > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, u32 val, > + void __iomem *reg) > +{ > + if (fpc->endianess == FTM_BIG) > + val = cpu_to_be32(val); > + else > + val = cpu_to_le32(val); > + > + __raw_writel(val, reg); > +} Using the __raw variants also loses you the memory barriers. Does this create any ordering issues in the rest of the driver? [...] > +static int fsl_pwm_probe(struct platform_device *pdev) > +{ > + int ret; > + const char *endianess; > + 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; > + > + ret = fsl_pwm_parse_clk_ps(fpc); > + if (ret < 0) > + return ret; > + > + if (of_property_read_string(np, "endianess", &endianess)) > + pr_warning("missing \"endianess\" property, " > + "the FTM IP block is little " > + "endian as default\n"); > + else if (!strcmp("big", endianess)) > + fpc->endianess = FTM_BIG; Please don't do this. One option is to have a new compatible string -- a big-endian device can't be poked in the same way as its little-endian variants, so they're not compatible. You can then associate some data with the compatible strings to figure out which accessors to use. If you really want this to be a separate property, make it a boolean property (e.g. "fsl,big-endian", which you can read with of_property_read_bool. There's no need for this to be a string. I'd prefer the compatible string option. Thanks, Mark.
> > I'm sending the RFC patch about the FTM IP block registers read and > > write endian fix for comments and more parcticed ideas. > > > > In Vybird VF610 Tower, all the IP blocks expect LE data. In the LS-1, > > some of the IP blocks expect LE data, while others expect BE data. And > > the CPU always operates in LE mode in these two platforms. > > Could you elaborate on "expect BE data" please? Is this all registers > within a given device, or a subset thereof? > Are there data structures involved (i.e. buffers)? > It actually means big-endian mode, and it's all the registers and hasn't any buffers or descriptors involved within FTM IP block. But maybe in other IP blocks, such as eDMA and USB, should consider about the buffers/descriptors issue. > > > > So now I must take care of all these two cases. I'm not very sure if > > there is other better ways to resolve this problem. In this patch I > > have implemented two functions fsl_pwm_readl() and fsl_pwm_writel() to > > replace readl() and writel(). At the same time there should add one > > "endianess" property in the DT file. > > If you're adding DT properties, binding updates are helpful to describe > their intended use. > Yes, that surely will be added later. > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, void > > +__iomem *reg) { > > + u32 val; > > + > > + val = __raw_readl(reg); > > + > > + if (fpc->endianess == FTM_BIG) > > + return be32_to_cpu(val); > > + else > > + return le32_to_cpu(val); } > > + > > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, u32 val, > > + void __iomem *reg) > > +{ > > + if (fpc->endianess == FTM_BIG) > > + val = cpu_to_be32(val); > > + else > > + val = cpu_to_le32(val); > > + > > + __raw_writel(val, reg); > > +} > > Using the __raw variants also loses you the memory barriers. Does this > create any ordering issues in the rest of the driver? > No, I had checked about this, so I just droped them. Maybe it will be much better and safer if there are some memory barriers. > > +static int fsl_pwm_probe(struct platform_device *pdev) { > > + int ret; > > + const char *endianess; > > + 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; > > + > > + ret = fsl_pwm_parse_clk_ps(fpc); > > + if (ret < 0) > > + return ret; > > + > > + if (of_property_read_string(np, "endianess", &endianess)) > > + pr_warning("missing \"endianess\" property, " > > + "the FTM IP block is little " > > + "endian as default\n"); > > + else if (!strcmp("big", endianess)) > > + fpc->endianess = FTM_BIG; > > Please don't do this. > > One option is to have a new compatible string -- a big-endian device can't > be poked in the same way as its little-endian variants, so they're not > compatible. You can then associate some data with the compatible strings to > figure out which accessors to use. > Do you mean: + static const struct of_device_id fsl_pwm_dt_ids[] = { + { .compatible = "fsl,vf610-ftm-pwm,big-endian", }, + { .compatible = "fsl,vf610-ftm-pwm,little-endian", }, + { /* sentinel */ } + }; ? Or just adding one property in DT file likes: pwm0: pwm@40038000 { compatible = "fsl,vf610-ftm-pwm"; #pwm-cells = <3>; reg = <0x40038000 0x1000>; clock-names = "ftm0", "ftm0_fix_sel", "ftm0_ext_sel"; clocks = <&clks VF610_CLK_FTM0>, <&clks VF610_CLK_FTM0_FIX_SEL>, <&clks VF610_CLK_FTM0_EXT_SEL>; + big-endian; ---> both registers and buffers/descriptors Or + big-endian-regs; ---> registers + big-endian-desc; ---> buffers or descriptors status = "disabled"; }; And then in FTM driver probe function adds: + if (of_get_property(np, "big-endian", NULL)) + fpc->be_reg = fpc->be_desc = 1; + else if (of_get_property(np, "big-endian-regs", NULL)) + fpc->be_reg = 1; + else if (of_get_property(np, "big-endian-desc", NULL)) + fpc->be_desc = 1; In this case, if all of the following properties is absent, that means they all are operated in little-endian mode. "big-endian" "big-endian-regs" "big-endian-desc" -- Best Regards,
On Tue, Dec 03, 2013 at 04:02:48AM +0000, Li Xiubo wrote: > > > I'm sending the RFC patch about the FTM IP block registers read and > > > write endian fix for comments and more parcticed ideas. > > > > > > In Vybird VF610 Tower, all the IP blocks expect LE data. In the LS-1, > > > some of the IP blocks expect LE data, while others expect BE data. And > > > the CPU always operates in LE mode in these two platforms. > > > > Could you elaborate on "expect BE data" please? Is this all registers > > within a given device, or a subset thereof? > > Are there data structures involved (i.e. buffers)? > > > > It actually means big-endian mode, and it's all the registers and hasn't any > buffers or descriptors involved within FTM IP block. > But maybe in other IP blocks, such as eDMA and USB, should consider about the > buffers/descriptors issue. Ok. So all the registers are big-endian, and there are no memory accesses to care about. So for the binding for this device, we only have one property to worry about -- the endianness of all registers. > > > > > > > > So now I must take care of all these two cases. I'm not very sure if > > > there is other better ways to resolve this problem. In this patch I > > > have implemented two functions fsl_pwm_readl() and fsl_pwm_writel() to > > > replace readl() and writel(). At the same time there should add one > > > "endianess" property in the DT file. > > > > If you're adding DT properties, binding updates are helpful to describe > > their intended use. > > > > Yes, that surely will be added later. Ok. I'd like to see that at the same time as the patch modifying the driver. > > > > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, void > > > +__iomem *reg) { > > > + u32 val; > > > + > > > + val = __raw_readl(reg); > > > + > > > + if (fpc->endianess == FTM_BIG) > > > + return be32_to_cpu(val); > > > + else > > > + return le32_to_cpu(val); } > > > + > > > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, u32 val, > > > + void __iomem *reg) > > > +{ > > > + if (fpc->endianess == FTM_BIG) > > > + val = cpu_to_be32(val); > > > + else > > > + val = cpu_to_le32(val); > > > + > > > + __raw_writel(val, reg); > > > +} > > > > Using the __raw variants also loses you the memory barriers. Does this > > create any ordering issues in the rest of the driver? > > > > No, I had checked about this, so I just droped them. > Maybe it will be much better and safer if there are some memory barriers. If you've checked, is it safe or is it not? > > > > > > +static int fsl_pwm_probe(struct platform_device *pdev) { > > > + int ret; > > > + const char *endianess; > > > + 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; > > > + > > > + ret = fsl_pwm_parse_clk_ps(fpc); > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (of_property_read_string(np, "endianess", &endianess)) > > > + pr_warning("missing \"endianess\" property, " > > > + "the FTM IP block is little " > > > + "endian as default\n"); > > > + else if (!strcmp("big", endianess)) > > > + fpc->endianess = FTM_BIG; > > > > Please don't do this. > > > > One option is to have a new compatible string -- a big-endian device can't > > be poked in the same way as its little-endian variants, so they're not > > compatible. You can then associate some data with the compatible strings to > > figure out which accessors to use. > > > > Do you mean: > + static const struct of_device_id fsl_pwm_dt_ids[] = { > + { .compatible = "fsl,vf610-ftm-pwm,big-endian", }, > + { .compatible = "fsl,vf610-ftm-pwm,little-endian", }, > + { /* sentinel */ } > + }; ? That's one option. Something like that would be fine. Then you could store flags in of_device_id::data to tell you which accessors to use. > > Or just adding one property in DT file likes: > pwm0: pwm@40038000 { > compatible = "fsl,vf610-ftm-pwm"; > #pwm-cells = <3>; > reg = <0x40038000 0x1000>; > clock-names = "ftm0", "ftm0_fix_sel", "ftm0_ext_sel"; > clocks = <&clks VF610_CLK_FTM0>, > <&clks VF610_CLK_FTM0_FIX_SEL>, > <&clks VF610_CLK_FTM0_EXT_SEL>; > + big-endian; ---> both registers and buffers/descriptors > Or > + big-endian-regs; ---> registers > + big-endian-desc; ---> buffers or descriptors As you've described above, in this case we don't need to make the distinction. Other bindings seem to have a "big-endian" boolean property for this case. It's probably best to align with the existing bindings and use a "big-endian" boolean property. Thanks, Mark.
> > > > I'm sending the RFC patch about the FTM IP block registers read > > > > and write endian fix for comments and more parcticed ideas. > > > > > > > > In Vybird VF610 Tower, all the IP blocks expect LE data. In the > > > > LS-1, some of the IP blocks expect LE data, while others expect BE > > > > data. And the CPU always operates in LE mode in these two platforms. > > > > > > Could you elaborate on "expect BE data" please? Is this all > > > registers within a given device, or a subset thereof? > > > Are there data structures involved (i.e. buffers)? > > > > > > > It actually means big-endian mode, and it's all the registers and > > hasn't any buffers or descriptors involved within FTM IP block. > > But maybe in other IP blocks, such as eDMA and USB, should consider > > about the buffers/descriptors issue. > > Ok. So all the registers are big-endian, and there are no memory accesses > to care about. > > So for the binding for this device, we only have one property to worry > about -- the endianness of all registers. > Yes, it is. > > > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, void > > > > +__iomem *reg) { > > > > + u32 val; > > > > + > > > > + val = __raw_readl(reg); > > > > + > > > > + if (fpc->endianess == FTM_BIG) > > > > + return be32_to_cpu(val); > > > > + else > > > > + return le32_to_cpu(val); } > > > > + > > > > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, u32 val, > > > > + void __iomem *reg) { > > > > + if (fpc->endianess == FTM_BIG) > > > > + val = cpu_to_be32(val); > > > > + else > > > > + val = cpu_to_le32(val); > > > > + > > > > + __raw_writel(val, reg); > > > > +} > > > > > > Using the __raw variants also loses you the memory barriers. Does > > > this create any ordering issues in the rest of the driver? > > > > > > > No, I had checked about this, so I just droped them. > > Maybe it will be much better and safer if there are some memory barriers. > > If you've checked, is it safe or is it not? > Safe, but now I think I should add the memory barriers here, and that will be better. > > > > > > > > > > +static int fsl_pwm_probe(struct platform_device *pdev) { > > > > + int ret; > > > > + const char *endianess; > > > > + 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; > > > > + > > > > + ret = fsl_pwm_parse_clk_ps(fpc); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + if (of_property_read_string(np, "endianess", &endianess)) > > > > + pr_warning("missing \"endianess\" property, " > > > > + "the FTM IP block is little " > > > > + "endian as default\n"); > > > > + else if (!strcmp("big", endianess)) > > > > + fpc->endianess = FTM_BIG; > > > > > > Please don't do this. > > > > > > One option is to have a new compatible string -- a big-endian device > > > can't be poked in the same way as its little-endian variants, so > > > they're not compatible. You can then associate some data with the > > > compatible strings to figure out which accessors to use. > > > > > > > Do you mean: > > + static const struct of_device_id fsl_pwm_dt_ids[] = { > > + { .compatible = "fsl,vf610-ftm-pwm,big-endian", }, > > + { .compatible = "fsl,vf610-ftm-pwm,little-endian", }, > > + { /* sentinel */ } > > + }; ? > > That's one option. Something like that would be fine. Then you could store > flags in of_device_id::data to tell you which accessors to use. > I will have a try. > > > > Or just adding one property in DT file likes: > > pwm0: pwm@40038000 { > > compatible = "fsl,vf610-ftm-pwm"; > > #pwm-cells = <3>; > > reg = <0x40038000 0x1000>; > > clock-names = "ftm0", "ftm0_fix_sel", > "ftm0_ext_sel"; > > clocks = <&clks VF610_CLK_FTM0>, > > <&clks VF610_CLK_FTM0_FIX_SEL>, > > <&clks VF610_CLK_FTM0_EXT_SEL>; > > + big-endian; ---> both registers and > buffers/descriptors > > Or > > + big-endian-regs; ---> registers > > + big-endian-desc; ---> buffers or descriptors > > As you've described above, in this case we don't need to make the > distinction. Other bindings seem to have a "big-endian" boolean property > for this case. > > It's probably best to align with the existing bindings and use a "big- > endian" boolean property. > Yes, I agree. Well as we can see that, PowerPC has already implemented about the big-endian and little-endian register reading and writing interfaces for IP block devices, which are just I needed here in this case. And also the same with other IP blocks in our Vybird , LS-1 and maybe other boards. So, I think there should be the similar implementations for ARM too. Or for every IP block, we must do the same work then. What do you think about this? Thanks, -- Xiubo
On 2 Dec 2013, Li.Xiubo at freescale.com wrote: > The FTM PWM device can be found on Vybrid VF610 Tower and > Layerscape LS-1 SoCs. [snip] > In Vybird VF610 Tower, all the IP blocks expect LE data. In the LS-1, some of > the IP blocks expect LE data, while others expect BE data. And the CPU always > operates in LE mode in these two platforms. [snip] > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, void __iomem *reg) > +{ > + u32 val; > + > + val = __raw_readl(reg); > + > + if (fpc->endianess == FTM_BIG) > + return be32_to_cpu(val); > + else > + return le32_to_cpu(val); > +} > + > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, u32 val, > + void __iomem *reg) > +{ > + if (fpc->endianess == FTM_BIG) > + val = cpu_to_be32(val); > + else > + val = cpu_to_le32(val); > + > + __raw_writel(val, reg); > +} Remove above and alter as below, 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 = readl(fpc, fpc->base + FTM_POL); if (polarity == PWM_POLARITY_INVERSED) - val |= BIT(pwm->hwpwm); + val |= BIT(fpc->chn_bit); else - val &= ~BIT(pwm->hwpwm); + val &= ~BIT(fpc->chn_bit); 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; } val = readl(fpc, fpc->base + FTM_SC); + val |= val >> 24; /* get value on big-endian. */ val &= ~((FTM_SC_CLK_MASK << FTM_SC_CLK_SHIFT) | (FTM_SC_PS_MASK << FTM_SC_PS_SHIFT)); /* select counter clock source */ switch (fpc->counter_clk_select) { case VF610_CLK_FTM0: val |= FTM_SC_CLK_SYS; break; case VF610_CLK_FTM0_FIX_SEL: val |= FTM_SC_CLK_FIX; break; case VF610_CLK_FTM0_EXT_SEL: val |= FTM_SC_CLK_EXT; break; default: fpc->counter_clk_enable--; return -EINVAL; } val |= fpc->clk_ps; + val |= val << 24; /* modify to high byte for big-endian. */ writel(fpc, val, fpc->base + FTM_SC); return 0; } That is instead of modifying the low level accessor, you can alter the driver masks based on the OF selected endian. Many of the registers are already accessed as run-time fields because each channel is in a different bit position. There are only two registers with fields that cross a byte boundary; COUNT and MOD. These two must be swapped. The others are either a byte or are accessed base on a channel number. For example in fsl_pwm_set_polarity(), we would read the memory, swap the value, calculate a bit to set, clear or set the bit and then write back the swapped value all on the 'big_endian' condition. This way, you just read a bit shift which is conditional on the endian at probe time and don't double swap. In the fsl_counter_clock_enable(), the example is using 'write ignored bits' and duplicating the value in both big/little bytes. An alternative is to parameterize all the mask/values in 'const struct' and have a different pointer for big/little and store this in-place (or via a pointer) in fsl_pwm_chip. Fwiw, Bill Pringlemeir.
Hi Bill, > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, void __iomem *reg) > > +{ > > + u32 val; > > + > > + val = __raw_readl(reg); > > + > > + if (fpc->endianess == FTM_BIG) > > + return be32_to_cpu(val); > > + else > > + return le32_to_cpu(val); > > +} > > + > > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, u32 val, > > + void __iomem *reg) > > +{ > > + if (fpc->endianess == FTM_BIG) > > + val = cpu_to_be32(val); > > + else > > + val = cpu_to_le32(val); > > + > > + __raw_writel(val, reg); > > +} > > Remove above and alter as below, > > 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 = readl(fpc, fpc->base + FTM_POL); > if (polarity == PWM_POLARITY_INVERSED) > - val |= BIT(pwm->hwpwm); > + val |= BIT(fpc->chn_bit); > else > - val &= ~BIT(pwm->hwpwm); > + val &= ~BIT(fpc->chn_bit); > 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; > } > > val = readl(fpc, fpc->base + FTM_SC); > + val |= val >> 24; /* get value on big-endian. */ > val &= ~((FTM_SC_CLK_MASK << FTM_SC_CLK_SHIFT) | > (FTM_SC_PS_MASK << FTM_SC_PS_SHIFT)); > > /* select counter clock source */ > switch (fpc->counter_clk_select) { > case VF610_CLK_FTM0: > val |= FTM_SC_CLK_SYS; > break; > case VF610_CLK_FTM0_FIX_SEL: > val |= FTM_SC_CLK_FIX; > break; > case VF610_CLK_FTM0_EXT_SEL: > val |= FTM_SC_CLK_EXT; > break; > default: > fpc->counter_clk_enable--; > return -EINVAL; > } > > val |= fpc->clk_ps; > + val |= val << 24; /* modify to high byte for big-endian. */ > writel(fpc, val, fpc->base + FTM_SC); > > return 0; > } > > That is instead of modifying the low level accessor, you can alter the > driver masks based on the OF selected endian. Many of the registers are > already accessed as run-time fields because each channel is in a > different bit position. There are only two registers with fields that > cross a byte boundary; COUNT and MOD. These two must be swapped. The > others are either a byte or are accessed base on a channel number. > > For example in fsl_pwm_set_polarity(), we would read the memory, swap > the value, calculate a bit to set, clear or set the bit and then write > back the swapped value all on the 'big_endian' condition. This way, you > just read a bit shift which is conditional on the endian at probe time > and don't double swap. > > In the fsl_counter_clock_enable(), the example is using 'write ignored > bits' and duplicating the value in both big/little bytes. An > alternative is to parameterize all the mask/values in 'const struct' and > have a different pointer for big/little and store this in-place (or via > a pointer) in fsl_pwm_chip. > Yes, this is one optional method. The PWM is only one feature of the FlexTimer Module, and in the future there will implements all of them, and then there will be many other bits that cross a byte position. And now the fsl_pwm_readl()/fsl_pwm_writel() are just copied from readl()/writel(), and then added the endianess arbitration. And there are many other IP blocks like FTM need to do same work for LS-1 series and Vybird machines. IMO, this is more readable and easier to extern to full functions for these device driver code, and also easier to debug. Thanks, Best Regards, Xiubo
> That is instead of modifying the low level accessor, you can alter the > driver masks based on the OF selected endian. Many of the registers are > already accessed as run-time fields because each channel is in a > different bit position. There are only two registers with fields that > cross a byte boundary; COUNT and MOD. These two must be swapped. The > others are either a byte or are accessed base on a channel number. > > For example in fsl_pwm_set_polarity(), we would read the memory, swap > the value, calculate a bit to set, clear or set the bit and then write > back the swapped value all on the 'big_endian' condition. This way, you > just read a bit shift which is conditional on the endian at probe time > and don't double swap. > > In the fsl_counter_clock_enable(), the example is using 'write ignored > bits' and duplicating the value in both big/little bytes. An > alternative is to parameterize all the mask/values in 'const struct' and > have a different pointer for big/little and store this in-place (or via > a pointer) in fsl_pwm_chip. > Bill, I have implemented one version using the method above, and will send the RFC patch, could you help me review it ? Thanks, -- Best Regards, Xiubo
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index eece329..b7d0737 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 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..2401f2d --- /dev/null +++ b/drivers/pwm/pwm-fsl-ftm.c @@ -0,0 +1,455 @@ +/* + * Freescale 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> + +#include <dt-bindings/clock/vf610-clock.h> + +#define FTM_SC 0x00 +#define FTM_SC_CLK_MASK 0x3 +#define FTM_SC_CLK_SHIFT 3 +#define FTM_SC_CLK_SYS (0x1 << FTM_SC_CLK_SHIFT) +#define FTM_SC_CLK_FIX (0x2 << FTM_SC_CLK_SHIFT) +#define FTM_SC_CLK_EXT (0x3 << 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 + +#define FTM_LITTLE 0 +#define FTM_BIG 1 + +struct fsl_pwm_chip { + struct pwm_chip chip; + + struct mutex lock; + + struct clk *sys_clk; + struct clk *counter_clk; + unsigned int counter_clk_select; + unsigned int counter_clk_enable; + unsigned int clk_ps; + + void __iomem *base; + + int period_ns; + int endianess; +}; + +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc, void __iomem *reg) +{ + u32 val; + + val = __raw_readl(reg); + + if (fpc->endianess == FTM_BIG) + return be32_to_cpu(val); + else + return le32_to_cpu(val); +} + +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, u32 val, + void __iomem *reg) +{ + if (fpc->endianess == FTM_BIG) + val = cpu_to_be32(val); + else + val = cpu_to_le32(val); + + __raw_writel(val, reg); +} + +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 unsigned long fsl_rate_to_cycles(struct fsl_pwm_chip *fpc, + unsigned long time_ns) +{ + unsigned long long c; + unsigned long ps = 1 << fpc->clk_ps; + + c = clk_get_rate(fpc->counter_clk); + c = c * time_ns; + do_div(c, 1000000000UL); + do_div(c, ps); + + return (unsigned long)c; +} + +static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + unsigned long 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, + "All the PWMs' period value should be " + "the same\n"); + mutex_unlock(&fpc->lock); + return -EINVAL; + } else { + period = fsl_rate_to_cycles(fpc, period_ns); + if (period > 0xFFFF) { + mutex_unlock(&fpc->lock); + return -EINVAL; + } + + fsl_pwm_writel(fpc, period - 1, fpc->base + FTM_MOD); + fpc->period_ns = period_ns; + } + mutex_unlock(&fpc->lock); + + duty = fsl_rate_to_cycles(fpc, duty_ns); + if (duty >= 0xFFFF) + return -EINVAL; + + fsl_pwm_writel(fpc, FTM_CSC_MSB | FTM_CSC_ELSB, + fpc->base + FTM_CSC(pwm->hwpwm)); + fsl_pwm_writel(fpc, 0, fpc->base + FTM_CNTIN); + 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; + } + + val = fsl_pwm_readl(fpc, fpc->base + FTM_SC); + val &= ~((FTM_SC_CLK_MASK << FTM_SC_CLK_SHIFT) | + (FTM_SC_PS_MASK << FTM_SC_PS_SHIFT)); + + /* select counter clock source */ + switch (fpc->counter_clk_select) { + case VF610_CLK_FTM0: + val |= FTM_SC_CLK_SYS; + break; + case VF610_CLK_FTM0_FIX_SEL: + val |= FTM_SC_CLK_FIX; + break; + case VF610_CLK_FTM0_EXT_SEL: + val |= FTM_SC_CLK_EXT; + break; + default: + fpc->counter_clk_enable--; + return -EINVAL; + } + + val |= fpc->clk_ps; + fsl_pwm_writel(fpc, val, fpc->base + FTM_SC); + + return 0; +} + +static 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); +} + +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); + + val = fsl_pwm_readl(fpc, fpc->base + FTM_OUTINIT); + val &= ~BIT(pwm->hwpwm); + fsl_pwm_writel(fpc, val, fpc->base + FTM_OUTINIT); + + mutex_lock(&fpc->lock); + ret = fsl_counter_clock_enable(fpc); + mutex_unlock(&fpc->lock); + + return ret; +} + +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; + 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_calculate_ps(struct fsl_pwm_chip *fpc) +{ + unsigned long long sys_rate, counter_rate, ratio; + + sys_rate = clk_get_rate(fpc->sys_clk); + if (!sys_rate) + return -EINVAL; + + counter_rate = clk_get_rate(fpc->counter_clk); + if (!counter_rate) { + fpc->counter_clk = fpc->sys_clk; + fpc->counter_clk_select = VF610_CLK_FTM0; + dev_warn(fpc->chip.dev, + "the counter source clock is a dummy clock, " + "so select the system clock as default!\n"); + } + + switch (fpc->counter_clk_select) { + case VF610_CLK_FTM0_FIX_SEL: + ratio = 2 * counter_rate - 1; + do_div(ratio, sys_rate); + fpc->clk_ps = ratio; + break; + case VF610_CLK_FTM0_EXT_SEL: + ratio = 4 * counter_rate - 1; + do_div(ratio, sys_rate); + fpc->clk_ps = ratio; + break; + case VF610_CLK_FTM0: + fpc->clk_ps = 7; + break; + default: + return -EINVAL; + } + + return 0; +} + +static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc) +{ + int ret, index; + struct of_phandle_args clkspec; + struct device_node *np = fpc->chip.dev->of_node; + + fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0"); + if (IS_ERR(fpc->sys_clk)) { + ret = PTR_ERR(fpc->sys_clk); + dev_err(fpc->chip.dev, + "failed to get \"ftm0\" clock %d\n", ret); + return ret; + } + + fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter"); + if (IS_ERR(fpc->counter_clk)) { + ret = PTR_ERR(fpc->counter_clk); + dev_err(fpc->chip.dev, + "failed to get \"ftm0_counter\" clock %d\n", + ret); + return ret; + } + + index = of_property_match_string(np, "clock-names", "ftm0_counter"); + ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, + &clkspec); + if (ret) + return ret; + + fpc->counter_clk_select = clkspec.args[0]; + + return fsl_pwm_calculate_ps(fpc); +} + +static int fsl_pwm_probe(struct platform_device *pdev) +{ + int ret; + const char *endianess; + 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; + + ret = fsl_pwm_parse_clk_ps(fpc); + if (ret < 0) + return ret; + + if (of_property_read_string(np, "endianess", &endianess)) + pr_warning("missing \"endianess\" property, " + "the FTM IP block is little " + "endian as default\n"); + else if (!strcmp("big", endianess)) + fpc->endianess = FTM_BIG; + + 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->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 FTM PWM Driver"); +MODULE_AUTHOR("Xiubo Li <Li.Xiubo@freescale.com>"); +MODULE_ALIAS("platform:fsl-ftm-pwm"); +MODULE_LICENSE("GPL");