Hello Ulf, all,
I tried this new patch and did not face issues so far.
That is said, some comments below.
Regards, Jean-Nicolas.
On 7/22/19 1:13 PM, Ulf Hansson wrote:
> The code dealing with busy detection is somewhat complicated. In a way to
> make it a bit clearer, let's try to clarify the comments in the code about
> it.
>
> Additionally, move the part for clearing the so called busy start IRQ, to
> the place where the IRQ is actually delivered. Ideally, this should make
> the code a bit more robust, but also a bit easier to understand.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> ---
>
> Changes in v2:
> - Squashed patch 1 and patch 2.
> - Keep the re-read on the status register, but explain why it's needed.
> - Move the clearing of the busy start IRQ at the point it's delivered.
>
> ---
> drivers/mmc/host/mmci.c | 63 ++++++++++++++++++++++-------------------
> 1 file changed, 34 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 356833a606d5..d3f876c8c292 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1222,47 +1222,58 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND)))
> return;
>
> - /*
> - * ST Micro variant: handle busy detection.
> - */
> + /* Handle busy detection on DAT0 if the variant supports it. */
> if (busy_resp && host->variant->busy_detect) {
>
> - /* We are busy with a command, return */
> + /*
> + * If there is a command in-progress that has been successfully
> + * sent, then bail out if busy status is set and wait for the
> + * busy end IRQ.
> + *
> + * Note that, the HW triggers an IRQ on both edges while
> + * monitoring DAT0 for busy completion, but there is only one
> + * status bit in MMCISTATUS for the busy state. Therefore
> + * both the start and the end interrupts needs to be cleared,
> + * one after the other. So, clear the busy start IRQ here.
> + */
> if (host->busy_status &&
> - (status & host->variant->busy_detect_flag))
To my mind, purpose of that if condition is to wait for busy end event
while the one just below is to clear start event and unmask busy end irq.
So I think maybe it's a bit late to clear busy start event ? What do you
think ?
> + (status & host->variant->busy_detect_flag)) {
> + writel(host->variant->busy_detect_mask,
> + host->base + MMCICLEAR);
> return;
> + }
>
> /*
> - * We were not busy, but we now got a busy response on
> - * something that was not an error, and we double-check
> - * that the special busy status bit is still set before
> - * proceeding.
> + * Before unmasking for the busy end IRQ, confirm that the
> + * command was sent successfully. To keep track of having a
> + * command in-progress, waiting for busy signaling to end,
> + * store the status in host->busy_status.
> + *
> + * Note that, the card may need a couple of clock cycles before
> + * it starts signaling busy on DAT0, hence re-read the
> + * MMCISTATUS register here, to allow the busy bit to be set.
> + * Potentially we may even need to poll the register for a
> + * while, to allow it to be set, but tests indicates that it
> + * isn't needed.
> */
> if (!host->busy_status &&
> !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
> (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
>
> - /* Clear the busy start IRQ */
> - writel(host->variant->busy_detect_mask,
> - host->base + MMCICLEAR);
Why not clearing busy start event as soon as possible ? Maybe I am wrong
but as far as I understand,
we shall (always) enter that if condition before the one just above ?
> -
> - /* Unmask the busy end IRQ */
> writel(readl(base + MMCIMASK0) |
> host->variant->busy_detect_mask,
> base + MMCIMASK0);
> - /*
> - * Now cache the last response status code (until
> - * the busy bit goes low), and return.
> - */
> +
> host->busy_status =
> status & (MCI_CMDSENT|MCI_CMDRESPEND);
> return;
> }
>
> /*
> - * At this point we are not busy with a command, we have
> - * not received a new busy request, clear and mask the busy
> - * end IRQ and fall through to process the IRQ.
> + * If there is a command in-progress that has been successfully
> + * sent and the busy bit isn't set, it means we have received
> + * the busy end IRQ. Clear and mask the IRQ, then continue to
> + * process the command.
> */
> if (host->busy_status) {
>
> @@ -1508,14 +1519,8 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> }
>
> /*
> - * We intentionally clear the MCI_ST_CARDBUSY IRQ (if it's
> - * enabled) in mmci_cmd_irq() function where ST Micro busy
> - * detection variant is handled. Considering the HW seems to be
> - * triggering the IRQ on both edges while monitoring DAT0 for
> - * busy completion and that same status bit is used to monitor
> - * start and end of busy detection, special care must be taken
> - * to make sure that both start and end interrupts are always
> - * cleared one after the other.
> + * Busy detection is managed by mmci_cmd_irq(), including to
> + * clear the corresponding IRQ.
> */
> status &= readl(host->base + MMCIMASK0);
> if (host->variant->busy_detect)
