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

[Masahiro Yamada]

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.

I used this helper function in v4.
http://patchwork.ozlabs.org/patch/442791/

I hope DM USB is supported in the mainline soon.



> >
> >
> >> > +
> >> > +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.




> >
> > 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.



Best Regards
Masahiro Yamada