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

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

[Johan Jonker]

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@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(-)

diff --git a/drivers/timer/dw-apb-timer.c b/drivers/timer/dw-apb-timer.c
index 9aed5dd2..9ba8695f 100644
--- a/drivers/timer/dw-apb-timer.c
+++ b/drivers/timer/dw-apb-timer.c
@@ -8,6 +8,7 @@
 #include <common.h>
 #include <dm.h>
 #include <clk.h>
+#include <dt-structs.h>
 #include <malloc.h>
 #include <reset.h>
 #include <timer.h>
@@ -25,6 +26,12 @@ struct dw_apb_timer_priv {
 	struct reset_ctl_bulk resets;
 };
 
+struct dw_apb_timer_plat {
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+	struct dtd_snps_dw_apb_timer dtplat;
+#endif
+};
+
 static u64 dw_apb_timer_get_count(struct udevice *dev)
 {
 	struct dw_apb_timer_priv *priv = dev_get_priv(dev);
@@ -43,7 +50,18 @@ static int dw_apb_timer_probe(struct udevice *dev)
 	struct dw_apb_timer_priv *priv = dev_get_priv(dev);
 	struct clk clk;
 	int ret;
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+	struct dw_apb_timer_plat *plat = dev_get_plat(dev);
+	struct dtd_snps_dw_apb_timer *dtplat = &plat->dtplat;
 
+	priv->regs = dtplat->reg[0];
+
+	ret = clk_get_by_phandle(dev, &dtplat->clocks[0], &clk);
+	if (ret < 0)
+		return ret;
+
+	uc_priv->clock_rate = dtplat->clock_frequency;
+#else
 	ret = reset_get_bulk(dev, &priv->resets);
 	if (ret)
 		dev_warn(dev, "Can't get reset: %d\n", ret);
@@ -57,7 +75,7 @@ static int dw_apb_timer_probe(struct udevice *dev)
 	uc_priv->clock_rate = clk_get_rate(&clk);
 
 	clk_free(&clk);
-
+#endif
 	/* init timer */
 	writel(0xffffffff, priv->regs + DW_APB_LOAD_VAL);
 	writel(0xffffffff, priv->regs + DW_APB_CURR_VAL);
@@ -68,10 +86,11 @@ static int dw_apb_timer_probe(struct udevice *dev)
 
 static int dw_apb_timer_of_to_plat(struct udevice *dev)
 {
+#if CONFIG_IS_ENABLED(OF_REAL)
 	struct dw_apb_timer_priv *priv = dev_get_priv(dev);
 
 	priv->regs = dev_read_addr(dev);
-
+#endif
 	return 0;
 }
 
@@ -91,13 +110,14 @@ static const struct udevice_id dw_apb_timer_ids[] = {
 	{}
 };
 
-U_BOOT_DRIVER(dw_apb_timer) = {
-	.name		= "dw_apb_timer",
+U_BOOT_DRIVER(snps_dw_apb_timer) = {
+	.name		= "snps_dw_apb_timer",
 	.id		= UCLASS_TIMER,
 	.ops		= &dw_apb_timer_ops,
 	.probe		= dw_apb_timer_probe,
 	.of_match	= dw_apb_timer_ids,
-	.of_to_plat = dw_apb_timer_of_to_plat,
+	.of_to_plat	= dw_apb_timer_of_to_plat,
 	.remove		= dw_apb_timer_remove,
 	.priv_auto	= sizeof(struct dw_apb_timer_priv),
+	.plat_auto	= sizeof(struct dw_apb_timer_plat),
 };
-- 
2.20.1

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

[Simon Glass]

Hi Johan,

On Tue, 4 Jan 2022 at 19:15, Johan Jonker <jbx6244@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@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.

Can you use if() instead of #if for the CONFIG_IS_ENABLED(OF_REAL) ?

Regards,
Simon

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

[Johan Jonker]

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

Re: [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@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@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
>>

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

[Simon Glass]

Hi Johan,

On Wed, 5 Jan 2022 at 09:40, Johan Jonker <jbx6244@gmail.com> 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

You need CONFIG_IS_ENABLED(OF_REAL)

(please could you avoid top posting?)

Regards,
Simon

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