Page select register restrictions in regmap core

Page select register restrictions in regmap core

[Guenter Roeck]

Hi,

I have been trying to use the regmap page select mechanism to implement
page access in the upcoming spd5118 driver [1]. Unfortunately, that fails
with the following error.

spd5118 0-0050: Range 0: selector for 1 in window
spd5118 0-0050: error -EINVAL: regmap init failed

The page select register in spd5118 devices is outside the paged area
in an area which is otherwise (i.e., for other registers in that area)
only accessible from page 0. The regmap range configuration looks as
follows.

static const struct regmap_range_cfg regmap_range_cfg[] = {
	/*
	 * volatile registers, only accessible from page 0 except for
	 * the page select register
	 */
        {
        .selector_reg     = SPD5118_REG_I2C_LEGACY_MODE,	// 0x0b
        .selector_mask    = SPD5118_LEGACY_PAGE_MASK,		// 0x07
        .selector_shift   = 0,
        .window_start     = 0,
        .window_len       = SPD5118_PAGE_SIZE,			// 128
        .range_min        = 0,
        .range_max        = SPD5118_PAGE_SIZE - 1,		// 127
        },
	/* non-volatile data, pages 0..7 */
        {
        .selector_reg     = SPD5118_REG_I2C_LEGACY_MODE,	// 0x0b
        .selector_mask    = SPD5118_LEGACY_PAGE_MASK,		// 0x07
        .selector_shift   = 0,
        .window_start     = SPD5118_PAGE_SIZE,			// 128
        .window_len       = SPD5118_PAGE_SIZE,			// 128
        .range_min        = SPD5118_PAGE_SIZE,			// 128
        .range_max        = 9 * SPD5118_PAGE_SIZE - 1,		// 0x47f
        },
};

This works just fine if I comment out the select register validation in
the regmap core (the one which generates the error). Is there a reason
for having this restriction, or would it be possible to drop it ?
If dropping it is not possible, could we possibly add some flag to the
configuration data, indicating that this "violation" is on purpose ?
Alternatively, do you see some other means to describe the ranges which
would not violate the restrictions ?

Thanks,
Guenter

---
[1] https://patchwork.kernel.org/project/linux-hwmon/patch/20240610144103.1970359-3-linux@roeck-us.net/

Re: Page select register restrictions in regmap core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

On Mon, Jun 17, 2024 at 07:20:28AM -0700, Guenter Roeck wrote:

>         .window_start     = 0,
>         .window_len       = SPD5118_PAGE_SIZE,			// 128
>         .range_min        = 0,
>         .range_max        = SPD5118_PAGE_SIZE - 1,		// 127


>         .window_start     = SPD5118_PAGE_SIZE,			// 128
>         .window_len       = SPD5118_PAGE_SIZE,			// 128
>         .range_min        = SPD5118_PAGE_SIZE,			// 128
>         .range_max        = 9 * SPD5118_PAGE_SIZE - 1,		// 0x47f

You appear to be trying to define ranges that overlap with the windows
that you're trying to expose.  I can't understand what that's trying to
represent or how that would work.  The window is the physical registers
that the host can actually see, the range is the virtual addresses which
users of the region should use to access registers behind the window.
This should be a range of register values which don't physically exist
on the device.  I really can't understand what a sensible handling of an
overlap would be, any attempt to access the window should recursively
trigger selection of the range so no actual register should work.  I
can't tell what it's trying to model.

This configuration would also be rejected by the next test which
verifies that the window does not overlap with the range.

> This works just fine if I comment out the select register validation in
> the regmap core (the one which generates the error). Is there a reason
> for having this restriction, or would it be possible to drop it ?

I'm very surprised this does anything useful TBH, possibly that's a bug
of some kind.  A configuration with the selector within the range is
nonsensical as far as I can see since the range can't be accessed
without programming the selector, the range should be purely virtual
registers and the selector must be a physical register.

> If dropping it is not possible, could we possibly add some flag to the
> configuration data, indicating that this "violation" is on purpose ?
> Alternatively, do you see some other means to describe the ranges which
> would not violate the restrictions ?

Like I say I can't tell what this is trying to describe or how it could
possibly work.  The range should be completely distinct from the window.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Page select register restrictions in regmap core

[Guenter Roeck]

Hi Mark,

On Mon, Jun 17, 2024 at 04:08:08PM +0100, Mark Brown wrote:
> On Mon, Jun 17, 2024 at 07:20:28AM -0700, Guenter Roeck wrote:
> 
> >         .window_start     = 0,
> >         .window_len       = SPD5118_PAGE_SIZE,			// 128
> >         .range_min        = 0,
> >         .range_max        = SPD5118_PAGE_SIZE - 1,		// 127
> 
> 
> >         .window_start     = SPD5118_PAGE_SIZE,			// 128
> >         .window_len       = SPD5118_PAGE_SIZE,			// 128
> >         .range_min        = SPD5118_PAGE_SIZE,			// 128
> >         .range_max        = 9 * SPD5118_PAGE_SIZE - 1,		// 0x47f
> 
> You appear to be trying to define ranges that overlap with the windows
> that you're trying to expose.  I can't understand what that's trying to
> represent or how that would work.  The window is the physical registers
> that the host can actually see, the range is the virtual addresses which
> users of the region should use to access registers behind the window.
> This should be a range of register values which don't physically exist
> on the device.  I really can't understand what a sensible handling of an

Can you point me to an example ? All examples I can find have overlapping
values for .range_min/.range_max and .window_start/.window_len, and pretty
much all of them start range_min with 0.

> overlap would be, any attempt to access the window should recursively
> trigger selection of the range so no actual register should work.  I
> can't tell what it's trying to model.
> 

page 0: 0x00-0x7f	Volatile registers, page selector at 0x0b
	0x80-0xff	page 0 of non-volatile memory
page 1:	0x0b		page selector register	<-- this is what trips the check
	0x80-0xff	page 1 of non-volatile memory
...
page 7:	0x0b		page selector register
	0x80-0xff	page 7 of non-volatile memory

> This configuration would also be rejected by the next test which
> verifies that the window does not overlap with the range.
> 

No, it isn't. The windows in the two ranges don't overlap, and neither
do the ranges. The only overlap is the selector register. The check you
refer to explicitly does not apply to a single range.

> > This works just fine if I comment out the select register validation in
> > the regmap core (the one which generates the error). Is there a reason
> > for having this restriction, or would it be possible to drop it ?
> 
> I'm very surprised this does anything useful TBH, possibly that's a bug
> of some kind.  A configuration with the selector within the range is
> nonsensical as far as I can see since the range can't be accessed
> without programming the selector, the range should be purely virtual
> registers and the selector must be a physical register.

Pretty much all drivers I looked at start the range with 0, having
the selector register within the range is explicitly accepted by the
regmap code, and pretty much all drivers using regmap for page
selection do that. The difference here is that the page selector
register is in the first range and visible from all pages, but the
other volatile registers are only visible in page 0.
Yes, I would agree that this doesn't make much sense, but it is what
the spd5118 standard calls for, and at least the Renesas/IDT spd5188
chip implements it that way.

Anyway, how should I model this ?

Thanks,
Guenter

Re: Page select register restrictions in regmap core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3337 bytes --]

On Mon, Jun 17, 2024 at 09:59:50AM -0700, Guenter Roeck wrote:
> On Mon, Jun 17, 2024 at 04:08:08PM +0100, Mark Brown wrote:

> > You appear to be trying to define ranges that overlap with the windows
> > that you're trying to expose.  I can't understand what that's trying to
> > represent or how that would work.  The window is the physical registers
> > that the host can actually see, the range is the virtual addresses which
> > users of the region should use to access registers behind the window.
> > This should be a range of register values which don't physically exist
> > on the device.  I really can't understand what a sensible handling of an

> Can you point me to an example ? All examples I can find have overlapping
> values for .range_min/.range_max and .window_start/.window_len, and pretty
> much all of them start range_min with 0.

sound/soc/codecs/wm2200.c.  I do see a bunch of bad examples now I grep,
bluntly I'm astonished any of them do anything useful and wonder if
anyone has even run the code.

> > overlap would be, any attempt to access the window should recursively
> > trigger selection of the range so no actual register should work.  I
> > can't tell what it's trying to model.

> page 0: 0x00-0x7f	Volatile registers, page selector at 0x0b
> 	0x80-0xff	page 0 of non-volatile memory
> page 1:	0x0b		page selector register	<-- this is what trips the check
> 	0x80-0xff	page 1 of non-volatile memory
> ...
> page 7:	0x0b		page selector register
> 	0x80-0xff	page 7 of non-volatile memory

So you've got two windows from 0-0x7f and 0x80-0xff which share a
selector register because of course that makes sense, the selector is
placed inside one of the ranges.  That's all perfectly fine, modulo the
multi-use selector register the hardware seems fine.  What I don't
understand is what the attempt to put the window on top of this is
supposed to mean.

> > This configuration would also be rejected by the next test which
> > verifies that the window does not overlap with the range.

> No, it isn't. The windows in the two ranges don't overlap, and neither
> do the ranges. The only overlap is the selector register. The check you
> refer to explicitly does not apply to a single range.

Ugh, it should - like I say these configurations are just incoherent
nonsense.

> Pretty much all drivers I looked at start the range with 0, having
> the selector register within the range is explicitly accepted by the
> regmap code, and pretty much all drivers using regmap for page
> selection do that. The difference here is that the page selector
> register is in the first range and visible from all pages, but the
> other volatile registers are only visible in page 0.

Having the page selector register be inside the page is pretty common.

> Yes, I would agree that this doesn't make much sense, but it is what
> the spd5118 standard calls for, and at least the Renesas/IDT spd5188
> chip implements it that way.

The range is *entirely* defined within the driver, it is 100% a software
construct, the hardware only influences our choice of range in that we
can't place it on top of hardware registers.

> Anyway, how should I model this ?

To repeat:

> > Like I say I can't tell what this is trying to describe or how it could
> > possibly work.  The range should be completely distinct from the window.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Page select register restrictions in regmap core

[Guenter Roeck]

On 6/17/24 10:22, Mark Brown wrote:
[ ... ]
> 
> The range is *entirely* defined within the driver, it is 100% a software
> construct, the hardware only influences our choice of range in that we
> can't place it on top of hardware registers.
> 

I _think_ what you are saying is that I'd have to model all registers
which are to be addressed through regmap as virtual registers with an offset
outside the range of real registers. Something like adding 0x100 to the
each register address and then accessing, say, the revision register
not as register 0x02 but as register 0x102. I would then define the matching
range from 0x100 .. 0x17f and the window from 0x00..0x7f.

Hmm, yes, I see that this should work. I don't think it is worth doing though
since I need to be able to access some registers outside regmap, and I'd have
to define two sets of addresses for all those registers. That would simplify
the code a bit but one would have to remember that register addresses through
regmap are different than register addresses when calling smbus functions
directly. I think we'll just stick with the current code and keep the paging
implementation in the driver.

Thanks a lot for the advice.

Guenter

Re: Page select register restrictions in regmap core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]

On Mon, Jun 17, 2024 at 02:55:09PM -0700, Guenter Roeck wrote:
> On 6/17/24 10:22, Mark Brown wrote:

> > The range is *entirely* defined within the driver, it is 100% a software
> > construct, the hardware only influences our choice of range in that we
> > can't place it on top of hardware registers.

> I _think_ what you are saying is that I'd have to model all registers
> which are to be addressed through regmap as virtual registers with an offset
> outside the range of real registers. Something like adding 0x100 to the

No, only registers that are accessed through a window need to be
mapped into a range.  Any other registers can just be accessed.

> each register address and then accessing, say, the revision register
> not as register 0x02 but as register 0x102. I would then define the matching
> range from 0x100 .. 0x17f and the window from 0x00..0x7f.

That would make the range exactly the same size as the window so there'd
be no paging going on and the registers could be accessed directly?  I
guess that's another check that should be added...

> Hmm, yes, I see that this should work. I don't think it is worth doing though
> since I need to be able to access some registers outside regmap, and I'd have
> to define two sets of addresses for all those registers. That would simplify
> the code a bit but one would have to remember that register addresses through
> regmap are different than register addresses when calling smbus functions
> directly. I think we'll just stick with the current code and keep the paging
> implementation in the driver.

Mixing regmap and non-regmap access to the same registers seems like a
bad idea in general, you will have locking issues (especially around the
paging).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Page select register restrictions in regmap core

[Guenter Roeck]

On 6/17/24 15:47, Mark Brown wrote:
> On Mon, Jun 17, 2024 at 02:55:09PM -0700, Guenter Roeck wrote:
>> On 6/17/24 10:22, Mark Brown wrote:
> 
>>> The range is *entirely* defined within the driver, it is 100% a software
>>> construct, the hardware only influences our choice of range in that we
>>> can't place it on top of hardware registers.
> 
>> I _think_ what you are saying is that I'd have to model all registers
>> which are to be addressed through regmap as virtual registers with an offset
>> outside the range of real registers. Something like adding 0x100 to the
> 
> No, only registers that are accessed through a window need to be
> mapped into a range.  Any other registers can just be accessed.
> 
See below.

>> each register address and then accessing, say, the revision register
>> not as register 0x02 but as register 0x102. I would then define the matching
>> range from 0x100 .. 0x17f and the window from 0x00..0x7f.
> 
> That would make the range exactly the same size as the window so there'd
> be no paging going on and the registers could be accessed directly?  I
> guess that's another check that should be added...
> 

I tried to explain this before. The registers in address range 00..0x7f
are physical, but they are only accessible from page 0 with the exception
of the page select register. So, sure, the registers are not actually paged,
but page 0 must be selected to access them. That is the one and only reason
for specifying that first range and window. It ensures that page 0 is
selected when accessing the registers. If that wasn't the case, I could
define a single range for the actually paged addresses in the 0x80..0xff
window and be done with it.

>> Hmm, yes, I see that this should work. I don't think it is worth doing though
>> since I need to be able to access some registers outside regmap, and I'd have
>> to define two sets of addresses for all those registers. That would simplify
>> the code a bit but one would have to remember that register addresses through
>> regmap are different than register addresses when calling smbus functions
>> directly. I think we'll just stick with the current code and keep the paging
>> implementation in the driver.
> 
> Mixing regmap and non-regmap access to the same registers seems like a
> bad idea in general, you will have locking issues (especially around the
> paging).

The non-regmap access all happens in the probe function before regmap is
initialized. It is needed for basic chip identification, to prevent someone
from instantiating the driver on a random nvram/eeprom and messing it up
with attempts to write the page select register. I would not want to be
held responsible for someone with, say, DDR4 DIMMs force-instantiating
the spd5118 driver and then complaining about bricked DIMMs.

Thanks,
Guenter

Re: Page select register restrictions in regmap core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

On Mon, Jun 17, 2024 at 04:15:33PM -0700, Guenter Roeck wrote:
> On 6/17/24 15:47, Mark Brown wrote:
> > On Mon, Jun 17, 2024 at 02:55:09PM -0700, Guenter Roeck wrote:
> > > On 6/17/24 10:22, Mark Brown wrote:

> > > each register address and then accessing, say, the revision register
> > > not as register 0x02 but as register 0x102. I would then define the matching
> > > range from 0x100 .. 0x17f and the window from 0x00..0x7f.

> > That would make the range exactly the same size as the window so there'd
> > be no paging going on and the registers could be accessed directly?  I
> > guess that's another check that should be added...

> I tried to explain this before. The registers in address range 00..0x7f
> are physical, but they are only accessible from page 0 with the exception
> of the page select register. So, sure, the registers are not actually paged,
> but page 0 must be selected to access them. That is the one and only reason
> for specifying that first range and window. It ensures that page 0 is
> selected when accessing the registers. If that wasn't the case, I could
> define a single range for the actually paged addresses in the 0x80..0xff
> window and be done with it.

So surely this means that the entire register map is one window and
there's no point in defining two ranges?  Those registers are paged with
the same selector as the other registers.  At which point you can just
sidestep the issue and be like the other current problematic drivers.

> The non-regmap access all happens in the probe function before regmap is
> initialized. It is needed for basic chip identification, to prevent someone
> from instantiating the driver on a random nvram/eeprom and messing it up
> with attempts to write the page select register. I would not want to be
> held responsible for someone with, say, DDR4 DIMMs force-instantiating
> the spd5118 driver and then complaining about bricked DIMMs.

