* [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay
@ 2017-02-16 13:06 ` Piotr Sroka
0 siblings, 0 replies; 11+ messages in thread
From: Piotr Sroka @ 2017-02-16 13:06 UTC (permalink / raw)
To: linux-mmc
Cc: Ulf Hansson, Rob Herring, Mark Rutland, Adrian Hunter,
linux-kernel, devicetree, Piotr Sroka
Add polling for ACK to be sure that data are written to PHY register.
Signed-off-by: Piotr Sroka <piotrs@cadence.com>
---
drivers/mmc/host/sdhci-cadence.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index 4b0ecb9..c946e45 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -65,11 +65,12 @@ struct sdhci_cdns_priv {
void __iomem *hrs_addr;
};
-static void sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
+static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
u8 addr, u8 data)
{
void __iomem *reg = priv->hrs_addr + SDHCI_CDNS_HRS04;
u32 tmp;
+ int ret;
tmp = (data << SDHCI_CDNS_HRS04_WDATA_SHIFT) |
(addr << SDHCI_CDNS_HRS04_ADDR_SHIFT);
@@ -78,8 +79,14 @@ static void sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
tmp |= SDHCI_CDNS_HRS04_WR;
writel(tmp, reg);
+ ret = readl_poll_timeout(reg, tmp, tmp & SDHCI_CDNS_HRS04_ACK, 0, 10);
+ if (ret)
+ return ret;
+
tmp &= ~SDHCI_CDNS_HRS04_WR;
writel(tmp, reg);
+
+ return 0;
}
static void sdhci_cdns_phy_init(struct sdhci_cdns_priv *priv)
--
2.2.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay
@ 2017-02-16 13:06 ` Piotr Sroka
0 siblings, 0 replies; 11+ messages in thread
From: Piotr Sroka @ 2017-02-16 13:06 UTC (permalink / raw)
To: linux-mmc
Cc: Ulf Hansson, Rob Herring, Mark Rutland, Adrian Hunter,
linux-kernel, devicetree, Piotr Sroka
Add polling for ACK to be sure that data are written to PHY register.
Signed-off-by: Piotr Sroka <piotrs@cadence.com>
---
drivers/mmc/host/sdhci-cadence.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index 4b0ecb9..c946e45 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -65,11 +65,12 @@ struct sdhci_cdns_priv {
void __iomem *hrs_addr;
};
-static void sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
+static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
u8 addr, u8 data)
{
void __iomem *reg = priv->hrs_addr + SDHCI_CDNS_HRS04;
u32 tmp;
+ int ret;
tmp = (data << SDHCI_CDNS_HRS04_WDATA_SHIFT) |
(addr << SDHCI_CDNS_HRS04_ADDR_SHIFT);
@@ -78,8 +79,14 @@ static void sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
tmp |= SDHCI_CDNS_HRS04_WR;
writel(tmp, reg);
+ ret = readl_poll_timeout(reg, tmp, tmp & SDHCI_CDNS_HRS04_ACK, 0, 10);
+ if (ret)
+ return ret;
+
tmp &= ~SDHCI_CDNS_HRS04_WR;
writel(tmp, reg);
+
+ return 0;
}
static void sdhci_cdns_phy_init(struct sdhci_cdns_priv *priv)
--
2.2.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration
2017-02-16 13:06 ` Piotr Sroka
@ 2017-02-16 13:06 ` Piotr Sroka
-1 siblings, 0 replies; 11+ messages in thread
From: Piotr Sroka @ 2017-02-16 13:06 UTC (permalink / raw)
To: linux-mmc
Cc: Ulf Hansson, Rob Herring, Mark Rutland, Adrian Hunter,
linux-kernel, devicetree, Piotr Sroka
DTS properties are used instead of fixed data
because PHY settings can be different for different platforms.
Configuration of new three PHY delays were added
Signed-off-by: Piotr Sroka <piotrs@cadence.com>
---
.../devicetree/bindings/mmc/sdhci-cadence.txt | 54 ++++++++++++++
drivers/mmc/host/sdhci-cadence.c | 83 +++++++++++++++++++---
2 files changed, 129 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
index c0f37cb..221d3fe 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
@@ -19,6 +19,59 @@ if supported. See mmc.txt for details.
- mmc-hs400-1_8v
- mmc-hs400-1_2v
+- phy-input-delay-sd-hs:
+ Value of the delay in the input path for High Speed work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-sd-default:
+ Value of the delay in the input path for Default Speed work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-sd-sdr12:
+ Value of the delay in the input path for SDR12 work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-sd-sdr25:
+ Value of the delay in the input path for SDR25 work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-sd-sdr50:
+ Value of the delay in the input path for SDR50 work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-sd-ddr50:
+ Value of the delay in the input path for DDR50 work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-emmc-legacy:
+ Value of the delay in the input path for eMMC legacy work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-emmc-sdr:
+ Value of the delay in the input path for eMMC SDR work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-emmc-ddr:
+ Value of the delay in the input path for eMMC DDR work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-dll-delay-sdclk:
+ Value of the delay introduced on the sdclk output
+ for all modes except HS200, HS400 and HS400_ES.
+ Valid range = [0:0x7F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-dll-delay-sdclk-hsmmc:
+ Value of the delay introduced on the sdclk output
+ for HS200, HS400 and HS400_ES speed modes.
+ Valid range = [0:0x7F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-dll-delay-strobe:
+ Value of the delay introduced on the dat_strobe input
+ used in HS400 / HS400_ES speed modes.
+ Valid range = [0:0x7F].
+ Delay configuration stay unchanged if this property is not provided.
+
+
Example:
emmc: sdhci@5a000000 {
compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc";
@@ -29,4 +82,5 @@ Example:
mmc-ddr-1_8v;
mmc-hs200-1_8v;
mmc-hs400-1_8v;
+ phy-input-delay-sd-hs = <0>;
};
diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index c946e45..6c338d2 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -17,6 +17,7 @@
#include <linux/iopoll.h>
#include <linux/module.h>
#include <linux/mmc/host.h>
+#include <linux/of.h>
#include "sdhci-pltfm.h"
@@ -53,6 +54,9 @@
#define SDHCI_CDNS_PHY_DLY_EMMC_LEGACY 0x06
#define SDHCI_CDNS_PHY_DLY_EMMC_SDR 0x07
#define SDHCI_CDNS_PHY_DLY_EMMC_DDR 0x08
+#define SDHCI_CDNS_PHY_DLY_SDCLK 0x0b
+#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
@@ -89,13 +93,73 @@ static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
return 0;
}
-static void sdhci_cdns_phy_init(struct sdhci_cdns_priv *priv)
+static int sdhci_cdns_phy_parse_dt(struct device_node *np,
+ struct sdhci_cdns_priv *priv)
{
- sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_HS, 4);
- sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_DEFAULT, 4);
- sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_LEGACY, 9);
- sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_SDR, 2);
- sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_DDR, 3);
+ u32 tmp;
+ int ret;
+
+ if (!of_property_read_u32(np, "phy-input-delay-sd-hs", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_SD_HS, tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-input-delay-sd-default", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_SD_DEFAULT,
+ tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-input-delay-sd-sdr12", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_UHS_SDR12,
+ tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-input-delay-sd-sdr25", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_UHS_SDR25,
+ tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-input-delay-sd-sdr50", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_UHS_SDR50,
+ tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-input-delay-sd-ddr50", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_UHS_DDR50,
+ tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-dll-delay-sdclk", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_SDCLK, tmp);
+
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-dll-delay-sdclk-hsmmc", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_HSMMC, tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-dll-delay-strobe", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_STROBE, tmp);
+ if (ret)
+ return ret;
+ }
+ return 0;
}
static inline void *sdhci_cdns_priv(struct sdhci_host *host)
@@ -224,10 +288,11 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
struct sdhci_host *host;
struct sdhci_pltfm_host *pltfm_host;
struct sdhci_cdns_priv *priv;
+ struct device *dev = &pdev->dev;
struct clk *clk;
int ret;
- clk = devm_clk_get(&pdev->dev, NULL);
+ clk = devm_clk_get(dev, NULL);
if (IS_ERR(clk))
return PTR_ERR(clk);
@@ -253,7 +318,9 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
if (ret)
goto free;
- sdhci_cdns_phy_init(priv);
+ ret = sdhci_cdns_phy_parse_dt(dev->of_node, priv);
+ if (ret)
+ goto free;
ret = sdhci_add_host(host);
if (ret)
--
2.2.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration
@ 2017-02-16 13:06 ` Piotr Sroka
0 siblings, 0 replies; 11+ messages in thread
From: Piotr Sroka @ 2017-02-16 13:06 UTC (permalink / raw)
To: linux-mmc
Cc: Ulf Hansson, Rob Herring, Mark Rutland, Adrian Hunter,
linux-kernel, devicetree, Piotr Sroka
DTS properties are used instead of fixed data
because PHY settings can be different for different platforms.
Configuration of new three PHY delays were added
Signed-off-by: Piotr Sroka <piotrs@cadence.com>
---
.../devicetree/bindings/mmc/sdhci-cadence.txt | 54 ++++++++++++++
drivers/mmc/host/sdhci-cadence.c | 83 +++++++++++++++++++---
2 files changed, 129 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
index c0f37cb..221d3fe 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
@@ -19,6 +19,59 @@ if supported. See mmc.txt for details.
- mmc-hs400-1_8v
- mmc-hs400-1_2v
+- phy-input-delay-sd-hs:
+ Value of the delay in the input path for High Speed work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-sd-default:
+ Value of the delay in the input path for Default Speed work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-sd-sdr12:
+ Value of the delay in the input path for SDR12 work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-sd-sdr25:
+ Value of the delay in the input path for SDR25 work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-sd-sdr50:
+ Value of the delay in the input path for SDR50 work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-sd-ddr50:
+ Value of the delay in the input path for DDR50 work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-emmc-legacy:
+ Value of the delay in the input path for eMMC legacy work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-emmc-sdr:
+ Value of the delay in the input path for eMMC SDR work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-input-delay-emmc-ddr:
+ Value of the delay in the input path for eMMC DDR work mode.
+ Valid range = [0:0x1F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-dll-delay-sdclk:
+ Value of the delay introduced on the sdclk output
+ for all modes except HS200, HS400 and HS400_ES.
+ Valid range = [0:0x7F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-dll-delay-sdclk-hsmmc:
+ Value of the delay introduced on the sdclk output
+ for HS200, HS400 and HS400_ES speed modes.
+ Valid range = [0:0x7F].
+ Delay configuration stay unchanged if this property is not provided.
+- phy-dll-delay-strobe:
+ Value of the delay introduced on the dat_strobe input
+ used in HS400 / HS400_ES speed modes.
+ Valid range = [0:0x7F].
+ Delay configuration stay unchanged if this property is not provided.
+
+
Example:
emmc: sdhci@5a000000 {
compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc";
@@ -29,4 +82,5 @@ Example:
mmc-ddr-1_8v;
mmc-hs200-1_8v;
mmc-hs400-1_8v;
+ phy-input-delay-sd-hs = <0>;
};
diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index c946e45..6c338d2 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -17,6 +17,7 @@
#include <linux/iopoll.h>
#include <linux/module.h>
#include <linux/mmc/host.h>
+#include <linux/of.h>
#include "sdhci-pltfm.h"
@@ -53,6 +54,9 @@
#define SDHCI_CDNS_PHY_DLY_EMMC_LEGACY 0x06
#define SDHCI_CDNS_PHY_DLY_EMMC_SDR 0x07
#define SDHCI_CDNS_PHY_DLY_EMMC_DDR 0x08
+#define SDHCI_CDNS_PHY_DLY_SDCLK 0x0b
+#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
@@ -89,13 +93,73 @@ static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
return 0;
}
-static void sdhci_cdns_phy_init(struct sdhci_cdns_priv *priv)
+static int sdhci_cdns_phy_parse_dt(struct device_node *np,
+ struct sdhci_cdns_priv *priv)
{
- sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_HS, 4);
- sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_DEFAULT, 4);
- sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_LEGACY, 9);
- sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_SDR, 2);
- sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_DDR, 3);
+ u32 tmp;
+ int ret;
+
+ if (!of_property_read_u32(np, "phy-input-delay-sd-hs", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_SD_HS, tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-input-delay-sd-default", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_SD_DEFAULT,
+ tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-input-delay-sd-sdr12", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_UHS_SDR12,
+ tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-input-delay-sd-sdr25", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_UHS_SDR25,
+ tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-input-delay-sd-sdr50", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_UHS_SDR50,
+ tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-input-delay-sd-ddr50", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_UHS_DDR50,
+ tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-dll-delay-sdclk", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_SDCLK, tmp);
+
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-dll-delay-sdclk-hsmmc", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_HSMMC, tmp);
+ if (ret)
+ return ret;
+ }
+ if (!of_property_read_u32(np, "phy-dll-delay-strobe", &tmp)) {
+ ret = sdhci_cdns_write_phy_reg(priv,
+ SDHCI_CDNS_PHY_DLY_STROBE, tmp);
+ if (ret)
+ return ret;
+ }
+ return 0;
}
static inline void *sdhci_cdns_priv(struct sdhci_host *host)
@@ -224,10 +288,11 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
struct sdhci_host *host;
struct sdhci_pltfm_host *pltfm_host;
struct sdhci_cdns_priv *priv;
+ struct device *dev = &pdev->dev;
struct clk *clk;
int ret;
- clk = devm_clk_get(&pdev->dev, NULL);
+ clk = devm_clk_get(dev, NULL);
if (IS_ERR(clk))
return PTR_ERR(clk);
@@ -253,7 +318,9 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
if (ret)
goto free;
- sdhci_cdns_phy_init(priv);
+ ret = sdhci_cdns_phy_parse_dt(dev->of_node, priv);
+ if (ret)
+ goto free;
ret = sdhci_add_host(host);
if (ret)
--
2.2.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration
@ 2017-02-16 15:09 ` Ulf Hansson
0 siblings, 0 replies; 11+ messages in thread
From: Ulf Hansson @ 2017-02-16 15:09 UTC (permalink / raw)
To: Piotr Sroka
Cc: linux-mmc, Rob Herring, Mark Rutland, Adrian Hunter,
linux-kernel, devicetree
On 16 February 2017 at 14:06, Piotr Sroka <piotrs@cadence.com> wrote:
> DTS properties are used instead of fixed data
> because PHY settings can be different for different platforms.
> Configuration of new three PHY delays were added
>
> Signed-off-by: Piotr Sroka <piotrs@cadence.com>
> ---
> .../devicetree/bindings/mmc/sdhci-cadence.txt | 54 ++++++++++++++
Please split this patch.
DT docs should be a separate patch and make sure it precedes the
changes where the new bindings are being parsed in the driver code.
> drivers/mmc/host/sdhci-cadence.c | 83 +++++++++++++++++++---
> 2 files changed, 129 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> index c0f37cb..221d3fe 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> @@ -19,6 +19,59 @@ if supported. See mmc.txt for details.
> - mmc-hs400-1_8v
> - mmc-hs400-1_2v
>
> +- phy-input-delay-sd-hs:
> + Value of the delay in the input path for High Speed work mode.
> + Valid range = [0:0x1F].
Please specify what unit this in. And then also a suffix, like "-ns"
to the name of the binding.
Similar comment applies to all new bindings below.
> + Delay configuration stay unchanged if this property is not provided.
I would remove this information from the DT doc, it's just confusion
when you refer to something that should remain "unchanged".
Similar comment applies to all new bindings below.
> +- phy-input-delay-sd-default:
> + Value of the delay in the input path for Default Speed work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-sd-sdr12:
> + Value of the delay in the input path for SDR12 work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-sd-sdr25:
> + Value of the delay in the input path for SDR25 work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-sd-sdr50:
> + Value of the delay in the input path for SDR50 work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-sd-ddr50:
> + Value of the delay in the input path for DDR50 work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-emmc-legacy:
Legacy? As in legacy speed mode?
> + Value of the delay in the input path for eMMC legacy work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-emmc-sdr:
> + Value of the delay in the input path for eMMC SDR work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-emmc-ddr:
> + Value of the delay in the input path for eMMC DDR work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-dll-delay-sdclk:
> + Value of the delay introduced on the sdclk output
> + for all modes except HS200, HS400 and HS400_ES.
> + Valid range = [0:0x7F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-dll-delay-sdclk-hsmmc:
> + Value of the delay introduced on the sdclk output
> + for HS200, HS400 and HS400_ES speed modes.
> + Valid range = [0:0x7F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-dll-delay-strobe:
> + Value of the delay introduced on the dat_strobe input
> + used in HS400 / HS400_ES speed modes.
> + Valid range = [0:0x7F].
> + Delay configuration stay unchanged if this property is not provided.
A general comment, is that I suggest you look at the generic mmc DT
bindings for the different speedmodes, and align these new names of DT
bindings to those.
> +
> +
> Example:
> emmc: sdhci@5a000000 {
> compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc";
> @@ -29,4 +82,5 @@ Example:
> mmc-ddr-1_8v;
> mmc-hs200-1_8v;
> mmc-hs400-1_8v;
> + phy-input-delay-sd-hs = <0>;
> };
> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
> index c946e45..6c338d2 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence.c
> @@ -17,6 +17,7 @@
> #include <linux/iopoll.h>
> #include <linux/module.h>
> #include <linux/mmc/host.h>
> +#include <linux/of.h>
>
> #include "sdhci-pltfm.h"
>
> @@ -53,6 +54,9 @@
> #define SDHCI_CDNS_PHY_DLY_EMMC_LEGACY 0x06
> #define SDHCI_CDNS_PHY_DLY_EMMC_SDR 0x07
> #define SDHCI_CDNS_PHY_DLY_EMMC_DDR 0x08
> +#define SDHCI_CDNS_PHY_DLY_SDCLK 0x0b
> +#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
> @@ -89,13 +93,73 @@ static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> return 0;
> }
>
> -static void sdhci_cdns_phy_init(struct sdhci_cdns_priv *priv)
> +static int sdhci_cdns_phy_parse_dt(struct device_node *np,
> + struct sdhci_cdns_priv *priv)
> {
> - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_HS, 4);
> - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_DEFAULT, 4);
> - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_LEGACY, 9);
> - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_SDR, 2);
> - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_DDR, 3);
> + u32 tmp;
> + int ret;
> +
> + if (!of_property_read_u32(np, "phy-input-delay-sd-hs", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_SD_HS, tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-input-delay-sd-default", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_SD_DEFAULT,
> + tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-input-delay-sd-sdr12", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_UHS_SDR12,
> + tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-input-delay-sd-sdr25", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_UHS_SDR25,
> + tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-input-delay-sd-sdr50", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_UHS_SDR50,
> + tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-input-delay-sd-ddr50", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_UHS_DDR50,
> + tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-dll-delay-sdclk", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_SDCLK, tmp);
> +
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-dll-delay-sdclk-hsmmc", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_HSMMC, tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-dll-delay-strobe", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_STROBE, tmp);
> + if (ret)
> + return ret;
> + }
> + return 0;
This all looks very weird. Should you write all this to the phy
register, even before you know anything about what kind of
eMMC/MMC/SD/SDIO card that is attached? Does that even work?
> }
>
> static inline void *sdhci_cdns_priv(struct sdhci_host *host)
> @@ -224,10 +288,11 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
> struct sdhci_host *host;
> struct sdhci_pltfm_host *pltfm_host;
> struct sdhci_cdns_priv *priv;
> + struct device *dev = &pdev->dev;
> struct clk *clk;
> int ret;
>
> - clk = devm_clk_get(&pdev->dev, NULL);
> + clk = devm_clk_get(dev, NULL);
This seems like and unrelated change. Please remove this change from the patch.
> if (IS_ERR(clk))
> return PTR_ERR(clk);
>
> @@ -253,7 +318,9 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
> if (ret)
> goto free;
>
> - sdhci_cdns_phy_init(priv);
> + ret = sdhci_cdns_phy_parse_dt(dev->of_node, priv);
> + if (ret)
> + goto free;
>
> ret = sdhci_add_host(host);
> if (ret)
> --
> 2.2.2
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration
@ 2017-02-16 15:09 ` Ulf Hansson
0 siblings, 0 replies; 11+ messages in thread
From: Ulf Hansson @ 2017-02-16 15:09 UTC (permalink / raw)
To: Piotr Sroka
Cc: linux-mmc-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Mark Rutland,
Adrian Hunter, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 16 February 2017 at 14:06, Piotr Sroka <piotrs-vna1KIf7WgpBDgjK7y7TUQ@public.gmane.org> wrote:
> DTS properties are used instead of fixed data
> because PHY settings can be different for different platforms.
> Configuration of new three PHY delays were added
>
> Signed-off-by: Piotr Sroka <piotrs-vna1KIf7WgpBDgjK7y7TUQ@public.gmane.org>
> ---
> .../devicetree/bindings/mmc/sdhci-cadence.txt | 54 ++++++++++++++
Please split this patch.
DT docs should be a separate patch and make sure it precedes the
changes where the new bindings are being parsed in the driver code.
> drivers/mmc/host/sdhci-cadence.c | 83 +++++++++++++++++++---
> 2 files changed, 129 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> index c0f37cb..221d3fe 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> @@ -19,6 +19,59 @@ if supported. See mmc.txt for details.
> - mmc-hs400-1_8v
> - mmc-hs400-1_2v
>
> +- phy-input-delay-sd-hs:
> + Value of the delay in the input path for High Speed work mode.
> + Valid range = [0:0x1F].
Please specify what unit this in. And then also a suffix, like "-ns"
to the name of the binding.
Similar comment applies to all new bindings below.
> + Delay configuration stay unchanged if this property is not provided.
I would remove this information from the DT doc, it's just confusion
when you refer to something that should remain "unchanged".
Similar comment applies to all new bindings below.
> +- phy-input-delay-sd-default:
> + Value of the delay in the input path for Default Speed work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-sd-sdr12:
> + Value of the delay in the input path for SDR12 work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-sd-sdr25:
> + Value of the delay in the input path for SDR25 work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-sd-sdr50:
> + Value of the delay in the input path for SDR50 work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-sd-ddr50:
> + Value of the delay in the input path for DDR50 work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-emmc-legacy:
Legacy? As in legacy speed mode?
> + Value of the delay in the input path for eMMC legacy work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-emmc-sdr:
> + Value of the delay in the input path for eMMC SDR work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-input-delay-emmc-ddr:
> + Value of the delay in the input path for eMMC DDR work mode.
> + Valid range = [0:0x1F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-dll-delay-sdclk:
> + Value of the delay introduced on the sdclk output
> + for all modes except HS200, HS400 and HS400_ES.
> + Valid range = [0:0x7F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-dll-delay-sdclk-hsmmc:
> + Value of the delay introduced on the sdclk output
> + for HS200, HS400 and HS400_ES speed modes.
> + Valid range = [0:0x7F].
> + Delay configuration stay unchanged if this property is not provided.
> +- phy-dll-delay-strobe:
> + Value of the delay introduced on the dat_strobe input
> + used in HS400 / HS400_ES speed modes.
> + Valid range = [0:0x7F].
> + Delay configuration stay unchanged if this property is not provided.
A general comment, is that I suggest you look at the generic mmc DT
bindings for the different speedmodes, and align these new names of DT
bindings to those.
> +
> +
> Example:
> emmc: sdhci@5a000000 {
> compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc";
> @@ -29,4 +82,5 @@ Example:
> mmc-ddr-1_8v;
> mmc-hs200-1_8v;
> mmc-hs400-1_8v;
> + phy-input-delay-sd-hs = <0>;
> };
> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
> index c946e45..6c338d2 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence.c
> @@ -17,6 +17,7 @@
> #include <linux/iopoll.h>
> #include <linux/module.h>
> #include <linux/mmc/host.h>
> +#include <linux/of.h>
>
> #include "sdhci-pltfm.h"
>
> @@ -53,6 +54,9 @@
> #define SDHCI_CDNS_PHY_DLY_EMMC_LEGACY 0x06
> #define SDHCI_CDNS_PHY_DLY_EMMC_SDR 0x07
> #define SDHCI_CDNS_PHY_DLY_EMMC_DDR 0x08
> +#define SDHCI_CDNS_PHY_DLY_SDCLK 0x0b
> +#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
> @@ -89,13 +93,73 @@ static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> return 0;
> }
>
> -static void sdhci_cdns_phy_init(struct sdhci_cdns_priv *priv)
> +static int sdhci_cdns_phy_parse_dt(struct device_node *np,
> + struct sdhci_cdns_priv *priv)
> {
> - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_HS, 4);
> - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_DEFAULT, 4);
> - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_LEGACY, 9);
> - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_SDR, 2);
> - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_DDR, 3);
> + u32 tmp;
> + int ret;
> +
> + if (!of_property_read_u32(np, "phy-input-delay-sd-hs", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_SD_HS, tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-input-delay-sd-default", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_SD_DEFAULT,
> + tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-input-delay-sd-sdr12", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_UHS_SDR12,
> + tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-input-delay-sd-sdr25", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_UHS_SDR25,
> + tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-input-delay-sd-sdr50", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_UHS_SDR50,
> + tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-input-delay-sd-ddr50", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_UHS_DDR50,
> + tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-dll-delay-sdclk", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_SDCLK, tmp);
> +
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-dll-delay-sdclk-hsmmc", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_HSMMC, tmp);
> + if (ret)
> + return ret;
> + }
> + if (!of_property_read_u32(np, "phy-dll-delay-strobe", &tmp)) {
> + ret = sdhci_cdns_write_phy_reg(priv,
> + SDHCI_CDNS_PHY_DLY_STROBE, tmp);
> + if (ret)
> + return ret;
> + }
> + return 0;
This all looks very weird. Should you write all this to the phy
register, even before you know anything about what kind of
eMMC/MMC/SD/SDIO card that is attached? Does that even work?
> }
>
> static inline void *sdhci_cdns_priv(struct sdhci_host *host)
> @@ -224,10 +288,11 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
> struct sdhci_host *host;
> struct sdhci_pltfm_host *pltfm_host;
> struct sdhci_cdns_priv *priv;
> + struct device *dev = &pdev->dev;
> struct clk *clk;
> int ret;
>
> - clk = devm_clk_get(&pdev->dev, NULL);
> + clk = devm_clk_get(dev, NULL);
This seems like and unrelated change. Please remove this change from the patch.
> if (IS_ERR(clk))
> return PTR_ERR(clk);
>
> @@ -253,7 +318,9 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
> if (ret)
> goto free;
>
> - sdhci_cdns_phy_init(priv);
> + ret = sdhci_cdns_phy_parse_dt(dev->of_node, priv);
> + if (ret)
> + goto free;
>
> ret = sdhci_add_host(host);
> if (ret)
> --
> 2.2.2
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration
2017-02-16 15:09 ` Ulf Hansson
(?)
@ 2017-02-17 14:12 ` Piotr Sroka
2017-02-23 11:47 ` Masahiro Yamada
-1 siblings, 1 reply; 11+ messages in thread
From: Piotr Sroka @ 2017-02-17 14:12 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Rob Herring, Mark Rutland, Adrian Hunter,
linux-kernel, devicetree
> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: 16 February, 2017 4:10 PM
> Subject: Re: [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay
> configuration
>
> On 16 February 2017 at 14:06, Piotr Sroka <piotrs@cadence.com> wrote:
> > DTS properties are used instead of fixed data because PHY settings can
> > be different for different platforms.
> > Configuration of new three PHY delays were added
> >
> > Signed-off-by: Piotr Sroka <piotrs@cadence.com>
> > ---
> > .../devicetree/bindings/mmc/sdhci-cadence.txt | 54 ++++++++++++++
>
> Please split this patch.
>
> DT docs should be a separate patch and make sure it precedes the changes
> where the new bindings are being parsed in the driver code.
>
Ok I will do it in next version of patch.
> > drivers/mmc/host/sdhci-cadence.c | 83 +++++++++++++++++++-
> --
> > 2 files changed, 129 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> > b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> > index c0f37cb..221d3fe 100644
> > --- a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> > +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> > @@ -19,6 +19,59 @@ if supported. See mmc.txt for details.
> > - mmc-hs400-1_8v
> > - mmc-hs400-1_2v
> >
> > +- phy-input-delay-sd-hs:
> > + Value of the delay in the input path for High Speed work mode.
> > + Valid range = [0:0x1F].
>
> Please specify what unit this in. And then also a suffix, like "-ns"
> to the name of the binding.
The delay starts from 5ns (for delay parameter equal to 0), and it is increased by 2.5ns.
0 - means 5ns, 1 means 7.5 ns etc.
I will add this description.
I think the suffix in this case will not be necessary because the unit is a 2.5ns.
What is your opinion?
>
> Similar comment applies to all new bindings below.
>
> > + Delay configuration stay unchanged if this property is not provided.
>
> I would remove this information from the DT doc, it's just confusion when
> you refer to something that should remain "unchanged".
>
Ok I will remove it in next version of patch.
> Similar comment applies to all new bindings below.
>
> > +- phy-input-delay-sd-default:
> > + Value of the delay in the input path for Default Speed work mode.
> > + Valid range = [0:0x1F].
> > + Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-sd-sdr12:
> > + Value of the delay in the input path for SDR12 work mode.
> > + Valid range = [0:0x1F].
> > + Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-sd-sdr25:
> > + Value of the delay in the input path for SDR25 work mode.
> > + Valid range = [0:0x1F].
> > + Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-sd-sdr50:
> > + Value of the delay in the input path for SDR50 work mode.
> > + Valid range = [0:0x1F].
> > + Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-sd-ddr50:
> > + Value of the delay in the input path for DDR50 work mode.
> > + Valid range = [0:0x1F].
> > + Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-emmc-legacy:
>
> Legacy? As in legacy speed mode?
Yes as legacy speed mode. But there are two delays each for one specific mode.
One for SD legacy mode and one for MMC legacy mode.
>
> > + Value of the delay in the input path for eMMC legacy work mode.
> > + Valid range = [0:0x1F].
> > + Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-emmc-sdr:
> > + Value of the delay in the input path for eMMC SDR work mode.
> > + Valid range = [0:0x1F].
> > + Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-emmc-ddr:
> > + Value of the delay in the input path for eMMC DDR work mode.
> > + Valid range = [0:0x1F].
> > + Delay configuration stay unchanged if this property is not provided.
> > +- phy-dll-delay-sdclk:
> > + Value of the delay introduced on the sdclk output
> > + for all modes except HS200, HS400 and HS400_ES.
> > + Valid range = [0:0x7F].
> > + Delay configuration stay unchanged if this property is not provided.
> > +- phy-dll-delay-sdclk-hsmmc:
> > + Value of the delay introduced on the sdclk output
> > + for HS200, HS400 and HS400_ES speed modes.
> > + Valid range = [0:0x7F].
> > + Delay configuration stay unchanged if this property is not provided.
> > +- phy-dll-delay-strobe:
> > + Value of the delay introduced on the dat_strobe input
> > + used in HS400 / HS400_ES speed modes.
> > + Valid range = [0:0x7F].
> > + Delay configuration stay unchanged if this property is not provided.
>
> A general comment, is that I suggest you look at the generic mmc DT bindings
> for the different speedmodes, and align these new names of DT bindings to
> those.
Ok thanks for suggestion. Does below property names looks more clearly?
phy-input-delay-sd-highspeed,
phy-input-delay-sd-legacy,
phy-input-delay-sd-uhs-sdr12
phy-input-delay-sd-uhs-sdr25
phy-input-delay-sd-uhs-sdr50
phy-input-delay-sd-uhs-ddr50
phy-input-delay-mmc-legacy,
phy-input-delay-mmc-ddr
phy-input-delay-mmc-h200
phy-input-delay-mmc-h400
The last three delays are used for few modes so I will add the names of these modes
in description of each property. I propose do not change the names.
phy-dll-delay-sdclk
phy-dll-delay-sdclk-hsmmc
phy-dll-delay-strobe
> > + if (ret)
> > + return ret;
> > + }
> > + if (!of_property_read_u32(np, "phy-dll-delay-sdclk-hsmmc", &tmp)) {
> > + ret = sdhci_cdns_write_phy_reg(priv,
> > + SDHCI_CDNS_PHY_DLY_HSMMC, tmp);
> > + if (ret)
> > + return ret;
> > + }
> > + if (!of_property_read_u32(np, "phy-dll-delay-strobe", &tmp)) {
> > + ret = sdhci_cdns_write_phy_reg(priv,
> > + SDHCI_CDNS_PHY_DLY_STROBE, tmp);
> > + if (ret)
> > + return ret;
> > + }
> > + return 0;
>
> This all looks very weird. Should you write all this to the phy register, even
> before you know anything about what kind of eMMC/MMC/SD/SDIO card
> that is attached? Does that even work?
Yes it is initial configuration of PHY. Each mode has own input delay.
Delay for legacy timing mode is not used in high speed mode.
The do not interfere with each other.
It works.
> > }
> >
> > static inline void *sdhci_cdns_priv(struct sdhci_host *host) @@
> > -224,10 +288,11 @@ static int sdhci_cdns_probe(struct platform_device
> *pdev)
> > struct sdhci_host *host;
> > struct sdhci_pltfm_host *pltfm_host;
> > struct sdhci_cdns_priv *priv;
> > + struct device *dev = &pdev->dev;
> > struct clk *clk;
> > int ret;
> >
> > - clk = devm_clk_get(&pdev->dev, NULL);
> > + clk = devm_clk_get(dev, NULL);
>
> This seems like and unrelated change. Please remove this change from the
> patch.
Ok it will be removed.
Regards
Piotr
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay
2017-02-16 13:06 ` Piotr Sroka
(?)
(?)
@ 2017-02-23 11:28 ` Masahiro Yamada
2017-02-23 11:46 ` Piotr Sroka
-1 siblings, 1 reply; 11+ messages in thread
From: Masahiro Yamada @ 2017-02-23 11:28 UTC (permalink / raw)
To: Piotr Sroka
Cc: linux-mmc, Ulf Hansson, Rob Herring, Mark Rutland, Adrian Hunter,
Linux Kernel Mailing List, devicetree
Hi.
2017-02-16 22:06 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
> Add polling for ACK to be sure that data are written to PHY register.
>
> Signed-off-by: Piotr Sroka <piotrs@cadence.com>
> ---
> drivers/mmc/host/sdhci-cadence.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
> index 4b0ecb9..c946e45 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence.c
> @@ -65,11 +65,12 @@ struct sdhci_cdns_priv {
> void __iomem *hrs_addr;
> };
>
> -static void sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> +static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> u8 addr, u8 data)
If you have a chance to submit v2,
I want the indent of the above line adjusted.
Otherwise,
Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay
@ 2017-02-23 11:46 ` Piotr Sroka
0 siblings, 0 replies; 11+ messages in thread
From: Piotr Sroka @ 2017-02-23 11:46 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mmc, Ulf Hansson, Rob Herring, Mark Rutland, Adrian Hunter,
Linux Kernel Mailing List, devicetree
> -----Original Message-----
> From: Masahiro Yamada [mailto:yamada.masahiro@socionext.com]
> Sent: 23 February, 2017 12:29 PM
> Subject: Re: [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay
>
> Hi.
>
> > -static void sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> > +static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> > u8 addr, u8 data)
>
>
> If you have a chance to submit v2,
> I want the indent of the above line adjusted.
>
Ok thanks. I will fix the indent in v2.
Regards
Piotr Sroka
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay
@ 2017-02-23 11:46 ` Piotr Sroka
0 siblings, 0 replies; 11+ messages in thread
From: Piotr Sroka @ 2017-02-23 11:46 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mmc, Ulf Hansson, Rob Herring, Mark Rutland, Adrian Hunter,
Linux Kernel Mailing List, devicetree-u79uwXL29TY76Z2rM5mHXA
> -----Original Message-----
> From: Masahiro Yamada [mailto:yamada.masahiro@socionext.com]
> Sent: 23 February, 2017 12:29 PM
> Subject: Re: [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay
>
> Hi.
>
> > -static void sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> > +static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> > u8 addr, u8 data)
>
>
> If you have a chance to submit v2,
> I want the indent of the above line adjusted.
>
Ok thanks. I will fix the indent in v2.
Regards
Piotr Sroka
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration
2017-02-17 14:12 ` Piotr Sroka
@ 2017-02-23 11:47 ` Masahiro Yamada
0 siblings, 0 replies; 11+ messages in thread
From: Masahiro Yamada @ 2017-02-23 11:47 UTC (permalink / raw)
To: Piotr Sroka
Cc: Ulf Hansson, linux-mmc, Rob Herring, Mark Rutland, Adrian Hunter,
linux-kernel, devicetree
Hi.
2017-02-17 23:12 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: 16 February, 2017 4:10 PM
>> Subject: Re: [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay
>> configuration
>>
>> On 16 February 2017 at 14:06, Piotr Sroka <piotrs@cadence.com> wrote:
>> > DTS properties are used instead of fixed data because PHY settings can
>> > be different for different platforms.
>> > Configuration of new three PHY delays were added
>> >
>> > Signed-off-by: Piotr Sroka <piotrs@cadence.com>
>> > ---
>> > .../devicetree/bindings/mmc/sdhci-cadence.txt | 54 ++++++++++++++
>>
>> Please split this patch.
>>
>> DT docs should be a separate patch and make sure it precedes the changes
>> where the new bindings are being parsed in the driver code.
>>
>
> Ok I will do it in next version of patch.
>
>> > drivers/mmc/host/sdhci-cadence.c | 83 +++++++++++++++++++-
>> --
>> > 2 files changed, 129 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
>> > b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
>> > index c0f37cb..221d3fe 100644
>> > --- a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
>> > +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
>> > @@ -19,6 +19,59 @@ if supported. See mmc.txt for details.
>> > - mmc-hs400-1_8v
>> > - mmc-hs400-1_2v
>> >
>> > +- phy-input-delay-sd-hs:
>> > + Value of the delay in the input path for High Speed work mode.
>> > + Valid range = [0:0x1F].
Instead of bunch of new bindings,
a data associated with an SoC specific compatible will do in most cases.
static const struct of_device_id sdhci_cdns_match[] = {
{
.compatible = "socionext,uniphier-sd4hc",
.data = sdhci_cdns_uniphier_phy_data,
},
{ .compatible = "cdns,sd4hc" },
{ /* sentinel */ }
};
Strictly speaking, the DLL delays will depend on board design as well as SoC.
So, DT bindings would be more flexible, though.
>> Please specify what unit this in. And then also a suffix, like "-ns"
>> to the name of the binding.
>
> The delay starts from 5ns (for delay parameter equal to 0), and it is increased by 2.5ns.
> 0 - means 5ns, 1 means 7.5 ns etc.
In short, all the DT values here are
kind of mysterious register values for the PHY.
>
>> > + if (ret)
>> > + return ret;
>> > + }
>> > + if (!of_property_read_u32(np, "phy-dll-delay-sdclk-hsmmc", &tmp)) {
>> > + ret = sdhci_cdns_write_phy_reg(priv,
>> > + SDHCI_CDNS_PHY_DLY_HSMMC, tmp);
>> > + if (ret)
>> > + return ret;
>> > + }
>> > + if (!of_property_read_u32(np, "phy-dll-delay-strobe", &tmp)) {
>> > + ret = sdhci_cdns_write_phy_reg(priv,
>> > + SDHCI_CDNS_PHY_DLY_STROBE, tmp);
>> > + if (ret)
>> > + return ret;
>> > + }
>> > + return 0;
The repeat of the same pattern,
"look up a DT property, then if it exists, set it to a register."
Maybe, is it better to describe it with data array + loop, like this?
struct sdhci_cdns_phy_cfg {
const char *property;
u8 addr;
};
static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
{ "phy-input-delay-sd-hs", SDHCI_CDNS_PHY_DLY_SD_HS, },
{ "phy-input-delay-sd-default", SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
{ "phy-input-delay-sd-sdr12", SDHCI_CDNS_PHY_DLY_UHS_SDR12, },
{ "phy-input-delay-sd-sdr25", SDHCI_CDNS_PHY_DLY_UHS_SDR25, },
{ "phy-input-delay-sd-sdr50", SDHCI_CDNS_PHY_DLY_UHS_SDR50, },
...
};
static int sdhci_cdns_phy_init(struct device_node *np,
struct sdhci_cdns_priv *priv)
{
u32 val;
int i;
for (i = 0; i < ARRAY_SIZE(sdhci_cdns_phy_cfgs), i++) {
ret = of_property_read_u32(np, sdhci_cdns_phy_cfgs[i].property,
&val);
if (ret)
continue;
ret = sdhci_cdns_write_phy_reg(priv,
sdhci_cdns_phy_cfgs[i].addr,
val);
if (ret)
return ret;
}
}
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-02-23 13:21 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-16 13:06 [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay Piotr Sroka
2017-02-16 13:06 ` Piotr Sroka
2017-02-16 13:06 ` [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration Piotr Sroka
2017-02-16 13:06 ` Piotr Sroka
2017-02-16 15:09 ` Ulf Hansson
2017-02-16 15:09 ` Ulf Hansson
2017-02-17 14:12 ` Piotr Sroka
2017-02-23 11:47 ` Masahiro Yamada
2017-02-23 11:28 ` [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay Masahiro Yamada
2017-02-23 11:46 ` Piotr Sroka
2017-02-23 11:46 ` Piotr Sroka
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.