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

Groups > linux.kernel > #1400400 > unrolled thread

[PATCH 1/4] phy: rockchip-emmc: give DLL some extra time to be ready

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/4] phy: rockchip-emmc: give DLL some extra time to be ready Brian Norris <briannorris@chromium.org> - 2016-05-13 00:50 +0200
    [PATCH 3/4] phy: rockchip-emmc: configure default output tap delay Brian Norris <briannorris@chromium.org> - 2016-05-13 00:50 +0200
      Re: [PATCH 3/4] phy: rockchip-emmc: configure default output tap delay Doug Anderson <dianders@chromium.org> - 2016-05-14 00:30 +0200
        Re: [PATCH 3/4] phy: rockchip-emmc: configure default output tap  delay Shawn Lin <shawn.lin@rock-chips.com> - 2016-05-16 06:20 +0200
          Re: [PATCH 3/4] phy: rockchip-emmc: configure default output tap delay Doug Anderson <dianders@chromium.org> - 2016-05-16 17:20 +0200
    [PATCH 2/4] phy: rockchip-emmc: configure frequency range and drive impedance Brian Norris <briannorris@chromium.org> - 2016-05-13 00:50 +0200
      Re: [PATCH 2/4] phy: rockchip-emmc: configure frequency range and  drive impedance Shawn Lin <shawn.lin@rock-chips.com> - 2016-05-13 03:10 +0200
        Re: [PATCH 2/4] phy: rockchip-emmc: configure frequency range and  drive impedance Doug Anderson <dianders@chromium.org> - 2016-05-13 20:50 +0200
          Re: [PATCH 2/4] phy: rockchip-emmc: configure frequency range and  drive impedance Brian Norris <briannorris@chromium.org> - 2016-05-13 23:10 +0200
          Re: [PATCH 2/4] phy: rockchip-emmc: configure frequency range and  drive impedance Doug Anderson <dianders@chromium.org> - 2016-05-24 07:00 +0200
      [PATCH v2 2/4] phy: rockchip-emmc: configure frequency range and  drive impedance Brian Norris <briannorris@chromium.org> - 2016-05-13 23:10 +0200
        Re: [PATCH v2 2/4] phy: rockchip-emmc: configure frequency range and  drive impedance Doug Anderson <dianders@chromium.org> - 2016-05-14 00:10 +0200
    [PATCH 4/4] phy: rockchip-emmc: reindent the register definitions Brian Norris <briannorris@chromium.org> - 2016-05-13 00:50 +0200
      Re: [PATCH 4/4] phy: rockchip-emmc: reindent the register definitions Doug Anderson <dianders@chromium.org> - 2016-05-14 00:30 +0200
    Re: [PATCH 1/4] phy: rockchip-emmc: give DLL some extra time to be ready Doug Anderson <dianders@chromium.org> - 2016-05-14 00:10 +0200

#1400400 — [PATCH 1/4] phy: rockchip-emmc: give DLL some extra time to be ready

From: Shawn Lin <shawn.lin@rock-chips.com>

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 <shawn.lin@rock-chips.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/phy/phy-rockchip-emmc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
index 6ebcf3e41c46..48cbe691a889 100644
--- a/drivers/phy/phy-rockchip-emmc.c
+++ b/drivers/phy/phy-rockchip-emmc.c
@@ -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);
-- 
2.8.0.rc3.226.g39d4020

[toc] | [next] | [standalone]

#1400401 — [PATCH 3/4] phy: rockchip-emmc: configure default output tap delay

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 <shawn.lin@rock-chips.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/phy/phy-rockchip-emmc.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
index 5641dede32f6..f94d3a6587ed 100644
--- a/drivers/phy/phy-rockchip-emmc.c
+++ b/drivers/phy/phy-rockchip-emmc.c
@@ -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)
-- 
2.8.0.rc3.226.g39d4020

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

#1401018 — Re: [PATCH 3/4] phy: rockchip-emmc: configure default output tap delay

Hi,

