[PATCH v2] rockchip: timer: add OF_PLATDATA support for dw-apb-timer

[Johan Jonker]

Hi,

In addition of the previous message, when I compile with the opposite of
OF_REAL (= !OF_PLATDATA) it generates an error in SPL.
Like to know why OF_REAL doesn't work.
For these couple of extra lines to increase build coverage inside does
it matter a lot by adding another messy set of #if ....
Let me know if that's OK to leave it as it is for version 3 with extra
white space patch.

Kind regards,

Johan Jonker

static int dw_apb_timer_of_to_plat(struct udevice *dev)
{
	if (!IS_ENABLED(OF_PLATDATA)) {
		struct dw_apb_timer_priv *priv = dev_get_priv(dev);

		priv->regs = dev_read_addr(dev);
	}

	return 0;
}

arm-linux-gnueabihf-ld.bfd: drivers/timer/dw-apb-timer.o: in function
`dw_apb_timer_of_to_plat':
/drivers/timer/dw-apb-timer.c:95: undefined reference to `dev_read_addr'
arm-linux-gnueabihf-ld.bfd: drivers/timer/dw-apb-timer.o: in function
`dw_apb_timer_probe':
/drivers/timer/dw-apb-timer.c:73: undefined reference to `clk_get_by_index'
make[1]: *** [scripts/Makefile.spl:509: spl/u-boot-spl] Error 1
make: *** [Makefile:2011: spl/u-boot-spl] Error 2



On 1/5/22 5:40 PM, Johan Jonker wrote:
> Hi Simon,
> 
> Thanks you for your comments.
> Shown below are the objdump results of the full U-boot binary
> dw_apb_timer_of_to_plat() function.
> Same goes for the dw_apb_timer_probe() function.
> With if (IS_ENABLED(OF_REAL)) I don't get a useful timer result (boot
> hangs after timer probe, because in full U-boot the reg value isn't
> correct defined. Part of the init probe is missing.
> 
> Could you try it yourself?
> Please advise what options we have.
> 
> Kind regards,
> 
> Johan Jonker
> 
> ==========================================================================
> Patch version 1 with if (IS_ENABLED(OF_REAL)) {}:
> 
> arm-linux-gnueabihf-objdump -d drivers/timer/dw-apb-timer.o >
> ../dw-apb-timer-20220105-v1.asm
> 
> 
> 00000000 <dw_apb_timer_of_to_plat>:
>    0:	2000      	movs	r0, #0
>    2:	4770      	bx	lr
> 
> arm-linux-gnueabihf-objdump -d spl/drivers/timer/dw-apb-timer.o >
> ../dw-apb-timer-20220105-spl-v1.asm
> arm-linux-gnueabihf-objdump -d tpl/drivers/timer/dw-apb-timer.o >
> ../dw-apb-timer-20220105-tpl-v1.asm
> ==========================================================================
> patch version 2 with #if CONFIG_IS_ENABLED(OF_REAL):
> 
> arm-linux-gnueabihf-objdump -d drivers/timer/dw-apb-timer.o > ../dw-apb-
> timer-20220105-v2.asm
> 
> 00000000 <dw_apb_timer_of_to_plat>:
>    0:	b538      	push	{r3, r4, r5, lr}
>    2:	4605      	mov	r5, r0
>    4:	f7ff fffe 	bl	0 <dev_get_priv>
>    8:	4604      	mov	r4, r0
>    a:	4628      	mov	r0, r5
>    c:	f7ff fffe 	bl	0 <devfdt_get_addr>
>   10:	6020      	str	r0, [r4, #0]
>   12:	2000      	movs	r0, #0
>   14:	bd38      	pop	{r3, r4, r5, pc}
> 
> arm-linux-gnueabihf-objdump -d spl/drivers/timer/dw-apb-timer.o >
> ../dw-apb-timer-20220105-spl-v2.asm
> arm-linux-gnueabihf-objdump -d tpl/drivers/timer/dw-apb-timer.o >
> ../dw-apb-timer-20220105-tpl-v2.asm
> 
> ===========================================================================
> 
> On 1/5/22 3:03 PM, Simon Glass wrote:
>> Hi Johan,
>>
>> On Tue, 4 Jan 2022 at 19:15, Johan Jonker <jbx6244 at gmail.com> wrote:
>>>
>>> The Rockchip rk3066 SoC has 3 dw-apb-timer nodes.
>>> U-boot is compiled with OF_PLATDATA TPL/SPL options,
>>> so add OF_PLATDATA support for the dw-apb-timer.
>>> Also change driver name to be able to compile with
>>> U-boot scripts. No reset OF_PLATDATA support was added,
>>> because the rk3066 nodes don't need/have them.
>>>
>>> Signed-off-by: Johan Jonker <jbx6244 at gmail.com>
>>> ---
>>>
>>> Changed V2:
>>>   replace if (IS_ENABLED(OF_REAL)) by #if CONFIG_IS_ENABLED(OF_REAL)
>>> ---
>>>  drivers/timer/dw-apb-timer.c | 30 +++++++++++++++++++++++++-----
>>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>>
>>
>> This seems OK but you have included unrelated changes (whitespace)
>> which should go in a separate patch.
> 
> OK
> With whitespace did you mean this or something else?
> 
> -	.of_to_plat = dw_apb_timer_of_to_plat,
> +	.of_to_plat	= dw_apb_timer_of_to_plat,
> 
>>
>> Can you use if() instead of #if for the CONFIG_IS_ENABLED(OF_REAL) ?
> 
> See comments above.
> Currently not it seems.
> 
>>
>> Regards,
>> Simon
>>