On Mon, 2020-02-17 at 14:22 +0100, Mauro Condarelli wrote: > Hi Daniel, > a gentle reminder... > > I was unable to comply with a few of Your remarks (see below), > What should I do? > Submit v6 as is or do You have specific instructions? > > Thanks a lot > Mauro > > On 2/12/20 11:01 PM, Mauro Condarelli wrote: > > Thanks Daniel, > > > > On 2/12/20 5:58 PM, Daniel Schwierzeck wrote: > >> On Wed, Feb 12, 2020 at 4:30 PM Mauro Condarelli <mc5...@mclink.it> wrote: > >>> Small patch series to add support for VoCore/VoCore2 board. > >>> > >>> VoCore is open hardware and runs OpenWrt/LEDE. > >>> It has WIFI, USB, UART, 20+ GPIOs but is only one inch square. > >>> It will help you to make a smart house, study embedded system > >>> or even make the tiniest router in the world. > >>> > >>> ===8<--- > >>> diff --git a/board/vocore/vocore2/board.c b/board/vocore/vocore2/board.c > >>> new file mode 100644 > >>> index 0000000000..27e42d1414 > >>> --- /dev/null > >>> +++ b/board/vocore/vocore2/board.c > >>> @@ -0,0 +1,6 @@ > >>> +// SPDX-License-Identifier: GPL-2.0+ > >>> +/* > >>> + * Copyright (C) 2019 Mauro Condarelli <mc5...@mclink.it> > >>> + * > >>> + * Nothing actually needed here > >>> + */ > >> if that file is empty, don't add it at all > > Ahem... > > > > I tried to remove it, but if I do I also have to remove entry > > in board/vocore/vocore2/Makefile (leaving it empty) and this > > leads (on a clean compile) to error: > > mipsel-linux-ld.bfd: cannot find board/vocore/vocore2/built-in.o: No > > such file or directory > > Completely removing both files bombs with: > > scripts/Makefile.build:57: board/vocore/vocore2/Makefile: No such file > > or directory > > It seems I should also remove board/vocore/vocore2/Kconfig, > > but I'm unsure this is the right thing to do. > > > > I have no idea about how to solve this, sorry. > > Can You point me in the right direction, please? > > > > > >>> ===8<--- > >>> diff --git a/include/configs/vocore2.h b/include/configs/vocore2.h > >>> new file mode 100644 > >>> index 0000000000..6b43aa766e > >>> --- /dev/null > >>> +++ b/include/configs/vocore2.h > >>> @@ -0,0 +1,61 @@ > >>> +/* SPDX-License-Identifier: GPL-2.0+ */ > >>> +/* > >>> + * Copyright (C) 2019 Mauro Condarelli <mc5...@mclink.it> > >>> + */ > >>> + > >>> +#ifndef __VOCORE2_CONFIG_H__ > >>> +#define __VOCORE2_CONFIG_H__ > >>> + > >>> +/* CPU */ > >>> +#define CONFIG_SYS_MIPS_TIMER_FREQ 290000000 > >> is this still necessary? Weijie implemented a custom CPU clock > >> handling which supports > >> dynamic timer clocks. > > This is referenced in arch/mips/cpu/time.c > > I don't know if it's still relevant, but removing it doesn't seem > > task for this patch.
You have to keep this to make sure arch/mips/cpu/time.c can be compiled. > > > > > >>> + > >>> +/* RAM */ > >>> +#define CONFIG_SYS_SDRAM_BASE 0x80000000 > >>> + > >>> +#define CONFIG_SYS_LOAD_ADDR CONFIG_SYS_SDRAM_BASE + 0x100000 > >>> + > >>> +#define CONFIG_SYS_INIT_SP_OFFSET 0x400000 > >>> + > >>> +/* SPL */ > >>> +#if defined(CONFIG_SPL) && !defined(CONFIG_SPL_BUILD) > >>> +#define CONFIG_SKIP_LOWLEVEL_INIT > >>> +#endif > >> CONFIG_SPL_BUILD is only relevant in Makefiles and shouldn't be used > >> in config header files > > Removed, apparently without adverse consequences. > Appearence was misguiding. > Any change to this guard leads to code unable to boot from SPI NOR. > I'll have to leave it is as-is. > Someone more knowledgeable than me will have to understand why > they are needed and how to remove them, if that's a requirement. > After all the other boards with this SoC use the same code. CONFIG_SPL_BUILD is defined only when building the SPL binary. CONFIG_SPL is defined for both SPL and u-boot if SPL is enabled, and we can only use CONFIG_SPL_BUILD to distinguish whether we are building SPL or not. CONFIG_SKIP_LOWLEVEL_INIT apparently means do not do lowlevel_init. However lowlevel_init must be done once and only once for a SPI NOR bootable version. The following table describes whether lowlevel_init is needed: | SPL | u-boot ------------------------------ SPL enabled | Y | N ------------------------------ SPL disabled | | Y As you can see we only want to define CONFIG_SKIP_LOWLEVEL_INIT for just one case: SPL enabled and not building SPL, because lowlevel_init is done in SPL and u-boot is a ram boot binary. As a result this table produces the condition I used for other boards: #if defined(CONFIG_SPL) && !defined(CONFIG_SPL_BUILD) If we use only "defined(CONFIG_SPL)", CONFIG_SKIP_LOWLEVEL_INIT will be defined for both parts, and this will finally lead to code unable to boot from SPI NOR. > > > >>> + > >>> +#define CONFIG_SYS_UBOOT_START CONFIG_SYS_TEXT_BASE > >>> +#define CONFIG_SPL_BSS_START_ADDR 0x80010000 > >>> +#define CONFIG_SPL_BSS_MAX_SIZE 0x10000 > >>> +#define CONFIG_SPL_MAX_SIZE 0x10000 > >>> + > >>> +/* Dummy value */ > >>> +#define CONFIG_SYS_UBOOT_BASE 0 > >> where is that needed? > > This is referenced in common/spl/spl_nor.c > > I don't know if it's still relevant, but removing it doesn't seem > > task for this patch. Same as CONFIG_SYS_MIPS_TIMER_FREQ. > > > >>> ==8<--- > >>> > > I'll wait for input before submitting another iteration. > > > > Thanks a lot > > Mauro >