[PATCH RFC 0/2] i2c: i801: Call i2c_register_spd for muxed child segments

[PATCH RFC 0/2] i2c: i801: Call i2c_register_spd for muxed child segments

[Heiner Kallweit]

Once the gpio mux driver binds to the "i2c-mux-gpio" platform device,
this creates the i2c adapters for the muxed child segments.
We can use the bus notifier mechanism to check for creation of the
child i2d adapters, and call i2c_register_spd() for them. This allows
to detect all DIMM's on systems with more than 8 memory slots.

I tested that the events are properly recognized. However I don't have
hw with a muxed SMBUS, so I can't test the actual functionality.

Heiner Kallweit (2):
  i2c: smbus: Prepare i2c_register_spd for usage on muxed segments
  i2c: i801: Call i2c_register_spd for muxed child segments

 drivers/i2c/busses/i2c-i801.c | 23 +++++++++++++++++++++++
 drivers/i2c/i2c-smbus.c       | 18 +++++++++++-------
 2 files changed, 34 insertions(+), 7 deletions(-)

-- 
2.43.2

[PATCH RFC 1/2] i2c: smbus: Prepare i2c_register_spd for usage on muxed segments

[Heiner Kallweit]

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(-)

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);
 	}
 
 	/*
-- 
2.43.2

[PATCH RFC 2/2] i2c: i801: Call i2c_register_spd for muxed child segments

[Heiner Kallweit]

Once the gpio mux driver binds to the "i2c-mux-gpio" platform device,
this creates the i2c adapters for the muxed child segments.
We can use the bus notifier mechanism to check for creation of the
child i2d adapters, and call i2c_register_spd() for them. This allows
to detect all DIMM's on systems with more than 8 memory slots.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 8f225cd7b..4b9d04f20 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -105,6 +105,7 @@
 #include <linux/ioport.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
+#include <linux/i2c-mux.h>
 #include <linux/i2c-smbus.h>
 #include <linux/acpi.h>
 #include <linux/io.h>
@@ -291,6 +292,7 @@ struct i801_priv {
 #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI
 	struct platform_device *mux_pdev;
 	struct gpiod_lookup_table *lookup;
+	struct notifier_block mux_notifier_block;
 #endif
 	struct platform_device *tco_pdev;
 
@@ -1388,6 +1390,23 @@ static const struct dmi_system_id mux_dmi_table[] = {
 	{ }
 };
 
+static int i801_notifier_call(struct notifier_block *nb, unsigned long action,
+			      void *data)
+{
+	struct i801_priv *priv = container_of(nb, struct i801_priv, mux_notifier_block);
+	struct device *dev = data;
+
+	if (action != BUS_NOTIFY_ADD_DEVICE ||
+	    dev->type != &i2c_adapter_type ||
+	    i2c_root_adapter(dev) != &priv->adapter)
+		return NOTIFY_DONE;
+
+	/* Call i2c_register_spd for muxed child segments */
+	i2c_register_spd(to_i2c_adapter(dev));
+
+	return NOTIFY_OK;
+}
+
 /* Setup multiplexing if needed */
 static void i801_add_mux(struct i801_priv *priv)
 {
@@ -1425,6 +1444,9 @@ static void i801_add_mux(struct i801_priv *priv)
 	gpiod_add_lookup_table(lookup);
 	priv->lookup = lookup;
 
+	priv->mux_notifier_block.notifier_call = i801_notifier_call;
+	if (bus_register_notifier(&i2c_bus_type, &priv->mux_notifier_block))
+		return;
 	/*
 	 * Register the mux device, we use PLATFORM_DEVID_NONE here
 	 * because since we are referring to the GPIO chip by name we are
@@ -1443,6 +1465,7 @@ static void i801_add_mux(struct i801_priv *priv)
 
 static void i801_del_mux(struct i801_priv *priv)
 {
+	bus_unregister_notifier(&i2c_bus_type, &priv->mux_notifier_block);
 	platform_device_unregister(priv->mux_pdev);
 	gpiod_remove_lookup_table(priv->lookup);
 }
-- 
2.43.2

Re: [PATCH RFC 1/2] i2c: smbus: Prepare i2c_register_spd for usage on muxed segments

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 2/2] i2c: i801: Call i2c_register_spd for muxed child segments

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On Thu, Feb 22, 2024 at 11:25:43PM +0100, Heiner Kallweit wrote:
> Once the gpio mux driver binds to the "i2c-mux-gpio" platform device,
> this creates the i2c adapters for the muxed child segments.
> We can use the bus notifier mechanism to check for creation of the
> child i2d adapters, and call i2c_register_spd() for them. This allows
> to detect all DIMM's on systems with more than 8 memory slots.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Yay, this looks *much* better. Thanks for doing it! Sadly, I can also
only review, not test. Let's hope someone can step up to do the real
testing. Maybe resend it as RFT to lkml?

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 1/2] i2c: smbus: Prepare i2c_register_spd for usage on muxed segments

[Heiner Kallweit]

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

Re: [PATCH RFC 2/2] i2c: i801: Call i2c_register_spd for muxed child segments

[Heiner Kallweit]

On 26.02.2024 11:36, Wolfram Sang wrote:
> On Thu, Feb 22, 2024 at 11:25:43PM +0100, Heiner Kallweit wrote:
>> Once the gpio mux driver binds to the "i2c-mux-gpio" platform device,
>> this creates the i2c adapters for the muxed child segments.
>> We can use the bus notifier mechanism to check for creation of the
>> child i2d adapters, and call i2c_register_spd() for them. This allows
>> to detect all DIMM's on systems with more than 8 memory slots.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Yay, this looks *much* better. Thanks for doing it! Sadly, I can also
> only review, not test. Let's hope someone can step up to do the real
> testing. Maybe resend it as RFT to lkml?
> 
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
Ah yes, I'll do this.

Re: [PATCH RFC 1/2] i2c: smbus: Prepare i2c_register_spd for usage on muxed segments

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]


> 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!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 2/2] i2c: i801: Call i2c_register_spd for muxed child segments

[Heiner Kallweit]

On 26.02.2024 11:36, Wolfram Sang wrote:
> On Thu, Feb 22, 2024 at 11:25:43PM +0100, Heiner Kallweit wrote:
>> Once the gpio mux driver binds to the "i2c-mux-gpio" platform device,
>> this creates the i2c adapters for the muxed child segments.
>> We can use the bus notifier mechanism to check for creation of the
>> child i2d adapters, and call i2c_register_spd() for them. This allows
>> to detect all DIMM's on systems with more than 8 memory slots.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Yay, this looks *much* better. Thanks for doing it! Sadly, I can also
> only review, not test. Let's hope someone can step up to do the real
> testing. Maybe resend it as RFT to lkml?
> 
3 weeks ago I sent the change as RFT to lkml, as suggested.
However I didn't receive a single response yet.
Now that 6.9-rc1 is out, we would have several weeks in linux-next
before 6.10-rc1. Would it be ok for you to apply the patch and see
whether we get any feedback from linux-next users?
If yes, can you apply it as-is, or shall I resubmit the patch?

> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 

Re: [PATCH RFC 2/2] i2c: i801: Call i2c_register_spd for muxed child segments

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]


> Now that 6.9-rc1 is out, we would have several weeks in linux-next
> before 6.10-rc1. Would it be ok for you to apply the patch and see
> whether we get any feedback from linux-next users?

Yes, we can do that.

> If yes, can you apply it as-is, or shall I resubmit the patch?

This is a question for Andi who picks up driver patches meanwhile.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 2/2] i2c: i801: Call i2c_register_spd for muxed child segments

[Heiner Kallweit]

On 25.03.2024 18:36, Wolfram Sang wrote:
> 
>> Now that 6.9-rc1 is out, we would have several weeks in linux-next
>> before 6.10-rc1. Would it be ok for you to apply the patch and see
>> whether we get any feedback from linux-next users?
> 
> Yes, we can do that.
> 
>> If yes, can you apply it as-is, or shall I resubmit the patch?
> 
> This is a question for Andi who picks up driver patches meanwhile.
> 
I have to rebase the patch anyway, so I'll resubmit.