On 31/07/16 16:56, Marek Vasut wrote:
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 ?
Yes this works. I suggest you read about the MIPS memory map if you wish
to critique this code.
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.
You speak as though this patch introduces new general purpose accessor
functions that perform some arbitrary memory read. It does not. It
introduces functions each of which reads a single register in the only
sane way to read that register, via the standard __raw_readl. It does so
in a pretty well namespaced manner & with names that match the register
names of the platform. If everyone were to do that I fail to see what
the problem would be.
+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 ...
read_c0_prid() is generic, it's the read_boston_core_cl() that is
board-specific & used to print the CPU's RTL revision, as I described
with "goes on to...". I disagree that this is a bad design. It's pretty
logical that an FPGA based development platform might wish to expose
more information about the CPU loaded on it, such as its RTL revision,
than that CPU would expose in general use.
You can insult the design of the system all you like if it makes you
feel better. However, if you expect me to pay any attention to your
opinions then I suggest that you'd do better to make an effort to
understand the system rather than than spewing insulting words & false
assertions about memory accesses being broken or branches being
incorrectly written.
Thanks,
Paul
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
