diff mbox

[2/3,v2] mmc: mmci: refactor ST Micro busy detection

Message ID 1477386367-18514-2-git-send-email-linus.walleij@linaro.org
State New
Headers show

Commit Message

Linus Walleij Oct. 25, 2016, 9:06 a.m. UTC 
The ST Micro-specific busy detection was made after the assumption
that only this variant supports busy detection. So when doing busy
detection, the host immediately tries to use some ST-specific
register bits.

Since the qualcomm variant also supports some busy detection
schemes, encapsulate the variant flags better in the variant struct
and prepare to add more variants by just providing some bitmasks
to the logic.

Put the entire busy detection logic within an if()-clause in the
mmci_cmd_irq() function so the code is only executed when busy
detection is enabled, and so that it is kept in (almost) one
place, and add comments describing what is going on so the
code can be understood.

Tested on the Ux500 by introducing some prints in the busy
detection path and noticing how the IRQ is enabled, used and
disabled successfully.

Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
ChangeLog v1->v2:
- Rebase on new register naming.
---
 drivers/mmc/host/mmci.c | 113 +++++++++++++++++++++++++++++++++++-------------
 drivers/mmc/host/mmci.h |   2 +-
 2 files changed, 85 insertions(+), 30 deletions(-)

-- 
2.7.4

--
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

Comments

Ulf Hansson Nov. 7, 2016, 12:43 p.m. UTC | #1 
On 25 October 2016 at 11:06, Linus Walleij <linus.walleij@linaro.org> wrote:
> The ST Micro-specific busy detection was made after the assumption

> that only this variant supports busy detection. So when doing busy

> detection, the host immediately tries to use some ST-specific

> register bits.

>

> Since the qualcomm variant also supports some busy detection

> schemes, encapsulate the variant flags better in the variant struct

> and prepare to add more variants by just providing some bitmasks

> to the logic.

>

> Put the entire busy detection logic within an if()-clause in the

> mmci_cmd_irq() function so the code is only executed when busy

> detection is enabled, and so that it is kept in (almost) one

> place, and add comments describing what is going on so the

> code can be understood.

>

> Tested on the Ux500 by introducing some prints in the busy

> detection path and noticing how the IRQ is enabled, used and

> disabled successfully.

>

> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


Thanks, applied for next!

Kind regards
Uffe

> ---

> ChangeLog v1->v2:

> - Rebase on new register naming.

> ---

>  drivers/mmc/host/mmci.c | 113 +++++++++++++++++++++++++++++++++++-------------

>  drivers/mmc/host/mmci.h |   2 +-

>  2 files changed, 85 insertions(+), 30 deletions(-)

>

> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c

> index 6a8ea9c633d4..7f68fa7a961e 100644

> --- a/drivers/mmc/host/mmci.c

> +++ b/drivers/mmc/host/mmci.c

> @@ -71,7 +71,12 @@ static unsigned int fmax = 515633;

>   * @f_max: maximum clk frequency supported by the controller.

>   * @signal_direction: input/out direction of bus signals can be indicated

>   * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock

> - * @busy_detect: true if busy detection on dat0 is supported

> + * @busy_detect: true if the variant supports busy detection on DAT0.

> + * @busy_dpsm_flag: bitmask enabling busy detection in the DPSM

> + * @busy_detect_flag: bitmask identifying the bit in the MMCISTATUS register

> + *                   indicating that the card is busy

> + * @busy_detect_mask: bitmask identifying the bit in the MMCIMASK0 to mask for

> + *                   getting busy end detection interrupts

>   * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply

>   * @explicit_mclk_control: enable explicit mclk control in driver.

>   * @qcom_fifo: enables qcom specific fifo pio read logic.

> @@ -98,6 +103,9 @@ struct variant_data {

>         bool                    signal_direction;

>         bool                    pwrreg_clkgate;

>         bool                    busy_detect;

> +       u32                     busy_dpsm_flag;

> +       u32                     busy_detect_flag;

> +       u32                     busy_detect_mask;

>         bool                    pwrreg_nopower;

>         bool                    explicit_mclk_control;

>         bool                    qcom_fifo;

> @@ -178,6 +186,9 @@ static struct variant_data variant_ux500 = {

>         .signal_direction       = true,

>         .pwrreg_clkgate         = true,

>         .busy_detect            = true,

> +       .busy_dpsm_flag         = MCI_DPSM_ST_BUSYMODE,

> +       .busy_detect_flag       = MCI_ST_CARDBUSY,

> +       .busy_detect_mask       = MCI_ST_BUSYENDMASK,

>         .pwrreg_nopower         = true,

>  };

>

> @@ -199,6 +210,9 @@ static struct variant_data variant_ux500v2 = {

>         .signal_direction       = true,

>         .pwrreg_clkgate         = true,

>         .busy_detect            = true,

> +       .busy_dpsm_flag         = MCI_DPSM_ST_BUSYMODE,

> +       .busy_detect_flag       = MCI_ST_CARDBUSY,

> +       .busy_detect_mask       = MCI_ST_BUSYENDMASK,

>         .pwrreg_nopower         = true,

>  };

>

> @@ -220,6 +234,7 @@ static struct variant_data variant_qcom = {

>         .qcom_dml               = true,

>  };

>

> +/* Busy detection for the ST Micro variant */

>  static int mmci_card_busy(struct mmc_host *mmc)

>  {

>         struct mmci_host *host = mmc_priv(mmc);

> @@ -227,7 +242,7 @@ static int mmci_card_busy(struct mmc_host *mmc)

>         int busy = 0;

>

>         spin_lock_irqsave(&host->lock, flags);

> -       if (readl(host->base + MMCISTATUS) & MCI_ST_CARDBUSY)

> +       if (readl(host->base + MMCISTATUS) & host->variant->busy_detect_flag)

>                 busy = 1;

>         spin_unlock_irqrestore(&host->lock, flags);

>

> @@ -294,8 +309,8 @@ static void mmci_write_pwrreg(struct mmci_host *host, u32 pwr)

>   */

>  static void mmci_write_datactrlreg(struct mmci_host *host, u32 datactrl)

>  {

> -       /* Keep ST Micro busy mode if enabled */

> -       datactrl |= host->datactrl_reg & MCI_DPSM_ST_BUSYMODE;

> +       /* Keep busy mode in DPSM if enabled */

> +       datactrl |= host->datactrl_reg & host->variant->busy_dpsm_flag;

>

>         if (host->datactrl_reg != datactrl) {

>                 host->datactrl_reg = datactrl;

> @@ -973,37 +988,66 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,

>              unsigned int status)

>  {

>         void __iomem *base = host->base;

> -       bool sbc, busy_resp;

> +       bool sbc;

>

>         if (!cmd)

>                 return;

>

>         sbc = (cmd == host->mrq->sbc);

> -       busy_resp = host->variant->busy_detect && (cmd->flags & MMC_RSP_BUSY);

>

> -       if (!((status|host->busy_status) & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|

> -               MCI_CMDSENT|MCI_CMDRESPEND)))

> +       /*

> +        * We need to be one of these interrupts to be considered worth

> +        * handling. Note that we tag on any latent IRQs postponed

> +        * due to waiting for busy status.

> +        */

> +       if (!((status|host->busy_status) &

> +             (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND)))

>                 return;

>

> -       /* Check if we need to wait for busy completion. */

> -       if (host->busy_status && (status & MCI_ST_CARDBUSY))

> -               return;

> +       /*

> +        * ST Micro variant: handle busy detection.

> +        */

> +       if (host->variant->busy_detect) {

> +               bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);

>

> -       /* Enable busy completion if needed and supported. */

> -       if (!host->busy_status && busy_resp &&

> -               !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&

> -               (readl(base + MMCISTATUS) & MCI_ST_CARDBUSY)) {

> -               writel(readl(base + MMCIMASK0) | MCI_ST_BUSYEND,

> -                       base + MMCIMASK0);

> -               host->busy_status = status & (MCI_CMDSENT|MCI_CMDRESPEND);

> -               return;

> -       }

> +               /* We are busy with a command, return */

> +               if (host->busy_status &&

> +                   (status & host->variant->busy_detect_flag))

> +                       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.

> +                */

> +               if (!host->busy_status && busy_resp &&

> +                   !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&

> +                   (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {

> +                       /* Unmask the busy 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 busy completion, mask the IRQ and complete the request. */

> -       if (host->busy_status) {

> -               writel(readl(base + MMCIMASK0) & ~MCI_ST_BUSYEND,

> -                       base + MMCIMASK0);

> -               host->busy_status = 0;

> +               /*

> +                * At this point we are not busy with a command, we have

> +                * not recieved a new busy request, mask the busy IRQ and

> +                * fall through to process the IRQ.

> +                */

> +               if (host->busy_status) {

> +                       writel(readl(base + MMCIMASK0) &

> +                              ~host->variant->busy_detect_mask,

> +                              base + MMCIMASK0);

> +                       host->busy_status = 0;

> +               }

>         }

>

>         host->cmd = NULL;

> @@ -1257,9 +1301,11 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)

>                         mmci_data_irq(host, host->data, status);

>                 }

>

> -               /* Don't poll for busy completion in irq context. */

> -               if (host->busy_status)

> -                       status &= ~MCI_ST_CARDBUSY;

> +               /*

> +                * Don't poll for busy completion in irq context.

> +                */

> +               if (host->variant->busy_detect && host->busy_status)

> +                       status &= ~host->variant->busy_detect_flag;

>

>                 ret = 1;

>         } while (status);

> @@ -1612,9 +1658,18 @@ static int mmci_probe(struct amba_device *dev,

>         /* We support these capabilities. */

>         mmc->caps |= MMC_CAP_CMD23;

>

> +       /*

> +        * Enable busy detection.

> +        */

>         if (variant->busy_detect) {

>                 mmci_ops.card_busy = mmci_card_busy;

> -               mmci_write_datactrlreg(host, MCI_DPSM_ST_BUSYMODE);

> +               /*

> +                * Not all variants have a flag to enable busy detection

> +                * in the DPSM, but if they do, set it here.

> +                */

> +               if (variant->busy_dpsm_flag)

> +                       mmci_write_datactrlreg(host,

> +                                              host->variant->busy_dpsm_flag);

>                 mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;

>                 mmc->max_busy_timeout = 0;

>         }

> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h

> index 7cabf270050b..56322c6afba4 100644

> --- a/drivers/mmc/host/mmci.h

> +++ b/drivers/mmc/host/mmci.h

> @@ -174,7 +174,7 @@

>  /* Extended status bits for the ST Micro variants */

>  #define MCI_ST_SDIOITMASK      (1 << 22)

>  #define MCI_ST_CEATAENDMASK    (1 << 23)

> -#define MCI_ST_BUSYEND         (1 << 24)

> +#define MCI_ST_BUSYENDMASK     (1 << 24)

>

>  #define MMCIMASK1              0x040

>  #define MMCIFIFOCNT            0x048

> --

> 2.7.4

>

--
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
diff mbox

Patch

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 6a8ea9c633d4..7f68fa7a961e 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -71,7 +71,12 @@  static unsigned int fmax = 515633;
  * @f_max: maximum clk frequency supported by the controller.
  * @signal_direction: input/out direction of bus signals can be indicated
  * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock
- * @busy_detect: true if busy detection on dat0 is supported
+ * @busy_detect: true if the variant supports busy detection on DAT0.
+ * @busy_dpsm_flag: bitmask enabling busy detection in the DPSM
+ * @busy_detect_flag: bitmask identifying the bit in the MMCISTATUS register
+ * 	 	      indicating that the card is busy
+ * @busy_detect_mask: bitmask identifying the bit in the MMCIMASK0 to mask for
+ * 	  	      getting busy end detection interrupts
  * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
  * @explicit_mclk_control: enable explicit mclk control in driver.
  * @qcom_fifo: enables qcom specific fifo pio read logic.
@@ -98,6 +103,9 @@  struct variant_data {
 	bool			signal_direction;
 	bool			pwrreg_clkgate;
 	bool			busy_detect;
+	u32			busy_dpsm_flag;
+	u32			busy_detect_flag;
+	u32			busy_detect_mask;
 	bool			pwrreg_nopower;
 	bool			explicit_mclk_control;
 	bool			qcom_fifo;
@@ -178,6 +186,9 @@  static struct variant_data variant_ux500 = {
 	.signal_direction	= true,
 	.pwrreg_clkgate		= true,
 	.busy_detect		= true,
+	.busy_dpsm_flag		= MCI_DPSM_ST_BUSYMODE,
+	.busy_detect_flag	= MCI_ST_CARDBUSY,
+	.busy_detect_mask	= MCI_ST_BUSYENDMASK,
 	.pwrreg_nopower		= true,
 };
 
@@ -199,6 +210,9 @@  static struct variant_data variant_ux500v2 = {
 	.signal_direction	= true,
 	.pwrreg_clkgate		= true,
 	.busy_detect		= true,
+	.busy_dpsm_flag		= MCI_DPSM_ST_BUSYMODE,
+	.busy_detect_flag	= MCI_ST_CARDBUSY,
+	.busy_detect_mask	= MCI_ST_BUSYENDMASK,
 	.pwrreg_nopower		= true,
 };
 
@@ -220,6 +234,7 @@  static struct variant_data variant_qcom = {
 	.qcom_dml		= true,
 };
 
+/* Busy detection for the ST Micro variant */
 static int mmci_card_busy(struct mmc_host *mmc)
 {
 	struct mmci_host *host = mmc_priv(mmc);
@@ -227,7 +242,7 @@  static int mmci_card_busy(struct mmc_host *mmc)
 	int busy = 0;
 
 	spin_lock_irqsave(&host->lock, flags);
-	if (readl(host->base + MMCISTATUS) & MCI_ST_CARDBUSY)
+	if (readl(host->base + MMCISTATUS) & host->variant->busy_detect_flag)
 		busy = 1;
 	spin_unlock_irqrestore(&host->lock, flags);
 
@@ -294,8 +309,8 @@  static void mmci_write_pwrreg(struct mmci_host *host, u32 pwr)
  */
 static void mmci_write_datactrlreg(struct mmci_host *host, u32 datactrl)
 {
-	/* Keep ST Micro busy mode if enabled */
-	datactrl |= host->datactrl_reg & MCI_DPSM_ST_BUSYMODE;
+	/* Keep busy mode in DPSM if enabled */
+	datactrl |= host->datactrl_reg & host->variant->busy_dpsm_flag;
 
 	if (host->datactrl_reg != datactrl) {
 		host->datactrl_reg = datactrl;
@@ -973,37 +988,66 @@  mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 	     unsigned int status)
 {
 	void __iomem *base = host->base;
-	bool sbc, busy_resp;
+	bool sbc;
 
 	if (!cmd)
 		return;
 
 	sbc = (cmd == host->mrq->sbc);
-	busy_resp = host->variant->busy_detect && (cmd->flags & MMC_RSP_BUSY);
 
-	if (!((status|host->busy_status) & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|
-		MCI_CMDSENT|MCI_CMDRESPEND)))
+	/*
+	 * We need to be one of these interrupts to be considered worth
+	 * handling. Note that we tag on any latent IRQs postponed
+	 * due to waiting for busy status.
+	 */
+	if (!((status|host->busy_status) &
+	      (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT|MCI_CMDSENT|MCI_CMDRESPEND)))
 		return;
 
-	/* Check if we need to wait for busy completion. */
-	if (host->busy_status && (status & MCI_ST_CARDBUSY))
-		return;
+	/*
+	 * ST Micro variant: handle busy detection.
+	 */
+	if (host->variant->busy_detect) {
+		bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
 
-	/* Enable busy completion if needed and supported. */
-	if (!host->busy_status && busy_resp &&
-		!(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
-		(readl(base + MMCISTATUS) & MCI_ST_CARDBUSY)) {
-		writel(readl(base + MMCIMASK0) | MCI_ST_BUSYEND,
-			base + MMCIMASK0);
-		host->busy_status = status & (MCI_CMDSENT|MCI_CMDRESPEND);
-		return;
-	}
+		/* We are busy with a command, return */
+		if (host->busy_status &&
+		    (status & host->variant->busy_detect_flag))
+			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.
+		 */
+		if (!host->busy_status && busy_resp &&
+		    !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
+		    (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
+			/* Unmask the busy 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 busy completion, mask the IRQ and complete the request. */
-	if (host->busy_status) {
-		writel(readl(base + MMCIMASK0) & ~MCI_ST_BUSYEND,
-			base + MMCIMASK0);
-		host->busy_status = 0;
+		/*
+		 * At this point we are not busy with a command, we have
+		 * not recieved a new busy request, mask the busy IRQ and
+		 * fall through to process the IRQ.
+		 */
+		if (host->busy_status) {
+			writel(readl(base + MMCIMASK0) &
+			       ~host->variant->busy_detect_mask,
+			       base + MMCIMASK0);
+			host->busy_status = 0;
+		}
 	}
 
 	host->cmd = NULL;
@@ -1257,9 +1301,11 @@  static irqreturn_t mmci_irq(int irq, void *dev_id)
 			mmci_data_irq(host, host->data, status);
 		}
 
-		/* Don't poll for busy completion in irq context. */
-		if (host->busy_status)
-			status &= ~MCI_ST_CARDBUSY;
+		/*
+		 * Don't poll for busy completion in irq context.
+		 */
+		if (host->variant->busy_detect && host->busy_status)
+			status &= ~host->variant->busy_detect_flag;
 
 		ret = 1;
 	} while (status);
@@ -1612,9 +1658,18 @@  static int mmci_probe(struct amba_device *dev,
 	/* We support these capabilities. */
 	mmc->caps |= MMC_CAP_CMD23;
 
+	/*
+	 * Enable busy detection.
+	 */
 	if (variant->busy_detect) {
 		mmci_ops.card_busy = mmci_card_busy;
-		mmci_write_datactrlreg(host, MCI_DPSM_ST_BUSYMODE);
+		/*
+		 * Not all variants have a flag to enable busy detection
+		 * in the DPSM, but if they do, set it here.
+		 */
+		if (variant->busy_dpsm_flag)
+			mmci_write_datactrlreg(host,
+					       host->variant->busy_dpsm_flag);
 		mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
 		mmc->max_busy_timeout = 0;
 	}
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 7cabf270050b..56322c6afba4 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -174,7 +174,7 @@ 
 /* Extended status bits for the ST Micro variants */
 #define MCI_ST_SDIOITMASK	(1 << 22)
 #define MCI_ST_CEATAENDMASK	(1 << 23)
-#define MCI_ST_BUSYEND		(1 << 24)
+#define MCI_ST_BUSYENDMASK	(1 << 24)
 
 #define MMCIMASK1		0x040
 #define MMCIFIFOCNT		0x048