Re: [PATCH v4 3/3] arm64: dts: rockchip: add dts for Ariaboard Photonicat RK3568

[Jonas Karlman <jonas@kwiboo.se>]

On 2024-09-21 15:27, Junhao Xie wrote:
> 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?

Looks like below props in phy node should match the deprecated
snps,reset props.

> 
> /* 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?

Ahh, sorry, I looked in rk3588 sdhci node, please ignore my comment
regarding max-frequency prop :-)

Regards,
Jonas

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