diff mbox

[v2,06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

Message ID 1458181615-27782-7-git-send-email-david@lechnology.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Lechner March 17, 2016, 2:26 a.m. UTC 
This is a new phy driver for the SoC USB controllers on the TI DA8XX
family of microcontrollers. The USB 1.1 PHY is just a simple on/off.
The USB 2.0 PHY also allows overriding the VBUS and ID pins.

Signed-off-by: David Lechner <david@lechnology.com>
---

v2 changes: This is new patch in v2.



 drivers/phy/Kconfig         |   9 ++
 drivers/phy/Makefile        |   1 +
 drivers/phy/phy-da8xx-usb.c | 295 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 305 insertions(+)
 create mode 100644 drivers/phy/phy-da8xx-usb.c

Comments

Sergei Shtylyov March 17, 2016, 12:38 p.m. UTC | #1 
On 3/17/2016 5:26 AM, David Lechner wrote:

> This is a new phy driver for the SoC USB controllers on the TI DA8XX

    DA8xx, please.

> family of microcontrollers. The USB 1.1 PHY is just a simple on/off.
> The USB 2.0 PHY also allows overriding the VBUS and ID pins.
>
> Signed-off-by: David Lechner <david@lechnology.com>
[...]

> diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
> new file mode 100644
> index 0000000..93a5f4d
> --- /dev/null
> +++ b/drivers/phy/phy-da8xx-usb.c
> @@ -0,0 +1,295 @@
[...]
> +static inline u32 da8xx_usbphy_readl(void __iomem *base)
> +{
> +	return readl(base);
> +}
> +
> +static inline void da8xx_usbphy_writel(void __iomem *base, u32 value)
> +{
> +	writel(value, base);

    Why wrap them at all?

> +}
> +
> +static int da8xx_usb11_phy_init(struct phy *phy)
> +{
> +	struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> +	int ret;
> +	u32 val;
> +
> +	ret = clk_prepare_enable(d_phy->usb11_clk);
> +	if (ret)
> +		return ret;
> +
> +	val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> +	val |= USB1SUSPENDM;
> +	da8xx_usbphy_writel(d_phy->phy_ctrl, val);

    Hum, I'd think this needs to be done in the power_on() method...

> +
> +	return 0;
> +}
> +
> +static int da8xx_usb11_phy_shutdown(struct phy *phy)
> +{
> +	struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> +	u32 val;
> +
> +	val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> +	val &= ~USB1SUSPENDM;
> +	da8xx_usbphy_writel(d_phy->phy_ctrl, val);

    And this in power_off()...

> +
> +	clk_disable_unprepare(d_phy->usb11_clk);
> +
> +	return 0;
> +}
> +
> +static const struct phy_ops da8xx_usb11_phy_ops = {
> +	.power_on	= da8xx_usb11_phy_init,
> +	.power_off	= da8xx_usb11_phy_shutdown,

    Aha, it's just that the names don't match...
    Please call the implementations da8xx_usb11_phy_power_{on|off}().
The same with USB 2.0 PHY.

[...]

    Looks good otherwise on my superficial review. :-)

MBR, Sergei
Sekhar Nori March 23, 2016, 5:21 p.m. UTC | #2 
On Thursday 17 March 2016 07:56 AM, David Lechner wrote:
> This is a new phy driver for the SoC USB controllers on the TI DA8XX
> family of microcontrollers. The USB 1.1 PHY is just a simple on/off.
> The USB 2.0 PHY also allows overriding the VBUS and ID pins.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

> diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
> new file mode 100644
> index 0000000..93a5f4d
> --- /dev/null
> +++ b/drivers/phy/phy-da8xx-usb.c
> @@ -0,0 +1,295 @@
> +/*
> + * phy-da8xx-usb - TI DaVinci DA8XX USB PHY driver
> + *
> + * Copyright (C) 2016 David Lechner <david@lechnology.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/musb.h>
> +#include <linux/usb/otg.h>
> +
> +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
> +#define PHYCLKGD		(1 << 17)
> +#define VBUSSENSE		(1 << 16)
> +#define RESET			(1 << 15)
> +#define OTGMODE_MASK		(3 << 13)
> +#define NO_OVERRIDE		(0 << 13)
> +#define FORCE_HOST		(1 << 13)
> +#define FORCE_DEVICE		(2 << 13)
> +#define FORCE_HOST_VBUS_LOW	(3 << 13)
> +#define USB1PHYCLKMUX		(1 << 12)
> +#define USB2PHYCLKMUX		(1 << 11)
> +#define PHYPWRDN		(1 << 10)
> +#define OTGPWRDN		(1 << 9)
> +#define DATPOL			(1 << 8)
> +#define USB1SUSPENDM		(1 << 7)
> +#define PHY_PLLON		(1 << 6)
> +#define SESENDEN		(1 << 5)
> +#define VBDTCTEN		(1 << 4)
> +#define REFFREQ_MASK		(0xf << 0)
> +#define REFFREQ_12MHZ		(1 << 0)
> +#define REFFREQ_24MHZ		(2 << 0)
> +#define REFFREQ_48MHZ		(3 << 0)
> +#define REFFREQ_19_2MHZ		(4 << 0)
> +#define REFFREQ_38_4MHZ		(5 << 0)
> +#define REFFREQ_13MHZ		(6 << 0)
> +#define REFFREQ_26MHZ		(7 << 0)
> +#define REFFREQ_20MHZ		(8 << 0)
> +#define REFFREQ_40MHZ		(9 << 0)

Many of these register bits are unused. I guess opinion varies around
this, but I get confused with unnecessary bit definitions and register
offsets. I tend to search for it and its sort of disappointing to see
that its basically unused. Of course, you should wait for PHY
maintainers preference.

> +
> +struct da8xx_usbphy {
> +	struct phy_provider	*phy_provider;
> +	struct phy		*usb11_phy;
> +	struct phy		*usb20_phy;
> +	struct clk		*usb11_clk;
> +	struct clk		*usb20_clk;
> +	void __iomem		*phy_ctrl;
> +};
> +
> +static inline u32 da8xx_usbphy_readl(void __iomem *base)
> +{
> +	return readl(base);
> +}
> +
> +static inline void da8xx_usbphy_writel(void __iomem *base, u32 value)
> +{
> +	writel(value, base);
> +}

Agree with Sergei that these don't add much value.

> +int da8xx_usb20_phy_set_mode(struct phy *phy, enum musb_mode mode)
> +{
> +	struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> +	u32 val;
> +
> +	val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> +
> +	val &= ~OTGMODE_MASK;
> +	switch (mode) {
> +	case MUSB_HOST:		/* Force VBUS valid, ID = 0 */
> +		val |= FORCE_HOST;
> +		break;
> +	case MUSB_PERIPHERAL:	/* Force VBUS valid, ID = 1 */
> +		val |= FORCE_DEVICE;
> +		break;
> +	case MUSB_OTG:		/* Don't override the VBUS/ID comparators */
> +		val |= NO_OVERRIDE;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);

Hmm, since this driver exports this symbol, I think it should also
provide an include file in include/linux/phy for users of the symbol. Or
perhaps there should be a generic API around this since it looks like
most USB phys will need something similar?

Thanks,
Sekhar
David Lechner March 23, 2016, 6:06 p.m. UTC | #3 
On 03/23/2016 12:21 PM, Sekhar Nori wrote:
>> +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
>> +#define PHYCLKGD		(1 << 17)
>> +#define VBUSSENSE		(1 << 16)
>> +#define RESET			(1 << 15)
>> +#define OTGMODE_MASK		(3 << 13)
>> +#define NO_OVERRIDE		(0 << 13)
>> +#define FORCE_HOST		(1 << 13)
>> +#define FORCE_DEVICE		(2 << 13)
>> +#define FORCE_HOST_VBUS_LOW	(3 << 13)
>> +#define USB1PHYCLKMUX		(1 << 12)
>> +#define USB2PHYCLKMUX		(1 << 11)
>> +#define PHYPWRDN		(1 << 10)
>> +#define OTGPWRDN		(1 << 9)
>> +#define DATPOL			(1 << 8)
>> +#define USB1SUSPENDM		(1 << 7)
>> +#define PHY_PLLON		(1 << 6)
>> +#define SESENDEN		(1 << 5)
>> +#define VBDTCTEN		(1 << 4)
>> +#define REFFREQ_MASK		(0xf << 0)
>> +#define REFFREQ_12MHZ		(1 << 0)
>> +#define REFFREQ_24MHZ		(2 << 0)
>> +#define REFFREQ_48MHZ		(3 << 0)
>> +#define REFFREQ_19_2MHZ		(4 << 0)
>> +#define REFFREQ_38_4MHZ		(5 << 0)
>> +#define REFFREQ_13MHZ		(6 << 0)
>> +#define REFFREQ_26MHZ		(7 << 0)
>> +#define REFFREQ_20MHZ		(8 << 0)
>> +#define REFFREQ_40MHZ		(9 << 0)
>
> Many of these register bits are unused. I guess opinion varies around
> this, but I get confused with unnecessary bit definitions and register
> offsets. I tend to search for it and its sort of disappointing to see
> that its basically unused. Of course, you should wait for PHY
> maintainers preference.

My thinking was that since this driver *owns* the CFGCHIP2 register that 
is would eventually register clocks using the common clock framework, in 
which case, it would use many of these registers.

But, based on feedback on another commit, if we go the syscon route, 
then yes, I will remove the unused values.


>> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
>
> Hmm, since this driver exports this symbol, I think it should also
> provide an include file in include/linux/phy for users of the symbol. Or
> perhaps there should be a generic API around this since it looks like
> most USB phys will need something similar?
>

The reason I didn't make a header file is that this function is only 
meant to be use in one place and would likely cause a crash if used 
anywhere else.

I agree that it would be nice if the generic phy driver had a mechanism 
for user-defined callbacks such as this, however, I think the scope of 
this patch series has grown enough already.
David Laight March 24, 2016, 2:01 p.m. UTC | #4 
From: David Lechner
> Sent: 23 March 2016 18:07
> On 03/23/2016 12:21 PM, Sekhar Nori wrote:
> >> +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
> >> +#define PHYCLKGD		(1 << 17)
> >> +#define VBUSSENSE		(1 << 16)
> >> +#define RESET			(1 << 15)
> >> +#define OTGMODE_MASK		(3 << 13)
> >> +#define NO_OVERRIDE		(0 << 13)
> >> +#define FORCE_HOST		(1 << 13)
> >> +#define FORCE_DEVICE		(2 << 13)
> >> +#define FORCE_HOST_VBUS_LOW	(3 << 13)

I think I'd define the above as:
#define OTG_MODE(x) ((x) << 13)
#define OTGMODE_MASK		OTG_MODE(3)
#define NO_OVERRIDE		OTG_MODE(0)
#define FORCE_HOST		OTG_MODE(1)
#define FORCE_DEVICE		OTG_MODE(2)
#define FORCE_HOST_VBUS_LOW	OTG_MODE(3)
Then realise that all the names need changing (to include OTG).

> >> +#define USB1PHYCLKMUX		(1 << 12)
> >> +#define USB2PHYCLKMUX		(1 << 11)
> >> +#define PHYPWRDN		(1 << 10)
> >> +#define OTGPWRDN		(1 << 9)
> >> +#define DATPOL			(1 << 8)
> >> +#define USB1SUSPENDM		(1 << 7)
> >> +#define PHY_PLLON		(1 << 6)
> >> +#define SESENDEN		(1 << 5)
> >> +#define VBDTCTEN		(1 << 4)
> >> +#define REFFREQ_MASK		(0xf << 0)
> >> +#define REFFREQ_12MHZ		(1 << 0)
> >> +#define REFFREQ_24MHZ		(2 << 0)
> >> +#define REFFREQ_48MHZ		(3 << 0)
> >> +#define REFFREQ_19_2MHZ		(4 << 0)
> >> +#define REFFREQ_38_4MHZ		(5 << 0)
> >> +#define REFFREQ_13MHZ		(6 << 0)
> >> +#define REFFREQ_26MHZ		(7 << 0)
> >> +#define REFFREQ_20MHZ		(8 << 0)
> >> +#define REFFREQ_40MHZ		(9 << 0)

I'd define these similarly to the OTGxxx values.

> > Many of these register bits are unused. I guess opinion varies around
> > this, but I get confused with unnecessary bit definitions and register
> > offsets. I tend to search for it and its sort of disappointing to see
> > that its basically unused. Of course, you should wait for PHY
> > maintainers preference.

It can be useful to see what isn't being set.
Sometimes this can be very useful when making changes to a driver.
For status values it is particularly important to know what all the bits mean.

> My thinking was that since this driver *owns* the CFGCHIP2 register that
> is would eventually register clocks using the common clock framework, in
> which case, it would use many of these registers.
> 
> But, based on feedback on another commit, if we go the syscon route,
> then yes, I will remove the unused values.
> 
> 
> >> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
> >
> > Hmm, since this driver exports this symbol, I think it should also
> > provide an include file in include/linux/phy for users of the symbol. Or
> > perhaps there should be a generic API around this since it looks like
> > most USB phys will need something similar?
> >
> 
> The reason I didn't make a header file is that this function is only
> meant to be use in one place and would likely cause a crash if used
> anywhere else.

And a compilation error if compiled with -Wmissing-prototypes.

Sounds like you need a header file just for this driver's 'private' exports.

IMHO .c files shouldn't contain 'extern' anywhere.
I removed a load from some local code and found quite a few lurking bugs
where the types didn't match.

	David
diff mbox

Patch

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 26566db..a6060ea 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -35,6 +35,15 @@  config ARMADA375_USBCLUSTER_PHY
 	depends on OF && HAS_IOMEM
 	select GENERIC_PHY
 
+config PHY_DA8XX_USB
+	tristate "TI DA8XX USB PHY Driver"
+	depends on ARCH_DAVINCI_DA8XX
+	select GENERIC_PHY
+	help
+	  Enable this to support the USB PHY on DA8XX SoCs.
+
+	  This driver controls both the USB 1.1 PHY and the USB 2.0 PHY.
+
 config PHY_DM816X_USB
 	tristate "TI dm816x USB PHY driver"
 	depends on ARCH_OMAP2PLUS
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 24596a9..722e01c 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -5,6 +5,7 @@ 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
 obj-$(CONFIG_PHY_BERLIN_USB)		+= phy-berlin-usb.o
 obj-$(CONFIG_PHY_BERLIN_SATA)		+= phy-berlin-sata.o
+obj-$(CONFIG_PHY_DA8XX_USB)		+= phy-da8xx-usb.o
 obj-$(CONFIG_PHY_DM816X_USB)		+= phy-dm816x-usb.o
 obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)	+= phy-armada375-usb2.o
 obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
new file mode 100644
index 0000000..93a5f4d
--- /dev/null
+++ b/drivers/phy/phy-da8xx-usb.c
@@ -0,0 +1,295 @@ 
+/*
+ * phy-da8xx-usb - TI DaVinci DA8XX USB PHY driver
+ *
+ * Copyright (C) 2016 David Lechner <david@lechnology.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/musb.h>
+#include <linux/usb/otg.h>
+
+/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
+#define PHYCLKGD		(1 << 17)
+#define VBUSSENSE		(1 << 16)
+#define RESET			(1 << 15)
+#define OTGMODE_MASK		(3 << 13)
+#define NO_OVERRIDE		(0 << 13)
+#define FORCE_HOST		(1 << 13)
+#define FORCE_DEVICE		(2 << 13)
+#define FORCE_HOST_VBUS_LOW	(3 << 13)
+#define USB1PHYCLKMUX		(1 << 12)
+#define USB2PHYCLKMUX		(1 << 11)
+#define PHYPWRDN		(1 << 10)
+#define OTGPWRDN		(1 << 9)
+#define DATPOL			(1 << 8)
+#define USB1SUSPENDM		(1 << 7)
+#define PHY_PLLON		(1 << 6)
+#define SESENDEN		(1 << 5)
+#define VBDTCTEN		(1 << 4)
+#define REFFREQ_MASK		(0xf << 0)
+#define REFFREQ_12MHZ		(1 << 0)
+#define REFFREQ_24MHZ		(2 << 0)
+#define REFFREQ_48MHZ		(3 << 0)
+#define REFFREQ_19_2MHZ		(4 << 0)
+#define REFFREQ_38_4MHZ		(5 << 0)
+#define REFFREQ_13MHZ		(6 << 0)
+#define REFFREQ_26MHZ		(7 << 0)
+#define REFFREQ_20MHZ		(8 << 0)
+#define REFFREQ_40MHZ		(9 << 0)
+
+struct da8xx_usbphy {
+	struct phy_provider	*phy_provider;
+	struct phy		*usb11_phy;
+	struct phy		*usb20_phy;
+	struct clk		*usb11_clk;
+	struct clk		*usb20_clk;
+	void __iomem		*phy_ctrl;
+};
+
+static inline u32 da8xx_usbphy_readl(void __iomem *base)
+{
+	return readl(base);
+}
+
+static inline void da8xx_usbphy_writel(void __iomem *base, u32 value)
+{
+	writel(value, base);
+}
+
+static int da8xx_usb11_phy_init(struct phy *phy)
+{
+	struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
+	int ret;
+	u32 val;
+
+	ret = clk_prepare_enable(d_phy->usb11_clk);
+	if (ret)
+		return ret;
+
+	val = da8xx_usbphy_readl(d_phy->phy_ctrl);
+	val |= USB1SUSPENDM;
+	da8xx_usbphy_writel(d_phy->phy_ctrl, val);
+
+	return 0;
+}
+
+static int da8xx_usb11_phy_shutdown(struct phy *phy)
+{
+	struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
+	u32 val;
+
+	val = da8xx_usbphy_readl(d_phy->phy_ctrl);
+	val &= ~USB1SUSPENDM;
+	da8xx_usbphy_writel(d_phy->phy_ctrl, val);
+
+	clk_disable_unprepare(d_phy->usb11_clk);
+
+	return 0;
+}
+
+static const struct phy_ops da8xx_usb11_phy_ops = {
+	.power_on	= da8xx_usb11_phy_init,
+	.power_off	= da8xx_usb11_phy_shutdown,
+	.owner		= THIS_MODULE,
+};
+
+static int da8xx_usb20_phy_init(struct phy *phy)
+{
+	struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
+	int ret;
+	u32 val;
+
+	ret = clk_prepare_enable(d_phy->usb20_clk);
+	if (ret)
+		return ret;
+
+	val = da8xx_usbphy_readl(d_phy->phy_ctrl);
+	val &= ~OTGPWRDN;
+	da8xx_usbphy_writel(d_phy->phy_ctrl, val);
+
+	return 0;
+}
+
+static int da8xx_usb20_phy_shutdown(struct phy *phy)
+{
+	struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
+	u32 val;
+
+	val = da8xx_usbphy_readl(d_phy->phy_ctrl);
+	val |= OTGPWRDN;
+	da8xx_usbphy_writel(d_phy->phy_ctrl, val);
+
+	clk_disable_unprepare(d_phy->usb20_clk);
+
+	return 0;
+}
+
+static const struct phy_ops da8xx_usb20_phy_ops = {
+	.power_on	= da8xx_usb20_phy_init,
+	.power_off	= da8xx_usb20_phy_shutdown,
+	.owner		= THIS_MODULE,
+};
+
+int da8xx_usb20_phy_set_mode(struct phy *phy, enum musb_mode mode)
+{
+	struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
+	u32 val;
+
+	val = da8xx_usbphy_readl(d_phy->phy_ctrl);
+
+	val &= ~OTGMODE_MASK;
+	switch (mode) {
+	case MUSB_HOST:		/* Force VBUS valid, ID = 0 */
+		val |= FORCE_HOST;
+		break;
+	case MUSB_PERIPHERAL:	/* Force VBUS valid, ID = 1 */
+		val |= FORCE_DEVICE;
+		break;
+	case MUSB_OTG:		/* Don't override the VBUS/ID comparators */
+		val |= NO_OVERRIDE;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	da8xx_usbphy_writel(d_phy->phy_ctrl, val);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
+
+static struct phy *da8xx_usbphy_of_xlate(struct device *dev,
+					 struct of_phandle_args *args)
+{
+	struct da8xx_usbphy *d_phy = dev_get_drvdata(dev);
+
+	if (!d_phy)
+		return ERR_PTR(-ENODEV);
+
+	switch (args->args[0]) {
+	case 1:
+		return d_phy->usb11_phy;
+	case 2:
+		return d_phy->usb20_phy;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+}
+
+static int da8xx_usbphy_probe(struct platform_device *pdev)
+{
+	struct device		*dev = &pdev->dev;
+	struct device_node	*node = dev->of_node;
+	struct da8xx_usbphy	*d_phy;
+	struct resource		*res;
+
+	d_phy = devm_kzalloc(dev, sizeof(*d_phy), GFP_KERNEL);
+	if (!d_phy)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	d_phy->phy_ctrl = devm_ioremap_resource(dev, res);
+	if (IS_ERR(d_phy->phy_ctrl)) {
+		dev_err(dev, "Failed to map resource.\n");
+		return PTR_ERR(d_phy->phy_ctrl);
+	}
+
+	d_phy->usb11_clk = devm_clk_get(dev, "usb11_phy");
+	if (IS_ERR(d_phy->usb11_clk)) {
+		dev_err(dev, "Failed to get usb11_phy clock.\n");
+		return PTR_ERR(d_phy->usb11_clk);
+	}
+
+	d_phy->usb20_clk = devm_clk_get(dev, "usb20_phy");
+	if (IS_ERR(d_phy->usb20_clk)) {
+		dev_err(dev, "Failed to get usb20_phy clock.\n");
+		return PTR_ERR(d_phy->usb20_clk);
+	}
+
+	d_phy->usb11_phy = devm_phy_create(dev, node, &da8xx_usb11_phy_ops);
+	if (IS_ERR(d_phy->usb11_phy)) {
+		dev_err(dev, "Failed to create usb11 phy.\n");
+		return PTR_ERR(d_phy->usb11_phy);
+	}
+
+	d_phy->usb20_phy = devm_phy_create(dev, node, &da8xx_usb20_phy_ops);
+	if (IS_ERR(d_phy->usb20_phy)) {
+		dev_err(dev, "Failed to create usb20 phy.\n");
+		return PTR_ERR(d_phy->usb20_phy);
+	}
+
+	platform_set_drvdata(pdev, d_phy);
+	phy_set_drvdata(d_phy->usb11_phy, d_phy);
+	phy_set_drvdata(d_phy->usb20_phy, d_phy);
+
+	if (node) {
+		d_phy->phy_provider = devm_of_phy_provider_register(dev,
+							da8xx_usbphy_of_xlate);
+		if (IS_ERR(d_phy->phy_provider)) {
+			dev_err(dev, "Failed to create phy provider.\n");
+			return PTR_ERR(d_phy->phy_provider);
+		}
+	} else {
+		int ret;
+
+		ret = phy_create_lookup(d_phy->usb11_phy, "usbphy", "ohci.0");
+		if (ret)
+			dev_warn(dev, "Failed to create usb11 phy lookup .\n");
+		ret = phy_create_lookup(d_phy->usb20_phy, "usbphy", "musb-da8xx");
+		if (ret)
+			dev_warn(dev, "Failed to create usb20 phy lookup .\n");
+	}
+
+	return 0;
+}
+
+static int da8xx_usbphy_remove(struct platform_device *pdev)
+{
+	struct da8xx_usbphy *d_phy = platform_get_drvdata(pdev);
+
+	if (!pdev->dev.of_node) {
+		phy_remove_lookup(d_phy->usb20_phy, "usbphy", "musb-da8xx");
+		phy_remove_lookup(d_phy->usb11_phy, "usbphy", "ohci.0");
+	}
+
+	return 0;
+}
+
+static const struct of_device_id da8xx_usbphy_ids[] = {
+	{ .compatible = "ti,da830-usbphy" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, da8xx_usbphy_ids);
+
+static struct platform_driver da8xx_usbphy_driver = {
+	.probe	= da8xx_usbphy_probe,
+	.remove	= da8xx_usbphy_remove,
+	.driver	= {
+		.name	= "da8xx-usbphy",
+		.of_match_table = da8xx_usbphy_ids,
+	},
+};
+
+module_platform_driver(da8xx_usbphy_driver);
+
+MODULE_ALIAS("platform:da8xx-usbphy");
+MODULE_AUTHOR("David Lechner <david@lechnology.com>");
+MODULE_DESCRIPTION("TI DA8XX USB PHY driver");
+MODULE_LICENSE("GPL v2");