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

Hi Stephen,

On Wed, Jan 25, 2012 at 12:31 PM, Stephen Warren [off-list ref] wrote:

Simon Glass wrote at Wednesday, January 25, 2012 1:14 PM:

quoted

On Wed, Jan 25, 2012 at 11:57 AM, Stephen Warren [off-list ref] wrote:

quoted

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.

...

quoted

quoted

v2:

...

quoted

quoted

* 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).

quoted

quoted

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

quoted

quoted

diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt

...

quoted

quoted

+  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