Re: [edk2] [PATCH edk2-platforms 34/39] Silicon/NXP: Implement EFI_CPU_IO2_PROTOCOL

2018-04-24 Thread Ard Biesheuvel
On 24 April 2018 at 15:36, Vabhav Sharma  wrote:
>
>
>>-Original Message-
>>From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>Sent: Tuesday, April 24, 2018 6:04 PM
>>To: Vabhav Sharma 
>>Cc: Meenakshi Aggarwal ; Leif Lindholm
>>; Kinney, Michael D ;
>>edk2-devel@lists.01.org; Udit Kumar ; Varun Sethi
>>
>>Subject: Re: [PATCH edk2-platforms 34/39] Silicon/NXP: Implement
>>EFI_CPU_IO2_PROTOCOL
>>
>>On 24 April 2018 at 14:26, Vabhav Sharma  wrote:
>>>
>>>
-Original Message-
From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
Sent: Friday, April 20, 2018 2:11 PM
To: Meenakshi Aggarwal 
Cc: Leif Lindholm ; Kinney, Michael D
; edk2-devel@lists.01.org; Udit Kumar
; Varun Sethi ; Vabhav Sharma

Subject: Re: [PATCH edk2-platforms 34/39] Silicon/NXP: Implement
EFI_CPU_IO2_PROTOCOL

On 16 February 2018 at 09:50, Meenakshi 
wrote:
> From: Vabhav 
>
> NXP SOC has mutiple PCIe RCs,Adding respective implementation of
> EFI_CPU_IO2_PROTOCOL to provide Memory Space Read/Write functions
> used by generic Host Bridge Driver including correct value for the
> translation offset during MMIO accesses
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vabhav 
> Signed-off-by: Meenakshi Aggarwal 

This driver looks completely wrong to me: MMIO access is memory
mapped, and given that you don't implement PCI to CPU translation of
MMIO accesses, the memory read and write functions should not perform
any translation at all, and just relay the accesses. On the other
hand, the I/O accessors are not implemented at all, and these are the
ones that require translation, given that the I/O port addresses in the CPU
>>space need translation to MMIO addressess.
>>>
>>> On NXP SoC, Mapping between CPU view and PCIe view is not 1:1 and require
>>CPU view translation for MMIO regions access, Accordingly translation is added
>>during memory read/write services.
>>> Bus driver relays the address range where PCIe device Bar region is split 
>>> from,
>>Translation is required for relaying it to correct PCIe controller cpu view 
>>address.
>>>
>>
>>You cannot implement this only in the EFI_CPU_IO2_PROTOCOL driver.
>>That way, EFI_PCI_IO_PROTOCOL.GetBarAttributes() will return resource
>>descriptors with untranslated addresses, breaking drivers that rely on this
>>information.
>>
>>If your PCIe implementation relies on MMIO translation, please refer to the
>>recently merged code in EDK2 and edk2-platforms implementing this for
>>Socionext SynQuacer.
> Ok, PciIo->GetBarAttributes is expected to return CPU view address.

Yes

> Are you referring to edk2 commit 74d0a33(Address translation support added to 
> generic PciHostBridge driver)? Submitted patch development is done prior to 
> this commit.

I understand. But that does not make your code correct.

> I will refer the commits for Socionext SynQuacer in edk2-platforms for MMIO 
> translation.

Yes please. And I/O translation needs to be implemented as well.




Also, you don't seem to be using the PcdPciExp?BaseAddr PCDs anywhere,
so you can drop them from the .dsc
>>> No, It's used for checking the access to MMIO32 region and CPU view
>>> base address varies between different NXP SoCs

