[U-Boot] [PATCH v2 10/10] boston: Introduce support for the MIPS Boston development board

[Paul Burton]

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.

>>>> 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.

>> 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. 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. I 
don't think this series, which has already grown to include various 
generic changes, is the place to do that.

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
>
>