What some devices do for enumeration if the fully specified regmap won't
work for all is create a trivial regmap used for enumeration, then throw
that away and instantiate a new regmap based on the results of initial
identification, though that wouldn't really work for letting you skip
paging.  I don't see how you avoid handling paging in the probe theough,
unless you just assume that the chip is left on page 0 by whatever came
before.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Page select register restrictions in regmap core

[Guenter Roeck]

On 6/18/24 04:35, Mark Brown wrote:
> On Mon, Jun 17, 2024 at 04:15:33PM -0700, Guenter Roeck wrote:
>> On 6/17/24 15:47, Mark Brown wrote:
>>> On Mon, Jun 17, 2024 at 02:55:09PM -0700, Guenter Roeck wrote:
>>>> On 6/17/24 10:22, Mark Brown wrote:
> 
>>>> each register address and then accessing, say, the revision register
>>>> not as register 0x02 but as register 0x102. I would then define the matching
>>>> range from 0x100 .. 0x17f and the window from 0x00..0x7f.
> 
>>> That would make the range exactly the same size as the window so there'd
>>> be no paging going on and the registers could be accessed directly?  I
>>> guess that's another check that should be added...
> 
>> I tried to explain this before. The registers in address range 00..0x7f
>> are physical, but they are only accessible from page 0 with the exception
>> of the page select register. So, sure, the registers are not actually paged,
>> but page 0 must be selected to access them. That is the one and only reason
>> for specifying that first range and window. It ensures that page 0 is
>> selected when accessing the registers. If that wasn't the case, I could
>> define a single range for the actually paged addresses in the 0x80..0xff
>> window and be done with it.
> 
> So surely this means that the entire register map is one window and
> there's no point in defining two ranges?  Those registers are paged with
> the same selector as the other registers.  At which point you can just
> sidestep the issue and be like the other current problematic drivers.
> 

Just define a single range covering the entire window ? That might actually
work if I manipulate the nvmem addresses such that they always point to the
upper 64 bytes. I'll give that a try.

>> The non-regmap access all happens in the probe function before regmap is
>> initialized. It is needed for basic chip identification, to prevent someone
>> from instantiating the driver on a random nvram/eeprom and messing it up
>> with attempts to write the page select register. I would not want to be
>> held responsible for someone with, say, DDR4 DIMMs force-instantiating
>> the spd5118 driver and then complaining about bricked DIMMs.
> 
> What some devices do for enumeration if the fully specified regmap won't
> work for all is create a trivial regmap used for enumeration, then throw
> that away and instantiate a new regmap based on the results of initial
> identification, though that wouldn't really work for letting you skip
> paging.  I don't see how you avoid handling paging in the probe theough,
> unless you just assume that the chip is left on page 0 by whatever came
> before.

Since it is known that other registers return 0 if they are on a page
other than 0, I can check if the page register is not 0 and valid, and
if the other registers do all return 0. That is not perfect either,
but if that passes I can select page 0 and check if those other registers
now return valid data. If that is not the case I write the old value back
into the page register and abort. Yes, I know, that isn't perfect.

Anyway, this may be all irrelevant in respect to regmap support.
It turns out that at least some i801 controllers don't work with the
access mechanism used by regmap, or maybe the spd5118 chips don't support
I2C_FUNC_SMBUS_I2C_BLOCK. I already found that those chips don't support
auto-incrementing the register address and actually reset the address on byte
reads (i.e., subsequent calls to i2c_smbus_read_byte() always return the data
from the first register). Since regmap doesn't have a means for me to say
"don't use I2C_FUNC_SMBUS_I2C_BLOCK even if the controller supports it",
I may have to drop regmap support entirely anyway. That would be annoying,
but right now I have no idea how to work around that problem.

Thanks,
Guenter

Re: Page select register restrictions in regmap core

[Guenter Roeck]

Hi Mark,

On 6/18/24 09:14, Guenter Roeck wrote:
[ ... ]
>> So surely this means that the entire register map is one window and
>> there's no point in defining two ranges?  Those registers are paged with
>> the same selector as the other registers.  At which point you can just
>> sidestep the issue and be like the other current problematic drivers.
>>
> 
> Just define a single range covering the entire window ? That might actually
> work if I manipulate the nvmem addresses such that they always point to the
> upper 64 bytes. I'll give that a try.
> 

Excellent, that worked. Thanks a lot, and sorry for being slow in understanding
what I needed to do.

[ ... ]
> 
> Anyway, this may be all irrelevant in respect to regmap support.
> It turns out that at least some i801 controllers don't work with the
> access mechanism used by regmap, or maybe the spd5118 chips don't support
> I2C_FUNC_SMBUS_I2C_BLOCK. I already found that those chips don't support
> auto-incrementing the register address and actually reset the address on byte
> reads (i.e., subsequent calls to i2c_smbus_read_byte() always return the data
> from the first register). Since regmap doesn't have a means for me to say
> "don't use I2C_FUNC_SMBUS_I2C_BLOCK even if the controller supports it",
> I may have to drop regmap support entirely anyway. That would be annoying,
> but right now I have no idea how to work around that problem.
> 

I think I found a workaround for that problem: All I needed to do
was to define a regmap bus with its own read and write functions.
Please let me know if there is a better way to solve that problem.

Thanks,
Guenter

Re: Page select register restrictions in regmap core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 966 bytes --]

On Tue, Jun 18, 2024 at 09:14:56AM -0700, Guenter Roeck wrote:

> Anyway, this may be all irrelevant in respect to regmap support.
> It turns out that at least some i801 controllers don't work with the
> access mechanism used by regmap, or maybe the spd5118 chips don't support
> I2C_FUNC_SMBUS_I2C_BLOCK. I already found that those chips don't support
> auto-incrementing the register address and actually reset the address on byte
> reads (i.e., subsequent calls to i2c_smbus_read_byte() always return the data
> from the first register). Since regmap doesn't have a means for me to say
> "don't use I2C_FUNC_SMBUS_I2C_BLOCK even if the controller supports it",
> I may have to drop regmap support entirely anyway. That would be annoying,
> but right now I have no idea how to work around that problem.

You can set the use_single_read and use_single_write flags in the config
to ensure registers are accessed one at a time, that restriction is
moderately common.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Page select register restrictions in regmap core

[Guenter Roeck]

On 6/18/24 10:45, Mark Brown wrote:
> On Tue, Jun 18, 2024 at 09:14:56AM -0700, Guenter Roeck wrote:
> 
>> Anyway, this may be all irrelevant in respect to regmap support.
>> It turns out that at least some i801 controllers don't work with the
>> access mechanism used by regmap, or maybe the spd5118 chips don't support
>> I2C_FUNC_SMBUS_I2C_BLOCK. I already found that those chips don't support
>> auto-incrementing the register address and actually reset the address on byte
>> reads (i.e., subsequent calls to i2c_smbus_read_byte() always return the data
>> from the first register). Since regmap doesn't have a means for me to say
>> "don't use I2C_FUNC_SMBUS_I2C_BLOCK even if the controller supports it",
>> I may have to drop regmap support entirely anyway. That would be annoying,
>> but right now I have no idea how to work around that problem.
> 
> You can set the use_single_read and use_single_write flags in the config
> to ensure registers are accessed one at a time, that restriction is
> moderately common.

I'll give it a try.

Thanks!

Guenter

Re: Page select register restrictions in regmap core

[Guenter Roeck]

On 6/18/24 10:45, Mark Brown wrote:
> On Tue, Jun 18, 2024 at 09:14:56AM -0700, Guenter Roeck wrote:
> 
>> Anyway, this may be all irrelevant in respect to regmap support.
>> It turns out that at least some i801 controllers don't work with the
>> access mechanism used by regmap, or maybe the spd5118 chips don't support
>> I2C_FUNC_SMBUS_I2C_BLOCK. I already found that those chips don't support
>> auto-incrementing the register address and actually reset the address on byte
>> reads (i.e., subsequent calls to i2c_smbus_read_byte() always return the data
>> from the first register). Since regmap doesn't have a means for me to say
>> "don't use I2C_FUNC_SMBUS_I2C_BLOCK even if the controller supports it",
>> I may have to drop regmap support entirely anyway. That would be annoying,
>> but right now I have no idea how to work around that problem.
> 
> You can set the use_single_read and use_single_write flags in the config
> to ensure registers are accessed one at a time, that restriction is
> moderately common.

