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

[Simon Glass]

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?

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

There is a problem with adopting that approach in general - e.g.
*which* UART you write to has a big effect on whether the user sees
any output.

>
> (With the exception of USB port 0 being device-capable, but that should
> be configured by an explicit property, not based on aliases or anything
> like that)

Yes it is - in fact I added that property.

Regards,
Simon

>
> --
> nvpublic