LinuxLists.cc - [PATCH v2 0/4] Add phy support for AM335X platform using Generic PHy framework

[permalink] [raw]

Subject: [PATCH v2 1/4] usb: phy: phy-omap-control: Add API to power and wakeup

on/off USB PHY for AM335X

Add APIs to
-power on/off USB PHY for AM335X
-enable/disable PHY wakeup.

This API will be called from phy-amxxxx-usb driver.

Signed-off-by: George Cherian <[email protected]>
---
drivers/usb/phy/phy-omap-control.c | 67 ++++++++++++++++++++++++++++++++++++
include/linux/usb/omap_control_usb.h | 24 +++++++++++++
2 files changed, 91 insertions(+)

diff --git a/drivers/usb/phy/phy-omap-control.c b/drivers/usb/phy/phy-omap-control.c
index 1419ced..4f2502c 100644
--- a/drivers/usb/phy/phy-omap-control.c
+++ b/drivers/usb/phy/phy-omap-control.c
@@ -46,6 +46,73 @@ struct device *omap_get_control_dev(void)
EXPORT_SYMBOL_GPL(omap_get_control_dev);

/**
+ * omap_control_am335x_phy_wkup - PHY wakeup on/off using control
+ * module
+ * @dev: the control module device
+ * @on: 0 to off and 1 to on PHY wakeup feature
+ * @id: phy id 0 or 1 for phy instance 0 and 1 repectively
+ *
+ * AMXXXX PHY driver should call this API to enable or disable PHY wakeup.
+ */
+void omap_control_am335x_phy_wkup(struct device *dev, bool on, u8 id)
+{
+ u32 val;
+ u32 *phy_wkup_reg;
+ struct omap_control_usb *control_usb = dev_get_drvdata(dev);
+
+ if (control_usb->type != OMAP_CTRL_DEV_TYPE3 || id < 0 || id > 1)
+ return;
+
+ phy_wkup_reg = control_usb->dev_conf + AM335X_USB_WKUP_OFFSET;
+ val = readl(phy_wkup_reg);
+
+ if (on)
+ val |= id ? AM335X_USB1_WKUP_CTRL_ENABLE :
+ AM335X_USB0_WKUP_CTRL_ENABLE;
+ else
+ val &= id ? ~AM335X_USB1_WKUP_CTRL_ENABLE :
+ ~AM335X_USB0_WKUP_CTRL_ENABLE;
+
+
+ writel(val, phy_wkup_reg);
+}
+EXPORT_SYMBOL_GPL(omap_control_am335x_phy_wkup);
+
+/**
+ * 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;
+ u32 *phy_pwr_reg;
+ struct omap_control_usb *control_usb = dev_get_drvdata(dev);
+
+ if (control_usb->type != OMAP_CTRL_DEV_TYPE3 || id < 0 || id > 1)
+ return;
+
+ phy_pwr_reg = GET_PHY_POWER_REG(control_usb->dev_conf, id);
+ val = readl(phy_pwr_reg);
+
+ 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, phy_pwr_reg);
+}
+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..a92b417 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,23 @@ enum omap_control_usb_mode {
#define OMAP_CTRL_USB3_PHY_TX_RX_POWERON 0x3
#define OMAP_CTRL_USB3_PHY_TX_RX_POWEROFF 0x0

+#define GET_PHY_POWER_REG(base, id) ((u32 *)base + (id * 2))
+#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_USB_WKUP_OFFSET 0xA
+#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_am335x_phy_wkup(struct device *dev, bool on, u8 id);
extern void omap_control_usb_set_mode(struct device *dev,
enum omap_control_usb_mode mode);
#else
@@ -87,6 +101,16 @@ static inline void omap_control_usb_set_mode(struct device *dev,
enum omap_control_usb_mode mode)
{
}
+
+static inline void omap_control_am335x_phy_power(struct device *dev,
+ bool on, u8 id)
+{
+}
+
+static inline void omap_control_am335x_phy_wkup(struct device *dev,
+ bool on, u8 id)
+{
+}
#endif

#endif /* __OMAP_CONTROL_USB_H__ */
--
1.8.1.4

2013-07-19 12:35:12

[permalink] [raw]

Subject: [PATCH v2 3/4] arm: dts: Add USB phy nodes for AM33XX

Add phy nodes for AM33XX platform and split the musb nodes
per instance.

Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
Signed-off-by: George Cherian <[email protected]>
---
arch/arm/boot/dts/am33xx.dtsi | 68 +++++++++++++++++++++++++++++++++----------
1 file changed, 53 insertions(+), 15 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 8e1248f..e3890c4 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -326,21 +326,59 @@
status = "disabled";
};

- usb@47400000 {
- compatible = "ti,musb-am33xx";
- reg = <0x47400000 0x1000 /* usbss */
- 0x47401000 0x800 /* musb instance 0 */
- 0x47401800 0x800>; /* musb instance 1 */
- interrupts = <17 /* usbss */
- 18 /* musb instance 0 */
- 19>; /* musb instance 1 */
- multipoint = <1>;
- num-eps = <16>;
- ram-bits = <12>;
- port0-mode = <3>;
- port1-mode = <3>;
- power = <250>;
- ti,hwmods = "usb_otg_hs";
+ phy1: am335x-usb0@44e10620 {
+ compatible = "ti,am335x-usb2";
+ #phy-cells = <0>;
+ id= <0>;
+ };
+
+ phy2: am335x-usb1@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>;
+ };
+
+ musb: usb@47400000 {
+ compatible = "ti,musb-am33xx";
+ reg = <0x47400000 0x1000>;
+ ranges;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ interrupts = <17>;
+ ti,hwmods = "usb_otg_hs";
+
+ usb0@47401000 {
+ reg = <0x47401000 0x800>;
+ interrupts = <18>;
+ interrupt-names = "mc";
+ multipoint = <1>;
+ num-eps = <16>;
+ ram-bits = <12>;
+ port-mode = <3>;
+ power = <250>;
+ phys = <&phy1>;
+ phy-names = "am335x-usb0";
+ };
+
+ usb1@47401800 {
+ reg = <0x47401800 0x800>;
+ interrupts = <19>;
+ interrupt-names = "mc";
+ multipoint = <1>;
+ num-eps = <16>;
+ ram-bits = <12>;
+ port-mode = <3>;
+ power = <250>;
+ phys = <&phy2>;
+ phy-names = "am335x-usb1";
+ };
};

