On 08/02/2016 03:12 PM, Paul Burton wrote: > On 01/08/16 19:36, Marek Vasut wrote: >> On 07/31/2016 07:32 PM, Paul Burton wrote: >>> 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. >> >> What am I missing ? > > Hi Marek, > > You're missing that in MIPS the virtual address space includes the > unmapped regions kseg0 & kseg1. To perform uncached access to a physical > address beneath 512MB one can simply use it as an offset into kseg1, > with no need to perform any mapping.
The map_physmem() does this translation, no ? >>>>> 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. >> >> Yes it does, the accessor is globally available. > > They're only available if you include boston-regs.h which lives inside > board/imgtec/boston/, and regardless their availability does not make > them general purpose. Each accesses only a single register in a single > way. That is not a general purpose accessor like readl, __raw_readl, inl > or whatever else - indeed it's built using the standard __raw_readl. OK, I see we have two stubborn people bashing heads against one another. I'll leave this decision about this accessor thing to Dan if you don't mind. >>> 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. >> >> Say you want to find all register accesses -- with random functions with >> ad-hoc names, you cannot do simple git grep, you need to grep for these >> ad-hoc functions as well ... but they won't show up, since there >> is also preprocessor string concatenation, which further obfuscates >> things and makes it unpleasant to work with. >> >> In my opinion, this macro has no value. > > I disagree & find it rather pleasant to use with minimal costs, but > given that there are only 2 such register accesses left since the clock > changes in v2 I've removed it. > >>>>>>> +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...". >> >> So this stuff should be in print_cpuinfo() if it's generic. >> >>> 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. >> >> I am fine with this, you can print an ascii-art pikachu there if you >> want. But board info should go into show_board_info() and CPU info >> should be in print_cpuinfo() . > > You don't seem to understand what I'm saying: this is information about > the CPU but provided by the board. Well this is kinda weird, yes. > This could be tidied up at some point > if we had some way for the arch code to print basic CPU info & the board > to extend it, but that isn't in place. I think potentially the neatest > way to handle this would be via a CPU driver, which we don't yet have. No point in overcomplicating things. > I > don't think this series, which has already grown to include various > generic changes, is the place to do that. ok > Thanks, > Paul > >>> 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. >> >> I am trying to wrap my mind around the design, sorry if that sounded >> like I'm trying to step on your toys. >> >>> Thanks, >>> Paul >> >> -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot