mmc: dw_mmc: change to use recommended reset procedure

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook 7.2.13.

v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
 drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/dw_mmc.h |  1 +
 2 files changed, 55 insertions(+), 1 deletion(-)

drivers/mmc/host/dw_mmc.c +1 -0

index 55cd110..aff57e1 100644
@@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 
 static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 {
+	u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
 	/*
 	 * Reseting generates a block interrupt, hence setting
 	 * the scatter-gather pointer to NULL.

@@ -2334,7 +2335,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 		host->sg = NULL;
 	}

drivers/mmc/host/dw_mmc.c +4 -0

+			} while (time_before(jiffies, timeout));
+
+			if (status & SDMMC_STATUS_DMA_REQ) {
+				dev_err(host->dev,

drivers/mmc/host/dw_mmc.c +12 -0

+		return false;
+	}
+
+#ifdef CONFIG_MMC_DW_IDMAC
+	/* It is also recommended that we reset and reprogram idmac */
+	dw_mci_idmac_reset(host);
+#endif
+
+	/* After a CTRL reset we need to have CIU set clock registers  */
+	mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+	return true;
 }
 
 static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)

drivers/mmc/host/dw_mmc.h +1 -1

index 6bf24ab..2505804 100644
@@ -129,6 +129,7 @@
 #define SDMMC_CMD_INDX(n)		((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)		(((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ		BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)	(((m) & 0x7) << 28 | \
 					 ((r) & 0xFFF) << 16 | \
--

1.9.1.423.g4596e3a

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

As I mentioned in previous version, you put all reset stuff in existing fifo_reset function. Although databook mentions ciu_reset case for SBE error, it's not obvious when ciu_reset is needed in other error cases. If you intend to add some robust error handing, it would be better to make another function.

Please check my comments below.

Quoted 32 lines · Sonny Rao

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook 7.2.13.

v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
 drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/dw_mmc.h |  1 +
 2 files changed, 55 insertions(+), 1 deletion(-)

drivers/mmc/host/dw_mmc.c +1 -0

index 55cd110..aff57e1 100644
@@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 
 static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 {
+     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
      /*
       * Reseting generates a block interrupt, hence setting
       * the scatter-gather pointer to NULL.

@@ -2334,7 +2335,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
              host->sg = NULL;
      }
 

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2345

Quoted 9 lines

drivers/mmc/host/dw_mmc.c +8 -1

-     return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+     /*
+      * The recommended method for resetting is to always reset the
+      * controller and the fifo, but differs slightly depending on the mode.
+      * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
+      * mode resets IDMAC at the end.
+      *
+      */
+#ifndef CONFIG_MMC_DW_IDMAC

Is it added for generic DMA?
IDMAC should be considered for dma_reseet as well. Please check databook.

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2353

Quoted 8 lines

drivers/mmc/host/dw_mmc.c +8 -0

+     if (host->use_dma)
+             flags |= SDMMC_CTRL_DMA_RESET;
+#endif
+     if (dw_mci_ctrl_reset(host, flags)) {
+             /*
+              * In all cases we clear the RAWINTS register to clear any
+              * interrupts.
+              */

I think interrupt masking is needed before reset because we will not handle it anymore. And Is there any reason to move interrupt clear here compared with previous version?

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2364

Quoted 11 lines

drivers/mmc/host/dw_mmc.c +11 -0

+             mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+             /* if using dma we wait for dma_req to clear */
+             if (host->use_dma) {
+                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
+                     u32 status;
+                     do {
+                             status = mci_readl(host, STATUS);
+                             if (!(status & SDMMC_STATUS_DMA_REQ))
+                                     break;
+                             cpu_relax();

What did you intend here?
If you intent busy-wait, how about using sleep instead?

Quoted 7 lines

drivers/mmc/host/dw_mmc.c +7 -0

+                     } while (time_before(jiffies, timeout));
+
+                     if (status & SDMMC_STATUS_DMA_REQ) {
+                             dev_err(host->dev,
+                                     "%s: Timeout waiting for dma_req to "
+                                     "clear during reset", __func__);
+                             return false;

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2378

Quoted 7 lines

drivers/mmc/host/dw_mmc.c +7 -0

+                     }
+
+                     /* when using DMA next we reset the fifo again */
+                     dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+             }
+     } else {
+             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);

If ciu_reset is cleared, clk update below is needed?

Thanks,
Seungwon Jeon

Quoted 34 lines

drivers/mmc/host/dw_mmc.c +12 -0

+             return false;
+     }
+
+#ifdef CONFIG_MMC_DW_IDMAC
+     /* It is also recommended that we reset and reprogram idmac */
+     dw_mci_idmac_reset(host);
+#endif
+
+     /* After a CTRL reset we need to have CIU set clock registers  */
+     mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+     return true;
 }
 
 static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)

drivers/mmc/host/dw_mmc.h +1 -1

index 6bf24ab..2505804 100644
@@ -129,6 +129,7 @@
 #define SDMMC_CMD_INDX(n)            ((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)            (((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ         BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)    (((m) & 0x7) << 28 | \
                                       ((r) & 0xFFF) << 16 | \
--

1.9.1.423.g4596e3a

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 2 lines · Seungwon Jeon

As I mentioned in previous version, you put all reset stuff in existing fifo_reset function. Although databook mentions ciu_reset case for SBE error, it's not obvious when ciu_reset is needed

in other error cases.

Quoted 1 line

If you intend to add some robust error handing, it would be better to make another function.

The document I have actually doesn't mention error cases, it describes this procedure as "Controller/DMA/FIFO Reset Usage" so I believe it is saying this is the correct procedure in all cases, and based on our testing it seems to work. I understand the skepticism, as I shared it initially when I saw this, but based on our experience, this is correct and it doesn't need to live in a separate function.

Quoted 7 lines

Please check my comments below.

Quoted 3 lines · Sonny Rao

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook 7.2.13.

Quoted 29 lines

Quoted 29 lines

v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
 drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/dw_mmc.h |  1 +
 2 files changed, 55 insertions(+), 1 deletion(-)

drivers/mmc/host/dw_mmc.c +1 -0

index 55cd110..aff57e1 100644
@@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 
 static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 {
+     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
      /*
       * Reseting generates a block interrupt, hence setting
       * the scatter-gather pointer to NULL.

@@ -2334,7 +2335,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
              host->sg = NULL;
      }
 

Quoted 13 lines

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2345

Quoted 9 lines

drivers/mmc/host/dw_mmc.c +8 -1

-     return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+     /*
+      * The recommended method for resetting is to always reset the
+      * controller and the fifo, but differs slightly depending on the mode.
+      * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
+      * mode resets IDMAC at the end.
+      *
+      */
+#ifndef CONFIG_MMC_DW_IDMAC

Is it added for generic DMA?
IDMAC should be considered for dma_reseet as well. Please check databook.

Yeah it's a little unclear. In the "7.2.13 Controller/DMA/FIFO Reset Usage" part of the document they mention It is set for what they call "generic" DMA, which I think is when there is an external DMA controller, and the part below that it says for "DW-DMA/Non-DW-DMA"
that controller_reset and fifo_reset should be set. I believe this
"DW-DMA" case refers to what is called IDMAC. So, I think it's not required for this case, but I admit I'm not sure why they also say "Non-DW-DMA". If you know of a good way to differentiate the "Generic DMA" case I can implement it. It wasn't clear to me if the driver even supported this "generic" dma case, but it sounds like it might, so I added this code. In practice, on the Exynos Systems we have, which are using IDMAC, the reset procedure seems to work okay without the dma_reset bit set, but I cannot test this "generic dma" case.

The other places in the doc where I see them mention the dma_reset bit are "7.2.21 Transmission and Reception with Internal DMAC (IDMAC)" and the description of the CTRL register, and in "7.1 Software/Hardware Restrictions" where they say it will terminate any pending transfer.

Quoted 11 lines

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2353

Quoted 9 lines

drivers/mmc/host/dw_mmc.c +8 -0

+     if (host->use_dma)
+             flags |= SDMMC_CTRL_DMA_RESET;
+#endif
+     if (dw_mci_ctrl_reset(host, flags)) {
+             /*
+              * In all cases we clear the RAWINTS register to clear any
+              * interrupts.
+              */

I think interrupt masking is needed before reset because we will not handle it anymore. And Is there any reason to move interrupt clear here compared with previous version?

Yeah I moved it to match the description in the document more closely.
The documents mentioned doing the interrupt clear after setting the
reset bits, and before waiting for the dma_req bit in the status register to clear. We've been running it with the interrupt clear below the loop, for a while, and I just tested it with the clear above the wait to make sure it still works properly and I was able to pass the tuning process with some errors, so I believe this works fine too, and more closely matches the description in the doc that I have.

Quoted 12 lines

Quoted 12 lines

drivers/mmc/host/dw_mmc.c +11 -0

+             mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+             /* if using dma we wait for dma_req to clear */
+             if (host->use_dma) {
+                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
+                     u32 status;
+                     do {
+                             status = mci_readl(host, STATUS);
+                             if (!(status & SDMMC_STATUS_DMA_REQ))
+                                     break;
+                             cpu_relax();

Quoted 2 lines

What did you intend here?
If you intent busy-wait, how about using sleep instead?

Yes it is a busy-wait. There is similar code in dw_mci_ctrl_reset, where that function is polling without sleeps, so I was trying to match that. The cpu_relax() is something I saw in other busy-waits in the kernel, but I'm happy to take it out if you'd like.

Quoted 16 lines

Quoted 5 lines

drivers/mmc/host/dw_mmc.c +4 -0

+                     } while (time_before(jiffies, timeout));
+
+                     if (status & SDMMC_STATUS_DMA_REQ) {
+                             dev_err(host->dev,

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2378

Quoted 10 lines

drivers/mmc/host/dw_mmc.c +10 -0

+                                     "%s: Timeout waiting for dma_req to "
+                                     "clear during reset", __func__);
+                             return false;
+                     }
+
+                     /* when using DMA next we reset the fifo again */
+                     dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+             }
+     } else {
+             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);

If ciu_reset is cleared, clk update below is needed?

I'm honestly not sure what happens if the reset bits don't clear. The docs say controller_reset and dma_reset will auto clear after a number of clocks, but fifo_reset will clear "after completion of the reset operation" So in this particular error case I'm not sure if it's possible to recover properly or not and might hang, so I thought it best to just return the error immediately.

Quoted 34 lines

Thanks,
Seungwon Jeon

Quoted 30 lines

drivers/mmc/host/dw_mmc.c +12 -0

+             return false;
+     }
+
+#ifdef CONFIG_MMC_DW_IDMAC
+     /* It is also recommended that we reset and reprogram idmac */
+     dw_mci_idmac_reset(host);
+#endif
+
+     /* After a CTRL reset we need to have CIU set clock registers  */
+     mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+     return true;
 }
 
 static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)

drivers/mmc/host/dw_mmc.h +1 -1

index 6bf24ab..2505804 100644
@@ -129,6 +129,7 @@
 #define SDMMC_CMD_INDX(n)            ((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)            (((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ         BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)    (((m) & 0x7) << 28 | \
                                       ((r) & 0xFFF) << 16 | \
--

1.9.1.423.g4596e3a

Quoted 5 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Sonny Rao

Quoted 2 lines · Seungwon Jeon

As I mentioned in previous version, you put all reset stuff in existing fifo_reset function. Although databook mentions ciu_reset case for SBE error, it's not obvious when ciu_reset is needed

in other error cases.

Quoted 8 lines

Quoted 1 line

If you intend to add some robust error handing, it would be better to make another function.

The document I have actually doesn't mention error cases, it describes this procedure as "Controller/DMA/FIFO Reset Usage" so I believe it is saying this is the correct procedure in all cases, and based on our testing it seems to work. I understand the skepticism, as I shared it initially when I saw this, but based on our experience, this is correct and it doesn't need to live in a separate function.

I agree this active error handling.
But, existing fifo_reset function is focused on fifo reset purely.
I think your change is close to error recovery and it seems overloaded to basic function. So, you suggest renaming function for new sequence.

And look into dw_mci_work_routine_card(). dw_mci_idmac_reset() is redundancy. I expect it can be cleaned.

<Quot>

                                /* Clear down the FIFO */
                                dw_mci_fifo_reset(host);
#ifdef CONFIG_MMC_DW_IDMAC
                                dw_mci_idmac_reset(host);

#endif
</Quot>

Quoted 7 lines

Quoted 7 lines

Please check my comments below.

Quoted 3 lines · Sonny Rao

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook 7.2.13.

Please remove this section number.
No needed. It's old version.

Quoted 57 lines

Quoted 29 lines

Quoted 29 lines

v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
 drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/dw_mmc.h |  1 +
 2 files changed, 55 insertions(+), 1 deletion(-)

drivers/mmc/host/dw_mmc.c +1 -0

index 55cd110..aff57e1 100644
@@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 
 static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 {
+     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
      /*
       * Reseting generates a block interrupt, hence setting
       * the scatter-gather pointer to NULL.

@@ -2334,7 +2335,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
              host->sg = NULL;
      }
 

Quoted 13 lines

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2345

Quoted 9 lines

drivers/mmc/host/dw_mmc.c +8 -1

-     return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+     /*
+      * The recommended method for resetting is to always reset the
+      * controller and the fifo, but differs slightly depending on the mode.
+      * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
+      * mode resets IDMAC at the end.
+      *
+      */
+#ifndef CONFIG_MMC_DW_IDMAC

Is it added for generic DMA?
IDMAC should be considered for dma_reseet as well. Please check databook.

Yeah it's a little unclear. In the "7.2.13 Controller/DMA/FIFO Reset Usage" part of the document they mention It is set for what they call "generic" DMA, which I think is when there is an external DMA controller, and the part below that it says for "DW-DMA/Non-DW-DMA"
that controller_reset and fifo_reset should be set. I believe this
"DW-DMA" case refers to what is called IDMAC. So, I think it's not required for this case, but I admit I'm not sure why they also say "Non-DW-DMA". If you know of a good way to differentiate the "Generic DMA" case I can implement it. It wasn't clear to me if the driver even supported this "generic" dma case, but it sounds like it might, so I added this code. In practice, on the Exynos Systems we have, which are using IDMAC, the reset procedure seems to work okay without the dma_reset bit set, but I cannot test this "generic dma" case.

Quoted 5 lines

The other places in the doc where I see them mention the dma_reset bit are "7.2.21 Transmission and Reception with Internal DMAC (IDMAC)" and the description of the CTRL register, and in "7.1 Software/Hardware Restrictions" where they say it will terminate any pending transfer.

"DW-DMA" means Synopsys's DMA controller not IDMAC. SDMMC_CTRL_DMA_RESET can apply in all type DMA interface.

Quoted 13 lines

Quoted 10 lines

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2353

Quoted 10 lines

drivers/mmc/host/dw_mmc.c +8 -0

+     if (host->use_dma)
+             flags |= SDMMC_CTRL_DMA_RESET;
+#endif
+     if (dw_mci_ctrl_reset(host, flags)) {
+             /*
+              * In all cases we clear the RAWINTS register to clear any
+              * interrupts.
+              */

Quoted 2 lines

I think interrupt masking is needed before reset because we will not handle it anymore. And Is there any reason to move interrupt clear here compared with previous version?

Quoted 8 lines

Yeah I moved it to match the description in the document more closely.
The documents mentioned doing the interrupt clear after setting the
reset bits, and before waiting for the dma_req bit in the status register to clear. We've been running it with the interrupt clear below the loop, for a while, and I just tested it with the clear above the wait to make sure it still works properly and I was able to pass the tuning process with some errors, so I believe this works fine too, and more closely matches the description in the doc that I have.

When I tried ciu_reset, I found some unexpected interrupts occurred. It means that interrupt handler will run to handle extra interrupts. Then, we may need mask the interrupt.
Can you check it for test?

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

drivers/mmc/host/dw_mmc.c +11 -0

+             mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+             /* if using dma we wait for dma_req to clear */
+             if (host->use_dma) {
+                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
+                     u32 status;
+                     do {
+                             status = mci_readl(host, STATUS);
+                             if (!(status & SDMMC_STATUS_DMA_REQ))
+                                     break;
+                             cpu_relax();

Quoted 7 lines

Quoted 2 lines

What did you intend here?
If you intent busy-wait, how about using sleep instead?

Yes it is a busy-wait. There is similar code in dw_mci_ctrl_reset, where that function is polling without sleeps, so I was trying to match that. The cpu_relax() is something I saw in other busy-waits in the kernel, but I'm happy to take it out if you'd like.

In case of ctrl_reset, waiting is during 2 clock.
If this polling status is not long, I'm OK.

Quoted 17 lines

Quoted 16 lines

Quoted 5 lines

drivers/mmc/host/dw_mmc.c +4 -0

+                     } while (time_before(jiffies, timeout));
+
+                     if (status & SDMMC_STATUS_DMA_REQ) {
+                             dev_err(host->dev,

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2378

Quoted 10 lines

drivers/mmc/host/dw_mmc.c +10 -0

+                                     "%s: Timeout waiting for dma_req to "
+                                     "clear during reset", __func__);
+                             return false;
+                     }
+
+                     /* when using DMA next we reset the fifo again */
+                     dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+             }
+     } else {
+             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);

If ciu_reset is cleared, clk update below is needed?

Quoted 6 lines

I'm honestly not sure what happens if the reset bits don't clear. The docs say controller_reset and dma_reset will auto clear after a number of clocks, but fifo_reset will clear "after completion of the reset operation" So in this particular error case I'm not sure if it's possible to recover properly or not and might hang, so I thought it best to just return the error immediately.

Yes.
But at least if ciu_reset is done successfully, it may need clock update sequence again. In addition, printing reset bit[2:0] will be helpful for debug information.

Thanks,
Seungwon Jeon

Quoted 43 lines

Quoted 34 lines

Thanks,
Seungwon Jeon

Quoted 30 lines

drivers/mmc/host/dw_mmc.c +12 -0

+             return false;
+     }
+
+#ifdef CONFIG_MMC_DW_IDMAC
+     /* It is also recommended that we reset and reprogram idmac */
+     dw_mci_idmac_reset(host);
+#endif
+
+     /* After a CTRL reset we need to have CIU set clock registers  */
+     mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+     return true;
 }
 
 static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)

drivers/mmc/host/dw_mmc.h +1 -1

index 6bf24ab..2505804 100644
@@ -129,6 +129,7 @@
 #define SDMMC_CMD_INDX(n)            ((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)            (((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ         BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)    (((m) & 0x7) << 28 | \
                                       ((r) & 0xFFF) << 16 | \
--

1.9.1.423.g4596e3a

Quoted 5 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Seungwon Jeon

Quoted 3 lines · Sonny Rao

Quoted 2 lines · Seungwon Jeon

As I mentioned in previous version, you put all reset stuff in existing fifo_reset function. Although databook mentions ciu_reset case for SBE error, it's not obvious when ciu_reset is

needed

Quoted 9 lines

Quoted 9 lines

in other error cases.

Quoted 1 line

If you intend to add some robust error handing, it would be better to make another function.

The document I have actually doesn't mention error cases, it describes this procedure as "Controller/DMA/FIFO Reset Usage" so I believe it is saying this is the correct procedure in all cases, and based on our testing it seems to work. I understand the skepticism, as I shared it initially when I saw this, but based on our experience, this is correct and it doesn't need to live in a separate function.

Quoted 4 lines

I agree this active error handling.
But, existing fifo_reset function is focused on fifo reset purely.
I think your change is close to error recovery and it seems overloaded to basic function. So, you suggest renaming function for new sequence.

I think the documentation says it should always be done, not just in error recovery. I can rename the function to dw_mci_reset rather than dw_mci_fifo_reset. Is that what you mean? Or do you mean make a new function that is only called in error cases?

Quoted 4 lines

And look into dw_mci_work_routine_card(). dw_mci_idmac_reset() is redundancy. I expect it can be cleaned.

Quoted 8 lines

<Quot>

                                /* Clear down the FIFO */
                                dw_mci_fifo_reset(host);
#ifdef CONFIG_MMC_DW_IDMAC
                                dw_mci_idmac_reset(host);

#endif
</Quot>

Ok, I'll remove that extra reset, thanks for catching.

Quoted 10 lines

Quoted 7 lines

Quoted 7 lines

Please check my comments below.

Quoted 3 lines · Sonny Rao

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook 7.2.13.

Please remove this section number.
No needed. It's old version.

Ok

Quoted 62 lines

Quoted 62 lines

Quoted 29 lines

Quoted 29 lines

v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
 drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/dw_mmc.h |  1 +
 2 files changed, 55 insertions(+), 1 deletion(-)

drivers/mmc/host/dw_mmc.c +1 -0

