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

[Mario Six]

Hi Bin,
On Tue, Sep 25, 2018 at 8:56 AM Bin Meng <bmeng.cn at gmail.com> wrote:
>
> 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 :)
>
Sounds good; I'll add some documentation in v7.

> Regards,
> Bin
>
Best regards,
Mario