[U-Boot] [PATCH v3 1/1] board: arm: Add support for Broadcom BCM7445

[Thomas Fitzsimmons]

Florian Fainelli <f.fainelli at gmail.com> writes:

> On 06/06/2018 11:35 AM, Thomas Fitzsimmons wrote:
>> Add support for loading U-Boot on the Broadcom 7445 SoC.  This port
>> assumes Broadcom's BOLT bootloader is acting as the second stage
>> bootloader, and U-Boot is acting as the third stage bootloader, loaded
>> as an ELF program by BOLT.
>> 
>> Signed-off-by: Thomas Fitzsimmons <fitzsim at fitzsim.org>
>> Cc: Stefan Roese <sr at denx.de>
>> Cc: Tom Rini <trini at konsulko.com>
>> Cc: Florian Fainelli <f.fainelli at gmail.com>
>> ---
>
> Looks good, still some minor comments about the choice of representation
> for physical addresses of peripherals, see below.
>
>> +config BCMSTB_TIMER_LOW
>> +	hex "Address of BCMSTB timer low register"
>> +	default 0xf0412008
>
> This looks very simplistic here since the CPU system control timer is a
> 64-bit timer.

This worked via the default get_ticks implementation in lib/time.c,
which tracks rollovers and converts to a 64-bit value.  But I agree it's
better to use the high timer register, so that (among other reasons)
get_ticks reflects total uptime including time spent in BOLT.  I
overrode get_ticks in v4 of the patch to use the high and low timer
registers.

> I am really not a big fan of all of those configurable addresses which
> are a) fixed given a specific SoC family (7445, 7439 etc.) and b) are
> error prone because we let an user change those without necessarily
> knowing what is the implication. I really think sticking those constants
> into a header file would be much more appropriate.

Makes sense, moved to a 7445-specific header in v4.

>> +void enable_caches(void)
>> +{
>> +	/*
>> +	 * Nothing required here, since the prior stage bootloader has
>> +	 * enabled I-cache and D-cache already.  Implementing this
>> +	 * function silences the warning in the default function.
>> +	 */
>
> This heavily depends on how you load your binary from BOLT, so you must
> be careful about this statement here.

In v4 I adjusted the comment and added an entry to the README to
document the expectation.

Thanks,
Thomas