Re: [edk2-platforms: PATCH v2 06/14] Marvell/Armada7k8k: Add PciExpressLib implementation

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Fri, 24 May 2019 at 16:28, Marcin Wojtas <mw@semihalf.com> wrote:
>
> pt., 24 maj 2019 o 15:08 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
> >
> > On Fri, 24 May 2019 at 15:03, Marcin Wojtas <mw@semihalf.com> wrote:
> > >
> > > pt., 24 maj 2019 o 14:50 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
> > > >
> > > > On Mon, 20 May 2019 at 17:27, Marcin Wojtas <mw@semihalf.com> wrote:
> > > > >
> > > > > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > >
> > > > > Implement a special version of PciExpressLib that takes the quirky
> > > > > nature of the Synopsys Designware PCIe IP into account. In particular,
> > > > > we need to ignore config space accesses to all devices on the first
> > > > > bus except device 0, because the broadcast nature of type 0 configuration
> > > > > cycles will result in whatever device is in the slot to appear at each
> > > > > of the 32 device positions.
> > > > >
> > > >
> > > > I never bothered to implement multisegment support for this SoC, since
> > > > MacchiatoBin has only one segment wired up, but since your interest is
> > > > in generic support, it might make sense to drop this patch and
> > > > implement PciSegmentLib instead (without depending on any of the other
> > > > library classes that the generic PciExpressLib depends on)
> > > >
> > >
> > > This was (and still is) my plan, but I've been having some serious
> > > time shortages for extra development. In order not to postpone this
> > > support any longer I prefer to get merged, what I have and possibly
> > > rework on top.
> > >
> > > About depending on a generic PciExpressLib - do you mean I can filter
> > > out devices from bus0 in PciSegmentLib?
> > >
> >
> > Yes, please look at Silicon/Socionext/SynQuacer/Library/SynQuacerPciSegmentLib
> >
> > If you follow the same approach, you no longer need PciLib or
> > PciExpressLib, it is all flattened into PciSegmentLib.
>
> For PciLib I use MdePkg implementation, but if that could be dropped
> that's good. I saw your library and it would be perfect for reusing on
> Armada, with one difference: PciSegmentLibGetConfigBase.
>
> 1. As a first step I could reuse Synquacer library using single base
> in PciSegmentLibGetConfigBase (PcdPciExpressBaseAddress) as a stub,
> for extending later.
>
> 2. Afterwards (i.e. in some next patchset) I'd like to use my board
> description infrastructure for obtaining config space base addresses.
> How about following solution:
> - add PciSegmentLib constructor routine, where I'd create a global
> array with config space addesses
> - add dispatching for it in PciSegmentLibGetConfigBase?
>

This sounds like a reasonable approach, yes.


> Looking forward to your feedback. Other than that - do you have any
> remarks to the rest of the patches in v2?
>

The rest looks fine.