linux-kernel - Re: [PATCH v4 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: <ea7da1f8-5a0e-45ea-a1bb-977a19f8b8b7@classfun.cn>
Date: Sat, 21 Sep 2024 21:27:35 +0800
From: Junhao Xie <bigfoot@...ssfun.cn>
To: Jonas Karlman <jonas@...boo.se>
Cc: devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Chukun Pan <amadeus@....edu.cn>, FUKAUMI Naoki <naoki@...xa.com>,
 Dragan Simic <dsimic@...jaro.org>, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org, Junhao Xie <bigfoot@...ssfun.cn>
Subject: Re: [PATCH v4 3/3] arm64: dts: rockchip: add dts for Ariaboard
 Photonicat RK3568

On 2024/9/19 22:52, Jonas Karlman wrote:
> Hi Junhao,
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-orangepi-5-plus.dtb
>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-photonicat.dtb
> 
> This should probably be added where the rest of rk3568 dtb is located,
> not with the rk3588 ones.

Thanks for your reminder, I made a mistake in the order when renaming dts, I will fix it

>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-quartzpro64.dtb
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5-itx.dtb
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b.dtb
[...]
>> +
>> +&gmac0 {
>> +	status = "disabled";
>> +	/* Motorcomm YT8521SC LAN port (require SGMII) */
> 
> nit: Please be consistent where comments is added, for recgulators above
> the comments is above the node, suggest you move the comment above the
> "&gmac0 {" line, and same for similar comments.

I will edit it. This looks better:

/* Motorcomm YT8521SC LAN port (require SGMII) */
&gmac0 {
	status = "disabled";
};

>> +};
>> +
>> +&gmac1 {
>> +	assigned-clocks = <&cru SCLK_GMAC1_RX_TX>, <&cru SCLK_GMAC1>;
>> +	assigned-clock-parents = <&cru SCLK_GMAC1_RGMII_SPEED>;
>> +	assigned-clock-rates = <0>, <125000000>;
>> +	clock_in_out = "output";
>> +	phy-handle = <&rgmii_phy1>;
>> +	phy-mode = "rgmii-id";
>> +	phy-supply = <&vcc_3v3>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&gmac1m1_miim
>> +		     &gmac1m1_tx_bus2
>> +		     &gmac1m1_rx_bus2
>> +		     &gmac1m1_rgmii_clk
>> +		     &gmac1m1_rgmii_bus>;
>> +	snps,reset-gpio = <&gpio4 RK_PC0 GPIO_ACTIVE_LOW>;
>> +	snps,reset-active-low;
>> +	snps,reset-delays-us = <0 20000 100000>;
> 
> The snps,reset props is deprecated use resets props in phy node instad.

I edited it and it works fine, is the following correct to write?

/* Motorcomm YT8521SC WAN port */
&gmac1 {
	assigned-clocks = <&cru SCLK_GMAC1_RX_TX>, <&cru SCLK_GMAC1>;
	assigned-clock-parents = <&cru SCLK_GMAC1_RGMII_SPEED>;
	assigned-clock-rates = <0>, <125000000>;
	clock_in_out = "output";
	phy-handle = <&rgmii_phy1>;
	phy-mode = "rgmii-id";
	phy-supply = <&vcc_3v3>;
	pinctrl-names = "default";
	pinctrl-0 = <&gmac1m1_miim
		     &gmac1m1_tx_bus2
		     &gmac1m1_rx_bus2
		     &gmac1m1_rgmii_clk
		     &gmac1m1_rgmii_bus>;
	tx_delay = <0>;
	rx_delay = <0>;
	status = "okay";
};

&mdio1 {
	rgmii_phy1: ethernet-phy@0 {
		compatible = "ethernet-phy-ieee802.3-c22";
		reg = <0x0>;
		pinctrl-names = "default";
		pinctrl-0 = <&eth1_phy_rst>;
		reset-assert-us = <20000>;
		reset-deassert-us = <100000>;
		reset-gpios = <&gpio4 RK_PC0 GPIO_ACTIVE_LOW>;
		rx-internal-delay-ps = <1500>;
		tx-internal-delay-ps = <1500>;
	};
};

&pinctrl {
	[...]
	ethernet {
		eth1_phy_rst: eth1_phy_rst {
			rockchip,pins = <4 RK_PC0 RK_FUNC_GPIO &pcfg_pull_none>;
		};
	};
	[...]
};

> 
>> +	status = "okay";
>> +	/* Motorcomm YT8521SC WAN port */
>> +};
[...]
>> +&sdhci {
>> +	bus-width = <8>;
>> +	max-frequency = <200000000>;
> 
> Can be dropped, already in base dtsi.

max-frequency is not in sdhci node of rk356x.dtsi and rk3568.dtsi.
I simply removed the max-frequency and it worked fine.
Can it really be removed?

>> +	mmc-hs200-1_8v;
>> +	non-removable;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&emmc_bus8 &emmc_clk &emmc_cmd>;
>> +	vmmc-supply = <&vcc_3v3>;
>> +	vqmmc-supply = <&vcc_1v8>;
>> +	status = "okay";
>> +	/* eMMC */
>> +};

Thanks for your review, I will fix all problems in next version!

Best regards,
Junhao