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

[Marek Vasut]

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


-- 
Best regards,
Marek Vasut