Meson-8b and Meson-gxbb USB phy code re-structure

RFC In review Anand Moon 3 participants 38 messages Jun 17, 2021 – Jul 2, 2021

Subsystems (2)

Arm/Amlogic Meson SoC support Generic PHY framework

lore

Share & tools

lore

Anand Moon

author

2021-06-17 19:42 UTC #0 lore raw

cover Meson-8b and Meson-gxbb USB phy code re-structure

On Odroid C1+ and Odroid C2 USB feature is broken this code changes are just re-structure the code so that proper callback function execute in proper order.

Code changes losely based on phy-meson-gxl-usb2.c

Initially if we connect USB HDD at initial boot it get detected. but after that usb hotplug of USB HDD is broken.
I did not observe and USB debug events messages to trace the root cause.

Another issue I observed is increase of USB interrupts event even if there is not much activity on USB ports.
I tried some clk changes but it did not workout for me.

$ cat /proc/interrupts | grep usb
 45:          0          0          0          0     GICv2  62 Level     c9000000.usb, dwc2_hsotg:usb1
 46:   17488032          0          0          0     GICv2  63 Level     c9100000.usb, dwc2_hsotg:usb2

Thanks
-Anand

Anand Moon (8):
phy: amlogic: meson8b-usb2: Use clock bulk to get clocks for phy
phy: amlogic: meson8b-usb2: Use phy init callback function
phy: amlogic: meson8b-usb2: Use phy exit callback function
phy: amlogic: meson8b-usb2: Use phy set_mode callback function
phy: amlogic: meson8b-usb2: Reorder phy poweroff callback function
phy: amlogic: meson8b-usb2: Use phy reset callback function
phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset
mode.
phy: amlogic: meson8b-usb2: don't log an error on -EPROBE_DEFER

 drivers/phy/amlogic/phy-meson8b-usb2.c | 159 +++++++++++++++++--------
 1 file changed, 109 insertions(+), 50 deletions(-)

Signature · 1 line

2.31.1

Anand Moon

author

2021-06-17 19:42 UTC #1 lore raw

RFC 1/8 phy: amlogic: meson8b-usb2: Use clock bulk to get clocks for phy

Use clock bulk helpers to get/enable/disable clocks,
it will be easier to handle clocks. No functional change intended.

Cc Martin Blumenstingl <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 44 ++++++++++++++------------
 1 file changed, 23 insertions(+), 21 deletions(-)

drivers/phy/amlogic/phy-meson8b-usb2.c +23 ~~-21~~

index 03c061dd5f0d..771b73f3b44e 100644
@@ -121,11 +121,16 @@ struct phy_meson8b_usb2_match_data {
 	bool			host_enable_aca;
 };
 
+static const char * const meson_phy_clks[] = {
+	"usb_general",
+	"usb",
+};
+
 struct phy_meson8b_usb2_priv {
 	struct regmap					*regmap;
 	enum usb_dr_mode				dr_mode;
-	struct clk					*clk_usb_general;
-	struct clk					*clk_usb;
+	int                                             num_clks;
+	struct clk_bulk_data                            *clks;
 	struct reset_control				*reset;
 	const struct phy_meson8b_usb2_match_data	*match;
 };

@@ -151,16 +156,9 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
 		}
 	}
 
-	ret = clk_prepare_enable(priv->clk_usb_general);
+	ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
 	if (ret) {
-		dev_err(&phy->dev, "Failed to enable USB general clock\n");
-		return ret;
-	}
-
-	ret = clk_prepare_enable(priv->clk_usb);
-	if (ret) {
-		dev_err(&phy->dev, "Failed to enable USB DDR clock\n");
-		clk_disable_unprepare(priv->clk_usb_general);
+		dev_err(&phy->dev, "Failed to enable USB clock\n");
 		return ret;
 	}

@@ -197,8 +195,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
 			regmap_read(priv->regmap, REG_ADP_BC, &reg);
 			if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
 				dev_warn(&phy->dev, "USB ID detect failed!\n");
-				clk_disable_unprepare(priv->clk_usb);
-				clk_disable_unprepare(priv->clk_usb_general);
+				clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
 				return -EINVAL;
 			}
 		}

@@ -216,8 +213,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
 				   REG_DBG_UART_SET_IDDQ,
 				   REG_DBG_UART_SET_IDDQ);
 
-	clk_disable_unprepare(priv->clk_usb);
-	clk_disable_unprepare(priv->clk_usb_general);
+	clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
 
 	return 0;
 }

@@ -234,6 +230,7 @@ static int phy_meson8b_usb2_probe(struct platform_device *pdev)
 	struct phy *phy;
 	struct phy_provider *phy_provider;
 	void __iomem *base;
+	int i, ret;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)

@@ -252,13 +249,18 @@ static int phy_meson8b_usb2_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->regmap))
 		return PTR_ERR(priv->regmap);
 
-	priv->clk_usb_general = devm_clk_get(&pdev->dev, "usb_general");
-	if (IS_ERR(priv->clk_usb_general))
-		return PTR_ERR(priv->clk_usb_general);
+	priv->num_clks = ARRAY_SIZE(meson_phy_clks);
+	priv->clks = devm_kcalloc(&pdev->dev, priv->num_clks,
+				  sizeof(*priv->clks), GFP_KERNEL);
+	if (!priv->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < priv->num_clks; i++)
+		priv->clks[i].id = meson_phy_clks[i];
 
-	priv->clk_usb = devm_clk_get(&pdev->dev, "usb");
-	if (IS_ERR(priv->clk_usb))
-		return PTR_ERR(priv->clk_usb);
+	ret = devm_clk_bulk_get(&pdev->dev, priv->num_clks, priv->clks);
+	if (ret)
+		return ret;
 
 	priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
 	if (PTR_ERR(priv->reset) == -EPROBE_DEFER)

Signature · 1 line

2.31.1

Martin Blumenstingl

reviewer

2021-06-17 22:33 UTC #2 lore raw

Re: [RFCv1 1/8] phy: amlogic: meson8b-usb2: Use clock bulk to get clocks for phy

Hi Anand,

On Thu, Jun 17, 2021 at 9:42 PM Anand Moon <[email protected]> wrote:
[...]

