| Message ID | 20180124104052.9920-1-ard.biesheuvel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [edk2] Silicon/SynQuacer/PlatformDxe: enable spread spectrum mode for ASM1061 SATA | expand |
On Wed, Jan 24, 2018 at 10:40:52AM +0000, Ard Biesheuvel wrote: > The ASM1061 SATA controller integrated into the DeveloperBox board > emits too much electromagnetic radiation, so it needs spread spectrum > mode enabled. I presume the spectrum mode is on the PCIe side rather than the SATA side? Can this be explicitly mentioned? > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/{Asmedia118x.c => Asmedia.c} | 79 +++++++++++++++----- Since you started renaming things, I invoke the right of bikeshedding. The SATA controller is only Asmedia as well due to happenstance - if this file is becoming "various PCI tweaks", can we just call it Pci.c (or PciTweaks.c by all means)? > Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf | 2 +- > 2 files changed, 60 insertions(+), 21 deletions(-) > > diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c > similarity index 64% > rename from Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c > rename to Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c > index c4cbacd3dff9..6c289fa1892e 100644 > --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c > +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c > @@ -15,9 +15,12 @@ > #include "PlatformDxe.h" > > #define ASMEDIA_VID 0x1b21 > +#define ASM1061_PID 0x0612 > #define ASM1182E_PID 0x1182 > #define ASM1184E_PID 0x1184 > > +#define ASM1061_SSC_OFFSET 0xA10 > + > #define ASM118x_PCIE_CAPABILITY_OFFSET 0x80 > #define ASM118x_PCIE_LINK_CONTROL_OFFSET (ASM118x_PCIE_CAPABILITY_OFFSET + \ > OFFSET_OF (PCI_CAPABILITY_PCIEXP, \ > @@ -39,24 +42,10 @@ RetrainAsm1184eDownstreamPort ( > IN EFI_PCI_IO_PROTOCOL *PciIo > ) > { > - UINT16 PciVidPid[2]; > EFI_STATUS Status; > PCIE_CAP Cap; > PCI_REG_PCIE_LINK_CONTROL LinkControl; > > - Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, PCI_VENDOR_ID_OFFSET, > - ARRAY_SIZE (PciVidPid), &PciVidPid); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_WARN, "%a: failed to read PCI vendor/product ID - %r\n", > - __FUNCTION__, Status)); > - return; > - } > - > - if (PciVidPid[0] != ASMEDIA_VID || > - (PciVidPid[1] != ASM1182E_PID && PciVidPid[1] != ASM1184E_PID)) { > - return; > - } > - > // > // The upstream and downstream ports share the same PID/VID, so check > // the port type. This assumes the PCIe Express capability block lives > @@ -91,6 +80,34 @@ RetrainAsm1184eDownstreamPort ( > > STATIC > VOID > +EnableAsm1061SpreadSpectrum ( > + IN EFI_PCI_IO_PROTOCOL *PciIo > + ) > +{ > + EFI_STATUS Status; > + UINT8 SscVal; > + > + DEBUG ((DEBUG_INFO, "%a: enabling spread spectrum mode 0 for ASM1061\n", > + __FUNCTION__)); > + > + // SSC mode 0~-4000 ppm, 1:1 modulation > + > + SscVal = 0; > + Status = PciIo->Pci.Write (PciIo, EfiPciIoWidthUint8, ASM1061_SSC_OFFSET, 1, > + &SscVal); > + ASSERT_EFI_ERROR (Status); > + > + MemoryFence (); > + gBS->Stall (1); // delay at least 100 ns between writes of the same register > + > + SscVal = 1; > + Status = PciIo->Pci.Write (PciIo, EfiPciIoWidthUint8, ASM1061_SSC_OFFSET, 1, > + &SscVal); > + ASSERT_EFI_ERROR (Status); > +} > + > +STATIC > +VOID > EFIAPI > OnPciIoProtocolNotify ( > IN EFI_EVENT Event, > @@ -101,6 +118,7 @@ OnPciIoProtocolNotify ( > EFI_STATUS Status; > EFI_HANDLE HandleBuffer; > UINTN BufferSize; > + UINT16 PciVidPid[2]; > > while (TRUE) { > BufferSize = sizeof (EFI_HANDLE); > @@ -114,12 +132,33 @@ OnPciIoProtocolNotify ( > (VOID **)&PciIo); > ASSERT_EFI_ERROR (Status); > > - // > - // The ASM1184E 4-port PCIe switch on the DeveloperBox board (and its > - // 2-port sibling of which samples were used in development) needs a > - // little nudge to get it to train the downstream links at Gen2 speed. > - // > - RetrainAsm1184eDownstreamPort (PciIo); > + Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, PCI_VENDOR_ID_OFFSET, > + ARRAY_SIZE (PciVidPid), &PciVidPid); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, "%a: failed to read PCI vendor/product ID - %r\n", > + __FUNCTION__, Status)); > + continue; > + } > + > + if (PciVidPid[0] != ASMEDIA_VID) { > + continue; > + } > + > + if (PciVidPid[1] == ASM1061_PID) { > + // > + // The ASM1061 SATA controller as integrated into the DeveloperBox design > + // emits too much electromagnetic radiation. So enable spread spectrum > + // mode. > + // > + EnableAsm1061SpreadSpectrum (PciIo); > + } else if (PciVidPid[1] == ASM1182E_PID || PciVidPid[1] == ASM1184E_PID) { Does this "else if" prevent the SATA controller from training GEN2? Could it be just another if? / Leif > + // > + // The ASM1184E 4-port PCIe switch on the DeveloperBox board (and its > + // 2-port sibling of which samples were used in development) needs a > + // little nudge to get it to train the downstream links at Gen2 speed. > + // > + RetrainAsm1184eDownstreamPort (PciIo); > + } > } > } > > diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf > index 16412b999a40..64c90ac74e8e 100644 > --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf > +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf > @@ -23,7 +23,7 @@ [Defines] > ENTRY_POINT = PlatformDxeEntryPoint > > [Sources] > - Asmedia118x.c > + Asmedia.c > Emmc.c > PlatformDxe.c > PlatformDxeHii.uni > -- > 2.11.0 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 24 January 2018 at 11:10, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On Wed, Jan 24, 2018 at 10:40:52AM +0000, Ard Biesheuvel wrote: >> The ASM1061 SATA controller integrated into the DeveloperBox board >> emits too much electromagnetic radiation, so it needs spread spectrum >> mode enabled. > > I presume the spectrum mode is on the PCIe side rather than the SATA > side? Can this be explicitly mentioned? > >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/{Asmedia118x.c => Asmedia.c} | 79 +++++++++++++++----- > > Since you started renaming things, I invoke the right of bikeshedding. > > The SATA controller is only Asmedia as well due to happenstance - if > this file is becoming "various PCI tweaks", can we just call it Pci.c > (or PciTweaks.c by all means)? > Sure >> Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf | 2 +- >> 2 files changed, 60 insertions(+), 21 deletions(-) >> >> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c >> similarity index 64% >> rename from Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c >> rename to Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c >> index c4cbacd3dff9..6c289fa1892e 100644 >> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c >> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c >> @@ -15,9 +15,12 @@ >> #include "PlatformDxe.h" >> >> #define ASMEDIA_VID 0x1b21 >> +#define ASM1061_PID 0x0612 >> #define ASM1182E_PID 0x1182 >> #define ASM1184E_PID 0x1184 >> >> +#define ASM1061_SSC_OFFSET 0xA10 >> + >> #define ASM118x_PCIE_CAPABILITY_OFFSET 0x80 >> #define ASM118x_PCIE_LINK_CONTROL_OFFSET (ASM118x_PCIE_CAPABILITY_OFFSET + \ >> OFFSET_OF (PCI_CAPABILITY_PCIEXP, \ >> @@ -39,24 +42,10 @@ RetrainAsm1184eDownstreamPort ( >> IN EFI_PCI_IO_PROTOCOL *PciIo >> ) >> { >> - UINT16 PciVidPid[2]; >> EFI_STATUS Status; >> PCIE_CAP Cap; >> PCI_REG_PCIE_LINK_CONTROL LinkControl; >> >> - Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, PCI_VENDOR_ID_OFFSET, >> - ARRAY_SIZE (PciVidPid), &PciVidPid); >> - if (EFI_ERROR (Status)) { >> - DEBUG ((DEBUG_WARN, "%a: failed to read PCI vendor/product ID - %r\n", >> - __FUNCTION__, Status)); >> - return; >> - } >> - >> - if (PciVidPid[0] != ASMEDIA_VID || >> - (PciVidPid[1] != ASM1182E_PID && PciVidPid[1] != ASM1184E_PID)) { >> - return; >> - } >> - >> // >> // The upstream and downstream ports share the same PID/VID, so check >> // the port type. This assumes the PCIe Express capability block lives >> @@ -91,6 +80,34 @@ RetrainAsm1184eDownstreamPort ( >> >> STATIC >> VOID >> +EnableAsm1061SpreadSpectrum ( >> + IN EFI_PCI_IO_PROTOCOL *PciIo >> + ) >> +{ >> + EFI_STATUS Status; >> + UINT8 SscVal; >> + >> + DEBUG ((DEBUG_INFO, "%a: enabling spread spectrum mode 0 for ASM1061\n", >> + __FUNCTION__)); >> + >> + // SSC mode 0~-4000 ppm, 1:1 modulation >> + >> + SscVal = 0; >> + Status = PciIo->Pci.Write (PciIo, EfiPciIoWidthUint8, ASM1061_SSC_OFFSET, 1, >> + &SscVal); >> + ASSERT_EFI_ERROR (Status); >> + >> + MemoryFence (); >> + gBS->Stall (1); // delay at least 100 ns between writes of the same register >> + >> + SscVal = 1; >> + Status = PciIo->Pci.Write (PciIo, EfiPciIoWidthUint8, ASM1061_SSC_OFFSET, 1, >> + &SscVal); >> + ASSERT_EFI_ERROR (Status); >> +} >> + >> +STATIC >> +VOID >> EFIAPI >> OnPciIoProtocolNotify ( >> IN EFI_EVENT Event, >> @@ -101,6 +118,7 @@ OnPciIoProtocolNotify ( >> EFI_STATUS Status; >> EFI_HANDLE HandleBuffer; >> UINTN BufferSize; >> + UINT16 PciVidPid[2]; >> >> while (TRUE) { >> BufferSize = sizeof (EFI_HANDLE); >> @@ -114,12 +132,33 @@ OnPciIoProtocolNotify ( >> (VOID **)&PciIo); >> ASSERT_EFI_ERROR (Status); >> >> - // >> - // The ASM1184E 4-port PCIe switch on the DeveloperBox board (and its >> - // 2-port sibling of which samples were used in development) needs a >> - // little nudge to get it to train the downstream links at Gen2 speed. >> - // >> - RetrainAsm1184eDownstreamPort (PciIo); >> + Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, PCI_VENDOR_ID_OFFSET, >> + ARRAY_SIZE (PciVidPid), &PciVidPid); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_WARN, "%a: failed to read PCI vendor/product ID - %r\n", >> + __FUNCTION__, Status)); >> + continue; >> + } >> + >> + if (PciVidPid[0] != ASMEDIA_VID) { >> + continue; >> + } >> + >> + if (PciVidPid[1] == ASM1061_PID) { >> + // >> + // The ASM1061 SATA controller as integrated into the DeveloperBox design >> + // emits too much electromagnetic radiation. So enable spread spectrum >> + // mode. >> + // >> + EnableAsm1061SpreadSpectrum (PciIo); >> + } else if (PciVidPid[1] == ASM1182E_PID || PciVidPid[1] == ASM1184E_PID) { > > Does this "else if" prevent the SATA controller from training GEN2? > Could it be just another if? > Only if PciVidPid[1] equals ASM1061_PID and ASM1182E_PID/ASM1184E_PID at the same time. > >> + // >> + // The ASM1184E 4-port PCIe switch on the DeveloperBox board (and its >> + // 2-port sibling of which samples were used in development) needs a >> + // little nudge to get it to train the downstream links at Gen2 speed. >> + // >> + RetrainAsm1184eDownstreamPort (PciIo); >> + } >> } >> } >> >> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf >> index 16412b999a40..64c90ac74e8e 100644 >> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf >> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf >> @@ -23,7 +23,7 @@ [Defines] >> ENTRY_POINT = PlatformDxeEntryPoint >> >> [Sources] >> - Asmedia118x.c >> + Asmedia.c >> Emmc.c >> PlatformDxe.c >> PlatformDxeHii.uni >> -- >> 2.11.0 >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Wed, Jan 24, 2018 at 11:13:17AM +0000, Ard Biesheuvel wrote: > >> + if (PciVidPid[1] == ASM1061_PID) { > >> + // > >> + // The ASM1061 SATA controller as integrated into the DeveloperBox design > >> + // emits too much electromagnetic radiation. So enable spread spectrum > >> + // mode. > >> + // > >> + EnableAsm1061SpreadSpectrum (PciIo); > >> + } else if (PciVidPid[1] == ASM1182E_PID || PciVidPid[1] == ASM1184E_PID) { > > > > Does this "else if" prevent the SATA controller from training GEN2? > > Could it be just another if? > > > > Only if PciVidPid[1] equals ASM1061_PID and ASM1182E_PID/ASM1184E_PID > at the same time. Ah, gotcha. The flattened topology threw me off :) / Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c similarity index 64% rename from Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c rename to Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c index c4cbacd3dff9..6c289fa1892e 100644 --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c @@ -15,9 +15,12 @@ #include "PlatformDxe.h" #define ASMEDIA_VID 0x1b21 +#define ASM1061_PID 0x0612 #define ASM1182E_PID 0x1182 #define ASM1184E_PID 0x1184 +#define ASM1061_SSC_OFFSET 0xA10 + #define ASM118x_PCIE_CAPABILITY_OFFSET 0x80 #define ASM118x_PCIE_LINK_CONTROL_OFFSET (ASM118x_PCIE_CAPABILITY_OFFSET + \ OFFSET_OF (PCI_CAPABILITY_PCIEXP, \ @@ -39,24 +42,10 @@ RetrainAsm1184eDownstreamPort ( IN EFI_PCI_IO_PROTOCOL *PciIo ) { - UINT16 PciVidPid[2]; EFI_STATUS Status; PCIE_CAP Cap; PCI_REG_PCIE_LINK_CONTROL LinkControl; - Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, PCI_VENDOR_ID_OFFSET, - ARRAY_SIZE (PciVidPid), &PciVidPid); - if (EFI_ERROR (Status)) { - DEBUG ((DEBUG_WARN, "%a: failed to read PCI vendor/product ID - %r\n", - __FUNCTION__, Status)); - return; - } - - if (PciVidPid[0] != ASMEDIA_VID || - (PciVidPid[1] != ASM1182E_PID && PciVidPid[1] != ASM1184E_PID)) { - return; - } - // // The upstream and downstream ports share the same PID/VID, so check // the port type. This assumes the PCIe Express capability block lives @@ -91,6 +80,34 @@ RetrainAsm1184eDownstreamPort ( STATIC VOID +EnableAsm1061SpreadSpectrum ( + IN EFI_PCI_IO_PROTOCOL *PciIo + ) +{ + EFI_STATUS Status; + UINT8 SscVal; + + DEBUG ((DEBUG_INFO, "%a: enabling spread spectrum mode 0 for ASM1061\n", + __FUNCTION__)); + + // SSC mode 0~-4000 ppm, 1:1 modulation + + SscVal = 0; + Status = PciIo->Pci.Write (PciIo, EfiPciIoWidthUint8, ASM1061_SSC_OFFSET, 1, + &SscVal); + ASSERT_EFI_ERROR (Status); + + MemoryFence (); + gBS->Stall (1); // delay at least 100 ns between writes of the same register + + SscVal = 1; + Status = PciIo->Pci.Write (PciIo, EfiPciIoWidthUint8, ASM1061_SSC_OFFSET, 1, + &SscVal); + ASSERT_EFI_ERROR (Status); +} + +STATIC +VOID EFIAPI OnPciIoProtocolNotify ( IN EFI_EVENT Event, @@ -101,6 +118,7 @@ OnPciIoProtocolNotify ( EFI_STATUS Status; EFI_HANDLE HandleBuffer; UINTN BufferSize; + UINT16 PciVidPid[2]; while (TRUE) { BufferSize = sizeof (EFI_HANDLE); @@ -114,12 +132,33 @@ OnPciIoProtocolNotify ( (VOID **)&PciIo); ASSERT_EFI_ERROR (Status); - // - // The ASM1184E 4-port PCIe switch on the DeveloperBox board (and its - // 2-port sibling of which samples were used in development) needs a - // little nudge to get it to train the downstream links at Gen2 speed. - // - RetrainAsm1184eDownstreamPort (PciIo); + Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, PCI_VENDOR_ID_OFFSET, + ARRAY_SIZE (PciVidPid), &PciVidPid); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_WARN, "%a: failed to read PCI vendor/product ID - %r\n", + __FUNCTION__, Status)); + continue; + } + + if (PciVidPid[0] != ASMEDIA_VID) { + continue; + } + + if (PciVidPid[1] == ASM1061_PID) { + // + // The ASM1061 SATA controller as integrated into the DeveloperBox design + // emits too much electromagnetic radiation. So enable spread spectrum + // mode. + // + EnableAsm1061SpreadSpectrum (PciIo); + } else if (PciVidPid[1] == ASM1182E_PID || PciVidPid[1] == ASM1184E_PID) { + // + // The ASM1184E 4-port PCIe switch on the DeveloperBox board (and its + // 2-port sibling of which samples were used in development) needs a + // little nudge to get it to train the downstream links at Gen2 speed. + // + RetrainAsm1184eDownstreamPort (PciIo); + } } } diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf index 16412b999a40..64c90ac74e8e 100644 --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf @@ -23,7 +23,7 @@ [Defines] ENTRY_POINT = PlatformDxeEntryPoint [Sources] - Asmedia118x.c + Asmedia.c Emmc.c PlatformDxe.c PlatformDxeHii.uni
The ASM1061 SATA controller integrated into the DeveloperBox board emits too much electromagnetic radiation, so it needs spread spectrum mode enabled. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/{Asmedia118x.c => Asmedia.c} | 79 +++++++++++++++----- Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf | 2 +- 2 files changed, 60 insertions(+), 21 deletions(-) -- 2.11.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel