[3/4] phy: rockchip-emmc: configure default output tap delay
diff mbox

Message ID 1463092986-61777-3-git-send-email-briannorris@chromium.org
State New
Headers show

Commit Message

Brian Norris May 12, 2016, 10:43 p.m. UTC
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(+)

Comments

Doug Anderson May 13, 2016, 10:25 p.m. UTC | #1
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
Shawn Lin May 16, 2016, 4:15 a.m. UTC | #2
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
>
>
>
Doug Anderson May 16, 2016, 3:12 p.m. UTC | #3
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
Heiko Stübner June 16, 2016, 11:36 p.m. UTC | #4
Am Donnerstag, 12. Mai 2016, 15:43:05 schrieb 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.
> 
> Suggested-by: Shawn Lin <shawn.lin@rock-chips.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

on my rk3399-evb board the emmc still runs nicely with this patch, so

Tested-by: Heiko Stuebner <heiko@sntech.de>
Kishon Vijay Abraham I June 20, 2016, 1:11 p.m. UTC | #5
On Friday 13 May 2016 04:13 AM, Brian Norris 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.
> 
> Suggested-by: Shawn Lin <shawn.lin@rock-chips.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  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)
>

Patch
diff mbox

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)