On Thu, May 12, 2016 at 3:43 PM, Brian Norris <briannorris@chromium.org> wrote:
> 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

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

#1401305 — Re: [PATCH 3/4] phy: rockchip-emmc: configure default output tap delay

Hi Doug,

On 2016/5/14 6:25, Doug Anderson wrote:
> Hi,
>
> On Thu, May 12, 2016 at 3:43 PM, Brian Norris <briannorris@chromium.org> wrote:
>> 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.

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


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


-- 
Best Regards
Shawn Lin

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

#1401566 — Re: [PATCH 3/4] phy: rockchip-emmc: configure default output tap delay

Hi,

On Sun, May 15, 2016 at 9:15 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> Hi Doug,
>
>
> On 2016/5/14 6:25, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Thu, May 12, 2016 at 3:43 PM, Brian Norris <briannorris@chromium.org>
>> wrote:
>>>
>>> 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.
>
>
> 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...

Great to hear.  So we can probably just use your email as the basis of
the commit message?  How about this for the commit text then:

The output tap delay controls helps maintain the hold requirements for
eMMC.  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 won't vary too much for different SoCs.

If / when we find an instance of a crazy PCB layout that needs a value
different than 4, we will figure out how to best specify that,
possibly using the device tree in some way.

-Doug

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

#1400402 — [PATCH 2/4] phy: rockchip-emmc: configure frequency range and drive impedance

From: Shawn Lin <shawn.lin@rock-chips.com>

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 <shawn.lin@rock-chips.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/phy/phy-rockchip-emmc.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
index 48cbe691a889..5641dede32f6 100644
--- a/drivers/phy/phy-rockchip-emmc.c
+++ b/drivers/phy/phy-rockchip-emmc.c
@@ -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)
-- 
2.8.0.rc3.226.g39d4020

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

#1400438 — Re: [PATCH 2/4] phy: rockchip-emmc: configure frequency range and drive impedance

Hi Brian,

On 2016/5/13 6:43, Brian Norris wrote:
> From: Shawn Lin <shawn.lin@rock-chips.com>
>
> 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 <shawn.lin@rock-chips.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  drivers/phy/phy-rockchip-emmc.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
> index 48cbe691a889..5641dede32f6 100644
> --- a/drivers/phy/phy-rockchip-emmc.c
> +++ b/drivers/phy/phy-rockchip-emmc.c
> @@ -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 */

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.


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


-- 
Best Regards
Shawn Lin

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

#1400939 — Re: [PATCH 2/4] phy: rockchip-emmc: configure frequency range and drive impedance

Shawn,

On Thu, May 12, 2016 at 6:02 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> Hi Brian,
>
>
> On 2016/5/13 6:43, Brian Norris wrote:
>>
>> From: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> 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 <shawn.lin@rock-chips.com>
>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>> ---
>>  drivers/phy/phy-rockchip-emmc.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/phy/phy-rockchip-emmc.c
>> b/drivers/phy/phy-rockchip-emmc.c
>> index 48cbe691a889..5641dede32f6 100644
>> --- a/drivers/phy/phy-rockchip-emmc.c
>> +++ b/drivers/phy/phy-rockchip-emmc.c
>> @@ -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 */
>
>
> 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.


I _think_ that this just needs to match the input clock rate of the
eMMC, right?  So presumably the PHY should get a reference to the same
clock that was given to the controller clock.  It can check the clock
at probe time and then register a notifier to keep the in sync (if we
expect the clock to change).

IMHO adding all of that complexity seems like it could wait for a
followup patch.  For now we can assume 200 MHz I think?


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



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

Sounds like this should be handled as per above: make sure that the
PHY has access to the clock and can check it's rate.

---

So overall:

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

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

* Long term someone (hopefully at Rockchip) should think about making
the driver auto-adjust using the clock API.

* Long term someone (hopefully at Rockchip) should think about trying
to use the pinctrl API to allow adjusting drive strengths and pullups.


-Doug

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

#1400983 — Re: [PATCH 2/4] phy: rockchip-emmc: configure frequency range and drive impedance

