On Fri, Oct 30, 2015 at 04:49:41AM -0500, Yuan Yao-B46683 wrote:
> Hi Fabio Estevam,
>
> Thanks for your suggestion.
> We have an internal discussions for that.
>
> We think that:
> According to the initial commit message of regmap, it is targeting non-memory mapped buses. (regmap: Add generic non-memory mapped register access API) But in the imx2_wdt driver, it is used for memory-mapped register space. So it seems that using such a complex framework just to deal with endian is an over-kill.
>
> when it is not necessary to enable the clock every time we access the register.
> We don't think it is obvious to us how to use it for handling endianness, especially not the way imx2_wdt uses regmap. __regmap_init_mmio_clk() calls regmap_mmio_gen_context() which errors out if reg_format_endian is not REGMAP_ENDIAN_DEFAULT or REGMAP_ENDIAN_NATIVE, and elsewhere regmap-mmio.c It seems only little-endian accessors.
>
> Although it is possible to add the endianness support in the regmap_mmio driver, we don't see too much value in using it especially
>
> So we think:
> static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> *addr) {
> if (q->big_endian)
> iowrite32be(val, addr);
> else
> iowrite32(val, addr);
> }
> This way is an easier, more effective solution to do the endian issue.
>
> How about your think?
I think the implement is fine, but I prefer to use quirk rather than
read from dts? Please also rebase the patch to latest l2-mtd code.
>
> Best Regards,
> Yuan Yao
>
> On Sat, Oct 24, 2015 at 11:47 PM, Fabio Estevam <[email protected]> wrote:
> > On Fri, Oct 23, 2015 at 5:53 AM, Yuan Yao <[email protected]> wrote:
> >
> > > +static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > > +*addr) {
> > > + if (q->big_endian)
> > > + iowrite32be(val, addr);
> > > + else
> > > + iowrite32(val, addr);
> > > +}
> >
> > I suggest you to implement regmap support for this driver instead.
> >
> > Take a look at drivers/watchdog/imx2_wdt.c for a reference.
> >
> > Then you only need to pass 'big-endian' as a property for the qspi in the .dtsi
> > file and regmap core will take care of endianness.
--
Best Regards,
Han "Allen" Xu
On Wed, 2015-11-11 at 11:51 -0600, Han Xu wrote:
> On Fri, Oct 30, 2015 at 04:49:41AM -0500, Yuan Yao-B46683 wrote:
> > Hi Fabio Estevam,
> >
> > Thanks for your suggestion.
> > We have an internal discussions for that.
> >
> > We think that:
> > According to the initial commit message of regmap, it is targeting non
> > -memory mapped buses. (regmap: Add generic non-memory mapped register
> > access API) But in the imx2_wdt driver, it is used for memory-mapped
> > register space. So it seems that using such a complex framework just to
> > deal with endian is an over-kill.
> >
> > when it is not necessary to enable the clock every time we access the
> > register.
> > We don't think it is obvious to us how to use it for handling endianness,
> > especially not the way imx2_wdt uses regmap. __regmap_init_mmio_clk()
> > calls regmap_mmio_gen_context() which errors out if reg_format_endian is
> > not REGMAP_ENDIAN_DEFAULT or REGMAP_ENDIAN_NATIVE, and elsewhere regmap
> > -mmio.c It seems only little-endian accessors.
> >
> > Although it is possible to add the endianness support in the regmap_mmio
> > driver, we don't see too much value in using it especially
> >
> > So we think:
> > static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > *addr) {
> > if (q->big_endian)
> > iowrite32be(val, addr);
> > else
> > iowrite32(val, addr);
> > }
> > This way is an easier, more effective solution to do the endian issue.
> >
> > How about your think?
>
> I think the implement is fine, but I prefer to use quirk rather than
> read from dts? Please also rebase the patch to latest l2-mtd code.
What specifically do you mean by "use quirk rather than read from dts"? The
information has to come from the device tree at some point, and it can't be
inferred from the compatible string (and thus the choice of
fsl_qspi_devtype_data instance) because the variance is in how the controller
is hooked up to the SoC, not in the controller itself.
-Scott
+ devicetree
On Wed, Nov 11, 2015 at 11:51:13AM -0600, Han Xu wrote:
> On Fri, Oct 30, 2015 at 04:49:41AM -0500, Yuan Yao-B46683 wrote:
(BTW Yuan, replying on top doesn't make the conversation as easy to
follow)
> > Although it is possible to add the endianness support in the
> > regmap_mmio driver, we don't see too much value in using it
> > especially
> >
> > So we think:
> > static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > *addr) {
> > if (q->big_endian)
> > iowrite32be(val, addr);
> > else
> > iowrite32(val, addr);
> > }
> > This way is an easier, more effective solution to do the endian issue.
> >
> > How about your think?
>
> I think the implement is fine, but I prefer to use quirk rather than
> read from dts? Please also rebase the patch to latest l2-mtd code.
If it really is just a endianness difference, then I think it makes
sense to use the existing DT bindings for it, rather than relying on a
new compatible string / quirk option. That doesn't mean you can't have a
new SoC-inspired compatible property in addition...
> > Best Regards,
> > Yuan Yao
> >
> > On Sat, Oct 24, 2015 at 11:47 PM, Fabio Estevam <[email protected]> wrote:
> > > I suggest you to implement regmap support for this driver instead.
> > >
> > > Take a look at drivers/watchdog/imx2_wdt.c for a reference.
> > >
> > > Then you only need to pass 'big-endian' as a property for the qspi in the .dtsi
> > > file and regmap core will take care of endianness.
To use the standard binding also means that whether or not you choose to
use regmap right now, it's an easy option in the future, and the core
code will already handle it for you. That's really one of the main
reasons for using standardized bindings in the first place.
Brian
On Wed, 2015-11-11 at 11:51 -0600, Han Xu wrote:
> On Fri, Oct 30, 2015 at 04:49:41AM -0500, Yuan Yao-B46683 wrote:
> > Hi Fabio Estevam,
> >
> > Thanks for your suggestion.
> > We have an internal discussions for that.
> >
> > We think that:
> > According to the initial commit message of regmap, it is targeting non-
> memory mapped buses. (regmap: Add generic non-memory mapped register
> access API) But in the imx2_wdt driver, it is used for memory-mapped register
> space. So it seems that using such a complex framework just to deal with
> endian is an over-kill.
> >
> > when it is not necessary to enable the clock every time we access the register.
> > We don't think it is obvious to us how to use it for handling endianness,
> especially not the way imx2_wdt uses regmap. __regmap_init_mmio_clk()
> calls regmap_mmio_gen_context() which errors out if reg_format_endian is
> not REGMAP_ENDIAN_DEFAULT or REGMAP_ENDIAN_NATIVE, and elsewhere
> regmap-mmio.c It seems only little-endian accessors.
> >
> > Although it is possible to add the endianness support in the
> > regmap_mmio driver, we don't see too much value in using it especially
> >
> > So we think:
> > static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > *addr) {
> > if (q->big_endian)
> > iowrite32be(val, addr);
> > else
> > iowrite32(val, addr);
> > }
> > This way is an easier, more effective solution to do the endian issue.
> >
> > How about your think?
>
> I think the implement is fine, but I prefer to use quirk rather than read from dts?
> Please also rebase the patch to latest l2-mtd code.
>
Ok, I will rebase the patch to latest l2-mtd code in the next version.
Thanks.