That doesn't help, unfortunately. Thinking about it, that is not really
surprising. The failing write is to the page register, and that was
a single write anyway.

Thanks,
Guenter

Re: Page select register restrictions in regmap core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3020 bytes --]

On Tue, Jun 18, 2024 at 12:33:40PM -0700, Guenter Roeck wrote:
> On 6/18/24 10:45, Mark Brown wrote:
> > On Tue, Jun 18, 2024 at 09:14:56AM -0700, Guenter Roeck wrote:

> > > It turns out that at least some i801 controllers don't work with the
> > > access mechanism used by regmap, or maybe the spd5118 chips don't support
> > > I2C_FUNC_SMBUS_I2C_BLOCK. I already found that those chips don't support
> > > auto-incrementing the register address and actually reset the address on byte
> > > reads (i.e., subsequent calls to i2c_smbus_read_byte() always return the data
> > > from the first register). Since regmap doesn't have a means for me to say
> > > "don't use I2C_FUNC_SMBUS_I2C_BLOCK even if the controller supports it",
> > > I may have to drop regmap support entirely anyway. That would be annoying,
> > > but right now I have no idea how to work around that problem.

> > You can set the use_single_read and use_single_write flags in the config
> > to ensure registers are accessed one at a time, that restriction is
> > moderately common.

> That doesn't help, unfortunately. Thinking about it, that is not really
> surprising. The failing write is to the page register, and that was
> a single write anyway.

Oh, that's interesting - I'm kind of surprised the wire protocols differ
but it's been a while since I looked.  We should probably add this to
the quirking in regmap-i2c.c, have it select one the 8 bit only smbus
versions for devices that need register at a time operation.  I'd not be
surprised if other devices have issues, and anyway if it makes a
difference to the wire protocol we should try to select something as
close as possible to what we're actually doing.

Something like the below perhaps (this probably needs to be converted to
a match table type thing at this point, there's the SMBUS_WORD_DATA case
too):

diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
index a905e955bbfc..499fcec00f2d 100644
--- a/drivers/base/regmap/regmap-i2c.c
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -313,6 +313,11 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
 
 	if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
 		bus = &regmap_i2c;
+	else if (config->val_bits == 8 && config->reg_bits == 8 &&
+		 config->use_single_read && config->use_single_write &&
+		 i2c_check_functionality(i2c->adapter,
+					 I2C_FUNC_SMBUS_BYTE_DATA))
+		bus = &regmap_smbus_byte;
 	else if (config->val_bits == 8 && config->reg_bits == 8 &&
 		 i2c_check_functionality(i2c->adapter,
 					 I2C_FUNC_SMBUS_I2C_BLOCK))
@@ -334,10 +339,6 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
 		default:		/* everything else is not supported */
 			break;
 		}
-	else if (config->val_bits == 8 && config->reg_bits == 8 &&
-		 i2c_check_functionality(i2c->adapter,
-					 I2C_FUNC_SMBUS_BYTE_DATA))
-		bus = &regmap_smbus_byte;
 
 	if (!bus)
 		return ERR_PTR(-ENOTSUPP);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Page select register restrictions in regmap core

[Guenter Roeck]

