| Message ID | db378364-018e-4e6b-8e41-8cdd21ce2afd@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/2] i2c: smbus: Prepare i2c_register_spd for usage on muxed segments | expand |
Hi Heiner, > + /* Check whether we're a child adapter on a muxed segment */ The comment describes the 'if' but not 'then'. How about sth like "If we are a child on a muxed segment then limit slots to..."? > + if (i2c_parent_is_i2c_adapter(adap)) { > + slot_count = 8; I don't know much about DMI. I just noticed that there are no printouts in this code path. Will there be one for the parent? > + } else { > + if (slot_count > 8) { > + dev_err(&adap->dev, > + "More than 8 memory slots on a single bus, mux config missing?\n"); With this error message, I as a user would think I need to setup a mux config somewhere. But it is missing from DMI, or? Then, we should probably use even FW_BUG in the message? Happy hacking, Wolfram
On 26.02.2024 11:32, Wolfram Sang wrote: > Hi Heiner, > >> + /* Check whether we're a child adapter on a muxed segment */ > > The comment describes the 'if' but not 'then'. How about sth like "If we > are a child on a muxed segment then limit slots to..."? > OK, this would be better. >> + if (i2c_parent_is_i2c_adapter(adap)) { >> + slot_count = 8; > > I don't know much about DMI. I just noticed that there are no printouts > in this code path. Will there be one for the parent? > With the patch as-is there's we omit printout for systems with > 8 memory slots. I'm not aware of any way to find out how many memory slots belong to a specific child bus segment. So all we could do is print per child segment how many slots are populated. But we have a printout per populated slot already: "Successfully instantiated SPD at 0x%hx\n" So IMO we don't loose any relevant info. >> + } else { >> + if (slot_count > 8) { >> + dev_err(&adap->dev, >> + "More than 8 memory slots on a single bus, mux config missing?\n"); > > With this error message, I as a user would think I need to setup a mux > config somewhere. But it is missing from DMI, or? Then, we should > probably use even FW_BUG in the message? > Actually a developer has to add the config to i801's mux_dmi_table[]. So yes, we should change the message to something like: "More than 8 memory slots on a single bus, contact i801 maintainer to add the missing mux configuration" > Happy hacking, > > Wolfram > Heiner
> how many slots are populated. But we have a printout per populated slot > already: "Successfully instantiated SPD at 0x%hx\n" > So IMO we don't loose any relevant info. Right. That makes me think we could even remove the dev_info() entirely. But I leave this up to you. > Actually a developer has to add the config to i801's mux_dmi_table[]. > So yes, we should change the message to something like: > "More than 8 memory slots on a single bus, contact i801 maintainer to > add the missing mux configuration" Sounds good!
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c index 74807c6db..ad7ea0215 100644 --- a/drivers/i2c/i2c-smbus.c +++ b/drivers/i2c/i2c-smbus.c @@ -351,13 +351,17 @@ 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)) { + slot_count = 8; + } else { + if (slot_count > 8) { + dev_err(&adap->dev, + "More than 8 memory slots on a single bus, mux config missing?\n"); + return; + } + dev_info(&adap->dev, "%d/%d memory slots populated (from DMI)\n", + dimm_count, slot_count); } /*
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 | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)