usb: add devicetree helpers for determining dr_mode and phy_type

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

Should we agree on this I would convert the fsl-mph-dr driver to use these helpers.

Sascha

 drivers/usb/core/Makefile |    1 +
 drivers/usb/core/of.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/of.h    |   22 +++++++++++++
 include/linux/usb/phy.h   |    9 ++++++
 4 files changed, 108 insertions(+)
 create mode 100644 drivers/usb/core/of.c
 create mode 100644 include/linux/usb/of.h

drivers/usb/core/Makefile +1 -0

index 26059b9..5378add 100644
@@ -10,5 +10,6 @@ usbcore-y += devio.o notify.o generic.o quirks.o devices.o
 
 usbcore-$(CONFIG_PCI)		+= hcd-pci.o
 usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
+usbcore-$(CONFIG_OF)		+= of.o
 
 obj-$(CONFIG_USB)		+= usbcore.o

drivers/usb/core/of.c new file +76 -0

new file mode 100644
index 0000000..d000d9f
@@ -0,0 +1,76 @@
+/*
+ * OF helpers for usb devices.
+ *
+ * This file is released under the GPLv2
+ *
+ * Initially copied out of drivers/of/of_net.c
+ */
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/usb/of.h>
+#include <linux/usb/phy.h>
+#include <linux/export.h>
+
+static const char *usbphy_modes[] = {
+	[USBPHY_INTERFACE_MODE_NA]	= "",
+	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
+	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmi_wide",
+	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
+	[USBPHY_INTERFACE_MODE_SERIAL]	= "serial",
+	[USBPHY_INTERFACE_MODE_HSIC]	= "hsic",
+};
+
+/**
+ * of_get_usbphy_mode - Get phy mode for given device_node
+ * @np:	Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'phy_type',
+ * and returns the correspondig enum usb_phy_interface
+ */
+enum usb_phy_interface of_usb_get_phy_mode(struct device_node *np)
+{
+	const char *phy_type;
+	int err, i;
+
+	err = of_property_read_string(np, "phy_type", &phy_type);
+	if (err < 0)
+		return USBPHY_INTERFACE_MODE_NA;
+
+	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
+		if (!strcasecmp(phy_type, usbphy_modes[i]))
+			return i;
+
+	return USBPHY_INTERFACE_MODE_NA;
+}
+EXPORT_SYMBOL_GPL(of_usb_get_phy_mode);
+
+static const char *usb_dr_modes[] = {
+	[USB_DR_MODE_UNKNOWN]		= "",
+	[USB_DR_MODE_HOST]		= "host",
+	[USB_DR_MODE_PERIPHERAL]	= "peripheral",
+	[USB_DR_MODE_OTG]		= "otg",
+};
+
+/**
+ * of_usb_get_dr_mode - Get dual role mode for given device_node
+ * @np:	Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'dr_mode',
+ * and returns the correspondig enum usb_phy_dr_mode
+ */
+enum usb_phy_dr_mode of_usb_get_dr_mode(struct device_node *np)
+{
+	const char *dr_mode;
+	int err, i;
+
+	err = of_property_read_string(np, "dr_mode", &dr_mode);
+	if (err < 0)
+		return USB_DR_MODE_UNKNOWN;
+
+	for (i = 0; i < ARRAY_SIZE(usb_dr_modes); i++)
+		if (!strcasecmp(dr_mode, usb_dr_modes[i]))
+			return i;
+
+	return USB_DR_MODE_UNKNOWN;
+}
+EXPORT_SYMBOL_GPL(of_usb_get_dr_mode);

include/linux/usb/of.h new file +22 -0

new file mode 100644
index 0000000..582ba96
@@ -0,0 +1,22 @@
+/*
+ * OF helpers for usb devices.
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_USB_OF_H
+#define __LINUX_USB_OF_H
+
+#include <linux/usb/phy.h>
+
+enum usb_phy_dr_mode {
+	USB_DR_MODE_UNKNOWN,
+	USB_DR_MODE_HOST,
+	USB_DR_MODE_PERIPHERAL,
+	USB_DR_MODE_OTG,
+};
+
+enum usb_phy_interface of_usb_get_phy_mode(struct device_node *np);
+enum usb_phy_dr_mode of_usb_get_dr_mode(struct device_node *np);
+
+#endif /* __LINUX_USB_OF_H */

include/linux/usb/phy.h +9 -0

index a29ae1e..c5154cf 100644
@@ -12,6 +12,15 @@
 #include <linux/notifier.h>
 #include <linux/usb.h>
 
+enum usb_phy_interface {
+	USBPHY_INTERFACE_MODE_NA,
+	USBPHY_INTERFACE_MODE_UTMI,
+	USBPHY_INTERFACE_MODE_UTMIW,
+	USBPHY_INTERFACE_MODE_ULPI,
+	USBPHY_INTERFACE_MODE_SERIAL,
+	USBPHY_INTERFACE_MODE_HSIC,
+};
+
 enum usb_phy_events {
 	USB_EVENT_NONE,         /* no events or cable disconnected */
 	USB_EVENT_VBUS,         /* vbus valid event */

Signature · 6 lines

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

Quoted 20 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

Quoted 2 lines

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

As far as I know, it is a chipidea core. Adding Peter to Cc list, he can probably confirm.

Quoted 162 lines

Should we agree on this I would convert the fsl-mph-dr driver to use these helpers.

Sascha

 drivers/usb/core/Makefile |    1 +
 drivers/usb/core/of.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/of.h    |   22 +++++++++++++
 include/linux/usb/phy.h   |    9 ++++++
 4 files changed, 108 insertions(+)
 create mode 100644 drivers/usb/core/of.c
 create mode 100644 include/linux/usb/of.h

drivers/usb/core/Makefile +1 -0

index 26059b9..5378add 100644
@@ -10,5 +10,6 @@ usbcore-y += devio.o notify.o generic.o quirks.o devices.o
 
 usbcore-$(CONFIG_PCI)		+= hcd-pci.o
 usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
+usbcore-$(CONFIG_OF)		+= of.o
 
 obj-$(CONFIG_USB)		+= usbcore.o

drivers/usb/core/of.c new file +76 -0

new file mode 100644
index 0000000..d000d9f
@@ -0,0 +1,76 @@
+/*
+ * OF helpers for usb devices.
+ *
+ * This file is released under the GPLv2
+ *
+ * Initially copied out of drivers/of/of_net.c
+ */
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/usb/of.h>
+#include <linux/usb/phy.h>
+#include <linux/export.h>
+
+static const char *usbphy_modes[] = {
+	[USBPHY_INTERFACE_MODE_NA]	= "",
+	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
+	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmi_wide",
+	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
+	[USBPHY_INTERFACE_MODE_SERIAL]	= "serial",
+	[USBPHY_INTERFACE_MODE_HSIC]	= "hsic",
+};
+
+/**
+ * of_get_usbphy_mode - Get phy mode for given device_node
+ * @np:	Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'phy_type',
+ * and returns the correspondig enum usb_phy_interface
+ */
+enum usb_phy_interface of_usb_get_phy_mode(struct device_node *np)
+{
+	const char *phy_type;
+	int err, i;
+
+	err = of_property_read_string(np, "phy_type", &phy_type);
+	if (err < 0)
+		return USBPHY_INTERFACE_MODE_NA;
+
+	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
+		if (!strcasecmp(phy_type, usbphy_modes[i]))
+			return i;
+
+	return USBPHY_INTERFACE_MODE_NA;
+}
+EXPORT_SYMBOL_GPL(of_usb_get_phy_mode);
+
+static const char *usb_dr_modes[] = {
+	[USB_DR_MODE_UNKNOWN]		= "",
+	[USB_DR_MODE_HOST]		= "host",
+	[USB_DR_MODE_PERIPHERAL]	= "peripheral",
+	[USB_DR_MODE_OTG]		= "otg",
+};
+
+/**
+ * of_usb_get_dr_mode - Get dual role mode for given device_node
+ * @np:	Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'dr_mode',
+ * and returns the correspondig enum usb_phy_dr_mode
+ */
+enum usb_phy_dr_mode of_usb_get_dr_mode(struct device_node *np)
+{
+	const char *dr_mode;
+	int err, i;
+
+	err = of_property_read_string(np, "dr_mode", &dr_mode);
+	if (err < 0)
+		return USB_DR_MODE_UNKNOWN;
+
+	for (i = 0; i < ARRAY_SIZE(usb_dr_modes); i++)
+		if (!strcasecmp(dr_mode, usb_dr_modes[i]))
+			return i;
+
+	return USB_DR_MODE_UNKNOWN;
+}
+EXPORT_SYMBOL_GPL(of_usb_get_dr_mode);

include/linux/usb/of.h new file +22 -0

new file mode 100644
index 0000000..582ba96
@@ -0,0 +1,22 @@
+/*
+ * OF helpers for usb devices.
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_USB_OF_H
+#define __LINUX_USB_OF_H
+
+#include <linux/usb/phy.h>
+
+enum usb_phy_dr_mode {
+	USB_DR_MODE_UNKNOWN,
+	USB_DR_MODE_HOST,
+	USB_DR_MODE_PERIPHERAL,
+	USB_DR_MODE_OTG,
+};
+
+enum usb_phy_interface of_usb_get_phy_mode(struct device_node *np);
+enum usb_phy_dr_mode of_usb_get_dr_mode(struct device_node *np);
+
+#endif /* __LINUX_USB_OF_H */

include/linux/usb/phy.h +9 -0

index a29ae1e..c5154cf 100644
@@ -12,6 +12,15 @@
 #include <linux/notifier.h>
 #include <linux/usb.h>
 
+enum usb_phy_interface {
+	USBPHY_INTERFACE_MODE_NA,
+	USBPHY_INTERFACE_MODE_UTMI,
+	USBPHY_INTERFACE_MODE_UTMIW,
+	USBPHY_INTERFACE_MODE_ULPI,
+	USBPHY_INTERFACE_MODE_SERIAL,
+	USBPHY_INTERFACE_MODE_HSIC,
+};
+
 enum usb_phy_events {
 	USB_EVENT_NONE,         /* no events or cable disconnected */
 	USB_EVENT_VBUS,         /* vbus valid event */

Signature · 6 lines

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

Quoted 25 lines · Alexander Shishkin

Quoted 22 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

Quoted 2 lines

As far as I know, it is a chipidea core. Adding Peter to Cc list, he can probably confirm.

The fsl-mph-dr can't be used for chipdiea as it handles three platform drivers for three roles (peripheral , host, otg). But chipidea only has two platform drivers, one is the chipidea core, the other is related controller wrapper.

Quoted 29 lines · Peter Chen

Quoted 27 lines · Alexander Shishkin

Quoted 16 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Quoted 6 lines

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

As far as I know, it is a chipidea core. Adding Peter to Cc list, he can probably confirm.

Quoted 4 lines

The fsl-mph-dr can't be used for chipdiea as it handles three platform drivers for three roles (peripheral , host, otg). But chipidea only has two platform drivers, one is the chipidea core, the other is related controller wrapper.

What do you mean by 'three platform drivers'? That's only how the driver is built, no? I was talking about the hardware the fsl-mph-dr driver handles which definitely smells like chipidea.

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Quoted 35 lines · Sascha Hauer

Quoted 33 lines · Peter Chen

Quoted 22 lines · Alexander Shishkin

Quoted 20 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

Quoted 5 lines

Quoted 2 lines

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

As far as I know, it is a chipidea core. Adding Peter to Cc list, he can probably confirm.

The fsl-mph-dr can't be used for chipdiea as it handles three platform drivers for three roles (peripheral , host, otg). But chipidea only has two platform drivers, one is the chipidea core, the other is related controller wrapper.

Quoted 3 lines

What do you mean by 'three platform drivers'? That's only how the driver is built, no? I was talking about the hardware the fsl-mph-dr driver handles which definitely smells like chipidea.

It creates host/device/otg platform device according to dr_mode from the device tree.

Quoted 40 lines · Peter Chen

Quoted 38 lines · Sascha Hauer

Quoted 26 lines · Peter Chen

Quoted 25 lines · Alexander Shishkin

Quoted 22 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

Quoted 7 lines

Quoted 2 lines

As far as I know, it is a chipidea core. Adding Peter to Cc list, he can probably confirm.

The fsl-mph-dr can't be used for chipdiea as it handles three platform drivers for three roles (peripheral , host, otg). But chipidea only has two platform drivers, one is the chipidea core, the other is related controller wrapper.

What do you mean by 'three platform drivers'? That's only how the driver is built, no? I was talking about the hardware the fsl-mph-dr driver handles which definitely smells like chipidea.

Quoted 2 lines

It creates host/device/otg platform device according to dr_mode from the device tree.

Again, that's software specific. What I'd like to know is whether the *hardware* could be handled by the chipidea driver.

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Quoted 41 lines · Sascha Hauer

Quoted 40 lines · Peter Chen

Quoted 38 lines · Sascha Hauer

Quoted 26 lines · Peter Chen

Quoted 25 lines · Alexander Shishkin

Quoted 22 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

Quoted 7 lines

Quoted 2 lines

As far as I know, it is a chipidea core. Adding Peter to Cc list, he can probably confirm.

The fsl-mph-dr can't be used for chipdiea as it handles three platform drivers for three roles (peripheral , host, otg). But chipidea only has two platform drivers, one is the chipidea core, the other is related controller wrapper.

What do you mean by 'three platform drivers'? That's only how the driver is built, no? I was talking about the hardware the fsl-mph-dr driver handles which definitely smells like chipidea.

Quoted 5 lines

Quoted 2 lines

It creates host/device/otg platform device according to dr_mode from the device tree.

Again, that's software specific. What I'd like to know is whether the *hardware* could be handled by the chipidea driver.

not understand u, you mean the DT information at there? Those DT information may not be used for i.mx hardware.

Quoted 42 lines · Peter Chen

Quoted 41 lines · Sascha Hauer

Quoted 40 lines · Peter Chen

Quoted 38 lines · Sascha Hauer

Quoted 26 lines · Peter Chen

Quoted 25 lines · Alexander Shishkin

Quoted 22 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

Quoted 7 lines

Quoted 2 lines

As far as I know, it is a chipidea core. Adding Peter to Cc list, he can probably confirm.

The fsl-mph-dr can't be used for chipdiea as it handles three platform drivers for three roles (peripheral , host, otg). But chipidea only has two platform drivers, one is the chipidea core, the other is related controller wrapper.

What do you mean by 'three platform drivers'? That's only how the driver is built, no? I was talking about the hardware the fsl-mph-dr driver handles which definitely smells like chipidea.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

It creates host/device/otg platform device according to dr_mode from the device tree.

Again, that's software specific. What I'd like to know is whether the *hardware* could be handled by the chipidea driver.

not understand u, you mean the DT information at there? Those DT information may not be used for i.mx hardware.

The original question was:

There is a driver in the tree called fsl-mph-dr-of.c. Does this driver handle a hardware which is compatible to the hardware the chipidea driver handles?

I think the answer is yes, because said driver registers a ehci device, or fsl-usb2-udc device (the same we used on i.MX). This hardware also has a PORTSC register. All this seems to suggest that

drivers/usb/host/fsl-mph-dr-of.c
drivers/usb/host/ehci-fsl.c
drivers/usb/otg/fsl_otg.c
drivers/usb/gadget/fsl_usb2_udc.h
drivers/usb/gadget/fsl_udc_core.c

Could be replaced by the chipidea driver.

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

Should we agree on this I would convert the fsl-mph-dr driver to use these helpers.

Sascha

  drivers/usb/core/Makefile |    1 +
  drivers/usb/core/of.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++

  include/linux/usb/of.h    |   22 +++++++++++++
  include/linux/usb/phy.h   |    9 ++++++
  4 files changed, 108 insertions(+)

create mode 100644 drivers/usb/core/of.c
create mode 100644 include/linux/usb/of.h

drivers/usb/core/Makefile +0 -0

index 26059b9..5378add 100644

drivers/usb/core/Makefile +0 -0

  obj-$(CONFIG_USB)		+= usbcore.o

drivers/usb/core/of.c new file +27 -0

new file mode 100644
index 0000000..d000d9f
@@ -0,0 +1,76 @@
+/*
+ * OF helpers for usb devices.
+ *
+ * This file is released under the GPLv2
+ *
+ * Initially copied out of drivers/of/of_net.c
+ */
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/usb/of.h>
+#include <linux/usb/phy.h>
+#include <linux/export.h>
+
+static const char *usbphy_modes[] = {
+	[USBPHY_INTERFACE_MODE_NA]	= "",
+	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
+	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmi_wide",
+	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
+	[USBPHY_INTERFACE_MODE_SERIAL]	= "serial",
+	[USBPHY_INTERFACE_MODE_HSIC]	= "hsic",
+};
+
+/**
+ * of_get_usbphy_mode - Get phy mode for given device_node
+ * @np:	Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'phy_type',

drivers/usb/core/of.c new file +25 -0

+
+	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
+		if (!strcasecmp(phy_type, usbphy_modes[i]))
+			return i;
+
+	return USBPHY_INTERFACE_MODE_NA;
+}
+EXPORT_SYMBOL_GPL(of_usb_get_phy_mode);
+
+static const char *usb_dr_modes[] = {
+	[USB_DR_MODE_UNKNOWN]		= "",
+	[USB_DR_MODE_HOST]		= "host",
+	[USB_DR_MODE_PERIPHERAL]	= "peripheral",
+	[USB_DR_MODE_OTG]		= "otg",
+};
+
+/**
+ * of_usb_get_dr_mode - Get dual role mode for given device_node
+ * @np:	Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'dr_mode',
+ * and returns the correspondig enum usb_phy_dr_mode
+ */
+enum usb_phy_dr_mode of_usb_get_dr_mode(struct device_node *np)
+{

drivers/usb/core/of.c line 37

Quoted 4 lines

drivers/usb/core/of.c new file +3 -0

+	err = of_property_read_string(np, "phy_type", &phy_type);
+	if (err < 0)
+		return USBPHY_INTERFACE_MODE_NA;

Why don't we use a u32 property type for the *phy-type*? IMHO we should use string property only when the property should be absolutely unambiguous (e.g., compatible property should be string).

Quoted 7 lines

drivers/usb/core/of.c line 37

Quoted 3 lines

drivers/usb/core/of.c new file +3 -0

+	err = of_property_read_string(np, "phy_type", &phy_type);
+	if (err < 0)
+		return USBPHY_INTERFACE_MODE_NA;

Why don't we use a u32 property type for the *phy-type*? IMHO we should use string property only when the property should be absolutely unambiguous (e.g., compatible property should be string).

If we would use u32-numbers in the compatible entry, this would also be unambiguous, no? 0xd00dfeed would be the at24-driver. Pretty specific.

I don't mind having readable devicetrees. And we have it for ethernet phys already with strings, so it would be consistent.

Quoted 10 lines · Wolfram Sang

Quoted 7 lines

drivers/usb/core/of.c line 37

Quoted 3 lines

drivers/usb/core/of.c new file +3 -0

+	err = of_property_read_string(np, "phy_type", &phy_type);
+	if (err < 0)
+		return USBPHY_INTERFACE_MODE_NA;

Why don't we use a u32 property type for the *phy-type*? IMHO we should use string property only when the property should be absolutely unambiguous (e.g., compatible property should be string).

If we would use u32-numbers in the compatible entry, this would also be unambiguous, no? 0xd00dfeed would be the at24-driver. Pretty specific.

hehe... But we don't have a corresponding *enum* representing the drivers :-)

Quoted 3 lines

I don't mind having readable devicetrees. And we have it for ethernet phys already with strings, so it would be consistent.

Ok. Fine with it then :-)

I prefer u32 here, because we have the matching enum. Otherwise we end up with:

of_property_read_string(...,&type);

if (!strcmp(type, "ulpi"))

	foo();
else if (!strcmp(type, "utmi"))
	bar();
else if (!strcmp(type, "pipe3"))
	baz();
else
	BUG();

and I don't like that, it's ugly and error prone.

Quoted 15 lines

I prefer u32 here, because we have the matching enum. Otherwise we end up with:

of_property_read_string(...,&type);

if (!strcmp(type, "ulpi"))

	foo();
else if (!strcmp(type, "utmi"))
	bar();
else if (!strcmp(type, "pipe3"))
	baz();
else
	BUG();

Quoted 1 line

and I don't like that, it's ugly and error prone.

Error prone? I guess my mileage varies. Especially compared to the probability devicetree creators pick the wrong number.

It also removes the (probably implicit) rule that the enum mustn't be modified since it is exported to users.

Also, you could map the strings to the enum first and then switch-case over it to make the code nicer.

Hi,

Quoted 12 lines · kishon

Quoted 10 lines · Wolfram Sang

Quoted 7 lines

drivers/usb/core/of.c line 37

Quoted 3 lines

drivers/usb/core/of.c new file +3 -0

+  err = of_property_read_string(np, "phy_type", &phy_type);
+  if (err < 0)
+          return USBPHY_INTERFACE_MODE_NA;

Why don't we use a u32 property type for the *phy-type*? IMHO we should use string property only when the property should be absolutely unambiguous (e.g., compatible property should be string).

If we would use u32-numbers in the compatible entry, this would also be unambiguous, no? 0xd00dfeed would be the at24-driver. Pretty specific.

Quoted 7 lines

hehe... But we don't have a corresponding *enum* representing the drivers :-)

Quoted 3 lines

I don't mind having readable devicetrees. And we have it for ethernet phys already with strings, so it would be consistent.

Ok. Fine with it then :-)

I prefer u32 here, because we have the matching enum. Otherwise we end up with:

of_property_read_string(...,&type);

if (!strcmp(type, "ulpi"))

        foo();
else if (!strcmp(type, "utmi"))
        bar();
else if (!strcmp(type, "pipe3"))
        baz();
else
        BUG();

and I don't like that, it's ugly and error prone. Also looks awful when someone needs to change that, the diff looks messy. A u32 followed by a switch statement looks nicer.

Quoted 37 lines · Felipe Balbi

Hi,

Quoted 12 lines · kishon

Quoted 10 lines · Wolfram Sang

Quoted 7 lines

drivers/usb/core/of.c line 37

Quoted 3 lines

drivers/usb/core/of.c new file +3 -0

+  err = of_property_read_string(np, "phy_type", &phy_type);
+  if (err < 0)
+          return USBPHY_INTERFACE_MODE_NA;

Why don't we use a u32 property type for the *phy-type*? IMHO we should use string property only when the property should be absolutely unambiguous (e.g., compatible property should be string).

If we would use u32-numbers in the compatible entry, this would also be unambiguous, no? 0xd00dfeed would be the at24-driver. Pretty specific.

Quoted 7 lines

hehe... But we don't have a corresponding *enum* representing the drivers :-)

Quoted 3 lines

I don't mind having readable devicetrees. And we have it for ethernet phys already with strings, so it would be consistent.

Ok. Fine with it then :-)

