Re: [PATCH RFC] i2c: i801: Add i801_register_jc42, similar to i2c_register_spd

Jean Delvare
2023-11-15T18:27:28Z

On Tue, 14 Nov 2023 15:44:36 +0100, Heiner Kallweit wrote:
> On 14.11.2023 15:00, Jean Delvare wrote:
> > On Wed, 08 Nov 2023 11:28:45 +0100, Heiner Kallweit wrote:
> >> From a maintenance point of view the easiest solution would be:
> >> - set flag I2C_CLASS_DEPRECATED in addition to I2C_CLASS_SPD
> >> to encourage potential users to switch to explicit instantiating
> >
> > Bad idea. That's just going to spam a warning message on millions of
> > systems while there's just nothing most users can do about it. That's
> > not helpful, we are already aware of the problem, and we are the guys
> > looking into it.
>
> I'm afraid I wasn't precise enough when writing this. What I meant is
> adding I2C_CLASS_DEPRECATED for the mux'ed child segments in i801.
> So it should affect users of the few Asus systems only.

Oh right, should have been obvious actually. Nevertheless, my point
still stands, the purpose of I2C_CLASS_DEPRECATED is to draw the
attention of the developers, and in this case, the developers would be
us, so we are already aware.

> i2c_register_spd() isn't used there, so I'd assume these users don't
> miss the temp sensors on their RAM modules.

i2c_register_spd() only deals with SPD EEPROMs. The fact that it isn't
called for mux'd Asus boards only means that the users have to manually
declare the at24 devices from user-space (which is a regression from
earlier when just loading the eeprom driver would automatically create
the devices).

Users of these boards can still load the jc42 driver and the thermal
sensors on the memory modules will be supported, as long as
I2C_CLASS_SPD exists and is used by both i2c-i801 (for the mux'd
segments) and the jc42 driver.

> > (...)
> > I think the steps to follow are:
> > * Extend i2c_register_spd() to support up to 8 memory modules (I'm
> > already working on that, patch is coming).
> > * Call i2c_register_spd() on the mux'd SMBus segments on the Asus
> > boards.
> > * Extend i2c_register_spd() to also instantiate jc42 devices in
> > addition to at24 (or ee1004) devices. I think this is better than
> > writing a separate function as I initially suggested. The reason why
> > I think so is because the SPD EEPROM does contain the information
> > about thermal sensor presence. So the code which instantiates the at24
> > or ee1004 device could also read from it to figure out whether a jc42
> > device must be instantiated. This removes the need for probing.
>
> I miss some insight here on which type of memory modules we can expect
> jc42-4 compatible temp sensors. I saw DDR3 mentioned (including LPDDR3?),

The LP prefix is for low-power. For most practical purposes, on the
software side, you can consider these modules as equivalent to their
standard version, and treat them alike.

> not sure about DDR4. In case of DDR4 we would have to read the EE1004
> data structure to check the "temp sensor present" bit. So I wonder
> whether instantiating the temp sensor should be in ee1004 driver.

I wasn't sure so I looked up the technical literature. The Microchip
MCP98244 datasheet mentions use on a DDR4 memory module, and the NXP
SE97B and SE98A datasheet mentions use on DDR2 and DDR3 modules.

--
Jean Delvare
SUSE L3 Support