[v2,2/4] regmap: Allow providing read/write callbacks through struct regmap_config

Hi Simon,

On 07/05/20 07:36PM, Simon Glass wrote:
> >
> > If there a way to use DM w/o DT changes, we can definitely explore that...
> 
> You can certainly do that. Just device_bind() a driver that implements
> your access mechanism. So long as the driver uses the same access
> mechanism no matter what hardware it is running on, that makes sense.
> But from what you say above, that might not be true, so you would end
> up setting the platform data of the regmap driver from its parent.
> That's not terrible, but not ideal.
> 
> Given the simple access you need above, I think you could just add
> that as another option in the regmap, perhaps turning endianness into
> some flags?

I tried doing it without custom regmap accessors. The rough patch below 
is what I came up with. Does it look any better than custom accessors?

PS: I can probably play around with regmap ranges instead of using
'base_offset', but it gets the job done minimally so I think it is good 
enough for a proof-of-concept.
 
 -- 8< --
Subject: [RFC PATCH] regmap: allow specifying base offset and register shift and
 access size

To accommodate more complex access patterns that are needed in the
Cadence Sierra PHY driver (which will be added as a follow-up),
introduce 'base_offset' and 'reg_offset_shift', two values that can be
used to adjust the final regmap read/write address.

'base_offset' is an extra offset on the regmap base discovered from the
device tree. This is useful when some regmaps of a device need to access
memory at one offset, and some at another.

'reg_offset_shift' will shift the register offset left before access.

'size' can be used to specify the width of reads (1/2/4/8 bytes). If 0,
defaults to 4 bytes to maintain backward compatibility.

Signed-off-by: Pratyush Yadav <p.yadav at ti.com>
---
 drivers/core/regmap.c | 16 ++++++++++++----
 include/regmap.h      | 27 ++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 5 deletions(-)

Hi Pratyush,

On Mon, 11 May 2020 at 06:00, Yadav, Pratyush <p.yadav at ti.com> wrote:
>
> Hi Simon,
>
> On 07/05/20 07:36PM, Simon Glass wrote:
> > >
> > > If there a way to use DM w/o DT changes, we can definitely explore that...
> >
> > You can certainly do that. Just device_bind() a driver that implements
> > your access mechanism. So long as the driver uses the same access
> > mechanism no matter what hardware it is running on, that makes sense.
> > But from what you say above, that might not be true, so you would end
> > up setting the platform data of the regmap driver from its parent.
> > That's not terrible, but not ideal.
> >
> > Given the simple access you need above, I think you could just add
> > that as another option in the regmap, perhaps turning endianness into
> > some flags?
>
> I tried doing it without custom regmap accessors. The rough patch below
> is what I came up with. Does it look any better than custom accessors?

This looks OK. Should use a local variable instead of (*mapp)->
though. Also please add docs.


- SImon

