[PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs

[PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs

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

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

[PATCH v2 1/7] phy-sun4i-usb: Use bool where appropriate

[Hans de Goede]

We're using bool as true/false type in most places in phy-sun4i-usb.c
for consistency fixup the remaining uses of ints which are ever only
0 or 1 to be bools too.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/phy/phy-sun4i-usb.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index fcf4d95e..1cb84a8 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -445,7 +445,8 @@ static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
 	struct sun4i_usb_phy_data *data =
 		container_of(work, struct sun4i_usb_phy_data, detect.work);
 	struct phy *phy0 = data->phys[0].phy;
-	int id_det, vbus_det, id_notify = 0, vbus_notify = 0;
+	bool id_notify = false, vbus_notify = false;
+	int id_det, vbus_det;
 
 	if (phy0 == NULL)
 		return;
@@ -474,13 +475,13 @@ static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
 		}
 		sun4i_usb_phy0_set_id_detect(phy0, id_det);
 		data->id_det = id_det;
-		id_notify = 1;
+		id_notify = true;
 	}
 
 	if (vbus_det != data->vbus_det) {
 		sun4i_usb_phy0_set_vbus_detect(phy0, vbus_det);
 		data->vbus_det = vbus_det;
-		vbus_notify = 1;
+		vbus_notify = true;
 	}
 
 	mutex_unlock(&phy0->mutex);
-- 
2.7.4

[PATCH v2 2/7] phy-sun4i-usb: Refactor forced session ending

[Hans de Goede]

The phy-sun4i-usb code supports forced ending a session on systems
which lack Vbus detection, to allow switching between host and peripheral
mode on such systems.

Role switching via the musb driver "mode" sysfs attribute requires force
ending the session too. This commit refactors the code to allow other
parts of the phy-sun4i-usb code to request a forced session end.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/phy/phy-sun4i-usb.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index 1cb84a8..02cb65e 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -133,6 +133,7 @@ struct sun4i_usb_phy_data {
 	struct power_supply *vbus_power_supply;
 	struct notifier_block vbus_power_nb;
 	bool vbus_power_nb_registered;
+	bool force_session_end;
 	int id_det_irq;
 	int vbus_det_irq;
 	int id_det;
@@ -445,7 +446,7 @@ static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
 	struct sun4i_usb_phy_data *data =
 		container_of(work, struct sun4i_usb_phy_data, detect.work);
 	struct phy *phy0 = data->phys[0].phy;
-	bool id_notify = false, vbus_notify = false;
+	bool force_session_end, id_notify = false, vbus_notify = false;
 	int id_det, vbus_det;
 
 	if (phy0 == NULL)
@@ -461,14 +462,17 @@ static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
 		return;
 	}
 