I prefer u32 here, because we have the matching enum. Otherwise we end up with:

of_property_read_string(...,&type);

if (!strcmp(type, "ulpi"))

        foo();
else if (!strcmp(type, "utmi"))
        bar();
else if (!strcmp(type, "pipe3"))
        baz();
else
        BUG();

Quoted 3 lines

and I don't like that, it's ugly and error prone. Also looks awful when someone needs to change that, the diff looks messy. A u32 followed by a switch statement looks nicer.

I wholeheartedly and vehemently disagree.

Device trees don't exist to make Linux look prettier, *OR* clean up your source code of string comparisons when you think comparing an integer looks or feels cleaner. They exist to provide a hardware description. phy-type 0x3 does not describe anything except that you have to go look up what 0x3 means, and means device trees cannot be internally consistent within themselves or publically existing documentation (it is certain that there is no USB PHY specification that defines 0x3 as anything, which means the value is entirely Linux specific).

Matching an enum or magic number encoded into a u32 in some external source code to define a type that wasn't just an index is not something that anyone has ever reasonably done in any traditional IEEE1275 device tree or OpenFirmware-committee binding, and we shouldn't just be subverting the existing standard to make Linux code "look prettier".

Please consider other operating systems, which would need to copy the definition of the enum which may not be possible when thinking of Linux vs. FreeBSD or so, and in general the readability of the device tree - phy-type 0x3 is going to require a comment in the device tree source to remind people what that means, or a cross-reference to a binding which is more work than most developers want to do to make sure they specified the correct PHY type. A string is completely and unavoidably correct - a typo in the phy-type means a match against valid bindings is impossible and an instant bug. A mistaken u32 value means the wrong phy-type is defined which has increased potential to provide misconfigured phy with the wrong type and less warning than a string literal not being absolutely identical. I think that means more buggy device trees will get past where it doesn't actually work properly, and more time will be spent working out WHY it doesn't actually work properly.

