public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is set
@ 2018-09-11  2:03 Star Zeng
  2018-09-11  2:23 ` Ni, Ruiyu
  0 siblings, 1 reply; 7+ messages in thread
From: Star Zeng @ 2018-09-11  2:03 UTC (permalink / raw)
  To: edk2-devel; +Cc: Star Zeng, Ruiyu Ni, Jian J Wang, Fei1 Wang

When the HSEE in the USBCMD bit is a ‘1’ and the HSE bit in the USBSTS
register is a ‘1’, the xHC shall assert out-of-band error signaling to
the host and assert the SERR# pin.
To prevent masking any potential issues with SERR, this patch is to set
USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable Bit is
set.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Fei1 Wang <fei1.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 41 ++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
index 5f0736a516b6..89f073e1d83f 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
@@ -587,6 +587,39 @@ XhcIsSysError (
 }
 
 /**
+  Set USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable Bit is set.
+
+  The USBCMD HSEE Bit will be reset to default 0 by USBCMD Host Controller Reset(HCRST).
+  This function is to set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
+
+  @param Xhc            The XHCI Instance.
+
+**/
+VOID
+XhcSetHsee (
+  IN USB_XHCI_INSTANCE  *Xhc
+  )
+{
+  EFI_STATUS            Status;
+  EFI_PCI_IO_PROTOCOL   *PciIo;
+  UINT16                XhciCmd;
+
+  PciIo = Xhc->PciIo;
+  Status = PciIo->Pci.Read (
+                        PciIo,
+                        EfiPciIoWidthUint16,
+                        PCI_COMMAND_OFFSET,
+                        sizeof (XhciCmd),
+                        &XhciCmd
+                        );
+  if (!EFI_ERROR (Status)) {
+    if ((XhciCmd & EFI_PCI_COMMAND_SERR) != 0) {
+      XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
+    }
+  }
+}
+
+/**
   Reset the XHCI host controller.
 
   @param  Xhc          The XHCI Instance.
@@ -628,6 +661,14 @@ XhcResetHC (
     //
     gBS->Stall (XHC_1_MILLISECOND);
     Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_RESET, FALSE, Timeout);
+
+    if (!EFI_ERROR (Status)) {
+      //
+      // The USBCMD HSEE Bit will be reset to default 0 by USBCMD HCRST.
+      // Set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
+      //
+      XhcSetHsee (Xhc);
+    }
   }
 
   return Status;
-- 
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is set
  2018-09-11  2:03 [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is set Star Zeng
@ 2018-09-11  2:23 ` Ni, Ruiyu
  2018-09-25 15:41   ` Marcin Wojtas
  0 siblings, 1 reply; 7+ messages in thread
From: Ni, Ruiyu @ 2018-09-11  2:23 UTC (permalink / raw)
  To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Wang, Jian J, Wang, Fei1

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, September 11, 2018 10:04 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Wang, Fei1 <fei1.wang@intel.com>
> Subject: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is
> set
> 
> When the HSEE in the USBCMD bit is a '1' and the HSE bit in the USBSTS
> register is a '1', the xHC shall assert out-of-band error signaling to the host
> and assert the SERR# pin.
> To prevent masking any potential issues with SERR, this patch is to set
> USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable Bit is
> set.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Fei1 Wang <fei1.wang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 41
> ++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> index 5f0736a516b6..89f073e1d83f 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> @@ -587,6 +587,39 @@ XhcIsSysError (
>  }
> 
>  /**
> +  Set USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable
> Bit is set.
> +
> +  The USBCMD HSEE Bit will be reset to default 0 by USBCMD Host Controller
> Reset(HCRST).
> +  This function is to set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> +
> +  @param Xhc            The XHCI Instance.
> +
> +**/
> +VOID
> +XhcSetHsee (
> +  IN USB_XHCI_INSTANCE  *Xhc
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PCI_IO_PROTOCOL   *PciIo;
> +  UINT16                XhciCmd;
> +
> +  PciIo = Xhc->PciIo;
> +  Status = PciIo->Pci.Read (
> +                        PciIo,
> +                        EfiPciIoWidthUint16,
> +                        PCI_COMMAND_OFFSET,
> +                        sizeof (XhciCmd),
> +                        &XhciCmd
> +                        );
> +  if (!EFI_ERROR (Status)) {
> +    if ((XhciCmd & EFI_PCI_COMMAND_SERR) != 0) {
> +      XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
> +    }
> +  }
> +}
> +
> +/**
>    Reset the XHCI host controller.
> 
>    @param  Xhc          The XHCI Instance.
> @@ -628,6 +661,14 @@ XhcResetHC (
>      //
>      gBS->Stall (XHC_1_MILLISECOND);
>      Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET,
> XHC_USBCMD_RESET, FALSE, Timeout);
> +
> +    if (!EFI_ERROR (Status)) {
> +      //
> +      // The USBCMD HSEE Bit will be reset to default 0 by USBCMD HCRST.
> +      // Set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> +      //
> +      XhcSetHsee (Xhc);
> +    }
>    }
> 
>    return Status;
> --
> 2.7.0.windows.1



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is set
  2018-09-11  2:23 ` Ni, Ruiyu
