[PATCH v3 3/3] arm: mvebu: Initial ESPRESSOBin-Ultra board support

[Pali Rohár]

On Friday 13 August 2021 12:46:38 Pali Rohár wrote:
> On Friday 13 August 2021 12:25:46 Luka Kovacic wrote:
> > Hello Stefan and Pali,
> > 
> > On Fri, Aug 13, 2021 at 11:58 AM Stefan Roese <sr at denx.de> wrote:
> > >
> > > Hi,
> > >
> > > On 13.08.21 11:54, Pali Rohár wrote:
> > > > On Friday 13 August 2021 11:08:08 Luka Kovacic wrote:
> > > >> Hello Pali,
> > > >>
> > > >> On Fri, Aug 13, 2021 at 10:14 AM Pali Rohár <pali at kernel.org> wrote:
> > > >>>
> > > >>> On Friday 13 August 2021 01:39:38 Luka Kovacic wrote:
> > > >>>> Add initial support for the ESPRESSOBin-Ultra board from Globalscale
> > > >>>> Technologies, Inc.
> > > >>>>
> > > >>>> The board is based on the 64-bit dual-core Marvell Armada 3720 SoC.
> > > >>>> Peripherals:
> > > >>>>   - 5 Gigabit Ethernet ports (WAN has PoE, up to 30W, Topaz 6341 switch)
> > > >>>>   - RTC clock (PCF8563)
> > > >>>>   - USB 3.0 port
> > > >>>>   - USB 2.0 port
> > > >>>>   - 4x LED
> > > >>>>   - UART over Micro-USB
> > > >>>>   - M.2 slot (2280)
> > > >>>>   - Mini PCI-E slot
> > > >>>>
> > > >>>> Additionally, automatic import of the Marvell hw_info parameters is
> > > >>>> enabled via the recently added mac command for A37XX platforms.
> > > >>>> The parameters stored in Marvell hw_info are usually the board serial
> > > >>>> number and MAC addresses.
> > > >>>>
> > > >>>> Signed-off-by: Luka Kovacic <luka.kovacic at sartura.hr>
> > > >>>> Cc: Luka Perkov <luka.perkov at sartura.hr>
> > > >>>> Cc: Robert Marko <robert.marko at sartura.hr>
> > > >>>> ---
> > > >>>>   arch/arm/dts/Makefile                         |   1 +
> > > >>>>   .../arm/dts/armada-3720-espressobin-ultra.dts | 114 ++++++++++
> > > >>>>   arch/arm/dts/armada-3720-espressobin.dts      | 199 +----------------
> > > >>>>   arch/arm/dts/armada-3720-espressobin.dtsi     | 210 ++++++++++++++++++
> > > >>>>   board/Marvell/mvebu_armada-37xx/MAINTAINERS   |   8 +
> > > >>>>   board/Marvell/mvebu_armada-37xx/board.c       |  92 +++++++-
> > > >>>>   .../mvebu_espressobin-ultra-88f3720_defconfig |  93 ++++++++
> > > >>>>   7 files changed, 514 insertions(+), 203 deletions(-)
> > > >>>>   create mode 100644 arch/arm/dts/armada-3720-espressobin-ultra.dts
> > > >>>>   create mode 100644 arch/arm/dts/armada-3720-espressobin.dtsi
> > > >>>>   create mode 100644 configs/mvebu_espressobin-ultra-88f3720_defconfig
> > > >>>
> > > >>> Hello Luka! Please look at my comments from previous review:
> > > >>> https://lore.kernel.org/u-boot/20210227004852.5urcwnn6uxehuk72@pali/
> > > >>>
> > > >>> I think it is not a good idea to duplicate espressobin code, specially
> > > >>> now when differences between v5, v7, non-emmc and emmc were
> > > >>> de-duplicated and correctly detected at runtime. Just use one DTS and
> > > >>> one config file and differences can be handled in board code functions
> > > >>> "board_fix_fdt" and "board_late_init".
> > > >>
> > > >> I believe that patching the DTS at runtime diminishes the value of
> > > >> device trees in the first case.
> > > >>
> > > >> As far as the v5, v7, non-emmc and emmc boards go I do understand
> > > >> that, as they are physically similar and more or less different revisions
> > > >> of the same board.
> > > >>
> > > >> The ESPRESSOBin Ultra board is completely different from those boards
> > > >> physically and so I doesn't make sense to me, why we would merge all
> > > >> of them into one device tree.
> > > >
> > > > See email for reasons:
> > > > https://lore.kernel.org/u-boot/20210301154101.ke5j2r3lazjlxrsl@pali/
> > > >
> > > > Anyway, I'm looking at differences between ultra and non-ultra boards
> > > > which are used by U-Boot... And I see only:
> > > > * switch configuration & phy
> > > > * i2c rtc
> > > > * sdhci slot
> > > >
> > > > Other changes are not used by U-Boot (led).
> > > >
> > > > For switch configuration & phy there is already custom code in U-Boot
> > > > board file. For sdhci slot there is also (to enable/disable eMMC after
> > > > detection).
> > > >
> > > > So the only difference is presence of i2c rtc, right?
> > > >
> > > > I guess it does not make much sense to copy and duplicate whole DTS and
> > > > also defconfig file for such small differences. Insertion of just few
> > > > nodes is not a big problem.
> > > >
> > > > These boars are not very different, and having tons of u-boot binaries
> > > > for every combination just complicate it as explained in above email.
> > >
> > > I fully agree with Pali. It's much more conveniant to just have one
> > > U-Boot target (and binary) which can handle multiple board variants
> > > by runtime detection. I would very much welcome to see the support for
> > > the "Ultra" variant added this way.
> > 
> > I understand your points and concerns.
> > 
> > I'm just worried that this counterproductive as I don't see the same thing
> > happening in Linux and looking at this we could make similar arguments
> > for other boards, which aren't so different from the ESPRESSOBin boards
> > with the exception of the name.
> 
> I have already opened this question also in Linux, but there was no feedback:
> https://lore.kernel.org/linux-devicetree/20201022140007.hppmeyt34lubotbc@pali/t/#u
> 
> Anyway, as U-Boot can detect variant of the board, it also can load
> specific DTS kernel file for detected variant. So it has still advantage
> even when Linux does not use it (yet).
> 
> Also another argument is maintenance. This patch adds lot of lines of
> code and lot of them are duplicated. It means that somebody has to
> maintain it. So if Ultra variant can be supported by single binary too
> it should mean less code in U-Boot for Ultra (but needs to check!!!) and
> less code for maintaining.
> 
> > >
> > > > If you need help with this I can try to do something... as I was already
> > > > involved in unification of all v5/v7/emmc/non-emmc variants into one
> > > > binary with one DTS.
> > >
> > > Thanks Pali for all your work here.

Hello Luka! Do you need some help with this?