Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1267390 > unrolled thread

Re: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support Brian Norris <computersforpeace@gmail.com> - 2015-11-11 20:10 +0100
    RE: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support Yao Yuan <yao.yuan@freescale.com> - 2015-11-16 05:30 +0100
      Re: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support Brian Norris <computersforpeace@gmail.com> - 2015-11-16 05:30 +0100
        RE: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support Yao Yuan <yao.yuan@freescale.com> - 2015-11-16 10:20 +0100

#1267390 — Re: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

A few corrections for the record:

On Fri, Oct 30, 2015 at 09:49:41AM +0000, Yao Yuan 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.

It is definitely useful for non-MMIO cases, but it's certainly not
exclusive too it.

> when it is not necessary to enable the clock every time we access the register.  

You don't have to give it a clock. Just pass a NULL clk_id.

> 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 

It already has DT endianness configuration support. See __regmap_init(),
which reconfigures the endianness according to regmap_get_val_endian().
So you don't need to do anything but just try it... I exepct it'll work
just fine.

> 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 there's at least one more advantage: you get pretty good
tracing support for free. For debugging, for example, you can turn on
regmap tracing to see all the register reads and writes done in your
driver, all within the nice tracefs event infrastructure. I'm sure there
are other advantages too, but not all are applicable here.

Anyway, I do agree on the complexity argument. It's not actually that
complex to use (the imx2_wdt.c example really does show you everything
you need to know), it is a bit more complex to sort through all its
features and understand exactly what it's doing. But I'm confident it
has everything you need.

So, make your choice. I just wanted to educate on several points, so
that your decision is not just driven by lack of correct information.

For more information, a quick google search shows a few links, but no
official docs:

http://elinux.org/images/a/a3/Regmap-_The_Power_of_Subsystems_and_Abstractions.pdf
https://lwn.net/Articles/451789/

Brian

> Best Regards,
> Yuan Yao
> 
> On Sat, Oct 24, 2015 at 11:47 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > On Fri, Oct 23, 2015 at 5:53 AM, Yuan Yao <yao.yuan@freescale.com> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1269846

Hi Brian Norris,

Thanks for your information and the documents shared by you.
It's very helpful for me to understand the regmap.

But I think if we use:
static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
*addr) {
       if (q->big_endian)
               iowrite32be(val, addr);
       else
               iowrite32(val, addr);
 }
It looks like easier to read, use and change.
Is it?

And David Woodhouse, Xu Han, Mark Brown
is there any other comments from you?

Thank.

On Thu, Nov 12, 2015 3:04 AM Brian Norris wrote:
> On Fri, Oct 30, 2015 at 09:49:41AM +0000, Yao Yuan 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.
> 
> It is definitely useful for non-MMIO cases, but it's certainly not exclusive too it.
> 
> > when it is not necessary to enable the clock every time we access the register.
> 
> You don't have to give it a clock. Just pass a NULL clk_id.
> 
> > 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
> 
> It already has DT endianness configuration support. See __regmap_init(),
> which reconfigures the endianness according to regmap_get_val_endian().
> So you don't need to do anything but just try it... I exepct it'll work just fine.
> 
> > 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 there's at least one more advantage: you get pretty good tracing
> support for free. For debugging, for example, you can turn on regmap tracing
> to see all the register reads and writes done in your driver, all within the nice
> tracefs event infrastructure. I'm sure there are other advantages too, but not
> all are applicable here.
> 
> Anyway, I do agree on the complexity argument. It's not actually that complex
> to use (the imx2_wdt.c example really does show you everything you need to
> know), it is a bit more complex to sort through all its features and understand
> exactly what it's doing. But I'm confident it has everything you need.
> 
> So, make your choice. I just wanted to educate on several points, so that your
> decision is not just driven by lack of correct information.
> 
> For more information, a quick google search shows a few links, but no official
> docs:
> 
> http://elinux.org/images/a/a3/Regmap-
> _The_Power_of_Subsystems_and_Abstractions.pdf
> https://lwn.net/Articles/451789/
> 
> Brian
> 
> > Best Regards,
> > Yuan Yao
> >
> > On Sat, Oct 24, 2015 at 11:47 PM, Fabio Estevam <festevam@gmail.com>
> wrote:
> > > On Fri, Oct 23, 2015 at 5:53 AM, Yuan Yao <yao.yuan@freescale.com> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1269848

On Mon, Nov 16, 2015 at 04:08:39AM +0000, Yao Yuan wrote:
> It looks like easier to read, use and change.
> Is it?

It looks fine either way, IMO.

> And David Woodhouse, Xu Han, Mark Brown
> is there any other comments from you?

FYI, Mark Brown doesn't really have much to do with this. Though he
maintains the SPI subsystem (and regmap, as a matter of fact), SPI *NOR*
code is only a user of the SPI subsystem (and, potentially, regmap). I
hear he's not interested in reviewing MTD work, given the volume of
email he already receives.

Regards,
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1269931

Hi Brian,

Thanks for your information.

Best Regards,
Yuan Yao

On Mon, Nov 16, 2015 at 12:22:30PM +0000, Brian Norris wrote:
> On Mon, Nov 16, 2015 at 04:08:39AM +0000, Yao Yuan wrote:
> > It looks like easier to read, use and change.
> > Is it?
> 
> It looks fine either way, IMO.
> 
> > And David Woodhouse, Xu Han, Mark Brown is there any other comments
> > from you?
> 
> FYI, Mark Brown doesn't really have much to do with this. Though he maintains
> the SPI subsystem (and regmap, as a matter of fact), SPI *NOR* code is only a
> user of the SPI subsystem (and, potentially, regmap). I hear he's not interested
> in reviewing MTD work, given the volume of email he already receives.
> 
> Regards,
> Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web