@ 2018-09-25 15:41   ` Marcin Wojtas
  2018-09-25 15:51     ` Ard Biesheuvel
  2018-09-25 16:25     ` Andrew Fish
  0 siblings, 2 replies; 7+ messages in thread
From: Marcin Wojtas @ 2018-09-25 15:41 UTC (permalink / raw)
  To: Zeng, Star, Ard Biesheuvel
  Cc: edk2-devel-01, Ni, Ruiyu, fei1.wang, Grzegorz Jaszczyk, nadavh

Hi Star, Ard

With this patch, my platforms which use NonDiscoverableDevices layer
for supporting generic Xhci controller, fail in a strange way:
"Synchronous Exception at 0x000000003F910AFC
PC 0x00003F910AFC (0x00003F908000+0x00008AFC) [ 0] DxeCore.dll
PC 0x00003F910AE0 (0x00003F908000+0x00008AE0) [ 0] DxeCore.dll
PC 0x00003F91BDF4 (0x00003F908000+0x00013DF4) [ 0] DxeCore.dll
PC 0x0000BF5BD000 (0x0000BF5AF000+0x0000E000) [ 1] XhciDxe.dll
PC 0xAFAFAFAFAFAFAFAF

Recursive exception occurred while dumping the CPU state"

I've quickly checked and although XhcSetHsee() is eventually called
from XhcDriverBindingStart() sequence,
below line is not even executed:
XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
The XhcDriverBindingStart() returns EFI_SUCCESS and we get the sync
abort right afterwards (haven't found exact place yet).

What makes the difference is commenting out in XhcSetHsee():
//  Status = PciIo->Pci.Read (
//                        PciIo,
//                       EfiPciIoWidthUint16,
//                        PCI_COMMAND_OFFSET,
//                       sizeof (XhciCmd),
//                        &XhciCmd
//                        );

With that everything keeps working as usual. I'd appreciate any hint.

Best regards.
Marcin

wt., 11 wrz 2018 o 04:30 Ni, Ruiyu <ruiyu.ni@intel.com> napisał(a):
>
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thanks/Ray
>
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Tuesday, September 11, 2018 10:04 AM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.zeng@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Wang,
> > Jian J <jian.j.wang@intel.com>; Wang, Fei1 <fei1.wang@intel.com>
> > Subject: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is
> > set
> >
> > When the HSEE in the USBCMD bit is a '1' and the HSE bit in the USBSTS
> > register is a '1', the xHC shall assert out-of-band error signaling to the host
> > and assert the SERR# pin.
> > To prevent masking any potential issues with SERR, this patch is to set
> > USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable Bit is
> > set.
> >
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Fei1 Wang <fei1.wang@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > ---
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 41
> > ++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > index 5f0736a516b6..89f073e1d83f 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > @@ -587,6 +587,39 @@ XhcIsSysError (
> >  }
> >
> >  /**
> > +  Set USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable
> > Bit is set.
> > +
> > +  The USBCMD HSEE Bit will be reset to default 0 by USBCMD Host Controller
> > Reset(HCRST).
> > +  This function is to set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> > +
> > +  @param Xhc            The XHCI Instance.
> > +
> > +**/
> > +VOID
> > +XhcSetHsee (
> > +  IN USB_XHCI_INSTANCE  *Xhc
> > +  )
> > +{
> > +  EFI_STATUS            Status;
> > +  EFI_PCI_IO_PROTOCOL   *PciIo;
> > +  UINT16                XhciCmd;
> > +
> > +  PciIo = Xhc->PciIo;
> > +  Status = PciIo->Pci.Read (
> > +                        PciIo,
> > +                        EfiPciIoWidthUint16,
> > +                        PCI_COMMAND_OFFSET,
> > +                        sizeof (XhciCmd),
> > +                        &XhciCmd
> > +                        );
> > +  if (!EFI_ERROR (Status)) {
> > +    if ((XhciCmd & EFI_PCI_COMMAND_SERR) != 0) {
> > +      XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
> > +    }
> > +  }
> > +}
> > +
> > +/**
> >    Reset the XHCI host controller.
> >
> >    @param  Xhc          The XHCI Instance.
> > @@ -628,6 +661,14 @@ XhcResetHC (
> >      //
> >      gBS->Stall (XHC_1_MILLISECOND);
> >      Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET,
> > XHC_USBCMD_RESET, FALSE, Timeout);
> > +
> > +    if (!EFI_ERROR (Status)) {
> > +      //
> > +      // The USBCMD HSEE Bit will be reset to default 0 by USBCMD HCRST.
> > +      // Set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> > +      //
> > +      XhcSetHsee (Xhc);
> > +    }
> >    }
> >
> >    return Status;
> > --
> > 2.7.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is set
  2018-09-25 15:41   ` Marcin Wojtas
@ 2018-09-25 15:51     ` Ard Biesheuvel
  2018-09-25 16:19       ` Ard Biesheuvel
  2018-09-25 16:25     ` Andrew Fish
  1 sibling, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2018-09-25 15:51 UTC (permalink / raw)
  To: Marcin Wojtas
  Cc: Zeng, Star, edk2-devel@lists.01.org, Ruiyu Ni, fei1.wang,
	Grzegorz Jaszczyk, Nadav Haklai

On Tue, 25 Sep 2018 at 17:41, Marcin Wojtas <mw@semihalf.com> wrote:
>
> Hi Star, Ard
>
> With this patch, my platforms which use NonDiscoverableDevices layer
> for supporting generic Xhci controller, fail in a strange way:
> "Synchronous Exception at 0x000000003F910AFC
> PC 0x00003F910AFC (0x00003F908000+0x00008AFC) [ 0] DxeCore.dll
> PC 0x00003F910AE0 (0x00003F908000+0x00008AE0) [ 0] DxeCore.dll
> PC 0x00003F91BDF4 (0x00003F908000+0x00013DF4) [ 0] DxeCore.dll
> PC 0x0000BF5BD000 (0x0000BF5AF000+0x0000E000) [ 1] XhciDxe.dll
> PC 0xAFAFAFAFAFAFAFAF
>
> Recursive exception occurred while dumping the CPU state"
>
> I've quickly checked and although XhcSetHsee() is eventually called
> from XhcDriverBindingStart() sequence,
> below line is not even executed:
> XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
> The XhcDriverBindingStart() returns EFI_SUCCESS and we get the sync
> abort right afterwards (haven't found exact place yet).
>

FYI I am seeing something similar with a Renesas uPD70201.

> What makes the difference is commenting out in XhcSetHsee():
> //  Status = PciIo->Pci.Read (
> //                        PciIo,
> //                       EfiPciIoWidthUint16,
> //                        PCI_COMMAND_OFFSET,
> //                       sizeof (XhciCmd),
> //                        &XhciCmd
> //                        );
>
> With that everything keeps working as usual. I'd appreciate any hint.
>
> Best regards.
> Marcin
>
> wt., 11 wrz 2018 o 04:30 Ni, Ruiyu <ruiyu.ni@intel.com> napisał(a):
> >
> > Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> >
> > Thanks/Ray
> >
> > > -----Original Message-----
> > > From: Zeng, Star
> > > Sent: Tuesday, September 11, 2018 10:04 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Zeng, Star <star.zeng@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Wang,
> > > Jian J <jian.j.wang@intel.com>; Wang, Fei1 <fei1.wang@intel.com>
> > > Subject: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is
> > > set
> > >
> > > When the HSEE in the USBCMD bit is a '1' and the HSE bit in the USBSTS
> > > register is a '1', the xHC shall assert out-of-band error signaling to the host
> > > and assert the SERR# pin.
> > > To prevent masking any potential issues with SERR, this patch is to set
> > > USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable Bit is
> > > set.
> > >
> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Fei1 Wang <fei1.wang@intel.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > > ---
> > >  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 41
> > > ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 41 insertions(+)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > index 5f0736a516b6..89f073e1d83f 100644
> > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > @@ -587,6 +587,39 @@ XhcIsSysError (
> > >  }
> > >
> > >  /**
> > > +  Set USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable
> > > Bit is set.
> > > +
> > > +  The USBCMD HSEE Bit will be reset to default 0 by USBCMD Host Controller
> > > Reset(HCRST).
> > > +  This function is to set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> > > +
> > > +  @param Xhc            The XHCI Instance.
> > > +
> > > +**/
> > > +VOID
> > > +XhcSetHsee (
> > > +  IN USB_XHCI_INSTANCE  *Xhc
> > > +  )
> > > +{
> > > +  EFI_STATUS            Status;
> > > +  EFI_PCI_IO_PROTOCOL   *PciIo;
> > > +  UINT16                XhciCmd;
> > > +
> > > +  PciIo = Xhc->PciIo;
> > > +  Status = PciIo->Pci.Read (
> > > +                        PciIo,
> > > +                        EfiPciIoWidthUint16,
> > > +                        PCI_COMMAND_OFFSET,
> > > +                        sizeof (XhciCmd),
> > > +                        &XhciCmd
> > > +                        );
> > > +  if (!EFI_ERROR (Status)) {
> > > +    if ((XhciCmd & EFI_PCI_COMMAND_SERR) != 0) {
> > > +      XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
> > > +    }
> > > +  }
> > > +}
> > > +
> > > +/**
> > >    Reset the XHCI host controller.
> > >
> > >    @param  Xhc          The XHCI Instance.
> > > @@ -628,6 +661,14 @@ XhcResetHC (
> > >      //
> > >      gBS->Stall (XHC_1_MILLISECOND);
> > >      Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET,
> > > XHC_USBCMD_RESET, FALSE, Timeout);
> > > +
> > > +    if (!EFI_ERROR (Status)) {
> > > +      //
> > > +      // The USBCMD HSEE Bit will be reset to default 0 by USBCMD HCRST.
> > > +      // Set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> > > +      //
> > > +      XhcSetHsee (Xhc);
> > > +    }
> > >    }
> > >
> > >    return Status;
> > > --
> > > 2.7.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is set
  2018-09-25 15:51     ` Ard Biesheuvel
