[RFC 17/17] clk: zynq: remove call to of_clk_init

[Michal Simek]

On 08/23/2013 09:32 AM, Steffen Trumtrar wrote:
> Hi!
> 
> On Thu, Aug 22, 2013 at 05:59:36PM -0700, Sören Brinkmann wrote:
>> On Thu, Aug 22, 2013 at 05:26:47PM -0700, Sören Brinkmann wrote:
>>> Hi Sebastian,
>>>
>>> On Tue, Aug 20, 2013 at 04:04:31AM +0200, Sebastian Hesselbarth wrote:
>>>> With arch/arm calling of_clk_init(NULL) from time_init(), we can now
>>>> remove it from corresponding drivers/clk code.
>>>
>>> I think that would break Zynq.
>>> If I see this correctly you call of_clk_init() from common code,
>>> _before_ the SOC specific time init function is called.
>>> The problem is, that we have code setting up a global pointer which is
>>> required by zynq_clk_setup() which is triggered when of_clk_init() is
>>> called.
>>>
>>> Let me try to illustrate the current call graph:
>>>
>>> time_init()
>>> 	zynq_timer_init()		// this machines init_time()
>>> 		zynq_slcr_init()	// setup System Level Control Registers including a global pointer
>>> 			zynq_clock_init()
>>> 				of_clk_init()
>>> 					zynq_clk_setup()   // requires pointer setup in zynq_slcr_init()
>>> 					...
>>>
>>> IIUC, your series would change this to:
>>> time_init()
>>> 	of_clk_init()
>>> 		zynq_clk_setup()	// SLCR pointer is not setup/NULL
>>> 		...
>>> 	zynq_timer_init()
>>> 		zynq_slcr_init()	// now the pointer becomes valid
>>
>> I guess we could move zynq_slcr_init() into init_irq(). I'll give that a
>> shot tomorrow.
>>
> 
> I propose getting rid of the whole global pointer and let the clkc map the
> address itself instead.
> 
> Then there is no need to shuffle stuff around in the initcalls.
> I have some WIP patches (not rebased on next and not even tested with it,
> but with v3.11-rc4)
> 
> The dtsi would be something like:
> 
>        control-register at f8000000 {

This name is incorrect - it still should be slcr (system level control registers)

> 	       compatible = "simple-bus";

I expect that syscon compatible should be here not in the lock part because
you want to map the whole reg space.

>                #address-cells = <1>;
>                #size-cells = <1>;
>                reg = <0xf8000000 0x1000>;
>                ranges;
> 
>                slcr: slcr at f8000000 {

we should use different name here - lock/locks/etc.

>                        compatible = "xlnx,zynq-slcr", "syscon";
>                        reg = <0xf8000000 0x10>;
>                };



> 
>                clkc: clkc at f8000100 {
>                        #clock-cells = <1>;
>                        compatible = "xlnx,ps7-clkc";
>                        reg = <0xf8000100 0x100>;
>                        ps-clk-frequency = <33333333>;
> 		       xlnx,slcr = <&slcr>;

Currently there is no code which handles locks that's why I think at least
for now it is not necessary to extend binding which feature which is not used.

>                        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";
>                };
> 
>                mio: pinmux at f8000700 {
>                        compatible = "xlnx,ps7-pinctrl";

Have you created any driver for pinmuxing stuff?

I agree with Soren - let's fix the current problem and then when Steffen has patches with syscon
we can look at them.

If there is any discussion about early syscon registration please let me know.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130826/afe34678/attachment.sig>