Hi Marek,

On Mon, 2015-07-27 at 22:39 +0200, Marek Vasut wrote:

Endless timeouts are bad, since if we get stuck in one, we have no way out. Zap this one by implementing proper timeout.

Signed-off-by: Marek Vasut marex@denx.de Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Pantelis Antoniou panto@antoniou-consulting.com Cc: Tom Rini trini@konsulko.com

---

drivers/mmc/dw_mmc.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 3fffa71..0f61f16 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -211,14 +211,29 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, }

if (data) {

• do {
  

• start = get_timer(0);
  

• timeout = 1000;
  

• for (;;) {
  mask = dwmci_readl(host, DWMCI_RINTSTS);
  

• 	/* Error during data transfer. */
  if (mask & (DWMCI_DATA_ERR | DWMCI_DATA_TOUT)) {
  	printf("%s: DATA ERROR!\n", __func__);
  	bounce_buffer_stop(&bbstate);
  	return -1;
  }
  

• } while (!(mask & DWMCI_INTMSK_DTO));
  

• 	/* Data arrived correctly. */
  

• 	if (mask & DWMCI_INTMSK_DTO)
  

• 		break;
  

• 	/* Check for timeout. */
  

• 	if (get_timer(start) > timeout) {
  

• 		printf("%s: Timeout waiting for data!\n",
  

• 		       __func__);
  

• 		bounce_buffer_stop(&bbstate);
  

• 		return TIMEOUT;
  

• 	}
  

• }
  
  

  dwmci_writel(host, DWMCI_RINTSTS, mask);

It turned out that patch breaks functionality in some cases. For me on every attempt to download something significant (at least I see it on 5/7 Mb files) from SD I'm seeing timeout firing too early.

I added a bit of extra instrumentation to see where time is spent and why.

So my diff is: ----------------------------------->8-------------------------------- diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 77b87e0..2da77a7 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -213,7 +213,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,

if (data) { start = get_timer(0); - timeout = 1000; + timeout = 10000; // That's required to get to the end of the transfer for (;;) { mask = dwmci_readl(host, DWMCI_RINTSTS); /* Error during data transfer. */ @@ -226,6 +226,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, /* Data arrived correctly. */ if (mask & DWMCI_INTMSK_DTO) { ret = 0; + printf(" * time spent: %d, data size: %d, blocks: %d\n", (int)get_timer(start), data ->blocksize * data->blocks, data->blocks); break; } ----------------------------------->8--------------------------------

And that's what I see then: ----------------------------------->8-------------------------------- AXS# fatload mmc 0 * time spent: 0, data size: 8, blocks: 1 * time spent: 0, data size: 512, blocks: 1 * time spent: 0, data size: 512, blocks: 1 * time spent: 0, data size: 512, blocks: 1 reading uImage * time spent: 1, data size: 512, blocks: 1 * time spent: 0, data size: 1024, blocks: 2 * time spent: 1, data size: 3072, blocks: 6 * time spent: 1, data size: 3072, blocks: 6 * time spent: 1, data size: 3072, blocks: 6 * time spent: 0, data size: 3072, blocks: 6 * time spent: 0, data size: 3072, blocks: 6 * time spent: 1599, data size: 13338112, blocks: 26051 * time spent: 0, data size: 512, blocks: 1 13338188 bytes read in 1651 ms (7.7 MiB/s) ----------------------------------->8--------------------------------

So you see real data transfer takes ~1.7 seconds when getting 26k blocks.

In other words timeout check has to be a bit smarter, for example taking into account number of blocks to be transferred.

Any thoughts?

-Alexey

Rework the driver to probe the MMC controller from Device Tree and make it mandatory. There is no longer support for probing from the ancient qts-generated header files.

Signed-off-by: Marek Vasut marex@denx.de Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Pantelis Antoniou panto@antoniou-consulting.com Cc: Tom Rini trini@konsulko.com --- arch/arm/mach-socfpga/include/mach/dwmmc.h | 2 +- arch/arm/mach-socfpga/misc.c | 9 +--- drivers/mmc/socfpga_dw_mmc.c | 81 +++++++++++++++++++++++++----- include/fdtdec.h | 1 + lib/fdtdec.c | 1 + 5 files changed, 72 insertions(+), 22 deletions(-)

V2: Rebase the patch for Panto :-)

diff --git a/arch/arm/mach-socfpga/include/mach/dwmmc.h b/arch/arm/mach-socfpga/include/mach/dwmmc.h index 945eb64..e8ba901 100644 --- a/arch/arm/mach-socfpga/include/mach/dwmmc.h +++ b/arch/arm/mach-socfpga/include/mach/dwmmc.h @@ -7,6 +7,6 @@ #ifndef _SOCFPGA_DWMMC_H_ #define _SOCFPGA_DWMMC_H_

-extern int socfpga_dwmmc_init(u32 regbase, int bus_width, int index); +int socfpga_dwmmc_init(const void *blob);

#endif /* _SOCFPGA_SDMMC_H_ */ diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c index 6128d54..0940cc5 100644 --- a/arch/arm/mach-socfpga/misc.c +++ b/arch/arm/mach-socfpga/misc.c @@ -125,14 +125,7 @@ int cpu_eth_init(bd_t *bis) */ int cpu_mmc_init(bd_t *bis) { -/* - * FIXME: Temporarily define CONFIG_HPS_SDMMC_BUSWIDTH to prevent breakage - * due to missing patches in u-boot/master . The upcoming patch will - * switch this to OF probing, so this whole block will go away. - */ -#define CONFIG_HPS_SDMMC_BUSWIDTH 8 - return socfpga_dwmmc_init(SOCFPGA_SDMMC_ADDRESS, - CONFIG_HPS_SDMMC_BUSWIDTH, 0); + return socfpga_dwmmc_init(gd->fdt_blob); } #endif

diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c index eb69aed..8076761 100644 --- a/drivers/mmc/socfpga_dw_mmc.c +++ b/drivers/mmc/socfpga_dw_mmc.c @@ -6,6 +6,8 @@

#include <common.h> #include <malloc.h> +#include <fdtdec.h> +#include <libfdt.h> #include <dwmmc.h> #include <errno.h> #include <asm/arch/dwmmc.h> @@ -42,34 +44,87 @@ static void socfpga_dwmci_clksel(struct dwmci_host *host) CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK); }

-int socfpga_dwmmc_init(u32 regbase, int bus_width, int index) +static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx) { + /* FIXME: probe from DT eventually too/ */ + const unsigned long clk = cm_get_mmc_controller_clk_hz(); + struct dwmci_host *host; - unsigned long clk = cm_get_mmc_controller_clk_hz(); + fdt_addr_t reg_base; + int bus_width, fifo_depth;

if (clk == 0) { - printf("%s: MMC clock is zero!", __func__); + printf("DWMMC%d: MMC clock is zero!", idx); return -EINVAL; }

- /* calloc for zero init */ - host = calloc(1, sizeof(struct dwmci_host)); - if (!host) { - printf("%s: calloc() failed!\n", __func__); - return -ENOMEM; + /* Get the register address from the device node */ + reg_base = fdtdec_get_addr(blob, node, "reg"); + if (!reg_base) { + printf("DWMMC%d: Can't get base address\n", idx); + return -EINVAL; + } + + /* Get the bus width from the device node */ + bus_width = fdtdec_get_int(blob, node, "bus-width", 0); + if (bus_width <= 0) { + printf("DWMMC%d: Can't get bus-width\n", idx); + return -EINVAL; }

+ fifo_depth = fdtdec_get_int(blob, node, "fifo-depth", 0); + if (fifo_depth < 0) { + printf("DWMMC%d: Can't get FIFO depth\n", idx); + return -EINVAL; + } + + /* Allocate the host */ + host = calloc(1, sizeof(*host)); + if (!host) + return -ENOMEM; + host->name = "SOCFPGA DWMMC"; - host->ioaddr = (void *)regbase; + host->ioaddr = (void *)reg_base; host->buswidth = bus_width; host->clksel = socfpga_dwmci_clksel; - host->dev_index = index; - /* fixed clock divide by 4 which due to the SDMMC wrapper */ + host->dev_index = idx; + /* Fixed clock divide by 4 which due to the SDMMC wrapper */ host->bus_hz = clk; host->fifoth_val = MSIZE(0x2) | - RX_WMARK(CONFIG_SOCFPGA_DWMMC_FIFO_DEPTH / 2 - 1) | - TX_WMARK(CONFIG_SOCFPGA_DWMMC_FIFO_DEPTH / 2); + RX_WMARK(fifo_depth / 2 - 1) | TX_WMARK(fifo_depth / 2);

return add_dwmci(host, host->bus_hz, 400000); }

+static int socfpga_dwmci_process_node(const void *blob, int nodes[], + int count) +{ + int i, node, ret; + + for (i = 0; i < count; i++) { + node = nodes[i]; + if (node <= 0) + continue; + + ret = socfpga_dwmci_of_probe(blob, node, i); + if (ret) { + printf("%s: failed to decode dev %d\n", __func__, i); + return ret; + } + } + return 0; +} + +int socfpga_dwmmc_init(const void *blob) +{ + int nodes[2]; /* Max. two controllers. */ + int ret, count; + + count = fdtdec_find_aliases_for_id(blob, "mmc", + COMPAT_ALTERA_SOCFPGA_DWMMC, + nodes, ARRAY_SIZE(nodes)); + + ret = socfpga_dwmci_process_node(blob, nodes, count); + + return ret; +} diff --git a/include/fdtdec.h b/include/fdtdec.h index eac679e..cd2c926 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -182,6 +182,7 @@ enum fdt_compat_id { COMPAT_INTEL_PCH, /* Intel PCH */ COMPAT_INTEL_IRQ_ROUTER, /* Intel Interrupt Router */ COMPAT_ALTERA_SOCFPGA_DWMAC, /* SoCFPGA Ethernet controller */ + COMPAT_ALTERA_SOCFPGA_DWMMC, /* SoCFPGA DWMMC controller */

COMPAT_COUNT, }; diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b201787..7023e55 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -76,6 +76,7 @@ static const char * const compat_names[COMPAT_COUNT] = { COMPAT(COMPAT_INTEL_PCH, "intel,bd82x6x"), COMPAT(COMPAT_INTEL_IRQ_ROUTER, "intel,irq-router"), COMPAT(ALTERA_SOCFPGA_DWMAC, "altr,socfpga-stmmac"), + COMPAT(ALTERA_SOCFPGA_DWMMC, "altr,socfpga-dw-mshc"), };

const char *fdtdec_get_compatible(enum fdt_compat_id id)