Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1604409 > unrolled thread

[v4 3/3] mmc: sdhci-cadence: Update PHY delay configuration

Started byPiotr Sroka <piotrs@cadence.com>
First post2017-03-20 12:30 +0100
Last post2017-03-22 08:30 +0100
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [v4 3/3] mmc: sdhci-cadence: Update PHY delay configuration Piotr Sroka <piotrs@cadence.com> - 2017-03-20 12:30 +0100
    Re: [v4 3/3] mmc: sdhci-cadence: Update PHY delay configuration Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-21 02:50 +0100
      RE: [v4 3/3] mmc: sdhci-cadence: Update PHY delay configuration Piotr Sroka <piotrs@cadence.com> - 2017-03-21 08:10 +0100
        Re: [v4 3/3] mmc: sdhci-cadence: Update PHY delay configuration Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-21 08:40 +0100
          RE: [v4 3/3] mmc: sdhci-cadence: Update PHY delay configuration Piotr Sroka <piotrs@cadence.com> - 2017-03-22 08:10 +0100
            Re: [v4 3/3] mmc: sdhci-cadence: Update PHY delay configuration Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 08:30 +0100

#1604409 — [v4 3/3] mmc: sdhci-cadence: Update PHY delay configuration

FromPiotr Sroka <piotrs@cadence.com>
Date2017-03-20 12:30 +0100
Subject[v4 3/3] mmc: sdhci-cadence: Update PHY delay configuration
Message-ID<tn3FE-3Z3-19@gated-at.bofh.it>
DTS properties are used instead of fixed data
because PHY settings can be different for different chips/boards.

Signed-off-by: Piotr Sroka <piotrs@cadence.com>
---
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

[toc] | [next] | [standalone]

#1605246

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-03-21 02:50 +0100
Message-ID<tnh5T-4MQ-1@gated-at.bofh.it>
In reply to#1604409
Hi Piotr,


2017-03-20 20:20 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
> DTS properties are used instead of fixed data
> because PHY settings can be different for different chips/boards.
>
> Signed-off-by: Piotr Sroka <piotrs@cadence.com>


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

[toc] | [prev] | [next] | [standalone]

#1605352

FromPiotr Sroka <piotrs@cadence.com>
Date2017-03-21 08:10 +0100
Message-ID<tnm5z-8oE-11@gated-at.bofh.it>
In reply to#1605246
Hi Masahiro 

2017-03-21 02:46 AM Masahiro Yamada <yamada.masahiro@socionext.com>:
> Hi Piotr,
> 
> 
> 2017-03-20 20:20 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
> > DTS properties are used instead of fixed data
> > because PHY settings can be different for different chips/boards.
> >
> > Signed-off-by: Piotr Sroka <piotrs@cadence.com>
> 
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1605369

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-03-21 08:40 +0100
Message-ID<tnmyC-8M-19@gated-at.bofh.it>
In reply to#1605352
Hi Piotr,

2017-03-21 16:01 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
>
> Hi Masahiro
>
> 2017-03-21 02:46 AM Masahiro Yamada <yamada.masahiro@socionext.com>:
>> Hi Piotr,
>>
>>
>> 2017-03-20 20:20 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
>> > DTS properties are used instead of fixed data
>> > because PHY settings can be different for different chips/boards.
>> >
>> > Signed-off-by: Piotr Sroka <piotrs@cadence.com>
>>
>>
>> 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 <yamada.masahiro@socionext.com>




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

[toc] | [prev] | [next] | [standalone]

#1606246

FromPiotr Sroka <piotrs@cadence.com>
Date2017-03-22 08:10 +0100
Message-ID<tnIz7-74x-3@gated-at.bofh.it>
In reply to#1605369
Hi Masahiro

2017-03-21 08:3 AM  Masahiro Yamada <yamada.masahiro@socionext.com>:
> Hi Piotr,
> 
> 2017-03-21 16:01 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
> >
> > Hi Masahiro
> >
> > 2017-03-21 02:46 AM Masahiro Yamada <yamada.masahiro@socionext.com>:
> >> Hi Piotr,
> >>
> >>
> >> 2017-03-20 20:20 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
> >> > DTS properties are used instead of fixed data
> >> > because PHY settings can be different for different chips/boards.
> >> >
> >> > Signed-off-by: Piotr Sroka <piotrs@cadence.com>
> >>
> >>
> >> 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 <yamada.masahiro@socionext.com>
> 
> 
> 
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1606252

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-03-22 08:30 +0100
Message-ID<tnISu-7c5-19@gated-at.bofh.it>
In reply to#1606246
2017-03-22 16:08 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
> Hi Masahiro
>
> 2017-03-21 08:3 AM  Masahiro Yamada <yamada.masahiro@socionext.com>:
>> Hi Piotr,
>>
>> 2017-03-21 16:01 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
>> >
>> > Hi Masahiro
>> >
>> > 2017-03-21 02:46 AM Masahiro Yamada <yamada.masahiro@socionext.com>:
>> >> Hi Piotr,
>> >>
>> >>
>> >> 2017-03-20 20:20 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
>> >> > DTS properties are used instead of fixed data
>> >> > because PHY settings can be different for different chips/boards.
>> >> >
>> >> > Signed-off-by: Piotr Sroka <piotrs@cadence.com>
>> >>
>> >>
>> >> 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 <yamada.masahiro@socionext.com>
>>
>>
>>
>>
>> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web