musb: sunxi: Add support for run-time changing dr-mode through sysfs

Together with some musb sunxi glue changes this allows run-time dr_mode switching support via the "mode" musb sysfs attribute.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

drivers/phy/phy-sun4i-usb.c +23 -0

index fb2d4f3..d369081 100644
@@ -427,6 +427,29 @@ static int sun4i_usb_phy_power_off(struct phy *_phy)
 	return 0;
 }
 
+static int sun4i_usb_phy_set_mode(struct phy *_phy, enum phy_mode mode)
+{
+	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+	if (phy->index != 0)
+		return -EINVAL;
+
+	switch (mode) {
+	case PHY_MODE_USB_HOST:   data->dr_mode = USB_DR_MODE_HOST; break;
+	case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break;
+	case PHY_MODE_USB_OTG:    data->dr_mode = USB_DR_MODE_OTG; break;
+	default:
+		return -EINVAL;
+	}
+
+	dev_info(&_phy->dev, "Changing dr_mode to %d\n", (int)data->dr_mode);
+	data->force_session_end = true;
+	queue_delayed_work(system_wq, &data->detect, 0);
+
+	return 0;
+}
+
 void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled)
 {
 	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

Hello.

Quoted 42 lines · Hans de Goede

Together with some musb sunxi glue changes this allows run-time dr_mode switching support via the "mode" musb sysfs attribute.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

drivers/phy/phy-sun4i-usb.c +23 -0

index fb2d4f3..d369081 100644
@@ -427,6 +427,29 @@ static int sun4i_usb_phy_power_off(struct phy *_phy)
     return 0;
 }
 
+static int sun4i_usb_phy_set_mode(struct phy *_phy, enum phy_mode mode)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    if (phy->index != 0)
+        return -EINVAL;
+
+    switch (mode) {
+    case PHY_MODE_USB_HOST:   data->dr_mode = USB_DR_MODE_HOST; break;
+    case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break;
+    case PHY_MODE_USB_OTG:    data->dr_mode = USB_DR_MODE_OTG; break;
+    default:
+        return -EINVAL;
+    }
+
+    dev_info(&_phy->dev, "Changing dr_mode to %d\n", (int)data->dr_mode);
+    data->force_session_end = true;
+    queue_delayed_work(system_wq, &data->detect, 0);
+
+    return 0;
+}
+
 void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled)
 {
     struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

[...]

$ scripts/checkpatch.pl ~/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch
ERROR: trailing statements should be on next line
#29: FILE: drivers/phy/phy-sun4i-usb.c:439:
+    case PHY_MODE_USB_HOST:   data->dr_mode = USB_DR_MODE_HOST; break;

ERROR: trailing statements should be on next line
#30: FILE: drivers/phy/phy-sun4i-usb.c:440:
+ case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break;

ERROR: trailing statements should be on next line
#31: FILE: drivers/phy/phy-sun4i-usb.c:441:
+ case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break;

Hi,

Quoted 57 lines · Sergei Shtylyov

Hello.

Quoted 42 lines · Hans de Goede

Together with some musb sunxi glue changes this allows run-time dr_mode switching support via the "mode" musb sysfs attribute.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

drivers/phy/phy-sun4i-usb.c +23 -0

index fb2d4f3..d369081 100644
@@ -427,6 +427,29 @@ static int sun4i_usb_phy_power_off(struct phy *_phy)
     return 0;
 }
 
+static int sun4i_usb_phy_set_mode(struct phy *_phy, enum phy_mode mode)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    if (phy->index != 0)
+        return -EINVAL;
+
+    switch (mode) {
+    case PHY_MODE_USB_HOST:   data->dr_mode = USB_DR_MODE_HOST; break;
+    case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break;
+    case PHY_MODE_USB_OTG:    data->dr_mode = USB_DR_MODE_OTG; break;
+    default:
+        return -EINVAL;
+    }
+
+    dev_info(&_phy->dev, "Changing dr_mode to %d\n", (int)data->dr_mode);
+    data->force_session_end = true;
+    queue_delayed_work(system_wq, &data->detect, 0);
+
+    return 0;
+}
+
 void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled)
 {
     struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

[...]

$ scripts/checkpatch.pl ~/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch
ERROR: trailing statements should be on next line
#29: FILE: drivers/phy/phy-sun4i-usb.c:439:
+    case PHY_MODE_USB_HOST:   data->dr_mode = USB_DR_MODE_HOST; break;

ERROR: trailing statements should be on next line
#30: FILE: drivers/phy/phy-sun4i-usb.c:440:
+ case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break;

Quoted 3 lines

ERROR: trailing statements should be on next line
#31: FILE: drivers/phy/phy-sun4i-usb.c:441:
+ case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break;

This is normal codeing style for a switch-case assigning a single value per case, but checkpatch does not know this.

Hi,

Quoted 60 lines · Hans de Goede

Hi,

Quoted 57 lines · Sergei Shtylyov

Hello.

Quoted 42 lines · Hans de Goede

Together with some musb sunxi glue changes this allows run-time dr_mode switching support via the "mode" musb sysfs attribute.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

drivers/phy/phy-sun4i-usb.c +23 -0

index fb2d4f3..d369081 100644
@@ -427,6 +427,29 @@ static int sun4i_usb_phy_power_off(struct phy *_phy)
     return 0;
 }
 
+static int sun4i_usb_phy_set_mode(struct phy *_phy, enum phy_mode mode)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    if (phy->index != 0)
+        return -EINVAL;
+
+    switch (mode) {
+    case PHY_MODE_USB_HOST:   data->dr_mode = USB_DR_MODE_HOST; break;
+    case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break;
+    case PHY_MODE_USB_OTG:    data->dr_mode = USB_DR_MODE_OTG; break;
+    default:
+        return -EINVAL;
+    }
+
+    dev_info(&_phy->dev, "Changing dr_mode to %d\n", (int)data->dr_mode);
+    data->force_session_end = true;
+    queue_delayed_work(system_wq, &data->detect, 0);
+
+    return 0;
+}
+
 void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled)
 {
     struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

[...]

$ scripts/checkpatch.pl ~/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch
ERROR: trailing statements should be on next line
#29: FILE: drivers/phy/phy-sun4i-usb.c:439:
+    case PHY_MODE_USB_HOST:   data->dr_mode = USB_DR_MODE_HOST; break;

ERROR: trailing statements should be on next line
#30: FILE: drivers/phy/phy-sun4i-usb.c:440:
+ case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break;

Quoted 6 lines

Quoted 3 lines

ERROR: trailing statements should be on next line
#31: FILE: drivers/phy/phy-sun4i-usb.c:441:
+ case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break;

This is normal codeing style for a switch-case assigning a single value per case, but checkpatch does not know this.

I don't see that in CodingStyle

and it's quite ugly.

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 3 lines

void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled)

 {
     struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

[...]

$ scripts/checkpatch.pl ~/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch
ERROR: trailing statements should be on next line
#29: FILE: drivers/phy/phy-sun4i-usb.c:439:
+    case PHY_MODE_USB_HOST:   data->dr_mode = USB_DR_MODE_HOST; break;

ERROR: trailing statements should be on next line
#30: FILE: drivers/phy/phy-sun4i-usb.c:440:
+ case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break;

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

ERROR: trailing statements should be on next line
#31: FILE: drivers/phy/phy-sun4i-usb.c:441:
+ case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break;

This is normal codeing style for a switch-case assigning a single value per case, but checkpatch does not know this.

I don't see that in CodingStyle

It is an exception to the rule as such it is not listed, but this really is quite a normal thing to do in C code.

Quoted 1 line

and it's quite ugly.

So this is ugly:

     switch (mode) {
     case PHY_MODE_USB_HOST:   data->dr_mode = USB_DR_MODE_HOST; break;
     case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break;
     case PHY_MODE_USB_OTG:    data->dr_mode = USB_DR_MODE_OTG; break;
     default:
         return -EINVAL;
     }

Where as this is not:

     switch (mode) {
     case PHY_MODE_USB_HOST:
         data->dr_mode = USB_DR_MODE_HOST;
         break;
     case PHY_MODE_USB_DEVICE:
         data->dr_mode = USB_DR_MODE_PERIPHERAL;
         break;
     case PHY_MODE_USB_OTG:
         data->dr_mode = USB_DR_MODE_OTG;
         break;
     default:
         return -EINVAL;
     }

???

IMHO the original version is much easier to read / makes it much clearer what the code is doing.

But if you insist I can do a v3 changing the coding style to the (IMHO) uglier version.

Also note that the real ugliness is that we've 3 different enums for host / device / dual-role. For some reason the musb code has 2 all of its own and then there is "enum phy_mode".

Anyways let me know if you want a v3 with check-patch warnings fixed.

Hi,

Hans de Goede <[email protected]> writes:

[...]

Quoted 62 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 3 lines

void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled)

 {
     struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

[...]

$ scripts/checkpatch.pl ~/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch
ERROR: trailing statements should be on next line
#29: FILE: drivers/phy/phy-sun4i-usb.c:439:
+    case PHY_MODE_USB_HOST:   data->dr_mode = USB_DR_MODE_HOST; break;

ERROR: trailing statements should be on next line
#30: FILE: drivers/phy/phy-sun4i-usb.c:440:
+ case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break;

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

ERROR: trailing statements should be on next line
#31: FILE: drivers/phy/phy-sun4i-usb.c:441:
+ case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break;

This is normal codeing style for a switch-case assigning a single value per case, but checkpatch does not know this.

I don't see that in CodingStyle

It is an exception to the rule as such it is not listed, but this really is quite a normal thing to do in C code.

Quoted 1 line

and it's quite ugly.

So this is ugly:

     switch (mode) {
     case PHY_MODE_USB_HOST:   data->dr_mode = USB_DR_MODE_HOST; break;
     case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break;
     case PHY_MODE_USB_OTG:    data->dr_mode = USB_DR_MODE_OTG; break;
     default:
         return -EINVAL;
     }

Where as this is not:

     switch (mode) {
     case PHY_MODE_USB_HOST:
         data->dr_mode = USB_DR_MODE_HOST;
         break;
     case PHY_MODE_USB_DEVICE:
         data->dr_mode = USB_DR_MODE_PERIPHERAL;
         break;
     case PHY_MODE_USB_OTG:
         data->dr_mode = USB_DR_MODE_OTG;
         break;
     default:
         return -EINVAL;
     }

???

IMHO the original version is much easier to read / makes it much clearer what the code is doing.

But if you insist I can do a v3 changing the coding style to the (IMHO) uglier version.

Quoted 6 lines

Also note that the real ugliness is that we've 3 different enums for host / device / dual-role. For some reason the musb code has 2 all of its own and then there is "enum phy_mode".

Anyways let me know if you want a v3 with check-patch warnings fixed.

I see it's somewhat common even in drivers/usb:

$ git grep -ce "case \w+:.*break;" -- drivers/usb/ 
drivers/usb/gadget/udc/net2272.c:4
drivers/usb/host/ehci-hcd.c:3
drivers/usb/host/isp116x.h:2
drivers/usb/host/ohci-dbg.c:14
drivers/usb/host/sl811-hcd.c:7
drivers/usb/host/uhci-debug.c:8
drivers/usb/image/microtek.c:64
drivers/usb/mon/mon_text.c:6
drivers/usb/musb/musb_gadget.c:2
drivers/usb/serial/digi_acceleport.c:23
drivers/usb/serial/ftdi_sio.c:10
drivers/usb/serial/mct_u232.c:10
drivers/usb/serial/spcp8x5.c:17
drivers/usb/serial/whiteheat.c:4
drivers/usb/storage/debug.c:86

so I'm okay either way. Kishon has the final say here since he's drivers/phy/ maintainer.

On some devices the musb otg controller can be used in both device and host mode, but requires software mode switching since there is no id pin connected. The usb0 phy code will default to device mode in this case.

On some systems usb0 is connected to a female USB-A port. It can still be used in device mode when using software mode switching and a USB A to A cable. To configure the controller to support both modes we must set its "dr_mode" dt property to "otg" (*). For these setups the device mode default is wrong, for a female USB-A port the default should be host mode.

This commit adds support to the sun4i phy code for a new "allwinner,usb0-usb-a-connector" dt property which can be used in dt files to indicate this scenario and when present it changes the default mode to host mode.

*) dr_mode = "host" is used when device mode is _never_ used. E.g. a wifi module soldered onto the PCB is connected to the musb controller.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:

(quoted diff) +0 -2

-New patch in v2 of this patchset
---
 Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt | 2 ++
 drivers/phy/phy-sun4i-usb.c                             | 9 ++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt +2 -0

index 287150d..8646b53 100644
@@ -35,6 +35,8 @@ Optional properties:
 - usb0_vbus-supply : regulator phandle for controller usb0 vbus
 - usb1_vbus-supply : regulator phandle for controller usb1 vbus
 - usb2_vbus-supply : regulator phandle for controller usb2 vbus
+- allwinner,usb0-usb-a-connector: usb0 is connected to an USB-A connector,
+                     rather then an USB-B connector as one would expect (bool)
 
 Example:
 	usbphy: phy at 0x01c13400 {

drivers/phy/phy-sun4i-usb.c +8 -1

index c17b099..82fb46a 100644
@@ -137,6 +137,7 @@ struct sun4i_usb_phy_data {
 	int vbus_det_irq;
 	int id_det;
 	int vbus_det;
+	int id_det_default;
 	struct delayed_work detect;
 };
 

@@ -328,7 +329,7 @@ static int sun4i_usb_phy0_get_id_det(struct sun4i_usb_phy_data *data)
 		if (data->id_det_gpio)
 			return gpiod_get_value_cansleep(data->id_det_gpio);
 		else
-			return 1; /* Fallback to peripheral mode */
+			return data->id_det_default;
 	case USB_DR_MODE_HOST:
 		return 0;
 	case USB_DR_MODE_PERIPHERAL:

@@ -621,6 +622,12 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
 	if (IS_ERR(data->base))
 		return PTR_ERR(data->base);
 
+	/* Set id-det default for when there is no id-det gpio */
+	if (of_property_read_bool(np, "allwinner,usb0-usb-a-connector"))
+		data->id_det_default = 0; /* Host (USB-A connector) */
+	else
+		data->id_det_default = 1; /* Device (USB-B connector) */
+
 	data->id_det_gpio = devm_gpiod_get_optional(dev, "usb0_id_det",
 						    GPIOD_IN);
 	if (IS_ERR(data->id_det_gpio))

Signature · 1 line

2.7.4

This allows run-time dr_mode switching support via the "mode" musb sysfs attribute.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 4 deletions(-)

drivers/usb/musb/sunxi.c +5 -0

index c6ee166..1fe7451 100644
@@ -74,6 +74,7 @@
 #define SUNXI_MUSB_FL_HAS_SRAM			5
 #define SUNXI_MUSB_FL_HAS_RESET			6
 #define SUNXI_MUSB_FL_NO_CONFIGDATA		7
+#define SUNXI_MUSB_FL_PHY_MODE_PEND		8
 
 /* Our read/write methods need access and do not get passed in a musb ref :| */
 static struct musb *sunxi_musb;

@@ -87,6 +88,7 @@ struct sunxi_glue {
 	struct phy		*phy;
 	struct platform_device	*usb_phy;
 	struct usb_phy		*xceiv;
+	enum phy_mode		phy_mode;
 	unsigned long		flags;
 	struct work_struct	work;
 	struct extcon_dev	*extcon;

@@ -140,6 +142,9 @@ static void sunxi_musb_work(struct work_struct *work)
 			clear_bit(SUNXI_MUSB_FL_PHY_ON, &glue->flags);
 		}
 	}
+
+	if (test_and_clear_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags))
+		phy_set_mode(glue->phy, glue->phy_mode);
 }
 
 static void sunxi_musb_set_vbus(struct musb *musb, int is_on)

@@ -341,6 +346,41 @@ static void sunxi_musb_dma_controller_destroy(struct dma_controller *c)
 {
 }

drivers/usb/musb/sunxi.c +12 -0

+	default:
+		dev_err(musb->controller->parent,
+			"Error requested mode not supported by this kernel\n");
+		return -EINVAL;
+	}
+
+	if (glue->phy_mode == new_mode)
+		return 0;
+
+	if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) {
+		dev_err(musb->controller->parent,
+			"Error changing modes is only supported in dual role mode\n");

drivers/usb/musb/sunxi.c +12 -4

+
+	return 0;
+}
+
 /*
  * sunxi musb register layout
  * 0x00 - 0x17	fifo regs, 1 long per fifo

@@ -568,6 +608,7 @@ static const struct musb_platform_ops sunxi_musb_ops = {
 	.writew		= sunxi_musb_writew,
 	.dma_init	= sunxi_musb_dma_controller_create,
 	.dma_exit	= sunxi_musb_dma_controller_destroy,
+	.set_mode	= sunxi_musb_set_mode,
 	.set_vbus	= sunxi_musb_set_vbus,
 	.pre_root_reset_end = sunxi_musb_pre_root_reset_end,
 	.post_root_reset_end = sunxi_musb_post_root_reset_end,

@@ -614,21 +655,28 @@ static int sunxi_musb_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	glue = devm_kzalloc(&pdev->dev, sizeof(*glue), GFP_KERNEL);
+	if (!glue)
+		return -ENOMEM;
+
 	memset(&pdata, 0, sizeof(pdata));
 	switch (usb_get_dr_mode(&pdev->dev)) {
 #if defined CONFIG_USB_MUSB_DUAL_ROLE || defined CONFIG_USB_MUSB_HOST
 	case USB_DR_MODE_HOST:
 		pdata.mode = MUSB_PORT_MODE_HOST;
+		glue->phy_mode = PHY_MODE_USB_HOST;
 		break;
 #endif
 #if defined CONFIG_USB_MUSB_DUAL_ROLE || defined CONFIG_USB_MUSB_GADGET
 	case USB_DR_MODE_PERIPHERAL:
 		pdata.mode = MUSB_PORT_MODE_GADGET;
+		glue->phy_mode = PHY_MODE_USB_DEVICE;
 		break;
 #endif
 #ifdef CONFIG_USB_MUSB_DUAL_ROLE
 	case USB_DR_MODE_OTG:
 		pdata.mode = MUSB_PORT_MODE_DUAL_ROLE;
+		glue->phy_mode = PHY_MODE_USB_OTG;
 		break;
 #endif
 	default:

@@ -638,10 +686,6 @@ static int sunxi_musb_probe(struct platform_device *pdev)
 	pdata.platform_ops	= &sunxi_musb_ops;
 	pdata.config		= &sunxi_musb_hdrc_config;
 
-	glue = devm_kzalloc(&pdev->dev, sizeof(*glue), GFP_KERNEL);
-	if (!glue)
-		return -ENOMEM;
-
 	glue->dev = &pdev->dev;
 	INIT_WORK(&glue->work, sunxi_musb_work);
 	glue->host_nb.notifier_call = sunxi_musb_host_notifier;

Signature · 1 line

2.7.4

Hi,

Quoted 42 lines · Hans de Goede

This allows run-time dr_mode switching support via the "mode" musb sysfs attribute.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 4 deletions(-)

drivers/usb/musb/sunxi.c +5 -0

index c6ee166..1fe7451 100644
@@ -74,6 +74,7 @@
 #define SUNXI_MUSB_FL_HAS_SRAM			5
 #define SUNXI_MUSB_FL_HAS_RESET			6
 #define SUNXI_MUSB_FL_NO_CONFIGDATA		7
+#define SUNXI_MUSB_FL_PHY_MODE_PEND		8
 
 /* Our read/write methods need access and do not get passed in a musb ref :| */
 static struct musb *sunxi_musb;

@@ -87,6 +88,7 @@ struct sunxi_glue {
 	struct phy		*phy;
 	struct platform_device	*usb_phy;
 	struct usb_phy		*xceiv;
+	enum phy_mode		phy_mode;
 	unsigned long		flags;
 	struct work_struct	work;
 	struct extcon_dev	*extcon;

@@ -140,6 +142,9 @@ static void sunxi_musb_work(struct work_struct *work)
 			clear_bit(SUNXI_MUSB_FL_PHY_ON, &glue->flags);
 		}
 	}
+
+	if (test_and_clear_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags))
+		phy_set_mode(glue->phy, glue->phy_mode);
 }
 
 static void sunxi_musb_set_vbus(struct musb *musb, int is_on)

@@ -341,6 +346,41 @@ static void sunxi_musb_dma_controller_destroy(struct dma_controller *c)
 {
 }
 

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 357

Quoted 9 lines

drivers/usb/musb/sunxi.c +9 -0

+static int sunxi_musb_set_mode(struct musb *musb, u8 mode)
+{
+	struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
+	enum phy_mode new_mode;
+
+	switch (mode) {
+	case MUSB_HOST:		new_mode = PHY_MODE_USB_HOST; break;
+	case MUSB_PERIPHERAL:	new_mode = PHY_MODE_USB_DEVICE; break;
+	case MUSB_OTG:		new_mode = PHY_MODE_USB_OTG; break;

Please fix the code style as commented in patch 4/7.

Quoted 18 lines

drivers/usb/musb/sunxi.c +17 -0

+	default:
+		dev_err(musb->controller->parent,
+			"Error requested mode not supported by this kernel\n");
+		return -EINVAL;
+	}
+
+	if (glue->phy_mode == new_mode)
+		return 0;
+
+	if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) {
+		dev_err(musb->controller->parent,
+			"Error changing modes is only supported in dual role mode\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * phy_set_mode may sleep, and we're called with a spinlock held,

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 379

Quoted 5 lines

drivers/usb/musb/sunxi.c +5 -0

+	 * so let sunxi_musb_work deal with it.
+	 */
+	glue->phy_mode = new_mode;
+	set_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags);
+	schedule_work(&glue->work);

When switching from host to peripheral mode, if an usb device is still plugged and enumerated, how do you handle the device disconnect?

Regards,
-Bin.

Quoted 58 lines

drivers/usb/musb/sunxi.c +12 -5

+
+	return 0;
+}
+
 /*
  * sunxi musb register layout
  * 0x00 - 0x17	fifo regs, 1 long per fifo

@@ -568,6 +608,7 @@ static const struct musb_platform_ops sunxi_musb_ops = {
 	.writew		= sunxi_musb_writew,
 	.dma_init	= sunxi_musb_dma_controller_create,
 	.dma_exit	= sunxi_musb_dma_controller_destroy,
+	.set_mode	= sunxi_musb_set_mode,
 	.set_vbus	= sunxi_musb_set_vbus,
 	.pre_root_reset_end = sunxi_musb_pre_root_reset_end,
 	.post_root_reset_end = sunxi_musb_post_root_reset_end,

@@ -614,21 +655,28 @@ static int sunxi_musb_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	glue = devm_kzalloc(&pdev->dev, sizeof(*glue), GFP_KERNEL);
+	if (!glue)
+		return -ENOMEM;
+
 	memset(&pdata, 0, sizeof(pdata));
 	switch (usb_get_dr_mode(&pdev->dev)) {
 #if defined CONFIG_USB_MUSB_DUAL_ROLE || defined CONFIG_USB_MUSB_HOST
 	case USB_DR_MODE_HOST:
 		pdata.mode = MUSB_PORT_MODE_HOST;
+		glue->phy_mode = PHY_MODE_USB_HOST;
 		break;
 #endif
 #if defined CONFIG_USB_MUSB_DUAL_ROLE || defined CONFIG_USB_MUSB_GADGET
 	case USB_DR_MODE_PERIPHERAL:
 		pdata.mode = MUSB_PORT_MODE_GADGET;
+		glue->phy_mode = PHY_MODE_USB_DEVICE;
 		break;
 #endif
 #ifdef CONFIG_USB_MUSB_DUAL_ROLE
 	case USB_DR_MODE_OTG:
 		pdata.mode = MUSB_PORT_MODE_DUAL_ROLE;
+		glue->phy_mode = PHY_MODE_USB_OTG;
 		break;
 #endif
 	default:

@@ -638,10 +686,6 @@ static int sunxi_musb_probe(struct platform_device *pdev)
 	pdata.platform_ops	= &sunxi_musb_ops;
 	pdata.config		= &sunxi_musb_hdrc_config;
 
-	glue = devm_kzalloc(&pdev->dev, sizeof(*glue), GFP_KERNEL);
-	if (!glue)
-		return -ENOMEM;
-
 	glue->dev = &pdev->dev;
 	INIT_WORK(&glue->work, sunxi_musb_work);
 	glue->host_nb.notifier_call = sunxi_musb_host_notifier;
--

2.7.4

Hi,

Quoted 45 lines · Bin Liu

Hi,

Quoted 42 lines · Hans de Goede

This allows run-time dr_mode switching support via the "mode" musb sysfs attribute.

Signed-off-by Hans de Goede <[email protected]>

---
drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----

1 file changed, 48 insertions(+), 4 deletions(-)

drivers/usb/musb/sunxi.c +5 -0

index c6ee166..1fe7451 100644
@@ -74,6 +74,7 @@
 #define SUNXI_MUSB_FL_HAS_SRAM			5
 #define SUNXI_MUSB_FL_HAS_RESET			6
 #define SUNXI_MUSB_FL_NO_CONFIGDATA		7
+#define SUNXI_MUSB_FL_PHY_MODE_PEND		8
 
 /* Our read/write methods need access and do not get passed in a musb ref :| */
 static struct musb *sunxi_musb;

@@ -87,6 +88,7 @@ struct sunxi_glue {
 	struct phy		*phy;
 	struct platform_device	*usb_phy;
 	struct usb_phy		*xceiv;
+	enum phy_mode		phy_mode;
 	unsigned long		flags;
 	struct work_struct	work;
 	struct extcon_dev	*extcon;

@@ -140,6 +142,9 @@ static void sunxi_musb_work(struct work_struct *work)
 			clear_bit(SUNXI_MUSB_FL_PHY_ON, &glue->flags);
 		}
 	}
+
+	if (test_and_clear_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags))
+		phy_set_mode(glue->phy, glue->phy_mode);
 }
 
 static void sunxi_musb_set_vbus(struct musb *musb, int is_on)

@@ -341,6 +346,41 @@ static void sunxi_musb_dma_controller_destroy(struct dma_controller *c)
 {
 }
 

Quoted 11 lines

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 357

Quoted 9 lines

drivers/usb/musb/sunxi.c +9 -0

+static int sunxi_musb_set_mode(struct musb *musb, u8 mode)
+{
+	struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
+	enum phy_mode new_mode;
+
+	switch (mode) {
+	case MUSB_HOST:		new_mode = PHY_MODE_USB_HOST; break;
+	case MUSB_PERIPHERAL:	new_mode = PHY_MODE_USB_DEVICE; break;
+	case MUSB_OTG:		new_mode = PHY_MODE_USB_OTG; break;

Please fix the code style as commented in patch 4/7.

Ok I will send a new version with this fixed.

Quoted 18 lines

Quoted 18 lines

drivers/usb/musb/sunxi.c +17 -0

+	default:
+		dev_err(musb->controller->parent,
+			"Error requested mode not supported by this kernel\n");
+		return -EINVAL;
+	}
+
+	if (glue->phy_mode == new_mode)
+		return 0;
+
+	if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) {
+		dev_err(musb->controller->parent,
+			"Error changing modes is only supported in dual role mode\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * phy_set_mode may sleep, and we're called with a spinlock held,

Quoted 8 lines

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 379

Quoted 5 lines

drivers/usb/musb/sunxi.c +5 -0

+	 * so let sunxi_musb_work deal with it.
+	 */
+	glue->phy_mode = new_mode;
+	set_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags);
+	schedule_work(&glue->work);

When switching from host to peripheral mode, if an usb device is still plugged and enumerated, how do you handle the device disconnect?

The phy code will report vbus low for long enough for the musb to end the current session. It already does this for boards which do not have working vbus detection.

Regards,

Hans

Quoted 52 lines

Regards,
-Bin.

Quoted 48 lines

drivers/usb/musb/sunxi.c +12 -0

+
+	return 0;
+}
+
 /*
 * sunxi musb register layout
 * 0x00 - 0x17	fifo regs, 1 long per fifo

@@ -568,6 +608,7 @@ static const struct musb_platform_ops sunxi_musb_ops = {
 	.writew		= sunxi_musb_writew,
 	.dma_init	= sunxi_musb_dma_controller_create,
 	.dma_exit	= sunxi_musb_dma_controller_destroy,
+	.set_mode	= sunxi_musb_set_mode,
 	.set_vbus	= sunxi_musb_set_vbus,
 	.pre_root_reset_end = sunxi_musb_pre_root_reset_end,
 	.post_root_reset_end = sunxi_musb_post_root_reset_end,

@@ -614,21 +655,28 @@ static int sunxi_musb_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	glue = devm_kzalloc(&pdev->dev, sizeof(*glue), GFP_KERNEL);
+	if (!glue)
+		return -ENOMEM;
+
 	memset(&pdata, 0, sizeof(pdata));
 	switch (usb_get_dr_mode(&pdev->dev)) {
 #if defined CONFIG_USB_MUSB_DUAL_ROLE || defined CONFIG_USB_MUSB_HOST
 	case USB_DR_MODE_HOST:
 		pdata.mode = MUSB_PORT_MODE_HOST;
+		glue->phy_mode = PHY_MODE_USB_HOST;
 		break;
 #endif
 #if defined CONFIG_USB_MUSB_DUAL_ROLE || defined CONFIG_USB_MUSB_GADGET
 	case USB_DR_MODE_PERIPHERAL:
 		pdata.mode = MUSB_PORT_MODE_GADGET;
+		glue->phy_mode = PHY_MODE_USB_DEVICE;
 		break;
 #endif
 #ifdef CONFIG_USB_MUSB_DUAL_ROLE
 	case USB_DR_MODE_OTG:
 		pdata.mode = MUSB_PORT_MODE_DUAL_ROLE;
+		glue->phy_mode = PHY_MODE_USB_OTG;
 		break;
 #endif
 	default:

@@ -638,10 +686,6 @@ static int sunxi_musb_probe(struct platform_device *pdev)
 	pdata.platform_ops	= &sunxi_musb_ops;
 	pdata.config		= &sunxi_musb_hdrc_config;
 

Quoted 10 lines

Quoted 10 lines

drivers/usb/musb/sunxi.c +0 -5

-	glue = devm_kzalloc(&pdev->dev, sizeof(*glue), GFP_KERNEL);
-	if (!glue)
-		return -ENOMEM;
-
 	glue->dev = &pdev->dev;
 	INIT_WORK(&glue->work, sunxi_musb_work);
 	glue->host_nb.notifier_call = sunxi_musb_host_notifier;
--

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

Quoted 89 lines · Hans de Goede

Hi,

Quoted 45 lines · Bin Liu

Hi,

Quoted 42 lines · Hans de Goede

This allows run-time dr_mode switching support via the "mode" musb sysfs attribute.

Signed-off-by Hans de Goede <[email protected]>

---
drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----

1 file changed, 48 insertions(+), 4 deletions(-)

drivers/usb/musb/sunxi.c +5 -0

index c6ee166..1fe7451 100644
@@ -74,6 +74,7 @@
 #define SUNXI_MUSB_FL_HAS_SRAM			5
 #define SUNXI_MUSB_FL_HAS_RESET			6
 #define SUNXI_MUSB_FL_NO_CONFIGDATA		7
+#define SUNXI_MUSB_FL_PHY_MODE_PEND		8
 
 /* Our read/write methods need access and do not get passed in a musb ref :| */
 static struct musb *sunxi_musb;

@@ -87,6 +88,7 @@ struct sunxi_glue {
 	struct phy		*phy;
 	struct platform_device	*usb_phy;
 	struct usb_phy		*xceiv;
+	enum phy_mode		phy_mode;
 	unsigned long		flags;
 	struct work_struct	work;
 	struct extcon_dev	*extcon;

@@ -140,6 +142,9 @@ static void sunxi_musb_work(struct work_struct *work)
 			clear_bit(SUNXI_MUSB_FL_PHY_ON, &glue->flags);
 		}
 	}
+
+	if (test_and_clear_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags))
+		phy_set_mode(glue->phy, glue->phy_mode);
 }
 
 static void sunxi_musb_set_vbus(struct musb *musb, int is_on)

@@ -341,6 +346,41 @@ static void sunxi_musb_dma_controller_destroy(struct dma_controller *c)
 {
 }
 

Quoted 11 lines

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 357

Quoted 9 lines

drivers/usb/musb/sunxi.c +9 -0

+static int sunxi_musb_set_mode(struct musb *musb, u8 mode)
+{
+	struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
+	enum phy_mode new_mode;
+
+	switch (mode) {
+	case MUSB_HOST:		new_mode = PHY_MODE_USB_HOST; break;
+	case MUSB_PERIPHERAL:	new_mode = PHY_MODE_USB_DEVICE; break;
+	case MUSB_OTG:		new_mode = PHY_MODE_USB_OTG; break;

Please fix the code style as commented in patch 4/7.

Ok I will send a new version with this fixed.

Quoted 26 lines

Quoted 13 lines

drivers/usb/musb/sunxi.c +12 -0

+	default:
+		dev_err(musb->controller->parent,
+			"Error requested mode not supported by this kernel\n");
+		return -EINVAL;
+	}
+
+	if (glue->phy_mode == new_mode)
+		return 0;
+
+	if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) {
+		dev_err(musb->controller->parent,
+			"Error changing modes is only supported in dual role mode\n");

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 379

Quoted 10 lines

drivers/usb/musb/sunxi.c +10 -0

+		return -EINVAL;
+	}
+
+	/*
+	 * phy_set_mode may sleep, and we're called with a spinlock held,
+	 * so let sunxi_musb_work deal with it.
+	 */
+	glue->phy_mode = new_mode;
+	set_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags);
+	schedule_work(&glue->work);

When switching from host to peripheral mode, if an usb device is still plugged and enumerated, how do you handle the device disconnect?

Quoted 3 lines

The phy code will report vbus low for long enough for the musb to end the current session. It already does this for boards which do not have working vbus detection.

But you didn't disconnect DP/DM, right? then musb detects vbus is gone without receiving disconnect event, this is vbus error case, not a normal teardown.

Hi,

Quoted 96 lines · Bin Liu

Hi,

Quoted 92 lines · Hans de Goede

Hi,

Quoted 45 lines · Bin Liu

Hi,

Quoted 42 lines · Hans de Goede

This allows run-time dr_mode switching support via the "mode" musb sysfs attribute.

Signed-off-by Hans de Goede <[email protected]>

---
drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----

1 file changed, 48 insertions(+), 4 deletions(-)

drivers/usb/musb/sunxi.c +5 -0

index c6ee166..1fe7451 100644
@@ -74,6 +74,7 @@
 #define SUNXI_MUSB_FL_HAS_SRAM			5
 #define SUNXI_MUSB_FL_HAS_RESET			6
 #define SUNXI_MUSB_FL_NO_CONFIGDATA		7
+#define SUNXI_MUSB_FL_PHY_MODE_PEND		8
 
 /* Our read/write methods need access and do not get passed in a musb ref :| */
 static struct musb *sunxi_musb;

@@ -87,6 +88,7 @@ struct sunxi_glue {
 	struct phy		*phy;
 	struct platform_device	*usb_phy;
 	struct usb_phy		*xceiv;
+	enum phy_mode		phy_mode;
 	unsigned long		flags;
 	struct work_struct	work;
 	struct extcon_dev	*extcon;

@@ -140,6 +142,9 @@ static void sunxi_musb_work(struct work_struct *work)
 			clear_bit(SUNXI_MUSB_FL_PHY_ON, &glue->flags);
 		}
 	}
+
+	if (test_and_clear_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags))
+		phy_set_mode(glue->phy, glue->phy_mode);
 }
 
 static void sunxi_musb_set_vbus(struct musb *musb, int is_on)

@@ -341,6 +346,41 @@ static void sunxi_musb_dma_controller_destroy(struct dma_controller *c)
 {
 }
 

Quoted 11 lines

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 357

Quoted 9 lines

drivers/usb/musb/sunxi.c +9 -0

+static int sunxi_musb_set_mode(struct musb *musb, u8 mode)
+{
+	struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
+	enum phy_mode new_mode;
+
+	switch (mode) {
+	case MUSB_HOST:		new_mode = PHY_MODE_USB_HOST; break;
+	case MUSB_PERIPHERAL:	new_mode = PHY_MODE_USB_DEVICE; break;
+	case MUSB_OTG:		new_mode = PHY_MODE_USB_OTG; break;

Please fix the code style as commented in patch 4/7.

Ok I will send a new version with this fixed.

Quoted 18 lines

Quoted 18 lines

drivers/usb/musb/sunxi.c +17 -0

+	default:
+		dev_err(musb->controller->parent,
+			"Error requested mode not supported by this kernel\n");
+		return -EINVAL;
+	}
+
+	if (glue->phy_mode == new_mode)
+		return 0;
+
+	if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) {
+		dev_err(musb->controller->parent,
+			"Error changing modes is only supported in dual role mode\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * phy_set_mode may sleep, and we're called with a spinlock held,

Quoted 8 lines

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 379

Quoted 5 lines

drivers/usb/musb/sunxi.c +5 -0

+	 * so let sunxi_musb_work deal with it.
+	 */
+	glue->phy_mode = new_mode;
+	set_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags);
+	schedule_work(&glue->work);

When switching from host to peripheral mode, if an usb device is still plugged and enumerated, how do you handle the device disconnect?

The phy code will report vbus low for long enough for the musb to end the current session. It already does this for boards which do not have working vbus detection.

Quoted 3 lines

But you didn't disconnect DP/DM, right? then musb detects vbus is gone without receiving disconnect event, this is vbus error case, not a normal teardown.

Correct, there is no way to disconnect DP/DM and reporting Vbus low for a while does the trick.

Regards,

Hans

Quoted 103 lines · Hans de Goede

Hi,

Quoted 99 lines · Bin Liu

Hi,

Quoted 89 lines · Hans de Goede

Hi,

Quoted 45 lines · Bin Liu

Hi,

Quoted 42 lines · Hans de Goede

This allows run-time dr_mode switching support via the "mode" musb sysfs attribute.

Signed-off-by Hans de Goede <[email protected]>

---
drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----

1 file changed, 48 insertions(+), 4 deletions(-)

drivers/usb/musb/sunxi.c +5 -0

index c6ee166..1fe7451 100644
@@ -74,6 +74,7 @@
 #define SUNXI_MUSB_FL_HAS_SRAM			5
 #define SUNXI_MUSB_FL_HAS_RESET			6
 #define SUNXI_MUSB_FL_NO_CONFIGDATA		7
+#define SUNXI_MUSB_FL_PHY_MODE_PEND		8
 
 /* Our read/write methods need access and do not get passed in a musb ref :| */
 static struct musb *sunxi_musb;

@@ -87,6 +88,7 @@ struct sunxi_glue {
 	struct phy		*phy;
 	struct platform_device	*usb_phy;
 	struct usb_phy		*xceiv;
+	enum phy_mode		phy_mode;
 	unsigned long		flags;
 	struct work_struct	work;
 	struct extcon_dev	*extcon;

@@ -140,6 +142,9 @@ static void sunxi_musb_work(struct work_struct *work)
 			clear_bit(SUNXI_MUSB_FL_PHY_ON, &glue->flags);
 		}
 	}
+
+	if (test_and_clear_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags))
+		phy_set_mode(glue->phy, glue->phy_mode);
 }
 
 static void sunxi_musb_set_vbus(struct musb *musb, int is_on)

@@ -341,6 +346,41 @@ static void sunxi_musb_dma_controller_destroy(struct dma_controller *c)
 {
 }
 

Quoted 11 lines

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 357

Quoted 9 lines

drivers/usb/musb/sunxi.c +9 -0

+static int sunxi_musb_set_mode(struct musb *musb, u8 mode)
+{
+	struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
+	enum phy_mode new_mode;
+
+	switch (mode) {
+	case MUSB_HOST:		new_mode = PHY_MODE_USB_HOST; break;
+	case MUSB_PERIPHERAL:	new_mode = PHY_MODE_USB_DEVICE; break;
+	case MUSB_OTG:		new_mode = PHY_MODE_USB_OTG; break;

Please fix the code style as commented in patch 4/7.

Ok I will send a new version with this fixed.

Quoted 18 lines

Quoted 18 lines

drivers/usb/musb/sunxi.c +17 -0

+	default:
+		dev_err(musb->controller->parent,
+			"Error requested mode not supported by this kernel\n");
+		return -EINVAL;
+	}
+
+	if (glue->phy_mode == new_mode)
+		return 0;
+
+	if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) {
+		dev_err(musb->controller->parent,
+			"Error changing modes is only supported in dual role mode\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * phy_set_mode may sleep, and we're called with a spinlock held,

Quoted 8 lines

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 379

Quoted 5 lines

drivers/usb/musb/sunxi.c +5 -0

+	 * so let sunxi_musb_work deal with it.
+	 */
+	glue->phy_mode = new_mode;
+	set_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags);
+	schedule_work(&glue->work);

When switching from host to peripheral mode, if an usb device is still plugged and enumerated, how do you handle the device disconnect?

Quoted 3 lines

The phy code will report vbus low for long enough for the musb to end the current session. It already does this for boards which do not have working vbus detection.

But you didn't disconnect DP/DM, right? then musb detects vbus is gone without receiving disconnect event, this is vbus error case, not a normal teardown.

Quoted 2 lines

Correct, there is no way to disconnect DP/DM and reporting Vbus low for a while does the trick.

Without physically disconnecting DP/DM, we still have a way to properly teardown the enumerated devices. Please check musb_softconnect_write() in musb_debugfs.c.

HI,

Quoted 107 lines · Bin Liu

Quoted 105 lines · Hans de Goede

Hi,

Quoted 92 lines · Bin Liu

Hi,

Quoted 89 lines · Hans de Goede

Hi,

Quoted 45 lines · Bin Liu

Hi,

Quoted 42 lines · Hans de Goede

This allows run-time dr_mode switching support via the "mode" musb sysfs attribute.

Signed-off-by Hans de Goede <[email protected]>

---
drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----

1 file changed, 48 insertions(+), 4 deletions(-)

drivers/usb/musb/sunxi.c +5 -0

index c6ee166..1fe7451 100644
@@ -74,6 +74,7 @@
 #define SUNXI_MUSB_FL_HAS_SRAM			5
 #define SUNXI_MUSB_FL_HAS_RESET			6
 #define SUNXI_MUSB_FL_NO_CONFIGDATA		7
+#define SUNXI_MUSB_FL_PHY_MODE_PEND		8
 
 /* Our read/write methods need access and do not get passed in a musb ref :| */
 static struct musb *sunxi_musb;

@@ -87,6 +88,7 @@ struct sunxi_glue {
 	struct phy		*phy;
 	struct platform_device	*usb_phy;
 	struct usb_phy		*xceiv;
+	enum phy_mode		phy_mode;
 	unsigned long		flags;
 	struct work_struct	work;
 	struct extcon_dev	*extcon;

@@ -140,6 +142,9 @@ static void sunxi_musb_work(struct work_struct *work)
 			clear_bit(SUNXI_MUSB_FL_PHY_ON, &glue->flags);
 		}
 	}
+
+	if (test_and_clear_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags))
+		phy_set_mode(glue->phy, glue->phy_mode);
 }
 
 static void sunxi_musb_set_vbus(struct musb *musb, int is_on)

@@ -341,6 +346,41 @@ static void sunxi_musb_dma_controller_destroy(struct dma_controller *c)
 {
 }
 

Quoted 11 lines

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 357

Quoted 9 lines

drivers/usb/musb/sunxi.c +9 -0

+static int sunxi_musb_set_mode(struct musb *musb, u8 mode)
+{
+	struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
+	enum phy_mode new_mode;
+
+	switch (mode) {
+	case MUSB_HOST:		new_mode = PHY_MODE_USB_HOST; break;
+	case MUSB_PERIPHERAL:	new_mode = PHY_MODE_USB_DEVICE; break;
+	case MUSB_OTG:		new_mode = PHY_MODE_USB_OTG; break;

Please fix the code style as commented in patch 4/7.

Ok I will send a new version with this fixed.

Quoted 26 lines

Quoted 13 lines

drivers/usb/musb/sunxi.c +12 -0

+	default:
+		dev_err(musb->controller->parent,
+			"Error requested mode not supported by this kernel\n");
+		return -EINVAL;
+	}
+
+	if (glue->phy_mode == new_mode)
+		return 0;
+
+	if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) {
+		dev_err(musb->controller->parent,
+			"Error changing modes is only supported in dual role mode\n");

drivers/usb/musb/sunxi.c static void sunxi_musb_dma_controller_destroy(struct dma_controller *c) · line 379

Quoted 10 lines

drivers/usb/musb/sunxi.c +10 -0

+		return -EINVAL;
+	}
+
+	/*
+	 * phy_set_mode may sleep, and we're called with a spinlock held,
+	 * so let sunxi_musb_work deal with it.
+	 */
+	glue->phy_mode = new_mode;
+	set_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags);
+	schedule_work(&glue->work);

When switching from host to peripheral mode, if an usb device is still plugged and enumerated, how do you handle the device disconnect?

Quoted 7 lines

Quoted 3 lines

The phy code will report vbus low for long enough for the musb to end the current session. It already does this for boards which do not have working vbus detection.

But you didn't disconnect DP/DM, right? then musb detects vbus is gone without receiving disconnect event, this is vbus error case, not a normal teardown.

Correct, there is no way to disconnect DP/DM and reporting Vbus low for a while does the trick.

Quoted 3 lines

Without physically disconnecting DP/DM, we still have a way to properly teardown the enumerated devices. Please check musb_softconnect_write() in musb_debugfs.c.

That is manipulating the session bit in the devctl reg, that does not work to switch from device to host role or from host to device role, at least not on allwinner's musb implementation. I've already tried that before writing the code to report VBus low.

Regards,

Hans

On Mon, Aug 22, 2016 at 05:32:57PM +0200, Hans de Goede wrote:

Quoted 18 lines

Quoted 17 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

When switching from host to peripheral mode, if an usb device is still plugged and enumerated, how do you handle the device disconnect?

The phy code will report vbus low for long enough for the musb to end the current session. It already does this for boards which do not have working vbus detection.

Quoted 6 lines

Quoted 3 lines

But you didn't disconnect DP/DM, right? then musb detects vbus is gone without receiving disconnect event, this is vbus error case, not a normal teardown.

Correct, there is no way to disconnect DP/DM and reporting Vbus low for a while does the trick.

Without physically disconnecting DP/DM, we still have a way to properly teardown the enumerated devices. Please check musb_softconnect_write() in musb_debugfs.c.

Quoted 4 lines

That is manipulating the session bit in the devctl reg, that does not work to switch from device to host role or from host to device role, at least not on allwinner's musb implementation. I've already tried that before writing the code to report VBus low.

I would think you have to call musb_root_disconnect() first to notify the core to teardown the enumerated devices.

Hi,

Quoted 1 line · Bin Liu

On Mon, Aug 22, 2016 at 05:32:57PM +0200, Hans de Goede wrote:

<snip>

Quoted 23 lines

Quoted 22 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

When switching from host to peripheral mode, if an usb device is still plugged and enumerated, how do you handle the device disconnect?

The phy code will report vbus low for long enough for the musb to end the current session. It already does this for boards which do not have working vbus detection.

But you didn't disconnect DP/DM, right? then musb detects vbus is gone without receiving disconnect event, this is vbus error case, not a normal teardown.

Quoted 6 lines

Quoted 2 lines

Correct, there is no way to disconnect DP/DM and reporting Vbus low for a while does the trick.

Without physically disconnecting DP/DM, we still have a way to properly teardown the enumerated devices. Please check musb_softconnect_write() in musb_debugfs.c.

That is manipulating the session bit in the devctl reg, that does not work to switch from device to host role or from host to device role, at least not on allwinner's musb implementation. I've already tried that before writing the code to report VBus low.

Quoted 2 lines

I would think you have to call musb_root_disconnect() first to notify the core to teardown the enumerated devices.

I tried that it does not help. It only affects the software state, the hardware will still stay in host-mode if it does not see vbus low for long enough).

Note this is on devices which lack vbus detection (again this is simply physically not available on the PCB, just like some PCB's miss the id-pin). Normally this never is an issue, because when a host cable gets unplugged from a AB connector we see the id pin go high, disable the boards driving of Vbus and then the phy's Vbus detect will report low to the musb controller.

Anyways this is a solved problem, the reporting of Vbus low has been working fine for both boards which lack vbus-detection as well as for role-changing from sysfs.

Regards,

Hans

Quoted 30 lines · Hans de Goede

Hi,

Quoted 1 line · Bin Liu

On Mon, Aug 22, 2016 at 05:32:57PM +0200, Hans de Goede wrote:

<snip>

Quoted 23 lines

Quoted 22 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

When switching from host to peripheral mode, if an usb device is still plugged and enumerated, how do you handle the device disconnect?

The phy code will report vbus low for long enough for the musb to end the current session. It already does this for boards which do not have working vbus detection.

But you didn't disconnect DP/DM, right? then musb detects vbus is gone without receiving disconnect event, this is vbus error case, not a normal teardown.

Quoted 6 lines

Quoted 2 lines

Correct, there is no way to disconnect DP/DM and reporting Vbus low for a while does the trick.

Without physically disconnecting DP/DM, we still have a way to properly teardown the enumerated devices. Please check musb_softconnect_write() in musb_debugfs.c.

That is manipulating the session bit in the devctl reg, that does not work to switch from device to host role or from host to device role, at least not on allwinner's musb implementation. I've already tried that before writing the code to report VBus low.

Quoted 6 lines

Quoted 2 lines

I would think you have to call musb_root_disconnect() first to notify the core to teardown the enumerated devices.

I tried that it does not help. It only affects the software state, the hardware will still stay in host-mode if it does not see vbus low for long enough).

