Subject: [PATCH 4/4] clk: zynq: Use of_init_clk_data()

From: [email-masked] (Rob Herring)
Date: Fri, 21 Dec 2012 09:52:56 -0600

On 12/19/2012 02:54 PM, Stephen Boyd wrote:
> On 12/19/12 12:30, Stephen Boyd wrote:
>> On 12/19/12 11:22, Soren Brinkmann wrote:
>>> Hi Stephen,
>>>
>>> I guess Josh is the better person to talk about this, since he created the
>>> patches regarding common clock for mainline, but I tried running your series
>>> and ran into this:
>>>
>>> Unable to handle kernel NULL pointer dereference at virtual address 0000002a
>>> pgd = c0004000
>>> [0000002a] *pgd=00000000
>>> Internal error: Oops: 5 [#1] PREEMPT ARM
>>> Modules linked in:
>>> CPU: 0    Tainted: G        W     (3.7.0-rc3-00025-gc11ffdd-dirty #246)
>>> PC is at __clk_prepare+0x20/0x80
>>> LR is at clk_prepare+0x2c/0x44
>>> pc : [<c031659c>]    lr : [<c0316628>]    psr: a0000153
>>> sp : ee02fdd0  ip : ee02fde8  fp : ee02fde4
>>> r10: 00000000  r9 : 00000000  r8 : c0587884
>>> r7 : 00000000  r6 : c05aab98  r5 : fffffffe  r4 : fffffffe
>>> r3 : 00000000  r2 : 00000000  r1 : 00000000  r0 : fffffffe
>>> Flags: NzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
>>> Control: 18c5387d  Table: 00004059  DAC: 00000015
>>> Process swapper (pid: 1, stack limit = 0xee02e230)
>>> Stack: (0xee02fdd0 to 0xee030000)
>>> fdc0:                                     c05b6a18 fffffffe ee02fdfc ee02fde8
>>> fde0: c0316628 c0316588 ee085400 fffffffe ee02fe24 ee02fe00 c03c1358 c0316608
>>> fe00: c03c1328 ee085410 c05aab98 c05aab98 00000000 c0587884 ee02fe34 ee02fe28
>>> fe20: c026a5c0 c03c1334 ee02fe5c ee02fe38 c0268f50 c026a5a8 c026a7b8 c0313ac0
>>> fe40: ee085410 ee085410 ee085444 c05aab98 ee02fe7c ee02fe60 c02691c0 c0268e18
>>> fe60: c0269150 c05aab98 c0269150 00000000 ee02fea4 ee02fe80 c0267320 c026915c
>>> fe80: ee00948c ee096df0 c021e02c c05aab98 ed9a0bc0 c05ab3a8 ee02feb4 ee02fea8
>>> fea0: c0268a28 c02672d0 ee02fee4 ee02feb8 c0268408 c0268a0c c04b56d1 00000000
>>> fec0: ee02fee4 c05aab98 c0565e60 00000000 0000006f c0587884 ee02ff14 ee02fee8
>>> fee0: c0269990 c0268340 00000000 00000000 c0565e60 00000000 0000006f c0587884
>>> ff00: 00000000 00000000 ee02ff24 ee02ff18 c026a9cc c02698f0 ee02ff3c ee02ff28
>>> ff20: c0565e84 c026a984 ee02e000 ee02ff40 ee02ff74 ee02ff40 c00086f8 c0565e6c
>>> ff40: c04f72e4 00000000 ee02ff74 00000006 00000006 c0571dac c0571d8c 0000006f
>>> ff60: c0587884 00000000 ee02ffac ee02ff78 c03bf9c4 c0008664 00000006 00000006
>>> ff80: c05511a8 00000000 ee02ffac 00000000 c03bf8cc 00000000 00000000 00000000
>>> ffa0: 00000000 ee02ffb0 c000ea98 c03bf8d8 00000000 00000000 00000000 00000000
>>> ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>> ffe0: 00000000 00000000 00000000 00000000 00000013 00000000 0158ec21 0eced1ff
>>> [<c031659c>] (__clk_prepare+0x20/0x80) from [<c0316628>] (clk_prepare+0x2c/0x44)
>>> [<c0316628>] (clk_prepare+0x2c/0x44) from [<c03c1358>] (xuartps_probe+0x30/0x1ac
>>> )
>>> [<c03c1358>] (xuartps_probe+0x30/0x1ac) from [<c026a5c0>] (platform_drv_probe+0x
>>> 24/0x28)
>>> [<c026a5c0>] (platform_drv_probe+0x24/0x28) from [<c0268f50>] (driver_probe_devi
>>> ce+0x144/0x344)
>>> [<c0268f50>] (driver_probe_device+0x144/0x344) from [<c02691c0>] (__driver_attac
>>> h+0x70/0x94)
>>> [<c02691c0>] (__driver_attach+0x70/0x94) from [<c0267320>] (bus_for_each_dev+0x5
>>> c/0x8c)
>>> [<c0267320>] (bus_for_each_dev+0x5c/0x8c) from [<c0268a28>] (driver_attach+0x28/
>>> 0x30)
>>> [<c0268a28>] (driver_attach+0x28/0x30) from [<c0268408>] (bus_add_driver+0xd4/0x
>>> 254)
>>> [<c0268408>] (bus_add_driver+0xd4/0x254) from [<c0269990>] (driver_register+0xac
>>> /0x14c)
>>> [<c0269990>] (driver_register+0xac/0x14c) from [<c026a9cc>] (platform_driver_reg
>>> ister+0x54/0x68)
>>> [<c026a9cc>] (platform_driver_register+0x54/0x68) from [<c0565e84>] (xuartps_ini
>>> t+0x24/0x44)
>>> [<c0565e84>] (xuartps_init+0x24/0x44) from [<c00086f8>] (do_one_initcall+0xa0/0x
>>> 170)
>>> [<c00086f8>] (do_one_initcall+0xa0/0x170) from [<c03bf9c4>] (kernel_init+0xf8/0x
>>> 2ac)
>>> [<c03bf9c4>] (kernel_init+0xf8/0x2ac) from [<c000ea98>] (ret_from_fork+0x14/0x20
>>> )
>>> Code: e8bd4000 e2504000 01a05004 0a000015 (e594302c) 
>>> ---[ end trace 1b75b31a2719ed1d ]---
>>> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>>>
>>>
>>> A probably unique thing I do is, I set the status of uart0 to disabled. This way
>>> I can reuse my rootfs which does not run getty on ttyPS1. And this worked fine
>>> before.
>>>
>> Thanks for testing. It seems that clocks are failing to register. Please
>> try this patch.
>>
> 
> Also, why are clock-names an optional property in devicetree? It would
> be nice to not have two different APIs for consumers (i.e. drivers) to
> get clocks from devicetree bindings. I suggest we do this for your uart
> driver so that you can use the regular clock APIs and not the of_* ones.
> We should get rid of of_clk_get().

This needs to work w/o names as you are breaking any platform with an
old dtb and this applied. This is a no-no. We've ignored that on ARM as
DT support has been under development, but it is time to start enforcing
that requirement. At least some platforms are using DT in production.

We match by index for irqs, register space, etc., so we should be able
to for clocks as well.

Rob

> 
> ---->8-----
> 
> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> index 5914b56..eef5f0c 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -45,6 +45,7 @@
>                         reg = <0xE0000000 0x1000>;
>                         interrupts = <0 27 4>;
>                         clocks = <&uart_clk 0>;
> +                       clock-names = "ref";
>                 };
> 
>                 uart1: uart at e0001000 {
> @@ -52,6 +53,7 @@
>                         reg = <0xE0001000 0x1000>;
>                         interrupts = <0 50 4>;
>                         clocks = <&uart_clk 1>;
> +                       clock-names = "ref";
>                 };
> 
>                 slcr: slcr at f8000000 {
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index 2be22a2..6868e6b 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -947,10 +947,10 @@ static int xuartps_probe(struct platform_device *pdev)
>         struct resource *res, *res2;
>         struct clk *clk;
> 
> -       clk = of_clk_get(pdev->dev.of_node, 0);
> -       if (!clk) {
> -               dev_err(&pdev->dev, "no clock specified\n");
> -               return -ENODEV;
> +       clk = devm_clk_get(&pdev->dev, "ref");
> +       if (IS_ERR(clk)) {
> +               dev_err(&pdev->dev, "failed to get clock\n");
> +               return PTR_ERR(clk);
>         }
> 
>         rc = clk_prepare_enable(clk);
>