USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

PATCH Superseded Hans de Goede 3 participants 12 messages Jun 2, 2016 – Jun 3, 2016

Subsystems (4)

USB Arm/Allwinner sunXi SoC support Generic PHY framework MUSB multipoint high speed dual-role controller

lore

Share & tools

lore

Hans de Goede

author

2016-06-02 17:31 UTC #0 lore raw

PATCH 1/4 USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

Some SoCs have a single phy-hw-block with multiple phys, this is modelled by a single phy dts node, so we end up with multiple controller nodes with a phys property pointing to the phy-node of the otg-phy.

Only one of these controllers typically is an otg controller, yet we were checking the first controller who uses a phy from the block and then end up looking for a dr_mode property in e.g. the ehci controller.

Instead of looking for nodes with a phy property, look for nodes with a dr_mode property, so that we actually access the dr_mode property in a node which has it.

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

---
 drivers/usb/common/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/common/common.c +1 -1

index e3d0161..9806433 100644
@@ -145,7 +145,7 @@ enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
 	int err;
 
 	do {
-		controller = of_find_node_with_property(controller, "phys");
+		controller = of_find_node_with_property(controller, "dr_mode");
 		index = 0;
 		do {
 			phy = of_parse_phandle(controller, "phys", index);

Signature · 1 line

2.7.4

Hans de Goede

author

2016-06-02 17:31 UTC #1 lore raw

PATCH 2/4 phy-sun4i-usb: Add support for peripheral-only mode

Use the new of_usb_get_dr_mode_by_phy() function to get the dr_mode from the musb controller node instead of assuming that having an id_det gpio means otg mode, and not having one means host mode.

Implement peripheral-only mode by adding a sun4i_usb_phy0_get_id_det helper which looks at the dr_mode, always registering our extcon and always monitoring vbus.

If dr_mode is not specified in the dts, do not register phy0 as we then do not know how to treat it. This is actually a good thing as this means we will not be registering phy0 on devices where the otg controller is not enabled in the devicetree.

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

---
 drivers/phy/phy-sun4i-usb.c | 62 ++++++++++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 20 deletions(-)

drivers/phy/phy-sun4i-usb.c +42 ~~-20~~

index bae54f7..1ea71f5 100644
@@ -40,6 +40,7 @@
 #include <linux/power_supply.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
+#include <linux/usb/of.h>
 #include <linux/workqueue.h>
 
 #define REG_ISCR			0x00

@@ -109,6 +110,7 @@ struct sun4i_usb_phy_cfg {
 struct sun4i_usb_phy_data {
 	void __iomem *base;
 	const struct sun4i_usb_phy_cfg *cfg;
+	enum usb_dr_mode dr_mode;
 	struct mutex mutex;
 	struct sun4i_usb_phy {
 		struct phy *phy;

@@ -119,6 +121,7 @@ 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;

@@ -285,16 +288,10 @@ static int sun4i_usb_phy_init(struct phy *_phy)
 		sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_DPDM_PULLUP_EN);
 		sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_ID_PULLUP_EN);
 
-		if (data->id_det_gpio) {
-			/* OTG mode, force ISCR and cable state updates */
-			data->id_det = -1;
-			data->vbus_det = -1;
-			queue_delayed_work(system_wq, &data->detect, 0);
-		} else {
-			/* Host only mode */
-			sun4i_usb_phy0_set_id_detect(_phy, 0);
-			sun4i_usb_phy0_set_vbus_detect(_phy, 1);
-		}
+		/* Force ISCR and cable state updates */
+		data->id_det = -1;
+		data->vbus_det = -1;
+		queue_delayed_work(system_wq, &data->detect, 0);
 	}
 
 	return 0;

@@ -319,6 +316,19 @@ static int sun4i_usb_phy_exit(struct phy *_phy)
 	return 0;
 }
 
+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);
+	case USB_DR_MODE_HOST:
+		return 0;
+	case USB_DR_MODE_PERIPHERAL:
+	default:
+		return 1;
+	}
+}
+
 static int sun4i_usb_phy0_get_vbus_det(struct sun4i_usb_phy_data *data)
 {
 	if (data->vbus_det_gpio)

@@ -414,7 +424,10 @@ static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
 	struct phy *phy0 = data->phys[0].phy;
 	int id_det, vbus_det, id_notify = 0, vbus_notify = 0;
 
-	id_det = gpiod_get_value_cansleep(data->id_det_gpio);
+	if (phy0 == NULL)
+		return;
+
+	id_det = sun4i_usb_phy0_get_id_det(data);
 	vbus_det = sun4i_usb_phy0_get_vbus_det(data);
 
 	mutex_lock(&phy0->mutex);

@@ -501,7 +514,8 @@ 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->cfg->num_phys)
+	if (args->args[0] < data->first_phy ||
+	    args->args[0] >= data->cfg->num_phys)
 		return ERR_PTR(-ENODEV);
 
 	return data->phys[args->args[0]].phy;

