Re: [PATCH] ARM: rockchip: Add support for Pine64 PineTab 2

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Dang,

Thanks for your patch. It looks good, but some minor comments below.

On 02.12.24 17:35, Dang Huynh wrote:
> The Pine64 PineTab 2 tablet is basically the Quartz64 but as
> a finished product.
> 
> There are (currently) two revisions, v0.1 and v2.0.
> 
> v0.1 was sent to developers and there are a few units around
> (probably less than 10?)
> 
> v2.0 is the consumer available version, this version changed the
> display reset pin and hooked WLAN/BT chip to a GPIO.
> 
> There are currently no easy way to detect both revisions. For now,
> we'll only support v2.0.

This is outdated. The patch supports both, but not with the same image.

> +	barebox_set_hostname(model->shortname);

Good idea, otherwise the hostname autogenerated from DT would
contain a period (invalid character for hostnames).

> +	if (bootsource == BOOTSOURCE_MMC && instance == 0)
> +		of_device_enable_path("/chosen/environment-sd");
> +	else
> +		of_device_enable_path("/chosen/environment-emmc");
> +
> +	rockchip_bbu_mmc_register("sd", 0, "/dev/mmc0");
> +	rockchip_bbu_mmc_register("emmc", BBU_HANDLER_FLAG_DEFAULT, "/dev/mmc1");

Don't you want to set BBU_HANDLER_FLAG_DEFAULT depending on whether you
booted from SD or eMMC?

> +#include <arm64/rockchip/rk3566-pinetab2.dtsi>
> +#include "rk356x.dtsi"
> +
> +/ {
> +	chosen: chosen {
> +		environment-sd {
> +			compatible = "barebox,environment";
> +			device-path = &environment_sd;
> +			status = "disabled";
> +		};
> +
> +		environment-emmc {
> +			compatible = "barebox,environment";
> +			device-path = &environment_emmc;
> +			status = "disabled";
> +		};
> +	};
> +
> +	memory@a00000 {
> +		device_type = "memory";
> +		reg = <0x0 0x00a00000 0x0 0x7f600000>;
> +	};

barebox can autodetect this at runtime. I think you can safely drop this.

> +&sdhci {
> +	no-sd;

This property is already in the upstream kernel DT.

> +
> +	partitions {
> +		compatible = "fixed-partitions";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +
> +		environment_emmc: partition@408000 {

Why did you choose address 0x408000 in particular?

> +			label = "barebox-environment";
> +			reg = <0x0 0x408000 0x0 0x8000>;
> +		};
> +	};
> +};

> +config MACH_PINE64_PINETAB2
> +	select ARCH_RK3568
> +	bool "Pine64 PineTab 2"
> +	help
> +	  Say Y here if you are using a Pine64 PineTab 2

One config option is enough and just generate both images always
and merge the help text from the old options.

> --- a/images/Makefile.rockchip
> +++ b/images/Makefile.rockchip
> @@ -31,6 +31,8 @@ image-$(CONFIG_MACH_PHYTEC_SOM_RK3288) += barebox-rk3288-phycore-som.img
>  
>  $(call build_rockchip_image, CONFIG_MACH_RK3568_EVB, start_rk3568_evb, rockchip-rk3568-evb/sdram-init.bin, rk3568-evb)
>  $(call build_rockchip_image, CONFIG_MACH_RK3568_BPI_R2PRO, start_rk3568_bpi_r2pro, rockchip-rk3568-bpi-r2pro/sdram-init.bin, rk3568-bpi-r2pro)
> +$(call build_rockchip_image, CONFIG_MACH_PINE64_PINETAB2_V0, start_pinetab2_v0, pine64-pinetab2/sdram-init.bin, pinetab2-v0)
> +$(call build_rockchip_image, CONFIG_MACH_PINE64_PINETAB2_V2, start_pinetab2_v2, pine64-pinetab2/sdram-init.bin, pinetab2-v2)

Just use CONFIG_MACH_PINE64_PINETAB2 here. Also please enable in multi_v8_defconfig
and rockchip_v8_defconfig for CI coverage.

Thanks,
Ahmad

>  $(call build_rockchip_image, CONFIG_MACH_PINE64_QUARTZ64, start_quartz64a, pine64-quartz64/sdram-init.bin, quartz64a)
>  $(call build_rockchip_image, CONFIG_MACH_PROTONIC_MECSBC, start_mecsbc, protonic-mecsbc/sdram-init.bin, mecsbc)
>  $(call build_rockchip_image, CONFIG_MACH_RADXA_ROCK3, start_rock3a, radxa-rock3/sdram-init.bin, rock3a)
> 
> ---
> base-commit: 7a3cb7e6fd6338144972b7c83675fb5c709ea6f8
> change-id: 20241202-pt2-init-7122fe34f0d6
> 
> Best regards,


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