Received-SPF: pass (google.com: domain of linux-kernel+bounces-148592-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Chukun Pan <amadeus@jmu.edu.cn>
To: jonas@kwiboo.se
Cc: amadeus@jmu.edu.cn,
	conor+dt@kernel.org,
	devicetree@vger.kernel.org,
	heiko@sntech.de,
	krzk+dt@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	linux-rockchip@lists.infradead.org,
	robh@kernel.org
Subject: Re: [PATCH 2/2] arm64: dts: rockchip: Add Radxa ROCK3 Model C
Date: Wed, 17 Apr 2024 21:30:16 +0800
Message-Id: <20240417133016.62753-1-amadeus@jmu.edu.cn>
In-Reply-To: <d7de2213-8dd2-42ec-9a30-a569ac71be3e@kwiboo.se>
References: <d7de2213-8dd2-42ec-9a30-a569ac71be3e@kwiboo.se>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

Hi Jonas,
>> +	model = "Radxa ROCK3 Model C";
>
> The marketing name seems to be "Radxa ROCK 3C" according to the product
> page at [1].
>
> [1] https://radxa.com/products/rock3/3c

According to https://wiki.radxa.com/Rock3/3c , it should be called
"Radxa ROCK 3 Model C". I copied rock3a here without paying attention.

>> +	compatible = "radxa,rock3c", "rockchip,rk3566";
>
> A personal preference would be to match the product name and the dtb
> filename, e.g. "radxa,rock-3c".

I thought so too, here is also copied from rock3a.
I think rock3a needs fixing too?

>> +		led_user: led-0 {
> This is called user_led2 in the schematic, in case we want the symbol
> to match the schematic.

>> +		regulator-name = "vcc5v0_usb_host";
> This regulator is named vcc5v0_usb30_host in schematic.

Thanks, I will fix it.

>> +		#clock-cells = <1>;
>> +		clock-names = "mclk";
>> +		clocks = <&cru I2S1_MCLKOUT_TX>;
>
> I think clock-output-names may be missing? Something like:
>
>  clock-output-names = "rk809-clkout1", "rk809-clkout2";

Thanks, I'll add it in the next patch.

>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pmic_int_l>, <&i2s1m0_mclk>;
>> +		rockchip,system-power-controller;

> I think this prop is deprecated and system-power-controller
> should be used instead.

>> +				regulator-state-mem {
>> +					regulator-on-in-suspend;
>> +					regulator-suspend-microvolt = <900000>;

> Not sure we need this in suspend to ram and this can probably use:
>
>  regulator-off-in-suspend
>
> That is also what vendor kernel does.

Will be corrected in the next patch.

>> +			vcc3v3_sd: SWITCH_REG2 {
>> +				regulator-name = "vcc3v3_sd";
>> +				regulator-always-on;
>> +				regulator-boot-on;
>
> If I am reading the schematic correctly this is not connected.

Yes, I didn't notice it was NC, thanks.

>> +&sdhci {
>> +	bus-width = <8>;
>> +	max-frequency = <200000000>;
> This board support HS200, please add:
>
>  mmc-hs200-1_8v

Will be added in the next patch.

>> +&sdmmc0 {
>> +	bus-width = <4>;
>> +	cap-sd-highspeed;
>> +	cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
>
> Please drop the cd-gpios prop, sdmmc0_det is used below and
> works fine on this board.

Do other rk356x boards also need this change?

>> +	sd-uhs-sdr50;
>
> Why limit to sdr50? and not use sd-uhs-sdr104?

The sdr104 mode is not stable on the rk356x platform.
This problem has been reported on both rock3a and e25 boards.

>> +	vmmc-supply = <&vcc3v3_sd>;
>
> If I read the scematics correctly this is using the
> vcc3v3_sys regulator and not the vcc3v3_sd.

Yes, you are right. I didn't notice it, thanks.

>> +&sfc {
>
> This is missing pinctrl:
>
>  pinctrl-names = "default";
>  pinctrl-0 = <&fspi_pins>;

This is already defined on rk356x.dtsi:

	sfc: spi@fe300000 {
		compatible = "rockchip,sfc";
		......
		pinctrl-0 = <&fspi_pins>;
		pinctrl-names = "default";
		status = "disabled";
	};

>> +		spi-max-frequency = <104000000>;
>
> My board is using a GD25LQ128EWIGR same as mentioned in the schematic,
> and datasheet for this flash chip menion 120 mhz and not 104 mhz.

Will be corrected in the next patch, thanks.

Thanks,
Chukun