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
