public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] Enable using device address when programming BARs
@ 2018-05-03 22:54 Roman Bacik
  2018-05-09 20:17 ` Roman Bacik
  0 siblings, 1 reply; 8+ messages in thread
From: Roman Bacik @ 2018-05-03 22:54 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni, Vladimir Olovyannikov

Some SoCs require to use device address when BARs are programmed:
https://bugzilla.tianocore.org/show_bug.cgi?id=948

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf        | 1 +
 MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +++++---
 MdeModulePkg/MdeModulePkg.dec                       | 3 +++
 MdeModulePkg/MdeModulePkg.dsc                       | 1 +
 4 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
index 97608bfcf245..1368e5068574 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
@@ -110,6 +110,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport                  ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport                ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration    ##
SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress        ## CONSUMES

 [UserExtensions.TianoCore."ExtraFiles"]
   PciBusDxeExtra.uni
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
index 2f713fcee95e..a23bd1e258ef 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
@@ -1269,6 +1269,7 @@ ProgramBar (
   EFI_PCI_IO_PROTOCOL *PciIo;
   UINT64              Address;
   UINT32              Address32;
+  BOOLEAN             UseDeviceAddress;

   ASSERT (Node->Bar < PCI_MAX_BAR);

@@ -1282,8 +1283,9 @@ ProgramBar (

   Address = 0;
   PciIo   = &(Node->PciDev->PciIo);
+  UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress);

-  Address = Base + Node->Offset;
+  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;

   //
   // Indicate pci bus driver has allocated
@@ -1308,7 +1310,7 @@ ProgramBar (
                  &Address
                  );

-    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base +
Address: Address;

     break;

@@ -1335,7 +1337,7 @@ ProgramBar (
                  &Address32
                  );

-    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base +
Address: Address;

     break;

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index cc397185f7b9..58425ee0d57f 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1005,6 +1005,9 @@
   # @Prompt Enable UEFI Stack Guard.
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055

+  ## Indicates whether the device address should be used for BAR
programming
+
 gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE|BOOLEAN|0x30001056
+
 [PcdsFixedAtBuild, PcdsPatchableInModule]
   ## Dynamic type PCD can be registered callback function for Pcd setting
action.
   #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of
callback function
diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index ec24a50c7d0a..39b397cb13d9 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -200,6 +200,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|0x0
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizePopulateCapsule|0x0
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPerformanceLogEntries|28
+  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE

 [PcdsFixedAtBuild.IPF]
   gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0ffffc000000
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] Enable using device address when programming BARs
  2018-05-03 22:54 [PATCH] Enable using device address when programming BARs Roman Bacik
@ 2018-05-09 20:17 ` Roman Bacik
  2018-05-13 10:25   ` Ard Biesheuvel
  0 siblings, 1 reply; 8+ messages in thread
From: Roman Bacik @ 2018-05-09 20:17 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni, Vladimir Olovyannikov

I will upload v2 with the corrected subject - add package name MdeModulePkg/Bus
.



*From:* Roman Bacik [mailto:roman.bacik@broadcom.com]
*Sent:* Thursday, May 3, 2018 3:55 PM
*To:* edk2-devel@lists.01.org
*Cc:* Ruiyu Ni; Vladimir Olovyannikov
*Subject:* [edk2] [PATCH] Enable using device address when programming BARs



Some SoCs require to use device address when BARs are programmed:
https://bugzilla.tianocore.org/show_bug.cgi?id=948

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf        | 1 +
 MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +++++---
 MdeModulePkg/MdeModulePkg.dec                       | 3 +++
 MdeModulePkg/MdeModulePkg.dsc                       | 1 +
 4 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
index 97608bfcf245..1368e5068574 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
@@ -110,6 +110,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport                  ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport                ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration    ##
SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress        ## CONSUMES

 [UserExtensions.TianoCore."ExtraFiles"]
   PciBusDxeExtra.uni
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
index 2f713fcee95e..a23bd1e258ef 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
@@ -1269,6 +1269,7 @@ ProgramBar (
   EFI_PCI_IO_PROTOCOL *PciIo;
   UINT64              Address;
   UINT32              Address32;
+  BOOLEAN             UseDeviceAddress;

   ASSERT (Node->Bar < PCI_MAX_BAR);

@@ -1282,8 +1283,9 @@ ProgramBar (

   Address = 0;
   PciIo   = &(Node->PciDev->PciIo);
+  UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress);

-  Address = Base + Node->Offset;
+  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;

   //
   // Indicate pci bus driver has allocated
@@ -1308,7 +1310,7 @@ ProgramBar (
                  &Address
                  );

-    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base +
Address: Address;

     break;

@@ -1335,7 +1337,7 @@ ProgramBar (
                  &Address32
                  );

-    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base +
Address: Address;

     break;

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index cc397185f7b9..58425ee0d57f 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1005,6 +1005,9 @@
   # @Prompt Enable UEFI Stack Guard.
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055

+  ## Indicates whether the device address should be used for BAR
programming
+
 gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE|BOOLEAN|0x30001056
+
 [PcdsFixedAtBuild, PcdsPatchableInModule]
   ## Dynamic type PCD can be registered callback function for Pcd setting
action.
   #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of
callback function
diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index ec24a50c7d0a..39b397cb13d9 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -200,6 +200,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|0x0
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizePopulateCapsule|0x0
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPerformanceLogEntries|28
+  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE

 [PcdsFixedAtBuild.IPF]
   gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0ffffc000000
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] Enable using device address when programming BARs
  2018-05-09 20:17 ` Roman Bacik
