Re: [PATCH 1/5 v13] arm: omap: usb: ehci and ohci hwmod structures for omap4

["Munegowda, Keshava" <keshava_mgowda@ti.com>]

On Mon, Oct 10, 2011 at 3:35 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Mon, Oct 10, 2011 at 12:26:15PM +0300, Felipe Balbi wrote:
>> On Mon, Oct 10, 2011 at 02:47:42PM +0530, Munegowda, Keshava wrote:
>> > On Mon, Oct 10, 2011 at 2:33 PM, Felipe Balbi <balbi@ti.com> wrote:
>> > > Hi,
>> > >
>> > > On Mon, Oct 10, 2011 at 02:22:23PM +0530, Munegowda, Keshava wrote:
>> > >> Hi paul and Felipe
>> > >>
>> > >> Here is the highlights of the change in the design of  USB Host which
>> > >> we can do after kernel 3.2 release;
>> > >>
>> > >> 1. separate the TLL changes  from UHH
>> > >> 2. The TLL is be a new platform driver in ./drivers/mfd
>> > >> 3. the TLL platform driver will export apis  for enable/disable clocks
>> > >> and settings.
>> > >
>> > > TLL should control its clocks through pm_runtime APIs like anything
>> > > else. If you really must export APIs to be used by UHH, you need to have
>> > > an API so that you can claim/release a TLL channel and get/put for
>> > > increasing/decreasing PM counters.
>> > >
>> > > I still think though, you should try to avoid exporting OMAP-specific
>> > > APIs all over the place. Ideally, we would be able to have some way of
>> > > saying that UHH and TLL are closely related... something like having the
>> > > ability to say e.g. two devices are sibblings of each other, so that we
>> > > could ask for a sibbling to wakeup/sleep depending if we need it or not.
>> >
>> > do we have sibling structures today? I dont think so.
>>
>> no we don't.
>
> Ok, here's a first shot at it:
>
> From 600ae62f4b4a832d90a83d43227deb6f84b9def1 Mon Sep 17 00:00:00 2001
> From: Felipe Balbi <balbi@ti.com>
> Date: Mon, 10 Oct 2011 12:56:34 +0300
> Subject: [RFC/NOT-FOR-MERGING/PATCH] base: add the idea of sibling devices
> Organization: Texas Instruments\n
>
> It's possible that some devices, which share a
> common parent, need to talk to each due to a
> very close relationship between them.
>
> Generally, one device will provide some sort of
> resources to the other (e.g. clocks) while still
> both sharing another common parent.
>
> In such cases, it seems necessary to define those
> two devices as siblings, rather than a virtual
> parent relationship, and have means for one device
> to ask the sibling device to e.g. turn on its clocks.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/base/core.c    |   41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |    7 +++++++
>  2 files changed, 48 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index bc8729d..3b7f2e5 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -589,6 +589,7 @@ void device_initialize(struct device *dev)
>        dev->kobj.kset = devices_kset;
>        kobject_init(&dev->kobj, &device_ktype);
>        INIT_LIST_HEAD(&dev->dma_pools);
> +       INIT_LIST_HEAD(&dev->siblings);
>        mutex_init(&dev->mutex);
>        lockdep_set_novalidate_class(&dev->mutex);
>        spin_lock_init(&dev->devres_lock);
> @@ -889,6 +890,7 @@ int device_private_init(struct device *dev)
>  */
>  int device_add(struct device *dev)
>  {
> +       struct device *sibling, *n;
>        struct device *parent = NULL;
>        struct class_interface *class_intf;
>        int error = -EINVAL;
> @@ -923,6 +925,10 @@ int device_add(struct device *dev)
>        parent = get_device(dev->parent);
>        setup_parent(dev, parent);
>
> +       /* setup siblings */
> +       list_for_each_entry_safe(sibling, n, &dev->siblings, sibling_node)
> +               get_device(sibling);
> +
>        /* use parent numa_node */
>        if (parent)
>                set_dev_node(dev, dev_to_node(parent));
> @@ -1071,6 +1077,31 @@ void put_device(struct device *dev)
>  }
>
>  /**
> + * get_sibling_device_byname - finds a sibling device by its name
> + * @dev: device.
> + * @name: name for the sibling to find.
> + *
> + * This is here to help drivers which need to ask their siblings
> + * for something in particular (like ask sibling to turn clocks on)
> + * achieve that by first finding the correct device pointer for
> + * that sibling.
> + */
> +struct device *get_sibling_device_byname(struct device *dev, const char *name)
> +{
> +       struct device   *sibling, *n;
> +       struct device   *found = NULL;
> +
> +       list_for_each_entry_safe(sibling, n, &dev->siblings, sibling_node) {
> +               if (!strcmp(dev_name(sibling), name))
> +                       found = sibling;
> +                       goto found;
> +       }
> +
> +found:
> +       return found;
> +}
> +
> +/**
>  * device_del - delete device from system.
>  * @dev: device.
>  *
> @@ -1085,6 +1116,7 @@ void put_device(struct device *dev)
>  */
>  void device_del(struct device *dev)
>  {
> +       struct device *sibling, *n;
>        struct device *parent = dev->parent;
>        struct class_interface *class_intf;
>
> @@ -1120,6 +1152,15 @@ void device_del(struct device *dev)
>        device_remove_attrs(dev);
>        bus_remove_device(dev);
>
> +       /* teardown siblings */
> +       list_for_each_entry_safe(sibling, n, &dev->siblings, sibling_node) {
> +               /* siblings must have  the same parent */
> +               WARN(sibling->parent != parent,
> +                               "siblings must have a common parent\n");
> +
> +               get_device(sibling);
> +       }
> +
>        /*
>         * Some platform devices are driven without driver attached
>         * and managed resources may have been acquired.  Make sure
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c20dfbf..ae9cec9 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -551,6 +551,9 @@ struct device_dma_parameters {
>  struct device {
>        struct device           *parent;
>
> +       struct list_head        sibling_node;
> +       struct list_head        siblings;
> +
>        struct device_private   *p;
>
>        struct kobject kobj;
> @@ -764,6 +767,10 @@ extern int (*platform_notify_remove)(struct device *dev);
>  extern struct device *get_device(struct device *dev);
>  extern void put_device(struct device *dev);
>
> +/* finds a sibling struct device pointer */
> +extern struct device *get_sibling_device_byname(struct device *dev,
> +               const char *name);
> +
>  extern void wait_for_device_probe(void);
>
>  #ifdef CONFIG_DEVTMPFS
> --
> 1.7.6.396.ge0613
>
> one way to use this would be to mark both hwmods has having the same
> parent and pointing to each other as siblings. Then, from UHH you could:
>
> if (port->mode == TLL) {
>        tll = get_sibling_device_by_name(dev, "usbtll");
>        if (!tll)
>                error();
>
>        pm_runtime_get_sync(tll);
> }
>
> or something similar. As of now, I'm not so sure this is a very good
> idea, but decided to gather some comments anyway.
>
> Does anybody see any change of this getting used in more cases ? Greg,
> I'm adding you to this thread, if you have any comments to this, I'd
> like to hear them.


This design looks good and perfectly suits our requirements.

Paul/Others
          please provide your thoughts..

regards
keshava