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