linux-kernel - Re: [PATCH 3/3] arm64: dts: rockchip: add dts for Ariaboard Photonicat RK3568

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240905034009.28124-1-amadeus@jmu.edu.cn>
Date: Thu,  5 Sep 2024 11:40:09 +0800
From: Chukun Pan <amadeus@....edu.cn>
To: bigfoot@...ssfun.cn
Cc: amadeus@....edu.cn,
	conor+dt@...nel.org,
	devicetree@...r.kernel.org,
	dsimic@...jaro.org,
	heiko@...ech.de,
	jonas@...boo.se,
	krzk+dt@...nel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH 3/3] arm64: dts: rockchip: add dts for Ariaboard Photonicat RK3568

Hi Junhao,

> ...
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-ariaboard-photonicat.dts

This should be 'rk3568-photonicat.dts',
e.g. "Radxa ROCK 3A" -> rk3568-rock-3a.dts

> ...
> +	model = "Ariaboard Photonicat RK3568";
> +	compatible = "ariaboard,photonicat", "rockchip,rk3568";

The official model name does not include 'RK3568'.

> ...
> +	firmware {
> +		optee: optee {
> +			compatible = "linaro,optee-tz";
> +			method = "smc";
> +		};
> +	};
> +
> ...
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		ramoops: ramoops@...000 {
> +			compatible = "ramoops";
> +			reg = <0 0x110000 0 0xf0000>;
> +			console-size = <0x80000>;
> +			ftrace-size = <0x00000>;
> +			pmsg-size = <0x50000>;
> +			record-size = <0x20000>;
> +		};
> +	};

Maybe these can be moved to rk356x.dtsi?

> ...
> +	vcca1v8: regulator-1v8-vcca {

schematics: VCCA_1V8

> ...
+	vcc3v3_pcie: regulator-3v3-vcc-pcie {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpios = <&gpio0 RK_PA6 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pcie_enable_h>;

schematics: pcie_pwren_h
vcc_syson -> vcc3v3_pi6c
vcc_syson -> vcc3v3_pcie

> +		regulator-always-on;
> +		regulator-boot-on;

No need.

> ...
> +	vcc5v0_sys: regulator-5v0-vcc-sys {

There is no vcc5v0_sys, but vcc_syson.

vcc_syson (5v) -> vcc3v3_sys
vcc_sysin (5v) - (mcu) -> vcc_syson
vccin_5v -> vcc_sysin

> ...
> +	vcc5v0_usb_host: regulator-5v0-vcc-usb-host {

schematics: VCC5V0_USB30_OTG0 and usb_host_pwren_h
It's a little weird, but that's what they're calling it.
Also: VCCIN_5V -> VCC5V0_USB30_OTG0

> ...
> +	vcc5v0_usb_modem: regulator-5v0-vcc-usb-modem {

Are you sure this regulator is 5v?

> ...
> +	vdda0v9: regulator-0v9-vdda {

schematics: VDDA_0V9

> +	wifi_pwrseq: wifi-pwrseq {
> +		compatible = "mmc-pwrseq-simple";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&wifi_enable_h>;

schematics: wifi_reg_on_h
Also you need to enable the clk:

		clocks = <&pmucru CLK_RTC_32K>;
		clock-names = "ext_clock";
		pinctrl-names = "default";
		pinctrl-0 = <&wifi_reg_on_h &clk32k_out1>;

> +		post-power-on-delay-ms = <200>;
> +		reset-gpios = <&gpio2 RK_PB1 GPIO_ACTIVE_LOW>;
> +	};

> ...
> +&pcie30phy {
> +	phy-supply = <&vcc3v3_pcie>;

phy-supply = <&vcc3v3_pi6c>;

> ...
> +&pcie3x2 {
> +	max-link-speed = <1>;
> +	num-lanes = <1>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie30x2m1_pins>;

These are actually pcie30x1m0_pins.

> ...
> +&pmugrf {
> +	reboot-mode {

Maybe these can be moved to rk356x.dtsi?

> ...
> +&sdhci {

Missing mmc-hs200-1_8v;

> ...
> +&sdmmc0 {
> +	bus-width = <4>;
> +	cap-mmc-highspeed;

Why does sdcard need cap-mmc-highspeed?

> +	max-frequency = <150000000>;
> +	sd-uhs-sdr104;

The sdcard does not have 1.8v io voltage,
so this is wrong, please add no-1-8-v;

> +&sdmmc1 {
> +	bus-width = <4>;
> +	cap-sd-highspeed;
> +	cap-sdio-irq;
> +	disable-wp;

sdio wifi does not need disable-wp.

> +	qca_wifi: qca-wifi@1 {
> +		compatible = "qcom,ath10k";

ath10k does not need compatible.

> ...
> +&uart1 {
> ...
> +		clocks = <&pmucru CLK_RTC_32K>;
> +		enable-gpios = <&gpio2 RK_PB7 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&bt_enable_h>;

schematics: bt_reg_on_h
Missing clock-names = "lpo";

-- 
2.25.1