On 07/29/2016 10:36 AM, Paul Burton wrote: [...] >>> +#ifndef __ASSEMBLY__ >>> + >>> +#include <asm/io.h> >>> + >>> +#define BUILD_PLAT_ACCESSORS(offset, name) \ >>> +static inline uint32_t read_boston_##name(void) \ >>> +{ \ >>> + uint32_t *reg = (void *)CKSEG1ADDR(BOSTON_PLAT_BASE) + (offset);\ >>> + return __raw_readl(reg); \ >>> +} >> >> Don't we have enough standard accessors to confuse people ? >> Why do you add another custom ones ? Remove this and just use >> standard accessors throughout the code. > > Hi Marek, > > These accessors are simple wrappers around __raw_readl, I'd hardly say > they can be considered confusing. The alternative is lots of: > > val = __raw_readl((void *)CKSEG1ADDR(BOSTON_PLAT_BASE) + OFFSET); > > ...and that is just plain ugly.
This should be map_physmem() + readl(), see ie. the ag7xxx.c driver or whatever other stuff from the atheros ath79 port. Does this work ? > Invoking readl on a field of a struct > representing these registers would be nice, but some of them need to be > accessed from assembly so that would involve duplication which isn't > nice. The struct based access is deprecated, don't bother with it. > I think this way is the best option, where if you want to read the > Boston core_cl register you call read_boston_core_cl() - it's hardly > confusing what that does. Now imagine what would happen if everyone introduced his own my_platform_read_random_register() accessor(s) . This would be utter chaos. >> >>> +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_CORE_CL, core_cl) >>> +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_MMCMDIV, mmcmdiv) >>> +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_DDRCONF0, ddrconf0) >>> + >>> +#endif /* !__ASSEMBLY__ */ >>> + >>> +#endif /* __BOARD_BOSTON_REGS_H__ */ >>> diff --git a/board/imgtec/boston/checkboard.c >>> b/board/imgtec/boston/checkboard.c >>> new file mode 100644 >>> index 0000000..417ac4e >>> --- /dev/null >>> +++ b/board/imgtec/boston/checkboard.c >>> @@ -0,0 +1,29 @@ >>> +/* >>> + * Copyright (C) 2016 Imagination Technologies >>> + * >>> + * SPDX-License-Identifier: GPL-2.0 >>> + */ >>> + >>> +#include <common.h> >>> + >>> +#include <asm/mipsregs.h> >>> + >>> +#include "boston-lcd.h" >>> +#include "boston-regs.h" >>> >>> +int checkboard(void) >>> +{ >>> + u32 changelist; >>> + >>> + lowlevel_display("U-boot "); >>> + >>> + printf("Board: MIPS Boston\n"); >>> + >>> + printf("CPU: 0x%08x", read_c0_prid()); >> >> This should be in print_cpuinfo() > > I don't agree. This goes on to read a board-specific register to > determine information about the CPU (the revision of its RTL) and that > should not be done in arch-level code, which is what every other > implementation of print_cpuinfo is. Ah, so the register used to determine CPU info is board-specific ? That is utterly braindead design in my mind. The read_c0_prid() looked like it is reading some standard register, maybe that's not true ... > Perhaps it would be better if we had > a nice DM-using CPU driver which Boston could have an extension of, but > we don't have that for MIPS right now & this series is not the place to > add it. > >> >>> + changelist = read_boston_core_cl(); >>> + if (changelist > 1) >>> + printf(" cl%x", changelist); >>> + putc('\n'); >>> + >>> + return 0; >>> +} >>> diff --git a/board/imgtec/boston/ddr.c b/board/imgtec/boston/ddr.c >>> new file mode 100644 >>> index 0000000..7caed4b >>> --- /dev/null >>> +++ b/board/imgtec/boston/ddr.c >>> @@ -0,0 +1,30 @@ >>> +/* >>> + * Copyright (C) 2016 Imagination Technologies >>> + * >>> + * SPDX-License-Identifier: GPL-2.0 >>> + */ >>> + >>> +#include <common.h> >>> + >>> +#include <asm/addrspace.h> >>> + >>> +#include "boston-regs.h" >>> + >>> +phys_size_t initdram(int board_type) >>> +{ >>> + u32 ddrconf0 = read_boston_ddrconf0(); >>> + >>> + return (phys_size_t)(ddrconf0 & BOSTON_PLAT_DDRCONF0_SIZE) << 30; >>> +} >>> + >>> +ulong board_get_usable_ram_top(ulong total_size) >>> +{ >>> + DECLARE_GLOBAL_DATA_PTR; >>> + >>> + if (gd->ram_top < CONFIG_SYS_SDRAM_BASE) { >>> + /* 2GB wrapped around to 0 */ >>> + return CKSEG0ADDR(256 << 20); >>> + } >>> + >>> + return min_t(unsigned long, gd->ram_top, CKSEG0ADDR(256 << 20)); >>> +} >>> diff --git a/board/imgtec/boston/lowlevel_init.S >>> b/board/imgtec/boston/lowlevel_init.S >>> new file mode 100644 >>> index 0000000..8928172 >>> --- /dev/null >>> +++ b/board/imgtec/boston/lowlevel_init.S >>> @@ -0,0 +1,56 @@ >>> +/* >>> + * Copyright (C) 2016 Imagination Technologies >>> + * >>> + * SPDX-License-Identifier: GPL-2.0 >>> + */ >>> + >>> +#include <config.h> >>> + >>> +#include <asm/addrspace.h> >>> +#include <asm/asm.h> >>> +#include <asm/mipsregs.h> >>> +#include <asm/regdef.h> >>> + >>> +#include "boston-regs.h" >>> + >>> +.data >>> + >>> +msg_ddr_cal: .ascii "DDR Cal " >>> +msg_ddr_ok: .ascii "DDR OK " >>> + >>> +.text >>> + >>> +LEAF(lowlevel_init) >>> + move s0, ra >>> + >>> + PTR_LA a0, msg_ddr_cal >>> + bal lowlevel_display >> >> Don't you need nop after branch on mips ? > > No. Branch delay slots are filled automatically by the assembler unless > you explicitly tell it not to with a ".set noreorder" directive, and > they're not just filled with NOPs - they can contain essentially any > non-control-flow-affecting instruction that it's safe to execute > regardless of the outcome of the branch. They're also somewhat optional > in MIPSr6 where "compact" branches don't have delay slots, and gone > entirely in microMIPSr6. Oh ok. >>> + >>> + PTR_LI t0, CKSEG1ADDR(BOSTON_PLAT_BASE) >>> +1: lw t1, BOSTON_PLAT_DDR3STAT(t0) >>> + andi t1, t1, BOSTON_PLAT_DDR3STAT_CALIB >>> + beqz t1, 1b -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot