[U-Boot] [PATCH v6 08/14] regmap: Add raw read/write functions

[Bin Meng]

Hi Mario,

On Tue, Sep 25, 2018 at 2:24 PM Mario Six <mario.six at gdsys.cc> wrote:
>
> Hi Bin,
>
> On Tue, Sep 25, 2018 at 7:48 AM Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > On Sat, Sep 22, 2018 at 3:55 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Mario,
> > >
> > > On 21 September 2018 at 01:02, Mario Six <mario.six at gdsys.cc> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Fri, Aug 17, 2018 at 2:52 PM Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Hi Mario,
> > > > >
> > > > > On 13 August 2018 at 00:09, Mario Six <mario.six at gdsys.cc> wrote:
> > > > > > The regmap functions currently assume that all register map accesses
> > > > > > have a data width of 32 bits, but there are maps that have different
> > > > > > widths.
> > > > > >
> > > > > > To rectify this, implement the regmap_raw_read and regmap_raw_write
> > > > > > functions from the Linux kernel API that specify the width of a desired
> > > > > > read or write operation on a regmap.
> > > > > >
> > > > > > Implement the regmap_read and regmap_write functions using these raw
> > > > > > functions in a backwards-compatible manner.
> > > > > >
> > > > > > Reviewed-by: Anatolij Gustschin <agust at denx.de>
> > > > > > Signed-off-by: Mario Six <mario.six at gdsys.cc>
> > > > > >
> > > > > > ---
> > > > > >
> > > > > > v5 -> v6:
> > > > > > * Corrected format specifier
> > > > > > * Added support for 64-bit reads/writes
> > > > > >
> > > > > > v4 -> v5:
> > > > > > No changes
> > > > > >
> > > > > > v3 -> v4:
> > > > > > * Switched 'ranges[0] + offset' to 'ranges[0].start + offset'
> > > > > > * Explained the difference between the raw and non-raw read/write
> > > > > >   functions better in the docs
> > > > > >
> > > > > > v2 -> v3:
> > > > > > * Implement the "raw" functions from Linux instead of adding a size
> > > > > >   parameter to the regmap_{read,write} functions
> > > > > > * Fixed style violation
> > > > > > * Improved error handling
> > > > > >
> > > > > > v1 -> v2:
> > > > > > New in v2
> > > > > >
> > > > > > ---
> > > > > >  drivers/core/regmap.c | 59 +++++++++++++++++++++++++++++++++++++++++++++------
> > > > > >  include/regmap.h      | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  2 files changed, 110 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> > > > > > index 154426269d9..a2f82091af0 100644
> > > > > > --- a/drivers/core/regmap.c
> > > > > > +++ b/drivers/core/regmap.c
> > > > > > @@ -188,22 +188,67 @@ int regmap_uninit(struct regmap *map)
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > +int regmap_raw_read(struct regmap *map, uint offset, void *valp, size_t val_len)
> > > > > > +{
> > > > > > +       void *ptr;
> > > > > > +
> > > > > > +       ptr = map_physmem(map->ranges[0].start + offset, val_len, MAP_NOCACHE);
> > > > > > +
> > > > > > +       switch (val_len) {
> > > > > > +       case REGMAP_SIZE_8:
> > > > > > +               *((u8 *)valp) = in_8((u8 *)ptr);
> > > > > > +               break;
> > > > > > +       case REGMAP_SIZE_16:
> > > > > > +               *((u16 *)valp) = in_le16((u16 *)ptr);
> > > > > > +               break;
> > > > > > +       case REGMAP_SIZE_32:
> > > > > > +               *((u32 *)valp) = in_le32((u32 *)ptr);
> > > > > > +               break;
> > > > > > +#if defined(in_le64) && defined(readq)
> > > > > > +       case REGMAP_SIZE_64:
> > > > > > +               *((u32 *)valp) = in_le64((u64 *)ptr);
> > > > >
> > > > > How come this is u32? Can you please add a comment?
> > > > >
> > > >
> > > > That was a development version of the patch, sorry (I was in a bit of a hurry).
> > > >
> > > > I'll send a corrected version with v7.
> > > >
> > > > > Why is this using in/out rather than read/write? Does it not support
> > > > > memory-mapped I/O?
> > > > >
> > > >
> > > > It does, but I think the endianness of the read/write operations of the regmap
> > > > should not depend on the architecture, but only on the regmap itself (which is
> > > > little-endian for now; big-endian support can be introduced later on), so I
> > > > used in/out rather than read/write.
> > >
> >
> > If regmap currently only supports little-endian, can we document this somewhere?
> >
> Yes, good idea. I'll send a v7 to address this.
>
> > > What does endianness have to do with whether you use readl/writel or in/out?
> > >
> > > On x86 at least these are actually different things, so regmap() won't
> > > work on x86 with this change.
> > >
> >
> > Looks so far x86's {in,out}_{8,16,32} are using MMIO.
> >
> > > Adding Bin who may understand this better.
> >
> > BTW: I was trying to understand why we need regmap here. Shouldn't the
> > driver directly call {in,out}{bwl}, or {read,write}{bwlq} APIs?
> >
> In this version of the FPGA driver, the regmap is not strictly needed, true.
> But we also have devices that have multiple FPGAs where the "slave" FPGA's
> register maps are accessed through a custom bus interface (called MCLink) on
> the first "master FPGA". With the regmap, I'll be able to use the same driver
> for both, all that's needed is another version of regmap for the MCLink bus. So
> the regmap is really there for forward-compatibility reasons.

Ah, I see. Thanks for the clarification. So I suspect if the same FPGA
someday is connected to I2C, or SPI, the same driver can be used
without any issue. In such case, the regmap can be very helpfu. Isn't
such documented anywhere in U-Boot's source codes to help people
understand? If not, I suggest we can improve something :)

Regards,
Bin