drivers/phy/amlogic/phy-meson8b-usb2.c struct phy_meson8b_usb2_match_data { · line 133

Quoted 2 lines

drivers/phy/amlogic/phy-meson8b-usb2.c +2 -0

+       int                                             num_clks;
+       struct clk_bulk_data                            *clks;

personally I'd get rid of the num_clks as this value is static instead I would add a #define with the number of clocks and then...

Quoted 1 line

        struct reset_control                            *reset;

... use something like:
struct clk_bulk_data clks[MESON8B_USB2_PHY_NUM_CLKS];

[...]

Quoted 1 line

+ dev_err(&phy->dev, "Failed to enable USB clock\n");

clock -> clocks

[...]

drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_probe(struct platform_device *pdev) · line 256

Quoted 5 lines

drivers/phy/amlogic/phy-meson8b-usb2.c +5 -0

+       priv->num_clks = ARRAY_SIZE(meson_phy_clks);
+       priv->clks = devm_kcalloc(&pdev->dev, priv->num_clks,
+                                 sizeof(*priv->clks), GFP_KERNEL);
+       if (!priv->clks)
+               return -ENOMEM;

by using a fixed-size array as suggested above you don't need to dynamically allocate memory anymore

Best regards,
Martin

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-06-18 15:33 UTC #3 lore raw

Re: [RFCv1 1/8] phy: amlogic: meson8b-usb2: Use clock bulk to get clocks for phy

Hi Martin,

Quoted 18 lines · Martin Blumenstingl

Hi Anand,
On Thu, Jun 17, 2021 at 9:42 PM Anand Moon <[email protected]> wrote:
[...]
drivers/phy/amlogic/phy-meson8b-usb2.c struct phy_meson8b_usb2_match_data { · line 133
Quoted 2 lines
drivers/phy/amlogic/phy-meson8b-usb2.c +2 -0
+       int                                             num_clks;
+       struct clk_bulk_data                            *clks;
personally I'd get rid of the num_clks as this value is static instead I would add a #define with the number of clocks and then...
Quoted 1 line
        struct reset_control                            *reset;
... use something like:
struct clk_bulk_data clks[MESON8B_USB2_PHY_NUM_CLKS];
[...]
Quoted 1 line
+ dev_err(&phy->dev, "Failed to enable USB clock\n");
clock -> clocks

Quoted 10 lines

[...]
drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_probe(struct platform_device *pdev) · line 256
Quoted 5 lines
drivers/phy/amlogic/phy-meson8b-usb2.c +5 -0
+       priv->num_clks = ARRAY_SIZE(meson_phy_clks);
+       priv->clks = devm_kcalloc(&pdev->dev, priv->num_clks,
+                                 sizeof(*priv->clks), GFP_KERNEL);
+       if (!priv->clks)
+               return -ENOMEM;
by using a fixed-size array as suggested above you don't need to dynamically allocate memory anymore

Thanks for this tip.It works for me.

Quoted 2 lines

Best regards,
Martin

Thanks

-Anand

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-06-17 19:43 UTC #4 lore raw

RFC 2/8 phy: amlogic: meson8b-usb2: Use phy init callback function

In reply to Anand Moon

Reorder the code for bulk clk_enable into .init callback function.

Cc Martin Blumenstingl <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

drivers/phy/amlogic/phy-meson8b-usb2.c +10 -2

index 771b73f3b44e..d48171b0b32e 100644
@@ -142,10 +142,9 @@ static const struct regmap_config phy_meson8b_usb2_regmap_conf = {
 	.max_register = REG_TUNE,
 };
 
-static int phy_meson8b_usb2_power_on(struct phy *phy)
+static int phy_meson8b_usb2_init(struct phy *phy)
 {
 	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
-	u32 reg;
 	int ret;
 
 	if (!IS_ERR_OR_NULL(priv->reset)) {

@@ -162,6 +161,14 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
 		return ret;
 	}
 
+	return 0;
+}
+
+static int phy_meson8b_usb2_power_on(struct phy *phy)
+{
+	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
+	u32 reg;
+
 	regmap_update_bits(priv->regmap, REG_CONFIG, REG_CONFIG_CLK_32k_ALTSEL,
 			   REG_CONFIG_CLK_32k_ALTSEL);

@@ -219,6 +226,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
 }
 
 static const struct phy_ops phy_meson8b_usb2_ops = {
+	.init           = phy_meson8b_usb2_init,
 	.power_on	= phy_meson8b_usb2_power_on,
 	.power_off	= phy_meson8b_usb2_power_off,
 	.owner		= THIS_MODULE,

Signature · 7 lines

2.31.1


-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Martin Blumenstingl

reviewer

2021-06-18 12:26 UTC #5 lore raw

Re: [RFCv1 2/8] phy: amlogic: meson8b-usb2: Use phy init callback function

Hi Anand,

Quoted 2 lines · Anand Moon

Reorder the code for bulk clk_enable into .init callback function.

Depending on whether changes are made to the dwc2 driver this is either fine or might cause some issues.
My understanding is that drivers which are using a PHY should use the following code-flow:
- phy_init
- phy_power_on
- phy_power_off
- phy_exit

dwc2's platform.c [0] however currently uses the following order:
- phy_init
- phy_power_on
- (remaining ones omitted to keep this example short)

So with this patch and the way dwc2 is currently implemented the phy_meson8b_usb2_power_on implementation might not work anymore. That is because the clocks are turned off and in this case all registers will read 0.
You might be lucky that u-boot left the clocks enabled for your case - but in general I would not rely on this.

In case of the phy-meson-gxl-usb2 PHY driver the ordering is different in the "consumer" driver (dwc3-meson-g12a.c).
There the order is:
- phy_init
- phy_power_on
- (remaining ones omitted to keep this example short)

I don't know if the order in the dwc2 driver can be changed easily (without breaking other platforms).
Until that dwc2 driver issue is resolved I suggest not applying this patch (even though in general I like the approach).
The same thing also applies for patch #3 from this series (for implementing phy_ops.exit)

Best regards,
Martin

[0] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc2/platform.c#L157

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-06-18 13:17 UTC #6 lore raw

Re: [RFCv1 2/8] phy: amlogic: meson8b-usb2: Use phy init callback function

Hi Martin,

Quoted 42 lines · Martin Blumenstingl

Hi Anand,
Quoted 2 lines · Anand Moon
Reorder the code for bulk clk_enable into .init callback function.
Depending on whether changes are made to the dwc2 driver this is either fine or might cause some issues.
My understanding is that drivers which are using a PHY should use the following code-flow:
- phy_init
- phy_power_on
- phy_power_off
- phy_exit
dwc2's platform.c [0] however currently uses the following order:
- phy_init
- phy_power_on
- (remaining ones omitted to keep this example short)
So with this patch and the way dwc2 is currently implemented the phy_meson8b_usb2_power_on implementation might not work anymore. That is because the clocks are turned off and in this case all registers will read 0.
You might be lucky that u-boot left the clocks enabled for your case - but in general I would not rely on this.
In case of the phy-meson-gxl-usb2 PHY driver the ordering is different in the "consumer" driver (dwc3-meson-g12a.c).
There the order is:
- phy_init
- phy_power_on
- (remaining ones omitted to keep this example short)
I don't know if the order in the dwc2 driver can be changed easily (without breaking other platforms).
Until that dwc2 driver issue is resolved I suggest not applying this patch (even though in general I like the approach).
The same thing also applies for patch #3 from this series (for implementing phy_ops.exit)

Quoted 5 lines

Best regards,
Martin
[0] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc2/platform.c#L157

Ok got your point of view.
Thanks for your review comments.

Thanks
-Anand

Anand Moon

author

2021-06-17 19:43 UTC #7 lore raw

RFC 3/8 phy: amlogic: meson8b-usb2: Use phy exit callback function

In reply to Anand Moon

Reorder the code for phy bulkclk disable in .exit callback function.

Cc Martin Blumenstingl <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

drivers/phy/amlogic/phy-meson8b-usb2.c +10 -1

index d48171b0b32e..2b32a3eabccf 100644
@@ -164,6 +164,15 @@ static int phy_meson8b_usb2_init(struct phy *phy)
 	return 0;
 }
 
+static int phy_meson8b_usb2_exit(struct phy *phy)
+{
+	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
+
+	clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
+
+	return 0;
+}
+
 static int phy_meson8b_usb2_power_on(struct phy *phy)
 {
 	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);

@@ -220,13 +229,13 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
 				   REG_DBG_UART_SET_IDDQ,
 				   REG_DBG_UART_SET_IDDQ);
 
-	clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
 
 	return 0;
 }
 
 static const struct phy_ops phy_meson8b_usb2_ops = {
 	.init           = phy_meson8b_usb2_init,
+	.exit           = phy_meson8b_usb2_exit,
 	.power_on	= phy_meson8b_usb2_power_on,
 	.power_off	= phy_meson8b_usb2_power_off,
 	.owner		= THIS_MODULE,

Signature · 7 lines

2.31.1


-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-06-17 19:43 UTC #8 lore raw

RFC 4/8 phy: amlogic: meson8b-usb2: Use phy set_mode callback function

In reply to Anand Moon

Reorder the code for phy set_mode in .set_mode callback function. For now configure the phy in host mode.

Cc Martin Blumenstingl <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 62 ++++++++++++++++++--------
 1 file changed, 43 insertions(+), 19 deletions(-)

drivers/phy/amlogic/phy-meson8b-usb2.c +43 ~~-19~~

index 2b32a3eabccf..18e0986f6ed2 100644
@@ -173,11 +173,43 @@ static int phy_meson8b_usb2_exit(struct phy *phy)
 	return 0;
 }
 
-static int phy_meson8b_usb2_power_on(struct phy *phy)
+static int phy_meson8b_usb2_setmode(struct phy *phy, enum phy_mode mode,
+				    int submode)
 {
 	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
 	u32 reg;
 
+	switch (mode) {
+	case PHY_MODE_USB_HOST:
+		if (priv->match->host_enable_aca) {
+			regmap_update_bits(priv->regmap, REG_ADP_BC,
+					   REG_ADP_BC_ACA_ENABLE,
+					   REG_ADP_BC_ACA_ENABLE);
+
+			udelay(ACA_ENABLE_COMPLETE_TIME);
+
+			regmap_read(priv->regmap, REG_ADP_BC, &reg);
+			if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
+				dev_warn(&phy->dev, "USB ID detect failed!\n");
+				return -EINVAL;
+			}
+		}
+		break;
+	default:
+		dev_warn(&phy->dev, "USB ID detect failed to setnode! %d\n", mode);
+		return -EINVAL;
+	}
+
+	priv->dr_mode = mode;
+
+	return 0;
+}
+
+static int phy_meson8b_usb2_power_on(struct phy *phy)
+{
+	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
+	int ret;
+
 	regmap_update_bits(priv->regmap, REG_CONFIG, REG_CONFIG_CLK_32k_ALTSEL,
 			   REG_CONFIG_CLK_32k_ALTSEL);

@@ -197,24 +229,12 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
 	regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_SOF_TOGGLE_OUT,
 			   REG_CTRL_SOF_TOGGLE_OUT);
 
-	if (priv->dr_mode == USB_DR_MODE_HOST) {
-		regmap_update_bits(priv->regmap, REG_DBG_UART,
-				   REG_DBG_UART_SET_IDDQ, 0);
-
-		if (priv->match->host_enable_aca) {
-			regmap_update_bits(priv->regmap, REG_ADP_BC,
-					   REG_ADP_BC_ACA_ENABLE,
-					   REG_ADP_BC_ACA_ENABLE);
-
-			udelay(ACA_ENABLE_COMPLETE_TIME);
-
-			regmap_read(priv->regmap, REG_ADP_BC, &reg);
-			if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
-				dev_warn(&phy->dev, "USB ID detect failed!\n");
-				clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
-				return -EINVAL;
-			}
-		}
+	ret = phy_meson8b_usb2_setmode(phy, priv->dr_mode, 0);
+	if (ret) {
+		phy_meson8b_usb2_power_off(phy);
+		dev_err(&phy->dev, "Failed to initialize PHY with mode %d\n",
+			priv->dr_mode);
+		return ret;
 	}
 
 	return 0;

@@ -238,6 +258,7 @@ static const struct phy_ops phy_meson8b_usb2_ops = {
 	.exit           = phy_meson8b_usb2_exit,
 	.power_on	= phy_meson8b_usb2_power_on,
 	.power_off	= phy_meson8b_usb2_power_off,
+	.set_mode	= phy_meson8b_usb2_setmode,
 	.owner		= THIS_MODULE,
 };

@@ -261,6 +282,9 @@ static int phy_meson8b_usb2_probe(struct platform_device *pdev)
 	if (!priv->match)
 		return -ENODEV;
 
+	/* start in host mode */
+	priv->dr_mode = PHY_MODE_USB_HOST;
+
 	priv->regmap = devm_regmap_init_mmio(&pdev->dev, base,
 					     &phy_meson8b_usb2_regmap_conf);
 	if (IS_ERR(priv->regmap))

Signature · 1 line

2.31.1

Martin Blumenstingl

reviewer

2021-06-17 22:16 UTC #9 lore raw

Re: [RFCv1 4/8] phy: amlogic: meson8b-usb2: Use phy set_mode callback function

Hi Anand,

Quoted 3 lines · Anand Moon

Reorder the code for phy set_mode in .set_mode callback function. For now configure the phy in host mode.

as mentioned in the cover-letter: to my knowledge these register bits are "static"
The settings for dr_mode == USB_DR_MODE_HOST mainly apply to the second PHY (usb1_phy)

[...]

Quoted 15 lines

drivers/phy/amlogic/phy-meson8b-usb2.c +11 -0

+static int phy_meson8b_usb2_setmode(struct phy *phy, enum phy_mode mode,
+                                   int submode)
 {
        struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
        u32 reg;
 
+       switch (mode) {
+       case PHY_MODE_USB_HOST:
+               if (priv->match->host_enable_aca) {
+                       regmap_update_bits(priv->regmap, REG_ADP_BC,
+                                          REG_ADP_BC_ACA_ENABLE,
+                                          REG_ADP_BC_ACA_ENABLE);
+
+                       udelay(ACA_ENABLE_COMPLETE_TIME);
+

drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_exit(struct phy *phy) · line 200

Quoted 10 lines

drivers/phy/amlogic/phy-meson8b-usb2.c +10 -0

+                       regmap_read(priv->regmap, REG_ADP_BC, &reg);
+                       if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
+                               dev_warn(&phy->dev, "USB ID detect failed!\n");
+                               return -EINVAL;
+                       }
+               }
+               break;
+       default:
+               dev_warn(&phy->dev, "USB ID detect failed to setnode! %d\n", mode);
+               return -EINVAL;

I have tested this driver already with PHY_MODE_USB_DEVICE (on my Odroid-C1) so I don't see why we should drop support for this Also if we want runtime mode switching in this driver then we would need to undo the changes from "case PHY_MODE_USB_HOST" above

I suggest dropping this patch until we know for sure if and which registers need to be updated based on the DR mode.

Best regards,
Martin

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-06-18 13:19 UTC #10 lore raw

Re: [RFCv1 4/8] phy: amlogic: meson8b-usb2: Use phy set_mode callback function

hi Martin.

Thanks for your review comments.

Quoted 43 lines · Martin Blumenstingl

Hi Anand,
Quoted 3 lines · Anand Moon
Reorder the code for phy set_mode in .set_mode callback function. For now configure the phy in host mode.
as mentioned in the cover-letter: to my knowledge these register bits are "static"
The settings for dr_mode == USB_DR_MODE_HOST mainly apply to the second PHY (usb1_phy)
[...]
Quoted 15 lines
(quoted diff) +11 -0
+static int phy_meson8b_usb2_setmode(struct phy *phy, enum phy_mode mode,
+                                   int submode)
 {
        struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
        u32 reg;
 
+       switch (mode) {
+       case PHY_MODE_USB_HOST:
+               if (priv->match->host_enable_aca) {
+                       regmap_update_bits(priv->regmap, REG_ADP_BC,
+                                          REG_ADP_BC_ACA_ENABLE,
+                                          REG_ADP_BC_ACA_ENABLE);
+
+                       udelay(ACA_ENABLE_COMPLETE_TIME);
+
Review
Quoted 10 lines
(quoted diff) +10 -0
+                       regmap_read(priv->regmap, REG_ADP_BC, &reg);
+                       if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
+                               dev_warn(&phy->dev, "USB ID detect failed!\n");
+                               return -EINVAL;
+                       }
+               }
+               break;
+       default:
+               dev_warn(&phy->dev, "USB ID detect failed to setnode! %d\n", mode);
+               return -EINVAL;
I have tested this driver already with PHY_MODE_USB_DEVICE (on my Odroid-C1) so I don't see why we should drop support for this Also if we want runtime mode switching in this driver then we would need to undo the changes from "case PHY_MODE_USB_HOST" above

Quoted 2 lines

I suggest dropping this patch until we know for sure if and which registers need to be updated based on the DR mode.

Yes, I have observed this, Can you give these small changes a try? With the below changes, I got the PHY_MODE_USB_DEVICE support working.

Here is the boot log of odroid c1+
[0] https://pastebin.com/pCXLS5Vu

$ lsusb -t
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
        |__ Port 3: Dev 4, If 0, Class=Mass Storage, Driver=usb-storage, 480M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Human Interface Device, Driver=usbhid, 12M
    |__ Port 1: Dev 2, If 1, Class=Human Interface Device, Driver=usbhid, 12M

git diff drivers/phy/amlogic/phy-meson8b-usb2.c

(quoted diff) +0 -0

b/drivers/phy/amlogic/phy-meson8b-usb2.c
index bd624781d914..9b79e86d7a0d 100644

drivers/phy/amlogic/phy-meson8b-usb2.c +0 -0

@@ -204,6 +204,22 @@ static int phy_meson8b_usb2_setmode(struct phy

*phy, enum phy_mode mode,

                        }
                }
                break;

+ case PHY_MODE_USB_DEVICE:
+ case PHY_MODE_USB_OTG:
+ regmap_update_bits(priv->regmap, REG_ADP_BC,
+ REG_ADP_BC_DCD_ENABLE,
+ REG_ADP_BC_DCD_ENABLE);
+
+ udelay(ACA_ENABLE_COMPLETE_TIME);
+
+ regmap_read(priv->regmap, REG_ADP_BC, &reg);
+ if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
+ dev_warn(&phy->dev, "USB ID detect failed!\n");
+ return -EINVAL;
+ }
+ regmap_update_bits(priv->regmap, REG_ADP_BC,
+ REG_ADP_BC_ID_PULLUP, REG_ADP_BC_ID_PULLUP);

+               break;
        default:
                dev_warn(&phy->dev, "USB ID detect failed to setnode!
%d\n", mode);
                return -EINVAL;

Quoted 4 lines

Best regards,
Martin

Thanks

-Anand

Martin Blumenstingl

reviewer

2021-06-18 20:01 UTC #11 lore raw

Re: [RFCv1 4/8] phy: amlogic: meson8b-usb2: Use phy set_mode callback function

Hi Anand,

On Fri, Jun 18, 2021 at 3:19 PM Anand Moon <[email protected]> wrote:
[...]

Quoted 5 lines

Quoted 2 lines
I suggest dropping this patch until we know for sure if and which registers need to be updated based on the DR mode.
Yes, I have observed this, Can you give these small changes a try? With the below changes, I got the PHY_MODE_USB_DEVICE support working.

first of all: sorry that I have not linked my source of information previously:
- Meson8b: [0]
- Meson8 and Meson8m2: [1]

Unfortunately I don't have any datasheet or "better documentation" of how the registers should be programmed.
This is why I am a bit defensive when I am asked to change something there - as I simply have no way of knowing if the changes are good or not. I can only tell whether they're "identical" or "different" from what the vendor kernel does.

[...]

Quoted 6 lines

(quoted diff) +6 -0

+       case PHY_MODE_USB_DEVICE:
+       case PHY_MODE_USB_OTG:
+               regmap_update_bits(priv->regmap, REG_ADP_BC,
+                                  REG_ADP_BC_DCD_ENABLE,
+                                  REG_ADP_BC_DCD_ENABLE);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+               udelay(ACA_ENABLE_COMPLETE_TIME);
+
+               regmap_read(priv->regmap, REG_ADP_BC, &reg);
+               if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
+                       dev_warn(&phy->dev, "USB ID detect failed!\n");
+                       return -EINVAL;
+               }
+               regmap_update_bits(priv->regmap, REG_ADP_BC,
+                                  REG_ADP_BC_ID_PULLUP, REG_ADP_BC_ID_PULLUP);
+               break;

According to the vendor kernel this should only be applied to "host-only" USB_PORT_IDX_B (which is usb1 in the mainline .dtsi). Based on that I think it's not correct to apply this for DEVICE and OTG modes.

The vendor kernel does not configure REG_ADP_BC_ID_PULLUP anywhere. Also DCD_ENABLE is only ever set to 0 (while you are enabling it now), see [2].

As mentioned before: all I can say about this patch is that it programs the registers differently than the vendor kernel does. From your description I am not sure if you are now getting different behavior on Odroid-C1 with this patch (compared to what we had before).

Best regards,
Martin

[0] https://github.com/endlessm/linux-meson/blob/03393bb8e8478626e03ee93b0a2a225d6de242b5/arch/arm/mach-meson8b/usbclock.c#L120
[1] https://github.com/endlessm/linux-meson/blob/03393bb8e8478626e03ee93b0a2a225d6de242b5/arch/arm/mach-meson8/usbclock.c#L120
[2] https://github.com/endlessm/linux-meson/blob/d6e13c220931110fe676ede6da69fc61a7cb04b6/drivers/amlogic/usb/dwc_otg/310/dwc_otg_pcd.c#L71

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-06-21 07:20 UTC #12 lore raw

Re: [RFCv1 4/8] phy: amlogic: meson8b-usb2: Use phy set_mode callback function

Hi Martin,

Quoted 42 lines · Martin Blumenstingl

Hi Anand,
On Fri, Jun 18, 2021 at 3:19 PM Anand Moon <[email protected]> wrote:
[...]
Quoted 5 lines
Quoted 2 lines
I suggest dropping this patch until we know for sure if and which registers need to be updated based on the DR mode.
Yes, I have observed this, Can you give these small changes a try? With the below changes, I got the PHY_MODE_USB_DEVICE support working.
first of all: sorry that I have not linked my source of information previously:
- Meson8b: [0]
- Meson8 and Meson8m2: [1]
Unfortunately I don't have any datasheet or "better documentation" of how the registers should be programmed.
This is why I am a bit defensive when I am asked to change something there - as I simply have no way of knowing if the changes are good or not. I can only tell whether they're "identical" or "different" from what the vendor kernel does.
[...]
Quoted 6 lines
(quoted diff) +6 -0
+       case PHY_MODE_USB_DEVICE:
+       case PHY_MODE_USB_OTG:
+               regmap_update_bits(priv->regmap, REG_ADP_BC,
+                                  REG_ADP_BC_DCD_ENABLE,
+                                  REG_ADP_BC_DCD_ENABLE);
+
Review
Quoted 10 lines
(quoted diff) +10 -0
+               udelay(ACA_ENABLE_COMPLETE_TIME);
+
+               regmap_read(priv->regmap, REG_ADP_BC, &reg);
+               if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
+                       dev_warn(&phy->dev, "USB ID detect failed!\n");
+                       return -EINVAL;
+               }
+               regmap_update_bits(priv->regmap, REG_ADP_BC,
+                                  REG_ADP_BC_ID_PULLUP, REG_ADP_BC_ID_PULLUP);
+               break;
According to the vendor kernel this should only be applied to "host-only" USB_PORT_IDX_B (which is usb1 in the mainline .dtsi). Based on that I think it's not correct to apply this for DEVICE and OTG modes.

Quoted 9 lines

The vendor kernel does not configure REG_ADP_BC_ID_PULLUP anywhere. Also DCD_ENABLE is only ever set to 0 (while you are enabling it now), see [2].
As mentioned before: all I can say about this patch is that it programs the registers differently than the vendor kernel does. From your description I am not sure if you are now getting different behavior on Odroid-C1 with this patch (compared to what we had before).

In order to enable USB phy we probably need to do a little bit differently than the vendor kernel.
Yes I have observed many configuration parameters are missing.

OTG port on Odroid C1+ and Odroid C2 server two purposes
1 > It could act as USB host port.
2 > It could be used as USB power on the devices, just like Raspberry pi.
What I meant is we need some driver code to protect the power to SbC.

So depending on the mode, it gets configured host mode or PCD mode, I am not completely sure right now.
So I saw your work on extcon, that's the reason I would like to void any changes PHY right now.

I observe some failures like below.

[    6.013859] dwc2 c9000000.usb: DWC OTG HCD URB Dequeue
[    6.013897] dwc2 c9000000.usb: Called usb_hcd_giveback_urb()
[    6.013902] dwc2 c9000000.usb:   urb->status = -115

Still investigating this issue,

Quoted 4 lines

Best regards,
Martin

Yes, I will go through the features for debugging in the future.

Quoted 4 lines

[0] https://github.com/endlessm/linux-meson/blob/03393bb8e8478626e03ee93b0a2a225d6de242b5/arch/arm/mach-meson8b/usbclock.c#L120
[1] https://github.com/endlessm/linux-meson/blob/03393bb8e8478626e03ee93b0a2a225d6de242b5/arch/arm/mach-meson8/usbclock.c#L120
[2] https://github.com/endlessm/linux-meson/blob/d6e13c220931110fe676ede6da69fc61a7cb04b6/drivers/amlogic/usb/dwc_otg/310/dwc_otg_pcd.c#L71

Thanks
-Anand

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Martin Blumenstingl

reviewer

2021-06-22 20:28 UTC #13 lore raw

Re: [RFCv1 4/8] phy: amlogic: meson8b-usb2: Use phy set_mode callback function

Hi Anand,

On Mon, Jun 21, 2021 at 9:20 AM Anand Moon <[email protected]> wrote:
[...]

Quoted 2 lines

In order to enable USB phy we probably need to do a little bit differently than the vendor kernel.

I agree with you here, the vendor kernel is skipping most of the frameworks which we have available in mainline Linux

Quoted 4 lines

OTG port on Odroid C1+ and Odroid C2 server two purposes
1 > It could act as USB host port.
2 > It could be used as USB power on the devices, just like Raspberry pi.
What I meant is we need some driver code to protect the power to SbC.

yep, so we need something that controls mode switching depending on the mode the VBUS regulator needs to be enabled (HOST) or disabled (DEVICE/PERIPHERAL).
VBUS control however is not part of the PHY - in mainline Linux either the dwc2 driver or a USB connector driver are taking care of it.

Quoted 2 lines

So I saw your work on extcon, that's the reason I would like to void any changes PHY right now.

I believe that this specific PHY is unrelated to mode switching. Either it automatically detects the mode and changes it's settings internally or the PHY settings are the same for both modes. Since VBUS control is not the responsibility of the PHY this is certainly possible

My changes are not ready yet but I'll ping you once I have something to test

Quoted 4 lines

I observe some failures like below.

[    6.013859] dwc2 c9000000.usb: DWC OTG HCD URB Dequeue
[    6.013897] dwc2 c9000000.usb: Called usb_hcd_giveback_urb()
[    6.013902] dwc2 c9000000.usb:   urb->status = -115

-115 is -EINPROGRESS but I am not sure if/when that's a bad thing or to be expected

Best regards,
Martin

Anand Moon

author

2021-06-17 19:44 UTC #14 lore raw

RFC 5/8 phy: amlogic: meson8b-usb2: Reorder phy poweroff callback function

In reply to Anand Moon

Move the phy_meson8b_usb2_power_off fundtion to avoid compilation error.

drivers/phy/amlogic/phy-meson8b-usb2.c:247:3: error:

	implicit declaration of function 'phy_meson8b_usb2_power_off';

Cc Martin Blumenstingl <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

drivers/phy/amlogic/phy-meson8b-usb2.c +11 ~~-13~~

index 18e0986f6ed2..ab23a584d7b7 100644
@@ -205,6 +205,17 @@ static int phy_meson8b_usb2_setmode(struct phy *phy, enum phy_mode mode,
 	return 0;
 }
 
+static int phy_meson8b_usb2_power_off(struct phy *phy)
+{
+	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
+
+	if (priv->dr_mode == USB_DR_MODE_HOST)
+		regmap_update_bits(priv->regmap, REG_DBG_UART,
+				   REG_DBG_UART_SET_IDDQ,
+				   REG_DBG_UART_SET_IDDQ);
+	return 0;
+}
+
 static int phy_meson8b_usb2_power_on(struct phy *phy)
 {
 	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);

@@ -240,19 +251,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
 	return 0;
 }
 
-static int phy_meson8b_usb2_power_off(struct phy *phy)
-{
-	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
-
-	if (priv->dr_mode == USB_DR_MODE_HOST)
-		regmap_update_bits(priv->regmap, REG_DBG_UART,
-				   REG_DBG_UART_SET_IDDQ,
-				   REG_DBG_UART_SET_IDDQ);
-
-
-	return 0;
-}
-
 static const struct phy_ops phy_meson8b_usb2_ops = {
 	.init           = phy_meson8b_usb2_init,
 	.exit           = phy_meson8b_usb2_exit,

Signature · 1 line

2.31.1

Martin Blumenstingl

reviewer

2021-06-17 22:17 UTC #15 lore raw

Re: [RFCv1 5/8] phy: amlogic: meson8b-usb2: Reorder phy poweroff callback function

Hi Anand,

Quoted 6 lines · Anand Moon

Move the phy_meson8b_usb2_power_off fundtion to avoid compilation error.
drivers/phy/amlogic/phy-meson8b-usb2.c:247:3: error:
        implicit declaration of function 'phy_meson8b_usb2_power_off';

you have introduced this warning in patch #4 of this series

Best regards,
Martin

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-06-18 15:34 UTC #16 lore raw

Re: [RFCv1 5/8] phy: amlogic: meson8b-usb2: Reorder phy poweroff callback function

Hi Martin,

Quoted 13 lines · Martin Blumenstingl

Hi Anand,
Quoted 6 lines · Anand Moon
Move the phy_meson8b_usb2_power_off fundtion to avoid compilation error.
drivers/phy/amlogic/phy-meson8b-usb2.c:247:3: error:
        implicit declaration of function 'phy_meson8b_usb2_power_off';
you have introduced this warning in patch #4 of this series

Oops, I simply needed a forward declaration of the function to resolve the issue.

Quoted 2 lines

Best regards,
Martin

Thanks

-Anand

kernel test robot

build bot

2021-06-18 00:34 UTC #17 lore raw

Re: [RFCv1 5/8] phy: amlogic: meson8b-usb2: Reorder phy poweroff callback function

In reply to Anand Moon

[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]

Hi Anand,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on linus/master]
[also build test WARNING on v5.13-rc6 next-20210617]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anand-Moon/Meson-8b-and-Meson-gxbb-USB-phy-code-re-structure/20210618-034534
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 39519f6a56e398544d270fcb548de99b54421d43
config: mips-randconfig-s032-20210617 (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0

reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-341-g8af24329-dirty
# https://github.com/0day-ci/linux/commit/e76fc9ed24b7659aab9a154de394a51a70de2bd8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Anand-Moon/Meson-8b-and-Meson-gxbb-USB-phy-code-re-structure/20210618-034534 git checkout e76fc9ed24b7659aab9a154de394a51a70de2bd8
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=mips

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

sparse warnings: (new ones prefixed by >>)
command-line: note: in included file:
builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQUIRE redefined builtin:0:0: sparse: this was the original definition builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_SEQ_CST redefined builtin:0:0: sparse: this was the original definition builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQ_REL redefined builtin:0:0: sparse: this was the original definition builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_RELEASE redefined builtin:0:0: sparse: this was the original definition

>> drivers/phy/amlogic/phy-meson8b-usb2.c:203:25: sparse: sparse: mixing different enum types:
>> drivers/phy/amlogic/phy-meson8b-usb2.c:203:25: sparse:    unsigned int enum phy_mode
>> drivers/phy/amlogic/phy-meson8b-usb2.c:203:25: sparse:    unsigned int enum usb_dr_mode

drivers/phy/amlogic/phy-meson8b-usb2.c:243:49: sparse: sparse: mixing different enum types:
drivers/phy/amlogic/phy-meson8b-usb2.c:243:49: sparse: unsigned int enum usb_dr_mode
drivers/phy/amlogic/phy-meson8b-usb2.c:243:49: sparse: unsigned int enum phy_mode

vim +203 drivers/phy/amlogic/phy-meson8b-usb2.c

51c202a9ef028b Anand Moon 2021-06-17  175  
ee40e59b658c4a Anand Moon 2021-06-17  176  static int phy_meson8b_usb2_setmode(struct phy *phy, enum phy_mode mode,
ee40e59b658c4a Anand Moon 2021-06-17  177  				    int submode)
75c5cd9a01e14e Anand Moon 2021-06-17  178  {
75c5cd9a01e14e Anand Moon 2021-06-17  179  	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
75c5cd9a01e14e Anand Moon 2021-06-17  180  	u32 reg;
75c5cd9a01e14e Anand Moon 2021-06-17  181  
ee40e59b658c4a Anand Moon 2021-06-17  182  	switch (mode) {
ee40e59b658c4a Anand Moon 2021-06-17  183  	case PHY_MODE_USB_HOST:
ee40e59b658c4a Anand Moon 2021-06-17  184  		if (priv->match->host_enable_aca) {
ee40e59b658c4a Anand Moon 2021-06-17  185  			regmap_update_bits(priv->regmap, REG_ADP_BC,
ee40e59b658c4a Anand Moon 2021-06-17  186  					   REG_ADP_BC_ACA_ENABLE,
ee40e59b658c4a Anand Moon 2021-06-17  187  					   REG_ADP_BC_ACA_ENABLE);
ee40e59b658c4a Anand Moon 2021-06-17  188  
ee40e59b658c4a Anand Moon 2021-06-17  189  			udelay(ACA_ENABLE_COMPLETE_TIME);
ee40e59b658c4a Anand Moon 2021-06-17  190  
ee40e59b658c4a Anand Moon 2021-06-17  191  			regmap_read(priv->regmap, REG_ADP_BC, &reg);
ee40e59b658c4a Anand Moon 2021-06-17  192  			if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
ee40e59b658c4a Anand Moon 2021-06-17  193  				dev_warn(&phy->dev, "USB ID detect failed!\n");
ee40e59b658c4a Anand Moon 2021-06-17  194  				return -EINVAL;
ee40e59b658c4a Anand Moon 2021-06-17  195  			}
ee40e59b658c4a Anand Moon 2021-06-17  196  		}
ee40e59b658c4a Anand Moon 2021-06-17  197  		break;
ee40e59b658c4a Anand Moon 2021-06-17  198  	default:
ee40e59b658c4a Anand Moon 2021-06-17  199  		dev_warn(&phy->dev, "USB ID detect failed to setnode! %d\n", mode);
ee40e59b658c4a Anand Moon 2021-06-17  200  		return -EINVAL;
ee40e59b658c4a Anand Moon 2021-06-17  201  	}
ee40e59b658c4a Anand Moon 2021-06-17  202  
ee40e59b658c4a Anand Moon 2021-06-17 @203  	priv->dr_mode = mode;
ee40e59b658c4a Anand Moon 2021-06-17  204  
ee40e59b658c4a Anand Moon 2021-06-17  205  	return 0;
ee40e59b658c4a Anand Moon 2021-06-17  206  }
ee40e59b658c4a Anand Moon 2021-06-17  207

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Anand Moon

author

2021-06-17 19:44 UTC #18 lore raw

RFC 6/8 phy: amlogic: meson8b-usb2: Use phy reset callback function

In reply to Anand Moon

Reoder the code for phy reset mode in .reset callback function. Reset control is shared between two phy so use the phy name as shared id.

Cc Martin Blumenstingl <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 35 ++++++++++++++++++--------
 1 file changed, 25 insertions(+), 10 deletions(-)

drivers/phy/amlogic/phy-meson8b-usb2.c +25 ~~-10~~

index ab23a584d7b7..c1ed2e5c80d8 100644
@@ -133,6 +133,7 @@ struct phy_meson8b_usb2_priv {
 	struct clk_bulk_data                            *clks;
 	struct reset_control				*reset;
 	const struct phy_meson8b_usb2_match_data	*match;
+	int						is_enabled;
 };
 
 static const struct regmap_config phy_meson8b_usb2_regmap_conf = {

@@ -147,14 +148,6 @@ static int phy_meson8b_usb2_init(struct phy *phy)
 	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
 	int ret;
 
-	if (!IS_ERR_OR_NULL(priv->reset)) {
-		ret = reset_control_reset(priv->reset);
-		if (ret) {
-			dev_err(&phy->dev, "Failed to trigger USB reset\n");
-			return ret;
-		}
-	}
-
 	ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
 	if (ret) {
 		dev_err(&phy->dev, "Failed to enable USB clock\n");

@@ -173,6 +166,22 @@ static int phy_meson8b_usb2_exit(struct phy *phy)
 	return 0;
 }
 
+static int phy_meson8b_usb2_reset(struct phy *phy)
+{
+	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
+	int ret;
+
+	if (priv->is_enabled) {
+		ret = reset_control_reset(priv->reset);
+		if (ret) {
+			dev_err(&phy->dev, "Failed to trigger USB reset\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int phy_meson8b_usb2_setmode(struct phy *phy, enum phy_mode mode,
 				    int submode)
 {

@@ -200,6 +209,8 @@ static int phy_meson8b_usb2_setmode(struct phy *phy, enum phy_mode mode,
 		return -EINVAL;
 	}
 
+	phy_meson8b_usb2_reset(phy);
+
 	priv->dr_mode = mode;
 
 	return 0;

@@ -209,6 +220,8 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
 {
 	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
 
+	priv->is_enabled = 0;
+
 	if (priv->dr_mode == USB_DR_MODE_HOST)
 		regmap_update_bits(priv->regmap, REG_DBG_UART,
 				   REG_DBG_UART_SET_IDDQ,

@@ -221,6 +234,8 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
 	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
 	int ret;
 
+	priv->is_enabled = 1;
+
 	regmap_update_bits(priv->regmap, REG_CONFIG, REG_CONFIG_CLK_32k_ALTSEL,
 			   REG_CONFIG_CLK_32k_ALTSEL);

@@ -229,7 +244,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
 
 	regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_FSEL_MASK,
 			   0x5 << REG_CTRL_FSEL_SHIFT);
-
 	/* reset the PHY */
 	regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
 			   REG_CTRL_POWER_ON_RESET);

@@ -257,6 +271,7 @@ static const struct phy_ops phy_meson8b_usb2_ops = {
 	.power_on	= phy_meson8b_usb2_power_on,
 	.power_off	= phy_meson8b_usb2_power_off,
 	.set_mode	= phy_meson8b_usb2_setmode,
+	.reset		= phy_meson8b_usb2_reset,
 	.owner		= THIS_MODULE,
 };

@@ -301,7 +316,7 @@ static int phy_meson8b_usb2_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+	priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
 	if (PTR_ERR(priv->reset) == -EPROBE_DEFER)
 		return PTR_ERR(priv->reset);

Signature · 1 line

2.31.1

Martin Blumenstingl

reviewer

2021-06-17 22:24 UTC #19 lore raw

Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function

Hi Anand,

On Thu, Jun 17, 2021 at 9:44 PM Anand Moon <[email protected]> wrote:
[...]

Quoted 5 lines

drivers/phy/amlogic/phy-meson8b-usb2.c +5 -0

+static int phy_meson8b_usb2_reset(struct phy *phy)
+{
+       struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
+       int ret;
+

drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_exit(struct phy *phy) · line 183

Quoted 10 lines

drivers/phy/amlogic/phy-meson8b-usb2.c +10 -0

+       if (priv->is_enabled) {
+               ret = reset_control_reset(priv->reset);
+               if (ret) {
+                       dev_err(&phy->dev, "Failed to trigger USB reset\n");
+                       return ret;
+               }
+       }
+
+       return 0;
+}

will this reset change the register values back to some kind of default? If it does then we're not re-applying our desired register values afterwards - which is probably asking for trouble.

For shared resets (like the one we have here) reset_control_reset will only trigger the reset line once until all drivers using that reset line are unloaded.
So effectively this new phy_ops.reset callback will be a no-op.

[...]

drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_probe(struct platform_device *pdev) · line 319

Quoted 2 lines

drivers/phy/amlogic/phy-meson8b-usb2.c +1 -1

-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");

I think this breaks compatibility with existing .dtbs and our dt-bindings (as we're not documenting a "reset-names" property). What is the goal of this one?

Best regards,
Martin

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-06-18 15:33 UTC #20 lore raw

Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function

Hi Martin

Quoted 24 lines · Martin Blumenstingl

Hi Anand,
On Thu, Jun 17, 2021 at 9:44 PM Anand Moon <[email protected]> wrote:
[...]
Quoted 5 lines
drivers/phy/amlogic/phy-meson8b-usb2.c +5 -0
+static int phy_meson8b_usb2_reset(struct phy *phy)
+{
+       struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
+       int ret;
+
drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_exit(struct phy *phy) · line 183
Quoted 10 lines
drivers/phy/amlogic/phy-meson8b-usb2.c +10 -0
+       if (priv->is_enabled) {
+               ret = reset_control_reset(priv->reset);
+               if (ret) {
+                       dev_err(&phy->dev, "Failed to trigger USB reset\n");
+                       return ret;
+               }
+       }
+
+       return 0;
+}
will this reset change the register values back to some kind of default? If it does then we're not re-applying our desired register values afterwards - which is probably asking for trouble.

Quoted 4 lines

For shared resets (like the one we have here) reset_control_reset will only trigger the reset line once until all drivers using that reset line are unloaded.
So effectively this new phy_ops.reset callback will be a no-op.

I know his register is shared between two USB IPs, but I have not observed any issues.

Quoted 8 lines

[...]
drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_probe(struct platform_device *pdev) · line 319
Quoted 2 lines
drivers/phy/amlogic/phy-meson8b-usb2.c +1 -1
-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
I think this breaks compatibility with existing .dtbs and our dt-bindings (as we're not documenting a "reset-names" property). What is the goal of this one?

OK, If we pass NULL over here there is the possibility USB phy will not get registered.

Quoted 3 lines

Best regards,
Martin

Thanks

-Anand

Martin Blumenstingl

reviewer

2021-06-18 20:06 UTC #21 lore raw

Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function

Hi Anand,

On Fri, Jun 18, 2021 at 5:33 PM Anand Moon <[email protected]> wrote:
[...]

Quoted 7 lines

Quoted 4 lines
For shared resets (like the one we have here) reset_control_reset will only trigger the reset line once until all drivers using that reset line are unloaded.
So effectively this new phy_ops.reset callback will be a no-op.
I know his register is shared between two USB IPs, but I have not observed any issues.

have you checked at which point we're then actually triggering the reset? I assume that you will find that the reset is only triggered for the very first power_on/init call - which makes this patch effectively a no-op (yes, we're calling reset_control_reset then, but that doesn't mean that a reset is triggered on hardware level - see drivers/reset/core.c at around line 346).

[...]

Quoted 9 lines

Quoted 6 lines
drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_probe(struct platform_device *pdev) · line 319
Quoted 2 lines
drivers/phy/amlogic/phy-meson8b-usb2.c +1 -1
-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
I think this breaks compatibility with existing .dtbs and our dt-bindings (as we're not documenting a "reset-names" property). What is the goal of this one?
OK, If we pass NULL over here there is the possibility USB phy will not get registered.

I don't understand why - with NULL everything is working fine for me. Also no matter which name you give to the reset line (in reset-names), it will be the same reset line in all cases. If it's the same reset line before and after: why is this needed?

Best regards,
Martin

Anand Moon

author

2021-06-21 07:16 UTC #22 lore raw

Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function

Hi Martin

Thanks for your review comments.

Quoted 10 lines · Martin Blumenstingl

Hi Anand,
On Fri, Jun 18, 2021 at 5:33 PM Anand Moon <[email protected]> wrote:
[...]
Quoted 5 lines
Quoted 4 lines
For shared resets (like the one we have here) reset_control_reset will only trigger the reset line once until all drivers using that reset line are unloaded.
So effectively this new phy_ops.reset callback will be a no-op.

Quoted 9 lines

Quoted 2 lines
I know his register is shared between two USB IPs, but I have not observed any issues.
have you checked at which point we're then actually triggering the reset? I assume that you will find that the reset is only triggered for the very first power_on/init call - which makes this patch effectively a no-op (yes, we're calling reset_control_reset then, but that doesn't mean that a reset is triggered on hardware level - see drivers/reset/core.c at around line 346).

Ok Thanks for the inputs. got your point.

I was also looking into Amlogic source code for reset. (aml_cbus_update_bits)
[0] https://github.com/khadas/linux/blob/khadas-vims-4.9.y/drivers/amlogic/usb/phy/phy-aml-new-usb.c
is there some feature to iomap the USB with cbus?

Quoted 8 lines

[...]
Quoted 7 lines
Quoted 6 lines
drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_probe(struct platform_device *pdev) · line 319
Quoted 2 lines
drivers/phy/amlogic/phy-meson8b-usb2.c +1 -1
-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
I think this breaks compatibility with existing .dtbs and our dt-bindings (as we're not documenting a "reset-names" property). What is the goal of this one?

Quoted 7 lines

Quoted 2 lines
OK, If we pass NULL over here there is the possibility USB phy will not get registered.
I don't understand why - with NULL everything is working fine for me. Also no matter which name you give to the reset line (in reset-names), it will be the same reset line in all cases. If it's the same reset line before and after: why is this needed?

I need to investigate this reset feature. With my setup with current changes after I update the below.
- priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");

+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
        if (PTR_ERR(priv->reset) == -EPROBE_DEFER)
                return PTR_ERR(priv->reset);

Reset will break the USB initialization, see below output.

[    1.265403] dwc2 c9040000.usb: mapped PA c9040000 to VA (ptrval)
[    1.265540] dwc2 c9040000.usb: Looking up vusb_d-supply from device tree
[    1.265554] dwc2 c9040000.usb: Looking up vusb_d-supply property in
node /soc/usb@c9040000 failed
[    1.265585] dwc2 c9040000.usb: supply vusb_d not found, using dummy regulator
[    1.265717] dwc2 c9040000.usb: Looking up vusb_a-supply from device tree
[    1.265730] dwc2 c9040000.usb: Looking up vusb_a-supply property in
node /soc/usb@c9040000 failed
[    1.265752] dwc2 c9040000.usb: supply vusb_a not found, using dummy regulator
[    1.265812] dwc2 c9040000.usb: registering common handler for irq35
[    1.265867] dwc2 c9040000.usb: Looking up vbus-supply from device tree
[    1.265880] dwc2 c9040000.usb: Looking up vbus-supply property in
node /soc/usb@c9040000 failed
[    1.267066] dwc2 c9040000.usb: Core Release: 3.10a (snpsid=4f54310a)
[    1.270983] dwc2 c9040000.usb: dwc2_core_reset: HANG! Soft Reset
timeout GRSTCTL_CSFTRST
[    1.271319] dwc2: probe of c9040000.usb failed with error -16
[    1.273296] dwc2 c90c0000.usb: mapped PA c90c0000 to VA (ptrval)
[    1.273426] dwc2 c90c0000.usb: Looking up vusb_d-supply from device tree
[    1.273440] dwc2 c90c0000.usb: Looking up vusb_d-supply property in
node /soc/usb@c90c0000 failed
[    1.273471] dwc2 c90c0000.usb: supply vusb_d not found, using dummy regulator
[    1.273607] dwc2 c90c0000.usb: Looking up vusb_a-supply from device tree
[    1.273621] dwc2 c90c0000.usb: Looking up vusb_a-supply property in
node /soc/usb@c90c0000 failed
[    1.273641] dwc2 c90c0000.usb: supply vusb_a not found, using dummy regulator
[    1.273700] dwc2 c90c0000.usb: registering common handler for irq36
[    1.273750] dwc2 c90c0000.usb: Looking up vbus-supply from device tree
[    1.273762] dwc2 c90c0000.usb: Looking up vbus-supply property in
node /soc/usb@c90c0000 failed
[    1.274966] dwc2 c90c0000.usb: Core Release: 3.10a (snpsid=4f54310a)
[    1.278888] dwc2 c90c0000.usb: dwc2_core_reset: HANG! Soft Reset
timeout GRSTCTL_CSFTRST
[    1.279224] dwc2: probe of c90c0000.usb failed with error -16

Quoted 3 lines

Best regards,
Martin

Thanks
-Anand

Martin Blumenstingl

reviewer

2021-06-22 20:12 UTC #23 lore raw

Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function

Hi Anand,

On Mon, Jun 21, 2021 at 9:16 AM Anand Moon <[email protected]> wrote:
[...]

Quoted 5 lines

Ok Thanks for the inputs. got your point.
I was also looking into Amlogic source code for reset. (aml_cbus_update_bits)
[0] https://github.com/khadas/linux/blob/khadas-vims-4.9.y/drivers/amlogic/usb/phy/phy-aml-new-usb.c
is there some feature to iomap the USB with cbus?

for that specific code: that's what we do inside drivers/reset/reset-meson.c Amlogic's vendor kernel uses an increment of 4 bytes per value, so 0x1102 translates to 0x4408

then in mainline's meson8b.dtsi we have:

    compatible = "amlogic,meson8b-reset";
    reg = <0x4404 0x9c>;

as you can see 0x4408 is part of the reset controller node.

next in include/dt-bindings/reset/amlogic,meson8b-reset.h we have:

    #define RESET_USB_OTG                 34

the register used for reset line 34 is translated using:

    0x4404 (first register) + 4 (4 * reset line / 32 = 1) = 0x4408
then the bit inside this register is translated using:
    reset line % 32 = 2

that's how we express aml_cbus_update_bits(0x1102, 0x1<<2, 0x1<<2); in the mainline kernel (by going through the reset subsystem)

[...]

Quoted 21 lines

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_probe(struct platform_device *pdev) · line 319
Quoted 2 lines
drivers/phy/amlogic/phy-meson8b-usb2.c +1 -1
-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
I think this breaks compatibility with existing .dtbs and our dt-bindings (as we're not documenting a "reset-names" property). What is the goal of this one?
Quoted 7 lines
Quoted 2 lines
OK, If we pass NULL over here there is the possibility USB phy will not get registered.
I don't understand why - with NULL everything is working fine for me. Also no matter which name you give to the reset line (in reset-names), it will be the same reset line in all cases. If it's the same reset line before and after: why is this needed?
I need to investigate this reset feature. With my setup with current changes after I update the below.
drivers/phy/amlogic/phy-meson8b-usb2.c +1 -1
-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
        if (PTR_ERR(priv->reset) == -EPROBE_DEFER)
                return PTR_ERR(priv->reset);

Quoted 1 line

Reset will break the USB initialization, see below output.

interesting, I have not seen that USB problem before and neither is Kernel CI seeing it: [0]
Is it only happening with this patch or did you also see it before?

Best regards,
Martin

[0] https://storage.staging.kernelci.org/next/master/next-20210617/arm/multi_v7_defconfig+ltp-ima/gcc-8/lab-baylibre/baseline-meson8b-odroidc1.html

Anand Moon

author

2021-06-24 14:54 UTC #24 lore raw

Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function

Hi Martin,

Quoted 22 lines · Martin Blumenstingl

Hi Anand,
On Mon, Jun 21, 2021 at 9:16 AM Anand Moon <[email protected]> wrote:
[...]
Quoted 5 lines
Ok Thanks for the inputs. got your point.
I was also looking into Amlogic source code for reset. (aml_cbus_update_bits)
[0] https://github.com/khadas/linux/blob/khadas-vims-4.9.y/drivers/amlogic/usb/phy/phy-aml-new-usb.c
is there some feature to iomap the USB with cbus?
for that specific code: that's what we do inside drivers/reset/reset-meson.c Amlogic's vendor kernel uses an increment of 4 bytes per value, so 0x1102 translates to 0x4408
then in mainline's meson8b.dtsi we have:
    compatible = "amlogic,meson8b-reset";
    reg = <0x4404 0x9c>;
as you can see 0x4408 is part of the reset controller node.
next in include/dt-bindings/reset/amlogic,meson8b-reset.h we have:
    #define RESET_USB_OTG                 34

Quoted 8 lines

the register used for reset line 34 is translated using:
    0x4404 (first register) + 4 (4 * reset line / 32 = 1) = 0x4408
then the bit inside this register is translated using:
    reset line % 32 = 2
that's how we express aml_cbus_update_bits(0x1102, 0x1<<2, 0x1<<2); in the mainline kernel (by going through the reset subsystem)

Thank you very much for clearing my long-standing doubt on *reset logic* on Amlogic SoC.

Quoted 22 lines

[...]
Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_probe(struct platform_device *pdev) · line 319
Quoted 2 lines
drivers/phy/amlogic/phy-meson8b-usb2.c +1 -1
-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
I think this breaks compatibility with existing .dtbs and our dt-bindings (as we're not documenting a "reset-names" property). What is the goal of this one?
Quoted 7 lines
Quoted 2 lines
OK, If we pass NULL over here there is the possibility USB phy will not get registered.
I don't understand why - with NULL everything is working fine for me. Also no matter which name you give to the reset line (in reset-names), it will be the same reset line in all cases. If it's the same reset line before and after: why is this needed?
I need to investigate this reset feature. With my setup with current changes after I update the below.
drivers/phy/amlogic/phy-meson8b-usb2.c +1 -1
-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
        if (PTR_ERR(priv->reset) == -EPROBE_DEFER)
                return PTR_ERR(priv->reset);

Quoted 5 lines

Quoted 1 line
Reset will break the USB initialization, see below output.
interesting, I have not seen that USB problem before and neither is Kernel CI seeing it: [0]
Is it only happening with this patch or did you also see it before?

Yes, it could happen with this patch but It could be also linked to reorder the phy configuration.
See below logs, when core reset fails on USB PHY no USB is getting registered.

[    1.267620] dwc2 c9040000.usb: mapped PA c9040000 to VA (ptrval)
[    1.267768] dwc2 c9040000.usb: Looking up vusb_d-supply from device tree
[    1.267783] dwc2 c9040000.usb: Looking up vusb_d-supply property in
node /soc/usb@c9040000 failed
[    1.267814] dwc2 c9040000.usb: supply vusb_d not found, using dummy regulator
[    1.267940] dwc2 c9040000.usb: Looking up vusb_a-supply from device tree
[    1.267954] dwc2 c9040000.usb: Looking up vusb_a-supply property in
node /soc/usb@c9040000 failed
[    1.267975] dwc2 c9040000.usb: supply vusb_a not found, using dummy regulator
[    1.268037] dwc2 c9040000.usb: registering common handler for irq35
[    1.268090] dwc2 c9040000.usb: Looking up vbus-supply from device tree
[    1.268102] dwc2 c9040000.usb: Looking up vbus-supply property in
node /soc/usb@c9040000 failed
[    1.269267] dwc2 c9040000.usb: Core Release: 3.10a (snpsid=4f54310a)
[    1.273185] dwc2 c9040000.usb: dwc2_core_reset: HANG! Soft Reset
timeout GRSTCTL_CSFTRST
[    1.273510] dwc2: probe of c9040000.usb failed with error -16
[    1.275474] dwc2 c90c0000.usb: mapped PA c90c0000 to VA (ptrval)
[    1.275603] dwc2 c90c0000.usb: Looking up vusb_d-supply from device tree
[    1.275617] dwc2 c90c0000.usb: Looking up vusb_d-supply property in
node /soc/usb@c90c0000 failed
[    1.275646] dwc2 c90c0000.usb: supply vusb_d not found, using dummy regulator
[    1.275784] dwc2 c90c0000.usb: Looking up vusb_a-supply from device tree
[    1.275798] dwc2 c90c0000.usb: Looking up vusb_a-supply property in
node /soc/usb@c90c0000 failed
[    1.275819] dwc2 c90c0000.usb: supply vusb_a not found, using dummy regulator
[    1.275877] dwc2 c90c0000.usb: registering common handler for irq36
[    1.275930] dwc2 c90c0000.usb: Looking up vbus-supply from device tree
[    1.275942] dwc2 c90c0000.usb: Looking up vbus-supply property in
node /soc/usb@c90c0000 failed
[    1.277125] dwc2 c90c0000.usb: Core Release: 3.10a (snpsid=4f54310a)
[    1.281042] dwc2 c90c0000.usb: dwc2_core_reset: HANG! Soft Reset
timeout GRSTCTL_CSFTRST
[    1.281353] dwc2: probe of c90c0000.usb failed with error -16

Quoted 6 lines

Best regards,
Martin
[0] https://storage.staging.kernelci.org/next/master/next-20210617/arm/multi_v7_defconfig+ltp-ima/gcc-8/lab-baylibre/baseline-meson8b-odroidc1.html

Anand Moon

author

2021-06-27 20:07 UTC #25 lore raw

Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function

Hi Martin,

Quoted 96 lines · Anand Moon

Hi Martin,
Quoted 22 lines · Martin Blumenstingl
Hi Anand,
On Mon, Jun 21, 2021 at 9:16 AM Anand Moon <[email protected]> wrote:
[...]
Quoted 5 lines
Ok Thanks for the inputs. got your point.
I was also looking into Amlogic source code for reset. (aml_cbus_update_bits)
[0] https://github.com/khadas/linux/blob/khadas-vims-4.9.y/drivers/amlogic/usb/phy/phy-aml-new-usb.c
is there some feature to iomap the USB with cbus?
for that specific code: that's what we do inside drivers/reset/reset-meson.c Amlogic's vendor kernel uses an increment of 4 bytes per value, so 0x1102 translates to 0x4408
then in mainline's meson8b.dtsi we have:
    compatible = "amlogic,meson8b-reset";
    reg = <0x4404 0x9c>;
as you can see 0x4408 is part of the reset controller node.
next in include/dt-bindings/reset/amlogic,meson8b-reset.h we have:
    #define RESET_USB_OTG                 34
Quoted 8 lines
the register used for reset line 34 is translated using:
    0x4404 (first register) + 4 (4 * reset line / 32 = 1) = 0x4408
then the bit inside this register is translated using:
    reset line % 32 = 2
that's how we express aml_cbus_update_bits(0x1102, 0x1<<2, 0x1<<2); in the mainline kernel (by going through the reset subsystem)
Thank you very much for clearing my long-standing doubt on *reset logic* on Amlogic SoC.
Quoted 22 lines
[...]
Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
I think this breaks compatibility with existing .dtbs and our dt-bindings (as we're not documenting a "reset-names" property). What is the goal of this one?
Quoted 7 lines
Quoted 2 lines
OK, If we pass NULL over here there is the possibility USB phy will not get registered.
I don't understand why - with NULL everything is working fine for me. Also no matter which name you give to the reset line (in reset-names), it will be the same reset line in all cases. If it's the same reset line before and after: why is this needed?
I need to investigate this reset feature. With my setup with current changes after I update the below.
(quoted diff) +1 -1
-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
        if (PTR_ERR(priv->reset) == -EPROBE_DEFER)
                return PTR_ERR(priv->reset);
Quoted 5 lines
Quoted 1 line
Reset will break the USB initialization, see below output.
interesting, I have not seen that USB problem before and neither is Kernel CI seeing it: [0]
Is it only happening with this patch or did you also see it before?
Yes, it could happen with this patch but It could be also linked to reorder the phy configuration.
See below logs, when core reset fails on USB PHY no USB is getting registered.
[    1.267620] dwc2 c9040000.usb: mapped PA c9040000 to VA (ptrval)
[    1.267768] dwc2 c9040000.usb: Looking up vusb_d-supply from device tree
[    1.267783] dwc2 c9040000.usb: Looking up vusb_d-supply property in
node /soc/usb@c9040000 failed
[    1.267814] dwc2 c9040000.usb: supply vusb_d not found, using dummy regulator
[    1.267940] dwc2 c9040000.usb: Looking up vusb_a-supply from device tree
[    1.267954] dwc2 c9040000.usb: Looking up vusb_a-supply property in
node /soc/usb@c9040000 failed
[    1.267975] dwc2 c9040000.usb: supply vusb_a not found, using dummy regulator
[    1.268037] dwc2 c9040000.usb: registering common handler for irq35
[    1.268090] dwc2 c9040000.usb: Looking up vbus-supply from device tree
[    1.268102] dwc2 c9040000.usb: Looking up vbus-supply property in
node /soc/usb@c9040000 failed
[    1.269267] dwc2 c9040000.usb: Core Release: 3.10a (snpsid=4f54310a)
[    1.273185] dwc2 c9040000.usb: dwc2_core_reset: HANG! Soft Reset
timeout GRSTCTL_CSFTRST
[    1.273510] dwc2: probe of c9040000.usb failed with error -16
[    1.275474] dwc2 c90c0000.usb: mapped PA c90c0000 to VA (ptrval)
[    1.275603] dwc2 c90c0000.usb: Looking up vusb_d-supply from device tree
[    1.275617] dwc2 c90c0000.usb: Looking up vusb_d-supply property in
node /soc/usb@c90c0000 failed
[    1.275646] dwc2 c90c0000.usb: supply vusb_d not found, using dummy regulator
[    1.275784] dwc2 c90c0000.usb: Looking up vusb_a-supply from device tree
[    1.275798] dwc2 c90c0000.usb: Looking up vusb_a-supply property in
node /soc/usb@c90c0000 failed
[    1.275819] dwc2 c90c0000.usb: supply vusb_a not found, using dummy regulator

Quoted 9 lines

[    1.275877] dwc2 c90c0000.usb: registering common handler for irq36
[    1.275930] dwc2 c90c0000.usb: Looking up vbus-supply from device tree
[    1.275942] dwc2 c90c0000.usb: Looking up vbus-supply property in
node /soc/usb@c90c0000 failed
[    1.277125] dwc2 c90c0000.usb: Core Release: 3.10a (snpsid=4f54310a)
[    1.281042] dwc2 c90c0000.usb: dwc2_core_reset: HANG! Soft Reset
timeout GRSTCTL_CSFTRST
[    1.281353] dwc2: probe of c90c0000.usb failed with error -16

Sorry for the delay.
We could switch the reset logic to *devm_reset_control_get_optional_exclusive* as below
to fix the reset line, since both the dwc2 c90c0000.usb and c9040000.usb will have their own context to reset control register, it means the reset line is not share
between two USB PHY nodes.

- priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+ priv->reset = devm_reset_control_get_optional_exclusive(&pdev->dev,
+ "reset");

Quoted 6 lines

Quoted 6 lines
Best regards,
Martin
[0] https://storage.staging.kernelci.org/next/master/next-20210617/arm/multi_v7_defconfig+ltp-ima/gcc-8/lab-baylibre/baseline-meson8b-odroidc1.html

Thanks
-Anand

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Martin Blumenstingl

reviewer

2021-06-27 20:25 UTC #26 lore raw

Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function

Hi Anand,

On Sun, Jun 27, 2021 at 10:07 PM Anand Moon <[email protected]> wrote:
[...]

Quoted 7 lines

Sorry for the delay.
We could switch the reset logic to *devm_reset_control_get_optional_exclusive* as below
to fix the reset line, since both the dwc2 c90c0000.usb and c9040000.usb will have their own context to reset control register, it means the reset line is not share
between two USB PHY nodes.

This is something I don't understand.
As discussed in our previous mails reset_control_reset in case of the USB PHY driver (which uses the RESET_USB_OTG reset line for *both* PHYs) is equivalent to the following code in the vendor kernel:
aml_cbus_update_bits(0x1102, 0x1<<2, 0x1<<2)

We have two PHYs but only one reset line. So in my own words I describe the reset line as being shared.

Review

Quoted 4 lines

(quoted diff) +2 -1

-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+       priv->reset = devm_reset_control_get_optional_exclusive(&pdev->dev,
+                                                               "reset");

Have you boot-tested this?
Without any .dts changes this will return NULL because there's no reset-names = "reset"; in the .dts(i).
If you replace "reset" with NULL then I assume that the second PHY will fail to obtain the reset line because it's shared between two devices but we're trying to obtain it exclusively for both (PHYs).

Best regards,
Martin

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-07-02 19:13 UTC #27 lore raw

Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function

Hi Martin,

Quoted 25 lines · Martin Blumenstingl

Hi Anand,
On Sun, Jun 27, 2021 at 10:07 PM Anand Moon <[email protected]> wrote:
[...]
Quoted 7 lines
Sorry for the delay.
We could switch the reset logic to *devm_reset_control_get_optional_exclusive* as below
to fix the reset line, since both the dwc2 c90c0000.usb and c9040000.usb will have their own context to reset control register, it means the reset line is not share
between two USB PHY nodes.
This is something I don't understand.
As discussed in our previous mails reset_control_reset in case of the USB PHY driver (which uses the RESET_USB_OTG reset line for *both* PHYs) is equivalent to the following code in the vendor kernel:
aml_cbus_update_bits(0x1102, 0x1<<2, 0x1<<2)
We have two PHYs but only one reset line. So in my own words I describe the reset line as being shared.
Review
Quoted 4 lines
(quoted diff) +2 -1
-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+       priv->reset = devm_reset_control_get_optional_exclusive(&pdev->dev,
+                                                               "reset");

Quoted 7 lines

Have you boot-tested this?
Without any .dts changes this will return NULL because there's no reset-names = "reset"; in the .dts(i).
If you replace "reset" with NULL then I assume that the second PHY will fail to obtain the reset line because it's shared between two devices but we're trying to obtain it exclusively for both (PHYs).

Thanks for your review comments.

I have always tested with both the phy enable and with proper DTS changes. Yes, it gives false-positive results while initialization of the USB PHY. Odroid C2 it will pass but on Odroid C1 it will fail kid off.

But it seems to me that the order of the PHY reset is kind of a problem.

Thanks for looking into my changes.

Quoted 3 lines

Best regards,
Martin

Thanks
-Anand

Anand Moon

author

2021-06-17 19:44 UTC #28 lore raw

RFC 7/8 phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode.

In reply to Anand Moon

Power off the PHY by putting it into reset mode. Drop the phy power reset since we are doing reset of phy after we configure the phy. No functional change.

Cc Martin Blumenstingl <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/phy/amlogic/phy-meson8b-usb2.c +5 -2

index c1ed2e5c80d8..d5edd31686bb 100644
@@ -226,6 +226,11 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
 		regmap_update_bits(priv->regmap, REG_DBG_UART,
 				   REG_DBG_UART_SET_IDDQ,
 				   REG_DBG_UART_SET_IDDQ);
+
+	/* power off the PHY by putting it into reset mode */
+	regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
+			   REG_CTRL_POWER_ON_RESET);
+
 	return 0;
 }

@@ -245,8 +250,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
 	regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_FSEL_MASK,
 			   0x5 << REG_CTRL_FSEL_SHIFT);
 	/* reset the PHY */
-	regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
-			   REG_CTRL_POWER_ON_RESET);
 	udelay(RESET_COMPLETE_TIME);
 	regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET, 0);
 	udelay(RESET_COMPLETE_TIME);

Signature · 1 line

2.31.1

Martin Blumenstingl

reviewer

2021-06-17 22:37 UTC #29 lore raw

Re: [RFCv1 7/8] phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode.

Hi Anand,

On Thu, Jun 17, 2021 at 9:44 PM Anand Moon <[email protected]> wrote:
[...]

drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_power_on(struct phy *phy) · line 249

Quoted 6 lines

drivers/phy/amlogic/phy-meson8b-usb2.c +0 -2

@@ -245,8 +250,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
        regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_FSEL_MASK,
                           0x5 << REG_CTRL_FSEL_SHIFT);
        /* reset the PHY */
-       regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
-                          REG_CTRL_POWER_ON_RESET);

The vendor driver uses the following sequence for the power on reset:
- set the power on reset bit
- wait 500us
- clear the power on reset bit
- wait 500us

With your change we now:
- wait 500us
- clear the power on reset bit
- wait 500us

I don't know if this is sufficient to bring the PHY into a well-defined state. Maybe it works, maybe it doesn't reset at all in this case - I don't know how to verify this though.

Best regards,
Martin

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-06-21 07:15 UTC #30 lore raw

Re: [RFCv1 7/8] phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode.

Hi Martin,

Quoted 17 lines · Martin Blumenstingl

Hi Anand,
On Thu, Jun 17, 2021 at 9:44 PM Anand Moon <[email protected]> wrote:
[...]
drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_power_on(struct phy *phy) · line 249
Quoted 6 lines
drivers/phy/amlogic/phy-meson8b-usb2.c +0 -2
@@ -245,8 +250,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
        regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_FSEL_MASK,
                           0x5 << REG_CTRL_FSEL_SHIFT);
        /* reset the PHY */
-       regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
-                          REG_CTRL_POWER_ON_RESET);
The vendor driver uses the following sequence for the power on reset:
- set the power on reset bit
- wait 500us
- clear the power on reset bit
- wait 500us

Quoted 9 lines

With your change we now:
- wait 500us
- clear the power on reset bit
- wait 500us
I don't know if this is sufficient to bring the PHY into a well-defined state. Maybe it works, maybe it doesn't reset at all in this case - I don't know how to verify this though.

Initially, I tried to some bit mask code to resolve this but it failed, So no harm in keeping the original changes.

There is another parameter REG_CTRL_PORT_RESET to be considered.

Quoted 3 lines

Best regards,
Martin

Thanks

-Anand

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Martin Blumenstingl

reviewer

2021-06-22 20:01 UTC #31 lore raw

Re: [RFCv1 7/8] phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode.

Hi Anand,

Quoted 27 lines · Anand Moon

Hi Martin,
Quoted 22 lines · Martin Blumenstingl
Hi Anand,
On Thu, Jun 17, 2021 at 9:44 PM Anand Moon <[email protected]> wrote:
[...]
drivers/phy/amlogic/phy-meson8b-usb2.c static int phy_meson8b_usb2_power_on(struct phy *phy) · line 249
Quoted 6 lines
drivers/phy/amlogic/phy-meson8b-usb2.c +0 -2
@@ -245,8 +250,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
        regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_FSEL_MASK,
                           0x5 << REG_CTRL_FSEL_SHIFT);
        /* reset the PHY */
-       regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
-                          REG_CTRL_POWER_ON_RESET);
The vendor driver uses the following sequence for the power on reset:
- set the power on reset bit
- wait 500us
- clear the power on reset bit
- wait 500us
With your change we now:
- wait 500us
- clear the power on reset bit
- wait 500us

Quoted 6 lines

Quoted 4 lines
I don't know if this is sufficient to bring the PHY into a well-defined state. Maybe it works, maybe it doesn't reset at all in this case - I don't know how to verify this though.
Initially, I tried to some bit mask code to resolve this but it failed, So no harm in keeping the original changes.

yes, I feel more comfortable with that

Quoted 1 line

There is another parameter REG_CTRL_PORT_RESET to be considered.

none of the vendor kernels that I have sets or clears this bit explicitly I agree that the name seems related, but due to lack of an example or documentation how/when to use this bit I suggest we don't touch it for now

Best rergards,
Martin

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-06-17 19:44 UTC #32 lore raw

RFC 8/8 phy: amlogic: meson8b-usb2: don't log an error on -EPROBE_DEFER

In reply to Anand Moon

devm_phy_create can return -EPROBE_DEFER if the phy-supply is not ready yet. Silence this warning as the driver framework will re-attempt registering the PHY. Use dev_err_probe() for phy resources to indicate the deferral reason when waiting for the resource to come up.

Cc Martin Blumenstingl <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/phy/amlogic/phy-meson8b-usb2.c +2 -2

index d5edd31686bb..bd624781d914 100644
@@ -332,8 +332,8 @@ static int phy_meson8b_usb2_probe(struct platform_device *pdev)
 
 	phy = devm_phy_create(&pdev->dev, NULL, &phy_meson8b_usb2_ops);
 	if (IS_ERR(phy)) {
-		dev_err(&pdev->dev, "failed to create PHY\n");
-		return PTR_ERR(phy);
+		ret = PTR_ERR(phy);
+		return dev_err_probe(&pdev->dev, ret, "failed to create PHY\n");
 	}
 
 	phy_set_drvdata(phy, priv);

Signature · 1 line

2.31.1

Martin Blumenstingl

reviewer Reviewed-by on v1 8/8

2021-06-17 22:27 UTC #33 lore raw

Re: [RFCv1 8/8] phy: amlogic: meson8b-usb2: don't log an error on -EPROBE_DEFER

Quoted 5 lines · Anand Moon

devm_phy_create can return -EPROBE_DEFER if the phy-supply is not ready yet. Silence this warning as the driver framework will re-attempt registering the PHY. Use dev_err_probe() for phy resources to indicate the deferral reason when waiting for the resource to come up.

This is a good improvement - thank you!

Quoted 2 lines

Cc Martin Blumenstingl <[email protected]>
Signed-off-by Anand Moon <[email protected]>

Reviewed-by Martin Blumenstingl <[email protected]>

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Martin Blumenstingl

reviewer

2021-06-17 22:11 UTC #34 lore raw

In reply to Anand Moon

Hi Anand,

Quoted 2 lines · Anand Moon

On Odroid C1+ and Odroid C2 USB feature is broken

with "broken" you mean the hotplug and IRQ issues you describe below? I think that these are not PHY related but rather a problem with the dwc2 IP/driver

Quoted 4 lines

this code changes are just re-structure the code so that proper callback function execute in proper order.
Code changes losely based on phy-meson-gxl-usb2.c

there's a big difference between the Meson8 and GXL drivers:
GXL requires changing some of the PHY registers at runtime based on the mode (host/peripheral).
The Meson8 PHY however (as far as I can tell from the 3.10 and 3.14 kernels) don't require any runtime changes based on the mode.

Quoted 7 lines

Initially if we connect USB HDD at initial boot it get detected. but after that usb hotplug of USB HDD is broken.
I did not observe and USB debug events messages to trace the root cause.
Another issue I observed is increase of USB interrupts event even if there is not much activity on USB ports.
I tried some clk changes but it did not workout for me.

I suggest reporting this to the dwc2 maintainers and asking for advice on how to debug these issues.

Best regards,
Martin

Anand Moon

author

2021-06-18 13:20 UTC #35 lore raw

Hi Martin,

Thanks for your review comments.
(some email id got messed up while sending these changes.)

Quoted 13 lines · Martin Blumenstingl

Hi Anand,
Quoted 2 lines · Anand Moon
On Odroid C1+ and Odroid C2 USB feature is broken
with "broken" you mean the hotplug and IRQ issues you describe below? I think that these are not PHY related but rather a problem with the dwc2 IP/driver
Quoted 3 lines
this code changes are just re-structure the code so that proper callback function execute in proper order.

Quoted 7 lines

Quoted 1 line
Code changes losely based on phy-meson-gxl-usb2.c
there's a big difference between the Meson8 and GXL drivers:
GXL requires changing some of the PHY registers at runtime based on the mode (host/peripheral).
The Meson8 PHY however (as far as I can tell from the 3.10 and 3.14 kernels) don't require any runtime changes based on the mode.

Yes I have gone through 3.10 and 3.14 kernel + u-boot source code My intent is to simplify the code changes so that they will be flexible to be used
in the future API. It does not solve the problem.

Quoted 10 lines

Quoted 7 lines
Initially if we connect USB HDD at initial boot it get detected. but after that usb hotplug of USB HDD is broken.
I did not observe and USB debug events messages to trace the root cause.
Another issue I observed is increase of USB interrupts event even if there is not much activity on USB ports.
I tried some clk changes but it did not workout for me.
I suggest reporting this to the dwc2 maintainers and asking for advice on how to debug these issues.

Yes I have enabled the CONFIG_USB_DWC2_DEBUG
to help debug this issue but could not find much clue on why this is happening.

I will keep debugging the DWC2 code or try to understand the missing features.

Quoted 3 lines

Best regards,
Martin

Thanks

-Anand

Martin Blumenstingl

reviewer

2021-06-18 20:17 UTC #36 lore raw

Hi Anand,

On Fri, Jun 18, 2021 at 3:20 PM Anand Moon <[email protected]> wrote:
[...]

Quoted 1 line

(some email id got messed up while sending these changes.)

it happened to me before: don't worry, it's something that can be fixed

[...]

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Initially if we connect USB HDD at initial boot it get detected. but after that usb hotplug of USB HDD is broken.
I did not observe and USB debug events messages to trace the root cause.
Another issue I observed is increase of USB interrupts event even if there is not much activity on USB ports.
I tried some clk changes but it did not workout for me.

Quoted 6 lines

Quoted 3 lines
I suggest reporting this to the dwc2 maintainers and asking for advice on how to debug these issues.
Yes I have enabled the CONFIG_USB_DWC2_DEBUG
to help debug this issue but could not find much clue on why this is happening.

I think CONFIG_USB_DWC2_DEBUG is a good starting point.
For myself I came to the conclusion that the dwc2 IP is too complex to understand without additional information
Some additional information can be found in public datasheets of other SoCs which are also using a dwc2 core, see for example RK3128 [0] or RT3050 [1]
That's why my suggestion is to additionally ask the dwc2 maintainers (which are not Cc'ed on this mail) for debugging suggestions.

