Re: [PATCH] arm64: dts: rockchip: Enable SD UHS-I modes on ROCK Pi 4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: rockchip: sdmmc 1.8V switch races with rockchip_iodomain_probe()
- [Medium] arm64: dts: rockchip: vcc_sdio 3.0V suspend voltage with 1.8V UHS-I mode
--

commit b8883fc4178b08c2e5a74537faa16281101fdba5
Author: Igor Velkov <iav@iav.lv>

arm64: dts: rockchip: Enable SD UHS-I modes on ROCK Pi 4

This commit lowers the minimum voltage of the vcc_sdio regulator to 1.8 V and
enables UHS-I modes (SDR12 through SDR104) with vqmmc-supply on the sdmmc
controller for ROCK Pi 4 boards.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
> index 52de4a79e2f41..3de1d14529c1a 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
> @@ -356,7 +356,7 @@ vcc_sdio: LDO_REG4 {
>  				regulator-name = "vcc_sdio";
>  				regulator-always-on;
>  				regulator-boot-on;
> -				regulator-min-microvolt = <3000000>;
> +				regulator-min-microvolt = <1800000>;
>  				regulator-max-microvolt = <3000000>;
>  				regulator-state-mem {
>  					regulator-on-in-suspend;

[Severity: Medium]
Just below this in vcc_sdio, regulator-state-mem still configures a 3.0 V
suspend voltage:

arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi:
    vcc_sdio: LDO_REG4 {
        ...
        regulator-state-mem {
            regulator-on-in-suspend;
            regulator-suspend-microvolt = <3000000>;
        };
    };

At regulator registration, set_machine_constraints() ->
suspend_set_initial_state() -> __suspend_set_state() ->
rk808_set_suspend_voltage() programs RK808_LDO4_SLP_VSEL_REG to 3.0 V.

When a UHS-I card is initialized, mmc_sd_init_card() ->
mmc_set_uhs_voltage() -> dw_mci_switch_voltage() ->
mmc_regulator_set_vqmmc() switches vcc_sdio to 1.8 V, and
rockchip_iodomain_notify() -> rockchip_iodomain_write() configures the
RK3399 GRF_IO_VSEL SDMMC0_VDD bit for 1.8 V mode.

During system suspend to RAM (mem), _mmc_sd_suspend() -> mmc_power_off() ->
dw_mci_set_ios() -> mmc_regulator_disable_vqmmc() only drops the enable
reference on the regulator-always-on vcc_sdio regulator without restoring
its voltage to 3.0 V, and the non-switchable SD card supply stays powered
with both the card and GRF_IO_VSEL in 1.8 V mode. When PMIC_SLEEP is
asserted, the RK808 switches LDO_REG4 to RK808_LDO4_SLP_VSEL_REG (3.0 V) in
hardware without notifying rockchip-iodomain:

drivers/soc/rockchip/io-domain.c:rockchip_iodomain_notify()
    /*
     * According to Rockchip it's important to keep the SoC IO domain
     * higher than (or equal to) the external voltage.  That means we need
     * to change it before external voltage changes happen in the case
     * of an increase.

Does leaving regulator-suspend-microvolt at 3.0 V drive 3.0 V onto the
1.8 V-configured SDMMC0_VDD I/O domain and SD bus during mem suspend?

> @@ -696,6 +696,11 @@ &sdmmc {
>  	max-frequency = <150000000>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&sdmmc_clk &sdmmc_cd &sdmmc_cmd &sdmmc_bus4>;
> +	sd-uhs-sdr12;
> +	sd-uhs-sdr25;
> +	sd-uhs-sdr50;
> +	sd-uhs-sdr104;
> +	vqmmc-supply = <&vcc_sdio>;

[Severity: Medium]
In arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi, io_domains is a
sibling consumer of vcc_sdio rather than a supplier to sdmmc:

    &io_domains {
        ...
        sdmmc-supply = <&vcc_sdio>;
        status = "okay";
    };

Because drivers/pci/ and drivers/soc/ are linked before drivers/i2c/ in
drivers/Makefile while drivers/mmc/ is linked after drivers/i2c/, when
CONFIG_PCIE_ROCKCHIP_HOST=y both rockchip_pcie_probe() and
rockchip_iodomain_probe() defer on their RK808 regulators before i2c0 probes
and are queued on deferred_probe_pending_list with pcie0 ahead of
io_domains.

Once i2c0 registers the RK808 regulators, dw_mci_rockchip_pltfm_driver
(which uses PROBE_PREFER_ASYNCHRONOUS) finds vcc_sdio ready, probes
immediately without waiting for io_domains, and schedules mmc_rescan().
Meanwhile, deferred_probe_work_func() runs rockchip_pcie_probe() ->
rockchip_pcie_host_init_port() synchronously before
rockchip_iodomain_probe():

drivers/pci/controller/pcie-rockchip-host.c:rockchip_pcie_host_init_port()
    msleep(PCIE_T_PVPERL_MS);
    gpiod_set_value_cansleep(rockchip->perst_gpio, 1);

    msleep(PCIE_RESET_CONFIG_WAIT_MS);

    /* 500ms timeout value should be enough for Gen1/2 training */
    err = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_BASIC_STATUS1,
                             status, PCIE_LINK_UP(status), 20,
                             500 * USEC_PER_MSEC);

When the PCIe slot is empty, this blocks the deferred probe workqueue for
roughly 700 ms before rockchip_iodomain_probe() can call
regulator_register_notifier() on vcc_sdio. During that window,
mmc_rescan() -> mmc_sd_init_card() -> mmc_set_uhs_voltage() ->
dw_mci_switch_voltage() -> mmc_regulator_set_vqmmc() switches vcc_sdio to
1.8 V and runs dw_mci_rk3288_execute_tuning() while GRF_IO_VSEL is still at
its 3.3 V default.

Can this cause SDR104 tuning to fail at boot unless rockchip_iodomain_probe()
is guaranteed to register its vcc_sdio notifier before sdmmc switches voltage?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-rockpi4-sd-uhs-v1-1-fa394d2046e1@iav.lv?part=1