Re: [PATCH 3/3] arm64: dts: rockchip: add dts for LCKFB Taishan Pi RK3566

From: Junhao Xie <bigfoot@classfun.cn>
To: Chukun Pan <amadeus@jmu.edu.cn>
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>, Jonas Karlman <jonas@kwiboo.se>,
	Chukun Pan <amadeus@jmu.edu.cn>, FUKAUMI Naoki <naoki@radxa.com>,
	Dragan Simic <dsimic@manjaro.org>,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	Junhao Xie <bigfoot@classfun.cn>
Subject: Re: [PATCH 3/3] arm64: dts: rockchip: add dts for LCKFB Taishan Pi RK3566
Date: Mon, 26 Aug 2024 16:03:01 +0800	[thread overview]
Message-ID: <6edb8443-6bb4-4174-8fe0-906208757cef@classfun.cn> (raw)
In-Reply-To: <20240826070109.55659-1-amadeus@jmu.edu.cn>

On 2024/8/26 15:01, Chukun Pan wrote:
> Hi Junhao,
> 
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/rockchip/rk3566-lckfb-tspi.dts
>> ...
>> +	aliases {
>> +		mmc0 = &sdmmc0;
>> +		mmc1 = &sdhci;
>> +		mmc2 = &sdmmc1;
>> +		wifi = &brcmf;
>> +		bluetooth = &bluetooth;
>> +	};
> 
> WiFi and Bluetooth do not need aliases.
> 
>> ...
>> +	vcc5v0_host: vcc5v0-host-regulator {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc5v0_host";
>> +		regulator-boot-on;
>> +		regulator-always-on;
> 
> This regulator does not need always-on and boot-on.
> 

I will remove them.

> 
>> ...
>> +	vccio_flash: vccio-flash {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vccio_flash";
>> +		regulator-min-microvolt = <1800000>;
>> +		regulator-max-microvolt = <1800000>;
>> +		vin-supply = <&vcc_1v8>;
>> +	};
> 
> Generally speaking, vccio_flash is not a DCDC regulator,
> it is directly connected to vcc_1v8. Maybe you need to
> confirm the schematics.
> 

Yes, vccio_flash and vccio_wl are not DCDC regulator, they are connected to vcc_1v8 and vcc_3v3 through voltage selection resistors.

> 
>> +
>> +	vccio_wl: vccio-wl {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vccio_wl";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		vin-supply = <&vcc_3v3>;
>> +	};
> 
> Same as above, usually like this:
> VCC_1V8 or VCCA1V8_PMU - 0R - VCCIO_WL
> `sd-uhs-sdr104` requires 1.8v io voltage,
> you need to confirm the schematics.
> 

It is actually connected to vcc_1v8, I will fix it.

> 
>> ...
>> +&pinctrl {
>> ...
>> pmic {
>> +	pmic {
>> +		pmic_int: pmic_int {
> 
> Only the pmic_int is needed.
> Also `pmic_int: pmic-int {`
> 
>> ...
>> +&sdhci {
>> +	bus-width = <8>;
>> +	max-frequency = <200000000>;
>> +	non-removable;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&emmc_bus8 &emmc_clk &emmc_cmd &emmc_datastrobe &emmc_rstnout>;
>> +	status = "okay";
>> +	vmmc-supply = <&vccio_flash>;
>> +	vqmmc-supply = <&vccio_flash>;
>> +};
> 
> The vmmc requires 3.3V supply (on rk356x).
> 

Yes, the vmmc of eMMC is actually connected to vcc_3v3, I will fix it.

> 
>> ...
>> +&sdmmc1 {
>> ...
>> +	sd-uhs-sdr104;
>> +	vmmc-supply = <&vccio_wl>;
>> +	vqmmc-supply = <&vccio_wl>;
> 
> Same as above.
> 
>> ...
> +&uart1 {
>> ...
> +		vbat-supply = <&vcc3v3_sys>;
> +		vddio-supply = <&vccio_wl>;
> 
> Here corresponds to sdmmc1.
> 
> Thanks,
> Chukun
> 

Thanks for your review, I will fix all problems and post patch v2!

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip