Re: [RFC] define PCD for EFI_AHCI_MAX_DATA_PER_PRDT

Re: [RFC] define PCD for EFI_AHCI_MAX_DATA_PER_PRDT

[Zeng, Star]

Cc Ray for any comments.


Thanks,
Star
-----Original Message-----
From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] 
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>
Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
Subject: [RFC] define PCD for EFI_AHCI_MAX_DATA_PER_PRDT

Hi,

We need to implement an errata which is specific to NXP's SATA controller.
As we will be using edk2 AHCI layer, so i have to implement this errata in edk2.

If this change is acceptable then i will replace every occurence of EFI_AHCI_MAX_DATA_PER_PRDT with pcd PcdPrdtMaxDataLength accordingly.

Thanks,
Meenakshi

Meenakshi Aggarwal (1):
  SATA : Implemented NXP errata A008402

 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(-)

--
1.9.1

Re: [RFC] SATA : Implemented NXP errata A008402

[Zeng, Star]

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|0x4000000e
 
+  ## This PCD specifies the Maximum data length for a PRD Entry
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPrdtMaxDataLength|0x3FFFFF|UINT32|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://lists.01.org/mailman/listinfo/edk2-devel

Re: [RFC] SATA : Implemented NXP errata A008402

[Meenakshi Aggarwal]

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%2Flist
> 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

Re: [RFC] SATA : Implemented NXP errata A008402

[Meenakshi Aggarwal]

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%2Flist
> > 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

Re: [RFC] SATA : Implemented NXP errata A008402

[Zeng, Star]

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?

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."


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

Re: [RFC] SATA : Implemented NXP errata A008402

[Meenakshi Aggarwal]

> -----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

Re: [RFC] SATA : Implemented NXP errata A008402

[Zeng, Star]

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

Re: [RFC] SATA : Implemented NXP errata A008402

[Meenakshi Aggarwal]

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

[RFC] define PCD for EFI_AHCI_MAX_DATA_PER_PRDT

[Meenakshi Aggarwal]

Hi,

We need to implement an errata which is specific to NXP's SATA controller.
As we will be using edk2 AHCI layer, so i have to implement this errata in edk2.

If this change is acceptable then i will replace every occurence of
EFI_AHCI_MAX_DATA_PER_PRDT with pcd PcdPrdtMaxDataLength accordingly.

Thanks,
Meenakshi

Meenakshi Aggarwal (1):
  SATA : Implemented NXP errata A008402

 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(-)

-- 
1.9.1

[RFC] SATA : Implemented NXP errata A008402

[Meenakshi Aggarwal]

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|0x4000000e
 
+  ## This PCD specifies the Maximum data length for a PRD Entry
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPrdtMaxDataLength|0x3FFFFF|UINT32|0x4000000f
+
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## UART clock frequency is for the baud rate configuration.
   # @Prompt Serial Port Clock Rate.
-- 
1.9.1

Re: [RFC] SATA : Implemented NXP errata A008402

[Ni, Ruiyu]

On 1/8/2018 7:16 PM, Meenakshi Aggarwal wrote:
> 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.

Is the logic error mentioned here is the error in HW?
Then I do not prefer to add such PCD for a HW workaround.

> 
> 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|0x4000000e
>   
> +  ## This PCD specifies the Maximum data length for a PRD Entry
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdPrdtMaxDataLength|0x3FFFFF|UINT32|0x4000000f
> +
>   [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>     ## UART clock frequency is for the baud rate configuration.
>     # @Prompt Serial Port Clock Rate.
> 


-- 
Thanks,
Ray

Re: [RFC] SATA : Implemented NXP errata A008402

[Ni, Ruiyu]

On 1/10/2018 4:56 PM, Meenakshi Aggarwal wrote:
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ni,
>> Ruiyu
>> Sent: Tuesday, January 09, 2018 9:11 AM
>> To: edk2-devel@lists.01.org
>> Subject: Re: [edk2] [RFC] SATA : Implemented NXP errata A008402
>>
>> On 1/8/2018 7:16 PM, Meenakshi Aggarwal wrote:
>>> 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.
>>
>> Is the logic error mentioned here is the error in HW?
>> Then I do not prefer to add such PCD for a HW workaround.
>>
> Yes, the error is in hardware.
> There is no other option visible to implement this errata if we want to use the existing AHCI framework.
> 
> I completely agree with you, but no other solution seems to exist in this case.
> 
> And this change will not impact any other hardware so no one is basically impacted by this change.

Your buggy HW only need the value zero. But the addition of PCD
exposes an interface that can use any size of PRD.
I am not sure the AtaAtapiPassThru can work if some platform
sets the PCD value to others than 0 or 3F_FFFFh.

Can you please
  just duplicate the AtaAtapiPassThru in your platform?
Because the driver is very stable today, not much code sync effort
will be needed if core version is changed.

> 
>>>
>>> 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.
>>>
>>
>>
>> --
>> Thanks,
>> Ray
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
>> s.01.org%2Fmailman%2Flistinfo%2Fedk2-
>> devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C60cadf13ac95
>> 48486c4908d55712d85c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
>> 7C636510660888390946&sdata=4oW2agf8z869IMz%2F7%2B2di9vC%2BB3js7K
>> hJx1LR2b4Tc4%3D&reserved=0
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


-- 
Thanks,
Ray

Re: [RFC] SATA : Implemented NXP errata A008402

[Udit Kumar]

Hi Ruiyu, 

> -----Original Message-----
> >
> > And this change will not impact any other hardware so no one is basically
> impacted by this change.
> 
> Your buggy HW only need the value zero. But the addition of PCD exposes
> an interface that can use any size of PRD.
> I am not sure the AtaAtapiPassThru can work if some platform sets the PCD
> value to others than 0 or 3F_FFFFh.

I don't see someone using this driver will set Pcd randomly, but I agree on this
point other value should be handled. 
Error or Assert could be added, if Pcd value is not 0 or 3F_FFFFh.

> Can you please
>   just duplicate the AtaAtapiPassThru in your platform?
> Because the driver is very stable today, not much code sync effort will be
> needed if core version is changed.

Duplicating is always a possibility :), but When we will push this duplicated driver
(just for one line change) for upstreaming.
will this be acceptable ??