mac: ethernet@4a100000 {
--
1.8.1.4

2013-07-19 12:35:01

[permalink] [raw]

Subject: [PATCH v2 2/4] phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform

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 <[email protected]>
---
drivers/phy/Kconfig | 12 +++
drivers/phy/Makefile | 1 +
drivers/phy/phy-amxxxx-usb.c | 222 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 235 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..d0b0749
--- /dev/null
+++ b/drivers/phy/phy-amxxxx-usb.c
@@ -0,0 +1,222 @@
+/*
+ * phy-amxxxx-usb.c - USB PHY, talking to usb controller in AMXXX.
+ *
+ * Copyright (C) 2013 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 <[email protected]>
+ *
+ * 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/otg.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;
+ u32 id;
+};
+
+#define phy_to_amxxxxphy(x) container_of((x), struct amxxxx_phy, phy)
+
+
+static int amxxxx_usb_set_host(struct usb_otg *otg, struct usb_bus *host)
+{
+ struct usb_phy *phy = otg->phy;
+
+ otg->host = host;
+ if (!host)
+ phy->state = OTG_STATE_UNDEFINED;
+
+ return 0;
+}
+
+static int amxxxx_usb_set_peripheral(struct usb_otg *otg,
+ struct usb_gadget *gadget)
+{
+ struct usb_phy *phy = otg->phy;
+
+ otg->gadget = gadget;
+ if (!gadget)
+ phy->state = OTG_STATE_UNDEFINED;
+
+ return 0;
+}
+
+
+
+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);
+
+ otg = devm_kzalloc(&pdev->dev, sizeof(*otg), GFP_KERNEL);
+
+ phy->dev = &pdev->dev;
+
+ if (np)
+ of_property_read_u32(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->set_host = amxxxx_usb_set_host;
+ otg->set_peripheral = amxxxx_usb_set_peripheral;
+ 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);
+
+ device_init_wakeup(&pdev->dev, true);
+ 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);
+ if (device_may_wakeup(dev))
+ omap_control_am335x_phy_wkup(phy->control_dev, 1, 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);
+ if (device_may_wakeup(dev))
+ omap_control_am335x_phy_wkup(phy->control_dev, 0, 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

2013-07-19 12:35:29

[permalink] [raw]

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

Remove usb phy control module access from platform glue.
The same is now done using amxxxx phy driver and phy-omap-control.
Adapt the driver to the split dt nodes.

Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
Signed-off-by: George Cherian <[email protected]>
---
drivers/usb/musb/musb_dsps.c | 150 ++++++++++++++++---------------------------
1 file changed, 55 insertions(+), 95 deletions(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 41f135a..6bec458 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -43,6 +43,7 @@
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/of_address.h>
+#include <linux/of_irq.h>

#include "musb_core.h"

@@ -110,8 +111,6 @@ struct dsps_musb_wrapper {
/* miscellaneous stuff */
u32 musb_core_offset;
u8 poll_seconds;
- /* number of musb instances */
- u8 instances;
};

/**
@@ -123,49 +122,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 +372,28 @@ 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);
+ snprintf(phyname , 12, "am335x-usb%d", pdev->id);
+
+ if (dev->of_node) {
+ musb->phy = devm_phy_get(dev, phyname);
+ musb->xceiv = devm_usb_get_phy_by_phandle(dev,
+ "phys", 0);
+ } else {
+ 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))
+ return -EPROBE_DEFER;
+
/* Returns zero if e.g. not clocked */
rev = dsps_readl(reg_base, wrp->revision);
if (!rev) {
@@ -434,7 +407,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 +422,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 +435,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;
}
@@ -483,49 +453,34 @@ static struct musb_platform_ops dsps_ops = {

static u64 musb_dmamask = DMA_BIT_MASK(32);

-static int dsps_create_musb_pdev(struct dsps_glue *glue, u8 id)
+static int dsps_create_musb_pdev(struct dsps_glue *glue, u8 id,
+ struct device_node *np)
{
struct device *dev = glue->dev;
struct platform_device *pdev = to_platform_device(dev);
struct musb_hdrc_platform_data *pdata = dev->platform_data;
- struct device_node *np = pdev->dev.of_node;
struct musb_hdrc_config *config;
struct platform_device *musb;
- struct resource *res;
struct resource resources[2];
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) {
+ ret = of_address_to_resource(np, 0, &resources[0]);
+ if (ret) {
dev_err(dev, "failed to get memory for instance %d\n", id);
- ret = -ENODEV;
goto err0;
}
- res->parent = NULL;
- resources[0] = *res;

/* first resource is for usbss, so start index from 1 */
- res = platform_get_resource(pdev, IORESOURCE_IRQ, id + 1);
- if (!res) {
+ ret = of_irq_to_resource(np, 0, &resources[1]);
+ if (ret == 0) {
dev_err(dev, "failed to get irq for instance %d\n", id);
- ret = -ENODEV;
+ ret = -EINVAL;
goto err0;
}
- res->parent = NULL;
- resources[1] = *res;
- resources[1].name = "mc";
+ resources[0].parent = NULL;
+ resources[1].parent = NULL;
+

/* allocate the child platform device */
musb = platform_device_alloc("musb-hdrc", PLATFORM_DEVID_AUTO);
@@ -538,6 +493,7 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue, u8 id)
musb->dev.parent = dev;
musb->dev.dma_mask = &musb_dmamask;
musb->dev.coherent_dma_mask = musb_dmamask;
+ musb->dev.of_node = of_node_get(np);

glue->musb[id] = musb;

@@ -566,8 +522,7 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue, u8 id)

of_property_read_u32(np, "num-eps", (u32 *)&config->num_eps);
of_property_read_u32(np, "ram-bits", (u32 *)&config->ram_bits);
- snprintf(res_name, sizeof(res_name), "port%d-mode", id);
- of_property_read_u32(np, res_name, (u32 *)&pdata->mode);
+ of_property_read_u32(np, "port-mode", (u32 *)&pdata->mode);
of_property_read_u32(np, "power", (u32 *)&pdata->power);
config->multipoint = of_property_read_bool(np, "multipoint");

@@ -602,6 +557,7 @@ static int dsps_probe(struct platform_device *pdev)
const struct dsps_musb_wrapper *wrp;
struct dsps_glue *glue;
struct resource *iomem;
+ struct device_node *child;
int ret, i;

match = of_match_node(musb_dsps_of_match, pdev->dev.of_node);
@@ -648,8 +604,9 @@ static int dsps_probe(struct platform_device *pdev)
}

/* create the child platform device for all instances of musb */
- for (i = 0; i < wrp->instances ; i++) {
- ret = dsps_create_musb_pdev(glue, i);
+ i = 0;
+ for_each_available_child_of_node(pdev->dev.of_node, child) {
+ ret = dsps_create_musb_pdev(glue, i, child);
if (ret != 0) {
dev_err(&pdev->dev, "failed to create child pdev\n");
/* release resources of previously created instances */
@@ -657,6 +614,7 @@ static int dsps_probe(struct platform_device *pdev)
platform_device_unregister(glue->musb[i]);
goto err3;
}
+ i++;
}

return 0;
@@ -674,12 +632,13 @@ err0:
static int dsps_remove(struct platform_device *pdev)
{
struct dsps_glue *glue = platform_get_drvdata(pdev);
- const struct dsps_musb_wrapper *wrp = glue->wrp;
int i;

/* delete the child platform device */
- for (i = 0; i < wrp->instances ; i++)
- platform_device_unregister(glue->musb[i]);
+ for (i = 0; i < ARRAY_SIZE(glue->musb); i++) {
+ if (glue->musb[i])
+ platform_device_unregister(glue->musb[i]);
+ }

/* disable usbss clocks */
pm_runtime_put(&pdev->dev);
@@ -692,26 +651,28 @@ 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);
- const struct dsps_musb_wrapper *wrp = glue->wrp;
+ struct dsps_glue *glue = dev_get_drvdata(dev);
+ struct musb *musb;
int i;

- for (i = 0; i < wrp->instances; i++)
- musb_dsps_phy_control(glue, i, 0);
+ for (i = 0; i < ARRAY_SIZE(glue->musb); 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);
- const struct dsps_musb_wrapper *wrp = glue->wrp;
+ struct dsps_glue *glue = dev_get_drvdata(dev);
+ struct musb *musb;
int i;

- for (i = 0; i < wrp->instances; i++)
- musb_dsps_phy_control(glue, i, 1);
+ for (i = 0; i < ARRAY_SIZE(glue->musb); i++) {
+ musb = glue_to_musb(glue, i);
+ phy_power_off(musb->phy);
+ }

return 0;
}
@@ -747,7 +708,6 @@ static const struct dsps_musb_wrapper ti81xx_driver_data = {
.rxep_bitmap = (0xfffe << 16),
.musb_core_offset = 0x400,
.poll_seconds = 2,
- .instances = 2,
};

