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

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

[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
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 <hdegoede@redhat.com>
---
 drivers/usb/common/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index e3d0161..9806433 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -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);
-- 
2.7.4

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

[Hans de Goede]

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

diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index bae54f7..1ea71f5 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -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];
 
-- 
2.7.4

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

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

diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index 1ea71f5..2247cf2 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -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,
-- 
2.7.4

[PATCH 4/4] musb: sunxi: Simplify dr_mode handling

[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 <hdegoede@redhat.com>
---
 drivers/usb/musb/sunxi.c | 68 ++++++++++++++++++------------------------------
 1 file changed, 25 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
index e7d4617..b88a2f6 100644
--- a/drivers/usb/musb/sunxi.c
+++ b/drivers/usb/musb/sunxi.c
@@ -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)
-- 
2.7.4

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

[Bin Liu]

Hi,

On Thu, Jun 02, 2016 at 07:31:03PM +0200, Hans de Goede wrote:
> 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? 

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

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

[Hans de Goede]

Hi,

On 02-06-16 20:16, Bin Liu wrote:
> Hi,
>
> On Thu, Jun 02, 2016 at 07:31:03PM +0200, Hans de Goede wrote:
>> 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.

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

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

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

[Bin Liu]

Hi,

On Fri, Jun 03, 2016 at 12:34:35PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 02-06-16 20:16, Bin Liu wrote:
> >Hi,
> >
> >On Thu, Jun 02, 2016 at 07:31:03PM +0200, Hans de Goede wrote:
> >>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

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.

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

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

[Hans de Goede]

Hi,

On 03-06-16 15:04, Bin Liu wrote:
> Hi,
>
> On Fri, Jun 03, 2016 at 12:34:35PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 02-06-16 20:16, Bin Liu wrote:
>>> Hi,
>>>
>>> On Thu, Jun 02, 2016 at 07:31:03PM +0200, Hans de Goede wrote:
>>>> 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
>
> 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

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

[Kishon Vijay Abraham I]

Hi,

On Thursday 02 June 2016 11:01 PM, Hans de Goede wrote:
> 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
> 
> 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 <hdegoede@redhat.com>
> ---
>  drivers/usb/common/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> index e3d0161..9806433 100644
> --- a/drivers/usb/common/common.c
> +++ b/drivers/usb/common/common.c
> @@ -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);
> 

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

[Hans de Goede]

Hi,

On 03-06-16 13:20, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 02 June 2016 11:01 PM, Hans de Goede wrote:
>> 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



>
> Thanks
> Kishon
>>
>> 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 <hdegoede@redhat.com>
>> ---
>>  drivers/usb/common/common.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
>> index e3d0161..9806433 100644
>> --- a/drivers/usb/common/common.c
>> +++ b/drivers/usb/common/common.c
>> @@ -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);
>>

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

[Bin Liu]

Hi,

On Fri, Jun 03, 2016 at 01:39:26PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 03-06-16 13:20, Kishon Vijay Abraham I wrote:
> >Hi,
> >
> >On Thursday 02 June 2016 11:01 PM, Hans de Goede wrote:
> >>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

I believe we have to.

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

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

Regards,
-Bin.

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

[Hans de Goede]

Hi,

On 03-06-16 15:12, Bin Liu wrote:
> Hi,
>
> On Fri, Jun 03, 2016 at 01:39:26PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 03-06-16 13:20, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Thursday 02 June 2016 11:01 PM, Hans de Goede wrote:
>>>> 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
>
> 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!

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