* [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay @ 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-23 11:28 ` [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay Masahiro Yamada 0 siblings, 2 replies; 7+ 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] 7+ messages in thread
* [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration 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 15:09 ` Ulf Hansson 2017-02-23 11:28 ` [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay Masahiro Yamada 1 sibling, 1 reply; 7+ 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] 7+ messages in thread
* Re: [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration 2017-02-16 13:06 ` [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration Piotr Sroka @ 2017-02-16 15:09 ` Ulf Hansson 2017-02-17 14:12 ` Piotr Sroka 0 siblings, 1 reply; 7+ 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] 7+ 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 0 siblings, 1 reply; 7+ 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] 7+ 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; 7+ 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] 7+ messages in thread
* Re: [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay 2017-02-16 13:06 [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay Piotr Sroka 2017-02-16 13:06 ` [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration Piotr Sroka @ 2017-02-23 11:28 ` Masahiro Yamada 2017-02-23 11:46 ` Piotr Sroka 1 sibling, 1 reply; 7+ 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] 7+ messages in thread
* RE: [PATCH 1/2] [1/2] mmc: sdhci-cadence: Fix writing PHY delay 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 0 siblings, 0 replies; 7+ 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] 7+ messages in thread
end of thread, other threads:[~2017-02-23 13:21 UTC | newest] Thread overview: 7+ 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 ` [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration Piotr Sroka 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).