Re: [PATCH v1] ARM: Rockchip: Add rk3568 BananaPi R2 Pro board support

[Sascha Hauer <s.hauer@pengutronix.de>]

On Sun, Jan 23, 2022 at 05:51:49PM +0100, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> This adds support for the BananaPi R2 Pro board.
> It is basicly a copy of rk3568 evb board but with slightly modified DTS.
> Added GPIO-Leds to dts and modified the hw-detection a bit.
> 
> Tested features so far are:
> 
>  - 1st stage booting
>  - Network
>  - SD card (Emmc not tested but basicly same as on EVB)
>  - power LED (green)
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  arch/arm/boards/Makefile                      |   1 +
>  .../rockchip-rk3568-bpi-r2pro/.gitignore      |   1 +
>  .../boards/rockchip-rk3568-bpi-r2pro/Makefile |   2 +
>  .../boards/rockchip-rk3568-bpi-r2pro/board.c  |  93 +++
>  .../rockchip-rk3568-bpi-r2pro/lowlevel.c      |  49 ++
>  arch/arm/dts/Makefile                         |   1 +
>  arch/arm/dts/rk3568-bpi-r2-pro.dts            | 593 ++++++++++++++++++
>  arch/arm/mach-rockchip/Kconfig                |   6 +
>  dts/Bindings/arm/rockchip.yaml                |   5 +
>  images/Makefile.rockchip                      |   7 +
>  10 files changed, 758 insertions(+)
>  create mode 100644 arch/arm/boards/rockchip-rk3568-bpi-r2pro/.gitignore
>  create mode 100644 arch/arm/boards/rockchip-rk3568-bpi-r2pro/Makefile
>  create mode 100644 arch/arm/boards/rockchip-rk3568-bpi-r2pro/board.c
>  create mode 100644 arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
>  create mode 100644 arch/arm/dts/rk3568-bpi-r2-pro.dts
> 
> diff --git a/arch/arm/boards/Makefile b/arch/arm/boards/Makefile
> index 6fe1b5991455..c26f84dcd8de 100644
> --- a/arch/arm/boards/Makefile
> +++ b/arch/arm/boards/Makefile
> @@ -186,4 +186,5 @@ obj-$(CONFIG_MACH_TQMLS1046A)			+= tqmls1046a/
>  obj-$(CONFIG_MACH_MNT_REFORM)			+= mnt-reform/
>  obj-$(CONFIG_MACH_SKOV_ARM9CPU)			+= skov-arm9cpu/
>  obj-$(CONFIG_MACH_RK3568_EVB)			+= rockchip-rk3568-evb/
> +obj-$(CONFIG_MACH_RK3568_R2PRO)			+= rockchip-rk3568-bpi-r2pro/
>  obj-$(CONFIG_MACH_PINE64_QUARTZ64)		+= pine64-quartz64/
> diff --git a/arch/arm/boards/rockchip-rk3568-bpi-r2pro/.gitignore b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/.gitignore
> new file mode 100644
> index 000000000000..f458f794b54c
> --- /dev/null
> +++ b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/.gitignore
> @@ -0,0 +1 @@
> +sdram-init.bin
> diff --git a/arch/arm/boards/rockchip-rk3568-bpi-r2pro/Makefile b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/Makefile
> new file mode 100644
> index 000000000000..01c7a259e9a5
> --- /dev/null
> +++ b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/Makefile
> @@ -0,0 +1,2 @@
> +obj-y += board.o
> +lwl-y += lowlevel.o
> diff --git a/arch/arm/boards/rockchip-rk3568-bpi-r2pro/board.c b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/board.c
> new file mode 100644
> index 000000000000..37634a7e33ed
> --- /dev/null
> +++ b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/board.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#define pr_fmt(fmt) "rk3568-r2pro: " fmt
> +
> +#include <common.h>
> +#include <init.h>
> +#include <mach/bbu.h>
> +#include <aiodev.h>
> +#include <bootsource.h>
> +#include <environment.h>
> +#include <globalvar.h>
> +#include <magicvar.h>
> +#include <deep-probe.h>
> +
> +static bool machine_is_rk3568_r2pro = false;
> +
> +static int rk3568_r2pro_probe(struct device_d *dev)
> +{
> +	enum bootsource bootsource = bootsource_get();
> +	int instance = bootsource_get_instance();
> +
> +	barebox_set_model("RK3568 R2PRO");
> +	barebox_set_hostname("rk3568-r2pro");
> +	machine_is_rk3568_r2pro = true;

Maybe add a bpi to the hostname, model, variable names and instead drop
the rk3568?

> +	if (!IS_ENABLED(CONFIG_AIODEV))
> +		return 0;
> +
> +	if (!machine_is_rk3568_r2pro)
> +		return 0;
> +
> +	hwid_chan = aiochannel_by_name("aiodev0.in_value1_mV");
> +	if (IS_ERR(hwid_chan)) {
> +		ret = PTR_ERR(hwid_chan);
> +		goto err_hwid;
> +	}
> +
> +	ret = aiochannel_get_value(hwid_chan, &hwid_voltage);
> +	if (ret)
> +		goto err_hwid;
> +
> +	pr_info("hwid_voltage: %d\n", hwid_voltage);

The voltage is not really interesting. This should be a pr_debug.

> +
> +	if (hwid_voltage == 1800)
> +		hwid = "V00";
> +	else
> +		hwid = "unknown";

Have you verified this board really encodes the hardware revision using
this adc channel?

> diff --git a/arch/arm/dts/rk3568-bpi-r2-pro.dts b/arch/arm/dts/rk3568-bpi-r2-pro.dts

I saw this device tree looks different from the one you sent for kernel
inclusion. This is not a problem now, but once the kernel dts is
upstream it will show up in dts/ in the barebox tree. We then usually
include the upstream dts from the barebox board dts, so that only the
barebox specific additions are in the barebox dts. Given that it would
be nice if you could minimize the differences now already, so that later
inclusion of the upstream dts becomes easier.

Sascha


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