[U-Boot] [PATCH 0/8] mmc: some fixes for core and add HS200 support for sdhci-cadence
This series can be cleanly applied to u-boot-mmc/next.
I really hope the HS200 support will be pulled in the next MW.
Masahiro Yamada (8): dm: add dev_read_u32() mmc: do not overwrite cfg->f_max if "max-frequency" if missing mmc: let mmc_of_parse() fail for insane bus-width value mmc: sdhci: do not overwrite host_caps in sdhci_setup_cfg() mmc: sdhci-cadence: use bitfield access macros for cleanup mmc: sdhci-cadence: call mmc_of_parse() mmc: sdhci-cadence: add HS200 support mmc: sdhci: change data transfer failure into debug message
drivers/core/read.c | 5 ++ drivers/mmc/mmc-uclass.c | 9 ++-- drivers/mmc/sdhci-cadence.c | 111 ++++++++++++++++++++++++++++++++++++-------- drivers/mmc/sdhci.c | 6 +-- include/dm/read.h | 16 +++++++ 5 files changed, 120 insertions(+), 27 deletions(-)
dev_read_u32_default() always returns something even when the property is missing. So, it is impossible to do nothing in the case. One solution is to use ofnode_read_u32() instead, but adding dev_read_u32() will be helpful.
BTW, Linux has an equvalent function, device_property_read_u32(); it is clearer that it reads a property. I cannot understand the behavior of dev_read_u32() from its name.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
BTW, I do not understand why we want *_default helpers. It is pretty easy to do equivalent without _default.
priv->val = default; /* default in case property is missing */ dev_read_u32(dev, "foo-property", &priv->val);
On the other hand, if we want to do nothing for missing property, we will end up with clumsy code.
/* we do not want to overwrite priv->val if property is missing */ if (dev_read_bool(dev, "foo-property") { /* default value '0' will not be used anyway */ priv->val = dev_read_u32_default(dev, "foo-property", 0); }
One more BTW, it was just painful to write equivalent code twice for CONFIG_DM_DEV_READ_INLINE.
drivers/core/read.c | 5 +++++ include/dm/read.h | 16 ++++++++++++++++ 2 files changed, 21 insertions(+)
diff --git a/drivers/core/read.c b/drivers/core/read.c index 5d440ce..e027041 100644 --- a/drivers/core/read.c +++ b/drivers/core/read.c @@ -10,6 +10,11 @@ #include <mapmem.h> #include <dm/of_access.h>
+int dev_read_u32(struct udevice *dev, const char *propname, u32 *outp) +{ + return ofnode_read_u32(dev_ofnode(dev), propname, outp); +} + int dev_read_u32_default(struct udevice *dev, const char *propname, int def) { return ofnode_read_u32_default(dev_ofnode(dev), propname, def); diff --git a/include/dm/read.h b/include/dm/read.h index 8114037..5cacec8 100644 --- a/include/dm/read.h +++ b/include/dm/read.h @@ -46,6 +46,16 @@ static inline bool dev_of_valid(struct udevice *dev)
#ifndef CONFIG_DM_DEV_READ_INLINE /** + * dev_read_u32() - read a 32-bit integer from a device's DT property + * + * @dev: device to read DT property from + * @propname: name of the property to read from + * @outp: place to put value (if found) + * @return 0 if OK, -ve on error + */ +int dev_read_u32(struct udevice *dev, const char *propname, u32 *outp); + +/** * dev_read_u32_default() - read a 32-bit integer from a device's DT property * * @dev: device to read DT property from @@ -412,6 +422,12 @@ int dev_read_resource_byname(struct udevice *dev, const char *name,
#else /* CONFIG_DM_DEV_READ_INLINE is enabled */
+static inline int dev_read_u32(struct udevice *dev, + const char *propname, u32 *outp) +{ + return ofnode_read_u32(dev_ofnode(dev), propname, outp); +} + static inline int dev_read_u32_default(struct udevice *dev, const char *propname, int def) {
Hi Masahiro,
On 29 December 2017 at 10:00, Masahiro Yamada yamada.masahiro@socionext.com wrote:
dev_read_u32_default() always returns something even when the property is missing. So, it is impossible to do nothing in the case. One solution is to use ofnode_read_u32() instead, but adding dev_read_u32() will be helpful.
BTW, Linux has an equvalent function, device_property_read_u32(); it is clearer that it reads a property. I cannot understand the behavior of dev_read_u32() from its name.
I decided that 'property' was an unnecessary word since there is nothing else you can ready a value from in the DT.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com
BTW, I do not understand why we want *_default helpers. It is pretty easy to do equivalent without _default.
priv->val = default; /* default in case property is missing */ dev_read_u32(dev, "foo-property", &priv->val);
Your example explains it. Instead, we can do:
priv->val = dev_read_u32_default(dev,"foo-property", default);
which is simpler to understand and does not need a comment.
On the other hand, if we want to do nothing for missing property, we will end up with clumsy code.
/* we do not want to overwrite priv->val if property is missing */ if (dev_read_bool(dev, "foo-property") { /* default value '0' will not be used anyway */ priv->val = dev_read_u32_default(dev, "foo-property", 0); }
priv->val = dev_read_u32_default(dev,"foo-property", priv->val);
One more BTW, it was just painful to write equivalent code twice for CONFIG_DM_DEV_READ_INLINE.
This helps reduce code size for the case where we don't use livetree (SPL). Any ideas on how to make it easier?
drivers/core/read.c | 5 +++++ include/dm/read.h | 16 ++++++++++++++++ 2 files changed, 21 insertions(+)
Regards, Simon
mmc_of_parse() in U-Boot is a pussy helper; it sets cfg->f_max to 52MHz even if DT does not provide "max-frequency" at all. This can overwrite cfg->f_max that may have been set to a reasonable default.
As the DT binding says, "max-frequency" is an optional property. Do nothing if DT does not specify it. This is the behavior of mmc_of_parse() in Linux.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
drivers/mmc/mmc-uclass.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 793196b..37ce922 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -146,7 +146,8 @@ int mmc_of_parse(struct udevice *dev, struct mmc_config *cfg) break; }
- cfg->f_max = dev_read_u32_default(dev, "max-frequency", 52000000); + /* f_max is obtained from the optional "max-frequency" property */ + dev_read_u32(dev, "max-frequency", &cfg->f_max);
if (dev_read_bool(dev, "cap-sd-highspeed")) cfg->host_caps |= MMC_CAP(SD_HS);
You must fix your DT if it specifies insane bus-width, for example, bus-width = <3>;
debug() is not displayed in usual configuration, so people will not even notice weirdness. Use dev_err() instead, then let it fail.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
drivers/mmc/mmc-uclass.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 37ce922..44c36dc 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -140,10 +140,8 @@ int mmc_of_parse(struct udevice *dev, struct mmc_config *cfg) cfg->host_caps |= MMC_MODE_1BIT; break; default: - debug("warning: %s invalid bus-width property. using 1-bit\n", - dev_read_name(dev)); - cfg->host_caps |= MMC_MODE_1BIT; - break; + dev_err(dev, "Invalid "bus-width" value %u!\n", val); + return -EINVAL; }
/* f_max is obtained from the optional "max-frequency" property */
This line overwrites host_cap that has been set by drivers and/or helpers like mmc_of_parse(). Accumulate capabilities flags.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
drivers/mmc/sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index e2ddf5d..243e0e5 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -594,7 +594,7 @@ int sdhci_setup_cfg(struct mmc_config *cfg, struct sdhci_host *host, if (host->quirks & SDHCI_QUIRK_BROKEN_VOLTAGE) cfg->voltages |= host->voltages;
- cfg->host_caps = MMC_MODE_HS | MMC_MODE_HS_52MHz | MMC_MODE_4BIT; + cfg->host_caps |= MMC_MODE_HS | MMC_MODE_HS_52MHz | MMC_MODE_4BIT;
/* Since Host Controller Version3.0 */ if (SDHCI_GET_VERSION(host) >= SDHCI_SPEC_300) {
This driver is a counterpart from the one in Linux. Follow the clean-up I did in Linux.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
drivers/mmc/sdhci-cadence.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/sdhci-cadence.c b/drivers/mmc/sdhci-cadence.c index 72d1c64..712b18c 100644 --- a/drivers/mmc/sdhci-cadence.c +++ b/drivers/mmc/sdhci-cadence.c @@ -7,6 +7,7 @@
#include <common.h> #include <dm.h> +#include <linux/bitfield.h> #include <linux/io.h> #include <linux/iopoll.h> #include <linux/sizes.h> @@ -19,15 +20,14 @@ #define SDHCI_CDNS_HRS04_ACK BIT(26) #define SDHCI_CDNS_HRS04_RD BIT(25) #define SDHCI_CDNS_HRS04_WR BIT(24) -#define SDHCI_CDNS_HRS04_RDATA_SHIFT 16 -#define SDHCI_CDNS_HRS04_WDATA_SHIFT 8 -#define SDHCI_CDNS_HRS04_ADDR_SHIFT 0 +#define SDHCI_CDNS_HRS04_RDATA GENMASK(23, 16) +#define SDHCI_CDNS_HRS04_WDATA GENMASK(15, 8) +#define SDHCI_CDNS_HRS04_ADDR GENMASK(5, 0)
#define SDHCI_CDNS_HRS06 0x18 /* eMMC control */ #define SDHCI_CDNS_HRS06_TUNE_UP BIT(15) -#define SDHCI_CDNS_HRS06_TUNE_SHIFT 8 -#define SDHCI_CDNS_HRS06_TUNE_MASK 0x3f -#define SDHCI_CDNS_HRS06_MODE_MASK 0x7 +#define SDHCI_CDNS_HRS06_TUNE GENMASK(13, 8) +#define SDHCI_CDNS_HRS06_MODE GENMASK(2, 0) #define SDHCI_CDNS_HRS06_MODE_SD 0x0 #define SDHCI_CDNS_HRS06_MODE_MMC_SDR 0x2 #define SDHCI_CDNS_HRS06_MODE_MMC_DDR 0x3 @@ -84,8 +84,8 @@ static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_plat *plat, u32 tmp; int ret;
- tmp = (data << SDHCI_CDNS_HRS04_WDATA_SHIFT) | - (addr << SDHCI_CDNS_HRS04_ADDR_SHIFT); + tmp = FIELD_PREP(SDHCI_CDNS_HRS04_WDATA, data) | + FIELD_PREP(SDHCI_CDNS_HRS04_ADDR, addr); writel(tmp, reg);
tmp |= SDHCI_CDNS_HRS04_WR; @@ -152,8 +152,8 @@ static void sdhci_cdns_set_control_reg(struct sdhci_host *host) }
tmp = readl(plat->hrs_addr + SDHCI_CDNS_HRS06); - tmp &= ~SDHCI_CDNS_HRS06_MODE_MASK; - tmp |= mode; + tmp &= ~SDHCI_CDNS_HRS06_MODE; + tmp |= FIELD_PREP(SDHCI_CDNS_HRS06_MODE, mode); writel(tmp, plat->hrs_addr + SDHCI_CDNS_HRS06); }
This is needed to parse more capabilities such as mmc-hs200-1_8v.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
drivers/mmc/sdhci-cadence.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/mmc/sdhci-cadence.c b/drivers/mmc/sdhci-cadence.c index 712b18c..921095b 100644 --- a/drivers/mmc/sdhci-cadence.c +++ b/drivers/mmc/sdhci-cadence.c @@ -190,6 +190,10 @@ static int sdhci_cdns_probe(struct udevice *dev) host->ops = &sdhci_cdns_ops; host->quirks |= SDHCI_QUIRK_WAIT_SEND_CMD;
+ ret = mmc_of_parse(dev, &plat->cfg); + if (ret) + return ret; + ret = sdhci_cdns_phy_init(plat, gd->fdt_blob, dev_of_offset(dev)); if (ret) return ret;
Add HS200 timing setting and the MMC tuning callback.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
drivers/mmc/sdhci-cadence.c | 87 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 78 insertions(+), 9 deletions(-)
diff --git a/drivers/mmc/sdhci-cadence.c b/drivers/mmc/sdhci-cadence.c index 921095b..8658126 100644 --- a/drivers/mmc/sdhci-cadence.c +++ b/drivers/mmc/sdhci-cadence.c @@ -52,6 +52,13 @@ #define SDHCI_CDNS_PHY_DLY_HSMMC 0x0c #define SDHCI_CDNS_PHY_DLY_STROBE 0x0d
+/* + * The tuned val register is 6 bit-wide, but not the whole of the range is + * available. The range 0-42 seems to be available (then 43 wraps around to 0) + * but I am not quite sure if it is official. Use only 0 to 39 for safety. + */ +#define SDHCI_CDNS_MAX_TUNING_LOOP 40 + struct sdhci_cdns_plat { struct mmc_config cfg; struct mmc mmc; @@ -135,20 +142,18 @@ static void sdhci_cdns_set_control_reg(struct sdhci_host *host) * The mode should be decided by MMC_TIMING_* like Linux, but * U-Boot does not support timing. Use the clock frequency instead. */ - if (clock <= 26000000) + if (clock <= 26000000) { mode = SDHCI_CDNS_HRS06_MODE_SD; /* use this for Legacy */ - else if (clock <= 52000000) { + } else if (clock <= 52000000) { if (mmc->ddr_mode) mode = SDHCI_CDNS_HRS06_MODE_MMC_DDR; else mode = SDHCI_CDNS_HRS06_MODE_MMC_SDR; } else { - /* - * REVISIT: - * The IP supports HS200/HS400, revisit once U-Boot support it - */ - printf("unsupported frequency %d\n", clock); - return; + if (mmc->ddr_mode) + mode = SDHCI_CDNS_HRS06_MODE_MMC_HS400; + else + mode = SDHCI_CDNS_HRS06_MODE_MMC_HS200; }
tmp = readl(plat->hrs_addr + SDHCI_CDNS_HRS06); @@ -161,6 +166,68 @@ static const struct sdhci_ops sdhci_cdns_ops = { .set_control_reg = sdhci_cdns_set_control_reg, };
+static int sdhci_cdns_set_tune_val(struct sdhci_cdns_plat *plat, + unsigned int val) +{ + void __iomem *reg = plat->hrs_addr + SDHCI_CDNS_HRS06; + u32 tmp; + + if (WARN_ON(!FIELD_FIT(SDHCI_CDNS_HRS06_TUNE, val))) + return -EINVAL; + + tmp = readl(reg); + tmp &= ~SDHCI_CDNS_HRS06_TUNE; + tmp |= FIELD_PREP(SDHCI_CDNS_HRS06_TUNE, val); + tmp |= SDHCI_CDNS_HRS06_TUNE_UP; + writel(tmp, reg); + + return readl_poll_timeout(reg, tmp, !(tmp & SDHCI_CDNS_HRS06_TUNE_UP), + 1); +} + +static int sdhci_cdns_execute_tuning(struct udevice *dev, unsigned int opcode) +{ + struct sdhci_cdns_plat *plat = dev_get_platdata(dev); + struct mmc *mmc = &plat->mmc; + int cur_streak = 0; + int max_streak = 0; + int end_of_streak = 0; + int i; + + /* + * This handler only implements the eMMC tuning that is specific to + * this controller. The tuning for SD timing should be handled by the + * SDHCI core. + */ + if (!IS_MMC(mmc)) + return -ENOSYS; + + if (WARN_ON(opcode != MMC_CMD_SEND_TUNING_BLOCK_HS200)) + return -EINVAL; + + for (i = 0; i < SDHCI_CDNS_MAX_TUNING_LOOP; i++) { + if (sdhci_cdns_set_tune_val(plat, i) || + mmc_send_tuning(mmc, opcode, NULL)) { /* bad */ + cur_streak = 0; + } else { /* good */ + cur_streak++; + if (cur_streak > max_streak) { + max_streak = cur_streak; + end_of_streak = i; + } + } + } + + if (!max_streak) { + dev_err(dev, "no tuning point found\n"); + return -EIO; + } + + return sdhci_cdns_set_tune_val(plat, end_of_streak - max_streak / 2); +} + +static struct dm_mmc_ops sdhci_cdns_mmc_ops; + static int sdhci_cdns_bind(struct udevice *dev) { struct sdhci_cdns_plat *plat = dev_get_platdata(dev); @@ -189,6 +256,8 @@ static int sdhci_cdns_probe(struct udevice *dev) host->ioaddr = plat->hrs_addr + SDHCI_CDNS_SRS_BASE; host->ops = &sdhci_cdns_ops; host->quirks |= SDHCI_QUIRK_WAIT_SEND_CMD; + sdhci_cdns_mmc_ops = sdhci_ops; + sdhci_cdns_mmc_ops.execute_tuning = sdhci_cdns_execute_tuning;
ret = mmc_of_parse(dev, &plat->cfg); if (ret) @@ -223,5 +292,5 @@ U_BOOT_DRIVER(sdhci_cdns) = { .probe = sdhci_cdns_probe, .priv_auto_alloc_size = sizeof(struct sdhci_host), .platdata_auto_alloc_size = sizeof(struct sdhci_cdns_plat), - .ops = &sdhci_ops, + .ops = &sdhci_cdns_mmc_ops, };
Hi Masahiro,
On 12/30/2017 02:00 AM, Masahiro Yamada wrote:
Add HS200 timing setting and the MMC tuning callback.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com
drivers/mmc/sdhci-cadence.c | 87 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 78 insertions(+), 9 deletions(-)
diff --git a/drivers/mmc/sdhci-cadence.c b/drivers/mmc/sdhci-cadence.c index 921095b..8658126 100644 --- a/drivers/mmc/sdhci-cadence.c +++ b/drivers/mmc/sdhci-cadence.c @@ -52,6 +52,13 @@ #define SDHCI_CDNS_PHY_DLY_HSMMC 0x0c #define SDHCI_CDNS_PHY_DLY_STROBE 0x0d
+/*
- The tuned val register is 6 bit-wide, but not the whole of the range is
- available. The range 0-42 seems to be available (then 43 wraps around to 0)
- but I am not quite sure if it is official. Use only 0 to 39 for safety.
- */
+#define SDHCI_CDNS_MAX_TUNING_LOOP 40
struct sdhci_cdns_plat { struct mmc_config cfg; struct mmc mmc; @@ -135,20 +142,18 @@ static void sdhci_cdns_set_control_reg(struct sdhci_host *host) * The mode should be decided by MMC_TIMING_* like Linux, but * U-Boot does not support timing. Use the clock frequency instead. */
- if (clock <= 26000000)
- if (clock <= 26000000) { mode = SDHCI_CDNS_HRS06_MODE_SD; /* use this for Legacy */
- else if (clock <= 52000000) {
- } else if (clock <= 52000000) { if (mmc->ddr_mode) mode = SDHCI_CDNS_HRS06_MODE_MMC_DDR; else mode = SDHCI_CDNS_HRS06_MODE_MMC_SDR; } else {
/** REVISIT:* The IP supports HS200/HS400, revisit once U-Boot support it*/printf("unsupported frequency %d\n", clock);return;
if (mmc->ddr_mode)mode = SDHCI_CDNS_HRS06_MODE_MMC_HS400;elsemode = SDHCI_CDNS_HRS06_MODE_MMC_HS200;}
tmp = readl(plat->hrs_addr + SDHCI_CDNS_HRS06);
@@ -161,6 +166,68 @@ static const struct sdhci_ops sdhci_cdns_ops = { .set_control_reg = sdhci_cdns_set_control_reg, };
+static int sdhci_cdns_set_tune_val(struct sdhci_cdns_plat *plat,
unsigned int val)+{
- void __iomem *reg = plat->hrs_addr + SDHCI_CDNS_HRS06;
- u32 tmp;
- if (WARN_ON(!FIELD_FIT(SDHCI_CDNS_HRS06_TUNE, val)))
return -EINVAL;- tmp = readl(reg);
- tmp &= ~SDHCI_CDNS_HRS06_TUNE;
- tmp |= FIELD_PREP(SDHCI_CDNS_HRS06_TUNE, val);
- tmp |= SDHCI_CDNS_HRS06_TUNE_UP;
- writel(tmp, reg);
- return readl_poll_timeout(reg, tmp, !(tmp & SDHCI_CDNS_HRS06_TUNE_UP),
1);+}
+static int sdhci_cdns_execute_tuning(struct udevice *dev, unsigned int opcode) +{
- struct sdhci_cdns_plat *plat = dev_get_platdata(dev);
- struct mmc *mmc = &plat->mmc;
- int cur_streak = 0;
- int max_streak = 0;
- int end_of_streak = 0;
- int i;
- /*
* This handler only implements the eMMC tuning that is specific to* this controller. The tuning for SD timing should be handled by the* SDHCI core.*/- if (!IS_MMC(mmc))
return -ENOSYS;- if (WARN_ON(opcode != MMC_CMD_SEND_TUNING_BLOCK_HS200))
return -EINVAL;- for (i = 0; i < SDHCI_CDNS_MAX_TUNING_LOOP; i++) {
if (sdhci_cdns_set_tune_val(plat, i) ||mmc_send_tuning(mmc, opcode, NULL)) { /* bad */cur_streak = 0;} else { /* good */cur_streak++;if (cur_streak > max_streak) {max_streak = cur_streak;end_of_streak = i;}}- }
- if (!max_streak) {
dev_err(dev, "no tuning point found\n");return -EIO;- }
- return sdhci_cdns_set_tune_val(plat, end_of_streak - max_streak / 2);
+}
+static struct dm_mmc_ops sdhci_cdns_mmc_ops;
static int sdhci_cdns_bind(struct udevice *dev) { struct sdhci_cdns_plat *plat = dev_get_platdata(dev); @@ -189,6 +256,8 @@ static int sdhci_cdns_probe(struct udevice *dev) host->ioaddr = plat->hrs_addr + SDHCI_CDNS_SRS_BASE; host->ops = &sdhci_cdns_ops; host->quirks |= SDHCI_QUIRK_WAIT_SEND_CMD;
- sdhci_cdns_mmc_ops = sdhci_ops;
- sdhci_cdns_mmc_ops.execute_tuning = sdhci_cdns_execute_tuning;
It needs to add the #ifdef MMC_SUPPORT_TUNING. When MMC_SUPPPORT_TUNING is enabled, excute_tuning is included as member.
Best Regards, Jaehoon Chung
ret = mmc_of_parse(dev, &plat->cfg); if (ret) @@ -223,5 +292,5 @@ U_BOOT_DRIVER(sdhci_cdns) = { .probe = sdhci_cdns_probe, .priv_auto_alloc_size = sizeof(struct sdhci_host), .platdata_auto_alloc_size = sizeof(struct sdhci_cdns_plat),
- .ops = &sdhci_ops,
- .ops = &sdhci_cdns_mmc_ops,
};
Hi Jaehoon,
2018-01-12 17:44 GMT+09:00 Jaehoon Chung jh80.chung@samsung.com:
Hi Masahiro,
On 12/30/2017 02:00 AM, Masahiro Yamada wrote:
Add HS200 timing setting and the MMC tuning callback.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com
drivers/mmc/sdhci-cadence.c | 87 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 78 insertions(+), 9 deletions(-)
diff --git a/drivers/mmc/sdhci-cadence.c b/drivers/mmc/sdhci-cadence.c index 921095b..8658126 100644 --- a/drivers/mmc/sdhci-cadence.c +++ b/drivers/mmc/sdhci-cadence.c @@ -52,6 +52,13 @@ #define SDHCI_CDNS_PHY_DLY_HSMMC 0x0c #define SDHCI_CDNS_PHY_DLY_STROBE 0x0d
+/*
- The tuned val register is 6 bit-wide, but not the whole of the range is
- available. The range 0-42 seems to be available (then 43 wraps around to 0)
- but I am not quite sure if it is official. Use only 0 to 39 for safety.
- */
+#define SDHCI_CDNS_MAX_TUNING_LOOP 40
struct sdhci_cdns_plat { struct mmc_config cfg; struct mmc mmc; @@ -135,20 +142,18 @@ static void sdhci_cdns_set_control_reg(struct sdhci_host *host) * The mode should be decided by MMC_TIMING_* like Linux, but * U-Boot does not support timing. Use the clock frequency instead. */
if (clock <= 26000000)
if (clock <= 26000000) { mode = SDHCI_CDNS_HRS06_MODE_SD; /* use this for Legacy */
else if (clock <= 52000000) {
} else if (clock <= 52000000) { if (mmc->ddr_mode) mode = SDHCI_CDNS_HRS06_MODE_MMC_DDR; else mode = SDHCI_CDNS_HRS06_MODE_MMC_SDR; } else {
/** REVISIT:* The IP supports HS200/HS400, revisit once U-Boot support it*/printf("unsupported frequency %d\n", clock);return;
if (mmc->ddr_mode)mode = SDHCI_CDNS_HRS06_MODE_MMC_HS400;elsemode = SDHCI_CDNS_HRS06_MODE_MMC_HS200; } tmp = readl(plat->hrs_addr + SDHCI_CDNS_HRS06);@@ -161,6 +166,68 @@ static const struct sdhci_ops sdhci_cdns_ops = { .set_control_reg = sdhci_cdns_set_control_reg, };
+static int sdhci_cdns_set_tune_val(struct sdhci_cdns_plat *plat,
unsigned int val)+{
void __iomem *reg = plat->hrs_addr + SDHCI_CDNS_HRS06;u32 tmp;if (WARN_ON(!FIELD_FIT(SDHCI_CDNS_HRS06_TUNE, val)))return -EINVAL;tmp = readl(reg);tmp &= ~SDHCI_CDNS_HRS06_TUNE;tmp |= FIELD_PREP(SDHCI_CDNS_HRS06_TUNE, val);tmp |= SDHCI_CDNS_HRS06_TUNE_UP;writel(tmp, reg);return readl_poll_timeout(reg, tmp, !(tmp & SDHCI_CDNS_HRS06_TUNE_UP),1);+}
+static int sdhci_cdns_execute_tuning(struct udevice *dev, unsigned int opcode) +{
struct sdhci_cdns_plat *plat = dev_get_platdata(dev);struct mmc *mmc = &plat->mmc;int cur_streak = 0;int max_streak = 0;int end_of_streak = 0;int i;/** This handler only implements the eMMC tuning that is specific to* this controller. The tuning for SD timing should be handled by the* SDHCI core.*/if (!IS_MMC(mmc))return -ENOSYS;if (WARN_ON(opcode != MMC_CMD_SEND_TUNING_BLOCK_HS200))return -EINVAL;for (i = 0; i < SDHCI_CDNS_MAX_TUNING_LOOP; i++) {if (sdhci_cdns_set_tune_val(plat, i) ||mmc_send_tuning(mmc, opcode, NULL)) { /* bad */cur_streak = 0;} else { /* good */cur_streak++;if (cur_streak > max_streak) {max_streak = cur_streak;end_of_streak = i;}}}if (!max_streak) {dev_err(dev, "no tuning point found\n");return -EIO;}return sdhci_cdns_set_tune_val(plat, end_of_streak - max_streak / 2);+}
+static struct dm_mmc_ops sdhci_cdns_mmc_ops;
static int sdhci_cdns_bind(struct udevice *dev) { struct sdhci_cdns_plat *plat = dev_get_platdata(dev); @@ -189,6 +256,8 @@ static int sdhci_cdns_probe(struct udevice *dev) host->ioaddr = plat->hrs_addr + SDHCI_CDNS_SRS_BASE; host->ops = &sdhci_cdns_ops; host->quirks |= SDHCI_QUIRK_WAIT_SEND_CMD;
sdhci_cdns_mmc_ops = sdhci_ops;sdhci_cdns_mmc_ops.execute_tuning = sdhci_cdns_execute_tuning;It needs to add the #ifdef MMC_SUPPORT_TUNING. When MMC_SUPPPORT_TUNING is enabled, excute_tuning is included as member.
Nice catch!
I've sent v2. Thanks!
Hi Masahiro.
On 01/12/2018 06:11 PM, Masahiro Yamada wrote:
Hi Jaehoon,
2018-01-12 17:44 GMT+09:00 Jaehoon Chung jh80.chung@samsung.com:
Hi Masahiro,
On 12/30/2017 02:00 AM, Masahiro Yamada wrote:
Add HS200 timing setting and the MMC tuning callback.
[..snip..]
+static struct dm_mmc_ops sdhci_cdns_mmc_ops;
static int sdhci_cdns_bind(struct udevice *dev) { struct sdhci_cdns_plat *plat = dev_get_platdata(dev); @@ -189,6 +256,8 @@ static int sdhci_cdns_probe(struct udevice *dev) host->ioaddr = plat->hrs_addr + SDHCI_CDNS_SRS_BASE; host->ops = &sdhci_cdns_ops; host->quirks |= SDHCI_QUIRK_WAIT_SEND_CMD;
sdhci_cdns_mmc_ops = sdhci_ops;sdhci_cdns_mmc_ops.execute_tuning = sdhci_cdns_execute_tuning;It needs to add the #ifdef MMC_SUPPORT_TUNING. When MMC_SUPPPORT_TUNING is enabled, excute_tuning is included as member.
Nice catch!
I've sent v2. Thanks!
Thanks! I'm testing other patches. After finishing them, i will apply yours and other patches. And will send PR on Today! Thanks for resending patch v2.
Best Regards, Jaehoon Chung
During the tuning, drivers repeat data transfer, changing timing parameters in the controller hardware. So, the tuning commands (CMD19 for SD, CMD21 for eMMC) fail, and this is not a problem at all.
Showing "Error detected..." in normal operation just make users upset. This should not be shown.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
drivers/mmc/sdhci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index 243e0e5..d31793a 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -86,8 +86,8 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data, do { stat = sdhci_readl(host, SDHCI_INT_STATUS); if (stat & SDHCI_INT_ERROR) { - printf("%s: Error detected in status(0x%X)!\n", - __func__, stat); + pr_debug("%s: Error detected in status(0x%X)!\n", + __func__, stat); return -EIO; } if (!transfer_done && (stat & rdy)) {
On 29 December 2017 at 10:00, Masahiro Yamada yamada.masahiro@socionext.com wrote:
This series can be cleanly applied to u-boot-mmc/next.
I really hope the HS200 support will be pulled in the next MW.
Yes please, it has been too long.
On 12/30/2017 02:00 AM, Masahiro Yamada wrote:
This series can be cleanly applied to u-boot-mmc/next.
I really hope the HS200 support will be pulled in the next MW.
Applied this series to u-boot-mmc with "[PATCH v2] mmc: sdhci-cadence: add HS200 support". Thanks!
Best Regards, Jaehoon Chung
Masahiro Yamada (8): dm: add dev_read_u32() mmc: do not overwrite cfg->f_max if "max-frequency" if missing mmc: let mmc_of_parse() fail for insane bus-width value mmc: sdhci: do not overwrite host_caps in sdhci_setup_cfg() mmc: sdhci-cadence: use bitfield access macros for cleanup mmc: sdhci-cadence: call mmc_of_parse() mmc: sdhci-cadence: add HS200 support mmc: sdhci: change data transfer failure into debug message
drivers/core/read.c | 5 ++ drivers/mmc/mmc-uclass.c | 9 ++-- drivers/mmc/sdhci-cadence.c | 111 ++++++++++++++++++++++++++++++++++++-------- drivers/mmc/sdhci.c | 6 +-- include/dm/read.h | 16 +++++++ 5 files changed, 120 insertions(+), 27 deletions(-)
participants (3)
-
Jaehoon Chung -
Masahiro Yamada -
Simon Glass