Hi Daniel, On lun., déc. 10 2018, Daniel Schwierzeck <daniel.schwierz...@gmail.com> wrote:
>> diff --git a/arch/mips/mach-mscc/include/ioremap.h >> b/arch/mips/mach-mscc/include/ioremap.h >> new file mode 100644 >> index 0000000000..8ea5c65ce3 >> --- /dev/null >> +++ b/arch/mips/mach-mscc/include/ioremap.h >> @@ -0,0 +1,51 @@ >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ > > this line should start with a //. There are more files in this patch > which need to be fixed. Actually, according to the documentation (Licenses/README): The SPDX license identifier is added in form of a comment. The comment style depends on the file type:: C source: // SPDX-License-Identifier: <SPDX License Expression> C header: /* SPDX-License-Identifier: <SPDX License Expression> */ So for a C header file, /* comment */ is correct. > >> +/* >> + * 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) > > this define is superfluous because this directory is only added to the > include paths when CONFIG_ARCH_MSCC is selected OK > >> + 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; >> +} [...] >> +/* >> + * DDR memory sanity checking failed, tally and do hard reset >> + * >> + * NB: Assumes inlining as no stack is available! >> + */ >> +static inline void hal_vcoreiii_ddr_failed(void) >> +{ >> + register u32 reset; >> + >> + writel(readl(BASE_CFG + ICPU_GPR(6)) + 1, BASE_CFG + ICPU_GPR(6)); >> + >> + clrbits_le32(BASE_DEVCPU_GCB + PERF_GPIO_OE, BIT(19)); >> + >> + /* Jump to reset - does not return */ >> + reset = KSEG0ADDR(_machine_restart); >> + /* Reset while running from cache */ >> + icache_lock((void *)reset, 128); >> + asm volatile ("jr %0"::"r" (reset)); > > could you briefly describe the reason for this in a comment? It's not > clear why this code is necessary without knowing the SoC. AFAIU from > your last mail the boot SPI flash is mapped to KUSEG and you need to > establish a TLB mapping in lowlevel_init() to be able to move to > KSEG0. The reboot workaround in _machine_restart() will change the SPI NOR into SW bitbang. This will render the CPU unable to execute directly from the NOR, which is why the reset instructions are prefetched into the I-cache. When failing the DDR initialization we are executing from NOR. The last instruction in _machine_restart() will reset the MIPS CPU (and the cache), and the CPU will start executing from the reset vactor. I will add this explanation as comment. > >> + >> + panic("DDR init failed\n"); >> +} >> + >> +/* >> + * 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 */ >> + register u32 val = readl(BASE_CFG + ICPU_MEMCTRL_CFG); >> + >> + val |= ICPU_MEMCTRL_CFG_DDR_ECC_ERR_ENA; >> + val &= ~ICPU_MEMCTRL_CFG_BURST_SIZE; >> + >> + writel(val, BASE_CFG + ICPU_MEMCTRL_CFG); >> +#endif >> + >> + /* Reset Status register - sticky bits */ >> + writel(readl(BASE_CFG + ICPU_MEMCTRL_STAT), BASE_CFG + >> ICPU_MEMCTRL_STAT); >> +} >> + >> +/* NB: Assumes inlining as no stack is available! */ >> +static inline int look_for(u32 bytelane) >> +{ >> + register u32 i; >> + >> + /* Reset FIFO in case any previous access failed */ >> + for (i = 0; i < sizeof(training_data); i++) { >> + register u32 byte; >> + >> + memphy_soft_reset(); >> + /* Reset sticky bits */ >> + writel(readl(BASE_CFG + ICPU_MEMCTRL_STAT), >> + BASE_CFG + ICPU_MEMCTRL_STAT); >> + /* Read data */ >> + byte = ((volatile u8 *)MSCC_DDR_TO)[bytelane + (i * 4)]; > > __raw_readl()? I had tried it before but without luck, but after trying harder this time I managed to use read(b|l)/write(b|l) everywhere and get ride of the volatile variable. > >> + /* >> + * Prevent the compiler reordering the instruction so >> + * the read of RAM happens after the check of the >> + * errors. >> + */ >> + asm volatile("" : : : "memory"); > > this is available as barrier(). But according to context you could use > rmb(). Anyway with the current volatile pointer or the suggested > __raw_readl() the compiler shouldn't reorder at all I had a close look on the code generating the __raw_readl and there is nothing there to guaranty the ordering. Actually in our case (32 bits) __read_readl is just: static inline u32 __raw_readl(const volatile void __iomem *mem) { u32 __val; __val = *mem; return __val; } initial code is here: https://elixir.bootlin.com/u-boot/v2018.11-rc3/source/arch/mips/include/asm/io.h#L265 but __swizzle_addr_l() did nothing https://elixir.bootlin.com/u-boot/v2018.11-rc3/source/arch/mips/include/asm/mach-generic/mangle-port.h#L10 same for __raw_ioswabl(): https://elixir.bootlin.com/u-boot/v2018.11-rc3/source/arch/mips/include/asm/io.h#L35 So the code is the same that we have written. I agree it is cleaner to use __raw_readl but it doesn't add anything about the ordering. It is the same for the use of the volatile, it ensures that the compiler will always produce a operation to read the data in memory, but it is not about ordering. As you suggested I will use rmb(); >> +static inline int hal_vcoreiii_train_bytelane(u32 bytelane) >> +{ >> + register int res; >> + register u32 dqs_s; >> + >> + set_dly(bytelane, 0); // Start training at DQS=0 > > no C++ style comments OK > >> + while ((res = look_for(bytelane)) == DDR_TRAIN_CONTINUE) >> + ; >> + if (res != DDR_TRAIN_OK) >> + return res; >> + >> + dqs_s = readl(BASE_CFG + ICPU_MEMCTRL_DQS_DLY(bytelane)); >> + while ((res = look_past(bytelane)) == DDR_TRAIN_CONTINUE) >> + ; >> + if (res != DDR_TRAIN_OK) >> + return res; >> + /* Reset FIFO - for good measure */ >> + memphy_soft_reset(); >> + /* Adjust to center [dqs_s;cur] */ >> + center_dly(bytelane, dqs_s); >> + return DDR_TRAIN_OK; >> +} >> + >> +/* This algorithm is converted from the TCL training algorithm used >> + * during silicon simulation. >> + * NB: Assumes inlining as no stack is available! >> + */ >> +static inline int hal_vcoreiii_init_dqs(void) >> +{ >> +#define MAX_DQS 32 >> + register u32 i, j; >> + >> + for (i = 0; i < MAX_DQS; i++) { >> + set_dly(0, i); // Byte-lane 0 > > no C++ style comments OK > >> + for (j = 0; j < MAX_DQS; j++) { >> + register u32 __attribute__ ((unused)) byte; > > why unused? If you really need it, you could use __maybe_unused Because the purpose of this variable is just to access the memory, we don't do nothing of the value read, and gcc complain about it. But as you suggest I will use __maybe_unused. > >> + set_dly(1, j); // Byte-lane 1 >> + /* Reset FIFO in case any previous access failed */ >> + memphy_soft_reset(); >> + writel(readl(BASE_CFG + ICPU_MEMCTRL_STAT), >> + BASE_CFG + ICPU_MEMCTRL_STAT); >> + byte = ((volatile u8 *)MSCC_DDR_TO)[0]; >> + byte = ((volatile u8 *)MSCC_DDR_TO)[1]; >> + if (!(readl(BASE_CFG + ICPU_MEMCTRL_STAT) & >> + (ICPU_MEMCTRL_STAT_RDATA_MASKED | >> + ICPU_MEMCTRL_STAT_RDATA_DUMMY))) >> + return 0; >> + } >> + } >> + return -1; >> +} >> + >> +static inline int dram_check(void) >> +{ >> +#define DDR ((volatile u32 *) MSCC_DDR_TO) >> + register u32 i; >> + >> + for (i = 0; i < 8; i++) { >> + DDR[i] = ~i; >> + if (DDR[i] != ~i) > > __raw_readl(), __raw_writel() and drop the explicit volatile? Yes, as explain above, it s done now. >> + >> +/* >> + * Target offset base(s) >> + */ >> +#define MSCC_IO_ORIGIN1_OFFSET 0x70000000 >> +#define MSCC_IO_ORIGIN1_SIZE 0x00200000 >> +#define MSCC_IO_ORIGIN2_OFFSET 0x71000000 >> +#define MSCC_IO_ORIGIN2_SIZE 0x01000000 >> +#define BASE_CFG ((void __iomem *)0x70000000) >> +#define BASE_DEVCPU_GCB ((void __iomem *)0x71070000) > > Would it be possible on that SoC to define those register offsets as > simple physical address and create the mapping when needed? > For example: > > void foo() > { > void __iomem *base_cfg = ioremap(BASE_CFG, ...); > writel(base_cfg + XXX, 0); > } Actually creating the mapping is just casting the physical address in an (void __iomem *), see our plat_ioremap. Calling ioremap in every function will just grow them with little benefit. If you really want it, what I could is sharing void __iomem *base_cfg and void __iomem *base_devcpu_gcb at platform level, and initialize them only once very early during the boot. >> +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 >> + >> + jal vcoreiii_tlb_init >> + nop > > we use the same style as Linux MIPS where instructions in the delay slot > should be indented by an extra space. OK Thanks, 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