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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > >>>> Cc: Luka Perkov <[email protected]> > > > >>>> Cc: Robert Marko <[email protected]> > > > >>>> --- > > > >>>> 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?
