[U-Boot] [PATCH 11/14] usb: UniPhier: add UniPhier on-chip xHCI host driver support

[Simon Glass]

Hi Masahiro,

On 23 February 2015 at 21:54, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> Hi Simon,
>
>
> On Fri, 20 Feb 2015 10:21:27 -0700
> Simon Glass <sjg at chromium.org> wrote:
>
>> Hi Masahiro,
>>
>> On 20 February 2015 at 05:12, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
>> > Hi Simon,
>> >
>> >
>> >
>> > On Tue, 17 Feb 2015 22:01:53 -0700
>> > Simon Glass <sjg at chromium.org> wrote:
>> >
>> >> > +#include <common.h>
>> >> > +#include <linux/err.h>
>> >> > +#include <usb.h>
>> >> > +#include <fdtdec.h>
>> >> > +#include "xhci.h"
>> >> > +
>> >> > +DECLARE_GLOBAL_DATA_PTR;
>> >> > +
>> >> > +#define FDT            gd->fdt_blob
>> >>
>> >> Ick, please don't do this. Just use a local variable if you like.
>> >>
>> >> > +#define COMPAT         "panasonic,uniphier-xhci"
>> >>
>> >> This too.
>> >
>> > Why?
>> > Each of them appears twice in the code.
>>
>> In the first case you could just a local variable as is done elsewhere
>> in the code:
>>
>> const void *blob = gd->fdt_blob;
>>
>> Then use 'blob'.
>
> Sounds reasonable.
>
>
>> In the second case, you could easily restructure your loop to remove
>> the duplication. But really you should use the alias function in
>> fdtdec until you convert to driver model.
>>
>> >
>> > Also, if I directly write the strings here, the line exceeds 80 columns
>> > and the code gets unreadable for line-wrapping.
>> >
>> >
>> > Anyway, this is not a big deal.
>> > You have already posted DM USB patch series.
>> > Once it is merged into the mainline, I will not have to scan a device tree
>> > in my own driver.
>>
>> Even now you don't need to do that, as above.
>
> OK.
>
> I am still not convinced with one thing of the fdtdec_find_aliases_for_id().
>
> I think it could take the compatibility string directly like this:
>
>    node_offset =  fdtdec_find_aliases_for_id(gd->fdt_blob, "usb", "panasonic,uniphier-xhci", index);
>
>
> Anyway, that is OK if it is temporal.

Yes it could - the current approach is really just to mark what needs
conversion.

>
> I used this helper function in v4.
> http://patchwork.ozlabs.org/patch/442791/
>
> I hope DM USB is supported in the mainline soon.

I think it will be. I posted a series a few weeks back and Vivek
posted a different one. I'll ping him to see if he's had a look.

>
>
>
>> >
>> >
>> >> > +
>> >> > +static int get_uniphier_xhci_base(int index, struct xhci_hccr **base)
>> >> > +{
>> >> > +       int offset;
>> >> > +
>> >> > +       for (offset = fdt_node_offset_by_compatible(FDT, 0, COMPAT);
>> >> > +            offset >= 0;
>> >> > +            offset = fdt_node_offset_by_compatible(FDT, offset, COMPAT)) {
>> >> > +               if (index == 0) {
>> >> > +                       *base = (struct xhci_hccr *)
>> >> > +                                       fdtdec_get_addr(FDT, offset, "reg");
>> >> > +                       return 0;
>> >> > +               }
>> >> > +               index--;
>> >> > +       }
>> >>
>> >> How about this"
>> >>
>> >> count = fdtdec_find_aliasses_for_id(gd->fdt_blob, "usb",
>> >> COMPAT_PANASONIC_XHCI, node_list, 4);
>> >> if (index >= count)
>> >>    return -ENOENT;
>> >> offset = node_list[index];
>> >>
>> >> then aliases will work. You need to ad the COMPAT to
>> >> include/fdtdec.h/c.  See ehci-tegra.c for an example.
>> >
>> >
>> > I do not like this idea.
>> >
>> >
>> > fdt_compat_id is one of what I think should be removed asap because:
>> >
>> >   - The two separete tables,  enum fdt_compat_id  and compat_names[],
>> >     must be maintained in sync.  If someone modifies just one of them,
>> >     it gets broken.  Anybody who checks those two tables are arranged
>> >      in the same order?
>> >
>> >   - It seems ridiculous to collect many devices from various platforms
>> >      into the common place.
>> >     If everyone starts to follow this way, the table grows too big soon.
>>
>> The purpose is that we then know which drivers are directly looking at
>> the device tree. As we add driver model support, we remove these
>> strings. When the table is empty, we can remove all that support from
>> fdtdec, including aliases.
>>
>
> OK.  If you assume this tabile is like a TODO list, it makes sense.

Yes, we should remove it. I am chipping away at it - this morning I
sent a series that removed two fo them.

>
>
>
>
>> >
>> > Moreover, I am not happy about adding aliases to my device tree again.
>> >
>> > Rather, I am searching for an idea to avoid adding an alias to every device, like this.
>> > http://patchwork.ozlabs.org/patch/441922/
>>
>> This is up to you.
>>
>> Bu why? It allows people to make adjustments if needed. The numbering
>> of devices is an important thing in U-Boot - most commands use this
>> although I would like one day to allow devices to be named (just use
>> dev->name with device tree).
>>
>> Anyway this is up to you.
>
> I read your explanation and I am convinced.
>
> Let's do not apply this series.

OK.

Regards,
Simon