It is much better to be totally unambiguous in the device tree as per the type and a string is the best way. If you really want effective, less error-prone code, define all the existing or useful types as preprocessor defines (#define PHY_TYPE_UTMI_WIDE "utmiw") and use those to match the binding. I wouldn't hand-code a property string inline even if you offered me a million dollars to do so. Matching the dr_mode property is already done in a drivers/of or of_phy.h include so just move the potential match code to there and return the correct enum (which is arguably Linux-specific) from the string and give a big fat error from the match function if none of the valid bindings matches up.

BTW I disagree with the use of underscores in device trees as well, I wouldn't use an underscore for a new property. But in the case of dr_mode it is well used already especially for Freescale controllers on PPC where there are a significant handful of DTS (or real OF) that implement it. It might not be a bad idea, though, to update the binding deprecating dr_mode in favor of something that is much less ambiguous and descriptive itself - "dr-mode" is an acceptable fix but a "role" property or "dual-role-mode" is much more descriptive. Moving the matching to some common code since more than one controller uses it and then adding a new, better property name with dr_mode as an acceptable but unrecommended fallback means this can be left to history.

--
Matt Sealey <[email protected]>
Product Development Analyst, Genesi USA, Inc.

Hi,

Quoted 17 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

drivers/usb/core/of.c new file +11 -0

+/**
+ * of_get_usbphy_mode - Get phy mode for given device_node
+ * @np:    Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'phy_type',
+ * and returns the correspondig enum usb_phy_interface
+ */
+enum usb_phy_interface of_usb_get_phy_mode(struct device_node *np)
+{
+    const char *phy_type;
+    int err, i;

drivers/usb/core/of.c line 37

Quoted 4 lines

drivers/usb/core/of.c new file +4 -0

+
+    err = of_property_read_string(np, "phy_type", &phy_type);
+    if (err < 0)
+        return USBPHY_INTERFACE_MODE_NA;

Why don't we use a u32 property type for the *phy-type*? IMHO we should use string property only when the property should be absolutely unambiguous (e.g., compatible property should be string).

Hi,

Quoted 26 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

Should we agree on this I would convert the fsl-mph-dr driver to use these helpers.

Quoted 4 lines

Sascha

 drivers/usb/core/Makefile |    1 +
 drivers/usb/core/of.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++

This file should ideally go into drivers/usb/phy/.

Quoted 12 lines

 include/linux/usb/of.h    |   22 +++++++++++++
 include/linux/usb/phy.h   |    9 ++++++
 4 files changed, 108 insertions(+)
 create mode 100644 drivers/usb/core/of.c
 create mode 100644 include/linux/usb/of.h

drivers/usb/core/Makefile +0 -0

index 26059b9..5378add 100644
@@ -10,5 +10,6 @@ usbcore-y += devio.o notify.o generic.o quirks.o devices.o
 

drivers/usb/core/Makefile usbcore-y += devio.o notify.o generic.o quirks.o devices.o · line 13

Quoted 3 lines

drivers/usb/core/Makefile +1 -0

 usbcore-$(CONFIG_PCI)		+= hcd-pci.o
 usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
+usbcore-$(CONFIG_OF)		+= of.o

No Kconfig? Shouldn't this file be compiled only when some one is going to use the PHY?

Quoted 31 lines · kishon

Hi,

Quoted 28 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

Should we agree on this I would convert the fsl-mph-dr driver to use these helpers.

Sascha

Quoted 4 lines

Quoted 2 lines

  drivers/usb/core/Makefile |    1 +
  drivers/usb/core/of.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++

This file should ideally go into drivers/usb/phy/.

I originally wanted to do that, but the host/peripheral/otg property is not phy specific. DO you still want to move it there?

Quoted 35 lines · Sascha Hauer

Quoted 34 lines · kishon

Hi,

Quoted 30 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

Should we agree on this I would convert the fsl-mph-dr driver to use these helpers.

Sascha

 drivers/usb/core/Makefile |    1 +
 drivers/usb/core/of.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++

Quoted 4 lines

Quoted 1 line

This file should ideally go into drivers/usb/phy/.

I originally wanted to do that, but the host/peripheral/otg property is not phy specific. DO you still want to move it there?

I think then you can just move of_usb_get_phy_mode() to phy/of.c. Then we can also move some functions defined in otg.c (specific to PHY and dt) to phy/of.c.

Quoted 41 lines · kishon

Quoted 39 lines · Sascha Hauer

Quoted 29 lines · kishon

Hi,

Quoted 26 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

I need this binding for the chipidea driver. I suspect that the fsl-mph-dr driver also really handles a chipidea core.

Should we agree on this I would convert the fsl-mph-dr driver to use these helpers.

Quoted 6 lines

Quoted 4 lines

Sascha

  drivers/usb/core/Makefile |    1 +
  drivers/usb/core/of.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++

This file should ideally go into drivers/usb/phy/.

I originally wanted to do that, but the host/peripheral/otg property is not phy specific. DO you still want to move it there?

Quoted 3 lines

I think then you can just move of_usb_get_phy_mode() to phy/of.c. Then we can also move some functions defined in otg.c (specific to PHY and dt) to phy/of.c.

The phy specific stuff in otg.c can't easily be moved as all functions operate on a static list and spinlock. Also nothing in otg/otg.c is currently of specific.

What about the dr_mode helper? Moving it to otg/ would mean that all

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

Quoted 16 lines · Sascha Hauer

From: Michael Grzeschik <[email protected]>

This adds two little devicetree helper functions for determining the dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from the devicetree.

Signed-off-by Michael Grzeschik <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---

The properties and their values have been taken from the fsl-mph-dr driver. This binding is also documented (though currently not used) for the tegra ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt). This is a first attempt to parse these bindings at a common place so that others can make use of it.

Quoted 3 lines

Basically I want to know whether this binding is recommended for new drivers since normally the devicetree uses '-' instead of '_', and maybe there are other problems with it.

It's certainly typical to use - not _ for freshly defined properties. However, since this property already exists and is in-use, I don't think there's any choice but to maintain its current definition.

The code looked fine to me.