[PATCH 1/2] ARM: tegra: Define Tegra20 CAR binding

[Simon Glass]

Hi Stephen,

On Wed, Jan 25, 2012 at 12:31 PM, Stephen Warren <swarren at nvidia.com> wrote:
> Simon Glass wrote at Wednesday, January 25, 2012 1:14 PM:
>> On Wed, Jan 25, 2012 at 11:57 AM, Stephen Warren <swarren at nvidia.com> wrote:
>> > Document a binding for the Tegra20 CAR (Clock And Reset) Controller,
>> > add it to tegra20.dtsi, and configure it for the board in tegra-
>> > seaboard.dts.
> ...
>> > v2:
> ...
>> > * Resolve FIXME re: multiple clocks with the same "reset ID"; give each
>> >  unique clock an ID, and ignore the reset bits, since this is purely a
>> >  clock binding. Code (e.g. U-Boot) that wants to use this to determine
>> >  CAR reset bit numbers would need a table to convert from the clock IDs
>> >  in this binding to the related reset bit number, if any. In general, I
>> >  think that's true, and the U-Boot code that handles "peripheral IDs"
>> >  should be reworked to handle "clocks", the peripheral clocks being a
>> >  subset of all clocks.
>>
>> The clock enable and reset enable bits use the same numbering. I think
>> you have invented a third which is an arbitrary number which doesn't
>> correspond to anything in hardware. That makes sense for pin mux
>> function perhaps, since the indirection provides a useful concept of
>> function number, but here I can't see the benefit.
>
> I didn't do it because there was specific benefit, but simply because
> we have no choice.

I was quite happy with your original proposal which made them line up
where they could, and used other numbers where they couldn't.

>
> There are clocks that don't have reset bits or clock enable bits.
>
> There are some clocks that are really different clocks (different mux
> selection or divider control registers) yet share the same bit for reset
> and clock enable.
>
> Therefore it simply isn't true that there's a 1:1 mapping between clocks
> and clock-enable/reset bits.
>
> I deliberately made this updated binding have a different numbering
> scheme to the clock-enable/reset bits to make this clear, so that no one
> would accidentally confuse the two concepts.

I think it makes sense to line them up where you can (all but two
cases out of about 60 I think).

>
>> > * Define clock IDs for all the non-peripheral clocks too; inputs, PLLs,
>> >  etc.
>> > * Separate tegra-seaboard.dts usage example into a separate patch.
>> >
>> > This patch semantically relies on Grant Likely's common clock binding patch
>> > series. Once that's finalized, this patch could be checked into the kernel
>> > provided there are no relevant changes to Grant's patches. I believe that
>> > Simon Glass wants to start using this within U-Boot ASAP though.
>>
>> As I may have said I am really not keen on the idea of having a table
>> just to use it in U-Boot.
>
> I don't see any alternative given the way the HW is designed.

You had the alternative yourself the first time around. There clearly
is an alternative.

>
> We could ignore the way the HW works and assume that clock ID == clock-
> enable/reset bits is true for many clocks. However, it's not true for
> all, so I think that'd be too error-prone.
>
> Equally, I know that you will need a table sometime in U-Boot to map
> from clock ID to clock-enable/reset bit and many other per-clock
> parameters if you're going to be initializing stuff in U-Boot from DT
> rather than hard-coding it, so I think you may as well just add it now.

I am happy that there is now a concept of a peripheral number and we
don't have to refer to everything with strings. I don't mind if for
hardware reasons this peripheral number doesn't always correspond to
everything we point it at (clock, reset, pinmux, clock source). But I
am uncomfortable with it corresponding to nothing because it might be
'error-prone'. This seems to be introducing a layer of indirection
which is not needed at all in U-Boot, for example.

I prefer a clock number which corresponds to the clock enable/reset
bit positions in all cases it can (all but two) and a different number
where it can't. At least this saves one table. Alternatively perhaps
these bit numbers should be specified in the device tree also? I was
rather hoping that the device tree would take us away from having lots
of tables in the C code.

>
>> > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> ...
>> > +  The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB
>> > +  registers. Later, subsequent IDs will be assigned to all other clocks the
>> > +  CAR controls; mainly the PLLs.
>>
>> Are you sure? The ordering doesn't seem to fit with U-Boot's enum
>> periph_id in arch/arm/include/asm/arch-tegra2/clock.h. That file
>> follows along with the hardware.
>
> No, that paragraph is wrong. I simply forgot to remove it.

Well I vote for a return :-)

>
> --
> nvpublic
>

Regards,
Simon