Am Do., 13. Dez. 2018 um 15:05 Uhr schrieb Gregory CLEMENT <gregory.clem...@bootlin.com>: > > 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.
oh sorry, I missed that there is a difference > > > > >> +/* > >> + * 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. thanks > > > > >> + > >> + 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. ok, it was only a question. Normally ioremap should be completely optimised away by the compiler. > > > >> +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 -- - Daniel _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot