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@xxxxxxxxxxx>
> ---
> 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;
