Date: Wed, 19 Jun 2024 07:18:02 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Paul Menzel <pmenzel@...gen.mpg.de>, Armin Wolf <W_Armin@....de>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 linux-kernel@...r.kernel.org, René Rebe
 <rene@...ctcode.de>, Stephen Horvath <s.horvath@...look.com.au>,
 Sasha Kozachuk <skozachuk@...gle.com>, John Hamrick <johnham@...gle.com>,
 Chris Sarra <chrissarra@...gle.com>, linux-hwmon@...r.kernel.org,
 Jean Delvare <jdelvare@...e.com>, Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific
 read/write operations

On 6/19/24 02:13, Thomas Weißschuh wrote:
> On 2024-06-18 18:02:51+0000, Guenter Roeck wrote:
>> On 6/18/24 17:50, Thomas Weißschuh wrote:
>>> On 2024-06-18 17:23:44+0000, Guenter Roeck wrote:
>>>> On 6/18/24 16:39, Paul Menzel wrote:
>>>>> [Cc: +Heiner]
>>>>>
>>>>>
>>>>> Dear Armin,
>>>>>
>>>>>
>>>>> Am 19.06.24 um 01:28 schrieb Armin Wolf:
>>>>>> Am 19.06.24 um 00:28 schrieb Wolfram Sang:
>>>>>>
>>>>>>>> to 86 degrees C. If that doesn't work, we'll be really out of luck
>>>>>>>> with that controller (or at least I don't have an idea what else to try).
>>>>>>>
>>>>>>> Try CCing Heiner Kallweit for ideas about the i801 controller.
>>>>>
>>>>>> i am not Heiner Kallweit, but i found something interesting in
>>>>>> commit ba9ad2af7019 ("i2c: i801: Fix I2C Block Read on 8-Series/C220 and later").
>>>>>>
>>>>>> Basically, it seems that the i802 i2c controller indeed features a SPD write disable bit which blocks all writes for slave addresses 0x50-0x57.
>>>>>>
>>>>>> Does the i801 i2c controller driver print something like "SPD Write Disable is set" during boot?
>>>>>
>>>>> Nice find. Yes, it does:
>>>>>
>>>>
>>>> Yes, definitely. I didn't have any recent datasheets, so I missed that flag.
>>>> Oh well :-(.
>>>>
>>>>>        [    5.462605] i801_smbus 0000:00:1f.4: SPD Write Disable is set
>>>>>        [    5.468399] i801_smbus 0000:00:1f.4: SMBus using PCI interrupt
>>>>>
>>>>
>>>> Bummer. That explains the problem. It means that the BIOS effectively
>>>> blocks reading the eeprom on your system (because that would require writing
>>>> the page register), as well as changing temperature limits. That is really
>>>> annoying, but there is nothing we can do about it. Maybe the BIOS has a
>>>> configuration flag to enable or disable write protect, but I doubt it.
>>>
>>> What about using 16bit addressing mode?
>>>
>>>       Alternatively, at initial power on, the host can set the Table 112, “MR11” [3] = ‘1’ to address the entire 1024 bytes of
>>>       non-volatile memory with 2 bytes of address and hence not required to go through page selection to address entire
>>>       non-volatile memory.
>>>
>>> regmap-i2c allows 16bit addresses when I2C_FUNC_SMBUS_I2C_BLOCK is supported,
>>> which to me looks like it should be the case on i801 for ICH5.
>>>
>>
>> Good idea, but it doesn't work. I can get write operations with
>> 16-bit register addresses to work even on piix4, but read operations
>> require writing a 16-bit register address followed by byte reads (see
>> regmap_i2c_smbus_i2c_read_reg16). Unfortunately, spd5118 devices
>> don't auto-increment the address on byte read operations, meaning
>> each byte read returns data from address 0x00 (i.e., it returns
>> 0x51). Try "i2cdump -y -f 0 0x50 c" and you'll see what I mean.
>> Maybe there is a way around it, but I have not found it.
> 
> Thanks for the pointer to regmap_i2c_smbus_i2c_read_reg16().
> I'm not really familiar with I2C/SMBUS ...
> 
> Did you look into "2.6.8.3 Default Read Address Pointer Mode"?
> 
Yes, I did, and, yes, setting that for each planned read operation
might do the trick, but even that would not work here since writes
are blocked. On top of that, it would be extremely expensive (one
would have to write the address into the default address registers
before starting a read). The reason to use 16-bit access mode would
be to simplify access, not to make it more expensive.

> I am failing to understand how that address pointer mode would ever make
> sense without address auto-increment.

Oh, it kind of does, as long as each access is a single i2c operation.
One would have to use a SMBus read word operation to read two bytes
with a single SMBus command. I guess one could also use something similar
to an i2c block operation - start a read with no command byte and just
keep going.

> 
>> On top of that, configuring 16-bit mode requires a write operation
>> into the page register, and that is blocked.
> 
> ... this one on the other hand is really obvious.
> 

There is actually another problem: When I tried enabling 16-bit mode
in my system, I initially had trouble clearing it. When I rebooted,
I got a BIOS error telling me that the configuration changed, and
it gave me the option to either enter setup or continue. A soft
reset did not clear the bit. Power cycle did, but I got the
"configuration changed" message again.

So even if we would get 16-bit mode to work, it would not be a good idea
because it would expose people to the "configuration changed" BIOS
message. Resetting the bit on shutdown and when unloading the driver
would not help because that would not happen when the system crashes.

So, in summary, 16-bit mode is just not usable. If some BIOS actually
enables it, we might have to disable it or figure out how to use it
without depending on "Default Read Address Pointer Mode".

Thanks,
Guenter

Powered by blists - more mailing lists