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

2016-08-02 Thread 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 
>>> +
>>> +#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 000..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 
>>> +
>>> +#include 
>>> +
>>> +#include "boston-lcd.h"
>>> +#include "boston-regs.h"
>>>
>>> +int checkboard(void)
>>> +{
>>> +u32 changelist;
>>> +
>>> +lowlevel_display("U-boot  ");
>>> +
>>> +printf("Board: MIPS Boston\n");

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

2016-08-02 Thread 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 
+
+#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 000..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 
+
+#include 
+
+#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

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

2016-08-01 Thread Marek Vasut 
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 
> +
> +#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 ?

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

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

> +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 000..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 
> +
> +#include 
> +
> +#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 can insult the design of the system all you like if it makes you
> feel better. However, if you expect me

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

2016-07-31 Thread Paul Burton 

On 31/07/16 16:56, Marek Vasut wrote:

On 07/29/2016 10:36 AM, Paul Burton wrote:
[...]

+#ifndef __ASSEMBLY__
+
+#include 
+
+#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.



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



+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 000..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 
+
+#include 
+
+#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...". 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.


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.


Thanks,
Paul
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-07-31 Thread Marek Vasut 
On 07/29/2016 10:36 AM, Paul Burton wrote:
[...]
>>> +#ifndef __ASSEMBLY__
>>> +
>>> +#include 
>>> +
>>> +#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 000..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 
>>> +
>>> +#include 
>>> +
>>> +#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 000..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 
>>> +
>>> +#include 
>>> +
>>> +#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 000..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 
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include "boston-regs.h"
>>> +
>>> +.data
>>> +
>>> +msg_ddr_cal:.ascii "DDR Cal "
>>> +msg_ddr_ok:.ascii "DDR OK  "
>>> +
>>> +.text
>>> +
>>> +LEAF(lowlevel_init)
>>> +moves0, ra
>>> +
>>> +PTR_LAa0, msg_ddr_cal
>>> +bal

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

2016-07-31 Thread Daniel Schwierzeck 


Am 31.07.2016 um 12:04 schrieb Paul Burton:
> On 27/07/16 20:21, Daniel Schwierzeck wrote:
>>> diff --git a/configs/boston_defconfig b/configs/boston_defconfig
>>> new file mode 100644
>>> index 000..381203a
>>> --- /dev/null
>>> +++ b/configs/boston_defconfig
>>> @@ -0,0 +1,41 @@
>>> +CONFIG_MIPS=y
>>> +CONFIG_TARGET_BOSTON=y
>>> +CONFIG_SYS_LITTLE_ENDIAN=y
>>> +CONFIG_CPU_MIPS64_R6=y
>>
>> I've noticed that this defconfig with MIPS64r6 breaks buildman and
>> TravisCI for me and likely a lot of other people because most available
>> toolchains (ELDK, kernel.org, etc.) don't have gcc-5.x yet. And Imgtec
>> provides two different toolchains for R6 and R1..R5 so this needs to be
>> handled too in buildman.
>>
>> Does it make sense to add different defconfigs for Boston similar to
>> Malta to have build coverage for MIPS32r2 and MIPS64r2 as well as
>> BigEndian and LittleEndian? We can add the MIPS32r6 and MIPS64r6
>> variants later when more toolchains with a recent gcc are available.
>> What do you think?
> 
> Hi Daniel,
> 
> I don't mind so much - I'm happy to change the defconfig to R2 & have
> separate 32 & 64 bit ones if you prefer. I chose MIPS64r6 for it since
> that's what's being used primarily on real Boston boards. Would you like
> me to submit a v3?

yes please, but only for patch 10/10. The other patches are already good
for merging.

> 
> FYI whilst the codescape toolchains on imgtec.com have a split between
> pre-R6 (MTI toolchain) & R6 (IMG toolchain) they only differ in the
> prebuilt libraries included with them, so since U-Boot (or Linux) don't
> link with those libraries you can build with either toolchain for any
> arch revision. For internal U-Boot releases I compile for all
> combinations of r2 & r6, 32 & 64 bit, big & little endian with the same
> toolchain (then bolt them all together into 1 binary with a bit of magic
> trampoline code at the start, which it'd be neat to get tidied &
> upstream some time!).
> 

thanks for the info. I'll have a look into extending buildman to fetch
the Codescape IMG toolchains for MIPS.

-- 
- Daniel



signature.asc
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-07-31 Thread Paul Burton 

On 27/07/16 20:21, Daniel Schwierzeck wrote:

diff --git a/configs/boston_defconfig b/configs/boston_defconfig
new file mode 100644
index 000..381203a
--- /dev/null
+++ b/configs/boston_defconfig
@@ -0,0 +1,41 @@
+CONFIG_MIPS=y
+CONFIG_TARGET_BOSTON=y
+CONFIG_SYS_LITTLE_ENDIAN=y
+CONFIG_CPU_MIPS64_R6=y


I've noticed that this defconfig with MIPS64r6 breaks buildman and
TravisCI for me and likely a lot of other people because most available
toolchains (ELDK, kernel.org, etc.) don't have gcc-5.x yet. And Imgtec
provides two different toolchains for R6 and R1..R5 so this needs to be
handled too in buildman.

Does it make sense to add different defconfigs for Boston similar to
Malta to have build coverage for MIPS32r2 and MIPS64r2 as well as
BigEndian and LittleEndian? We can add the MIPS32r6 and MIPS64r6
variants later when more toolchains with a recent gcc are available.
What do you think?


Hi Daniel,

I don't mind so much - I'm happy to change the defconfig to R2 & have 
separate 32 & 64 bit ones if you prefer. I chose MIPS64r6 for it since 
that's what's being used primarily on real Boston boards. Would you like 
me to submit a v3?


FYI whilst the codescape toolchains on imgtec.com have a split between 
pre-R6 (MTI toolchain) & R6 (IMG toolchain) they only differ in the 
prebuilt libraries included with them, so since U-Boot (or Linux) don't 
link with those libraries you can build with either toolchain for any 
arch revision. For internal U-Boot releases I compile for all 
combinations of r2 & r6, 32 & 64 bit, big & little endian with the same 
toolchain (then bolt them all together into 1 binary with a bit of magic 
trampoline code at the start, which it'd be neat to get tidied & 
upstream some time!).


Thanks,
Paul
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-07-29 Thread 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_BASE0x17fff000
+
+/*
+ * Platform Register Definitions
+ */
+#define BOSTON_PLAT_CORE_CL0x04
+
+#define BOSTON_PLAT_DDR3STAT   0x14
+# define BOSTON_PLAT_DDR3STAT_CALIB(1 << 2)
+
+#define BOSTON_PLAT_MMCMDIV0x30
+# 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 
+
+#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 000..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 
+
+#include 
+
+#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 000..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 
+
+#include 
+
+#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 000..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 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "boston-regs.h"
+
+.data
+
+msg_ddr_cal:   .ascii "DDR Cal "
+msg_ddr_ok:.ascii "DDR OK  "
+
+.text
+
+LEAF(lowlevel_init)
+   moves0, ra
+
+   PTR_LA  a0, msg_ddr_cal
+   bal lowlevel_display


Don't you need nop after branch on mips ?


No.

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

2016-07-28 Thread Marek Vasut 
On 07/27/2016 04:26 PM, Paul Burton wrote:
> This patch introduces support for building U-Boot to run on the MIPS
> Boston development board. This is a board built around an FPGA & an
> Intel EG20T Platform Controller Hub, used largely as part of the
> development of new CPUs and their software support. It is essentially
> the successor to the older MIPS Malta board.
> 
> Signed-off-by: Paul Burton 


[...]

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

> +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 000..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 
> +
> +#include 
> +
> +#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()

> + 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 000..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 
> +
> +#include 
> +
> +#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 000..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 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "boston-regs.h"
> +
> +.data
> +
> +msg_ddr_cal: .ascii "DDR Cal "
> +msg_ddr_ok:  .ascii "DDR OK  "
> +
> +.text
> +
> +LEAF(lowlevel_init)
> + moves0, ra
> +
> + PTR_LA  a0, msg_ddr_cal
> + bal lowlevel_display

Don't you need nop after branch on mips ?

> +
> + PTR_LI  t0, CKSEG1ADDR(BOSTON_PLAT_BASE)
> +1:   lw  t1, BOSTON_PLAT_DDR3STAT(t0)
> + andit1, t1, BOSTON_PLAT_DDR3STAT_CALIB
> + beqzt1, 1b
> +
> + PTR_LA  a0, msg_ddr_ok
> + bal lowlevel_display
> +
> + movev0, zero
> + jr  s0
> + END(lowlevel_init)
> +
> +LEAF(lowlevel_display)
> + .setpush
> + .setnoat
> + PTR_LI  AT, CKSEG1ADDR(BOSTON_LCD_BASE)

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

2016-07-27 Thread Daniel Schwierzeck 


Am 27.07.2016 um 16:26 schrieb Paul Burton:
> This patch introduces support for building U-Boot to run on the MIPS
> Boston development board. This is a board built around an FPGA & an
> Intel EG20T Platform Controller Hub, used largely as part of the
> development of new CPUs and their software support. It is essentially
> the successor to the older MIPS Malta board.
> 
> Signed-off-by: Paul Burton 
> 
> ---
> 
> Changes in v2:
> - Use AT instead of $1
> - Use a clock driver instead of patching the DT
> 
>  arch/mips/Kconfig   |  16 +++
>  arch/mips/dts/Makefile  |   1 +
>  arch/mips/dts/img,boston.dts| 216 
> 
>  board/imgtec/boston/Kconfig |  16 +++
>  board/imgtec/boston/MAINTAINERS |   6 +
>  board/imgtec/boston/Makefile|   9 ++
>  board/imgtec/boston/boston-lcd.h|  21 
>  board/imgtec/boston/boston-regs.h   |  47 
>  board/imgtec/boston/checkboard.c|  29 +
>  board/imgtec/boston/ddr.c   |  30 +
>  board/imgtec/boston/lowlevel_init.S |  56 ++
>  configs/boston_defconfig|  41 +++
>  include/configs/boston.h|  68 
>  13 files changed, 556 insertions(+)
>  create mode 100644 arch/mips/dts/img,boston.dts
>  create mode 100644 board/imgtec/boston/Kconfig
>  create mode 100644 board/imgtec/boston/MAINTAINERS
>  create mode 100644 board/imgtec/boston/Makefile
>  create mode 100644 board/imgtec/boston/boston-lcd.h
>  create mode 100644 board/imgtec/boston/boston-regs.h
>  create mode 100644 board/imgtec/boston/checkboard.c
>  create mode 100644 board/imgtec/boston/ddr.c
>  create mode 100644 board/imgtec/boston/lowlevel_init.S
>  create mode 100644 configs/boston_defconfig
>  create mode 100644 include/configs/boston.h
> 
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 21066f0..7ba0ef2 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -73,9 +73,25 @@ config MACH_PIC32
>   select OF_CONTROL
>   select DM
>  
> +config TARGET_BOSTON
> + bool "Support Boston"
> + select DM
> + select DM_SERIAL
> + select OF_CONTROL
> + select MIPS_L1_CACHE_SHIFT_6
> + select SUPPORTS_BIG_ENDIAN
> + select SUPPORTS_LITTLE_ENDIAN
> + select SUPPORTS_CPU_MIPS32_R1
> + select SUPPORTS_CPU_MIPS32_R2
> + select SUPPORTS_CPU_MIPS32_R6
> + select SUPPORTS_CPU_MIPS64_R1
> + select SUPPORTS_CPU_MIPS64_R2
> + select SUPPORTS_CPU_MIPS64_R6
> +
>  endchoice
>  
>  source "board/dbau1x00/Kconfig"
> +source "board/imgtec/boston/Kconfig"
>  source "board/imgtec/malta/Kconfig"
>  source "board/micronas/vct/Kconfig"
>  source "board/pb1x00/Kconfig"
> diff --git a/arch/mips/dts/Makefile b/arch/mips/dts/Makefile
> index 2f04d73..6a5e43e 100644
> --- a/arch/mips/dts/Makefile
> +++ b/arch/mips/dts/Makefile
> @@ -4,6 +4,7 @@
>  
>  dtb-$(CONFIG_TARGET_AP121) += ap121.dtb
>  dtb-$(CONFIG_TARGET_AP143) += ap143.dtb
> +dtb-$(CONFIG_TARGET_BOSTON) += img,boston.dtb
>  dtb-$(CONFIG_TARGET_MALTA) += mti,malta.dtb
>  dtb-$(CONFIG_TARGET_PIC32MZDASK) += pic32mzda_sk.dtb
>  dtb-$(CONFIG_BOARD_TPLINK_WDR4300) += tplink_wdr4300.dtb
> diff --git a/arch/mips/dts/img,boston.dts b/arch/mips/dts/img,boston.dts
> new file mode 100644
> index 000..2fbcb93
> --- /dev/null
> +++ b/arch/mips/dts/img,boston.dts
> @@ -0,0 +1,216 @@
> +/dts-v1/;
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "img,boston";
> +
> + chosen {
> + stdout-path = 
> + };
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu@0 {
> + device_type = "cpu";
> + compatible = "img,mips";
> + reg = <0>;
> + clocks = <_boston BOSTON_CLK_CPU>;
> + };
> + };
> +
> + memory@0 {
> + device_type = "memory";
> + reg = <0x 0x1000>;
> + };
> +
> + gic: interrupt-controller {
> + compatible = "mti,gic";
> +
> + interrupt-controller;
> + #interrupt-cells = <3>;
> +
> + timer {
> + compatible = "mti,gic-timer";
> + interrupts = ;
> + clocks = <_boston BOSTON_CLK_CPU>;
> + };
> + };
> +
> + pci0: pci@1000 {
> + status = "disabled";
> + compatible = "xlnx,axi-pcie-host-1.00.a";
> + device_type = "pci";
> + reg = <0x1000 0x200>;
> +
> + #address-cells = <3>;
> + #size-cells = <2>;
> + #interrupt-cells = <1>;
> +
> + interrupt-parent = <>;
> + interrupts = ;
> +
> + ranges = <0x0200 0 0x4000
> +   0x4000 0 0x4000>;
> +
> +