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

[Georgi Djakov <georgi.djakov@linaro.org>]

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