Thanks

 
> >
> >>>
> >>> 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.
> >>>
> >>
> >>
> >> --
> >> Thanks,
> >> Ray
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fli
> >> st
> >> s.01.org%2Fmailman%2Flistinfo%2Fedk2-
> >>
> devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C60cadf13ac95
> >>
> 48486c4908d55712d85c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %
> >>
> 7C636510660888390946&sdata=4oW2agf8z869IMz%2F7%2B2di9vC%2BB3js7K
> >> hJx1LR2b4Tc4%3D&reserved=0
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> > ts.01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cudit.kumar%
> >
> 40nxp.com%7C0b697e2364f348745a7c08d5580cf5bf%7C686ea1d3bc2b4c6fa9
> 2cd99
> >
> c5c301635%7C0%7C0%7C636511735060291299&sdata=eWI7ueE5wzr2oSNv0t
> CP5hBE0
> > uEfB9S0PyABHWocBaA%3D&reserved=0
> >
> 
> 
> --
> Thanks,
> Ray
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.
> 01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cudit.kumar%40nxp.com%7C0b697e2364f348745a7c
> 08d5580cf5bf%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636511
> 735060291299&sdata=eWI7ueE5wzr2oSNv0tCP5hBE0uEfB9S0PyABHWocBaA
> %3D&reserved=0

Re: [RFC] SATA : Implemented NXP errata A008402

[Ard Biesheuvel]

On 10 January 2018 at 09:43, Udit Kumar <udit.kumar@nxp.com> wrote:
> Hi Ruiyu,
>
>> -----Original Message-----
>> >
>> > And this change will not impact any other hardware so no one is basically
>> impacted by this change.
>>
>> Your buggy HW only need the value zero. But the addition of PCD exposes
>> an interface that can use any size of PRD.
>> I am not sure the AtaAtapiPassThru can work if some platform sets the PCD
>> value to others than 0 or 3F_FFFFh.
>
> I don't see someone using this driver will set Pcd randomly, but I agree on this
> point other value should be handled.
> Error or Assert could be added, if Pcd value is not 0 or 3F_FFFFh.
>
>> Can you please
>>   just duplicate the AtaAtapiPassThru in your platform?
>> Because the driver is very stable today, not much code sync effort will be
>> needed if core version is changed.
>
> Duplicating is always a possibility :), but When we will push this duplicated driver
> (just for one line change) for upstreaming.
> will this be acceptable ??

My main concern with this (and with using a PCD) is that the setting
affects all SATA controllers in the system, including ones the you
stick into a PCIe slot.

So I think forking the driver is the only possible solution, but it
will not be a one-line change: you need to ensure that you apply the
workaround only to the SATA controllers in the SoC.

-- 
Ard.

Re: [RFC] SATA : Implemented NXP errata A008402

[Udit Kumar]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, January 10, 2018 3:23 PM
> To: Udit Kumar <udit.kumar@nxp.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; edk2-devel@lists.01.org; Zeng, Star
> <star.zeng@intel.com>; leif.lindholm@linaro.org
> Subject: Re: [edk2] [RFC] SATA : Implemented NXP errata A008402
> 
> On 10 January 2018 at 09:43, Udit Kumar <udit.kumar@nxp.com> wrote:
> > Hi Ruiyu,
> >
> >> -----Original Message-----
> >> >
> >> > And this change will not impact any other hardware so no one is
> >> > basically
> >> impacted by this change.
> >>
> >> Your buggy HW only need the value zero. But the addition of PCD
> >> exposes an interface that can use any size of PRD.
> >> I am not sure the AtaAtapiPassThru can work if some platform sets the
> >> PCD value to others than 0 or 3F_FFFFh.
> >
> > I don't see someone using this driver will set Pcd randomly, but I
> > agree on this point other value should be handled.
> > Error or Assert could be added, if Pcd value is not 0 or 3F_FFFFh.
> >
> >> Can you please
> >>   just duplicate the AtaAtapiPassThru in your platform?
> >> Because the driver is very stable today, not much code sync effort
> >> will be needed if core version is changed.
> >
> > Duplicating is always a possibility :), but When we will push this
> > duplicated driver (just for one line change) for upstreaming.
> > will this be acceptable ??
> 
> My main concern with this (and with using a PCD) is that the setting affects
> all SATA controllers in the system, including ones the you stick into a PCIe
> slot.

