Add polling for ACK to be sure that data are written to PHY register.
Signed-off-by: Piotr Sroka <[email protected]>
---
Changes for v2:
- fix indent
---
Changes for v3:
- none
---
Changes for v4:
- none
---
drivers/mmc/host/sdhci-cadence.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index 316cfec..b2334ec 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -66,11 +66,12 @@ struct sdhci_cdns_priv {
void __iomem *hrs_addr;
};
-static void sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
- u8 addr, u8 data)
+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);
@@ -79,8 +80,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
DTS properties are used instead of fixed data
because PHY settings can be different for different chips/boards.
Signed-off-by: Piotr Sroka <[email protected]>
---
Changes for v2:
- dts part was removed from this patch
- most delays were moved from dts file
to data associated with an SoC specific compatible
- remove unrelated changes
---
Changes for v3:
- move all delays back to dts because they are also boards dependent
- prefix all of the Cadence-specific properties with cdns prefix
- put checking delay properties inside the for loop
instead of using a lot of single if expressions
---
Changes for v4:
- remove unecessary declaration of sdhci_cdns_match
- format fix (blank line removed)
---
drivers/mmc/host/sdhci-cadence.c | 53 ++++++++++++++++++++++++++++++++++------
1 file changed, 46 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index b2334ec..308a372 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -18,6 +18,7 @@
#include <linux/module.h>
#include <linux/mmc/host.h>
#include <linux/mmc/mmc.h>
+#include <linux/of.h>
#include "sdhci-pltfm.h"
@@ -54,6 +55,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
@@ -66,6 +70,25 @@ struct sdhci_cdns_priv {
void __iomem *hrs_addr;
};
+struct sdhci_cdns_phy_cfg {
+ const char *property;
+ u8 addr;
+};
+
+static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
+ { "cdns,phy-input-delay-sd-highspeed", SDHCI_CDNS_PHY_DLY_SD_HS, },
+ { "cdns,phy-input-delay-sd-legacy", SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
+ { "cdns,phy-input-delay-sd-uhs-sdr12", SDHCI_CDNS_PHY_DLY_UHS_SDR12, },
+ { "cdns,phy-input-delay-sd-uhs-sdr25", SDHCI_CDNS_PHY_DLY_UHS_SDR25, },
+ { "cdns,phy-input-delay-sd-uhs-sdr50", SDHCI_CDNS_PHY_DLY_UHS_SDR50, },
+ { "cdns,phy-input-delay-sd-uhs-ddr50", SDHCI_CDNS_PHY_DLY_UHS_DDR50, },
+ { "cdns,phy-input-delay-mmc-highspeed", SDHCI_CDNS_PHY_DLY_EMMC_SDR, },
+ { "cdns,phy-input-delay-mmc-ddr", SDHCI_CDNS_PHY_DLY_EMMC_DDR, },
+ { "cdns,phy-dll-delay-sdclk", SDHCI_CDNS_PHY_DLY_SDCLK, },
+ { "cdns,phy-dll-delay-sdclk-hsmmc", SDHCI_CDNS_PHY_DLY_HSMMC, },
+ { "cdns,phy-dll-delay-strobe", SDHCI_CDNS_PHY_DLY_STROBE, },
+};
+
static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
u8 addr, u8 data)
{
@@ -90,13 +113,26 @@ 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_init(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 val;
+ int ret, 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;
+ }
+
+ return 0;
}
static inline void *sdhci_cdns_priv(struct sdhci_host *host)
@@ -227,6 +263,7 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
struct sdhci_cdns_priv *priv;
struct clk *clk;
int ret;
+ struct device *dev = &pdev->dev;
clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(clk))
@@ -254,7 +291,9 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
if (ret)
goto free;
- sdhci_cdns_phy_init(priv);
+ ret = sdhci_cdns_phy_init(dev->of_node, priv);
+ if (ret)
+ goto free;
ret = sdhci_add_host(host);
if (ret)
--
2.2.2
DTS properties are used instead of fixed data
because PHY settings can be different for different chips/boards.
Add description of new DLL PHY delays.
Signed-off-by: Piotr Sroka <[email protected]>
---
Changes for v2:
- file was created in v2. It was a part of driver source file patch.
- most delays were moved from dts file
to data associated with an SoC specific compatible
- description of delays was updated to be more clearly
---
Changes for v3:
- move all delays back to dts because they are also boards dependent
- prefix all of the Cadence-specific properties with cdns prefix
---
Changes for v4:
- change the beginning of the commit subject
---
.../devicetree/bindings/mmc/sdhci-cadence.txt | 48 ++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
index c0f37cb..c341820 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
@@ -19,6 +19,53 @@ if supported. See mmc.txt for details.
- mmc-hs400-1_8v
- mmc-hs400-1_2v
+Some PHY delays can be configured by following properties.
+PHY DLL input delays:
+They are used to delay the data valid window, and align the window
+to sampling clock. The delay starts from 5ns (for delay parameter equal to 0)
+and it is increased by 2.5ns in each step.
+- cdns,phy-input-delay-sd-highspeed:
+ Value of the delay in the input path for SD high-speed timing
+ Valid range = [0:0x1F].
+- cdns,phy-input-delay-legacy:
+ Value of the delay in the input path for SD legacy timing
+ Valid range = [0:0x1F].
+- cdns,phy-input-delay-sd-uhs-sdr12:
+ Value of the delay in the input path for SD UHS SDR12 timing
+ Valid range = [0:0x1F].
+- cdns,phy-input-delay-sd-uhs-sdr25:
+ Value of the delay in the input path for SD UHS SDR25 timing
+ Valid range = [0:0x1F].
+- cdns,phy-input-delay-sd-uhs-sdr50:
+ Value of the delay in the input path for SD UHS SDR50 timing
+ Valid range = [0:0x1F].
+- cdns,phy-input-delay-sd-uhs-ddr50:
+ Value of the delay in the input path for SD UHS DDR50 timing
+ Valid range = [0:0x1F].
+- cdns,phy-input-delay-mmc-highspeed:
+ Value of the delay in the input path for MMC high-speed timing
+ Valid range = [0:0x1F].
+- cdns,phy-input-delay-mmc-ddr:
+ Value of the delay in the input path for eMMC high-speed DDR timing
+ Valid range = [0:0x1F].
+
+PHY DLL clock delays:
+Each delay property represents the fraction of the clock period.
+The approximate delay value will be
+(<delay property value>/128)*sdmclk_clock_period.
+- cdns,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].
+- cdns,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].
+- cdns,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].
+
Example:
emmc: sdhci@5a000000 {
compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc";
@@ -29,4 +76,5 @@ Example:
mmc-ddr-1_8v;
mmc-hs200-1_8v;
mmc-hs400-1_8v;
+ cdns,phy-dll-delay-sdclk = <0>;
};
--
2.2.2
Hi Piotr,
2017-03-20 20:20 GMT+09:00 Piotr Sroka <[email protected]>:
> DTS properties are used instead of fixed data
> because PHY settings can be different for different chips/boards.
>
> Signed-off-by: Piotr Sroka <[email protected]>
I found this version is a problem for me.
> +
> +static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
> + { "cdns,phy-input-delay-sd-highspeed", SDHCI_CDNS_PHY_DLY_SD_HS, },
> + { "cdns,phy-input-delay-sd-legacy", SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
> + { "cdns,phy-input-delay-sd-uhs-sdr12", SDHCI_CDNS_PHY_DLY_UHS_SDR12, },
> + { "cdns,phy-input-delay-sd-uhs-sdr25", SDHCI_CDNS_PHY_DLY_UHS_SDR25, },
> + { "cdns,phy-input-delay-sd-uhs-sdr50", SDHCI_CDNS_PHY_DLY_UHS_SDR50, },
> + { "cdns,phy-input-delay-sd-uhs-ddr50", SDHCI_CDNS_PHY_DLY_UHS_DDR50, },
> + { "cdns,phy-input-delay-mmc-highspeed", SDHCI_CDNS_PHY_DLY_EMMC_SDR, },
> + { "cdns,phy-input-delay-mmc-ddr", SDHCI_CDNS_PHY_DLY_EMMC_DDR, },
> + { "cdns,phy-dll-delay-sdclk", SDHCI_CDNS_PHY_DLY_SDCLK, },
> + { "cdns,phy-dll-delay-sdclk-hsmmc", SDHCI_CDNS_PHY_DLY_HSMMC, },
> + { "cdns,phy-dll-delay-strobe", SDHCI_CDNS_PHY_DLY_STROBE, },
> +};
> +
I see mmc-legacy property in v1,
but it is missing now.
> static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> u8 addr, u8 data)
> {
> @@ -90,13 +113,26 @@ 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_init(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);
I need to set SDHCI_CDNS_PHY_DLY_EMMC_LEGACY to 9 for my SoC.
Maybe, do we need a DT property for this, too?
--
Best Regards
Masahiro Yamada
Hi Masahiro
2017-03-21 02:46 AM Masahiro Yamada <[email protected]>:
> Hi Piotr,
>
>
> 2017-03-20 20:20 GMT+09:00 Piotr Sroka <[email protected]>:
> > DTS properties are used instead of fixed data
> > because PHY settings can be different for different chips/boards.
> >
> > Signed-off-by: Piotr Sroka <[email protected]>
>
>
> I found this version is a problem for me.
>
>
> > +
> > +static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
> > + { "cdns,phy-input-delay-sd-highspeed", SDHCI_CDNS_PHY_DLY_SD_HS, },
> > + { "cdns,phy-input-delay-sd-legacy", SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
> > + { "cdns,phy-input-delay-sd-uhs-sdr12", SDHCI_CDNS_PHY_DLY_UHS_SDR12, },
> > + { "cdns,phy-input-delay-sd-uhs-sdr25", SDHCI_CDNS_PHY_DLY_UHS_SDR25, },
> > + { "cdns,phy-input-delay-sd-uhs-sdr50", SDHCI_CDNS_PHY_DLY_UHS_SDR50, },
> > + { "cdns,phy-input-delay-sd-uhs-ddr50", SDHCI_CDNS_PHY_DLY_UHS_DDR50, },
> > + { "cdns,phy-input-delay-mmc-highspeed", SDHCI_CDNS_PHY_DLY_EMMC_SDR, },
> > + { "cdns,phy-input-delay-mmc-ddr", SDHCI_CDNS_PHY_DLY_EMMC_DDR, },
> > + { "cdns,phy-dll-delay-sdclk", SDHCI_CDNS_PHY_DLY_SDCLK, },
> > + { "cdns,phy-dll-delay-sdclk-hsmmc", SDHCI_CDNS_PHY_DLY_HSMMC, },
> > + { "cdns,phy-dll-delay-strobe", SDHCI_CDNS_PHY_DLY_STROBE, },
> > +};
> > +
>
>
> I see mmc-legacy property in v1,
> but it is missing now.
>
>
> > static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> > u8 addr, u8 data)
> > {
> > @@ -90,13 +113,26 @@ 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_init(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);
>
>
> I need to set SDHCI_CDNS_PHY_DLY_EMMC_LEGACY to 9 for my SoC.
>
> Maybe, do we need a DT property for this, too?
>
I can add it but could you check if you realy need it? There is no selection MMC legacy mode
in this driver.
Best Regards
Piotr Sroka
Hi Piotr,
2017-03-21 16:01 GMT+09:00 Piotr Sroka <[email protected]>:
>
> Hi Masahiro
>
> 2017-03-21 02:46 AM Masahiro Yamada <[email protected]>:
>> Hi Piotr,
>>
>>
>> 2017-03-20 20:20 GMT+09:00 Piotr Sroka <[email protected]>:
>> > DTS properties are used instead of fixed data
>> > because PHY settings can be different for different chips/boards.
>> >
>> > Signed-off-by: Piotr Sroka <[email protected]>
>>
>>
>> I found this version is a problem for me.
>>
>>
>> > +
>> > +static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
>> > + { "cdns,phy-input-delay-sd-highspeed", SDHCI_CDNS_PHY_DLY_SD_HS, },
>> > + { "cdns,phy-input-delay-sd-legacy", SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
>> > + { "cdns,phy-input-delay-sd-uhs-sdr12", SDHCI_CDNS_PHY_DLY_UHS_SDR12, },
>> > + { "cdns,phy-input-delay-sd-uhs-sdr25", SDHCI_CDNS_PHY_DLY_UHS_SDR25, },
>> > + { "cdns,phy-input-delay-sd-uhs-sdr50", SDHCI_CDNS_PHY_DLY_UHS_SDR50, },
>> > + { "cdns,phy-input-delay-sd-uhs-ddr50", SDHCI_CDNS_PHY_DLY_UHS_DDR50, },
>> > + { "cdns,phy-input-delay-mmc-highspeed", SDHCI_CDNS_PHY_DLY_EMMC_SDR, },
>> > + { "cdns,phy-input-delay-mmc-ddr", SDHCI_CDNS_PHY_DLY_EMMC_DDR, },
>> > + { "cdns,phy-dll-delay-sdclk", SDHCI_CDNS_PHY_DLY_SDCLK, },
>> > + { "cdns,phy-dll-delay-sdclk-hsmmc", SDHCI_CDNS_PHY_DLY_HSMMC, },
>> > + { "cdns,phy-dll-delay-strobe", SDHCI_CDNS_PHY_DLY_STROBE, },
>> > +};
>> > +
>>
>>
>> I see mmc-legacy property in v1,
>> but it is missing now.
>>
>>
>> > static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
>> > u8 addr, u8 data)
>> > {
>> > @@ -90,13 +113,26 @@ 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_init(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);
>>
>>
>> I need to set SDHCI_CDNS_PHY_DLY_EMMC_LEGACY to 9 for my SoC.
>>
>> Maybe, do we need a DT property for this, too?
>>
> I can add it but could you check if you realy need it? There is no selection MMC legacy mode
> in this driver.
>
Ah, you are right.
For Linux, SD-Legacy and MMC-Legacy share the same timing flag.
The legacy mode is covered by SDHCI_CDNS_PHY_DLY_SD_DEFAULT,
so we need not to set SDHCI_CDNS_PHY_DLY_EMMC_LEGACY.
So, I have no problem with patch.
Reviewed-by: Masahiro Yamada <[email protected]>
>From here, just my minor comments.
The binding should not be too Linux-oriented.
Device Tree is hardware description language, and project-neutral from
its concept.
I wonder if there is a project that handles SD-Legacy and MMC-Legacy separately.
I am not sure about this, and I do not have a strong opinion, either.
I leave this to you (and other developers).
--
Best Regards
Masahiro Yamada
2017-03-20 20:20 GMT+09:00 Piotr Sroka <[email protected]>:
> DTS properties are used instead of fixed data
> because PHY settings can be different for different chips/boards.
> Add description of new DLL PHY delays.
>
> Signed-off-by: Piotr Sroka <[email protected]>
> ---
> Changes for v2:
> - file was created in v2. It was a part of driver source file patch.
> - most delays were moved from dts file
> to data associated with an SoC specific compatible
> - description of delays was updated to be more clearly
> ---
> Changes for v3:
> - move all delays back to dts because they are also boards dependent
> - prefix all of the Cadence-specific properties with cdns prefix
> ---
> Changes for v4:
> - change the beginning of the commit subject
> ---
Reviewed-by: Masahiro Yamada <[email protected]>
--
Best Regards
Masahiro Yamada
Hi Piotr,
2017-03-20 20:12 GMT+09:00 Piotr Sroka <[email protected]>:
> Add polling for ACK to be sure that data are written to PHY register.
>
> Signed-off-by: Piotr Sroka <[email protected]>
Reviewed-by: Masahiro Yamada <[email protected]>
One you get Acked-by or Reviewed-by,
please make sure to add it in the next version.
I issued my Reviewed-by multiple times
(because maintainers usually pick up the latest version).
--
Best Regards
Masahiro Yamada
Hi Masahiro
2017-03-21 08:3 AM Masahiro Yamada <[email protected]>:
> Hi Piotr,
>
> 2017-03-21 16:01 GMT+09:00 Piotr Sroka <[email protected]>:
> >
> > Hi Masahiro
> >
> > 2017-03-21 02:46 AM Masahiro Yamada <[email protected]>:
> >> Hi Piotr,
> >>
> >>
> >> 2017-03-20 20:20 GMT+09:00 Piotr Sroka <[email protected]>:
> >> > DTS properties are used instead of fixed data
> >> > because PHY settings can be different for different chips/boards.
> >> >
> >> > Signed-off-by: Piotr Sroka <[email protected]>
> >>
> >>
> >> I found this version is a problem for me.
> >>
> >>
> >> > +
> >> > +static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
> >> > + { "cdns,phy-input-delay-sd-highspeed", SDHCI_CDNS_PHY_DLY_SD_HS, },
> >> > + { "cdns,phy-input-delay-sd-legacy", SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
> >> > + { "cdns,phy-input-delay-sd-uhs-sdr12", SDHCI_CDNS_PHY_DLY_UHS_SDR12, },
> >> > + { "cdns,phy-input-delay-sd-uhs-sdr25", SDHCI_CDNS_PHY_DLY_UHS_SDR25, },
> >> > + { "cdns,phy-input-delay-sd-uhs-sdr50", SDHCI_CDNS_PHY_DLY_UHS_SDR50, },
> >> > + { "cdns,phy-input-delay-sd-uhs-ddr50", SDHCI_CDNS_PHY_DLY_UHS_DDR50, },
> >> > + { "cdns,phy-input-delay-mmc-highspeed", SDHCI_CDNS_PHY_DLY_EMMC_SDR, },
> >> > + { "cdns,phy-input-delay-mmc-ddr", SDHCI_CDNS_PHY_DLY_EMMC_DDR, },
> >> > + { "cdns,phy-dll-delay-sdclk", SDHCI_CDNS_PHY_DLY_SDCLK, },
> >> > + { "cdns,phy-dll-delay-sdclk-hsmmc", SDHCI_CDNS_PHY_DLY_HSMMC, },
> >> > + { "cdns,phy-dll-delay-strobe", SDHCI_CDNS_PHY_DLY_STROBE, },
> >> > +};
> >> > +
> >>
> >>
> >> I see mmc-legacy property in v1,
> >> but it is missing now.
> >>
> >>
> >> > static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> >> > u8 addr, u8 data)
> >> > {
> >> > @@ -90,13 +113,26 @@ 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_init(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);
> >>
> >>
> >> I need to set SDHCI_CDNS_PHY_DLY_EMMC_LEGACY to 9 for my SoC.
> >>
> >> Maybe, do we need a DT property for this, too?
> >>
> > I can add it but could you check if you realy need it? There is no selection MMC legacy mode
> > in this driver.
> >
>
> Ah, you are right.
>
> For Linux, SD-Legacy and MMC-Legacy share the same timing flag.
> The legacy mode is covered by SDHCI_CDNS_PHY_DLY_SD_DEFAULT,
> so we need not to set SDHCI_CDNS_PHY_DLY_EMMC_LEGACY.
>
> So, I have no problem with patch.
>
> Reviewed-by: Masahiro Yamada <[email protected]>
>
>
>
>
> From here, just my minor comments.
> The binding should not be too Linux-oriented.
> Device Tree is hardware description language, and project-neutral from
> its concept.
> I wonder if there is a project that handles SD-Legacy and MMC-Legacy separately.
> I am not sure about this, and I do not have a strong opinion, either.
>
> I leave this to you (and other developers).
>
Thanks for your comments.
Because patch adding HS400 enhanced strobe support was applied
for next branch I needed to create v5. BTW I added one extra patch modifing
probe function as you suggested.
I also modify binding to be consistent with linux MMC timing names.
The driver does not use MMC legacy at all so I think there is no need
to distinguish between SD Legacy and MMC Legacy. So there are no contraindications
for being consistent with Linux.
Best Regards
Piotr Sroka
2017-03-22 16:08 GMT+09:00 Piotr Sroka <[email protected]>:
> Hi Masahiro
>
> 2017-03-21 08:3 AM Masahiro Yamada <[email protected]>:
>> Hi Piotr,
>>
>> 2017-03-21 16:01 GMT+09:00 Piotr Sroka <[email protected]>:
>> >
>> > Hi Masahiro
>> >
>> > 2017-03-21 02:46 AM Masahiro Yamada <[email protected]>:
>> >> Hi Piotr,
>> >>
>> >>
>> >> 2017-03-20 20:20 GMT+09:00 Piotr Sroka <[email protected]>:
>> >> > DTS properties are used instead of fixed data
>> >> > because PHY settings can be different for different chips/boards.
>> >> >
>> >> > Signed-off-by: Piotr Sroka <[email protected]>
>> >>
>> >>
>> >> I found this version is a problem for me.
>> >>
>> >>
>> >> > +
>> >> > +static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
>> >> > + { "cdns,phy-input-delay-sd-highspeed", SDHCI_CDNS_PHY_DLY_SD_HS, },
>> >> > + { "cdns,phy-input-delay-sd-legacy", SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
>> >> > + { "cdns,phy-input-delay-sd-uhs-sdr12", SDHCI_CDNS_PHY_DLY_UHS_SDR12, },
>> >> > + { "cdns,phy-input-delay-sd-uhs-sdr25", SDHCI_CDNS_PHY_DLY_UHS_SDR25, },
>> >> > + { "cdns,phy-input-delay-sd-uhs-sdr50", SDHCI_CDNS_PHY_DLY_UHS_SDR50, },
>> >> > + { "cdns,phy-input-delay-sd-uhs-ddr50", SDHCI_CDNS_PHY_DLY_UHS_DDR50, },
>> >> > + { "cdns,phy-input-delay-mmc-highspeed", SDHCI_CDNS_PHY_DLY_EMMC_SDR, },
>> >> > + { "cdns,phy-input-delay-mmc-ddr", SDHCI_CDNS_PHY_DLY_EMMC_DDR, },
>> >> > + { "cdns,phy-dll-delay-sdclk", SDHCI_CDNS_PHY_DLY_SDCLK, },
>> >> > + { "cdns,phy-dll-delay-sdclk-hsmmc", SDHCI_CDNS_PHY_DLY_HSMMC, },
>> >> > + { "cdns,phy-dll-delay-strobe", SDHCI_CDNS_PHY_DLY_STROBE, },
>> >> > +};
>> >> > +
>> >>
>> >>
>> >> I see mmc-legacy property in v1,
>> >> but it is missing now.
>> >>
>> >>
>> >> > static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
>> >> > u8 addr, u8 data)
>> >> > {
>> >> > @@ -90,13 +113,26 @@ 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_init(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);
>> >>
>> >>
>> >> I need to set SDHCI_CDNS_PHY_DLY_EMMC_LEGACY to 9 for my SoC.
>> >>
>> >> Maybe, do we need a DT property for this, too?
>> >>
>> > I can add it but could you check if you realy need it? There is no selection MMC legacy mode
>> > in this driver.
>> >
>>
>> Ah, you are right.
>>
>> For Linux, SD-Legacy and MMC-Legacy share the same timing flag.
>> The legacy mode is covered by SDHCI_CDNS_PHY_DLY_SD_DEFAULT,
>> so we need not to set SDHCI_CDNS_PHY_DLY_EMMC_LEGACY.
>>
>> So, I have no problem with patch.
>>
>> Reviewed-by: Masahiro Yamada <[email protected]>
>>
>>
>>
>>
>> From here, just my minor comments.
>> The binding should not be too Linux-oriented.
>> Device Tree is hardware description language, and project-neutral from
>> its concept.
>> I wonder if there is a project that handles SD-Legacy and MMC-Legacy separately.
>> I am not sure about this, and I do not have a strong opinion, either.
>>
>> I leave this to you (and other developers).
>>
>
> Thanks for your comments.
>
> Because patch adding HS400 enhanced strobe support was applied
> for next branch I needed to create v5. BTW I added one extra patch modifing
> probe function as you suggested.
> I also modify binding to be consistent with linux MMC timing names.
> The driver does not use MMC legacy at all so I think there is no need
> to distinguish between SD Legacy and MMC Legacy. So there are no contraindications
> for being consistent with Linux.
OK. Make sense.
--
Best Regards
Masahiro Yamada