mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: John Watts <contact@jookia.org>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH v2 1/4] ARM: novena: Add Kosagi Novena board
Date: Thu, 26 Jan 2023 18:50:30 +1100	[thread overview]
Message-ID: <Y9IwxqnzVSIvKbr1@novena-choice-citizen> (raw)
In-Reply-To: <20230125193357.tdtpryxe5au6vne6@pengutronix.de>

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.



  parent reply	other threads:[~2023-01-26  7:53 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-22 17:51 [PATCH 0/5] Add support for the " John Watts
2023-01-22 17:51 ` [PATCH 1/5] ARM: novena: Add " John Watts
2023-01-23  9:20   ` Sascha Hauer
2023-01-22 17:51 ` [PATCH 2/5] ARM: novena: Setup RAM using static configuration John Watts
2023-01-22 17:51 ` [PATCH 3/5] ARM: novena: Require the PFUZE regulator John Watts
2023-01-22 17:51 ` [PATCH 4/5] ARM: novena: Read Ethernet MAC address from EEPROM John Watts
2023-01-23  9:33   ` Sascha Hauer
2023-01-23  9:55     ` John Watts
2023-01-24 18:35     ` John Watts
2023-01-25  8:04       ` Sascha Hauer
2023-01-25  8:11         ` John Watts
2023-01-25  8:19           ` Sascha Hauer
2023-01-25 13:31             ` John Watts
2023-01-25 13:48               ` Ahmad Fatoum
2023-01-25 14:04                 ` John Watts
2023-01-25 14:16                   ` Ahmad Fatoum
2023-01-25 14:28                     ` John Watts
2023-01-25 14:33                       ` Ahmad Fatoum
2023-01-25 14:50                         ` John Watts
2023-01-25 15:42               ` Sascha Hauer
2023-01-25 16:13                 ` John Watts
2023-01-22 17:51 ` [PATCH 5/5] ARM: novena: Use DDR3 information from SPD EEPROM John Watts
2023-01-22 23:20   ` John Watts
2023-01-25 16:42 ` [PATCH v2 0/4] Add support for the Kosagi Novena board John Watts
2023-01-25 16:42 ` [PATCH v2 1/4] ARM: novena: Add " John Watts
2023-01-25 19:33   ` Marco Felsch
2023-01-26  7:25     ` Sascha Hauer
2023-01-26  7:50     ` John Watts [this message]
2023-01-26  9:13       ` Marco Felsch
2023-01-25 16:42 ` [PATCH v2 2/4] ARM: novena: Setup RAM using static configuration John Watts
2023-01-25 19:39   ` Marco Felsch
2023-01-26  7:54     ` John Watts
2023-01-26  8:11       ` Sascha Hauer
2023-01-26  9:14         ` Marco Felsch
2023-01-25 16:42 ` [PATCH v2 3/4] ARM: novena: Read Ethernet MAC address from EEPROM John Watts
2023-01-25 20:07   ` Marco Felsch
2023-01-26  8:05     ` John Watts
2023-01-26  9:07       ` Marco Felsch
2023-01-25 16:42 ` [PATCH v2 4/4] ARM: novena: Use DDR3 information from SPD EEPROM John Watts
2023-01-29 23:27 ` [PATCH v3 0/4] Add support for the Kosagi Novena board John Watts
2023-01-29 23:27 ` [PATCH v3 1/4] ARM: novena: Add " John Watts
2023-01-30 20:13   ` Marco Felsch
2023-01-30 20:26     ` John Watts
2023-01-31  9:45       ` Marco Felsch
2023-01-29 23:27 ` [PATCH v3 2/4] ARM: novena: Setup RAM using static configuration John Watts
2023-01-29 23:27 ` [PATCH v3 3/4] ARM: novena: Read Ethernet MAC address from EEPROM John Watts
2023-01-30 20:04   ` Marco Felsch
2023-01-30 20:25     ` John Watts
2023-01-31  9:29       ` Marco Felsch
2023-01-29 23:28 ` [PATCH v3 4/4] ARM: novena: Use DDR3 information from SPD EEPROM John Watts
2023-01-30 20:18   ` Marco Felsch
2023-01-30 20:41     ` John Watts

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y9IwxqnzVSIvKbr1@novena-choice-citizen \
    --to=contact@jookia.org \
    --cc=barebox@lists.infradead.org \
    --cc=m.felsch@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox