Re: [PATCHv4 2/6] phy: improved lookup method
From: Vivek Gautam
Date: Tue Nov 11 2014 - 03:54:04 EST
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.
--
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India
--
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/