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

["Leif Lindholm" <leif.lindholm@linaro.org>]

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?

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.

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

Best Regards,

Leif