2013-03-20 09:14:41

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: [PATCH v3 0/6] Generic PHY Framework

Added a generic PHY framework that provides a set of APIs for the PHY drivers
to create/destroy a PHY and APIs for the PHY users to obtain a reference to
the PHY with or without using phandle. To obtain a reference to the PHY
without using phandle, the platform specfic intialization code (say from board
file) should have already called phy_bind with the binding information. The
binding information consists of phy's device name, phy user device name and an
index. The index is used when the same phy user binds to mulitple phys.

This framework will be of use only to devices that uses external PHY (PHY
functionality is not embedded within the controller).

The intention of creating this framework is to bring the phy drivers spread
all over the Linux kernel to drivers/phy to increase code re-use and to
increase code maintainability.

Comments to make PHY as bus wasn't done because PHY devices can be part of
other bus and making a same device attached to multiple bus leads to bad
design.

Making omap-usb2 and twl4030 to use this framework is provided as a sample.

This patch series is developed on 3.9-rc3. Once the patch series gets finalised
I'll resend omap-usb2 and twl4030 part based on Felipe's tree.

Changes from v2:
* removed phy_descriptor structure completely so changed the APIs which were
taking phy_descriptor as parameters
* Added 2 more APIs *of_phy_get_byname* and *devm_of_phy_get_byname* to be used
by PHY user drivers which has *phy* and *phy-names* binding in the dt data
* Fixed a few typos
* Removed phy_list and we now use class_dev_iter_init, class_dev_iter_next and
class_dev_iter_exit for traversing through the phy list. (Note we still need
phy_bind list and phy_bind_mutex).
* Changed the sysfs entry name from *bind* to *phy_bind*.

Changes from v1:
* Added Documentation for the PHY framework
* Added few more APIs mostly w.r.t devres
* Modified omap-usb2 and twl4030 to make use of the new framework

Did USB enumeration testing in panda and beagle.
Kishon Vijay Abraham I (6):
drivers: phy: add generic PHY framework
usb: phy: omap-usb2: use the new generic PHY framework
usb: otg: twl4030: use the new generic PHY framework
ARM: OMAP: USB: Add phy binding information
ARM: dts: omap: update usb_otg_hs data
usb: musb: omap2430: use the new generic PHY framework

Documentation/ABI/testing/sysfs-class-phy | 15 +
Documentation/devicetree/bindings/usb/omap-usb.txt | 5 +
Documentation/phy.txt | 108 ++++
MAINTAINERS | 7 +
arch/arm/boot/dts/omap3.dtsi | 2 +
arch/arm/boot/dts/omap4.dtsi | 2 +
arch/arm/mach-omap2/board-2430sdp.c | 2 +
arch/arm/mach-omap2/board-3430sdp.c | 2 +
arch/arm/mach-omap2/board-4430sdp.c | 2 +
arch/arm/mach-omap2/board-cm-t35.c | 2 +
arch/arm/mach-omap2/board-devkit8000.c | 2 +
arch/arm/mach-omap2/board-igep0020.c | 2 +
arch/arm/mach-omap2/board-ldp.c | 2 +
arch/arm/mach-omap2/board-omap3beagle.c | 2 +
arch/arm/mach-omap2/board-omap3evm.c | 2 +
arch/arm/mach-omap2/board-omap3logic.c | 2 +
arch/arm/mach-omap2/board-omap3pandora.c | 2 +
arch/arm/mach-omap2/board-omap3stalker.c | 2 +
arch/arm/mach-omap2/board-omap3touchbook.c | 2 +
arch/arm/mach-omap2/board-omap4panda.c | 2 +
arch/arm/mach-omap2/board-overo.c | 2 +
arch/arm/mach-omap2/board-rm680.c | 2 +
arch/arm/mach-omap2/board-rx51.c | 2 +
arch/arm/mach-omap2/board-zoom-peripherals.c | 2 +
drivers/Kconfig | 2 +
drivers/Makefile | 2 +
drivers/phy/Kconfig | 13 +
drivers/phy/Makefile | 5 +
drivers/phy/phy-core.c | 574 ++++++++++++++++++++
drivers/usb/musb/musb_core.h | 2 +
drivers/usb/musb/omap2430.c | 22 +-
drivers/usb/otg/twl4030-usb.c | 36 ++
drivers/usb/phy/omap-usb2.c | 47 ++
include/linux/phy/phy.h | 218 ++++++++
34 files changed, 1090 insertions(+), 6 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-phy
create mode 100644 Documentation/phy.txt
create mode 100644 drivers/phy/Kconfig
create mode 100644 drivers/phy/Makefile
create mode 100644 drivers/phy/phy-core.c
create mode 100644 include/linux/phy/phy.h

--
1.7.10.4


2013-03-20 09:13:06

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: [PATCH v3 5/6] ARM: dts: omap: update usb_otg_hs data

Updated the usb_otg_hs dt data to include the *phy* and *phy-names*
binding in order for the driver to use the new generic PHY framework.
Also updated the Documentation to include the binding information.

Signed-off-by: Kishon Vijay Abraham I <[email protected]>
---
Documentation/devicetree/bindings/usb/omap-usb.txt | 5 +++++
arch/arm/boot/dts/omap3.dtsi | 2 ++
arch/arm/boot/dts/omap4.dtsi | 2 ++
3 files changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/omap-usb.txt b/Documentation/devicetree/bindings/usb/omap-usb.txt
index abce256..3d6f9f6 100644
--- a/Documentation/devicetree/bindings/usb/omap-usb.txt
+++ b/Documentation/devicetree/bindings/usb/omap-usb.txt
@@ -19,6 +19,9 @@ OMAP MUSB GLUE
- power : Should be "50". This signifies the controller can supply upto
100mA when operating in host mode.
- usb-phy : the phandle for the PHY device
+ - phy : the phandle for the PHY device (used by generic PHY framework)
+ - phy-names : the names of the PHY corresponding to the PHYs present in the
+ *phy* phandle.

Optional properties:
- ctrl-module : phandle of the control module this glue uses to write to
@@ -33,6 +36,8 @@ usb_otg_hs: [email protected] {
num_eps = <16>;
ram_bits = <12>;
ctrl-module = <&omap_control_usb>;
+ phy = <&usb2_phy>;
+ phy-names = "usb2-phy";
};

Board specific device node entry
diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index 1e21565..cf50c18 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -405,6 +405,8 @@
interrupt-names = "mc", "dma";
ti,hwmods = "usb_otg_hs";
usb-phy = <&usb2_phy>;
+ phy = <&usb2_phy>;
+ phy-names = "usb2-phy";
multipoint = <1>;
num-eps = <16>;
ram-bits = <12>;
diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index 06d044e..188d5b8 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -550,6 +550,8 @@
interrupt-names = "mc", "dma";
ti,hwmods = "usb_otg_hs";
usb-phy = <&usb2_phy>;
+ phy = <&usb2_phy>;
+ phy-names = "usb2-phy";
multipoint = <1>;
num-eps = <16>;
ram-bits = <12>;
--
1.7.10.4

2013-03-20 09:13:19

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: [PATCH v3 3/6] usb: otg: twl4030: use the new generic PHY framework

Used the generic PHY framework API to create the PHY. twl4030_usb_suspend
and twl4030_usb_resume is added to phy_ops in order to align
with the new framework.

However using the old USB PHY library cannot be completely removed
because OTG is intertwined with PHY and moving to the new
framework completely will break OTG. Once we have a separate OTG state machine,
we can get rid of the USB PHY library.

Signed-off-by: Kishon Vijay Abraham I <[email protected]>
---
drivers/usb/otg/twl4030-usb.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)

diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
index a994715..aebcd6a 100644
--- a/drivers/usb/otg/twl4030-usb.c
+++ b/drivers/usb/otg/twl4030-usb.c
@@ -33,6 +33,7 @@
#include <linux/io.h>
#include <linux/delay.h>
#include <linux/usb/otg.h>
+#include <linux/phy/phy.h>
#include <linux/usb/musb-omap.h>
#include <linux/usb/ulpi.h>
#include <linux/i2c/twl.h>
@@ -575,10 +576,38 @@ static int twl4030_set_host(struct usb_otg *otg, struct usb_bus *host)
return 0;
}

+static int twl4030_usb_suspend(struct phy *phy)
+{
+ struct twl4030_usb *twl = dev_get_drvdata(&phy->dev);
+
+ twl4030_phy_suspend(twl, 1);
+
+ return 0;
+}
+
+static int twl4030_usb_resume(struct phy *phy)
+{
+ struct twl4030_usb *twl = dev_get_drvdata(&phy->dev);
+
+ if (!twl->asleep)
+ return -EBUSY;
+ __twl4030_phy_resume(twl);
+ twl->asleep = 0;
+
+ return 0;
+}
+
+static struct phy_ops ops = {
+ .suspend = twl4030_usb_suspend,
+ .resume = twl4030_usb_resume,
+ .owner = THIS_MODULE,
+};
+
static int twl4030_usb_probe(struct platform_device *pdev)
{
struct twl4030_usb_data *pdata = pdev->dev.platform_data;
struct twl4030_usb *twl;
+ struct phy *phy;
int status, err;
struct usb_otg *otg;
struct device_node *np = pdev->dev.of_node;
@@ -617,6 +646,13 @@ static int twl4030_usb_probe(struct platform_device *pdev)
otg->set_host = twl4030_set_host;
otg->set_peripheral = twl4030_set_peripheral;

+ phy = devm_phy_create(twl->dev, "twl4030", pdev->dev.of_node,
+ USB_PHY_TYPE_USB2, &ops, twl);
+ if (IS_ERR(phy)) {
+ dev_dbg(&pdev->dev, "Failed to create PHY\n");
+ return PTR_ERR(phy);
+ }
+
/* init spinlock for workqueue */
spin_lock_init(&twl->lock);

--
1.7.10.4

2013-03-20 09:13:35

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: [PATCH v3 1/6] drivers: phy: add generic PHY framework

The PHY framework provides a set of APIs for the PHY drivers to
create/destroy a PHY and APIs for the PHY users to obtain a reference to the
PHY with or without using phandle. To obtain a reference to the PHY without
using phandle, the platform specfic intialization code (say from board file)
should have already called phy_bind with the binding information. The binding
information consists of phy's device name, phy user device name and an index.
The index is used when the same phy user binds to mulitple phys.

PHY drivers should create the PHY by passing phy_descriptor that has
describes the PHY (label, type etc..) and ops like init, exit, suspend, resume,
poweron, shutdown.

The documentation for the generic PHY framework is added in
Documentation/phy.txt and the documentation for the sysfs entry is added
in Documentation/ABI/testing/sysfs-class-phy.

Signed-off-by: Kishon Vijay Abraham I <[email protected]>
---
Documentation/ABI/testing/sysfs-class-phy | 15 +
Documentation/phy.txt | 108 ++++++
MAINTAINERS | 7 +
drivers/Kconfig | 2 +
drivers/Makefile | 2 +
drivers/phy/Kconfig | 13 +
drivers/phy/Makefile | 5 +
drivers/phy/phy-core.c | 574 +++++++++++++++++++++++++++++
include/linux/phy/phy.h | 218 +++++++++++
9 files changed, 944 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-phy
create mode 100644 Documentation/phy.txt
create mode 100644 drivers/phy/Kconfig
create mode 100644 drivers/phy/Makefile
create mode 100644 drivers/phy/phy-core.c
create mode 100644 include/linux/phy/phy.h

diff --git a/Documentation/ABI/testing/sysfs-class-phy b/Documentation/ABI/testing/sysfs-class-phy
new file mode 100644
index 0000000..47f17c9
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-phy
@@ -0,0 +1,15 @@
+What: /sys/class/phy/<phy>/label
+Date: Feb 2013
+KernelVersion: 3.10
+Contact: Kishon Vijay Abraham I <[email protected]>
+Description:
+ This is a read-only file for getting the label of the phy.
+
+What: /sys/class/phy/<phy>/phy_bind
+Date: Feb 2013
+KernelVersion: 3.10
+Contact: Kishon Vijay Abraham I <[email protected]>
+Description:
+ This is a read-only file for reading the phy binding
+ information. It contains the device name of the controller,
+ the index and the device name of the PHY in that order.
diff --git a/Documentation/phy.txt b/Documentation/phy.txt
new file mode 100644
index 0000000..6ba3192
--- /dev/null
+++ b/Documentation/phy.txt
@@ -0,0 +1,108 @@
+ The Generic PHY Framework
+ Kishon Vijay Abraham I <[email protected]>
+
+This document explains the Generic PHY Framework along with the APIs provided,
+how-to-use, current status and the future work list.
+
+1. Introduction
+
+*PHY* is the abbreviation for physical layer. It is used to connect a device
+to the physical medium e.g., the USB controller has a PHY to provide functions
+such as serialization, de-serialization, encoding, decoding and is responsible
+for obtaining the required data transmission rate. Note that some USB
+controller has PHY functionality embedded into it and others use an external
+PHY. Other peripherals that uses a PHY include Wireless LAN, Ethernet,
+SATA etc.
+
+The intention of creating this framework is to bring the phy drivers spread
+all over the Linux kernel to drivers/phy to increase code re-use and to
+increase code maintainability.
+
+This framework will be of use only to devices that uses external PHY (PHY
+functionality is not embedded within the controller).
+
+2. Creating the PHY
+
+The PHY driver should create the PHY in order for other peripheral controllers
+to make use of it. The PHY framework provides 2 APIs to create the PHY.
+
+struct phy *phy_create(struct device *dev, const char *label,
+ struct device_node *of_node, int type, struct phy_ops *ops,
+ void *priv);
+struct phy *devm_phy_create(struct device *dev, const char *label,
+ struct device_node *of_node, int type, struct phy_ops *ops,
+ void *priv);
+
+The PHY drivers can use one of the above 2 APIs to create the PHY by passing
+the device pointer, label, device node, type, phy ops and a driver data.
+phy_ops is a set of function pointers for performing PHY operations such as
+init, exit, suspend, resume, poweron and shutdown.
+
+3. Binding the PHY to the controller
+
+The framework provides an API for binding the controller to the PHY in the
+case of non dt boot.
+
+struct phy_bind *phy_bind(const char *dev_name, int index,
+ const char *phy_dev_name);
+
+The API fills the phy_bind structure with the dev_name (device name of the
+controller), index and phy_dev_name (device name of the PHY). This will
+be used when the controller requests this phy. This API should be used by
+platform specific initialization code (board file).
+
+In the case of dt boot, the binding information should be added in the dt
+data of the controller.
+
+4. Getting a reference to the PHY
+
+Before the controller can make use of the PHY, it has to get a reference to
+it. This framework provides 6 APIs to get a reference to the PHY.
+
+struct phy *phy_get(struct device *dev, int index);
+struct phy *devm_phy_get(struct device *dev, int index);
+struct phy *of_phy_get(struct device *dev, const char *phandle, int index);
+struct phy *devm_of_phy_get(struct device *dev, const char *phandle, int index);
+struct phy *of_phy_get_byname(struct device *dev, const char *string);
+struct phy *devm_of_phy_get_byname(struct device *dev, const char *string);
+
+phy_get and devm_phy_get can be used to get the PHY in non-dt boot. This API
+uses the binding information added using the phy_bind API to find and return
+the appropriate PHY. The only difference between the two APIs is that
+devm_phy_get associates the device with the PHY using devres on successful PHY
+get. On driver detach, release function is invoked on the the devres data and
+devres data is freed.
+
+of_phy_get and devm_of_phy_get can be used to get the PHY in dt boot. These
+APIs take the phandle and index to get a reference to the PHY. The only
+difference between the two APIs is that devm_of_phy_get associates the device
+with the PHY using devres on successful phy get. On driver detach, release
+function is invoked on the devres data and it is freed.
+
+of_phy_get_byname and devm_of_phy_get_byname can also be used to get the PHY
+in dt boot. It is same as the above API except that the user has to pass the
+phy name as filled in "phy-names" phandle in dt data and the framework will
+find the index and get the PHY.
+
+5. Releasing a reference to the PHY
+
+When the controller no longer needs the PHY, it has to release the reference
+to the PHY it has obtained using the APIs mentioned in the above section. The
+PHY framework provides 2 APIS to release a reference to the PHY.
+
+void phy_put(struct phy *phy);
+void devm_phy_put(struct device *dev, struct phy *phy);
+
+Both these APIs are used to release a reference to the PHY and devm_phy_put
+destroys the devres associated with this PHY.
+
+6. Destroying the PHY
+
+When the driver that created the PHY is unloaded, it should destroy the PHY it
+created using one of the following 2 APIs.
+
+void phy_destroy(struct phy *phy);
+void devm_phy_destroy(struct device *dev, struct phy *phy);
+
+Both these APIs destroys the PHY and devm_phy_destroy destroys the devres
+associated with this PHY.
diff --git a/MAINTAINERS b/MAINTAINERS
index 50b4d73..efb0498 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3474,6 +3474,13 @@ S: Maintained
F: include/asm-generic
F: include/uapi/asm-generic

+GENERIC PHY FRAMEWORK
+M: Kishon Vijay Abraham I <[email protected]>
+L: [email protected]
+S: Supported
+F: drivers/phy/
+F: include/linux/phy/
+
GENERIC UIO DRIVER FOR PCI DEVICES
M: "Michael S. Tsirkin" <[email protected]>
L: [email protected]
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 202fa6d..ad2c374a 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -162,4 +162,6 @@ source "drivers/irqchip/Kconfig"

source "drivers/ipack/Kconfig"

+source "drivers/phy/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index dce39a9..9da8321 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -45,6 +45,8 @@ obj-y += char/
# gpu/ comes after char for AGP vs DRM startup
obj-y += gpu/

+obj-y += phy/
+
obj-$(CONFIG_CONNECTOR) += connector/

# i810fb and intelfb depend on char/agp/
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
new file mode 100644
index 0000000..5f85909
--- /dev/null
+++ b/drivers/phy/Kconfig
@@ -0,0 +1,13 @@
+#
+# PHY
+#
+
+menuconfig GENERIC_PHY
+ tristate "PHY Subsystem"
+ help
+ Generic PHY support.
+
+ This framework is designed to provide a generic interface for 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.
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
new file mode 100644
index 0000000..9e9560f
--- /dev/null
+++ b/drivers/phy/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for the phy drivers.
+#
+
+obj-$(CONFIG_GENERIC_PHY) += phy-core.o
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
new file mode 100644
index 0000000..1c87592
--- /dev/null
+++ b/drivers/phy/phy-core.c
@@ -0,0 +1,574 @@
+/*
+ * phy-core.c -- Generic Phy framework.
+ *
+ * Copyright (C) 2013 Texas Instruments
+ *
+ * Author: Kishon Vijay Abraham I <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/kernel.h>
+#include <linux/export.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+
+static struct class *phy_class;
+static DEFINE_MUTEX(phy_bind_mutex);
+static LIST_HEAD(phy_bind_list);
+
+static void devm_phy_release(struct device *dev, void *res)
+{
+ struct phy *phy = *(struct phy **)res;
+
+ phy_put(phy);
+}
+
+static void devm_phy_consume(struct device *dev, void *res)
+{
+ struct phy *phy = *(struct phy **)res;
+
+ phy_destroy(phy);
+}
+
+static int devm_phy_match(struct device *dev, void *res, void *match_data)
+{
+ return res == match_data;
+}
+
+static struct phy *phy_lookup(struct device *dev, int index)
+{
+ struct phy_bind *phy_bind = NULL;
+
+ list_for_each_entry(phy_bind, &phy_bind_list, list) {
+ if (!(strcmp(phy_bind->dev_name, dev_name(dev)) &&
+ phy_bind->index == index)) {
+ if (phy_bind->phy)
+ return phy_bind->phy;
+ else
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+ }
+
+ return ERR_PTR(-ENODEV);
+}
+
+static struct phy *of_phy_lookup(struct device *dev, struct device_node *node)
+{
+ struct phy *phy;
+ struct class_dev_iter iter;
+
+ class_dev_iter_init(&iter, phy_class, NULL, NULL);
+ while ((dev = class_dev_iter_next(&iter))) {
+ phy = container_of(dev, struct phy, dev);
+ if (node != phy->of_node)
+ continue;
+
+ class_dev_iter_exit(&iter);
+ return phy;
+ }
+
+ class_dev_iter_exit(&iter);
+ return ERR_PTR(-EPROBE_DEFER);
+}
+
+/**
+ * phy_put - release the PHY
+ * @phy: the phy returned by phy_get()
+ *
+ * Releases a refcount the caller received from phy_get().
+ */
+void phy_put(struct phy *phy)
+{
+ if (phy) {
+ module_put(phy->ops->owner);
+ put_device(&phy->dev);
+ }
+}
+EXPORT_SYMBOL_GPL(phy_put);
+
+/**
+ * devm_phy_put - release the PHY
+ * @dev: device that wants to release this phy
+ * @phy: the phy returned by devm_phy_get()
+ *
+ * destroys the devres associated with this phy and invokes phy_put
+ * to release the phy.
+ */
+void devm_phy_put(struct device *dev, struct phy *phy)
+{
+ int r;
+
+ r = devres_destroy(dev, devm_phy_release, devm_phy_match, phy);
+ dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n");
+}
+EXPORT_SYMBOL_GPL(devm_phy_put);
+
+/**
+ * of_phy_get - lookup and obtain a reference to a phy by phandle
+ * @dev: device that requests this phy
+ * @index - the index of the phy
+ *
+ * Returns the phy associated with the given phandle value,
+ * after getting a refcount to it or -ENODEV if there is no such phy or
+ * -EPROBE_DEFER if there is a phandle to the phy, but the device is
+ * not yet loaded.
+ */
+struct phy *of_phy_get(struct device *dev, int index)
+{
+ struct phy *phy = NULL;
+ struct phy_bind *phy_map = NULL;
+ struct device_node *node;
+
+ if (!dev->of_node) {
+ dev_dbg(dev, "device does not have a device node entry\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ node = of_parse_phandle(dev->of_node, "phy", index);
+ if (!node) {
+ dev_dbg(dev, "failed to get phy in %s node\n",
+ dev->of_node->full_name);
+ return ERR_PTR(-ENODEV);
+ }
+
+ phy = of_phy_lookup(dev, node);
+ if (IS_ERR(phy) || !try_module_get(phy->ops->owner)) {
+ phy = ERR_PTR(-EPROBE_DEFER);
+ goto err0;
+ }
+
+ mutex_lock(&phy_bind_mutex);
+ phy_map = phy_bind(dev_name(dev), index, dev_name(&phy->dev));
+ if (!IS_ERR(phy_map)) {
+ phy_map->phy = phy;
+ phy_map->auto_bind = true;
+ }
+ mutex_unlock(&phy_bind_mutex);
+
+ get_device(&phy->dev);
+
+err0:
+ of_node_put(node);
+
+ return phy;
+}
+EXPORT_SYMBOL_GPL(of_phy_get);
+
+/**
+ * devm_of_phy_get - lookup and obtain a reference to a phy by phandle
+ * @dev: device that requests this phy
+ * @index - the index of the phy
+ *
+ * Calls of_phy_get to get a reference to the PHY and passes on the return
+ * value of of_phy_get. While at that, it also associates the device with the
+ * phy using devres on successful phy get. On driver detach, release function
+ * is invoked on the the devres data and devres data is freed.
+ */
+struct phy *devm_of_phy_get(struct device *dev, int index)
+{
+ struct phy *phy = NULL, **ptr;
+
+ ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ phy = of_phy_get(dev, index);
+ if (!IS_ERR(phy)) {
+ *ptr = phy;
+ devres_add(dev, ptr);
+ } else {
+ devres_free(ptr);
+ }
+
+ return phy;
+}
+EXPORT_SYMBOL_GPL(devm_of_phy_get);
+
+/**
+ * of_phy_get_byname - lookup and obtain a reference to a phy by name
+ * @dev: device that requests this phy
+ * @string - the phy name as given in the dt data
+ *
+ * Calls of_phy_get to get a reference to the PHY and passes on the return
+ * value of of_phy_get.
+ */
+struct phy *of_phy_get_byname(struct device *dev, const char *string)
+{
+ int index;
+
+ if (!dev->of_node) {
+ dev_dbg(dev, "device does not have a device node entry\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ index = of_property_match_string(dev->of_node, "phy-names", string);
+
+ return of_phy_get(dev, index);
+}
+EXPORT_SYMBOL_GPL(of_phy_get_byname);
+
+/**
+ * devm_of_phy_get_byname - lookup and obtain a reference to a phy by name
+ * @dev: device that requests this phy
+ * @string - the phy name as given in the dt data
+ *
+ * Calls devm_of_phy_get (which associates the device with the phy using devres
+ * on successful phy get) and passes on the return value of devm_of_phy_get.
+ */
+struct phy *devm_of_phy_get_byname(struct device *dev, const char *string)
+{
+ int index;
+
+ if (!dev->of_node) {
+ dev_dbg(dev, "device does not have a device node entry\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ index = of_property_match_string(dev->of_node, "phy-names", string);
+
+ return devm_of_phy_get(dev, index);
+}
+EXPORT_SYMBOL_GPL(devm_of_phy_get_byname);
+
+/**
+ * phy_get - lookup and obtain a reference to a phy.
+ * @dev: device that requests this phy
+ * @index: the index of the phy
+ *
+ * Returns the phy driver, after getting a refcount to it; or
+ * -ENODEV if there is no such phy. The caller is responsible for
+ * calling phy_put() to release that count.
+ */
+struct phy *phy_get(struct device *dev, int index)
+{
+ struct phy *phy = NULL;
+
+ phy = phy_lookup(dev, index);
+ if (IS_ERR(phy)) {
+ dev_err(dev, "unable to find phy\n");
+ goto err0;
+ }
+
+ if (!try_module_get(phy->ops->owner)) {
+ phy = ERR_PTR(-EPROBE_DEFER);
+ goto err0;
+ }
+
+ get_device(&phy->dev);
+
+err0:
+ return phy;
+}
+EXPORT_SYMBOL_GPL(phy_get);
+
+/**
+ * devm_phy_get - lookup and obtain a reference to a phy.
+ * @dev: device that requests this phy
+ * @index: the index of the phy
+ *
+ * Gets the phy using phy_get(), and associates a device with it using
+ * devres. On driver detach, release function is invoked on the devres data,
+ * then, devres data is freed.
+ */
+struct phy *devm_phy_get(struct device *dev, int index)
+{
+ struct phy **ptr, *phy;
+
+ ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ phy = phy_get(dev, index);
+ if (!IS_ERR(phy)) {
+ *ptr = phy;
+ devres_add(dev, ptr);
+ } else {
+ devres_free(ptr);
+ }
+
+ return phy;
+}
+EXPORT_SYMBOL_GPL(devm_phy_get);
+
+/**
+ * phy_create - create a new phy
+ * @dev: device that is creating the new phy
+ * @label: label given to phy
+ * @of_node: device node of the phy
+ * @type: specifies the phy type
+ * @ops: function pointers for performing phy operations
+ * @priv: private data from phy driver
+ *
+ * Called to create a phy using phy framework.
+ */
+struct phy *phy_create(struct device *dev, const char *label,
+ struct device_node *of_node, int type, struct phy_ops *ops,
+ void *priv)
+{
+ int ret;
+ struct phy *phy;
+ struct phy_bind *phy_bind;
+ const char *devname = NULL;
+
+ if (!dev) {
+ dev_err(dev, "no device provided for PHY\n");
+ ret = -EINVAL;
+ goto err0;
+ }
+
+ if (!ops || !priv) {
+ dev_err(dev, "no PHY ops/PHY data provided\n");
+ ret = -EINVAL;
+ goto err0;
+ }
+
+ phy = kzalloc(sizeof(*phy), GFP_KERNEL);
+ if (!phy) {
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ devname = dev_name(dev);
+ device_initialize(&phy->dev);
+
+ phy->dev.class = phy_class;
+ phy->dev.parent = dev;
+ phy->label = label;
+ phy->of_node = of_node;
+ phy->type = type;
+ phy->ops = ops;
+
+ dev_set_drvdata(&phy->dev, priv);
+
+ ret = dev_set_name(&phy->dev, "%s", devname);
+ if (ret)
+ goto err1;
+
+ mutex_lock(&phy_bind_mutex);
+ list_for_each_entry(phy_bind, &phy_bind_list, list)
+ if (!(strcmp(phy_bind->phy_dev_name, devname)))
+ phy_bind->phy = phy;
+ mutex_unlock(&phy_bind_mutex);
+
+ ret = device_add(&phy->dev);
+ if (ret)
+ goto err2;
+
+ return phy;
+
+err2:
+ phy_bind->phy = NULL;
+
+err1:
+ put_device(&phy->dev);
+ kfree(phy);
+
+err0:
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(phy_create);
+
+/**
+ * devm_phy_create - create a new phy
+ * @dev: device that is creating the new phy
+ * @dev: device that is creating the new phy
+ * @label: label given to phy
+ * @of_node: device node of the phy
+ * @type: specifies the phy type
+ * @ops: function pointers for performing phy operations
+ * @priv: private data from phy driver
+ *
+ * Creates a new PHY device adding it to the PHY class.
+ * While at that, it also associates the device with the phy using devres.
+ * On driver detach, release function is invoked on the devres data,
+ * then, devres data is freed.
+ */
+struct phy *devm_phy_create(struct device *dev, const char *label,
+ struct device_node *of_node, int type, struct phy_ops *ops,
+ void *priv)
+{
+ struct phy **ptr, *phy;
+
+ ptr = devres_alloc(devm_phy_consume, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ phy = phy_create(dev, label, of_node, type, ops, priv);
+ if (!IS_ERR(phy)) {
+ *ptr = phy;
+ devres_add(dev, ptr);
+ } else {
+ devres_free(ptr);
+ }
+
+ return phy;
+}
+EXPORT_SYMBOL_GPL(devm_phy_create);
+
+/**
+ * phy_destroy - destroy the phy
+ * @phy: the phy to be destroyed
+ *
+ * Called to destroy the phy.
+ */
+void phy_destroy(struct phy *phy)
+{
+ struct phy_bind *phy_bind;
+
+ mutex_lock(&phy_bind_mutex);
+ list_for_each_entry(phy_bind, &phy_bind_list, list) {
+ if (phy_bind->phy == phy)
+ phy_bind->phy = NULL;
+
+ if (phy_bind->auto_bind) {
+ list_del(&phy_bind->list);
+ kfree(phy_bind);
+ }
+ }
+ mutex_unlock(&phy_bind_mutex);
+
+ device_unregister(&phy->dev);
+}
+EXPORT_SYMBOL_GPL(phy_destroy);
+
+/**
+ * devm_phy_destroy - destroy the PHY
+ * @dev: device that wants to release this phy
+ * @phy: the phy returned by devm_phy_get()
+ *
+ * destroys the devres associated with this phy and invokes phy_destroy
+ * to destroy the phy.
+ */
+void devm_phy_destroy(struct device *dev, struct phy *phy)
+{
+ int r;
+
+ r = devres_destroy(dev, devm_phy_consume, devm_phy_match, phy);
+ dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n");
+}
+EXPORT_SYMBOL_GPL(devm_phy_destroy);
+
+/**
+ * phy_bind - bind the phy and the controller that uses the phy
+ * @dev_name: the device name of the device that will bind to the phy
+ * @index: index to specify the port number
+ * @phy_dev_name: the device name of the phy
+ *
+ * Fills the phy_bind structure with the dev_name and phy_dev_name. This will
+ * be used when the phy driver registers the phy and when the controller
+ * requests this phy.
+ *
+ * To be used by platform specific initialization code.
+ */
+struct phy_bind *phy_bind(const char *dev_name, int index,
+ const char *phy_dev_name)
+{
+ struct phy_bind *phy_bind;
+
+ phy_bind = kzalloc(sizeof(*phy_bind), GFP_KERNEL);
+ if (!phy_bind)
+ return ERR_PTR(-ENOMEM);
+
+ phy_bind->dev_name = dev_name;
+ phy_bind->phy_dev_name = phy_dev_name;
+ phy_bind->index = index;
+ phy_bind->auto_bind = false;
+
+ list_add_tail(&phy_bind->list, &phy_bind_list);
+
+ return phy_bind;
+}
+EXPORT_SYMBOL_GPL(phy_bind);
+
+static ssize_t phy_name_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct phy *phy;
+
+ phy = container_of(dev, struct phy, dev);
+
+ return sprintf(buf, "%s\n", phy->label);
+}
+
+static ssize_t phy_bind_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct phy_bind *phy_bind;
+ struct phy *phy;
+ char *p = buf;
+ int len;
+
+ phy = container_of(dev, struct phy, dev);
+
+ list_for_each_entry(phy_bind, &phy_bind_list, list)
+ if (phy_bind->phy == phy)
+ p += sprintf(p, "%s %d %s\n", phy_bind->dev_name,
+ phy_bind->index, phy_bind->phy_dev_name);
+ len = (p - buf);
+
+ return len;
+}
+
+static struct device_attribute phy_dev_attrs[] = {
+ __ATTR(label, 0444, phy_name_show, NULL),
+ __ATTR(phy_bind, 0444, phy_bind_show, NULL),
+ __ATTR_NULL,
+};
+
+/**
+ * phy_release - release the phy
+ * @dev: the dev member within phy
+ *
+ * when the last reference to the device is removed; it is called
+ * from the embedded kobject as release method.
+ */
+static void phy_release(struct device *dev)
+{
+ struct phy *phy;
+
+ phy = container_of(dev, struct phy, dev);
+ dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
+ kfree(phy);
+}
+
+static int __init phy_core_init(void)
+{
+ phy_class = class_create(THIS_MODULE, "phy");
+ if (IS_ERR(phy_class)) {
+ pr_err("failed to create phy class --> %ld\n",
+ PTR_ERR(phy_class));
+ return PTR_ERR(phy_class);
+ }
+
+ phy_class->dev_release = phy_release;
+ phy_class->dev_attrs = phy_dev_attrs;
+
+ return 0;
+}
+subsys_initcall(phy_core_init);
+
+static void __exit phy_core_exit(void)
+{
+ class_destroy(phy_class);
+}
+module_exit(phy_core_exit);
+
+MODULE_DESCRIPTION("Generic PHY Framework");
+MODULE_AUTHOR("Kishon Vijay Abraham I <[email protected]>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
new file mode 100644
index 0000000..76952c6
--- /dev/null
+++ b/include/linux/phy/phy.h
@@ -0,0 +1,218 @@
+/*
+ * phy.h -- generic phy header file
+ *
+ * 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: Kishon Vijay Abraham I <[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.
+ *
+ */
+
+#ifndef __DRIVERS_PHY_H
+#define __DRIVERS_PHY_H
+
+#include <linux/err.h>
+
+struct phy;
+
+/**
+ * struct phy_ops - set of function pointers for performing phy operations
+ * @init: operation to be performed for initializing phy
+ * @exit: operation to be performed while exiting
+ * @suspend: suspending the phy
+ * @resume: resuming the phy
+ * @poweron: powering on the phy
+ * @shutdown: shutting down the phy
+ * @owner: the module owner containing the ops
+ */
+struct phy_ops {
+ int (*init)(struct phy *phy);
+ int (*exit)(struct phy *phy);
+ int (*suspend)(struct phy *phy);
+ int (*resume)(struct phy *phy);
+ int (*poweron)(struct phy *phy);
+ int (*shutdown)(struct phy *phy);
+ struct module *owner;
+};
+
+/**
+ * struct phy - represent the phy device
+ * @dev: phy device
+ * @label: label given to phy
+ * @type: specifies the phy type
+ * @of_node: device node of the phy
+ * @ops: function pointers for performing phy operations
+ */
+struct phy {
+ struct device dev;
+ const char *label;
+ int type;
+ struct bus_type *bus;
+ struct device_node *of_node;
+ struct phy_ops *ops;
+};
+
+/**
+ * struct phy_bind - represent the binding for the phy
+ * @dev_name: the device name of the device that will bind to the phy
+ * @phy_dev_name: the device name of the phy
+ * @index: used if a single controller uses multiple phys
+ * @auto_bind: tells if the binding is done explicitly from board file or not
+ * @phy: reference to the phy
+ * @list: to maintain a linked list of the binding information
+ */
+struct phy_bind {
+ const char *dev_name;
+ const char *phy_dev_name;
+ int index;
+ int auto_bind:1;
+ struct phy *phy;
+ struct list_head list;
+};
+
+#if IS_ENABLED(CONFIG_GENERIC_PHY)
+extern struct phy *phy_get(struct device *dev, int index);
+extern struct phy *devm_phy_get(struct device *dev, int index);
+extern struct phy *of_phy_get(struct device *dev, int index);
+extern struct phy *devm_of_phy_get(struct device *dev, int index);
+extern struct phy *of_phy_get_byname(struct device *dev, const char *string);
+extern struct phy *devm_of_phy_get_byname(struct device *dev,
+ const char *string);
+extern void phy_put(struct phy *phy);
+extern void devm_phy_put(struct device *dev, struct phy *phy);
+extern struct phy *phy_create(struct device *dev, const char *label,
+ struct device_node *of_node, int type, struct phy_ops *ops,
+ void *priv);
+extern struct phy *devm_phy_create(struct device *dev, const char *label,
+ struct device_node *of_node, int type, struct phy_ops *ops,
+ void *priv);
+extern void phy_destroy(struct phy *phy);
+extern void devm_phy_destroy(struct device *dev, struct phy *phy);
+extern struct phy_bind *phy_bind(const char *dev_name, int index,
+ const char *phy_dev_name);
+#else
+static inline struct phy *phy_get(struct device *dev, int index)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline struct phy *devm_phy_get(struct device *dev, int index)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline struct phy *of_phy_get(struct device *dev, int index)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline struct phy *devm_of_phy_get(struct device *dev, int index)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline struct phy *of_phy_get_byname(struct device *dev,
+ const char *string)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline struct phy *devm_of_phy_get_byname(struct device *dev,
+ const char *string)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void phy_put(struct phy *phy)
+{
+}
+
+static inline void devm_phy_put(struct device *dev, struct phy *phy)
+{
+}
+
+static inline struct phy *phy_create(struct device *dev, const char *label,
+ struct device_node *of_node, int type, struct phy_ops *ops,
+ void *priv)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline struct phy *devm_phy_create(struct device *dev, const char *label,
+ struct device_node *of_node, int type, struct phy_ops *ops,
+ void *priv)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void phy_destroy(struct phy *phy)
+{
+}
+
+static inline void devm_phy_destroy(struct device *dev, struct phy *phy)
+{
+}
+
+static inline struct phy_bind *phy_bind(const char *dev_name, int index,
+ const char *phy_dev_name)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+#endif
+
+static inline int phy_init(struct phy *phy)
+{
+ if (phy->ops->init)
+ return phy->ops->init(phy);
+
+ return -EINVAL;
+}
+
+static inline int phy_exit(struct phy *phy)
+{
+ if (phy->ops->exit)
+ return phy->ops->exit(phy);
+
+ return -EINVAL;
+}
+
+static inline int phy_suspend(struct phy *phy)
+{
+ if (phy->ops->suspend)
+ return phy->ops->suspend(phy);
+
+ return -EINVAL;
+}
+
+static inline int phy_resume(struct phy *phy)
+{
+ if (phy->ops->resume)
+ return phy->ops->resume(phy);
+
+ return -EINVAL;
+}
+
+static inline int phy_poweron(struct phy *phy)
+{
+ if (phy->ops->poweron)
+ return phy->ops->poweron(phy);
+
+ return -EINVAL;
+}
+
+static inline int phy_shutdown(struct phy *phy)
+{
+ if (phy->ops->shutdown)
+ return phy->ops->shutdown(phy);
+
+ return -EINVAL;
+}
+#endif /* __DRIVERS_PHY_H */
--
1.7.10.4

2013-03-20 09:13:16

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: [PATCH v3 6/6] usb: musb: omap2430: use the new generic PHY framework

Use the generic PHY framework API to get the PHY. The usb_phy_set_suspend
and usb_phy_set_resume is replaced with phy_suspend and phy_resume to
align with the new PHY framework.

musb->xceiv can't be removed as of now because musb core uses xceiv.state and
xceiv.otg. Once there is a separate state machine to handle otg, these can be
moved out of xceiv and then we can start using the generic PHY framework.

Signed-off-by: Kishon Vijay Abraham I <[email protected]>
---
drivers/usb/musb/musb_core.h | 2 ++
drivers/usb/musb/omap2430.c | 22 ++++++++++++++++------
2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 7fb4819..78251fd 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -46,6 +46,7 @@
#include <linux/usb.h>
#include <linux/usb/otg.h>
#include <linux/usb/musb.h>
+#include <linux/phy/phy.h>

struct musb;
struct musb_hw_ep;
@@ -357,6 +358,7 @@ struct musb {
u16 int_tx;

struct usb_phy *xceiv;
+ struct phy *phy;

int nIrq;
unsigned irq_wake:1;
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 1a42a45..55f071d 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -349,14 +349,24 @@ static int omap2430_musb_init(struct musb *musb)
* up through ULPI. TWL4030-family PMICs include one,
* which needs a driver, drivers aren't always needed.
*/
- if (dev->parent->of_node)
+ if (dev->parent->of_node) {
+ musb->phy = devm_of_phy_get_byname(dev->parent, "usb2-phy");
+
+ /* We can't totally remove musb->xceiv as of now because
+ * musb core uses xceiv.state and xceiv.otg. Once we have
+ * a separate state machine to handle otg, these can be moved
+ * out of xceiv and then we can start using the generic PHY
+ * framework
+ */
musb->xceiv = devm_usb_get_phy_by_phandle(dev->parent,
"usb-phy", 0);
- else
+ } else {
musb->xceiv = devm_usb_get_phy_dev(dev, 0);
+ musb->phy = devm_phy_get(dev, 0);
+ }

- if (IS_ERR_OR_NULL(musb->xceiv)) {
- pr_err("HS USB OTG: no transceiver configured\n");
+ if (IS_ERR_OR_NULL(musb->xceiv) || IS_ERR_OR_NULL(musb->phy)) {
+ dev_err(dev, "no transceiver configured\n");
return -EPROBE_DEFER;
}

@@ -612,7 +622,7 @@ static int omap2430_runtime_suspend(struct device *dev)
OTG_INTERFSEL);

omap2430_low_level_exit(musb);
- usb_phy_set_suspend(musb->xceiv, 1);
+ phy_suspend(musb->phy);
}

return 0;
@@ -628,7 +638,7 @@ static int omap2430_runtime_resume(struct device *dev)
musb_writel(musb->mregs, OTG_INTERFSEL,
musb->context.otg_interfsel);

- usb_phy_set_suspend(musb->xceiv, 0);
+ phy_resume(musb->phy);
}

return 0;
--
1.7.10.4

2013-03-20 09:13:14

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: [PATCH v3 4/6] ARM: OMAP: USB: Add phy binding information

In order for controllers to get PHY in case of non dt boot, the phy
binding information should be added in the platform specific
initialization code using phy_bind. The previously added usb_bind_phy
can't be removed yet because the musb controller continues to use the
old PHY library which has OTG in it (struct usb_phy has struct usb_otg
as member). Until we have a separate OTG state machine to handle all of
that, the new generic PHY framework and the old phy library will co-exist.

Signed-off-by: Kishon Vijay Abraham I <[email protected]>
---
arch/arm/mach-omap2/board-2430sdp.c | 2 ++
arch/arm/mach-omap2/board-3430sdp.c | 2 ++
arch/arm/mach-omap2/board-4430sdp.c | 2 ++
arch/arm/mach-omap2/board-cm-t35.c | 2 ++
arch/arm/mach-omap2/board-devkit8000.c | 2 ++
arch/arm/mach-omap2/board-igep0020.c | 2 ++
arch/arm/mach-omap2/board-ldp.c | 2 ++
arch/arm/mach-omap2/board-omap3beagle.c | 2 ++
arch/arm/mach-omap2/board-omap3evm.c | 2 ++
arch/arm/mach-omap2/board-omap3logic.c | 2 ++
arch/arm/mach-omap2/board-omap3pandora.c | 2 ++
arch/arm/mach-omap2/board-omap3stalker.c | 2 ++
arch/arm/mach-omap2/board-omap3touchbook.c | 2 ++
arch/arm/mach-omap2/board-omap4panda.c | 2 ++
arch/arm/mach-omap2/board-overo.c | 2 ++
arch/arm/mach-omap2/board-rm680.c | 2 ++
arch/arm/mach-omap2/board-rx51.c | 2 ++
arch/arm/mach-omap2/board-zoom-peripherals.c | 2 ++
18 files changed, 36 insertions(+)

diff --git a/arch/arm/mach-omap2/board-2430sdp.c b/arch/arm/mach-omap2/board-2430sdp.c
index a3e0aaa..271458b 100644
--- a/arch/arm/mach-omap2/board-2430sdp.c
+++ b/arch/arm/mach-omap2/board-2430sdp.c
@@ -28,6 +28,7 @@
#include <linux/io.h>
#include <linux/gpio.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -265,6 +266,7 @@ static void __init omap_2430sdp_init(void)

omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP);
usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);

board_smc91x_init();
diff --git a/arch/arm/mach-omap2/board-3430sdp.c b/arch/arm/mach-omap2/board-3430sdp.c
index ce812de..bf6ce1d 100644
--- a/arch/arm/mach-omap2/board-3430sdp.c
+++ b/arch/arm/mach-omap2/board-3430sdp.c
@@ -27,6 +27,7 @@
#include <linux/platform_data/spi-omap2-mcspi.h>
#include <linux/platform_data/omap-twl4030.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -601,6 +602,7 @@ static void __init omap_3430sdp_init(void)
omap_serial_init();
omap_sdrc_init(hyb18m512160af6_sdrc_params, NULL);
usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);
board_smc91x_init();
board_flash_init(sdp_flash_partitions, chip_sel_3430, 0);
diff --git a/arch/arm/mach-omap2/board-4430sdp.c b/arch/arm/mach-omap2/board-4430sdp.c
index 35f3ad0..1a236cb 100644
--- a/arch/arm/mach-omap2/board-4430sdp.c
+++ b/arch/arm/mach-omap2/board-4430sdp.c
@@ -32,6 +32,7 @@
#include <linux/platform_data/omap4-keypad.h>
#include <linux/usb/musb.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -725,6 +726,7 @@ static void __init omap_4430sdp_init(void)
omap4_twl6030_hsmmc_init(mmc);

usb_bind_phy("musb-hdrc.0.auto", 0, "omap-usb2.1.auto");
+ phy_bind("musb-hdrc.0.auto", 0, "omap-usb2.1.auto");
usb_musb_init(&musb_board_data);

status = omap_ethernet_init();
diff --git a/arch/arm/mach-omap2/board-cm-t35.c b/arch/arm/mach-omap2/board-cm-t35.c
index af2bb21..6a2615a 100644
--- a/arch/arm/mach-omap2/board-cm-t35.c
+++ b/arch/arm/mach-omap2/board-cm-t35.c
@@ -31,6 +31,7 @@
#include <linux/regulator/machine.h>
#include <linux/mmc/host.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <linux/spi/spi.h>
#include <linux/spi/tdo24m.h>
@@ -726,6 +727,7 @@ static void __init cm_t3x_common_init(void)
omap_twl4030_audio_init("cm-t3x", NULL);

usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);
cm_t35_init_usbh();
cm_t35_init_camera();
diff --git a/arch/arm/mach-omap2/board-devkit8000.c b/arch/arm/mach-omap2/board-devkit8000.c
index 53056c3..4ca7d23 100644
--- a/arch/arm/mach-omap2/board-devkit8000.c
+++ b/arch/arm/mach-omap2/board-devkit8000.c
@@ -30,6 +30,7 @@
#include <linux/mtd/nand.h>
#include <linux/mmc/host.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <linux/regulator/machine.h>
#include <linux/i2c/twl.h>
@@ -624,6 +625,7 @@ static void __init devkit8000_init(void)
omap_ads7846_init(2, OMAP3_DEVKIT_TS_GPIO, 0, NULL);

usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);
usbhs_init(&usbhs_bdata);
board_nand_init(devkit8000_nand_partitions,
diff --git a/arch/arm/mach-omap2/board-igep0020.c b/arch/arm/mach-omap2/board-igep0020.c
index bf92678..f0c28bc 100644
--- a/arch/arm/mach-omap2/board-igep0020.c
+++ b/arch/arm/mach-omap2/board-igep0020.c
@@ -19,6 +19,7 @@
#include <linux/interrupt.h>
#include <linux/input.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <linux/regulator/machine.h>
#include <linux/regulator/fixed.h>
@@ -627,6 +628,7 @@ static void __init igep_init(void)
omap_sdrc_init(m65kxxxxam_sdrc_params,
m65kxxxxam_sdrc_params);
usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);

igep_flash_init();
diff --git a/arch/arm/mach-omap2/board-ldp.c b/arch/arm/mach-omap2/board-ldp.c
index b12fe96..3f43626 100644
--- a/arch/arm/mach-omap2/board-ldp.c
+++ b/arch/arm/mach-omap2/board-ldp.c
@@ -29,6 +29,7 @@
#include <linux/smsc911x.h>
#include <linux/mmc/host.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>
#include <linux/platform_data/spi-omap2-mcspi.h>

#include <asm/mach-types.h>
@@ -420,6 +421,7 @@ static void __init omap_ldp_init(void)
omap_serial_init();
omap_sdrc_init(NULL, NULL);
usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);
board_nand_init(ldp_nand_partitions, ARRAY_SIZE(ldp_nand_partitions),
ZOOM_NAND_CS, 0, nand_default_timings);
diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index c3558f9..5dc0f81 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -33,6 +33,7 @@
#include <linux/mtd/nand.h>
#include <linux/mmc/host.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <linux/regulator/machine.h>
#include <linux/i2c/twl.h>
@@ -542,6 +543,7 @@ static void __init omap3_beagle_init(void)
mt46h32m32lf6_sdrc_params);

usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);
usbhs_init(&usbhs_bdata);
board_nand_init(omap3beagle_nand_partitions,
diff --git a/arch/arm/mach-omap2/board-omap3evm.c b/arch/arm/mach-omap2/board-omap3evm.c
index 48789e0..47e68cd 100644
--- a/arch/arm/mach-omap2/board-omap3evm.c
+++ b/arch/arm/mach-omap2/board-omap3evm.c
@@ -42,6 +42,7 @@
#include <linux/mmc/host.h>
#include <linux/export.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -736,6 +737,7 @@ static void __init omap3_evm_init(void)
usbhs_bdata.reset_gpio_port[1] = 135;
}
usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(&musb_board_data);
usbhs_init(&usbhs_bdata);
board_nand_init(omap3evm_nand_partitions,
diff --git a/arch/arm/mach-omap2/board-omap3logic.c b/arch/arm/mach-omap2/board-omap3logic.c
index bab51e6..2c35ae2 100644
--- a/arch/arm/mach-omap2/board-omap3logic.c
+++ b/arch/arm/mach-omap2/board-omap3logic.c
@@ -30,6 +30,7 @@
#include <linux/i2c/twl.h>
#include <linux/mmc/host.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -217,6 +218,7 @@ static void __init omap3logic_init(void)
board_smsc911x_init();

usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);

/* Ensure SDRC pins are mux'd for self-refresh */
diff --git a/arch/arm/mach-omap2/board-omap3pandora.c b/arch/arm/mach-omap2/board-omap3pandora.c
index 2bba362..07cede4 100644
--- a/arch/arm/mach-omap2/board-omap3pandora.c
+++ b/arch/arm/mach-omap2/board-omap3pandora.c
@@ -36,6 +36,7 @@
#include <linux/mmc/card.h>
#include <linux/regulator/fixed.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>
#include <linux/platform_data/spi-omap2-mcspi.h>

#include <asm/mach-types.h>
@@ -603,6 +604,7 @@ static void __init omap3pandora_init(void)
omap_ads7846_init(1, OMAP3_PANDORA_TS_GPIO, 0, NULL);
usbhs_init(&usbhs_bdata);
usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);
gpmc_nand_init(&pandora_nand_data, NULL);

diff --git a/arch/arm/mach-omap2/board-omap3stalker.c b/arch/arm/mach-omap2/board-omap3stalker.c
index 95c10b3..bf2575b 100644
--- a/arch/arm/mach-omap2/board-omap3stalker.c
+++ b/arch/arm/mach-omap2/board-omap3stalker.c
@@ -34,6 +34,7 @@
#include <linux/smsc911x.h>
#include <linux/i2c/at24.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -406,6 +407,7 @@ static void __init omap3_stalker_init(void)
omap_serial_init();
omap_sdrc_init(mt46h32m32lf6_sdrc_params, NULL);
usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);
usbhs_init(&usbhs_bdata);
omap_ads7846_init(1, OMAP3_STALKER_TS_GPIO, 310, NULL);
diff --git a/arch/arm/mach-omap2/board-omap3touchbook.c b/arch/arm/mach-omap2/board-omap3touchbook.c
index bcd44fb..2665d47 100644
--- a/arch/arm/mach-omap2/board-omap3touchbook.c
+++ b/arch/arm/mach-omap2/board-omap3touchbook.c
@@ -29,6 +29,7 @@
#include <linux/mtd/nand.h>
#include <linux/mmc/host.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <linux/platform_data/spi-omap2-mcspi.h>
#include <linux/spi/spi.h>
@@ -367,6 +368,7 @@ static void __init omap3_touchbook_init(void)
/* Touchscreen and accelerometer */
omap_ads7846_init(4, OMAP3_TS_GPIO, 310, &ads7846_pdata);
usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);
usbhs_init(&usbhs_bdata);
board_nand_init(omap3touchbook_nand_partitions,
diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c
index b02c2f0..d977c11 100644
--- a/arch/arm/mach-omap2/board-omap4panda.c
+++ b/arch/arm/mach-omap2/board-omap4panda.c
@@ -31,6 +31,7 @@
#include <linux/ti_wilink_st.h>
#include <linux/usb/musb.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>
#include <linux/wl12xx.h>
#include <linux/irqchip/arm-gic.h>
#include <linux/platform_data/omap-abe-twl6040.h>
@@ -449,6 +450,7 @@ static void __init omap4_panda_init(void)
omap4_twl6030_hsmmc_init(mmc);
omap4_ehci_init();
usb_bind_phy("musb-hdrc.0.auto", 0, "omap-usb2.1.auto");
+ phy_bind("musb-hdrc.0.auto", 0, "omap-usb2.1.auto");
usb_musb_init(&musb_board_data);
omap4_panda_display_init();
}
diff --git a/arch/arm/mach-omap2/board-overo.c b/arch/arm/mach-omap2/board-overo.c
index 86bab51..f75cb33 100644
--- a/arch/arm/mach-omap2/board-overo.c
+++ b/arch/arm/mach-omap2/board-overo.c
@@ -37,6 +37,7 @@
#include <linux/mtd/partitions.h>
#include <linux/mmc/host.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <linux/platform_data/mtd-nand-omap2.h>
#include <linux/platform_data/spi-omap2-mcspi.h>
@@ -501,6 +502,7 @@ static void __init overo_init(void)
board_nand_init(overo_nand_partitions,
ARRAY_SIZE(overo_nand_partitions), NAND_CS, 0, NULL);
usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);
usbhs_init(&usbhs_bdata);
overo_spi_init();
diff --git a/arch/arm/mach-omap2/board-rm680.c b/arch/arm/mach-omap2/board-rm680.c
index 345e8c4..2895fe8 100644
--- a/arch/arm/mach-omap2/board-rm680.c
+++ b/arch/arm/mach-omap2/board-rm680.c
@@ -19,6 +19,7 @@
#include <linux/regulator/consumer.h>
#include <linux/platform_data/mtd-onenand-omap2.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <asm/mach/arch.h>
#include <asm/mach-types.h>
@@ -136,6 +137,7 @@ static void __init rm680_init(void)
omap_sdrc_init(sdrc_params, sdrc_params);

usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);
rm680_peripherals_init();
}
diff --git a/arch/arm/mach-omap2/board-rx51.c b/arch/arm/mach-omap2/board-rx51.c
index d2ea68e..508f8d9 100644
--- a/arch/arm/mach-omap2/board-rx51.c
+++ b/arch/arm/mach-omap2/board-rx51.c
@@ -18,6 +18,7 @@
#include <linux/gpio.h>
#include <linux/leds.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>
#include <linux/usb/musb.h>
#include <linux/platform_data/spi-omap2-mcspi.h>

@@ -100,6 +101,7 @@ static void __init rx51_init(void)
omap_sdrc_init(sdrc_params, sdrc_params);

usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(&musb_board_data);
rx51_peripherals_init();

diff --git a/arch/arm/mach-omap2/board-zoom-peripherals.c b/arch/arm/mach-omap2/board-zoom-peripherals.c
index cdc0c10..b58d7cd 100644
--- a/arch/arm/mach-omap2/board-zoom-peripherals.c
+++ b/arch/arm/mach-omap2/board-zoom-peripherals.c
@@ -22,6 +22,7 @@
#include <linux/platform_data/gpio-omap.h>
#include <linux/platform_data/omap-twl4030.h>
#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>

#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -303,6 +304,7 @@ void __init zoom_peripherals_init(void)
omap_i2c_init();
platform_device_register(&omap_vwlan_device);
usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
+ phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
usb_musb_init(NULL);
enable_board_wakeup_source();
omap_serial_init();
--
1.7.10.4

2013-03-20 09:14:43

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: [PATCH v3 2/6] usb: phy: omap-usb2: use the new generic PHY framework

Used the generic PHY framework API to create the PHY. omap_usb2_suspend
is split into omap_usb_suspend and omap_usb_resume in order to align
with the new framework.

However using the old USB PHY library cannot be completely removed
because OTG is intertwined with PHY and moving to the new framework
will break OTG. Once we have a separate OTG state machine, we
can get rid of the USB PHY library.

Signed-off-by: Kishon Vijay Abraham I <[email protected]>
---
drivers/usb/phy/omap-usb2.c | 47 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)

diff --git a/drivers/usb/phy/omap-usb2.c b/drivers/usb/phy/omap-usb2.c
index 844ab68..819ba71 100644
--- a/drivers/usb/phy/omap-usb2.c
+++ b/drivers/usb/phy/omap-usb2.c
@@ -28,6 +28,7 @@
#include <linux/pm_runtime.h>
#include <linux/delay.h>
#include <linux/usb/omap_control_usb.h>
+#include <linux/phy/phy.h>

/**
* omap_usb2_set_comparator - links the comparator present in the sytem with
@@ -119,9 +120,48 @@ static int omap_usb2_suspend(struct usb_phy *x, int suspend)
return 0;
}

+static int omap_usb_suspend(struct phy *x)
+{
+ struct omap_usb *phy = dev_get_drvdata(&x->dev);
+
+ if (!phy->is_suspended) {
+ omap_control_usb_phy_power(phy->control_dev, 0);
+ pm_runtime_put_sync(phy->dev);
+ phy->is_suspended = 1;
+ }
+
+ return 0;
+}
+
+static int omap_usb_resume(struct phy *x)
+{
+ u32 ret;
+ struct omap_usb *phy = dev_get_drvdata(&x->dev);
+
+ if (phy->is_suspended) {
+ ret = pm_runtime_get_sync(phy->dev);
+ if (ret < 0) {
+ dev_err(phy->dev, "get_sync failed with err %d\n",
+ ret);
+ return ret;
+ }
+ omap_control_usb_phy_power(phy->control_dev, 1);
+ phy->is_suspended = 0;
+ }
+
+ return 0;
+}
+
+static struct phy_ops ops = {
+ .suspend = omap_usb_suspend,
+ .resume = omap_usb_resume,
+ .owner = THIS_MODULE,
+};
+
static int omap_usb2_probe(struct platform_device *pdev)
{
struct omap_usb *phy;
+ struct phy *generic_phy;
struct usb_otg *otg;

phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
@@ -144,6 +184,13 @@ static int omap_usb2_probe(struct platform_device *pdev)
phy->phy.otg = otg;
phy->phy.type = USB_PHY_TYPE_USB2;

+ generic_phy = devm_phy_create(phy->dev, "omap-usb2", pdev->dev.of_node,
+ USB_PHY_TYPE_USB2, &ops, phy);
+ if (IS_ERR(generic_phy)) {
+ dev_dbg(&pdev->dev, "Failed to create PHY\n");
+ return PTR_ERR(generic_phy);
+ }
+
phy->control_dev = omap_get_control_dev();
if (IS_ERR(phy->control_dev)) {
dev_dbg(&pdev->dev, "Failed to get control device\n");
--
1.7.10.4

2013-03-20 16:51:58

by Tony Lindgren

[permalink] [raw]
Subject: Re: [PATCH v3 4/6] ARM: OMAP: USB: Add phy binding information

* Kishon Vijay Abraham I <[email protected]> [130320 02:17]:
>
> --- a/arch/arm/mach-omap2/board-2430sdp.c
> +++ b/arch/arm/mach-omap2/board-2430sdp.c
> @@ -265,6 +266,7 @@ static void __init omap_2430sdp_init(void)
>
> omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP);
> usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
> + phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
> usb_musb_init(NULL);
>
> board_smc91x_init();
> --- a/arch/arm/mach-omap2/board-3430sdp.c
> +++ b/arch/arm/mach-omap2/board-3430sdp.c
> @@ -601,6 +602,7 @@ static void __init omap_3430sdp_init(void)
> omap_serial_init();
> omap_sdrc_init(hyb18m512160af6_sdrc_params, NULL);
> usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
> + phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
> usb_musb_init(NULL);
> board_smc91x_init();
> board_flash_init(sdp_flash_partitions, chip_sel_3430, 0);

Can't you call phy_bind() from usb_musb_init() with the default
values automatically when usb_musb_init() is passed NULL?

That way you don't have to patch every board-*.c file with the
same lines, and don't need to include <linux/phy/phy.h> in each
board-*.c file.

> --- a/arch/arm/mach-omap2/board-4430sdp.c
> +++ b/arch/arm/mach-omap2/board-4430sdp.c
> @@ -32,6 +32,7 @@
> #include <linux/platform_data/omap4-keypad.h>
> #include <linux/usb/musb.h>
> #include <linux/usb/phy.h>
> +#include <linux/phy/phy.h>
>
> #include <asm/mach-types.h>
> #include <asm/mach/arch.h>
> @@ -725,6 +726,7 @@ static void __init omap_4430sdp_init(void)
> omap4_twl6030_hsmmc_init(mmc);
>
> usb_bind_phy("musb-hdrc.0.auto", 0, "omap-usb2.1.auto");
> + phy_bind("musb-hdrc.0.auto", 0, "omap-usb2.1.auto");
> usb_musb_init(&musb_board_data);
>
> status = omap_ethernet_init();

Here usb_musb_init() gets called with musb_board_data, so
keeping the phy_bind() in the board-4430sdp.c can then override
the default in usb_musb_init().

Regards,

Tony

2013-03-20 20:59:51

by Stephen Warren

[permalink] [raw]
Subject: Re: [PATCH v3 5/6] ARM: dts: omap: update usb_otg_hs data

On 03/20/2013 03:12 AM, Kishon Vijay Abraham I wrote:
> Updated the usb_otg_hs dt data to include the *phy* and *phy-names*
> binding in order for the driver to use the new generic PHY framework.
> Also updated the Documentation to include the binding information.

> diff --git a/Documentation/devicetree/bindings/usb/omap-usb.txt b/Documentation/devicetree/bindings/usb/omap-usb.txt
> index abce256..3d6f9f6 100644
> --- a/Documentation/devicetree/bindings/usb/omap-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/omap-usb.txt
> @@ -19,6 +19,9 @@ OMAP MUSB GLUE
> - power : Should be "50". This signifies the controller can supply upto
> 100mA when operating in host mode.
> - usb-phy : the phandle for the PHY device
> + - phy : the phandle for the PHY device (used by generic PHY framework)
> + - phy-names : the names of the PHY corresponding to the PHYs present in the
> + *phy* phandle.

If the intent is for those properties to be generic and used by any DT
binding that refers to a PHY node, I think you'd want to define those
properties in e.g. Documentation/devicetree/bindings/phy/phy.txt, just
like common clock/GPIO/... properties are defined in standalone common
files.

I think you want to require that DT nodes that represent PHYs have a
#phy-cells property, and that the format of the phy property be
<&phy_phandle phy_specifier*>, where #phy-cells in the referenced node
defines how many cells are part of phy_specifier*, just like (almost)
any other DT property that references another node by phandle. That way,
if a single DT node represents a HW block that implements e.g. 3 PHYs,
it can use #phy-cells = <1>, and the referencing phy property can
include a cell that indicates which of those 3 PHYs is being referenced.

2013-03-20 22:36:27

by Sylwester Nawrocki

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] drivers: phy: add generic PHY framework

Hi Kishon,

On 03/20/2013 10:12 AM, Kishon Vijay Abraham I wrote:
> The PHY framework provides a set of APIs for the PHY drivers to
> create/destroy a PHY and APIs for the PHY users to obtain a reference to the
> PHY with or without using phandle. To obtain a reference to the PHY without
> using phandle, the platform specfic intialization code (say from board file)
> should have already called phy_bind with the binding information. The binding
> information consists of phy's device name, phy user device name and an index.
> The index is used when the same phy user binds to mulitple phys.
>
> PHY drivers should create the PHY by passing phy_descriptor that has
> describes the PHY (label, type etc..) and ops like init, exit, suspend, resume,
> poweron, shutdown.
>
> The documentation for the generic PHY framework is added in
> Documentation/phy.txt and the documentation for the sysfs entry is added
> in Documentation/ABI/testing/sysfs-class-phy.
>
> Signed-off-by: Kishon Vijay Abraham I<[email protected]>
> ---
[...]
> +static inline struct phy *phy_get(struct device *dev, int index)
> +{
> + return ERR_PTR(-EOPNOTSUPP);

Shouldn't these be -ENOSYS ? EOPNOTSUPP is defined by POSIX as
"Operation not supported on socket". And EOPNOTSUPP appears to be
mostly used in the network code in the kernel.

> +}
> +
> +static inline struct phy *devm_phy_get(struct device *dev, int index)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline struct phy *of_phy_get(struct device *dev, int index)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline struct phy *devm_of_phy_get(struct device *dev, int index)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline struct phy *of_phy_get_byname(struct device *dev,
> + const char *string)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline struct phy *devm_of_phy_get_byname(struct device *dev,
> + const char *string)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}

Thanks,
Sylwester

2013-03-21 05:47:34

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] drivers: phy: add generic PHY framework

Hi,

On Thursday 21 March 2013 04:06 AM, Sylwester Nawrocki wrote:
> Hi Kishon,
>
> On 03/20/2013 10:12 AM, Kishon Vijay Abraham I wrote:
>> The PHY framework provides a set of APIs for the PHY drivers to
>> create/destroy a PHY and APIs for the PHY users to obtain a reference
>> to the
>> PHY with or without using phandle. To obtain a reference to the PHY
>> without
>> using phandle, the platform specfic intialization code (say from board
>> file)
>> should have already called phy_bind with the binding information. The
>> binding
>> information consists of phy's device name, phy user device name and an
>> index.
>> The index is used when the same phy user binds to mulitple phys.
>>
>> PHY drivers should create the PHY by passing phy_descriptor that has
>> describes the PHY (label, type etc..) and ops like init, exit,
>> suspend, resume,
>> poweron, shutdown.
>>
>> The documentation for the generic PHY framework is added in
>> Documentation/phy.txt and the documentation for the sysfs entry is added
>> in Documentation/ABI/testing/sysfs-class-phy.
>>
>> Signed-off-by: Kishon Vijay Abraham I<[email protected]>
>> ---
> [...]
>> +static inline struct phy *phy_get(struct device *dev, int index)
>> +{
>> + return ERR_PTR(-EOPNOTSUPP);
>
> Shouldn't these be -ENOSYS ? EOPNOTSUPP is defined by POSIX as
> "Operation not supported on socket". And EOPNOTSUPP appears to be
> mostly used in the network code in the kernel.

Fair enough. Will change to ENOSYS.

Thanks
Kishon

2013-03-21 05:48:58

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: Re: [PATCH v3 4/6] ARM: OMAP: USB: Add phy binding information

Hi,

On Wednesday 20 March 2013 10:21 PM, Tony Lindgren wrote:
> * Kishon Vijay Abraham I <[email protected]> [130320 02:17]:
>>
>> --- a/arch/arm/mach-omap2/board-2430sdp.c
>> +++ b/arch/arm/mach-omap2/board-2430sdp.c
>> @@ -265,6 +266,7 @@ static void __init omap_2430sdp_init(void)
>>
>> omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP);
>> usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
>> + phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
>> usb_musb_init(NULL);
>>
>> board_smc91x_init();
>> --- a/arch/arm/mach-omap2/board-3430sdp.c
>> +++ b/arch/arm/mach-omap2/board-3430sdp.c
>> @@ -601,6 +602,7 @@ static void __init omap_3430sdp_init(void)
>> omap_serial_init();
>> omap_sdrc_init(hyb18m512160af6_sdrc_params, NULL);
>> usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb");
>> + phy_bind("musb-hdrc.0.auto", 0, "twl4030_usb");
>> usb_musb_init(NULL);
>> board_smc91x_init();
>> board_flash_init(sdp_flash_partitions, chip_sel_3430, 0);
>
> Can't you call phy_bind() from usb_musb_init() with the default
> values automatically when usb_musb_init() is passed NULL?
>
> That way you don't have to patch every board-*.c file with the
> same lines, and don't need to include <linux/phy/phy.h> in each
> board-*.c file.
>
>> --- a/arch/arm/mach-omap2/board-4430sdp.c
>> +++ b/arch/arm/mach-omap2/board-4430sdp.c
>> @@ -32,6 +32,7 @@
>> #include <linux/platform_data/omap4-keypad.h>
>> #include <linux/usb/musb.h>
>> #include <linux/usb/phy.h>
>> +#include <linux/phy/phy.h>
>>
>> #include <asm/mach-types.h>
>> #include <asm/mach/arch.h>
>> @@ -725,6 +726,7 @@ static void __init omap_4430sdp_init(void)
>> omap4_twl6030_hsmmc_init(mmc);
>>
>> usb_bind_phy("musb-hdrc.0.auto", 0, "omap-usb2.1.auto");
>> + phy_bind("musb-hdrc.0.auto", 0, "omap-usb2.1.auto");
>> usb_musb_init(&musb_board_data);
>>
>> status = omap_ethernet_init();
>
> Here usb_musb_init() gets called with musb_board_data, so
> keeping the phy_bind() in the board-4430sdp.c can then override
> the default in usb_musb_init().

Currently phy_bind has a limitation to do that. Will change phy_bind()
and have your comment incorporated.

Thanks
Kishon

2013-03-21 06:24:01

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: Re: [PATCH v3 5/6] ARM: dts: omap: update usb_otg_hs data

Hi,

On Thursday 21 March 2013 02:29 AM, Stephen Warren wrote:
> On 03/20/2013 03:12 AM, Kishon Vijay Abraham I wrote:
>> Updated the usb_otg_hs dt data to include the *phy* and *phy-names*
>> binding in order for the driver to use the new generic PHY framework.
>> Also updated the Documentation to include the binding information.
>
>> diff --git a/Documentation/devicetree/bindings/usb/omap-usb.txt b/Documentation/devicetree/bindings/usb/omap-usb.txt
>> index abce256..3d6f9f6 100644
>> --- a/Documentation/devicetree/bindings/usb/omap-usb.txt
>> +++ b/Documentation/devicetree/bindings/usb/omap-usb.txt
>> @@ -19,6 +19,9 @@ OMAP MUSB GLUE
>> - power : Should be "50". This signifies the controller can supply upto
>> 100mA when operating in host mode.
>> - usb-phy : the phandle for the PHY device
>> + - phy : the phandle for the PHY device (used by generic PHY framework)
>> + - phy-names : the names of the PHY corresponding to the PHYs present in the
>> + *phy* phandle.
>
> If the intent is for those properties to be generic and used by any DT
> binding that refers to a PHY node, I think you'd want to define those
> properties in e.g. Documentation/devicetree/bindings/phy/phy.txt, just
> like common clock/GPIO/... properties are defined in standalone common
> files.

Ok. Will add it.
>
> I think you want to require that DT nodes that represent PHYs have a
> #phy-cells property, and that the format of the phy property be
> <&phy_phandle phy_specifier*>, where #phy-cells in the referenced node
> defines how many cells are part of phy_specifier*, just like (almost)
> any other DT property that references another node by phandle. That way,
> if a single DT node represents a HW block that implements e.g. 3 PHYs,
> it can use #phy-cells = <1>, and the referencing phy property can
> include a cell that indicates which of those 3 PHYs is being referenced.

Currently, if a single phandle have reference to multiple PHYs, we can
get PHY by passing index or by name as give in phy-names.
I'm not sure if we have <&phy_phandle phy_specifier*>, what could that
phy_specifier be? Maybe phy_type?

Thanks
Kishon

2013-03-21 17:10:17

by Stephen Warren

[permalink] [raw]
Subject: Re: [PATCH v3 5/6] ARM: dts: omap: update usb_otg_hs data

On 03/21/2013 12:23 AM, kishon wrote:
> Hi,
>
> On Thursday 21 March 2013 02:29 AM, Stephen Warren wrote:
>> On 03/20/2013 03:12 AM, Kishon Vijay Abraham I wrote:
>>> Updated the usb_otg_hs dt data to include the *phy* and *phy-names*
>>> binding in order for the driver to use the new generic PHY framework.
>>> Also updated the Documentation to include the binding information.
>>
>>> diff --git a/Documentation/devicetree/bindings/usb/omap-usb.txt
>>> b/Documentation/devicetree/bindings/usb/omap-usb.txt
>>> index abce256..3d6f9f6 100644
>>> --- a/Documentation/devicetree/bindings/usb/omap-usb.txt
>>> +++ b/Documentation/devicetree/bindings/usb/omap-usb.txt
>>> @@ -19,6 +19,9 @@ OMAP MUSB GLUE
>>> - power : Should be "50". This signifies the controller can supply
>>> upto
>>> 100mA when operating in host mode.
>>> - usb-phy : the phandle for the PHY device
>>> + - phy : the phandle for the PHY device (used by generic PHY framework)
>>> + - phy-names : the names of the PHY corresponding to the PHYs
>>> present in the
>>> + *phy* phandle.
>>
>> If the intent is for those properties to be generic and used by any DT
>> binding that refers to a PHY node, I think you'd want to define those
>> properties in e.g. Documentation/devicetree/bindings/phy/phy.txt, just
>> like common clock/GPIO/... properties are defined in standalone common
>> files.
>
> Ok. Will add it.
>>
>> I think you want to require that DT nodes that represent PHYs have a
>> #phy-cells property, and that the format of the phy property be
>> <&phy_phandle phy_specifier*>, where #phy-cells in the referenced node
>> defines how many cells are part of phy_specifier*, just like (almost)
>> any other DT property that references another node by phandle. That way,
>> if a single DT node represents a HW block that implements e.g. 3 PHYs,
>> it can use #phy-cells = <1>, and the referencing phy property can
>> include a cell that indicates which of those 3 PHYs is being referenced.
>
> Currently, if a single phandle have reference to multiple PHYs, we can
> get PHY by passing index or by name as give in phy-names.
> I'm not sure if we have <&phy_phandle phy_specifier*>, what could that
> phy_specifier be? Maybe phy_type?

I'm not talking about the case where a consumer node references multiple
PHYs. As you say, that is indeed handled by the driver looking at a
particular index in the phys property, or using phy-names.

I'm talking about the case where a single provider provides multiple
PHYs. For example, consider:

phys: phy {
compatible = "xxx";
reg = <...>;
#phy-cells = <1>;
};

That node describes an IP block that implements 3 different PHYs. The
registers are inter-mixed in such a way that you can't split it into 3
separate nodes each with a separate device instance. If the consumers
simply say:

phys = <&phys>;

then which of the 3 PHYs are you referring to?

Instead, the consumer needs to say one of:

phys = <&phys 0>;
phys = <&phys 1>;
phys = <&phys 2>;

in order to specify which of the PHYs it refers to.

The number of cells in the phy property after the phandle is specified
by the #phy-cells property in the node referred to by the phandle. The
meaning of all those cells is defined by the binding for the phy node.
This could include both the PHY ID (as in my example here), or whatever
configuration information or flags the provider needs. For example, both
GPIOs and interrupts have specifiers than describe both of these things.

For more background, take a look at almost any other binding that uses
phandles.

By the way, the property in the consumer should probably be "phys" not
"phy" to be consistent with other similar properties (e.g. gpios,
interrupts, ... which are all plural).

2013-03-22 09:22:10

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: Re: [PATCH v3 5/6] ARM: dts: omap: update usb_otg_hs data

Hi,

On Thursday 21 March 2013 10:40 PM, Stephen Warren wrote:
> On 03/21/2013 12:23 AM, kishon wrote:
>> Hi,
>>
>> On Thursday 21 March 2013 02:29 AM, Stephen Warren wrote:
>>> On 03/20/2013 03:12 AM, Kishon Vijay Abraham I wrote:
>>>> Updated the usb_otg_hs dt data to include the *phy* and *phy-names*
>>>> binding in order for the driver to use the new generic PHY framework.
>>>> Also updated the Documentation to include the binding information.
>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/omap-usb.txt
>>>> b/Documentation/devicetree/bindings/usb/omap-usb.txt
>>>> index abce256..3d6f9f6 100644
>>>> --- a/Documentation/devicetree/bindings/usb/omap-usb.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/omap-usb.txt
>>>> @@ -19,6 +19,9 @@ OMAP MUSB GLUE
>>>> - power : Should be "50". This signifies the controller can supply
>>>> upto
>>>> 100mA when operating in host mode.
>>>> - usb-phy : the phandle for the PHY device
>>>> + - phy : the phandle for the PHY device (used by generic PHY framework)
>>>> + - phy-names : the names of the PHY corresponding to the PHYs
>>>> present in the
>>>> + *phy* phandle.
>>>
>>> If the intent is for those properties to be generic and used by any DT
>>> binding that refers to a PHY node, I think you'd want to define those
>>> properties in e.g. Documentation/devicetree/bindings/phy/phy.txt, just
>>> like common clock/GPIO/... properties are defined in standalone common
>>> files.
>>
>> Ok. Will add it.
>>>
>>> I think you want to require that DT nodes that represent PHYs have a
>>> #phy-cells property, and that the format of the phy property be
>>> <&phy_phandle phy_specifier*>, where #phy-cells in the referenced node
>>> defines how many cells are part of phy_specifier*, just like (almost)
>>> any other DT property that references another node by phandle. That way,
>>> if a single DT node represents a HW block that implements e.g. 3 PHYs,
>>> it can use #phy-cells = <1>, and the referencing phy property can
>>> include a cell that indicates which of those 3 PHYs is being referenced.
>>
>> Currently, if a single phandle have reference to multiple PHYs, we can
>> get PHY by passing index or by name as give in phy-names.
>> I'm not sure if we have <&phy_phandle phy_specifier*>, what could that
>> phy_specifier be? Maybe phy_type?
>
> I'm not talking about the case where a consumer node references multiple
> PHYs. As you say, that is indeed handled by the driver looking at a
> particular index in the phys property, or using phy-names.
>
> I'm talking about the case where a single provider provides multiple
> PHYs. For example, consider:
>
> phys: phy {
> compatible = "xxx";
> reg = <...>;
> #phy-cells = <1>;
> };
>
> That node describes an IP block that implements 3 different PHYs. The
> registers are inter-mixed in such a way that you can't split it into 3
> separate nodes each with a separate device instance. If the consumers
> simply say:
>
> phys = <&phys>;
>
> then which of the 3 PHYs are you referring to?
>
> Instead, the consumer needs to say one of:
>
> phys = <&phys 0>;
> phys = <&phys 1>;
> phys = <&phys 2>;
>
> in order to specify which of the PHYs it refers to.
>
> The number of cells in the phy property after the phandle is specified
> by the #phy-cells property in the node referred to by the phandle. The
> meaning of all those cells is defined by the binding for the phy node.
> This could include both the PHY ID (as in my example here), or whatever
> configuration information or flags the provider needs. For example, both
> GPIOs and interrupts have specifiers than describe both of these things.

Thanks for the explanation. I'll add it in my next version.
>
> For more background, take a look at almost any other binding that uses
> phandles.
>
> By the way, the property in the consumer should probably be "phys" not
> "phy" to be consistent with other similar properties (e.g. gpios,
> interrupts, ... which are all plural).
>
Ok.

Thanks
Kishon

2013-04-15 10:20:18

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH v3 0/6] Generic PHY Framework

On Wed, 20 Mar 2013 14:41:59 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
> Added a generic PHY framework that provides a set of APIs for the PHY drivers
> to create/destroy a PHY and APIs for the PHY users to obtain a reference to
> the PHY with or without using phandle. To obtain a reference to the PHY
> without using phandle, the platform specfic intialization code (say from board
> file) should have already called phy_bind with the binding information. The
> binding information consists of phy's device name, phy user device name and an
> index. The index is used when the same phy user binds to mulitple phys.
>
> This framework will be of use only to devices that uses external PHY (PHY
> functionality is not embedded within the controller).
>
> The intention of creating this framework is to bring the phy drivers spread
> all over the Linux kernel to drivers/phy to increase code re-use and to
> increase code maintainability.
>
> Comments to make PHY as bus wasn't done because PHY devices can be part of
> other bus and making a same device attached to multiple bus leads to bad
> design.
>
> Making omap-usb2 and twl4030 to use this framework is provided as a sample.
>
> This patch series is developed on 3.9-rc3. Once the patch series gets finalised
> I'll resend omap-usb2 and twl4030 part based on Felipe's tree.
>

[...]

> drivers/Kconfig | 2 +
> drivers/Makefile | 2 +
> drivers/phy/Kconfig | 13 +
> drivers/phy/Makefile | 5 +
> drivers/phy/phy-core.c | 574 ++++++++++++++++++++

This looks to be very specific for USB PHYs. Are you intending it to be
used for other types of PHYs, like Ethernet PHYs? If not, then this
infrastruction should be named something like usb-phy so that it isn't
confused with other layers, and it really should live under drivers/usb.

g.

2013-04-15 10:37:44

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: Re: [PATCH v3 0/6] Generic PHY Framework

Hi,

On Monday 15 April 2013 03:50 PM, Grant Likely wrote:
> On Wed, 20 Mar 2013 14:41:59 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
>> Added a generic PHY framework that provides a set of APIs for the PHY drivers
>> to create/destroy a PHY and APIs for the PHY users to obtain a reference to
>> the PHY with or without using phandle. To obtain a reference to the PHY
>> without using phandle, the platform specfic intialization code (say from board
>> file) should have already called phy_bind with the binding information. The
>> binding information consists of phy's device name, phy user device name and an
>> index. The index is used when the same phy user binds to mulitple phys.
>>
>> This framework will be of use only to devices that uses external PHY (PHY
>> functionality is not embedded within the controller).
>>
>> The intention of creating this framework is to bring the phy drivers spread
>> all over the Linux kernel to drivers/phy to increase code re-use and to
>> increase code maintainability.
>>
>> Comments to make PHY as bus wasn't done because PHY devices can be part of
>> other bus and making a same device attached to multiple bus leads to bad
>> design.
>>
>> Making omap-usb2 and twl4030 to use this framework is provided as a sample.
>>
>> This patch series is developed on 3.9-rc3. Once the patch series gets finalised
>> I'll resend omap-usb2 and twl4030 part based on Felipe's tree.
>>
>
> [...]
>
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 2 +
>> drivers/phy/Kconfig | 13 +
>> drivers/phy/Makefile | 5 +
>> drivers/phy/phy-core.c | 574 ++++++++++++++++++++
>
> This looks to be very specific for USB PHYs. Are you intending it to be
> used for other types of PHYs, like Ethernet PHYs? If not, then this

Not really. This can be used by USB, SATA and Sylwester was planning to
use it for video PHY's.

Thanks
Kishon

2013-04-15 11:27:45

by Sylwester Nawrocki

[permalink] [raw]
Subject: Re: [PATCH v3 0/6] Generic PHY Framework

On 04/15/2013 12:36 PM, Kishon Vijay Abraham I wrote:
> On Monday 15 April 2013 03:50 PM, Grant Likely wrote:
>> On Wed, 20 Mar 2013 14:41:59 +0530, Kishon Vijay Abraham I <[email protected]>
>> wrote:
>>> Added a generic PHY framework that provides a set of APIs for the PHY drivers
>>> to create/destroy a PHY and APIs for the PHY users to obtain a reference to
>>> the PHY with or without using phandle. To obtain a reference to the PHY
>>> without using phandle, the platform specfic intialization code (say from board
>>> file) should have already called phy_bind with the binding information. The
>>> binding information consists of phy's device name, phy user device name and an
>>> index. The index is used when the same phy user binds to mulitple phys.
>>>
>>> This framework will be of use only to devices that uses external PHY (PHY
>>> functionality is not embedded within the controller).
>>>
>>> The intention of creating this framework is to bring the phy drivers spread
>>> all over the Linux kernel to drivers/phy to increase code re-use and to
>>> increase code maintainability.
>>>
>>> Comments to make PHY as bus wasn't done because PHY devices can be part of
>>> other bus and making a same device attached to multiple bus leads to bad
>>> design.
>>>
>>> Making omap-usb2 and twl4030 to use this framework is provided as a sample.
>>>
>>> This patch series is developed on 3.9-rc3. Once the patch series gets finalised
>>> I'll resend omap-usb2 and twl4030 part based on Felipe's tree.
>>>
>>
>> [...]
>>
>>> drivers/Kconfig | 2 +
>>> drivers/Makefile | 2 +
>>> drivers/phy/Kconfig | 13 +
>>> drivers/phy/Makefile | 5 +
>>> drivers/phy/phy-core.c | 574
>>> ++++++++++++++++++++
>>
>> This looks to be very specific for USB PHYs. Are you intending it to be
>> used for other types of PHYs, like Ethernet PHYs? If not, then this
>
> Not really. This can be used by USB, SATA and Sylwester was planning to use it
> for video PHY's.

Yes, I have already some RFC patches to handle the display and the camera
interface DPHYs (MIPI DSI, MIPI CSI-2) with this API. I didn't post it, since
this framework is not settled yet. Those DPHYs need very few operations, like
disable/enable only and there was really not suitable API in the kernel until
now to handle them. We had plans in the past to write something like this
generic PHY framework for the Samsung SoCs.

Some SoCs have plenty different PHYs: USB, SATA, MIPI CSI-2, MIPI DSI, HDMI...
And some of them are simple enough to be covered by this generic PHY API.

Thanks,
Sylwester

2013-04-15 11:34:25

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] drivers: phy: add generic PHY framework

On Wed, 20 Mar 2013 14:42:00 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
> The PHY framework provides a set of APIs for the PHY drivers to
> create/destroy a PHY and APIs for the PHY users to obtain a reference to the
> PHY with or without using phandle. To obtain a reference to the PHY without
> using phandle, the platform specfic intialization code (say from board file)
> should have already called phy_bind with the binding information. The binding
> information consists of phy's device name, phy user device name and an index.
> The index is used when the same phy user binds to mulitple phys.
>
> PHY drivers should create the PHY by passing phy_descriptor that has
> describes the PHY (label, type etc..) and ops like init, exit, suspend, resume,
> poweron, shutdown.
>
> The documentation for the generic PHY framework is added in
> Documentation/phy.txt and the documentation for the sysfs entry is added
> in Documentation/ABI/testing/sysfs-class-phy.
>
> Signed-off-by: Kishon Vijay Abraham I <[email protected]>

Hi Kishon,

For review purposes, I'll skip most of the implementation and focus on
the data structures. I think you need to take another look at the
approach your using. The kernel already provides a lot of support for
implementing devices and binding them to drivers that you should be
able to use...


[...]
> +/**
> + * struct phy - represent the phy device
> + * @dev: phy device
> + * @label: label given to phy
> + * @type: specifies the phy type
> + * @of_node: device node of the phy
> + * @ops: function pointers for performing phy operations
> + */
> +struct phy {
> + struct device dev;
> + const char *label;
> + int type;
> + struct bus_type *bus;
> + struct device_node *of_node;
> + struct phy_ops *ops;
> +};

Alright, so the core of the functionality is this 'struct phy' which
tracks all the instances of PHY devices. As I understand it each
physical phy will have a 'struct phy' instance tracking it. That makes
sense.

struct phy embeds a struct device. Also good. The linux driver model
knows all about devices and how to handle them. However, most of the
rest of this structure looks to be reinventing stuff the driver model
already does.

'label' seems unnecessary. struct device embeds a struct kobject, which
means it has a name and shows up in sysfs. Is there a reason that the
device name cannot be used directly?

'type' I just don't understand. I don't see any users of it in this
patch. I only see where it is set.

'bus' absolutely should not be here. The bus type should be set in the
struct device by this subsystem when the device gets registered. There
is no reason to have a pointer to it here.

'of_node' is already in struct device

Finally, it really doesn't look right for a device object to have an
'ops' structure. The whole point of the driver model is that a struct
device doesn't encapsulate any behaviour. A device gets registers to a
bus type, and then the driver core will associate a struct device_driver
to a device that it is able to drive, and the struct device_driver is
supposed to contain any ops structure used to control the device.

> +
> +/**
> + * struct phy_bind - represent the binding for the phy
> + * @dev_name: the device name of the device that will bind to the phy
> + * @phy_dev_name: the device name of the phy
> + * @index: used if a single controller uses multiple phys
> + * @auto_bind: tells if the binding is done explicitly from board file or not
> + * @phy: reference to the phy
> + * @list: to maintain a linked list of the binding information
> + */
> +struct phy_bind {
> + const char *dev_name;
> + const char *phy_dev_name;
> + int index;
> + int auto_bind:1;
> + struct phy *phy;
> + struct list_head list;
> +};

How are PHYs attached to the system. Would the PHY be a direct child of
the host controller device, or would a PHY hang off another bus (like
i2c) for control? (this is how Ethernet phys work; they hang off the
MDIO bus, but may be attached to any Ethernet controller).

Since there is at most a 1:N relationship between host controllers and
PHYs, there shouldn't be any need for a separate structure to describe
binding. Put the inding data into the struct phy itself. Each host
controller can have a list of phys that it is bound to.

Tring to maintain a separate global list of PHY bindings isn't a good
idea. Keep it local to the host controller and the specific phys
instead.

g.

2013-04-15 12:26:42

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH v3 0/6] Generic PHY Framework

On Mon, 15 Apr 2013 16:06:37 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
> Hi,
>
> On Monday 15 April 2013 03:50 PM, Grant Likely wrote:
> > On Wed, 20 Mar 2013 14:41:59 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
> >> Added a generic PHY framework that provides a set of APIs for the PHY drivers
> >> to create/destroy a PHY and APIs for the PHY users to obtain a reference to
> >> the PHY with or without using phandle. To obtain a reference to the PHY
> >> without using phandle, the platform specfic intialization code (say from board
> >> file) should have already called phy_bind with the binding information. The
> >> binding information consists of phy's device name, phy user device name and an
> >> index. The index is used when the same phy user binds to mulitple phys.
> >>
> >> This framework will be of use only to devices that uses external PHY (PHY
> >> functionality is not embedded within the controller).
> >>
> >> The intention of creating this framework is to bring the phy drivers spread
> >> all over the Linux kernel to drivers/phy to increase code re-use and to
> >> increase code maintainability.
> >>
> >> Comments to make PHY as bus wasn't done because PHY devices can be part of
> >> other bus and making a same device attached to multiple bus leads to bad
> >> design.
> >>
> >> Making omap-usb2 and twl4030 to use this framework is provided as a sample.
> >>
> >> This patch series is developed on 3.9-rc3. Once the patch series gets finalised
> >> I'll resend omap-usb2 and twl4030 part based on Felipe's tree.
> >>
> >
> > [...]
> >
> >> drivers/Kconfig | 2 +
> >> drivers/Makefile | 2 +
> >> drivers/phy/Kconfig | 13 +
> >> drivers/phy/Makefile | 5 +
> >> drivers/phy/phy-core.c | 574 ++++++++++++++++++++
> >
> > This looks to be very specific for USB PHYs. Are you intending it to be
> > used for other types of PHYs, like Ethernet PHYs? If not, then this
>
> Not really. This can be used by USB, SATA and Sylwester was planning to
> use it for video PHY's.

So what are the common bits that are shared between those phys? Merely
matching phys to controllers? Besides that, each of those devices have
very different behaviour. You wouldn't be able to attach any interface
logic to the generic struct phy. I don't think it makes a whole lot of
sense to lump them all into the same type of registration.

g.

2013-04-15 12:27:09

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] drivers: phy: add generic PHY framework

Hi,

On Monday 15 April 2013 05:04 PM, Grant Likely wrote:
> On Wed, 20 Mar 2013 14:42:00 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
>> The PHY framework provides a set of APIs for the PHY drivers to
>> create/destroy a PHY and APIs for the PHY users to obtain a reference to the
>> PHY with or without using phandle. To obtain a reference to the PHY without
>> using phandle, the platform specfic intialization code (say from board file)
>> should have already called phy_bind with the binding information. The binding
>> information consists of phy's device name, phy user device name and an index.
>> The index is used when the same phy user binds to mulitple phys.
>>
>> PHY drivers should create the PHY by passing phy_descriptor that has
>> describes the PHY (label, type etc..) and ops like init, exit, suspend, resume,
>> poweron, shutdown.
>>
>> The documentation for the generic PHY framework is added in
>> Documentation/phy.txt and the documentation for the sysfs entry is added
>> in Documentation/ABI/testing/sysfs-class-phy.
>>
>> Signed-off-by: Kishon Vijay Abraham I <[email protected]>
>
> Hi Kishon,
>
> For review purposes, I'll skip most of the implementation and focus on
> the data structures. I think you need to take another look at the
> approach your using. The kernel already provides a lot of support for
> implementing devices and binding them to drivers that you should be
> able to use...
>
>
> [...]
>> +/**
>> + * struct phy - represent the phy device
>> + * @dev: phy device
>> + * @label: label given to phy
>> + * @type: specifies the phy type
>> + * @of_node: device node of the phy
>> + * @ops: function pointers for performing phy operations
>> + */
>> +struct phy {
>> + struct device dev;
>> + const char *label;
>> + int type;
>> + struct bus_type *bus;
>> + struct device_node *of_node;
>> + struct phy_ops *ops;
>> +};
>
> Alright, so the core of the functionality is this 'struct phy' which
> tracks all the instances of PHY devices. As I understand it each
> physical phy will have a 'struct phy' instance tracking it. That makes
> sense.
>
> struct phy embeds a struct device. Also good. The linux driver model
> knows all about devices and how to handle them. However, most of the
> rest of this structure looks to be reinventing stuff the driver model
> already does.
>
> 'label' seems unnecessary. struct device embeds a struct kobject, which
> means it has a name and shows up in sysfs. Is there a reason that the
> device name cannot be used directly?

hmm.. the label name is supposed to be a simpler name than device name.
Say for instance "omap-usb2.1.auto" device name can simply be
"omap-usb2-phy". Further the device name while using dt will have
register address in it. However it's used only for displaying the label
in sysfs entry (/sys/class/phy/<phy>/label).
>
> 'type' I just don't understand. I don't see any users of it in this
> patch. I only see where it is set.

yeah. Was planning to remove this in my next version (btw the latest is
version 5).
>
> 'bus' absolutely should not be here. The bus type should be set in the
> struct device by this subsystem when the device gets registered. There
> is no reason to have a pointer to it here.

right. I had removed it in version 5 of this patch series.
>
> 'of_node' is already in struct device

I wasn't sure if we can manually assign the of_node of one device to
of_node of an another device. Here the of_node comes from _phy provider_.
>
> Finally, it really doesn't look right for a device object to have an
> 'ops' structure. The whole point of the driver model is that a struct
> device doesn't encapsulate any behaviour. A device gets registers to a
> bus type, and then the driver core will associate a struct device_driver

We have decided not to implement the PHY layer as a separate bus layer.
The PHY provider can be part of any other bus. Making the PHY layer as a
bus will make the PHY provider to be part of multiple buses which will
lead to bad design. All we are trying to do here is keep the pool of PHY
devices under PHY class in this layer and help any controller that wants
to use the PHY to get it.
> to a device that it is able to drive, and the struct device_driver is
> supposed to contain any ops structure used to control the device.
>
>> +
>> +/**
>> + * struct phy_bind - represent the binding for the phy
>> + * @dev_name: the device name of the device that will bind to the phy
>> + * @phy_dev_name: the device name of the phy
>> + * @index: used if a single controller uses multiple phys
>> + * @auto_bind: tells if the binding is done explicitly from board file or not
>> + * @phy: reference to the phy
>> + * @list: to maintain a linked list of the binding information
>> + */
>> +struct phy_bind {
>> + const char *dev_name;
>> + const char *phy_dev_name;
>> + int index;
>> + int auto_bind:1;
>> + struct phy *phy;
>> + struct list_head list;
>> +};
>
> How are PHYs attached to the system. Would the PHY be a direct child of
> the host controller device, or would a PHY hang off another bus (like
> i2c) for control? (this is how Ethernet phys work; they hang off the
> MDIO bus, but may be attached to any Ethernet controller).

This layer does not have any restriction on how the PHY is attached to
the system. For all the sample cases (USB OTG controller in OMAP
platforms) that follows this patch, the PHY is attached to the platform
bus and the PHY provider is a platform driver. All this framework does
is maintain a list of PHY's added in the system and helps the controller
to find the appropriate PHY.
We are currently not looking at having Ethernet use this layer because
it itself has a comprehensive PHY layer and merging it with this will
take some effort. However other subsystems like USB, SATA, video etc..
can make use of this.
>
> Since there is at most a 1:N relationship between host controllers and
> PHYs, there shouldn't be any need for a separate structure to describe
> binding. Put the inding data into the struct phy itself. Each host
> controller can have a list of phys that it is bound to.

No. Having the host controller to have a list of phys wont be a good
idea IMHO. The host controller is just an IP and the PHY to which it
will be connected can vary from board to board, platform to platform. So
ideally this binding should come from platform initialization code/dt data.

Thanks
Kishon

2013-04-15 12:33:46

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: Re: [PATCH v3 0/6] Generic PHY Framework

Hi,

On Monday 15 April 2013 05:56 PM, Grant Likely wrote:
> On Mon, 15 Apr 2013 16:06:37 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
>> Hi,
>>
>> On Monday 15 April 2013 03:50 PM, Grant Likely wrote:
>>> On Wed, 20 Mar 2013 14:41:59 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
>>>> Added a generic PHY framework that provides a set of APIs for the PHY drivers
>>>> to create/destroy a PHY and APIs for the PHY users to obtain a reference to
>>>> the PHY with or without using phandle. To obtain a reference to the PHY
>>>> without using phandle, the platform specfic intialization code (say from board
>>>> file) should have already called phy_bind with the binding information. The
>>>> binding information consists of phy's device name, phy user device name and an
>>>> index. The index is used when the same phy user binds to mulitple phys.
>>>>
>>>> This framework will be of use only to devices that uses external PHY (PHY
>>>> functionality is not embedded within the controller).
>>>>
>>>> The intention of creating this framework is to bring the phy drivers spread
>>>> all over the Linux kernel to drivers/phy to increase code re-use and to
>>>> increase code maintainability.
>>>>
>>>> Comments to make PHY as bus wasn't done because PHY devices can be part of
>>>> other bus and making a same device attached to multiple bus leads to bad
>>>> design.
>>>>
>>>> Making omap-usb2 and twl4030 to use this framework is provided as a sample.
>>>>
>>>> This patch series is developed on 3.9-rc3. Once the patch series gets finalised
>>>> I'll resend omap-usb2 and twl4030 part based on Felipe's tree.
>>>>
>>>
>>> [...]
>>>
>>>> drivers/Kconfig | 2 +
>>>> drivers/Makefile | 2 +
>>>> drivers/phy/Kconfig | 13 +
>>>> drivers/phy/Makefile | 5 +
>>>> drivers/phy/phy-core.c | 574 ++++++++++++++++++++
>>>
>>> This looks to be very specific for USB PHYs. Are you intending it to be
>>> used for other types of PHYs, like Ethernet PHYs? If not, then this
>>
>> Not really. This can be used by USB, SATA and Sylwester was planning to
>> use it for video PHY's.
>
> So what are the common bits that are shared between those phys? Merely
> matching phys to controllers? Besides that, each of those devices have

The same PIPE3 PHY IP is used for USB (usb3) and SATA in OMAP5. Even for
just matching PHYs to controllers you need a simple framework (like
this) or else we'll end up in creating such frameworks for lot of these
subsystems.

Thanks
Kishon

2013-04-15 19:50:39

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] drivers: phy: add generic PHY framework

On Mon, 15 Apr 2013 17:56:10 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
> Hi,
>
> On Monday 15 April 2013 05:04 PM, Grant Likely wrote:
> > On Wed, 20 Mar 2013 14:42:00 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
> >> The PHY framework provides a set of APIs for the PHY drivers to
> >> create/destroy a PHY and APIs for the PHY users to obtain a reference to the
> >> PHY with or without using phandle. To obtain a reference to the PHY without
> >> using phandle, the platform specfic intialization code (say from board file)
> >> should have already called phy_bind with the binding information. The binding
> >> information consists of phy's device name, phy user device name and an index.
> >> The index is used when the same phy user binds to mulitple phys.
> >>
> >> PHY drivers should create the PHY by passing phy_descriptor that has
> >> describes the PHY (label, type etc..) and ops like init, exit, suspend, resume,
> >> poweron, shutdown.
> >>
> >> The documentation for the generic PHY framework is added in
> >> Documentation/phy.txt and the documentation for the sysfs entry is added
> >> in Documentation/ABI/testing/sysfs-class-phy.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <[email protected]>
> >
> > Hi Kishon,
> >
> > For review purposes, I'll skip most of the implementation and focus on
> > the data structures. I think you need to take another look at the
> > approach your using. The kernel already provides a lot of support for
> > implementing devices and binding them to drivers that you should be
> > able to use...
> >
> >
> > [...]
> >> +/**
> >> + * struct phy - represent the phy device
> >> + * @dev: phy device
> >> + * @label: label given to phy
> >> + * @type: specifies the phy type
> >> + * @of_node: device node of the phy
> >> + * @ops: function pointers for performing phy operations
> >> + */
> >> +struct phy {
> >> + struct device dev;
> >> + const char *label;
> >> + int type;
> >> + struct bus_type *bus;
> >> + struct device_node *of_node;
> >> + struct phy_ops *ops;
> >> +};
> >
> > Alright, so the core of the functionality is this 'struct phy' which
> > tracks all the instances of PHY devices. As I understand it each
> > physical phy will have a 'struct phy' instance tracking it. That makes
> > sense.
> >
> > struct phy embeds a struct device. Also good. The linux driver model
> > knows all about devices and how to handle them. However, most of the
> > rest of this structure looks to be reinventing stuff the driver model
> > already does.
> >
> > 'label' seems unnecessary. struct device embeds a struct kobject, which
> > means it has a name and shows up in sysfs. Is there a reason that the
> > device name cannot be used directly?
>
> hmm.. the label name is supposed to be a simpler name than device name.
> Say for instance "omap-usb2.1.auto" device name can simply be
> "omap-usb2-phy". Further the device name while using dt will have
> register address in it. However it's used only for displaying the label
> in sysfs entry (/sys/class/phy/<phy>/label).

I wouldn't go mucking with names in that way. Stick with one name and
drop the separate label. Otherwise you introduce addtional sources of
confusion.

> >
> > 'type' I just don't understand. I don't see any users of it in this
> > patch. I only see where it is set.
>
> yeah. Was planning to remove this in my next version (btw the latest is
> version 5).
> >
> > 'bus' absolutely should not be here. The bus type should be set in the
> > struct device by this subsystem when the device gets registered. There
> > is no reason to have a pointer to it here.
>
> right. I had removed it in version 5 of this patch series.
> >
> > 'of_node' is already in struct device
>
> I wasn't sure if we can manually assign the of_node of one device to
> of_node of an another device. Here the of_node comes from _phy provider_.

There is no problem with multiple devices referencing the same node. The
only time it may cause problems is when two devices of the same bus type
are referencing the same of_node. In that situation the device will get
probed more than once. You're not in that situation.

> > Finally, it really doesn't look right for a device object to have an
> > 'ops' structure. The whole point of the driver model is that a struct
> > device doesn't encapsulate any behaviour. A device gets registers to a
> > bus type, and then the driver core will associate a struct device_driver
>
> We have decided not to implement the PHY layer as a separate bus layer.
> The PHY provider can be part of any other bus. Making the PHY layer as a
> bus will make the PHY provider to be part of multiple buses which will
> lead to bad design. All we are trying to do here is keep the pool of PHY
> devices under PHY class in this layer and help any controller that wants
> to use the PHY to get it.

If you're using a class, then you already have your list of registered
phy devices! :-) No need to create another global list that you need to
manage.

You really need to be careful here though. By lumping all the phy types
into a single pot your glossing over the fact that different subsystems
have phys that behave differently. It is just fine to have a framework
that works in lots of places, it's actively encouraged, but by putting
the struct device into struct phy, you're taping into a large amount of
assumptions that the linux kernel makes about what devices are. For
instance, it is generally assumed that a class contains a bunch of the
same devices; but in this framework it is explicitly not true.

I would also imagine that each of the subsystems will want to wrap the
struct phy with subsystem specific additions so it will be important to
ensure the correct type of device is always returned.

You'd be better off providing a set of utility functions that work for
all the types of phys, but have a separate instance for each one. For
example, you could have a separate class for each phy type, but all of
them use the same utility function.

> > to a device that it is able to drive, and the struct device_driver is
> > supposed to contain any ops structure used to control the device.
> >
> >> +
> >> +/**
> >> + * struct phy_bind - represent the binding for the phy
> >> + * @dev_name: the device name of the device that will bind to the phy
> >> + * @phy_dev_name: the device name of the phy
> >> + * @index: used if a single controller uses multiple phys
> >> + * @auto_bind: tells if the binding is done explicitly from board file or not
> >> + * @phy: reference to the phy
> >> + * @list: to maintain a linked list of the binding information
> >> + */
> >> +struct phy_bind {
> >> + const char *dev_name;
> >> + const char *phy_dev_name;
> >> + int index;
> >> + int auto_bind:1;
> >> + struct phy *phy;
> >> + struct list_head list;
> >> +};
> >
> > How are PHYs attached to the system. Would the PHY be a direct child of
> > the host controller device, or would a PHY hang off another bus (like
> > i2c) for control? (this is how Ethernet phys work; they hang off the
> > MDIO bus, but may be attached to any Ethernet controller).
>
> This layer does not have any restriction on how the PHY is attached to
> the system. For all the sample cases (USB OTG controller in OMAP
> platforms) that follows this patch, the PHY is attached to the platform
> bus and the PHY provider is a platform driver. All this framework does
> is maintain a list of PHY's added in the system and helps the controller
> to find the appropriate PHY.

Okay, that's what I thought. A lot of systems end up looking this way.
So I would assume that for an i2c-controlled phy, and i2c driver would
bind against the device, and the probe hook would create and register a
phy device. That would make the sysfs tree look something like:

/sys/devices/.../[email protected]/[email protected]/phy0

where, [email protected] is the i2c master, [email protected] is what the phy's driver
binds against, and phy0 is created and registered by the driver of
[email protected] at probe time. Correct?

> We are currently not looking at having Ethernet use this layer because
> it itself has a comprehensive PHY layer and merging it with this will
> take some effort. However other subsystems like USB, SATA, video etc..
> can make use of this.
> >
> > Since there is at most a 1:N relationship between host controllers and
> > PHYs, there shouldn't be any need for a separate structure to describe
> > binding. Put the inding data into the struct phy itself. Each host
> > controller can have a list of phys that it is bound to.
>
> No. Having the host controller to have a list of phys wont be a good
> idea IMHO. The host controller is just an IP and the PHY to which it
> will be connected can vary from board to board, platform to platform. So
> ideally this binding should come from platform initialization code/dt data.

That is not what I mean. I mean the host controller instance should
contain a list of all the PHYs that are attached to it. There should not
be a global list. That has nothing to do with where the data of binding
comes from.

g.

2013-04-16 10:19:19

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] drivers: phy: add generic PHY framework

Hi,

On Tuesday 16 April 2013 01:20 AM, Grant Likely wrote:
> On Mon, 15 Apr 2013 17:56:10 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
>> Hi,
>>
>> On Monday 15 April 2013 05:04 PM, Grant Likely wrote:
>>> On Wed, 20 Mar 2013 14:42:00 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
>>>> The PHY framework provides a set of APIs for the PHY drivers to
>>>> create/destroy a PHY and APIs for the PHY users to obtain a reference to the
>>>> PHY with or without using phandle. To obtain a reference to the PHY without
>>>> using phandle, the platform specfic intialization code (say from board file)
>>>> should have already called phy_bind with the binding information. The binding
>>>> information consists of phy's device name, phy user device name and an index.
>>>> The index is used when the same phy user binds to mulitple phys.
>>>>
>>>> PHY drivers should create the PHY by passing phy_descriptor that has
>>>> describes the PHY (label, type etc..) and ops like init, exit, suspend, resume,
>>>> poweron, shutdown.
>>>>
>>>> The documentation for the generic PHY framework is added in
>>>> Documentation/phy.txt and the documentation for the sysfs entry is added
>>>> in Documentation/ABI/testing/sysfs-class-phy.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <[email protected]>
>>>
>>> Hi Kishon,
>>>
>>> For review purposes, I'll skip most of the implementation and focus on
>>> the data structures. I think you need to take another look at the
>>> approach your using. The kernel already provides a lot of support for
>>> implementing devices and binding them to drivers that you should be
>>> able to use...
>>>
>>>
>>> [...]
>>>> +/**
>>>> + * struct phy - represent the phy device
>>>> + * @dev: phy device
>>>> + * @label: label given to phy
>>>> + * @type: specifies the phy type
>>>> + * @of_node: device node of the phy
>>>> + * @ops: function pointers for performing phy operations
>>>> + */
>>>> +struct phy {
>>>> + struct device dev;
>>>> + const char *label;
>>>> + int type;
>>>> + struct bus_type *bus;
>>>> + struct device_node *of_node;
>>>> + struct phy_ops *ops;
>>>> +};
>>>
>>> Alright, so the core of the functionality is this 'struct phy' which
>>> tracks all the instances of PHY devices. As I understand it each
>>> physical phy will have a 'struct phy' instance tracking it. That makes
>>> sense.
>>>
>>> struct phy embeds a struct device. Also good. The linux driver model
>>> knows all about devices and how to handle them. However, most of the
>>> rest of this structure looks to be reinventing stuff the driver model
>>> already does.
>>>
>>> 'label' seems unnecessary. struct device embeds a struct kobject, which
>>> means it has a name and shows up in sysfs. Is there a reason that the
>>> device name cannot be used directly?
>>
>> hmm.. the label name is supposed to be a simpler name than device name.
>> Say for instance "omap-usb2.1.auto" device name can simply be
>> "omap-usb2-phy". Further the device name while using dt will have
>> register address in it. However it's used only for displaying the label
>> in sysfs entry (/sys/class/phy/<phy>/label).
>
> I wouldn't go mucking with names in that way. Stick with one name and
> drop the separate label. Otherwise you introduce addtional sources of
> confusion.

hmm.. ok.
>
>>>
>>> 'type' I just don't understand. I don't see any users of it in this
>>> patch. I only see where it is set.
>>
>> yeah. Was planning to remove this in my next version (btw the latest is
>> version 5).
>>>
>>> 'bus' absolutely should not be here. The bus type should be set in the
>>> struct device by this subsystem when the device gets registered. There
>>> is no reason to have a pointer to it here.
>>
>> right. I had removed it in version 5 of this patch series.
>>>
>>> 'of_node' is already in struct device
>>
>> I wasn't sure if we can manually assign the of_node of one device to
>> of_node of an another device. Here the of_node comes from _phy provider_.
>
> There is no problem with multiple devices referencing the same node. The
> only time it may cause problems is when two devices of the same bus type
> are referencing the same of_node. In that situation the device will get
> probed more than once. You're not in that situation.

right. Will re-use of_node.
>
>>> Finally, it really doesn't look right for a device object to have an
>>> 'ops' structure. The whole point of the driver model is that a struct
>>> device doesn't encapsulate any behaviour. A device gets registers to a
>>> bus type, and then the driver core will associate a struct device_driver
>>
>> We have decided not to implement the PHY layer as a separate bus layer.
>> The PHY provider can be part of any other bus. Making the PHY layer as a
>> bus will make the PHY provider to be part of multiple buses which will
>> lead to bad design. All we are trying to do here is keep the pool of PHY
>> devices under PHY class in this layer and help any controller that wants
>> to use the PHY to get it.
>
> If you're using a class, then you already have your list of registered
> phy devices! :-) No need to create another global list that you need to
> manage.

right. We already use _class_dev_iter_ for finding the phy device.
.
.
+static struct phy *of_phy_lookup(struct device *dev, struct device_node
*node)
+{
+ struct phy *phy;
+ struct class_dev_iter iter;
+
+ class_dev_iter_init(&iter, phy_class, NULL, NULL);
+ while ((dev = class_dev_iter_next(&iter))) {
+ phy = container_of(dev, struct phy, dev);
+ if (node != phy->of_node)
+ continue;
+
+ class_dev_iter_exit(&iter);
+ return phy;
+ }
+
+ class_dev_iter_exit(&iter);
+ return ERR_PTR(-EPROBE_DEFER);
+}
.
.

however we can't get rid of the other list (phy_bind_list) where we
maintain the phy binding information. It's used for the non-dt boot case.
>
> You really need to be careful here though. By lumping all the phy types
> into a single pot your glossing over the fact that different subsystems
> have phys that behave differently. It is just fine to have a framework
> that works in lots of places, it's actively encouraged, but by putting
> the struct device into struct phy, you're taping into a large amount of
> assumptions that the linux kernel makes about what devices are. For
> instance, it is generally assumed that a class contains a bunch of the
> same devices; but in this framework it is explicitly not true.
>
> I would also imagine that each of the subsystems will want to wrap the
> struct phy with subsystem specific additions so it will be important to
> ensure the correct type of device is always returned.

right. Probably this framework right now tries to implement the
simplest/default case which wouldn't need subsystem specific additions.
But yes, we might need to add subsystem specific additions in the future.
>
> You'd be better off providing a set of utility functions that work for
> all the types of phys, but have a separate instance for each one. For
> example, you could have a separate class for each phy type, but all of
> them use the same utility function.
>
>>> to a device that it is able to drive, and the struct device_driver is
>>> supposed to contain any ops structure used to control the device.
>>>
>>>> +
>>>> +/**
>>>> + * struct phy_bind - represent the binding for the phy
>>>> + * @dev_name: the device name of the device that will bind to the phy
>>>> + * @phy_dev_name: the device name of the phy
>>>> + * @index: used if a single controller uses multiple phys
>>>> + * @auto_bind: tells if the binding is done explicitly from board file or not
>>>> + * @phy: reference to the phy
>>>> + * @list: to maintain a linked list of the binding information
>>>> + */
>>>> +struct phy_bind {
>>>> + const char *dev_name;
>>>> + const char *phy_dev_name;
>>>> + int index;
>>>> + int auto_bind:1;
>>>> + struct phy *phy;
>>>> + struct list_head list;
>>>> +};
>>>
>>> How are PHYs attached to the system. Would the PHY be a direct child of
>>> the host controller device, or would a PHY hang off another bus (like
>>> i2c) for control? (this is how Ethernet phys work; they hang off the
>>> MDIO bus, but may be attached to any Ethernet controller).
>>
>> This layer does not have any restriction on how the PHY is attached to
>> the system. For all the sample cases (USB OTG controller in OMAP
>> platforms) that follows this patch, the PHY is attached to the platform
>> bus and the PHY provider is a platform driver. All this framework does
>> is maintain a list of PHY's added in the system and helps the controller
>> to find the appropriate PHY.
>
> Okay, that's what I thought. A lot of systems end up looking this way.
> So I would assume that for an i2c-controlled phy, and i2c driver would
> bind against the device, and the probe hook would create and register a
> phy device. That would make the sysfs tree look something like:
>
> /sys/devices/.../[email protected]/[email protected]/phy0
>
> where, [email protected] is the i2c master, [email protected] is what the phy's driver
> binds against, and phy0 is created and registered by the driver of
> [email protected] at probe time. Correct?

correct.
>
>> We are currently not looking at having Ethernet use this layer because
>> it itself has a comprehensive PHY layer and merging it with this will
>> take some effort. However other subsystems like USB, SATA, video etc..
>> can make use of this.
>>>
>>> Since there is at most a 1:N relationship between host controllers and
>>> PHYs, there shouldn't be any need for a separate structure to describe
>>> binding. Put the inding data into the struct phy itself. Each host
>>> controller can have a list of phys that it is bound to.
>>
>> No. Having the host controller to have a list of phys wont be a good
>> idea IMHO. The host controller is just an IP and the PHY to which it
>> will be connected can vary from board to board, platform to platform. So
>> ideally this binding should come from platform initialization code/dt data.
>
> That is not what I mean. I mean the host controller instance should
> contain a list of all the PHYs that are attached to it. There should not

Doesn't sound correct IMO. The host controller instance need not know
anything about the PHY instances that is connected to it. Think of it
similar to regulator, the controller wouldn't know which regulator it is
connected to, all it has to know is it just has a regulator connected to
it. It's up-to the regulator framework to give the controller the
correct regulator. It's similar here. It makes sense for me to keep a
list in the PHY framework in order for it to return the correct PHY (but
note that this list is not needed for dt boot).

Thanks
Kishon

2013-04-19 09:09:40

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] drivers: phy: add generic PHY framework

On Tue, 16 Apr 2013 15:48:07 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
> On Tuesday 16 April 2013 01:20 AM, Grant Likely wrote:
> > On Mon, 15 Apr 2013 17:56:10 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
> >> On Monday 15 April 2013 05:04 PM, Grant Likely wrote:
> >>> On Wed, 20 Mar 2013 14:42:00 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
> >> We have decided not to implement the PHY layer as a separate bus layer.
> >> The PHY provider can be part of any other bus. Making the PHY layer as a
> >> bus will make the PHY provider to be part of multiple buses which will
> >> lead to bad design. All we are trying to do here is keep the pool of PHY
> >> devices under PHY class in this layer and help any controller that wants
> >> to use the PHY to get it.
> >
> > If you're using a class, then you already have your list of registered
> > phy devices! :-) No need to create another global list that you need to
> > manage.
>
> right. We already use _class_dev_iter_ for finding the phy device.
> .
> .
> +static struct phy *of_phy_lookup(struct device *dev, struct device_node
> *node)
> +{
> + struct phy *phy;
> + struct class_dev_iter iter;
> +
> + class_dev_iter_init(&iter, phy_class, NULL, NULL);
> + while ((dev = class_dev_iter_next(&iter))) {
> + phy = container_of(dev, struct phy, dev);
> + if (node != phy->of_node)
> + continue;
> +
> + class_dev_iter_exit(&iter);
> + return phy;
> + }
> +
> + class_dev_iter_exit(&iter);
> + return ERR_PTR(-EPROBE_DEFER);
> +}
> .
> .
>
> however we can't get rid of the other list (phy_bind_list) where we
> maintain the phy binding information. It's used for the non-dt boot case.

Why? If you're using a class, then it is always there. Why would non-DT
and DT be different in this regard? (more below)

> >>> Since there is at most a 1:N relationship between host controllers and
> >>> PHYs, there shouldn't be any need for a separate structure to describe
> >>> binding. Put the inding data into the struct phy itself. Each host
> >>> controller can have a list of phys that it is bound to.
> >>
> >> No. Having the host controller to have a list of phys wont be a good
> >> idea IMHO. The host controller is just an IP and the PHY to which it
> >> will be connected can vary from board to board, platform to platform. So
> >> ideally this binding should come from platform initialization code/dt data.
> >
> > That is not what I mean. I mean the host controller instance should
> > contain a list of all the PHYs that are attached to it. There should not
>
> Doesn't sound correct IMO. The host controller instance need not know
> anything about the PHY instances that is connected to it. Think of it
> similar to regulator, the controller wouldn't know which regulator it is
> connected to, all it has to know is it just has a regulator connected to
> it. It's up-to the regulator framework to give the controller the
> correct regulator. It's similar here. It makes sense for me to keep a
> list in the PHY framework in order for it to return the correct PHY (but
> note that this list is not needed for dt boot).

With regulators and clocks it makes sense to have a global
registration place becase both implement an interconnected network
independent of the device that use them. (clocks depend on other clocks;
regulators depend on other regulators).

Phys are different. There is a 1:N relationship between host controllers
and phys, and you don't get a interconnected network of PHYs. Its a bad
idea to keep the binding data separate from the actual host controller
when there is nothing else that actually needs to use the data. It
creates a new set of data structures that need housekeeping to keep them
in sync with the actual device structures. It really is just a bad idea
and it becomes more difficult (in the non-DT case) to determine what
data is associated with a given host controller. You can't tell by
looking at the struct device.

Instead, for the non-DT case, do what we've always done for describing
connections. Put the phy reference into the host controllers
platform_data structure. That is what it is there for. That completely
eliminates the need to housekeep a new set of data structures.

g.

2013-04-19 10:54:07

by Sekhar Nori

[permalink] [raw]
Subject: Re: [PATCH v3 0/6] Generic PHY Framework

Hi Kishon,

On 3/20/2013 2:41 PM, Kishon Vijay Abraham I wrote:
> Added a generic PHY framework that provides a set of APIs for the PHY drivers
> to create/destroy a PHY and APIs for the PHY users to obtain a reference to
> the PHY with or without using phandle. To obtain a reference to the PHY
> without using phandle, the platform specfic intialization code (say from board
> file) should have already called phy_bind with the binding information. The
> binding information consists of phy's device name, phy user device name and an
> index. The index is used when the same phy user binds to mulitple phys.
>
> This framework will be of use only to devices that uses external PHY (PHY
> functionality is not embedded within the controller).

>From a top level what you are doing looks closely related to External
connector (extcon).

I understand a connector is not the same as a phy, but it will still be
useful to know why extcon framework (or some extension of it) will not
suffice your needs.

You can probably note it in the cover-letter so folks like me get their
answer.

Thanks,
Sekhar

2013-04-22 06:11:08

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] drivers: phy: add generic PHY framework

Hi,

On Friday 19 April 2013 02:39 PM, Grant Likely wrote:
> On Tue, 16 Apr 2013 15:48:07 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
>> On Tuesday 16 April 2013 01:20 AM, Grant Likely wrote:
>>> On Mon, 15 Apr 2013 17:56:10 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
>>>> On Monday 15 April 2013 05:04 PM, Grant Likely wrote:
>>>>> On Wed, 20 Mar 2013 14:42:00 +0530, Kishon Vijay Abraham I <[email protected]> wrote:
>>>> We have decided not to implement the PHY layer as a separate bus layer.
>>>> The PHY provider can be part of any other bus. Making the PHY layer as a
>>>> bus will make the PHY provider to be part of multiple buses which will
>>>> lead to bad design. All we are trying to do here is keep the pool of PHY
>>>> devices under PHY class in this layer and help any controller that wants
>>>> to use the PHY to get it.
>>>
>>> If you're using a class, then you already have your list of registered
>>> phy devices! :-) No need to create another global list that you need to
>>> manage.
>>
>> right. We already use _class_dev_iter_ for finding the phy device.
>> .
>> .
>> +static struct phy *of_phy_lookup(struct device *dev, struct device_node
>> *node)
>> +{
>> + struct phy *phy;
>> + struct class_dev_iter iter;
>> +
>> + class_dev_iter_init(&iter, phy_class, NULL, NULL);
>> + while ((dev = class_dev_iter_next(&iter))) {
>> + phy = container_of(dev, struct phy, dev);
>> + if (node != phy->of_node)
>> + continue;
>> +
>> + class_dev_iter_exit(&iter);
>> + return phy;
>> + }
>> +
>> + class_dev_iter_exit(&iter);
>> + return ERR_PTR(-EPROBE_DEFER);
>> +}
>> .
>> .
>>
>> however we can't get rid of the other list (phy_bind_list) where we
>> maintain the phy binding information. It's used for the non-dt boot case.
>
> Why? If you're using a class, then it is always there. Why would non-DT
> and DT be different in this regard? (more below)
>
>>>>> Since there is at most a 1:N relationship between host controllers and
>>>>> PHYs, there shouldn't be any need for a separate structure to describe
>>>>> binding. Put the inding data into the struct phy itself. Each host
>>>>> controller can have a list of phys that it is bound to.
>>>>
>>>> No. Having the host controller to have a list of phys wont be a good
>>>> idea IMHO. The host controller is just an IP and the PHY to which it
>>>> will be connected can vary from board to board, platform to platform. So
>>>> ideally this binding should come from platform initialization code/dt data.
>>>
>>> That is not what I mean. I mean the host controller instance should
>>> contain a list of all the PHYs that are attached to it. There should not
>>
>> Doesn't sound correct IMO. The host controller instance need not know
>> anything about the PHY instances that is connected to it. Think of it
>> similar to regulator, the controller wouldn't know which regulator it is
>> connected to, all it has to know is it just has a regulator connected to
>> it. It's up-to the regulator framework to give the controller the
>> correct regulator. It's similar here. It makes sense for me to keep a
>> list in the PHY framework in order for it to return the correct PHY (but
>> note that this list is not needed for dt boot).
>
> With regulators and clocks it makes sense to have a global
> registration place becase both implement an interconnected network
> independent of the device that use them. (clocks depend on other clocks;
> regulators depend on other regulators).
>
> Phys are different. There is a 1:N relationship between host controllers
> and phys, and you don't get a interconnected network of PHYs. Its a bad
> idea to keep the binding data separate from the actual host controller
> when there is nothing else that actually needs to use the data. It
> creates a new set of data structures that need housekeeping to keep them
> in sync with the actual device structures. It really is just a bad idea
> and it becomes more difficult (in the non-DT case) to determine what
> data is associated with a given host controller. You can't tell by
> looking at the struct device.
>
> Instead, for the non-DT case, do what we've always done for describing
> connections. Put the phy reference into the host controllers
> platform_data structure.
hmm... my only concern here is there is no way we can enforce the phy
reference is added in the platform_data structure.
That is what it is there for. That completely
> eliminates the need to housekeep a new set of data structures.

Ok. Makes sense.

Thanks
Kishon