[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 2:15 PM, Stephen Warren <swarren at nvidia.com> wrote:
> On 12/05/2011 02:46 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Dec 5, 2011 at 1:33 PM, Stephen Warren <swarren at nvidia.com> wrote:
>>> On 12/02/2011 05:59 PM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> Here are my comments on the rest of your email.
>>>>
>>>> On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren at nvidia.com> wrote:
>>>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>>>> This adds basic support for the Tegra2 USB controller. Board files should
>>>>>> call board_usb_init() to set things up.
>>>
>>>>>> +       config->enabled = fdtdec_get_is_enabled(blob, node);
>>>>>> +       config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>>>>>
>>>>> periph-id is a U-Boot specific concept, not HW description. The DT
>>>>> shouldn't contain that value.
>>>>
>>>> See my previous comments as to why this is desirable. We can change
>>>> over to a clock-based approach once the kernel implements it.
>>>
>>> That will cause backwards-compatibility problems; older FDTs won't work
>>> with newer U-Boots. We should avoid incompatible changes like this if at
>>> all possible.
>>
>> At the moment the fdts are in the U-Boot tree, so we should be able to
>> change them at the same time. But of course when we update the fdt  we
>> need to update the U-Boot code. Is there any alternative other than
>> doing nothing until the kernel decides everything?
>
> You can choose not to represent these parameters in the device tree at
> all, but rather hard-code the values in the driver. This is what the
> kernel currently does; as luck would have it, the required values are
> identical for all the boards the kernel currently supports. Once that's
> all in place and working, we can work on defining a binding for those
> parameters and review/implement it in both U-Boot and the kernel.

OK, but how about we have this conversation when things are actually
in the kernel and working? The scheme used in this series (peripheral
IDs) is very handy for U-Boot and avoids this hard-coding that you
refer to.

Also the only way I can see it being hard-coded is by the kernel
looking at the peripheral address and converting this to an ID. That
seems really ugly to me. Or am I missing something?

>
>>>>>> +int board_usb_init(const void *blob)
>>>>>> +{
>>>>>> +#ifdef CONFIG_OF_CONTROL
>>>>>> +       struct fdt_usb config;
>>>>>> +       int clk_done = 0;
>>>>>> +       int node, upto = 0;
>>>>>> +       unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>>>>>> +
>>>>>> +       do {
>>>>>> +               node = fdtdec_next_alias(blob, "usb",
>>>>>> +                                        COMPAT_NVIDIA_TEGRA20_USB, &upto);
>>>>>
>>>>> Why only initialize USB controllers with aliases? Surely this should
>>>>> enumerate all nodes with a specific compatible flag?
>>>>
>>>> See my other comments - we want to know that port 0 is USB3 on Seaboard.
>>>
>>> Why does U-Boot care? Everything should be enumerating which peripherals
>>> happen to appear on the various USB busses, and not care which host
>>> controller they're attached to.
>>
>> At present we do:
>>
>> 'usb start 0'
>> 'usb start 1'
>>
>> to select between the ports. There is a patch in the works to change
>> that but it hasn't gone upstream yet, or at least wasn't accepted.
>
> Can you point out the patch that changes this, and what exactly it
> changes. Hopefully it removes the parameter from the "usb start" command.

Yes, they are in the Chromium tree:

https://gerrit.chromium.org/gerrit/#change,4951
https://gerrit.chromium.org/gerrit/#change,4981

>
> I'm still not convinced why U-Boot cares about this (as opposed to the
> user using U-Boot).

Well if U-Boot presents the ports in the wrong order, the user's
scripts will fail.

Also, what about the console UART problem? Surely the kernel provides
a way to select the ordering of those? How do I know what UART I am
getting on /dev/ttyS0?

Regards,
Simon

>
> --
> nvpublic