> ---
>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c   | 529
++
>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf |  48 ++
>  2 files changed, 577 insertions(+)
>  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>  create mode 100644
> Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf
>
> diff --git a/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
> b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
> new file mode 100644
> index 000..b5fb72c
> --- /dev/null
> +++ b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
> @@ -0,0 +1,529 @@
> +/** @file
> +  Produces the CPU I/O 2 Protocol.
> +
> +  Copyright (c) 2009 - 2012, Intel Corporation. All rights
> + reserved.  Copyright (c) 2016, Linaro Ltd. All rights
> + reserved.  Copyright 2018 NXP
> +
> +  This program and the accompanying materials  are licensed and
> + made available under the terms and conditions of the BSD License
> + which accompanies this distribution.  The full text of the license
> + may be found at
> +
> + https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> 

Re: [edk2] [PATCH edk2-platforms 34/39] Silicon/NXP: Implement EFI_CPU_IO2_PROTOCOL

2018-04-24 Thread Vabhav Sharma


>-Original Message-
>From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>Sent: Tuesday, April 24, 2018 6:04 PM
>To: Vabhav Sharma 
>Cc: Meenakshi Aggarwal ; Leif Lindholm
>; Kinney, Michael D ;
>edk2-devel@lists.01.org; Udit Kumar ; Varun Sethi
>
>Subject: Re: [PATCH edk2-platforms 34/39] Silicon/NXP: Implement
>EFI_CPU_IO2_PROTOCOL
>
>On 24 April 2018 at 14:26, Vabhav Sharma  wrote:
>>
>>
>>>-Original Message-
>>>From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>>Sent: Friday, April 20, 2018 2:11 PM
>>>To: Meenakshi Aggarwal 
>>>Cc: Leif Lindholm ; Kinney, Michael D
>>>; edk2-devel@lists.01.org; Udit Kumar
>>>; Varun Sethi ; Vabhav Sharma
>>>
>>>Subject: Re: [PATCH edk2-platforms 34/39] Silicon/NXP: Implement
>>>EFI_CPU_IO2_PROTOCOL
>>>
>>>On 16 February 2018 at 09:50, Meenakshi 
>>>wrote:
 From: Vabhav 

 NXP SOC has mutiple PCIe RCs,Adding respective implementation of
 EFI_CPU_IO2_PROTOCOL to provide Memory Space Read/Write functions
 used by generic Host Bridge Driver including correct value for the
 translation offset during MMIO accesses

 Contributed-under: TianoCore Contribution Agreement 1.1
 Signed-off-by: Vabhav 
 Signed-off-by: Meenakshi Aggarwal 
>>>
>>>This driver looks completely wrong to me: MMIO access is memory
>>>mapped, and given that you don't implement PCI to CPU translation of
>>>MMIO accesses, the memory read and write functions should not perform
>>>any translation at all, and just relay the accesses. On the other
>>>hand, the I/O accessors are not implemented at all, and these are the
>>>ones that require translation, given that the I/O port addresses in the CPU
>space need translation to MMIO addressess.
>>
>> On NXP SoC, Mapping between CPU view and PCIe view is not 1:1 and require
>CPU view translation for MMIO regions access, Accordingly translation is added
>during memory read/write services.
>> Bus driver relays the address range where PCIe device Bar region is split 
>> from,
>Translation is required for relaying it to correct PCIe controller cpu view 
>address.
>>
>
>You cannot implement this only in the EFI_CPU_IO2_PROTOCOL driver.
>That way, EFI_PCI_IO_PROTOCOL.GetBarAttributes() will return resource
>descriptors with untranslated addresses, breaking drivers that rely on this
>information.
>
>If your PCIe implementation relies on MMIO translation, please refer to the
>recently merged code in EDK2 and edk2-platforms implementing this for
>Socionext SynQuacer.
Ok, PciIo->GetBarAttributes is expected to return CPU view address.
Are you referring to edk2 commit 74d0a33(Address translation support added to 
generic PciHostBridge driver)? Submitted patch development is done prior to 
this commit.
I will refer the commits for Socionext SynQuacer in edk2-platforms for MMIO 
translation.
>
>
>>>
>>>Also, you don't seem to be using the PcdPciExp?BaseAddr PCDs anywhere,
>>>so you can drop them from the .dsc
>> No, It's used for checking the access to MMIO32 region and CPU view
>> base address varies between different NXP SoCs
>>>
 ---
  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c   | 529
>>>++
  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf |  48 ++
  2 files changed, 577 insertions(+)
  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
  create mode 100644
 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf

 diff --git a/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
 b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
 new file mode 100644
 index 000..b5fb72c
 --- /dev/null
 +++ b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
 @@ -0,0 +1,529 @@
 +/** @file
 +  Produces the CPU I/O 2 Protocol.
 +
 +  Copyright (c) 2009 - 2012, Intel Corporation. All rights
 + reserved.  Copyright (c) 2016, Linaro Ltd. All rights
 + reserved.  Copyright 2018 NXP
 +
 +  This program and the accompanying materials  are licensed and
 + made available under the terms and conditions of the BSD License
 + which accompanies this distribution.  The full text of the license
 + may be found at
 +
 + https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
 + op
 + ensource.org%2Flicenses%2Fbsd-
>license.php=02%7C01%7Cvabhav.sh
 + ar
 +
>>>ma%40nxp.com%7C4507c0726b244ad6476d08d5a69a64ee%7C686ea1d3bc2b
>4c
>>>6fa9
 +
>>>2cd99c5c301635%7C0%7C0%7C636598104414046440=IFSU0%2FeTdW
>rw
>>>gg0f
 + 0Lq2qSVGgogG68tYZevrRmC%2BkV8%3D=0
 +
 +  THE 

Re: [edk2] [PATCH edk2-platforms 34/39] Silicon/NXP: Implement EFI_CPU_IO2_PROTOCOL

2018-04-24 Thread Vabhav Sharma


>-Original Message-
>From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
>Sent: Friday, April 20, 2018 8:46 PM
>To: Meenakshi Aggarwal 
>Cc: ard.biesheu...@linaro.org; edk2-devel@lists.01.org; Udit Kumar
>; Varun Sethi ; Vabhav Sharma
>
>Subject: Re: [PATCH edk2-platforms 34/39] Silicon/NXP: Implement
>EFI_CPU_IO2_PROTOCOL
>
>On Fri, Feb 16, 2018 at 02:20:30PM +0530, Meenakshi wrote:
>> From: Vabhav 
>>
>> NXP SOC has mutiple PCIe RCs,Adding respective implementation of
>> EFI_CPU_IO2_PROTOCOL to provide Memory Space Read/Write functions used
>> by generic Host Bridge Driver including correct value for the
>> translation offset during MMIO accesses
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Vabhav 
>> Signed-off-by: Meenakshi Aggarwal 
>> ---
>>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c   | 529
>++
>>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf |  48 ++
>>  2 files changed, 577 insertions(+)
>>  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>>  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf
>>
>> diff --git a/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>> b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>> new file mode 100644
>> index 000..b5fb72c
>> --- /dev/null
>> +++ b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>> @@ -0,0 +1,529 @@
>> +/** @file
>> +  Produces the CPU I/O 2 Protocol.
>> +
>> +  Copyright (c) 2009 - 2012, Intel Corporation. All rights
>> + reserved.  Copyright (c) 2016, Linaro Ltd. All rights
>> + reserved.  Copyright 2018 NXP
>> +
>> +  This program and the accompanying materials  are licensed and made
>> + available under the terms and conditions of the BSD License  which
>> + accompanies this distribution.  The full text of the license may be
>> + found at
>> +
>> + https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
>> + ensource.org%2Flicenses%2Fbsd-license.php=02%7C01%7Cvabhav.shar
>> +
>ma%40nxp.com%7C42500fab2cd1447bbe6308d5a6d19d8b%7C686ea1d3bc2b4c
>6fa9
>> +
>2cd99c5c301635%7C0%7C0%7C636598341623135085=Zo6s2LhxPSElw4F
>XsV
>> + 7%2Bx3Veb5yptglf1UQiA%2FNRRc4%3D=0
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>> + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define MAX_IO_PORT_ADDRESS PCI_SEG2_PORTIO_MAX
>> +
>> +//
>> +// Handle for the CPU I/O 2 Protocol
>> +//
>> +STATIC EFI_HANDLE  mHandle;
>> +
>> +//
>> +// Lookup table for increment values based on transfer widths //
>> +STATIC CONST UINT8 mInStride[] = {
>> +  1, // EfiCpuIoWidthUint8
>> +  2, // EfiCpuIoWidthUint16
>> +  4, // EfiCpuIoWidthUint32
>> +  8, // EfiCpuIoWidthUint64
>> +  0, // EfiCpuIoWidthFifoUint8
>> +  0, // EfiCpuIoWidthFifoUint16
>> +  0, // EfiCpuIoWidthFifoUint32
>> +  0, // EfiCpuIoWidthFifoUint64
>> +  1, // EfiCpuIoWidthFillUint8
>> +  2, // EfiCpuIoWidthFillUint16
>> +  4, // EfiCpuIoWidthFillUint32
>> +  8  // EfiCpuIoWidthFillUint64
>> +};
>> +
>> +//
>> +// Lookup table for increment values based on transfer widths //
>> +STATIC CONST UINT8 mOutStride[] = {
>> +  1, // EfiCpuIoWidthUint8
>> +  2, // EfiCpuIoWidthUint16
>> +  4, // EfiCpuIoWidthUint32
>> +  8, // EfiCpuIoWidthUint64
>> +  1, // EfiCpuIoWidthFifoUint8
>> +  2, // EfiCpuIoWidthFifoUint16
>> +  4, // EfiCpuIoWidthFifoUint32
>> +  8, // EfiCpuIoWidthFifoUint64
>> +  0, // EfiCpuIoWidthFillUint8
>> +  0, // EfiCpuIoWidthFillUint16
>> +  0, // EfiCpuIoWidthFillUint32
>> +  0  // EfiCpuIoWidthFillUint64
>> +};
>> +
>> +/**
>> +  Check parameters to a CPU I/O 2 Protocol service request.
>> +
>> +  The I/O operations are carried out exactly as requested. The caller
>> + is responsible  for satisfying any alignment and I/O width
>> + restrictions that a PI System on a  platform might require. For
>> + example on some platforms, width requests of
>> +  EfiCpuIoWidthUint64 do not work.
>> +
>> +  @param[in] MmioOperation  TRUE for an MMIO operation, FALSE for I/O
>Port operation.
>> +  @param[in] Width  Signifies the width of the I/O or Memory 
>> operation.
>> +  @param[in] AddressThe base address of the I/O operation.
>> +  @param[in] Count  The number of I/O operations to perform. The
>number of
>> +bytes moved is Width size * Count, starting at 
>> Address.
>> +  @param[in] Buffer For read operations, the destination buffer to 
>> store
>the results.
>> +For write operations, the source buffer from 
>> which to write
>data.
>> +
>> +  @retval EFI_SUCCESSThe parameters for this request pass the 
>> checks.
>> +  @retval 

Re: [edk2] [PATCH edk2-platforms 34/39] Silicon/NXP: Implement EFI_CPU_IO2_PROTOCOL

2018-04-24 Thread Ard Biesheuvel
On 24 April 2018 at 14:26, Vabhav Sharma  wrote:
>
>
>>-Original Message-
>>From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>Sent: Friday, April 20, 2018 2:11 PM
>>To: Meenakshi Aggarwal 
>>Cc: Leif Lindholm ; Kinney, Michael D
>>; edk2-devel@lists.01.org; Udit Kumar
>>; Varun Sethi ; Vabhav Sharma
>>
>>Subject: Re: [PATCH edk2-platforms 34/39] Silicon/NXP: Implement
>>EFI_CPU_IO2_PROTOCOL
>>
>>On 16 February 2018 at 09:50, Meenakshi 
>>wrote:
>>> From: Vabhav 
>>>
>>> NXP SOC has mutiple PCIe RCs,Adding respective implementation of
>>> EFI_CPU_IO2_PROTOCOL to provide Memory Space Read/Write functions used
>>> by generic Host Bridge Driver including correct value for the
>>> translation offset during MMIO accesses
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Vabhav 
>>> Signed-off-by: Meenakshi Aggarwal 
>>
>>This driver looks completely wrong to me: MMIO access is memory mapped, and
>>given that you don't implement PCI to CPU translation of MMIO accesses, the
>>memory read and write functions should not perform any translation at all, and
>>just relay the accesses. On the other hand, the I/O accessors are not
>>implemented at all, and these are the ones that require translation, given 
>>that the
>>I/O port addresses in the CPU space need translation to MMIO addressess.
>
> On NXP SoC, Mapping between CPU view and PCIe view is not 1:1 and require CPU 
> view translation for MMIO regions access, Accordingly translation is added 
> during memory read/write services.
> Bus driver relays the address range where PCIe device Bar region is split 
> from, Translation is required for relaying it to correct PCIe controller cpu 
> view address.
>

You cannot implement this only in the EFI_CPU_IO2_PROTOCOL driver.
That way, EFI_PCI_IO_PROTOCOL.GetBarAttributes() will return resource
descriptors with untranslated addresses, breaking drivers that rely on
this information.

If your PCIe implementation relies on MMIO translation, please refer
to the recently merged code in EDK2 and edk2-platforms implementing
this for Socionext SynQuacer.


>>
>>Also, you don't seem to be using the PcdPciExp?BaseAddr PCDs anywhere, so you
>>can drop them from the .dsc
> No, It's used for checking the access to MMIO32 region and CPU view base 
> address varies between different NXP SoCs
>>
>>> ---
>>>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c   | 529
>>++
>>>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf |  48 ++
>>>  2 files changed, 577 insertions(+)
>>>  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>>>  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf
>>>
>>> diff --git a/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>>> b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>>> new file mode 100644
>>> index 000..b5fb72c
>>> --- /dev/null
>>> +++ b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>>> @@ -0,0 +1,529 @@
>>> +/** @file
>>> +  Produces the CPU I/O 2 Protocol.
>>> +
>>> +  Copyright (c) 2009 - 2012, Intel Corporation. All rights
>>> + reserved.  Copyright (c) 2016, Linaro Ltd. All rights
>>> + reserved.  Copyright 2018 NXP
>>> +
>>> +  This program and the accompanying materials  are licensed and made
>>> + available under the terms and conditions of the BSD License  which
>>> + accompanies this distribution.  The full text of the license may be
>>> + found at
>>> +
>>> + https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
>>> + ensource.org%2Flicenses%2Fbsd-license.php=02%7C01%7Cvabhav.shar
>>> +
>>ma%40nxp.com%7C4507c0726b244ad6476d08d5a69a64ee%7C686ea1d3bc2b4c
>>6fa9
>>> +
>>2cd99c5c301635%7C0%7C0%7C636598104414046440=IFSU0%2FeTdWrw
>>gg0f
>>> + 0Lq2qSVGgogG68tYZevrRmC%2BkV8%3D=0
>>> +
>>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>>> + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>>EXPRESS OR IMPLIED.
>>> +
>>> +**/
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#define MAX_IO_PORT_ADDRESS PCI_SEG2_PORTIO_MAX
>>> +
>>> +//
>>> +// Handle for the CPU I/O 2 Protocol
>>> +//
>>> +STATIC EFI_HANDLE  mHandle;
>>> +
>>> +//
>>> +// Lookup table for increment values based on transfer widths //
>>> +STATIC CONST UINT8 mInStride[] = {
>>> +  1, // EfiCpuIoWidthUint8
>>> +  2, // EfiCpuIoWidthUint16
>>> +  4, // EfiCpuIoWidthUint32
>>> +  8, // EfiCpuIoWidthUint64
>>> +  0, // EfiCpuIoWidthFifoUint8
>>> +  0, // EfiCpuIoWidthFifoUint16
>>> +  0, // EfiCpuIoWidthFifoUint32
>>> +  0, // EfiCpuIoWidthFifoUint64
>>> +  1, // EfiCpuIoWidthFillUint8
>>> +  2, // EfiCpuIoWidthFillUint16
>>> +  4, // 

Re: [edk2] [PATCH edk2-platforms 34/39] Silicon/NXP: Implement EFI_CPU_IO2_PROTOCOL

2018-04-24 Thread Vabhav Sharma


>-Original Message-
>From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>Sent: Friday, April 20, 2018 2:11 PM
>To: Meenakshi Aggarwal 
>Cc: Leif Lindholm ; Kinney, Michael D
>; edk2-devel@lists.01.org; Udit Kumar
>; Varun Sethi ; Vabhav Sharma
>
>Subject: Re: [PATCH edk2-platforms 34/39] Silicon/NXP: Implement
>EFI_CPU_IO2_PROTOCOL
>
>On 16 February 2018 at 09:50, Meenakshi 
>wrote:
>> From: Vabhav 
>>
>> NXP SOC has mutiple PCIe RCs,Adding respective implementation of
>> EFI_CPU_IO2_PROTOCOL to provide Memory Space Read/Write functions used
>> by generic Host Bridge Driver including correct value for the
>> translation offset during MMIO accesses
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Vabhav 
>> Signed-off-by: Meenakshi Aggarwal 
>
>This driver looks completely wrong to me: MMIO access is memory mapped, and
>given that you don't implement PCI to CPU translation of MMIO accesses, the
>memory read and write functions should not perform any translation at all, and
>just relay the accesses. On the other hand, the I/O accessors are not
>implemented at all, and these are the ones that require translation, given 
>that the
>I/O port addresses in the CPU space need translation to MMIO addressess.

On NXP SoC, Mapping between CPU view and PCIe view is not 1:1 and require CPU 
view translation for MMIO regions access, Accordingly translation is added 
during memory read/write services.
Bus driver relays the address range where PCIe device Bar region is split from, 
Translation is required for relaying it to correct PCIe controller cpu view 
address.

>
>Also, you don't seem to be using the PcdPciExp?BaseAddr PCDs anywhere, so you
>can drop them from the .dsc
No, It's used for checking the access to MMIO32 region and CPU view base 
address varies between different NXP SoCs
>
>> ---
>>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c   | 529
>++
>>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf |  48 ++
>>  2 files changed, 577 insertions(+)
>>  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>>  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf
>>
>> diff --git a/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>> b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>> new file mode 100644
>> index 000..b5fb72c
>> --- /dev/null
>> +++ b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>> @@ -0,0 +1,529 @@
>> +/** @file
>> +  Produces the CPU I/O 2 Protocol.
>> +
>> +  Copyright (c) 2009 - 2012, Intel Corporation. All rights
>> + reserved.  Copyright (c) 2016, Linaro Ltd. All rights
>> + reserved.  Copyright 2018 NXP
>> +
>> +  This program and the accompanying materials  are licensed and made
>> + available under the terms and conditions of the BSD License  which
>> + accompanies this distribution.  The full text of the license may be
>> + found at
>> +
>> + https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
>> + ensource.org%2Flicenses%2Fbsd-license.php=02%7C01%7Cvabhav.shar
>> +
>ma%40nxp.com%7C4507c0726b244ad6476d08d5a69a64ee%7C686ea1d3bc2b4c
>6fa9
>> +
>2cd99c5c301635%7C0%7C0%7C636598104414046440=IFSU0%2FeTdWrw
>gg0f
>> + 0Lq2qSVGgogG68tYZevrRmC%2BkV8%3D=0
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>> + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define MAX_IO_PORT_ADDRESS PCI_SEG2_PORTIO_MAX
>> +
>> +//
>> +// Handle for the CPU I/O 2 Protocol
>> +//
>> +STATIC EFI_HANDLE  mHandle;
>> +
>> +//
>> +// Lookup table for increment values based on transfer widths //
>> +STATIC CONST UINT8 mInStride[] = {
>> +  1, // EfiCpuIoWidthUint8
>> +  2, // EfiCpuIoWidthUint16
>> +  4, // EfiCpuIoWidthUint32
>> +  8, // EfiCpuIoWidthUint64
>> +  0, // EfiCpuIoWidthFifoUint8
>> +  0, // EfiCpuIoWidthFifoUint16
>> +  0, // EfiCpuIoWidthFifoUint32
>> +  0, // EfiCpuIoWidthFifoUint64
>> +  1, // EfiCpuIoWidthFillUint8
>> +  2, // EfiCpuIoWidthFillUint16
>> +  4, // EfiCpuIoWidthFillUint32
>> +  8  // EfiCpuIoWidthFillUint64
>> +};
>> +
>> +//
>> +// Lookup table for increment values based on transfer widths //
>> +STATIC CONST UINT8 mOutStride[] = {
>> +  1, // EfiCpuIoWidthUint8
>> +  2, // EfiCpuIoWidthUint16
>> +  4, // EfiCpuIoWidthUint32
>> +  8, // EfiCpuIoWidthUint64
>> +  1, // EfiCpuIoWidthFifoUint8
>> +  2, // EfiCpuIoWidthFifoUint16
>> +  4, // EfiCpuIoWidthFifoUint32
>> +  8, // EfiCpuIoWidthFifoUint64
>> +  0, // EfiCpuIoWidthFillUint8
>> +  0, // EfiCpuIoWidthFillUint16
>> +  0, // EfiCpuIoWidthFillUint32
>> +  0  // EfiCpuIoWidthFillUint64
>> +};

Re: [edk2] [PATCH edk2-platforms 34/39] Silicon/NXP: Implement EFI_CPU_IO2_PROTOCOL

2018-04-20 Thread Leif Lindholm
On Fri, Feb 16, 2018 at 02:20:30PM +0530, Meenakshi wrote:
> From: Vabhav 
> 
> NXP SOC has mutiple PCIe RCs,Adding respective implementation of
> EFI_CPU_IO2_PROTOCOL to provide Memory Space Read/Write functions
> used by generic Host Bridge Driver including correct value for
> the translation offset during MMIO accesses
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vabhav 
> Signed-off-by: Meenakshi Aggarwal 
> ---
>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c   | 529 
> ++
>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf |  48 ++
>  2 files changed, 577 insertions(+)
>  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf
> 
> diff --git a/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c 
> b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
> new file mode 100644
> index 000..b5fb72c
> --- /dev/null
> +++ b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
> @@ -0,0 +1,529 @@
> +/** @file
> +  Produces the CPU I/O 2 Protocol.
> +
> +  Copyright (c) 2009 - 2012, Intel Corporation. All rights reserved.
> +  Copyright (c) 2016, Linaro Ltd. All rights reserved.
> +  Copyright 2018 NXP
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD 
> License
> +  which accompanies this distribution.  The full text of the license may be 
> found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define MAX_IO_PORT_ADDRESS PCI_SEG2_PORTIO_MAX
> +
> +//
> +// Handle for the CPU I/O 2 Protocol
> +//
> +STATIC EFI_HANDLE  mHandle;
> +
> +//
> +// Lookup table for increment values based on transfer widths
> +//
> +STATIC CONST UINT8 mInStride[] = {
> +  1, // EfiCpuIoWidthUint8
> +  2, // EfiCpuIoWidthUint16
> +  4, // EfiCpuIoWidthUint32
> +  8, // EfiCpuIoWidthUint64
> +  0, // EfiCpuIoWidthFifoUint8
> +  0, // EfiCpuIoWidthFifoUint16
> +  0, // EfiCpuIoWidthFifoUint32
> +  0, // EfiCpuIoWidthFifoUint64
> +  1, // EfiCpuIoWidthFillUint8
> +  2, // EfiCpuIoWidthFillUint16
> +  4, // EfiCpuIoWidthFillUint32
> +  8  // EfiCpuIoWidthFillUint64
> +};
> +
> +//
> +// Lookup table for increment values based on transfer widths
> +//
> +STATIC CONST UINT8 mOutStride[] = {
> +  1, // EfiCpuIoWidthUint8
> +  2, // EfiCpuIoWidthUint16
> +  4, // EfiCpuIoWidthUint32
> +  8, // EfiCpuIoWidthUint64
> +  1, // EfiCpuIoWidthFifoUint8
> +  2, // EfiCpuIoWidthFifoUint16
> +  4, // EfiCpuIoWidthFifoUint32
> +  8, // EfiCpuIoWidthFifoUint64
> +  0, // EfiCpuIoWidthFillUint8
> +  0, // EfiCpuIoWidthFillUint16
> +  0, // EfiCpuIoWidthFillUint32
> +  0  // EfiCpuIoWidthFillUint64
> +};
> +
> +/**
> +  Check parameters to a CPU I/O 2 Protocol service request.
> +
> +  The I/O operations are carried out exactly as requested. The caller is 
> responsible
> +  for satisfying any alignment and I/O width restrictions that a PI System 
> on a
> +  platform might require. For example on some platforms, width requests of
> +  EfiCpuIoWidthUint64 do not work.
> +
> +  @param[in] MmioOperation  TRUE for an MMIO operation, FALSE for I/O Port 
> operation.
> +  @param[in] Width  Signifies the width of the I/O or Memory 
> operation.
> +  @param[in] AddressThe base address of the I/O operation.
> +  @param[in] Count  The number of I/O operations to perform. The 
> number of
> +bytes moved is Width size * Count, starting at 
> Address.
> +  @param[in] Buffer For read operations, the destination buffer to 
> store the results.
> +For write operations, the source buffer from 
> which to write data.
> +
> +  @retval EFI_SUCCESSThe parameters for this request pass the 
> checks.
> +  @retval EFI_INVALID_PARAMETER  Width is invalid for this PI system.
> +  @retval EFI_INVALID_PARAMETER  Buffer is NULL.
> +  @retval EFI_UNSUPPORTEDThe Buffer is not aligned for the given 
> Width.
> +  @retval EFI_UNSUPPORTEDThe address range specified by Address, 
> Width,
> + and Count is not valid for this PI system.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +CpuIoCheckParameter (
> +  IN BOOLEANMmioOperation,
> +  IN EFI_CPU_IO_PROTOCOL_WIDTH  Width,
> +  IN UINT64 Address,
> +  IN UINTN  Count,
> +  IN VOID   *Buffer
> +  )
> +{
> +  UINT64  MaxCount;
> +  UINT64  Limit;
> +
> +  //
> +  // Check to see if Buffer is NULL
> +  //
> +  if (Buffer == NULL) {
> +ASSERT (FALSE);
> +

Re: [edk2] [PATCH edk2-platforms 34/39] Silicon/NXP: Implement EFI_CPU_IO2_PROTOCOL

2018-04-20 Thread Ard Biesheuvel
On 16 February 2018 at 09:50, Meenakshi  wrote:
> From: Vabhav 
>
> NXP SOC has mutiple PCIe RCs,Adding respective implementation of
> EFI_CPU_IO2_PROTOCOL to provide Memory Space Read/Write functions
> used by generic Host Bridge Driver including correct value for
> the translation offset during MMIO accesses
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vabhav 
> Signed-off-by: Meenakshi Aggarwal 

This driver looks completely wrong to me: MMIO access is memory
mapped, and given that you don't implement PCI to CPU translation of
MMIO accesses, the memory read and write functions should not perform
any translation at all, and just relay the accesses. On the other
hand, the I/O accessors are not implemented at all, and these are the
ones that require translation, given that the I/O port addresses in
the CPU space need translation to MMIO addressess.

Also, you don't seem to be using the PcdPciExp?BaseAddr PCDs anywhere,
so you can drop them from the .dsc

> ---
>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c   | 529 
> ++
>  Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf |  48 ++
>  2 files changed, 577 insertions(+)
>  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
>  create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf
>
> diff --git a/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c 
> b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
> new file mode 100644
> index 000..b5fb72c
> --- /dev/null
> +++ b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
> @@ -0,0 +1,529 @@
> +/** @file
> +  Produces the CPU I/O 2 Protocol.
> +
> +  Copyright (c) 2009 - 2012, Intel Corporation. All rights reserved.
> +  Copyright (c) 2016, Linaro Ltd. All rights reserved.
> +  Copyright 2018 NXP
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD 
> License
> +  which accompanies this distribution.  The full text of the license may be 
> found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define MAX_IO_PORT_ADDRESS PCI_SEG2_PORTIO_MAX
> +
> +//
> +// Handle for the CPU I/O 2 Protocol
> +//
> +STATIC EFI_HANDLE  mHandle;
> +
> +//
> +// Lookup table for increment values based on transfer widths
> +//
> +STATIC CONST UINT8 mInStride[] = {
> +  1, // EfiCpuIoWidthUint8
> +  2, // EfiCpuIoWidthUint16
> +  4, // EfiCpuIoWidthUint32
> +  8, // EfiCpuIoWidthUint64
> +  0, // EfiCpuIoWidthFifoUint8
> +  0, // EfiCpuIoWidthFifoUint16
> +  0, // EfiCpuIoWidthFifoUint32
> +  0, // EfiCpuIoWidthFifoUint64
> +  1, // EfiCpuIoWidthFillUint8
> +  2, // EfiCpuIoWidthFillUint16
> +  4, // EfiCpuIoWidthFillUint32
> +  8  // EfiCpuIoWidthFillUint64
> +};
> +
> +//
> +// Lookup table for increment values based on transfer widths
> +//
> +STATIC CONST UINT8 mOutStride[] = {
> +  1, // EfiCpuIoWidthUint8
> +  2, // EfiCpuIoWidthUint16
> +  4, // EfiCpuIoWidthUint32
> +  8, // EfiCpuIoWidthUint64
> +  1, // EfiCpuIoWidthFifoUint8
> +  2, // EfiCpuIoWidthFifoUint16
> +  4, // EfiCpuIoWidthFifoUint32
> +  8, // EfiCpuIoWidthFifoUint64
> +  0, // EfiCpuIoWidthFillUint8
> +  0, // EfiCpuIoWidthFillUint16
> +  0, // EfiCpuIoWidthFillUint32
> +  0  // EfiCpuIoWidthFillUint64
> +};
> +
> +/**
> +  Check parameters to a CPU I/O 2 Protocol service request.
> +
> +  The I/O operations are carried out exactly as requested. The caller is 
> responsible
> +  for satisfying any alignment and I/O width restrictions that a PI System 
> on a
> +  platform might require. For example on some platforms, width requests of
> +  EfiCpuIoWidthUint64 do not work.
> +
> +  @param[in] MmioOperation  TRUE for an MMIO operation, FALSE for I/O Port 
> operation.
> +  @param[in] Width  Signifies the width of the I/O or Memory 
> operation.
> +  @param[in] AddressThe base address of the I/O operation.
> +  @param[in] Count  The number of I/O operations to perform. The 
> number of
> +bytes moved is Width size * Count, starting at 
> Address.
> +  @param[in] Buffer For read operations, the destination buffer to 
> store the results.
> +For write operations, the source buffer from 
> which to write data.
> +
> +  @retval EFI_SUCCESSThe parameters for this request pass the 
> checks.
> +  @retval EFI_INVALID_PARAMETER  Width is invalid for this PI system.
> +  @retval EFI_INVALID_PARAMETER  Buffer is NULL.
> +  @retval EFI_UNSUPPORTEDThe Buffer is not aligned for the given 
> Width.
> +  @retval EFI_UNSUPPORTED

[edk2] [PATCH edk2-platforms 34/39] Silicon/NXP: Implement EFI_CPU_IO2_PROTOCOL

2018-02-16 Thread Meenakshi
From: Vabhav 

NXP SOC has mutiple PCIe RCs,Adding respective implementation of
EFI_CPU_IO2_PROTOCOL to provide Memory Space Read/Write functions
used by generic Host Bridge Driver including correct value for
the translation offset during MMIO accesses

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Vabhav 
Signed-off-by: Meenakshi Aggarwal 
---
 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c   | 529 ++
 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf |  48 ++
 2 files changed, 577 insertions(+)
 create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
 create mode 100644 Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.inf

diff --git a/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c 
b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
new file mode 100644
index 000..b5fb72c
--- /dev/null
+++ b/Silicon/NXP/Drivers/PciCpuIo2Dxe/PciCpuIo2Dxe.c
@@ -0,0 +1,529 @@
+/** @file
+  Produces the CPU I/O 2 Protocol.
+
+  Copyright (c) 2009 - 2012, Intel Corporation. All rights reserved.
+  Copyright (c) 2016, Linaro Ltd. All rights reserved.
+  Copyright 2018 NXP
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD 
License
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define MAX_IO_PORT_ADDRESS PCI_SEG2_PORTIO_MAX
+
+//
+// Handle for the CPU I/O 2 Protocol
+//
+STATIC EFI_HANDLE  mHandle;
+
+//
+// Lookup table for increment values based on transfer widths
+//
+STATIC CONST UINT8 mInStride[] = {
+  1, // EfiCpuIoWidthUint8
+  2, // EfiCpuIoWidthUint16
+  4, // EfiCpuIoWidthUint32
+  8, // EfiCpuIoWidthUint64
+  0, // EfiCpuIoWidthFifoUint8
+  0, // EfiCpuIoWidthFifoUint16
+  0, // EfiCpuIoWidthFifoUint32
+  0, // EfiCpuIoWidthFifoUint64
+  1, // EfiCpuIoWidthFillUint8
+  2, // EfiCpuIoWidthFillUint16
+  4, // EfiCpuIoWidthFillUint32
+  8  // EfiCpuIoWidthFillUint64
+};
+
+//
+// Lookup table for increment values based on transfer widths
+//
+STATIC CONST UINT8 mOutStride[] = {
+  1, // EfiCpuIoWidthUint8
+  2, // EfiCpuIoWidthUint16
+  4, // EfiCpuIoWidthUint32
+  8, // EfiCpuIoWidthUint64
+  1, // EfiCpuIoWidthFifoUint8
+  2, // EfiCpuIoWidthFifoUint16
+  4, // EfiCpuIoWidthFifoUint32
+  8, // EfiCpuIoWidthFifoUint64
+  0, // EfiCpuIoWidthFillUint8
+  0, // EfiCpuIoWidthFillUint16
+  0, // EfiCpuIoWidthFillUint32
+  0  // EfiCpuIoWidthFillUint64
+};
+
+/**
+  Check parameters to a CPU I/O 2 Protocol service request.
+
+  The I/O operations are carried out exactly as requested. The caller is 
responsible
+  for satisfying any alignment and I/O width restrictions that a PI System on a
+  platform might require. For example on some platforms, width requests of
+  EfiCpuIoWidthUint64 do not work.
+
+  @param[in] MmioOperation  TRUE for an MMIO operation, FALSE for I/O Port 
operation.
+  @param[in] Width  Signifies the width of the I/O or Memory operation.
+  @param[in] AddressThe base address of the I/O operation.
+  @param[in] Count  The number of I/O operations to perform. The 
number of
+bytes moved is Width size * Count, starting at 
Address.
+  @param[in] Buffer For read operations, the destination buffer to 
store the results.
+For write operations, the source buffer from which 
to write data.
+
+  @retval EFI_SUCCESSThe parameters for this request pass the 
checks.
+  @retval EFI_INVALID_PARAMETER  Width is invalid for this PI system.
+  @retval EFI_INVALID_PARAMETER  Buffer is NULL.
+  @retval EFI_UNSUPPORTEDThe Buffer is not aligned for the given Width.
+  @retval EFI_UNSUPPORTEDThe address range specified by Address, Width,
+ and Count is not valid for this PI system.
+
+**/
+STATIC
+EFI_STATUS
+CpuIoCheckParameter (
+  IN BOOLEANMmioOperation,
+  IN EFI_CPU_IO_PROTOCOL_WIDTH  Width,
+  IN UINT64 Address,
+  IN UINTN  Count,
+  IN VOID   *Buffer
+  )
+{
+  UINT64  MaxCount;
+  UINT64  Limit;
+
+  //
+  // Check to see if Buffer is NULL
+  //
+  if (Buffer == NULL) {
+ASSERT (FALSE);
+return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // Check to see if Width is in the valid range
+  //
+  if ((UINT32)Width >= EfiCpuIoWidthMaximum) {
+ASSERT (FALSE);
+return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // For FIFO type, the target address won't increase during the access,
+  // so treat Count as 1
+  //
+  if (Width >= EfiCpuIoWidthFifoUint8 &&