Message-ID: <e44aac14-ca51-446c-b564-00a7a440384a@TX2EHSMHS041.ehs.local>
Date:	Mon, 25 Mar 2013 10:08:32 -0700
From:	Sören Brinkmann <soren.brinkmann@...inx.com>
To:	Lars-Peter Clausen <lars@...afoo.de>
CC:	Mike Turquette <mturquette@...aro.org>,
	Josh Cartwright <josh.cartwright@...com>,
	Michal Simek <michal.simek@...inx.com>,
	Peter Crosthwaite <pcrost@...inx.com>,
	Prashant Gaikwad <pgaikwad@...dia.com>,
	<devicetree-discuss@...ts.ozlabs.org>,
	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <git@...inx.com>,
	Jan Lübbe <jlu@...gutronix.de>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Stephen Warren <swarren@...dotorg.org>,
	Peter De Schrijver <pdeschrijver@...dia.com>
Subject: Re: RFC v2: Zynq Clock Controller

Hi Lars,

On Mon, Mar 25, 2013 at 03:46:35PM +0100, Lars-Peter Clausen wrote:
> Hi,
> 
> On 03/22/2013 11:41 PM, Sören Brinkmann wrote:
> > Hi Lars,
> > 
> > On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
> >> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
> >>> Hi,
> >>>
> >>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> >>>
> >>> Required properties:
> >>>  - #clock-cells : Must be 1
> >>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
> >>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> >>>                      (usually 33 MHz oscillators are used for Zynq platforms)
> >>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> >>>
> >>> Optional properties:
> >>>  - clocks : as described in the clock bindings
> >>>  - clock-names : as described in the clock bindings
> >>>
> >>> Clock inputs:
> >>> The following strings are optional parameters to the 'clock-names' property in
> >>> order to provide optional (E)MIO clock sources.
> >>>  - swdt_ext_clk
> >>>  - gem0_emio_clk
> >>>  - gem1_emio_clk
> >>>  - mio_clk_XX          # with XX = 00..53
> >>>
> >>> Example:
> >>>         clkc: clkc {
> >>>                 #clock-cells = <1>;
> >>>                 compatible = "xlnx,ps7-clkc";
> >>>                 ps-clk-frequency = <33333333>;
> >>
> >> The input frequency should be a clock as well.
> > Again, monolithic vs split. I don't see a reason not to just internally
> > call clk_register_fixed_rate(). That way its children do not have to
> > cope with a variable name for the xtal.
> > Also, with my proposal 'clocks' and 'clock-names' would be purely
> > optional properties, only required if optional external inputs are
> > present. Having the xtal defined externally would add mandatory entries for
> > those props.
> 
> 
> 
> > 
> >>
> >>>                 clock-output-names = "armpll", "ddrpll", "iopll", "cpu_6or4x", "cpu_3or2x", "cpu_2x", "cpu_1x", "ddr2x", "ddr3x", "dci", "lqspi", "smc", "pcap", "gem0", "gem1", "fclk0", "fclk1", "fclk2", "fclk3", "can0", "can1", "sdio0", "sdio1", "uart0", "uart1", "spi0", "spi1", "dma", "usb0_aper", "usb1_aper", "gem0_aper", "gem1_aper", "sdio0_aper", "sdio1_aper", "spi0_aper", "spi1_aper", "can0_aper", "can1_aper", "i2c0_aper", "i2c1_aper", "uart0_aper", "uart1_aper", "gpio_aper", "lqspi_aper", "smc_aper", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
> >>>                 /* optional props */
> >>>                 clocks = <&clkc 16>, <&clk_foo>;
> >>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
> >>>         };
> >>>
> >>> With this revised bindings arbitrary upstream and downstream clock providers should be supported and it's also possible to loop back an output as input. The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
> >>> The reason for this is, that a downstream clock provider should use of_clk_get_parent_name() to obtain its parent clock name. For a block with multiple outputs of_clk_get_parent_name() can return a valid clock name only when 'clock-output-names' is present.
> >>> Probably the fclks are the only realistic use case to become parent of downstream clock providers, but I could imagine that e.g. a device driver like UART wants to use the CCF to model its internal clocks, hence it would require its parent clock name. Even though a device driver could use clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should probably work as well. I simply have a bad feeling about breaking of_clk_get_parent_name() for any clock.
> >>> But after all, I'm open for finding a better solution for this.
> >>>
> >>>
> >>> Similar, inputs for optional clock sources through (E)MIO pins can be defined as described in the clock bindings using the 'clocks' and 'clock-names' properties, with 'clock-names' being an arbitrary subset of the documented names. The actual parent clock names are internally resolved using of_clk_get_parent_name().
> >>>
> >>>
> >>> Regarding this monolithic solution versus a finer granular split:
> >>>
> >>> On cost of more complex probing we would also have:
> >>>  - one clock driver covering the peripheral clocks
> >>>  - one for the CPU clock domain
> >>>  - one for the DDR clock domain
> >>>  - one for GEM
> >>>  - one for CAN
> >>>  - one for the APER clks
> >>>  - one for the PLLs
> >>>  - one for fclks
> >>
> >> fclks are the same as peripheral clocks except for the gate bit, as far as I
> >> can see.
> > And that makes them quite different, since they have to access multiple
> > registers instead of a single one. Also, the fclks have two dividers.
> > If you want to cover all of those with a single driver, you need a
> > plethora of arguments/properties to catch the small differences.
> > 
> >>
> >>>  - probably one for the debug clocks (not sure whether we need those)
> >>>
> >>> Except for the peripheral one and probably the fclk, they would all be instantiated just once. So, there is not a lot of reuse going on.
> >>
> >> PLLs are going to be instantiated multiple times as well.
> > As mentioned in the very next sentence, I rather see a single driver
> > with multiple outputs. Take suspend: My plan is to have a few functions
> > like zynq_clk_(suspend|resume). That should take care of bypassing
> > shutting down the PLLs (and probably more). Therefore it's easier to
> > have them all in a single driver.
> > And if it turned out, that other clocks require special handling for
> > such system level functions, that could be addressed that way too with
> > the monolithic approach.
> > 
> >>
> >>> Fclks I would probably also rather put into one driver with four outputs instead of instantiating a single output driver multiple times. Same for PLLs.
> >>>
> >>> And then there is e.g. a mux for the system watchdog input which doesn't really fit anywhere and it would be pretty much ridiculous to have another clock driver just for that one and it would also become "hidden" in one of the others.
> >>>
> >>> In my opinion that's just not necessary. We would create a bunch of clock drivers including DT bindings for them, probing would become more complex and it doesn't really help with the probe/naming issue. So, I don't think it's beneficial to go down that road.
> >>>
> >>> The monolithic solution would need one custom driver for the PLLs, DT bindings wouldn't be required for it though. Everything else should be internally described using the clock-primitives.
> >>>
> >>> Other than having a much simpler probe and init process, I still think it might be beneficial to have this monolithic block with a holistic view of the clock tree. For suspend e.g. I think the clock controller could export functions like zynq_clkc_(suspend|resume) and then the controller handles the PLL bypassing/shutdown.
> >>> Regarding full dynamic reparenting, I don't know how exactly that could work, but with the clock controller there is at least a block where that intelligence would be going and which has knowledge of all the 'struct clk *' required to reparent clocks.
> >>>
> >>> Regarding the DT description, it is probably controversial what is considered better. I, like the Tegra folks, think having one clock controller in there is fine and hides irrelevant implementation details.
> >>>
> >>
> >> I still don't like the monolithic solution. From my point of view it is
> >> architecturally inferior, it is a bad abstraction. You argue that for a
> >> non-monolithic version you'd have to implement a clock driver for each
> >> different type of clock. But you still have to do this for the monolithic
> >> version, they'd probably just end up in one huge messy file. Unless you are
> >> going to duplicate huge amounts of lines you'd probably have functions like
> >> add_gem_clk, add_peripheral_clk, add_pll_clk and so on in your monolithic drivers.
> > It probably makes sense to differ between having custom drivers and
> > describing the whole clock tree in DT.
> > 
> > From reusability perspective, it may make sense to factor out some code
> > in drivers. IMHO, this does only apply to the peripheral clocks, since
> > everything else isn't reused and/or quite Zynq specific.
> > But a monolithic approach would not even prevent this. You could just
> > transparently change the implementation: Just add a clock driver,
> > replace the original code in the controller to use the new driver
> > instead and you're good. No need to touch DT bindings.
> > 
> > In Zynq a peripheral clock is essentially described by:
> >         clk = clk_register_mux(NULL, mux_name, parents, 4, 0,                    
> >                         clk_ctrl, 4, 2, 0, lock);                                
> >                                                                                  
> >         clk = clk_register_divider(NULL, div_name, mux_name, 0, clk_ctrl, 8, 6,  
> >                         CLK_DIVIDER_ONE_BASED, lock);                            
> >                                                                                  
> >         clks[clk0] = clk_register_gate(NULL, clk_name0, div_name,                
> >                         CLK_SET_RATE_PARENT, clk_ctrl, 0, 0, lock);              
> > 
> > If we wrapped this by a driver, any code outside of that driver couldn't
> > change the mux setting, since its struct clk* is not exposed outside.
> > To get around this we'd have to reimplement all the clock ops, to create a
> > clock which supports all possible operations.
> > In the monolithic approach we could simply remember that struct clk* and
> > work with it.
> > 
> > Bottom line: Factoring out some parts of the monolithic driver might
> > make sense for some parts. But it can be done later in a transparent way,
> > especially w/o touching DT bindings.
> > 
> > 
> > The other thing is describing the whole clock tree in DT:
> > That would force us to not only describe clocks for which it might make
> > sense, but also all Zynq specific clocks in custom drivers and DT
> > bindings and we gain nothing from it.
> > 
> >>
> >> The SLCR is a virtual construct, which groups the clock units together. But the
> >> clock units are the actual entities. E.g. imagine a Zync 2.0, it would probably
> >> reuse the same clock units, but there would quite likely be different clocks
> >> mapped at different addresses. If you choose a non-monolithic implementation
> >> you'd just have to update your devicetree to make this work, with a monolithic
> >> version you'd have to add a almost identical driver for the v2.
> > Either way you'll end up with a lot of lines describing the hierarchy.
> > Either in the dts or in the clock driver.
> > 
> > 
> > I think, my problem is, that I do not yet know how certain functionality
> > will be implemented later and what exact use-cases have to be supported.
> > With the monolithic approach I keep all options open. We hopefully will
> > never have to touch its DT bindings again. And refactoring the code and
> > migrating it to use dedicated clock drivers should be transparent
> > changes, if deemed beneficial.
> > Furthermore, I have a driver, which has references to all strucht clk*
> > in the system and can work with the whole clock tree leveraging a system
> > level view. Separating things in smaller blocks might complicate things
> > if a use case requires this.
> > And finally pushing the whole hierarchy description into the DT seems to
> > be the most restrictive approach, without offering big rewards.
> 
> I don't see those restrictions you see with a dt binding which has nodes for
> each clock block. You seem to be under the impression, that each device tree
> node requires its own driver or device. This is not the case, have you looked
> at how the current upstream zynq clock support is implemented? This is still
> one monolithic driver, but there are multiple dt nodes, one for each clock
> block.
I thought the one "huge messy" file was one thing you wanted to avoid?
Also, how does it address my concern regarding inaccessible
'struct clk *'? A clk_register_foo() will only return a single struct
clk *. So, wrapping several clk_register() calls within one
clk_register_foo() makes all but the actual output inaccessible.
And if we do not directly call clk_register_foo() but do the probing
through DT the clock init code does not even get hold of that struct clk
*.

> This leads to a very clean and well structured code and also nicely
> structured dt, which represents the tree as it is in hardware. If I understand
> you correctly what you suggest is that you basically want to copy 'n paste the
> same 10 lines to instantiate a peripheral clock, which you mentioned above,
> again and again. This will be quite messy.
For the peripheral clocks you're right, there is some reuse possible.
For those I have a helper, like zynq_clk_register_periph(). But for pretty much
everything else I don't.

In conclusion, I see the DT approach limiting my ability to modify the
clock tree when implementing system level functionality. 
Probably quite a stretch, but imagine some smart power management code,
which might try to reparent all and every clock in the system to certain
PLLs. How would such a power manager get hold of all the struct clk * it
needs. In a system probing fully through DT we cannot get hold of those
and even less if we wrap several clocks in one clock_foo. Unless, we add
a dummy device which has every clock in the system as its input. And
that sounds even wackier than having a clock controller which provides
all the clocks to consumers.

	Sören


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists