[U-Boot] [PATCH 2/6] MSCC: add support for VCoreIII SoCs

[Gregory CLEMENT <gregory.clement@bootlin.com>]

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();
>> +        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