On Tue, Jan 07, 2020 at 12:44:59PM +0100, Claudio Jeker wrote:
> On Tue, Jan 07, 2020 at 09:27:50AM +0100, Claudio Jeker wrote:
> > In -current I added support for the additional I2C busses on piixpm(4)
> > now I noticed that on my old AMD system the I2C bus seems to either
> > connect all those 4 busses together (or there is a bug in the driver).
>
> Looks like this is indeed a problem with the driver.
>
> With the following diff I now get:
> piixpm0 at pci0 dev 20 function 0 "ATI SBx00 SMBus" rev 0x41: polling
> iic0 at piixpm0
> sdtemp0 at iic0 addr 0x18: stts2002
> sdtemp1 at iic0 addr 0x19: stts2002
> sdtemp2 at iic0 addr 0x1a: stts2002
> sdtemp3 at iic0 addr 0x1b: stts2002
> spdmem0 at iic0 addr 0x50: 4GB DDR3 SDRAM ECC PC3-10600 with thermal sensor
> spdmem1 at iic0 addr 0x51: 4GB DDR3 SDRAM ECC PC3-10600 with thermal sensor
> spdmem2 at iic0 addr 0x52: 4GB DDR3 SDRAM ECC PC3-10600 with thermal sensor
> spdmem3 at iic0 addr 0x53: 4GB DDR3 SDRAM ECC PC3-10600 with thermal sensor
> iic1 at piixpm0
> piixpm0: exec: op 1, addr 0x18, cmdlen 1, len 0, flags 0x08: timeout,
> status 0x1
> <BUSY>
> iic2 at piixpm0
> iic3 at piixpm0
>
> Not sure what is up with the timeout on iic @ addr 0x18. If that is seen
> on more systems it may be better to only use the primary bus on SB800
> devices that don't support SB800_PMREG_SMB0SEL.
>
> Please test and report back.
Picking this up again. Since the second bus causes read timeouts and the
docu mentions that this bus is used for fan and temp control I think it is
better to just not attach the extra busses on these old AMD chipsets.
The additional SB800 busses will only be used if SB800_PMREG_SMB0SELEN
tells us that SMB0SELEN is enabled.
While looking at this I also investigated a bit more into the
PCI_PRODUCT_AMD_HUDSON2_SMB issues. So PCI ID 1022:780b are used by AMD
Bolton and Family 16h model 30h-3fh. AMD Bolton still needs the old layout
while the family 16h chips use the FCH register layout. So merge the if
statements a bit so that AMD Bolton uses the old code path and Family 16h
uses the new.
Last but not least I was informed that we reversed the meaning of
SB800_SMB_HOSTC_SMI (if SB800_SMB_HOSTC bit 1 is set then use IRQ else
SMI). This is visible in the dmesg line of piixpm.
Please test this since I can't test this properly at the moment.
Works for me on
piixpm0 at pci0 dev 20 function 0 "AMD FCH SMBus" rev 0x61: SMI
--
:wq Claudio
Index: piixpm.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/piixpm.c,v
retrieving revision 1.41
diff -u -p -r1.41 piixpm.c
--- piixpm.c 9 Jan 2020 14:35:19 -0000 1.41
+++ piixpm.c 21 Jan 2020 01:23:21 -0000
@@ -158,8 +158,13 @@ piixpm_attach(struct device *parent, str
return;
}
+ /*
+ * AMD Bolton matches PCI_PRODUCT_AMD_HUDSON2_SMB but
+ * uses old register layout. Therefor check PCI_REVISION.
+ */
if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
- (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB ||
+ ((PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB &&
+ PCI_REVISION(pa->pa_class) >= 0x1f) ||
PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_KERNCZ_SMB)) {
bus_space_write_1(sc->sc_iot, ioh, 0,
AMDFCH41_PM_DECODE_EN);
@@ -170,6 +175,9 @@ piixpm_attach(struct device *parent, str
AMDFCH41_PM_DECODE_EN + 1);
val = bus_space_read_1(sc->sc_iot, ioh, 1) << 8;
base = val;
+
+ sc->sc_is_fch = 1;
+ numbusses = 2;
} else {
/* Read "SmBus0En" */
bus_space_write_1(sc->sc_iot, ioh, 0,
@@ -181,6 +189,14 @@ piixpm_attach(struct device *parent, str
val |= (bus_space_read_1(sc->sc_iot, ioh, 1) << 8);
smb0en = val & SB800_SMB0EN_EN;
base = val & SB800_SMB0EN_BASE_MASK;
+
+ bus_space_write_1(sc->sc_iot, ioh, 0,
+ SB800_PMREG_SMB0SELEN);
+ val = bus_space_read_1(sc->sc_iot, ioh, 1);
+ if (val & SB800_SMB0SELEN_EN) {
+ sc->sc_is_sb800 = 1;
+ numbusses = 4;
+ }
}
if (smb0en == 0) {
@@ -190,17 +206,6 @@ piixpm_attach(struct device *parent, str
}
sc->sc_sb800_ioh = ioh;
- if ((PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
- PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_KERNCZ_SMB) ||
- (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
- PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB &&
- PCI_REVISION(pa->pa_class) >= 0x1f)) {
- sc->sc_is_fch = 1;
- numbusses = 2;
- } else {
- sc->sc_is_sb800 = 1;
- numbusses = 4;
- }
/* Map I/O space */
if (base == 0 || bus_space_map(sc->sc_iot, base,
@@ -213,10 +218,10 @@ piixpm_attach(struct device *parent, str
/* Read configuration */
conf = bus_space_read_1(sc->sc_iot, sc->sc_ioh,
SB800_SMB_HOSTC);
- if (conf & SB800_SMB_HOSTC_SMI)
- conf = PIIX_SMB_HOSTC_SMI;
- else
+ if (conf & SB800_SMB_HOSTC_INTMASK)
conf = PIIX_SMB_HOSTC_IRQ;
+ else
+ conf = PIIX_SMB_HOSTC_SMI;
} else {
/* Read configuration */
conf = pci_conf_read(pa->pa_pc, pa->pa_tag, PIIX_SMB_HOSTC);
Index: piixreg.h
===================================================================
RCS file: /cvs/src/sys/dev/pci/piixreg.h,v
retrieving revision 1.5
diff -u -p -r1.5 piixreg.h
--- piixreg.h 16 Dec 2019 21:39:40 -0000 1.5
+++ piixreg.h 21 Jan 2020 01:22:49 -0000
@@ -70,11 +70,13 @@
#define SB800_PMREG_SIZE 2 /* index/data pair */
#define SB800_PMREG_SMB0EN 0x2c /* 16-bit register */
#define SB800_PMREG_SMB0SEL 0x2e /* bus selection */
+#define SB800_PMREG_SMB0SELEN 0x2f /* bus selection enable */
#define SB800_SMB0EN_EN 0x0001
#define SB800_SMB0EN_BASE_MASK 0xffe0
+#define SB800_SMB0SELEN_EN 0x01
#define SB800_SMB_HOSTC 0x10 /* I2C bus configuration */
-#define SB800_SMB_HOSTC_SMI (1 << 0) /* SMI */
+#define SB800_SMB_HOSTC_INTMASK 0x1 /* 0: SMI 1: IRQ */
#define SB800_SMB_SIZE 0x14 /* SMBus I/O space size */
