Re: [PATCH 2/8] clk: berlin: add clock binding docs for Marvell Berlin2 SoCs

[Alexandre Belloni <alexandre.belloni@free-electrons.com>]

On 14/05/2014 at 21:41:06 -0700, Mike Turquette wrote :
> > > The following is a copy/paste from an email I sent earlier today[1]. Of
> > > course per-clock data makes great sense if you have an off-SoC clock
> > > such as a fixed-rate oscillator (e.g. the fixed-clock binding). Let me
> > > know what you think:
> > [...]
> > > Using this type of binding you only need to declare your clock generator
> > > IP node in dts, and then define a mapping in the DT include chroot. Then
> > > you can define your per-clock data inside of your clock driver instead
> > > of putting all of the details inside of DT.
> > > 
> > > If you have a strong reason to do it the way that you originally posted
> > > then let me know.
> > 
> > Actually, the intermediate patch set sent before this one had a single
> > DT clock node. The most important draw-back of a single clock node
> > is that Berlin's global registers are more like a register dumpster.
> > Vital other registers, e.g. reset, are intermixed with clock registers.
> 
> Yeah, this is pretty common. The compatible string should reflect the IP
> block as a whole, not just the clocks part of it. Lots of vendors have
> PRCMs or PRCMUs or CARs or whatever.
> 
> Check out the recent series to have the reset bits and regulator support
> added to the qcom binding[1]. (I'm using qcom quite a bit in my examples
> but they are not the first to add reset control to their clock driver. I
> think Tegra did it first...)
> 
> > 
> > Given the lack of public datasheets (I look everything up in some
> > auto-generated BSP includes), I like the current approach because it
> > helps to get in at least some structure to the register mess ;)
> > 
> > Considering the postponed of_clk_create_name() helper, that would allow
> > us to remove at least the names from DT again. Another option would be
> > a syscon node for the registers, that clk, reset, pinctrl drivers can
> > access. But IIRC early syscon support isn't settled, yet?
> 
> Yeah, I'm not sure of the state of syscon. And modeling this stuff in
> the clock driver isn't the end of the world. There might be better
> places than drivers/clk/* for sure... I sometimes joke that the name of
> the IP block determines where the code lands. If it is Power, Reset &
> Clock Manager (several platforms use this acronym) then it can end up in
> drivers/clk or drivers/reset really easily. Same for Clock and Reset IP
> blocks (Tegra).
> 

So, this is called "global". On BG2/BG2CD, you have pinmuxing, a
register enabling nand power and switching the pll bypasses, all the pll
configuration, complex clocks, reset, product ID, nand write protect,
simple clocks, more resets, more clocks for which we don't have a driver
yet and finally pad control.

Now, on BG2Q, it is almost the same but between the cpupll registers and
the global registers, there is the whole APB bus, so timers, gpio
controllers, ...

I would believe that is the strong reason you are asking for.

It would also break the DT ABI (not that I care) almost each time we add
a new clock that we didn't know about.

Let me suggest something completely wild (to rephrase Thomas Petazzoni
and Kevin Hilman at ELC):

How about we have only a "marvell,berlin2", "marvell,berlin2cd" or
"marvell,berlin2q" compatible string and handle that in either
arch/arm/mach-berlin or driver/soc/mach-berlin and then use a bunch of
structures to add all the peripherals like clocks, reset, timer, gpio
controllers, pinmux, padconf...
Maybe we could call those structures "platform_data" :)


> > 
> > So, my current idea is:
> > - take this as is, stabilize berlin branches for v3.16
> > - review of_clk_create_name() and of_clk_get_parent_name() to allow
> >   to remove clock-output-names properties from Berlin (and other) dtsis
> > - maybe switch to early syscon if it is available in v3.16
> > 
> > I know this would likely break DT ABI policy, but hey who else boots
> > mainline Linux on his Chromecast currently except me :P
> 
> I'm not a big fan of DT stable ABI, but if you plan on changing it for
> 3.17 why not just do it the right way the first time? And switching to
> syscon is not a hard requirement. I'm OK with you putting the reset and
> regulator stuff in the clock driver if that makes the most sense for
> your platform (especially if registers are shared and the same locks
> need to be used, etc).
> 


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com