>
> PS: I can probably play around with regmap ranges instead of using
> 'base_offset', but it gets the job done minimally so I think it is good
> enough for a proof-of-concept.
>
>  -- 8< --
> Subject: [RFC PATCH] regmap: allow specifying base offset and register shift and
>  access size
>
> To accommodate more complex access patterns that are needed in the
> Cadence Sierra PHY driver (which will be added as a follow-up),
> introduce 'base_offset' and 'reg_offset_shift', two values that can be
> used to adjust the final regmap read/write address.
>
> 'base_offset' is an extra offset on the regmap base discovered from the
> device tree. This is useful when some regmaps of a device need to access
> memory at one offset, and some at another.
>
> 'reg_offset_shift' will shift the register offset left before access.
>
> 'size' can be used to specify the width of reads (1/2/4/8 bytes). If 0,
> defaults to 4 bytes to maintain backward compatibility.
>
> Signed-off-by: Pratyush Yadav <p.yadav at ti.com>
> ---
>  drivers/core/regmap.c | 16 ++++++++++++----
>  include/regmap.h      | 27 ++++++++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> index 41293e5af0..9bce719b45 100644
> --- a/drivers/core/regmap.c
> +++ b/drivers/core/regmap.c
> @@ -38,6 +38,7 @@ static struct regmap *regmap_alloc(int count)
>         map = malloc(sizeof(*map) + sizeof(map->ranges[0]) * count);
>         if (!map)
>                 return NULL;
> +       memset(map, 0, sizeof(*map));
>         map->range_count = count;
>
>         return map;
> @@ -258,6 +259,10 @@ struct regmap *devm_regmap_init(struct udevice *dev,
>         if (rc)
>                 return ERR_PTR(rc);
>
> +       (*mapp)->base_offset = config->base_offset;
> +       (*mapp)->reg_offset_shift = config->reg_offset_shift;
> +       (*mapp)->size = config->size;
> +
>         devres_add(dev, mapp);
>         return *mapp;
>  }
> @@ -343,7 +348,9 @@ int regmap_raw_read_range(struct regmap *map, uint range_num, uint offset,
>         }
>         range = &map->ranges[range_num];
>
> -       ptr = map_physmem(range->start + offset, val_len, MAP_NOCACHE);
> +       offset <<= map->reg_offset_shift;
> +
> +       ptr = map_physmem(range->start + map->base_offset + offset, val_len, MAP_NOCACHE);
>
>         if (offset + val_len > range->size) {
>                 debug("%s: offset/size combination invalid\n", __func__);
> @@ -380,7 +387,7 @@ int regmap_raw_read(struct regmap *map, uint offset, void *valp, size_t val_len)
>
>  int regmap_read(struct regmap *map, uint offset, uint *valp)
>  {
> -       return regmap_raw_read(map, offset, valp, REGMAP_SIZE_32);
> +       return regmap_raw_read(map, offset, valp, map->size ? map->size : REGMAP_SIZE_32);
>  }
>
>  static inline void __write_8(u8 *addr, const u8 *val,
> @@ -452,7 +459,8 @@ int regmap_raw_write_range(struct regmap *map, uint range_num, uint offset,
>         }
>         range = &map->ranges[range_num];
>
> -       ptr = map_physmem(range->start + offset, val_len, MAP_NOCACHE);
> +       offset <<= map->reg_offset_shift;
> +       ptr = map_physmem(range->start + map->base_offset + offset, val_len, MAP_NOCACHE);
>
>         if (offset + val_len > range->size) {
>                 debug("%s: offset/size combination invalid\n", __func__);
> @@ -490,7 +498,7 @@ int regmap_raw_write(struct regmap *map, uint offset, const void *val,
>
>  int regmap_write(struct regmap *map, uint offset, uint val)
>  {
> -       return regmap_raw_write(map, offset, &val, REGMAP_SIZE_32);
> +       return regmap_raw_write(map, offset, &val, map->size ? map->size : REGMAP_SIZE_32);
>  }
>
>  int regmap_update_bits(struct regmap *map, uint offset, uint mask, uint val)
> diff --git a/include/regmap.h b/include/regmap.h
> index 2edbf9359a..4adb04f7e3 100644
> --- a/include/regmap.h
> +++ b/include/regmap.h
> @@ -74,16 +74,41 @@ struct regmap_range {
>  };
>
>  struct regmap_bus;
> -struct regmap_config;
> +/**
> + * struct regmap_config - a way of accessing hardware/bus registers
> + *
> + * @base_offset: Offset from the base of the regmap for reads and writes.
> + *              (OPTIONAL)
> + * @reg_offset_shift: Left shift register offset by this value before
> + *                   performing reads or writes. (OPTIONAL)
> + * @size: Size/width of the read or write operation. Defaults to
> + *       REGMAP_SIZE_32 if set to zero.
> + */
> +struct regmap_config {
> +       u32 base_offset;
> +       u32 reg_offset_shift;
> +       enum regmap_size_t size;
> +};
>
>  /**
>   * struct regmap - a way of accessing hardware/bus registers
>   *
> + * @base_offset: Offset from the base of the regmap for reads and writes.
> + *              (OPTIONAL)
> + * @reg_offset_shift: Left shift register offset by this value before
> + *                   performing reads or writes. (OPTIONAL)
> + * @size: Size/width of the read or write operation. Defaults to
> + *       REGMAP_SIZE_32 if set to zero.
>   * @range_count:       Number of ranges available within the map
>   * @ranges:            Array of ranges
>   */
>  struct regmap {
>         enum regmap_endianness_t endianness;
> +
> +       u32 base_offset;
> +       u32 reg_offset_shift;
> +       enum regmap_size_t size;
> +
>         int range_count;
>         struct regmap_range ranges[0];
>  };
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments India