mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Ahmad Fatoum <a.fatoum@pengutronix.de>
To: "Daniel Brát" <danek.brat@gmail.com>, barebox@lists.infradead.org
Subject: Re: [PATCH] ARM: rpi: parse useful data from vc fdt
Date: Mon, 13 Jun 2022 10:37:41 +0200	[thread overview]
Message-ID: <ef933b65-7c24-826e-0233-1d3ceb17d725@pengutronix.de> (raw)
In-Reply-To: <20220611183939.16503-1-danek.brat@gmail.com>

Hello Daniel,

On 11.06.22 20:39, Daniel Brát wrote:
> Videocore first-stage loader on rpi passes us many useful information
> inside the vc fdt, including the real value of PM_RSTS register, not
> easily available by other means and which we can use to determine
> the reset cause.
> Also make the relevant funtions ignore most of the errors since the fdt
> from vc is now optional for barebox's basic function.

Still, it would be good to get a warning message if e.g. the DT couldn't
be parsed.

> 
> Signed-off-by: Daniel Brát <danek.brat@gmail.com>
> ---
>  arch/arm/boards/raspberry-pi/rpi-common.c | 155 ++++++++++++++++------
>  drivers/watchdog/bcm2835_wdt.c            |  23 +---
>  drivers/watchdog/bcm2835_wdt.h            |  40 ++++++
>  3 files changed, 153 insertions(+), 65 deletions(-)
>  create mode 100644 drivers/watchdog/bcm2835_wdt.h
> 
> diff --git a/arch/arm/boards/raspberry-pi/rpi-common.c b/arch/arm/boards/raspberry-pi/rpi-common.c
> index 12a4f4a0a..884dc7239 100644
> --- a/arch/arm/boards/raspberry-pi/rpi-common.c
> +++ b/arch/arm/boards/raspberry-pi/rpi-common.c
> @@ -23,12 +23,14 @@
>  #include <linux/sizes.h>
>  #include <globalvar.h>
>  #include <asm/system_info.h>
> +#include <reset_source.h>
>  
>  #include <mach/core.h>
>  #include <mach/mbox.h>
>  #include <mach/platform.h>
>  
>  #include "lowlevel.h"
> +#include "../../../../drivers/watchdog/bcm2835_wdt.h"

Better move this into include/soc

