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

[Marco Felsch <m.felsch@pengutronix.de>]

Hi John,

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

...

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

I meant this:

static const struct of_device_id novena_of_match[] = {
	{ .compatible = "kosagi,imx6q-novena", },

> > > +	/* 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. :)

Then an comment like:

/* This should never be reached */

helps :)

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

I think so. I wondered because I saw boards not having a barrier() call
and some having. This triggered my curiosity and I checked the setup_c()
code.

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

I think that correct :)

> Thanks for the review,

You're welcome.

Regards,
  Marco