Re: [edk2-devel] [edk2-platforms][PATCH V1 07/11] Platform/ARM/Morello: Port PCI Express library

["chandni cherukuri" <ch.chandni@gmail.com>]

On Mon, Dec 13, 2021 at 8:08 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 13 Dec 2021 at 13:41, chandni cherukuri <ch.chandni@gmail.com> wrote:
> >
> > Hi Ard,
> >
> > Thanks for the response.
> >
> > I had a look at the Synquacer platform where there is a single custom
> > PCIe library implemented.
> > So, is it better to implement a single custom PCIe library instead of
> > two custom PCIe libraries?
> >
>
> Yes, for two reasons:
> - there is prior art for this solution, and solving the same problem
> in two different ways in the same codebase is bad form;
> - those two libraries are layered one on top of the other, which means
> that your fix requires changes to two different layers, making the
> interface between those two layers obviously unfit to represent the
> hardware in question.
>
>
Thanks for the response.
Will implement a single custom PCIe library and post a new patchset.

Thanks
Chandni

> > If that is the case then will implement a single custom PCIe library
> > for Morello.
> >
> > Thanks
> > Chandni
> > On Fri, Dec 10, 2021 at 4:12 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Thu, 9 Dec 2021 at 13:30, chandni cherukuri <ch.chandni@gmail.com> wrote:
> > > >
> > > > Hi Ard, Leif, Sami, Pierre,
> > > >
> > > > For Morello platform, we created two custom libraries based on the
> > > > below two native libraries:
> > > > -          https://github.com/tianocore/edk2/tree/master/MdePkg/Library/BasePciSegmentLibPci
> > > > -          https://github.com/tianocore/edk2/tree/master/MdePkg/Library/BasePciExpressLib
> > > >
> > > > because of the following reasons.
> > > > 1.       In Morello platform, the ECAM region of the two root ports
> > > > are placed in non-contiguous memory as per the memory map architecture
> > > > of the Morello platform. The native PCI Express Library expects both
> > > > the ECAM regions for all the root ports to be contiguous.
> > >
> > > Do you mean root ports or host bridges? If root ports don't share the
> > > MMIO resource windows provided by the respective host bridges, they
> > > are really different host bridges and should be modeled as such.
> > >
> > > Note that the Cadence and Synopsys IP usually does not implement a
> > > root port at all, and gets away with this because it only implements a
> > > single link.
> > >
> > > > 2.       IORT and kernel currently require each root port to be in a
> > > > separate segment. You can refer to the code for more details -
> > > >
> > >
> > > Please take a look at the SynQuaecer platform for inspiration. That
> > > platform is very similar in this respect.
> > >
> > >  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/acpi/arm64/iort.c#n307.
> > > > The native PCI Segment Library supports only a single segment.
> > > >
> > > > Because of these reasons, the current patch series adapts the native
> > > > libraries as follows:
> > > >
> > > > The custom PCI Segment Library passes the complete address which is
> > > > consumed by the custom PCI Express library where based on the Segment
> > > > number, the base address of the PCI Express is returned.
> > > >
> > > > The rationale behind maintaining two separate libraries is that when
> > > > the software evolves and support for multiple segments is adapted in
> > > > the native PCI Segment Library the custom library could be removed.
> > > > Also, we might have other protocols which might try to use the PCI
> > > > Express library directly. However, there are some other platforms
> > > > where all the platform specific changes have gone in a single custom
> > > > PCI library
> > > >
> > > > Could you please provide some inputs as to which of two approaches
> > > > would be better to follow as there is one more platform to follow
> > > > based on the decision taken?
> > > >
> > > > Thanks
> > > > Chandni
> > > >
> > > > On Wed, Dec 8, 2021 at 8:25 AM Khasim Mohammed <khasim.mohammed@arm.com> wrote:
> > > > >
> > > > > Hi Sami, Chandni,
> > > > >
> > > > > There was a suggestion from Pierre on a similar patch for N1SDP to remove PCIExpressLib.c and move to workarounds to PCISegmentLib.c,
> > > > >
> > > > > https://edk2.groups.io/g/devel/message/84165?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2Ckhasim%2C20%2C2%2C0%2C87257273
> > > > >
> > > > > I think we should discuss this implementation for both N1SDP and Morello as they have similar implementation.
> > > > >
> > > > > Regards,
> > > > > Khasim
> > > > >