>  
>  struct rpi_priv;
>  struct rpi_machine_data {
> @@ -186,54 +188,114 @@ static int rpi_env_init(void)
>  	return 0;
>  }
>  
> -/* Extract /chosen/bootargs from the VideoCore FDT into vc.bootargs
> - * global variable. */
> -static int rpi_vc_fdt_bootargs(void *fdt)
> +/* Extract useful information from the VideoCore FDT we got.
> + * Some parameters are defined here:
> + * https://www.raspberrypi.com/documentation/computers/configuration.html#part4
> + */
> +static void rpi_vc_fdt_parse(void *fdt)
>  {
> -	int ret = 0;
> -	struct device_node *root = NULL, *node;
> -	const char *cmdline;
> +	int ret, len;
> +	struct device_node *root, *chosen, *bootloader;
> +	enum reset_src_type rst_src = RESET_UKWN;
> +	const char *str;
> +	char *model;
> +	u32 num, *pnum;
>  
>  	root = of_unflatten_dtb(fdt, INT_MAX);
> -	if (IS_ERR(root)) {
> -		ret = PTR_ERR(root);
> -		root = NULL;
> -		goto out;
> -	}
> +	if (IS_ERR(root))
> +		return;
>  
> -	node = of_find_node_by_path_from(root, "/chosen");
> -	if (!node) {
> -		pr_err("no /chosen node\n");
> -		ret = -ENOENT;
> -		goto out;
> +	str = of_get_property(root, "serial-number", &len);
> +	if (!str)
> +		pr_warn("no 'serial-number' property found in vc fdt\n");
> +	else
> +		globalvar_add_simple("vc.bootargs", xstrndup(str, len));

What does serial-number have to do with bootargs? Typo?

By the way, there is barebox_set_serial_number() now that you
should use instead of a custom $global.vc.serial_number variable.

> +	str = of_get_property(root, "model", &len);
> +	if (!str) {
> +		pr_warn("no 'model' property found in vc fdt\n");
> +	} else {
> +		model = xstrndup(str, len);

How about something like:

else if (len && str[len - 1] == '\0') {
  barebox_set_model(str);
}

A string value for a property without a trailing NUL is malformed
and barebox_set_model already strdups.


> +		barebox_set_model(model);
> +		free(model);
>  	}
>  
> -	cmdline = of_get_property(node, "bootargs", NULL);
> -	if (!cmdline) {
> -		pr_err("no bootargs property in the /chosen node\n");
> -		ret = -ENOENT;
> +	chosen = of_find_node_by_path_from(root, "/chosen");
> +	if (!chosen) {
> +		pr_err("no '/chosen' node found in vc fdt\n");
>  		goto out;
>  	}
>  
> -	globalvar_add_simple("vc.bootargs", cmdline);
> -
> -	switch(cpu_architecture()) {
> -	case CPU_ARCH_ARMv6:
> -		globalvar_add_simple("vc.kernel", "kernel.img");
> -		break;
> -	case CPU_ARCH_ARMv7:
> -		globalvar_add_simple("vc.kernel", "kernel7.img");
> -		break;
> -	case CPU_ARCH_ARMv8:
> -		globalvar_add_simple("vc.kernel", "kernel8.img");
> -		break;
> +	bootloader = of_find_node_by_name(chosen, "bootloader");
> +
> +	str = of_get_property(chosen, "bootargs", &len);
> +	if (!str)
> +		pr_warn("no 'bootargs' property in the /chosen node\n");
> +	else
> +		globalvar_add_simple("vc.bootargs", xstrndup(str, len));
> +
> +	str = of_get_property(chosen, "overlay_prefix", &len);
> +	if (!str)
> +		pr_warn("no 'overlay_prefix' property in the /chosen node\n");
> +	else
> +		globalvar_add_simple("vc.overlay_prefix", xstrndup(str, len));
> +
> +	str = of_get_property(chosen, "os_prefix", &len);
> +	if (!str)
> +		pr_warn("no 'os_prefix' property in the /chosen node\n");
> +	else
> +		globalvar_add_simple("vc.os_prefix", xstrndup(str, len));

Can't we just prepend that to $global.vs.kernel?

> +
> +	ret = of_property_read_u32(chosen, "boot-mode", &num);
> +	if (ret && bootloader)
> +		ret = of_property_read_u32(bootloader, "boot-mode", &num);
> +        if (ret) {
> +        	pr_warn("'boot-mode' property not found\n");
> +	} else {
> +		pnum = xmalloc(sizeof(u32));
> +		*pnum = num;
> +		globalvar_add_simple_int("vc.boot_mode", pnum, "%u");
>  	}

This is apparently an enum, could we use symbolic names instead and
globalvar_add_simple_enum?

>  
> +	ret = of_property_read_u32(chosen, "partition", &num);
> +	if (ret && bootloader)
> +		ret = of_property_read_u32(bootloader, "partition", &num);
> +	if (ret) {
> +		pr_warn("'partition' property not found\n");
> +        } else {

Indentation looks funky.

> +        	pnum = xmalloc(sizeof(u32));
> +        	*pnum = num;
> +        	globalvar_add_simple_int("vc.boot_partition", pnum, "%u");
> +        }
> +
> +	if (IS_ENABLED(CONFIG_RESET_SOURCE)) {
> +		ret = of_property_read_u32(chosen, "pm_rsts", &num);
> +		if (ret && bootloader)
> +			 ret = of_property_read_u32(bootloader, "rsts", &num);
> +
> +		if (ret) {
> +			pr_warn("'pm_rsts'/'rsts' property not found\n");
> +		} else {
> +/*
> + * https://github.com/raspberrypi/linux/issues/932#issuecomment-93989581
> + */
> +			if (num & PM_RSTS_HADPOR_SET)
> +				rst_src = RESET_POR;
> +			else if (num & PM_RSTS_HADDR_SET)
> +				rst_src = RESET_JTAG;
> +			else if (num & PM_RSTS_HADWR_SET)
> +				rst_src = RESET_WDG;
> +			else if (num & PM_RSTS_HADSR_SET)
> +				rst_src = RESET_RST;
> +			reset_source_set_prinst(rst_src,
> +						RESET_SOURCE_DEFAULT_PRIORITY,
> +						reset_source_get_instance());

reset_source_get_instance() is misplaced in the code that's supposed to set it.
Use reset_source_set_priority() instead.

> +		}
> +	}
>  out:
>  	if (root)
>  		of_delete_node(root);
> -
> -	return ret;
> +	return;
>  }
>  
>  static void rpi_vc_fdt(void)
> @@ -241,7 +303,6 @@ static void rpi_vc_fdt(void)
>  	void *saved_vc_fdt;
>  	struct fdt_header *oftree;
>  	unsigned long magic, size;
> -	int ret;
>  
>  	/* VideoCore FDT was copied in PBL just above Barebox memory */
>  	saved_vc_fdt = (void *)(arm_mem_endmem_get());
> @@ -260,16 +321,23 @@ static void rpi_vc_fdt(void)
>  		return;
>  
>  	size = be32_to_cpu(oftree->totalsize);
> -	if (write_file("/vc.dtb", saved_vc_fdt, size)) {
> +	if (write_file("/vc.dtb", saved_vc_fdt, size))
>  		pr_err("failed to save videocore fdt to a file\n");
> -		return;
> -	}
>  
> -	ret = rpi_vc_fdt_bootargs(saved_vc_fdt);
> -	if (ret) {
> -		pr_err("failed to extract bootargs from videocore fdt: %d\n",
> -									ret);
> -		return;
> +	rpi_vc_fdt_parse(saved_vc_fdt);
> +}
> +
> +static void rpi_set_kernel_name(void) {
> +	switch(cpu_architecture()) {
> +	case CPU_ARCH_ARMv6:
> +		globalvar_add_simple("vc.kernel", "kernel.img");
> +		break;
> +	case CPU_ARCH_ARMv7:
> +		globalvar_add_simple("vc.kernel", "kernel7.img");
> +		break;
> +	case CPU_ARCH_ARMv8:
> +		globalvar_add_simple("vc.kernel", "kernel8.img");
> +		break;
>  	}
>  }
>  
> @@ -348,6 +416,7 @@ static int rpi_devices_probe(struct device_d *dev)
>  	armlinux_set_architecture(MACH_TYPE_BCM2708);
>  	rpi_env_init();
>  	rpi_vc_fdt();
> +	rpi_set_kernel_name();
>  
>  	if (dcfg && dcfg->init)
>  		dcfg->init(priv);
> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
> index ece80837b..a7ae25185 100644
> --- a/drivers/watchdog/bcm2835_wdt.c
> +++ b/drivers/watchdog/bcm2835_wdt.c
> @@ -10,28 +10,7 @@
>  #include <restart.h>
>  #include <watchdog.h>
>  
> -#define PM_RSTC		0x1c
> -#define PM_RSTS		0x20
> -#define PM_WDOG		0x24
> -
> -#define PM_WDOG_RESET				0000000000
> -#define PM_PASSWORD				0x5a000000
> -#define PM_WDOG_TIME_SET			0x000fffff
> -#define PM_RSTC_WRCFG_CLR			0xffffffcf
> -#define PM_RSTC_WRCFG_SET			0x00000030
> -#define PM_RSTC_WRCFG_FULL_RESET		0x00000020
> -#define PM_RSTC_RESET				0x00000102
> -
> -#define PM_RSTS_HADPOR_SET			0x00001000
> -#define PM_RSTS_HADSRH_SET			0x00000400
> -#define PM_RSTS_HADSRF_SET			0x00000200
> -#define PM_RSTS_HADSRQ_SET			0x00000100
> -#define PM_RSTS_HADWRH_SET			0x00000040
> -#define PM_RSTS_HADWRF_SET			0x00000020
> -#define PM_RSTS_HADWRQ_SET			0x00000010
> -#define PM_RSTS_HADDRH_SET			0x00000004
> -#define PM_RSTS_HADDRF_SET			0x00000002
> -#define PM_RSTS_HADDRQ_SET			0x00000001
> +#include "bcm2835_wdt.h"
>  
>  #define SECS_TO_WDOG_TICKS(x) ((x) << 16)
>  
> diff --git a/drivers/watchdog/bcm2835_wdt.h b/drivers/watchdog/bcm2835_wdt.h
> new file mode 100644
> index 000000000..ce97b1eb5
> --- /dev/null
> +++ b/drivers/watchdog/bcm2835_wdt.h
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright (C) 2017 Pengutronix, Lucas Stach <l.stach@pengutronix.de>
> + *
> + * Based on code from  Carlo Caione <carlo@carlocaione.org>
> + */
> +
> +#ifndef __BCM2835_WDT_H
> +#define __BCM2835_WDT_H
> +
> +#define PM_RSTC				0x1c
> +#define PM_RSTS				0x20
> +#define PM_WDOG				0x24
> +
> +#define PM_WDOG_RESET			0000000000
> +#define PM_PASSWORD			0x5a000000
> +#define PM_WDOG_TIME_SET		0x000fffff
> +#define PM_RSTC_WRCFG_CLR		0xffffffcf
> +#define PM_RSTC_WRCFG_SET		0x00000030
> +#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
> +#define PM_RSTC_RESET			0x00000102
> +
> +#define PM_RSTS_HADPOR_SET		0x00001000
> +#define PM_RSTS_HADSRH_SET		0x00000400
> +#define PM_RSTS_HADSRF_SET		0x00000200
> +#define PM_RSTS_HADSRQ_SET		0x00000100
> +#define PM_RSTS_HADWRH_SET		0x00000040
> +#define PM_RSTS_HADWRF_SET		0x00000020
> +#define PM_RSTS_HADWRQ_SET		0x00000010
> +#define PM_RSTS_HADDRH_SET		0x00000004
> +#define PM_RSTS_HADDRF_SET		0x00000002
> +#define PM_RSTS_HADDRQ_SET		0x00000001
> +
> +#define PM_RSTS_HADDR_SET \
> +	(PM_RSTS_HADDRQ_SET | PM_RSTS_HADDRF_SET | PM_RSTS_HADDRH_SET)
> +#define PM_RSTS_HADWR_SET \
> +	(PM_RSTS_HADWRQ_SET | PM_RSTS_HADWRF_SET | PM_RSTS_HADWRH_SET)
> +#define PM_RSTS_HADSR_SET \
> +	(PM_RSTS_HADSRQ_SET | PM_RSTS_HADSRF_SET | PM_RSTS_HADSRH_SET)
> +
> +#endif


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

      reply	other threads:[~2022-06-13  8:39 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-11 18:39 Daniel Brát
2022-06-13  8:37 ` Ahmad Fatoum [this message]

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=ef933b65-7c24-826e-0233-1d3ceb17d725@pengutronix.de \
    --to=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=danek.brat@gmail.com \
    --subject='Re: [PATCH] ARM: rpi: parse useful data from vc fdt' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox