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
