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.
