| Message ID | 20230324093644.464704-3-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Introduce a generic regmap-based MDIO driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
On Fri, Mar 24, 2023 at 10:36:39AM +0100, Maxime Chevallier wrote: > With regmap->reg_base and regmap->reg_downshift, the actual register > address that is going to be used for the next operation might not be the > same as the one we have as an input. Addresses can be offset with > reg_base and shifted, which will affect alignment. > > Check for alignment on the real register address. It is not at all clear to me that the combination of stride and downshift particularly makes sense, and especially not that the stride should be applied after downshifting rather than to what the user is passing in.
Hello Mark, On Fri, 24 Mar 2023 18:51:19 +0000 Mark Brown <broonie@kernel.org> wrote: > On Fri, Mar 24, 2023 at 10:36:39AM +0100, Maxime Chevallier wrote: > > With regmap->reg_base and regmap->reg_downshift, the actual register > > address that is going to be used for the next operation might not > > be the same as the one we have as an input. Addresses can be offset > > with reg_base and shifted, which will affect alignment. > > > > Check for alignment on the real register address. > > It is not at all clear to me that the combination of stride and > downshift particularly makes sense, and especially not that the > stride should be applied after downshifting rather than to what > the user is passing in. I agree on the part where the ordering of "adding and offset, then down/upshifting" isn't natural. This is the order in which operations are done today, and from what I could gather, only the ocelot-spi MFD driver uses both of these operations. It would indeed make sense to first shift the register to have the proper spacing between register addresses, then adding the offset. So maybe we should address that in ocelot-spi in the next iteration, Colin, would you agree ? Thanks, Maxime
On Thu, Mar 30, 2023 at 11:45:46AM +0200, Maxime Chevallier wrote: > Mark Brown <broonie@kernel.org> wrote: > > It is not at all clear to me that the combination of stride and > > downshift particularly makes sense, and especially not that the > > stride should be applied after downshifting rather than to what > > the user is passing in. > I agree on the part where the ordering of "adding and offset, then > down/upshifting" isn't natural. This is the order in which operations > are done today, and from what I could gather, only the ocelot-spi MFD > driver uses both of these operations. Right. I don't think the ordering is particularly intentional, like I say it's not clear that combinding the two makes sense so I think it just wasn't considered at all.
Hi Maxime, On Thu, Mar 30, 2023 at 11:45:46AM +0200, Maxime Chevallier wrote: > Hello Mark, > > On Fri, 24 Mar 2023 18:51:19 +0000 > Mark Brown <broonie@kernel.org> wrote: > > > On Fri, Mar 24, 2023 at 10:36:39AM +0100, Maxime Chevallier wrote: > > > With regmap->reg_base and regmap->reg_downshift, the actual register > > > address that is going to be used for the next operation might not > > > be the same as the one we have as an input. Addresses can be offset > > > with reg_base and shifted, which will affect alignment. > > > > > > Check for alignment on the real register address. > > > > It is not at all clear to me that the combination of stride and > > downshift particularly makes sense, and especially not that the > > stride should be applied after downshifting rather than to what > > the user is passing in. > > I agree on the part where the ordering of "adding and offset, then > down/upshifting" isn't natural. This is the order in which operations > are done today, and from what I could gather, only the ocelot-spi MFD > driver uses both of these operations. > > It would indeed make sense to first shift the register to have the > proper spacing between register addresses, then adding the offset. > > So maybe we should address that in ocelot-spi in the next iteration, > Colin, would you agree ? I'm curious what you mean by "proper spacing". The proper spacing of the VSC7512 (ocelot-spi) is 4, and that's what the reg_stride is. All registers can be accessed in many ways, as I described. Consider the case I brought up, where we're trying to access a register at 0x71070004. If you're on the processor internal to ocelot, this would be uint32 val = *(uint32 *)0x71070004; Accesses to 0x71070001, 0x71070002, and 0x71070003 are not possible. (Well maybe they are _possible_ on some architectures... you get the point though) Translate this to accessing the same register via SPI, where everything is shifted down two bits. We're still accessing 0x71070004, but the way this gets sent on the bus is 0x71070004 >> 2 = 0x1c41c001. This patch set would have allowed accesses to 0x71070001 in the ocelot-spi scenario, but not in the MMIO scenario. That wouldn't be desired. The result of all this is a consistent driver interface. "Give me register 0x4 from a base address of 0x71070000" works in both MMIO and SPI. Everything in /sys/kernel/debug/regmap should be the same in both scenarios as well. All of these scenarios were the motivation behind reg_shift. Colin Foster
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index a4e4367648bf..726f59612fd6 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -2016,7 +2016,7 @@ int regmap_write(struct regmap *map, unsigned int reg, unsigned int val) { int ret; - if (!IS_ALIGNED(reg, map->reg_stride)) + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) return -EINVAL; map->lock(map->lock_arg); @@ -2043,7 +2043,7 @@ int regmap_write_async(struct regmap *map, unsigned int reg, unsigned int val) { int ret; - if (!IS_ALIGNED(reg, map->reg_stride)) + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) return -EINVAL; map->lock(map->lock_arg); @@ -2258,7 +2258,7 @@ int regmap_noinc_write(struct regmap *map, unsigned int reg, return -EINVAL; if (val_len % map->format.val_bytes) return -EINVAL; - if (!IS_ALIGNED(reg, map->reg_stride)) + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) return -EINVAL; if (val_len == 0) return -EINVAL; @@ -2399,7 +2399,7 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, int ret = 0, i; size_t val_bytes = map->format.val_bytes; - if (!IS_ALIGNED(reg, map->reg_stride)) + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) return -EINVAL; /* @@ -2638,7 +2638,7 @@ static int _regmap_multi_reg_write(struct regmap *map, int reg = regs[i].reg; if (!map->writeable_reg(map->dev, reg)) return -EINVAL; - if (!IS_ALIGNED(reg, map->reg_stride)) + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) return -EINVAL; } @@ -2789,7 +2789,7 @@ int regmap_raw_write_async(struct regmap *map, unsigned int reg, if (val_len % map->format.val_bytes) return -EINVAL; - if (!IS_ALIGNED(reg, map->reg_stride)) + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) return -EINVAL; map->lock(map->lock_arg); @@ -2911,7 +2911,7 @@ int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val) { int ret; - if (!IS_ALIGNED(reg, map->reg_stride)) + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) return -EINVAL; map->lock(map->lock_arg); @@ -2945,7 +2945,7 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val, if (val_len % map->format.val_bytes) return -EINVAL; - if (!IS_ALIGNED(reg, map->reg_stride)) + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) return -EINVAL; if (val_count == 0) return -EINVAL; @@ -3040,7 +3040,7 @@ int regmap_noinc_read(struct regmap *map, unsigned int reg, if (val_len % map->format.val_bytes) return -EINVAL; - if (!IS_ALIGNED(reg, map->reg_stride)) + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) return -EINVAL; if (val_len == 0) return -EINVAL; @@ -3162,7 +3162,7 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val, size_t val_bytes = map->format.val_bytes; bool vol = regmap_volatile_range(map, reg, val_count); - if (!IS_ALIGNED(reg, map->reg_stride)) + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) return -EINVAL; if (val_count == 0) return -EINVAL;
With regmap->reg_base and regmap->reg_downshift, the actual register address that is going to be used for the next operation might not be the same as the one we have as an input. Addresses can be offset with reg_base and shifted, which will affect alignment. Check for alignment on the real register address. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- drivers/base/regmap/regmap.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)