Hi Daniel, First thanks for you prompt review, it is much appreciate. :)
This week I am at kernel recipes conference, so I won't be able to fully address your comments but I will do it next week. However, here are some answers: On mer., sept. 26 2018, Daniel Schwierzeck <daniel.schwierz...@gmail.com> wrote: > Hi Gregory, > > On 25.09.2018 15:01, Gregory CLEMENT wrote: >> These families of SoCs are found in the Microsemi Switches solution. >> >> Currently the support for two families support is added: >> - Ocelot (VSC7513, VSC7514) already supported in Linux >> - Luton (Luton10: VSC7423, VSC7424, VSC7428 and Luton26: VSC7425, >> VSC7426, VSC7426, VSC7427, VSC7429) > > Is this some polished version of the original vendor U-Boot? No the original vendor version was RedBoot > Could you maybe add Ocelot and Luton in separate patches? Yes sure the intent to have a uniq patch was to justify the common code between both SoC. > >> >> Signed-off-by: Gregory CLEMENT <gregory.clem...@bootlin.com> >> --- [..] >> +endif > > from the code below I assume you have a MIPS24k core? If so you should > use the automatic cache size detection Yes it is a MIPS24k core. I will have a look on the automatic cache size detection. > >> + >> +menu "MSCC VCore-III platforms" >> + depends on ARCH_MSCC >> + >> +config SOC_VCOREIII >> + select SUPPORTS_LITTLE_ENDIAN >> + select SUPPORTS_BIG_ENDIAN >> + select SUPPORTS_CPU_MIPS32_R1 >> + select SUPPORTS_CPU_MIPS32_R2 >> + select ROM_EXCEPTION_VECTORS >> + select MIPS_TUNE_24KC >> + bool > > sort this alpahetically OK > >> + [...] >> +void vcoreiii_tlb_init(void) >> +{ >> + register int tlbix = 0; >> + >> + init_tlb(); >> + >> + /* 0x70000000 size 32M (0x02000000) */ >> + create_tlb(tlbix++, MSCC_IO_ORIGIN1_OFFSET, SZ_16M, MMU_REGIO_RW, >> MMU_REGIO_RW); >> +#ifdef CONFIG_SOC_LUTON >> + create_tlb(tlbix++, MSCC_IO_ORIGIN2_OFFSET, SZ_16M, MMU_REGIO_RW, >> MMU_REGIO_RW); >> +#endif >> + /* 0x40000000 - 0x43ffffff - NON-CACHED! */ >> + /* Flash CS0-3, each 16M = 64M total (16 x 4 below ) */ >> + create_tlb(tlbix++, MSCC_FLASH_TO, SZ_16M, MMU_REGIO_RO, >> MMU_REGIO_RO); >> + create_tlb(tlbix++, MSCC_FLASH_TO+SZ_32M, SZ_16M, MMU_REGIO_RO, >> MMU_REGIO_RO); >> + >> + /* 0x20000000 - up */ >> +#if CONFIG_SYS_SDRAM_SIZE <= SZ_64M >> + create_tlb(tlbix++, MSCC_DDR_TO, SZ_64M, MMU_REGIO_RW, >> MMU_REGIO_INVAL); >> +#elif CONFIG_SYS_SDRAM_SIZE <= SZ_128M >> + create_tlb(tlbix++, MSCC_DDR_TO, SZ_64M, MMU_REGIO_RW, >> MMU_REGIO_RW); >> +#elif CONFIG_SYS_SDRAM_SIZE <= SZ_256M >> + create_tlb(tlbix++, MSCC_DDR_TO, SZ_256M, MMU_REGIO_RW, >> MMU_REGIO_INVAL); >> +#elif CONFIG_SYS_SDRAM_SIZE <= SZ_512M >> + create_tlb(tlbix++, MSCC_DDR_TO, SZ_256M, MMU_REGIO_RW, >> MMU_REGIO_RW); >> +#else /* 1024M */ >> + create_tlb(tlbix++, MSCC_DDR_TO, SZ_512M, MMU_REGIO_RW, >> MMU_REGIO_RW); >> +#endif > > can't you leave that to the kernel? U-Boot is only running in kernel > mode and doesn't need MMU mappings. You should be right, I will check it. > >> +} >> + >> +int mach_cpu_init(void) >> +{ >> + /* Speed up NOR flash access */ >> +#ifdef CONFIG_SOC_LUTON >> + writel(ICPU_SPI_MST_CFG_FAST_READ_ENA + >> +#else >> + writel( >> +#endif >> + ICPU_SPI_MST_CFG_CS_DESELECT_TIME(0x19) + >> + ICPU_SPI_MST_CFG_CLK_DIV(9), REG_CFG(ICPU_SPI_MST_CFG)); >> + >> + /* Disable all IRQs - map to destination map 0 */ >> + writel(0, REG_CFG(ICPU_INTR_ENA)); >> +#ifdef CONFIG_SOC_OCELOT >> + writel(~0, REG_CFG(ICPU_DST_INTR_MAP(0))); >> + writel(0, REG_CFG(ICPU_DST_INTR_MAP(1))); >> + writel(0, REG_CFG(ICPU_DST_INTR_MAP(2))); >> + writel(0, REG_CFG(ICPU_DST_INTR_MAP(3))); >> +#else >> + writel(ICPU_INTR_IRQ0_ENA_IRQ0_ENA, REG_CFG(ICPU_INTR_IRQ0_ENA)); >> +#endif > > do you really need to disable interrupts after a cold or warm boot? I think it is needed, but I will check. > >> + return 0; >> +} >> diff --git a/arch/mips/mach-mscc/dram.c b/arch/mips/mach-mscc/dram.c >> new file mode 100644 >> index 0000000000..2db9001fe9 >> --- /dev/null >> +++ b/arch/mips/mach-mscc/dram.c >> @@ -0,0 +1,62 @@ >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> +/* >> + * Copyright (c) 2018 Microsemi Corporation >> + */ >> + >> +#include <common.h> >> + >> +#include <asm/io.h> >> +#include <asm/types.h> >> + >> +#include <mach/tlb.h> >> +#include <mach/ddr.h> >> + >> +DECLARE_GLOBAL_DATA_PTR; >> + >> +int vcoreiii_ddr_init(void) >> +{ >> + int res; >> + if (!(readl(REG_CFG(ICPU_MEMCTRL_STAT)) & >> + ICPU_MEMCTRL_STAT_INIT_DONE)) { >> + hal_vcoreiii_init_memctl(); >> + hal_vcoreiii_wait_memctl(); >> + if (hal_vcoreiii_init_dqs() != 0 || >> + hal_vcoreiii_train_bytelane(0) != 0 >> +#ifdef CONFIG_SOC_OCELOT >> + || hal_vcoreiii_train_bytelane(1) != 0 >> +#endif >> + ) >> + hal_vcoreiii_ddr_failed(); >> + } >> + >> +#if (CONFIG_SYS_TEXT_BASE != 0x20000000) >> + res = dram_check(); >> + if (res == 0) >> + hal_vcoreiii_ddr_verified(); >> + else >> + hal_vcoreiii_ddr_failed(); >> + >> + /* Clear boot-mode and read-back to activate/verify */ >> + writel(readl(REG_CFG(ICPU_GENERAL_CTRL))& >> ~ICPU_GENERAL_CTRL_BOOT_MODE_ENA, >> + REG_CFG(ICPU_GENERAL_CTRL)); > > clrbits_32()? I missed this function, thanks to point it! > >> + readl(REG_CFG(ICPU_GENERAL_CTRL)); >> +#else >> + res = 0; >> +#endif >> + return res; >> +} >> + >> +int print_cpuinfo(void) >> +{ >> + printf("MSCC VCore-III MIPS 24Kec\n"); >> + >> + return 0; >> +} >> + >> +int dram_init(void) >> +{ >> + while (vcoreiii_ddr_init()); >> + >> + gd->ram_size = CONFIG_SYS_SDRAM_SIZE; >> + return 0; >> +} >> diff --git a/arch/mips/mach-mscc/include/ioremap.h >> b/arch/mips/mach-mscc/include/ioremap.h >> new file mode 100644 >> index 0000000000..684f89168c >> --- /dev/null >> +++ b/arch/mips/mach-mscc/include/ioremap.h >> @@ -0,0 +1,49 @@ >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> +/* >> + * Copyright (c) 2018 Microsemi Corporation >> + */ >> + >> +#ifndef __ASM_MACH_MSCC_IOREMAP_H >> +#define __ASM_MACH_MSCC_IOREMAP_H >> + >> +#include <linux/types.h> >> +#include <mach/common.h> >> + >> +/* >> + * Allow physical addresses to be fixed up to help peripherals located >> + * outside the low 32-bit range -- generic pass-through version. >> + */ >> +static inline phys_addr_t fixup_bigphys_addr(phys_addr_t phys_addr, >> + phys_addr_t size) >> +{ >> + return phys_addr; >> +} >> + >> +static inline int is_vcoreiii_internal_registers(phys_addr_t offset) >> +{ >> +#if defined(CONFIG_ARCH_MSCC) >> + if ((offset >= MSCC_IO_ORIGIN1_OFFSET && offset < >> (MSCC_IO_ORIGIN1_OFFSET+MSCC_IO_ORIGIN1_SIZE)) || >> + (offset >= MSCC_IO_ORIGIN2_OFFSET && offset < >> (MSCC_IO_ORIGIN2_OFFSET+MSCC_IO_ORIGIN2_SIZE))) >> + return 1; >> +#endif >> + >> + return 0; >> +} >> + >> +static inline void __iomem *plat_ioremap(phys_addr_t offset, unsigned long >> size, >> + unsigned long flags) >> +{ >> + if (is_vcoreiii_internal_registers(offset)) >> + return (void __iomem *)offset; >> + >> + return NULL; >> +} >> + >> +static inline int plat_iounmap(const volatile void __iomem *addr) >> +{ >> + return is_vcoreiii_internal_registers((unsigned long)addr); >> +} >> + >> +#define _page_cachable_default _CACHE_CACHABLE_NONCOHERENT >> + >> +#endif /* __ASM_MACH_MSCC_IOREMAP_H */ >> diff --git a/arch/mips/mach-mscc/include/mach/cache.h >> b/arch/mips/mach-mscc/include/mach/cache.h >> new file mode 100644 >> index 0000000000..f3d09014f3 >> --- /dev/null >> +++ b/arch/mips/mach-mscc/include/mach/cache.h >> @@ -0,0 +1,36 @@ >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> +/* >> + * Copyright (c) 2018 Microsemi Corporation >> + */ >> +#define MIPS32_CACHE_OP(which, op) (which | (op << 2)) >> + >> +#define MIPS32_WHICH_ICACHE 0x0 >> +#define MIPS32_WHICH_DCACHE 0x1 >> + >> +#define MIPS32_INDEX_INVALIDATE 0x0 >> +#define MIPS32_INDEX_LOAD_TAG 0x1 >> +#define MIPS32_INDEX_STORE_TAG 0x2 >> +#define MIPS32_HIT_INVALIDATE 0x4 >> +#define MIPS32_ICACHE_FILL 0x5 >> +#define MIPS32_DCACHE_HIT_INVALIDATE 0x5 >> +#define MIPS32_DCACHE_HIT_WRITEBACK 0x6 >> +#define MIPS32_FETCH_AND_LOCK 0x7 >> + >> +#define ICACHE_LOAD_LOCK (MIPS32_CACHE_OP(MIPS32_WHICH_ICACHE, >> MIPS32_FETCH_AND_LOCK)) >> + >> +#define CACHE_LINE_LEN 32 > > you can use ARCH_DMA_MINALIGN for this OK > >> + >> +/* Prefetch and lock instructions into cache */ >> +static inline void icache_lock(void *func, size_t len) >> +{ >> + int i, lines = ((len - 1) / CACHE_LINE_LEN) + 1; >> + for (i = 0; i < lines; i++) { >> + asm volatile (" cache %0, %1(%2)" >> + : /* No Output */ >> + : "I" ICACHE_LOAD_LOCK, >> + "n" (i*CACHE_LINE_LEN), >> + "r" (func) >> + : /* No Clobbers */); >> + } >> +} > > could you try to implement this in a generic way in > arch/mips/lib/cache.c or arch/mips/include/asm/cacheops.h? I will do it [...] >> +static inline void hal_vcoreiii_ddr_failed(void) >> +{ >> + register u32 reset; >> + >> + writel(readl(REG_CFG(ICPU_GPR(6))) + 1, REG_CFG(ICPU_GPR(6))); >> + >> + writel(readl(REG_GCB(PERF_GPIO_OE)) & ~BIT(19), >> + REG_GCB(PERF_GPIO_OE)); >> + >> + /* Jump to reset - does not return */ >> + reset = KSEG0ADDR(_machine_restart); >> + icache_lock((void*)reset, 128); // Reset while running from cache >> + asm volatile ("jr %0" : : "r" (reset)); >> + >> + while(1) >> + ; > > can't you just use panic() or hang()? Indeed panic would be a better option. > >> +} >> +/* >> + * DDR memory sanity checking done, possibly enable ECC. >> + * >> + * NB: Assumes inlining as no stack is available! >> + */ >> +static inline void hal_vcoreiii_ddr_verified(void) >> +{ >> +#ifdef MIPS_VCOREIII_MEMORY_ECC >> + /* Finally, enable ECC */ >> + u32 val = readl(REG_CFG(ICPU_MEMCTRL_CFG)); > > shouldn't it "register u32 val" like in the other functions? Right > >> + sleep_100ns(10000); >> +#ifdef CONFIG_SOC_OCELOT >> + /* Establish data contents in DDR RAM for training */ >> + ((volatile u32 *)MSCC_DDR_TO)[0] = 0xcacafefe; >> + ((volatile u32 *)MSCC_DDR_TO)[1] = 0x22221111; >> + ((volatile u32 *)MSCC_DDR_TO)[2] = 0x44443333; >> + ((volatile u32 *)MSCC_DDR_TO)[3] = 0x66665555; >> + ((volatile u32 *)MSCC_DDR_TO)[4] = 0x88887777; >> + ((volatile u32 *)MSCC_DDR_TO)[5] = 0xaaaa9999; >> + ((volatile u32 *)MSCC_DDR_TO)[6] = 0xccccbbbb; >> + ((volatile u32 *)MSCC_DDR_TO)[7] = 0xeeeedddd; >> +#else >> + ((volatile u32 *)MSCC_DDR_TO)[0] = 0xff; >> +#endif > > use writel() or at least __raw_writel() Yes of course, this part was not poperly converted from redboot. > [...] >> +#define MSCC_IO_ORIGIN1_OFFSET 0x60000000 >> +#define MSCC_IO_ORIGIN1_SIZE 0x01000000 >> +#define MSCC_IO_ORIGIN2_OFFSET 0x70000000 >> +#define MSCC_IO_ORIGIN2_SIZE 0x00200000 >> +#ifndef MSCC_IO_OFFSET1 >> +#define MSCC_IO_OFFSET1(offset) (MSCC_IO_ORIGIN1_OFFSET + offset) >> +#endif >> +#ifndef MSCC_IO_OFFSET2 >> +#define MSCC_IO_OFFSET2(offset) (MSCC_IO_ORIGIN2_OFFSET + offset) >> +#endif >> +#define BASE_CFG 0x70000000 >> +#define BASE_UART 0x70100000 >> +#define BASE_DEVCPU_GCB 0x60070000 >> +#define BASE_MACRO 0x600a0000 >> + >> +#define REG_OFFSET(t, o) ((volatile unsigned long *)(t + (o))) >> +#define REG_CFG(x) REG_OFFSET(BASE_CFG, x) >> +#define REG_GCB(x) REG_OFFSET(BASE_DEVCPU_GCB, x) >> +#define REG_MACRO(x) REG_OFFSET(BASE_MACRO, x) > > No header files with offsets please except when needed for ASM code. > This should come from device-tree. Most of this value are used before the device tree was available. But I will double check if some of them are used after the device tree is parsed. [...] > > No header files with thousand of register definitions please. Only > define what you need per driver. Actually it should no more the case with the other version I sent on hte mailing list, after this one was rejected as being too large >> +static inline void init_tlb(void) >> +{ >> + register int i, max; >> + >> + max = get_tlb_count(); >> + for(i = 0; i < max; i++) >> + create_tlb(i, i * SZ_1M, SZ_4K, MMU_REGIO_INVAL, MMU_REGIO_INVAL); >> +} > > again can't you leave the setup of MMU mappings to the kernel? I will check again > >> + >> +#endif /* __ASM_MACH_TLB_H */ >> diff --git a/arch/mips/mach-mscc/lowlevel_init.S >> b/arch/mips/mach-mscc/lowlevel_init.S >> new file mode 100644 >> index 0000000000..87439439f0 >> --- /dev/null >> +++ b/arch/mips/mach-mscc/lowlevel_init.S >> @@ -0,0 +1,29 @@ >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> +/* >> + * Copyright (c) 2018 Microsemi Corporation >> + */ >> + >> +#include <asm/asm.h> >> +#include <asm/regdef.h> >> + >> + .text > > don't set .text in ASM files. The LEAF() and NODE() macros are hacked to > place the code in .text.FUNCNAME to allow section garbage collect. OK > >> + .set noreorder >> + .extern vcoreiii_tlb_init >> +#ifdef CONFIG_SOC_LUTON >> + .extern pll_init >> +#endif >> + >> +LEAF(lowlevel_init) >> + // As we have no stack yet, we can assume the restricted >> + // luxury of the sX-registers without saving them >> + move s0,ra > > various coding style issues like C++ comments and wrong indentation I will fix it > >> + >> + jal vcoreiii_tlb_init >> + nop >> +#ifdef CONFIG_SOC_LUTON >> + jal pll_init >> + nop >> +#endif >> + jr s0 >> + nop > > please indent instructions in the delay slot by one space OK > - Daniel Thanks again for your review. Gregory -- Gregory Clement, Bootlin Embedded Linux and Kernel engineering http://bootlin.com _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot