> -----Original Message----- > From: Ard Biesheuvel [mailto:[email protected]] > Sent: Monday, May 14, 2018 10:35 AM > To: Roman Bacik > Cc: [email protected]; Ruiyu Ni; Vladimir Olovyannikov > Subject: Re: [edk2] [PATCH] Enable using device address when programming > BARs > > On 14 May 2018 at 19:28, Roman Bacik <[email protected]> wrote: > > Ard, > > > > Thank you very much for your comment. > > > >> -----Original Message----- > >> From: Ard Biesheuvel [mailto:[email protected]] > >> Sent: Sunday, May 13, 2018 3:25 AM > >> To: Roman Bacik > >> Cc: [email protected]; Ruiyu Ni; Vladimir Olovyannikov > >> Subject: Re: [edk2] [PATCH] Enable using device address when > >> programming BARs > >> > >> On 9 May 2018 at 22:17, Roman Bacik <[email protected]> > 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:[email protected]] > >> > *Sent:* Thursday, May 3, 2018 3:55 PM > >> > *To:* [email protected] > >> > *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 <[email protected]> > >> > Cc: Vladimir Olovyannikov <[email protected]> > >> > Contributed-under: TianoCore Contribution Agreement 1.1 > >> > Signed-off-by: Roman Bacik <[email protected]> > >> > --- > >> > 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 > >> > [email protected] > >> > https://lists.01.org/mailman/listinfo/edk2-devel > > > > Thanks, > > > > Roman _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
