Re: [PATCH v10 3/9] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

From: Stephen Warren
Date: Thu Apr 07 2016 - 16:43:11 EST

On 04/06/2016 11:08 AM, Thierry Reding wrote:
On Tue, Apr 05, 2016 at 03:10:16PM -0600, Stephen Warren wrote:
On 04/05/2016 08:44 AM, Thierry Reding wrote:
On Wed, Mar 16, 2016 at 11:59:44AM -0600, Stephen Warren wrote:
On 03/04/2016 09:19 AM, Thierry Reding wrote:
From: Thierry Reding <treding@xxxxxxxxxx>

Extend the binding to cover the set of feature found in Tegra210.

Acked-by: Stephen Warren <swarren@xxxxxxxxxx>

diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt

+ padctl@0,7009f000 {
...
+ pads {
...
+ };
+
+ ports {
...
+ };

As a comment not affecting my ack in any way: At the top-level, we place all
the child nodes into "array container" nodes named "pads" and "ports". This
is nice since it separates different types of child nodes and allows easily
adding more types of child nodes in the future without interference, and in
a way that allows us to explicitly know what each node is without having to
interpret its name or compatible value to do so. However, we haven't done
this with the per-lane child nodes inside each pad. If we were to rev the
design, I'd be tempted to suggest:

padctl@0,7009f000 {
pads {
usb2 {
lanes { // This level is new
usb2-0 {

I tried to make this work, but it's unfortunately not possible with the
current code. The reason is that the PHY subsystem assumes that either
the provider DT node corresponds to that of the device (the usb2 pad in
the above example) or one of its children. Hence, putting everything
into one more level further down would require some mechanism to tell
the subsystem about it so that it can be found.

When the padctl driver registers the PHY objects with the PHY subsystem, can
it pass the lanes node as the DT node? That woulud mean each lane /was/ a
child of the node registered with the PHY subsystem.

Perhaps the PHY subsystem requires a struct device rather than a DT node
registered with it?

Yes, that's how the PHY subsystem works. You pass it a struct device *
that it will wrap a struct phy_provider around. It will then use that
struct device's ->of_node as the parent for the child lookup.

If so, does it make sense to create a separate struct device with the
of_node pointing at lanes{}?

I suspect that that would work, but I'm already somewhat uncomfortable
about how many devices the driver creates. Adding more dummy devices
seems like a workaround.

Arguably the current support code isn't a good argument for designing a
binding, so perhaps it'd be useful to add this mechanism in order to get
a better binding. On the other hand, I'm not sure it's really worth it,
since we already have generic bindings that specify the layout of child
devices, and those have been agreed upon broadly (presumably).

In light of the recent discussion on DPAUX vs. I2C, I see how having the
extra level would be useful to provide additional context. If you think
it's worth it I can spend the extra time to get this implemented in the
core.

Naively, it sounds like it'd be a good idea to fix the PHY core. It really
shouldn't care about the parent of any object registered with it; it should
only interact with the specific object it was given, and any other data such
as "ops" callbacks. Do you have any inkling how much work that would be?

I attached what I came up with. It extends the OF PHY provider registry
by allowing an additional node to be specified that if specified will
serve as the parent for the child lookup (and hence overrides the
default node that's taken from the struct device).

It is a fairly trivial patch, and you'll notice the bulk of the changes
is adding the additional parameter in a number of different places. The
only thing I'm not quite happy about is that we start needing to pass a
fairly large number of arguments. But perhaps it's still okay.

An alternative would be to make struct phy_provider embeddable into
driver private structures. That way they could be completely initialized
by a driver before being passed to the __of_phy_provider_register()
function (much like struct gpio_chip and others). That would be a fairly
intrusive change, one that I'd be willing to take on, though I'd like to
have Kishon's opinion on this before going ahead.

Either of those options look OK to me, and don't seem too invasive. I imagine the amount of work is not too great given you've already implemented the first option:-)

For reference, here's what I'm imagining:

struct foo_phy_provider {
struct phy_provider base;
...
};

int foo_probe(struct platform_device *pdev)
{
struct foo_phy_provider *priv;
...

priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;

priv->base.dev = &pdev->dev;
priv->base.of_xlate = foo_of_xlate;

err = devm_of_phy_provider_register(&priv->base);
if (err < 0)
return err;

...
}

And of course adapt the signature of the __of_phy_provider_register()
function and remove the allocation from it.

Thierry

--- >8 ---
From 15e5348a1a63837efd00309fdce5cda979498f77 Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@xxxxxxxxxx>
Date: Tue, 5 Apr 2016 17:17:34 +0200
Subject: [PATCH] phy: core: Allow children node to be overridden

In order to more flexibly support device tree bindings, allow drivers to
override the container of the child nodes. By default the device node of
the PHY provider is assumed to be the parent for children, but bindings
may decide to add additional levels for better organization.

Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
---
Documentation/phy.txt | 16 ++++++++++++++--
drivers/phy/phy-core.c | 27 +++++++++++++++++++++------
include/linux/phy/phy.h | 31 +++++++++++++++++++++----------
3 files changed, 56 insertions(+), 18 deletions(-)

diff --git a/Documentation/phy.txt b/Documentation/phy.txt
index b388c5af9e72..0aa994bd9a91 100644
--- a/Documentation/phy.txt
+++ b/Documentation/phy.txt
@@ -31,16 +31,28 @@ should provide its own implementation of of_xlate. of_xlate is used only for
dt boot case.

#define of_phy_provider_register(dev, xlate) \
- __of_phy_provider_register((dev), THIS_MODULE, (xlate))
+ __of_phy_provider_register((dev), NULL, THIS_MODULE, (xlate))

#define devm_of_phy_provider_register(dev, xlate) \
- __devm_of_phy_provider_register((dev), THIS_MODULE, (xlate))
+ __devm_of_phy_provider_register((dev), NULL, THIS_MODULE, (xlate))

of_phy_provider_register and devm_of_phy_provider_register macros can be used to
register the phy_provider and it takes device and of_xlate as
arguments. For the dt boot case, all PHY providers should use one of the above
2 macros to register the PHY provider.

+Often the device tree nodes associated with a PHY provider will contain a set
+of children that each represent a single PHY. Some bindings may nest the child
+nodes within extra levels for context and extensibility, in which case the low
+level of_phy_provider_register_full() and devm_of_phy_provider_register_full()
+macros can be used to override the node containing the children.
+
+#define of_phy_provider_register_full(dev, children, xlate) \
+ __of_phy_provider_register(dev, children, THIS_MODULE, xlate)
+
+#define devm_of_phy_provider_register_full(dev, children, xlate) \
+ __devm_of_phy_provider_register_full(dev, children, THIS_MODULE, xlate)
+
void devm_of_phy_provider_unregister(struct device *dev,
struct phy_provider *phy_provider);
void of_phy_provider_unregister(struct phy_provider *phy_provider);
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index e7e574dc667a..7f7da2138c82 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -135,13 +135,19 @@ static struct phy *phy_find(struct device *dev, const char *con_id)
static struct phy_provider *of_phy_provider_lookup(struct device_node *node)
{
struct phy_provider *phy_provider;
+ struct device_node *children;
struct device_node *child;

list_for_each_entry(phy_provider, &phy_provider_list, list) {
if (phy_provider->dev->of_node == node)
return phy_provider;

- for_each_child_of_node(phy_provider->dev->of_node, child)
+ if (!phy_provider->children)
+ children = phy_provider->dev->of_node;
+ else
+ children = phy_provider->children;
+
+ for_each_child_of_node(children, child)
if (child == node)
return phy_provider;
}
@@ -811,16 +817,22 @@ EXPORT_SYMBOL_GPL(devm_phy_destroy);
/**
* __of_phy_provider_register() - create/register phy provider with the framework
* @dev: struct device of the phy provider
+ * @children: device node containing children (if different from dev->of_node)
* @owner: the module owner containing of_xlate
* @of_xlate: function pointer to obtain phy instance from phy provider
*
* Creates struct phy_provider from dev and of_xlate function pointer.
* This is used in the case of dt boot for finding the phy instance from
* phy provider.
+ *
+ * If the PHY provider doesn't nest children directly but uses a separate
+ * child node to contain the individual children, the @children parameter
+ * can be used to override the default (i.e. dev->of_node).
*/
struct phy_provider *__of_phy_provider_register(struct device *dev,
- struct module *owner, struct phy * (*of_xlate)(struct device *dev,
- struct of_phandle_args *args))
+ struct device_node *children, struct module *owner,
+ struct phy * (*of_xlate)(struct device *dev,
+ struct of_phandle_args *args))
{
struct phy_provider *phy_provider;

@@ -829,6 +841,7 @@ struct phy_provider *__of_phy_provider_register(struct device *dev,
return ERR_PTR(-ENOMEM);

phy_provider->dev = dev;
+ phy_provider->children = children;
phy_provider->owner = owner;
phy_provider->of_xlate = of_xlate;

@@ -854,8 +867,9 @@ EXPORT_SYMBOL_GPL(__of_phy_provider_register);
* on the devres data, then, devres data is freed.
*/
struct phy_provider *__devm_of_phy_provider_register(struct device *dev,
- struct module *owner, struct phy * (*of_xlate)(struct device *dev,
- struct of_phandle_args *args))
+ struct device_node *children, struct module *owner,
+ struct phy * (*of_xlate)(struct device *dev,
+ struct of_phandle_args *args))
{
struct phy_provider **ptr, *phy_provider;

@@ -863,7 +877,8 @@ struct phy_provider *__devm_of_phy_provider_register(struct device *dev,
if (!ptr)
return ERR_PTR(-ENOMEM);

- phy_provider = __of_phy_provider_register(dev, owner, of_xlate);
+ phy_provider = __of_phy_provider_register(dev, children, owner,
+ of_xlate);
if (!IS_ERR(phy_provider)) {
*ptr = phy_provider;
devres_add(dev, ptr);
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 8cf05e341cff..a810f2a18842 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -77,6 +77,7 @@ struct phy {
*/
struct phy_provider {
struct device *dev;
+ struct device_node *children;
struct module *owner;
struct list_head list;
struct phy * (*of_xlate)(struct device *dev,
@@ -93,10 +94,16 @@ struct phy_lookup {
#define to_phy(a) (container_of((a), struct phy, dev))

#define of_phy_provider_register(dev, xlate) \
- __of_phy_provider_register((dev), THIS_MODULE, (xlate))
+ __of_phy_provider_register((dev), NULL, THIS_MODULE, (xlate))

#define devm_of_phy_provider_register(dev, xlate) \
- __devm_of_phy_provider_register((dev), THIS_MODULE, (xlate))
+ __devm_of_phy_provider_register((dev), NULL, THIS_MODULE, (xlate))
+
+#define of_phy_provider_register_full(dev, children, xlate) \
+ __of_phy_provider_register(dev, children, THIS_MODULE, xlate)
+
+#define devm_of_phy_provider_register_full(dev, children, xlate) \
+ __devm_of_phy_provider_register(dev, children, THIS_MODULE, xlate)

static inline void phy_set_drvdata(struct phy *phy, void *data)
{
@@ -147,11 +154,13 @@ struct phy *devm_phy_create(struct device *dev, struct device_node *node,
void phy_destroy(struct phy *phy);
void devm_phy_destroy(struct device *dev, struct phy *phy);
struct phy_provider *__of_phy_provider_register(struct device *dev,
- struct module *owner, struct phy * (*of_xlate)(struct device *dev,
- struct of_phandle_args *args));
+ struct device_node *children, struct module *owner,
+ struct phy * (*of_xlate)(struct device *dev,
+ struct of_phandle_args *args));
struct phy_provider *__devm_of_phy_provider_register(struct device *dev,
- struct module *owner, struct phy * (*of_xlate)(struct device *dev,
- struct of_phandle_args *args));
+ struct device_node *children, struct module *owner,
+ struct phy * (*of_xlate)(struct device *dev,
+ struct of_phandle_args *args));
void of_phy_provider_unregister(struct phy_provider *phy_provider);
void devm_of_phy_provider_unregister(struct device *dev,
struct phy_provider *phy_provider);
@@ -312,15 +321,17 @@ static inline void devm_phy_destroy(struct device *dev, struct phy *phy)
}

static inline struct phy_provider *__of_phy_provider_register(
- struct device *dev, struct module *owner, struct phy * (*of_xlate)(
- struct device *dev, struct of_phandle_args *args))
+ struct device *dev, struct device_node *children, struct module *owner,
+ struct phy * (*of_xlate)(struct device *dev,
+ struct of_phandle_args *args))
{
return ERR_PTR(-ENOSYS);
}

static inline struct phy_provider *__devm_of_phy_provider_register(struct device
- *dev, struct module *owner, struct phy * (*of_xlate)(struct device *dev,
- struct of_phandle_args *args))
+ *dev, struct device_node *children, struct module *owner,
+ struct phy * (*of_xlate)(struct device *dev,
+ struct of_phandle_args *args))
{
return ERR_PTR(-ENOSYS);
}