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

[Marek Vasut]

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