Re: [PATCH V1] mmc: sdhci-msm: Ensure SD card power isn't ON when card removed

[Adrian Hunter <adrian.hunter@intel.com>]

On 5/11/24 11:35, Sarthak Garg wrote:
> Make sure SD card power is not enabled when the card is
> being removed.
> On multi-card tray designs, the same card-tray would be used for SD
> card and SIM cards. If SD card is placed at the outermost location
> in the tray, then SIM card may come in contact with SD card power-
> supply while removing the tray. It may result in SIM damage.
> So in sdhci_msm_handle_pwr_irq we skip the BUS_ON request when the
> SD card is removed to be in consistent with the MGPI hardware fix to
> prevent any damage to the SIM card in case of mult-card tray designs.
> But we need to have a similar check in sdhci_msm_check_power_status to
> be in consistent with the sdhci_msm_handle_pwr_irq function.
> Also reset host->pwr and POWER_CONTROL register accordingly since we
> are not turning ON the power actually.
> 
> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
> ---
>  drivers/mmc/host/sdhci-msm.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index e00208535bd1..443526c56194 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -1516,10 +1516,11 @@ static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> -	bool done = false;
> -	u32 val = SWITCHABLE_SIGNALING_VOLTAGE;
>  	const struct sdhci_msm_offset *msm_offset =
>  					msm_host->offset;
> +	struct mmc_host *mmc = host->mmc;
> +	bool done = false;
> +	u32 val = SWITCHABLE_SIGNALING_VOLTAGE;

Please don't make unrelated changes.  The above 2 lines
have not changed and should stay where they are.  If you
feel the need to make cosmetic changes, make a separate
patch.

>  
>  	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
>  			mmc_hostname(host->mmc), __func__, req_type,
> @@ -1573,6 +1574,13 @@ static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
>  				 "%s: pwr_irq for req: (%d) timed out\n",
>  				 mmc_hostname(host->mmc), req_type);
>  	}
> +
> +	if (mmc->card && mmc->ops && mmc->ops->get_cd &&
> +		!mmc->ops->get_cd(mmc) && (req_type & REQ_BUS_ON)) {

It would be tidier to have a separate fn for calling get_cd()
e.g.

static int get_cd(struct sdhci_host *host)
{
	struct mmc_host *mmc = host->mmc;

	return mmc->card && mmc->ops && mmc->ops->get_cd ? mmc->ops->get_cd(mmc) : 0;
}

and put the other check first to avoid calling ->get_cd() for no reason:

	if ((req_type & REQ_BUS_ON) && !get_cd(host)) {
		...


> +		sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> +		host->pwr = 0;
> +	}
> +
>  	pr_debug("%s: %s: request %d done\n", mmc_hostname(host->mmc),
>  			__func__, req_type);
>  }
> @@ -1631,6 +1639,14 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  		udelay(10);
>  	}
>  
> +	if (mmc->card && mmc->ops && mmc->ops->get_cd &&
> +		!mmc->ops->get_cd(mmc) && irq_status & CORE_PWRCTL_BUS_ON) {

If the card is being removed, how do you know mmc->ops
won't disappear under you?  You need READ_ONCE otherwise
e.g.

	const struct mmc_host_ops *mmc_ops = READ_ONCE(mmc->ops);

so like:

static int get_cd(struct sdhci_host *host)
{
	struct mmc_host *mmc = host->mmc;
	const struct mmc_host_ops *mmc_ops = READ_ONCE(mmc->ops);

	return mmc->card && mmc_ops && mmc_ops->get_cd ? mmc_ops->get_cd(mmc) : 0;
}


And again, put the other check first e.g.

	if ((irq_status & CORE_PWRCTL_BUS_ON) && !get_cd(host)) {
		...


> +		irq_ack = CORE_PWRCTL_BUS_FAIL;
> +		msm_host_writel(msm_host, irq_ack, host,
> +				msm_offset->core_pwrctl_ctl);
> +		return;
> +	}
> +
>  	/* Handle BUS ON/OFF*/
>  	if (irq_status & CORE_PWRCTL_BUS_ON) {
>  		pwr_state = REQ_BUS_ON;