[PATCH 0/5] Add phy support for AM335X platform using Generic PHy framework

[PATCH 0/5] Add phy support for AM335X platform using Generic PHy framework

[George Cherian]

This patch series adds phy support for AM335X platform.
This patch series is based on Generic PHY framework [1]. 


This series has
	- adds dual musb instances support for am335x platform (just for testing)
	- adds phy-amxxxx-usb driver used in AMxxxx platforms
	- adds dt  bindings for the phys
	- removes usb-phy and replaced with generic phy apis in glue layer

All these changes are avilable at [2]

[1] - http://marc.info/?l=linux-usb&m=137224750928570&w=2
[2] - git://git.ti.com/~georgecherian/ti-linux-kernel/georgec-connectivity-linux-feature-tree.git 
	am335x-phy-driver

George Cherian (4):
  usb: phy: phy-omap-control: Add API to power on/off USB PHY for AM335X
  phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform
  arm: dts: Add USB phy nodes for AM33XX
  usb: musb: dsps: Remove the phy control from glue and add phy driver
    APIs

Ravi Babu (1):
  usb: musb: dsps: enable dual instance support for am33xx platform

 arch/arm/boot/dts/am33xx.dtsi        |  22 ++++
 drivers/phy/Kconfig                  |  12 +++
 drivers/phy/Makefile                 |   1 +
 drivers/phy/phy-amxxxx-usb.c         | 201 +++++++++++++++++++++++++++++++++++
 drivers/usb/musb/musb_dsps.c         | 107 +++++++------------
 drivers/usb/phy/phy-omap-control.c   |  32 ++++++
 include/linux/usb/omap_control_usb.h |  11 ++
 7 files changed, 316 insertions(+), 70 deletions(-)
 create mode 100644 drivers/phy/phy-amxxxx-usb.c

-- 
1.8.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] usb: musb: dsps: enable dual instance support for am33xx platform

[George Cherian]

From: Ravi Babu <ravibabu@ti.com>

The dsps am33xx platform has two instances of musb controller,
enable the support for dual musb instances

Signed-off-by: Ravi Babu <ravibabu@ti.com>
---
 drivers/usb/musb/musb_dsps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index e1b661d..41f135a 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -747,7 +747,7 @@ static const struct dsps_musb_wrapper ti81xx_driver_data = {
 	.rxep_bitmap		= (0xfffe << 16),
 	.musb_core_offset	= 0x400,
 	.poll_seconds		= 2,
-	.instances		= 1,
+	.instances		= 2,
 };
 
 static const struct platform_device_id musb_dsps_id_table[] = {
-- 
1.8.1.4

[PATCH 2/5] usb: phy: phy-omap-control: Add API to power on/off USB PHY for AM335X

[George Cherian]

Add API to power on/off USB PHY for AM335X. This API will be called
from phy-amxxxx-usb driver.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/usb/phy/phy-omap-control.c   | 32 ++++++++++++++++++++++++++++++++
 include/linux/usb/omap_control_usb.h | 11 +++++++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/usb/phy/phy-omap-control.c b/drivers/usb/phy/phy-omap-control.c
index 1419ced..0c1822d 100644
--- a/drivers/usb/phy/phy-omap-control.c
+++ b/drivers/usb/phy/phy-omap-control.c
@@ -46,6 +46,38 @@ struct device *omap_get_control_dev(void)
 EXPORT_SYMBOL_GPL(omap_get_control_dev);
 
 /**
+ * omap_control_am335x_phy_power - power on/off the serializer using control
+ *	module
+ * @dev: the control module device
+ * @on: 0 to off and 1 to on based on powering on or off the PHY
+ * @id: phy id 0 or 1 for phy instance 0 and 1 repectively
+ *
+ * AMXXXX PHY driver should call this API to power on or off the PHY.
+ */
+void omap_control_am335x_phy_power(struct device *dev, bool on, u8 id)
+{
+	u32 val;
+	struct omap_control_usb	*control_usb = dev_get_drvdata(dev);
+
+	if (control_usb->type != OMAP_CTRL_DEV_TYPE3)
+		return;
+
+	val = readl(control_usb->dev_conf + (id * 2));
+
+	if (on) {
+		val &= ~(AM335X_USBPHY_CM_PWRDN | AM335X_USBPHY_OTG_PWRDN);
+		val |= AM335X_USBPHY_OTGVDET_EN |
+			AM335X_USBPHY_OTGSESSEND_EN;
+	} else
+		val |= AM335X_USBPHY_CM_PWRDN | AM335X_USBPHY_OTG_PWRDN;
+
+
+	writel(val, (control_usb->dev_conf + (id * 2)));
+}
+EXPORT_SYMBOL_GPL(omap_control_am335x_phy_power);
+
+
+/**
  * omap_control_usb3_phy_power - power on/off the serializer using control
  *	module
  * @dev: the control module device
diff --git a/include/linux/usb/omap_control_usb.h b/include/linux/usb/omap_control_usb.h
index 27b5b8c..11d7a34 100644
--- a/include/linux/usb/omap_control_usb.h
+++ b/include/linux/usb/omap_control_usb.h
@@ -45,6 +45,7 @@ enum omap_control_usb_mode {
 /* To differentiate ctrl module IP having either mailbox or USB3 PHY power */
 #define	OMAP_CTRL_DEV_TYPE1		0x1
 #define	OMAP_CTRL_DEV_TYPE2		0x2
+#define	OMAP_CTRL_DEV_TYPE3		0x3
 
 #define	OMAP_CTRL_DEV_PHY_PD		BIT(0)
 
@@ -63,10 +64,20 @@ enum omap_control_usb_mode {
 #define	OMAP_CTRL_USB3_PHY_TX_RX_POWERON	0x3
 #define	OMAP_CTRL_USB3_PHY_TX_RX_POWEROFF	0x0
 
+#define AM335X_USBPHY_CM_PWRDN		(1 << 0)
+#define AM335X_USBPHY_OTG_PWRDN		(1 << 1)
+#define AM335X_USBPHY_OTGVDET_EN		(1 << 19)
+#define AM335X_USBPHY_OTGSESSEND_EN	(1 << 20)
+#define AM335X_USB0_WKUP_CTRL_ENABLE	(1 << 0)
+#define AM335X_USB1_WKUP_CTRL_ENABLE	(1 << 8)
+
+
+
 #if IS_ENABLED(CONFIG_OMAP_CONTROL_USB)
 extern struct device *omap_get_control_dev(void);
 extern void omap_control_usb_phy_power(struct device *dev, int on);
 extern void omap_control_usb3_phy_power(struct device *dev, bool on);
+extern void omap_control_am335x_phy_power(struct device *dev, bool on, u8 id);
 extern void omap_control_usb_set_mode(struct device *dev,
 	enum omap_control_usb_mode mode);
 #else
-- 
1.8.1.4

Re: [PATCH 2/5] usb: phy: phy-omap-control: Add API to power on/off USB PHY for AM335X

[Kishon Vijay Abraham I]

Hi,

On Monday 08 July 2013 04:13 PM, George Cherian wrote:
> Add API to power on/off USB PHY for AM335X. This API will be called
> from phy-amxxxx-usb driver.
>
> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>   drivers/usb/phy/phy-omap-control.c   | 32 ++++++++++++++++++++++++++++++++
>   include/linux/usb/omap_control_usb.h | 11 +++++++++++
>   2 files changed, 43 insertions(+)
>
> diff --git a/drivers/usb/phy/phy-omap-control.c b/drivers/usb/phy/phy-omap-control.c
> index 1419ced..0c1822d 100644
> --- a/drivers/usb/phy/phy-omap-control.c
> +++ b/drivers/usb/phy/phy-omap-control.c
> @@ -46,6 +46,38 @@ struct device *omap_get_control_dev(void)
>   EXPORT_SYMBOL_GPL(omap_get_control_dev);
>
>   /**
> + * omap_control_am335x_phy_power - power on/off the serializer using control
> + *	module
> + * @dev: the control module device
> + * @on: 0 to off and 1 to on based on powering on or off the PHY
> + * @id: phy id 0 or 1 for phy instance 0 and 1 repectively
> + *
> + * AMXXXX PHY driver should call this API to power on or off the PHY.
> + */
> +void omap_control_am335x_phy_power(struct device *dev, bool on, u8 id)
> +{
> +	u32 val;
> +	struct omap_control_usb	*control_usb = dev_get_drvdata(dev);
> +
> +	if (control_usb->type != OMAP_CTRL_DEV_TYPE3)
> +		return;

Lets validate id also here.
> +
> +	val = readl(control_usb->dev_conf + (id * 2));

Understand you use id to calculate the offset. But looks a bit hacky to me.
Can we define macros and use it instead?
> +
> +	if (on) {
> +		val &= ~(AM335X_USBPHY_CM_PWRDN | AM335X_USBPHY_OTG_PWRDN);
> +		val |= AM335X_USBPHY_OTGVDET_EN |
> +			AM335X_USBPHY_OTGSESSEND_EN;
> +	} else
> +		val |= AM335X_USBPHY_CM_PWRDN | AM335X_USBPHY_OTG_PWRDN;
> +
> +
> +	writel(val, (control_usb->dev_conf + (id * 2)));
> +}
> +EXPORT_SYMBOL_GPL(omap_control_am335x_phy_power);
> +
> +
> +/**
>    * omap_control_usb3_phy_power - power on/off the serializer using control
>    *	module
>    * @dev: the control module device
> diff --git a/include/linux/usb/omap_control_usb.h b/include/linux/usb/omap_control_usb.h
> index 27b5b8c..11d7a34 100644
> --- a/include/linux/usb/omap_control_usb.h
> +++ b/include/linux/usb/omap_control_usb.h
> @@ -45,6 +45,7 @@ enum omap_control_usb_mode {
>   /* To differentiate ctrl module IP having either mailbox or USB3 PHY power */
>   #define	OMAP_CTRL_DEV_TYPE1		0x1
>   #define	OMAP_CTRL_DEV_TYPE2		0x2
> +#define	OMAP_CTRL_DEV_TYPE3		0x3
>
>   #define	OMAP_CTRL_DEV_PHY_PD		BIT(0)
>
> @@ -63,10 +64,20 @@ enum omap_control_usb_mode {
>   #define	OMAP_CTRL_USB3_PHY_TX_RX_POWERON	0x3
>   #define	OMAP_CTRL_USB3_PHY_TX_RX_POWEROFF	0x0
>
> +#define AM335X_USBPHY_CM_PWRDN		(1 << 0)
> +#define AM335X_USBPHY_OTG_PWRDN		(1 << 1)
> +#define AM335X_USBPHY_OTGVDET_EN		(1 << 19)
> +#define AM335X_USBPHY_OTGSESSEND_EN	(1 << 20)
> +#define AM335X_USB0_WKUP_CTRL_ENABLE	(1 << 0)
> +#define AM335X_USB1_WKUP_CTRL_ENABLE	(1 << 8)
> +
> +
> +
>   #if IS_ENABLED(CONFIG_OMAP_CONTROL_USB)
>   extern struct device *omap_get_control_dev(void);
>   extern void omap_control_usb_phy_power(struct device *dev, int on);
>   extern void omap_control_usb3_phy_power(struct device *dev, bool on);
> +extern void omap_control_am335x_phy_power(struct device *dev, bool on, u8 id);

You might also need to provide an empty stub when CONFIG_OMAP_CONTROL_USB is
not set.

Thanks
Kishon

[PATCH 3/5] phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform

[George Cherian]

Adds phy driver support for am33xx platform, the host/device
peripheral controller shall get this phy object to control the phy
operations.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/phy/Kconfig          |  12 +++
 drivers/phy/Makefile         |   1 +
 drivers/phy/phy-amxxxx-usb.c | 201 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 214 insertions(+)
 create mode 100644 drivers/phy/phy-amxxxx-usb.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 5f85909..55087ab 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -11,3 +11,15 @@ menuconfig GENERIC_PHY
 	  devices present in the kernel. This layer will have the generic
 	  API by which phy drivers can create PHY using the phy framework and
 	  phy users can obtain reference to the PHY.
+if GENERIC_PHY
+
+config PHY_AMXXXX_USB
+	tristate "AMXXXX USB2 PHY Driver"
+	depends on SOC_AM33XX
+	help
+	  Enable this to support the transceiver that is part of SOC. This
+	  phy supports all LS/FS/HS speed and also supports OTG functionality.
+	  The USB OTG controller communicates with this phy through stand UTMI
+	  interface.
+
+endif
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 9e9560f..471c525 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
+obj-$(CONFIG_PHY_AMXXXX_USB)	+= phy-amxxxx-usb.o
diff --git a/drivers/phy/phy-amxxxx-usb.c b/drivers/phy/phy-amxxxx-usb.c
new file mode 100644
index 0000000..52c48eb
--- /dev/null
+++ b/drivers/phy/phy-amxxxx-usb.c
@@ -0,0 +1,201 @@
+/*
+ * phy-amxxxx-usb.c - USB PHY, talking to usb controller in AMXXX.
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.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.
+ *
+ * Author: George Cherian <george.cherian@ti.com>
+ *
+ * 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/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/usb/phy_companion.h>
+#include <linux/clk.h>
+#include <linux/pm_runtime.h>
+#include <linux/usb/omap_control_usb.h>
+#include <linux/phy/phy.h>
+
+struct amxxxx_usb {
+	struct usb_phy	phy;
+	struct device	*dev;
+	struct device	*control_dev;
+	struct clk	*optclk;
+	u8		id;
+};
+
+#define phy_to_amxxxxphy(x)	container_of((x), struct amxxxx_phy, phy)
+
+
+static int amxxxx_usb_power_off(struct phy *x)
+{
+	struct amxxxx_usb *phy = phy_get_drvdata(x);
+
+	omap_control_am335x_phy_power(phy->control_dev, 0, phy->id);
+
+	return 0;
+}
+
+static int amxxxx_usb_power_on(struct phy *x)
+{
+	struct amxxxx_usb *phy = phy_get_drvdata(x);
+
+	omap_control_am335x_phy_power(phy->control_dev, 1, phy->id);
+
+	return 0;
+}
+
+static struct phy_ops ops = {
+	.power_on	= amxxxx_usb_power_on,
+	.power_off	= amxxxx_usb_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int amxxxx_usb2_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct amxxxx_usb			*phy;
+	struct phy			*generic_phy;
+	struct usb_otg			*otg;
+	struct phy_provider		*phy_provider;
+
+	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
+	if (!phy) {
+		dev_err(&pdev->dev, "unable to allocate memory for USB2 PHY\n");
+		return -ENOMEM;
+	}
+
+	otg = devm_kzalloc(&pdev->dev, sizeof(*otg), GFP_KERNEL);
+	if (!otg) {
+		dev_err(&pdev->dev, "unable to allocate memory for USB OTG\n");
+		return -ENOMEM;
+	}
+
+	phy->dev		= &pdev->dev;
+
+	if (np)
+		of_property_read_u8(np, "id", &phy->id);
+
+	phy->phy.dev		= phy->dev;
+	phy->phy.label		= "amxxxx-usb2";
+	phy->phy.otg		= otg;
+	phy->phy.type		= USB_PHY_TYPE_USB2;
+
+	phy_provider = devm_of_phy_provider_register(phy->dev,
+			of_phy_simple_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	phy->control_dev = omap_get_control_dev();
+	if (IS_ERR(phy->control_dev)) {
+		dev_dbg(&pdev->dev, "Failed to get control device\n");
+		return -ENODEV;
+	}
+
+	omap_control_am335x_phy_power(phy->control_dev, 0, phy->id);
+
+	otg->phy		= &phy->phy;
+
+	platform_set_drvdata(pdev, phy);
+	pm_runtime_enable(phy->dev);
+
+	generic_phy = devm_phy_create(phy->dev, 0, &ops, "amxxxx-usb2");
+
+	if (IS_ERR(generic_phy))
+		return PTR_ERR(generic_phy);
+
+	phy_set_drvdata(generic_phy, phy);
+
+	phy->optclk = devm_clk_get(phy->dev, "usbotg_fck");
+
+	if (IS_ERR(phy->optclk))
+		dev_err(&pdev->dev, "unable to get usbotg_fck\n");
+	else
+		clk_prepare(phy->optclk);
+
+
+	usb_add_phy_dev(&phy->phy);
+
+	return 0;
+}
+
+static int amxxxx_usb2_remove(struct platform_device *pdev)
+{
+	struct amxxxx_usb	*phy = platform_get_drvdata(pdev);
+
+	clk_unprepare(phy->optclk);
+	usb_remove_phy(&phy->phy);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_RUNTIME
+
+static int amxxxx_usb2_runtime_suspend(struct device *dev)
+{
+	struct platform_device	*pdev = to_platform_device(dev);
+	struct amxxxx_usb	*phy = platform_get_drvdata(pdev);
+
+	omap_control_am335x_phy_power(phy->control_dev, 0, phy->id);
+	clk_disable(phy->optclk);
+
+	return 0;
+}
+
+static int amxxxx_usb2_runtime_resume(struct device *dev)
+{
+	struct platform_device	*pdev = to_platform_device(dev);
+	struct amxxxx_usb	*phy = platform_get_drvdata(pdev);
+
+
+	omap_control_am335x_phy_power(phy->control_dev, 1, phy->id);
+	clk_enable(phy->optclk);
+
+	return 0;
+
+}
+
+static const struct dev_pm_ops amxxxx_usb2_pm_ops = {
+	SET_RUNTIME_PM_OPS(amxxxx_usb2_runtime_suspend,
+		amxxxx_usb2_runtime_resume, NULL)
+};
+
+#define DEV_PM_OPS     (&amxxxx_usb2_pm_ops)
+#else
+#define DEV_PM_OPS     NULL
+#endif
+
+#ifdef CONFIG_OF
+static const struct of_device_id amxxxx_usb2_id_table[] = {
+	{ .compatible = "ti,am335x-usb2" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, amxxxx_usb2_id_table);
+#endif
+
+static struct platform_driver amxxxx_usb2_driver = {
+	.probe		= amxxxx_usb2_probe,
+	.remove		= amxxxx_usb2_remove,
+	.driver		= {
+		.name	= "amxxxx-usb2",
+		.owner	= THIS_MODULE,
+		.pm	= DEV_PM_OPS,
+		.of_match_table = of_match_ptr(amxxxx_usb2_id_table),
+	},
+};
+
+module_platform_driver(amxxxx_usb2_driver);
+
+MODULE_ALIAS("platform: amxxxx_usb2");
+MODULE_AUTHOR("Texas Instruments Inc.");
+MODULE_DESCRIPTION("AMXXXX USB2 phy driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.1.4

Re: [PATCH 3/5] phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform

[Kishon Vijay Abraham I]

Hi,

On Monday 08 July 2013 04:13 PM, George Cherian wrote:
> Adds phy driver support for am33xx platform, the host/device
> peripheral controller shall get this phy object to control the phy
> operations.
>
> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>   drivers/phy/Kconfig          |  12 +++
>   drivers/phy/Makefile         |   1 +
>   drivers/phy/phy-amxxxx-usb.c | 201 +++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 214 insertions(+)
>   create mode 100644 drivers/phy/phy-amxxxx-usb.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 5f85909..55087ab 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -11,3 +11,15 @@ menuconfig GENERIC_PHY
>   	  devices present in the kernel. This layer will have the generic
>   	  API by which phy drivers can create PHY using the phy framework and
>   	  phy users can obtain reference to the PHY.
> +if GENERIC_PHY
> +
> +config PHY_AMXXXX_USB
> +	tristate "AMXXXX USB2 PHY Driver"
> +	depends on SOC_AM33XX
> +	help
> +	  Enable this to support the transceiver that is part of SOC. This
> +	  phy supports all LS/FS/HS speed and also supports OTG functionality.
> +	  The USB OTG controller communicates with this phy through stand UTMI
> +	  interface.
> +
> +endif
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 9e9560f..471c525 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -3,3 +3,4 @@
>   #
>
>   obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
> +obj-$(CONFIG_PHY_AMXXXX_USB)	+= phy-amxxxx-usb.o
> diff --git a/drivers/phy/phy-amxxxx-usb.c b/drivers/phy/phy-amxxxx-usb.c
> new file mode 100644
> index 0000000..52c48eb
> --- /dev/null
> +++ b/drivers/phy/phy-amxxxx-usb.c
> @@ -0,0 +1,201 @@
> +/*
> + * phy-amxxxx-usb.c - USB PHY, talking to usb controller in AMXXX.
> + *
> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com

2013 ;-)
> + * 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.
> + *
> + * Author: George Cherian <george.cherian@ti.com>
> + *
> + * 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/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/usb/phy_companion.h>

phy_companion not needed here.
> +#include <linux/clk.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/usb/omap_control_usb.h>
> +#include <linux/phy/phy.h>
> +
> +struct amxxxx_usb {
> +	struct usb_phy	phy;
> +	struct device	*dev;
> +	struct device	*control_dev;
> +	struct clk	*optclk;
> +	u8		id;
> +};
> +
> +#define phy_to_amxxxxphy(x)	container_of((x), struct amxxxx_phy, phy)
> +
> +

Additional blank line.
> +static int amxxxx_usb_power_off(struct phy *x)
> +{
> +	struct amxxxx_usb *phy = phy_get_drvdata(x);
> +
> +	omap_control_am335x_phy_power(phy->control_dev, 0, phy->id);
> +
> +	return 0;
> +}
> +
> +static int amxxxx_usb_power_on(struct phy *x)
> +{
> +	struct amxxxx_usb *phy = phy_get_drvdata(x);
> +
> +	omap_control_am335x_phy_power(phy->control_dev, 1, phy->id);
> +
> +	return 0;
> +}
> +
> +static struct phy_ops ops = {
> +	.power_on	= amxxxx_usb_power_on,
> +	.power_off	= amxxxx_usb_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static int amxxxx_usb2_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct amxxxx_usb			*phy;
> +	struct phy			*generic_phy;
> +	struct usb_otg			*otg;
> +	struct phy_provider		*phy_provider;

Alignment is not needed for local variables.
> +
> +	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
> +	if (!phy) {
> +		dev_err(&pdev->dev, "unable to allocate memory for USB2 PHY\n");

remove this dev_err. memory allocation failures will throw some dumps already.
> +		return -ENOMEM;
> +	}
> +
> +	otg = devm_kzalloc(&pdev->dev, sizeof(*otg), GFP_KERNEL);
> +	if (!otg) {
> +		dev_err(&pdev->dev, "unable to allocate memory for USB OTG\n");

ditto.
> +		return -ENOMEM;
> +	}
> +
> +	phy->dev		= &pdev->dev;
> +
> +	if (np)
> +		of_property_read_u8(np, "id", &phy->id);
> +
> +	phy->phy.dev		= phy->dev;
> +	phy->phy.label		= "amxxxx-usb2";
> +	phy->phy.otg		= otg;
> +	phy->phy.type		= USB_PHY_TYPE_USB2;
> +
> +	phy_provider = devm_of_phy_provider_register(phy->dev,
> +			of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
> +	phy->control_dev = omap_get_control_dev();
> +	if (IS_ERR(phy->control_dev)) {
> +		dev_dbg(&pdev->dev, "Failed to get control device\n");
> +		return -ENODEV;
> +	}
> +
> +	omap_control_am335x_phy_power(phy->control_dev, 0, phy->id);
> +
> +	otg->phy		= &phy->phy;
> +
> +	platform_set_drvdata(pdev, phy);
> +	pm_runtime_enable(phy->dev);
> +
> +	generic_phy = devm_phy_create(phy->dev, 0, &ops, "amxxxx-usb2");
> +
blank line..
> +	if (IS_ERR(generic_phy))
> +		return PTR_ERR(generic_phy);
> +
> +	phy_set_drvdata(generic_phy, phy);
> +
> +	phy->optclk = devm_clk_get(phy->dev, "usbotg_fck");
> +
blank line..
> +	if (IS_ERR(phy->optclk))
> +		dev_err(&pdev->dev, "unable to get usbotg_fck\n");
> +	else
> +		clk_prepare(phy->optclk);
> +
> +
blank line..
> +	usb_add_phy_dev(&phy->phy);

Any reason why you are using the old USB PHY APIs. Because of
MUSB?

Thanks
Kishon

[PATCH 4/5] arm: dts: Add USB phy nodes for AM33XX

[George Cherian]

Add DT bindings of phy nodes for AM33XX platform.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 arch/arm/boot/dts/am33xx.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 8e1248f..4f3cca0 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -326,6 +326,25 @@
 			status = "disabled";
 		};
 
+		phy1: usbphy-gs70@44e10620 {
+			compatible = "ti,am335x-usb2";
+			#phy-cells = <0>;
+			id = <0>;
+		};
+
+		phy2: usbphy-gs71@44e10628 {
+			compatible = "ti,am335x-usb2";
+			#phy-cells = <0>;
+			id = <1>;
+		};
+
+		omap_control_usb: omap-control-usb@44e10620 {
+			compatible = "ti,omap-control-usb";
+			reg = <0x44e10620 0x10>;
+			reg-names = "control_dev_conf";
+			ti,type = <3>;
+		};
+
 		usb@47400000 {
 			compatible = "ti,musb-am33xx";
 			reg = <0x47400000 0x1000	/* usbss */
@@ -341,6 +360,9 @@
 			port1-mode = <3>;
 			power = <250>;
 			ti,hwmods = "usb_otg_hs";
+			phy-names = "am335x-usb0", "am335x-usb1";
+			phys	= <&phy1>, <&phy2>;
+
 		};
 
 		mac: ethernet@4a100000 {
-- 
1.8.1.4

Re: [PATCH 4/5] arm: dts: Add USB phy nodes for AM33XX

[Kishon Vijay Abraham I]

Hi,

On Monday 08 July 2013 04:13 PM, George Cherian wrote:
> Add DT bindings of phy nodes for AM33XX platform.
>
> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>   arch/arm/boot/dts/am33xx.dtsi | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index 8e1248f..4f3cca0 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -326,6 +326,25 @@
>   			status = "disabled";
>   		};
>
> +		phy1: usbphy-gs70@44e10620 {

*44e10620* is not needed if you are not having the reg property.
> +			compatible = "ti,am335x-usb2";
> +			#phy-cells = <0>;
> +			id = <0>;
> +		};
> +
> +		phy2: usbphy-gs71@44e10628 {
ditto.
> +			compatible = "ti,am335x-usb2";
> +			#phy-cells = <0>;
> +			id = <1>;
> +		};
> +
> +		omap_control_usb: omap-control-usb@44e10620 {
> +			compatible = "ti,omap-control-usb";
> +			reg = <0x44e10620 0x10>;
> +			reg-names = "control_dev_conf";
> +			ti,type = <3>;
> +		};
> +
>   		usb@47400000 {
>   			compatible = "ti,musb-am33xx";
>   			reg = <0x47400000 0x1000	/* usbss */
> @@ -341,6 +360,9 @@
>   			port1-mode = <3>;
>   			power = <250>;
>   			ti,hwmods = "usb_otg_hs";
> +			phy-names = "am335x-usb0", "am335x-usb1";
> +			phys	= <&phy1>, <&phy2>;

Lets have *phys* before *phy-names*.

Thanks
Kishon

[PATCH 5/5] usb: musb: dsps: Remove the phy control from glue and add phy driver APIs

[George Cherian]

Remove usb phy control module access from platform glue.
The same is now done using amxxxx phy driver and phy-omap-control.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/usb/musb/musb_dsps.c | 105 +++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 69 deletions(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 41f135a..8f4e04a 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -123,49 +123,11 @@ struct dsps_glue {
 	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
 	struct timer_list timer[2];	/* otg_workaround timer */
 	unsigned long last_timer[2];    /* last timer data for each instance */
-	u32 __iomem *usb_ctrl[2];
 };
 
-#define	DSPS_AM33XX_CONTROL_MODULE_PHYS_0	0x44e10620
-#define	DSPS_AM33XX_CONTROL_MODULE_PHYS_1	0x44e10628
+#define glue_to_musb(g, i)		platform_get_drvdata(g->musb[i])
 
-static const resource_size_t dsps_control_module_phys[] = {
-	DSPS_AM33XX_CONTROL_MODULE_PHYS_0,
-	DSPS_AM33XX_CONTROL_MODULE_PHYS_1,
-};
-
-#define USBPHY_CM_PWRDN		(1 << 0)
-#define USBPHY_OTG_PWRDN	(1 << 1)
-#define USBPHY_OTGVDET_EN	(1 << 19)
-#define USBPHY_OTGSESSEND_EN	(1 << 20)
-
-/**
- * musb_dsps_phy_control - phy on/off
- * @glue: struct dsps_glue *
- * @id: musb instance
- * @on: flag for phy to be switched on or off
- *
- * This is to enable the PHY using usb_ctrl register in system control
- * module space.
- *
- * XXX: This function will be removed once we have a seperate driver for
- * control module
- */
-static void musb_dsps_phy_control(struct dsps_glue *glue, u8 id, u8 on)
-{
-	u32 usbphycfg;
 
-	usbphycfg = readl(glue->usb_ctrl[id]);
-
-	if (on) {
-		usbphycfg &= ~(USBPHY_CM_PWRDN | USBPHY_OTG_PWRDN);
-		usbphycfg |= USBPHY_OTGVDET_EN | USBPHY_OTGSESSEND_EN;
-	} else {
-		usbphycfg |= USBPHY_CM_PWRDN | USBPHY_OTG_PWRDN;
-	}
-
-	writel(usbphycfg, glue->usb_ctrl[id]);
-}
 /**
  * dsps_musb_enable - enable interrupts
  */
@@ -411,16 +373,31 @@ static int dsps_musb_init(struct musb *musb)
 	void __iomem *reg_base = musb->ctrl_base;
 	u32 rev, val;
 	int status;
+	char phyname[12];
 
 	/* mentor core register starts at offset of 0x400 from musb base */
 	musb->mregs += wrp->musb_core_offset;
 
-	/* NOP driver needs change if supporting dual instance */
-	usb_nop_xceiv_register();
-	musb->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
+	if (dev->parent->of_node) {
+		snprintf(phyname , 12, "am335x-usb%d", pdev->id);
+		musb->phy = devm_phy_get(dev->parent, phyname);
+		musb->xceiv = devm_usb_get_phy_by_phandle(dev->parent,
+							"phys", pdev->id);
+	} else {
+		snprintf(phyname , 12, "am335x-usb%d", pdev->id);
+		musb->phy = devm_phy_get(dev, musb->phy_label);
+		musb->xceiv = devm_usb_get_phy_dev(dev, pdev->id);
+	}
+
 	if (IS_ERR_OR_NULL(musb->xceiv))
 		return -EPROBE_DEFER;
 
+	if (IS_ERR_OR_NULL(musb->phy)) {
+		pr_err("no PHY configured\n");
+		return -EPROBE_DEFER;
+	}
+
+
 	/* Returns zero if e.g. not clocked */
 	rev = dsps_readl(reg_base, wrp->revision);
 	if (!rev) {
@@ -434,7 +411,8 @@ static int dsps_musb_init(struct musb *musb)
 	dsps_writel(reg_base, wrp->control, (1 << wrp->reset));
 
 	/* Start the on-chip PHY and its PLL. */
-	musb_dsps_phy_control(glue, pdev->id, 1);
+	phy_init(musb->phy);
+	phy_power_on(musb->phy);
 
 	musb->isr = dsps_interrupt;
 
@@ -448,8 +426,7 @@ static int dsps_musb_init(struct musb *musb)
 
 	return 0;
 err0:
-	usb_put_phy(musb->xceiv);
-	usb_nop_xceiv_unregister();
+	devm_phy_put(dev->parent, musb->phy);
 	return status;
 }
 
@@ -462,11 +439,8 @@ static int dsps_musb_exit(struct musb *musb)
 	del_timer_sync(&glue->timer[pdev->id]);
 
 	/* Shutdown the on-chip PHY and its PLL. */
-	musb_dsps_phy_control(glue, pdev->id, 0);
-
-	/* NOP driver needs change if supporting dual instance */
-	usb_put_phy(musb->xceiv);
-	usb_nop_xceiv_unregister();
+	phy_power_off(musb->phy);
+	phy_exit(musb->phy);
 
 	return 0;
 }
@@ -496,16 +470,6 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue, u8 id)
 	char res_name[11];
 	int ret;
 
-	resources[0].start = dsps_control_module_phys[id];
-	resources[0].end = resources[0].start + SZ_4 - 1;
-	resources[0].flags = IORESOURCE_MEM;
-
-	glue->usb_ctrl[id] = devm_ioremap_resource(&pdev->dev, resources);
-	if (IS_ERR(glue->usb_ctrl[id])) {
-		ret = PTR_ERR(glue->usb_ctrl[id]);
-		goto err0;
-	}
-
 	/* first resource is for usbss, so start index from 1 */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, id + 1);
 	if (!res) {
@@ -692,26 +656,29 @@ static int dsps_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM_SLEEP
 static int dsps_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev->parent);
-	struct dsps_glue *glue = platform_get_drvdata(pdev);
+	struct dsps_glue *glue = dev_get_drvdata(dev);
 	const struct dsps_musb_wrapper *wrp = glue->wrp;
+	struct musb			*musb;
 	int i;
 
-	for (i = 0; i < wrp->instances; i++)
-		musb_dsps_phy_control(glue, i, 0);
-
+	for (i = 0; i < wrp->instances; i++) {
+		musb = glue_to_musb(glue, i);
+		phy_power_off(musb->phy);
+	}
 	return 0;
 }
 
 static int dsps_resume(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev->parent);
-	struct dsps_glue *glue = platform_get_drvdata(pdev);
+	struct dsps_glue *glue = dev_get_drvdata(dev);
 	const struct dsps_musb_wrapper *wrp = glue->wrp;
+	struct musb			*musb;
 	int i;
 
-	for (i = 0; i < wrp->instances; i++)
-		musb_dsps_phy_control(glue, i, 1);
+	for (i = 0; i < wrp->instances; i++) {
+		musb = glue_to_musb(glue, i);
+		phy_power_on(musb->phy);
+	}
 
 	return 0;
 }
-- 
1.8.1.4

Re: [PATCH 5/5] usb: musb: dsps: Remove the phy control from glue and add phy driver APIs

[Kishon Vijay Abraham I]

Hi,

On Monday 08 July 2013 04:13 PM, George Cherian wrote:
> Remove usb phy control module access from platform glue.
> The same is now done using amxxxx phy driver and phy-omap-control.
>
> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>   drivers/usb/musb/musb_dsps.c | 105 +++++++++++++++----------------------------
>   1 file changed, 36 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index 41f135a..8f4e04a 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -123,49 +123,11 @@ struct dsps_glue {
>   	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
>   	struct timer_list timer[2];	/* otg_workaround timer */
>   	unsigned long last_timer[2];    /* last timer data for each instance */
> -	u32 __iomem *usb_ctrl[2];
>   };
>
> -#define	DSPS_AM33XX_CONTROL_MODULE_PHYS_0	0x44e10620
> -#define	DSPS_AM33XX_CONTROL_MODULE_PHYS_1	0x44e10628
> +#define glue_to_musb(g, i)		platform_get_drvdata(g->musb[i])
>
> -static const resource_size_t dsps_control_module_phys[] = {
> -	DSPS_AM33XX_CONTROL_MODULE_PHYS_0,
> -	DSPS_AM33XX_CONTROL_MODULE_PHYS_1,
> -};
> -
> -#define USBPHY_CM_PWRDN		(1 << 0)
> -#define USBPHY_OTG_PWRDN	(1 << 1)
> -#define USBPHY_OTGVDET_EN	(1 << 19)
> -#define USBPHY_OTGSESSEND_EN	(1 << 20)
> -
> -/**
> - * musb_dsps_phy_control - phy on/off
> - * @glue: struct dsps_glue *
> - * @id: musb instance
> - * @on: flag for phy to be switched on or off
> - *
> - * This is to enable the PHY using usb_ctrl register in system control
> - * module space.
> - *
> - * XXX: This function will be removed once we have a seperate driver for
> - * control module
> - */
> -static void musb_dsps_phy_control(struct dsps_glue *glue, u8 id, u8 on)
> -{
> -	u32 usbphycfg;
>
> -	usbphycfg = readl(glue->usb_ctrl[id]);
> -
> -	if (on) {
> -		usbphycfg &= ~(USBPHY_CM_PWRDN | USBPHY_OTG_PWRDN);
> -		usbphycfg |= USBPHY_OTGVDET_EN | USBPHY_OTGSESSEND_EN;
> -	} else {
> -		usbphycfg |= USBPHY_CM_PWRDN | USBPHY_OTG_PWRDN;
> -	}
> -
> -	writel(usbphycfg, glue->usb_ctrl[id]);
> -}
>   /**
>    * dsps_musb_enable - enable interrupts
>    */
> @@ -411,16 +373,31 @@ static int dsps_musb_init(struct musb *musb)
>   	void __iomem *reg_base = musb->ctrl_base;
>   	u32 rev, val;
>   	int status;
> +	char phyname[12];
>
>   	/* mentor core register starts at offset of 0x400 from musb base */
>   	musb->mregs += wrp->musb_core_offset;
>
> -	/* NOP driver needs change if supporting dual instance */
> -	usb_nop_xceiv_register();
> -	musb->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
> +	if (dev->parent->of_node) {
> +		snprintf(phyname , 12, "am335x-usb%d", pdev->id);
> +		musb->phy = devm_phy_get(dev->parent, phyname);
> +		musb->xceiv = devm_usb_get_phy_by_phandle(dev->parent,
> +							"phys", pdev->id);
> +	} else {
> +		snprintf(phyname , 12, "am335x-usb%d", pdev->id);

Both these snprintf can be moved out of the *if*.
> +		musb->phy = devm_phy_get(dev, musb->phy_label);
> +		musb->xceiv = devm_usb_get_phy_dev(dev, pdev->id);
> +	}
> +
>   	if (IS_ERR_OR_NULL(musb->xceiv))
>   		return -EPROBE_DEFER;
>
> +	if (IS_ERR_OR_NULL(musb->phy)) {

The framework should never return NULL.

Thanks
Kishon

Re: [PATCH 0/5] Add phy support for AM335X platform using Generic PHy framework

[Sebastian Andrzej Siewior]

On 07/08/2013 12:43 PM, George Cherian wrote:
> This patch series adds phy support for AM335X platform.
> This patch series is based on Generic PHY framework [1]. 
> 
> 
> This series has
> 	- adds dual musb instances support for am335x platform (just for testing)
> 	- adds phy-amxxxx-usb driver used in AMxxxx platforms
> 	- adds dt  bindings for the phys
> 	- removes usb-phy and replaced with generic phy apis in glue layer

No, I don't like this all. You did the one thing I tried to avoid while
posting my quick-and-dirty phy driver recently: You duplicated a lot of
code which can be served by the nop driver and added only power
on/power off callbacks.
In numbers:
>  7 files changed, 316 insertions(+), 70 deletions(-)
vs
   2 files changed, 117 insertions, 12 deletions

I assumed you had also the OTG callbacks (set host/device mode) and
wake up but I don't see it there.
Adding a power regulator would do the same job, wouldn't it? If the phy
driver remains just doing power on/off I suggest simply adding a power
regulator. If it will do more I would move the am35xx specific bits
into a separate file and glue it to the nop driver.

What else? The abstraction in device tree is wrong. It remains wrong if
add stuff on top to it.
We need two nodes each one with a glue layer and a musb child node. The
instances crap in kernel has to vanish. Also that means your phy nodes
are wrong. This is not musb with two ports but two musb instances each
with one port.

Sebastian

Re: [PATCH 0/5] Add phy support for AM335X platform using Generic PHy framework

[Ezequiel Garcia]

Hi,

On Mon, Jul 08, 2013 at 09:44:33PM +0200, Sebastian Andrzej Siewior wrote:

> We need two nodes each one with a glue layer and a musb child node. The
> instances crap in kernel has to vanish. Also that means your phy nodes
> are wrong. This is not musb with two ports but two musb instances each
> with one port.
> 

I agree completely. The current DT representation looks definitely odd,
and we should be looking at improving it.

I wonder if this is now possible, given the DT is supposed to be stable ABI.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Add phy support for AM335X platform using Generic PHy framework

[Sebastian Andrzej Siewior]

On 07/08/2013 10:34 PM, Ezequiel Garcia wrote:
> Hi,

Hi,

> On Mon, Jul 08, 2013 at 09:44:33PM +0200, Sebastian Andrzej Siewior wrote:
> 
>> We need two nodes each one with a glue layer and a musb child node. The
>> instances crap in kernel has to vanish. Also that means your phy nodes
>> are wrong. This is not musb with two ports but two musb instances each
>> with one port.
>>
> 
> I agree completely. The current DT representation looks definitely odd,
> and we should be looking at improving it.
> 
> I wonder if this is now possible, given the DT is supposed to be stable ABI.

I posted this [0] and Felipe + Benoit were pro change. I am still not
sure if this is okay or just one glue layer per instance so I delay
this until I am sure. Stable or not, what currently have in is beyond
broken and it can't be fixed in kernel.
I would add some code to check for the old nodes and give a proper
warning and how to react. This should reduce the pain full search.

[0]
http://git.breakpoint.cc/cgit.cgi/bigeasy/linux.git/commit/?h=am335x_usb&id=0a60cd77ee50edd8cd07cbd699ed67b2c4b2ab93

Sebastian

Re: [PATCH 0/5] Add phy support for AM335X platform using Generic PHy framework

[George Cherian]

On 7/9/2013 1:14 AM, Sebastian Andrzej Siewior wrote:
> On 07/08/2013 12:43 PM, George Cherian wrote:
>> This patch series adds phy support for AM335X platform.
>> This patch series is based on Generic PHY framework [1].
>>
>>
>> This series has
>> 	- adds dual musb instances support for am335x platform (just for testing)
>> 	- adds phy-amxxxx-usb driver used in AMxxxx platforms
>> 	- adds dt  bindings for the phys
>> 	- removes usb-phy and replaced with generic phy apis in glue layer
> No, I don't like this all. You did the one thing I tried to avoid while
> posting my quick-and-dirty phy driver recently: You duplicated a lot of
> code which can be served by the nop driver and added only power
> on/power off callbacks.
I wanted to add phy wakeup control also, but currently phy_ops  dont 
have an op for wkup_ctrl
Kishon, Can we add one?
> In numbers:
>>   7 files changed, 316 insertions(+), 70 deletions(-)
> vs
>     2 files changed, 117 insertions, 12 deletions
>
> I assumed you had also the OTG callbacks (set host/device mode) and
> wake up but I don't see it there.
> Adding a power regulator would do the same job, wouldn't it? If the phy
> driver remains just doing power on/off I suggest simply adding a power
> regulator. If it will do more I would move the am35xx specific bits
> into a separate file and glue it to the nop driver.
>
> What else? The abstraction in device tree is wrong. It remains wrong if
> add stuff on top to it.
Yes definitely , I liked the way you split the device node. Probably I 
should base on your dt patch and add
phy nodes.
> We need two nodes each one with a glue layer and a musb child node. The
> instances crap in kernel has to vanish.
Same as above  I just did it for quick testing (mentioned in the commit 
log).
> Also that means your phy nodes
> are wrong. This is not musb with two ports but two musb instances each
> with one port.
yes true.
> Sebastian

Re: [PATCH 0/5] Add phy support for AM335X platform using Generic PHy framework

[Kishon Vijay Abraham I]

Hi,

On Tuesday 09 July 2013 11:10 AM, George Cherian wrote:
> On 7/9/2013 1:14 AM, Sebastian Andrzej Siewior wrote:
>> On 07/08/2013 12:43 PM, George Cherian wrote:
>>> This patch series adds phy support for AM335X platform.
>>> This patch series is based on Generic PHY framework [1].
>>>
>>>
>>> This series has
>>>     - adds dual musb instances support for am335x platform (just for testing)
>>>     - adds phy-amxxxx-usb driver used in AMxxxx platforms
>>>     - adds dt  bindings for the phys
>>>     - removes usb-phy and replaced with generic phy apis in glue layer
>> No, I don't like this all. You did the one thing I tried to avoid while
>> posting my quick-and-dirty phy driver recently: You duplicated a lot of
>> code which can be served by the nop driver and added only power
>> on/power off callbacks.
> I wanted to add phy wakeup control also, but currently phy_ops  dont have an op
> for wkup_ctrl
> Kishon, Can we add one?

Since this should be a capability of the PHY, can't we have wkup_ctrl always
enabled if the PHY has such a capability? or if it needs more user control,
should we implement a sysfs entry to enable wakeup?

Thanks
Kishon

Re: [PATCH 0/5] Add phy support for AM335X platform using Generic PHy framework

[George Cherian]

On 7/9/2013 5:05 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 09 July 2013 11:10 AM, George Cherian wrote:
>> On 7/9/2013 1:14 AM, Sebastian Andrzej Siewior wrote:
>>> On 07/08/2013 12:43 PM, George Cherian wrote:
>>>> This patch series adds phy support for AM335X platform.
>>>> This patch series is based on Generic PHY framework [1].
>>>>
>>>>
>>>> This series has
>>>>      - adds dual musb instances support for am335x platform (just for testing)
>>>>      - adds phy-amxxxx-usb driver used in AMxxxx platforms
>>>>      - adds dt  bindings for the phys
>>>>      - removes usb-phy and replaced with generic phy apis in glue layer
>>> No, I don't like this all. You did the one thing I tried to avoid while
>>> posting my quick-and-dirty phy driver recently: You duplicated a lot of
>>> code which can be served by the nop driver and added only power
>>> on/power off callbacks.
>> I wanted to add phy wakeup control also, but currently phy_ops  dont have an op
>> for wkup_ctrl
>> Kishon, Can we add one?
> Since this should be a capability of the PHY, can't we have wkup_ctrl always
> enabled if the PHY has such a capability?

No, we cant have wakeup always enabled. Normally we enable it only when 
we go to low power states and
if the user needs USB a wakeup source.

So how about enable/disable  phy wakeup from phy suspend/resume?
> or if it needs more user control,
> should we implement a sysfs entry to enable wakeup?

Nope, I dont want to create another sysfs entry.
>
> Thanks
> Kishon


-- 
-George

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Add phy support for AM335X platform using Generic PHy framework

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]

On Wed, Jul 10, 2013 at 10:26:25AM +0530, George Cherian wrote:
> On 7/9/2013 5:05 PM, Kishon Vijay Abraham I wrote:
> >Hi,
> >
> >On Tuesday 09 July 2013 11:10 AM, George Cherian wrote:
> >>On 7/9/2013 1:14 AM, Sebastian Andrzej Siewior wrote:
> >>>On 07/08/2013 12:43 PM, George Cherian wrote:
> >>>>This patch series adds phy support for AM335X platform.
> >>>>This patch series is based on Generic PHY framework [1].
> >>>>
> >>>>
> >>>>This series has
> >>>>     - adds dual musb instances support for am335x platform (just for testing)
> >>>>     - adds phy-amxxxx-usb driver used in AMxxxx platforms
> >>>>     - adds dt  bindings for the phys
> >>>>     - removes usb-phy and replaced with generic phy apis in glue layer
> >>>No, I don't like this all. You did the one thing I tried to avoid while
> >>>posting my quick-and-dirty phy driver recently: You duplicated a lot of
> >>>code which can be served by the nop driver and added only power
> >>>on/power off callbacks.
> >>I wanted to add phy wakeup control also, but currently phy_ops  dont have an op
> >>for wkup_ctrl
> >>Kishon, Can we add one?
> >Since this should be a capability of the PHY, can't we have wkup_ctrl always
> >enabled if the PHY has such a capability?
> 
> No, we cant have wakeup always enabled. Normally we enable it only
> when we go to low power states and
> if the user needs USB a wakeup source.
> 
> So how about enable/disable  phy wakeup from phy suspend/resume?

you should use something like so on your ->suspend() or
->runtime_suspend() method

static int my_phy_{suspend,runtime_suspend}(struct device *dev)
{
	struct my_phy *phy = dev_get_drvdata(dev);

	if (device_may_wakeup(dev))
		my_phy_enable_wakeup(phy);

	return 0;
}

> >or if it needs more user control,
> >should we implement a sysfs entry to enable wakeup?

that already exists ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/5] Add phy support for AM335X platform using Generic PHy framework

[George Cherian]

On 7/10/2013 10:53 AM, Felipe Balbi wrote:
> On Wed, Jul 10, 2013 at 10:26:25AM +0530, George Cherian wrote:
>> On 7/9/2013 5:05 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Tuesday 09 July 2013 11:10 AM, George Cherian wrote:
>>>> On 7/9/2013 1:14 AM, Sebastian Andrzej Siewior wrote:
>>>>> On 07/08/2013 12:43 PM, George Cherian wrote:
>>>>>> This patch series adds phy support for AM335X platform.
>>>>>> This patch series is based on Generic PHY framework [1].
>>>>>>
>>>>>>
>>>>>> This series has
>>>>>>      - adds dual musb instances support for am335x platform (just for testing)
>>>>>>      - adds phy-amxxxx-usb driver used in AMxxxx platforms
>>>>>>      - adds dt  bindings for the phys
>>>>>>      - removes usb-phy and replaced with generic phy apis in glue layer
>>>>> No, I don't like this all. You did the one thing I tried to avoid while
>>>>> posting my quick-and-dirty phy driver recently: You duplicated a lot of
>>>>> code which can be served by the nop driver and added only power
>>>>> on/power off callbacks.
>>>> I wanted to add phy wakeup control also, but currently phy_ops  dont have an op
>>>> for wkup_ctrl
>>>> Kishon, Can we add one?
>>> Since this should be a capability of the PHY, can't we have wkup_ctrl always
>>> enabled if the PHY has such a capability?
>> No, we cant have wakeup always enabled. Normally we enable it only
>> when we go to low power states and
>> if the user needs USB a wakeup source.
>>
>> So how about enable/disable  phy wakeup from phy suspend/resume?
> you should use something like so on your ->suspend() or
> ->runtime_suspend() method
>
> static int my_phy_{suspend,runtime_suspend}(struct device *dev)
> {
> 	struct my_phy *phy = dev_get_drvdata(dev);
>
> 	if (device_may_wakeup(dev))
> 		my_phy_enable_wakeup(phy);
>
> 	return 0;
> }

Makes sense. will do it in v2.
>>> or if it needs more user control,
>>> should we implement a sysfs entry to enable wakeup?
> that already exists ;-)
>


-- 
-George