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

Groups > linux.kernel > #1369306 > unrolled thread

Re: [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

  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
    Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY David Lechner <david@lechnology.com> - 2016-04-13 23:00 +0200
      Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY Kishon Vijay Abraham I <kishon@ti.com> - 2016-04-14 14:40 +0200

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

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] | [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] | [next] | [standalone]

#1378293

On 04/01/2016 08:16 AM, Kishon Vijay Abraham I 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.
>
> I think it'll be better to create a new attribute and use it?
>

Just having an attribute that keeps track of state does not work. I need 
a callback to poke registers. Would this be acceptable instead?

-----8<------
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index e7e574d..a13c7e4 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -342,6 +342,36 @@ int phy_power_off(struct phy *phy)
  }
  EXPORT_SYMBOL_GPL(phy_power_off);

+int phy_get_mode(struct phy *phy, enum phy_mode *mode)
+{
+       int ret;
+
+       if (!phy || !phy->ops->get_mode)
+               return 0;
+
+       mutex_lock(&phy->mutex);
+       ret = phy->ops->get_mode(phy, mode);
+       mutex_unlock(&phy->mutex);
+
+       return ret;
+}
+EXPORT_SYMBOL_GPL(phy_get_mode);
+
+int phy_set_mode(struct phy *phy, enum phy_mode mode)
+{
+       int ret;
+
+       if (!phy || !phy->ops->set_mode)
+               return 0;
+
+       mutex_lock(&phy->mutex);
+       ret = phy->ops->set_mode(phy, mode);
+       mutex_unlock(&phy->mutex);
+
+       return ret;
+}
+EXPORT_SYMBOL_GPL(phy_set_mode);
+
  /**
   * _of_phy_get() - lookup and obtain a reference to a phy by phandle
   * @np: device_node for which to get the phy
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 8cf05e3..12c1986 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -22,12 +22,21 @@

  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
   * @exit: operation to be performed while exiting
   * @power_on: powering on the phy
   * @power_off: powering off the phy
+ * @get_mode: get the current mode of the phy
+ * @set_mode: set the mode of the phy
   * @owner: the module owner containing the ops
   */
  struct phy_ops {
@@ -35,6 +44,8 @@ struct phy_ops {
         int     (*exit)(struct phy *phy);
         int     (*power_on)(struct phy *phy);
         int     (*power_off)(struct phy *phy);
+       int     (*get_mode)(struct phy *phy, enum phy_mode *mode);
+       int     (*set_mode)(struct phy *phy, enum phy_mode mode);
         struct module *owner;
  };

@@ -119,6 +130,8 @@ int phy_init(struct phy *phy);
  int phy_exit(struct phy *phy);
  int phy_power_on(struct phy *phy);
  int phy_power_off(struct phy *phy);
+int phy_get_mode(struct phy *phy, enum phy_mode *mode);
+int phy_set_mode(struct phy *phy, enum phy_mode mode);
  static inline int phy_get_bus_width(struct phy *phy)
  {
         return phy->attrs.bus_width;
@@ -224,6 +237,16 @@ static inline int phy_power_off(struct phy *phy)
         return -ENOSYS;
  }

+static inline int phy_get_mode(struct phy *phy, enum phy_mode *mode)
+{
+       return 0;
+}
+
+static inline int phy_set_mode(struct phy *phy, enum phy_mode mode)
+{
+       return 0;
+}
+
  static inline int phy_get_bus_width(struct phy *phy)
  {
         return -ENOSYS;

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

#1378790

Hi,

On Thursday 14 April 2016 02:21 AM, David Lechner wrote:
> On 04/01/2016 08:16 AM, Kishon Vijay Abraham I 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.
>>
>> I think it'll be better to create a new attribute and use it?
>>
> 
> Just having an attribute that keeps track of state does not work. I need a
> callback to poke registers. Would this be acceptable instead?
> 
> -----8<------
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index e7e574d..a13c7e4 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -342,6 +342,36 @@ int phy_power_off(struct phy *phy)
>  }
>  EXPORT_SYMBOL_GPL(phy_power_off);
> 
> +int phy_get_mode(struct phy *phy, enum phy_mode *mode)
> +{
> +       int ret;
> +
> +       if (!phy || !phy->ops->get_mode)
> +               return 0;
> +
> +       mutex_lock(&phy->mutex);
> +       ret = phy->ops->get_mode(phy, mode);
> +       mutex_unlock(&phy->mutex);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_get_mode);

'ops' for get_mode doesn't make much sense.
> +
> +int phy_set_mode(struct phy *phy, enum phy_mode mode)
> +{
> +       int ret;
> +
> +       if (!phy || !phy->ops->set_mode)
> +               return 0;
> +
> +       mutex_lock(&phy->mutex);
> +       ret = phy->ops->set_mode(phy, mode);
> +       mutex_unlock(&phy->mutex);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_set_mode);

this should be fine.

Thanks
Kishon

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web