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

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

[Piotr Sroka]

PHY settings can be different for different platforms and SoCs.
Fixed PHY input delays was replaced with SoC specific compatible data.
DTS properties are used for configuration new PHY DLL delays.

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
---

 drivers/mmc/host/sdhci-cadence.c | 124 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 116 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index b2334ec..29b5d11 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
@@ -62,10 +66,24 @@
  */
 #define SDHCI_CDNS_MAX_TUNING_LOOP	40
 
+static const struct of_device_id sdhci_cdns_match[];
+
 struct sdhci_cdns_priv {
 	void __iomem *hrs_addr;
 };
 
+struct sdhci_cdns_config {
+	u8 phy_dly_sd_highspeed;
+	u8 phy_dly_sd_legacy;
+	u8 phy_dly_sd_uhs_sdr12;
+	u8 phy_dly_sd_uhs_sdr25;
+	u8 phy_dly_sd_uhs_sdr50;
+	u8 phy_dly_sd_uhs_ddr50;
+	u8 phy_dly_emmc_legacy;
+	u8 phy_dly_emmc_sdr;
+	u8 phy_dly_emmc_ddr;
+};
+
 static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
 				    u8 addr, u8 data)
 {
@@ -90,13 +108,77 @@ 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_in_delay_init(struct sdhci_cdns_priv *priv,
+					const struct sdhci_cdns_config *config)
+{
+	int ret = 0;
+
+	ret = sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_HS,
+				       config->phy_dly_sd_highspeed);
+	if (ret)
+		return ret;
+	ret = sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_DEFAULT,
+				       config->phy_dly_sd_legacy);
+	if (ret)
+		return ret;
+	ret = sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_UHS_SDR12,
+				       config->phy_dly_sd_uhs_sdr12);
+	if (ret)
+		return ret;
+	ret = sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_UHS_SDR25,
+				       config->phy_dly_sd_uhs_sdr25);
+	if (ret)
+		return ret;
+	ret = sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_UHS_SDR50,
+				       config->phy_dly_sd_uhs_sdr50);
+	if (ret)
+		return ret;
+	ret = sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_UHS_DDR50,
+				       config->phy_dly_sd_uhs_ddr50);
+	if (ret)
+		return ret;
+	ret = sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_LEGACY,
+				       config->phy_dly_emmc_legacy);
+	if (ret)
+		return ret;
+	ret = sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_SDR,
+				       config->phy_dly_emmc_sdr);
+	if (ret)
+		return ret;
+	ret = sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_DDR,
+				       config->phy_dly_emmc_ddr);
+	if (ret)
+		return ret;
+	return 0;
+}
+
+static int sdhci_cdns_phy_dll_delay_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-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)
@@ -227,6 +309,8 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
 	struct sdhci_cdns_priv *priv;
 	struct clk *clk;
 	int ret;
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *match;
 
 	clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk))
@@ -254,7 +338,16 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
 	if (ret)
 		goto free;
 
-	sdhci_cdns_phy_init(priv);
+	match = of_match_node(sdhci_cdns_match, dev->of_node);
+	if (match && match->data) {
+		ret = sdhci_cdns_phy_in_delay_init(priv, match->data);
+		if (ret)
+			goto free;
+	}
+
+	ret = sdhci_cdns_phy_dll_delay_parse_dt(dev->of_node, priv);
+	if (ret)
+		goto free;
 
 	ret = sdhci_add_host(host);
 	if (ret)
@@ -269,8 +362,23 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static const struct sdhci_cdns_config sdhci_cdns_uniphier_config = {
+	.phy_dly_sd_highspeed = 4,
+	.phy_dly_sd_legacy = 4,
+	.phy_dly_sd_uhs_sdr12 = 0,
+	.phy_dly_sd_uhs_sdr25 = 0,
+	.phy_dly_sd_uhs_sdr50 = 0,
+	.phy_dly_sd_uhs_ddr50 = 0,
+	.phy_dly_emmc_legacy = 9,
+	.phy_dly_emmc_sdr = 2,
+	.phy_dly_emmc_ddr = 3,
+};
+
 static const struct of_device_id sdhci_cdns_match[] = {
-	{ .compatible = "socionext,uniphier-sd4hc" },
+	{
+		.compatible = "socionext,uniphier-sd4hc",
+		.data = &sdhci_cdns_uniphier_config
+	},
 	{ .compatible = "cdns,sd4hc" },
 	{ /* sentinel */ }
 };
-- 
2.2.2

Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration

[Masahiro Yamada]

Hi Piotr,

2017-03-06 22:39 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
> PHY settings can be different for different platforms and SoCs.
> Fixed PHY input delays was replaced with SoC specific compatible data.
> DTS properties are used for configuration new PHY DLL delays.


Probably you are familiar with this IP.

Please teach me this.

With this patch, we will have two groups for PHY parameters.

(A) specified via a data array associated with a compatible string
SDHCI_CDNS_PHY_DLY_SD_HS
SDHCI_CDNS_PHY_DLY_SD_DEFAULT
SDHCI_CDNS_PHY_DLY_UHS_SDR12
SDHCI_CDNS_PHY_DLY_UHS_SDR25
SDHCI_CDNS_PHY_DLY_UHS_SDR50
SDHCI_CDNS_PHY_DLY_UHS_DDR50
SDHCI_CDNS_PHY_DLY_EMMC_LEGACY
SDHCI_CDNS_PHY_DLY_EMMC_SDR
SDHCI_CDNS_PHY_DLY_EMMC_DDR

(B) specified with DT property
SDHCI_CDNS_PHY_DLY_SDCLK
SDHCI_CDNS_PHY_DLY_HSMMC
SDHCI_CDNS_PHY_DLY_STROBE


I am confused.
What is the difference between (A) and (B)?


A little more minor issues below.


>
>  /*
>   * The tuned val register is 6 bit-wide, but not the whole of the range is
> @@ -62,10 +66,24 @@
>   */
>  #define SDHCI_CDNS_MAX_TUNING_LOOP     40
>
> +static const struct of_device_id sdhci_cdns_match[];
> +

You need not add this declaration
if you use of_device_get_match_data().
(please see below)





> @@ -90,13 +108,77 @@ 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_in_delay_init(struct sdhci_cdns_priv *priv,
> +                                       const struct sdhci_cdns_config *config)
> +{
> +       int ret = 0;


The initializer "= 0" is unneeded here
because the variable is re-assigned with a return value of functions.




> @@ -254,7 +338,16 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
>         if (ret)
>                 goto free;
>
> -       sdhci_cdns_phy_init(priv);
> +       match = of_match_node(sdhci_cdns_match, dev->of_node);
> +       if (match && match->data) {
> +               ret = sdhci_cdns_phy_in_delay_init(priv, match->data);
> +               if (ret)
> +                       goto free;
> +       }

You can simplify the code with of_device_get_match_data():

     config = of_device_get_match_data(dev);
     if (config) {
             ret = sdhci_cdns_phy_in_delay_init(priv, config);
             if (ret)
                     goto free;
     }




>  static const struct of_device_id sdhci_cdns_match[] = {
> -       { .compatible = "socionext,uniphier-sd4hc" },
> +       {
> +               .compatible = "socionext,uniphier-sd4hc",
> +               .data = &sdhci_cdns_uniphier_config
> +       },
>         { .compatible = "cdns,sd4hc" },


I prefer cdns,sd4hc indented in the same way:

          {
                  .compatible = "cdns,sd4hc",
          },





-- 
Best Regards
Masahiro Yamada

RE: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration

[Piotr Sroka]

Hi Masahiro,

> -----Original Message-----
> Sent: 07 March, 2017 9:03 AM
> To: Piotr Sroka
> Subject: Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration
> 
> Hi Piotr,
> 
> 2017-03-06 22:39 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
> > PHY settings can be different for different platforms and SoCs.
> > Fixed PHY input delays was replaced with SoC specific compatible data.
> > DTS properties are used for configuration new PHY DLL delays.
> 
> 
> Probably you are familiar with this IP.
> 
> Please teach me this.
> 
> With this patch, we will have two groups for PHY parameters.
> 
> (A) specified via a data array associated with a compatible string SDHCI_CDNS_PHY_DLY_SD_HS SDHCI_CDNS_PHY_DLY_SD_DEFAULT
> SDHCI_CDNS_PHY_DLY_UHS_SDR12
> SDHCI_CDNS_PHY_DLY_UHS_SDR25
> SDHCI_CDNS_PHY_DLY_UHS_SDR50
> SDHCI_CDNS_PHY_DLY_UHS_DDR50
> SDHCI_CDNS_PHY_DLY_EMMC_LEGACY
> SDHCI_CDNS_PHY_DLY_EMMC_SDR
> SDHCI_CDNS_PHY_DLY_EMMC_DDR
> 
> (B) specified with DT property
> SDHCI_CDNS_PHY_DLY_SDCLK
> SDHCI_CDNS_PHY_DLY_HSMMC
> SDHCI_CDNS_PHY_DLY_STROBE
> 
> I am confused.
> What is the difference between (A) and (B)?

The first group of delays are input delays. These delays are set in current version of sdhci-cadence driver in sdhci_cdns_phy_init function.
Following by spec:
They are provided to help in meeting timings relations between data window and sampling clock. 
The clock is fixed position in respect to the SDCLK. And the idea of sampling is to delay and align the data to the data window.
If the default values of the delays are not sufficient/correct for the chip/board implementation those can be adjusted

The second group are DLL delays.
There are three delays
SDHCI_CDNS_PHY_DLY_SDCLK  - sdclk delay line use to delay outgoing sdclk signal
SDHCI_CDNS_PHY_DLY_HSMMC - sdclk delay line use to delay outgoing sdclk signal for for HS200, HS400 and HS400ES
SDHCI_CDNS_PHY_DLY_STROBE - DLL strobe delay for HS400ES
Following by spec:
They allows to setup basic DLL parameters. In general the default values are sufficient to start working in any speed mode. The default values of delays and phase detect select can be adjusted depending on the chip/board implementation. 

In general all PHY delays values either should be properly hardcoded in HW or they should be properly set  by FW depending on the chip/board.
So PHY driver should do not touch PHY delays at all or should set values which are proper for specific chip/board.

I am not sure where exactly they should be placed in dts file or in compatible data. 
  
Best Regards
Piotr Sroka

Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration

[Masahiro Yamada]

Hi Piotr,

2017-03-07 20:00 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
> Hi Masahiro,
>
>> -----Original Message-----
>> Sent: 07 March, 2017 9:03 AM
>> To: Piotr Sroka
>> Subject: Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration
>>
>> Hi Piotr,
>>
>> 2017-03-06 22:39 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
>> > PHY settings can be different for different platforms and SoCs.
>> > Fixed PHY input delays was replaced with SoC specific compatible data.
>> > DTS properties are used for configuration new PHY DLL delays.
>>
>>
>> Probably you are familiar with this IP.
>>
>> Please teach me this.
>>
>> With this patch, we will have two groups for PHY parameters.
>>
>> (A) specified via a data array associated with a compatible string SDHCI_CDNS_PHY_DLY_SD_HS SDHCI_CDNS_PHY_DLY_SD_DEFAULT
>> SDHCI_CDNS_PHY_DLY_UHS_SDR12
>> SDHCI_CDNS_PHY_DLY_UHS_SDR25
>> SDHCI_CDNS_PHY_DLY_UHS_SDR50
>> SDHCI_CDNS_PHY_DLY_UHS_DDR50
>> SDHCI_CDNS_PHY_DLY_EMMC_LEGACY
>> SDHCI_CDNS_PHY_DLY_EMMC_SDR
>> SDHCI_CDNS_PHY_DLY_EMMC_DDR
>>
>> (B) specified with DT property
>> SDHCI_CDNS_PHY_DLY_SDCLK
>> SDHCI_CDNS_PHY_DLY_HSMMC
>> SDHCI_CDNS_PHY_DLY_STROBE
>>
>> I am confused.
>> What is the difference between (A) and (B)?
>
> The first group of delays are input delays. These delays are set in current version of sdhci-cadence driver in sdhci_cdns_phy_init function.
> Following by spec:
> They are provided to help in meeting timings relations between data window and sampling clock.
> The clock is fixed position in respect to the SDCLK. And the idea of sampling is to delay and align the data to the data window.
> If the default values of the delays are not sufficient/correct for the chip/board implementation those can be adjusted
>
> The second group are DLL delays.
> There are three delays
> SDHCI_CDNS_PHY_DLY_SDCLK  - sdclk delay line use to delay outgoing sdclk signal
> SDHCI_CDNS_PHY_DLY_HSMMC - sdclk delay line use to delay outgoing sdclk signal for for HS200, HS400 and HS400ES
> SDHCI_CDNS_PHY_DLY_STROBE - DLL strobe delay for HS400ES
> Following by spec:
> They allows to setup basic DLL parameters. In general the default values are sufficient to start working in any speed mode. The default values of delays and phase detect select can be adjusted depending on the chip/board implementation.



It was not clear what makes one group different from the other.

After all, parameters from both groups
should be adjusted depending on chip/board implementation.


> In general all PHY delays values either should be properly hardcoded in HW or they should be properly set  by FW depending on the chip/board.
> So PHY driver should do not touch PHY delays at all or should set values which are proper for specific chip/board.
>
> I am not sure where exactly they should be placed in dts file or in compatible data.

I am not quite sure, either.
(comments are appreciated.)


FWIW:
The first group (data associated with compatible) allows per-chip adjustment,
but not per-board.   Pros are, we will not break DT compatibility,
and we can avoid a list of properties difficult to understand.
Cons are, we can not make fine-grained adjustment for each board.


The second group (DT property) gives more flexibility for per-chip and
per-board adjustment.
A bad thing is we will end up with specifying a bunch of mysterious
properties from DT.





-- 
Best Regards
Masahiro Yamada

RE: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration

[Piotr Sroka]

Hi Masahiro

> -----Original Message-----
> From: Masahiro Yamada [mailto:yamada.masahiro@socionext.com]
> Sent: 09 March, 2017 3:37 AM
> Subject: Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration
> 
> Hi Piotr,
> 
> 2017-03-07 20:00 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
> > Hi Masahiro,
> >
> >> -----Original Message-----
> >> Sent: 07 March, 2017 9:03 AM
> >> To: Piotr Sroka
> >> Subject: Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay
> >> configuration
> >>
> >>
> >> With this patch, we will have two groups for PHY parameters.
> >>
> >> (A) specified via a data array associated with a compatible string
> >>
> >> (B) specified with DT property
> >>
> >> I am confused.
> >> What is the difference between (A) and (B)?
> >
> > The first group of delays are input delays. These delays are set in current version of sdhci-cadence driver in sdhci_cdns_phy_init
> function.
> > Following by spec:
> > They are provided to help in meeting timings relations between data window and sampling clock.
> > The clock is fixed position in respect to the SDCLK. And the idea of sampling is to delay and align the data to the data window.
> > If the default values of the delays are not sufficient/correct for the
> > chip/board implementation those can be adjusted
> >
> > The second group are DLL delays.
> > There are three delays
> > SDHCI_CDNS_PHY_DLY_SDCLK  - sdclk delay line use to delay outgoing
> > sdclk signal SDHCI_CDNS_PHY_DLY_HSMMC - sdclk delay line use to delay
> > outgoing sdclk signal for for HS200, HS400 and HS400ES
> > SDHCI_CDNS_PHY_DLY_STROBE - DLL strobe delay for HS400ES Following by spec:
> > They allows to setup basic DLL parameters. In general the default values are sufficient to start working in any speed mode. The
> default values of delays and phase detect select can be adjusted depending on the chip/board implementation.
> 
> 
> 
> It was not clear what makes one group different from the other.
> 
> After all, parameters from both groups
> should be adjusted depending on chip/board implementation.
> 
> 
> > In general all PHY delays values either should be properly hardcoded in HW or they should be properly set  by FW depending on the
> chip/board.
> > So PHY driver should do not touch PHY delays at all or should set values which are proper for specific chip/board.
> >
> > I am not sure where exactly they should be placed in dts file or in compatible data.
> 
> I am not quite sure, either.
> (comments are appreciated.)
> 
> 
> FWIW:
> The first group (data associated with compatible) allows per-chip adjustment,
> but not per-board.   Pros are, we will not break DT compatibility,
> and we can avoid a list of properties difficult to understand.
> Cons are, we can not make fine-grained adjustment for each board.
> 
> 
> The second group (DT property) gives more flexibility for per-chip and per-board adjustment.
> A bad thing is we will end up with specifying a bunch of mysterious properties from DT.
> 
> 

It looks that "input delays" and "DLL sdclk delays" should be defined in dts file because they depend on a chip and a board implementation. On the other hand the less dts properties the better.

There is one more way to handle input delays. It can be achieved by PHY training. PHY training is similar to the tuning and it should be done when proper timing mode is selected and clock frequency is set.
To make it possible the sdhci_set_ios function need to be global. Then I could create sdhci_cdns_set_ios function as follows:
void sdhci_cdns_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
{
	. . .

	sdhci_set_ios(mmc, ios);
	/* execute PHY training if needed */
	sdhci_cdns_exec_phy_training(host);
}

The mmc framework configures timing and frequency separately so PHY training should be executed every time if timing or clock frequency is changed. I am not sure If I can change sdhci_set_ios to global function.
So maybe put all delays to dts file would be a better solution? What do you think?

Best Regards
Piotr Sroka

Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration

[Masahiro Yamada]

Hi Piotr,

Sorry for my late reply.


2017-03-13 16:57 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
> Hi Masahiro
>
>> -----Original Message-----
>> From: Masahiro Yamada [mailto:yamada.masahiro@socionext.com]
>> Sent: 09 March, 2017 3:37 AM
>> Subject: Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration
>>
>> Hi Piotr,
>>
>> 2017-03-07 20:00 GMT+09:00 Piotr Sroka <piotrs@cadence.com>:
>> > Hi Masahiro,
>> >
>> >> -----Original Message-----
>> >> Sent: 07 March, 2017 9:03 AM
>> >> To: Piotr Sroka
>> >> Subject: Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay
>> >> configuration
>> >>
>> >>
>> >> With this patch, we will have two groups for PHY parameters.
>> >>
>> >> (A) specified via a data array associated with a compatible string
>> >>
>> >> (B) specified with DT property
>> >>
>> >> I am confused.
>> >> What is the difference between (A) and (B)?
>> >
>> > The first group of delays are input delays. These delays are set in current version of sdhci-cadence driver in sdhci_cdns_phy_init
>> function.
>> > Following by spec:
>> > They are provided to help in meeting timings relations between data window and sampling clock.
>> > The clock is fixed position in respect to the SDCLK. And the idea of sampling is to delay and align the data to the data window.
>> > If the default values of the delays are not sufficient/correct for the
>> > chip/board implementation those can be adjusted
>> >
>> > The second group are DLL delays.
>> > There are three delays
>> > SDHCI_CDNS_PHY_DLY_SDCLK  - sdclk delay line use to delay outgoing
>> > sdclk signal SDHCI_CDNS_PHY_DLY_HSMMC - sdclk delay line use to delay
>> > outgoing sdclk signal for for HS200, HS400 and HS400ES
>> > SDHCI_CDNS_PHY_DLY_STROBE - DLL strobe delay for HS400ES Following by spec:
>> > They allows to setup basic DLL parameters. In general the default values are sufficient to start working in any speed mode. The
>> default values of delays and phase detect select can be adjusted depending on the chip/board implementation.
>>
>>
>>
>> It was not clear what makes one group different from the other.
>>
>> After all, parameters from both groups
>> should be adjusted depending on chip/board implementation.
>>
>>
>> > In general all PHY delays values either should be properly hardcoded in HW or they should be properly set  by FW depending on the
>> chip/board.
>> > So PHY driver should do not touch PHY delays at all or should set values which are proper for specific chip/board.
>> >
>> > I am not sure where exactly they should be placed in dts file or in compatible data.
>>
>> I am not quite sure, either.
>> (comments are appreciated.)
>>
>>
>> FWIW:
>> The first group (data associated with compatible) allows per-chip adjustment,
>> but not per-board.   Pros are, we will not break DT compatibility,
>> and we can avoid a list of properties difficult to understand.
>> Cons are, we can not make fine-grained adjustment for each board.
>>
>>
>> The second group (DT property) gives more flexibility for per-chip and per-board adjustment.
>> A bad thing is we will end up with specifying a bunch of mysterious properties from DT.
>>
>>
>
> It looks that "input delays" and "DLL sdclk delays" should be defined in dts file because they depend on a chip and a board implementation. On the other hand the less dts properties the better.
>
> There is one more way to handle input delays. It can be achieved by PHY training. PHY training is similar to the tuning and it should be done when proper timing mode is selected and clock frequency is set.
> To make it possible the sdhci_set_ios function need to be global. Then I could create sdhci_cdns_set_ios function as follows:
> void sdhci_cdns_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> {
>         . . .
>
>         sdhci_set_ios(mmc, ios);
>         /* execute PHY training if needed */
>         sdhci_cdns_exec_phy_training(host);
> }
>
> The mmc framework configures timing and frequency separately so PHY training should be executed every time if timing or clock frequency is changed. I am not sure If I can change sdhci_set_ios to global function.


I am OK with this, but I hope Adrian can advise us.




> So maybe put all delays to dts file would be a better solution? What do you think?

I am OK with DT approach too
because this way seems simpler, after all.

(My suggestion for data array approach was misleading, sorry.)





-- 
Best Regards
Masahiro Yamada

RE: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration

[Piotr Sroka]

> -----Original Message-----
> From: Masahiro Yamada [mailto:yamada.masahiro@socionext.com]
> Sent: 17 March, 2017 6:23 PM
> Subject: Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration
> 
> Hi Piotr,
> 
> Sorry for my late reply.
> 
> 
> >
> > It looks that "input delays" and "DLL sdclk delays" should be defined in dts file because they depend on a chip and a board
> implementation. On the other hand the less dts properties the better.
> >
> > There is one more way to handle input delays. It can be achieved by PHY training. PHY training is similar to the tuning and it should be
> done when proper timing mode is selected and clock frequency is set.
> > To make it possible the sdhci_set_ios function need to be global. Then I could create sdhci_cdns_set_ios function as follows:
> > void sdhci_cdns_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) {
> >         . . .
> >
> >         sdhci_set_ios(mmc, ios);
> >         /* execute PHY training if needed */
> >         sdhci_cdns_exec_phy_training(host);
> > }
> >
> > The mmc framework configures timing and frequency separately so PHY training should be executed every time if timing or clock
> frequency is changed. I am not sure If I can change sdhci_set_ios to global function.
> 
> 
> I am OK with this, but I hope Adrian can advise us.
> 
> 
> 
> 
> > So maybe put all delays to dts file would be a better solution? What do you think?
> 
> I am OK with DT approach too
> because this way seems simpler, after all.
> 
> (My suggestion for data array approach was misleading, sorry.)
> 
Thanks for review anyway it was useful. Now decision between DTS and data array is more clear for me. 

Regards 
Piotr Sroka

Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration

[Adrian Hunter]

On 20/03/17 10:38, Piotr Sroka wrote:
> 
> 
>> -----Original Message-----
>> From: Masahiro Yamada [mailto:yamada.masahiro@socionext.com]
>> Sent: 17 March, 2017 6:23 PM
>> Subject: Re: [v2 PATCH 3/3] mmc: sdhci-cadence: Update PHY delay configuration
>>
>> Hi Piotr,
>>
>> Sorry for my late reply.
>>
>>
>>>
>>> It looks that "input delays" and "DLL sdclk delays" should be defined in dts file because they depend on a chip and a board
>> implementation. On the other hand the less dts properties the better.
>>>
>>> There is one more way to handle input delays. It can be achieved by PHY training. PHY training is similar to the tuning and it should be
>> done when proper timing mode is selected and clock frequency is set.
>>> To make it possible the sdhci_set_ios function need to be global. Then I could create sdhci_cdns_set_ios function as follows:
>>> void sdhci_cdns_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) {
>>>         . . .
>>>
>>>         sdhci_set_ios(mmc, ios);
>>>         /* execute PHY training if needed */
>>>         sdhci_cdns_exec_phy_training(host);
>>> }
>>>
>>> The mmc framework configures timing and frequency separately so PHY training should be executed every time if timing or clock
>> frequency is changed. I am not sure If I can change sdhci_set_ios to global function.
>>
>>
>> I am OK with this, but I hope Adrian can advise us.

There is no problem exporting sdhci_set_ios()

>>
>>
>>
>>
>>> So maybe put all delays to dts file would be a better solution? What do you think?
>>
>> I am OK with DT approach too
>> because this way seems simpler, after all.
>>
>> (My suggestion for data array approach was misleading, sorry.)
>>
> Thanks for review anyway it was useful. Now decision between DTS and data array is more clear for me. 
> 
> Regards 
> Piotr Sroka
>