Best regards,
Martin

[0] https://rockchip.fr/RK312X%20TRM/chapter-26-usb-otg-2-0.pdf
[1] http://static6.arrow.com/aropdfconversion/aa9a14376a75e7c5d6daa9f6aaed8411909d2021/rt3050_5x_v2.0_081408_0902.pdf

Signature · 3 lines

linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-06-21 07:21 UTC #37 lore raw

Hi Martin,

Quoted 22 lines · Martin Blumenstingl

Hi Anand,
On Fri, Jun 18, 2021 at 3:20 PM Anand Moon <[email protected]> wrote:
[...]
Quoted 1 line
(some email id got messed up while sending these changes.)
it happened to me before: don't worry, it's something that can be fixed
[...]
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Initially if we connect USB HDD at initial boot it get detected. but after that usb hotplug of USB HDD is broken.
I did not observe and USB debug events messages to trace the root cause.
Another issue I observed is increase of USB interrupts event even if there is not much activity on USB ports.
I tried some clk changes but it did not workout for me.
Quoted 6 lines
Quoted 3 lines
I suggest reporting this to the dwc2 maintainers and asking for advice on how to debug these issues.
Yes I have enabled the CONFIG_USB_DWC2_DEBUG
to help debug this issue but could not find much clue on why this is happening.

Quoted 9 lines

I think CONFIG_USB_DWC2_DEBUG is a good starting point.
For myself I came to the conclusion that the dwc2 IP is too complex to understand without additional information
Some additional information can be found in public datasheets of other SoCs which are also using a dwc2 core, see for example RK3128 [0] or RT3050 [1]
That's why my suggestion is to additionally ask the dwc2 maintainers (which are not Cc'ed on this mail) for debugging suggestions.

Yes, I will investigate with mode debugging and Cc USB maintainers in the next version for more details.

Quoted 4 lines

Best regards,
Martin

Thanks for these inputs.

Quoted 3 lines

[0] https://rockchip.fr/RK312X%20TRM/chapter-26-usb-otg-2-0.pdf
[1] http://static6.arrow.com/aropdfconversion/aa9a14376a75e7c5d6daa9f6aaed8411909d2021/rt3050_5x_v2.0_081408_0902.pdf

Thanks

-Anand