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

["Paweł Anikiel" <pan@semihalf.com>]

On Fri, Sep 2, 2022 at 9:59 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Paweł,
>
> On Fri, 2 Sept 2022 at 07:16, Paweł Anikiel <pan@semihalf.com> wrote:
> >
> > On Tue, Aug 30, 2022 at 5:57 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Paweł,
> > >
> > > On Tue, 30 Aug 2022 at 05:51, Paweł Anikiel <pan@semihalf.com> wrote:
> > > >
> > > > On Tue, Aug 30, 2022 at 5:13 AM Alexandru M Stan <amstan@chromium.org> wrote:
> > > > >
> > > > > Hey Simon,
> > > > >
> > > > > On Mon, Aug 29, 2022 at 7:29 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Paweł,
> > > > > >
> > > > > > On Mon, 29 Aug 2022 at 02:23, Paweł Anikiel <pan@semihalf.com> wrote:
> > > > > > >
> > > > > > > On Sat, Aug 27, 2022 at 2:22 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Paweł,
> > > > > > > >
> > > > > > > > On Fri, 26 Aug 2022 at 01:54, Paweł Anikiel <pan@semihalf.com> wrote:
> > > > > > > > >
> > > > > > > > > Add devicetree for chameleonv3 with the 270-2I2-D11E variant of the
> > > > > > > > > Mercury+ AA1 module
> > > > > > > > >
> > > > > > > > > Signed-off-by: Paweł Anikiel <pan@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.
>
> Sure, but the alternative is to upstream one of the other .dts files
> from U-Boot, if the maintainer would prefer that.
>
> Have you thought about upstreaming the FPGA binding?

I just don't think having the different chameleon variants in Linux
makes sense (or FPGA bindings for the same reason), since the only
difference is with bitstream names and the clock settings which Linux
doesn't do.

I think it would be simpler to rename chameleonv3.dts to .dtsi in
u-boot. If that’s fine for you, I’ll prepare a v2 right away.

Regards,
Paweł