diff mbox

[RFC] pwm: Add Freescale FTM PWM driver support

Message ID 1385979309-10505-1-git-send-email-Li.Xiubo@freescale.com (mailing list archive)
State New, archived
Headers show

Commit Message

Xiubo Li Dec. 2, 2013, 10:15 a.m. UTC
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.

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.

And this patch is based the V7 series.


 drivers/pwm/Kconfig       |  10 +
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-fsl-ftm.c | 455 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 466 insertions(+)
 create mode 100644 drivers/pwm/pwm-fsl-ftm.c

Comments

Mark Rutland Dec. 2, 2013, 12:34 p.m. UTC | #1
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.
Xiubo Li Dec. 3, 2013, 4:02 a.m. UTC | #2
> > 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,
Mark Rutland Dec. 3, 2013, 10:36 a.m. UTC | #3
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.
Xiubo Li Dec. 4, 2013, 3:12 a.m. UTC | #4
> > > > 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
Bill Pringlemeir Dec. 19, 2013, 9:06 p.m. UTC | #5
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.
Xiubo Li Dec. 26, 2013, 9:11 a.m. UTC | #6
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
Xiubo Li Jan. 3, 2014, 6:13 a.m. UTC | #7
> 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 mbox

Patch

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");