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