Re: [PATCH v5 1/6] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I
Date: Thu Apr 04 2013 - 05:22:53 EST
Hi,
On Thursday 04 April 2013 03:16 AM, Sylwester Nawrocki wrote:
On 04/03/2013 02:53 PM, Kishon Vijay Abraham I wrote:
.
.
<snip>
.
.
diff --git a/Documentation/phy.txt b/Documentation/phy.txt
new file mode 100644
index 0000000..7785ec0
--- /dev/null
+++ b/Documentation/phy.txt
@@ -0,0 +1,113 @@
+ PHY SUBSYSTEM
+ Kishon Vijay Abraham I<kishon@xxxxxx>
+
+This document explains the Generic PHY Framework along with the APIs
provided,
+and how-to-use.
+
+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
Shouldn't it be "...medium, e.g. the USB..." ?
+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
"controllers have PHY functionality embedded into them" ?
+PHY. Other peripherals that uses a PHY include Wireless LAN, Ethernet,
s/uses/use ?
+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
s/uses/use ?
+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, power_on and power_off.
+
+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.
s/in the dt data of/in the device tree node of ?
+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);
Why do we need separate functions for dt and non-dt ? Couldn't it be
handled
internally by the framework ? So the PHY users can use same calls for dt
and non-dt, like in case of e.g. the regulators API ?
yeah. Indeed it makes sense to do it that way.
Also signatures of some functions are different now:
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);
My bad :-(
BTW, I think "extern" could be dropped, does it have any significance in
function declarations in header files ?
it shouldn't have any effect I guess. It's harmless nevertheless.
+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
s/the the/the
+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
s/phandle/property ?
+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.
s/APIS/APIs ?
+
+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
s/APIs destroys/APIs destroy ?
+associated with this PHY.
+
+7. DeviceTree Binding
+
+The documentation for PHY dt binding can be found @
+Documentation/devicetree/bindings/phy/phy-bindings.txt
--- /dev/null
+++ b/drivers/phy/phy-core.c
@@ -0,0 +1,616 @@
+static struct phy *of_phy_lookup(struct device_node *node)
+{
+ struct phy *phy;
+ struct device *dev;
+ struct class_dev_iter iter;
+
+ class_dev_iter_init(&iter, phy_class, NULL, NULL);
There is currently nothing preventing a call to this function before
phy_class is initialized. It happened during my tests, and the nice
stack dump showed that it was the PHY user attempting to get the PHY
before the framework got initialized. So either phy_core_init should
be a subsys_initcall or we need a better protection against phy_class
being NULL or ERR_PTR in more places.
Whats the initcall used in your PHY user? Looks like more people prefer
having module_init and any issue because of it should be fixed in PHY
providers and PHY users.
+ 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);
+}
+/**
+ * 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)
+{
+ int ret;
+ struct phy *phy = NULL;
+ struct phy_bind *phy_map = NULL;
+ struct of_phandle_args args;
+ struct device_node *node;
+
+ if (!dev->of_node) {
+ dev_dbg(dev, "device does not have a device node entry\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ ret = of_parse_phandle_with_args(dev->of_node, "phys", "#phy-cells",
+ index,&args);
+ if (ret) {
+ dev_dbg(dev, "failed to get phy in %s node\n",
+ dev->of_node->full_name);
+ return ERR_PTR(-ENODEV);
+ }
+
+ phy = of_phy_lookup(args.np);
+ if (IS_ERR(phy) || !try_module_get(phy->ops->owner)) {
+ phy = ERR_PTR(-EPROBE_DEFER);
+ goto err0;
+ }
+
+ phy = phy->ops->of_xlate(phy,&args);
+ if (IS_ERR(phy))
+ goto err1;
+
+ 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;
Shouldn't it be done with the phy_bind_mutex held ? I guess an unlocked
version of the phy_bind functions is needed, so it can be used internally.
The locking is done inside phy_bind function. I'm not sure but I vaguely
remember getting a dump stack (incorrect mutex ordering or something)
when trying to have phy_bind_mutex here. I can check it again.
+ }
+
+ get_device(&phy->dev);
+
+err1:
+ module_put(phy->ops->owner);
+
+err0:
+ of_node_put(node);
+
+ return phy;
+}
+EXPORT_SYMBOL_GPL(of_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 || !ops->of_xlate || !priv) {
+ dev_err(dev, "no PHY ops/PHY data provided\n");
+ ret = -EINVAL;
+ goto err0;
+ }
+
+ if (!phy_class)
+ phy_core_init();
+
+ phy = kzalloc(sizeof(*phy), GFP_KERNEL);
+ if (!phy) {
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ devname = dev_name(dev);
OK, a sort of serious issue here is that you can't call phy_create()
multiple times for same PHY provider device.
Ah.. right.
device_add() further will fail as sysfs won't let you create multiple
objects with same name. So I would assume we need to add a new argument
to phy_create() or rename @type to e.g. @phy_id, which could be
concatenated with dev_name(dev) to create a unique name of the phy
device.
hmm.. ok
And how is the PHY provider supposed to identify a PHY in its common PHY
ops, now when the struct phy port field is removed ? I have temporarily
(ab)used the type field in order to select proper registers within the
phy ops.
Can't it be handled in the PHY provider driver without using struct phy
*? Moreover the PHY ops passes on the *phy to phy provider right? Not
sure I understand you here.
+ device_initialize(&phy->dev);
+
+ phy->dev.class = phy_class;
+ phy->dev.parent = dev;
+ phy->label = label;
What about duplicating the string here ? That would make the API a bit
more convenient and the callers wouldn't be required to keep all the
labels around.
you mean use dev_name? The device names are sometime more cryptic so
preferred to have it like this.
+ 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
Duplicated line.
+ * @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.
+ */
+/**
+ * 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;
+
+ mutex_lock(&phy_bind_mutex);
+ list_for_each_entry(phy_bind,&phy_bind_list, list) {
+ if (!strcmp(phy_bind->dev_name, dev_name)&& phy_bind->index ==
+ index) {
+ phy_bind->phy_dev_name = phy_dev_name;
+ goto ret0;
+ }
+ }
+
+ phy_bind = kzalloc(sizeof(*phy_bind), GFP_KERNEL);
+ if (!phy_bind) {
+ phy_bind = ERR_PTR(-ENOMEM);
+ goto ret0;
+ }
+
+ 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);
+
+ret0:
+ mutex_unlock(&phy_bind_mutex);
+ return phy_bind;
+}
+EXPORT_SYMBOL_GPL(phy_bind);
+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);
+
Shouldn't this iteration also be protected with the phy_bind_mutex ?
Indeed.
+ 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 int phy_core_init(void)
+{
+ if (phy_class)
+ return 0;
+
+ 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;
+}
+module_init(phy_core_init);
Having this framework initialized before the PHY provider and consumer
drivers could save some unnecessary probe deferrals. I was wondering,
what is really an advantage of having it as module_init(), rather than
subsys_initcall() ?
I'm not sure of the exact reason but after the advent of EPROBE_DEFER,
everyone recommends to use module_init only.
Thanks for the detailed review.
Regards
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/