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 |
