Hi Bin,
On Tue, Sep 25, 2018 at 8:56 AM Bin Meng <bmeng...@gmail.com> wrote:
>
> Hi Mario,
>
> On Tue, Sep 25, 2018 at 2:24 PM Mario Six <mario....@gdsys.cc> wrote:
> >
> > Hi Bin,
> >
> > On Tue, Sep 25, 2018 at 7:48 AM Bin Meng <bmeng...@gmail.com> wrote:
> > >
> > > On Sat, Sep 22, 2018 at 3:55 AM Simon Glass <s...@chromium.org> wrote:
> > > >
> > > > Hi Mario,
> > > >
> > > > On 21 September 2018 at 01:02, Mario Six <mario....@gdsys.cc> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Fri, Aug 17, 2018 at 2:52 PM Simon Glass <s...@chromium.org> wrote:
> > > > > >
> > > > > > Hi Mario,
> > > > > >
> > > > > > On 13 August 2018 at 00:09, Mario Six <mario....@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 <ag...@denx.de>
> > > > > > > Signed-off-by: Mario Six <mario....@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
