Re: [PATCH v2 4/4] mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr irq
From: Vijay Viswanath
Date: Wed May 30 2018 - 03:11:21 EST
Hi Georgi,
Thanks for testing the patch on 8096 and pointing out this issue.
The issue is coming because, when card is removed, the HOST_CONTROL2
register is retaining the 1.8V Signalling enable bit till SDHCI reset
happens after a new card is inserted.
Adding the change you suggested can avoid this wait, but I feel a better
solution is to clear the 1.8V signalling bit when the card is removed.
When a new card is inserted, we shouldn't be keeping the 1.8V bit set
until we send cmd11 to the SD card. A new SD card should start with 3V.
One solution is to explicitly clear the HOST_CONTROL2 register when card
is removed.
Other way is to revert the commit:
9718f84b85396e090ca42fafa730410d286d61e3 "mmc: sdhci-msm: Do not reset
the controller if no card in the slot"
The sdhci-msm doesn't require "SDHCI_QUIRK_NO_CARD_NO_RESET". The issue
which above commit is trying to avoid is fixed by the pwr-irq patches.
Resetting the controller will clear the HOST_CONTROL2 register and avoid
this issue.
Can you please try this ? I tested reverting the QUIRK on two platforms:
db410c(8916) and sdm845. SD card insert/remove worked fine after that
and I didn't get any "Reset 0x1 never completed" error during card
insert/remove or shutdown.
Thanks,
Vijay
On 5/29/2018 5:49 PM, Georgi Djakov wrote:
Hello Vijay,
On 09/27/2017 08:34 AM, Vijay Viswanath wrote:
Register writes which change voltage of IO lines or turn the IO bus
on/off require controller to be ready before progressing further. When
the controller is ready, it will generate a power irq which needs to be
handled. The thread which initiated the register write should wait for
power irq to complete. This will be done through the new sdhc msm write
APIs which will check whether the particular write can trigger a power
irq and wait for it with a timeout if it is expected.
The SDHC core power control IRQ gets triggered when -
* There is a state change in power control bit (bit 0)
of SDHCI_POWER_CONTROL register.
* There is a state change in 1.8V enable bit (bit 3) of
SDHCI_HOST_CONTROL2 register.
* Bit 1 of SDHCI_SOFTWARE_RESET is set.
Also add support APIs which are used by sdhc msm write APIs to check
if power irq is expected to be generated and wait for the power irq
to come and complete if the irq is expected.
This patch requires CONFIG_MMC_SDHCI_IO_ACCESSORS to be enabled.
Signed-off-by: Sahitya Tummala <stummala@xxxxxxxxxxxxxx>
Signed-off-by: Vijay Viswanath <vviswana@xxxxxxxxxxxxxx>
---
drivers/mmc/host/sdhci-msm.c | 173 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 171 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
[..]
+/*
+ * 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.
+ * To what state the register writes will change the IO lines should be passed
+ * as the argument req_type. This API will check whether the IO line's state
+ * is already the expected state and will wait for power irq only if
+ * power irq is expected to be trigerred based on the current IO line state
+ * and expected IO line state.
+ */
+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;
+
+ pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
+ mmc_hostname(host->mmc), __func__, req_type,
+ msm_host->curr_pwr_state, msm_host->curr_io_level);
+
+ /*
+ * The IRQ for request type IO High/LOW will be generated when -
+ * there is a state change in 1.8V enable bit (bit 3) of
+ * SDHCI_HOST_CONTROL2 register. The reset state of that bit is 0
+ * which indicates 3.3V IO voltage. So, when MMC core layer tries
+ * to set it to 3.3V before card detection happens, the
+ * IRQ doesn't get triggered as there is no state change in this bit.
+ * The driver already handles this case by changing the IO voltage
+ * level to high as part of controller power up sequence. Hence, check
+ * for host->pwr to handle a case where IO voltage high request is
+ * issued even before controller power up.
+ */
+ if ((req_type & REQ_IO_HIGH) && !host->pwr) {
+ pr_debug("%s: do not wait for power IRQ that never comes, req_type: %d\n",
+ mmc_hostname(host->mmc), req_type);
+ return;
+ }
+ if ((req_type & msm_host->curr_pwr_state) ||
+ (req_type & msm_host->curr_io_level))
+ done = true;
+ /*
+ * This is needed here to handle cases where register writes will
+ * not change the current bus state or io level of the controller.
+ * In this case, no power irq will be triggerred and we should
+ * not wait.
+ */
+ if (!done) {
+ if (!wait_event_timeout(msm_host->pwr_irq_wait,
+ msm_host->pwr_irq_flag,
+ msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
+ __WARN_printf("%s: pwr_irq for req: (%d) timed out\n",
+ mmc_hostname(host->mmc), req_type);
I am seeing the above error message on a db820c device (apq8096). When i
unplug the SD card and then plug it back in, there is a 5 sec card
detection delay and a timeout. Here is a log:
[ 50.253997] mmc0: card 59b4 removed
[ 50.381874] mmc0: sdhci_msm_check_power_status: request 1
curr_pwr_state 2 curr_io_level 4 sdhci_host_ctrl2 b
[ 50.382656] mmc0: sdhci_msm_check_power_status: request 1 done
[ 50.392493] mmc0: sdhci_msm_check_power_status: request 4
curr_pwr_state 1 curr_io_level 4 sdhci_host_ctrl2 b
[ 50.398258] mmc0: sdhci_msm_check_power_status: request 4 done
[ 50.408728] mmc0: sdhci_msm_check_power_status: request 4
curr_pwr_state 1 curr_io_level 4 sdhci_host_ctrl2 8
[ 50.413865] mmc0: sdhci_msm_check_power_status: request 4 done
[ 54.966316] mmc0: sdhci_msm_check_power_status: request 2
curr_pwr_state 1 curr_io_level 4 sdhci_host_ctrl2 8 <-- card is plugged
[ 54.967756] mmc0: sdhci_msm_check_power_status: request 2 done
[ 54.976822] mmc0: sdhci_msm_check_power_status: request 4
curr_pwr_state 2 curr_io_level 8 sdhci_host_ctrl2 8
[ 60.132253] sdhci_msm 74a4900.sdhci: mmc0: pwr_irq for req: (4) timed out
[ 60.132782] mmc0: sdhci_msm_check_power_status: request 4 done
[ 60.139888] mmc0: sdhci_msm_check_power_status: request 4
curr_pwr_state 2 curr_io_level 8 sdhci_host_ctrl2 8
[ 65.252497] sdhci_msm 74a4900.sdhci: mmc0: pwr_irq for req: (4) timed out
The following patch seem to "fix" it.
-- >8 --
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index b2d875afae5f..ca8ad4edcf80 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1049,6 +1049,11 @@ static void sdhci_msm_check_power_status(struct
sdhci_host *host, u32 req_type)
return;
}
+ if (req_type & REQ_IO_LOW &&
+ msm_host->curr_pwr_state & REQ_BUS_ON &&
+ msm_host->curr_io_level & REQ_IO_HIGH &&
+ !host->pwr)
+ done = true;
/*
* The IRQ for request type IO High/LOW will be generated when -
* there is a state change in 1.8V enable bit (bit 3) of
-- >8 --
Reverting this patch series or applying the above patch makes the issue
go away. According to the log, there is no change in the
sdhci_host_ctrl2 register. What do you think?
Thanks,
Georgi
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html