[RFC] pwm: Add Freescale FTM PWM driver support

[Li Xiubo]

> > > > 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 at 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