Hi Stephen,
On Wed, Jan 25, 2012 at 12:31 PM, Stephen Warren swarren@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@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