[PATCH 4/4] arm: dts: chameleonv3: Add 270-2 variant

[Paweł Anikiel]

On Tue, Aug 30, 2022 at 5:57 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Paweł,
>
> On Tue, 30 Aug 2022 at 05:51, Paweł Anikiel <pan at semihalf.com> wrote:
> >
> > On Tue, Aug 30, 2022 at 5:13 AM Alexandru M Stan <amstan at chromium.org> wrote:
> > >
> > > Hey Simon,
> > >
> > > On Mon, Aug 29, 2022 at 7:29 PM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Paweł,
> > > >
> > > > On Mon, 29 Aug 2022 at 02:23, Paweł Anikiel <pan at semihalf.com> wrote:
> > > > >
> > > > > On Sat, Aug 27, 2022 at 2:22 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Paweł,
> > > > > >
> > > > > > On Fri, 26 Aug 2022 at 01:54, Paweł Anikiel <pan at semihalf.com> wrote:
> > > > > > >
> > > > > > > Add devicetree for chameleonv3 with the 270-2I2-D11E variant of the
> > > > > > > Mercury+ AA1 module
> > > > > > >
> > > > > > > Signed-off-by: Paweł Anikiel <pan at semihalf.com>
> > > > > > > ---
> > > > > > >  arch/arm/dts/Makefile                                |  1 +
> > > > > > >  .../socfpga_arria10_chameleonv3_270_2-u-boot.dtsi    | 12 ++++++++++++
> > > > > > >  arch/arm/dts/socfpga_arria10_chameleonv3_270_2.dts   |  5 +++++
> > > > > > >  3 files changed, 18 insertions(+)
> > > > > > >  create mode 100644 arch/arm/dts/socfpga_arria10_chameleonv3_270_2-u-boot.dtsi
> > > > > > >  create mode 100644 arch/arm/dts/socfpga_arria10_chameleonv3_270_2.dts
> > > > > > >
> > > >
> > > >
> > > > > > > diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> > > > > > > index 7330121dba..36d5d65595 100644
> > > > > > > --- a/arch/arm/dts/Makefile
> > > > > > > +++ b/arch/arm/dts/Makefile
> > > > > > > @@ -425,6 +425,7 @@ dtb-$(CONFIG_ARCH_SOCFPGA) +=                               \
> > > > > > >         socfpga_agilex_socdk.dtb                        \
> > > > > > >         socfpga_arria5_secu1.dtb                        \
> > > > > > >         socfpga_arria5_socdk.dtb                        \
> > > > > > > +       socfpga_arria10_chameleonv3_270_2.dtb           \
> > > > > > >         socfpga_arria10_chameleonv3_270_3.dtb           \
> > > > > > >         socfpga_arria10_chameleonv3_480_2.dtb           \
> > > > > > >         socfpga_arria10_socdk_sdmmc.dtb                 \
> > > > > > > diff --git a/arch/arm/dts/socfpga_arria10_chameleonv3_270_2-u-boot.dtsi b/arch/arm/dts/socfpga_arria10_chameleonv3_270_2-u-boot.dtsi
> > > > > > > new file mode 100644
> > > > > > > index 0000000000..05b4485cf3
> > > > > > > --- /dev/null
> > > > > > > +++ b/arch/arm/dts/socfpga_arria10_chameleonv3_270_2-u-boot.dtsi
> > > > > > > @@ -0,0 +1,12 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > +/*
> > > > > > > + * Copyright 2022 Google LLC
> > > > > > > + */
> > > > > > > +#include "socfpga_arria10_chameleonv3_480_2_handoff.h"
> > > > > > > +#include "socfpga_arria10-handoff.dtsi"
> > > > > > > +#include "socfpga_arria10_handoff_u-boot.dtsi"
> > > > > > > +#include "socfpga_arria10_mercury_aa1-u-boot.dtsi"
> > > > > > > +
> > > > > > > +&fpga_mgr {
> > > > > > > +       altr,bitstream = "fpga-270-2.itb";
> > > > > > > +};
> > > > > > > diff --git a/arch/arm/dts/socfpga_arria10_chameleonv3_270_2.dts b/arch/arm/dts/socfpga_arria10_chameleonv3_270_2.dts
> > > > > > > new file mode 100644
> > > > > > > index 0000000000..5f40af6eb9
> > > > > > > --- /dev/null
> > > > > > > +++ b/arch/arm/dts/socfpga_arria10_chameleonv3_270_2.dts
> > > > > > > @@ -0,0 +1,5 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > +/*
> > > > > > > + * Copyright 2022 Google LLC
> > > > > > > + */
> > > > > > > +#include "socfpga_arria10_chameleonv3.dts"
> > > > > >
> > > > > > Can you create a common .dtsi file instead? We should not be including
> > > > > > a .dts file into another file.
> > > > > >
> > > > > Do you mean renaming chameleonv3.dts to .dtsi? In Linux it's a .dts,
> > > > > because nothing includes it (no handoff headers are needed). Is it
> > > > > fine to have the names differ across U-Boot and Linux?
> > > >
> > > > Ideally not, but we should not include a .dts file in another one and
> > > > it is probably more important to follow that rule. But why is Linux
> > > > not getting this variant?
> > > >
> > > > Regards,
> > > > Simon
> > >
> > > Linux (at least for the near future) does not care about which variant
> > > it is. The big differences between 270, 480, -2, -3 are mostly about
> > > the number of FPGA logic gates and speed grades. Such things affect
> > > the FPGA bitstream greatly, and might even affect clock presets that
> > > u-boot cares about, but by the time linux loads it doesn't matter
> > > anymore.
> >
> > Perhaps a more detailed explanation:
> >
> > The Main and Peripheral PLLs (as well as some other clocks) are
> > configured by U-Boot. On the other hand, Linux expects them to be
> > configured when it boots, and does not touch them.
> >
> > The clock configuration depends mainly on the speed grade of the Arria
> > 10 SoC (marked by us as -2 and -3), but also on the fpga hardware
> > design (e.g. user-defined clocks for the fpga), and is included in the
> > u-boot devicetree:
> > > +#include "socfpga_arria10_chameleonv3_480_2_handoff.h"
> > > +#include "socfpga_arria10-handoff.dtsi"
> > > +#include "socfpga_arria10_handoff_u-boot.dtsi"
> >
> > Linux, on the other hand, doesn't need such information, and there is
> > no distinction between the different chameleon variants.
>
> One option would be to put everything in a .dtsi in linux with a
> single top-level dts that just includes it. Then U-Boot is not that
> different.

I assume you mean a .dts file with a single #include "... .dtsi". Do
you think the maintainers will be fine with such change? From the
perspective of Linux it seems strange IMO, especially when there's
nothing else including that .dtsi.

Regards,
Paweł