Agreed,  If two types of Sata controllers are used then this patch 
may break one controller. 
At present, we don’t have anyone asking SATA controller behind PCIe
but we shouldn’t ignore this. This is valid use case. 

> So I think forking the driver is the only possible solution, but it will not be a
> one-line change: you need to ensure that you apply the workaround only to
> the SATA controllers in the SoC.

Yes, Considering unrelated SATA controllers this will be more than copy 

> --
> Ard.

Thanks 
Udit

Re: [RFC] SATA : Implemented NXP errata A008402

[Ni, Ruiyu]

On 1/10/2018 5:52 PM, Ard Biesheuvel wrote:
> On 10 January 2018 at 09:43, Udit Kumar <udit.kumar@nxp.com> wrote:
>> Hi Ruiyu,
>>
>>> -----Original Message-----
>>>>
>>>> And this change will not impact any other hardware so no one is basically
>>> impacted by this change.
>>>
>>> Your buggy HW only need the value zero. But the addition of PCD exposes
>>> an interface that can use any size of PRD.
>>> I am not sure the AtaAtapiPassThru can work if some platform sets the PCD
>>> value to others than 0 or 3F_FFFFh.
>>
>> I don't see someone using this driver will set Pcd randomly, but I agree on this
>> point other value should be handled.
>> Error or Assert could be added, if Pcd value is not 0 or 3F_FFFFh.
>>
>>> Can you please
>>>    just duplicate the AtaAtapiPassThru in your platform?
>>> Because the driver is very stable today, not much code sync effort will be
>>> needed if core version is changed.
>>
>> Duplicating is always a possibility :), but When we will push this duplicated driver
>> (just for one line change) for upstreaming.
>> will this be acceptable ??
> 
> My main concern with this (and with using a PCD) is that the setting
> affects all SATA controllers in the system, including ones the you
> stick into a PCIe slot.
> 
> So I think forking the driver is the only possible solution, but it
> will not be a one-line change: you need to ensure that you apply the
> workaround only to the SATA controllers in the SoC.
> 

Depending on the new driver's location. The package owner decides
whether forking is acceptable :)


-- 
Thanks,
Ray

Re: [RFC] SATA : Implemented NXP errata A008402

[Ard Biesheuvel]

On 11 January 2018 at 02:25, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> On 1/10/2018 5:52 PM, Ard Biesheuvel wrote:
>>
>> On 10 January 2018 at 09:43, Udit Kumar <udit.kumar@nxp.com> wrote:
>>>
>>> Hi Ruiyu,
>>>
>>>> -----Original Message-----
>>>>>
>>>>>
>>>>> And this change will not impact any other hardware so no one is
>>>>> basically
>>>>
>>>> impacted by this change.
>>>>
>>>> Your buggy HW only need the value zero. But the addition of PCD exposes
>>>> an interface that can use any size of PRD.
>>>> I am not sure the AtaAtapiPassThru can work if some platform sets the
>>>> PCD
>>>> value to others than 0 or 3F_FFFFh.
>>>
>>>
>>> I don't see someone using this driver will set Pcd randomly, but I agree
>>> on this
>>> point other value should be handled.
>>> Error or Assert could be added, if Pcd value is not 0 or 3F_FFFFh.
>>>
>>>> Can you please
>>>>    just duplicate the AtaAtapiPassThru in your platform?
>>>> Because the driver is very stable today, not much code sync effort will
>>>> be
>>>> needed if core version is changed.
>>>
>>>
>>> Duplicating is always a possibility :), but When we will push this
>>> duplicated driver
>>> (just for one line change) for upstreaming.
>>> will this be acceptable ??
>>
>>
>> My main concern with this (and with using a PCD) is that the setting
>> affects all SATA controllers in the system, including ones the you
>> stick into a PCIe slot.
>>
>> So I think forking the driver is the only possible solution, but it
>> will not be a one-line change: you need to ensure that you apply the
>> workaround only to the SATA controllers in the SoC.
>>
>
> Depending on the new driver's location. The package owner decides
> whether forking is acceptable :)
>

I think forking is reasonable in this case: the workaround does not
belong in generic code, and it seems only early silicon is affected
anyway.