From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Fitzsimmons Date: Fri, 08 Jun 2018 18:25:44 -0400 Subject: [U-Boot] [PATCH v3 1/1] board: arm: Add support for Broadcom BCM7445 In-Reply-To: <253ca2bc-3a6c-0086-87e1-8914d391a416@gmail.com> (Florian Fainelli's message of "Thu, 7 Jun 2018 09:54:11 -0700") References: <7357ba59e5f71037a5c628dc5d3efe371eb2298a.1528297389.git.fitzsim@fitzsim.org> <253ca2bc-3a6c-0086-87e1-8914d391a416@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Florian Fainelli 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 >> Cc: Stefan Roese >> Cc: Tom Rini >> Cc: Florian Fainelli >> --- > > 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