[PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.

[PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.

[Christoph Muellner]

The rockchip-emmc PHY can be configured with different
drive impedance values. Currenlty a value of 50 Ohm is
hard coded into the driver.

This patch introduces the DTS property 'drive-impedance-ohm'
for the rockchip-emmc phy node, which uses the value from the DTS
to setup the drive impedance accordingly.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
 drivers/phy/rockchip/phy-rockchip-emmc.c | 38 ++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 19bf84f0bc67..5413fa73dd45 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -87,6 +87,7 @@ struct rockchip_emmc_phy {
 	unsigned int	reg_offset;
 	struct regmap	*reg_base;
 	struct clk	*emmcclk;
+	unsigned int drive_impedance;
 };
 
 static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
@@ -281,10 +282,10 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
 {
 	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
 
-	/* Drive impedance: 50 Ohm */
+	/* Drive impedance: from DTS */
 	regmap_write(rk_phy->reg_base,
 		     rk_phy->reg_offset + GRF_EMMCPHY_CON6,
-		     HIWORD_UPDATE(PHYCTRL_DR_50OHM,
+		     HIWORD_UPDATE(rk_phy->drive_impedance,
 				   PHYCTRL_DR_MASK,
 				   PHYCTRL_DR_SHIFT));
 
@@ -314,6 +315,28 @@ static const struct phy_ops ops = {
 	.owner		= THIS_MODULE,
 };
 
+static u32 convert_drive_impedance_ohm(struct platform_device *pdev, u32 dr_ohm)
+{
+	switch (dr_ohm) {
+	case 100:
+		return PHYCTRL_DR_100OHM;
+	case 66:
+		return PHYCTRL_DR_66OHM;
+	case 50:
+		return PHYCTRL_DR_50OHM;
+	case 40:
+		return PHYCTRL_DR_40OHM;
+	case 33:
+		return PHYCTRL_DR_33OHM;
+	}
+
+	dev_warn(&pdev->dev,
+		"Invalid value %u for drive-impedance-ohm. "
+		"Falling back to 50 Ohm.\n",
+		dr_ohm);
+	return PHYCTRL_DR_50OHM;
+}
+
 static int rockchip_emmc_phy_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -322,6 +345,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
 	struct phy_provider *phy_provider;
 	struct regmap *grf;
 	unsigned int reg_offset;
+	u32 val;
 
 	if (!dev->parent || !dev->parent->of_node)
 		return -ENODEV;
@@ -345,6 +369,16 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
 	rk_phy->reg_offset = reg_offset;
 	rk_phy->reg_base = grf;
 
+	if (of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val)) {
+		dev_info(dev,
+			"Missing drive-impedance-ohm property in node %s "
+			"Falling back to 50 Ohm.\n",
+			dev->of_node->name);
+		rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
+	} else {
+		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
+	}
+
 	generic_phy = devm_phy_create(dev, dev->of_node, &ops);
 	if (IS_ERR(generic_phy)) {
 		dev_err(dev, "failed to create PHY\n");
-- 
2.11.0

[PATCH 2/3] arm64: dts: rockchip: Define drive-impedance-ohm for RK3399's emmc-phy.

[Christoph Muellner]

A previous patch introduced the property 'drive-impedance-ohm'
for the RK3399's emmc phy node. This patch sets this value
explicitly to the default value of 50 Ohm.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 6cc1c9fa4ea6..b875364a7709 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -1450,6 +1450,7 @@
 			clock-names = "refclk";
 			#phy-cells = <1>;
 			resets = <&cru SRST_PCIEPHY>;
+			drive-impedance-ohm = <50>;
 			reset-names = "phy";
 			status = "disabled";
 		};
-- 
2.11.0

[PATCH 3/3] arm64: dts: rockchip: Decrease emmc-phy's drive impedance on rk3399-puma

[Christoph Muellner]

The RK3399-Q7 (Puma) requires 33 Ohm drive strength to ensure signal
integrity at HS-400 (200MHz clock, DDR signalling).

A repeated EMC testing run validates that this increase does not
negatively impact EMC compliance (emissions have ample distance to
the regulatory limits).

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Tested-by: Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com>
Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
---
 arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
index 0130b9f98c9d..4f75bb6b2f14 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
@@ -146,6 +146,7 @@
 
 &emmc_phy {
 	status = "okay";
+	drive-impedance-ohm = <33>;
 };
 
 &gmac {
-- 
2.11.0

Re: [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.

[Heiko Stuebner]

Hi Christoph,

Am Freitag, 1. März 2019, 16:33:43 CET schrieb Christoph Muellner:
> The rockchip-emmc PHY can be configured with different
> drive impedance values. Currenlty a value of 50 Ohm is
> hard coded into the driver.
> 
> This patch introduces the DTS property 'drive-impedance-ohm'
> for the rockchip-emmc phy node, which uses the value from the DTS
> to setup the drive impedance accordingly.
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>  drivers/phy/rockchip/phy-rockchip-emmc.c | 38 ++++++++++++++++++++++++++++++--

looks good on first glance, but is missing an addition to the emmc-phy
devicetree binding in
	Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt


Heiko

>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
> index 19bf84f0bc67..5413fa73dd45 100644
> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> @@ -87,6 +87,7 @@ struct rockchip_emmc_phy {
>  	unsigned int	reg_offset;
>  	struct regmap	*reg_base;
>  	struct clk	*emmcclk;
> +	unsigned int drive_impedance;
>  };
>  
>  static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
> @@ -281,10 +282,10 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>  {
>  	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
>  
> -	/* Drive impedance: 50 Ohm */
> +	/* Drive impedance: from DTS */
>  	regmap_write(rk_phy->reg_base,
>  		     rk_phy->reg_offset + GRF_EMMCPHY_CON6,
> -		     HIWORD_UPDATE(PHYCTRL_DR_50OHM,
> +		     HIWORD_UPDATE(rk_phy->drive_impedance,
>  				   PHYCTRL_DR_MASK,
>  				   PHYCTRL_DR_SHIFT));
>  
> @@ -314,6 +315,28 @@ static const struct phy_ops ops = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +static u32 convert_drive_impedance_ohm(struct platform_device *pdev, u32 dr_ohm)
> +{
> +	switch (dr_ohm) {
> +	case 100:
> +		return PHYCTRL_DR_100OHM;
> +	case 66:
> +		return PHYCTRL_DR_66OHM;
> +	case 50:
> +		return PHYCTRL_DR_50OHM;
> +	case 40:
> +		return PHYCTRL_DR_40OHM;
> +	case 33:
> +		return PHYCTRL_DR_33OHM;
> +	}
> +
> +	dev_warn(&pdev->dev,
> +		"Invalid value %u for drive-impedance-ohm. "
> +		"Falling back to 50 Ohm.\n",
> +		dr_ohm);
> +	return PHYCTRL_DR_50OHM;
> +}
> +
>  static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -322,6 +345,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>  	struct phy_provider *phy_provider;
>  	struct regmap *grf;
>  	unsigned int reg_offset;
> +	u32 val;
>  
>  	if (!dev->parent || !dev->parent->of_node)
>  		return -ENODEV;
> @@ -345,6 +369,16 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>  	rk_phy->reg_offset = reg_offset;
>  	rk_phy->reg_base = grf;
>  
> +	if (of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val)) {
> +		dev_info(dev,
> +			"Missing drive-impedance-ohm property in node %s "
> +			"Falling back to 50 Ohm.\n",
> +			dev->of_node->name);
> +		rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> +	} else {
> +		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
> +	}
> +
>  	generic_phy = devm_phy_create(dev, dev->of_node, &ops);
>  	if (IS_ERR(generic_phy)) {
>  		dev_err(dev, "failed to create PHY\n");
> 

Re: [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.

[Christoph Müllner]

Hi Heiko,

> On 01.03.2019, at 16:59, Heiko Stuebner <heiko@sntech.de> wrote:
> 
> Hi Christoph,
> 
> Am Freitag, 1. März 2019, 16:33:43 CET schrieb Christoph Muellner:
>> The rockchip-emmc PHY can be configured with different
>> drive impedance values. Currenlty a value of 50 Ohm is
>> hard coded into the driver.
>> 
>> This patch introduces the DTS property 'drive-impedance-ohm'
>> for the rockchip-emmc phy node, which uses the value from the DTS
>> to setup the drive impedance accordingly.
>> 
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>> drivers/phy/rockchip/phy-rockchip-emmc.c | 38 ++++++++++++++++++++++++++++++--
> 
> looks good on first glance, but is missing an addition to the emmc-phy
> devicetree binding in
> 	Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt

you are right.
I've just sent that in a separate patch (DT doc changes need to be in a separate commit anyways).

Thanks,
Christoph

> 
> 
> Heiko
> 
>> 1 file changed, 36 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> index 19bf84f0bc67..5413fa73dd45 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> @@ -87,6 +87,7 @@ struct rockchip_emmc_phy {
>> 	unsigned int	reg_offset;
>> 	struct regmap	*reg_base;
>> 	struct clk	*emmcclk;
>> +	unsigned int drive_impedance;
>> };
>> 
>> static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>> @@ -281,10 +282,10 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>> {
>> 	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
>> 
>> -	/* Drive impedance: 50 Ohm */
>> +	/* Drive impedance: from DTS */
>> 	regmap_write(rk_phy->reg_base,
>> 		     rk_phy->reg_offset + GRF_EMMCPHY_CON6,
>> -		     HIWORD_UPDATE(PHYCTRL_DR_50OHM,
>> +		     HIWORD_UPDATE(rk_phy->drive_impedance,
>> 				   PHYCTRL_DR_MASK,
>> 				   PHYCTRL_DR_SHIFT));
>> 
>> @@ -314,6 +315,28 @@ static const struct phy_ops ops = {
>> 	.owner		= THIS_MODULE,
>> };
>> 
>> +static u32 convert_drive_impedance_ohm(struct platform_device *pdev, u32 dr_ohm)
>> +{
>> +	switch (dr_ohm) {
>> +	case 100:
>> +		return PHYCTRL_DR_100OHM;
>> +	case 66:
>> +		return PHYCTRL_DR_66OHM;
>> +	case 50:
>> +		return PHYCTRL_DR_50OHM;
>> +	case 40:
>> +		return PHYCTRL_DR_40OHM;
>> +	case 33:
>> +		return PHYCTRL_DR_33OHM;
>> +	}
>> +
>> +	dev_warn(&pdev->dev,
>> +		"Invalid value %u for drive-impedance-ohm. "
>> +		"Falling back to 50 Ohm.\n",
>> +		dr_ohm);
>> +	return PHYCTRL_DR_50OHM;
>> +}
>> +
>> static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>> {
>> 	struct device *dev = &pdev->dev;
>> @@ -322,6 +345,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>> 	struct phy_provider *phy_provider;
>> 	struct regmap *grf;
>> 	unsigned int reg_offset;
>> +	u32 val;
>> 
>> 	if (!dev->parent || !dev->parent->of_node)
>> 		return -ENODEV;
>> @@ -345,6 +369,16 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>> 	rk_phy->reg_offset = reg_offset;
>> 	rk_phy->reg_base = grf;
>> 
>> +	if (of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val)) {
>> +		dev_info(dev,
>> +			"Missing drive-impedance-ohm property in node %s "
>> +			"Falling back to 50 Ohm.\n",
>> +			dev->of_node->name);
>> +		rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
>> +	} else {
>> +		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>> +	}
>> +
>> 	generic_phy = devm_phy_create(dev, dev->of_node, &ops);
>> 	if (IS_ERR(generic_phy)) {
>> 		dev_err(dev, "failed to create PHY\n");
>> 
> 
> 
> 
> 

Re: [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.

[Doug Anderson]

Hi,

On Fri, Mar 1, 2019 at 7:37 AM Christoph Muellner
<christoph.muellner@theobroma-systems.com> wrote:
>
> The rockchip-emmc PHY can be configured with different
> drive impedance values. Currenlty a value of 50 Ohm is
> hard coded into the driver.
>
> This patch introduces the DTS property 'drive-impedance-ohm'
> for the rockchip-emmc phy node, which uses the value from the DTS
> to setup the drive impedance accordingly.
>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>  drivers/phy/rockchip/phy-rockchip-emmc.c | 38 ++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
> index 19bf84f0bc67..5413fa73dd45 100644
> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> @@ -87,6 +87,7 @@ struct rockchip_emmc_phy {
>         unsigned int    reg_offset;
>         struct regmap   *reg_base;
>         struct clk      *emmcclk;
> +       unsigned int drive_impedance;
>  };
>
>  static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
> @@ -281,10 +282,10 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>  {
>         struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
>
> -       /* Drive impedance: 50 Ohm */
> +       /* Drive impedance: from DTS */
>         regmap_write(rk_phy->reg_base,
>                      rk_phy->reg_offset + GRF_EMMCPHY_CON6,
> -                    HIWORD_UPDATE(PHYCTRL_DR_50OHM,
> +                    HIWORD_UPDATE(rk_phy->drive_impedance,
>                                    PHYCTRL_DR_MASK,
>                                    PHYCTRL_DR_SHIFT));
>
> @@ -314,6 +315,28 @@ static const struct phy_ops ops = {
>         .owner          = THIS_MODULE,
>  };
>
> +static u32 convert_drive_impedance_ohm(struct platform_device *pdev, u32 dr_ohm)
> +{
> +       switch (dr_ohm) {
> +       case 100:
> +               return PHYCTRL_DR_100OHM;
> +       case 66:
> +               return PHYCTRL_DR_66OHM;
> +       case 50:
> +               return PHYCTRL_DR_50OHM;
> +       case 40:
> +               return PHYCTRL_DR_40OHM;
> +       case 33:
> +               return PHYCTRL_DR_33OHM;
> +       }
> +
> +       dev_warn(&pdev->dev,
> +               "Invalid value %u for drive-impedance-ohm. "
> +               "Falling back to 50 Ohm.\n",
> +               dr_ohm);
> +       return PHYCTRL_DR_50OHM;
> +}
> +
>  static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -322,6 +345,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>         struct phy_provider *phy_provider;
>         struct regmap *grf;
>         unsigned int reg_offset;
> +       u32 val;
>
>         if (!dev->parent || !dev->parent->of_node)
>                 return -ENODEV;
> @@ -345,6 +369,16 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>         rk_phy->reg_offset = reg_offset;
>         rk_phy->reg_base = grf;
>
> +       if (of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val)) {
> +               dev_info(dev,
> +                       "Missing drive-impedance-ohm property in node %s "
> +                       "Falling back to 50 Ohm.\n",
> +                       dev->of_node->name);

This is awfully noisy for something that pretty much all existing
boards will run.  debug level instead of info level?  Also:

* Don't split strings
across lines

* There's a magic % thing to get the name of an OF node.  Use that.


> +               rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> +       } else {
> +               rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
> +       }

It's been a long time since I looked at this, but I could have sworn
that it was more complicated than that.  Specifically I though you
were supposed to query the eMMC card for what it supported and then
resolve that with what the host could support.

Assuming that this is supposed to be queried from the card (which is
how I remember it) then you definitely don't want it in the device
tree since you want to be able to stuff various different eMMC parts
and we should be able to figure out the impedance at runtime.


NOTE: IIRC the old value of 50 ohms is required to be supported by all
hosts and cards and is specified to be the default.


I do see at least the summary of what I thought of this before at
<https://patchwork.kernel.org/patch/9086541/>


(Sorry if the above is wrong and feel free to correct me--I don't have
time at the moment to do all the full research but hopefully you can
dig more based on my pointers.  Feel free to call me on my BS)


-Doug

Re: [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.

[Robin Murphy]

On 01/03/2019 15:33, Christoph Muellner wrote:
> The rockchip-emmc PHY can be configured with different
> drive impedance values. Currenlty a value of 50 Ohm is
> hard coded into the driver.
> 
> This patch introduces the DTS property 'drive-impedance-ohm'
> for the rockchip-emmc phy node, which uses the value from the DTS
> to setup the drive impedance accordingly.
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>   drivers/phy/rockchip/phy-rockchip-emmc.c | 38 ++++++++++++++++++++++++++++++--
>   1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
> index 19bf84f0bc67..5413fa73dd45 100644
> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> @@ -87,6 +87,7 @@ struct rockchip_emmc_phy {
>   	unsigned int	reg_offset;
>   	struct regmap	*reg_base;
>   	struct clk	*emmcclk;
> +	unsigned int drive_impedance;
>   };
>   
>   static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
> @@ -281,10 +282,10 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>   {
>   	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
>   
> -	/* Drive impedance: 50 Ohm */
> +	/* Drive impedance: from DTS */
>   	regmap_write(rk_phy->reg_base,
>   		     rk_phy->reg_offset + GRF_EMMCPHY_CON6,
> -		     HIWORD_UPDATE(PHYCTRL_DR_50OHM,
> +		     HIWORD_UPDATE(rk_phy->drive_impedance,
>   				   PHYCTRL_DR_MASK,
>   				   PHYCTRL_DR_SHIFT));
>   
> @@ -314,6 +315,28 @@ static const struct phy_ops ops = {
>   	.owner		= THIS_MODULE,
>   };
>   
> +static u32 convert_drive_impedance_ohm(struct platform_device *pdev, u32 dr_ohm)
> +{
> +	switch (dr_ohm) {
> +	case 100:
> +		return PHYCTRL_DR_100OHM;
> +	case 66:
> +		return PHYCTRL_DR_66OHM;
> +	case 50:
> +		return PHYCTRL_DR_50OHM;
> +	case 40:
> +		return PHYCTRL_DR_40OHM;
> +	case 33:
> +		return PHYCTRL_DR_33OHM;
> +	}
> +
> +	dev_warn(&pdev->dev,
> +		"Invalid value %u for drive-impedance-ohm. "
> +		"Falling back to 50 Ohm.\n",
> +		dr_ohm);
> +	return PHYCTRL_DR_50OHM;
> +}
> +
>   static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -322,6 +345,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>   	struct phy_provider *phy_provider;
>   	struct regmap *grf;
>   	unsigned int reg_offset;
> +	u32 val;
>   
>   	if (!dev->parent || !dev->parent->of_node)
>   		return -ENODEV;
> @@ -345,6 +369,16 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>   	rk_phy->reg_offset = reg_offset;
>   	rk_phy->reg_base = grf;
>   
> +	if (of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val)) {
> +		dev_info(dev,
> +			"Missing drive-impedance-ohm property in node %s "
> +			"Falling back to 50 Ohm.\n",
> +			dev->of_node->name);

I think we're aiming to use %pOF for node names now, but here it looks 
redundant anyway - dev_info() will already show the device name, which 
for an of_platform device is the same thing.

> +		rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> +	} else {
> +		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
> +	}

FWIW, if you initialise val to the default (i.e. 50) then you could get 
rid of the "else" case and just do the conversion unconditionally. 
That's a fairly common idiom for reading optional properties.

Robin.

> +
>   	generic_phy = devm_phy_create(dev, dev->of_node, &ops);
>   	if (IS_ERR(generic_phy)) {
>   		dev_err(dev, "failed to create PHY\n");
> 

Re: [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.

[Philipp Tomsich]

Doug,

> On 01.03.2019, at 17:48, Doug Anderson <dianders@chromium.org> wrote:
> 
> Hi,
> 
> On Fri, Mar 1, 2019 at 7:37 AM Christoph Muellner
> <christoph.muellner@theobroma-systems.com> wrote:
>> 
>> The rockchip-emmc PHY can be configured with different
>> drive impedance values. Currenlty a value of 50 Ohm is
>> hard coded into the driver.
>> 
>> This patch introduces the DTS property 'drive-impedance-ohm'
>> for the rockchip-emmc phy node, which uses the value from the DTS
>> to setup the drive impedance accordingly.
>> 
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>> drivers/phy/rockchip/phy-rockchip-emmc.c | 38 ++++++++++++++++++++++++++++++--
>> 1 file changed, 36 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> index 19bf84f0bc67..5413fa73dd45 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> @@ -87,6 +87,7 @@ struct rockchip_emmc_phy {
>>        unsigned int    reg_offset;
>>        struct regmap   *reg_base;
>>        struct clk      *emmcclk;
>> +       unsigned int drive_impedance;
>> };
>> 
>> static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>> @@ -281,10 +282,10 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>> {
>>        struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
>> 
>> -       /* Drive impedance: 50 Ohm */
>> +       /* Drive impedance: from DTS */
>>        regmap_write(rk_phy->reg_base,
>>                     rk_phy->reg_offset + GRF_EMMCPHY_CON6,
>> -                    HIWORD_UPDATE(PHYCTRL_DR_50OHM,
>> +                    HIWORD_UPDATE(rk_phy->drive_impedance,
>>                                   PHYCTRL_DR_MASK,
>>                                   PHYCTRL_DR_SHIFT));
>> 
>> @@ -314,6 +315,28 @@ static const struct phy_ops ops = {
>>        .owner          = THIS_MODULE,
>> };
>> 
>> +static u32 convert_drive_impedance_ohm(struct platform_device *pdev, u32 dr_ohm)
>> +{
>> +       switch (dr_ohm) {
>> +       case 100:
>> +               return PHYCTRL_DR_100OHM;
>> +       case 66:
>> +               return PHYCTRL_DR_66OHM;
>> +       case 50:
>> +               return PHYCTRL_DR_50OHM;
>> +       case 40:
>> +               return PHYCTRL_DR_40OHM;
>> +       case 33:
>> +               return PHYCTRL_DR_33OHM;
>> +       }
>> +
>> +       dev_warn(&pdev->dev,
>> +               "Invalid value %u for drive-impedance-ohm. "
>> +               "Falling back to 50 Ohm.\n",
>> +               dr_ohm);
>> +       return PHYCTRL_DR_50OHM;
>> +}
>> +
>> static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>> {
>>        struct device *dev = &pdev->dev;
>> @@ -322,6 +345,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>        struct phy_provider *phy_provider;
>>        struct regmap *grf;
>>        unsigned int reg_offset;
>> +       u32 val;
>> 
>>        if (!dev->parent || !dev->parent->of_node)
>>                return -ENODEV;
>> @@ -345,6 +369,16 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>        rk_phy->reg_offset = reg_offset;
>>        rk_phy->reg_base = grf;
>> 
>> +       if (of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val)) {
>> +               dev_info(dev,
>> +                       "Missing drive-impedance-ohm property in node %s "
>> +                       "Falling back to 50 Ohm.\n",
>> +                       dev->of_node->name);
> 
> This is awfully noisy for something that pretty much all existing
> boards will run.  debug level instead of info level?  Also:
> 
> * Don't split strings
> across lines
> 
> * There's a magic % thing to get the name of an OF node.  Use that.
> 
> 
>> +               rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
>> +       } else {
>> +               rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>> +       }
> 
> It's been a long time since I looked at this, but I could have sworn
> that it was more complicated than that.  Specifically I though you
> were supposed to query the eMMC card for what it supported and then
> resolve that with what the host could support.
> 
> Assuming that this is supposed to be queried from the card (which is
> how I remember it) then you definitely don't want it in the device
> tree since you want to be able to stuff various different eMMC parts
> and we should be able to figure out the impedance at runtime.

Not necessarily (i.e. there’s not a bijective relationship, as far as I know):
Higher drive-strenghts allow for faster speeds, but lower drive-strenghts
may have an improved emission profile.  For the RK3399-Q7, emissions
with a 33 Ohm preset are ok and we don’t see any reflections.

The chip documentation has the following info:
	Impedance	Relative		Remark
	50 Ohm		x1			Default driver type, supports up to 200MHz operation
	33 Ohm		x1.5			Supports up to 200MHz operation
	66 Ohm		x0.75		The weakest driver that supports up to 200MHz operation.
	100 Ohm		x0.5			For Low noise and low EMI systems, Minimum operating frequency is system dependent.
	40 Ohm		x1.2			Supports up to DDR 200MHz operation.


> NOTE: IIRC the old value of 50 ohms is required to be supported by all
> hosts and cards and is specified to be the default.

For our board, we have a series resistor in the line (originally a precaution against
emissions and the BIOS_DISABLE circuitry) — do there’s more dampening on the
signal than on the reference design.

> I do see at least the summary of what I thought of this before at
> <https://patchwork.kernel.org/patch/9086541/>
> 
> 
> (Sorry if the above is wrong and feel free to correct me--I don't have
> time at the moment to do all the full research but hopefully you can
> dig more based on my pointers.  Feel free to call me on my BS)
> 
> 
> -Doug

Re: [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.

[Christoph Müllner]

Hi Robin,

> On 01.03.2019, at 17:51, Robin Murphy <robin.murphy@arm.com> wrote:
> 
> On 01/03/2019 15:33, Christoph Muellner wrote:
>> The rockchip-emmc PHY can be configured with different
>> drive impedance values. Currenlty a value of 50 Ohm is
>> hard coded into the driver.
>> This patch introduces the DTS property 'drive-impedance-ohm'
>> for the rockchip-emmc phy node, which uses the value from the DTS
>> to setup the drive impedance accordingly.
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-emmc.c | 38 ++++++++++++++++++++++++++++++--
>>  1 file changed, 36 insertions(+), 2 deletions(-)
>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> index 19bf84f0bc67..5413fa73dd45 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> @@ -87,6 +87,7 @@ struct rockchip_emmc_phy {
>>  	unsigned int	reg_offset;
>>  	struct regmap	*reg_base;
>>  	struct clk	*emmcclk;
>> +	unsigned int drive_impedance;
>>  };
>>    static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>> @@ -281,10 +282,10 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>>  {
>>  	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
>>  -	/* Drive impedance: 50 Ohm */
>> +	/* Drive impedance: from DTS */
>>  	regmap_write(rk_phy->reg_base,
>>  		     rk_phy->reg_offset + GRF_EMMCPHY_CON6,
>> -		     HIWORD_UPDATE(PHYCTRL_DR_50OHM,
>> +		     HIWORD_UPDATE(rk_phy->drive_impedance,
>>  				   PHYCTRL_DR_MASK,
>>  				   PHYCTRL_DR_SHIFT));
>>  @@ -314,6 +315,28 @@ static const struct phy_ops ops = {
>>  	.owner		= THIS_MODULE,
>>  };
>>  +static u32 convert_drive_impedance_ohm(struct platform_device *pdev, u32 dr_ohm)
>> +{
>> +	switch (dr_ohm) {
>> +	case 100:
>> +		return PHYCTRL_DR_100OHM;
>> +	case 66:
>> +		return PHYCTRL_DR_66OHM;
>> +	case 50:
>> +		return PHYCTRL_DR_50OHM;
>> +	case 40:
>> +		return PHYCTRL_DR_40OHM;
>> +	case 33:
>> +		return PHYCTRL_DR_33OHM;
>> +	}
>> +
>> +	dev_warn(&pdev->dev,
>> +		"Invalid value %u for drive-impedance-ohm. "
>> +		"Falling back to 50 Ohm.\n",
>> +		dr_ohm);
>> +	return PHYCTRL_DR_50OHM;
>> +}
>> +
>>  static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> @@ -322,6 +345,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>  	struct phy_provider *phy_provider;
>>  	struct regmap *grf;
>>  	unsigned int reg_offset;
>> +	u32 val;
>>    	if (!dev->parent || !dev->parent->of_node)
>>  		return -ENODEV;
>> @@ -345,6 +369,16 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>  	rk_phy->reg_offset = reg_offset;
>>  	rk_phy->reg_base = grf;
>>  +	if (of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val)) {
>> +		dev_info(dev,
>> +			"Missing drive-impedance-ohm property in node %s "
>> +			"Falling back to 50 Ohm.\n",
>> +			dev->of_node->name);
> 
> I think we're aiming to use %pOF for node names now, but here it looks redundant anyway - dev_info() will already show the device name, which for an of_platform device is the same thing.
> 
>> +		rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
>> +	} else {
>> +		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>> +	}
> 
> FWIW, if you initialise val to the default (i.e. 50) then you could get rid of the "else" case and just do the conversion unconditionally. That's a fairly common idiom for reading optional properties.

Ack. For v2 I've removed the else case (not defined value).

Thanks,
Christoph

> 
> Robin.
> 
>> +
>>  	generic_phy = devm_phy_create(dev, dev->of_node, &ops);
>>  	if (IS_ERR(generic_phy)) {
>>  		dev_err(dev, "failed to create PHY\n");

Re: [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.

[Christoph Müllner]

Hi Doug,

> On 01.03.2019, at 17:48, Doug Anderson <dianders@chromium.org> wrote:
> 
> Hi,
> 
> On Fri, Mar 1, 2019 at 7:37 AM Christoph Muellner
> <christoph.muellner@theobroma-systems.com> wrote:
>> 
>> The rockchip-emmc PHY can be configured with different
>> drive impedance values. Currenlty a value of 50 Ohm is
>> hard coded into the driver.
>> 
>> This patch introduces the DTS property 'drive-impedance-ohm'
>> for the rockchip-emmc phy node, which uses the value from the DTS
>> to setup the drive impedance accordingly.
>> 
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>> drivers/phy/rockchip/phy-rockchip-emmc.c | 38 ++++++++++++++++++++++++++++++--
>> 1 file changed, 36 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> index 19bf84f0bc67..5413fa73dd45 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> @@ -87,6 +87,7 @@ struct rockchip_emmc_phy {
>>        unsigned int    reg_offset;
>>        struct regmap   *reg_base;
>>        struct clk      *emmcclk;
>> +       unsigned int drive_impedance;
>> };
>> 
>> static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>> @@ -281,10 +282,10 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>> {
>>        struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
>> 
>> -       /* Drive impedance: 50 Ohm */
>> +       /* Drive impedance: from DTS */
>>        regmap_write(rk_phy->reg_base,
>>                     rk_phy->reg_offset + GRF_EMMCPHY_CON6,
>> -                    HIWORD_UPDATE(PHYCTRL_DR_50OHM,
>> +                    HIWORD_UPDATE(rk_phy->drive_impedance,
>>                                   PHYCTRL_DR_MASK,
>>                                   PHYCTRL_DR_SHIFT));
>> 
>> @@ -314,6 +315,28 @@ static const struct phy_ops ops = {
>>        .owner          = THIS_MODULE,
>> };
>> 
>> +static u32 convert_drive_impedance_ohm(struct platform_device *pdev, u32 dr_ohm)
>> +{
>> +       switch (dr_ohm) {
>> +       case 100:
>> +               return PHYCTRL_DR_100OHM;
>> +       case 66:
>> +               return PHYCTRL_DR_66OHM;
>> +       case 50:
>> +               return PHYCTRL_DR_50OHM;
>> +       case 40:
>> +               return PHYCTRL_DR_40OHM;
>> +       case 33:
>> +               return PHYCTRL_DR_33OHM;
>> +       }
>> +
>> +       dev_warn(&pdev->dev,
>> +               "Invalid value %u for drive-impedance-ohm. "
>> +               "Falling back to 50 Ohm.\n",
>> +               dr_ohm);
>> +       return PHYCTRL_DR_50OHM;
>> +}
>> +
>> static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>> {
>>        struct device *dev = &pdev->dev;
>> @@ -322,6 +345,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>        struct phy_provider *phy_provider;
>>        struct regmap *grf;
>>        unsigned int reg_offset;
>> +       u32 val;
>> 
>>        if (!dev->parent || !dev->parent->of_node)
>>                return -ENODEV;
>> @@ -345,6 +369,16 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>        rk_phy->reg_offset = reg_offset;
>>        rk_phy->reg_base = grf;
>> 
>> +       if (of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val)) {
>> +               dev_info(dev,
>> +                       "Missing drive-impedance-ohm property in node %s "
>> +                       "Falling back to 50 Ohm.\n",
>> +                       dev->of_node->name);
> 
> This is awfully noisy for something that pretty much all existing
> boards will run.  debug level instead of info level?  Also:

Removed for v2 as suggested by Robin.

> 
> * Don't split strings
> across lines
> 
> * There's a magic % thing to get the name of an OF node.  Use that.
> 
> 
>> +               rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
>> +       } else {
>> +               rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>> +       }
> 
> It's been a long time since I looked at this, but I could have sworn
> that it was more complicated than that.  Specifically I though you
> were supposed to query the eMMC card for what it supported and then
> resolve that with what the host could support.
> 
> Assuming that this is supposed to be queried from the card (which is
> how I remember it) then you definitely don't want it in the device
> tree since you want to be able to stuff various different eMMC parts
> and we should be able to figure out the impedance at runtime.
> 
> 
> NOTE: IIRC the old value of 50 ohms is required to be supported by all
> hosts and cards and is specified to be the default.
> 

When using 50 ohms on our board in HS-400 mode (200 MHz), then
we see communication errors. These are cause by additional components
on the clock line, which damp the 200 Mhz signal too much.

We could do the following:

* sdhci.c provides a default implementation to set the drive impedance (in HOST_CONTROL2,
  like it is done already now as part of sdhci_set_ios())
* sdhci-of-arasan.c installs an alternative implementation in case of Rockchip's eMMC controller
* the alternative implementation uses a callback to the Rockchip's eMMC phy driver
* the Rockchip eMMC phy driver sets the drive impedance accordingly

But still we would need a mechanism to override this for our board.
And exactly this override mechanism is provided by the patch series.

Thanks,
Christoph

> 
> I do see at least the summary of what I thought of this before at
> <https://patchwork.kernel.org/patch/9086541/>
> 
> 
> (Sorry if the above is wrong and feel free to correct me--I don't have
> time at the moment to do all the full research but hopefully you can
> dig more based on my pointers.  Feel free to call me on my BS)
> 
> 
> -Doug

Re: [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.

[Philipp Tomsich]

> On 01.03.2019, at 19:09, Christoph Müllner <christoph.muellner@theobroma-systems.com> wrote:
> 
> Hi Doug,
> 
>> On 01.03.2019, at 17:48, Doug Anderson <dianders@chromium.org> wrote:
>> 
>> Hi,
>> 
>> On Fri, Mar 1, 2019 at 7:37 AM Christoph Muellner
>> <christoph.muellner@theobroma-systems.com> wrote:
>>> 
>>> The rockchip-emmc PHY can be configured with different
>>> drive impedance values. Currenlty a value of 50 Ohm is
>>> hard coded into the driver.
>>> 
>>> This patch introduces the DTS property 'drive-impedance-ohm'
>>> for the rockchip-emmc phy node, which uses the value from the DTS
>>> to setup the drive impedance accordingly.
>>> 
>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>> ---
>>> drivers/phy/rockchip/phy-rockchip-emmc.c | 38 ++++++++++++++++++++++++++++++--
>>> 1 file changed, 36 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> index 19bf84f0bc67..5413fa73dd45 100644
>>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> @@ -87,6 +87,7 @@ struct rockchip_emmc_phy {
>>>       unsigned int    reg_offset;
>>>       struct regmap   *reg_base;
>>>       struct clk      *emmcclk;
>>> +       unsigned int drive_impedance;
>>> };
>>> 
>>> static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>>> @@ -281,10 +282,10 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>>> {
>>>       struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
>>> 
>>> -       /* Drive impedance: 50 Ohm */
>>> +       /* Drive impedance: from DTS */
>>>       regmap_write(rk_phy->reg_base,
>>>                    rk_phy->reg_offset + GRF_EMMCPHY_CON6,
>>> -                    HIWORD_UPDATE(PHYCTRL_DR_50OHM,
>>> +                    HIWORD_UPDATE(rk_phy->drive_impedance,
>>>                                  PHYCTRL_DR_MASK,
>>>                                  PHYCTRL_DR_SHIFT));
>>> 
>>> @@ -314,6 +315,28 @@ static const struct phy_ops ops = {
>>>       .owner          = THIS_MODULE,
>>> };
>>> 
>>> +static u32 convert_drive_impedance_ohm(struct platform_device *pdev, u32 dr_ohm)
>>> +{
>>> +       switch (dr_ohm) {
>>> +       case 100:
>>> +               return PHYCTRL_DR_100OHM;
>>> +       case 66:
>>> +               return PHYCTRL_DR_66OHM;
>>> +       case 50:
>>> +               return PHYCTRL_DR_50OHM;
>>> +       case 40:
>>> +               return PHYCTRL_DR_40OHM;
>>> +       case 33:
>>> +               return PHYCTRL_DR_33OHM;
>>> +       }
>>> +
>>> +       dev_warn(&pdev->dev,
>>> +               "Invalid value %u for drive-impedance-ohm. "
>>> +               "Falling back to 50 Ohm.\n",
>>> +               dr_ohm);
>>> +       return PHYCTRL_DR_50OHM;
>>> +}
>>> +
>>> static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>> {
>>>       struct device *dev = &pdev->dev;
>>> @@ -322,6 +345,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>>       struct phy_provider *phy_provider;
>>>       struct regmap *grf;
>>>       unsigned int reg_offset;
>>> +       u32 val;
>>> 
>>>       if (!dev->parent || !dev->parent->of_node)
>>>               return -ENODEV;
>>> @@ -345,6 +369,16 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>>       rk_phy->reg_offset = reg_offset;
>>>       rk_phy->reg_base = grf;
>>> 
>>> +       if (of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val)) {
>>> +               dev_info(dev,
>>> +                       "Missing drive-impedance-ohm property in node %s "
>>> +                       "Falling back to 50 Ohm.\n",
>>> +                       dev->of_node->name);
>> 
>> This is awfully noisy for something that pretty much all existing
>> boards will run.  debug level instead of info level?  Also:
> 
> Removed for v2 as suggested by Robin.
> 
>> 
>> * Don't split strings
>> across lines
>> 
>> * There's a magic % thing to get the name of an OF node.  Use that.
>> 
>> 
>>> +               rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
>>> +       } else {
>>> +               rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>>> +       }
>> 
>> It's been a long time since I looked at this, but I could have sworn
>> that it was more complicated than that.  Specifically I though you
>> were supposed to query the eMMC card for what it supported and then
>> resolve that with what the host could support.
>> 
>> Assuming that this is supposed to be queried from the card (which is
>> how I remember it) then you definitely don't want it in the device
>> tree since you want to be able to stuff various different eMMC parts
>> and we should be able to figure out the impedance at runtime.
>> 
>> 
>> NOTE: IIRC the old value of 50 ohms is required to be supported by all
>> hosts and cards and is specified to be the default.
>> 
> 
> When using 50 ohms on our board in HS-400 mode (200 MHz), then
> we see communication errors. These are cause by additional components
> on the clock line, which damp the 200 Mhz signal too much.
> 
> We could do the following:
> 
> * sdhci.c provides a default implementation to set the drive impedance (in HOST_CONTROL2,
>  like it is done already now as part of sdhci_set_ios())
> * sdhci-of-arasan.c installs an alternative implementation in case of Rockchip's eMMC controller
> * the alternative implementation uses a callback to the Rockchip's eMMC phy driver

Generally, I would not make this either-or (I may have been less than clear
in my comments to the internal ticket), but rather teach the sdhci-driver to
always notify the eMMC PHY driver (if there is one) of the change.

For the RK3399’s sdhci implementation, the bits in the HOST_CONTROL2
register are ‘reserved’ and marked R/O, so the current implementation will
not work anyway and the PHY driver needs to be notified of the change in
requested drive-strength.

> * the Rockchip eMMC phy driver sets the drive impedance accordingly
> 
> But still we would need a mechanism to override this for our board.
> And exactly this override mechanism is provided by the patch series.

The PHY would then need to determine the proper operation point (or an
mapping from a table) to achieve the requested line characteristic for any
given board.  In other words: the DTS for puma would still contain an entry
to allow us to override to 33 Ohm when switching to HS-400.

> Thanks,
> Christoph
> 
>> 
>> I do see at least the summary of what I thought of this before at
>> <https://patchwork.kernel.org/patch/9086541/>
>> 
>> 
>> (Sorry if the above is wrong and feel free to correct me--I don't have
>> time at the moment to do all the full research but hopefully you can
>> dig more based on my pointers.  Feel free to call me on my BS)
>> 
>> 
>> -Doug

arm64: dts: rockchip: Disable HS400 for mmc on rk3399-roc-pc

[Markus Reichl]

Working with rootfs on two 128GB mmcs on rk3399-roc-pc.

One (mmc name 128G72, one screw hole) works fine in HS400 mode.
Other (mmc name DJNB4R, firefly on pcb, two screw holes) gets lots of
mmc1: "running CQE recovery", even hangs with damaged fs,
when running under heavy load, e.g. compiling kernel.
Both run fine with HS200.

Disabling CQ with patch mmc: core: Add MMC Command Queue Support kernel parameter [0] did not help.
[0] https://gitlab.com/ayufan-repos/rock64/linux-mainline-kernel/commit/54e264154b87dfe32a8359b2726e2d5611adbaf3

Therefore I propose to disable HS400 mode on roc-pc for now.

Signed-off-by: Markus Reichl <m.reichl@fivetechno.de>
---
 arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
index 29a50a083c42..33df95e384b4 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
@@ -660,8 +660,6 @@
 
 &sdhci {
 	bus-width = <8>;
-	mmc-hs400-1_8v;
-	mmc-hs400-enhanced-strobe;
 	non-removable;
 	status = "okay";
 };
-- 
2.20.1

Re: arm64: dts: rockchip: Disable HS400 for mmc on rk3399-roc-pc

[Heiko Stuebner]

Hi Markus,

$subject is missing the [PATCH] prefix

Am Montag, 11. November 2019, 10:51:04 CET schrieb Markus Reichl:
> Working with rootfs on two 128GB mmcs on rk3399-roc-pc.
> 
> One (mmc name 128G72, one screw hole) works fine in HS400 mode.
> Other (mmc name DJNB4R, firefly on pcb, two screw holes) gets lots of
> mmc1: "running CQE recovery", even hangs with damaged fs,
> when running under heavy load, e.g. compiling kernel.
> Both run fine with HS200.
> 
> Disabling CQ with patch mmc: core: Add MMC Command Queue Support kernel parameter [0] did not help.
> [0] https://gitlab.com/ayufan-repos/rock64/linux-mainline-kernel/commit/54e264154b87dfe32a8359b2726e2d5611adbaf3

I'm hoping for some input from other people in Cc but your mail headers
also referenced the drive-impendance series from Christoph [0], which
it seems we need to poke the phy maintainer again.

Did you check if changing the impedance helped (like the signal dampening
Philipp described in one of the replies there).

[0] https://patchwork.kernel.org/patch/10835567/
most current v2 it seems is https://patchwork.kernel.org/patch/10842421/

> Therefore I propose to disable HS400 mode on roc-pc for now.

Hoping for more input :-)


Heiko


> Signed-off-by: Markus Reichl <m.reichl@fivetechno.de>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
> index 29a50a083c42..33df95e384b4 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
> @@ -660,8 +660,6 @@
>  
>  &sdhci {
>  	bus-width = <8>;
> -	mmc-hs400-1_8v;
> -	mmc-hs400-enhanced-strobe;
>  	non-removable;
>  	status = "okay";
>  };
> 

Re: arm64: dts: rockchip: Disable HS400 for mmc on rk3399-roc-pc

[Markus Reichl]

Hi Heiko,

Am 14.11.19 um 14:10 schrieb Heiko Stuebner:
> Hi Markus,
> 
> $subject is missing the [PATCH] prefix
will fix.
> 
> Am Montag, 11. November 2019, 10:51:04 CET schrieb Markus Reichl:
>> Working with rootfs on two 128GB mmcs on rk3399-roc-pc.
>> 
>> One (mmc name 128G72, one screw hole) works fine in HS400 mode.
>> Other (mmc name DJNB4R, firefly on pcb, two screw holes) gets lots of
>> mmc1: "running CQE recovery", even hangs with damaged fs,
>> when running under heavy load, e.g. compiling kernel.
>> Both run fine with HS200.
>> 
>> Disabling CQ with patch mmc: core: Add MMC Command Queue Support kernel parameter [0] did not help.
>> [0] https://gitlab.com/ayufan-repos/rock64/linux-mainline-kernel/commit/54e264154b87dfe32a8359b2726e2d5611adbaf3
> 
> I'm hoping for some input from other people in Cc but your mail headers
> also referenced the drive-impendance series from Christoph [0], which
> it seems we need to poke the phy maintainer again.
> 
> Did you check if changing the impedance helped (like the signal dampening
> Philipp described in one of the replies there).

checked with

&emmc_phy {
+       drive-impedance-ohm = <33>;

gives no improvement:
[    1.688590] mmc1: CQHCI version 5.10
[    1.714157] mmc1: SDHCI controller on fe330000.sdhci [fe330000.sdhci] using ADMA
[    1.797822] mmc1: Command Queue Engine enabled
[    1.798259] mmc1: new HS400 Enhanced strobe MMC card at address 0001
[    1.800034] mmcblk1: mmc1:0001 DJNB4R 116 GiB 
[    1.800643] mmcblk1boot0: mmc1:0001 DJNB4R partition 1 4.00 MiB
[    1.801408] mmcblk1boot1: mmc1:0001 DJNB4R partition 2 4.00 MiB
[    1.802886] mmcblk1rpmb: mmc1:0001 DJNB4R partition 3 4.00 MiB, chardev (246:0)
...
[  100.118020] mmc1: running CQE recovery
[  100.120890] ------------[ cut here ]------------
[  100.121310] mmc1: cqhci: spurious TCN for tag 2
[  100.121802] WARNING: CPU: 0 PID: 232 at drivers/mmc/host/cqhci.c:729 cqhci_irq+0x30c/0x480
[  100.122530] Modules linked in: rfkill snd_soc_hdmi_codec dw_hdmi_i2s_audio dw_hdmi_cec rockchipdrm crct10dif_ce analogix_dp dw_mipi_dsi snd_soc_simple_card dw_hdmi cec snd_soc_rockchip_i2s snd_soc_simple_card_utils panfrost snd_soc_rockchip_pcm drm_kms_helper snd_soc_core gpu_sched snd_pcm_dmaengine syscopyarea sysfillrect sysimgblt snd_pcm fb_sys_fops snd_timer snd drm soundcore fusb302 tcpm typec rockchip_saradc drm_panel_orientation_quirks ipv6
[  100.126097] CPU: 0 PID: 232 Comm: kworker/0:2H Not tainted 5.4.0-rc7-next-20191112-00007-g2ad177c77749 #44
[  100.126946] Hardware name: Firefly ROC-RK3399-PC Board (DT)
[  100.127454] Workqueue: kblockd blk_mq_run_work_fn
[  100.127885] pstate: 20000085 (nzCv daIf -PAN -UAO)
[  100.128318] pc : cqhci_irq+0x30c/0x480
[  100.128660] lr : cqhci_irq+0x30c/0x480
[  100.128997] sp : ffff800010003d10
[  100.129298] x29: ffff800010003d10 x28: 0000000000000000 
[  100.129777] x27: 0000000000000001 x26: ffff8000108e0fe8 
[  100.130257] x25: ffff0000f5ae5c98 x24: ffff800010b7e1a8 
[  100.130737] x23: ffff800010b198c8 x22: ffff0000f5a80000 
[  100.131216] x21: 0000000000000002 x20: 0000000000000002 
[  100.131694] x19: ffff0000f5ae5c80 x18: 0000000000000010 
[  100.132173] x17: 0000000000000000 x16: 0000000000000000 
[  100.132653] x15: ffffffffffffffff x14: ffff800010b198c8 
[  100.133132] x13: ffff800090003a77 x12: ffff800010003a7f 
[  100.133612] x11: ffff800010b30000 x10: ffff800010003a00 
[  100.134092] x9 : 00000000ffffffd0 x8 : ffff80001045a060 
[  100.134571] x7 : 000000000000025f x6 : ffff800010b979e2 
[  100.135050] x5 : 0000000000000001 x4 : 0000000000000000 
[  100.135529] x3 : 0000000000000000 x2 : 0000000000000000 
[  100.136007] x1 : e005989da46f2500 x0 : 0000000000000000 
[  100.136486] Call trace:
[  100.136719]  cqhci_irq+0x30c/0x480
[  100.137033]  sdhci_arasan_cqhci_irq+0x58/0x80
[  100.137430]  sdhci_irq+0xb8/0xbb8
[  100.137742]  __handle_irq_event_percpu+0x6c/0x170
[  100.138169]  handle_irq_event_percpu+0x34/0x88
[  100.138575]  handle_irq_event+0x44/0xc8
[  100.138927]  handle_fasteoi_irq+0xb4/0x160
[  100.139302]  generic_handle_irq+0x24/0x38
[  100.139670]  __handle_domain_irq+0x60/0xb8
[  100.140045]  gic_handle_irq+0xc0/0x158
[  100.140389]  el1_irq+0xb8/0x180
[  100.140684]  _raw_spin_unlock_irqrestore+0x20/0x48
[  100.141119]  sdhci_cqe_enable+0xec/0x108
[  100.141478]  sdhci_arasan_cqe_enable+0x4c/0x58
[  100.141880]  cqhci_request+0x2d0/0x500
[  100.142226]  mmc_cqe_start_req+0x50/0x60
[  100.142585]  mmc_blk_mq_issue_rq+0x430/0x794
[  100.142974]  mmc_mq_queue_rq+0x104/0x270
[  100.143332]  blk_mq_dispatch_rq_list+0xa8/0x580
[  100.143746]  blk_mq_sched_dispatch_requests+0xf4/0x1d8
[  100.144210]  __blk_mq_run_hw_queue+0xac/0x120
[  100.144605]  blk_mq_run_work_fn+0x1c/0x28
[  100.144974]  process_one_work+0x1e0/0x358
[  100.145340]  worker_thread+0x40/0x488
[  100.145679]  kthread+0x118/0x120
[  100.145977]  ret_from_fork+0x10/0x18
[  100.146302] ---[ end trace 6277099ad751ed64 ]---
[  100.213815] mmc1: running CQE recovery
[  100.413933] mmc1: running CQE recovery

> 
> [0] https://patchwork.kernel.org/patch/10835567/
> most current v2 it seems is https://patchwork.kernel.org/patch/10842421/
> 
>> Therefore I propose to disable HS400 mode on roc-pc for now.
> 
> Hoping for more input :-)

Hoping, too.


Gruß,
-- 
Markus Reichl
> 
> 
> Heiko
> 
> 
>> Signed-off-by: Markus Reichl <m.reichl@fivetechno.de>
>> ---
>>  arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
>> index 29a50a083c42..33df95e384b4 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
>> @@ -660,8 +660,6 @@
>>  
>>  &sdhci {
>>  	bus-width = <8>;
>> -	mmc-hs400-1_8v;
>> -	mmc-hs400-enhanced-strobe;
>>  	non-removable;
>>  	status = "okay";
>>  };
>> 
> 

Re: arm64: dts: rockchip: Disable HS400 for mmc on rk3399-roc-pc

[Heiko Stübner]

Hi Markus,

Am Freitag, 15. November 2019, 11:37:58 CET schrieb Markus Reichl:
> Am 14.11.19 um 14:10 schrieb Heiko Stuebner:
> > $subject is missing the [PATCH] prefix
> will fix.

no need to resend just for this ... just to keep in mind for future patches ;-)


> > Am Montag, 11. November 2019, 10:51:04 CET schrieb Markus Reichl:
> >> Working with rootfs on two 128GB mmcs on rk3399-roc-pc.
> >> 
> >> One (mmc name 128G72, one screw hole) works fine in HS400 mode.
> >> Other (mmc name DJNB4R, firefly on pcb, two screw holes) gets lots of
> >> mmc1: "running CQE recovery", even hangs with damaged fs,
> >> when running under heavy load, e.g. compiling kernel.
> >> Both run fine with HS200.
> >> 
> >> Disabling CQ with patch mmc: core: Add MMC Command Queue Support kernel parameter [0] did not help.
> >> [0] https://gitlab.com/ayufan-repos/rock64/linux-mainline-kernel/commit/54e264154b87dfe32a8359b2726e2d5611adbaf3
> > 
> > I'm hoping for some input from other people in Cc but your mail headers
> > also referenced the drive-impendance series from Christoph [0], which
> > it seems we need to poke the phy maintainer again.
> > 
> > Did you check if changing the impedance helped (like the signal dampening
> > Philipp described in one of the replies there).
> 
> checked with
> 
> &emmc_phy {
> +       drive-impedance-ohm = <33>;
> 
> gives no improvement:

That is sad ... I guess we really should disable hs400 then ...
that may give others more incentive to dive deeper ;-)


Heiko


> [    1.688590] mmc1: CQHCI version 5.10
> [    1.714157] mmc1: SDHCI controller on fe330000.sdhci [fe330000.sdhci] using ADMA
> [    1.797822] mmc1: Command Queue Engine enabled
> [    1.798259] mmc1: new HS400 Enhanced strobe MMC card at address 0001
> [    1.800034] mmcblk1: mmc1:0001 DJNB4R 116 GiB 
> [    1.800643] mmcblk1boot0: mmc1:0001 DJNB4R partition 1 4.00 MiB
> [    1.801408] mmcblk1boot1: mmc1:0001 DJNB4R partition 2 4.00 MiB
> [    1.802886] mmcblk1rpmb: mmc1:0001 DJNB4R partition 3 4.00 MiB, chardev (246:0)
> ...
> [  100.118020] mmc1: running CQE recovery
> [  100.120890] ------------[ cut here ]------------
> [  100.121310] mmc1: cqhci: spurious TCN for tag 2
> [  100.121802] WARNING: CPU: 0 PID: 232 at drivers/mmc/host/cqhci.c:729 cqhci_irq+0x30c/0x480
> [  100.122530] Modules linked in: rfkill snd_soc_hdmi_codec dw_hdmi_i2s_audio dw_hdmi_cec rockchipdrm crct10dif_ce analogix_dp dw_mipi_dsi snd_soc_simple_card dw_hdmi cec snd_soc_rockchip_i2s snd_soc_simple_card_utils panfrost snd_soc_rockchip_pcm drm_kms_helper snd_soc_core gpu_sched snd_pcm_dmaengine syscopyarea sysfillrect sysimgblt snd_pcm fb_sys_fops snd_timer snd drm soundcore fusb302 tcpm typec rockchip_saradc drm_panel_orientation_quirks ipv6
> [  100.126097] CPU: 0 PID: 232 Comm: kworker/0:2H Not tainted 5.4.0-rc7-next-20191112-00007-g2ad177c77749 #44
> [  100.126946] Hardware name: Firefly ROC-RK3399-PC Board (DT)
> [  100.127454] Workqueue: kblockd blk_mq_run_work_fn
> [  100.127885] pstate: 20000085 (nzCv daIf -PAN -UAO)
> [  100.128318] pc : cqhci_irq+0x30c/0x480
> [  100.128660] lr : cqhci_irq+0x30c/0x480
> [  100.128997] sp : ffff800010003d10
> [  100.129298] x29: ffff800010003d10 x28: 0000000000000000 
> [  100.129777] x27: 0000000000000001 x26: ffff8000108e0fe8 
> [  100.130257] x25: ffff0000f5ae5c98 x24: ffff800010b7e1a8 
> [  100.130737] x23: ffff800010b198c8 x22: ffff0000f5a80000 
> [  100.131216] x21: 0000000000000002 x20: 0000000000000002 
> [  100.131694] x19: ffff0000f5ae5c80 x18: 0000000000000010 
> [  100.132173] x17: 0000000000000000 x16: 0000000000000000 
> [  100.132653] x15: ffffffffffffffff x14: ffff800010b198c8 
> [  100.133132] x13: ffff800090003a77 x12: ffff800010003a7f 
> [  100.133612] x11: ffff800010b30000 x10: ffff800010003a00 
> [  100.134092] x9 : 00000000ffffffd0 x8 : ffff80001045a060 
> [  100.134571] x7 : 000000000000025f x6 : ffff800010b979e2 
> [  100.135050] x5 : 0000000000000001 x4 : 0000000000000000 
> [  100.135529] x3 : 0000000000000000 x2 : 0000000000000000 
> [  100.136007] x1 : e005989da46f2500 x0 : 0000000000000000 
> [  100.136486] Call trace:
> [  100.136719]  cqhci_irq+0x30c/0x480
> [  100.137033]  sdhci_arasan_cqhci_irq+0x58/0x80
> [  100.137430]  sdhci_irq+0xb8/0xbb8
> [  100.137742]  __handle_irq_event_percpu+0x6c/0x170
> [  100.138169]  handle_irq_event_percpu+0x34/0x88
> [  100.138575]  handle_irq_event+0x44/0xc8
> [  100.138927]  handle_fasteoi_irq+0xb4/0x160
> [  100.139302]  generic_handle_irq+0x24/0x38
> [  100.139670]  __handle_domain_irq+0x60/0xb8
> [  100.140045]  gic_handle_irq+0xc0/0x158
> [  100.140389]  el1_irq+0xb8/0x180
> [  100.140684]  _raw_spin_unlock_irqrestore+0x20/0x48
> [  100.141119]  sdhci_cqe_enable+0xec/0x108
> [  100.141478]  sdhci_arasan_cqe_enable+0x4c/0x58
> [  100.141880]  cqhci_request+0x2d0/0x500
> [  100.142226]  mmc_cqe_start_req+0x50/0x60
> [  100.142585]  mmc_blk_mq_issue_rq+0x430/0x794
> [  100.142974]  mmc_mq_queue_rq+0x104/0x270
> [  100.143332]  blk_mq_dispatch_rq_list+0xa8/0x580
> [  100.143746]  blk_mq_sched_dispatch_requests+0xf4/0x1d8
> [  100.144210]  __blk_mq_run_hw_queue+0xac/0x120
> [  100.144605]  blk_mq_run_work_fn+0x1c/0x28
> [  100.144974]  process_one_work+0x1e0/0x358
> [  100.145340]  worker_thread+0x40/0x488
> [  100.145679]  kthread+0x118/0x120
> [  100.145977]  ret_from_fork+0x10/0x18
> [  100.146302] ---[ end trace 6277099ad751ed64 ]---
> [  100.213815] mmc1: running CQE recovery
> [  100.413933] mmc1: running CQE recovery
> 
> > 
> > [0] https://patchwork.kernel.org/patch/10835567/
> > most current v2 it seems is https://patchwork.kernel.org/patch/10842421/
> > 
> >> Therefore I propose to disable HS400 mode on roc-pc for now.
> > 
> > Hoping for more input :-)
> 
> Hoping, too.
> 
> 
> Gruß,
> > 
> > 
> > Heiko
> > 
> > 
> >> Signed-off-by: Markus Reichl <m.reichl@fivetechno.de>
> >> ---
> >>  arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi | 2 --
> >>  1 file changed, 2 deletions(-)
> >> 
> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
> >> index 29a50a083c42..33df95e384b4 100644
> >> --- a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
> >> +++ b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
> >> @@ -660,8 +660,6 @@
> >>  
> >>  &sdhci {
> >>  	bus-width = <8>;
> >> -	mmc-hs400-1_8v;
> >> -	mmc-hs400-enhanced-strobe;
> >>  	non-removable;
> >>  	status = "okay";
> >>  };
> >> 
> > 
> 
> 

Re: arm64: dts: rockchip: Disable HS400 for mmc on rk3399-roc-pc

[Heiko Stuebner]

Am Montag, 11. November 2019, 10:51:04 CET schrieb Markus Reichl:
> Working with rootfs on two 128GB mmcs on rk3399-roc-pc.
> 
> One (mmc name 128G72, one screw hole) works fine in HS400 mode.
> Other (mmc name DJNB4R, firefly on pcb, two screw holes) gets lots of
> mmc1: "running CQE recovery", even hangs with damaged fs,
> when running under heavy load, e.g. compiling kernel.
> Both run fine with HS200.
> 
> Disabling CQ with patch mmc: core: Add MMC Command Queue Support kernel parameter [0] did not help.
> [0] https://gitlab.com/ayufan-repos/rock64/linux-mainline-kernel/commit/54e264154b87dfe32a8359b2726e2d5611adbaf3
> 
> Therefore I propose to disable HS400 mode on roc-pc for now.
> 
> Signed-off-by: Markus Reichl <m.reichl@fivetechno.de>

applied for 5.6 (or maybe still 5.5)

Thanks
Heiko

Re: arm64: dts: rockchip: Disable HS400 for mmc on rk3399-roc-pc

[Doug Anderson]

Hi,


On Fri, Nov 15, 2019 at 3:19 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi Markus,
>
> Am Freitag, 15. November 2019, 11:37:58 CET schrieb Markus Reichl:
> > Am 14.11.19 um 14:10 schrieb Heiko Stuebner:
> > > $subject is missing the [PATCH] prefix
> > will fix.
>
> no need to resend just for this ... just to keep in mind for future patches ;-)
>
>
> > > Am Montag, 11. November 2019, 10:51:04 CET schrieb Markus Reichl:
> > >> Working with rootfs on two 128GB mmcs on rk3399-roc-pc.
> > >>
> > >> One (mmc name 128G72, one screw hole) works fine in HS400 mode.
> > >> Other (mmc name DJNB4R, firefly on pcb, two screw holes) gets lots of
> > >> mmc1: "running CQE recovery", even hangs with damaged fs,
> > >> when running under heavy load, e.g. compiling kernel.
> > >> Both run fine with HS200.
> > >>
> > >> Disabling CQ with patch mmc: core: Add MMC Command Queue Support kernel parameter [0] did not help.
> > >> [0] https://gitlab.com/ayufan-repos/rock64/linux-mainline-kernel/commit/54e264154b87dfe32a8359b2726e2d5611adbaf3
> > >
> > > I'm hoping for some input from other people in Cc but your mail headers
> > > also referenced the drive-impendance series from Christoph [0], which
> > > it seems we need to poke the phy maintainer again.
> > >
> > > Did you check if changing the impedance helped (like the signal dampening
> > > Philipp described in one of the replies there).
> >
> > checked with
> >
> > &emmc_phy {
> > +       drive-impedance-ohm = <33>;
> >
> > gives no improvement:
>
> That is sad ... I guess we really should disable hs400 then ...
> that may give others more incentive to dive deeper ;-)

Just out of curiosity, is the problem with the strobe line, or with
hs400?  Have you tried using the solution from "rk3399-gru.dtsi"?
Namely:

        /*
         * Signal integrity isn't great at 200 MHz and 150 MHz (DDR) gives the
         * same (or nearly the same) performance for all eMMC that are intended
         * to be used.
         */
        assigned-clock-rates = <150000000>;

IIRC hs400 on rk3399 was a bit iffy but running at 150 MHz made it
much more reliable and still gave you 300 MB/s transfer rate (so much
better than hs200).  In reality many eMMC chips can't do > 300 MB/s
anyway.

-Doug

Re: arm64: dts: rockchip: Disable HS400 for mmc on rk3399-roc-pc

[Markus Reichl]

Hi Doug,

Am 18.11.19 um 17:08 schrieb Doug Anderson:
> Hi,
> 
> 
> On Fri, Nov 15, 2019 at 3:19 AM Heiko Stübner <heiko@sntech.de> wrote:
>>
>> Hi Markus,
>>
>> Am Freitag, 15. November 2019, 11:37:58 CET schrieb Markus Reichl:
>> > Am 14.11.19 um 14:10 schrieb Heiko Stuebner:
>> > > $subject is missing the [PATCH] prefix
>> > will fix.
>>
>> no need to resend just for this ... just to keep in mind for future patches ;-)
>>
>>
>> > > Am Montag, 11. November 2019, 10:51:04 CET schrieb Markus Reichl:
>> > >> Working with rootfs on two 128GB mmcs on rk3399-roc-pc.
>> > >>
>> > >> One (mmc name 128G72, one screw hole) works fine in HS400 mode.
>> > >> Other (mmc name DJNB4R, firefly on pcb, two screw holes) gets lots of
>> > >> mmc1: "running CQE recovery", even hangs with damaged fs,
>> > >> when running under heavy load, e.g. compiling kernel.
>> > >> Both run fine with HS200.
>> > >>
>> > >> Disabling CQ with patch mmc: core: Add MMC Command Queue Support kernel parameter [0] did not help.
>> > >> [0] https://gitlab.com/ayufan-repos/rock64/linux-mainline-kernel/commit/54e264154b87dfe32a8359b2726e2d5611adbaf3
>> > >
>> > > I'm hoping for some input from other people in Cc but your mail headers
>> > > also referenced the drive-impendance series from Christoph [0], which
>> > > it seems we need to poke the phy maintainer again.
>> > >
>> > > Did you check if changing the impedance helped (like the signal dampening
>> > > Philipp described in one of the replies there).
>> >
>> > checked with
>> >
>> > &emmc_phy {
>> > +       drive-impedance-ohm = <33>;
>> >
>> > gives no improvement:
>>
>> That is sad ... I guess we really should disable hs400 then ...
>> that may give others more incentive to dive deeper ;-)
> 
> Just out of curiosity, is the problem with the strobe line, or with
> hs400?  Have you tried using the solution from "rk3399-gru.dtsi"?
> Namely:
> 
>         /*
>          * Signal integrity isn't great at 200 MHz and 150 MHz (DDR) gives the
>          * same (or nearly the same) performance for all eMMC that are intended
>          * to be used.
>          */
>         assigned-clock-rates = <150000000>;
> 
> IIRC hs400 on rk3399 was a bit iffy but running at 150 MHz made it
> much more reliable and still gave you 300 MB/s transfer rate (so much
> better than hs200).  In reality many eMMC chips can't do > 300 MB/s
> anyway.
> 
I tried 150000000 and 100000000, but it did not help.

Gruß,
--
Markus

> -Doug
>