Re: [RFC] SATA : Implemented NXP errata A008402

[Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>]

Hi,

We will set PCD value to 0 to support our board and its default value is set to maximum data length.

If someone change its value to any other value in their package, then he/she must be aware of the consequences.

I cannot say how controller will react if value is other than 0x3f_ffff and 0 (in our case).

Thanks,
Meenakshi

> -----Original Message-----
> From: Zeng, Star [mailto:star.zeng@intel.com]
> Sent: Monday, January 08, 2018 3:54 PM
> To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; edk2-
> devel@lists.01.org; Dong, Eric <eric.dong@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [RFC] SATA : Implemented NXP errata A008402
> 
> How will the code work based on your patch if the this PCD is configured to
> other value, for example 0x200000/0x300000?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com]
> Sent: Monday, January 8, 2018 5:54 PM
> To: Zeng, Star <star.zeng@intel.com>; ard.biesheuvel@linaro.org;
> leif.lindholm@linaro.org; edk2-devel@lists.01.org; Dong, Eric
> <eric.dong@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: RE: [edk2] [RFC] SATA : Implemented NXP errata A008402
> 
> 
> > -----Original Message-----
> > From: Zeng, Star [mailto:star.zeng@intel.com]
> > Sent: Monday, January 08, 2018 3:18 PM
> > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; edk2-
> > devel@lists.01.org; Dong, Eric <eric.dong@intel.com>
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: RE: [edk2] [RFC] SATA : Implemented NXP errata A008402
> >
> > So this PCD needs to be defined as 0x3FFFFF or may be 0x400000, then
> > it needs to be configured to 0 for your case, right?
> > Could the PCD be configured to other values?
> No, in my case i need to set it to 0 only. No other value is needed.
> >
> > According to the statement you provided, is it possible to handle the
> > case in the device firmware?
> >
> > " Due to a logic error, 3F_FFFFh is misinterpreted by the device as
> > zero length."
> >
> No, it can't be handle in device firmware for existing SATA controller.
> There are chances that in future releases of SATA controller it will get fixed in
> RTL, but this change will still be needed for LS2088 board.
> 
> 
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com]
> > Sent: Monday, January 8, 2018 2:26 PM
> > To: Zeng, Star <star.zeng@intel.com>; ard.biesheuvel@linaro.org;
> > leif.lindholm@linaro.org; edk2-devel@lists.01.org; Dong, Eric
> > <eric.dong@intel.com>
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: RE: [edk2] [RFC] SATA : Implemented NXP errata A008402
> >
> > Hi Star,
> >
> > Apologies and some correction in my last reply.
> >
> > As per the errata, PRDT Maximum value needs to be set to 0 only  when
> > creating a PRD entry for a maximum data transfer size.
> >
> > So there is no need to replace all occurrences of
> > EFI_AHCI_MAX_DATA_PER_PRDT in file.
> > Just need to change it where we are setting the Data length.
> >
> > I define it to 0x3FFFFF, as this is the actual value we are setting
> > and this PCD need to be used only once.
> >
> > I know, its NXP specific patch only, and i try to made changes which
> > will not impact any other package.
> >
> >
> > Thanks,
> > Meenakshi
> >
> > > -----Original Message-----
> > > From: Meenakshi Aggarwal
> > > Sent: Monday, January 08, 2018 11:25 AM
> > > To: 'Zeng, Star' <star.zeng@intel.com>; ard.biesheuvel@linaro.org;
> > > leif.lindholm@linaro.org; edk2-devel@lists.01.org; Dong, Eric
> > > <eric.dong@intel.com>
> > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > > Subject: RE: [edk2] [RFC] SATA : Implemented NXP errata A008402
> > >
> > > Hi,
> > >
> > > I didn't prepare the full patch but will send in next few minutes,
> > >
> > > i  made the very basic changes required to test Errata implementation.
> > >
> > > I will redefine  it to 0x400000.
> > > PcdPrdtMaxDataLength was defined to 0x3FFFFF just for testing purpose.
> > >
> > > Thanks,
> > > Meenakshi
> > >
> > > > -----Original Message-----
> > > > From: Zeng, Star [mailto:star.zeng@intel.com]
> > > > Sent: Monday, January 08, 2018 11:19 AM
> > > > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > > ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; edk2-
> > > > devel@lists.01.org; Dong, Eric <eric.dong@intel.com>
> > > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star
> > > > <star.zeng@intel.com>
> > > > Subject: RE: [edk2] [RFC] SATA : Implemented NXP errata A008402
> > > >
> > > > Do you have a full patch already?
> > > > Why the PcdPrdtMaxDataLength is defined to 0x3FFFFF, but not
> > 0x400000?
> > > >
> > > >
> > > > Thanks,
> > > > Star
> > > > -----Original Message-----
> > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> > > > Behalf Of Meenakshi Aggarwal
> > > > Sent: Monday, January 8, 2018 7:17 PM
> > > > To: ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; edk2-
> > > > devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Dong, Eric
> > > > <eric.dong@intel.com>
> > > > Subject: [edk2] [RFC] SATA : Implemented NXP errata A008402
> > > >
> > > > Description:
> > > > Commands with 4 MB PRD length entries fail if PRD[DBC] is set to
> > > > the value according to AHCI standard spec.
> > > > Due to a logic error, 3F_FFFFh is misinterpreted by the device as
> > > > zero length.
> > > >
> > > > Workaround:
> > > > Set PRD length to 0 when creating a PRD entry for a maximum data
> > > > transfer size of 4 MB to fix the erratum.
> > > >
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> > > > ---
> > > >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c           | 2 +-
> > > >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf | 1 +
> > > >  MdeModulePkg/MdeModulePkg.dec                              | 3 +++
> > > >  3 files changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > > > index e6de5d6..fb6dc0b 100644
> > > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > > > @@ -591,7 +591,7 @@ AhciBuildCommand (
> > > >      if (RemainedData < EFI_AHCI_MAX_DATA_PER_PRDT) {
> > > >        AhciRegisters->AhciCommandTable-
> > > >PrdtTable[PrdtIndex].AhciPrdtDbc  = (UINT32)RemainedData - 1;
> > > >      } else {
> > > > -      AhciRegisters->AhciCommandTable-
> > > >PrdtTable[PrdtIndex].AhciPrdtDbc
> > > > = EFI_AHCI_MAX_DATA_PER_PRDT - 1;
> > > > +      AhciRegisters->AhciCommandTable-
> > > >PrdtTable[PrdtIndex].AhciPrdtDbc
> > > > = PcdGet32 (PcdPrdtMaxDataLength);
> > > >      }
> > > >
> > > >      Data64.Uint64 = (UINT64)MemAddr; diff --git
> > > a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> > > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> > > > index 82d5f7a..8921dd5 100644
> > > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> > > > +++
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> > > > @@ -70,6 +70,7 @@
> > > >
> > > >  [Pcd]
> > > >    gEfiMdeModulePkgTokenSpaceGuid.PcdAtaSmartEnable   ##
> > > > SOMETIMES_CONSUMES
> > > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdPrdtMaxDataLength
> > > >
> > > >  # [Event]
> > > >  # EVENT_TYPE_PERIODIC_TIMER ## SOMETIMES_CONSUMES diff --git
> > > > a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec
> > > > index 8efad57..b2f9f2b 100644
> > > > --- a/MdeModulePkg/MdeModulePkg.dec
> > > > +++ b/MdeModulePkg/MdeModulePkg.dec
> > > > @@ -1434,6 +1434,9 @@
> > > >    # @Prompt Console Output Row of Text Setup
> > > >
> > > >
> > >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetupConOutRow|25|UINT32|0x40
> > > > 00000e
> > > >
> > > > +  ## This PCD specifies the Maximum data length for a PRD Entry
> > > > +
> > > >
> > >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdPrdtMaxDataLength|0x3FFFFF|UIN
> > > > T32|0x4000000f
> > > > +
> > > >  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic,
> > > PcdsDynamicEx]
> > > >    ## UART clock frequency is for the baud rate configuration.
> > > >    # @Prompt Serial Port Clock Rate.
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > edk2-devel mailing list
> > > > edk2-devel@lists.01.org
> > > >
> > >
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> > > t
> > > > s.01.org%2Fmailman%2Flistinfo%2Fedk2-
> > > >
> > >
> >
> devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C9b1e2bde5b
> > > >
> > >
> >
> 4b47746d0e08d5565b8394%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> > > >
> > >
> >
> 0%7C636509873433437533&sdata=wg3Sg5SiU2MrbZSkZMlboldcXnMBKKg3jG
> > > > yA45YZo1Q%3D&reserved=0