phy: rockchip-emmc: give DLL some extra time to be ready

From: Shawn Lin <[email protected]>

Signal integrity analysis has suggested we set these values. Do this in power_on(), so that they get reconfigured after suspend/resume.

Signed-off-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---
 drivers/phy/phy-rockchip-emmc.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/phy/phy-rockchip-emmc.c +13 -0

index 48cbe691a889..5641dede32f6 100644
@@ -56,6 +56,19 @@
 #define PHYCTRL_DLLRDY_SHIFT	0x5
 #define PHYCTRL_DLLRDY_DONE	0x1
 #define PHYCTRL_DLLRDY_GOING	0x0
+#define PHYCTRL_FREQSEL_200M	0x0
+#define PHYCTRL_FREQSEL_50M	0x1
+#define PHYCTRL_FREQSEL_100M	0x2
+#define PHYCTRL_FREQSEL_150M	0x3
+#define PHYCTRL_FREQSEL_MASK	0x3
+#define PHYCTRL_FREQSEL_SHIFT	0xc
+#define PHYCTRL_DR_MASK		0x7
+#define PHYCTRL_DR_SHIFT	0x4
+#define PHYCTRL_DR_50OHM	0x0
+#define PHYCTRL_DR_33OHM	0x1
+#define PHYCTRL_DR_66OHM	0x2
+#define PHYCTRL_DR_100OHM	0x3
+#define PHYCTRL_DR_40OHM	0x4
 
 struct rockchip_emmc_phy {
 	unsigned int	reg_offset;

drivers/phy/phy-rockchip-emmc.c +13 -0

+	regmap_write(rk_phy->reg_base,
+		     rk_phy->reg_offset + GRF_EMMCPHY_CON0,
+		     HIWORD_UPDATE(PHYCTRL_FREQSEL_200M,
+				   PHYCTRL_FREQSEL_MASK,
+				   PHYCTRL_FREQSEL_SHIFT));
+
+	/* Drive impedance: 50 Ohm */
+	regmap_write(rk_phy->reg_base,
+		     rk_phy->reg_offset + GRF_EMMCPHY_CON6,
+		     HIWORD_UPDATE(PHYCTRL_DR_50OHM,
+				   PHYCTRL_DR_MASK,
+				   PHYCTRL_DR_SHIFT));
+
 	/* Power up emmc phy analog blocks */
 	ret = rockchip_emmc_phy_power(rk_phy, PHYCTRL_PDB_PWR_ON);
 	if (ret)

Hi Brian,

Quoted 42 lines · Brian Norris

From: Shawn Lin <[email protected]>

Signal integrity analysis has suggested we set these values. Do this in power_on(), so that they get reconfigured after suspend/resume.

Signed-off-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---
 drivers/phy/phy-rockchip-emmc.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/phy/phy-rockchip-emmc.c +13 -0

index 48cbe691a889..5641dede32f6 100644
@@ -56,6 +56,19 @@
 #define PHYCTRL_DLLRDY_SHIFT   0x5
 #define PHYCTRL_DLLRDY_DONE    0x1
 #define PHYCTRL_DLLRDY_GOING   0x0
+#define PHYCTRL_FREQSEL_200M   0x0
+#define PHYCTRL_FREQSEL_50M    0x1
+#define PHYCTRL_FREQSEL_100M   0x2
+#define PHYCTRL_FREQSEL_150M   0x3
+#define PHYCTRL_FREQSEL_MASK   0x3
+#define PHYCTRL_FREQSEL_SHIFT  0xc
+#define PHYCTRL_DR_MASK                0x7
+#define PHYCTRL_DR_SHIFT       0x4
+#define PHYCTRL_DR_50OHM       0x0
+#define PHYCTRL_DR_33OHM       0x1
+#define PHYCTRL_DR_66OHM       0x2
+#define PHYCTRL_DR_100OHM      0x3
+#define PHYCTRL_DR_40OHM       0x4
 
 struct rockchip_emmc_phy {
        unsigned int    reg_offset;

@@ -154,6 +167,20 @@ static int rockchip_emmc_phy_power_on(struct phy
 *phy)
        struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
        int ret = 0;
 

drivers/phy/phy-rockchip-emmc.c static int rockchip_emmc_phy_power_on(struct phy *phy) · line 170

Quoted 1 line

drivers/phy/phy-rockchip-emmc.c +1 -0

+       /* DLL operation: 170 to 200 MHz */

What is 170 here? Should we expose them to dt instead of hardcoding them?

Per the commit msg, signal may vary from board to board, so I guess 50ohm may not always be the best selection?

Another thing I need to elaborate more here is that emmc phy only supports 50/100/150/200Mhz. Presumably people love to use the highest speed mode with its upper limiting frequency, but in case of some special requirement or bad board design, they want to add max-frequency in dts for emmc controller. In this case PHYCTRL_FREQSEL_XXXM should meet the actual max-frequency, take 100M for example, otherwise emmc_phy's tuning block will use 200M to do some calculation but it certainly should be 100M. This leads emmc_phy to choose the wrong phase. Finally maybe you will see triggering re-tune easily or some worse case of even tuning failure when probing card.

Quoted 11 lines · Shawn Lin

drivers/phy/phy-rockchip-emmc.c static int rockchip_emmc_phy_power_on(struct phy · line 171

Quoted 6 lines · Brian Norris

drivers/phy/phy-rockchip-emmc.c +1 -0

@@ -154,6 +167,20 @@ static int rockchip_emmc_phy_power_on(struct phy
 *phy)
        struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
        int ret = 0;
 
+       /* DLL operation: 170 to 200 MHz */

What is 170 here? Should we expose them to dt instead of hardcoding them?

This was probably my fault. I did some searching and found
<https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf>.
It appears to be docs for a similar (but not identical) PHY. We were looking at it to try to get more clarity on some bits that were hard to understand in the docs we had.

In that doc there appear to be 3 bits for selecting the DLL operation and they have ranges defined. In Rockchip's PHY there are only 2 bits. Thus things don't map totally properly.

Anyway, comment should probably be removed.

So overall:

* Should re-spin and remove the comment about 170 MHz.

* I think this could land as-is other than the comment.

Quoted 2 lines

Per the commit msg, signal may vary from board to board, so I guess 50ohm may not always be the best selection?

Starting out with something sane like 50 ohms seems like it makes sense for now. It's OK to start with a default for now to get things basically working and then add device tree support once we have a second user.

When we're ready to make this more generic, IMHO we might consider having the PHY implement the pinctrl API and officially be a pin controller and we use those bindings. We are controlling pins so using the pinctrl API seems like it might make sense?

I _think_ that perhaps what we're specifying here is actually slew rate, but feel free to correct me if I'm wrong. It looks as if "drive strength" is supposed to be specified in terms of mA and the docs I find show that we're actually controlling how fast the pins will toggle.

I'm not 100% certain I know how the pinctrl bindings apply in this case (maybe Heiko has ideas, or maybe we should send a proposal to Linus W?), but from the bindings they look like they offer some flexibility.

Maybe this would look like below (or maybe you need some extra sub-nodes)

       sdhci: sdhci at fe330000 {
               compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";
               reg = <0x0 0xfe330000 0x0 0x10000>;
               interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
               clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
               clock-names = "clk_xin", "clk_ahb";
               assigned-clocks = <&cru SCLK_EMMC>;
               assigned-clock-rates = <200000000>;
               phys = <&emmc_phy>;
               phy-names = "phy_arasan";
               pinctrl-names = "default";
               pinctrl-0 = <&pcfg_emmc_slew_rate_x1_00>;
               status = "disabled";
       };

               emmc_phy: phy at f780 {
                       compatible = "rockchip,rk3399-emmc-phy";
                       reg = <0xf780 0x20>;
                       #phy-cells = <0>;
                       status = "disabled";

                       pcfg_emmc_slew_rate_x1_00: pcfg-emmc-slew-rate-x1-00 {
                               slew-rate = <100>;
                       };
                       pcfg_emmc_slew_rate_x1_50: pcfg-emmc-slew-rate-x1-50 {
                               slew-rate = <150>;
                       };
                       pcfg_emmc_slew_rate_x0_75: pcfg-emmc-slew-rate-x0-75 {
                               slew-rate = <75>;
                       };
                       pcfg_emmc_slew_rate_x0_50: pcfg-emmc-slew-rate-x0-50 {
                               slew-rate = <50>;
                       };
                       pcfg_emmc_slew_rate_x1_20: pcfg-emmc-slew-rate-x1-20 {
                               slew-rate = <120>;
                       };
                       pcfg_emmc_slew_rate_x1_20: pcfg-emmc-slew-rate-x1-20 {
                               slew-rate = <120>;
                       };
           };

The nice thing about using the pinctrl API is that:

* It allows us to _also_ control pullups / pulldowns. We probably want to control those also since some boards may use external pullups and others may want to use the internal ones.

* If SDHCI needs to dynamically adjust things (like turning on pulls, adjusting drive strengths, etc) it can do it in a sane API.

From: Shawn Lin <[email protected]>

Signal integrity analysis has suggested we set these values. Do this in power_on(), so that they get reconfigured after suspend/resume.

Signed-off-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---
v2:
* Sent only patch 2/4 with version 2, to avoid spamming; will move on to v3 for all patches if I need to send another
* Drop 170 MHz comment; this was only applicable to a subtly different Arasan PHY

 drivers/phy/phy-rockchip-emmc.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

From: Shawn Lin <[email protected]>

Signal integrity analysis has suggested we set these values. Do this in power_on(), so that they get reconfigured after suspend/resume.

Signed-off-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

From: Shawn Lin <[email protected]>

Signal integrity analysis has suggested we set these values. Do this in power_on(), so that they get reconfigured after suspend/resume.

Signed-off-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---
 drivers/phy/phy-rockchip-emmc.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/phy/phy-rockchip-emmc.c +27 -0

index 48cbe691a889..5641dede32f6 100644
@@ -56,6 +56,19 @@
 #define PHYCTRL_DLLRDY_SHIFT	0x5
 #define PHYCTRL_DLLRDY_DONE	0x1
 #define PHYCTRL_DLLRDY_GOING	0x0
+#define PHYCTRL_FREQSEL_200M	0x0
+#define PHYCTRL_FREQSEL_50M	0x1
+#define PHYCTRL_FREQSEL_100M	0x2
+#define PHYCTRL_FREQSEL_150M	0x3
+#define PHYCTRL_FREQSEL_MASK	0x3
+#define PHYCTRL_FREQSEL_SHIFT	0xc
+#define PHYCTRL_DR_MASK		0x7
+#define PHYCTRL_DR_SHIFT	0x4
+#define PHYCTRL_DR_50OHM	0x0
+#define PHYCTRL_DR_33OHM	0x1
+#define PHYCTRL_DR_66OHM	0x2
+#define PHYCTRL_DR_100OHM	0x3
+#define PHYCTRL_DR_40OHM	0x4
 
 struct rockchip_emmc_phy {
 	unsigned int	reg_offset;

@@ -154,6 +167,20 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
 	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
 	int ret = 0;
 
+	/* DLL operation: 170 to 200 MHz */
+	regmap_write(rk_phy->reg_base,
+		     rk_phy->reg_offset + GRF_EMMCPHY_CON0,
+		     HIWORD_UPDATE(PHYCTRL_FREQSEL_200M,
+				   PHYCTRL_FREQSEL_MASK,
+				   PHYCTRL_FREQSEL_SHIFT));
+
+	/* Drive impedance: 50 Ohm */
+	regmap_write(rk_phy->reg_base,
+		     rk_phy->reg_offset + GRF_EMMCPHY_CON6,
+		     HIWORD_UPDATE(PHYCTRL_DR_50OHM,
+				   PHYCTRL_DR_MASK,
+				   PHYCTRL_DR_SHIFT));
+
 	/* Power up emmc phy analog blocks */
 	ret = rockchip_emmc_phy_power(rk_phy, PHYCTRL_PDB_PWR_ON);
 	if (ret)

The output tap delay controls helps maintain the hold requirements for eMMC. The exact value is dependent on the SoC and other factors, though it isn't really an exact science. But the default of 0 is not very good, as it doesn't give the eMMC much hold time, so let's bump up to 4 (approx 90 degree phase?). If we need to configure this any further (e.g., based on board or speed factors), we may need to consider a device tree representation.

Hi,

Quoted 7 lines · Brian Norris

The output tap delay controls helps maintain the hold requirements for eMMC. The exact value is dependent on the SoC and other factors, though it isn't really an exact science. But the default of 0 is not very good, as it doesn't give the eMMC much hold time, so let's bump up to 4 (approx 90 degree phase?). If we need to configure this any further (e.g., based on board or speed factors), we may need to consider a device tree representation.

As I understand it, this solves much the same problem as my patch in
<https://patchwork.kernel.org/patch/9085581/>, but for the eMMC port on rk3399 (which doesn't use dw_mmc). As argued in that patch and also in the discussion from
<https://patchwork.kernel.org/patch/9030621/>, if we eventually end up needing to put something in the device tree we need to be really careful. Specifically to get the exact right value here I think you need to consider the input clock, speed mode, and any SoC-specific delays differences between the clock and the data lines. That would imply that, if anything, the device tree data would only contain information about the SoC-specific delay differences and all other work to set this value would involve coordination between the PHY and the SDHCI controller.

However, as also discussed previously, we don't appear to need to be very exact about the value here. It seems like setting this to 4 (~90 degrees?) is a much better starting point than leaving it at the default of 0.

...so I'd be all for landing this patch. Perhaps Shawn can chime in and confirm that our understanding is correct and possibly we can update the commit message. Then presumably someone at Rockchip can keep working to find a better way to set this long term.

Sound good?

-Doug

Hi Doug,

Quoted 30 lines · Doug Anderson

Hi,

Quoted 8 lines · Brian Norris

The output tap delay controls helps maintain the hold requirements for eMMC. The exact value is dependent on the SoC and other factors, though it isn't really an exact science. But the default of 0 is not very good, as it doesn't give the eMMC much hold time, so let's bump up to 4 (approx 90 degree phase?). If we need to configure this any further (e.g., based on board or speed factors), we may need to consider a device tree representation.

As I understand it, this solves much the same problem as my patch in
<https://patchwork.kernel.org/patch/9085581/>, but for the eMMC port on rk3399 (which doesn't use dw_mmc). As argued in that patch and also in the discussion from
<https://patchwork.kernel.org/patch/9030621/>, if we eventually end up needing to put something in the device tree we need to be really careful. Specifically to get the exact right value here I think you need to consider the input clock, speed mode, and any SoC-specific delays differences between the clock and the data lines. That would imply that, if anything, the device tree data would only contain information about the SoC-specific delay differences and all other work to set this value would involve coordination between the PHY and the SDHCI controller.

Quoted 4 lines

However, as also discussed previously, we don't appear to need to be very exact about the value here. It seems like setting this to 4 (~90 degrees?) is a much better starting point than leaving it at the default of 0.

The value, 4, is based on real silicon test observed from the oscilloscope, and of course it meets the requirement of speed modes. For arasan't phy, its phase is very accurate, so the real timing of the value you set almost won't vary too much for different Socs.

So explicitly assigning 4 here looks sane currently except for crazy PCB layout...

The output tap delay controls helps maintain the hold requirements for eMMC. The exact value is dependent on the SoC and other factors, though it isn't really an exact science. But the default of 0 is not very good, as it doesn't give the eMMC much hold time, so let's bump up to 4 (approx 90 degree phase?). If we need to configure this any further (e.g., based on board or speed factors), we may need to consider a device tree representation.

Suggested-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

The output tap delay controls helps maintain the hold requirements for eMMC. The exact value is dependent on the SoC and other factors, though it isn't really an exact science. But the default of 0 is not very good, as it doesn't give the eMMC much hold time, so let's bump up to 4 (approx 90 degree phase?). If we need to configure this any further (e.g., based on board or speed factors), we may need to consider a device tree representation.

Suggested-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---
 drivers/phy/phy-rockchip-emmc.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

drivers/phy/phy-rockchip-emmc.c +19 -0

index 5641dede32f6..f94d3a6587ed 100644
@@ -69,6 +69,11 @@
 #define PHYCTRL_DR_66OHM	0x2
 #define PHYCTRL_DR_100OHM	0x3
 #define PHYCTRL_DR_40OHM	0x4
+#define PHYCTRL_OTAPDLYENA		0x1
+#define PHYCTRL_OTAPDLYENA_MASK		0x1
+#define PHYCTRL_OTAPDLYENA_SHIFT	0xb
+#define PHYCTRL_OTAPDLYSEL_MASK		0xf
+#define PHYCTRL_OTAPDLYSEL_SHIFT	0x7
 
 struct rockchip_emmc_phy {
 	unsigned int	reg_offset;

@@ -181,6 +186,20 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
 				   PHYCTRL_DR_MASK,
 				   PHYCTRL_DR_SHIFT));
 
+	/* Output tap delay: enable */
+	regmap_write(rk_phy->reg_base,
+		     rk_phy->reg_offset + GRF_EMMCPHY_CON0,
+		     HIWORD_UPDATE(PHYCTRL_OTAPDLYENA,
+				   PHYCTRL_OTAPDLYENA_MASK,
+				   PHYCTRL_OTAPDLYENA_SHIFT));
+
+	/* Output tap delay */
+	regmap_write(rk_phy->reg_base,
+		     rk_phy->reg_offset + GRF_EMMCPHY_CON0,
+		     HIWORD_UPDATE(4,
+				   PHYCTRL_OTAPDLYSEL_MASK,
+				   PHYCTRL_OTAPDLYSEL_SHIFT));
+
 	/* Power up emmc phy analog blocks */
 	ret = rockchip_emmc_phy_power(rk_phy, PHYCTRL_PDB_PWR_ON);
 	if (ret)

Some of the spacing was wrong (spaces instead of tabs), and due to longer entries added later, the columns weren't aligned. Let's get everything consistent.

Signed-off-by Brian Norris <[email protected]>

---
 drivers/phy/phy-rockchip-emmc.c | 76 ++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 38 deletions(-)

Some of the spacing was wrong (spaces instead of tabs), and due to longer entries added later, the columns weren't aligned. Let's get everything consistent.

Signed-off-by Brian Norris <[email protected]>

Some of the spacing was wrong (spaces instead of tabs), and due to longer entries added later, the columns weren't aligned. Let's get everything consistent.

Signed-off-by Brian Norris <[email protected]>

---
 drivers/phy/phy-rockchip-emmc.c | 76 ++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 38 deletions(-)

drivers/phy/phy-rockchip-emmc.c +38 -38

index f94d3a6587ed..c27ca2b39dfe 100644
@@ -31,44 +31,44 @@
 		((val) << (shift) | (mask) << ((shift) + 16))
 
 /* Register definition */
-#define GRF_EMMCPHY_CON0	0x0
-#define GRF_EMMCPHY_CON1	0x4
-#define GRF_EMMCPHY_CON2	0x8
-#define GRF_EMMCPHY_CON3	0xc
-#define GRF_EMMCPHY_CON4	0x10
-#define GRF_EMMCPHY_CON5	0x14
-#define GRF_EMMCPHY_CON6	0x18
-#define GRF_EMMCPHY_STATUS	0x20
-
-#define PHYCTRL_PDB_MASK	0x1
-#define PHYCTRL_PDB_SHIFT	0x0
-#define PHYCTRL_PDB_PWR_ON	0x1
-#define PHYCTRL_PDB_PWR_OFF	0x0
-#define PHYCTRL_ENDLL_MASK	0x1
-#define PHYCTRL_ENDLL_SHIFT     0x1
-#define PHYCTRL_ENDLL_ENABLE	0x1
-#define PHYCTRL_ENDLL_DISABLE	0x0
-#define PHYCTRL_CALDONE_MASK	0x1
-#define PHYCTRL_CALDONE_SHIFT   0x6
-#define PHYCTRL_CALDONE_DONE	0x1
-#define PHYCTRL_CALDONE_GOING	0x0
-#define PHYCTRL_DLLRDY_MASK	0x1
-#define PHYCTRL_DLLRDY_SHIFT	0x5
-#define PHYCTRL_DLLRDY_DONE	0x1
-#define PHYCTRL_DLLRDY_GOING	0x0
-#define PHYCTRL_FREQSEL_200M	0x0
-#define PHYCTRL_FREQSEL_50M	0x1
-#define PHYCTRL_FREQSEL_100M	0x2
-#define PHYCTRL_FREQSEL_150M	0x3
-#define PHYCTRL_FREQSEL_MASK	0x3
-#define PHYCTRL_FREQSEL_SHIFT	0xc
-#define PHYCTRL_DR_MASK		0x7
-#define PHYCTRL_DR_SHIFT	0x4
-#define PHYCTRL_DR_50OHM	0x0
-#define PHYCTRL_DR_33OHM	0x1
-#define PHYCTRL_DR_66OHM	0x2
-#define PHYCTRL_DR_100OHM	0x3
-#define PHYCTRL_DR_40OHM	0x4
+#define GRF_EMMCPHY_CON0		0x0
+#define GRF_EMMCPHY_CON1		0x4
+#define GRF_EMMCPHY_CON2		0x8
+#define GRF_EMMCPHY_CON3		0xc
+#define GRF_EMMCPHY_CON4		0x10
+#define GRF_EMMCPHY_CON5		0x14
+#define GRF_EMMCPHY_CON6		0x18
+#define GRF_EMMCPHY_STATUS		0x20
+
+#define PHYCTRL_PDB_MASK		0x1
+#define PHYCTRL_PDB_SHIFT		0x0
+#define PHYCTRL_PDB_PWR_ON		0x1
+#define PHYCTRL_PDB_PWR_OFF		0x0
+#define PHYCTRL_ENDLL_MASK		0x1
+#define PHYCTRL_ENDLL_SHIFT		0x1
+#define PHYCTRL_ENDLL_ENABLE		0x1
+#define PHYCTRL_ENDLL_DISABLE		0x0
+#define PHYCTRL_CALDONE_MASK		0x1
+#define PHYCTRL_CALDONE_SHIFT		0x6
+#define PHYCTRL_CALDONE_DONE		0x1
+#define PHYCTRL_CALDONE_GOING		0x0
+#define PHYCTRL_DLLRDY_MASK		0x1
+#define PHYCTRL_DLLRDY_SHIFT		0x5
+#define PHYCTRL_DLLRDY_DONE		0x1
+#define PHYCTRL_DLLRDY_GOING		0x0
+#define PHYCTRL_FREQSEL_200M		0x0
+#define PHYCTRL_FREQSEL_50M		0x1
+#define PHYCTRL_FREQSEL_100M		0x2
+#define PHYCTRL_FREQSEL_150M		0x3
+#define PHYCTRL_FREQSEL_MASK		0x3
+#define PHYCTRL_FREQSEL_SHIFT		0xc
+#define PHYCTRL_DR_MASK			0x7
+#define PHYCTRL_DR_SHIFT		0x4
+#define PHYCTRL_DR_50OHM		0x0
+#define PHYCTRL_DR_33OHM		0x1
+#define PHYCTRL_DR_66OHM		0x2
+#define PHYCTRL_DR_100OHM		0x3
+#define PHYCTRL_DR_40OHM		0x4
 #define PHYCTRL_OTAPDLYENA		0x1
 #define PHYCTRL_OTAPDLYENA_MASK		0x1
 #define PHYCTRL_OTAPDLYENA_SHIFT	0xb

From: Shawn Lin <[email protected]>

According to the databook, 10.2us is the max time for dll to be ready to work. However in testing, some chips need 20us for dll to be ready. This patch adds some extra margin for dllrdy to be ready, fixing our -ETIMEDOUT issues.

Signed-off-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---
 drivers/phy/phy-rockchip-emmc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/phy/phy-rockchip-emmc.c +0 -1

index 6ebcf3e41c46..48cbe691a889 100644
@@ -119,10 +119,11 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
                                   PHYCTRL_ENDLL_MASK,
                                   PHYCTRL_ENDLL_SHIFT));
        /*
-        * After enable analog DLL circuits, we need extra 10.2us

From: Shawn Lin <[email protected]>

According to the databook, 10.2us is the max time for dll to be ready to work. However in testing, some chips need 20us for dll to be ready. This patch adds some extra margin for dllrdy to be ready, fixing our -ETIMEDOUT issues.

Signed-off-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

From: Shawn Lin <[email protected]>

According to the databook, 10.2us is the max time for dll to be ready to work. However in testing, some chips need 20us for dll to be ready. This patch adds some extra margin for dllrdy to be ready, fixing our -ETIMEDOUT issues.

Signed-off-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---
 drivers/phy/phy-rockchip-emmc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/phy/phy-rockchip-emmc.c +4 -3

index 6ebcf3e41c46..48cbe691a889 100644
@@ -119,10 +119,11 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
 				   PHYCTRL_ENDLL_MASK,
 				   PHYCTRL_ENDLL_SHIFT));
 	/*
-	 * After enable analog DLL circuits, we need extra 10.2us
-	 * for dll to be ready for work.
+	 * After enable analog DLL circuits, we need an extra 10.2us
+	 * for dll to be ready for work. But according to testing, we
+	 * find some chips need more than 25us.
 	 */
-	udelay(11);
+	udelay(30);
 	regmap_read(rk_phy->reg_base,
 		    rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
 		    &dllrdy);

Quoted 7 lines · Brian Norris

From: Shawn Lin <[email protected]>

According to the databook, 10.2us is the max time for dll to be ready to work. However in testing, some chips need 20us for dll to be ready. This patch adds some extra margin for dllrdy to be ready, fixing our -ETIMEDOUT issues.

Quoted 2 lines

Signed-off-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

Acked-by Kishon Vijay Abraham I <[email protected]>

+ linux-mmc, Ulf

Quoted 8 lines · Kishon Vijay Abraham I

Quoted 7 lines · Brian Norris

From: Shawn Lin <[email protected]>

According to the databook, 10.2us is the max time for dll to be ready to work. However in testing, some chips need 20us for dll to be ready. This patch adds some extra margin for dllrdy to be ready, fixing our -ETIMEDOUT issues.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Shawn Lin <[email protected]>

Signed-off-by Brian Norris <[email protected]>

Acked-by Kishon Vijay Abraham I <[email protected]>

Thanks! I take it that to mean you're agreeing with Doug's suggestion here:

https://lkml.org/lkml/2016/6/13/1067

"Since [the $subject series patches] aren't in 4.7-rc1 presumably they would also make sense to take through the MMC tree if others agree."

Ulf,

I see I didn't CC you or linux-mmc on this patch series. Please let me know if I should resend, or if you can pick these up off of LKML -- Doug gave nice patchwork links in his linked series. NB: patch 2 has a version 2, while the others were left unmodified.

Quoted 6 lines · Brian Norris

Ulf,

I see I didn't CC you or linux-mmc on this patch series. Please let me know if I should resend, or if you can pick these up off of LKML -- Doug gave nice patchwork links in his linked series. NB: patch 2 has a version 2, while the others were left unmodified.

I actually need to spin my series to address a small bit of feedback from Heiko. Since Brian's original patches weren't on linux-mmc, I'll include them in my series (along with collected tags) to make it easy. Hope this is OK.