@ 2018-09-25 16:19       ` Ard Biesheuvel
  0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2018-09-25 16:19 UTC (permalink / raw)
  To: Marcin Wojtas
  Cc: Zeng, Star, edk2-devel@lists.01.org, Ruiyu Ni, fei1.wang,
	Grzegorz Jaszczyk, Nadav Haklai

On Tue, 25 Sep 2018 at 17:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Tue, 25 Sep 2018 at 17:41, Marcin Wojtas <mw@semihalf.com> wrote:
> >
> > Hi Star, Ard
> >
> > With this patch, my platforms which use NonDiscoverableDevices layer
> > for supporting generic Xhci controller, fail in a strange way:
> > "Synchronous Exception at 0x000000003F910AFC
> > PC 0x00003F910AFC (0x00003F908000+0x00008AFC) [ 0] DxeCore.dll
> > PC 0x00003F910AE0 (0x00003F908000+0x00008AE0) [ 0] DxeCore.dll
> > PC 0x00003F91BDF4 (0x00003F908000+0x00013DF4) [ 0] DxeCore.dll
> > PC 0x0000BF5BD000 (0x0000BF5AF000+0x0000E000) [ 1] XhciDxe.dll
> > PC 0xAFAFAFAFAFAFAFAF
> >
> > Recursive exception occurred while dumping the CPU state"
> >
> > I've quickly checked and although XhcSetHsee() is eventually called
> > from XhcDriverBindingStart() sequence,
> > below line is not even executed:
> > XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
> > The XhcDriverBindingStart() returns EFI_SUCCESS and we get the sync
> > abort right afterwards (haven't found exact place yet).
> >
>
> FYI I am seeing something similar with a Renesas uPD70201.
>

... which notably is a PCI device. I need to dig into this further,
I'll try to add more context tomorrow.


> > What makes the difference is commenting out in XhcSetHsee():
> > //  Status = PciIo->Pci.Read (
> > //                        PciIo,
> > //                       EfiPciIoWidthUint16,
> > //                        PCI_COMMAND_OFFSET,
> > //                       sizeof (XhciCmd),
> > //                        &XhciCmd
> > //                        );
> >
> > With that everything keeps working as usual. I'd appreciate any hint.
> >
> > Best regards.
> > Marcin
> >
> > wt., 11 wrz 2018 o 04:30 Ni, Ruiyu <ruiyu.ni@intel.com> napisał(a):
> > >
> > > Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > >
> > > Thanks/Ray
> > >
> > > > -----Original Message-----
> > > > From: Zeng, Star
> > > > Sent: Tuesday, September 11, 2018 10:04 AM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Zeng, Star <star.zeng@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Wang,
> > > > Jian J <jian.j.wang@intel.com>; Wang, Fei1 <fei1.wang@intel.com>
> > > > Subject: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is
> > > > set
> > > >
> > > > When the HSEE in the USBCMD bit is a '1' and the HSE bit in the USBSTS
> > > > register is a '1', the xHC shall assert out-of-band error signaling to the host
> > > > and assert the SERR# pin.
> > > > To prevent masking any potential issues with SERR, this patch is to set
> > > > USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable Bit is
> > > > set.
> > > >
> > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > Cc: Fei1 Wang <fei1.wang@intel.com>
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > > > ---
> > > >  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 41
> > > > ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 41 insertions(+)
> > > >
> > > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > index 5f0736a516b6..89f073e1d83f 100644
> > > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > @@ -587,6 +587,39 @@ XhcIsSysError (
> > > >  }
> > > >
> > > >  /**
> > > > +  Set USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable
> > > > Bit is set.
> > > > +
> > > > +  The USBCMD HSEE Bit will be reset to default 0 by USBCMD Host Controller
> > > > Reset(HCRST).
> > > > +  This function is to set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> > > > +
> > > > +  @param Xhc            The XHCI Instance.
> > > > +
> > > > +**/
> > > > +VOID
> > > > +XhcSetHsee (
> > > > +  IN USB_XHCI_INSTANCE  *Xhc
> > > > +  )
> > > > +{
> > > > +  EFI_STATUS            Status;
> > > > +  EFI_PCI_IO_PROTOCOL   *PciIo;
> > > > +  UINT16                XhciCmd;
> > > > +
> > > > +  PciIo = Xhc->PciIo;
> > > > +  Status = PciIo->Pci.Read (
> > > > +                        PciIo,
> > > > +                        EfiPciIoWidthUint16,
> > > > +                        PCI_COMMAND_OFFSET,
> > > > +                        sizeof (XhciCmd),
> > > > +                        &XhciCmd
> > > > +                        );
> > > > +  if (!EFI_ERROR (Status)) {
> > > > +    if ((XhciCmd & EFI_PCI_COMMAND_SERR) != 0) {
> > > > +      XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
> > > > +    }
> > > > +  }
> > > > +}
> > > > +
> > > > +/**
> > > >    Reset the XHCI host controller.
> > > >
> > > >    @param  Xhc          The XHCI Instance.
> > > > @@ -628,6 +661,14 @@ XhcResetHC (
> > > >      //
> > > >      gBS->Stall (XHC_1_MILLISECOND);
> > > >      Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET,
> > > > XHC_USBCMD_RESET, FALSE, Timeout);
> > > > +
> > > > +    if (!EFI_ERROR (Status)) {
> > > > +      //
> > > > +      // The USBCMD HSEE Bit will be reset to default 0 by USBCMD HCRST.
> > > > +      // Set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> > > > +      //
> > > > +      XhcSetHsee (Xhc);
> > > > +    }
> > > >    }
> > > >
> > > >    return Status;
> > > > --
> > > > 2.7.0.windows.1
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is set
  2018-09-25 15:41   ` Marcin Wojtas
  2018-09-25 15:51     ` Ard Biesheuvel
@ 2018-09-25 16:25     ` Andrew Fish
  2018-09-25 17:29       ` Marcin Wojtas
  1 sibling, 1 reply; 7+ messages in thread
From: Andrew Fish @ 2018-09-25 16:25 UTC (permalink / raw)
  To: Marcin Wojtas
  Cc: Zeng, Star, Ard Biesheuvel, Ni, Ruiyu, Grzegorz Jaszczyk,
	edk2-devel-01, nadavh, fei1.wang



> On Sep 25, 2018, at 8:41 AM, Marcin Wojtas <mw@semihalf.com> wrote:
> 
> Hi Star, Ard
> 
> With this patch, my platforms which use NonDiscoverableDevices layer
> for supporting generic Xhci controller, fail in a strange way:
> "Synchronous Exception at 0x000000003F910AFC
> PC 0x00003F910AFC (0x00003F908000+0x00008AFC) [ 0] DxeCore.dll
> PC 0x00003F910AE0 (0x00003F908000+0x00008AE0) [ 0] DxeCore.dll
> PC 0x00003F91BDF4 (0x00003F908000+0x00013DF4) [ 0] DxeCore.dll
> PC 0x0000BF5BD000 (0x0000BF5AF000+0x0000E000) [ 1] XhciDxe.dll
> PC 0xAFAFAFAFAFAFAFAF
> 

Marcin,

A lot of times 0xAFAFAFAFAFAFAFAF is related to using freed memory. 

See this PCD and its use in the DebugLib:
MdePkg.dec:2168:  gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue|0xAF|UINT8|0x00000008

On an X86 system 0xAFAFAFAFAFAFAFAF is a non canonical address and it will generate a general Protection Fault exception 13 (0xD) on access. 

Thanks,

Andrew Fish


> Recursive exception occurred while dumping the CPU state"
> 
> I've quickly checked and although XhcSetHsee() is eventually called
> from XhcDriverBindingStart() sequence,
> below line is not even executed:
> XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
> The XhcDriverBindingStart() returns EFI_SUCCESS and we get the sync
> abort right afterwards (haven't found exact place yet).
> 
> What makes the difference is commenting out in XhcSetHsee():
> //  Status = PciIo->Pci.Read (
> //                        PciIo,
> //                       EfiPciIoWidthUint16,
> //                        PCI_COMMAND_OFFSET,
> //                       sizeof (XhciCmd),
> //                        &XhciCmd
> //                        );
> 
> With that everything keeps working as usual. I'd appreciate any hint.
> 
> Best regards.
> Marcin
> 
> wt., 11 wrz 2018 o 04:30 Ni, Ruiyu <ruiyu.ni@intel.com <mailto:ruiyu.ni@intel.com>> napisał(a):
>> 
>> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> 
>> Thanks/Ray
>> 
>>> -----Original Message-----
>>> From: Zeng, Star
>>> Sent: Tuesday, September 11, 2018 10:04 AM
>>> To: edk2-devel@lists.01.org
>>> Cc: Zeng, Star <star.zeng@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Wang,
>>> Jian J <jian.j.wang@intel.com>; Wang, Fei1 <fei1.wang@intel.com>
>>> Subject: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is
>>> set
>>> 
>>> When the HSEE in the USBCMD bit is a '1' and the HSE bit in the USBSTS
>>> register is a '1', the xHC shall assert out-of-band error signaling to the host
>>> and assert the SERR# pin.
>>> To prevent masking any potential issues with SERR, this patch is to set
>>> USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable Bit is
>>> set.
>>> 
>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Fei1 Wang <fei1.wang@intel.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>>> ---
>>> MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 41
>>> ++++++++++++++++++++++++++++++++++
>>> 1 file changed, 41 insertions(+)
>>> 
>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>> index 5f0736a516b6..89f073e1d83f 100644
>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>> @@ -587,6 +587,39 @@ XhcIsSysError (
>>> }
>>> 
>>> /**
>>> +  Set USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable
>>> Bit is set.
>>> +
>>> +  The USBCMD HSEE Bit will be reset to default 0 by USBCMD Host Controller
>>> Reset(HCRST).
>>> +  This function is to set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
>>> +
>>> +  @param Xhc            The XHCI Instance.
>>> +
>>> +**/
>>> +VOID
>>> +XhcSetHsee (
>>> +  IN USB_XHCI_INSTANCE  *Xhc
>>> +  )
>>> +{
>>> +  EFI_STATUS            Status;
>>> +  EFI_PCI_IO_PROTOCOL   *PciIo;
>>> +  UINT16                XhciCmd;
>>> +
>>> +  PciIo = Xhc->PciIo;
>>> +  Status = PciIo->Pci.Read (
>>> +                        PciIo,
>>> +                        EfiPciIoWidthUint16,
>>> +                        PCI_COMMAND_OFFSET,
>>> +                        sizeof (XhciCmd),
>>> +                        &XhciCmd
>>> +                        );
>>> +  if (!EFI_ERROR (Status)) {
>>> +    if ((XhciCmd & EFI_PCI_COMMAND_SERR) != 0) {
>>> +      XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
>>> +    }
>>> +  }
>>> +}
>>> +
>>> +/**
>>>   Reset the XHCI host controller.
>>> 
>>>   @param  Xhc          The XHCI Instance.
>>> @@ -628,6 +661,14 @@ XhcResetHC (
>>>     //
>>>     gBS->Stall (XHC_1_MILLISECOND);
>>>     Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET,
>>> XHC_USBCMD_RESET, FALSE, Timeout);
>>> +
>>> +    if (!EFI_ERROR (Status)) {
>>> +      //
>>> +      // The USBCMD HSEE Bit will be reset to default 0 by USBCMD HCRST.
>>> +      // Set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
>>> +      //
>>> +      XhcSetHsee (Xhc);
>>> +    }
>>>   }
>>> 
>>>   return Status;
>>> --
>>> 2.7.0.windows.1
>> 
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is set
  2018-09-25 16:25     ` Andrew Fish
