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

[Stephen Warren]

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.

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

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

-- 
nvpublic