On 14/05/15 17:34, Maxime Coquelin wrote:

2015-05-13 21:37 GMT+02:00 Arnd Bergmann [off-list ref]:

quoted

On Wednesday 13 May 2015 20:29:12 Daniel Thompson wrote:

quoted

On 13/05/15 17:54, Maxime Coquelin wrote:

quoted

2015-05-13 18:37 GMT+02:00 Arnd Bergmann [off-list ref]:

quoted

We should definitely try to use the same compatible string for all of
them, and make a binding that is easy to use.

I haven't fully understood the requirements for the various parts that
are involved here. My understanding so far was that the driver could
use the index from the first cell and compute

          void __iomem *reset_reg = rcc_base + 0x10 + 4 * index;
          void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;

This calculation is true, but we have to take into account there is a
hole in the middle, between AHB3, and APB1 register:

... and equally importantly, only allows us to use hardware mappings for
the gated clocks.

quoted

AHB1RSTR : offset = 0x10, index = 0
AHB2RSTR : offset = 0x14, index = 1
AHB3RSTR : offset = 0x18, index = 2
<HOLE >     : offset = 0x1c, index = 3
APB1RSTR : offset = 0x20, index = 4
APB2RSTR : offset = 0x24, index = 5

So we have to carefully document this hole in the bindings, maybe by
listing indexes in the documentation?

The register set has PLL, mux and dividers in the registers at 0x00,
0x04 and 0x08.

Many of these clocks can be kept out of DT entirely because they are
only there to feed other parts of the clock tree. However some of the
dividers flow directly into cells that appear in device tree (such as
the systick) and so we need to be able to reference them.

In other words the proposed mapping cannot allow us to express the
dividers properly (because the index would have to be negative):
    void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;

Thus I'd favour using different indexes for reset and clock bindings,
both using the naive mapping function:
    void __iomem *reg =  rcc_base + 4 * index

I think that its so much easier to check against the datasheet like
that. Admittedly is we follow the block-of-4-bytes idiom we have to
divide a hex number by four but thats not so hard and we end up with:

               resets = <&rcc  8 0>;
               clocks = <&rcc 16 0>;

At the end of the day if we say we want to follow the datasheet, lets be
do it in the most direct way properly.

Daniel, I'm fine with your proposal.
Doing that, we can have a single compatible string for stm32 family,
even if the reset start offset change between two chips.

quoted

quoted

PS
I've written a custom lookup function to to get from the DT index to an
offset into the struct clk *array I'm using. That means I don't care
much about any big holes in the register space.

How about using the first cell to indicate the type (pll, mux, div, gate)
and the second cell for the number (between 0 and 256)? That way, the
gates numbers would match the reset numbers, and your internal mapping
function would look a bit nicer.

That's another option.
In this case, for reset, we will only need one cell, right?

I think so. For the reset, this is essentially no change versus v7 
except for applying an offset of 0x10 when calculating the base address.

I won't get time to polish up the clk driver this weekend but I will try 
to write a documentation file proposing some clock bindings for you to 
look at.


Daniel.