On Fri, May 13, 2016 at 11:46:33AM -0700, Doug Anderson wrote:
> On Thu, May 12, 2016 at 6:02 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> > On 2016/5/13 6:43, Brian Norris wrote:
> >> @@ -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.

Right, will fix the first bullet point.

Brian

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

#1405831 — Re: [PATCH 2/4] phy: rockchip-emmc: configure frequency range and drive impedance

Hi,

On Fri, May 13, 2016 at 11:46 AM, Doug Anderson <dianders@chromium.org> wrote:
>> 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@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@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.

Note that I still believe that we could land the 50 Ohm first (AKA
land the patch Brian posted), but I'm also convinced that my pinctrl
proposal above is not a good idea for the way to move forward, at
least in terms of the "driver strength" part.  Specifically it seems
like the 50 Ohm / 33 Ohm / 66 Ohm / 100 Ohm / 40 Ohm is a concept from
eMMC 5.0 and probably doesn't fit to pinctrl.

I also _think_ it needs to be matched against what's available from
the card (card->drive_strength) and the card needs to be told about
it, but I could be wrong about that.



-Doug

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

#1400985 — [PATCH v2 2/4] phy: rockchip-emmc: configure frequency range and drive impedance

From: Shawn Lin <shawn.lin@rock-chips.com>

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 <shawn.lin@rock-chips.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
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(+)

diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
index 48cbe691a889..f2f75cf69af1 100644
--- a/drivers/phy/phy-rockchip-emmc.c
+++ b/drivers/phy/phy-rockchip-emmc.c
@@ -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: 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)
-- 
2.8.0.rc3.226.g39d4020

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

#1401008 — Re: [PATCH v2 2/4] phy: rockchip-emmc: configure frequency range and drive impedance

Hi,

On Fri, May 13, 2016 at 2:09 PM, Brian Norris <briannorris@chromium.org> wrote:
> From: Shawn Lin <shawn.lin@rock-chips.com>
>
> 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 <shawn.lin@rock-chips.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 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(+)

As per my comments on v1, this is a sane starting point and seems like
a good idea to land.  Hopefully someone at Rockchip can pick things up
and continue making this more configurable.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

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

#1400403 — [PATCH 4/4] phy: rockchip-emmc: reindent the register definitions

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 <briannorris@chromium.org>
---
 drivers/phy/phy-rockchip-emmc.c | 76 ++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 38 deletions(-)

diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
index f94d3a6587ed..c27ca2b39dfe 100644
--- a/drivers/phy/phy-rockchip-emmc.c
+++ b/drivers/phy/phy-rockchip-emmc.c
@@ -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
-- 
2.8.0.rc3.226.g39d4020

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

#1401017 — Re: [PATCH 4/4] phy: rockchip-emmc: reindent the register definitions

Hi,

On Thu, May 12, 2016 at 3:43 PM, Brian Norris <briannorris@chromium.org> wrote:
> 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 <briannorris@chromium.org>
> ---
>  drivers/phy/phy-rockchip-emmc.c | 76 ++++++++++++++++++++---------------------
>  1 file changed, 38 insertions(+), 38 deletions(-)

Sure, why not?

Reviewed-by: Douglas Anderson <dianders@chromium.org>

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

#1401007

Hi,

On Thu, May 12, 2016 at 3:43 PM, Brian Norris <briannorris@chromium.org> wrote:
> From: Shawn Lin <shawn.lin@rock-chips.com>
>
> 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 <shawn.lin@rock-chips.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  drivers/phy/phy-rockchip-emmc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
> index 6ebcf3e41c46..48cbe691a889 100644
> --- a/drivers/phy/phy-rockchip-emmc.c
> +++ b/drivers/phy/phy-rockchip-emmc.c
> @@ -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);

Seems sane.  This is a "random delay" but you've documented it well,
and an extra 19 microseconds won't be the end of the world.

If we truly trusted the "DLLRDY" bit we could actually do a loop here,
but it's probably not worth it...

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web