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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z8dZWYpABghRHHge@pie.lan>
Date: Tue, 4 Mar 2025 19:49:45 +0000
From: Yao Zi <ziyao@...root.org>
To: Chukun Pan <amadeus@....edu.cn>
Cc: conor+dt@...nel.org, cristian.ciocaltea@...labora.com,
	detlev.casanova@...labora.com, devicetree@...r.kernel.org,
	frank.wang@...k-chips.com, heiko@...ech.de, jonas@...boo.se,
	krzk+dt@...nel.org, linux-arm-kernel@...ts.infradead.org,
	linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mmc@...r.kernel.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on
 Radxa E20C

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