Message-ID: <75cc31d9-beb2-456b-a9e8-8f7ec4711029@kwiboo.se>
Date: Thu, 2 May 2024 14:45:21 +0200
From: Jonas Karlman <jonas@...boo.se>
To: Chukun Pan <amadeus@....edu.cn>
Cc: conor+dt@...nel.org, devicetree@...r.kernel.org, heiko@...ech.de,
 krzk+dt@...nel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org,
 robh@...nel.org
Subject: Re: [PATCH 2/2] arm64: dts: rockchip: Add Radxa ROCK3 Model C

Hi Chukun,

Sorry for late reply, I will test and review your v2 later today :-)

On 2024-04-17 15:30, Chukun Pan wrote:
> 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?

Yes, I think in general on Rockchip platform it is preferred to
configure the pin for sd-card detect function instead of using the
cd-gpios prop (pin configured to use gpio function) when the default
pin is used and correctly wired to sd-slot.

The RK3566 HW design guide also mention following:

"""
RK3566 reuses JTAG function and SDMMC function together to reduce IO pin
count and take into account the convenience of the complete machine
debugging. The SDMMC0_DET pin is used to switch the output function.
Therefore, this pin must be configured before power-on. Otherwise, the
debugging during the boot period will be affected if JTAG has no output,
and SDMMC0 has no output will affects SDMMC boot.
- SDMMC0_DET pin detects that the level is high, the IO switches to the
  JTAG function;
- SDMMC0_DET detects that the level is low (normal state of SD card
  insertion, and the PIN is pulled down by SD card slot), the IO
  function is switched to SDMMC;
"""

And similar was changed on RK3588 boards in:

arm64: dts: rockchip: rk3588: remove redundant cd-gpios in sdmmc node
https://lore.kernel.org/linux-rockchip/20240201034621.1970279-1-kever.yang@rock-chips.com/

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

Do you have any references for these issues?

When I tested on Radxa ZERO 3W/3E there was an issue executing tuning
that resulted in a not working sdr104 mode. However, after the card is
removed and re-inserted the tuning works and the card can use sdr104
mode.

That issue seems to be related to the io-domain driver not being notified
about the voltage change when mmc driver is probed during boot. When the
card is removed and re-inserted the io-domain driver gets notified and
re-configure the io-domain.

Testing on a RK3328 board (Rock64) the io-domain driver gets notified
about the voltage change during boot and the tuning is successful for
sdr104 mode.

Will re-run tests on my ROCK 3C boards to validate is that is the same
issue, guessing it may affect multiple RK356x boards.

Regards,
Jonas

> 
>>> +	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@...00000 {
> 		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

Powered by blists - more mailing lists