index 55cd110..aff57e1 100644
@@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 
 static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 {
+     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
      /*
       * Reseting generates a block interrupt, hence setting
       * the scatter-gather pointer to NULL.

@@ -2334,7 +2335,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
              host->sg = NULL;
      }
 

Quoted 13 lines

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2345

Quoted 9 lines

drivers/mmc/host/dw_mmc.c +8 -1

-     return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+     /*
+      * The recommended method for resetting is to always reset the
+      * controller and the fifo, but differs slightly depending on the mode.
+      * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
+      * mode resets IDMAC at the end.
+      *
+      */
+#ifndef CONFIG_MMC_DW_IDMAC

Is it added for generic DMA?
IDMAC should be considered for dma_reseet as well. Please check databook.

Yeah it's a little unclear. In the "7.2.13 Controller/DMA/FIFO Reset Usage" part of the document they mention It is set for what they call "generic" DMA, which I think is when there is an external DMA controller, and the part below that it says for "DW-DMA/Non-DW-DMA"
that controller_reset and fifo_reset should be set. I believe this
"DW-DMA" case refers to what is called IDMAC. So, I think it's not required for this case, but I admit I'm not sure why they also say "Non-DW-DMA". If you know of a good way to differentiate the "Generic DMA" case I can implement it. It wasn't clear to me if the driver even supported this "generic" dma case, but it sounds like it might, so I added this code. In practice, on the Exynos Systems we have, which are using IDMAC, the reset procedure seems to work okay without the dma_reset bit set, but I cannot test this "generic dma" case.

The other places in the doc where I see them mention the dma_reset bit are "7.2.21 Transmission and Reception with Internal DMAC (IDMAC)" and the description of the CTRL register, and in "7.1 Software/Hardware Restrictions" where they say it will terminate any pending transfer.

Quoted 2 lines

"DW-DMA" means Synopsys's DMA controller not IDMAC. SDMMC_CTRL_DMA_RESET can apply in all type DMA interface.

So, do you think I should always set that bit if we're using DMA?

Quoted 22 lines

Quoted 14 lines

Quoted 11 lines

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2353

Quoted 11 lines

drivers/mmc/host/dw_mmc.c +8 -0

+     if (host->use_dma)
+             flags |= SDMMC_CTRL_DMA_RESET;
+#endif
+     if (dw_mci_ctrl_reset(host, flags)) {
+             /*
+              * In all cases we clear the RAWINTS register to clear any
+              * interrupts.
+              */

Quoted 2 lines

I think interrupt masking is needed before reset because we will not handle it anymore. And Is there any reason to move interrupt clear here compared with previous version?

Quoted 8 lines

Yeah I moved it to match the description in the document more closely.
The documents mentioned doing the interrupt clear after setting the
reset bits, and before waiting for the dma_req bit in the status register to clear. We've been running it with the interrupt clear below the loop, for a while, and I just tested it with the clear above the wait to make sure it still works properly and I was able to pass the tuning process with some errors, so I believe this works fine too, and more closely matches the description in the doc that I have.

Quoted 4 lines

When I tried ciu_reset, I found some unexpected interrupts occurred. It means that interrupt handler will run to handle extra interrupts. Then, we may need mask the interrupt.
Can you check it for test?

You're right. When I have the controller reset bit set, I see one interrupt coming in shortly after we write the reset bits, which has status bits of 0x180. This has Response timeout (RTO) and Data CRC error set. It sounds like some interrupt ocurring may be expected since they say to clear RINTSTS as part of the procedure though.

Quoted 48 lines

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

drivers/mmc/host/dw_mmc.c +11 -0

+             mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+             /* if using dma we wait for dma_req to clear */
+             if (host->use_dma) {
+                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
+                     u32 status;
+                     do {
+                             status = mci_readl(host, STATUS);
+                             if (!(status & SDMMC_STATUS_DMA_REQ))
+                                     break;
+                             cpu_relax();

Quoted 7 lines

Quoted 2 lines

What did you intend here?
If you intent busy-wait, how about using sleep instead?

Yes it is a busy-wait. There is similar code in dw_mci_ctrl_reset, where that function is polling without sleeps, so I was trying to match that. The cpu_relax() is something I saw in other busy-waits in the kernel, but I'm happy to take it out if you'd like.

In case of ctrl_reset, waiting is during 2 clock.
If this polling status is not long, I'm OK.

Quoted 17 lines

Quoted 16 lines

Quoted 5 lines

drivers/mmc/host/dw_mmc.c +4 -0

+                     } while (time_before(jiffies, timeout));
+
+                     if (status & SDMMC_STATUS_DMA_REQ) {
+                             dev_err(host->dev,

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2378

Quoted 10 lines

drivers/mmc/host/dw_mmc.c +10 -0

+                                     "%s: Timeout waiting for dma_req to "
+                                     "clear during reset", __func__);
+                             return false;
+                     }
+
+                     /* when using DMA next we reset the fifo again */
+                     dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+             }
+     } else {
+             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);

If ciu_reset is cleared, clk update below is needed?

Quoted 6 lines

I'm honestly not sure what happens if the reset bits don't clear. The docs say controller_reset and dma_reset will auto clear after a number of clocks, but fifo_reset will clear "after completion of the reset operation" So in this particular error case I'm not sure if it's possible to recover properly or not and might hang, so I thought it best to just return the error immediately.

Quoted 3 lines

Yes.
But at least if ciu_reset is done successfully, it may need clock update sequence again. In addition, printing reset bit[2:0] will be helpful for debug information.

The dw_mci_ctrl_reset() function would run this code if some of the bits didn't clear:

        dev_err(host->dev,
                "Timeout resetting block (ctrl reset %#x)\n",
                ctrl & reset);

Does that give you what you're looking for?

Quoted 49 lines

Thanks,
Seungwon Jeon

Quoted 43 lines

Quoted 34 lines

Thanks,
Seungwon Jeon

Quoted 30 lines

drivers/mmc/host/dw_mmc.c +12 -0

+             return false;
+     }
+
+#ifdef CONFIG_MMC_DW_IDMAC
+     /* It is also recommended that we reset and reprogram idmac */
+     dw_mci_idmac_reset(host);
+#endif
+
+     /* After a CTRL reset we need to have CIU set clock registers  */
+     mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+     return true;
 }
 
 static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)

drivers/mmc/host/dw_mmc.h +1 -1

index 6bf24ab..2505804 100644
@@ -129,6 +129,7 @@
 #define SDMMC_CMD_INDX(n)            ((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)            (((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ         BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)    (((m) & 0x7) << 28 | \
                                       ((r) & 0xFFF) << 16 | \
--

1.9.1.423.g4596e3a

Quoted 5 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 20 lines · Sonny Rao

Quoted 4 lines · Seungwon Jeon

Quoted 3 lines · Sonny Rao

Quoted 2 lines · Seungwon Jeon

As I mentioned in previous version, you put all reset stuff in existing fifo_reset function. Although databook mentions ciu_reset case for SBE error, it's not obvious when ciu_reset is

needed

Quoted 13 lines

Quoted 9 lines

in other error cases.

Quoted 1 line

If you intend to add some robust error handing, it would be better to make another function.

The document I have actually doesn't mention error cases, it describes this procedure as "Controller/DMA/FIFO Reset Usage" so I believe it is saying this is the correct procedure in all cases, and based on our testing it seems to work. I understand the skepticism, as I shared it initially when I saw this, but based on our experience, this is correct and it doesn't need to live in a separate function.

I agree this active error handling.
But, existing fifo_reset function is focused on fifo reset purely.
I think your change is close to error recovery and it seems overloaded to basic function. So, you suggest renaming function for new sequence.

Quoted 4 lines

I think the documentation says it should always be done, not just in error recovery. I can rename the function to dw_mci_reset rather than dw_mci_fifo_reset. Is that what you mean? Or do you mean make a new function that is only called in error cases?

Both are okay.

Quoted 90 lines

Quoted 4 lines

And look into dw_mci_work_routine_card(). dw_mci_idmac_reset() is redundancy. I expect it can be cleaned.

Quoted 8 lines

<Quot>

                                /* Clear down the FIFO */
                                dw_mci_fifo_reset(host);
#ifdef CONFIG_MMC_DW_IDMAC
                                dw_mci_idmac_reset(host);

#endif
</Quot>

Ok, I'll remove that extra reset, thanks for catching.

Quoted 10 lines

Quoted 7 lines

Quoted 7 lines

Please check my comments below.

Quoted 3 lines · Sonny Rao

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook 7.2.13.

Please remove this section number.
No needed. It's old version.

Ok

Quoted 62 lines

Quoted 62 lines

Quoted 29 lines

Quoted 29 lines

v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
 drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/dw_mmc.h |  1 +
 2 files changed, 55 insertions(+), 1 deletion(-)

drivers/mmc/host/dw_mmc.c +1 -0

