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.schwierzeck@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.clement@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();elsehal_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 cacheasm 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