Re: [PATCH v2 4/4] mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr irq

Vijay Viswanath
2018-05-30T07:11:16Z

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@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>