static const struct platform_device_id musb_dsps_id_table[] = {
--
1.8.1.4

2013-07-19 13:18:05

[permalink] [raw]

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

Hi,

On Friday 19 July 2013 06:04 PM, George Cherian wrote:
> This patch series adds phy support for AM335X platform.
> This patch series is based on Generic PHY framework [1].

There are some comments on the generic PHY framework. So you might have to
adapt it once those comments are fixed.

Thanks
Kishon

2013-07-19 13:20:53

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] usb: phy: phy-omap-control: Add API to power and wakeup

Hi,

On Friday 19 July 2013 06:04 PM, George Cherian wrote:
> on/off USB PHY for AM335X
>
> Add APIs to
> -power on/off USB PHY for AM335X
> -enable/disable PHY wakeup.
>
> This API will be called from phy-amxxxx-usb driver.
>
> Signed-off-by: George Cherian <[email protected]>
> ---
> drivers/usb/phy/phy-omap-control.c | 67 ++++++++++++++++++++++++++++++++++++
> include/linux/usb/omap_control_usb.h | 24 +++++++++++++
> 2 files changed, 91 insertions(+)
>
> diff --git a/drivers/usb/phy/phy-omap-control.c b/drivers/usb/phy/phy-omap-control.c
> index 1419ced..4f2502c 100644
> --- a/drivers/usb/phy/phy-omap-control.c
> +++ b/drivers/usb/phy/phy-omap-control.c
> @@ -46,6 +46,73 @@ struct device *omap_get_control_dev(void)
> EXPORT_SYMBOL_GPL(omap_get_control_dev);
>
> /**
> + * omap_control_am335x_phy_wkup - PHY wakeup on/off using control
> + * module
> + * @dev: the control module device
> + * @on: 0 to off and 1 to on PHY wakeup feature

%s/off/disable %s/on/enable
> + * @id: phy id 0 or 1 for phy instance 0 and 1 repectively
> + *
> + * AMXXXX PHY driver should call this API to enable or disable PHY wakeup.
> + */
> +void omap_control_am335x_phy_wkup(struct device *dev, bool on, u8 id)
> +{
> + u32 val;
> + u32 *phy_wkup_reg;
> + struct omap_control_usb *control_usb = dev_get_drvdata(dev);
> +
> + if (control_usb->type != OMAP_CTRL_DEV_TYPE3 || id < 0 || id > 1)
> + return;
> +
> + phy_wkup_reg = control_usb->dev_conf + AM335X_USB_WKUP_OFFSET;
> + val = readl(phy_wkup_reg);
> +
> + if (on)
> + val |= id ? AM335X_USB1_WKUP_CTRL_ENABLE :
> + AM335X_USB0_WKUP_CTRL_ENABLE;
> + else
> + val &= id ? ~AM335X_USB1_WKUP_CTRL_ENABLE :
> + ~AM335X_USB0_WKUP_CTRL_ENABLE;
> +
> +

You really need two blank spaces here?
> + writel(val, phy_wkup_reg);
> +}
> +EXPORT_SYMBOL_GPL(omap_control_am335x_phy_wkup);
> +
> +/**
> + * 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;
> + u32 *phy_pwr_reg;
> + struct omap_control_usb *control_usb = dev_get_drvdata(dev);
> +
> + if (control_usb->type != OMAP_CTRL_DEV_TYPE3 || id < 0 || id > 1)
> + return;
> +
> + phy_pwr_reg = GET_PHY_POWER_REG(control_usb->dev_conf, id);
> + val = readl(phy_pwr_reg);
> +
> + 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;
> +
> +
ditto.
> + writel(val, phy_pwr_reg);
> +}
> +EXPORT_SYMBOL_GPL(omap_control_am335x_phy_power);
> +
> +
ditto.
> +/**
> * 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..a92b417 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,23 @@ enum omap_control_usb_mode {
> #define OMAP_CTRL_USB3_PHY_TX_RX_POWERON 0x3
> #define OMAP_CTRL_USB3_PHY_TX_RX_POWEROFF 0x0
>
> +#define GET_PHY_POWER_REG(base, id) ((u32 *)base + (id * 2))

how about
#define GET_PHY_POWER_REG(base, id) ((u32 *)(base) + ((id) * 2))
> +#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_USB_WKUP_OFFSET 0xA
> +#define AM335X_USB0_WKUP_CTRL_ENABLE (1 << 0)
> +#define AM335X_USB1_WKUP_CTRL_ENABLE (1 << 8)
> +
> +
> +

too many blank lines...

Thanks
Kishon

2013-07-19 13:32:44

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform

Hi,

On Friday 19 July 2013 06:04 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 <[email protected]>
> ---
> drivers/phy/Kconfig | 12 +++
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-amxxxx-usb.c | 222 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 235 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

Greg has some comments about the generic PHY framework. So instead of having
like this, you might have to select GENERIC_PHY in the Kconfig of your phy driver.
> +
> +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..d0b0749
> --- /dev/null
> +++ b/drivers/phy/phy-amxxxx-usb.c
> @@ -0,0 +1,222 @@
> +/*
> + * phy-amxxxx-usb.c - USB PHY, talking to usb controller in AMXXX.
> + *
> + * Copyright (C) 2013 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 <[email protected]>
> + *
> + * 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/otg.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;
> + u32 id;
> +};
> +
> +#define phy_to_amxxxxphy(x) container_of((x), struct amxxxx_phy, phy)
> +
> +

additional blank line..
> +static int amxxxx_usb_set_host(struct usb_otg *otg, struct usb_bus *host)
> +{
> + struct usb_phy *phy = otg->phy;
> +
> + otg->host = host;
> + if (!host)
> + phy->state = OTG_STATE_UNDEFINED;
> +
> + return 0;
> +}
> +
> +static int amxxxx_usb_set_peripheral(struct usb_otg *otg,
> + struct usb_gadget *gadget)
> +{
> + struct usb_phy *phy = otg->phy;
> +
> + otg->gadget = gadget;
> + if (!gadget)
> + phy->state = OTG_STATE_UNDEFINED;
> +
> + return 0;
> +}
> +
> +
> +
ditto..
> +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);

You might have to bail out of memory allocation fails.
> +
> + otg = devm_kzalloc(&pdev->dev, sizeof(*otg), GFP_KERNEL);

ditto.
> +
> + phy->dev = &pdev->dev;
> +
> + if (np)
> + of_property_read_u32(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->set_host = amxxxx_usb_set_host;
> + otg->set_peripheral = amxxxx_usb_set_peripheral;
> + 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");

Will the device be functional without this clock? If it's not functional,, then
you might have to bail out.
> + if (IS_ERR(phy->optclk))
> + dev_err(&pdev->dev, "unable to get usbotg_fck\n");
> + else
> + clk_prepare(phy->optclk);
> +
> + device_init_wakeup(&pdev->dev, true);
> + 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);

Should we be doing a power off of phy here?
> +
> + 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);
> + if (device_may_wakeup(dev))
> + omap_control_am335x_phy_wkup(phy->control_dev, 1, 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);
> +
> +
empty blank line..

Thanks
Kishon

2013-07-19 13:34:30

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] arm: dts: Add USB phy nodes for AM33XX

Hi,

On Friday 19 July 2013 06:04 PM, George Cherian wrote:
> Add phy nodes for AM33XX platform and split the musb nodes
> per instance.
>
> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
> Signed-off-by: George Cherian <[email protected]>
> ---
> arch/arm/boot/dts/am33xx.dtsi | 68 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 53 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index 8e1248f..e3890c4 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -326,21 +326,59 @@
> status = "disabled";
> };
>
> - usb@47400000 {
> - compatible = "ti,musb-am33xx";
> - reg = <0x47400000 0x1000 /* usbss */
> - 0x47401000 0x800 /* musb instance 0 */
> - 0x47401800 0x800>; /* musb instance 1 */
> - interrupts = <17 /* usbss */
> - 18 /* musb instance 0 */
> - 19>; /* musb instance 1 */
> - multipoint = <1>;
> - num-eps = <16>;
> - ram-bits = <12>;
> - port0-mode = <3>;
> - port1-mode = <3>;
> - power = <250>;
> - ti,hwmods = "usb_otg_hs";
> + phy1: am335x-usb0@44e10620 {

*44e10620* is not needed if you are not having the reg property.
> + compatible = "ti,am335x-usb2";
> + #phy-cells = <0>;
> + id= <0>;
> + };
> +
> + phy2: am335x-usb1@44e10628 {
> + compatible = "ti,am335x-usb2";

ditto..
> + #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>;
> + };
> +
> + musb: usb@47400000 {
> + compatible = "ti,musb-am33xx";
> + reg = <0x47400000 0x1000>;
> + ranges;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + interrupts = <17>;
> + ti,hwmods = "usb_otg_hs";
> +
> + usb0@47401000 {
> + reg = <0x47401000 0x800>;
> + interrupts = <18>;
> + interrupt-names = "mc";
> + multipoint = <1>;
> + num-eps = <16>;
> + ram-bits = <12>;
> + port-mode = <3>;
> + power = <250>;
> + phys = <&phy1>;
> + phy-names = "am335x-usb0";

Looks like alignment has gone wrong here.
> + };
> +
> + usb1@47401800 {
> + reg = <0x47401800 0x800>;
> + interrupts = <19>;
> + interrupt-names = "mc";
> + multipoint = <1>;
> + num-eps = <16>;
> + ram-bits = <12>;
> + port-mode = <3>;
> + power = <250>;
> + phys = <&phy2>;
> + phy-names = "am335x-usb1";
ditto.

Thanks
Kishon

2013-07-19 13:44:57

[permalink] [raw]

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

On Friday 19 July 2013 06:04 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.
> Adapt the driver to the split dt nodes.

This patch should be split.
>
> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
> Signed-off-by: George Cherian <[email protected]>
> ---
> drivers/usb/musb/musb_dsps.c | 150 ++++++++++++++++---------------------------
> 1 file changed, 55 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index 41f135a..6bec458 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -43,6 +43,7 @@
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/of_address.h>
> +#include <linux/of_irq.h>
>
> #include "musb_core.h"
>
> @@ -110,8 +111,6 @@ struct dsps_musb_wrapper {
> /* miscellaneous stuff */
> u32 musb_core_offset;
> u8 poll_seconds;
> - /* number of musb instances */
> - u8 instances;
> };
>
> /**
> @@ -123,49 +122,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 +372,28 @@ 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);
> + snprintf(phyname , 12, "am335x-usb%d", pdev->id);
> +
> + if (dev->of_node) {
> + musb->phy = devm_phy_get(dev, phyname);
> + musb->xceiv = devm_usb_get_phy_by_phandle(dev,
> + "phys", 0);
> + } else {
> + 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))
> + return -EPROBE_DEFER;
> +
> /* Returns zero if e.g. not clocked */
> rev = dsps_readl(reg_base, wrp->revision);
> if (!rev) {
> @@ -434,7 +407,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 +422,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 +435,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;
> }
> @@ -483,49 +453,34 @@ static struct musb_platform_ops dsps_ops = {
>
> static u64 musb_dmamask = DMA_BIT_MASK(32);
>
> -static int dsps_create_musb_pdev(struct dsps_glue *glue, u8 id)
> +static int dsps_create_musb_pdev(struct dsps_glue *glue, u8 id,
> + struct device_node *np)
> {
> struct device *dev = glue->dev;
> struct platform_device *pdev = to_platform_device(dev);
> struct musb_hdrc_platform_data *pdata = dev->platform_data;
> - struct device_node *np = pdev->dev.of_node;
> struct musb_hdrc_config *config;
> struct platform_device *musb;
> - struct resource *res;
> struct resource resources[2];
> char res_name[11];

res_name should be removed.
> 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) {
> + ret = of_address_to_resource(np, 0, &resources[0]);
> + if (ret) {
> dev_err(dev, "failed to get memory for instance %d\n", id);
> - ret = -ENODEV;
> goto err0;
> }
> - res->parent = NULL;
> - resources[0] = *res;
>
> /* first resource is for usbss, so start index from 1 */
> - res = platform_get_resource(pdev, IORESOURCE_IRQ, id + 1);
> - if (!res) {
> + ret = of_irq_to_resource(np, 0, &resources[1]);
> + if (ret == 0) {
> dev_err(dev, "failed to get irq for instance %d\n", id);
> - ret = -ENODEV;
> + ret = -EINVAL;
> goto err0;
> }
> - res->parent = NULL;
> - resources[1] = *res;
> - resources[1].name = "mc";
> + resources[0].parent = NULL;
> + resources[1].parent = NULL;

What's the significance of setting parent to NULL?
> +
>
> /* allocate the child platform device */
> musb = platform_device_alloc("musb-hdrc", PLATFORM_DEVID_AUTO);
> @@ -538,6 +493,7 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue, u8 id)
> musb->dev.parent = dev;
> musb->dev.dma_mask = &musb_dmamask;
> musb->dev.coherent_dma_mask = musb_dmamask;
> + musb->dev.of_node = of_node_get(np);

Do you have a corresponding of_node_put?

Thanks
Kishon

2013-07-19 13:55:58

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] arm: dts: Add USB phy nodes for AM33XX

Hello.

On 19-07-2013 16:34, George Cherian wrote:

> Add phy nodes for AM33XX platform and split the musb nodes
> per instance.

> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
> Signed-off-by: George Cherian <[email protected]>
> ---
> arch/arm/boot/dts/am33xx.dtsi | 68 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 53 insertions(+), 15 deletions(-)

> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index 8e1248f..e3890c4 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -326,21 +326,59 @@
> status = "disabled";
> };
>
> - usb@47400000 {
> - compatible = "ti,musb-am33xx";
> - reg = <0x47400000 0x1000 /* usbss */
> - 0x47401000 0x800 /* musb instance 0 */
> - 0x47401800 0x800>; /* musb instance 1 */
> - interrupts = <17 /* usbss */
> - 18 /* musb instance 0 */
> - 19>; /* musb instance 1 */
> - multipoint = <1>;
> - num-eps = <16>;
> - ram-bits = <12>;
> - port0-mode = <3>;
> - port1-mode = <3>;
> - power = <250>;
> - ti,hwmods = "usb_otg_hs";
> + phy1: am335x-usb0@44e10620 {

Shouldn't the PHYs be *under* the usb0/1 device nodes in the hierarchy?
They're not on the same bus as the MUSB controllers for sure.

> + compatible = "ti,am335x-usb2";
> + #phy-cells = <0>;
> + id= <0>;

Forgot space before =.

> + };
> +
> + phy2: am335x-usb1@44e10628 {
> + compatible = "ti,am335x-usb2";
> + #phy-cells = <0>;
> + id= <1>;

Same here.

> + };
> +
> + omap_control_usb: omap-control-usb@44e10620 {
> + compatible = "ti,omap-control-usb";
> + reg = <0x44e10620 0x10>;
> + reg-names = "control_dev_conf";
> + ti,type = <3>;
> + };
> +
> + musb: usb@47400000 {
> + compatible = "ti,musb-am33xx";
> + reg = <0x47400000 0x1000>;
> + ranges;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + interrupts = <17>;
> + ti,hwmods = "usb_otg_hs";
> +
> + usb0@47401000 {

I don't think you need index in the name since you have the address as a
part of the name anyway. That way it's closer to the ePAPR spec.

> + reg = <0x47401000 0x800>;
> + interrupts = <18>;
> + interrupt-names = "mc";
> + multipoint = <1>;
> + num-eps = <16>;
> + ram-bits = <12>;
> + port-mode = <3>;
> + power = <250>;
> + phys = <&phy1>;

The above lines are indented with spaces, the below one with tabs. Use
tabs please.

> + phy-names = "am335x-usb0";
> + };
> +
> + usb1@47401800 {
> + reg = <0x47401800 0x800>;
> + interrupts = <19>;
> + interrupt-names = "mc";
> + multipoint = <1>;
> + num-eps = <16>;
> + ram-bits = <12>;
> + port-mode = <3>;
> + power = <250>;
> + phys = <&phy2>;

The above lines are indented with spaces, the below one with tabs. Use
tabs please.

> + phy-names = "am335x-usb1";
> + };
> };

WBR, Sergei

2013-07-19 14:20:41

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] arm: dts: Add USB phy nodes for AM33XX

On 07/19/2013 03:56 PM, Sergei Shtylyov wrote:
> On 19-07-2013 16:34, George Cherian wrote:
>
>> diff --git a/arch/arm/boot/dts/am33xx.dtsi
>> b/arch/arm/boot/dts/am33xx.dtsi
>> index 8e1248f..e3890c4 100644
>> --- a/arch/arm/boot/dts/am33xx.dtsi
>> +++ b/arch/arm/boot/dts/am33xx.dtsi
>> @@ -326,21 +326,59 @@
>> status = "disabled";
>> };
>>
>> - usb@47400000 {
>> - compatible = "ti,musb-am33xx";
>> - reg = <0x47400000 0x1000 /* usbss */
>> - 0x47401000 0x800 /* musb instance 0 */
>> - 0x47401800 0x800>; /* musb instance 1 */
>> - interrupts = <17 /* usbss */
>> - 18 /* musb instance 0 */
>> - 19>; /* musb instance 1 */
>> - multipoint = <1>;
>> - num-eps = <16>;
>> - ram-bits = <12>;
>> - port0-mode = <3>;
>> - port1-mode = <3>;
>> - power = <250>;
>> - ti,hwmods = "usb_otg_hs";
>> + phy1: am335x-usb0@44e10620 {
>
> Shouldn't the PHYs be *under* the usb0/1 device nodes in the hierarchy?
> They're not on the same bus as the MUSB controllers for sure.

I redo the complete thing. I have now:

usb: usb@47400000 {
compatible = "ti,am33xx-usb";

usb0_phy: phy@47401300 {
compatible = "ti,am335x-usb-phy";
}
usb0: usb@47401000 {
musb0: usb@47401400 {
compatible = "mg,musbmhdrc";
}
}
usb1_phy: phy@47402300 {
compatible = "ti,am335x-usb-phy";
}
usb1: usb@47402000 {
musb1: usb@47402400 {
compatible = "mg,musbmhdrc";
}
}
}

And you want usb0_phy to be child of usb0? In the TRM they are all in
the same block.

>
> WBR, Sergei
>

Sebastian

2013-07-19 18:33:07

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] arm: dts: Add USB phy nodes for AM33XX

Hello.

On 07/19/2013 06:20 PM, Sebastian Andrzej Siewior wrote:

>>> diff --git a/arch/arm/boot/dts/am33xx.dtsi
>>> b/arch/arm/boot/dts/am33xx.dtsi
>>> index 8e1248f..e3890c4 100644
>>> --- a/arch/arm/boot/dts/am33xx.dtsi
>>> +++ b/arch/arm/boot/dts/am33xx.dtsi
>>> @@ -326,21 +326,59 @@
>>> status = "disabled";
>>> };
>>>
>>> - usb@47400000 {
>>> - compatible = "ti,musb-am33xx";
>>> - reg = <0x47400000 0x1000 /* usbss */
>>> - 0x47401000 0x800 /* musb instance 0 */
>>> - 0x47401800 0x800>; /* musb instance 1 */
>>> - interrupts = <17 /* usbss */
>>> - 18 /* musb instance 0 */
>>> - 19>; /* musb instance 1 */
>>> - multipoint = <1>;
>>> - num-eps = <16>;
>>> - ram-bits = <12>;
>>> - port0-mode = <3>;
>>> - port1-mode = <3>;
>>> - power = <250>;
>>> - ti,hwmods = "usb_otg_hs";
>>> + phy1: am335x-usb0@44e10620 {

>> Shouldn't the PHYs be *under* the usb0/1 device nodes in the hierarchy?
>> They're not on the same bus as the MUSB controllers for sure.

> I redo the complete thing. I have now:

> usb: usb@47400000 {
> compatible = "ti,am33xx-usb";
>
> usb0_phy: phy@47401300 {
> compatible = "ti,am335x-usb-phy";
> }
> usb0: usb@47401000 {
> musb0: usb@47401400 {
> compatible = "mg,musbmhdrc";
> }
> }
> usb1_phy: phy@47402300 {
> compatible = "ti,am335x-usb-phy";
> }
> usb1: usb@47402000 {
> musb1: usb@47402400 {
> compatible = "mg,musbmhdrc";
> }
> }
> }

> And you want usb0_phy to be child of usb0? In the TRM they are all in
> the same block.

Ah, the fact that PHYs didn't have the "reg" property got me muddled, I
didn't pay attention to the address part of the node names... BTW, where is
the "reg" prop? I see PHYs share the address space with
"omap-control-usb@44e10620" device -- what's the point with this?

> Sebastian

WBR, Sergei

2013-07-19 18:42:37

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] arm: dts: Add USB phy nodes for AM33XX

On 07/19/2013 08:33 PM, Sergei Shtylyov wrote:
> Hello.

Hello,

>> usb: usb@47400000 {
>> compatible = "ti,am33xx-usb";
>>
>> usb0_phy: phy@47401300 {
>> compatible = "ti,am335x-usb-phy";
>> }
>> usb0: usb@47401000 {
>> musb0: usb@47401400 {
>> compatible = "mg,musbmhdrc";
>> }
>> }
>> usb1_phy: phy@47402300 {
>> compatible = "ti,am335x-usb-phy";
>> }
>> usb1: usb@47402000 {
>> musb1: usb@47402400 {
>> compatible = "mg,musbmhdrc";
>> }
>> }
>> }
>
>> And you want usb0_phy to be child of usb0? In the TRM they are all in
>> the same block.
>
> Ah, the fact that PHYs didn't have the "reg" property got me muddled,
> I didn't pay attention to the address part of the node names... BTW,
> where is the "reg" prop?

I skipped it for the general idea. I planned to repost is today but I
messed up dsps and need to get it working first?

> I see PHYs share the address space with
> "omap-control-usb@44e10620" device -- what's the point with this?

I decided to get rid of this. Both phys have 8 bytes (2 registers)
which are exclusive for them.
There is one register for the wakeup which is shared by both.
I changed this to limit it only to the 8bytes per phy. I care about
wakeup later - hopefully George will take care of this :)

>
>> Sebastian
>
> WBR, Sergei
>
Sebastian

2013-07-20 03:36:51

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] arm: dts: Add USB phy nodes for AM33XX

On 7/20/2013 12:03 AM, Sergei Shtylyov wrote:
> Hello.
>
> On 07/19/2013 06:20 PM, Sebastian Andrzej Siewior wrote:
>
>>>> diff --git a/arch/arm/boot/dts/am33xx.dtsi
>>>> b/arch/arm/boot/dts/am33xx.dtsi
>>>> index 8e1248f..e3890c4 100644
>>>> --- a/arch/arm/boot/dts/am33xx.dtsi
>>>> +++ b/arch/arm/boot/dts/am33xx.dtsi
>>>> @@ -326,21 +326,59 @@
>>>> status = "disabled";
>>>> };
>>>>
>>>> - usb@47400000 {
>>>> - compatible = "ti,musb-am33xx";
>>>> - reg = <0x47400000 0x1000 /* usbss */
>>>> - 0x47401000 0x800 /* musb instance 0 */
>>>> - 0x47401800 0x800>; /* musb instance 1 */
>>>> - interrupts = <17 /* usbss */
>>>> - 18 /* musb instance 0 */
>>>> - 19>; /* musb instance 1 */
>>>> - multipoint = <1>;
>>>> - num-eps = <16>;
>>>> - ram-bits = <12>;
>>>> - port0-mode = <3>;
>>>> - port1-mode = <3>;
>>>> - power = <250>;
>>>> - ti,hwmods = "usb_otg_hs";
>>>> + phy1: am335x-usb0@44e10620 {
>
>>> Shouldn't the PHYs be *under* the usb0/1 device nodes in the
>>> hierarchy?
>>> They're not on the same bus as the MUSB controllers for sure.
>
>> I redo the complete thing. I have now:
>
>> usb: usb@47400000 {
>> compatible = "ti,am33xx-usb";
>>
>> usb0_phy: phy@47401300 {
>> compatible = "ti,am335x-usb-phy";
>> }
>> usb0: usb@47401000 {
>> musb0: usb@47401400 {
>> compatible = "mg,musbmhdrc";
>> }
>> }
>> usb1_phy: phy@47402300 {
>> compatible = "ti,am335x-usb-phy";
>> }
>> usb1: usb@47402000 {
>> musb1: usb@47402400 {
>> compatible = "mg,musbmhdrc";
>> }
>> }
>> }
>
>> And you want usb0_phy to be child of usb0? In the TRM they are all in
>> the same block.
>
> Ah, the fact that PHYs didn't have the "reg" property got me
> muddled, I didn't pay attention to the address part of the node
> names... BTW, where is the "reg" prop? I see PHYs share the address
> space with "omap-control-usb@44e10620" device -- what's the point with
> this?
In control module(CM) each USB has got 2 registers in which one is a
shared register( wakeup register)
So all the CM access are done using the control-usb driver
(phy-omap-control-usb.c). Same reason why phy's dont have a reg property.
>
>> Sebastian
>
> WBR, Sergei
>

--
-George

2013-07-20 03:41:46

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] arm: dts: Add USB phy nodes for AM33XX

On 7/20/2013 12:12 AM, Sebastian Andrzej Siewior wrote:
> On 07/19/2013 08:33 PM, Sergei Shtylyov wrote:
>> Hello.
> Hello,
>
>>> usb: usb@47400000 {
>>> compatible = "ti,am33xx-usb";
>>>
>>> usb0_phy: phy@47401300 {
>>> compatible = "ti,am335x-usb-phy";
>>> }
>>> usb0: usb@47401000 {
>>> musb0: usb@47401400 {
>>> compatible = "mg,musbmhdrc";
>>> }
>>> }
>>> usb1_phy: phy@47402300 {
>>> compatible = "ti,am335x-usb-phy";
>>> }
>>> usb1: usb@47402000 {
>>> musb1: usb@47402400 {
>>> compatible = "mg,musbmhdrc";
>>> }
>>> }
>>> }
>>> And you want usb0_phy to be child of usb0? In the TRM they are all in
>>> the same block.
>> Ah, the fact that PHYs didn't have the "reg" property got me muddled,
>> I didn't pay attention to the address part of the node names... BTW,
>> where is the "reg" prop?
> I skipped it for the general idea. I planned to repost is today but I
> messed up dsps and need to get it working first?
>
>> I see PHYs share the address space with
>> "omap-control-usb@44e10620" device -- what's the point with this?
> I decided to get rid of this. Both phys have 8 bytes (2 registers)
> which are exclusive for them.
> There is one register for the wakeup which is shared by both.
> I changed this to limit it only to the 8bytes per phy. I care about
> wakeup later - hopefully George will take care of this :)
But for wakeup how can we map it since its the same register. That is
the main reason i took the
omap-control-usb route.

>
>>> Sebastian
>> WBR, Sergei
>>
> Sebastian

--
-George

2013-07-22 05:45:59

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] arm: dts: Add USB phy nodes for AM33XX

On 7/20/2013 9:11 AM, George Cherian wrote:
> On 7/20/2013 12:12 AM, Sebastian Andrzej Siewior wrote:
>> On 07/19/2013 08:33 PM, Sergei Shtylyov wrote:
>>> Hello.
>> Hello,
>>
>>>> usb: usb@47400000 {
>>>> compatible = "ti,am33xx-usb";
>>>> usb0_phy: phy@47401300 {
>>>> compatible = "ti,am335x-usb-phy";
>>>> }
>>>> usb0: usb@47401000 {
>>>> musb0: usb@47401400 {
>>>> compatible = "mg,musbmhdrc";
>>>> }
>>>> }
>>>> usb1_phy: phy@47402300 {
>>>> compatible = "ti,am335x-usb-phy";
>>>> }
>>>> usb1: usb@47402000 {
>>>> musb1: usb@47402400 {
>>>> compatible = "mg,musbmhdrc";
>>>> }
>>>> }
>>>> }

How about something like this ?

Here am making
wrapper -> musb instance 0 -> phy0
wrapper -> musb instance 1 -> phy1

musb: usb@47400000 {
compatible = "ti,musb-am33xx";
reg = <0x47400000 0x1000>;
ranges;
#address-cells = <1>;
#size-cells = <1>;
interrupts = <17>;
ti,hwmods = "usb_otg_hs";

usb0@47401000 {
reg = <0x47401000 0x800>;
interrupts = <18>;
interrupt-names = "mc";
multipoint = <1>;
num-eps = <16>;
ram-bits = <12>;
port-mode = <3>;
power = <250>;
phys = <&phy1>;
phy-names = "am335x-usb0";
phy1: am335x-usb0 {
compatible = "ti,am335x-usb2";
#phy-cells = <0>;
id= <0>;
};
};

usb1@47401800 {
reg = <0x47401800 0x800>;
interrupts = <19>;
interrupt-names = "mc";
multipoint = <1>;
num-eps = <16>;
ram-bits = <12>;
port-mode = <3>;
power = <250>;
phys = <&phy2>;
phy-names = "am335x-usb1";
phy2: am335x-usb1 {
compatible = "ti,am335x-usb2";
#phy-cells = <0>;
id= <1>;
};
};

};

--
-George

2013-07-29 14:25:21

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] usb: phy: phy-omap-control: Add API to power and wakeup

* George Cherian | 2013-07-19 18:04:34 [+0530]:

>diff --git a/drivers/usb/phy/phy-omap-control.c b/drivers/usb/phy/phy-omap-control.c
>index 1419ced..4f2502c 100644
>--- a/drivers/usb/phy/phy-omap-control.c
>+++ b/drivers/usb/phy/phy-omap-control.c
>@@ -46,6 +46,73 @@ struct device *omap_get_control_dev(void)
> EXPORT_SYMBOL_GPL(omap_get_control_dev);
>
> /**
>+ * omap_control_am335x_phy_wkup - PHY wakeup on/off using control
>+ * module
>+ * @dev: the control module device
>+ * @on: 0 to off and 1 to on PHY wakeup feature
>+ * @id: phy id 0 or 1 for phy instance 0 and 1 repectively
>+ *
>+ * AMXXXX PHY driver should call this API to enable or disable PHY wakeup.
>+ */
>+void omap_control_am335x_phy_wkup(struct device *dev, bool on, u8 id)
>+{
>+ u32 val;
>+ u32 *phy_wkup_reg;
>+ struct omap_control_usb *control_usb = dev_get_drvdata(dev);
>+
>+ if (control_usb->type != OMAP_CTRL_DEV_TYPE3 || id < 0 || id > 1)
>+ return;
>+
>+ phy_wkup_reg = control_usb->dev_conf + AM335X_USB_WKUP_OFFSET;
>+ val = readl(phy_wkup_reg);

Where do you get the memory from? In the DT patches I've made I export
one memory space of 8 bytes for each phy. This memory space applies
directly to the on/off register. I didn't export the register for wakeup
so far since I had no idea how this will be used.
So how do you get your re-mapped memory here?

>+
>+ if (on)
>+ val |= id ? AM335X_USB1_WKUP_CTRL_ENABLE :
>+ AM335X_USB0_WKUP_CTRL_ENABLE;
>+ else
>+ val &= id ? ~AM335X_USB1_WKUP_CTRL_ENABLE :
>+ ~AM335X_USB0_WKUP_CTRL_ENABLE;
>+
>+
>+ writel(val, phy_wkup_reg);
>+}
>+EXPORT_SYMBOL_GPL(omap_control_am335x_phy_wkup);

Sebastian

2013-07-29 15:17:58

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform

* George Cherian | 2013-07-19 18:04:35 [+0530]:

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

If you rebase this on-top of the two instances patches I've sent earlier
then you can bury patch 3 and 4, right?

I don't like very much the way you obtain the phy id:
>+ of_property_read_u32(np, "id", &phy->id);

with the .dts changes I made you should able to use
of_alias_get_id()
instead.

Let me look what you have additionaly:
- usbotg_fck
Is this really required? I have the phy as a child of the "main"
device which has a hwmod property which is associated with this clock.
So pm enable/ disable should also enable this clock if possible,
right?

- device wakeup via omap_control_am335x_phy_wkup()
Now. that is one thing that the simple phy driver is missing. If you
call a magic function for this to happen than I don't have to worry
about the missing memory space for this function.

So from what I see now, it is most likely the easiest thing to just add
that wakeup to the phy driver I posted. Do you agree?
If so we need to figure out where the memory for the wakeup register is
comming from. We need also to ensure that both phys can not write at the
same time. A look would be nice.

>Signed-off-by: George Cherian <[email protected]>

Sebastian

2013-07-30 04:53:31

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] usb: phy: phy-omap-control: Add API to power and wakeup

On 7/29/2013 7:55 PM, Sebastian Andrzej Siewior wrote:
> * George Cherian | 2013-07-19 18:04:34 [+0530]:
>
>> diff --git a/drivers/usb/phy/phy-omap-control.c b/drivers/usb/phy/phy-omap-control.c
>> index 1419ced..4f2502c 100644
>> --- a/drivers/usb/phy/phy-omap-control.c
>> +++ b/drivers/usb/phy/phy-omap-control.c
>> @@ -46,6 +46,73 @@ struct device *omap_get_control_dev(void)
>> EXPORT_SYMBOL_GPL(omap_get_control_dev);
>>
>> /**
>> + * omap_control_am335x_phy_wkup - PHY wakeup on/off using control
>> + * module
>> + * @dev: the control module device
>> + * @on: 0 to off and 1 to on PHY wakeup feature
>> + * @id: phy id 0 or 1 for phy instance 0 and 1 repectively
>> + *
>> + * AMXXXX PHY driver should call this API to enable or disable PHY wakeup.
>> + */
>> +void omap_control_am335x_phy_wkup(struct device *dev, bool on, u8 id)
>> +{
>> + u32 val;
>> + u32 *phy_wkup_reg;
>> + struct omap_control_usb *control_usb = dev_get_drvdata(dev);
>> +
>> + if (control_usb->type != OMAP_CTRL_DEV_TYPE3 || id < 0 || id > 1)
>> + return;
>> +
>> + phy_wkup_reg = control_usb->dev_conf + AM335X_USB_WKUP_OFFSET;
>> + val = readl(phy_wkup_reg);
> Where do you get the memory from? In the DT patches I've made I export
> one memory space of 8 bytes for each phy. This memory space applies
> directly to the on/off register.

Control module have 2 separate registers for phy on/off per instance
(offset 0x620 and 0x628), where as
wkup_ctrl is a shared control module register (offset 0x648). Currently
the control module driver maps
memory from 0x620 till beyond 0x648 and uses the same mapping for
writing to wkup_reg.
> I didn't export the register for wakeup
> so far since I had no idea how this will be used.
> So how do you get your re-mapped memory here?

Mapping it per instance is not possible since its the same register (if
we use devm_* calls).

--
-George

2013-07-30 05:19:54

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform

On 7/29/2013 8:47 PM, Sebastian Andrzej Siewior wrote:
> * George Cherian | 2013-07-19 18:04:35 [+0530]:
>
>> Adds phy driver support for am33xx platform, the host/device
>> peripheral controller shall get this phy object to control the phy
>> operations.
> If you rebase this on-top of the two instances patches I've sent earlier
> then you can bury patch 3 and 4, right?

Yes True. I posted it for completeness.

>
> I don't like very much the way you obtain the phy id:
>> + of_property_read_u32(np, "id", &phy->id);
> with the .dts changes I made you should able to use
> of_alias_get_id()
> instead.
>
> Let me look what you have additionaly:
> - usbotg_fck
> Is this really required? I have the phy as a child of the "main"
> device which has a hwmod property which is associated with this clock.
> So pm enable/ disable should also enable this clock if possible,
> right?
>
> - device wakeup via omap_control_am335x_phy_wkup()
> Now. that is one thing that the simple phy driver is missing. If you
> call a magic function for this to happen than I don't have to worry
> about the missing memory space for this function.
>
> So from what I see now, it is most likely the easiest thing to just add
> that wakeup to the phy driver I posted. Do you agree?

The whole idea of writing a seperate phy driver was to use the generic phy framework
and most of the amxxxx devices have the same phy (eg am335x, am437x).
Now since the register is shared in am335x for phy_wkup (Not in the case of am437x)
how are you planning to map it. I feel if omap_control_usb can delegate the writes
to phy_wkup, phy_on and phy_off, it makes the life simpler.

Thoughts???

> If so we need to figure out where the memory for the wakeup register is
> comming from. We need also to ensure that both phys can not write at the
> same time. A look would be nice.
>
>> Signed-off-by: George Cherian <[email protected]>
> Sebastian

--
-George

2013-07-30 07:35:52

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] usb: phy: phy-omap-control: Add API to power and wakeup

On 07/30/2013 06:53 AM, George Cherian wrote:
> Control module have 2 separate registers for phy on/off per instance
> (offset 0x620 and 0x628), where as
> wkup_ctrl is a shared control module register (offset 0x648). Currently
> the control module driver maps
> memory from 0x620 till beyond 0x648 and uses the same mapping for
> writing to wkup_reg.

This I know about. My question is where do you remap this memory. You
have to put the memory address somewhere.

Sebastian

2013-07-30 08:53:47

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform

On 07/30/2013 07:19 AM, George Cherian wrote:
>> So from what I see now, it is most likely the easiest thing to just add
>> that wakeup to the phy driver I posted. Do you agree?
>
> The whole idea of writing a seperate phy driver was to use the generic
> phy framework
> and most of the amxxxx devices have the same phy (eg am335x, am437x).
> Now since the register is shared in am335x for phy_wkup (Not in the case
> of am437x)
> how are you planning to map it. I feel if omap_control_usb can delegate
> the writes
> to phy_wkup, phy_on and phy_off, it makes the life simpler.

that omap-control driver looks a little strange. It has a compatible
field saying ti,omap-control-usb and then it requires additionally a
ti,type property which should have been avoided.

But back to the initial problem. I don't really like the idea of
touching in the control-module registers but others do it as well.
So the idea of a control driver doesn't sound that bad.
- an am335x-reset device
- a phy driver with a reference to that reset device.
- non-standard phy calls for power & wak eup on/off.

Let me think about it.

>
> Thoughts???

I think I buy it but give me a bit…

>

Sebastian

2013-07-30 14:25:09

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform

On 7/30/2013 2:23 PM, Sebastian Andrzej Siewior wrote:
> On 07/30/2013 07:19 AM, George Cherian wrote:
>>> So from what I see now, it is most likely the easiest thing to just add
>>> that wakeup to the phy driver I posted. Do you agree?
>> The whole idea of writing a seperate phy driver was to use the generic
>> phy framework
>> and most of the amxxxx devices have the same phy (eg am335x, am437x).
>> Now since the register is shared in am335x for phy_wkup (Not in the case
>> of am437x)
>> how are you planning to map it. I feel if omap_control_usb can delegate
>> the writes
>> to phy_wkup, phy_on and phy_off, it makes the life simpler.
> that omap-control driver looks a little strange. It has a compatible
> field saying ti,omap-control-usb and then it requires additionally a
> ti,type property which should have been avoided.

ti,type property is to differentiate between usb2 and usb3 phy for a
single soc.
for eg: OMAP5 has both usb2 and usb3 phy

--
-George

2013-07-30 14:34:02

by Felipe Balbi

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform

On Tue, Jul 30, 2013 at 07:54:55PM +0530, George Cherian wrote:
> On 7/30/2013 2:23 PM, Sebastian Andrzej Siewior wrote:
> >On 07/30/2013 07:19 AM, George Cherian wrote:
> >>>So from what I see now, it is most likely the easiest thing to just add
> >>>that wakeup to the phy driver I posted. Do you agree?
> >>The whole idea of writing a seperate phy driver was to use the generic
> >>phy framework
> >>and most of the amxxxx devices have the same phy (eg am335x, am437x).
> >>Now since the register is shared in am335x for phy_wkup (Not in the case
> >>of am437x)
> >>how are you planning to map it. I feel if omap_control_usb can delegate
> >>the writes
> >>to phy_wkup, phy_on and phy_off, it makes the life simpler.
> >that omap-control driver looks a little strange. It has a compatible
> >field saying ti,omap-control-usb and then it requires additionally a
> >ti,type property which should have been avoided.
>
> ti,type property is to differentiate between usb2 and usb3 phy for a
> single soc.
> for eg: OMAP5 has both usb2 and usb3 phy

let's try not to add any new TI-specific DT bindings, can you figure
that out by reading some revision register ? Or perhaps by using
different compatible strings ?

--
balbi

Attachments:

(No filename) (1.20 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-07-30 14:39:34

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 07/30/2013 04:33 PM, Felipe Balbi wrote:
> let's try not to add any new TI-specific DT bindings, can you
> figure that out by reading some revision register ? Or perhaps by
> using different compatible strings ?

I would suggest to use a different binding. But I'm currently trying to
come up with something for am335x with a device reference?

Sebastian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)

iQIcBAEBCAAGBQJR99AZAAoJEHuW6BYqjPXRf5AP/jcMFUstlqqujRppC2gMsqh6
+xlpwh5hCHJPxQizwkG8kWW9XGMIMPmqODqoYwA9dFI3a5VgVVTRUkhnHEXlAt9A
8X0Sl+mdKx0XwvjxjRDPUEYqvpDFAPYxgnwvxEfjtnDgQs5ikxFj4zaMonAj5oK0
s7XnOImJmdoHbFfvR5Cx10zmfa0BjQETqk5bt/j8+8jnv86cZk8fEGE8XYoUnhFw
BsEZgfSZH/pV6s339gTs+x4+zW0luJIhJdtQmh5Ylo+xt/3TYAmDuZ0v88A6gFV+
MZjJLh2L9Sr6g0Pl/Rk5aMZhe5GSSrZYuu4Vltpz2xF1SjRRqg6ykmw6sqnme2uj
BWwq5qNG5EFPtyhZCupTZ/z388kKOhC54kbFwJW7Duv9TkDUOctVuUbehEN4Pu/u
HljW3FoYvvB/7EOXJhBcG6qy+ClS7mKkJJHeavnzbtzeg2jO1ZSKV1S/bJfpY4F6
ualEKdC+A+UXYNHSnSkbdJPAfIQw9BOhdD3nszdzrB+0xUU1ul2V3Zdf0xKsUVPj
8qkK9IuHum/pUfUEK/3C/igOhtag24LOhiGu1MG8Axe4FdmiTixhnV4DZzGyAMcm
8Br/gtKVCuoU89y0mYhsAA5o+3e6YKE7lPQVULtrZO7/+wbOTrEaukvwCMB/NngL
+qNzg8WzrRHCAQnSKni9
=712g
-----END PGP SIGNATURE-----

2013-08-10 11:59:44

by Ezequiel Garcia

[permalink] [raw]

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

On Fri, Jul 19, 2013 at 06:04:33PM +0530, 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
> - adds phy-amxxxx-usb driver used in AMxxxx platforms
> - adds OTG callbacks
> - adds PHY wakeup enable/disable
> - 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-v2
>
> George Cherian (4):
> usb: phy: phy-omap-control: Add API to power and wakeup
> phy: phy-amxxxx-usb: Add PHY driver for amxxxx platform
> arm: dts: Add USB phy nodes for AM33XX

Is there any reason why there's no DT binding patch to this
series? (aka Documentation/devicetree/... + sent to devicetree list)

(sorry if this has been already explained somewhere...)

--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

2013-08-14 10:36:11