Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1386153 > unrolled thread

Re: [PATCH v8] drivers: amba: properly handle devices with power domains

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8] drivers: amba: properly handle devices with power domains Ulf Hansson <ulf.hansson@linaro.org> - 2016-04-25 11:20 +0200
    Re: [PATCH v8] drivers: amba: properly handle devices with power  domains Marek Szyprowski <m.szyprowski@samsung.com> - 2016-04-26 12:40 +0200

#1386153 — Re: [PATCH v8] drivers: amba: properly handle devices with power domains

On 15 April 2016 at 11:13, Marek Szyprowski <m.szyprowski@samsung.com> 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 <m.szyprowski@samsung.com>
> ---
> 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.

Kind regards
Uffe

[toc] | [next] | [standalone]

#1387343 — Re: [PATCH v8] drivers: amba: properly handle devices with power domains

Hello,


On 2016-04-25 11:19, Ulf Hansson wrote:
> On 15 April 2016 at 11:13, Marek Szyprowski <m.szyprowski@samsung.com> 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 <m.szyprowski@samsung.com>
>> ---
>> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web