index 55cd110..aff57e1 100644
@@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 
 static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 {
+     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
      /*
       * Reseting generates a block interrupt, hence setting
       * the scatter-gather pointer to NULL.

@@ -2334,7 +2335,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
              host->sg = NULL;
      }
 

Quoted 13 lines

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2345

Quoted 9 lines

drivers/mmc/host/dw_mmc.c +8 -1

-     return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+     /*
+      * The recommended method for resetting is to always reset the
+      * controller and the fifo, but differs slightly depending on the mode.
+      * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
+      * mode resets IDMAC at the end.
+      *
+      */
+#ifndef CONFIG_MMC_DW_IDMAC

Is it added for generic DMA?
IDMAC should be considered for dma_reseet as well. Please check databook.

Yeah it's a little unclear. In the "7.2.13 Controller/DMA/FIFO Reset Usage" part of the document they mention It is set for what they call "generic" DMA, which I think is when there is an external DMA controller, and the part below that it says for "DW-DMA/Non-DW-DMA"
that controller_reset and fifo_reset should be set. I believe this
"DW-DMA" case refers to what is called IDMAC. So, I think it's not required for this case, but I admit I'm not sure why they also say "Non-DW-DMA". If you know of a good way to differentiate the "Generic DMA" case I can implement it. It wasn't clear to me if the driver even supported this "generic" dma case, but it sounds like it might, so I added this code. In practice, on the Exynos Systems we have, which are using IDMAC, the reset procedure seems to work okay without the dma_reset bit set, but I cannot test this "generic dma" case.

The other places in the doc where I see them mention the dma_reset bit are "7.2.21 Transmission and Reception with Internal DMAC (IDMAC)" and the description of the CTRL register, and in "7.1 Software/Hardware Restrictions" where they say it will terminate any pending transfer.

Quoted 4 lines

Quoted 2 lines

"DW-DMA" means Synopsys's DMA controller not IDMAC. SDMMC_CTRL_DMA_RESET can apply in all type DMA interface.

So, do you think I should always set that bit if we're using DMA?

Yes. Although you used it for general dma at first, IDMAC also needs it.

Quoted 27 lines

Quoted 22 lines

Quoted 14 lines

Quoted 11 lines

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2353

Quoted 11 lines

drivers/mmc/host/dw_mmc.c +8 -0

+     if (host->use_dma)
+             flags |= SDMMC_CTRL_DMA_RESET;
+#endif
+     if (dw_mci_ctrl_reset(host, flags)) {
+             /*
+              * In all cases we clear the RAWINTS register to clear any
+              * interrupts.
+              */

Quoted 2 lines

I think interrupt masking is needed before reset because we will not handle it anymore. And Is there any reason to move interrupt clear here compared with previous version?

Quoted 8 lines

Yeah I moved it to match the description in the document more closely.
The documents mentioned doing the interrupt clear after setting the
reset bits, and before waiting for the dma_req bit in the status register to clear. We've been running it with the interrupt clear below the loop, for a while, and I just tested it with the clear above the wait to make sure it still works properly and I was able to pass the tuning process with some errors, so I believe this works fine too, and more closely matches the description in the doc that I have.

Quoted 4 lines

When I tried ciu_reset, I found some unexpected interrupts occurred. It means that interrupt handler will run to handle extra interrupts. Then, we may need mask the interrupt.
Can you check it for test?

Quoted 5 lines

You're right. When I have the controller reset bit set, I see one interrupt coming in shortly after we write the reset bits, which has status bits of 0x180. This has Response timeout (RTO) and Data CRC error set. It sounds like some interrupt ocurring may be expected since they say to clear RINTSTS as part of the procedure though.

Yes, it needs to clear RINTSTS.
While this interrupt occurs, interrupt handler is called and thus tasklet function is also called. Can we handle this unexpected interrupt properly?
To prevent these calling, we may need interrupt mask before resetting.

Quoted 52 lines

Quoted 48 lines

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

drivers/mmc/host/dw_mmc.c +11 -0

+             mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+             /* if using dma we wait for dma_req to clear */
+             if (host->use_dma) {
+                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
+                     u32 status;
+                     do {
+                             status = mci_readl(host, STATUS);
+                             if (!(status & SDMMC_STATUS_DMA_REQ))
+                                     break;
+                             cpu_relax();

Quoted 7 lines

Quoted 2 lines

What did you intend here?
If you intent busy-wait, how about using sleep instead?

Yes it is a busy-wait. There is similar code in dw_mci_ctrl_reset, where that function is polling without sleeps, so I was trying to match that. The cpu_relax() is something I saw in other busy-waits in the kernel, but I'm happy to take it out if you'd like.

In case of ctrl_reset, waiting is during 2 clock.
If this polling status is not long, I'm OK.

Quoted 17 lines

Quoted 16 lines

Quoted 5 lines

drivers/mmc/host/dw_mmc.c +4 -0

+                     } while (time_before(jiffies, timeout));
+
+                     if (status & SDMMC_STATUS_DMA_REQ) {
+                             dev_err(host->dev,

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2378

Quoted 10 lines

drivers/mmc/host/dw_mmc.c +10 -0

+                                     "%s: Timeout waiting for dma_req to "
+                                     "clear during reset", __func__);
+                             return false;
+                     }
+
+                     /* when using DMA next we reset the fifo again */
+                     dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+             }
+     } else {
+             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);

If ciu_reset is cleared, clk update below is needed?

Quoted 6 lines

I'm honestly not sure what happens if the reset bits don't clear. The docs say controller_reset and dma_reset will auto clear after a number of clocks, but fifo_reset will clear "after completion of the reset operation" So in this particular error case I'm not sure if it's possible to recover properly or not and might hang, so I thought it best to just return the error immediately.

Quoted 3 lines

Yes.
But at least if ciu_reset is done successfully, it may need clock update sequence again. In addition, printing reset bit[2:0] will be helpful for debug information.

Quoted 8 lines

The dw_mci_ctrl_reset() function would run this code if some of the bits didn't clear:

        dev_err(host->dev,
                "Timeout resetting block (ctrl reset %#x)\n",
                ctrl & reset);

Does that give you what you're looking for?

Ah, it was there.

Thanks,
Seungwon Jeon

Quoted 53 lines

Quoted 49 lines

Thanks,
Seungwon Jeon

Quoted 43 lines

Quoted 34 lines

Thanks,
Seungwon Jeon

Quoted 30 lines

drivers/mmc/host/dw_mmc.c +12 -0

+             return false;
+     }
+
+#ifdef CONFIG_MMC_DW_IDMAC
+     /* It is also recommended that we reset and reprogram idmac */
+     dw_mci_idmac_reset(host);
+#endif
+
+     /* After a CTRL reset we need to have CIU set clock registers  */
+     mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+     return true;
 }
 
 static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)

drivers/mmc/host/dw_mmc.h +1 -1

index 6bf24ab..2505804 100644
@@ -129,6 +129,7 @@
 #define SDMMC_CMD_INDX(n)            ((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)            (((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ         BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)    (((m) & 0x7) << 28 | \
                                       ((r) & 0xFFF) << 16 | \
--

1.9.1.423.g4596e3a

Quoted 5 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook.

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)
v3: rename fifo reset function, and change callers
use this combined reset function in dw_mci_resume()
just one caller left (probe), so get rid of dw_mci_ctrl_all_reset() use DMA reset bit for all systems which use DMA
remove extra IDMAC reset in dw_mci_work_routine_card() do CIU clock update in error path, if CIU reset cleared

 drivers/mmc/host/dw_mmc.c | 85 +++++++++++++++++++++++++++++++++++------------
 drivers/mmc/host/dw_mmc.h |  1 +
 2 files changed, 64 insertions(+), 22 deletions(-)

drivers/mmc/host/dw_mmc.c +3 -4

index 55cd110..988492c 100644
@@ -111,8 +111,7 @@ static const u8 tuning_blk_pattern_8bit[] = {
 	0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
 };
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host);
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host);
+static inline bool dw_mci_reset(struct dw_mci *host);
 
 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)

@@ -1254,7 +1253,7 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
 		 * After an error, there may be data lingering
 		 * in the FIFO
 		 */
-		dw_mci_fifo_reset(host);
+		dw_mci_reset(host);
 	} else {
 		data->bytes_xfered = data->blocks * data->blksz;
 		data->error = 0;

@@ -1371,7 +1370,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
 
 			/* CMD error in data command */
 			if (mrq->cmd->error && mrq->data)
-				dw_mci_fifo_reset(host);
+				dw_mci_reset(host);
 
 			host->cmd = NULL;
 			host->data = NULL;

drivers/mmc/host/dw_mmc.c +30 -15

-				dw_mci_fifo_reset(host);
-#ifdef CONFIG_MMC_DW_IDMAC
-				dw_mci_idmac_reset(host);
-#endif
-
-			}
+				dw_mci_reset(host);
 
 			spin_unlock_bh(&host->lock);
 

@@ -2323,8 +2317,11 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 	return false;
 }
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host)
+static inline bool dw_mci_reset(struct dw_mci *host)
 {
+	u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
+	bool ret = false;
+
 	/*
 	 * Reseting generates a block interrupt, hence setting
 	 * the scatter-gather pointer to NULL.

@@ -2334,15 +2331,57 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 		host->sg = NULL;
 	}
 
-	return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
-}
+	if (host->use_dma)
+		flags |= SDMMC_CTRL_DMA_RESET;
 
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
-{
-	return dw_mci_ctrl_reset(host,
-				 SDMMC_CTRL_FIFO_RESET |
-				 SDMMC_CTRL_RESET |
-				 SDMMC_CTRL_DMA_RESET);
+	if (dw_mci_ctrl_reset(host, flags)) {
+		/*
+		 * In all cases we clear the RAWINTS register to clear any
+		 * interrupts.
+		 */
+		mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+		/* if using dma we wait for dma_req to clear */
+		if (host->use_dma) {
+			unsigned long timeout = jiffies + msecs_to_jiffies(500);
+			u32 status;
+			do {
+				status = mci_readl(host, STATUS);
+				if (!(status & SDMMC_STATUS_DMA_REQ))
+					break;
+				cpu_relax();
+			} while (time_before(jiffies, timeout));
+
+			if (status & SDMMC_STATUS_DMA_REQ) {
+				dev_err(host->dev,
+					"%s: Timeout waiting for dma_req to "
+					"clear during reset", __func__);
+				goto ciu_out;

drivers/mmc/host/dw_mmc.c +16 -0

+		if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET))
+			goto ciu_out;
+	}
+
+	if (IS_ENABLED(CONFIG_MMC_DW_IDMAC))
+		/* It is also recommended that we reset and reprogram idmac */
+		dw_mci_idmac_reset(host);
+
+	ret = true;
+
+ciu_out:
+	/* After a CTRL reset we need to have CIU set clock registers  */
+	mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+	return ret;
 }
 
 #ifdef CONFIG_OF

@@ -2432,6 +2471,7 @@ int dw_mci_probe(struct dw_mci *host)
 	const struct dw_mci_drv_data *drv_data = host->drv_data;
 	int width, i, ret = 0;
 	u32 fifo_size;
+	u32 flags;
 	int init_slots = 0;
 
 	if (!host->pdata) {

drivers/mmc/host/dw_mmc.c +2 -1

+	if (!dw_mci_ctrl_reset(host, flags))
 		return -ENODEV;
 
 	host->dma_ops = host->pdata->dma_ops;

@@ -2744,7 +2785,7 @@ int dw_mci_resume(struct dw_mci *host)
 		}
 	}
 
-	if (!dw_mci_ctrl_all_reset(host)) {
+	if (!dw_mci_reset(host)) {
 		ret = -ENODEV;
 		return ret;
 	}

drivers/mmc/host/dw_mmc.h +1 -0

index 6bf24ab..2505804 100644
@@ -129,6 +129,7 @@
 #define SDMMC_CMD_INDX(n)		((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)		(((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ		BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)	(((m) & 0x7) << 28 | \
 					 ((r) & 0xFFF) << 16 | \

Hi, Sonny.

Quoted 56 lines · Sonny Rao

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook.

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)
v3: rename fifo reset function, and change callers
use this combined reset function in dw_mci_resume()
just one caller left (probe), so get rid of dw_mci_ctrl_all_reset() use DMA reset bit for all systems which use DMA
remove extra IDMAC reset in dw_mci_work_routine_card() do CIU clock update in error path, if CIU reset cleared

 drivers/mmc/host/dw_mmc.c | 85 +++++++++++++++++++++++++++++++++++------------
 drivers/mmc/host/dw_mmc.h |  1 +
 2 files changed, 64 insertions(+), 22 deletions(-)

drivers/mmc/host/dw_mmc.c +3 -4

index 55cd110..988492c 100644
@@ -111,8 +111,7 @@ static const u8 tuning_blk_pattern_8bit[] = {
      0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
 };
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host);
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host);
+static inline bool dw_mci_reset(struct dw_mci *host);
 
 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)

@@ -1254,7 +1253,7 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
               * After an error, there may be data lingering
               * in the FIFO
               */
-             dw_mci_fifo_reset(host);
+             dw_mci_reset(host);
      } else {
              data->bytes_xfered = data->blocks * data->blksz;
              data->error = 0;

@@ -1371,7 +1370,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
 
                      /* CMD error in data command */
                      if (mrq->cmd->error && mrq->data)
-                             dw_mci_fifo_reset(host);
+                             dw_mci_reset(host);
 
                      host->cmd = NULL;
                      host->data = NULL;

@@ -1982,14 +1981,9 @@ static void dw_mci_work_routine_card(struct work_struct *work)
                      }
 

drivers/mmc/host/dw_mmc.c static void dw_mci_work_routine_card(struct work_struct *work) · line 1985

Quoted 4 lines

drivers/mmc/host/dw_mmc.c +1 -1

                      /* Power down slot */
-                     if (present == 0) {
+                     if (present == 0)
                              /* Clear down the FIFO */

Didn't Need to change the Comment?

Quoted 61 lines

drivers/mmc/host/dw_mmc.c +30 -15

-                             dw_mci_fifo_reset(host);
-#ifdef CONFIG_MMC_DW_IDMAC
-                             dw_mci_idmac_reset(host);
-#endif
-
-                     }
+                             dw_mci_reset(host);
 
                      spin_unlock_bh(&host->lock);
 

@@ -2323,8 +2317,11 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
      return false;
 }
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host)
+static inline bool dw_mci_reset(struct dw_mci *host)
 {
+     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
+     bool ret = false;
+
      /*
       * Reseting generates a block interrupt, hence setting
       * the scatter-gather pointer to NULL.

@@ -2334,15 +2331,57 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
              host->sg = NULL;
      }
 
-     return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
-}
+     if (host->use_dma)
+             flags |= SDMMC_CTRL_DMA_RESET;
 
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
-{
-     return dw_mci_ctrl_reset(host,
-                              SDMMC_CTRL_FIFO_RESET |
-                              SDMMC_CTRL_RESET |
-                              SDMMC_CTRL_DMA_RESET);
+     if (dw_mci_ctrl_reset(host, flags)) {
+             /*
+              * In all cases we clear the RAWINTS register to clear any
+              * interrupts.
+              */
+             mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+             /* if using dma we wait for dma_req to clear */
+             if (host->use_dma) {
+                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
+                     u32 status;
+                     do {
+                             status = mci_readl(host, STATUS);
+                             if (!(status & SDMMC_STATUS_DMA_REQ))
+                                     break;
+                             cpu_relax();
+                     } while (time_before(jiffies, timeout));
+
+                     if (status & SDMMC_STATUS_DMA_REQ) {
+                             dev_err(host->dev,
+                                     "%s: Timeout waiting for dma_req to "
+                                     "clear during reset", __func__);
+                             goto ciu_out;

drivers/mmc/host/dw_mmc.c static inline bool dw_mci_fifo_reset(struct dw_mci *host) · line 2369

Quoted 10 lines

drivers/mmc/host/dw_mmc.c +10 -0

+                     }
+
+                     /* when using DMA next we reset the fifo again */
+                     if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET))
+                             goto ciu_out;
+             }
+     } else {
+             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
+
+             /* if the controller reset bit did clear, then set clock regs */

I have some confusion at this point. you added the above error message "Reset bits didn't clear". But this comment is "if the controller reset bit did clear..".

I think that dev_err message would be better to locate into "if (!(mci_read(host, CTRL) & ...)".

Quoted 29 lines

drivers/mmc/host/dw_mmc.c +16 -0

+             if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET))
+                     goto ciu_out;
+     }
+
+     if (IS_ENABLED(CONFIG_MMC_DW_IDMAC))
+             /* It is also recommended that we reset and reprogram idmac */
+             dw_mci_idmac_reset(host);
+
+     ret = true;
+
+ciu_out:
+     /* After a CTRL reset we need to have CIU set clock registers  */
+     mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+     return ret;
 }
 
 #ifdef CONFIG_OF

@@ -2432,6 +2471,7 @@ int dw_mci_probe(struct dw_mci *host)
      const struct dw_mci_drv_data *drv_data = host->drv_data;
      int width, i, ret = 0;
      u32 fifo_size;
+     u32 flags;
      int init_slots = 0;
 
      if (!host->pdata) {

@@ -2555,7 +2595,8 @@ int dw_mci_probe(struct dw_mci *host)
      }
 

drivers/mmc/host/dw_mmc.c int dw_mci_probe(struct dw_mci *host) · line 2598

Quoted 3 lines

drivers/mmc/host/dw_mmc.c +1 -1

      /* Reset all blocks */
-     if (!dw_mci_ctrl_all_reset(host))
+     flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET | SDMMC_CTRL_DMA_RESET;

how about adding the SDMMC_CTRL_ALL_RESET macro into header file? #define SDMMC_CTRL_ALL_RESET (.....)

Quoted 26 lines

drivers/mmc/host/dw_mmc.c +2 -1

+     if (!dw_mci_ctrl_reset(host, flags))
              return -ENODEV;
 
      host->dma_ops = host->pdata->dma_ops;

@@ -2744,7 +2785,7 @@ int dw_mci_resume(struct dw_mci *host)
              }
      }
 
-     if (!dw_mci_ctrl_all_reset(host)) {
+     if (!dw_mci_reset(host)) {
              ret = -ENODEV;
              return ret;
      }

drivers/mmc/host/dw_mmc.h +1 -0

index 6bf24ab..2505804 100644
@@ -129,6 +129,7 @@
 #define SDMMC_CMD_INDX(n)            ((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)            (((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ         BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)    (((m) & 0x7) << 28 | \
                                       ((r) & 0xFFF) << 16 | \

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook.

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)
v3: rename fifo reset function, and change callers
use this combined reset function in dw_mci_resume()
just one caller left (probe), so get rid of dw_mci_ctrl_all_reset() use DMA reset bit for all systems which use DMA
remove extra IDMAC reset in dw_mci_work_routine_card() do CIU clock update in error path, if CIU reset cleared
v4: remove comment about FIFO reset in dw_mci_work_routine_card()
move down error message when control reset clears but others don't
and clarify the error stating that we will still update clocks
make flags for all reset bits a macro

 drivers/mmc/host/dw_mmc.c | 86 ++++++++++++++++++++++++++++++++++-------------
 drivers/mmc/host/dw_mmc.h |  5 +++
 2 files changed, 68 insertions(+), 23 deletions(-)

drivers/mmc/host/dw_mmc.c +62 -22

index 55cd110..1d6d984 100644
@@ -111,8 +111,7 @@ static const u8 tuning_blk_pattern_8bit[] = {
 	0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
 };
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host);
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host);
+static inline bool dw_mci_reset(struct dw_mci *host);
 
 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)

@@ -1254,7 +1253,7 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
 		 * After an error, there may be data lingering
 		 * in the FIFO
 		 */
-		dw_mci_fifo_reset(host);
+		dw_mci_reset(host);
 	} else {
 		data->bytes_xfered = data->blocks * data->blksz;
 		data->error = 0;

@@ -1371,7 +1370,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
 
 			/* CMD error in data command */
 			if (mrq->cmd->error && mrq->data)
-				dw_mci_fifo_reset(host);
+				dw_mci_reset(host);
 
 			host->cmd = NULL;
 			host->data = NULL;

@@ -1982,14 +1981,8 @@ static void dw_mci_work_routine_card(struct work_struct *work)
 			}
 
 			/* Power down slot */
-			if (present == 0) {
-				/* Clear down the FIFO */
-				dw_mci_fifo_reset(host);
-#ifdef CONFIG_MMC_DW_IDMAC
-				dw_mci_idmac_reset(host);
-#endif
-
-			}
+			if (present == 0)
+				dw_mci_reset(host);
 
 			spin_unlock_bh(&host->lock);
 

@@ -2323,8 +2316,11 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 	return false;
 }
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host)
+static inline bool dw_mci_reset(struct dw_mci *host)
 {
+	u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
+	bool ret = false;
+
 	/*
 	 * Reseting generates a block interrupt, hence setting
 	 * the scatter-gather pointer to NULL.

@@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 		host->sg = NULL;
 	}
 
-	return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
-}
+	if (host->use_dma)
+		flags |= SDMMC_CTRL_DMA_RESET;
 
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
-{
-	return dw_mci_ctrl_reset(host,
-				 SDMMC_CTRL_FIFO_RESET |
-				 SDMMC_CTRL_RESET |
-				 SDMMC_CTRL_DMA_RESET);
+	if (dw_mci_ctrl_reset(host, flags)) {
+		/*
+		 * In all cases we clear the RAWINTS register to clear any
+		 * interrupts.
+		 */
+		mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+		/* if using dma we wait for dma_req to clear */
+		if (host->use_dma) {
+			unsigned long timeout = jiffies + msecs_to_jiffies(500);
+			u32 status;
+			do {
+				status = mci_readl(host, STATUS);
+				if (!(status & SDMMC_STATUS_DMA_REQ))
+					break;
+				cpu_relax();
+			} while (time_before(jiffies, timeout));
+
+			if (status & SDMMC_STATUS_DMA_REQ) {
+				dev_err(host->dev,
+					"%s: Timeout waiting for dma_req to "
+					"clear during reset", __func__);
+				goto ciu_out;
+			}
+
+			/* when using DMA next we reset the fifo again */
+			if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET))
+				goto ciu_out;
+		}
+	} else {
+		/* if the controller reset bit did clear, then set clock regs */
+		if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) {
+			dev_err(host->dev, "%s: fifo/dma reset bits didn't "
+				"clear but ciu was reset, doing clock update.",
+				__func__);
+			goto ciu_out;
+		}
+	}
+
+	if (IS_ENABLED(CONFIG_MMC_DW_IDMAC))
+		/* It is also recommended that we reset and reprogram idmac */
+		dw_mci_idmac_reset(host);
+
+	ret = true;
+
+ciu_out:
+	/* After a CTRL reset we need to have CIU set clock registers  */
+	mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+	return ret;
 }
 
 #ifdef CONFIG_OF

@@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host)
 	}
 
 	/* Reset all blocks */
-	if (!dw_mci_ctrl_all_reset(host))
+	if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS))
 		return -ENODEV;
 
 	host->dma_ops = host->pdata->dma_ops;

Hi Sonny,

I have missed this patch.

You finally choose to take extra interrupt handling. If it is not harm, it's fine.

Please check one thing below.

Quoted 163 lines · Sonny Rao

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook.

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)
v3: rename fifo reset function, and change callers
use this combined reset function in dw_mci_resume()
just one caller left (probe), so get rid of dw_mci_ctrl_all_reset() use DMA reset bit for all systems which use DMA
remove extra IDMAC reset in dw_mci_work_routine_card() do CIU clock update in error path, if CIU reset cleared
v4: remove comment about FIFO reset in dw_mci_work_routine_card()
move down error message when control reset clears but others don't
and clarify the error stating that we will still update clocks
make flags for all reset bits a macro

 drivers/mmc/host/dw_mmc.c | 86 ++++++++++++++++++++++++++++++++++-------------
 drivers/mmc/host/dw_mmc.h |  5 +++
 2 files changed, 68 insertions(+), 23 deletions(-)

drivers/mmc/host/dw_mmc.c +62 -22

index 55cd110..1d6d984 100644
@@ -111,8 +111,7 @@ static const u8 tuning_blk_pattern_8bit[] = {
      0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
 };
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host);
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host);
+static inline bool dw_mci_reset(struct dw_mci *host);
 
 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)

@@ -1254,7 +1253,7 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
               * After an error, there may be data lingering
               * in the FIFO
               */
-             dw_mci_fifo_reset(host);
+             dw_mci_reset(host);
      } else {
              data->bytes_xfered = data->blocks * data->blksz;
              data->error = 0;

@@ -1371,7 +1370,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
 
                      /* CMD error in data command */
                      if (mrq->cmd->error && mrq->data)
-                             dw_mci_fifo_reset(host);
+                             dw_mci_reset(host);
 
                      host->cmd = NULL;
                      host->data = NULL;

@@ -1982,14 +1981,8 @@ static void dw_mci_work_routine_card(struct work_struct *work)
                      }
 
                      /* Power down slot */
-                     if (present == 0) {
-                             /* Clear down the FIFO */
-                             dw_mci_fifo_reset(host);
-#ifdef CONFIG_MMC_DW_IDMAC
-                             dw_mci_idmac_reset(host);
-#endif
-
-                     }
+                     if (present == 0)
+                             dw_mci_reset(host);
 
                      spin_unlock_bh(&host->lock);
 

@@ -2323,8 +2316,11 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
      return false;
 }
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host)
+static inline bool dw_mci_reset(struct dw_mci *host)
 {
+     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
+     bool ret = false;
+
      /*
       * Reseting generates a block interrupt, hence setting
       * the scatter-gather pointer to NULL.

@@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
              host->sg = NULL;
      }
 
-     return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
-}
+     if (host->use_dma)
+             flags |= SDMMC_CTRL_DMA_RESET;
 
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
-{
-     return dw_mci_ctrl_reset(host,
-                              SDMMC_CTRL_FIFO_RESET |
-                              SDMMC_CTRL_RESET |
-                              SDMMC_CTRL_DMA_RESET);
+     if (dw_mci_ctrl_reset(host, flags)) {
+             /*
+              * In all cases we clear the RAWINTS register to clear any
+              * interrupts.
+              */
+             mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+             /* if using dma we wait for dma_req to clear */
+             if (host->use_dma) {
+                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
+                     u32 status;
+                     do {
+                             status = mci_readl(host, STATUS);
+                             if (!(status & SDMMC_STATUS_DMA_REQ))
+                                     break;
+                             cpu_relax();
+                     } while (time_before(jiffies, timeout));
+
+                     if (status & SDMMC_STATUS_DMA_REQ) {
+                             dev_err(host->dev,
+                                     "%s: Timeout waiting for dma_req to "
+                                     "clear during reset", __func__);
+                             goto ciu_out;
+                     }
+
+                     /* when using DMA next we reset the fifo again */
+                     if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET))
+                             goto ciu_out;
+             }
+     } else {
+             /* if the controller reset bit did clear, then set clock regs */
+             if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) {
+                     dev_err(host->dev, "%s: fifo/dma reset bits didn't "
+                             "clear but ciu was reset, doing clock update.",
+                             __func__);
+                     goto ciu_out;
+             }
+     }
+
+     if (IS_ENABLED(CONFIG_MMC_DW_IDMAC))
+             /* It is also recommended that we reset and reprogram idmac */
+             dw_mci_idmac_reset(host);
+
+     ret = true;
+
+ciu_out:
+     /* After a CTRL reset we need to have CIU set clock registers  */
+     mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+     return ret;
 }
 
 #ifdef CONFIG_OF

@@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host)
      }
 
      /* Reset all blocks */
-     if (!dw_mci_ctrl_all_reset(host))
+     if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS))
              return -ENODEV;
 

drivers/mmc/host/dw_mmc.c int dw_mci_resume(struct dw_mci *host) · line 2787

Quoted 7 lines

drivers/mmc/host/dw_mmc.c +1 -1

      host->dma_ops = host->pdata->dma_ops;

@@ -2744,7 +2784,7 @@ int dw_mci_resume(struct dw_mci *host)
              }
      }
 
-     if (!dw_mci_ctrl_all_reset(host)) {
+     if (!dw_mci_reset(host)) {

Do you have any reason to use dw_mci_reset() unlike doing on probing?

Thanks,
Seungwon Jeon

Quoted 4 lines · Seungwon Jeon

Hi Sonny,

I have missed this patch.

Quoted 2 lines

You finally choose to take extra interrupt handling. If it is not harm, it's fine.

Hi, thanks for coming back to it. Based on my tracing, the interrupt seems to be okay and is just ignored.

Quoted 2 lines

drivers/mmc/host/dw_mmc.c static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset) · line 2319

Quoted 2 lines

drivers/mmc/host/dw_mmc.c +1 -1

-static inline bool dw_mci_fifo_reset(struct dw_mci *host)
+static inline bool dw_mci_reset(struct dw_mci *host)

Quoted 83 lines

Quoted 83 lines

{

drivers/mmc/host/dw_mmc.c +56 -9

+     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
+     bool ret = false;
+
      /*
       * Reseting generates a block interrupt, hence setting
       * the scatter-gather pointer to NULL.

@@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
              host->sg = NULL;
      }
 
-     return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
-}
+     if (host->use_dma)
+             flags |= SDMMC_CTRL_DMA_RESET;
 
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
-{
-     return dw_mci_ctrl_reset(host,
-                              SDMMC_CTRL_FIFO_RESET |
-                              SDMMC_CTRL_RESET |
-                              SDMMC_CTRL_DMA_RESET);
+     if (dw_mci_ctrl_reset(host, flags)) {
+             /*
+              * In all cases we clear the RAWINTS register to clear any
+              * interrupts.
+              */
+             mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+             /* if using dma we wait for dma_req to clear */
+             if (host->use_dma) {
+                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
+                     u32 status;
+                     do {
+                             status = mci_readl(host, STATUS);
+                             if (!(status & SDMMC_STATUS_DMA_REQ))
+                                     break;
+                             cpu_relax();
+                     } while (time_before(jiffies, timeout));
+
+                     if (status & SDMMC_STATUS_DMA_REQ) {
+                             dev_err(host->dev,
+                                     "%s: Timeout waiting for dma_req to "
+                                     "clear during reset", __func__);
+                             goto ciu_out;
+                     }
+
+                     /* when using DMA next we reset the fifo again */
+                     if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET))
+                             goto ciu_out;
+             }
+     } else {
+             /* if the controller reset bit did clear, then set clock regs */
+             if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) {
+                     dev_err(host->dev, "%s: fifo/dma reset bits didn't "
+                             "clear but ciu was reset, doing clock update.",
+                             __func__);
+                     goto ciu_out;
+             }
+     }
+
+     if (IS_ENABLED(CONFIG_MMC_DW_IDMAC))
+             /* It is also recommended that we reset and reprogram idmac */
+             dw_mci_idmac_reset(host);
+
+     ret = true;
+
+ciu_out:
+     /* After a CTRL reset we need to have CIU set clock registers  */
+     mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+     return ret;
 }
 
 #ifdef CONFIG_OF

@@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host)
      }
 
      /* Reset all blocks */
-     if (!dw_mci_ctrl_all_reset(host))
+     if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS))
              return -ENODEV;
 

Quoted 8 lines

drivers/mmc/host/dw_mmc.c int dw_mci_resume(struct dw_mci *host) · line 2787

Quoted 7 lines

drivers/mmc/host/dw_mmc.c +1 -1

      host->dma_ops = host->pdata->dma_ops;

@@ -2744,7 +2784,7 @@ int dw_mci_resume(struct dw_mci *host)
              }
      }
 
-     if (!dw_mci_ctrl_all_reset(host)) {
+     if (!dw_mci_reset(host)) {

Do you have any reason to use dw_mci_reset() unlike doing on probing?

I really wanted to use dw_mci_reset() everwhere, including probe, because that would be simplest, where there is just one reset function always, but the host structure is not completely set up at probe time, so the code in dw_mci_reset() where we try to send a command to update clock fails, so that's why I had to just do a reset.

Quoted 5 lines · Sonny Rao

Quoted 4 lines · Seungwon Jeon

Hi Sonny,

I have missed this patch.

Quoted 8 lines

Quoted 2 lines

You finally choose to take extra interrupt handling. If it is not harm, it's fine.

Hi, thanks for coming back to it. Based on my tracing, the interrupt seems to be okay and is just ignored.

Quoted 2 lines

drivers/mmc/host/dw_mmc.c static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset) · line 2319

Quoted 2 lines

drivers/mmc/host/dw_mmc.c +1 -1

-static inline bool dw_mci_fifo_reset(struct dw_mci *host)
+static inline bool dw_mci_reset(struct dw_mci *host)

"inline" is no needed anymore.

Quoted 92 lines

Quoted 83 lines

Quoted 83 lines

{

drivers/mmc/host/dw_mmc.c +56 -9

+     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
+     bool ret = false;
+
      /*
       * Reseting generates a block interrupt, hence setting
       * the scatter-gather pointer to NULL.

@@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
              host->sg = NULL;
      }
 
-     return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
-}
+     if (host->use_dma)
+             flags |= SDMMC_CTRL_DMA_RESET;
 
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
-{
-     return dw_mci_ctrl_reset(host,
-                              SDMMC_CTRL_FIFO_RESET |
-                              SDMMC_CTRL_RESET |
-                              SDMMC_CTRL_DMA_RESET);
+     if (dw_mci_ctrl_reset(host, flags)) {
+             /*
+              * In all cases we clear the RAWINTS register to clear any
+              * interrupts.
+              */
+             mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+             /* if using dma we wait for dma_req to clear */
+             if (host->use_dma) {
+                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
+                     u32 status;
+                     do {
+                             status = mci_readl(host, STATUS);
+                             if (!(status & SDMMC_STATUS_DMA_REQ))
+                                     break;
+                             cpu_relax();
+                     } while (time_before(jiffies, timeout));
+
+                     if (status & SDMMC_STATUS_DMA_REQ) {
+                             dev_err(host->dev,
+                                     "%s: Timeout waiting for dma_req to "
+                                     "clear during reset", __func__);
+                             goto ciu_out;
+                     }
+
+                     /* when using DMA next we reset the fifo again */
+                     if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET))
+                             goto ciu_out;
+             }
+     } else {
+             /* if the controller reset bit did clear, then set clock regs */
+             if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) {
+                     dev_err(host->dev, "%s: fifo/dma reset bits didn't "
+                             "clear but ciu was reset, doing clock update.",
+                             __func__);
+                     goto ciu_out;
+             }
+     }
+
+     if (IS_ENABLED(CONFIG_MMC_DW_IDMAC))
+             /* It is also recommended that we reset and reprogram idmac */
+             dw_mci_idmac_reset(host);
+
+     ret = true;
+
+ciu_out:
+     /* After a CTRL reset we need to have CIU set clock registers  */
+     mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+     return ret;
 }
 
 #ifdef CONFIG_OF

@@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host)
      }
 
      /* Reset all blocks */
-     if (!dw_mci_ctrl_all_reset(host))
+     if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS))
              return -ENODEV;
 

Quoted 8 lines

drivers/mmc/host/dw_mmc.c int dw_mci_resume(struct dw_mci *host) · line 2787

Quoted 7 lines

drivers/mmc/host/dw_mmc.c +1 -1

      host->dma_ops = host->pdata->dma_ops;

@@ -2744,7 +2784,7 @@ int dw_mci_resume(struct dw_mci *host)
              }
      }
 
-     if (!dw_mci_ctrl_all_reset(host)) {
+     if (!dw_mci_reset(host)) {

Do you have any reason to use dw_mci_reset() unlike doing on probing?

Quoted 5 lines

I really wanted to use dw_mci_reset() everwhere, including probe, because that would be simplest, where there is just one reset function always, but the host structure is not completely set up at probe time, so the code in dw_mci_reset() where we try to send a command to update clock fails, so that's why I had to just do a reset.

Yes, if we can keep one interface, it would be good.
But can you check below?
Did you see the kernel panic on probing with "host->cur_slot" is NULL?

If right, resume seems not different from probe in case of removable type. And dw_mci_idmac_reset() is redundant when dw_mci_reset() is used at resume.

I think dw_mci_ctrl_reset() can be also used at resume like at probe for simple way. For safety's sake, "host->cur_slot" should be guaranteed it's not NULL.

Thanks,
Seungwon Jeon

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook.

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)
v3: rename fifo reset function, and change callers
use this combined reset function in dw_mci_resume()
just one caller left (probe), so get rid of dw_mci_ctrl_all_reset() use DMA reset bit for all systems which use DMA
remove extra IDMAC reset in dw_mci_work_routine_card() do CIU clock update in error path, if CIU reset cleared
v4: remove comment about FIFO reset in dw_mci_work_routine_card()
move down error message when control reset clears but others don't
and clarify the error stating that we will still update clocks
make flags for all reset bits a macro
v5: don't use dw_mci_reset() in dw_mci_resume() and instead match what
is done in dw_mci_probe() and don't force inline dw_mci_resume()

---
 drivers/mmc/host/dw_mmc.c | 86 ++++++++++++++++++++++++++++++++++-------------
 drivers/mmc/host/dw_mmc.h |  5 +++
 2 files changed, 68 insertions(+), 23 deletions(-)

drivers/mmc/host/dw_mmc.c +23 -21

index 55cd110..db25494 100644
@@ -111,8 +111,7 @@ static const u8 tuning_blk_pattern_8bit[] = {
 	0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
 };
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host);
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host);
+static bool dw_mci_reset(struct dw_mci *host);
 
 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)

@@ -1254,7 +1253,7 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
 		 * After an error, there may be data lingering
 		 * in the FIFO
 		 */
-		dw_mci_fifo_reset(host);
+		dw_mci_reset(host);
 	} else {
 		data->bytes_xfered = data->blocks * data->blksz;
 		data->error = 0;

@@ -1371,7 +1370,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
 
 			/* CMD error in data command */
 			if (mrq->cmd->error && mrq->data)
-				dw_mci_fifo_reset(host);
+				dw_mci_reset(host);
 
 			host->cmd = NULL;
 			host->data = NULL;

@@ -1982,14 +1981,8 @@ static void dw_mci_work_routine_card(struct work_struct *work)
 			}
 
 			/* Power down slot */
-			if (present == 0) {
-				/* Clear down the FIFO */
-				dw_mci_fifo_reset(host);
-#ifdef CONFIG_MMC_DW_IDMAC
-				dw_mci_idmac_reset(host);
-#endif
-
-			}
+			if (present == 0)
+				dw_mci_reset(host);
 
 			spin_unlock_bh(&host->lock);
 

@@ -2323,8 +2316,11 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 	return false;
 }
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host)
+static bool dw_mci_reset(struct dw_mci *host)
 {
+	u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
+	bool ret = false;
+
 	/*
 	 * Reseting generates a block interrupt, hence setting
 	 * the scatter-gather pointer to NULL.

@@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 		host->sg = NULL;
 	}
 
-	return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
-}
+	if (host->use_dma)
+		flags |= SDMMC_CTRL_DMA_RESET;
 
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
-{
-	return dw_mci_ctrl_reset(host,
-				 SDMMC_CTRL_FIFO_RESET |
-				 SDMMC_CTRL_RESET |
-				 SDMMC_CTRL_DMA_RESET);
+	if (dw_mci_ctrl_reset(host, flags)) {
+		/*
+		 * In all cases we clear the RAWINTS register to clear any
+		 * interrupts.
+		 */
+		mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+		/* if using dma we wait for dma_req to clear */
+		if (host->use_dma) {
+			unsigned long timeout = jiffies + msecs_to_jiffies(500);
+			u32 status;
+			do {

drivers/mmc/host/dw_mmc.c +17 -2

+			goto ciu_out;
+		}
+	}
+
+	if (IS_ENABLED(CONFIG_MMC_DW_IDMAC))
+		/* It is also recommended that we reset and reprogram idmac */
+		dw_mci_idmac_reset(host);
+
+	ret = true;
+
+ciu_out:
+	/* After a CTRL reset we need to have CIU set clock registers  */
+	mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+	return ret;
 }
 
 #ifdef CONFIG_OF

@@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host)
 	}
 
 	/* Reset all blocks */
-	if (!dw_mci_ctrl_all_reset(host))
+	if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS))
 		return -ENODEV;
 
 	host->dma_ops = host->pdata->dma_ops;

@@ -2744,7 +2784,7 @@ int dw_mci_resume(struct dw_mci *host)
 		}
 	}
 
-	if (!dw_mci_ctrl_all_reset(host)) {
+	if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) {
 		ret = -ENODEV;
 		return ret;
 	}

drivers/mmc/host/dw_mmc.h +5 -0

index 6bf24ab..9ab8ccd 100644
@@ -129,6 +129,7 @@
 #define SDMMC_CMD_INDX(n)		((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)		(((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ		BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)	(((m) & 0x7) << 28 | \
 					 ((r) & 0xFFF) << 16 | \

@@ -150,6 +151,10 @@
 /* Card read threshold */
 #define SDMMC_SET_RD_THLD(v, x)		(((v) & 0x1FFF) << 16 | (x))
 
+/* All ctrl reset bits */
+#define SDMMC_CTRL_ALL_RESET_FLAGS \
+	(SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET | SDMMC_CTRL_DMA_RESET)
+
 /* Register access macros */
 #define mci_readl(dev, reg)			\
 	__raw_readl((dev)->regs + SDMMC_##reg)

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook.

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)
v3: rename fifo reset function, and change callers
use this combined reset function in dw_mci_resume()
just one caller left (probe), so get rid of dw_mci_ctrl_all_reset() use DMA reset bit for all systems which use DMA
remove extra IDMAC reset in dw_mci_work_routine_card() do CIU clock update in error path, if CIU reset cleared
v4: remove comment about FIFO reset in dw_mci_work_routine_card()
move down error message when control reset clears but others don't
and clarify the error stating that we will still update clocks
make flags for all reset bits a macro
v5: don't use dw_mci_reset() in dw_mci_resume() and instead match what
is done in dw_mci_probe() and don't force inline dw_mci_resume()
v6: add newlines to dev_err() messages

---
 drivers/mmc/host/dw_mmc.c | 86 ++++++++++++++++++++++++++++++++++-------------
 drivers/mmc/host/dw_mmc.h |  5 +++
 2 files changed, 68 insertions(+), 23 deletions(-)

drivers/mmc/host/dw_mmc.c +63 -23

index 55cd110..0c0aecb 100644
@@ -111,8 +111,7 @@ static const u8 tuning_blk_pattern_8bit[] = {
 	0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
 };
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host);
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host);
+static bool dw_mci_reset(struct dw_mci *host);
 
 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)

@@ -1254,7 +1253,7 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
 		 * After an error, there may be data lingering
 		 * in the FIFO
 		 */
-		dw_mci_fifo_reset(host);
+		dw_mci_reset(host);
 	} else {
 		data->bytes_xfered = data->blocks * data->blksz;
 		data->error = 0;

@@ -1371,7 +1370,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
 
 			/* CMD error in data command */
 			if (mrq->cmd->error && mrq->data)
-				dw_mci_fifo_reset(host);
+				dw_mci_reset(host);
 
 			host->cmd = NULL;
 			host->data = NULL;

@@ -1982,14 +1981,8 @@ static void dw_mci_work_routine_card(struct work_struct *work)
 			}
 
 			/* Power down slot */
-			if (present == 0) {
-				/* Clear down the FIFO */
-				dw_mci_fifo_reset(host);
-#ifdef CONFIG_MMC_DW_IDMAC
-				dw_mci_idmac_reset(host);
-#endif
-
-			}
+			if (present == 0)
+				dw_mci_reset(host);
 
 			spin_unlock_bh(&host->lock);
 

@@ -2323,8 +2316,11 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 	return false;
 }
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host)
+static bool dw_mci_reset(struct dw_mci *host)
 {
+	u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
+	bool ret = false;
+
 	/*
 	 * Reseting generates a block interrupt, hence setting
 	 * the scatter-gather pointer to NULL.

@@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 		host->sg = NULL;
 	}
 
-	return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
-}
+	if (host->use_dma)
+		flags |= SDMMC_CTRL_DMA_RESET;
 
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
-{
-	return dw_mci_ctrl_reset(host,
-				 SDMMC_CTRL_FIFO_RESET |
-				 SDMMC_CTRL_RESET |
-				 SDMMC_CTRL_DMA_RESET);
+	if (dw_mci_ctrl_reset(host, flags)) {
+		/*
+		 * In all cases we clear the RAWINTS register to clear any
+		 * interrupts.
+		 */
+		mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+		/* if using dma we wait for dma_req to clear */
+		if (host->use_dma) {
+			unsigned long timeout = jiffies + msecs_to_jiffies(500);
+			u32 status;
+			do {
+				status = mci_readl(host, STATUS);
+				if (!(status & SDMMC_STATUS_DMA_REQ))
+					break;
+				cpu_relax();
+			} while (time_before(jiffies, timeout));
+
+			if (status & SDMMC_STATUS_DMA_REQ) {
+				dev_err(host->dev,
+					"%s: Timeout waiting for dma_req to "
+					"clear during reset\n", __func__);
+				goto ciu_out;
+			}
+
+			/* when using DMA next we reset the fifo again */
+			if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET))
+				goto ciu_out;
+		}
+	} else {
+		/* if the controller reset bit did clear, then set clock regs */
+		if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) {
+			dev_err(host->dev, "%s: fifo/dma reset bits didn't "
+				"clear but ciu was reset, doing clock update\n",
+				__func__);
+			goto ciu_out;
+		}
+	}
+
+	if (IS_ENABLED(CONFIG_MMC_DW_IDMAC))
+		/* It is also recommended that we reset and reprogram idmac */
+		dw_mci_idmac_reset(host);
+
+	ret = true;
+
+ciu_out:
+	/* After a CTRL reset we need to have CIU set clock registers  */
+	mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+	return ret;
 }
 
 #ifdef CONFIG_OF

@@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host)
 	}
 
 	/* Reset all blocks */
-	if (!dw_mci_ctrl_all_reset(host))
+	if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS))
 		return -ENODEV;
 
 	host->dma_ops = host->pdata->dma_ops;

@@ -2744,7 +2784,7 @@ int dw_mci_resume(struct dw_mci *host)
 		}
 	}
 
-	if (!dw_mci_ctrl_all_reset(host)) {
+	if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) {
 		ret = -ENODEV;
 		return ret;
 	}

drivers/mmc/host/dw_mmc.h +5 -1

index 6bf24ab..9ab8ccd 100644
@@ -129,6 +129,7 @@
 #define SDMMC_CMD_INDX(n)		((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)		(((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ		BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)	(((m) & 0x7) << 28 | \
 					 ((r) & 0xFFF) << 16 | \

@@ -150,6 +151,10 @@
 /* Card read threshold */
 #define SDMMC_SET_RD_THLD(v, x)		(((v) & 0x1FFF) << 16 | (x))
 
+/* All ctrl reset bits */
+#define SDMMC_CTRL_ALL_RESET_FLAGS \
+	(SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET | SDMMC_CTRL_DMA_RESET)
+
 /* Register access macros */
 #define mci_readl(dev, reg)			\
 	__raw_readl((dev)->regs + SDMMC_##reg)
--

2.0.0.526.g5318336

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

This patch changes the fifo reset code to follow the reset procedure outlined in the documentation of Synopsys Mobile storage host databook.

Signed-off-by Sonny Rao <[email protected]>

Signed-off-by Yuvaraj Kumar C D <[email protected]>

---
v2: Add Generic DMA support
per the documentation, move interrupt clear before wait make the test for DMA host->use_dma rather than host->using_dma add proper return values (although it appears no caller checks)
v3: rename fifo reset function, and change callers
use this combined reset function in dw_mci_resume()
just one caller left (probe), so get rid of dw_mci_ctrl_all_reset() use DMA reset bit for all systems which use DMA
remove extra IDMAC reset in dw_mci_work_routine_card() do CIU clock update in error path, if CIU reset cleared
v4: remove comment about FIFO reset in dw_mci_work_routine_card()
move down error message when control reset clears but others don't
and clarify the error stating that we will still update clocks
make flags for all reset bits a macro
v5: don't use dw_mci_reset() in dw_mci_resume() and instead match what
is done in dw_mci_probe() and don't force inline dw_mci_resume()
v6: add newlines to dev_err() messages

---
 drivers/mmc/host/dw_mmc.c | 86 ++++++++++++++++++++++++++++++++++-------------
 drivers/mmc/host/dw_mmc.h |  5 +++
 2 files changed, 68 insertions(+), 23 deletions(-)

drivers/mmc/host/dw_mmc.c +63 -23

index 55cd110..0c0aecb 100644
@@ -111,8 +111,7 @@ static const u8 tuning_blk_pattern_8bit[] = {
        0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
 };
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host);
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host);
+static bool dw_mci_reset(struct dw_mci *host);
 
 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)

@@ -1254,7 +1253,7 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
                 * After an error, there may be data lingering
                 * in the FIFO
                 */
-               dw_mci_fifo_reset(host);
+               dw_mci_reset(host);
        } else {
                data->bytes_xfered = data->blocks * data->blksz;
                data->error = 0;

@@ -1371,7 +1370,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
 
                        /* CMD error in data command */
                        if (mrq->cmd->error && mrq->data)
-                               dw_mci_fifo_reset(host);
+                               dw_mci_reset(host);
 
                        host->cmd = NULL;
                        host->data = NULL;

@@ -1982,14 +1981,8 @@ static void dw_mci_work_routine_card(struct work_struct *work)
                        }
 
                        /* Power down slot */
-                       if (present == 0) {
-                               /* Clear down the FIFO */
-                               dw_mci_fifo_reset(host);
-#ifdef CONFIG_MMC_DW_IDMAC
-                               dw_mci_idmac_reset(host);
-#endif
-
-                       }
+                       if (present == 0)
+                               dw_mci_reset(host);
 
                        spin_unlock_bh(&host->lock);
 

@@ -2323,8 +2316,11 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
        return false;
 }
 
-static inline bool dw_mci_fifo_reset(struct dw_mci *host)
+static bool dw_mci_reset(struct dw_mci *host)
 {
+       u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
+       bool ret = false;
+
        /*
         * Reseting generates a block interrupt, hence setting
         * the scatter-gather pointer to NULL.

@@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
                host->sg = NULL;
        }
 
-       return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
-}
+       if (host->use_dma)
+               flags |= SDMMC_CTRL_DMA_RESET;
 
-static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
-{
-       return dw_mci_ctrl_reset(host,
-                                SDMMC_CTRL_FIFO_RESET |
-                                SDMMC_CTRL_RESET |
-                                SDMMC_CTRL_DMA_RESET);
+       if (dw_mci_ctrl_reset(host, flags)) {
+               /*
+                * In all cases we clear the RAWINTS register to clear any
+                * interrupts.
+                */
+               mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+               /* if using dma we wait for dma_req to clear */
+               if (host->use_dma) {
+                       unsigned long timeout = jiffies + msecs_to_jiffies(500);
+                       u32 status;
+                       do {
+                               status = mci_readl(host, STATUS);
+                               if (!(status & SDMMC_STATUS_DMA_REQ))
+                                       break;
+                               cpu_relax();
+                       } while (time_before(jiffies, timeout));
+
+                       if (status & SDMMC_STATUS_DMA_REQ) {
+                               dev_err(host->dev,
+                                       "%s: Timeout waiting for dma_req to "
+                                       "clear during reset\n", __func__);
+                               goto ciu_out;
+                       }
+
+                       /* when using DMA next we reset the fifo again */
+                       if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET))
+                               goto ciu_out;
+               }
+       } else {
+               /* if the controller reset bit did clear, then set clock regs */
+               if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) {
+                       dev_err(host->dev, "%s: fifo/dma reset bits didn't "
+                               "clear but ciu was reset, doing clock update\n",
+                               __func__);
+                       goto ciu_out;
+               }
+       }
+
+       if (IS_ENABLED(CONFIG_MMC_DW_IDMAC))
+               /* It is also recommended that we reset and reprogram idmac */
+               dw_mci_idmac_reset(host);
+
+       ret = true;
+
+ciu_out:
+       /* After a CTRL reset we need to have CIU set clock registers  */
+       mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+       return ret;
 }
 
 #ifdef CONFIG_OF

@@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host)
        }
 
        /* Reset all blocks */
-       if (!dw_mci_ctrl_all_reset(host))
+       if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS))
                return -ENODEV;
 
        host->dma_ops = host->pdata->dma_ops;

@@ -2744,7 +2784,7 @@ int dw_mci_resume(struct dw_mci *host)
                }
        }
 
-       if (!dw_mci_ctrl_all_reset(host)) {
+       if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) {
                ret = -ENODEV;
                return ret;
        }

drivers/mmc/host/dw_mmc.h +5 -1

index 6bf24ab..9ab8ccd 100644
@@ -129,6 +129,7 @@
 #define SDMMC_CMD_INDX(n)              ((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)              (((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ           BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)      (((m) & 0x7) << 28 | \
                                         ((r) & 0xFFF) << 16 | \

@@ -150,6 +151,10 @@
 /* Card read threshold */
 #define SDMMC_SET_RD_THLD(v, x)                (((v) & 0x1FFF) << 16 | (x))
 
+/* All ctrl reset bits */
+#define SDMMC_CTRL_ALL_RESET_FLAGS \
+       (SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET | SDMMC_CTRL_DMA_RESET)
+
 /* Register access macros */
 #define mci_readl(dev, reg)                    \
        __raw_readl((dev)->regs + SDMMC_##reg)
--

2.0.0.526.g5318336

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html