Re: [PATCH 1/3] hwmon: spd5118: Do not fail resume on temporary I2C errors

[Armin Wolf <W_Armin@gmx.de>]

Am 13.01.26 um 20:16 schrieb TINSAE TADESSE:

> On Mon, Jan 12, 2026 at 9:22 PM Armin Wolf <W_Armin@gmx.de> wrote:
>> Am 12.01.26 um 19:17 schrieb Guenter Roeck:
>>
>>> On 1/12/26 09:46, Armin Wolf wrote:
>>>> Am 12.01.26 um 17:36 schrieb Guenter Roeck:
>>>>
>>>>> On 1/10/26 14:27, Armin Wolf wrote:
>>>>>> Am 10.01.26 um 18:19 schrieb Tinsae Tadesse:
>>>>>>
>>>>>>> SPD5118 DDR5 temperature sensors may be temporarily unavailable
>>>>>>> during s2idle resume. Ignore temporary -ENXIO and -EIO errors
>>>>>>> during resume and allow
>>>>>>> register synchronization to be retried later.
>>>>>> Hi,
>>>>>>
>>>>>> do you know if the error is caused by the SPD5118 device itself or
>>>>>> by the underlying
>>>>>> i2c controller? Please also share the output of "acpidump" and the
>>>>>> name of the i2c
>>>>>> controller used to communicate with the SPD5118.
>>>>>>
>>>>>>> Signed-off-by: Tinsae Tadesse <tinsaetadesse2015@gmail.com>
>>>>>>> ---
>>>>>>>    drivers/hwmon/spd5118.c | 8 +++++++-
>>>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
>>>>>>> index 5da44571b6a0..ec9f14f6e0df 100644
>>>>>>> --- a/drivers/hwmon/spd5118.c
>>>>>>> +++ b/drivers/hwmon/spd5118.c
>>>>>>> @@ -512,9 +512,15 @@ static int spd5118_resume(struct device *dev)
>>>>>>>    {
>>>>>>>        struct spd5118_data *data = dev_get_drvdata(dev);
>>>>>>>        struct regmap *regmap = data->regmap;
>>>>>>> +    int ret;
>>>>>>>        regcache_cache_only(regmap, false);
>>>>>>> -    return regcache_sync(regmap);
>>>>>>> +    ret = regcache_sync(regmap);
>>>>>>> +    if(ret == -ENXIO || ret == -EIO) {
>>>>>>> +        dev_warn(dev, "SPD hub not responding on resume (%d),
>>>>>>> deferring init\n", ret);
>>>>>>> +        return 0;
>>>>>>> +    }
>>>>>> The specification says that the SPD5118 might take up to 10ms to
>>>>>> initialize its i2c interface
>>>>>> after power on. Can you test if simply waiting for 10ms before
>>>>>> syncing the regcache solves this
>>>>>> issue?
>>>>>>
>>>>> It seems to be highly unlikely that this code executes within 10ms
>>>>> of powering on the memory.
>>>>>
>>>>> Guenter
>>>>>
>>>> AFAIK the 10ms are associated with the VDDIO supply, the VDDSPD main
>>>> supply is different from that.
>>>> I just want to test if this device disables VDDIO during
>>>> suspend-to-idle.
>>>>
>>>> I have another theory: if the SPD5118 somehow looses power, we might
>>>> still need to manually put the
>>>> device into 16-bit address mode using standard 8-bit i2c commands.
>>>>
>>> Uh, no, we can not do that. I tried. Changing the access mode causes
>>> bad hiccups at least
>>> with some boards. They interpret that as a memory configuration
>>> change, and the next warm
>>> reboot will end up in the BIOS. Then, after the RAM configuration is
>>> updated, a cold reboot
>>> will again detect a configuration change and BIOS will be entered again.
>>>
>>> That does make me wonder how the problem shows up in the first place,
>>> since the BIOS
>>> usually does access the SPD5118 during resume, at least on my systems
>>> with DDR5. Granted,
>>> those are with AMD CPUs, but I would assume that Intel BIOS versions
>>> are not different.
>>>
>>> Guenter
>>>
>> During suspend-to-idle the RAM stays active, so the firmware does not really need to access the SPD device.
>> I meant that if the SPD device is configured during boot to operate in 16-bit mode and looses power during
>> suspend-to-idle, the firmware might not reconfigure the SPD to continue operate in 16-bit mode after resume.
>>
>> Thanks,
>> Armin Wolf
>>
> Hi Armin,
>
> I tested whether firmware reinitializes the SPD5118 by comparing key
> registers across cold boot and s2idle resume.
> Register values remained unchanged across resume cycles, suggesting
> firmware does not reconfigure the device during resume.
> To avoid introducing platform-specific regressions, no attempt was
> made to restore SPD5118_REG_I2C_LEGACY_MODE.
>
Alright, thanks for checking. In this case the error indeed seems to come from the i2c controller

Thanks,
Armin Wolf