Re: [PATCH v2 3/3] mmc: k1: do not release the shared AXI clock and reset on probe failure

[Yixun Lan <dlan@kernel.org>]

Hi Dave,

On 18:11 Mon 05 Oct     , Dave Klotz wrote:
> When the probe fails, the driver releases its clock and reset bulks.
> clk_release_bulk() disables every clock and reset_release_bulk()
> asserts every reset, and the first entry of each is the SDH AXI bus
> clock and reset, which all three SDHCI controllers of the K1 share.
> 
> One controller failing to probe therefore stops the bus of the others,
> and the next register access to any of them hangs the hart. On an
> OrangePi RV2, whose device tree enables the eMMC controller without a
> max-frequency, the eMMC probe failed in sdhci_setup_cfg() and U-Boot
> then hung at "MMC:" probing the microSD controller.
> 
> Keep the clocks running and the resets deasserted on failure.
> 
For the clock part:

This doesn't make sense to me, but I've not put my hands on board to 
test and actually replicate the case myself..

It occur to me the clk driver should take care of the consumers and
only disable the clock once 'enable_count' drop to zero, see

https://git.u-boot-project.org/u-boot/custodians/u-boot-spacemit/-/blob/main/drivers/clk/clk-uclass.c?ref_type=heads#L744


For the reset part:
As I checked the code, it seems exactly match what you described here,
where there is no enable count tracking mechanism, so release() will
trigger an assert() operation in hardware layer

so, would this issue only relate to reset?

> Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
> Reviewed-by: Tanmay Kathpalia <tanmay.kathpalia@altera.com>
> ---
> Validation: executable diff unchanged from v1; exact-base replay and
> checkpatch pass. Combined-firmware warm-boot results are in the cover;
> probe-failure paths have not been retested on hardware.
> 
>  drivers/mmc/spacemit_sdhci.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/spacemit_sdhci.c b/drivers/mmc/spacemit_sdhci.c
> index 4284553177..88e06a681c 100644
> --- a/drivers/mmc/spacemit_sdhci.c
> +++ b/drivers/mmc/spacemit_sdhci.c
> @@ -642,10 +642,14 @@
>  	spacemit_sdhci_phy_init(dev, host);
>  	return 0;
>  
> +	/*
> +	 * Leave the clocks and resets alone on failure. The "core" clock and
> +	 * the "axi" reset are the AXI clock and reset that every SDHCI
> +	 * controller of the K1 shares: releasing them would gate the bus of
> +	 * the other controllers, and the next access to one of them hangs.
> +	 */
>  err_reset:
> -	reset_release_bulk(&plat->resets);
>  err_clk:
> -	clk_release_bulk(&plat->clks);
>  	return ret;
>  }
>  
> -- 
> 2.55.0
> 

-- 
Yixun Lan (dlan)