Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751873AbcDZKhq (ORCPT ); Tue, 26 Apr 2016 06:37:46 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:51832 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751082AbcDZKho (ORCPT ); Tue, 26 Apr 2016 06:37:44 -0400 X-AuditID: cbfec7f4-f796c6d000001486-4f-571f44f4a035 Subject: Re: [PATCH v8] drivers: amba: properly handle devices with power domains To: Ulf Hansson References: <1460711612-5268-1-git-send-email-m.szyprowski@samsung.com> Cc: linux-samsung-soc , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Russell King - ARM Linux , Greg Kroah-Hartman , Krzysztof Kozlowski , Bartlomiej Zolnierkiewicz From: Marek Szyprowski Message-id: Date: Tue, 26 Apr 2016 12:37:38 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrCLMWRmVeSWpSXmKPExsVy+t/xK7pfXOTDDc4+57DYOGM9q0Xz4vVs Fq9fGFpsenyN1eLyrjlsFjPO72OyuH2Z1+L42nAHDo+W5h42jzvX9rB57J+7ht1j85J6j74t qxg9Pm+SC2CL4rJJSc3JLEst0rdL4Mpom7+ctWC6c8XqZ9dYGxhPG3UxcnJICJhIvF53iBHC FpO4cG89WxcjF4eQwFJGiQ/fXrBDOM8ZJZ7dnMUOUiUsECyx5tstsA4RAQ2JPQ/Ps0IU9TFK LNyyghnEYRaYwyzx4ss6NpAqNgFDia63XWA2r4CdRMOJ5ywgNouAqsT+pcvA4qICURJPTq9l hqgRlPgx+R5YDSfQtsc/pjCB2MwCZhJfXh5mhbDlJTavecs8gVFgFpKWWUjKZiEpW8DIvIpR NLU0uaA4KT3XUK84Mbe4NC9dLzk/dxMjJOy/7GBcfMzqEKMAB6MSD6/CMrlwIdbEsuLK3EOM EhzMSiK8h5zlw4V4UxIrq1KL8uOLSnNSiw8xSnOwKInzzt31PkRIID2xJDU7NbUgtQgmy8TB KdXAmHBaIrZuonee5ZHWmSamjtN3REuo5++YqbQ84SWr1KWFUXExPKGbPTmObG40lxFujmlT MNRWmKW/fuG+qyJ6jzgEwh+2b+Zf9/6xVp3592umJXbdMrPuL9kbeaek2aqy+Hozf5qEs+yv CjOZI3V307vn9by/fuZkqGnuqtpPTsdqGQ+9P7JKiaU4I9FQi7moOBEAawZ9UHcCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8381 Lines: 216 Hello, On 2016-04-25 11:19, Ulf Hansson wrote: > On 15 April 2016 at 11:13, Marek Szyprowski wrote: >> To read pid/cid registers, the probed device need to be properly turned on. >> When it is inside a power domain, the bus code should ensure that the >> given power domain is enabled before trying to access device's registers. >> However in some cases power domain (or clocks) might not be yet available. >> Returning -EPROBE_DEFER is not a solution in such case, because callers >> don't handle this special error code. Instead such devices are added to the >> special list and their registration is retried from periodic worker until >> all resources are available. >> >> Signed-off-by: Marek Szyprowski >> --- >> Changelog: >> >> v8: >> - replaced notifier with periodic workqueue on Greg request >> >> v7: https://lkml.org/lkml/2016/4/13/201 >> - replaced late_initcall approach with a notifier registered to device core >> >> v6: https://lkml.org/lkml/2016/4/12/414 >> - got back to v1-style approach on Russell King request to avoid ABI break >> - use list for storing deferred devices and retry their registration from >> late_initcall >> >> v5: https://lkml.org/lkml/2016/2/10/179 >> - added 2 more patches to avoid regression with existing drivers (nvdimm and >> sa1111), for more information, see https://lkml.org/lkml/2015/12/17/390 >> - changed thread name to "AMBA: add complete support for power domains" >> >> v4: https://lkml.org/lkml/2015/12/2/52 >> - fixed more issues pointed by Ulf Hansson and Russell King >> >> v3: https://lkml.org/lkml/2015/12/1/334 >> - fixed issues pointed by Ulf Hansson >> - dropped patch for exynos4210 dts, because it already got queued for merging >> >> v2: https://lkml.org/lkml/2015/11/26/229 >> - added 2 patches from 'On-demand device probing' thread >> (https://lkml.org/lkml/2015/9/29/189), which move PID/CIR reading >> from amba_device_add() to amba_match() >> - moved dev_pm_domain_attach() to amba_match(), which is allowed to >> return -EPROBE_DEFER >> >> v1: http://www.spinics.net/lists/arm-kernel/msg463185.html >> - initial version >> --- >> drivers/amba/bus.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 90 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c >> index f0099360039e..a5b5c87e2114 100644 >> --- a/drivers/amba/bus.c >> +++ b/drivers/amba/bus.c >> @@ -336,16 +336,7 @@ static void amba_device_release(struct device *dev) >> kfree(d); >> } >> >> -/** >> - * amba_device_add - add a previously allocated AMBA device structure >> - * @dev: AMBA device allocated by amba_device_alloc >> - * @parent: resource parent for this devices resources >> - * >> - * Claim the resource, and read the device cell ID if not already >> - * initialized. Register the AMBA device with the Linux device >> - * manager. >> - */ >> -int amba_device_add(struct amba_device *dev, struct resource *parent) >> +static int amba_device_try_add(struct amba_device *dev, struct resource *parent) >> { >> u32 size; >> void __iomem *tmp; >> @@ -373,6 +364,12 @@ int amba_device_add(struct amba_device *dev, struct resource *parent) >> goto err_release; >> } >> >> + ret = dev_pm_domain_attach(&dev->dev, true); >> + if (ret == -EPROBE_DEFER) { >> + iounmap(tmp); >> + goto err_release; >> + } >> + >> ret = amba_get_enable_pclk(dev); >> if (ret == 0) { >> u32 pid, cid; >> @@ -398,6 +395,7 @@ int amba_device_add(struct amba_device *dev, struct resource *parent) >> } >> >> iounmap(tmp); >> + dev_pm_domain_detach(&dev->dev, true); >> >> if (ret) >> goto err_release; >> @@ -421,6 +419,88 @@ int amba_device_add(struct amba_device *dev, struct resource *parent) >> err_out: >> return ret; >> } >> + >> +/* >> + * Registration of AMBA device require reading its pid and cid registers. >> + * To do this, the device must be turned on (if it is a part of power domain) >> + * and have clocks enabled. However in some cases those resources might not be >> + * yet available. Returning EPROBE_DEFER is not a solution in such case, >> + * because callers don't handle this special error code. Instead such devices >> + * are added to the special list and their registration is retried from >> + * periodic worker, until all resources are available and registration succeeds. >> + */ >> +struct deferred_device { >> + struct amba_device *dev; >> + struct resource *parent; >> + struct list_head node; >> +}; >> + >> +static LIST_HEAD(deferred_devices); >> +static DEFINE_MUTEX(deferred_devices_lock); >> + >> +static void amba_deferred_retry_func(struct work_struct *dummy); >> +static DECLARE_DELAYED_WORK(deferred_retry_work, amba_deferred_retry_func); >> + >> +#define DEFERRED_DEVICE_TIMEOUT (msecs_to_jiffies(5 * 1000)) >> + >> +static void amba_deferred_retry_func(struct work_struct *dummy) >> +{ >> + struct deferred_device *ddev, *tmp; >> + >> + mutex_lock(&deferred_devices_lock); >> + >> + list_for_each_entry_safe(ddev, tmp, &deferred_devices, node) { >> + int ret = amba_device_try_add(ddev->dev, ddev->parent); >> + >> + if (ret == -EPROBE_DEFER) >> + continue; >> + >> + list_del_init(&ddev->node); >> + kfree(ddev); >> + } >> + >> + if (!list_empty(&deferred_devices)) >> + schedule_delayed_work(&deferred_retry_work, >> + DEFERRED_DEVICE_TIMEOUT); >> + >> + mutex_unlock(&deferred_devices_lock); >> +} >> + >> +/** >> + * amba_device_add - add a previously allocated AMBA device structure >> + * @dev: AMBA device allocated by amba_device_alloc >> + * @parent: resource parent for this devices resources >> + * >> + * Claim the resource, and read the device cell ID if not already >> + * initialized. Register the AMBA device with the Linux device >> + * manager. >> + */ >> +int amba_device_add(struct amba_device *dev, struct resource *parent) >> +{ >> + int ret = amba_device_try_add(dev, parent); >> + >> + if (ret == -EPROBE_DEFER) { >> + struct deferred_device *ddev; >> + >> + ddev = kmalloc(sizeof(*ddev), GFP_KERNEL); >> + if (!ddev) >> + return -ENOMEM; >> + >> + ddev->dev = dev; >> + ddev->parent = parent; >> + ret = 0; >> + >> + mutex_lock(&deferred_devices_lock); >> + >> + if (list_empty(&deferred_devices)) >> + schedule_delayed_work(&deferred_retry_work, >> + DEFERRED_DEVICE_TIMEOUT); >> + list_add_tail(&ddev->node, &deferred_devices); >> + >> + mutex_unlock(&deferred_devices_lock); >> + } >> + return ret; >> +} >> EXPORT_SYMBOL_GPL(amba_device_add); >> >> static struct amba_device * >> -- >> 1.9.2 >> > Overall this looks okay to me! Even if I don't really like the > approach, it seems like this is the only viable option there is right > now. > > One minor improvement that I could think of, is to use also a late > initcall in conjunction with the delayed work. So, instead of > scheduling a work unconditionally when -EPROBE_DEFER is returned from > amba_device_try_add(), you can postpone the *first* re-try to be done > from a late initcall. > > Of course, as devices may be added also after the late initcall, you > need to keep a flag which tells amba_device_add() whether it can rely > on the late initcall to re-try, or if it needs to schedule a work by > itself. I don't think that such micro-optimization makes sense in this case. Please note that there is still no platform and driver which require this deferred probe logic - in my case exynos power domain driver is registered very early, so dev_pm_domain_attach() calls succeeds in the standard amba device registration path. This patch is mainly to get the device registered to power domain to properly read its cid/pid pair. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland