Re: [PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

[Jonas Karlman <jonas@kwiboo.se>]

Hi Yao Zi,

On 2025-03-04 20:49, Yao Zi wrote:
> On Tue, Mar 04, 2025 at 08:10:36PM +0800, Chukun Pan wrote:
>> Hi,
>>
>>> +	aliases {
>>> +		mmc0 = &sdmmc;
>>
>> s/mmc0/mmc1
> 
> Will take it and add the missing pinctrl, as Jonas already pointed out.
> 
>>> +&sdmmc {
>>> +	bus-width = <4>;
>>> +	cap-mmc-highspeed;
>>> +	cap-sd-highspeed;
>>
>> I think for sdcard, only cap-sd-highspeed
>> is needed, not cap-mmc-highspeed?
> 
> This makes sense, will remove it in the next version.

Please do not remove the cap-mmc-highspeed prop, I tested the controller
with a microSD to eMMC adapter and MMC HS speed is supported:

  mmc1: card 59b4 removed
  mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
  mmc_host mmc1: Bus speed (slot 0) = 49800000Hz (slot req 52000000Hz, actual 49800000HZ div = 0)
  mmc1: new high speed MMC card at address 0001
  mmcblk1: mmc1:0001 DG4008 7.28 GiB
   mmcblk1: p1 p2
  mmcblk1boot0: mmc1:0001 DG4008 4.00 MiB
  mmcblk1boot1: mmc1:0001 DG4008 4.00 MiB
  mmcblk1rpmb: mmc1:0001 DG4008 4.00 MiB, chardev (499:0)

  ~ # cat /sys/kernel/debug/mmc1/ios
  clock:          52000000 Hz
  vdd:            21 (3.3 ~ 3.4 V)
  bus mode:       2 (push-pull)
  chip select:    0 (don't care)
  power mode:     2 (on)
  bus width:      2 (4 bits)
  timing spec:    1 (mmc high-speed)
  signal voltage: 0 (3.30 V)
  driver type:    0 (driver type B)

Regards,
Jonas

> 
>>> +	disable-wp;
>>
>> Missing pinctrl.
>>
>>> +	rockchip,default-sample-phase = <90>;
>>
>> It seems that all rk3528 devices need to set this
>> default phase, so maybe this can be placed in dtsi?
> 
> Yes, since the tuned phase offset is a SoC-specific value, as pointed
> out by comment in the driver,
> 
> 	this is _not_ a value that is dynamically tuned and is also
> 	_not_ a value that will vary from board to board.  It is a value
> 	that could vary between different SoC models.
> 
> Will take it in the next version, thanks for finding it!
> 
>>> +	sd-uhs-sdr104;
>>
>> The rk3528 devices uses gpio to switch IO voltage, maybe
>> more modes should be added here like vendor kernel?
> 
> I cannot get the relationship between things you mentioned. For the
> regulator, yes, here vqmmc-supply is missing, as already pointed out by
> Jonas.
> 
>> And these devices use 3.3V IO voltage by default.
>>
>> 	sd-uhs-sdr12;
>> 	sd-uhs-sdr25;
>> 	sd-uhs-sdr50;
>> 	sd-uhs-sdr104;
> 
> But I don't think it's necessary to lay out these slower modes
> explicitly, since SDR104 seems to imply them, see
> sd_update_bus_speed_mode() in drivers/mmc/core/sd.c[1],
> 
>         if ((card->host->caps & MMC_CAP_UHS_SDR104) &&
>             (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104)) {
>                         card->sd_bus_speed = UHS_SDR104_BUS_SPEED;
>         } else if ((card->host->caps & MMC_CAP_UHS_DDR50) &&
>                    (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_DDR50)) {
>                         card->sd_bus_speed = UHS_DDR50_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50)) && (card->sw_caps.sd3_bus_mode &
>                     SD_MODE_UHS_SDR50)) {
>                         card->sd_bus_speed = UHS_SDR50_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25)) &&
>                    (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR25)) {
>                         card->sd_bus_speed = UHS_SDR25_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25 |
>                     MMC_CAP_UHS_SDR12)) && (card->sw_caps.sd3_bus_mode &
>                     SD_MODE_UHS_SDR12)) {
>                         card->sd_bus_speed = UHS_SDR12_BUS_SPEED;
>         }
> 
>> Thanks,
>> Chukun
>>
>> -- 
>> 2.25.1
>>
> 
> Regards,
> Yao Zi
> 
> [1]: https://elixir.bootlin.com/linux/v6.13.5/source/drivers/mmc/core/sd.c#L448-L479