[PATCH v3 0/2] i2c: smbus cleanups and SPD support for piix4

[PATCH v3 0/2] i2c: smbus cleanups and SPD support for piix4

[Thomas Weißschuh]

Patches 1 is a preparation patch.
Patch 2 is the actual change to piix4.

Patch 1 drops the warning about muxed busses.
I didn't feel that the warning only would warrant the additional
complexity it introduces with multiple callers of i2c_register_spd().

If others feel different, maybe a more generic warning/info or a source
code comment would suffice.

On a machine with 32 slots of which 16 are populated only the first 8
slots are addressable, half of which are empty.
Unfortunately I couldn't run a custom kernel for testing.
But manually instantiating ee1004 devices worked as expected,
so the proposed changes should also work.

Tested with spd5118 and two DIMMs.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v3:
- Apply tags from Guenter
- Squash commits 1-3, and reword the message slightly
- Drop Fixes:
- Link to v2: https://lore.kernel.org/r/20240627-piix4-spd-v2-0-617ce47b8ff4@weissschuh.net

Changes in v2:
- Add new i2c-smbus prep patches
- Only register SPD for port 0
- Link to v1: https://lore.kernel.org/r/20240530-piix4-spd-v1-1-9cbf1abebf41@weissschuh.net

---
Thomas Weißschuh (2):
      i2c: smbus: remove i801 assumptions from SPD probing
      i2c: piix4: Register SPDs

 drivers/i2c/busses/Kconfig     |  1 +
 drivers/i2c/busses/i2c-piix4.c |  4 ++++
 drivers/i2c/i2c-smbus.c        | 15 ++++-----------
 3 files changed, 9 insertions(+), 11 deletions(-)
---
base-commit: 9746c2dd0307c80bd695e4e3065367f3e0154723
change-id: 20240530-piix4-spd-39c156b22959

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v3 1/2] i2c: smbus: remove i801 assumptions from SPD probing

[Thomas Weißschuh]

The check and warning are very specific to the SPD usage of the i801
driver. That was fine as long as i801 was the only caller of
i2c_register_spd(). Now that piix4 will be added as another user of that
function, the check and warning are not accurate anymore.
Instead of introducing a more complicated calling protocol only to print
a warning, drop the warning.
Even in cases where not all slots can be probed,
then at least probe the 8 slots that can be.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/i2c/i2c-smbus.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index f809f0ef2004..f0ac35fd0c5a 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -352,18 +352,11 @@ void i2c_register_spd(struct i2c_adapter *adap)
 		return;
 
 	/*
-	 * If we're a child adapter on a muxed segment, then limit slots to 8,
-	 * as this is the max number of SPD EEPROMs that can be addressed per bus.
+	 * The max number of SPD EEPROMs that can be addressed per bus is 8.
+	 * If more slots are present either muxed or multiple busses are
+	 * necessary or the additional slots are ignored.
 	 */
