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

[PATCH V2] 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>
---
Changes from v1:
As per Adrian Hunter's comment :
- Removed unrelated changes
- Created a separate function get_cd for cleaner code
- Used READ_ONCE when getting mmc->ops to handle card removal cases
- Reordered if check conditions
---
 drivers/mmc/host/sdhci-msm.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index bf91cb96a0ea..97a895d839c9 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1566,6 +1566,14 @@ static inline void sdhci_msm_complete_pwr_irq_wait(
 	wake_up(&msm_host->pwr_irq_wait);
 }
 
+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_ops && mmc->ops->get_cd ? mmc->ops->get_cd(mmc) : 0;
+}
+
 /*
  * sdhci_msm_check_power_status API should be called when registers writes
  * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
@@ -1579,6 +1587,7 @@ 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);
+	struct mmc_host *mmc = host->mmc;
 	bool done = false;
 	u32 val = SWITCHABLE_SIGNALING_VOLTAGE;
 	const struct sdhci_msm_offset *msm_offset =
@@ -1636,6 +1645,12 @@ 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 ((req_type & REQ_BUS_ON) && mmc->card && !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);
 }
@@ -1694,6 +1709,13 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 		udelay(10);
 	}
 
+	if ((irq_status & CORE_PWRCTL_BUS_ON) && mmc->card && !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;
-- 
2.34.1

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

[Konrad Dybcio]

On 6/20/25 11:03 AM, 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.

This is very difficult to parse. How about:

Many mobile phones feature multi-card tray designs, where the same
tray is used for both SD and SIM cards. If the SD card is placed
at the outermost location in the tray, the SIM card may come in
contact with SD card power-supply while removing the tray, possibly
resulting in SIM damage.

To prevent that, make sure the SD card is really inserted by reading
the Card Detect pin state. If it's not, turn off the power in
sdhci_msm_check_power_status() and set the BUS_FAIL power state on the
controller as part of pwr_irq handling.


(Now I don't know if this is a good fix as far as logic goes, but I'm
simply looking at the patch)

> 
> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
> ---
> Changes from v1:
> As per Adrian Hunter's comment :
> - Removed unrelated changes
> - Created a separate function get_cd for cleaner code
> - Used READ_ONCE when getting mmc->ops to handle card removal cases
> - Reordered if check conditions
> ---
>  drivers/mmc/host/sdhci-msm.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index bf91cb96a0ea..97a895d839c9 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -1566,6 +1566,14 @@ static inline void sdhci_msm_complete_pwr_irq_wait(
>  	wake_up(&msm_host->pwr_irq_wait);
>  }
>  
> +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);

What do you need the READ_ONCE for?> +
> +	return mmc_ops && mmc->ops->get_cd ? mmc->ops->get_cd(mmc) : 0;

I think this op will always exist for our driver, since we call:

sdhci_msm_probe()
 -> sdhci_pltfm_init()
    -> sdhci_alloc_host()

which assigns:

host->mmc_host_ops = sdhci_ops;
mmc->ops = &host->mmc_host_ops;

which contains:

.get_cd         = sdhci_get_cd,

there's some more layers to this matryoshka, so I'm not a 100% sure

> +}
> +
>  /*
>   * sdhci_msm_check_power_status API should be called when registers writes
>   * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
> @@ -1579,6 +1587,7 @@ 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);
> +	struct mmc_host *mmc = host->mmc;
>  	bool done = false;
>  	u32 val = SWITCHABLE_SIGNALING_VOLTAGE;
>  	const struct sdhci_msm_offset *msm_offset =
> @@ -1636,6 +1645,12 @@ 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 ((req_type & REQ_BUS_ON) && mmc->card && !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);
>  }
> @@ -1694,6 +1709,13 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  		udelay(10);
>  	}
>  
> +	if ((irq_status & CORE_PWRCTL_BUS_ON) && mmc->card && !get_cd(host)) {
> +		irq_ack = CORE_PWRCTL_BUS_FAIL;
> +		msm_host_writel(msm_host, irq_ack, host,
> +				msm_offset->core_pwrctl_ctl);

Since you're dropping out if this function, you can pass the parameter
directly to msm_host_writel

Konrad

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

[Adrian Hunter]

On 21/06/2025 13:59, Konrad Dybcio wrote:
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index bf91cb96a0ea..97a895d839c9 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -1566,6 +1566,14 @@ static inline void sdhci_msm_complete_pwr_irq_wait(
>>  	wake_up(&msm_host->pwr_irq_wait);
>>  }
>>  
>> +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);
> What do you need the READ_ONCE for?> +

Yeah I was confusing it with bus_ops.  READ_ONCE() is not needed.
sdhci already assumes mmc->ops->get_cd always exists, so
separate get_cd() is not much use.

With that and cosmetic changes, you can add:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

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

[Sarthak Garg]

On 6/21/2025 4:29 PM, Konrad Dybcio wrote:
> On 6/20/25 11:03 AM, 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.
> 
> This is very difficult to parse. How about:
> 
> Many mobile phones feature multi-card tray designs, where the same
> tray is used for both SD and SIM cards. If the SD card is placed
> at the outermost location in the tray, the SIM card may come in
> contact with SD card power-supply while removing the tray, possibly
> resulting in SIM damage.
> 
> To prevent that, make sure the SD card is really inserted by reading
> the Card Detect pin state. If it's not, turn off the power in
> sdhci_msm_check_power_status() and set the BUS_FAIL power state on the
> controller as part of pwr_irq handling.
> 
> 
> (Now I don't know if this is a good fix as far as logic goes, but I'm
> simply looking at the patch)
> 

Sure will update the commit text.

>>
>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>> ---
>> Changes from v1:
>> As per Adrian Hunter's comment :
>> - Removed unrelated changes
>> - Created a separate function get_cd for cleaner code
>> - Used READ_ONCE when getting mmc->ops to handle card removal cases
>> - Reordered if check conditions
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index bf91cb96a0ea..97a895d839c9 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -1566,6 +1566,14 @@ static inline void sdhci_msm_complete_pwr_irq_wait(
>>   	wake_up(&msm_host->pwr_irq_wait);
>>   }
>>   
>> +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);
> 
> What do you need the READ_ONCE for?> +
>> +	return mmc_ops && mmc->ops->get_cd ? mmc->ops->get_cd(mmc) : 0;
> 
> I think this op will always exist for our driver, since we call:
> 
> sdhci_msm_probe()
>   -> sdhci_pltfm_init()
>      -> sdhci_alloc_host()
> 
> which assigns:
> 
> host->mmc_host_ops = sdhci_ops;
> mmc->ops = &host->mmc_host_ops;
> 
> which contains:
> 
> .get_cd         = sdhci_get_cd,
> 
> there's some more layers to this matryoshka, so I'm not a 100% sure
> 

Yes its not needed will remove it.

>> +}
>> +
>>   /*
>>    * sdhci_msm_check_power_status API should be called when registers writes
>>    * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
>> @@ -1579,6 +1587,7 @@ 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);
>> +	struct mmc_host *mmc = host->mmc;
>>   	bool done = false;
>>   	u32 val = SWITCHABLE_SIGNALING_VOLTAGE;
>>   	const struct sdhci_msm_offset *msm_offset =
>> @@ -1636,6 +1645,12 @@ 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 ((req_type & REQ_BUS_ON) && mmc->card && !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);
>>   }
>> @@ -1694,6 +1709,13 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>>   		udelay(10);
>>   	}
>>   
>> +	if ((irq_status & CORE_PWRCTL_BUS_ON) && mmc->card && !get_cd(host)) {
>> +		irq_ack = CORE_PWRCTL_BUS_FAIL;
>> +		msm_host_writel(msm_host, irq_ack, host,
>> +				msm_offset->core_pwrctl_ctl);
> 
> Since you're dropping out if this function, you can pass the parameter
> directly to msm_host_writel
> 
> Konrad

Sure will update.

~Best regards
Sarthak