Re: [PATCHv4 2/6] phy: improved lookup method

From: Vivek Gautam
Date: Thu Nov 13 2014 - 08:33:09 EST

Hi Heikki, Kishon,


On Tue, Nov 11, 2014 at 2:23 PM, Vivek Gautam <gautamvivek1987@xxxxxxxxx> wrote:
> Hi Kishon,
>
>
> On Tue, Nov 11, 2014 at 2:20 PM, Kishon Vijay Abraham I <kishon@xxxxxx> wrote:
>> Hi,
>>
>> On Tuesday 11 November 2014 02:07 PM, Vivek Gautam wrote:
>>> On Tue, Nov 11, 2014 at 12:12 PM, Kishon Vijay Abraham I <kishon@xxxxxx> wrote:
>>>> Hi,
>>>>
>>>> On Friday 31 October 2014 06:03 PM, Vivek Gautam wrote:
>>>>> Hi Heikki,
>>>>>
>>>>>
>>>>> On Fri, Oct 17, 2014 at 8:09 PM, Heikki Krogerus
>>>>> <heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>>>>>> Removes the need for the phys to be aware of their users
>>>>>> even when not using DT. The method is copied from clkdev.c.
>>>>>>
>>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
>>>>>> Tested-by: Vivek Gautam <gautam.vivek@xxxxxxxxxxx>
>>>>>> ---
>>>>>> Documentation/phy.txt | 66 ++++++++---------------
>>>>>> drivers/phy/phy-core.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>> include/linux/phy/phy.h | 27 ++++++++++
>>>>>> 3 files changed, 183 insertions(+), 45 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/phy.txt b/Documentation/phy.txt
>>>>>> index c6594af..8add515 100644
>>>>>> --- a/Documentation/phy.txt
>>>>>> +++ b/Documentation/phy.txt
>>>>>> @@ -54,18 +54,14 @@ 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, struct device_node *node,
>>>>>> - const struct phy_ops *ops,
>>>>>> - struct phy_init_data *init_data);
>>>>>> + const struct phy_ops *ops);
>>>>>> struct phy *devm_phy_create(struct device *dev, struct device_node *node,
>>>>>> - const struct phy_ops *ops,
>>>>>> - struct phy_init_data *init_data);
>>>>>> + const struct phy_ops *ops);
>>>>>>
>>>>>> The PHY drivers can use one of the above 2 APIs to create the PHY by passing
>>>>>> -the device pointer, phy ops and init_data.
>>>>>> +the device pointer and phy ops.
>>>>>> phy_ops is a set of function pointers for performing PHY operations such as
>>>>>> -init, exit, power_on and power_off. *init_data* is mandatory to get a reference
>>>>>> -to the PHY in the case of non-dt boot. See section *Board File Initialization*
>>>>>> -on how init_data should be used.
>>>>>> +init, exit, power_on and power_off.
>>>>>>
>>>>>> Inorder to dereference the private data (in phy_ops), the phy provider driver
>>>>>> can use phy_set_drvdata() after creating the PHY and use phy_get_drvdata() in
>>>>>> @@ -137,42 +133,24 @@ There are exported APIs like phy_pm_runtime_get, phy_pm_runtime_get_sync,
>>>>>> phy_pm_runtime_put, phy_pm_runtime_put_sync, phy_pm_runtime_allow and
>>>>>> phy_pm_runtime_forbid for performing PM operations.
>>>>>>
>>>>>> -8. Board File Initialization
>>>>>> -
>>>>>> -Certain board file initialization is necessary in order to get a reference
>>>>>> -to the PHY in the case of non-dt boot.
>>>>>> -Say we have a single device that implements 3 PHYs that of USB, SATA and PCIe,
>>>>>> -then in the board file the following initialization should be done.
>>>>>> -
>>>>>> -struct phy_consumer consumers[] = {
>>>>>> - PHY_CONSUMER("dwc3.0", "usb"),
>>>>>> - PHY_CONSUMER("pcie.0", "pcie"),
>>>>>> - PHY_CONSUMER("sata.0", "sata"),
>>>>>> -};
>>>>>> -PHY_CONSUMER takes 2 parameters, first is the device name of the controller
>>>>>> -(PHY consumer) and second is the port name.
>>>>>> -
>>>>>> -struct phy_init_data init_data = {
>>>>>> - .consumers = consumers,
>>>>>> - .num_consumers = ARRAY_SIZE(consumers),
>>>>>> -};
>>>>>> -
>>>>>> -static const struct platform_device pipe3_phy_dev = {
>>>>>> - .name = "pipe3-phy",
>>>>>> - .id = -1,
>>>>>> - .dev = {
>>>>>> - .platform_data = {
>>>>>> - .init_data = &init_data,
>>>>>> - },
>>>>>> - },
>>>>>> -};
>>>>>> -
>>>>>> -then, while doing phy_create, the PHY driver should pass this init_data
>>>>>> - phy_create(dev, ops, pdata->init_data);
>>>>>> -
>>>>>> -and the controller driver (phy consumer) should pass the port name along with
>>>>>> -the device to get a reference to the PHY
>>>>>> - phy_get(dev, "pcie");
>>>>>> +8. PHY Mappings
>>>>>> +
>>>>>> +In order to get reference to a PHY without help from DeviceTree, the framework
>>>>>> +offers lookups which can be compared to clkdev that allow clk structures to be
>>>>>> +bound to devices. A lookup can be made statically by directly registering
>>>>>> +phy_lookup structure which contains the name of the PHY device, the name of the
>>>>>> +device which the PHY will be bind to and Connection ID string. Alternatively a
>>>>>> +lookup can be made during runtime when a handle to the struct phy already
>>>>>> +exists.
>>>>>> +
>>>>>> +The framework offers the following APIs for registering and unregistering the
>>>>>> +lookups.
>>>>>> +
>>>>>> +void phy_register_lookup(struct phy_lookup *pl);
>>>>>> +int phy_create_lookup(struct phy *phy, const char *con_id, const char *dev_id);
>>>>>> +
>>>>>> +void phy_unregister_lookup(struct phy_lookup *pl);
>>>>>> +void phy_remove_lookup(struct phy *phy, const char *con_id, const char *dev_id);
>>>>>>
>>>>>> 9. DeviceTree Binding
>>>>>>
>>>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>>>> index ff5eec5..c8d0f66 100644
>>>>>> --- a/drivers/phy/phy-core.c
>>>>>> +++ b/drivers/phy/phy-core.c
>>>>>> @@ -26,6 +26,7 @@
>>>>>> static struct class *phy_class;
>>>>>> static DEFINE_MUTEX(phy_provider_mutex);
>>>>>> static LIST_HEAD(phy_provider_list);
>>>>>> +static LIST_HEAD(phys);
>>>>>> static DEFINE_IDA(phy_ida);
>>>>>>
>>>>>> static void devm_phy_release(struct device *dev, void *res)
>>>>>> @@ -84,6 +85,138 @@ static struct phy *phy_lookup(struct device *device, const char *port)
>>>>>> return ERR_PTR(-ENODEV);
>>>>>> }
>>>>>>
>>>>>> +/**
>>>>>> + * phy_register_lookup() - register PHY/device association
>>>>>> + * @pl: association to register
>>>>>> + */
>>>>>> +void phy_register_lookup(struct phy_lookup *pl)
>>>>>> +{
>>>>>> + mutex_lock(&phy_provider_mutex);
>>>>>> + list_add_tail(&pl->node, &phys);
>>>>>> + mutex_unlock(&phy_provider_mutex);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * phy_unregister_lookup() - remove PHY/device association
>>>>>> + * @pl: association to be removed
>>>>>> + */
>>>>>> +void phy_unregister_lookup(struct phy_lookup *pl)
>>>>>> +{
>>>>>> + mutex_lock(&phy_provider_mutex);
>>>>>> + list_del(&pl->node);
>>>>>> + mutex_unlock(&phy_provider_mutex);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * phy_create_lookup() - allocate and register PHY/device association
>>>>>> + * @phy: the phy of the association
>>>>>> + * @con_id: connection ID string on device
>>>>>> + * @dev_id: the device of the association
>>>>>> + *
>>>>>> + * Creates and registers phy_lookup entry.
>>>>>> + */
>>>>>> +int phy_create_lookup(struct phy *phy, const char *con_id, const char *dev_id)
>>>>>> +{
>>>>>> + struct phy_lookup *pl;
>>>>>> +
>>>>>> + if (!phy || (!dev_id && !con_id))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + pl = kzalloc(sizeof(*pl), GFP_KERNEL);
>>>>>> + if (!pl)
>>>>>> + return -ENOMEM;
>>>>>> +
>>>>>> + pl->phy_name = dev_name(phy->dev.parent);
>>>>>> + pl->dev_id = dev_id;
>>>>>> + pl->con_id = con_id;
>>>>>> +
>>>>>> + phy_register_lookup(pl);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(phy_create_lookup);
>>>>>> +
>>>>>> +/**
>>>>>> + * phy_remove_lookup() - find and remove PHY/device association
>>>>>> + * @phy: the phy of the association
>>>>>> + * @con_id: connection ID string on device
>>>>>> + * @dev_id: the device of the association
>>>>>> + *
>>>>>> + * Finds and unregisters phy_lookup entry that was created with
>>>>>> + * phy_create_lookup().
>>>>>> + */
>>>>>> +void phy_remove_lookup(struct phy *phy, const char *con_id, const char *dev_id)
>>>>>> +{
>>>>>> + struct phy_lookup *pl;
>>>>>> +
>>>>>> + if (!phy || (!dev_id && !con_id))
>>>>>> + return;
>>>>>> +
>>>>>> + list_for_each_entry(pl, &phys, node)
>>>>>> + if (!strcmp(pl->phy_name, dev_name(phy->dev.parent)) &&
>>>>>> + !strcmp(pl->dev_id, dev_id) &&
>>>>>> + !strcmp(pl->con_id, con_id)) {
>>>>>> + phy_unregister_lookup(pl);
>>>>>> + kfree(pl);
>>>>>> + return;
>>>>>> + }
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(phy_remove_lookup);
>>>>>> +
>>>>>> +static struct phy *phy_find(struct device *dev, const char *con_id)
>>>>>> +{
>>>>>> + const char *dev_id = dev ? dev_name(dev) : NULL;
>>>>>> + int match, best_found = 0, best_possible = 0;
>>>>>> + struct phy *phy = ERR_PTR(-ENODEV);
>>>>>> + struct phy_lookup *p, *pl = NULL;
>>>>>> +
>>>>>> + if (dev_id)
>>>>>> + best_possible += 2;
>>>>>> + if (con_id)
>>>>>> + best_possible += 1;
>>>>>> +
>>>>>> + list_for_each_entry(p, &phys, node) {
>>>>>> + match = 0;
>>>>>> + if (p->dev_id) {
>>>>>> + if (!dev_id || strcmp(p->dev_id, dev_id))
>>>>>> + continue;
>>>>>> + match += 2;
>>>>>> + }
>>>>>> + if (p->con_id) {
>>>>>> + if (!con_id || strcmp(p->con_id, con_id))
>>>>>> + continue;
>>>>>> + match += 1;
>>>>>> + }
>>>>>> +
>>>>>> + if (match > best_found) {
>>>>>> + pl = p;
>>>>>> + if (match != best_possible)
>>>>>> + best_found = match;
>>>>>> + else
>>>>>> + break;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + if (pl) {
>>>>>> + struct class_dev_iter iter;
>>>>>> + struct device *phy_dev;
>>>>>> +
>>>>>> + class_dev_iter_init(&iter, phy_class, NULL, NULL);
>>>>>> + while ((phy_dev = class_dev_iter_next(&iter))) {
>>>>>
>>>>> We have the case with phy-exynos5-usbdrd driver, which registers two
>>>>> phys usb2-phy and usb3-phy
>>>>> both being used by xhci (after dwc3 creates a lookup table).
>>>>>
>>>>> The phy_dev is coming same for both the PHYs, and that's the reason
>>>>> when i try to get
>>>>> "usb2-phy" and "usb3-phy" i end up getting only usb2-phy.
>>>>> This is happening with V4 of this patch; V3 seems fine. The only
>>>>> differnece i see is
>>>>> we are creating the phy_lookup_table using phy_dev->parent.
>>>>>
>>>>> Is there something that i am missing ?
>>>>
>>>> looks like a genuine problem.
>>>>>
>>>>>> + if (!strcmp(dev_name(phy_dev->parent), pl->phy_name)) {
>>>>
>>>> here there are two phys which has the same parent and the first one that
>>>> matches will be returned. Hence you always get "usb2-phy".
>>>>
>>>> IIUC just with device names of parent, we won't be able to get the PHY. We need
>>>> another 'variable' to differentiate it's children.
>>>
>>>> Or have *phy* pointer directly in the lookup table like how clk driver does?
>>>
>>> We do create the lookup table with actual *phy* pointer isn't it ?
>>> Like if you see Heikki's last patch in this series:
>>> [PATCHv4 6/6] usb: dwc3: host: convey the PHYs to xhci
>>> We do pass the usb2_phy and usb3_phy pointers to phy_create_lookup().
>>
>> right, but phy_create_lookup doesn't store the *phy* pointer in the lookup table.
>
> Right, so i think that's the place where we should rework to get the
> correct phy.

How about adding the change in attached patch [1] on top of this patch.
Just introduced the phy pointer in "phy_lookup" structure, and
modified phy_find() accordingly.

[1] Attachment:
0001-phy-core-Use-phy-pointer-with-phy_lookup_table.patch



--
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India
From 848e691cf36a039f7a7cefac11df49230c71e806 Mon Sep 17 00:00:00 2001
From: Vivek Gautam <gautam.vivek@xxxxxxxxxxx>
Date: Thu, 13 Nov 2014 18:33:51 +0530
Subject: [PATCH RFC] phy: core: Use phy pointer with phy_lookup_table

Signed-off-by: Vivek Gautam <gautam.vivek@xxxxxxxxxxx>
---

drivers/phy/phy-core.c | 31 +++++++++----------------------
include/linux/phy/phy.h | 1 +
2 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 0b279d3..a4a67e2 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -99,6 +99,7 @@ int phy_create_lookup(struct phy *phy, const char *con_id, const char *dev_id)
pl->phy_name = dev_name(phy->dev.parent);
pl->dev_id = dev_id;
pl->con_id = con_id;
+ pl->phy = phy;

phy_register_lookup(pl);

@@ -167,18 +168,10 @@ static struct phy *phy_find(struct device *dev, const char *con_id)
}
}

- if (pl) {
- struct class_dev_iter iter;
- struct device *phy_dev;
-
- class_dev_iter_init(&iter, phy_class, NULL, NULL);
- while ((phy_dev = class_dev_iter_next(&iter))) {
- if (!strcmp(dev_name(phy_dev->parent), pl->phy_name)) {
- phy = to_phy(phy_dev);
- break;
- }
- }
- class_dev_iter_exit(&iter);
+ if (pl && pl->phy) {
+ phy = pl->phy;
+ if (!try_module_get(phy->ops->owner))
+ phy = ERR_PTR(-EPROBE_DEFER);
}
return phy;
}
@@ -549,7 +542,6 @@ EXPORT_SYMBOL_GPL(of_phy_simple_xlate);
*/
struct phy *phy_get(struct device *dev, const char *string)
{
- int index = 0;
struct phy *phy;

if (string == NULL) {
@@ -557,19 +549,14 @@ struct phy *phy_get(struct device *dev, const char *string)
return ERR_PTR(-EINVAL);
}

- if (dev->of_node) {
- index = of_property_match_string(dev->of_node, "phy-names",
- string);
- phy = _of_phy_get(dev->of_node, index);
- } else {
+ if (dev->of_node)
+ phy = of_phy_get(dev->of_node, string);
+ else
phy = phy_find(dev, string);
- }
+
if (IS_ERR(phy))
return phy;

- if (!try_module_get(phy->ops->owner))
- return ERR_PTR(-EPROBE_DEFER);
-
get_device(&phy->dev);

return phy;
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index d983051..4b21d72 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -88,6 +88,7 @@ struct phy_lookup {
const char *phy_name;
const char *dev_id;
const char *con_id;
+ struct phy *phy;
};

#define PHY_LOOKUP(a, b, c) \
--
1.7.10.4