Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1359548 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY David Lechner <david@lechnology.com> - 2016-03-17 04:40 +0100
    Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-03-17 13:40 +0100
    Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY Sekhar Nori <nsekhar@ti.com> - 2016-03-23 18:30 +0100
      Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY David Lechner <david@lechnology.com> - 2016-03-23 19:10 +0100
        RE: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB  PHY David Laight <David.Laight@ACULAB.COM> - 2016-03-24 15:10 +0100
    Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY Kishon Vijay Abraham I <kishon@ti.com> - 2016-04-01 15:20 +0200
      Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY Bin Liu <b-liu@ti.com> - 2016-04-01 16:50 +0200
        Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY David Lechner <david@lechnology.com> - 2016-04-01 18:10 +0200
          Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY Bin Liu <b-liu@ti.com> - 2016-04-01 18:30 +0200
            Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-04-01 21:50 +0200
          Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-04-01 21:50 +0200
            Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY Bin Liu <b-liu@ti.com> - 2016-04-01 22:00 +0200

#1359548 — [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

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

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");
-- 
1.9.1

[toc] | [next] | [standalone]

#1359805

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

[toc] | [prev] | [next] | [standalone]

#1363569

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

[toc] | [prev] | [next] | [standalone]

#1363592

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.

[toc] | [prev] | [next] | [standalone]

#1364181 — RE: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

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

[toc] | [prev] | [next] | [standalone]

#1369306

Hi,

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

Don't prefer export symbols from PHY driver. That'll create unnecessary
dependencies between the controller and the PHY.

I think it'll be better to create a new attribute and use it?

8<============

diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 8cf05e3..3d92b9b 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -22,6 +22,13 @@

 struct phy;

+enum phy_mode {
+	PHY_MODE_INVALID,
+	PHY_MODE_USB_HOST,
+	PHY_MODE_USB_DEVICE,
+	PHY_MODE_USB_OTG,
+};
+
 /**
  * struct phy_ops - set of function pointers for performing phy operations
  * @init: operation to be performed for initializing phy
@@ -44,6 +51,7 @@ struct phy_ops {
  */
 struct phy_attrs {
 	u32			bus_width;
+	enum phy_mode		mode;
 };

 /**
@@ -127,6 +135,14 @@ static inline void phy_set_bus_width(struct phy *phy, int
bus_width)
 {
 	phy->attrs.bus_width = bus_width;
 }
+static inline enum phy_mode phy_get_mode(struct phy *phy)
+{
+	return phy->attrs.mode;
+}
+static inline void phy_set_mode(struct phy *phy, enum phy_mode mode)
+{
+	phy->attrs.mode = mode;
+}
 struct phy *phy_get(struct device *dev, const char *string);
 struct phy *phy_optional_get(struct device *dev, const char *string);
 struct phy *devm_phy_get(struct device *dev, const char *string);
@@ -234,6 +250,16 @@ static inline void phy_set_bus_width(struct phy *phy, int
bus_width)
 	return;
 }

+static inline enum phy_mode phy_get_mode(struct phy *phy)
+{
+	return PHY_MODE_INVALID;
+}
+
+static inline void phy_set_mode(struct phy *phy, enum phy_mode mode)
+{
+	return;
+}
+
 static inline struct phy *phy_get(struct device *dev, const char *string)
 {
 	return ERR_PTR(-ENOSYS);


=====================>8

Thanks
Kishon

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

[toc] | [prev] | [next] | [standalone]

#1369395

Hi,

On Fri, Apr 01, 2016 at 06:46:28PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
> 
> 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>
> > ---
> > 
> > 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
> > 
> > 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);
> 
> Don't prefer export symbols from PHY driver. That'll create unnecessary
> dependencies between the controller and the PHY.

Agreed.

> 
> I think it'll be better to create a new attribute and use it?

Another simpler option is to not support _set_mode() for DA8xx, and the
phy driver set the otgmode bit in probe() based on dr_mode of the
controller.

Regards,
-Bin.

[toc] | [prev] | [next] | [standalone]

#1369457

On 04/01/2016 09:45 AM, Bin Liu wrote:
>>> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
>>
>> Don't prefer export symbols from PHY driver. That'll create unnecessary
>> dependencies between the controller and the PHY.
>
> Agreed.
>
>>
>> I think it'll be better to create a new attribute and use it?
>
> Another simpler option is to not support _set_mode() for DA8xx, and the
> phy driver set the otgmode bit in probe() based on dr_mode of the
> controller.
>
> Regards,
> -Bin.
>

This certainly works for my particular use case, however, Sergei 
Shtylyov, who wrote the da8xx musb glue layer originally, seemed to 
recall in his review of the patch series that this really is necessary 
for OTG mode to work properly.

By the way, there is v3 of this patch series already, although this 
patch is nearly the same.

[toc] | [prev] | [next] | [standalone]

#1369471

Hi,

On Fri, Apr 01, 2016 at 11:02:23AM -0500, David Lechner wrote:
> On 04/01/2016 09:45 AM, Bin Liu wrote:
> >>>+EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
> >>
> >>Don't prefer export symbols from PHY driver. That'll create unnecessary
> >>dependencies between the controller and the PHY.
> >
> >Agreed.
> >
> >>
> >>I think it'll be better to create a new attribute and use it?
> >
> >Another simpler option is to not support _set_mode() for DA8xx, and the
> >phy driver set the otgmode bit in probe() based on dr_mode of the
> >controller.
> >
> >Regards,
> >-Bin.
> >
> 
> This certainly works for my particular use case, however, Sergei
> Shtylyov, who wrote the da8xx musb glue layer originally, seemed to
> recall in his review of the patch series that this really is
> necessary for OTG mode to work properly.

I don't know much about the usb module on DA8xx, but it seems to me that
_set_mode() has nothing to do with OTG mode. musb_core only calls
_set_mode() in two places - 1) sysfs: musb_mode_store(), 2) init:
musb_init_controller(). None of them should be related to OTG mode.

Regards,
-Bin.

[toc] | [prev] | [next] | [standalone]

#1369561

On 04/01/2016 07:19 PM, Bin Liu wrote:

>>>>> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
>>>>
>>>> Don't prefer export symbols from PHY driver. That'll create unnecessary
>>>> dependencies between the controller and the PHY.
>>>
>>> Agreed.
>>>
>>>>
>>>> I think it'll be better to create a new attribute and use it?
>>>
>>> Another simpler option is to not support _set_mode() for DA8xx, and the
>>> phy driver set the otgmode bit in probe() based on dr_mode of the
>>> controller.
>>>
>>> Regards,
>>> -Bin.
>>>
>>
>> This certainly works for my particular use case, however, Sergei
>> Shtylyov, who wrote the da8xx musb glue layer originally, seemed to
>> recall in his review of the patch series that this really is
>> necessary for OTG mode to work properly.
>
> I don't know much about the usb module on DA8xx, but it seems to me that
> _set_mode() has nothing to do with OTG mode.

    No, it does.

> musb_core only calls
> _set_mode() in two places - 1) sysfs: musb_mode_store(), 2) init:
> musb_init_controller(). None of them should be related to OTG mode.

    Both these places do select OTG mode.

> Regards,
> -Bin.

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1369560

On 04/01/2016 07:02 PM, David Lechner wrote:

>>>> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
>>>
>>> Don't prefer export symbols from PHY driver. That'll create unnecessary
>>> dependencies between the controller and the PHY.
>>
>> Agreed.
>>
>>> I think it'll be better to create a new attribute and use it?
>>
>> Another simpler option is to not support _set_mode() for DA8xx, and the
>> phy driver set the otgmode bit in probe() based on dr_mode of the
>> controller.
>>
>> Regards,
>> -Bin.
>
> This certainly works for my particular use case, however, Sergei Shtylyov, who
> wrote the da8xx musb glue layer originally, seemed to recall in his review of
> the patch series that this really is necessary for OTG mode to work properly.

    No, it's still not necessary, however this method is now called from 
musb_init_controller() (it wasn't in my time). ISTR however that you needed to 
enforce the mode override in CFGCHIP2 for the host mode to function properly.

WBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1369566

Hi,

On Fri, Apr 01, 2016 at 10:45:47PM +0300, Sergei Shtylyov wrote:
> On 04/01/2016 07:02 PM, David Lechner wrote:
> 
> >>>>+EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
> >>>
> >>>Don't prefer export symbols from PHY driver. That'll create unnecessary
> >>>dependencies between the controller and the PHY.
> >>
> >>Agreed.
> >>
> >>>I think it'll be better to create a new attribute and use it?
> >>
> >>Another simpler option is to not support _set_mode() for DA8xx, and the
> >>phy driver set the otgmode bit in probe() based on dr_mode of the
> >>controller.
> >>
> >>Regards,
> >>-Bin.
> >
> >This certainly works for my particular use case, however, Sergei Shtylyov, who
> >wrote the da8xx musb glue layer originally, seemed to recall in his review of
> >the patch series that this really is necessary for OTG mode to work properly.
> 
>    No, it's still not necessary, however this method is now called
> from musb_init_controller() (it wasn't in my time). ISTR however
> that you needed to enforce the mode override in CFGCHIP2 for the
> host mode to function properly.

Yes, but I think we don't have to rely on musb_init_controller(), what
we can do is the phy probe() calls of_usb_get_dr_mode_by_phy() to get
the dr_mode of the controller then sets CFGCHIP2 bits accordingly, so
that we can drop _set_mode() in the phy driver.

Regards,
-Bin.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web