[RFC] usb: add devicetree helpers for determining dr_mode and phy_type

Commit Message

Sascha Hauer Jan. 29, 2013, 11:22 a.m. UTC

From: Michael Grzeschik <[e-mail]>

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 <[e-mail]>
Signed-off-by: Marc Kleine-Budde <[e-mail]>
---

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

Comments

Alexander Shishkin Jan. 29, 2013, 11:55 a.m. UTC | #1

Sascha Hauer <[e-mail]> writes:

> From: Michael Grzeschik <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Marc Kleine-Budde <[e-mail]>
> ---
>
> 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.

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

> 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
>
> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> index 26059b9..5378add 100644
> --- a/drivers/usb/core/Makefile
> +++ b/drivers/usb/core/Makefile
> @@ -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
> diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
> new file mode 100644
> index 0000000..d000d9f
> --- /dev/null
> +++ b/drivers/usb/core/of.c
> @@ -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);
> diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
> new file mode 100644
> index 0000000..582ba96
> --- /dev/null
> +++ b/include/linux/usb/of.h
> @@ -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 */
> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index a29ae1e..c5154cf 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -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 */
> -- 
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kishon Vijay Abraham I Jan. 29, 2013, 1:44 p.m. UTC | #2

Hi,

On Tuesday 29 January 2013 04:52 PM, Sascha Hauer wrote:
> From: Michael Grzeschik <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Marc Kleine-Budde <[e-mail]>
> ---
>
> 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 +++++++++++++++++++++++++++++++++++++++++++++

This file should ideally go into drivers/usb/phy/.
>   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
>
> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> index 26059b9..5378add 100644
> --- a/drivers/usb/core/Makefile
> +++ b/drivers/usb/core/Makefile
> @@ -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

No Kconfig? Shouldn't this file be compiled only when some one is going 
to use the PHY?
>
>   obj-$(CONFIG_USB)		+= usbcore.o
> diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
> new file mode 100644
> index 0000000..d000d9f
> --- /dev/null
> +++ b/drivers/usb/core/of.c
> @@ -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;

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

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

Same comment applies here too.

Thanks
Kishon

Wolfram Sang Jan. 29, 2013, 1:53 p.m. UTC | #3

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

Kishon Vijay Abraham I Jan. 29, 2013, 2:10 p.m. UTC | #4

On Tuesday 29 January 2013 07:23 PM, Wolfram Sang wrote:
>>> +	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 :-)
>
> 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 :-)

Thanks
Kishon

Felipe Balbi Jan. 29, 2013, 2:33 p.m. UTC | #5

Hi,

On Tue, Jan 29, 2013 at 07:40:23PM +0530, kishon wrote:
> On Tuesday 29 January 2013 07:23 PM, Wolfram Sang wrote:
> >>>+	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 :-)
> >
> >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.

Wolfram Sang Jan. 29, 2013, 2:55 p.m. UTC | #6

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

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.

Marc Kleine-Budde Jan. 29, 2013, 3:05 p.m. UTC | #7

On 01/29/2013 03:55 PM, Wolfram Sang wrote:
> 
>> 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.
> 
> 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.

That's what the code already does.

Marc

Stephen Warren Jan. 29, 2013, 5:10 p.m. UTC | #8

On 01/29/2013 06:44 AM, kishon wrote:
> Hi,
> 
> On Tuesday 29 January 2013 04:52 PM, Sascha Hauer wrote:
>> From: Michael Grzeschik <[e-mail]>
>>
>> This adds two little devicetree helper functions for determining the
>> dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from
>> the devicetree.

>> +/**
>> + * 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;
> 
> 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).

Well, this DT property is already defined an in-use, so while a
different decision might (or might not) be made today about the property
name/content, it's already defined and widely in use, so can't really be
changed.

Stephen Warren Jan. 29, 2013, 5:11 p.m. UTC | #9

On 01/29/2013 04:22 AM, Sascha Hauer wrote:
> From: Michael Grzeschik <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Marc Kleine-Budde <[e-mail]>
> ---
> 
> 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.

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.

Marc Kleine-Budde Jan. 29, 2013, 5:16 p.m. UTC | #10

On 01/29/2013 06:11 PM, Stephen Warren wrote:
> On 01/29/2013 04:22 AM, Sascha Hauer wrote:
>> From: Michael Grzeschik <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Marc Kleine-Budde <[e-mail]>
>> ---
>>
>> 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.
> 
> 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.

The code for phy_type is lifted from the ethernet guys and adopted to
the usb phy types.

"dr_mode" is, as Sascha pointed out, only used in
drivers/usb/host/fsl-mph-dr-of.c

Marc

Sascha Hauer Jan. 29, 2013, 8:30 p.m. UTC | #11

On Tue, Jan 29, 2013 at 07:14:51PM +0530, kishon wrote:
> Hi,
> 
> On Tuesday 29 January 2013 04:52 PM, Sascha Hauer wrote:
> >From: Michael Grzeschik <[e-mail]>
> >
> >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 <[e-mail]>
> >Signed-off-by: Marc Kleine-Budde <[e-mail]>
> >---
> >
> >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 +++++++++++++++++++++++++++++++++++++++++++++
> 
> 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?

> >  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
> >
> >diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> >index 26059b9..5378add 100644
> >--- a/drivers/usb/core/Makefile
> >+++ b/drivers/usb/core/Makefile
> >@@ -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
> 
> No Kconfig? Shouldn't this file be compiled only when some one is
> going to use the PHY?

Yes. Just skipped that for the first shot.

Sascha

Peter Chen Jan. 30, 2013, 2:06 a.m. UTC | #12

On Tue, Jan 29, 2013 at 01:55:04PM +0200, Alexander Shishkin wrote:
> Sascha Hauer <[e-mail]> writes:
> 
> > From: Michael Grzeschik <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Marc Kleine-Budde <[e-mail]>
> > ---
> >
> > 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.
> 
> 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.

However, two common helpers are useful, dr_mode can override controller
capability, and using user defined controller operation mode.
phy_type can be used at controller initialization to set phy's type at
controller's register, eg, for chipidea code, there is a PTS
(Parallel Transceiver Select) domain at portsc to set phy's type.

> 
> > 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
> >
> > diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> > index 26059b9..5378add 100644
> > --- a/drivers/usb/core/Makefile
> > +++ b/drivers/usb/core/Makefile
> > @@ -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
> > diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
> > new file mode 100644
> > index 0000000..d000d9f
> > --- /dev/null
> > +++ b/drivers/usb/core/of.c
> > @@ -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);
> > diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
> > new file mode 100644
> > index 0000000..582ba96
> > --- /dev/null
> > +++ b/include/linux/usb/of.h
> > @@ -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 */
> > diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> > index a29ae1e..c5154cf 100644
> > --- a/include/linux/usb/phy.h
> > +++ b/include/linux/usb/phy.h
> > @@ -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 */
> > -- 
> > 1.7.10.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Kishon Vijay Abraham I Jan. 30, 2013, 5:51 a.m. UTC | #13

On Wednesday 30 January 2013 02:00 AM, Sascha Hauer wrote:
> On Tue, Jan 29, 2013 at 07:14:51PM +0530, kishon wrote:
>> Hi,
>>
>> On Tuesday 29 January 2013 04:52 PM, Sascha Hauer wrote:
>>> From: Michael Grzeschik <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Marc Kleine-Budde <[e-mail]>
>>> ---
>>>
>>> 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 +++++++++++++++++++++++++++++++++++++++++++++
>>
>> 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.

Thanks
Kishon

Sascha Hauer Jan. 30, 2013, 10:11 a.m. UTC | #14

On Wed, Jan 30, 2013 at 11:21:35AM +0530, kishon wrote:
> On Wednesday 30 January 2013 02:00 AM, Sascha Hauer wrote:
> >On Tue, Jan 29, 2013 at 07:14:51PM +0530, kishon wrote:
> >>Hi,
> >>
> >>On Tuesday 29 January 2013 04:52 PM, Sascha Hauer wrote:
> >>>From: Michael Grzeschik <[e-mail]>
> >>>
> >>>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 <[e-mail]>
> >>>Signed-off-by: Marc Kleine-Budde <[e-mail]>
> >>>---
> >>>
> >>>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 +++++++++++++++++++++++++++++++++++++++++++++
> >>
> >>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.

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
users which want to use it would have to select USB_OTG_UTILS. At least
the fsl mph driver currently does not need USB_OTG_UTILS.

ATM I'm feeling like killing USB_OTG_UTILS completely, that would make
things easier.

Sascha

Kishon Vijay Abraham I Jan. 30, 2013, 10:31 a.m. UTC | #15

Hi,

On Wednesday 30 January 2013 03:41 PM, Sascha Hauer wrote:
> On Wed, Jan 30, 2013 at 11:21:35AM +0530, kishon wrote:
>> On Wednesday 30 January 2013 02:00 AM, Sascha Hauer wrote:
>>> On Tue, Jan 29, 2013 at 07:14:51PM +0530, kishon wrote:
>>>> Hi,
>>>>
>>>> On Tuesday 29 January 2013 04:52 PM, Sascha Hauer wrote:
>>>>> From: Michael Grzeschik <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Marc Kleine-Budde <[e-mail]>
>>>>> ---
>>>>>
>>>>> 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 +++++++++++++++++++++++++++++++++++++++++++++
>>>>
>>>> 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.
>
> 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.

Actually nothing in otg.c is specific to OTG except one function 
otg_state_string(). So we should ideally have all the list and spinlock 
stuff be moved to phy.c

Some of them got added recently (like devm_usb_get_phy_by_phandle). It 
should be in 
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-next
>
> What about the dr_mode helper? Moving it to otg/ would mean that all

*dr_mode* doesn't look like it should be in phy/ or otg/. You can keep 
it as is in core/of.c

Thanks
Kishon

Sascha Hauer Jan. 30, 2013, 2 p.m. UTC | #16

On Wed, Jan 30, 2013 at 10:06:28AM +0800, Peter Chen wrote:
> On Tue, Jan 29, 2013 at 01:55:04PM +0200, Alexander Shishkin wrote:
> > Sascha Hauer <[e-mail]> writes:
> > 
> > > From: Michael Grzeschik <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Signed-off-by: Marc Kleine-Budde <[e-mail]>
> > > ---
> > >
> > > 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.
> > 
> > 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.

Sascha

Matt Sealey Jan. 30, 2013, 7:33 p.m. UTC | #17

On Tue, Jan 29, 2013 at 8:33 AM, Felipe Balbi <[e-mail]> wrote:
> Hi,
>
> On Tue, Jan 29, 2013 at 07:40:23PM +0530, kishon wrote:
>> On Tuesday 29 January 2013 07:23 PM, Wolfram Sang wrote:
>> >>>+  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 :-)
>> >
>> >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.

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 <[e-mail]>
Product Development Analyst, Genesi USA, Inc.

Matt Sealey Jan. 30, 2013, 7:35 p.m. UTC | #18

s/is already done/should already be done.
Matt Sealey <[e-mail]>
Product Development Analyst, Genesi USA, Inc.


On Wed, Jan 30, 2013 at 1:33 PM, Matt Sealey <[e-mail]> wrote:
> On Tue, Jan 29, 2013 at 8:33 AM, Felipe Balbi <[e-mail]> wrote:
>> Hi,
>>
>> On Tue, Jan 29, 2013 at 07:40:23PM +0530, kishon wrote:
>>> On Tuesday 29 January 2013 07:23 PM, Wolfram Sang wrote:
>>> >>>+  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 :-)
>>> >
>>> >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.
>
> 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 <[e-mail]>
> Product Development Analyst, Genesi USA, Inc.

Peter Chen Jan. 31, 2013, 2:05 a.m. UTC | #19

On Wed, Jan 30, 2013 at 03:00:15PM +0100, Sascha Hauer wrote:
> On Wed, Jan 30, 2013 at 10:06:28AM +0800, Peter Chen wrote:
> > On Tue, Jan 29, 2013 at 01:55:04PM +0200, Alexander Shishkin wrote:
> > > Sascha Hauer <[e-mail]> writes:
> > > 
> > > > From: Michael Grzeschik <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Signed-off-by: Marc Kleine-Budde <[e-mail]>
> > > > ---
> > > >
> > > > 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.
> > > 
> > > 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.

It creates host/device/otg platform device according to dr_mode from
the device tree.
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |
>

Sascha Hauer Jan. 31, 2013, 10:29 a.m. UTC | #20

On Thu, Jan 31, 2013 at 10:05:44AM +0800, Peter Chen wrote:
> On Wed, Jan 30, 2013 at 03:00:15PM +0100, Sascha Hauer wrote:
> > On Wed, Jan 30, 2013 at 10:06:28AM +0800, Peter Chen wrote:
> > > On Tue, Jan 29, 2013 at 01:55:04PM +0200, Alexander Shishkin wrote:
> > > > Sascha Hauer <[e-mail]> writes:
> > > > 
> > > > > From: Michael Grzeschik <[e-mail]>
> > > > >
> > > > > 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 <[e-mail]>
> > > > > Signed-off-by: Marc Kleine-Budde <[e-mail]>
> > > > > ---
> > > > >
> > > > > 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.
> > > > 
> > > > 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.
> 
> 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

Peter Chen Feb. 1, 2013, 1:11 a.m. UTC | #21

On Thu, Jan 31, 2013 at 11:29:13AM +0100, Sascha Hauer wrote:
> On Thu, Jan 31, 2013 at 10:05:44AM +0800, Peter Chen wrote:
> > On Wed, Jan 30, 2013 at 03:00:15PM +0100, Sascha Hauer wrote:
> > > On Wed, Jan 30, 2013 at 10:06:28AM +0800, Peter Chen wrote:
> > > > On Tue, Jan 29, 2013 at 01:55:04PM +0200, Alexander Shishkin wrote:
> > > > > Sascha Hauer <[e-mail]> writes:
> > > > > 
> > > > > > From: Michael Grzeschik <[e-mail]>
> > > > > >
> > > > > > 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 <[e-mail]>
> > > > > > Signed-off-by: Marc Kleine-Budde <[e-mail]>
> > > > > > ---
> > > > > >
> > > > > > 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.
> > > > > 
> > > > > 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.
> > 
> > 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.
> 
> Sascha
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |
>

Sascha Hauer Feb. 1, 2013, 6:58 a.m. UTC | #22

On Fri, Feb 01, 2013 at 09:11:54AM +0800, Peter Chen wrote:
> On Thu, Jan 31, 2013 at 11:29:13AM +0100, Sascha Hauer wrote:
> > On Thu, Jan 31, 2013 at 10:05:44AM +0800, Peter Chen wrote:
> > > On Wed, Jan 30, 2013 at 03:00:15PM +0100, Sascha Hauer wrote:
> > > > On Wed, Jan 30, 2013 at 10:06:28AM +0800, Peter Chen wrote:
> > > > > On Tue, Jan 29, 2013 at 01:55:04PM +0200, Alexander Shishkin wrote:
> > > > > > Sascha Hauer <[e-mail]> writes:
> > > > > > 
> > > > > > > From: Michael Grzeschik <[e-mail]>
> > > > > > >
> > > > > > > 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 <[e-mail]>
> > > > > > > Signed-off-by: Marc Kleine-Budde <[e-mail]>
> > > > > > > ---
> > > > > > >
> > > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > 
> > > 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

Peter Chen Feb. 1, 2013, 12:21 p.m. UTC | #23

On Fri, Feb 01, 2013 at 07:58:34AM +0100, Sascha Hauer wrote:
> On Fri, Feb 01, 2013 at 09:11:54AM +0800, Peter Chen wrote:
> > On Thu, Jan 31, 2013 at 11:29:13AM +0100, Sascha Hauer wrote:
> > > On Thu, Jan 31, 2013 at 10:05:44AM +0800, Peter Chen wrote:
> > > > On Wed, Jan 30, 2013 at 03:00:15PM +0100, Sascha Hauer wrote:
> > > > > On Wed, Jan 30, 2013 at 10:06:28AM +0800, Peter Chen wrote:
> > > > > > On Tue, Jan 29, 2013 at 01:55:04PM +0200, Alexander Shishkin wrote:
> > > > > > > Sascha Hauer <[e-mail]> writes:
> > > > > > > 
> > > > > > > > From: Michael Grzeschik <[e-mail]>
> > > > > > > >
> > > > > > > > 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 <[e-mail]>
> > > > > > > > Signed-off-by: Marc Kleine-Budde <[e-mail]>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > 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.
> > > > > > > 
> > > > > > > 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.
> > > > 
> > > > 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.

Yes, after creating a PowerPC's glue driver, like ci13xxx_ppc, or whatever.
> 
> Sascha
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |
>

Patch

diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
index 26059b9..5378add 100644
--- a/drivers/usb/core/Makefile
+++ b/drivers/usb/core/Makefile
@@ -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
diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
new file mode 100644
index 0000000..d000d9f
--- /dev/null
+++ b/drivers/usb/core/of.c
@@ -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);
diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
new file mode 100644
index 0000000..582ba96
--- /dev/null
+++ b/include/linux/usb/of.h
@@ -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 */
diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
index a29ae1e..c5154cf 100644
--- a/include/linux/usb/phy.h
+++ b/include/linux/usb/phy.h
@@ -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 */