I didn't mean to use musb_root_disconnect() to replace your implementation. I suggest to add this call before lowing vbus to let the core tearing down the numerated devices.

Quoted 7 lines

Note this is on devices which lack vbus detection (again this is simply physically not available on the PCB, just like some PCB's miss the id-pin). Normally this never is an issue, because when a host cable gets unplugged from a AB connector we see the id pin go high, disable the boards driving of Vbus and then the phy's Vbus detect will report low to the musb controller.

In this case, DP/DM get disconnected before vbus line, so musb_root_disconnect() gets called in handling the disconnect interrupt event. I think it is better simulate this in your usecase, which does not generate disconnect event.

Regards,
-Bin.

Quoted 8 lines

Anyways this is a solved problem, the reporting of Vbus low has been working fine for both boards which lack vbus-detection as well as for role-changing from sysfs.

Regards,

Hans

Hi All,

Here is a patch series which implements run-time changing the dr-mode of sunxi musb controllers through the (already existing) musb "mode" sysfs attribute.

This is useful on boards where there is no id pin, e.g. some tv-boxes use the musb controller to get an extra usb A port without needing a hub chip. Except for the missing id pin when using a usb A<->A cable these ports can do peripheral mode just fine. This series makes it possible to do e.g. this by doing echo "peripheral" > mode before plugging in the usb A<->A cable.

This series has both sun4i-usb-phy driver and sunxi-musb-glue changes, both are necessary for the run-time changing to work, but they can be merged independently without breaking anything.

Changed in v2:

After sleeping on it a night I realized that always passing port_mode = DUAL_ROLE to the musb-core was wrong. There is a distintion between the id-pin not working properly which we can work around with software mode switching (and we want to register both the host and udc driver in this case) vs cases where we really only want to register a host (wifi module connected to musb soldered onto the PCB).

So v2 of this series drops the
"musb: sunxi: Always register both host and udc when build with dual-role support" patch.

Instead systems which are dual-role capable, but lack an id-pin should use dr_mode = "otg" in the dts file. There is one problem with this, some systems are dual-role capable but use a female USB-A connector connected to the musb controller. These should come up in host mode by default,

rather then peripheral mode which is the default for systems which lack an id-pin. This patch set introduces:

"phy-sun4i-usb: Add "allwinner,usb0-usb-a-connector" dt property"

Which allows specifying the use of a female USB-A connector for the musb controller in the phy dt node, the presence of this dt property changes the default to host mode.

Please review (and if no issues are found merge).

Thanks & Regards,

Hans

Hi,

Quoted 6 lines · Hans de Goede

Hi All,

Here is a patch series which implements run-time changing the dr-mode of sunxi musb controllers through the (already existing) musb "mode" sysfs attribute.

Quoted 6 lines

This is useful on boards where there is no id pin, e.g. some tv-boxes use the musb controller to get an extra usb A port without needing a hub chip. Except for the missing id pin when using a usb A<->A cable these ports can do peripheral mode just fine. This series makes it possible to do e.g. this by doing echo "peripheral" > mode before plugging in the usb A<->A cable.

Well, this is an illegal usecase. A-A cable is invalid by USB Spec.

With type-A receptacle the controller should be in host-only mode, switching to peripheral mode should not be allowed.

Quoted 17 lines

This series has both sun4i-usb-phy driver and sunxi-musb-glue changes, both are necessary for the run-time changing to work, but they can be merged independently without breaking anything.

Changed in v2:

After sleeping on it a night I realized that always passing port_mode = DUAL_ROLE to the musb-core was wrong. There is a distintion between the id-pin not working properly which we can work around with software mode switching (and we want to register both the host and udc driver in this case) vs cases where we really only want to register a host (wifi module connected to musb soldered onto the PCB).

So v2 of this series drops the
"musb: sunxi: Always register both host and udc when build with dual-role support" patch.

Quoted 4 lines

Instead systems which are dual-role capable, but lack an id-pin should use dr_mode = "otg" in the dts file. There is one problem with this, some systems are dual-role capable but use a female USB-A connector connected to the musb controller. These should come up in host mode by default,

This is not a problem. With a type-A connector, the dual-role controller should work in host-only mode.
Role switching should only be allowed if an AB connector is used.

Using the sysfs entry to switch roles for generic purpose is really a bad idea, it opens up ton of problems.

For systems which lack of id-pin should use a discrete circuit (for example GPIO) to detect the id pin in the AB receptacle, then the USB driver will handle the role switching transparently.

Quoted 8 lines

rather then peripheral mode which is the default for systems which lack an id-pin. This patch set introduces:

"phy-sun4i-usb: Add "allwinner,usb0-usb-a-connector" dt property"

Which allows specifying the use of a female USB-A connector for the musb controller in the phy dt node, the presence of this dt property changes the default to host mode.

This is unnecessary, if using a type-A connector, dr_mode should be "host" in DT.

Hi,

Quoted 16 lines · Bin Liu

Hi,

Quoted 12 lines · Hans de Goede

Hi All,

Here is a patch series which implements run-time changing the dr-mode of sunxi musb controllers through the (already existing) musb "mode" sysfs attribute.

This is useful on boards where there is no id pin, e.g. some tv-boxes use the musb controller to get an extra usb A port without needing a hub chip. Except for the missing id pin when using a usb A<->A cable these ports can do peripheral mode just fine. This series makes it possible to do e.g. this by doing echo "peripheral" > mode before plugging in the usb A<->A cable.

Quoted 1 line

Well, this is an illegal usecase. A-A cable is invalid by USB Spec.

Yet they exist and there are USB devices (e.g. harddisk enclosures) which use a USB-A connector even though they are a device not a host. I own several such devices myself. I agree that this is wrong but these devices exist, typical case of reality trumping the spec.

Quoted 3 lines

With type-A receptacle the controller should be in host-only mode, switching to peripheral mode should not be allowed.

Peripheral mode can still be quite useful, e.g. to do ethernet over USB (some of these devices lack wired ethernet).

Also the manual of these devices typically instructs users to use an A<->A cable for firmware updates, since the bootROM in

the SoC does not care anmd will happily put the device in Allwinner's custom DFU mode using the USB-A connector.

Although I agree with this being not ideal, the fact is that an USB-A connector combined with a A<->A cable is electrically 100% identical to a USB-B connector with its id-pin not connected (on the PCB side), which also happens see below.

Quoted 23 lines

Quoted 22 lines

This series has both sun4i-usb-phy driver and sunxi-musb-glue changes, both are necessary for the run-time changing to work, but they can be merged independently without breaking anything.

Changed in v2:

After sleeping on it a night I realized that always passing port_mode = DUAL_ROLE to the musb-core was wrong. There is a distintion between the id-pin not working properly which we can work around with software mode switching (and we want to register both the host and udc driver in this case) vs cases where we really only want to register a host (wifi module connected to musb soldered onto the PCB).

So v2 of this series drops the
"musb: sunxi: Always register both host and udc when build with dual-role support" patch.

Instead systems which are dual-role capable, but lack an id-pin should use dr_mode = "otg" in the dts file. There is one problem with this, some systems are dual-role capable but use a female USB-A connector connected to the musb controller. These should come up in host mode by default,

Quoted 3 lines

This is not a problem. With a type-A connector, the dual-role controller should work in host-only mode.
Role switching should only be allowed if an AB connector is used.

Yet people may want to use it in peripheral mode, I strongly believe that we should not be telling people what they can and cannot do with their hardware. That is policy and the kernel does not set

such policy, that is up to userspace. OTOH the port should clearly default to host mode hence the new property.

Quoted 3 lines

Using the sysfs entry to switch roles for generic purpose is really a bad idea, it opens up ton of problems.

Yet the sysfs entry exists already, and the problems depend on how you hook it up. I'm merely using the sysfs entry as an id-pin for cases where there is no id pin hooked up. If you look at the patches you will see that all that is changed by writing the sysfs entry is the phy reporting a different id-pin value to the musb ip. Everything else is handled the same as for any normal otg mode port.

Quoted 3 lines

For systems which lack of id-pin should use a discrete circuit (for example GPIO) to detect the id pin in the AB receptacle, then the USB driver will handle the role switching transparently.

You're again talking theory here in reality there is hardware where for whatever reasons the PCB uses a mini or micro usb receptacle, but they did not bother to _physically_ hook up the id pin.

As said before reality trumps the Spec / theory.

Quoted 9 lines

Quoted 8 lines

rather then peripheral mode which is the default for systems which lack an id-pin. This patch set introduces:

"phy-sun4i-usb: Add "allwinner,usb0-usb-a-connector" dt property"

Which allows specifying the use of a female USB-A connector for the musb controller in the phy dt node, the presence of this dt property changes the default to host mode.

Quoted 2 lines

This is unnecessary, if using a type-A connector, dr_mode should be "host" in DT.

Which means we're telling users what they can and cannot do with their hardware, which we should not be doing.

Regards,

Hans

Hi,

Quoted 19 lines · Hans de Goede

Hi,

Quoted 16 lines · Bin Liu

Hi,

Quoted 12 lines · Hans de Goede

Hi All,

Here is a patch series which implements run-time changing the dr-mode of sunxi musb controllers through the (already existing) musb "mode" sysfs attribute.

This is useful on boards where there is no id pin, e.g. some tv-boxes use the musb controller to get an extra usb A port without needing a hub chip. Except for the missing id pin when using a usb A<->A cable these ports can do peripheral mode just fine. This series makes it possible to do e.g. this by doing echo "peripheral" > mode before plugging in the usb A<->A cable.

Quoted 7 lines

Quoted 1 line

Well, this is an illegal usecase. A-A cable is invalid by USB Spec.

Yet they exist and there are USB devices (e.g. harddisk enclosures) which use a USB-A connector even though they are a device not a host. I own several such devices myself. I agree that this is wrong but these devices exist, typical case of reality trumping the spec.

I know those products exist, but they are different from yours. Those existing devices are usb peripheral-only, which never source vbus so do not hurt anything other than violate the USB Spec. But your usecase allows the device switching to host mode, which could damage the usb host on the other end of the connection due to vbus contention.

Quoted 6 lines

Quoted 3 lines

With type-A receptacle the controller should be in host-only mode, switching to peripheral mode should not be allowed.

Peripheral mode can still be quite useful, e.g. to do ethernet over USB (some of these devices lack wired ethernet).

For such usecase, a micro-AB connector should be used, or use two connectors - type-A and type-B which are mux'd to the same usb port.

Quoted 3 lines

Also the manual of these devices typically instructs users to use an A<->A cable for firmware updates, since the bootROM in

(Nobody reads the manual, we all know it...) This does not prevent users to connect two hosts together then damage the hardware. So this design should not be allowed.

Quoted 7 lines

the SoC does not care anmd will happily put the device in Allwinner's custom DFU mode using the USB-A connector.

Although I agree with this being not ideal, the fact is that an USB-A connector combined with a A<->A cable is electrically 100% identical to a USB-B connector with its id-pin not connected (on the PCB side), which also happens see below.

they are two different cables. A-B cable does not provide a chance to connect two hosts together, but A-A cable does.

Quoted 23 lines

Quoted 23 lines

Quoted 22 lines

This series has both sun4i-usb-phy driver and sunxi-musb-glue changes, both are necessary for the run-time changing to work, but they can be merged independently without breaking anything.

Changed in v2:

After sleeping on it a night I realized that always passing port_mode = DUAL_ROLE to the musb-core was wrong. There is a distintion between the id-pin not working properly which we can work around with software mode switching (and we want to register both the host and udc driver in this case) vs cases where we really only want to register a host (wifi module connected to musb soldered onto the PCB).

So v2 of this series drops the
"musb: sunxi: Always register both host and udc when build with dual-role support" patch.

Instead systems which are dual-role capable, but lack an id-pin should use dr_mode = "otg" in the dts file. There is one problem with this, some systems are dual-role capable but use a female USB-A connector connected to the musb controller. These should come up in host mode by default,

Quoted 7 lines

Quoted 3 lines

This is not a problem. With a type-A connector, the dual-role controller should work in host-only mode.
Role switching should only be allowed if an AB connector is used.

Yet people may want to use it in peripheral mode, I strongly believe that we should not be telling people what they can and cannot do with their hardware. That is policy and the kernel does not set

I agree with the policy, but we are responsible to tell people don't do something which is not correct.

Quoted 2 lines

such policy, that is up to userspace. OTOH the port should clearly default to host mode hence the new property.

Due to the problem with A-A cable I explained above, A-A cable should not be used in role-switching cases (it should be used at the first

place), so this new property is unnecessary.

Quoted 12 lines

Quoted 3 lines

Using the sysfs entry to switch roles for generic purpose is really a bad idea, it opens up ton of problems.

Yet the sysfs entry exists already, and the problems depend on how you hook it up. I'm merely using the sysfs entry as an id-pin for cases where there is no id pin hooked up. If you look at the patches you will see that all that is changed by writing the sysfs entry is the phy reporting a different id-pin value to the musb ip. Everything else is handled the same as for any normal otg mode port.

Quoted 7 lines

Quoted 3 lines

For systems which lack of id-pin should use a discrete circuit (for example GPIO) to detect the id pin in the AB receptacle, then the USB driver will handle the role switching transparently.

You're again talking theory here in reality there is hardware where for whatever reasons the PCB uses a mini or micro usb receptacle, but they did not bother to _physically_ hook up the id pin.

Who are they? the manufactures who use musb controller to make products, so as Linux or musb drivers, we have control, it is our responsibility to tell those manufactures to do things in the right way - design the hw correctly at the first place. We can't compromise by using an A-A cable to provide a change to damage the usb host.

This is different from the cases, for example, from the xhci controller perspective, the xhci driver has to compromise any usb thumb drives which do not follow the USB Specs, to make them work on PCs.

Quoted 12 lines

As said before reality trumps the Spec / theory.

Quoted 9 lines

Quoted 8 lines

rather then peripheral mode which is the default for systems which lack an id-pin. This patch set introduces:

"phy-sun4i-usb: Add "allwinner,usb0-usb-a-connector" dt property"

Which allows specifying the use of a female USB-A connector for the musb controller in the phy dt node, the presence of this dt property changes the default to host mode.

Quoted 5 lines

Quoted 2 lines

This is unnecessary, if using a type-A connector, dr_mode should be "host" in DT.

Which means we're telling users what they can and cannot do with their hardware, which we should not be doing.

To summary up my opinion from my lengthy comments above,

A-A cable should not be used, especially in role-switching cases, so the DT prop is unnecessary;

Quoted 4 lines

Regards,

Hans

Regards,
-Bin.

Hi,

Quoted 19 lines · Hans de Goede

Hi,

Quoted 16 lines · Bin Liu

Hi,

Quoted 12 lines · Hans de Goede

Hi All,

Here is a patch series which implements run-time changing the dr-mode of sunxi musb controllers through the (already existing) musb "mode" sysfs attribute.

This is useful on boards where there is no id pin, e.g. some tv-boxes use the musb controller to get an extra usb A port without needing a hub chip. Except for the missing id pin when using a usb A<->A cable these ports can do peripheral mode just fine. This series makes it possible to do e.g. this by doing echo "peripheral" > mode before plugging in the usb A<->A cable.

Quoted 7 lines

Quoted 1 line

Well, this is an illegal usecase. A-A cable is invalid by USB Spec.

Yet they exist and there are USB devices (e.g. harddisk enclosures) which use a USB-A connector even though they are a device not a host. I own several such devices myself. I agree that this is wrong but these devices exist, typical case of reality trumping the spec.

I know those products exist, but they are different from yours. Those existing devices are usb peripheral-only, which never source vbus so do not hurt anything other than violate the USB Spec. But your usecase allows the device switching to host mode, which could damage the usb host on the other end of the connection due to vbus contention.

Quoted 5 lines

Quoted 2 lines

With type-A receptacle the controller should be in host-only mode, switching to peripheral mode should not be allowed.

Peripheral mode can still be quite useful, e.g. to do ethernet over USB (some of these devices lack wired ethernet).

For such usecase, a micro-AB connector should be used, or use two connectors - type-A and type-B which are mux'd to the same usb port.

Quoted 1 line

Quoted 2 lines

Also the manual of these devices typically instructs users to use an A<->A cable for firmware updates, since the bootROM in

(Nobody reads the manual, we all know it...) This does not prevent users to connect two hosts together then damage the hardware. So this design should not be allowed.

Quoted 7 lines

the SoC does not care anmd will happily put the device in Allwinner's custom DFU mode using the USB-A connector.

Although I agree with this being not ideal, the fact is that an USB-A connector combined with a A<->A cable is electrically 100% identical to a USB-B connector with its id-pin not connected (on the PCB side), which also happens see below.

they are two different cables. A-B cable does not provide a chance to connect two hosts together, but A-A cable does.

Quoted 22 lines

Quoted 22 lines

Quoted 21 lines

This series has both sun4i-usb-phy driver and sunxi-musb-glue changes, both are necessary for the run-time changing to work, but they can be merged independently without breaking anything.

Changed in v2:

After sleeping on it a night I realized that always passing port_mode = DUAL_ROLE to the musb-core was wrong. There is a distintion between the id-pin not working properly which we can work around with software mode switching (and we want to register both the host and udc driver in this case) vs cases where we really only want to register a host (wifi module connected to musb soldered onto the PCB).

So v2 of this series drops the
"musb: sunxi: Always register both host and udc when build with dual-role support" patch.

Instead systems which are dual-role capable, but lack an id-pin should use dr_mode = "otg" in the dts file. There is one problem with this, some systems are dual-role capable but use a female USB-A connector connected to the musb controller. These should come up in host mode by default,

Quoted 7 lines

Quoted 3 lines

This is not a problem. With a type-A connector, the dual-role controller should work in host-only mode.
Role switching should only be allowed if an AB connector is used.

Yet people may want to use it in peripheral mode, I strongly believe that we should not be telling people what they can and cannot do with their hardware. That is policy and the kernel does not set

I agree with the policy, but we are responsible to tell people don't do something which is not correct.

Quoted 2 lines

such policy, that is up to userspace. OTOH the port should clearly default to host mode hence the new property.

Due to the problem with A-A cable I explained above, A-A cable should not be used in role-switching cases (it should be used at the first place), so this new property is unnecessary.

Quoted 16 lines

Quoted 3 lines

Using the sysfs entry to switch roles for generic purpose is really a bad idea, it opens up ton of problems.

Yet the sysfs entry exists already, and the problems depend on how you hook it up. I'm merely using the sysfs entry as an id-pin for cases where there is no id pin hooked up. If you look at the patches you will see that all that is changed by writing the sysfs entry is the phy reporting a different id-pin value to the musb ip. Everything else is handled the same as for any normal otg mode port.

Quoted 3 lines

For systems which lack of id-pin should use a discrete circuit (for example GPIO) to detect the id pin in the AB receptacle, then the USB driver will handle the role switching transparently.

Quoted 3 lines

You're again talking theory here in reality there is hardware where for whatever reasons the PCB uses a mini or micro usb receptacle, but they did not bother to _physically_ hook up the id pin.

Who are they?

the manufactures who use musb controller to make products, so as Linux or musb drivers, we have control, it is our responsibility to tell those manufactures to do things in the right way - design the hw correctly at the first place. We can't compromise by using an A-A cable to provide a change to damage the usb host.

This is different from the cases, for example, from the xhci controller perspective, the xhci driver has to compromise any usb thumb drives which do not follow the USB Specs, to make them work on PCs.

Quoted 14 lines

As said before reality trumps the Spec / theory.

Quoted 11 lines

Quoted 8 lines

rather then peripheral mode which is the default for systems which lack an id-pin. This patch set introduces:

"phy-sun4i-usb: Add "allwinner,usb0-usb-a-connector" dt property"

Which allows specifying the use of a female USB-A connector for the musb controller in the phy dt node, the presence of this dt property changes the default to host mode.

This is unnecessary, if using a type-A connector, dr_mode should be "host" in DT.

Quoted 2 lines

Which means we're telling users what they can and cannot do with their hardware, which we should not be doing.

To summary up my opinion from my lengthy comments above,

A-A cable should not be used, especially in role-switching cases, so the DT prop is unnecessary;

Hi,

Quoted 30 lines · Bin Liu

Hi,

Quoted 19 lines · Hans de Goede

Hi,

Quoted 16 lines · Bin Liu

Hi,

Quoted 12 lines · Hans de Goede

Hi All,

Here is a patch series which implements run-time changing the dr-mode of sunxi musb controllers through the (already existing) musb "mode" sysfs attribute.

This is useful on boards where there is no id pin, e.g. some tv-boxes use the musb controller to get an extra usb A port without needing a hub chip. Except for the missing id pin when using a usb A<->A cable these ports can do peripheral mode just fine. This series makes it possible to do e.g. this by doing echo "peripheral" > mode before plugging in the usb A<->A cable.

Quoted 7 lines

Quoted 1 line

Well, this is an illegal usecase. A-A cable is invalid by USB Spec.

Yet they exist and there are USB devices (e.g. harddisk enclosures) which use a USB-A connector even though they are a device not a host. I own several such devices myself. I agree that this is wrong but these devices exist, typical case of reality trumping the spec.

Quoted 5 lines

I know those products exist, but they are different from yours. Those existing devices are usb peripheral-only, which never source vbus so do not hurt anything other than violate the USB Spec. But your usecase allows the device switching to host mode, which could damage the usb host on the other end of the connection due to vbus contention.

Actually the sun4i phy driver avoids this by checking vbus-detect for external vbus presence before enabling the boards own Vbus.

More importantly, not supporting peripheral mode on musb attached to an USB-A receptacle will not stop users from trying to use it and insert an A<->A cable at which point the damage is already done.

I understand why you dislike this, but we cannot stop a user from trying this. So it is better to actually be prepared for this (e.g. do the vbus-detect check we're already doing) then it is to not support this.

For example:

Lets say we do not have vbus-detect and the user plugs in an A<->A cable causing Vbus being driven from 2 sides.

Not good, now we have current flowing between the 2 power supplies and things are heating up.

Now there are 2 possible follow-up scenarios:

1) We support peripheral mode, the user does "echo peripheral > mode" in sysfs and we disable the boards Vbus, no more current, things are cooling down again before components burn up.

2) We do not support peripheral mode (which is what you're suggesting), the user does "echo peripheral > mode" in sysfs, nothing happens. Now we can only happen the user unplugs the A<->A cable soon, instead of trying a bunch of other things.

Neither is ideal, but do you see how not allowing peripheral mode is not helping here ? As soon as the user plugs in an A<->A cable bad things may happen, we cannot avoid that from the software side.

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

With type-A receptacle the controller should be in host-only mode, switching to peripheral mode should not be allowed.

Peripheral mode can still be quite useful, e.g. to do ethernet over USB (some of these devices lack wired ethernet).

For such usecase, a micro-AB connector should be used, or use two connectors - type-A and type-B which are mux'd to the same usb port.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

Also the manual of these devices typically instructs users to use an A<->A cable for firmware updates, since the bootROM in

(Nobody reads the manual, we all know it...) This does not prevent users to connect two hosts together then damage the hardware. So this design should not be allowed.

Nothing prevents an user from doing that, as soon as they have an A<->A cable they can easily do that, one can only hope they will not do it.

Quoted 10 lines

Quoted 7 lines

the SoC does not care anmd will happily put the device in Allwinner's custom DFU mode using the USB-A connector.

Although I agree with this being not ideal, the fact is that an USB-A connector combined with a A<->A cable is electrically 100% identical to a USB-B connector with its id-pin not connected (on the PCB side), which also happens see below.

they are two different cables. A-B cable does not provide a chance to connect two hosts together, but A-A cable does.

Right, but an A-B cable with its id-pin wrongly connected to ground (some so called otg charging hubs do this) will cause the exact same issue.

This is why usb-ports and especially otg ports should have current limiting on their Vbus and why the sun4i phy driver checks vbus-detect not reporting an external vbus before enabling Vbus.

Quoted 56 lines

Quoted 22 lines

Quoted 22 lines

Quoted 21 lines

This series has both sun4i-usb-phy driver and sunxi-musb-glue changes, both are necessary for the run-time changing to work, but they can be merged independently without breaking anything.

Changed in v2:

After sleeping on it a night I realized that always passing port_mode = DUAL_ROLE to the musb-core was wrong. There is a distintion between the id-pin not working properly which we can work around with software mode switching (and we want to register both the host and udc driver in this case) vs cases where we really only want to register a host (wifi module connected to musb soldered onto the PCB).

So v2 of this series drops the
"musb: sunxi: Always register both host and udc when build with dual-role support" patch.

Instead systems which are dual-role capable, but lack an id-pin should use dr_mode = "otg" in the dts file. There is one problem with this, some systems are dual-role capable but use a female USB-A connector connected to the musb controller. These should come up in host mode by default,

Quoted 7 lines

Quoted 3 lines

This is not a problem. With a type-A connector, the dual-role controller should work in host-only mode.
Role switching should only be allowed if an AB connector is used.

Yet people may want to use it in peripheral mode, I strongly believe that we should not be telling people what they can and cannot do with their hardware. That is policy and the kernel does not set

I agree with the policy, but we are responsible to tell people don't do something which is not correct.

Quoted 2 lines

such policy, that is up to userspace. OTOH the port should clearly default to host mode hence the new property.

Due to the problem with A-A cable I explained above, A-A cable should not be used in role-switching cases (it should be used at the first place), so this new property is unnecessary.

Quoted 16 lines

Quoted 3 lines

Using the sysfs entry to switch roles for generic purpose is really a bad idea, it opens up ton of problems.

Yet the sysfs entry exists already, and the problems depend on how you hook it up. I'm merely using the sysfs entry as an id-pin for cases where there is no id pin hooked up. If you look at the patches you will see that all that is changed by writing the sysfs entry is the phy reporting a different id-pin value to the musb ip. Everything else is handled the same as for any normal otg mode port.

Quoted 3 lines

For systems which lack of id-pin should use a discrete circuit (for example GPIO) to detect the id pin in the AB receptacle, then the USB driver will handle the role switching transparently.

Quoted 5 lines

Quoted 3 lines

You're again talking theory here in reality there is hardware where for whatever reasons the PCB uses a mini or micro usb receptacle, but they did not bother to _physically_ hook up the id pin.

Who are they?

One example of such a device is the quite popular mk802 tv stick (the original version) with A10s SoC.

Quoted 9 lines

the manufactures who use musb controller to make products, so as Linux or musb drivers, we have control, it is our responsibility to tell those manufactures to do things in the right way - design the hw correctly at the first place. We can't compromise by using an A-A cable to provide a change to damage the usb host.

This is different from the cases, for example, from the xhci controller perspective, the xhci driver has to compromise any usb thumb drives which do not follow the USB Specs, to make them work on PCs.

That makes no sense, why would the xhci driver have to compromise but we don't ? Either actually having things working is more important then following the spec or the spec is more important...

And in reality as your xhci example points out having things working (and that means offering all possible functionality) is more important.

Quoted 19 lines

Quoted 11 lines

As said before reality trumps the Spec / theory.

Quoted 9 lines

Quoted 8 lines

rather then peripheral mode which is the default for systems which lack an id-pin. This patch set introduces:

"phy-sun4i-usb: Add "allwinner,usb0-usb-a-connector" dt property"

Which allows specifying the use of a female USB-A connector for the musb controller in the phy dt node, the presence of this dt property changes the default to host mode.

Quoted 5 lines

Quoted 2 lines

This is unnecessary, if using a type-A connector, dr_mode should be "host" in DT.

Which means we're telling users what they can and cannot do with their hardware, which we should not be doing.

To summary up my opinion from my lengthy comments above,

Quoted 2 lines

A-A cable should not be used, especially in role-switching cases, so the DT prop is unnecessary;

A<_>A cables exist, users will try to plug them in, those are facts.

So we'd better be prepared to deal with this, denying this scenario exists is not helpful, not supporting it is equally unhelpful.

Regards,

Hans

Hi,

Quoted 6 lines · Hans de Goede

Hi All,

Here is a patch series which implements run-time changing the dr-mode of sunxi musb controllers through the (already existing) musb "mode" sysfs attribute.

Quoted 6 lines

This is useful on boards where there is no id pin, e.g. some tv-boxes use the musb controller to get an extra usb A port without needing a hub chip. Except for the missing id pin when using a usb A<->A cable these ports can do peripheral mode just fine. This series makes it possible to do e.g. this by doing echo "peripheral" > mode before plugging in the usb A<->A cable.

Well, this is an illegal usecase. A-A cable is invalid by USB Spec.

With type-A receptacle the controller should be in host-only mode, switching to peripheral mode should not be allowed.

This is not a problem. With a type-A connector, the dual-role controller should work in host-only mode.

Role switching should only be allowed if an AB connector is used.

Using the sysfs entry to switch roles for generic purpose is really a bad idea, it opens up ton of problems.