Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752262AbaKKIhN (ORCPT ); Tue, 11 Nov 2014 03:37:13 -0500 Received: from mail-qc0-f171.google.com ([209.85.216.171]:35221 "EHLO mail-qc0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751705AbaKKIhK (ORCPT ); Tue, 11 Nov 2014 03:37:10 -0500 MIME-Version: 1.0 In-Reply-To: <5461AFD3.5020302@ti.com> References: <1413556756-5050-1-git-send-email-heikki.krogerus@linux.intel.com> <1413556756-5050-3-git-send-email-heikki.krogerus@linux.intel.com> <5461AFD3.5020302@ti.com> Date: Tue, 11 Nov 2014 14:07:08 +0530 Message-ID: Subject: Re: [PATCHv4 2/6] phy: improved lookup method From: Vivek Gautam To: Kishon Vijay Abraham I Cc: Vivek Gautam , Heikki Krogerus , "linux-kernel@vger.kernel.org" , Linux USB Mailing List , andrew.kim@intel.com Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 11, 2014 at 12:12 PM, Kishon Vijay Abraham I 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 >> 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 >>> Tested-by: Vivek Gautam >>> --- >>> 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(). But while finding the phy (using phy_find()) we don't seem to be utilizing these. > > Thanks > Kishon >>> + phy = to_phy(phy_dev); >>> + break; >>> + } >>> + } >>> + class_dev_iter_exit(&iter); >>> + } >>> + >>> + /* fall-back to the old lookup method for now */ >>> + if (IS_ERR(phy)) >>> + phy = phy_lookup(dev, con_id); >>> + return phy; >>> +} >>> + >>> static struct phy_provider *of_phy_provider_lookup(struct device_node *node) >>> { >>> struct phy_provider *phy_provider; >>> @@ -463,7 +596,7 @@ struct phy *phy_get(struct device *dev, const char *string) >>> string); >>> phy = _of_phy_get(dev->of_node, index); >>> } else { >>> - phy = phy_lookup(dev, string); >>> + phy = phy_find(dev, string); >>> } >>> if (IS_ERR(phy)) >>> return phy; >>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h >>> index 9fda683..2696b95 100644 >>> --- a/include/linux/phy/phy.h >>> +++ b/include/linux/phy/phy.h >>> @@ -110,6 +110,20 @@ struct phy_init_data { >>> .port = _port, \ >>> } >>> >>> +struct phy_lookup { >>> + struct list_head node; >>> + const char *phy_name; >>> + const char *dev_id; >>> + const char *con_id; >>> +}; >>> + >>> +#define PHY_LOOKUP(a, b, c) \ >>> + { \ >>> + .phy_name = a, \ >>> + .dev_id = b, \ >>> + .con_id = c, \ >>> + } >>> + >>> #define to_phy(a) (container_of((a), struct phy, dev)) >>> >>> #define of_phy_provider_register(dev, xlate) \ >>> @@ -174,6 +188,10 @@ struct phy_provider *__devm_of_phy_provider_register(struct device *dev, >>> void of_phy_provider_unregister(struct phy_provider *phy_provider); >>> void devm_of_phy_provider_unregister(struct device *dev, >>> struct phy_provider *phy_provider); >>> +void phy_register_lookup(struct phy_lookup *pl); >>> +void phy_unregister_lookup(struct phy_lookup *pl); >>> +int phy_create_lookup(struct phy *phy, const char *con_id, const char *dev_id); >>> +void phy_remove_lookup(struct phy *phy, const char *con_id, const char *dev_id); >>> #else >>> static inline int phy_pm_runtime_get(struct phy *phy) >>> { >>> @@ -345,6 +363,15 @@ static inline void devm_of_phy_provider_unregister(struct device *dev, >>> struct phy_provider *phy_provider) >>> { >>> } >>> +static inline void phy_register_lookup(struct phy_lookup *pl) { } >>> +static inline void phy_unregister_lookup(struct phy_lookup *pl) { } >>> +static inline int >>> +phy_create_lookup(struct phy *phy, const char *con_id, const char *dev_id) >>> +{ >>> + return 0; >>> +} >>> +static inline void phy_remove_lookup(struct phy *phy, const char *con_id, >>> + const char *dev_id) { } >>> #endif >>> >>> #endif /* __DRIVERS_PHY_H */ >>> -- >>> 2.1.1 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/