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

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

[Sarthak Garg]

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;
 
 	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)) {
+		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) {
+		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;
-- 
2.17.1

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

[Adrian Hunter]

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;

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

[Sarthak Garg]

> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Monday, November 11, 2024 1:41 PM
> To: Sarthak Garg (QUIC) <quic_sartgarg@quicinc.com>; Ulf Hansson
> <ulf.hansson@linaro.org>
> Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Can Guo (QUIC) <quic_cang@quicinc.com>; Bao D.
> Nguyen (QUIC) <quic_nguyenb@quicinc.com>; Ram Prakash Gupta (QUIC)
> <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC)
> <quic_pragalla@quicinc.com>; Sayali Lokhande (QUIC)
> <quic_sayalil@quicinc.com>; Nitin Rawat (QUIC)
> <quic_nitirawa@quicinc.com>; Sachin Gupta (QUIC)
> <quic_sachgupt@quicinc.com>; Bhaskar Valaboju (QUIC)
> <quic_bhaskarv@quicinc.com>; Naveen Kumar Goud Arepalli (QUIC)
> <quic_narepall@quicinc.com>; kernel <kernel@quicinc.com>
> Subject: Re: [PATCH V1] mmc: sdhci-msm: Ensure SD card power isn't ON
> when card removed
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> 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.
> 

Reviving this as back from break.
Sure will take care.

> >
> >       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)) {
>                 ...
> 
> 

Sure looks more cleaner.

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

Thanks for pointing this will correct in V2.

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