[PATCH] i2c: Detect secondary SMBus controller on AMD AM4 chipsets

[PATCH] i2c: Detect secondary SMBus controller on AMD AM4 chipsets

[Adam Honse]

The AMD X370 and other AM4 chipsets (A/B/X 3/4/5 parts) and Threadripper equivalents have a secondary SMBus controller at I/O port address 0x0B20.  This bus is used by several manufacturers to control motherboard RGB lighting via embedded controllers.  I have been using this bus in my OpenRGB project to control the Aura RGB on many motherboards and ASRock also uses this bus for their Polychrome RGB controller.

See this kernel bug report: https://bugzilla.kernel.org/show_bug.cgi?id=202587

I am not aware of any CZ-compatible platforms which do not have the second SMBus channel.  All of AMD's AM4- and Threadripper- series chipsets that OpenRGB users have tested appear to have this secondary bus.  I also noticed this secondary bus is present on older AMD platforms including my FM1 home server.

Signed-off-by: Adam Honse <calcprogrammer1@gmail.com>

---
 drivers/i2c/busses/i2c-piix4.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 30ded6422e7b..69740a4ff1db 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -977,7 +977,8 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	}
 
 	if (dev->vendor == PCI_VENDOR_ID_AMD &&
-	    dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) {
+	    (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS ||
+	     dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)) {
 		retval = piix4_setup_sb800(dev, id, 1);
 	}
 
-- 
2.25.1

Re: [PATCH] i2c: Detect secondary SMBus controller on AMD AM4 chipsets

[Jean Delvare]

Hi Adam,

On Fri, 10 Apr 2020 15:48:44 -0500, Adam Honse wrote:
> The AMD X370 and other AM4 chipsets (A/B/X 3/4/5 parts) and Threadripper equivalents have a secondary SMBus controller at I/O port address 0x0B20.  This bus is used by several manufacturers to control motherboard RGB lighting via embedded controllers.  I have been using this bus in my OpenRGB project to control the Aura RGB on many motherboards and ASRock also uses this bus for their Polychrome RGB controller.
> 
> See this kernel bug report: https://bugzilla.kernel.org/show_bug.cgi?id=202587
> 
> I am not aware of any CZ-compatible platforms which do not have the second SMBus channel.  All of AMD's AM4- and Threadripper- series chipsets that OpenRGB users have tested appear to have this secondary bus.  I also noticed this secondary bus is present on older AMD platforms including my FM1 home server.

Please wrap the patch description at 75 columns. Note that
./scripts/checkpatch.pl would tell you about that.

Maybe Wolfram can fix if for your this time, if not you'll have to
resubmit.

> 
> Signed-off-by: Adam Honse <calcprogrammer1@gmail.com>
> 
> ---
>  drivers/i2c/busses/i2c-piix4.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 30ded6422e7b..69740a4ff1db 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -977,7 +977,8 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	}
>  
>  	if (dev->vendor == PCI_VENDOR_ID_AMD &&
> -	    dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) {
> +	    (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS ||
> +	     dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)) {
>  		retval = piix4_setup_sb800(dev, id, 1);
>  	}
>  

Fine with me.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: Detect secondary SMBus controller on AMD AM4 chipsets

[Jean Delvare]

Ah, one thing I forgot: subject line should start with "i2c: piix4: "
so that it is clear which driver is affected.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: Detect secondary SMBus controller on AMD AM4 chipsets

[Wolfram Sang]

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

On Fri, Apr 10, 2020 at 03:48:44PM -0500, Adam Honse wrote:
> The AMD X370 and other AM4 chipsets (A/B/X 3/4/5 parts) and Threadripper equivalents have a secondary SMBus controller at I/O port address 0x0B20.  This bus is used by several manufacturers to control motherboard RGB lighting via embedded controllers.  I have been using this bus in my OpenRGB project to control the Aura RGB on many motherboards and ASRock also uses this bus for their Polychrome RGB controller.
> 
> See this kernel bug report: https://bugzilla.kernel.org/show_bug.cgi?id=202587
> 
> I am not aware of any CZ-compatible platforms which do not have the second SMBus channel.  All of AMD's AM4- and Threadripper- series chipsets that OpenRGB users have tested appear to have this secondary bus.  I also noticed this secondary bus is present on older AMD platforms including my FM1 home server.
> 
> Signed-off-by: Adam Honse <calcprogrammer1@gmail.com>
> 

Reformatted the patch description, added Sebastians tags and applied to
for-next, thanks!


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

[PATCH] i2c: Detect secondary SMBus controller on AMD AM4 chipsets

[Adam Honse]

The AMD X370 and other AM4 chipsets (A/B/X 3/4/5 parts) and Threadripper equivalents have a secondary SMBus controller at I/O port address 0x0B20.  This bus is used by several manufacturers to control motherboard RGB lighting via embedded controllers.  I have been using this bus in my OpenRGB project to control the Aura RGB on many motherboards and ASRock also uses this bus for their Polychrome RGB controller.

See this kernel bug report: https://bugzilla.kernel.org/show_bug.cgi?id=202587

Thanks,

Adam Honse (calcprogrammer1@gmail.com)

---
 drivers/i2c/busses/i2c-piix4.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 30ded6422e7b..6068364b84f6 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -981,6 +981,11 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		retval = piix4_setup_sb800(dev, id, 1);
 	}
 
+	if (dev->vendor == PCI_VENDOR_ID_AMD &&
+	    dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
+		retval = piix4_setup_sb800(dev, id, 1);
+	}
+
 	if (retval > 0) {
 		/* Try to add the aux adapter if it exists,
 		 * piix4_add_adapter will clean up if this fails */
-- 
2.25.1

Re: [PATCH] i2c: Detect secondary SMBus controller on AMD AM4 chipsets

[Jean Delvare]

Hi Adam,

On Sun, 29 Mar 2020 12:44:40 -0500, Adam Honse wrote:
> The AMD X370 and other AM4 chipsets (A/B/X 3/4/5 parts) and Threadripper equivalents have a secondary SMBus controller at I/O port address 0x0B20.  This bus is used by several manufacturers to control motherboard RGB lighting via embedded controllers.  I have been using this bus in my OpenRGB project to control the Aura RGB on many motherboards and ASRock also uses this bus for their Polychrome RGB controller.
> 
> See this kernel bug report: https://bugzilla.kernel.org/show_bug.cgi?id=202587
> 
> Thanks,
> 
> Adam Honse (calcprogrammer1@gmail.com)

In order for this patch to be acceptable, this would need to be turned
into a proper Signed-off-by statement. Please see:

https://www.kernel.org/doc/html/v5.5/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

> 
> ---
>  drivers/i2c/busses/i2c-piix4.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 30ded6422e7b..6068364b84f6 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -981,6 +981,11 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		retval = piix4_setup_sb800(dev, id, 1);
>  	}
>  
> +	if (dev->vendor == PCI_VENDOR_ID_AMD &&
> +	    dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
> +		retval = piix4_setup_sb800(dev, id, 1);
> +	}
> +
>  	if (retval > 0) {
>  		/* Try to add the aux adapter if it exists,
>  		 * piix4_add_adapter will clean up if this fails */

I'm a bit worried about this change. Sure it works on the systems which
do have the second SMBus channel, but what about the systems which
don't? If there no device revision that needs to be checked to ensure
that the second channel is present?

This patch needs to be tested on tested on systems with CZ-compatible
CPUs which do not have the second SMBus channel. Or do you believe they
all do have it?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: Detect secondary SMBus controller on AMD AM4 chipsets

[Jean Delvare]

Hi Adam,

On Sat, 4 Apr 2020 13:20:58 -0500, Adam Honse wrote:
> I'm not sure what all chipsets are covered by the KERNCZ ID.  We have found
> that the secondary bus exists on all AM4 chipsets and on X399 as well.
> Considering the older Hudson 2 has the same secondary bus (which is
> correctly detected without a patch on my old FM1 home server, also at
> 0x0B20) I would believe the secondary bus exists on all chipsets.  I could
> add a revision check though.  My X370 reports (rev 59) in lspci.

Fair enough, let's assume it's present on all chipsets and we can
refine later if needed.

If so, can you please merge the kerncz check with the hudson2 check
right before as we do the same for both?

Then please submit with your Signed-off-by statement and I'll review
and ack the patch for Wolfram to commit.

Thanks,
-- 
Jean Delvare
SUSE L3 Support