Hi Ahmad. On Thu, Mar 05, 2020 at 09:02:25AM +0100, Ahmad Fatoum wrote: > The Groboards Giant Board is a ATSAMA5D27C-D1G SiP-based SBC. > The board features a 500MHz ARM Cortex-A5 and 128MB DDR2 SDRAM in the > SiP as well as a MicroSD slot on the PCB. > > barebox doesn't yet support the sama5d2 SDHCI-variant, so board support > is limited to toggling the LED and interfacing with the UART, but add > this now till the rest may follow. Device tree is based on the vendor's > device-tree available at https://github.com/Groboards/giantboard-tools > > Signed-off-by: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx> Browsed the patch - everything looked good. A few nit for you to consider. With the nits considered, and not necessarily any changes: Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx> Sam > --- > arch/arm/boards/Makefile | 1 + > arch/arm/boards/sama5d27-giantboard/Makefile | 1 + > .../arm/boards/sama5d27-giantboard/lowlevel.c | 56 ++++ > arch/arm/dts/Makefile | 1 + > arch/arm/dts/at91-sama5d27_giantboard.dts | 299 ++++++++++++++++++ > arch/arm/mach-at91/Kconfig | 8 + > images/Makefile.at91 | 4 + > 7 files changed, 370 insertions(+) > create mode 100644 arch/arm/boards/sama5d27-giantboard/Makefile > create mode 100644 arch/arm/boards/sama5d27-giantboard/lowlevel.c > create mode 100644 arch/arm/dts/at91-sama5d27_giantboard.dts > > diff --git a/arch/arm/boards/Makefile b/arch/arm/boards/Makefile > index 14538af53bf6..9fe458e0a390 100644 > --- a/arch/arm/boards/Makefile > +++ b/arch/arm/boards/Makefile > @@ -113,6 +113,7 @@ obj-$(CONFIG_MACH_RPI_COMMON) += raspberry-pi/ > obj-$(CONFIG_MACH_SABRELITE) += freescale-mx6-sabrelite/ > obj-$(CONFIG_MACH_SABRESD) += freescale-mx6-sabresd/ > obj-$(CONFIG_MACH_FREESCALE_IMX6SX_SABRESDB) += freescale-mx6sx-sabresdb/ > +obj-$(CONFIG_MACH_SAMA5D27_GIANTBOARD) += sama5d27-giantboard/ > obj-$(CONFIG_MACH_SAMA5D27_SOM1) += sama5d27-som1/ > obj-$(CONFIG_MACH_SAMA5D3XEK) += sama5d3xek/ > obj-$(CONFIG_MACH_SAMA5D3_XPLAINED) += sama5d3_xplained/ > diff --git a/arch/arm/boards/sama5d27-giantboard/Makefile b/arch/arm/boards/sama5d27-giantboard/Makefile > new file mode 100644 > index 000000000000..b08c4a93ca27 > --- /dev/null > +++ b/arch/arm/boards/sama5d27-giantboard/Makefile > @@ -0,0 +1 @@ > +lwl-y += lowlevel.o > diff --git a/arch/arm/boards/sama5d27-giantboard/lowlevel.c b/arch/arm/boards/sama5d27-giantboard/lowlevel.c > new file mode 100644 > index 000000000000..cd762bd9f053 > --- /dev/null > +++ b/arch/arm/boards/sama5d27-giantboard/lowlevel.c > @@ -0,0 +1,56 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2019 Ahmad Fatoum, Pengutronix > + */ > + > +#include <common.h> > +#include <init.h> > + > +#include <asm/barebox-arm-head.h> > +#include <asm/barebox-arm.h> > +#include <mach/at91_pmc_ll.h> > + > +#include <mach/hardware.h> > +#include <mach/iomux.h> > +#include <debug_ll.h> > +#include <mach/at91_dbgu.h> > + > +/* PCK = 492MHz, MCK = 164MHz */ > +#define MASTER_CLOCK 164000000 > + > +#define sama5d2_pmc_enable_periph_clock(clk) \ > + at91_pmc_sam9x5_enable_periph_clock(IOMEM(SAMA5D2_BASE_PMC), clk) I see no point in this macro. It is not that this can change or will be reused by others. > + > +static __always_inline void dbgu_init(void) > +{ > + unsigned mck = MASTER_CLOCK / 2; > + > + sama5d2_pmc_enable_periph_clock(SAMA5D2_ID_PIOD); > + > + at91_mux_pio4_set_A_periph(IOMEM(SAMA5D2_BASE_PIOD), > + pin_to_mask(AT91_PIN_PD3)); /* DBGU TXD */ > + > + sama5d2_pmc_enable_periph_clock(SAMA5D2_ID_UART1); > + > + at91_dbgu_setup_ll(IOMEM(SAMA5D2_BASE_UART1), mck, 115200); > + > + putc_ll('>'); > +} > + > +extern char __dtb_z_at91_sama5d27_giantboard_start[]; > + > +ENTRY_FUNCTION(start_sama5d27_giantboard, r0, r1, r2) > +{ > + void *fdt; > + > + arm_cpu_lowlevel_init(); > + > + arm_setup_stack(SAMA5D2_SRAM_BASE + SAMA5D2_SRAM_SIZE - 16); This " - 16" is it cargo cult copied from somewhere else? Or does it really matter? > + > + if (IS_ENABLED(CONFIG_DEBUG_LL)) > + dbgu_init(); > + > + fdt = __dtb_z_at91_sama5d27_giantboard_start + get_runtime_offset(); > + > + barebox_arm_entry(SAMA5_DDRCS, SZ_128M, fdt); > +} The rest looked fine to me. Sam _______________________________________________ barebox mailing list barebox@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox
