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

[Paul Burton]

On 28/07/16 13:06, Marek Vasut wrote:
>> +++ b/board/imgtec/boston/boston-regs.h
>> @@ -0,0 +1,47 @@
>> +/*
>> + * Copyright (C) 2016 Imagination Technologies
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0
>> + */
>> +
>> +#ifndef __BOARD_BOSTON_REGS_H__
>> +#define __BOARD_BOSTON_REGS_H__
>> +
>> +#define BOSTON_PLAT_BASE		0x17ffd000
>> +#define BOSTON_LCD_BASE			0x17fff000
>> +
>> +/*
>> + * Platform Register Definitions
>> + */
>> +#define BOSTON_PLAT_CORE_CL		0x04
>> +
>> +#define BOSTON_PLAT_DDR3STAT		0x14
>> +# define BOSTON_PLAT_DDR3STAT_CALIB	(1 << 2)
>> +
>> +#define BOSTON_PLAT_MMCMDIV		0x30
>> +# define BOSTON_PLAT_MMCMDIV_CLK0DIV	(0xff << 0)
>> +# define BOSTON_PLAT_MMCMDIV_INPUT	(0xff << 8)
>> +# define BOSTON_PLAT_MMCMDIV_MUL	(0xff << 16)
>> +# define BOSTON_PLAT_MMCMDIV_CLK1DIV	(0xff << 24)
>> +
>> +#define BOSTON_PLAT_DDRCONF0		0x38
>> +# define BOSTON_PLAT_DDRCONF0_SIZE	(0xf << 0)
>> +
>> +#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. 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. 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.

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

>> +
>> +	PTR_LI	t0, CKSEG1ADDR(BOSTON_PLAT_BASE)
>> +1:	lw	t1, BOSTON_PLAT_DDR3STAT(t0)
>> +	andi	t1, t1, BOSTON_PLAT_DDR3STAT_CALIB
>> +	beqz	t1, 1b