Re: [edk2-platforms: PATCH 0/9] Marvell Octeon CN913X SoC family support

["Marcin Wojtas" <mw@semihalf.com>]

czw., 8 sie 2019 o 18:48 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> Hi Marcin.
>
> On Thu, Aug 08, 2019 at 03:51:15PM +0200, Marcin Wojtas wrote:
> > > > This patchset adds all necessary components (.dsc/.fdf,
> > > > libraries, ACPI, DT) to support all 3 variants, which
> > > > are available on a modular CN913x Development Board.
> > >
> > > Thanks for this contribution.
> > > Do you have any further information on this SoC/Devboard?
> > > Searching only gets me the CN8xxx SoCs.
> >
> > Indeed :/ I guess there should be some public information soon,
> > unfortunately I'm not in charge of it.
> >
> > FYI, 2 days ago the support for it was submitted to the Linux lists:
> > https://www.spinics.net/lists/arm-kernel/msg746208.html
>
> Ah, good to know, thanks.
>
> > In high level this SoC is successor of Armada - enhanced modularity,
> > more interfaces, higher freq, new DDR controller and so on.
>
> OK. Yes, I see now these platforms are implemented as overlays on top
> of existing Armada DB.
>
> > > This does not affect Cn9132DbA, since that one does not include the
> > > ACPI module. Is this intenional?
> > >
> >
> > Maybe I should've mention this explicitly - yes, as for now we do not
> > support ACPI on triple-CP115 variant. The reason is following -
> > currently we have a static configuration of the ICU (CP115 interrupt
> > controller) to GIC. Thanks to that, we can assign GIC interrupts in
> > static ACPI tables. Unfortunately dual CP115/CP110 setup uses all
> > available GIC IRQs for this. We need to create mapping only for the
> > used devices and pass it to the ACPI tables.
> >
> > What is needed to fix it properly:
> > - create ICU-GIC dynamic mapping
> > - dynamically fill this information in DSDT/SSDT.
> >
> > > Which version of iasl has this been tested with?
> > >
> >
> > I built it successfully with iasl versions: 20180105 and 20160108-2.
> >
> > Anyway, in v2 I'll shorten OEM table ID to 8 characters ( "CN9130DBA"
> > is 9 character).
>
> OK. Working around this manually (shortening the name), the 9130
> builds fine. The 9131 needs an identical change to Ssdt.aml.
>
> However, all platforms still fail when I try building for ARM (which
> is supported according to the .dsc files).
> It seems the build fails from the missing PcdDramRemapTarget
> definition, in the build of
> Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/ARM/ArmPlatformHelper.S
>
> Moreover, this also affects existing Armada70x0/80x0 platforms.
> Could you look into this issue separately?

Thanks for the notice, I will take a look.

>
> On a higher level, I confess to not being entirely convinced about the
> triplicate .dsc/.dsc.inc/.fdf.inc setup. (Of the three, the .dsc.inc
> is the one I object the least to.)
> For the .dscs, I understand the desire to separate the build
> directories, but could this be achieved with -D build flags instead?
> Certainly the differences in .fdf.inc could be handled via
> conditional statements determined in a single .dsc.
>
> If (and this is a possibility) the 3 different .dscs is the right way
> forward, I still think everything other than the [defines] section
> should be kept in a common .dsc.inc.
>

I will try the -D option. How about:
- single .dsc / .fdf.inc
- triple .dsc.inc (they overlap in really minimal way and show pretty
good the differences between 3 variants)
?

> (This quite possibly concludes my commentary on v1. Don't hold back a
> v2 waiting for more.)
>

Ok, thanks!
Marcin

> Best Regards,
>
> Leif