Message-ID: <27dae808-1d3a-4001-8eb2-b0a6e2a34b8f@AM1EHSMHS013.ehs.local>
Date:	Wed, 20 Mar 2013 16:56:11 -0700
From:	Sören Brinkmann <soren.brinkmann@...inx.com>
To:	Mike Turquette <mturquette@...aro.org>,
	Josh Cartwright <josh.cartwright@...com>,
	Michal Simek <michal.simek@...inx.com>,
	Peter Crosthwaite <pcrost@...inx.com>,
	Lars-Peter Clausen <lars@...afoo.de>
CC:	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: RFC v2: Zynq Clock Controller

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>;
                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
 - 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.
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.

        Regards,
        Sören


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.


--
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