On Sun, Mar 13, 2022 at 03:45:24PM -0700, Colin Foster wrote:
> There's an inconsistency that arises when a register set can be accessed
> internally via MMIO, or externally via SPI. The VSC7514 chip allows both
> modes of operation. When internally accessed, the system utilizes __iomem,
> devm_ioremap_resource, and devm_regmap_init_mmio.
> For SPI it isn't possible to utilize memory-mapped IO. To properly operate,
> the resource base must be added to the register before every operation.
The problem here isn't the mixed buses, the problem is that the hardware
designers have for some unknown reason decided to not use the same
register addresses on the two buses which just seems pointlessly
unhelpful.
>
> Signed-off-by: Colin Foster <[email protected]>
> ---
> drivers/base/regmap/internal.h | 1 +
> drivers/base/regmap/regmap.c | 6 ++++++
> include/linux/regmap.h | 3 +++
> 3 files changed, 10 insertions(+)
>
> diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
> index 88f710e7ce31..b4df36c7b17d 100644
> --- a/drivers/base/regmap/internal.h
> +++ b/drivers/base/regmap/internal.h
> @@ -63,6 +63,7 @@ struct regmap {
> regmap_unlock unlock;
> void *lock_arg; /* This is passed to lock/unlock functions */
> gfp_t alloc_flags;
> + unsigned int reg_base;
>
> struct device *dev; /* Device we do I/O on */
> void *work_buf; /* Scratch buffer used to format I/O */
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 1c7c6d6361af..5e12f7cb5147 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -821,6 +821,8 @@ struct regmap *__regmap_init(struct device *dev,
> else
> map->alloc_flags = GFP_KERNEL;
>
> + map->reg_base = config->reg_base;
> +
> map->format.reg_bytes = DIV_ROUND_UP(config->reg_bits, 8);
> map->format.pad_bytes = config->pad_bits / 8;
> map->format.reg_downshift = config->reg_downshift;
> @@ -1736,6 +1738,7 @@ static int _regmap_raw_write_impl(struct regmap *map, unsigned int reg,
> return ret;
> }
>
> + reg += map->reg_base;
> reg >>= map->format.reg_downshift;
> map->format.format_reg(map->work_buf, reg, map->reg_shift);
> regmap_set_work_buf_flag_mask(map, map->format.reg_bytes,
> @@ -1907,6 +1910,7 @@ static int _regmap_bus_formatted_write(void *context, unsigned int reg,
> return ret;
> }
>
> + reg += map->reg_base;
> reg >>= map->format.reg_downshift;
> map->format.format_write(map, reg, val);
>
> @@ -2349,6 +2353,7 @@ static int _regmap_raw_multi_reg_write(struct regmap *map,
> unsigned int reg = regs[i].reg;
> unsigned int val = regs[i].def;
> trace_regmap_hw_write_start(map, reg, 1);
> + reg += map->reg_base;
> reg >>= map->format.reg_downshift;
> map->format.format_reg(u8, reg, map->reg_shift);
> u8 += reg_bytes + pad_bytes;
> @@ -2677,6 +2682,7 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
> return ret;
> }
>
> + reg += map->reg_base;
> reg >>= map->format.reg_downshift;
> map->format.format_reg(map->work_buf, reg, map->reg_shift);
> regmap_set_work_buf_flag_mask(map, map->format.reg_bytes,
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index 40fb9399add6..de81a94d7b30 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -239,6 +239,8 @@ typedef void (*regmap_unlock)(void *);
> * used.
> * @reg_downshift: The number of bits to downshift the register before
> * performing any operations.
> + * @reg_base: Value to be added to every register address before performing any
> + * operation.
> * @pad_bits: Number of bits of padding between register and value.
> * @val_bits: Number of bits in a register value, mandatory.
> *
> @@ -363,6 +365,7 @@ struct regmap_config {
> int reg_bits;
> int reg_stride;
> int reg_downshift;
> + unsigned int reg_base;
> int pad_bits;
> int val_bits;
>
> --
> 2.25.1
>
On Mon, Mar 14, 2022 at 08:28:36AM +0000, Mark Brown wrote:
> On Sun, Mar 13, 2022 at 03:45:24PM -0700, Colin Foster wrote:
>
> > There's an inconsistency that arises when a register set can be accessed
> > internally via MMIO, or externally via SPI. The VSC7514 chip allows both
> > modes of operation. When internally accessed, the system utilizes __iomem,
> > devm_ioremap_resource, and devm_regmap_init_mmio.
>
> > For SPI it isn't possible to utilize memory-mapped IO. To properly operate,
> > the resource base must be added to the register before every operation.
>
> The problem here isn't the mixed buses, the problem is that the hardware
> designers have for some unknown reason decided to not use the same
> register addresses on the two buses which just seems pointlessly
> unhelpful.
That is true for the address downshift. But the resource start
seemingly still needs to be addressed somewhere. Currently that is
contained on a per-regmap basis inside my WIP drivers/mfd/ocelot-spi.c.
But that falls apart as soon as regmap_bus gets involved, as I'm finding
out.
So would it make sense to keep this patch, drop the downshift, and
re-define the offsets / stride to match the modified address space?
>
> >
> > Signed-off-by: Colin Foster <[email protected]>
> > ---
> > drivers/base/regmap/internal.h | 1 +
> > drivers/base/regmap/regmap.c | 6 ++++++
> > include/linux/regmap.h | 3 +++
> > 3 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
> > index 88f710e7ce31..b4df36c7b17d 100644
> > --- a/drivers/base/regmap/internal.h
> > +++ b/drivers/base/regmap/internal.h
> > @@ -63,6 +63,7 @@ struct regmap {
> > regmap_unlock unlock;
> > void *lock_arg; /* This is passed to lock/unlock functions */
> > gfp_t alloc_flags;
> > + unsigned int reg_base;
> >
> > struct device *dev; /* Device we do I/O on */
> > void *work_buf; /* Scratch buffer used to format I/O */
> > diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> > index 1c7c6d6361af..5e12f7cb5147 100644
> > --- a/drivers/base/regmap/regmap.c
> > +++ b/drivers/base/regmap/regmap.c
> > @@ -821,6 +821,8 @@ struct regmap *__regmap_init(struct device *dev,
> > else
> > map->alloc_flags = GFP_KERNEL;
> >
> > + map->reg_base = config->reg_base;
> > +
> > map->format.reg_bytes = DIV_ROUND_UP(config->reg_bits, 8);
> > map->format.pad_bytes = config->pad_bits / 8;
> > map->format.reg_downshift = config->reg_downshift;
> > @@ -1736,6 +1738,7 @@ static int _regmap_raw_write_impl(struct regmap *map, unsigned int reg,
> > return ret;
> > }
> >
> > + reg += map->reg_base;
> > reg >>= map->format.reg_downshift;
> > map->format.format_reg(map->work_buf, reg, map->reg_shift);
> > regmap_set_work_buf_flag_mask(map, map->format.reg_bytes,
> > @@ -1907,6 +1910,7 @@ static int _regmap_bus_formatted_write(void *context, unsigned int reg,
> > return ret;
> > }
> >
> > + reg += map->reg_base;
> > reg >>= map->format.reg_downshift;
> > map->format.format_write(map, reg, val);
> >
> > @@ -2349,6 +2353,7 @@ static int _regmap_raw_multi_reg_write(struct regmap *map,
> > unsigned int reg = regs[i].reg;
> > unsigned int val = regs[i].def;
> > trace_regmap_hw_write_start(map, reg, 1);
> > + reg += map->reg_base;
> > reg >>= map->format.reg_downshift;
> > map->format.format_reg(u8, reg, map->reg_shift);
> > u8 += reg_bytes + pad_bytes;
> > @@ -2677,6 +2682,7 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
> > return ret;
> > }
> >
> > + reg += map->reg_base;
> > reg >>= map->format.reg_downshift;
> > map->format.format_reg(map->work_buf, reg, map->reg_shift);
> > regmap_set_work_buf_flag_mask(map, map->format.reg_bytes,
> > diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> > index 40fb9399add6..de81a94d7b30 100644
> > --- a/include/linux/regmap.h
> > +++ b/include/linux/regmap.h
> > @@ -239,6 +239,8 @@ typedef void (*regmap_unlock)(void *);
> > * used.
> > * @reg_downshift: The number of bits to downshift the register before
> > * performing any operations.
> > + * @reg_base: Value to be added to every register address before performing any
> > + * operation.
> > * @pad_bits: Number of bits of padding between register and value.
> > * @val_bits: Number of bits in a register value, mandatory.
> > *
> > @@ -363,6 +365,7 @@ struct regmap_config {
> > int reg_bits;
> > int reg_stride;
> > int reg_downshift;
> > + unsigned int reg_base;
> > int pad_bits;
> > int val_bits;
> >
> > --
> > 2.25.1
> >