On 6/18/24 13:46, Mark Brown wrote:
> On Tue, Jun 18, 2024 at 12:33:40PM -0700, Guenter Roeck wrote:
>> On 6/18/24 10:45, Mark Brown wrote:
>>> On Tue, Jun 18, 2024 at 09:14:56AM -0700, Guenter Roeck wrote:
> 
>>>> It turns out that at least some i801 controllers don't work with the
>>>> access mechanism used by regmap, or maybe the spd5118 chips don't support
>>>> I2C_FUNC_SMBUS_I2C_BLOCK. I already found that those chips don't support
>>>> auto-incrementing the register address and actually reset the address on byte
>>>> reads (i.e., subsequent calls to i2c_smbus_read_byte() always return the data
>>>> from the first register). Since regmap doesn't have a means for me to say
>>>> "don't use I2C_FUNC_SMBUS_I2C_BLOCK even if the controller supports it",
>>>> I may have to drop regmap support entirely anyway. That would be annoying,
>>>> but right now I have no idea how to work around that problem.
> 
>>> You can set the use_single_read and use_single_write flags in the config
>>> to ensure registers are accessed one at a time, that restriction is
>>> moderately common.
> 
>> That doesn't help, unfortunately. Thinking about it, that is not really
>> surprising. The failing write is to the page register, and that was
>> a single write anyway.
> 
> Oh, that's interesting - I'm kind of surprised the wire protocols differ
> but it's been a while since I looked.  We should probably add this to

spd5118 devices are ... weird. For example, they don't auto-increment
addresses on a read operation, meaning regmap_i2c_smbus_i2c_read_reg16()
doesn't work at all because i2c_smbus_read_byte() always reads from
address 0.

> the quirking in regmap-i2c.c, have it select one the 8 bit only smbus
> versions for devices that need register at a time operation.  I'd not be
> surprised if other devices have issues, and anyway if it makes a
> difference to the wire protocol we should try to select something as
> close as possible to what we're actually doing.
> 
> Something like the below perhaps (this probably needs to be converted to
> a match table type thing at this point, there's the SMBUS_WORD_DATA case
> too):
> 
> diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
> index a905e955bbfc..499fcec00f2d 100644
> --- a/drivers/base/regmap/regmap-i2c.c
> +++ b/drivers/base/regmap/regmap-i2c.c
> @@ -313,6 +313,11 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
>   
>   	if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
>   		bus = &regmap_i2c;
> +	else if (config->val_bits == 8 && config->reg_bits == 8 &&
> +		 config->use_single_read && config->use_single_write &&
> +		 i2c_check_functionality(i2c->adapter,
> +					 I2C_FUNC_SMBUS_BYTE_DATA))
> +		bus = &regmap_smbus_byte;

That might be an option. Give me more time, though - as it turns out,
I2C_FUNC_SMBUS_BYTE_DATA doesn't work for writes either on the affected
system. Something else is going on.

Thanks,
Guenter

Re: Page select register restrictions in regmap core

[Guenter Roeck]

On 6/18/24 13:46, Mark Brown wrote:
> On Tue, Jun 18, 2024 at 12:33:40PM -0700, Guenter Roeck wrote:
>> On 6/18/24 10:45, Mark Brown wrote:
>>> On Tue, Jun 18, 2024 at 09:14:56AM -0700, Guenter Roeck wrote:
> 
>>>> It turns out that at least some i801 controllers don't work with the
>>>> access mechanism used by regmap, or maybe the spd5118 chips don't support
>>>> I2C_FUNC_SMBUS_I2C_BLOCK. I already found that those chips don't support
>>>> auto-incrementing the register address and actually reset the address on byte
>>>> reads (i.e., subsequent calls to i2c_smbus_read_byte() always return the data
>>>> from the first register). Since regmap doesn't have a means for me to say
>>>> "don't use I2C_FUNC_SMBUS_I2C_BLOCK even if the controller supports it",
>>>> I may have to drop regmap support entirely anyway. That would be annoying,
>>>> but right now I have no idea how to work around that problem.
> 
>>> You can set the use_single_read and use_single_write flags in the config
>>> to ensure registers are accessed one at a time, that restriction is
>>> moderately common.
> 
>> That doesn't help, unfortunately. Thinking about it, that is not really
>> surprising. The failing write is to the page register, and that was
>> a single write anyway.
> 
> Oh, that's interesting - I'm kind of surprised the wire protocols differ
> but it's been a while since I looked.  We should probably add this to

Magic solved: As it turns out, recent i801 controllers have a write protect
bit which can be set by the BIOS and prevents writes in the 0x50..0x57 i2c
address range. Apparently this is what happened here. Side effect is that
it is impossible to read the spd5118 eeprom from the operating system
if the write protect bit is set since doing that requires a write to set
the page register.

Sorry for the noise.

Thanks,
Guenter