Re: piixpm(4) add support for newer AMD chipsets
On Mon, Dec 16, 2019 at 09:05:47PM +0100, Claudio Jeker wrote:
> On Mon, Dec 16, 2019 at 08:02:55PM +0100, Mark Kettenis wrote:
> > > Date: Mon, 16 Dec 2019 12:37:51 +0100
> > > From: Claudio Jeker
> > >
> > > This diff should add support for newer smbus controllers used on newer AMD
> > > chipsets. Especially Hudson-2 and Kerncz based chipsets. On my Ryzen 5 the
> > > iic(4) busses attach but there is nothing detected on them (well possible
> > > that I missed something). I also implemented the up to 4 busses available
> > > on chipsets of the SBx00 series (on Hudson-2 and Kerncz only 2 ports).
> > >
> > > I would be interested if on systems with Ryzen CPUs something attaches to
> > > those iic(4) busses. Could be that I missed something and fail to properly
> > > access the bus.
> > > --
> > > :wq Claudio
> >
> > Looks good to me. A few nits below. Otherwise ok kettenis@
> >
> > > Index: piixpm.c
> > > ===
> > > RCS file: /cvs/src/sys/dev/pci/piixpm.c,v
> > > retrieving revision 1.39
> > > diff -u -p -r1.39 piixpm.c
> > > --- piixpm.c 1 Oct 2013 20:06:02 - 1.39
> > > +++ piixpm.c 16 Dec 2019 11:26:11 -
> > > @@ -45,15 +45,24 @@
> > > #define PIIXPM_DELAY 200
> > > #define PIIXPM_TIMEOUT 1
> > >
> > > +struct piixpm_smbus {
> > > + int sb_bus;
> > > + struct piixpm_softc *sb_sc;
> > > +};
> > > +
> > > struct piixpm_softc {
> > > struct device sc_dev;
> > >
> > > bus_space_tag_t sc_iot;
> > > bus_space_handle_t sc_ioh;
> > > + bus_space_handle_t sc_sb800_ioh;
> > > void * sc_ih;
> > > int sc_poll;
> > > + int sc_is_sb800;
> > > + int sc_is_kerncz;
> >
> > Can this be called sc_is_hudson2 or maybe sc_is_fch instead? It makes
> > more sense to have this variable describe the oldest variant instead
> > of the newest. I actually think sc_is_fch is the best name.
>
> Changed it to sc_is_fch.
>
> > >
> > > - struct i2c_controller sc_i2c_tag;
> > > + struct piixpm_smbus sc_busses[4];
> > > + struct i2c_controller sc_i2c_tag[4];
> > > struct rwlock sc_i2c_lock;
> > > struct {
> > > i2c_op_t op;
> > > @@ -86,6 +95,7 @@ struct cfdriver piixpm_cd = {
> > >
> > > const struct pci_matchid piixpm_ids[] = {
> > > { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_HUDSON2_SMB },
> > > + { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_KERNCZ_SMB },
> > >
> > > { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB200_SMB },
> > > { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB300_SMB },
> > > @@ -117,17 +127,21 @@ piixpm_attach(struct device *parent, str
> > > struct piixpm_softc *sc = (struct piixpm_softc *)self;
> > > struct pci_attach_args *pa = aux;
> > > bus_space_handle_t ioh;
> > > - u_int16_t smb0en;
> > > + u_int16_t val, smb0en;
> > > bus_addr_t base;
> > > pcireg_t conf;
> > > pci_intr_handle_t ih;
> > > const char *intrstr = NULL;
> > > struct i2cbus_attach_args iba;
> > > + int numbusses, i;
> > >
> > > sc->sc_iot = pa->pa_iot;
> > > + numbusses = 1;
> > >
> > > if ((PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
> > > PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB) ||
> > > + (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_ATI &&
> > > PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SBX00_SMB &&
> > > PCI_REVISION(pa->pa_class) >= 0x40)) {
> > > @@ -136,10 +150,7 @@ piixpm_attach(struct device *parent, str
> > >* hidden. We need to look at the "SMBus0En" Power
> > >* Management register to find out where they live.
> > >* We use indirect IO access through the index/data
> > > - * pair at 0xcd6/0xcd7 to access "SMBus0En". Since
> > > - * the index/data pair may be needed by other drivers,
> > > - * we only map them for the duration that we actually
> > > - * need them.
> > > + * pair at 0xcd6/0xcd7 to access "SMBus0En".
> > >*/
> >
> > I don't remember why we chose to immediately unmap the registers here.
> > So this may cause some breakage. Probably worth the risk.
>
> If someone wants to write a watchdog driver then something would be needed
> to allow both drivers access to this io range. Nothing else is using the
> piixreg.h file so I doubt this is a big issue. If we hit it than we can
> refactor the drivers.
Yes, I did this because I wasn't sure if some other driver might end
up needing to access those registers. The 0xcd6/0xcd7 pair were only
used to find the SMBus address, as the PCI device had no BARs.
I'm ok with this (and the rest).
> > > if (bus_space_map(sc->sc_iot, SB800_PMREG_BASE,
> > > SB800_PMREG_SIZE, 0, ) != 0) {
> > > @@ -147,29 +158,61 @@ piixpm_attach(struct device
Re: piixpm(4) add support for newer AMD chipsets
> Date: Mon, 16 Dec 2019 21:05:47 +0100
> From: Claudio Jeker
> Cc: tech@openbsd.org
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
>
> On Mon, Dec 16, 2019 at 08:02:55PM +0100, Mark Kettenis wrote:
> > > Date: Mon, 16 Dec 2019 12:37:51 +0100
> > > From: Claudio Jeker
> > >
> > > This diff should add support for newer smbus controllers used on newer AMD
> > > chipsets. Especially Hudson-2 and Kerncz based chipsets. On my Ryzen 5 the
> > > iic(4) busses attach but there is nothing detected on them (well possible
> > > that I missed something). I also implemented the up to 4 busses available
> > > on chipsets of the SBx00 series (on Hudson-2 and Kerncz only 2 ports).
> > >
> > > I would be interested if on systems with Ryzen CPUs something attaches to
> > > those iic(4) busses. Could be that I missed something and fail to properly
> > > access the bus.
> > > --
> > > :wq Claudio
> >
> > Looks good to me. A few nits below. Otherwise ok kettenis@
> >
> > > Index: piixpm.c
> > > ===
> > > RCS file: /cvs/src/sys/dev/pci/piixpm.c,v
> > > retrieving revision 1.39
> > > diff -u -p -r1.39 piixpm.c
> > > --- piixpm.c 1 Oct 2013 20:06:02 - 1.39
> > > +++ piixpm.c 16 Dec 2019 11:26:11 -
> > > @@ -45,15 +45,24 @@
> > > #define PIIXPM_DELAY 200
> > > #define PIIXPM_TIMEOUT 1
> > >
> > > +struct piixpm_smbus {
> > > + int sb_bus;
> > > + struct piixpm_softc *sb_sc;
> > > +};
> > > +
> > > struct piixpm_softc {
> > > struct device sc_dev;
> > >
> > > bus_space_tag_t sc_iot;
> > > bus_space_handle_t sc_ioh;
> > > + bus_space_handle_t sc_sb800_ioh;
> > > void * sc_ih;
> > > int sc_poll;
> > > + int sc_is_sb800;
> > > + int sc_is_kerncz;
> >
> > Can this be called sc_is_hudson2 or maybe sc_is_fch instead? It makes
> > more sense to have this variable describe the oldest variant instead
> > of the newest. I actually think sc_is_fch is the best name.
>
> Changed it to sc_is_fch.
>
> > >
> > > - struct i2c_controller sc_i2c_tag;
> > > + struct piixpm_smbus sc_busses[4];
> > > + struct i2c_controller sc_i2c_tag[4];
> > > struct rwlock sc_i2c_lock;
> > > struct {
> > > i2c_op_t op;
> > > @@ -86,6 +95,7 @@ struct cfdriver piixpm_cd = {
> > >
> > > const struct pci_matchid piixpm_ids[] = {
> > > { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_HUDSON2_SMB },
> > > + { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_KERNCZ_SMB },
> > >
> > > { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB200_SMB },
> > > { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB300_SMB },
> > > @@ -117,17 +127,21 @@ piixpm_attach(struct device *parent, str
> > > struct piixpm_softc *sc = (struct piixpm_softc *)self;
> > > struct pci_attach_args *pa = aux;
> > > bus_space_handle_t ioh;
> > > - u_int16_t smb0en;
> > > + u_int16_t val, smb0en;
> > > bus_addr_t base;
> > > pcireg_t conf;
> > > pci_intr_handle_t ih;
> > > const char *intrstr = NULL;
> > > struct i2cbus_attach_args iba;
> > > + int numbusses, i;
> > >
> > > sc->sc_iot = pa->pa_iot;
> > > + numbusses = 1;
> > >
> > > if ((PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
> > > PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB) ||
> > > + (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_ATI &&
> > > PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SBX00_SMB &&
> > > PCI_REVISION(pa->pa_class) >= 0x40)) {
> > > @@ -136,10 +150,7 @@ piixpm_attach(struct device *parent, str
> > >* hidden. We need to look at the "SMBus0En" Power
> > >* Management register to find out where they live.
> > >* We use indirect IO access through the index/data
> > > - * pair at 0xcd6/0xcd7 to access "SMBus0En". Since
> > > - * the index/data pair may be needed by other drivers,
> > > - * we only map them for the duration that we actually
> > > - * need them.
> > > + * pair at 0xcd6/0xcd7 to access "SMBus0En".
> > >*/
> >
> > I don't remember why we chose to immediately unmap the registers here.
> > So this may cause some breakage. Probably worth the risk.
>
> If someone wants to write a watchdog driver then something would be needed
> to allow both drivers access to this io range. Nothing else is using the
> piixreg.h file so I doubt this is a big issue. If we hit it than we can
> refactor the drivers.
>
> > > if (bus_space_map(sc->sc_iot, SB800_PMREG_BASE,
> > > SB800_PMREG_SIZE, 0, ) != 0) {
> > > @@ -147,29 +158,61 @@ piixpm_attach(struct device *parent, str
> > > return;
> > > }
> > >
> > > - /* Read "SmBus0En" */
> > > -
Re: piixpm(4) add support for newer AMD chipsets
On Mon, Dec 16, 2019 at 08:02:55PM +0100, Mark Kettenis wrote:
> > Date: Mon, 16 Dec 2019 12:37:51 +0100
> > From: Claudio Jeker
> >
> > This diff should add support for newer smbus controllers used on newer AMD
> > chipsets. Especially Hudson-2 and Kerncz based chipsets. On my Ryzen 5 the
> > iic(4) busses attach but there is nothing detected on them (well possible
> > that I missed something). I also implemented the up to 4 busses available
> > on chipsets of the SBx00 series (on Hudson-2 and Kerncz only 2 ports).
> >
> > I would be interested if on systems with Ryzen CPUs something attaches to
> > those iic(4) busses. Could be that I missed something and fail to properly
> > access the bus.
> > --
> > :wq Claudio
>
> Looks good to me. A few nits below. Otherwise ok kettenis@
>
> > Index: piixpm.c
> > ===
> > RCS file: /cvs/src/sys/dev/pci/piixpm.c,v
> > retrieving revision 1.39
> > diff -u -p -r1.39 piixpm.c
> > --- piixpm.c1 Oct 2013 20:06:02 - 1.39
> > +++ piixpm.c16 Dec 2019 11:26:11 -
> > @@ -45,15 +45,24 @@
> > #define PIIXPM_DELAY 200
> > #define PIIXPM_TIMEOUT 1
> >
> > +struct piixpm_smbus {
> > + int sb_bus;
> > + struct piixpm_softc *sb_sc;
> > +};
> > +
> > struct piixpm_softc {
> > struct device sc_dev;
> >
> > bus_space_tag_t sc_iot;
> > bus_space_handle_t sc_ioh;
> > + bus_space_handle_t sc_sb800_ioh;
> > void * sc_ih;
> > int sc_poll;
> > + int sc_is_sb800;
> > + int sc_is_kerncz;
>
> Can this be called sc_is_hudson2 or maybe sc_is_fch instead? It makes
> more sense to have this variable describe the oldest variant instead
> of the newest. I actually think sc_is_fch is the best name.
Changed it to sc_is_fch.
> >
> > - struct i2c_controller sc_i2c_tag;
> > + struct piixpm_smbus sc_busses[4];
> > + struct i2c_controller sc_i2c_tag[4];
> > struct rwlock sc_i2c_lock;
> > struct {
> > i2c_op_t op;
> > @@ -86,6 +95,7 @@ struct cfdriver piixpm_cd = {
> >
> > const struct pci_matchid piixpm_ids[] = {
> > { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_HUDSON2_SMB },
> > + { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_KERNCZ_SMB },
> >
> > { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB200_SMB },
> > { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB300_SMB },
> > @@ -117,17 +127,21 @@ piixpm_attach(struct device *parent, str
> > struct piixpm_softc *sc = (struct piixpm_softc *)self;
> > struct pci_attach_args *pa = aux;
> > bus_space_handle_t ioh;
> > - u_int16_t smb0en;
> > + u_int16_t val, smb0en;
> > bus_addr_t base;
> > pcireg_t conf;
> > pci_intr_handle_t ih;
> > const char *intrstr = NULL;
> > struct i2cbus_attach_args iba;
> > + int numbusses, i;
> >
> > sc->sc_iot = pa->pa_iot;
> > + numbusses = 1;
> >
> > if ((PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
> > PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB) ||
> > + (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_ATI &&
> > PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SBX00_SMB &&
> > PCI_REVISION(pa->pa_class) >= 0x40)) {
> > @@ -136,10 +150,7 @@ piixpm_attach(struct device *parent, str
> > * hidden. We need to look at the "SMBus0En" Power
> > * Management register to find out where they live.
> > * We use indirect IO access through the index/data
> > -* pair at 0xcd6/0xcd7 to access "SMBus0En". Since
> > -* the index/data pair may be needed by other drivers,
> > -* we only map them for the duration that we actually
> > -* need them.
> > +* pair at 0xcd6/0xcd7 to access "SMBus0En".
> > */
>
> I don't remember why we chose to immediately unmap the registers here.
> So this may cause some breakage. Probably worth the risk.
If someone wants to write a watchdog driver then something would be needed
to allow both drivers access to this io range. Nothing else is using the
piixreg.h file so I doubt this is a big issue. If we hit it than we can
refactor the drivers.
> > if (bus_space_map(sc->sc_iot, SB800_PMREG_BASE,
> > SB800_PMREG_SIZE, 0, ) != 0) {
> > @@ -147,29 +158,61 @@ piixpm_attach(struct device *parent, str
> > return;
> > }
> >
> > - /* Read "SmBus0En" */
> > - bus_space_write_1(sc->sc_iot, ioh, 0, SB800_PMREG_SMB0EN);
> > - smb0en = bus_space_read_1(sc->sc_iot, ioh, 1);
> > - bus_space_write_1(sc->sc_iot, ioh, 0, SB800_PMREG_SMB0EN + 1);
> > - smb0en |= (bus_space_read_1(sc->sc_iot, ioh, 1) << 8);
> > -
> > -
Re: piixpm(4) add support for newer AMD chipsets
On Mon, Dec 16, 2019 at 12:37:51PM +0100, Claudio Jeker wrote: > This diff should add support for newer smbus controllers used on newer AMD > chipsets. Especially Hudson-2 and Kerncz based chipsets. On my Ryzen 5 the > iic(4) busses attach but there is nothing detected on them (well possible > that I missed something). I also implemented the up to 4 busses available > on chipsets of the SBx00 series (on Hudson-2 and Kerncz only 2 ports). > > I would be interested if on systems with Ryzen CPUs something attaches to > those iic(4) busses. Could be that I missed something and fail to properly > access the bus. > -- > :wq Claudio On a ThinkPad x395 with: cpu0: AMD Ryzen 7 PRO 3700U w/ Radeon Vega Mobile Gfx, 2296.08 MHz, 17-18-01 I observe this change in dmesg: -"AMD FCH SMBus" rev 0x61 at pci0 dev 20 function 0 not configured +piixpm0 at pci0 dev 20 function 0 "AMD FCH SMBus" rev 0x61: polling +iic0 at piixpm0 +iic1 at piixpm0 Full dmesg below: OpenBSD 6.6-current (GENERIC.MP) #1: Mon Dec 16 20:44:36 CET 2019 jas...@tau.office.jasper.la:/sys/arch/amd64/compile/GENERIC.MP real mem = 14888513536 (14198MB) avail mem = 14424879104 (13756MB) mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 3.1 @ 0xb9ecc000 (63 entries) bios0: vendor LENOVO version "R13ET39W(1.13 )" date 10/11/2019 bios0: LENOVO 20NLCTO1WW acpi0 at bios0: ACPI 5.0 acpi0: sleep states S0 S3 S4 S5 acpi0: tables DSDT FACP SSDT SSDT SSDT MSDM SLIC BATB HPET APIC MCFG SBST WSMT IVRS SSDT CRAT CDIT FPDT SSDT SSDT SSDT UEFI acpi0: wakeup devices GPP0(S3) GPP1(S3) GPP2(S3) GPP3(S3) GPP4(S3) L850(S3) GPP5(S3) GPP6(S3) GP17(S3) XHC0(S3) XHC1(S3) GP18(S3) LID_(S3) SLPB(S3) acpitimer0 at acpi0: 3579545 Hz, 32 bits acpihpet0 at acpi0: 14318180 Hz acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: AMD Ryzen 7 PRO 3700U w/ Radeon Vega Mobile Gfx, 2296.00 MHz, 17-18-01 cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,SKINIT,TCE,TOPEXT,CPCTR,DBKP,PCTRL3,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,RDSEED,ADX,SMAP,CLFLUSHOPT,SHA,IBPB,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu0: 64KB 64b/line 4-way I-cache, 32KB 64b/line 8-way D-cache, 512KB 64b/line 8-way L2 cache, 4MB 64b/line 16-way L3 cache cpu0: ITLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu0: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges cpu0: apic clock running at 24MHz cpu0: mwait min=64, max=64, C-substates=1.1, IBE cpu1 at mainbus0: apid 1 (application processor) cpu1: AMD Ryzen 7 PRO 3700U w/ Radeon Vega Mobile Gfx, 2295.67 MHz, 17-18-01 cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,SKINIT,TCE,TOPEXT,CPCTR,DBKP,PCTRL3,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,RDSEED,ADX,SMAP,CLFLUSHOPT,SHA,IBPB,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu1: 64KB 64b/line 4-way I-cache, 32KB 64b/line 8-way D-cache, 512KB 64b/line 8-way L2 cache, 4MB 64b/line 16-way L3 cache cpu1: ITLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu1: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu1: smt 1, core 0, package 0 cpu2 at mainbus0: apid 2 (application processor) cpu2: AMD Ryzen 7 PRO 3700U w/ Radeon Vega Mobile Gfx, 2295.67 MHz, 17-18-01 cpu2: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,SKINIT,TCE,TOPEXT,CPCTR,DBKP,PCTRL3,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,RDSEED,ADX,SMAP,CLFLUSHOPT,SHA,IBPB,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu2: 64KB 64b/line 4-way I-cache, 32KB 64b/line 8-way D-cache, 512KB 64b/line 8-way L2 cache, 4MB 64b/line 16-way L3 cache cpu2: ITLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu2: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu2: smt 0, core 1, package 0 cpu3 at mainbus0: apid 3 (application processor) cpu3: AMD Ryzen 7 PRO 3700U w/ Radeon Vega Mobile Gfx, 2295.67 MHz, 17-18-01 cpu3:
Re: piixpm(4) add support for newer AMD chipsets
> Date: Mon, 16 Dec 2019 12:37:51 +0100
> From: Claudio Jeker
>
> This diff should add support for newer smbus controllers used on newer AMD
> chipsets. Especially Hudson-2 and Kerncz based chipsets. On my Ryzen 5 the
> iic(4) busses attach but there is nothing detected on them (well possible
> that I missed something). I also implemented the up to 4 busses available
> on chipsets of the SBx00 series (on Hudson-2 and Kerncz only 2 ports).
>
> I would be interested if on systems with Ryzen CPUs something attaches to
> those iic(4) busses. Could be that I missed something and fail to properly
> access the bus.
> --
> :wq Claudio
Looks good to me. A few nits below. Otherwise ok kettenis@
> Index: piixpm.c
> ===
> RCS file: /cvs/src/sys/dev/pci/piixpm.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 piixpm.c
> --- piixpm.c 1 Oct 2013 20:06:02 - 1.39
> +++ piixpm.c 16 Dec 2019 11:26:11 -
> @@ -45,15 +45,24 @@
> #define PIIXPM_DELAY 200
> #define PIIXPM_TIMEOUT 1
>
> +struct piixpm_smbus {
> + int sb_bus;
> + struct piixpm_softc *sb_sc;
> +};
> +
> struct piixpm_softc {
> struct device sc_dev;
>
> bus_space_tag_t sc_iot;
> bus_space_handle_t sc_ioh;
> + bus_space_handle_t sc_sb800_ioh;
> void * sc_ih;
> int sc_poll;
> + int sc_is_sb800;
> + int sc_is_kerncz;
Can this be called sc_is_hudson2 or maybe sc_is_fch instead? It makes
more sense to have this variable describe the oldest variant instead
of the newest. I actually think sc_is_fch is the best name.
>
> - struct i2c_controller sc_i2c_tag;
> + struct piixpm_smbus sc_busses[4];
> + struct i2c_controller sc_i2c_tag[4];
> struct rwlock sc_i2c_lock;
> struct {
> i2c_op_t op;
> @@ -86,6 +95,7 @@ struct cfdriver piixpm_cd = {
>
> const struct pci_matchid piixpm_ids[] = {
> { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_HUDSON2_SMB },
> + { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_KERNCZ_SMB },
>
> { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB200_SMB },
> { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB300_SMB },
> @@ -117,17 +127,21 @@ piixpm_attach(struct device *parent, str
> struct piixpm_softc *sc = (struct piixpm_softc *)self;
> struct pci_attach_args *pa = aux;
> bus_space_handle_t ioh;
> - u_int16_t smb0en;
> + u_int16_t val, smb0en;
> bus_addr_t base;
> pcireg_t conf;
> pci_intr_handle_t ih;
> const char *intrstr = NULL;
> struct i2cbus_attach_args iba;
> + int numbusses, i;
>
> sc->sc_iot = pa->pa_iot;
> + numbusses = 1;
>
> if ((PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
> PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB) ||
> + (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_ATI &&
> PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SBX00_SMB &&
> PCI_REVISION(pa->pa_class) >= 0x40)) {
> @@ -136,10 +150,7 @@ piixpm_attach(struct device *parent, str
>* hidden. We need to look at the "SMBus0En" Power
>* Management register to find out where they live.
>* We use indirect IO access through the index/data
> - * pair at 0xcd6/0xcd7 to access "SMBus0En". Since
> - * the index/data pair may be needed by other drivers,
> - * we only map them for the duration that we actually
> - * need them.
> + * pair at 0xcd6/0xcd7 to access "SMBus0En".
>*/
I don't remember why we chose to immediately unmap the registers here.
So this may cause some breakage. Probably worth the risk.
> if (bus_space_map(sc->sc_iot, SB800_PMREG_BASE,
> SB800_PMREG_SIZE, 0, ) != 0) {
> @@ -147,29 +158,61 @@ piixpm_attach(struct device *parent, str
> return;
> }
>
> - /* Read "SmBus0En" */
> - bus_space_write_1(sc->sc_iot, ioh, 0, SB800_PMREG_SMB0EN);
> - smb0en = bus_space_read_1(sc->sc_iot, ioh, 1);
> - bus_space_write_1(sc->sc_iot, ioh, 0, SB800_PMREG_SMB0EN + 1);
> - smb0en |= (bus_space_read_1(sc->sc_iot, ioh, 1) << 8);
> -
> - bus_space_unmap(sc->sc_iot, ioh, SB800_PMREG_SIZE);
> + 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_KERNCZ_SMB)) {
> + bus_space_write_1(sc->sc_iot, ioh, 0,
> + AMDFCH41_PM_DECODE_EN);
> + val = bus_space_read_1(sc->sc_iot, ioh,
Re: piixpm(4) add support for newer AMD chipsets
On Mon, Dec 16, 2019 at 03:19:30PM +0100, Claudio Jeker wrote: > On Mon, Dec 16, 2019 at 08:46:21AM -0500, Bryan Steele wrote: > > On Mon, Dec 16, 2019 at 12:37:51PM +0100, Claudio Jeker wrote: > > > This diff should add support for newer smbus controllers used on newer AMD > > > chipsets. Especially Hudson-2 and Kerncz based chipsets. On my Ryzen 5 the > > > iic(4) busses attach but there is nothing detected on them (well possible > > > that I missed something). I also implemented the up to 4 busses available > > > on chipsets of the SBx00 series (on Hudson-2 and Kerncz only 2 ports). > > > > > > I would be interested if on systems with Ryzen CPUs something attaches to > > > those iic(4) busses. Could be that I missed something and fail to properly > > > access the bus. > > > -- > > > :wq Claudio > > > > I had a similar diff (except without the additional busses), and didn't > > go further as none of my machines show any devices either (not even > > spdmem(4)), I assumed it was on another bus.. but maybe it's on an > > entirely different controller now. > > > > bios0: vendor American Megatrends Inc. version "5220" date 09/11/2019 > > bios0: ASUSTeK COMPUTER INC. PRIME X470-PRO > > .. > > piixpm0 at pci0 dev 20 function 0 "AMD FCH SMBus" rev 0x59: polling > > iic0 at piixpm0 > > iic1 at piixpm0 > > > > Otherwise, diff "looks" ok. I'll try it on my MateBook later, but > > I have a feeling it was the same story. > > > > Otherwise, diff "looks" ok. > > > > spdmem(4) needs an update to support DDR4 to show up on the bus. I did not > realize that until after I sent this out. This is why spdmem(4) does not > attach. I'm working now on spdmem(4) support for DDR4. Oh interesting. I hadn't considered that. Look forward to testing that then! :-) > -- > :wq Claudio >
Re: piixpm(4) add support for newer AMD chipsets
On Mon, Dec 16, 2019 at 08:46:21AM -0500, Bryan Steele wrote: > On Mon, Dec 16, 2019 at 12:37:51PM +0100, Claudio Jeker wrote: > > This diff should add support for newer smbus controllers used on newer AMD > > chipsets. Especially Hudson-2 and Kerncz based chipsets. On my Ryzen 5 the > > iic(4) busses attach but there is nothing detected on them (well possible > > that I missed something). I also implemented the up to 4 busses available > > on chipsets of the SBx00 series (on Hudson-2 and Kerncz only 2 ports). > > > > I would be interested if on systems with Ryzen CPUs something attaches to > > those iic(4) busses. Could be that I missed something and fail to properly > > access the bus. > > -- > > :wq Claudio > > I had a similar diff (except without the additional busses), and didn't > go further as none of my machines show any devices either (not even > spdmem(4)), I assumed it was on another bus.. but maybe it's on an > entirely different controller now. > > bios0: vendor American Megatrends Inc. version "5220" date 09/11/2019 > bios0: ASUSTeK COMPUTER INC. PRIME X470-PRO > .. > piixpm0 at pci0 dev 20 function 0 "AMD FCH SMBus" rev 0x59: polling > iic0 at piixpm0 > iic1 at piixpm0 > > Otherwise, diff "looks" ok. I'll try it on my MateBook later, but > I have a feeling it was the same story. > > Otherwise, diff "looks" ok. > spdmem(4) needs an update to support DDR4 to show up on the bus. I did not realize that until after I sent this out. This is why spdmem(4) does not attach. I'm working now on spdmem(4) support for DDR4. -- :wq Claudio
Re: piixpm(4) add support for newer AMD chipsets
On Mon, Dec 16, 2019 at 12:37:51PM +0100, Claudio Jeker wrote:
> This diff should add support for newer smbus controllers used on newer AMD
> chipsets. Especially Hudson-2 and Kerncz based chipsets. On my Ryzen 5 the
> iic(4) busses attach but there is nothing detected on them (well possible
> that I missed something). I also implemented the up to 4 busses available
> on chipsets of the SBx00 series (on Hudson-2 and Kerncz only 2 ports).
>
> I would be interested if on systems with Ryzen CPUs something attaches to
> those iic(4) busses. Could be that I missed something and fail to properly
> access the bus.
> --
> :wq Claudio
I had a similar diff (except without the additional busses), and didn't
go further as none of my machines show any devices either (not even
spdmem(4)), I assumed it was on another bus.. but maybe it's on an
entirely different controller now.
bios0: vendor American Megatrends Inc. version "5220" date 09/11/2019
bios0: ASUSTeK COMPUTER INC. PRIME X470-PRO
..
piixpm0 at pci0 dev 20 function 0 "AMD FCH SMBus" rev 0x59: polling
iic0 at piixpm0
iic1 at piixpm0
Otherwise, diff "looks" ok. I'll try it on my MateBook later, but
I have a feeling it was the same story.
Otherwise, diff "looks" ok.
> Index: piixpm.c
> ===
> RCS file: /cvs/src/sys/dev/pci/piixpm.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 piixpm.c
> --- piixpm.c 1 Oct 2013 20:06:02 - 1.39
> +++ piixpm.c 16 Dec 2019 11:26:11 -
> @@ -45,15 +45,24 @@
> #define PIIXPM_DELAY 200
> #define PIIXPM_TIMEOUT 1
>
> +struct piixpm_smbus {
> + int sb_bus;
> + struct piixpm_softc *sb_sc;
> +};
> +
> struct piixpm_softc {
> struct device sc_dev;
>
> bus_space_tag_t sc_iot;
> bus_space_handle_t sc_ioh;
> + bus_space_handle_t sc_sb800_ioh;
> void * sc_ih;
> int sc_poll;
> + int sc_is_sb800;
> + int sc_is_kerncz;
>
> - struct i2c_controller sc_i2c_tag;
> + struct piixpm_smbus sc_busses[4];
> + struct i2c_controller sc_i2c_tag[4];
> struct rwlock sc_i2c_lock;
> struct {
> i2c_op_t op;
> @@ -86,6 +95,7 @@ struct cfdriver piixpm_cd = {
>
> const struct pci_matchid piixpm_ids[] = {
> { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_HUDSON2_SMB },
> + { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_KERNCZ_SMB },
>
> { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB200_SMB },
> { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB300_SMB },
> @@ -117,17 +127,21 @@ piixpm_attach(struct device *parent, str
> struct piixpm_softc *sc = (struct piixpm_softc *)self;
> struct pci_attach_args *pa = aux;
> bus_space_handle_t ioh;
> - u_int16_t smb0en;
> + u_int16_t val, smb0en;
> bus_addr_t base;
> pcireg_t conf;
> pci_intr_handle_t ih;
> const char *intrstr = NULL;
> struct i2cbus_attach_args iba;
> + int numbusses, i;
>
> sc->sc_iot = pa->pa_iot;
> + numbusses = 1;
>
> if ((PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
> PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB) ||
> + (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_ATI &&
> PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SBX00_SMB &&
> PCI_REVISION(pa->pa_class) >= 0x40)) {
> @@ -136,10 +150,7 @@ piixpm_attach(struct device *parent, str
>* hidden. We need to look at the "SMBus0En" Power
>* Management register to find out where they live.
>* We use indirect IO access through the index/data
> - * pair at 0xcd6/0xcd7 to access "SMBus0En". Since
> - * the index/data pair may be needed by other drivers,
> - * we only map them for the duration that we actually
> - * need them.
> + * pair at 0xcd6/0xcd7 to access "SMBus0En".
>*/
> if (bus_space_map(sc->sc_iot, SB800_PMREG_BASE,
> SB800_PMREG_SIZE, 0, ) != 0) {
> @@ -147,29 +158,61 @@ piixpm_attach(struct device *parent, str
> return;
> }
>
> - /* Read "SmBus0En" */
> - bus_space_write_1(sc->sc_iot, ioh, 0, SB800_PMREG_SMB0EN);
> - smb0en = bus_space_read_1(sc->sc_iot, ioh, 1);
> - bus_space_write_1(sc->sc_iot, ioh, 0, SB800_PMREG_SMB0EN + 1);
> - smb0en |= (bus_space_read_1(sc->sc_iot, ioh, 1) << 8);
> -
> - bus_space_unmap(sc->sc_iot, ioh, SB800_PMREG_SIZE);
> + if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
> + (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB ||
> + PCI_PRODUCT(pa->pa_id) ==
piixpm(4) add support for newer AMD chipsets
This diff should add support for newer smbus controllers used on newer AMD
chipsets. Especially Hudson-2 and Kerncz based chipsets. On my Ryzen 5 the
iic(4) busses attach but there is nothing detected on them (well possible
that I missed something). I also implemented the up to 4 busses available
on chipsets of the SBx00 series (on Hudson-2 and Kerncz only 2 ports).
I would be interested if on systems with Ryzen CPUs something attaches to
those iic(4) busses. Could be that I missed something and fail to properly
access the bus.
--
:wq Claudio
Index: piixpm.c
===
RCS file: /cvs/src/sys/dev/pci/piixpm.c,v
retrieving revision 1.39
diff -u -p -r1.39 piixpm.c
--- piixpm.c1 Oct 2013 20:06:02 - 1.39
+++ piixpm.c16 Dec 2019 11:26:11 -
@@ -45,15 +45,24 @@
#define PIIXPM_DELAY 200
#define PIIXPM_TIMEOUT 1
+struct piixpm_smbus {
+ int sb_bus;
+ struct piixpm_softc *sb_sc;
+};
+
struct piixpm_softc {
struct device sc_dev;
bus_space_tag_t sc_iot;
bus_space_handle_t sc_ioh;
+ bus_space_handle_t sc_sb800_ioh;
void * sc_ih;
int sc_poll;
+ int sc_is_sb800;
+ int sc_is_kerncz;
- struct i2c_controller sc_i2c_tag;
+ struct piixpm_smbus sc_busses[4];
+ struct i2c_controller sc_i2c_tag[4];
struct rwlock sc_i2c_lock;
struct {
i2c_op_t op;
@@ -86,6 +95,7 @@ struct cfdriver piixpm_cd = {
const struct pci_matchid piixpm_ids[] = {
{ PCI_VENDOR_AMD, PCI_PRODUCT_AMD_HUDSON2_SMB },
+ { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_KERNCZ_SMB },
{ PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB200_SMB },
{ PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB300_SMB },
@@ -117,17 +127,21 @@ piixpm_attach(struct device *parent, str
struct piixpm_softc *sc = (struct piixpm_softc *)self;
struct pci_attach_args *pa = aux;
bus_space_handle_t ioh;
- u_int16_t smb0en;
+ u_int16_t val, smb0en;
bus_addr_t base;
pcireg_t conf;
pci_intr_handle_t ih;
const char *intrstr = NULL;
struct i2cbus_attach_args iba;
+ int numbusses, i;
sc->sc_iot = pa->pa_iot;
+ numbusses = 1;
if ((PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB) ||
+ (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_ATI &&
PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SBX00_SMB &&
PCI_REVISION(pa->pa_class) >= 0x40)) {
@@ -136,10 +150,7 @@ piixpm_attach(struct device *parent, str
* hidden. We need to look at the "SMBus0En" Power
* Management register to find out where they live.
* We use indirect IO access through the index/data
-* pair at 0xcd6/0xcd7 to access "SMBus0En". Since
-* the index/data pair may be needed by other drivers,
-* we only map them for the duration that we actually
-* need them.
+* pair at 0xcd6/0xcd7 to access "SMBus0En".
*/
if (bus_space_map(sc->sc_iot, SB800_PMREG_BASE,
SB800_PMREG_SIZE, 0, ) != 0) {
@@ -147,29 +158,61 @@ piixpm_attach(struct device *parent, str
return;
}
- /* Read "SmBus0En" */
- bus_space_write_1(sc->sc_iot, ioh, 0, SB800_PMREG_SMB0EN);
- smb0en = bus_space_read_1(sc->sc_iot, ioh, 1);
- bus_space_write_1(sc->sc_iot, ioh, 0, SB800_PMREG_SMB0EN + 1);
- smb0en |= (bus_space_read_1(sc->sc_iot, ioh, 1) << 8);
-
- bus_space_unmap(sc->sc_iot, ioh, SB800_PMREG_SIZE);
+ 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_KERNCZ_SMB)) {
+ bus_space_write_1(sc->sc_iot, ioh, 0,
+ AMDFCH41_PM_DECODE_EN);
+ val = bus_space_read_1(sc->sc_iot, ioh, 1);
+ smb0en = val & AMDFCH41_SMBUS_EN;
+
+ bus_space_write_1(sc->sc_iot, ioh, 0,
+ AMDFCH41_PM_DECODE_EN + 1);
+ val = bus_space_read_1(sc->sc_iot, ioh, 1) << 8;
+ base = val;
+ } else {
+ /* Read "SmBus0En" */
+ bus_space_write_1(sc->sc_iot, ioh, 0,
+ SB800_PMREG_SMB0EN);
+ val = bus_space_read_1(sc->sc_iot, ioh, 1);
+
