Hi,
On Thursday 28 March 2013 09:15 PM, Stephen Warren wrote:
On 03/27/2013 11:43 PM, 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
diff --git a/Documentation/devicetree/bindings/phy/phy-bindings.txt b/Documentation/devicetree/bindings/phy/phy-bindings.txt
+This document explains only the dt data binding. For general information about
+PHY subsystem refer Documentation/phy.txt
+
+PHY device node
+===============
+
+Optional Properties:
+#phy-cells: Number of cells in a PHY specifier; The meaning of all those
+ cells is defined by the binding for the phy node. However
+ in-order to return the correct PHY, the PHY susbsystem
+ requires the first cell always refers to the port.
Why impose that restriction? Other DT bindings do not.
This is typically implemented by having each provider driver implement a
.of_xlate() operation, which parses all of the specifier cells, and
returns the ID of the object it represents. This allows bindings to use
whatever arbitrary representation they want.
Do you mean something like this
struct phy *of_phy_get(struct device *dev, int index)
{
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);
}
//Here we have to get a reference to the phy in order to call of_xlate
which seems a little hacky to me. I'm not sure how else can we call the
provider driver :-(
phy = of_phy_lookup(dev, node);
if (IS_ERR(phy) || !try_module_get(phy->ops->owner)) {
phy = ERR_PTR(-EPROBE_DEFER);
goto err0;
}
//here we are checking if the phy has additional specifiers and if so
call of_xlate using the phy we just obtained. The provider driver should
check the args and return the appropriate *phy in this case.
if (args.args_count > 0) {
phy = phy->of_xlate(&args);
if (IS_ERR(phy))
goto err0;
}
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;
}
get_device(&phy->dev);
err0:
of_node_put(node);
return phy;
}
EXPORT_SYMBOL_GPL(of_phy_get);
For the common/simple cases where #phy-cells==0, or #phy-cells==1 and
directly represents the PHY ID, the PHY core can provide an
implementation of that common .of_xlate() function, which PHY provider
drivers can simply plug in as their .of_xlate() function.
+This property is optional because it is needed only for the case where a
+single IP implements multiple PHYs.
The property should always exist so that the DT-parsing code in the PHY
core can always validate exactly how many cells are present in the PHY
specifier.
+
+For example:
+
+phys: phy {
+ compatible = "xxx";
+ reg1 = <...>;
+ reg2 = <...>;
+ reg3 = <...>;
3 separate reg values should be 3 separate entries in a single reg
property, not 3 separate reg properties, with non-standard names.
+ .
+ .
+ #phy-cells = <1>;
+ .
+ .
+};
+
+That node describes an IP block that implements 3 different PHYs. In order to
+differentiate between these 3 PHYs, an additonal specifier should be given
+while trying to get a reference to it. (The PHY subsystem assumes the
+specifier is port id).
A single DT node would typically represent a single HW IP block, and
hence typically have a single reg property. If there are 3 separate HW
IP blocks, and their register aren't interleaved, and hence can be
represented by 3 separate reg values, then I'd typically expect to see 3
separate DT nodes, one for each independent register range.
The only case where I'd expect a single DT node to provide multipe PHYs,
is when a single HW IP block actually implements multiple PHYs /and/ the
registers for those 3 PHYs are interleaved (or share bits in the same
registers) such that each PHY can't be represented by a separate
non-overlapping reg property.
+example1:
+phys: phy {
How about:
Example 1:
usb1: usb@xxx {
+};
+This node represents a controller that uses two PHYs one for usb2 and one for
Blank line after }?
+usb3. The controller driver can get the appropriate PHY either by using
+devm_of_phy_get/of_phy_get by passing the correct index or by using
+of_phy_get_byname/devm_of_phy_get_byname by passing the names given in
+*phy-names*.
Discussions of Linux-specific driver APIs should be in the Linux API
documentation, not the DT binding documentation, which is supposed to be
OS-agnostic. Instead, perhaps say:
Individual bindings must specify the required set of entries the phys
property. Bindings must also specify either a required order for those
entries in the phys property, or specify required set of names that must
be present in the phy-names property, in which case the order is arbitrary.
+example2:
+phys: phy {
How about:
Example 2:
usb2: usb@yyy {
+This node represents a controller that uses one of the PHYs which is defined
+previously. Note that the phy handle has an additional specifier "1" to
+differentiate between the three PHYs. For this case, the controller driver
+should use of_phy_get_with_args/devm_of_phy_get_with_args.
I think tha last sentence should be removed, and perhaps the previous
sentence extended with:
, as required by #phy-cells = <1> in the PHY provider node.
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
+subsys_initcall(phy_core_init);
Why not make that module_init(); device probe() ordering should be
handled using -EPROBE_DEFER these days, so the exact initcall used
doesn't really matter, and hence it'd be best to use the most common one
rather than something unusual.
hmm.. ok.
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