@@ -575,13 +589,17 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
 			return -EPROBE_DEFER;
 	}
 
-	/* vbus_det without id_det makes no sense, and is not supported */
-	if (sun4i_usb_phy0_have_vbus_det(data) && !data->id_det_gpio) {
-		dev_err(dev, "usb0_id_det missing or invalid\n");
-		return -ENODEV;
-	}
-
-	if (data->id_det_gpio) {
+	data->dr_mode = of_usb_get_dr_mode_by_phy(np);
+	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))

@@ -592,9 +610,13 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
 			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;
 	}
 
-	for (i = 0; i < data->cfg->num_phys; i++) {
+	for (i = data->first_phy; i < data->cfg->num_phys; i++) {
 		struct sun4i_usb_phy *phy = data->phys + i;
 		char name[16];

Signature · 1 line

2.7.4

Hans de Goede

author

2016-06-02 17:31 UTC #2 lore raw

PATCH 3/4 phy-sun4i-usb: Add workaround for missing Vbus det interrupts on A31

In reply to Hans de Goede

The A31 companion pmic (axp221) does not generate vbus change interrupts when the board is driving vbus, so we must poll when using the pmic for vbus-det _and_ we're driving vbus.

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

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

drivers/phy/phy-sun4i-usb.c +24 ~~-10~~

index 1ea71f5..2247cf2 100644
@@ -95,6 +95,7 @@
 
 enum sun4i_usb_phy_type {
 	sun4i_a10_phy,
+	sun6i_a31_phy,
 	sun8i_a33_phy,
 	sun8i_h3_phy,
 };

@@ -125,7 +126,6 @@ struct sun4i_usb_phy_data {
 	/* phy0 / otg related variables */
 	struct extcon_dev *extcon;
 	bool phy0_init;
-	bool phy0_poll;
 	struct gpio_desc *id_det_gpio;
 	struct gpio_desc *vbus_det_gpio;
 	struct power_supply *vbus_power_supply;

@@ -353,6 +353,24 @@ static bool sun4i_usb_phy0_have_vbus_det(struct sun4i_usb_phy_data *data)
 	return data->vbus_det_gpio || data->vbus_power_supply;
 }
 
+static bool sun4i_usb_phy0_poll(struct sun4i_usb_phy_data *data)
+{
+	if ((data->id_det_gpio && data->id_det_irq < 0) ||
+	    (data->vbus_det_gpio && data->vbus_det_irq < 0))
+		return true;
+
+	/*
+	 * The A31 companion pmic (axp221) does not generate vbus change
+	 * interrupts when the board is driving vbus, so we must poll
+	 * when using the pmic for vbus-det _and_ we're driving vbus.
+	 */
+	if (data->cfg->type == sun6i_a31_phy &&
+	    data->vbus_power_supply && data->phys[0].regulator_on)
+		return true;
+
+	return false;
+}
+
 static int sun4i_usb_phy_power_on(struct phy *_phy)
 {
 	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

@@ -374,7 +392,7 @@ static int sun4i_usb_phy_power_on(struct phy *_phy)
 	phy->regulator_on = true;
 
 	/* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */
-	if (phy->index == 0 && data->vbus_det_gpio && data->phy0_poll)
+	if (phy->index == 0 && sun4i_usb_phy0_poll(data))
 		mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
 
 	return 0;

@@ -395,7 +413,7 @@ static int sun4i_usb_phy_power_off(struct phy *_phy)
 	 * phy0 vbus typically slowly discharges, sometimes this causes the
 	 * Vbus gpio to not trigger an edge irq on Vbus off, so force a rescan.
 	 */
-	if (phy->index == 0 && data->vbus_det_gpio && !data->phy0_poll)
+	if (phy->index == 0 && !sun4i_usb_phy0_poll(data))
 		mod_delayed_work(system_wq, &data->detect, POLL_TIME);
 
 	return 0;

@@ -481,7 +499,7 @@ static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
 	if (vbus_notify)
 		extcon_set_cable_state_(data->extcon, EXTCON_USB, vbus_det);
 
-	if (data->phy0_poll)
+	if (sun4i_usb_phy0_poll(data))
 		queue_delayed_work(system_wq, &data->detect, POLL_TIME);
 }

@@ -666,11 +684,6 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
 	}
 
 	data->id_det_irq = gpiod_to_irq(data->id_det_gpio);
-	data->vbus_det_irq = gpiod_to_irq(data->vbus_det_gpio);
-	if ((data->id_det_gpio && data->id_det_irq < 0) ||
-	    (data->vbus_det_gpio && data->vbus_det_irq < 0))
-		data->phy0_poll = true;
-
 	if (data->id_det_irq >= 0) {
 		ret = devm_request_irq(dev, data->id_det_irq,
 				sun4i_usb_phy0_id_vbus_det_irq,

@@ -682,6 +695,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
 		}
 	}
 
+	data->vbus_det_irq = gpiod_to_irq(data->vbus_det_gpio);
 	if (data->vbus_det_irq >= 0) {
 		ret = devm_request_irq(dev, data->vbus_det_irq,
 				sun4i_usb_phy0_id_vbus_det_irq,

@@ -733,7 +747,7 @@ static const struct sun4i_usb_phy_cfg sun5i_a13_cfg = {
 
 static const struct sun4i_usb_phy_cfg sun6i_a31_cfg = {
 	.num_phys = 3,
-	.type = sun4i_a10_phy,
+	.type = sun6i_a31_phy,
 	.disc_thresh = 3,
 	.phyctl_offset = REG_PHYCTL_A10,
 	.dedicated_clocks = true,

Signature · 1 line

2.7.4

Hans de Goede

author

2016-06-02 17:31 UTC #3 lore raw

PATCH 4/4 musb: sunxi: Simplify dr_mode handling

In reply to Hans de Goede

phy-sun4i-usb now has proper dr_mode handling, it always registers an extcon, and sends a notify with the mode (even when in peripheral- / host-only mode) at least once.

So we can simply the sunxi musb glue by always registering its extcon notifier and relying on sunxi_musb_work() to enable vbus when in host-only mode.

This also enables host- and peripheral-only mode with vbus monitoring.

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

---
 drivers/usb/musb/sunxi.c | 68 ++++++++++++++++++------------------------------
 1 file changed, 25 insertions(+), 43 deletions(-)

drivers/usb/musb/sunxi.c +25 ~~-43~~

index e7d4617..b88a2f6 100644
@@ -255,12 +255,10 @@ static int sunxi_musb_init(struct musb *musb)
 	writeb(SUNXI_MUSB_VEND0_PIO_MODE, musb->mregs + SUNXI_MUSB_VEND0);
 
 	/* Register notifier before calling phy_init() */
-	if (musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE) {
-		ret = extcon_register_notifier(glue->extcon, EXTCON_USB_HOST,
-					       &glue->host_nb);
-		if (ret)
-			goto error_reset_assert;
-	}
+	ret = extcon_register_notifier(glue->extcon, EXTCON_USB_HOST,
+				       &glue->host_nb);
+	if (ret)
+		goto error_reset_assert;
 
 	ret = phy_init(glue->phy);
 	if (ret)

@@ -274,9 +272,8 @@ static int sunxi_musb_init(struct musb *musb)
 	return 0;
 
 error_unregister_notifier:
-	if (musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE)
-		extcon_unregister_notifier(glue->extcon, EXTCON_USB_HOST,
-					   &glue->host_nb);
+	extcon_unregister_notifier(glue->extcon, EXTCON_USB_HOST,
+				   &glue->host_nb);
 error_reset_assert:
 	if (test_bit(SUNXI_MUSB_FL_HAS_RESET, &glue->flags))
 		reset_control_assert(glue->rst);

@@ -300,9 +297,8 @@ static int sunxi_musb_exit(struct musb *musb)
 
 	phy_exit(glue->phy);
 
-	if (musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE)
-		extcon_unregister_notifier(glue->extcon, EXTCON_USB_HOST,
-					   &glue->host_nb);
+	extcon_unregister_notifier(glue->extcon, EXTCON_USB_HOST,
+				   &glue->host_nb);
 
 	if (test_bit(SUNXI_MUSB_FL_HAS_RESET, &glue->flags))
 		reset_control_assert(glue->rst);

@@ -314,25 +310,6 @@ static int sunxi_musb_exit(struct musb *musb)
 	return 0;
 }
 
-static int sunxi_set_mode(struct musb *musb, u8 mode)
-{
-	struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
-	int ret;
-
-	if (mode == MUSB_HOST) {
-		ret = phy_power_on(glue->phy);
-		if (ret)
-			return ret;
-
-		set_bit(SUNXI_MUSB_FL_PHY_ON, &glue->flags);
-		/* Stop musb work from turning vbus off again */
-		set_bit(SUNXI_MUSB_FL_VBUS_ON, &glue->flags);
-		musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE;
-	}
-
-	return 0;
-}
-
 static void sunxi_musb_enable(struct musb *musb)
 {
 	struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);

@@ -579,7 +556,6 @@ static const struct musb_platform_ops sunxi_musb_ops = {
 	.exit		= sunxi_musb_exit,
 	.enable		= sunxi_musb_enable,
 	.disable	= sunxi_musb_disable,
-	.set_mode	= sunxi_set_mode,
 	.fifo_offset	= sunxi_musb_fifo_offset,
 	.ep_offset	= sunxi_musb_ep_offset,
 	.busctl_offset	= sunxi_musb_busctl_offset,

@@ -635,10 +611,6 @@ 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

@@ -646,15 +618,13 @@ static int sunxi_musb_probe(struct platform_device *pdev)
 		pdata.mode = MUSB_PORT_MODE_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;
+		break;
+#endif
 #ifdef CONFIG_USB_MUSB_DUAL_ROLE
 	case USB_DR_MODE_OTG:
-		glue->extcon = extcon_get_edev_by_phandle(&pdev->dev, 0);
-		if (IS_ERR(glue->extcon)) {
-			if (PTR_ERR(glue->extcon) == -EPROBE_DEFER)
-				return -EPROBE_DEFER;
-			dev_err(&pdev->dev, "Invalid or missing extcon\n");
-			return PTR_ERR(glue->extcon);
-		}
 		pdata.mode = MUSB_PORT_MODE_DUAL_ROLE;
 		break;
 #endif

@@ -665,6 +635,10 @@ 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;

@@ -698,6 +672,14 @@ static int sunxi_musb_probe(struct platform_device *pdev)
 		}
 	}
 
+	glue->extcon = extcon_get_edev_by_phandle(&pdev->dev, 0);
+	if (IS_ERR(glue->extcon)) {
+		if (PTR_ERR(glue->extcon) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		dev_err(&pdev->dev, "Invalid or missing extcon\n");
+		return PTR_ERR(glue->extcon);
+	}
+
 	glue->phy = devm_phy_get(&pdev->dev, "usb");
 	if (IS_ERR(glue->phy)) {
 		if (PTR_ERR(glue->phy) == -EPROBE_DEFER)

Signature · 1 line

2.7.4

Bin Liu

maintainer

2016-06-02 18:16 UTC #4 lore raw

PATCH 1/4 USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

In reply to Hans de Goede

Hi,

Quoted 6 lines · Hans de Goede

Some SoCs have a single phy-hw-block with multiple phys, this is modelled by a single phy dts node, so we end up with multiple controller nodes with a phys property pointing to the phy-node of the otg-phy.
Only one of these controllers typically is an otg controller, yet we

Is it guaranteed that only one of them will be otg?

Quoted 6 lines

were checking the first controller who uses a phy from the block and then end up looking for a dr_mode property in e.g. the ehci controller.
Instead of looking for nodes with a phy property, look for nodes with a dr_mode property, so that we actually access the dr_mode property in a node which has it.

Quote from Documentation/devicetree/bindings/usb/generic.txt:
"- dr_mode: ...
In case this attribute isn't
passed via DT, USB DRD controllers should default to OTG."

So it is not mandatory to define dr_mode in DT, then you wouldn't be able to find the controller in such case.

Regards,
-Bin.

Hans de Goede

author

2016-06-03 10:34 UTC #5 lore raw

PATCH 1/4 USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

Hi,

Quoted 11 lines · Bin Liu

Hi,
Quoted 6 lines · Hans de Goede
Some SoCs have a single phy-hw-block with multiple phys, this is modelled by a single phy dts node, so we end up with multiple controller nodes with a phys property pointing to the phy-node of the otg-phy.
Only one of these controllers typically is an otg controller, yet we
Is it guaranteed that only one of them will be otg?

I guess not, but if there are 2 then with my patch we are of no worse then today, we will then pick the first otg controller. Whereas of_usb_get_dr_mode_by_phy currently is broken even on setups with a shared phy dt-node and only 1 otg controller, which are quite common.

I believe it is unlikely that there will be more then 1 otg controller, so I believe that we should not worry about this until we actually hit this problem.

Quoted 6 lines

Quoted 5 lines
were checking the first controller who uses a phy from the block and>> then end up looking for a dr_mode property in e.g. the ehci controller.
Instead of looking for nodes with a phy property, look for nodes with a dr_mode property, so that we actually access the dr_mode property in a node which has it.

Quoted 8 lines

Quote from Documentation/devicetree/bindings/usb/generic.txt:
"- dr_mode: ...
In case this attribute isn't
passed via DT, USB DRD controllers should default to OTG."
So it is not mandatory to define dr_mode in DT, then you wouldn't be able to find the controller in such case.

If no controller is found then of_usb_get_dr_mode_by_phy will return USB_DR_MODE_UNKNOWN, just like it does today for controller nodes which do not set dr_mode.

So in this case my patch does not change anything.

Regards,

Hans

Bin Liu

maintainer

2016-06-03 13:04 UTC #6 lore raw

PATCH 1/4 USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

Hi,

Quoted 11 lines · Hans de Goede

Hi,
Quoted 8 lines · Bin Liu
Hi,
Quoted 5 lines · Hans de Goede
Some SoCs have a single phy-hw-block with multiple phys, this is modelled by a single phy dts node, so we end up with multiple controller nodes with a phys property pointing to the phy-node of the otg-phy.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Only one of these controllers typically is an otg controller, yet we
Is it guaranteed that only one of them will be otg?
I guess not, but if there are 2 then with my patch we are of no worse then today, we will then pick the first otg controller. Whereas

What if the first otg controller is not what we want? this patch does not solve the problem. I would think Kishon's suggestion in another email - seperate dt phy nodes - is a better option.

Quoted 3 lines

of_usb_get_dr_mode_by_phy currently is broken even on setups with a shared phy dt-node and only 1 otg controller, which are quite common.

If that is the case, the model has to be changed. Otherwise, a single phy driver is unable to handle different operations from multiple controllers.

Regards,
-Bin.

Hans de Goede

author

2016-06-03 13:09 UTC #7 lore raw

PATCH 1/4 USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

Hi,

Quoted 16 lines · Bin Liu

Hi,
Quoted 13 lines · Hans de Goede
Hi,
Quoted 10 lines · Bin Liu
Hi,
Quoted 6 lines · Hans de Goede
Some SoCs have a single phy-hw-block with multiple phys, this is modelled by a single phy dts node, so we end up with multiple controller nodes with a phys property pointing to the phy-node of the otg-phy.
Only one of these controllers typically is an otg controller, yet we

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Is it guaranteed that only one of them will be otg?
I guess not, but if there are 2 then with my patch we are of no worse then today, we will then pick the first otg controller. Whereas
What if the first otg controller is not what we want? this patch does not solve the problem. I would think Kishon's suggestion in another email - seperate dt phy nodes - is a better option.

That is not possible as it will break the dt-bindings for existing devices.

And that adds a lot of complexity without a good reason, as I mentioned in my reply to Kishon if we want to make 100% sure that we've the right controller, we should pass in the phy index argument to the phandle to of_usb_get_dr_mode_by_phy and make of_usb_get_dr_mode_by_phy check this.

I'll submit a v2 which does this.

Regards,

Hans

Kishon Vijay Abraham I

2016-06-03 11:20 UTC #8 lore raw

PATCH 1/4 USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

In reply to Hans de Goede

Hi,

Quoted 4 lines · Hans de Goede

Some SoCs have a single phy-hw-block with multiple phys, this is modelled by a single phy dts node, so we end up with multiple controller nodes with a phys property pointing to the phy-node of the otg-phy.

Maybe we should try to model each phy with a separate dt node?

Thanks
Kishon

Quoted 28 lines

Only one of these controllers typically is an otg controller, yet we were checking the first controller who uses a phy from the block and then end up looking for a dr_mode property in e.g. the ehci controller.
Instead of looking for nodes with a phy property, look for nodes with a dr_mode property, so that we actually access the dr_mode property in a node which has it.
Signed-off-by Hans de Goede <[email protected]>
---
 drivers/usb/common/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/common/common.c +1 -1
index e3d0161..9806433 100644
@@ -145,7 +145,7 @@ enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
 	int err;
 
 	do {
-		controller = of_find_node_with_property(controller, "phys");
+		controller = of_find_node_with_property(controller, "dr_mode");
 		index = 0;
 		do {
 			phy = of_parse_phandle(controller, "phys", index);

Hans de Goede

author

2016-06-03 11:39 UTC #9 lore raw

PATCH 1/4 USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

Hi,

Quoted 9 lines · Kishon Vijay Abraham I

Hi,
Quoted 4 lines · Hans de Goede
Some SoCs have a single phy-hw-block with multiple phys, this is modelled by a single phy dts node, so we end up with multiple controller nodes with a phys property pointing to the phy-node of the otg-phy.
Maybe we should try to model each phy with a separate dt node?

That seems like making things unnecessarily complicated. If we want to be 100% sure that of_usb_get_dr_mode_by_phy finds the right controller, we could add an "int index" parameter to of_usb_get_dr_mode_by_phy and make it check that first argument specified to the phandle used in the controller node matches the passed in index.

And use index == -1 to skip this test.

Regards,

Hans

Quoted 31 lines

Thanks
Kishon
Quoted 28 lines
Only one of these controllers typically is an otg controller, yet we were checking the first controller who uses a phy from the block and then end up looking for a dr_mode property in e.g. the ehci controller.
Instead of looking for nodes with a phy property, look for nodes with a dr_mode property, so that we actually access the dr_mode property in a node which has it.
Signed-off-by Hans de Goede <[email protected]>
---
 drivers/usb/common/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/common/common.c +1 -1
index e3d0161..9806433 100644
@@ -145,7 +145,7 @@ enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
 	int err;
 
 	do {
-		controller = of_find_node_with_property(controller, "phys");
+		controller = of_find_node_with_property(controller, "dr_mode");
 		index = 0;
 		do {
 			phy = of_parse_phandle(controller, "phys", index);

Bin Liu

maintainer

2016-06-03 13:12 UTC #10 lore raw

PATCH 1/4 USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

Hi,

Quoted 11 lines · Hans de Goede

Hi,
Quoted 8 lines · Kishon Vijay Abraham I
Hi,
Quoted 4 lines · Hans de Goede
Some SoCs have a single phy-hw-block with multiple phys, this is modelled by a single phy dts node, so we end up with multiple controller nodes with a phys property pointing to the phy-node of the otg-phy.

Quoted 4 lines

Quoted 1 line
Maybe we should try to model each phy with a separate dt node?
That seems like making things unnecessarily complicated. If we want to be 100% sure that of_usb_get_dr_mode_by_phy finds the right

I believe we have to.

Quoted 3 lines

controller, we could add an "int index" parameter to of_usb_get_dr_mode_by_phy and make it check that first argument specified to the phandle used in the controller node matches the passed in index.

Why we need the 'index'? Once the dt has seperate nodes for the phys, 'phy_np' passed in is the uniqe id of the phy node.

Quoted 6 lines

And use index == -1 to skip this test.
Regards,
Hans

Regards,
-Bin.

Hans de Goede

author

2016-06-03 13:15 UTC #11 lore raw

PATCH 1/4 USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

Hi,

Quoted 14 lines · Bin Liu

Hi,
Quoted 11 lines · Hans de Goede
Hi,
Quoted 8 lines · Kishon Vijay Abraham I
Hi,
Quoted 4 lines · Hans de Goede
Some SoCs have a single phy-hw-block with multiple phys, this is modelled by a single phy dts node, so we end up with multiple controller nodes with a phys property pointing to the phy-node of the otg-phy.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Maybe we should try to model each phy with a separate dt node?
That seems like making things unnecessarily complicated. If we want to be 100% sure that of_usb_get_dr_mode_by_phy finds the right
I believe we have to.

We do not have to, things work fine as is, and the problem we're discussing can be solved much simpler by passing the otg-phy index to of_usb_get_dr_mode_by_phy

Also doing this breaks the dt bindings and that simply is not allowed so not only do we not have to do this, we cannot do this!

Quoted 6 lines

Quoted 3 lines
controller, we could add an "int index" parameter to of_usb_get_dr_mode_by_phy and make it check that first argument specified to the phandle used in the controller node matches the passed in index.
Why we need the 'index'? Once the dt has seperate nodes for the phys, 'phy_np' passed in is the uniqe id of the phy node.

Because we cannot use separate nodes for the phys as that would break the devicetree bindings.

Anyways please wait for v2 of my patch, I believe that will solve this properly without needing to break devicetree bindings.

Regards,

Hans