+	force_session_end = data->force_session_end;
+	data->force_session_end = false;
+
 	if (id_det != data->id_det) {
-		/*
-		 * When a host cable (id == 0) gets plugged in on systems
-		 * without vbus detection report vbus low for long enough for
-		 * the musb-ip to end the current device session.
-		 */
+		/* id-change, force session end if we've no vbus detection */
 		if (data->dr_mode == USB_DR_MODE_OTG &&
-		    !sun4i_usb_phy0_have_vbus_det(data) && id_det == 0) {
+		    !sun4i_usb_phy0_have_vbus_det(data))
+			force_session_end = true;
+
+		/* When entering host mode (id = 0) force end the session now */
+		if (force_session_end && id_det == 0) {
 			sun4i_usb_phy0_set_vbus_detect(phy0, 0);
 			msleep(200);
 			sun4i_usb_phy0_set_vbus_detect(phy0, 1);
@@ -489,13 +493,8 @@ static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
 	if (id_notify) {
 		extcon_set_cable_state_(data->extcon, EXTCON_USB_HOST,
 					!id_det);
-		/*
-		 * When a host cable gets unplugged (id == 1) on systems
-		 * without vbus detection report vbus low for long enough to
-		 * the musb-ip to end the current host session.
-		 */
-		if (data->dr_mode == USB_DR_MODE_OTG &&
-		    !sun4i_usb_phy0_have_vbus_det(data) && id_det == 1) {
+		/* When leaving host mode force end the session here */
+		if (force_session_end && id_det == 1) {
 			mutex_lock(&phy0->mutex);
 			sun4i_usb_phy0_set_vbus_detect(phy0, 0);
 			msleep(1000);
-- 
2.7.4

[PATCH v2 3/7] phy-sun4i-usb: Simplify missing dr_mode handling

[Hans de Goede]

If we cannot get dr_mode or no id gpio is specified simply assume
peripheral mode, as this is always safe.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Keep sun4i_usb_phy0_get_id_det() logic as is rather then re-ordering
 the switch-case
---
 drivers/phy/phy-sun4i-usb.c | 42 ++++++++++++++----------------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index 02cb65e..fb2d4f3 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -124,7 +124,6 @@ struct sun4i_usb_phy_data {
 		bool regulator_on;
 		int index;
 	} phys[MAX_PHYS];
-	int first_phy;
 	/* phy0 / otg related variables */
 	struct extcon_dev *extcon;
 	bool phy0_init;
@@ -326,7 +325,10 @@ static int sun4i_usb_phy0_get_id_det(struct sun4i_usb_phy_data *data)
 {
 	switch (data->dr_mode) {
 	case USB_DR_MODE_OTG:
-		return gpiod_get_value_cansleep(data->id_det_gpio);
+		if (data->id_det_gpio)
+			return gpiod_get_value_cansleep(data->id_det_gpio);
+		else
+			return 1; /* Fallback to peripheral mode */
 	case USB_DR_MODE_HOST:
 		return 0;
 	case USB_DR_MODE_PERIPHERAL:
@@ -539,8 +541,7 @@ static struct phy *sun4i_usb_phy_xlate(struct device *dev,
 {
 	struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
 
-	if (args->args[0] < data->first_phy ||
-	    args->args[0] >= data->cfg->num_phys)
+	if (args->args[0] >= data->cfg->num_phys)
 		return ERR_PTR(-ENODEV);
 
 	return data->phys[args->args[0]].phy;
@@ -615,33 +616,18 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
 	}
 
 	data->dr_mode = of_usb_get_dr_mode_by_phy(np, 0);
-	switch (data->dr_mode) {
-	case USB_DR_MODE_OTG:
-		/* otg without id_det makes no sense, and is not supported */
-		if (!data->id_det_gpio) {
-			dev_err(dev, "usb0_id_det missing or invalid\n");
-			return -ENODEV;
-		}
-		/* fall through */
-	case USB_DR_MODE_HOST:
-	case USB_DR_MODE_PERIPHERAL:
-		data->extcon = devm_extcon_dev_allocate(dev,
-							sun4i_usb_phy0_cable);
-		if (IS_ERR(data->extcon))
-			return PTR_ERR(data->extcon);
 
-		ret = devm_extcon_dev_register(dev, data->extcon);
-		if (ret) {
-			dev_err(dev, "failed to register extcon: %d\n", ret);
-			return ret;
-		}
-		break;
-	default:
-		dev_info(dev, "dr_mode unknown, not registering usb phy0\n");
-		data->first_phy = 1;
+	data->extcon = devm_extcon_dev_allocate(dev, sun4i_usb_phy0_cable);
+	if (IS_ERR(data->extcon))
+		return PTR_ERR(data->extcon);
+
+	ret = devm_extcon_dev_register(dev, data->extcon);
+	if (ret) {
+		dev_err(dev, "failed to register extcon: %d\n", ret);
+		return ret;
 	}
 
-	for (i = data->first_phy; i < data->cfg->num_phys; i++) {
+	for (i = 0; i < data->cfg->num_phys; i++) {
 		struct sun4i_usb_phy *phy = data->phys + i;
 		char name[16];
 
-- 
2.7.4

[PATCH v2 4/7] phy-sun4i-usb: Add support for phy_set_mode

[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 <hdegoede@redhat.com>
---
 drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index fb2d4f3..d369081 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -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);
@@ -440,6 +463,7 @@ static const struct phy_ops sun4i_usb_phy_ops = {
 	.exit		= sun4i_usb_phy_exit,
 	.power_on	= sun4i_usb_phy_power_on,
 	.power_off	= sun4i_usb_phy_power_off,
+	.set_mode	= sun4i_usb_phy_set_mode,
 	.owner		= THIS_MODULE,
 };
 
-- 
2.7.4

[PATCH v2 5/7] phy-sun4i-usb: Warn when external vbus is detected

[Hans de Goede]

Warn when external vbus is detected when we're trying to enable our
own vbus.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/phy/phy-sun4i-usb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index d369081..c17b099 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -390,8 +390,10 @@ static int sun4i_usb_phy_power_on(struct phy *_phy)
 
 	/* For phy0 only turn on Vbus if we don't have an ext. Vbus */
 	if (phy->index == 0 && sun4i_usb_phy0_have_vbus_det(data) &&
-				data->vbus_det)
+				data->vbus_det) {
+		dev_warn(&_phy->dev, "External vbus detected, not enabling our own vbus\n");
 		return 0;
+	}
 
 	ret = regulator_enable(phy->vbus);
 	if (ret)
-- 
2.7.4

[PATCH v2 6/7] phy-sun4i-usb: Add "allwinner, usb0-usb-a-connector" dt property

[Hans de Goede]

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 <hdegoede@redhat.com>
---
Changes in v2:
-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(-)

diff --git a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
index 287150d..8646b53 100644
--- a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
+++ b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
@@ -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 {
diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index c17b099..82fb46a 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -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))
-- 
2.7.4

[PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode

[Hans de Goede]

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
index c6ee166..1fe7451 100644
--- a/drivers/usb/musb/sunxi.c
+++ b/drivers/usb/musb/sunxi.c
@@ -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)
 {
 }
 
+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;
+	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,
+	 * 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);
+
+	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

[PATCH v2 4/7] phy-sun4i-usb: Add support for phy_set_mode

[Sergei Shtylyov]

Hello.

On 08/15/2016 10:21 PM, Hans de Goede wrote:

> 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 <hdegoede@redhat.com>
> ---
>  drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
> index fb2d4f3..d369081 100644
> --- a/drivers/phy/phy-sun4i-usb.c
> +++ b/drivers/phy/phy-sun4i-usb.c
> @@ -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;

total: 3 errors, 0 warnings, 36 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or --fix-inplace.

/home/headless/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch has 
style problems, please review.

NOTE: If any of the errors are false positives, please report
       them to the maintainer, see CHECKPATCH in MAINTAINERS.

MBR, Sergei

[PATCH v2 4/7] phy-sun4i-usb: Add support for phy_set_mode

[Hans de Goede]

Hi,

On 08/16/2016 03:48 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 08/15/2016 10:21 PM, Hans de Goede wrote:
>
>> 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 <hdegoede@redhat.com>
>> ---
>>  drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
>> index fb2d4f3..d369081 100644
>> --- a/drivers/phy/phy-sun4i-usb.c
>> +++ b/drivers/phy/phy-sun4i-usb.c
>> @@ -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;

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

Regaards,

Hans

[PATCH v2 4/7] phy-sun4i-usb: Add support for phy_set_mode

[Felipe Balbi]

Hi,

Hans de Goede <hdegoede@redhat.com> writes:
> Hi,
>
> On 08/16/2016 03:48 PM, Sergei Shtylyov wrote:
>> Hello.
>>
>> On 08/15/2016 10:21 PM, Hans de Goede wrote:
>>
>>> 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 <hdegoede@redhat.com>
>>> ---
>>>  drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
>>> index fb2d4f3..d369081 100644
>>> --- a/drivers/phy/phy-sun4i-usb.c
>>> +++ b/drivers/phy/phy-sun4i-usb.c
>>> @@ -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;
>
> 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. In fact,
CodingStyle states clearly that you shouldn't put multiple statements in
one line.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160818/3926c05a/attachment.sig>

[PATCH v2 4/7] phy-sun4i-usb: Add support for phy_set_mode

[Hans de Goede]

HI,

On 18-08-16 09:40, Felipe Balbi wrote:
>
> Hi,
>
> Hans de Goede <hdegoede@redhat.com> writes:
>> Hi,
>>
>> On 08/16/2016 03:48 PM, Sergei Shtylyov wrote:
>>> Hello.
>>>
>>> On 08/15/2016 10:21 PM, Hans de Goede wrote:
>>>
>>>> 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 <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++
>>>>  1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
>>>> index fb2d4f3..d369081 100644
>>>> --- a/drivers/phy/phy-sun4i-usb.c
>>>> +++ b/drivers/phy/phy-sun4i-usb.c
>>>> @@ -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;
>>
>> 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.

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

Regards,

Hans

[PATCH v2 4/7] phy-sun4i-usb: Add support for phy_set_mode

[Felipe Balbi]

Hi,

Hans de Goede <hdegoede@redhat.com> writes:

[...]

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

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.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160818/83f1464c/attachment.sig>

[PATCH v2 4/7] phy-sun4i-usb: Add support for phy_set_mode

[Kishon Vijay Abraham I]

Hi,

On Thursday 18 August 2016 03:47 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Hans de Goede <hdegoede@redhat.com> writes:
> 
> [...]
> 
>>>>>>  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;
>>>>
>>>> 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.
>>
>>> 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.
> 
> 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.

hmm.. I'd prefer without checkpatch errors or warnings.

Thanks
Kishon

[PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs

[Bin Liu]

Hi,

On Mon, Aug 15, 2016 at 09:21:25PM +0200, Hans de Goede wrote:
> 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.

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

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.

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

> 
> Please review (and if no issues are found merge).
> 
> Thanks & Regards,
> 
> Hans

Regards,
-Bin.

[PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode

[Bin Liu]

Hi,

On Mon, Aug 15, 2016 at 09:21:32PM +0200, Hans de Goede wrote:
> This allows run-time dr_mode switching support via the "mode" musb
> sysfs attribute.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
> index c6ee166..1fe7451 100644
> --- a/drivers/usb/musb/sunxi.c
> +++ b/drivers/usb/musb/sunxi.c
> @@ -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)
>  {
>  }
>  
> +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.

> +	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,
> +	 * 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.

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

[PATCH v2 6/7] phy-sun4i-usb: Add "allwinner,usb0-usb-a-connector" dt property

[Bin Liu]

Hi,

On Mon, Aug 15, 2016 at 09:21:31PM +0200, Hans de Goede wrote:
> 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.

As I commented in patch 0/7, this dt prop is unneccesary.

Regards,
-Bin.

> 
> *) 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 <hdegoede@redhat.com>
> ---
> Changes in v2:
> -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(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
> index 287150d..8646b53 100644
> --- a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
> @@ -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 {
> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
> index c17b099..82fb46a 100644
> --- a/drivers/phy/phy-sun4i-usb.c
> +++ b/drivers/phy/phy-sun4i-usb.c
> @@ -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))
> -- 
> 2.7.4
> 

[PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs

[Hans de Goede]

Hi,

On 19-08-16 23:25, Bin Liu wrote:
> Hi,
>
> On Mon, Aug 15, 2016 at 09:21:25PM +0200, Hans de Goede wrote:
>> 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.
>
> 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.

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

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

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

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

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

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

Regards,

Hans

[PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode

[Hans de Goede]

Hi,

On 19-08-16 23:30, Bin Liu wrote:
> Hi,
>
> On Mon, Aug 15, 2016 at 09:21:32PM +0200, Hans de Goede wrote:
>> This allows run-time dr_mode switching support via the "mode" musb
>> sysfs attribute.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 48 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
>> index c6ee166..1fe7451 100644
>> --- a/drivers/usb/musb/sunxi.c
>> +++ b/drivers/usb/musb/sunxi.c
>> @@ -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)
>>  {
>>  }
>>
>> +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.

>
>> +	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,
>> +	 * 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


>
> Regards,
> -Bin.
>
>> +
>> +	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
>>

[PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs

[Bin Liu]

Hi,

On Sun, Aug 21, 2016 at 11:29:02AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 19-08-16 23:25, Bin Liu wrote:
> >Hi,
> >
> >On Mon, Aug 15, 2016 at 09:21:25PM +0200, Hans de Goede wrote:
> >>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.
> >
> >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.

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

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

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

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

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

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

> 
> As said before reality trumps the Spec / theory.
> 
> >>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.
> 
> 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;

> 
> Regards,
> 
> Hans

Regards,
-Bin.

[PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode

[Bin Liu]

Hi,

On Sun, Aug 21, 2016 at 12:10:26PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 19-08-16 23:30, Bin Liu wrote:
> >Hi,
> >
> >On Mon, Aug 15, 2016 at 09:21:32PM +0200, Hans de Goede wrote:
> >>This allows run-time dr_mode switching support via the "mode" musb
> >>sysfs attribute.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >> drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
> >> 1 file changed, 48 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
> >>index c6ee166..1fe7451 100644
> >>--- a/drivers/usb/musb/sunxi.c
> >>+++ b/drivers/usb/musb/sunxi.c
> >>@@ -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)
> >> {
> >> }
> >>
> >>+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.
> 
> >
> >>+	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,
> >>+	 * 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.

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.

Regards,
-Bin.

> 
> Regards,
> 
> Hans
> 
> 
> >
> >Regards,
> >-Bin.
> >
> >>+
> >>+	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
> >>
> --
> 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

[PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs

[Bin Liu]

On Mon, Aug 22, 2016 at 09:08:29AM -0500, Bin Liu wrote:
> Hi,
> 
> On Sun, Aug 21, 2016 at 11:29:02AM +0200, Hans de Goede wrote:
> > Hi,
> > 
> > On 19-08-16 23:25, Bin Liu wrote:
> > >Hi,
> > >
> > >On Mon, Aug 15, 2016 at 09:21:25PM +0200, Hans de Goede wrote:
> > >>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.
> > >
> > >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.
> 
> > 
> > >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.
> 
> > 
> > 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.
> 
> > 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.
> 
> > 
> > >>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,
> > >
> > >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.
> 
> > 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

s/should be/shouldn't be/

> place), so this new property is unnecessary.
> 
> > 
> > >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.
> > 
> > >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.
> 
> > 
> > As said before reality trumps the Spec / theory.
> > 
> > >>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.
> > 
> > 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;
> 
> > 
> > Regards,
> > 
> > Hans
> 
> Regards,
> -Bin.
> 

[PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode

[Hans de Goede]

Hi,

On 22-08-16 16:11, Bin Liu wrote:
> Hi,
>
> On Sun, Aug 21, 2016 at 12:10:26PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 19-08-16 23:30, Bin Liu wrote:
>>> Hi,
>>>
>>> On Mon, Aug 15, 2016 at 09:21:32PM +0200, Hans de Goede wrote:
>>>> This allows run-time dr_mode switching support via the "mode" musb
>>>> sysfs attribute.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
>>>> 1 file changed, 48 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
>>>> index c6ee166..1fe7451 100644
>>>> --- a/drivers/usb/musb/sunxi.c
>>>> +++ b/drivers/usb/musb/sunxi.c
>>>> @@ -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)
>>>> {
>>>> }
>>>>
>>>> +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.
>>
>>>
>>>> +	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,
>>>> +	 * 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.
>
> 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

[PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode

[Bin Liu]

On Mon, Aug 22, 2016 at 05:08:56PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 22-08-16 16:11, Bin Liu wrote:
> >Hi,
> >
> >On Sun, Aug 21, 2016 at 12:10:26PM +0200, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 19-08-16 23:30, Bin Liu wrote:
> >>>Hi,
> >>>
> >>>On Mon, Aug 15, 2016 at 09:21:32PM +0200, Hans de Goede wrote:
> >>>>This allows run-time dr_mode switching support via the "mode" musb
> >>>>sysfs attribute.
> >>>>
> >>>>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>---
> >>>>drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
> >>>>1 file changed, 48 insertions(+), 4 deletions(-)
> >>>>
> >>>>diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
> >>>>index c6ee166..1fe7451 100644
> >>>>--- a/drivers/usb/musb/sunxi.c
> >>>>+++ b/drivers/usb/musb/sunxi.c
> >>>>@@ -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)
> >>>>{
> >>>>}
> >>>>
> >>>>+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.
> >>
> >>>
> >>>>+	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,
> >>>>+	 * 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.
> >
> >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.

Regards,
-Bin.

> 
> Regards,
> 
> Hans

[PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode

[Hans de Goede]

HI,

On 22-08-16 17:24, Bin Liu wrote:
> On Mon, Aug 22, 2016 at 05:08:56PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 22-08-16 16:11, Bin Liu wrote:
>>> Hi,
>>>
>>> On Sun, Aug 21, 2016 at 12:10:26PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 19-08-16 23:30, Bin Liu wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Aug 15, 2016 at 09:21:32PM +0200, Hans de Goede wrote:
>>>>>> This allows run-time dr_mode switching support via the "mode" musb
>>>>>> sysfs attribute.
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>> drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
>>>>>> 1 file changed, 48 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
>>>>>> index c6ee166..1fe7451 100644
>>>>>> --- a/drivers/usb/musb/sunxi.c
>>>>>> +++ b/drivers/usb/musb/sunxi.c
>>>>>> @@ -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)
>>>>>> {
>>>>>> }
>>>>>>
>>>>>> +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.
>>>>
>>>>>
>>>>>> +	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,
>>>>>> +	 * 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.
>>>
>>> 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.

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

[PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode

[Bin Liu]

On Mon, Aug 22, 2016 at 05:32:57PM +0200, Hans de Goede wrote:
> HI,
> 
> On 22-08-16 17:24, Bin Liu wrote:
> >On Mon, Aug 22, 2016 at 05:08:56PM +0200, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 22-08-16 16:11, Bin Liu wrote:
> >>>Hi,
> >>>
> >>>On Sun, Aug 21, 2016 at 12:10:26PM +0200, Hans de Goede wrote:
> >>>>Hi,
> >>>>
> >>>>On 19-08-16 23:30, Bin Liu wrote:
> >>>>>Hi,
> >>>>>
> >>>>>On Mon, Aug 15, 2016 at 09:21:32PM +0200, Hans de Goede wrote:
> >>>>>>This allows run-time dr_mode switching support via the "mode" musb
> >>>>>>sysfs attribute.
> >>>>>>
> >>>>>>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>>>---
> >>>>>>drivers/usb/musb/sunxi.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
> >>>>>>1 file changed, 48 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>>diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
> >>>>>>index c6ee166..1fe7451 100644
> >>>>>>--- a/drivers/usb/musb/sunxi.c
> >>>>>>+++ b/drivers/usb/musb/sunxi.c
> >>>>>>@@ -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)
> >>>>>>{
> >>>>>>}
> >>>>>>
> >>>>>>+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.
> >>>>
> >>>>>
> >>>>>>+	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,
> >>>>>>+	 * 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.
> >>>
> >>>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.
> 
> 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.

> 
> Regards,
> 
> Hans

[PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs

[Hans de Goede]

Hi,

On 22-08-16 16:08, Bin Liu wrote:
> Hi,
>
> On Sun, Aug 21, 2016 at 11:29:02AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 19-08-16 23:25, Bin Liu wrote:
>>> Hi,
>>>
>>> On Mon, Aug 15, 2016 at 09:21:25PM +0200, Hans de Goede wrote:
>>>> 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.
>>>
>>> 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.

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.

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

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

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

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

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

>> As said before reality trumps the Spec / theory.
>>
>>>> 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.
>>
>> 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;

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

[PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode

[Hans de Goede]

Hi,

On 22-08-16 17:38, Bin Liu wrote:
> On Mon, Aug 22, 2016 at 05:32:57PM +0200, Hans de Goede wrote:

<snip>

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

[PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs

[Bin Liu]

Hi,

On Mon, Aug 22, 2016 at 05:50:25PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 22-08-16 16:08, Bin Liu wrote:
> >Hi,
> >
> >On Sun, Aug 21, 2016 at 11:29:02AM +0200, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 19-08-16 23:25, Bin Liu wrote:
> >>>Hi,
> >>>
> >>>On Mon, Aug 15, 2016 at 09:21:25PM +0200, Hans de Goede wrote:
> >>>>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.
> >>>
> >>>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.
> 
> 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.
> 
> >>>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.
> >
> >>
> >>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.
> 
> >>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.
> 
> >>>>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,
> >>>
> >>>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.
> >
> >>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.
> >
> >>
> >>>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.
> >>
> >>>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?
> 
> One example of such a device is the quite popular mk802 tv stick
> (the original version) with A10s SoC.
> 
> > 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.
> 
> >>As said before reality trumps the Spec / theory.
> >>
> >>>>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.
> >>
> >>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;
> 
> 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.

Well, I will stop arguing on this from here. It is up to the DT
maintainer to decide on this DT prop.

> 
> Regards,
> 
> Hans

Regards,
-Bin.

[PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode

[Bin Liu]

On Mon, Aug 22, 2016 at 05:55:50PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 22-08-16 17:38, Bin Liu wrote:
> >On Mon, Aug 22, 2016 at 05:32:57PM +0200, Hans de Goede wrote:
> 
> <snip>
> 
> >>>>>>>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.
> >>>>
> >>>>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.
> >
> >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.

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

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

[PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs

[Rask Ingemann Lambertsen]

On Fri, Aug 19, 2016 at 04:25:40PM -0500, Bin Liu wrote:
> Hi,
> 
> On Mon, Aug 15, 2016 at 09:21:25PM +0200, Hans de Goede wrote:
> > 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.
> 
> Well, this is an illegal usecase. A-A cable is invalid by USB Spec.

TV boxes with the Sunchip CX-A99 board inside are an example of such
devices and they ship with a cable that has a type-A plug at both ends,
precisely to allow you to connect the board to a USB host. It is standard
practice to do so with that board [1][2] and I've had mine connected that
way for the last few months while working on mainline kernel support.

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

The Openmoko Neo 1973 and Neo Freerunner have just a Mini-B connector
but the s3c24xx USB port is switchable between host mode and peripheral
mode.[3] Should that also be disallowed then?

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

Openmoko kernels have since November 2008 had the funtionality to switch
roles of the USB port using a sysfs attribute. Can you be more specific
about the "ton of problems"? I have one of those devices as well as a
CX-A99, so I'd like to know what I've overlooked.

[1] http://forum.tronsmart.com/forum/tronsmart-draco-aw80/211-instructions-for-firmware-updating-via-pc#642
[2] https://linux-sunxi.org/FEL
[3] http://wiki.openmoko.org/wiki/Specialized_USB_cables
-- 
Rask Ingemann Lambertsen

[PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode

[Hans de Goede]

Hi,

On 22-08-16 18:10, Bin Liu wrote:
> On Mon, Aug 22, 2016 at 05:55:50PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 22-08-16 17:38, Bin Liu wrote:
>>> On Mon, Aug 22, 2016 at 05:32:57PM +0200, Hans de Goede wrote:
>>
>> <snip>
>>
>>>>>>>>> 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.
>>>>>>
>>>>>> 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.
>>>
>>> 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.
>
>>
>> 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.

Ah right, I ran some tests and indeed the hcd does not see a disconnect
on role-change unless musb_root_disconnect() gets called, I'll send a v4
fixing this.

Thank you for catching this.

Regards,

Hans


>
> Regards,
> -Bin.
>
>>
>> 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