@ 2018-05-13 10:25   ` Ard Biesheuvel
  2018-05-14 17:28     ` Roman Bacik
  0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2018-05-13 10:25 UTC (permalink / raw)
  To: Roman Bacik; +Cc: edk2-devel@lists.01.org, Ruiyu Ni, Vladimir Olovyannikov

On 9 May 2018 at 22:17, Roman Bacik <roman.bacik@broadcom.com> wrote:
> I will upload v2 with the corrected subject - add package name MdeModulePkg/Bus
> .
>

I don't think this is the correct approach. Please use the address
translation support that has been added recently to PciHostBridgeDxe
and PciHostBridgeLib.

>
>
> *From:* Roman Bacik [mailto:roman.bacik@broadcom.com]
> *Sent:* Thursday, May 3, 2018 3:55 PM
> *To:* edk2-devel@lists.01.org
> *Cc:* Ruiyu Ni; Vladimir Olovyannikov
> *Subject:* [edk2] [PATCH] Enable using device address when programming BARs
>
>
>
> Some SoCs require to use device address when BARs are programmed:
> https://bugzilla.tianocore.org/show_bug.cgi?id=948
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf        | 1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +++++---
>  MdeModulePkg/MdeModulePkg.dec                       | 3 +++
>  MdeModulePkg/MdeModulePkg.dsc                       | 1 +
>  4 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> index 97608bfcf245..1368e5068574 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> @@ -110,6 +110,7 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport                  ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport                ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration    ##
> SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress        ## CONSUMES
>
>  [UserExtensions.TianoCore."ExtraFiles"]
>    PciBusDxeExtra.uni
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index 2f713fcee95e..a23bd1e258ef 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -1269,6 +1269,7 @@ ProgramBar (
>    EFI_PCI_IO_PROTOCOL *PciIo;
>    UINT64              Address;
>    UINT32              Address32;
> +  BOOLEAN             UseDeviceAddress;
>
>    ASSERT (Node->Bar < PCI_MAX_BAR);
>
> @@ -1282,8 +1283,9 @@ ProgramBar (
>
>    Address = 0;
>    PciIo   = &(Node->PciDev->PciIo);
> +  UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress);
>
> -  Address = Base + Node->Offset;
> +  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;
>
>    //
>    // Indicate pci bus driver has allocated
> @@ -1308,7 +1310,7 @@ ProgramBar (
>                   &Address
>                   );
>
> -    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
> +    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base +
> Address: Address;
>
>      break;
>
> @@ -1335,7 +1337,7 @@ ProgramBar (
>                   &Address32
>                   );
>
> -    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
> +    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base +
> Address: Address;
>
>      break;
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index cc397185f7b9..58425ee0d57f 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1005,6 +1005,9 @@
>    # @Prompt Enable UEFI Stack Guard.
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055
>
> +  ## Indicates whether the device address should be used for BAR
> programming
> +
>  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE|BOOLEAN|0x30001056
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule]
>    ## Dynamic type PCD can be registered callback function for Pcd setting
> action.
>    #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of
> callback function
> diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
> index ec24a50c7d0a..39b397cb13d9 100644
> --- a/MdeModulePkg/MdeModulePkg.dsc
> +++ b/MdeModulePkg/MdeModulePkg.dsc
> @@ -200,6 +200,7 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|0x0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizePopulateCapsule|0x0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPerformanceLogEntries|28
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE
>
>  [PcdsFixedAtBuild.IPF]
>    gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0ffffc000000
> --
> 1.9.1
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] Enable using device address when programming BARs
  2018-05-13 10:25   ` Ard Biesheuvel
@ 2018-05-14 17:28     ` Roman Bacik
  2018-05-14 17:35       ` Ard Biesheuvel
  0 siblings, 1 reply; 8+ messages in thread
From: Roman Bacik @ 2018-05-14 17:28 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel, Ruiyu Ni, Vladimir Olovyannikov

Ard,

Thank you very much for your comment.

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Sunday, May 13, 2018 3:25 AM
> To: Roman Bacik
> Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov
> Subject: Re: [edk2] [PATCH] Enable using device address when programming
> BARs
>
> On 9 May 2018 at 22:17, Roman Bacik <roman.bacik@broadcom.com> wrote:
> > I will upload v2 with the corrected subject - add package name
> > MdeModulePkg/Bus .
> >
>
> I don't think this is the correct approach. Please use the address
> translation
> support that has been added recently to PciHostBridgeDxe and
> PciHostBridgeLib.
>

Would you like to see this change:

  Address = Base + Node->Offset;
+ if (UseDeviceAddress)
+  Address = TO_DEVICE_ADDRESS(Address, -Base);

Instead of:

-  Address = Base + Node->Offset;
+  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;

> >
> >
> > *From:* Roman Bacik [mailto:roman.bacik@broadcom.com]
> > *Sent:* Thursday, May 3, 2018 3:55 PM
> > *To:* edk2-devel@lists.01.org
> > *Cc:* Ruiyu Ni; Vladimir Olovyannikov
> > *Subject:* [edk2] [PATCH] Enable using device address when programming
> > BARs
> >
> >
> >
> > Some SoCs require to use device address when BARs are programmed:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=948
> >
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf        | 1 +
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +++++---
> >  MdeModulePkg/MdeModulePkg.dec                       | 3 +++
> >  MdeModulePkg/MdeModulePkg.dsc                       | 1 +
> >  4 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> > index 97608bfcf245..1368e5068574 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> > @@ -110,6 +110,7 @@
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport                  ##
> CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport                ##
> CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration    ##
> > SOMETIMES_CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress        ##
> CONSUMES
> >
> >  [UserExtensions.TianoCore."ExtraFiles"]
> >    PciBusDxeExtra.uni
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> > index 2f713fcee95e..a23bd1e258ef 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> > @@ -1269,6 +1269,7 @@ ProgramBar (
> >    EFI_PCI_IO_PROTOCOL *PciIo;
> >    UINT64              Address;
> >    UINT32              Address32;
> > +  BOOLEAN             UseDeviceAddress;
> >
> >    ASSERT (Node->Bar < PCI_MAX_BAR);
> >
> > @@ -1282,8 +1283,9 @@ ProgramBar (
> >
> >    Address = 0;
> >    PciIo   = &(Node->PciDev->PciIo);
> > +  UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress);
> >
> > -  Address = Base + Node->Offset;
> > +  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;
> >
> >    //
> >    // Indicate pci bus driver has allocated @@ -1308,7 +1310,7 @@
> > ProgramBar (
> >                   &Address
> >                   );
> >
> > -    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
> > +    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress?
> > + Base +
> > Address: Address;
> >
> >      break;
> >
> > @@ -1335,7 +1337,7 @@ ProgramBar (
> >                   &Address32
> >                   );
> >
> > -    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
> > +    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress?
> > + Base +
> > Address: Address;
> >
> >      break;
> >
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec index cc397185f7b9..58425ee0d57f
> > 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -1005,6 +1005,9 @@
> >    # @Prompt Enable UEFI Stack Guard.
> >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0
> x300010
> > 55
> >
> > +  ## Indicates whether the device address should be used for BAR
> > programming
> > +
> >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE|BOOLEA
> N|0x300
> > 01056
> > +
> >  [PcdsFixedAtBuild, PcdsPatchableInModule]
> >    ## Dynamic type PCD can be registered callback function for Pcd
> > setting action.
> >    #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum
> > number of callback function diff --git
> a/MdeModulePkg/MdeModulePkg.dsc
> > b/MdeModulePkg/MdeModulePkg.dsc index
> ec24a50c7d0a..39b397cb13d9
> > 100644
> > --- a/MdeModulePkg/MdeModulePkg.dsc
> > +++ b/MdeModulePkg/MdeModulePkg.dsc
> > @@ -200,6 +200,7 @@
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|0x0
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizePopulateCapsule|0x0
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPerformanceLogEntries|28
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE
> >
> >  [PcdsFixedAtBuild.IPF]
> >
> gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0ffffc000000
> > --
> > 1.9.1
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Thanks,

Roman


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] Enable using device address when programming BARs
  2018-05-14 17:28     ` Roman Bacik
@ 2018-05-14 17:35       ` Ard Biesheuvel
  2018-05-14 18:02         ` Roman Bacik
  2018-05-14 18:04         ` Laszlo Ersek
  0 siblings, 2 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2018-05-14 17:35 UTC (permalink / raw)
  To: Roman Bacik; +Cc: edk2-devel@lists.01.org, Ruiyu Ni, Vladimir Olovyannikov

On 14 May 2018 at 19:28, Roman Bacik <roman.bacik@broadcom.com> wrote:
> Ard,
>
> Thank you very much for your comment.
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Sunday, May 13, 2018 3:25 AM
>> To: Roman Bacik
>> Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov
>> Subject: Re: [edk2] [PATCH] Enable using device address when programming
>> BARs
>>
>> On 9 May 2018 at 22:17, Roman Bacik <roman.bacik@broadcom.com> wrote:
>> > I will upload v2 with the corrected subject - add package name
>> > MdeModulePkg/Bus .
>> >
>>
>> I don't think this is the correct approach. Please use the address
>> translation
>> support that has been added recently to PciHostBridgeDxe and
>> PciHostBridgeLib.
>>
>
> Would you like to see this change:
>
>   Address = Base + Node->Offset;
> + if (UseDeviceAddress)
> +  Address = TO_DEVICE_ADDRESS(Address, -Base);
>
> Instead of:
>
> -  Address = Base + Node->Offset;
> +  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;
>

No.

Programming BARs should always involve device addresses, never CPU
addresses. If you wire up the MMIO translation support correctly, the
existing code will already do what you want.

>> >
>> >
>> > *From:* Roman Bacik [mailto:roman.bacik@broadcom.com]
>> > *Sent:* Thursday, May 3, 2018 3:55 PM
>> > *To:* edk2-devel@lists.01.org
>> > *Cc:* Ruiyu Ni; Vladimir Olovyannikov
>> > *Subject:* [edk2] [PATCH] Enable using device address when programming
>> > BARs
>> >
>> >
>> >
>> > Some SoCs require to use device address when BARs are programmed:
>> > https://bugzilla.tianocore.org/show_bug.cgi?id=948
>> >
>> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> > Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
>> > ---
>> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf        | 1 +
>> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +++++---
>> >  MdeModulePkg/MdeModulePkg.dec                       | 3 +++
>> >  MdeModulePkg/MdeModulePkg.dsc                       | 1 +
>> >  4 files changed, 10 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> > index 97608bfcf245..1368e5068574 100644
>> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> > @@ -110,6 +110,7 @@
>> >    gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport                  ##
>> CONSUMES
>> >    gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport                ##
>> CONSUMES
>> >    gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration    ##
>> > SOMETIMES_CONSUMES
>> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress        ##
>> CONSUMES
>> >
>> >  [UserExtensions.TianoCore."ExtraFiles"]
>> >    PciBusDxeExtra.uni
>> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> > index 2f713fcee95e..a23bd1e258ef 100644
>> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> > @@ -1269,6 +1269,7 @@ ProgramBar (
>> >    EFI_PCI_IO_PROTOCOL *PciIo;
>> >    UINT64              Address;
>> >    UINT32              Address32;
>> > +  BOOLEAN             UseDeviceAddress;
>> >
>> >    ASSERT (Node->Bar < PCI_MAX_BAR);
>> >
>> > @@ -1282,8 +1283,9 @@ ProgramBar (
>> >
>> >    Address = 0;
>> >    PciIo   = &(Node->PciDev->PciIo);
>> > +  UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress);
>> >
>> > -  Address = Base + Node->Offset;
>> > +  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;
>> >
>> >    //
>> >    // Indicate pci bus driver has allocated @@ -1308,7 +1310,7 @@
>> > ProgramBar (
>> >                   &Address
>> >                   );
>> >
>> > -    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
>> > +    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress?
>> > + Base +
>> > Address: Address;
>> >
>> >      break;
>> >
>> > @@ -1335,7 +1337,7 @@ ProgramBar (
>> >                   &Address32
>> >                   );
>> >
>> > -    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
>> > +    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress?
>> > + Base +
>> > Address: Address;
>> >
>> >      break;
>> >
>> > diff --git a/MdeModulePkg/MdeModulePkg.dec
>> > b/MdeModulePkg/MdeModulePkg.dec index cc397185f7b9..58425ee0d57f
>> > 100644
>> > --- a/MdeModulePkg/MdeModulePkg.dec
>> > +++ b/MdeModulePkg/MdeModulePkg.dec
>> > @@ -1005,6 +1005,9 @@
>> >    # @Prompt Enable UEFI Stack Guard.
>> >
>> >
>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0
>> x300010
>> > 55
>> >
>> > +  ## Indicates whether the device address should be used for BAR
>> > programming
>> > +
>> >
>> >
>> gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE|BOOLEA
>> N|0x300
>> > 01056
>> > +
>> >  [PcdsFixedAtBuild, PcdsPatchableInModule]
>> >    ## Dynamic type PCD can be registered callback function for Pcd
>> > setting action.
>> >    #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum
>> > number of callback function diff --git
>> a/MdeModulePkg/MdeModulePkg.dsc
>> > b/MdeModulePkg/MdeModulePkg.dsc index
>> ec24a50c7d0a..39b397cb13d9
>> > 100644
>> > --- a/MdeModulePkg/MdeModulePkg.dsc
>> > +++ b/MdeModulePkg/MdeModulePkg.dsc
>> > @@ -200,6 +200,7 @@
>> >
>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|0x0
>> >    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizePopulateCapsule|0x0
>> >
>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPerformanceLogEntries|28
>> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE
>> >
>> >  [PcdsFixedAtBuild.IPF]
>> >
>> gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0ffffc000000
>> > --
>> > 1.9.1
>> > _______________________________________________
>> > edk2-devel mailing list
>> > edk2-devel@lists.01.org
>> > https://lists.01.org/mailman/listinfo/edk2-devel
>
> Thanks,
>
> Roman


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] Enable using device address when programming BARs
  2018-05-14 17:35       ` Ard Biesheuvel
@ 2018-05-14 18:02         ` Roman Bacik
  2018-05-14 18:04         ` Laszlo Ersek
  1 sibling, 0 replies; 8+ messages in thread
From: Roman Bacik @ 2018-05-14 18:02 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel, Ruiyu Ni, Vladimir Olovyannikov

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, May 14, 2018 10:35 AM
> To: Roman Bacik
> Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov
> Subject: Re: [edk2] [PATCH] Enable using device address when programming
> BARs
>
> On 14 May 2018 at 19:28, Roman Bacik <roman.bacik@broadcom.com> wrote:
> > Ard,
> >
> > Thank you very much for your comment.
> >
> >> -----Original Message-----
> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >> Sent: Sunday, May 13, 2018 3:25 AM
> >> To: Roman Bacik
> >> Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov
> >> Subject: Re: [edk2] [PATCH] Enable using device address when
> >> programming BARs
> >>
> >> On 9 May 2018 at 22:17, Roman Bacik <roman.bacik@broadcom.com>
> wrote:
> >> > I will upload v2 with the corrected subject - add package name
> >> > MdeModulePkg/Bus .
> >> >
> >>
> >> I don't think this is the correct approach. Please use the address
> >> translation support that has been added recently to PciHostBridgeDxe
> >> and PciHostBridgeLib.
> >>
> >
> > Would you like to see this change:
> >
> >   Address = Base + Node->Offset;
> > + if (UseDeviceAddress)
> > +  Address = TO_DEVICE_ADDRESS(Address, -Base);
> >
> > Instead of:
> >
> > -  Address = Base + Node->Offset;
> > +  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;
> >
>
> No.
>
> Programming BARs should always involve device addresses, never CPU
> addresses. If you wire up the MMIO translation support correctly, the
> existing code will already do what you want.
>

Our code has base address 0x60000000 and offset 0x200. We need Address=0x200
to be passed to PciIo->Pci.Write() and Address=0x60000200 to be stored in
Node->PciDev->PciBar[Node->Bar].BaseAddress. The code as is does not seem to
do that unless PciIo->Pci.Write() itself changes the content of the Address
from 0x200 to 0x60000200 for us.

So if we modify MMIO and the existing code gets the correct Address 0x200
being passed to PciIo->Pci.Write() then it means Address = 0x200 will be
stored into Node->PciDev->PciBar[Node->Bar].BaseAddress, which would be
wrong in our case. It appears that regardless how we change MMIO translation
the existing code would not work for us as is. Or I may be missing
something.

> >> >
> >> >
> >> > *From:* Roman Bacik [mailto:roman.bacik@broadcom.com]
> >> > *Sent:* Thursday, May 3, 2018 3:55 PM
> >> > *To:* edk2-devel@lists.01.org
> >> > *Cc:* Ruiyu Ni; Vladimir Olovyannikov
> >> > *Subject:* [edk2] [PATCH] Enable using device address when
> >> > programming BARs
> >> >
> >> >
> >> >
> >> > Some SoCs require to use device address when BARs are programmed:
> >> > https://bugzilla.tianocore.org/show_bug.cgi?id=948
> >> >
> >> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> > Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
> >> > ---
> >> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf        | 1 +
> >> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +++++---
> >> >  MdeModulePkg/MdeModulePkg.dec                       | 3 +++
> >> >  MdeModulePkg/MdeModulePkg.dsc                       | 1 +
> >> >  4 files changed, 10 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> >> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> >> > index 97608bfcf245..1368e5068574 100644
> >> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> >> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> >> > @@ -110,6 +110,7 @@
> >> >    gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport                  ##
> >> CONSUMES
> >> >    gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport                ##
> >> CONSUMES
> >> >    gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
> ##
> >> > SOMETIMES_CONSUMES
> >> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress        ##
> >> CONSUMES
> >> >
> >> >  [UserExtensions.TianoCore."ExtraFiles"]
> >> >    PciBusDxeExtra.uni
> >> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> >> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> >> > index 2f713fcee95e..a23bd1e258ef 100644
> >> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> >> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> >> > @@ -1269,6 +1269,7 @@ ProgramBar (
> >> >    EFI_PCI_IO_PROTOCOL *PciIo;
> >> >    UINT64              Address;
> >> >    UINT32              Address32;
> >> > +  BOOLEAN             UseDeviceAddress;
> >> >
> >> >    ASSERT (Node->Bar < PCI_MAX_BAR);
> >> >
> >> > @@ -1282,8 +1283,9 @@ ProgramBar (
> >> >
> >> >    Address = 0;
> >> >    PciIo   = &(Node->PciDev->PciIo);
> >> > +  UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress);
> >> >
> >> > -  Address = Base + Node->Offset;
> >> > +  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;
> >> >
> >> >    //
> >> >    // Indicate pci bus driver has allocated @@ -1308,7 +1310,7 @@
> >> > ProgramBar (
> >> >                   &Address
> >> >                   );
> >> >
> >> > -    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
> >> > +    Node->PciDev->PciBar[Node->Bar].BaseAddress =
> UseDeviceAddress?
> >> > + Base +
> >> > Address: Address;
> >> >
> >> >      break;
> >> >
> >> > @@ -1335,7 +1337,7 @@ ProgramBar (
> >> >                   &Address32
> >> >                   );
> >> >
> >> > -    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
> >> > +    Node->PciDev->PciBar[Node->Bar].BaseAddress =
> UseDeviceAddress?
> >> > + Base +
> >> > Address: Address;
> >> >
> >> >      break;
> >> >
> >> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> >> > b/MdeModulePkg/MdeModulePkg.dec index
> cc397185f7b9..58425ee0d57f
> >> > 100644
> >> > --- a/MdeModulePkg/MdeModulePkg.dec
> >> > +++ b/MdeModulePkg/MdeModulePkg.dec
> >> > @@ -1005,6 +1005,9 @@
> >> >    # @Prompt Enable UEFI Stack Guard.
> >> >
> >> >
> >>
> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0
> >> x300010
> >> > 55
> >> >
> >> > +  ## Indicates whether the device address should be used for BAR
> >> > programming
> >> > +
> >> >
> >> >
> >>
> gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE|BOOLEA
> >> N|0x300
> >> > 01056
> >> > +
> >> >  [PcdsFixedAtBuild, PcdsPatchableInModule]
> >> >    ## Dynamic type PCD can be registered callback function for Pcd
> >> > setting action.
> >> >    #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum
> >> > number of callback function diff --git
> >> a/MdeModulePkg/MdeModulePkg.dsc
> >> > b/MdeModulePkg/MdeModulePkg.dsc index
> >> ec24a50c7d0a..39b397cb13d9
> >> > 100644
> >> > --- a/MdeModulePkg/MdeModulePkg.dsc
> >> > +++ b/MdeModulePkg/MdeModulePkg.dsc
> >> > @@ -200,6 +200,7 @@
> >> >
> >>
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|0x0
> >> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizePopulateCapsule|0x0
> >> >
> >>
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPerformanceLogEntries|28
> >> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE
> >> >
> >> >  [PcdsFixedAtBuild.IPF]
> >> >
> >>
> gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0ffffc000000
> >> > --
> >> > 1.9.1
> >> > _______________________________________________
> >> > edk2-devel mailing list
> >> > edk2-devel@lists.01.org
> >> > https://lists.01.org/mailman/listinfo/edk2-devel
> >
> > Thanks,
> >
> > Roman


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] Enable using device address when programming BARs
  2018-05-14 17:35       ` Ard Biesheuvel
  2018-05-14 18:02         ` Roman Bacik
@ 2018-05-14 18:04         ` Laszlo Ersek
  2018-05-15 14:40           ` Roman Bacik
  1 sibling, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2018-05-14 18:04 UTC (permalink / raw)
  To: Roman Bacik
  Cc: Ard Biesheuvel, Ruiyu Ni, edk2-devel@lists.01.org,
	Vladimir Olovyannikov

Hi Roman,

On 05/14/18 19:35, Ard Biesheuvel wrote:
> On 14 May 2018 at 19:28, Roman Bacik <roman.bacik@broadcom.com> wrote:
>> Ard,
>>
>> Thank you very much for your comment.
>>
>>> -----Original Message-----
>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>> Sent: Sunday, May 13, 2018 3:25 AM
>>> To: Roman Bacik
>>> Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov
>>> Subject: Re: [edk2] [PATCH] Enable using device address when
>>> programming BARs
>>>
>>> On 9 May 2018 at 22:17, Roman Bacik <roman.bacik@broadcom.com>
>>> wrote:
>>>> I will upload v2 with the corrected subject - add package name
>>>> MdeModulePkg/Bus .
>>>>
>>>
>>> I don't think this is the correct approach. Please use the address
>>> translation
>>> support that has been added recently to PciHostBridgeDxe and
>>> PciHostBridgeLib.
>>>
>>
>> Would you like to see this change:
>>
>>   Address = Base + Node->Offset;
>> + if (UseDeviceAddress)
>> +  Address = TO_DEVICE_ADDRESS(Address, -Base);
>>
>> Instead of:
>>
>> -  Address = Base + Node->Offset;
>> +  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;
>>
>
> No.
>
> Programming BARs should always involve device addresses, never CPU
> addresses. If you wire up the MMIO translation support correctly, the
> existing code will already do what you want.

To clarify a bit, please look at the following commits:

  1  5bb1866e5383 MdeModulePkg/PciHostBridgeLib.h: add address Translation
  2  74d0a339b832 MdeModulePkg/PciHostBridgeDxe: Add support for address translation
  3  c03860d05233 MdeModulePkg/PciBus: convert host address to device address
  4  dc080d3b61e5 MdeModulePkg/PciBus: return CPU address for GetBarAttributes

in particular, as Ard's first response suggests, the new field
introduced in commit 5bb1866e5383. The PciHostBridgeLib instance that
your platform plugs into MdeModulePkg/PciHostBridgeDxe should populate
this field correctly. Then PciHostBridgeDxe and PciBusDxe will do the
right thing for you.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] Enable using device address when programming BARs
  2018-05-14 18:04         ` Laszlo Ersek
@ 2018-05-15 14:40           ` Roman Bacik
  0 siblings, 0 replies; 8+ messages in thread
From: Roman Bacik @ 2018-05-15 14:40 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: Ard Biesheuvel, Ruiyu Ni, edk2-devel, Vladimir Olovyannikov

Hi Laszlo, Ard,

After seeing the commits you have pointed out your comments are starting to
be more clear. We will try to follow your suggestions and will let you know
if it works for us.
Thank you very much for your explanation,

Roman

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, May 14, 2018 11:05 AM
> To: Roman Bacik
> Cc: Ard Biesheuvel; Ruiyu Ni; edk2-devel@lists.01.org; Vladimir
> Olovyannikov
> Subject: Re: [edk2] [PATCH] Enable using device address when programming
> BARs
>
> Hi Roman,
>
> On 05/14/18 19:35, Ard Biesheuvel wrote:
> > On 14 May 2018 at 19:28, Roman Bacik <roman.bacik@broadcom.com>
> wrote:
> >> Ard,
> >>
> >> Thank you very much for your comment.
> >>
> >>> -----Original Message-----
> >>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >>> Sent: Sunday, May 13, 2018 3:25 AM
> >>> To: Roman Bacik
> >>> Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov
> >>> Subject: Re: [edk2] [PATCH] Enable using device address when
> >>> programming BARs
> >>>
> >>> On 9 May 2018 at 22:17, Roman Bacik <roman.bacik@broadcom.com>
> >>> wrote:
> >>>> I will upload v2 with the corrected subject - add package name
> >>>> MdeModulePkg/Bus .
> >>>>
> >>>
> >>> I don't think this is the correct approach. Please use the address
> >>> translation support that has been added recently to PciHostBridgeDxe
> >>> and PciHostBridgeLib.
> >>>
> >>
> >> Would you like to see this change:
> >>
> >>   Address = Base + Node->Offset;
> >> + if (UseDeviceAddress)
> >> +  Address = TO_DEVICE_ADDRESS(Address, -Base);
> >>
> >> Instead of:
> >>
> >> -  Address = Base + Node->Offset;
> >> +  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;
> >>
> >
> > No.
> >
> > Programming BARs should always involve device addresses, never CPU
> > addresses. If you wire up the MMIO translation support correctly, the
> > existing code will already do what you want.
>
> To clarify a bit, please look at the following commits:
>
>   1  5bb1866e5383 MdeModulePkg/PciHostBridgeLib.h: add address
> Translation
>   2  74d0a339b832 MdeModulePkg/PciHostBridgeDxe: Add support for
> address translation
>   3  c03860d05233 MdeModulePkg/PciBus: convert host address to device
> address
>   4  dc080d3b61e5 MdeModulePkg/PciBus: return CPU address for
> GetBarAttributes
>
> in particular, as Ard's first response suggests, the new field introduced
> in
> commit 5bb1866e5383. The PciHostBridgeLib instance that your platform
> plugs into MdeModulePkg/PciHostBridgeDxe should populate this field
> correctly. Then PciHostBridgeDxe and PciBusDxe will do the right thing for
> you.
>
> Thanks
> Laszlo


^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-05-15 14:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-03 22:54 [PATCH] Enable using device address when programming BARs Roman Bacik
2018-05-09 20:17 ` Roman Bacik
2018-05-13 10:25   ` Ard Biesheuvel
2018-05-14 17:28     ` Roman Bacik
2018-05-14 17:35       ` Ard Biesheuvel
2018-05-14 18:02         ` Roman Bacik
2018-05-14 18:04         ` Laszlo Ersek
2018-05-15 14:40           ` Roman Bacik

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