Re: [PATCH v8 14/16] ARM: dts: Introduce STM32F429 MCU

[Daniel Thompson]

On 14/05/15 17:34, Maxime Coquelin wrote:
> 2015-05-13 21:37 GMT+02:00 Arnd Bergmann <arnd@xxxxxxxx>:
> > On Wednesday 13 May 2015 20:29:12 Daniel Thompson wrote:
> > > On 13/05/15 17:54, Maxime Coquelin wrote:
> > > > 2015-05-13 18:37 GMT+02:00 Arnd Bergmann <arnd@xxxxxxxx>:
> > > > > 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.
> > >
> > > > 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.
>
> > > 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/