[U-Boot] [PATCH v8 08/15] regmap: Add raw read/write functions

[Daniel Schwierzeck]

On 28.09.2018 09:16, Mario Six wrote:
> Hi Daniel,
> On Thu, Sep 27, 2018 at 1:28 PM Daniel Schwierzeck
> <daniel.schwierzeck at gmail.com> wrote:
>>
>> Hi Mario,
>>
>> Am Do., 27. Sep. 2018 um 11:48 Uhr schrieb Mario Six <mario.six at gdsys.cc>:
>>>
>>> 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>
>>>
>>> ---
>>>
>>> v7 -> v8:
>>> No changes
>>>
>>> v6 -> v7:
>>> * Fixed wrong variable type in 64-bit read (u32 -> u64)
>>> * Added 64-bit case in write function
>>>
>>> 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 | 64 +++++++++++++++++++++++++++++++++++++++++++++------
>>>  include/regmap.h      | 58 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 115 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
>>> index 154426269d9..1025099797f 100644
>>> --- a/drivers/core/regmap.c
>>> +++ b/drivers/core/regmap.c
>>> @@ -188,22 +188,72 @@ 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:
>>> +               *((u64 *)valp) = in_le64((u64 *)ptr);
>>> +               break;
>>> +#endif
>>> +       default:
>>> +               debug("%s: regmap size %zu unknown\n", __func__, val_len);
>>> +               return -EINVAL;
>>> +       }
>>> +       return 0;
>>> +}
>>
>> I'm still not convinced why you want to enforce Little Endian here.
>> This makes regmap unuseable on archs like MIPS or PowerPC.
>>
>> Typically a SoC with a MIPS CPU core can have an AMBA bus or other LE
>> based sub-systems. Some SoCs can select the endianess of the CPU core
>> via pin strapping. Normally the endianess conversion is automatically
>> done in HW.
>>
>> So I suggest to simply use readl or __raw_readl to support native
>> endianess as default. Later we could add a DT properties
>> "native-endian", "big-endian" and "little-endian" like in the Linux
>> implementation.
>>
> Ironically, we want to use the interface on a PowerPC platform :-) (for a
> little-endian FPGA interface that's not auto-translating). But I do see your
> point.
> 
> I just finished testing a v9 of this series that implements support for the
> "native-endian", "big-endian", and "little-endian" DT properties (and uses
> native-endian as the default). That way we can use the interface on our
> platform, and native-endian is the default, so MIPS should be able to use
> regmap too.
> 
> I think that should solve the problem adequately for everyone.
> 

looks good, thank you.

-- 
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180928/469adb1e/attachment.sig>