Re: [PATCH V2] mmc: mmci: Clarify comments and some code for busy detection

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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)





[Index of Archives]     [Linux Memonry Technology]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux