Message-ID: <0287c32d-0b63-4873-9f71-0094a912f8bb@roeck-us.net>
Date: Tue, 18 Jun 2024 09:14:56 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Mark Brown <broonie@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 linux-kernel@...r.kernel.org, Armin Wolf <W_Armin@....de>
Subject: Re: Page select register restrictions in regmap core

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

Powered by blists - more mailing lists