-	if (i2c_parent_is_i2c_adapter(adap)) {
-		slot_count = 8;
-	} else {
-		if (slot_count > 8) {
-			dev_warn(&adap->dev,
-				 "More than 8 memory slots on a single bus, contact i801 maintainer to add missing mux config\n");
-			return;
-		}
-	}
+	slot_count = min(slot_count, 8);
 
 	/*
 	 * Memory types could be found at section 7.18.2 (Memory Device — Type), table 78

-- 
2.45.2

[PATCH v3 2/2] i2c: piix4: Register SPDs

[Thomas Weißschuh]

The piix4 I2C bus can carry SPDs, register them if present.
Only look on bus 0, as this is where the SPDs seem to be located.

Only the first 8 slots are supported. If the system has more,
then these will not be visible.

The AUX bus can not be probed as on some platforms it reports all
devices present and all reads return "0".
This would allow the ee1004 to be probed incorrectly.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/i2c/busses/Kconfig     | 1 +
 drivers/i2c/busses/i2c-piix4.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index fe6e8a1bb607..ff66e883b348 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -195,6 +195,7 @@ config I2C_ISMT
 config I2C_PIIX4
 	tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
 	depends on PCI && HAS_IOPORT
+	select I2C_SMBUS
 	help
 	  If you say yes to this option, support will be included for the Intel
 	  PIIX4 family of mainboard I2C interfaces.  Specifically, the following
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 6a0392172b2f..14752d946f58 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -29,6 +29,7 @@
 #include <linux/stddef.h>
 #include <linux/ioport.h>
 #include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
 #include <linux/slab.h>
 #include <linux/dmi.h>
 #include <linux/acpi.h>
@@ -982,6 +983,9 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 		return retval;
 	}
 
+	if (port == 0)
+		i2c_register_spd(adap);
+
 	*padap = adap;
 	return 0;
 }

-- 
2.45.2

Re: [PATCH v3 1/2] i2c: smbus: remove i801 assumptions from SPD probing

[Andi Shyti]

On Tue, Jul 09, 2024 at 07:35:35PM GMT, Thomas Weißschuh wrote:
> The check and warning are very specific to the SPD usage of the i801
> driver. That was fine as long as i801 was the only caller of
> i2c_register_spd(). Now that piix4 will be added as another user of that
> function, the check and warning are not accurate anymore.
> Instead of introducing a more complicated calling protocol only to print
> a warning, drop the warning.

Well... it's not just a warning, it also returns.

> Even in cases where not all slots can be probed,
> then at least probe the 8 slots that can be.

I'm good with the change.

Jean, Heiner, any comment here? Do we want to add an extra check
for i801?

Andi

Re: [PATCH v3 0/2] i2c: smbus cleanups and SPD support for piix4

[Andi Shyti]

Hi Wolfram,

> Thomas Weißschuh (2):
>       i2c: smbus: remove i801 assumptions from SPD probing

this is yours...

>       i2c: piix4: Register SPDs

... this is mine :-)

Do you want me to pick them both up?

Thanks,
Andi

Re: [PATCH v3 0/2] i2c: smbus cleanups and SPD support for piix4

[Wolfram Sang]

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

On Fri, Jul 12, 2024 at 02:02:11AM +0200, Andi Shyti wrote:
> Hi Wolfram,
> 
> > Thomas Weißschuh (2):
> >       i2c: smbus: remove i801 assumptions from SPD probing
> 
> this is yours...
> 
> >       i2c: piix4: Register SPDs
> 
> ... this is mine :-)
> 
> Do you want me to pick them both up?

I'd  really like to have feedback here from Heiner and/or Jean before we
apply this...


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

Re: [PATCH v3 1/2] i2c: smbus: remove i801 assumptions from SPD probing

[Heiner Kallweit]

On 09.07.2024 19:35, Thomas Weißschuh wrote:
> The check and warning are very specific to the SPD usage of the i801
> driver. That was fine as long as i801 was the only caller of
> i2c_register_spd(). Now that piix4 will be added as another user of that
> function, the check and warning are not accurate anymore.
> Instead of introducing a more complicated calling protocol only to print
> a warning, drop the warning.
> Even in cases where not all slots can be probed,
> then at least probe the 8 slots that can be.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/i2c/i2c-smbus.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 

Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>

Re: [PATCH v3 2/2] i2c: piix4: Register SPDs

[Wolfram Sang]

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


> Only the first 8 slots are supported. If the system has more,
> then these will not be visible.
> 
> The AUX bus can not be probed as on some platforms it reports all
> devices present and all reads return "0".
> This would allow the ee1004 to be probed incorrectly.

I think this information would also be helpful as a comment above the
code. But to allow this series to be applied now, I think an incremental
patch will do. With Heiner's ack, I think this can go in now.


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

Re: [PATCH v3 2/2] i2c: piix4: Register SPDs

[Andi Shyti]

Hi Thomas,

On Fri, Jul 12, 2024 at 02:53:49PM GMT, Wolfram Sang wrote:
> > Only the first 8 slots are supported. If the system has more,
> > then these will not be visible.
> > 
> > The AUX bus can not be probed as on some platforms it reports all
> > devices present and all reads return "0".
> > This would allow the ee1004 to be probed incorrectly.
> 
> I think this information would also be helpful as a comment above the
> code. But to allow this series to be applied now, I think an incremental
> patch will do. With Heiner's ack, I think this can go in now.

I agree with Wolfram here. Are you up for a v4 or do you want me
to add the comment while pushing?

Andi

Re: [PATCH v3 2/2] i2c: piix4: Register SPDs

[Thomas Weißschuh]

Hi Andi,

On 2024-07-16 19:46:43+0000, Andi Shyti wrote:
> On Fri, Jul 12, 2024 at 02:53:49PM GMT, Wolfram Sang wrote:
> > > Only the first 8 slots are supported. If the system has more,
> > > then these will not be visible.
> > > 
> > > The AUX bus can not be probed as on some platforms it reports all
> > > devices present and all reads return "0".
> > > This would allow the ee1004 to be probed incorrectly.
> > 
> > I think this information would also be helpful as a comment above the
> > code. But to allow this series to be applied now, I think an incremental
> > patch will do. With Heiner's ack, I think this can go in now.
> 
> I agree with Wolfram here. Are you up for a v4 or do you want me
> to add the comment while pushing?

I also agree. My first interpretation of that message was that I would
send the incremental patch during the 6.12 cycle.
But if it's still fine for 6.11, even better.
If you could add the comment, that would be great,
but I'm also fine with resending.


Thomas

Re: [PATCH v3 2/2] i2c: piix4: Register SPDs

[Andi Shyti]

On Tue, Jul 16, 2024 at 07:50:04PM GMT, Thomas Weißschuh wrote:
> On 2024-07-16 19:46:43+0000, Andi Shyti wrote:
> > On Fri, Jul 12, 2024 at 02:53:49PM GMT, Wolfram Sang wrote:
> > > > Only the first 8 slots are supported. If the system has more,
> > > > then these will not be visible.
> > > > 
> > > > The AUX bus can not be probed as on some platforms it reports all
> > > > devices present and all reads return "0".
> > > > This would allow the ee1004 to be probed incorrectly.
> > > 
> > > I think this information would also be helpful as a comment above the
> > > code. But to allow this series to be applied now, I think an incremental
> > > patch will do. With Heiner's ack, I think this can go in now.
> > 
> > I agree with Wolfram here. Are you up for a v4 or do you want me
> > to add the comment while pushing?
> 
> I also agree. My first interpretation of that message was that I would
> send the incremental patch during the 6.12 cycle.
> But if it's still fine for 6.11, even better.
> If you could add the comment, that would be great,
> but I'm also fine with resending.

That's why I'm pushing... Wolfram is allowing me a pull request
part 2 and I wanted this to go in.

I will fix the comment then.

Thanks,
Andi

Re: [PATCH v3 0/2] i2c: smbus cleanups and SPD support for piix4

[Andi Shyti]

Hi Thomas,

On Tue, Jul 09, 2024 at 07:35:34PM GMT, Thomas Weißschuh wrote:
> Patches 1 is a preparation patch.
> Patch 2 is the actual change to piix4.
> 
> Patch 1 drops the warning about muxed busses.
> I didn't feel that the warning only would warrant the additional
> complexity it introduces with multiple callers of i2c_register_spd().
> 
> If others feel different, maybe a more generic warning/info or a source
> code comment would suffice.
> 
> On a machine with 32 slots of which 16 are populated only the first 8
> slots are addressable, half of which are empty.
> Unfortunately I couldn't run a custom kernel for testing.
> But manually instantiating ee1004 devices worked as expected,
> so the proposed changes should also work.
> 
> Tested with spd5118 and two DIMMs.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

with the comment added, pushed to

i2c/i2c-host.

Thanks,
Andi