mbox series

[0/4] i2c: Support i2c_register_spd() on multiplexed bus segments

Message ID fc057deb-49f9-49cf-9549-13b2538ed92b@gmail.com
Headers show
Series i2c: Support i2c_register_spd() on multiplexed bus segments | expand

Message

Heiner Kallweit Jan. 10, 2024, 8:13 p.m. UTC
i801 is the last bus driver supporting I2C_CLASS_SPD. It's used for
device probing on muxed bus segments. Only known use case so far is
systems with more than 8 memory modules (with thermal sensor) on
muxed smbus segments.
As discussed with Jean, to be able to remove I2C_CLASS_SPD completely
the following has to be done:

1. Extend i2c_register_spd() for use on muxed bus segments
2. Enable explicit instantiation of thermal sensors on memory modules
3. Extend i801 to call i2c_register_spd() on muxed bus segments

Step 2 has been accomplished:
caba40ec3531 ("eeprom: at24: Probe for DDR3 thermal sensor in the SPD case")
393bd1000f81 ("eeprom: ee1004: add support for temperature sensor")

Patch 1 does step 1
Patches 2 and 3 provide the basis for patch 4
Patch 4 does step 3

Note: i801 creates the mux platform device, loading and probing of the
mux driver may be asynchronous. Therefore we can't call i2c_register_spd()
for the muxed segments from i801. Instead we have to add a flag to the
platform data, so that the mux driver knows it's supposed to call
i2c_register_spd().

This series replaces the earlier RFC series.

Heiner Kallweit (4):
  i2c: smbus: Prepare i2c_register_spd for usage on muxed segments
  i2c: mux: add basic support for calling i2c_register_spd on muxed bus
    segments
  i2c: mux: gpio: Allow to call i2c_register_spd on a muxed segment
  i2c: i801: Call i2c_register_spd() on muxed bus segments

 drivers/i2c/busses/i2c-i801.c              |  1 +
 drivers/i2c/i2c-mux.c                      |  4 ++++
 drivers/i2c/i2c-smbus.c                    | 19 ++++++++++++-------
 drivers/i2c/muxes/i2c-mux-gpio.c           |  1 +
 include/linux/i2c-mux.h                    |  1 +
 include/linux/platform_data/i2c-mux-gpio.h |  2 ++
 6 files changed, 21 insertions(+), 7 deletions(-)

Comments

Peter Rosin Jan. 10, 2024, 11 p.m. UTC | #1
Hi!

2024-01-10 at 21:13, Heiner Kallweit wrote:
> If this is an adapter on a muxed bus segment, assume that each segment
> is connected to a subset of the (> 8) overall memory slots. In this
> case let's probe the maximum of 8 slots, however stop if the number
> of overall populated slots is reached.
> 
> If we're not on a muxed segment and the total number of slots is > 8,
> report an error, because then not all SPD eeproms can be addressed.
> Presumably the bus is muxed, but the mux config is missing.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/i2c-smbus.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 74807c6db..e94714d5a 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -351,13 +351,18 @@ void i2c_register_spd(struct i2c_adapter *adap)
>  	if (!dimm_count)
>  		return;
>  
> -	dev_info(&adap->dev, "%d/%d memory slots populated (from DMI)\n",
> -		 dimm_count, slot_count);
> -
> -	if (slot_count > 8) {
> -		dev_warn(&adap->dev,
> -			 "Systems with more than 8 memory slots not supported yet, not instantiating SPD\n");
> -		return;
> +	/* Check whether we're a child adapter on a muxed segment */
> +	if (i2c_parent_is_i2c_adapter(adap)) {
> +		if (slot_count > 8)
> +			slot_count = 8;'

The comment "Only works on systems with 1 to 8 memory slots" above
i2c_register_spd() is now incorrect and needs adjusting.

> +	} else {
> +		dev_info(&adap->dev, "%d/%d memory slots populated (from DMI)\n",
> +			 dimm_count, slot_count);
> +		if (slot_count > 8) {
> +			dev_err(&adap->dev,
> +				"More than 8 memory slots on a single bus, mux config missing?\n");
> +			return;
> +		}
>  	}
>  
>  	/*

The loop below this hunk is limited by dimm_count, but it is checked
separately for each muxed segment. It is therefore now possible to
instantiate a total number of slots larger than the dimm_count.
That was not possible before. I don't know if that's a problem, but
the check have been (silently) relaxed.

Cheers,
Peter
Heiner Kallweit Jan. 11, 2024, 6:49 a.m. UTC | #2
On 11.01.2024 00:00, Peter Rosin wrote:
> Hi!
> 
> 2024-01-10 at 21:13, Heiner Kallweit wrote:
>> If this is an adapter on a muxed bus segment, assume that each segment
>> is connected to a subset of the (> 8) overall memory slots. In this
>> case let's probe the maximum of 8 slots, however stop if the number
>> of overall populated slots is reached.
>>
>> If we're not on a muxed segment and the total number of slots is > 8,
>> report an error, because then not all SPD eeproms can be addressed.
>> Presumably the bus is muxed, but the mux config is missing.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/i2c-smbus.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
>> index 74807c6db..e94714d5a 100644
>> --- a/drivers/i2c/i2c-smbus.c
>> +++ b/drivers/i2c/i2c-smbus.c
>> @@ -351,13 +351,18 @@ void i2c_register_spd(struct i2c_adapter *adap)
>>  	if (!dimm_count)
>>  		return;
>>  
>> -	dev_info(&adap->dev, "%d/%d memory slots populated (from DMI)\n",
>> -		 dimm_count, slot_count);
>> -
>> -	if (slot_count > 8) {
>> -		dev_warn(&adap->dev,
>> -			 "Systems with more than 8 memory slots not supported yet, not instantiating SPD\n");
>> -		return;
>> +	/* Check whether we're a child adapter on a muxed segment */
>> +	if (i2c_parent_is_i2c_adapter(adap)) {
>> +		if (slot_count > 8)
>> +			slot_count = 8;'
> 
> The comment "Only works on systems with 1 to 8 memory slots" above
> i2c_register_spd() is now incorrect and needs adjusting.
> 
Right, this comment can be removed.
I'll wait for more feedback on the series before submitting a v2.

>> +	} else {
>> +		dev_info(&adap->dev, "%d/%d memory slots populated (from DMI)\n",
>> +			 dimm_count, slot_count);
>> +		if (slot_count > 8) {
>> +			dev_err(&adap->dev,
>> +				"More than 8 memory slots on a single bus, mux config missing?\n");
>> +			return;
>> +		}
>>  	}
>>  
>>  	/*
> 
> The loop below this hunk is limited by dimm_count, but it is checked
> separately for each muxed segment. It is therefore now possible to
> instantiate a total number of slots larger than the dimm_count.
> That was not possible before. I don't know if that's a problem, but
> the check have been (silently) relaxed.
> 
That's intentional. Keeping the current logic would have required to
add some kind of counter at the parent level, keeping track of how many
memory modules were instantiated per muxed segment.
For the sake of simplicity this was omitted. Instead we may probe more
slots than needed, however only impact should be that i2c_register_spd()
may take a little bit longer on affected systems.

> Cheers,
> Peter

Heiner