@ 2018-09-25 17:29       ` Marcin Wojtas
  0 siblings, 0 replies; 7+ messages in thread
From: Marcin Wojtas @ 2018-09-25 17:29 UTC (permalink / raw)
  To: Andrew Fish
  Cc: Zeng, Star, Ard Biesheuvel, Ni, Ruiyu, Grzegorz Jaszczyk,
	edk2-devel-01, nadavh, fei1.wang

Hi Andrew,

wt., 25 wrz 2018 o 18:25 Andrew Fish <afish@apple.com> napisał(a):
>
>
>
> On Sep 25, 2018, at 8:41 AM, Marcin Wojtas <mw@semihalf.com> wrote:
>
> Hi Star, Ard
>
> With this patch, my platforms which use NonDiscoverableDevices layer
> for supporting generic Xhci controller, fail in a strange way:
> "Synchronous Exception at 0x000000003F910AFC
> PC 0x00003F910AFC (0x00003F908000+0x00008AFC) [ 0] DxeCore.dll
> PC 0x00003F910AE0 (0x00003F908000+0x00008AE0) [ 0] DxeCore.dll
> PC 0x00003F91BDF4 (0x00003F908000+0x00013DF4) [ 0] DxeCore.dll
> PC 0x0000BF5BD000 (0x0000BF5AF000+0x0000E000) [ 1] XhciDxe.dll
> PC 0xAFAFAFAFAFAFAFAF
>
>
> Marcin,
>
> A lot of times 0xAFAFAFAFAFAFAFAF is related to using freed memory.
>
> See this PCD and its use in the DebugLib:
> MdePkg.dec:2168:  gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue|0xAF|UINT8|0x00000008
>
> On an X86 system 0xAFAFAFAFAFAFAFAF is a non canonical address and it will generate a general Protection Fault exception 13 (0xD) on access.
>

Thanks for the hint. I found a root cause of this behavior. With
EfiPciIoWidthUint16 and sizeof (XhciCmd), we read in fact 4 bytes from
&XhciCmd, which is UINT16...
We can fix it in a couple of ways, but I'll create bugzilla entry and
submit below fix later today:

--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
@@ -607,7 +607,7 @@ XhcSetHsee (
   PciIo = Xhc->PciIo;
   Status = PciIo->Pci.Read (
                         PciIo,
-                        EfiPciIoWidthUint16,
+                        EfiPciIoWidthUint8,
                         PCI_COMMAND_OFFSET,
                         sizeof (XhciCmd),
                         &XhciCmd

Best regards,
Marcin

>
>
> Recursive exception occurred while dumping the CPU state"
>
> I've quickly checked and although XhcSetHsee() is eventually called
> from XhcDriverBindingStart() sequence,
> below line is not even executed:
> XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
> The XhcDriverBindingStart() returns EFI_SUCCESS and we get the sync
> abort right afterwards (haven't found exact place yet).
>
> What makes the difference is commenting out in XhcSetHsee():
> //  Status = PciIo->Pci.Read (
> //                        PciIo,
> //                       EfiPciIoWidthUint16,
> //                        PCI_COMMAND_OFFSET,
> //                       sizeof (XhciCmd),
> //                        &XhciCmd
> //                        );
>
> With that everything keeps working as usual. I'd appreciate any hint.
>
> Best regards.
> Marcin
>
> wt., 11 wrz 2018 o 04:30 Ni, Ruiyu <ruiyu.ni@intel.com> napisał(a):
>
>
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thanks/Ray
>
> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, September 11, 2018 10:04 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Wang, Fei1 <fei1.wang@intel.com>
> Subject: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is
> set
>
> When the HSEE in the USBCMD bit is a '1' and the HSE bit in the USBSTS
> register is a '1', the xHC shall assert out-of-band error signaling to the host
> and assert the SERR# pin.
> To prevent masking any potential issues with SERR, this patch is to set
> USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable Bit is
> set.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Fei1 Wang <fei1.wang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
> MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 41
> ++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> index 5f0736a516b6..89f073e1d83f 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> @@ -587,6 +587,39 @@ XhcIsSysError (
> }
>
> /**
> +  Set USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable
> Bit is set.
> +
> +  The USBCMD HSEE Bit will be reset to default 0 by USBCMD Host Controller
> Reset(HCRST).
> +  This function is to set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> +
> +  @param Xhc            The XHCI Instance.
> +
> +**/
> +VOID
> +XhcSetHsee (
> +  IN USB_XHCI_INSTANCE  *Xhc
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PCI_IO_PROTOCOL   *PciIo;
> +  UINT16                XhciCmd;
> +
> +  PciIo = Xhc->PciIo;
> +  Status = PciIo->Pci.Read (
> +                        PciIo,
> +                        EfiPciIoWidthUint16,
> +                        PCI_COMMAND_OFFSET,
> +                        sizeof (XhciCmd),
> +                        &XhciCmd
> +                        );
> +  if (!EFI_ERROR (Status)) {
> +    if ((XhciCmd & EFI_PCI_COMMAND_SERR) != 0) {
> +      XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
> +    }
> +  }
> +}
> +
> +/**
>   Reset the XHCI host controller.
>
>   @param  Xhc          The XHCI Instance.
> @@ -628,6 +661,14 @@ XhcResetHC (
>     //
>     gBS->Stall (XHC_1_MILLISECOND);
>     Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET,
> XHC_USBCMD_RESET, FALSE, Timeout);
> +
> +    if (!EFI_ERROR (Status)) {
> +      //
> +      // The USBCMD HSEE Bit will be reset to default 0 by USBCMD HCRST.
> +      // Set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> +      //
> +      XhcSetHsee (Xhc);
> +    }
>   }
>
>   return Status;
> --
> 2.7.0.windows.1
>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
>


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-09-25 17:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-11  2:03 [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is set Star Zeng
2018-09-11  2:23 ` Ni, Ruiyu
2018-09-25 15:41   ` Marcin Wojtas
2018-09-25 15:51     ` Ard Biesheuvel
2018-09-25 16:19       ` Ard Biesheuvel
2018-09-25 16:25     ` Andrew Fish
2018-09-25 17:29       ` Marcin Wojtas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox