Re: [PATCH v2 1/4] ARM: novena: Add Kosagi Novena board

[John Watts <contact@jookia.org>]

On Wed, Jan 25, 2023 at 08:33:57PM +0100, Marco Felsch wrote:
> Hi John,

Hello,

> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +// SPDX-FileCopyrightText: 2023 John Watts
> 						^
> I'm not sure if this is required, but can you add your mail address
> here as well?

Sure!

> 
> > +
> > +#include <common.h>
> > +#include <deep-probe.h>
> > +
> > +static int novena_probe(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id novena_of_match[] = {
> > +	{
> > +		.compatible = "kosagi,imx6q-novena",
> > +	},
> 
> Nit: could be a oneliner.

I'm not sure I understand. I copied this from another board.

> > +	/* NOTE: RX is needed for TX to work on this board */
> > +	imx_setup_pad(IOMEM(MX6_IOMUXC_BASE_ADDR), MX6Q_PAD_EIM_D26__UART2_RXD);
> > +	imx_setup_pad(IOMEM(MX6_IOMUXC_BASE_ADDR), MX6Q_PAD_EIM_D27__UART2_TXD);
> 
> Can we add a newline in between to make it more readable?
> 
> > +	imx6_uart_setup(IOMEM(MX6_UART2_BASE_ADDR));
> > +	pbl_set_putc(imx_uart_putc, IOMEM(MX6_UART2_BASE_ADDR));
> 
> Here as well.
> 
> > +	pr_debug(">");
> > +}

Okay.

> We could rewrite this to:
> 
> 	if (bootsrc == BOOTSOURCE_SERIAL)
> 		imx6_barebox_start_usb(IOMEM(MX6_MMDC_PORT01_BASE_ADDR));
> 	else if (bootsrc == BOOTSOURCE_MMC)
> 		imx6_esdhc_start_image(bootinstance);
> 
> 	pr_err("Unsupported boot source %i instance %i\n", bootsrc, bootinstance);
> 	
> 	hang();
> 
> or use switch-case.

I'll do a fix with a switch case. I'm not a fan of having the style you just
described because it's not immediately clear that _start_usb and _start_image
are the end of the function. :)

> 
> > +}
> > +
> > +static void boot_barebox(void)
> > +{
> > +	void *fdt = __dtb_imx6q_novena_start + get_runtime_offset();
> 
> The get_runtime_offset() can be dropped here since we already relocated.

Ah okay, that helps.

> Also we could move this function into the entry_function.

I like the symmetry and verbosity of having the two code paths both named,
it makes reading the entry a bit easier.

I have a habit of moving things to small functions that describe what they
do instead of writing comments.

> > +
> > +	imx6q_barebox_entry(fdt);
> > +}
> > +
> > +ENTRY_FUNCTION_WITHSTACK(start_imx6q_novena, STACK_TOP, r0, r1, r2)
> > +{
> > +	imx6_cpu_lowlevel_init();
> > +	relocate_to_current_adr();
> > +	setup_c();
> > +	barrier();
> 
> After reading the setup_c() and the cache-armv7.S code I think we don't
> need the barrier() here.

Lots of other i.MX6 platforms use this. Shall remove though.

> 
> > +	if (!running_from_ram()) {
> > +		imx6_ungate_all_peripherals();
> > +		setup_uart();
> > +		load_barebox();
> > +	} else {
> > +		boot_barebox();
> > +	}
> 
> This could be re-written to:
> 
> 	imx6_ungate_all_peripherals();
> 
> 	if (IS_ENABLED(CONFIG_DEBUG_LL))
> 		setup_uart();
> 
> 	if (!running_from_ram())
> 		load_barebox();
> 	
> 	boot_barebox;

As Sascha said, this isn't the DEBUG_LL, but I again have kind of the same
function where you can't tell that load_barebox is an exit.

I wouldn't mind something like this:

 	imx6_ungate_all_peripherals();
        setup_uart();
 
 	if (!running_from_ram())
 		load_barebox();
 	else
 		boot_barebox();

But do we want to setup uart twice? I guess it doesn't matter.

Thanks for the review,

John.