[PATCH v2 09/17] tegra: usb: fdt: Add additional device tree definitions for USB ports

Simon Glass sjg at chromium.org
Tue Dec 6 11:55:59 EST 2011


Hi Stephen,

On Mon, Dec 5, 2011 at 3:25 PM, Stephen Warren <swarren at nvidia.com> wrote:
> On 12/02/2011 07:11 PM, Simon Glass wrote:
>> This adds peripheral IDs and timing information to the USB part of the
>> device tree for U-Boot.
>>
>> The peripheral IDs provide easy access to clock registers. We will likely
>> remove this in favor of a full clock tree when it is available in the
>> kernel (but probably still retain the peripheral ID, just move it into
>> a clock node).
>>
>> The USB timing information does apparently vary between boards sometimes,
>> so is include in the fdt for convenience.
>
> Which parts vary? Most of it is PLL configuration, and it seems
> basically impossible for that to vary yet still create the correct
> output frequency.

See here for T30 differences, for example.

https://gerrit.chromium.org/gerrit/#patch,sidebyside,12440,1,board/nvidia/cardhu/tegra30.dtsi

>
>> +/* This table has USB timing parameters for each Oscillator frequency we
>
> This comment block should be indented to match the nodes.
>
>> + * support. There are four sets of values:
>> + *
>> + * 1. PLLU configuration information (reference clock is osc/clk_m and
>> + * PLLU-FOs are fixed at 12MHz/60MHz/480MHz).
>> + *
>> + *  Reference frequency     13.0MHz      19.2MHz      12.0MHz      26.0MHz
>> + *  ----------------------------------------------------------------------
>> + *      DIVN                960 (0x3c0)  200 (0c8)    960 (3c0h)   960 (3c0)
>> + *      DIVM                13 (0d)      4 (04)       12 (0c)      26 (1a)
>> + * Filter frequency (MHz)   1            4.8          6            2
>> + * CPCON                    1100b        0011b        1100b        1100b
>> + * LFCON0                   0            0            0            0
>> + *
>> + * 2. PLL CONFIGURATION & PARAMETERS for different clock generators:
>> + *
>> + * Reference frequency     13.0MHz         19.2MHz         12.0MHz     26.0MHz
>> + * ---------------------------------------------------------------------------
>> + * PLLU_ENABLE_DLY_COUNT   02 (0x02)       03 (03)         02 (02)     04 (04)
>> + * PLLU_STABLE_COUNT       51 (33)         75 (4B)         47 (2F)    102 (66)
>> + * PLL_ACTIVE_DLY_COUNT    05 (05)         06 (06)         04 (04)     09 (09)
>> + * XTAL_FREQ_COUNT        127 (7F)        187 (BB)        118 (76)    254 (FE)
>
> Those two sets of data seem like they should simply be part of the clock
> driver. Some part of the system boot or USB init process needs to say
> "enable the USB clocks", and that code needs to write those hard-coded
> values into the relevant clock registers (or calculate them at run-time;
> whatever). This stuff can't vary by board.

We don't have a USB clock driver. This driver is in CPU code, so I am
not suggesting that it varies by board, only by SOC. This stuff is in
the SOC dts.
>
>> + * 3. Debounce values IdDig, Avalid, Bvalid, VbusValid, VbusWakeUp, and
>> + * SessEnd. Each of these signals have their own debouncer and for each of
>> + * those one out of two debouncing times can be chosen (BIAS_DEBOUNCE_A or
>> + * BIAS_DEBOUNCE_B).
>> + *
>> + * The values of DEBOUNCE_A and DEBOUNCE_B are calculated as follows:
>> + *    0xffff -> No debouncing at all
>> + *    <n> ms = <n> *1000 / (1/19.2MHz) / 4
>> + *
>> + * So to program a 1 ms debounce for BIAS_DEBOUNCE_A, we have:
>> + * BIAS_DEBOUNCE_A[15:0] = 1000 * 19.2 / 4  = 4800 = 0x12c0
>> + *
>> + * We need to use only DebounceA for BOOTROM. We don't need the DebounceB
>> + * values, so we can keep those to default.
>> + *
>> + * a4. The 20 microsecond delay after bias cell operation.
>
>      ^ typo
>
>> + *  Reference frequency     13.0MHz      19.2MHz      12.0MHz      26.0MHz
>
> If this data varies at all, then those two sets of data seem
> port-specific to me, and hence should be moved into a per-port property.
> Having a single global set of parameters that gets applied to all ports
> at once doesn't make sense to me, if the data varies.

The hardware doesn't support this does it?

>
> I imagine the values are board specific too, and hence shouldn't be in
> tegra20.dtsi but rather in tegra-seaboard.dts.

I believe they vary by SOC, not board.

>
> The values in these fields should be specific in a way that's agnostic
> to the reference clock, e.g. as a number of mS/nS. That way, you'd just
> specify the single set of values to use, and the driver would do the
> calculations to map the time domain into the reference-clock-specific
> values to put into the registers.

Yes I agree. I don't have time to write this. Shall we go forward with
what we have, and Nvidia can tidy up later?

>
>> + */
>> +
>> +     usbtiming at 0 {
>> +             compatible = "nvidia,tegra20-usbtiming";
>> +             osc-frequency = <13000000>;
>> +             /* DivN, DivM, DivP, CPCON, LFCON, Delays     Debounce, Bias */
>> +             timing = <0x3c0 0x0d 0x00 0xc 0  0x02 0x33 0x05 0x7f 0x7ef4 5>;
>> +     };
>> +
>> +     usbtiming at 1 {
>> +             compatible = "nvidia,tegra20-usbtiming";
>> +             osc-frequency = <19200000>;
>> +             timing = <0x0c8 0x04 0x00 0x3 0  0x03 0x4b 0x06 0xbb 0xbb80 7>;
>> +     };
>> +
>> +     usbtiming at 2 {
>> +             compatible = "nvidia,tegra20-usbtiming";
>> +             osc-frequency = <12000000>;
>> +             timing = <0x3c0 0x0c 0x00 0xc 0  0x02 0x2f 0x04 0x76 0x7530 5>;
>> +     };
>> +
>> +     usbtiming at 3 {
>> +             compatible = "nvidia,tegra20-usbtiming";
>> +             osc-frequency = <26000000>;
>> +             timing = <0x3c0 0x1a 0x00 0xc 0  0x04 0x66 0x09 0xfe 0xfde8 9>;
>> +     };
>
> You can't include those timing nodes right there, because the unit
> addresses (@0, ... @3) aren't physical addresses in the same scheme as
> all the other nodes.

Shall I put them inside some other 'timing' node? Bear in mind we
don't have clock stuff yet.

>
>> +
>>       usb at c5000000 {
>>               compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>               reg = <0xc5000000 0x4000>;
>>               interrupts = < 52 >;
>>               phy_type = "utmi";
>> +             periph-id = <22>;       // PERIPH_ID_USBD
>
> Given this is a temporary U-Boot-specific solution, can the property be
> named u-boot,periph-id so it's obvious that when writing a .dts for the
> kernel only, you don't care about this value.

ok. I suggest the kernel does something similar.

>
> Personally, I'd suggest not having this property at all, but rather have
> the driver maintain a base_address -> periph_id mapping table
> internally. That table can be removed once the clock/reset bindings are
> all set up, and the information can be obtained there instead.

I really want to avoid pushing SOC things into the drivers! The SOC
code provides clock/reset functions. It seems useful to have a way of
identifying all the peripherals in the SOC by number.

Regards,
Simon

>
> --
> nvpublic


More information about the devicetree-discuss mailing list