[PATCH 0/2] clocksource: ostm: Add support for RZ/A2

[PATCH 0/2] clocksource: ostm: Add support for RZ/A2

[Chris Brandt]

Modify the driver to take into accound clock drivers that are not 100% DT,
and also update the binding documentation.

Chris Brandt (2):
  clocksource/drivers/ostm: Delay driver registration
  dt-bindings: timer: ostm: Add R7S9210 support

 .../devicetree/bindings/timer/renesas,ostm.txt     |  3 +-
 drivers/clocksource/renesas-ostm.c                 | 35 ++++++++++++++++++++--
 2 files changed, 35 insertions(+), 3 deletions(-)

-- 
2.16.1

[PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

The newer RZ/A clock control driver no longer registers all its clocks
using DT, therefore the clocks required by this driver are no longer
present at the beginning of boot.

Because of this, TIMER_OF_DECLARE can no longer be used because this
causes the driver to get probed too early before the parent clock exists,
and the probe will fail with "ostm: Failed to get clock".

So, we'll change this driver to register/probe during subsys_initcall which
is after the appropriate clocks have been registered.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/clocksource/renesas-ostm.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/renesas-ostm.c b/drivers/clocksource/renesas-ostm.c
index 6cffd7c6001a..fc5e4ac294b8 100644
--- a/drivers/clocksource/renesas-ostm.c
+++ b/drivers/clocksource/renesas-ostm.c
@@ -22,6 +22,7 @@
 #include <linux/interrupt.h>
 #include <linux/sched_clock.h>
 #include <linux/slab.h>
+#include <linux/platform_device.h>
 
 /*
  * The OSTM contains independent channels.
@@ -101,8 +102,12 @@ static u64 notrace ostm_read_sched_clock(void)
 static void __init ostm_init_sched_clock(struct ostm_device *ostm,
 			unsigned long rate)
 {
+	unsigned long flags;
+
 	system_clock = ostm->base + OSTM_CNT;
+	local_irq_save(flags);
 	sched_clock_register(ostm_read_sched_clock, 32, rate);
+	local_irq_restore(flags);
 }
 
 static int ostm_clock_event_next(unsigned long delta,
@@ -192,9 +197,10 @@ static int __init ostm_init_clkevt(struct ostm_device *ostm, int irq,
 	return 0;
 }
 
-static int __init ostm_init(struct device_node *np)
+static int __init ostm_probe(struct platform_device *pdev)
 {
 	struct ostm_device *ostm;
+	struct device_node *np = pdev->dev.of_node;
 	int ret = -EFAULT;
 	struct clk *ostm_clk = NULL;
 	int irq;
@@ -262,4 +268,29 @@ static int __init ostm_init(struct device_node *np)
 	return 0;
 }
 
-TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
+static const struct of_device_id ostm_of_table[] = {
+	{
+		.compatible = "renesas,ostm",
+	},
+	{ /* sentinel */ }
+};
+
+static int ostm_remove(struct platform_device *pdev)
+{
+	return -EBUSY; /* cannot unregister clockevent and clocksource */
+}
+
+static struct platform_driver ostm_device_driver = {
+	.remove		= ostm_remove,
+	.driver		= {
+		.name	= "ostm",
+		.of_match_table = of_match_ptr(ostm_of_table),
+	},
+};
+
+static int __init ostm_init(void)
+{
+	return platform_driver_probe(&ostm_device_driver, ostm_probe);
+}
+
+subsys_initcall(ostm_init);
-- 
2.16.1

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Daniel Lezcano]

On 29/08/2018 15:29, Chris Brandt wrote:
> The newer RZ/A clock control driver no longer registers all its clocks
> using DT, therefore the clocks required by this driver are no longer
> present at the beginning of boot.
> 
> Because of this, TIMER_OF_DECLARE can no longer be used because this
> causes the driver to get probed too early before the parent clock exists,
> and the probe will fail with "ostm: Failed to get clock".
> 
> So, we'll change this driver to register/probe during subsys_initcall which
> is after the appropriate clocks have been registered.

Can the boot constraints [1] solve this issue instead of the changes you
are proposing ?

[1] https://lwn.net/Articles/747250/




-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

On Wednesday, August 29, 2018 1, Daniel Lezcano wrote:
> Can the boot constraints [1] solve this issue instead of the changes you
> are proposing ?
> 
> [1] https://lwn.net/Articles/747250/

Thanks for the suggestion, but...

I grepped for "boot_constraint" and it shows up nowhere in the current 
kernel.

Also, this article was written Feb 16, 2018, and I can see that the 
patch series was still being submitted (V7) as of Feb 23, 2018.

And, while I'm not totally sure it would help me, when I did look at 
their example code, they were calling dev_boot_constraint_add_deferrable_of
in subsys_initcall.... which is what I was changing this driver to init
from anyway. And, by the time you get to subsys_initcall in the boot 
process, my issue has already occurred so their fix wouldn't help me 
anyway.


Chris

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Daniel Lezcano]

On 29/08/2018 17:44, Chris Brandt wrote:
> On Wednesday, August 29, 2018 1, Daniel Lezcano wrote:
>> Can the boot constraints [1] solve this issue instead of the changes you
>> are proposing ?
>>
>> [1] https://lwn.net/Articles/747250/
> 
> Thanks for the suggestion, but...
> 
> I grepped for "boot_constraint" and it shows up nowhere in the current 
> kernel.
> 
> Also, this article was written Feb 16, 2018, and I can see that the 
> patch series was still being submitted (V7) as of Feb 23, 2018.

Ah ok, fair enough, I thought it was merged. In any case, after thinking
about it, it wouldn't have helped.

My concern is if we can avoid changing the TIMER_OF_DECLARE because of
the boot order, it would be better.

Can returning EPROBE_DEFER fix this issue? Or use the 'complex
dependencies' [1]?

I'm pretty busy ATM, so I can not investigate now if the suggestions
above are fine or just stupid. I will have a look as soon as I can.

  -- Daniel

[1] https://www.kernel.org/doc/html/v4.15/driver-api/device_link.html




-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

On Wednesday, August 29, 2018 1, Daniel Lezcano wrote:
> 
> My concern is if we can avoid changing the TIMER_OF_DECLARE because of
> the boot order, it would be better.
> 
> Can returning EPROBE_DEFER fix this issue? Or use the 'complex
> dependencies' [1]?

I'll try returning EPROBE_DEFER and see if that is an easy fix.


Chris

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

On Wednesday, August 29, 2018 1, Daniel Lezcano wrote:
> My concern is if we can avoid changing the TIMER_OF_DECLARE because of
> the boot order, it would be better.
> 
> Can returning EPROBE_DEFER fix this issue? Or use the 'complex
> dependencies' [1]?

So I tried just returning -EPROBE_DEFER, but it didn't work.

The main reason is ostm_init() is called from timer_probe().

 start_kernel() -> time_init() -> timer_probe() -> ostm_init()

If you look at timer_probe, it only looks to see if the return value was
non-zero to know if something went wrong. It doesn't really care what 
the actual value was, or do anything different, so returning 
-EPROBE_DEFER does you no good.

When you use TIMER_OF_DECLARE, that puts your driver into the 
__timer_of_table, and start_kernel/time_init/timer_probe is your only shot at 
getting your driver running. You don't get another chance later in boot.
So, even if that boot constraint did exist, it wouldn't work for timers 
that used TIMER_OF_DECLARE.

I guess as a rule of thumb, if your timer requires a clock, that clock 
driver has to be an OF clock because it needs to reside in the 
__clk_of_table.

Chris

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Geert Uytterhoeven]

Hi Daniel,

On Wed, Aug 29, 2018 at 6:26 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 29/08/2018 17:44, Chris Brandt wrote:
> > On Wednesday, August 29, 2018 1, Daniel Lezcano wrote:
> >> Can the boot constraints [1] solve this issue instead of the changes you
> >> are proposing ?
> >>
> >> [1] https://lwn.net/Articles/747250/
> >
> > Thanks for the suggestion, but...
> >
> > I grepped for "boot_constraint" and it shows up nowhere in the current
> > kernel.
> >
> > Also, this article was written Feb 16, 2018, and I can see that the
> > patch series was still being submitted (V7) as of Feb 23, 2018.
>
> Ah ok, fair enough, I thought it was merged. In any case, after thinking
> about it, it wouldn't have helped.
>
> My concern is if we can avoid changing the TIMER_OF_DECLARE because of
> the boot order, it would be better.
>
> Can returning EPROBE_DEFER fix this issue? Or use the 'complex
> dependencies' [1]?

*_OF_DECLARE() is not compatible with EPROBE_DEFER, which causes
issues with complex dependencies.
That's exactly why many subsystems are moving away from it.
E.g. IOMMU_OF_DECLARE was removed in v4.19-rc1.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Daniel Lezcano]

On 30/08/2018 09:54, Geert Uytterhoeven wrote:
> Hi Daniel,
> 
> On Wed, Aug 29, 2018 at 6:26 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 29/08/2018 17:44, Chris Brandt wrote:
>>> On Wednesday, August 29, 2018 1, Daniel Lezcano wrote:
>>>> Can the boot constraints [1] solve this issue instead of the changes you
>>>> are proposing ?
>>>>
>>>> [1] https://lwn.net/Articles/747250/
>>>
>>> Thanks for the suggestion, but...
>>>
>>> I grepped for "boot_constraint" and it shows up nowhere in the current
>>> kernel.
>>>
>>> Also, this article was written Feb 16, 2018, and I can see that the
>>> patch series was still being submitted (V7) as of Feb 23, 2018.
>>
>> Ah ok, fair enough, I thought it was merged. In any case, after thinking
>> about it, it wouldn't have helped.
>>
>> My concern is if we can avoid changing the TIMER_OF_DECLARE because of
>> the boot order, it would be better.
>>
>> Can returning EPROBE_DEFER fix this issue? Or use the 'complex
>> dependencies' [1]?
> 
> *_OF_DECLARE() is not compatible with EPROBE_DEFER, which causes
> issues with complex dependencies.

What causes issues ? Add support for EPROBE_DEFER with OF_DECLARE or not
having support of it ?

> That's exactly why many subsystems are moving away from it.
> E.g. IOMMU_OF_DECLARE was removed in v4.19-rc1.

Ok, thanks for information.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Geert Uytterhoeven]

Hi Daniel,

On Thu, Aug 30, 2018 at 10:09 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 30/08/2018 09:54, Geert Uytterhoeven wrote:
> > On Wed, Aug 29, 2018 at 6:26 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >> On 29/08/2018 17:44, Chris Brandt wrote:
> >>> On Wednesday, August 29, 2018 1, Daniel Lezcano wrote:
> >>>> Can the boot constraints [1] solve this issue instead of the changes you
> >>>> are proposing ?
> >>>>
> >>>> [1] https://lwn.net/Articles/747250/
> >>>
> >>> Thanks for the suggestion, but...
> >>>
> >>> I grepped for "boot_constraint" and it shows up nowhere in the current
> >>> kernel.
> >>>
> >>> Also, this article was written Feb 16, 2018, and I can see that the
> >>> patch series was still being submitted (V7) as of Feb 23, 2018.
> >>
> >> Ah ok, fair enough, I thought it was merged. In any case, after thinking
> >> about it, it wouldn't have helped.
> >>
> >> My concern is if we can avoid changing the TIMER_OF_DECLARE because of
> >> the boot order, it would be better.
> >>
> >> Can returning EPROBE_DEFER fix this issue? Or use the 'complex
> >> dependencies' [1]?
> >
> > *_OF_DECLARE() is not compatible with EPROBE_DEFER, which causes
> > issues with complex dependencies.
>
> What causes issues ? Add support for EPROBE_DEFER with OF_DECLARE or not
> having support of it ?

Driver init functions declared using *_OF_DECLARE() are called exactly once.
Hence if they fail, they are never retried. Calling order among subsystems is
hardcoded, and the actual order was determined by historical reasons (legacy
PC systems with always-on clocks and power domains ;-).
This breaks as soon as e.g. timers or interrupt controllers start depending
on clocks and/or power domains.

Drivers using the device driver framework are retried later when their init
function returns -EPROBE_DEFER. So (eventually) they all succeed
initialization.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Daniel Lezcano]

On 30/08/2018 10:27, Geert Uytterhoeven wrote:
> Hi Daniel,
> 
> On Thu, Aug 30, 2018 at 10:09 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 30/08/2018 09:54, Geert Uytterhoeven wrote:
>>> On Wed, Aug 29, 2018 at 6:26 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>> On 29/08/2018 17:44, Chris Brandt wrote:
>>>>> On Wednesday, August 29, 2018 1, Daniel Lezcano wrote:
>>>>>> Can the boot constraints [1] solve this issue instead of the changes you
>>>>>> are proposing ?
>>>>>>
>>>>>> [1] https://lwn.net/Articles/747250/
>>>>>
>>>>> Thanks for the suggestion, but...
>>>>>
>>>>> I grepped for "boot_constraint" and it shows up nowhere in the current
>>>>> kernel.
>>>>>
>>>>> Also, this article was written Feb 16, 2018, and I can see that the
>>>>> patch series was still being submitted (V7) as of Feb 23, 2018.
>>>>
>>>> Ah ok, fair enough, I thought it was merged. In any case, after thinking
>>>> about it, it wouldn't have helped.
>>>>
>>>> My concern is if we can avoid changing the TIMER_OF_DECLARE because of
>>>> the boot order, it would be better.
>>>>
>>>> Can returning EPROBE_DEFER fix this issue? Or use the 'complex
>>>> dependencies' [1]?
>>>
>>> *_OF_DECLARE() is not compatible with EPROBE_DEFER, which causes
>>> issues with complex dependencies.
>>
>> What causes issues ? Add support for EPROBE_DEFER with OF_DECLARE or not
>> having support of it ?
> 
> Driver init functions declared using *_OF_DECLARE() are called exactly once.
> Hence if they fail, they are never retried. Calling order among subsystems is
> hardcoded, and the actual order was determined by historical reasons (legacy
> PC systems with always-on clocks and power domains ;-).
> This breaks as soon as e.g. timers or interrupt controllers start depending
> on clocks and/or power domains.
> 
> Drivers using the device driver framework are retried later when their init
> function returns -EPROBE_DEFER. So (eventually) they all succeed
> initialization.

Yeah, I got this point. But it is the meaning of your sentence: "...
which causes issues with complex dependencies.".

It is ambiguous *what* causes the issues.

Did you meant an attempt was done to support EPROBE_DEFER with
*_OF_DECLARE but caused too much issues with the complex dependencies?

Or the current situation is causing too much issues with the complex
dependencies?

(I know the latter is true, it is about the meaning of the sentence).


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Geert Uytterhoeven]

Hi Daniel,

On Thu, Aug 30, 2018 at 10:37 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 30/08/2018 10:27, Geert Uytterhoeven wrote:
> > On Thu, Aug 30, 2018 at 10:09 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >> On 30/08/2018 09:54, Geert Uytterhoeven wrote:
> >>> On Wed, Aug 29, 2018 at 6:26 PM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>> On 29/08/2018 17:44, Chris Brandt wrote:
> >>>>> On Wednesday, August 29, 2018 1, Daniel Lezcano wrote:
> >>>>>> Can the boot constraints [1] solve this issue instead of the changes you
> >>>>>> are proposing ?
> >>>>>>
> >>>>>> [1] https://lwn.net/Articles/747250/
> >>>>>
> >>>>> Thanks for the suggestion, but...
> >>>>>
> >>>>> I grepped for "boot_constraint" and it shows up nowhere in the current
> >>>>> kernel.
> >>>>>
> >>>>> Also, this article was written Feb 16, 2018, and I can see that the
> >>>>> patch series was still being submitted (V7) as of Feb 23, 2018.
> >>>>
> >>>> Ah ok, fair enough, I thought it was merged. In any case, after thinking
> >>>> about it, it wouldn't have helped.
> >>>>
> >>>> My concern is if we can avoid changing the TIMER_OF_DECLARE because of
> >>>> the boot order, it would be better.
> >>>>
> >>>> Can returning EPROBE_DEFER fix this issue? Or use the 'complex
> >>>> dependencies' [1]?
> >>>
> >>> *_OF_DECLARE() is not compatible with EPROBE_DEFER, which causes
> >>> issues with complex dependencies.
> >>
> >> What causes issues ? Add support for EPROBE_DEFER with OF_DECLARE or not
> >> having support of it ?
> >
> > Driver init functions declared using *_OF_DECLARE() are called exactly once.
> > Hence if they fail, they are never retried. Calling order among subsystems is
> > hardcoded, and the actual order was determined by historical reasons (legacy
> > PC systems with always-on clocks and power domains ;-).
> > This breaks as soon as e.g. timers or interrupt controllers start depending
> > on clocks and/or power domains.
> >
> > Drivers using the device driver framework are retried later when their init
> > function returns -EPROBE_DEFER. So (eventually) they all succeed
> > initialization.
>
> Yeah, I got this point. But it is the meaning of your sentence: "...
> which causes issues with complex dependencies.".
>
> It is ambiguous *what* causes the issues.
>
> Did you meant an attempt was done to support EPROBE_DEFER with
> *_OF_DECLARE but caused too much issues with the complex dependencies?
>
> Or the current situation is causing too much issues with the complex
> dependencies?
>
> (I know the latter is true, it is about the meaning of the sentence).

I meant the latter.

AFAIK no attempt was done to support EPROBE_DEFER with *_OF_DECLARE.
IMHO it would be pointless, as it would be much easier to just switch to real
platform drivers.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Daniel Lezcano]

[Added Arnd Bergmann, Bartosz Golaszewski and Mark Brown]

On 30/08/2018 10:48, Geert Uytterhoeven wrote:
> Hi Daniel,

[ ... ]

>> Yeah, I got this point. But it is the meaning of your sentence: "...
>> which causes issues with complex dependencies.".
>>
>> It is ambiguous *what* causes the issues.
>>
>> Did you meant an attempt was done to support EPROBE_DEFER with
>> *_OF_DECLARE but caused too much issues with the complex dependencies?
>>
>> Or the current situation is causing too much issues with the complex
>> dependencies?
>>
>> (I know the latter is true, it is about the meaning of the sentence).
> 
> I meant the latter.
> 
> AFAIK no attempt was done to support EPROBE_DEFER with *_OF_DECLARE.
> IMHO it would be pointless, as it would be much easier to just switch to real
> platform drivers.

May be, may be not.

>From your point of view, the change is simple because it touches only a
single driver.

>From my point of view, the change implies a split in the approach while
I'm trying to unify the drivers little by little and there are hundred
of them.

It is not the first time we face this situation and Bartosz Golaszewski
has a similar problem [1].

We have all the frameworks we need to solve this properly but I would
like something we can propagate to all drivers (OF and !OF) so we end up
with unified code.

It is time we clearly state the dependency issues and we find a proper
way to solve it.




[1] https://lkml.org/lkml/2018/4/26/657

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Bartosz Golaszewski]

2018-08-30 11:16 GMT+02:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>
> [Added Arnd Bergmann, Bartosz Golaszewski and Mark Brown]
>
> On 30/08/2018 10:48, Geert Uytterhoeven wrote:
>> Hi Daniel,
>
> [ ... ]
>
>>> Yeah, I got this point. But it is the meaning of your sentence: "...
>>> which causes issues with complex dependencies.".
>>>
>>> It is ambiguous *what* causes the issues.
>>>
>>> Did you meant an attempt was done to support EPROBE_DEFER with
>>> *_OF_DECLARE but caused too much issues with the complex dependencies?
>>>
>>> Or the current situation is causing too much issues with the complex
>>> dependencies?
>>>
>>> (I know the latter is true, it is about the meaning of the sentence).
>>
>> I meant the latter.
>>
>> AFAIK no attempt was done to support EPROBE_DEFER with *_OF_DECLARE.
>> IMHO it would be pointless, as it would be much easier to just switch to real
>> platform drivers.
>
> May be, may be not.
>
> From your point of view, the change is simple because it touches only a
> single driver.
>
> From my point of view, the change implies a split in the approach while
> I'm trying to unify the drivers little by little and there are hundred
> of them.
>
> It is not the first time we face this situation and Bartosz Golaszewski
> has a similar problem [1].
>

Hi,

thanks for Cc'in me on that.

This was my latest proposal for early platform drivers:

    https://lkml.org/lkml/2018/5/11/488

I still intend on continuing this work, I just don't have the time right now.

Best regards,
Bartosz Golaszewski

> We have all the frameworks we need to solve this properly but I would
> like something we can propagate to all drivers (OF and !OF) so we end up
> with unified code.
>
> It is time we clearly state the dependency issues and we find a proper
> way to solve it.
>
>
>
>
> [1] https://lkml.org/lkml/2018/4/26/657
>
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

On Thursday, August 30, 2018, Daniel Lezcano wrote:
> > AFAIK no attempt was done to support EPROBE_DEFER with *_OF_DECLARE.
> > IMHO it would be pointless, as it would be much easier to just switch to
> real
> > platform drivers.
> 
> May be, may be not.
> 
> From your point of view, the change is simple because it touches only a
> single driver.
> 
> From my point of view, the change implies a split in the approach while
> I'm trying to unify the drivers little by little and there are hundred
> of them.
> 
> It is not the first time we face this situation and Bartosz Golaszewski
> has a similar problem [1].
> 
> We have all the frameworks we need to solve this properly but I would
> like something we can propagate to all drivers (OF and !OF) so we end up
> with unified code.
> 
> It is time we clearly state the dependency issues and we find a proper
> way to solve it.


On Thursday, August 30, 2018, Bartosz Golaszewski wrote:
> This was my latest proposal for early platform drivers:
> 
>     https://lkml.org/lkml/2018/5/11/488
> 
> I still intend on continuing this work, I just don't have the time right
> now.

Daniel,

So what is your final thought on this?

The current OSTM driver uses TIMER_OF_DECLARE and that basically means 
it will never work with my new SoC.

For now, can I change the driver to register a standard platform driver 
in subsys_initcall like the other Renesas timer drivers?

Or do I have to live without a timer in my system for the unseeable 
future?

If there every becomes a fix for this resource dependence, I'll be happy
to modify the OSTM driver to comply. But at the moment, I'm stuck with 
nothing.

Thanks,
Chris

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Rob Herring]

On Fri, Sep 7, 2018 at 10:16 AM Chris Brandt <Chris.Brandt@renesas.com> wrote:
>
> On Thursday, August 30, 2018, Daniel Lezcano wrote:
> > > AFAIK no attempt was done to support EPROBE_DEFER with *_OF_DECLARE.
> > > IMHO it would be pointless, as it would be much easier to just switch to
> > real
> > > platform drivers.
> >
> > May be, may be not.
> >
> > From your point of view, the change is simple because it touches only a
> > single driver.
> >
> > From my point of view, the change implies a split in the approach while
> > I'm trying to unify the drivers little by little and there are hundred
> > of them.
> >
> > It is not the first time we face this situation and Bartosz Golaszewski
> > has a similar problem [1].
> >
> > We have all the frameworks we need to solve this properly but I would
> > like something we can propagate to all drivers (OF and !OF) so we end up
> > with unified code.
> >
> > It is time we clearly state the dependency issues and we find a proper
> > way to solve it.
>
>
> On Thursday, August 30, 2018, Bartosz Golaszewski wrote:
> > This was my latest proposal for early platform drivers:
> >
> >     https://lkml.org/lkml/2018/5/11/488
> >
> > I still intend on continuing this work, I just don't have the time right
> > now.
>
> Daniel,
>
> So what is your final thought on this?
>
> The current OSTM driver uses TIMER_OF_DECLARE and that basically means
> it will never work with my new SoC.
>
> For now, can I change the driver to register a standard platform driver
> in subsys_initcall like the other Renesas timer drivers?

I'm confused how this can even work as an initcall. The whole reason
*_OF_DECLARE exists is for things that have to be setup before
initcalls.

Rob

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

On Monday, September 10, 2018, Rob Herring wrote:
> > The current OSTM driver uses TIMER_OF_DECLARE and that basically means
> > it will never work with my new SoC.
> >
> > For now, can I change the driver to register a standard platform driver
> > in subsys_initcall like the other Renesas timer drivers?
> 
> I'm confused how this can even work as an initcall. The whole reason
> *_OF_DECLARE exists is for things that have to be setup before
> initcalls.

I wrote a long explanation of the issue, but the summary is:

The timer (which is currently using TIMER_OF_DECLARE) can't start up 
until the clocks are set up because of_clk_get fails().

But, the clock driver is a platform driver that is not started until 
subsys_initcall.

So, unless you have a clock driver with CLK_OF_DECLARE, you can't use
a timer driver with a TIMER_OF_DECLARE driver.

And, there is no such thing as a deferred probe for timer drivers 
declared with IMER_OF_DECLARE.

Chris

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Rob Herring]

On Mon, Sep 10, 2018 at 12:20 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
>
> On Monday, September 10, 2018, Rob Herring wrote:
> > > The current OSTM driver uses TIMER_OF_DECLARE and that basically means
> > > it will never work with my new SoC.
> > >
> > > For now, can I change the driver to register a standard platform driver
> > > in subsys_initcall like the other Renesas timer drivers?
> >
> > I'm confused how this can even work as an initcall. The whole reason
> > *_OF_DECLARE exists is for things that have to be setup before
> > initcalls.
>
> I wrote a long explanation of the issue, but the summary is:
>
> The timer (which is currently using TIMER_OF_DECLARE) can't start up
> until the clocks are set up because of_clk_get fails().
>
> But, the clock driver is a platform driver that is not started until
> subsys_initcall.
>
> So, unless you have a clock driver with CLK_OF_DECLARE, you can't use
> a timer driver with a TIMER_OF_DECLARE driver.
>
> And, there is no such thing as a deferred probe for timer drivers
> declared with IMER_OF_DECLARE.

Yes, I read the thread and understand all of this part.

Well before we get to initcalls, the kernel calls the arch specific
time_init() which (on ARM) calls of_clk_init (for all the reasons
above) and then timer_probe(). When timer_probe returns, it is
expected that you have setup a clocksource and clockevent. If you
haven't, then at some point before we get to initcalls we should hang
because we're not getting any timer interrupts and time is not
advancing. At least that's how it used to be and maybe something has
changed (It's been a while since I've looked at this area). Maybe you
just get lucky and it works as long as no thread blocks (e.g. on a
msleep). If things changed and you can setup a timer in an initcall,
then why are folks still trying to do things like early platform
drivers. Regular drivers would work and we should be able to
completely remove CLK_OF_DECLARE and TIMER_OF_DECLARE.

Rob

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

On Tuesday, September 11, 2018 1, Rob Herring wrote:
> Well before we get to initcalls, the kernel calls the arch specific
> time_init() which (on ARM) calls of_clk_init (for all the reasons
> above) and then timer_probe(). When timer_probe returns, it is
> expected that you have setup a clocksource and clockevent. If you
> haven't, then at some point before we get to initcalls we should hang
> because we're not getting any timer interrupts and time is not
> advancing.

What I get now is:

[    0.000000] timer_probe: no matching timers found
...
...
 [    0.000000] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
...
...


But then later on in boot, I'll get (with my subsys_initcall timer fix)

...
...
[    0.000000] SCSI subsystem initialized
[    0.000000] usbcore: registered new interface driver usbfs
[    0.000000] usbcore: registered new interface driver hub
[    0.000000] usbcore: registered new device driver usb
[    0.000000] clocksource: ostm: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 28958491609 ns
[    0.000619] sched_clock: 32 bits at 66MHz, resolution 15ns, wraps every 32537631224ns
[    0.008599] ostm: used for clocksource
[    0.018926] ostm: used for clock events
[    0.133339] clocksource: Switched to clocksource ostm
[    0.821474] NET: Registered protocol family 2
[    0.840624] tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 4096 bytes)
[    0.850549] TCP established hash table entries: 1024 (order: 0, 4096 bytes)
...
...



> Maybe you
> just get lucky and it works as long as no thread blocks (e.g. on a
> msleep).

You're right. If I put in a msleep() before my timer is up and running, it hangs.

As far as I can tell, nothing before device_initcall seems to call anything like msleep.

> If things changed and you can setup a timer in an initcall,
> then why are folks still trying to do things like early platform
> drivers. Regular drivers would work and we should be able to
> completely remove CLK_OF_DECLARE and TIMER_OF_DECLARE.

I wonder if the reason is because you can't assign a priority to your 
driver when you declare it with xxx_initcall( ). So, your driver ends up 
in the same table as all the other drivers and you are not guaranteed the
order in which they probe. So, the answer was to make a NEW table and 
register it using TIMER_OF_DECLARE and CLOCK_OF_DECLARE.

I wonder they just didn't make a clock_initcall() and timer_initcall() 
instead.

Chris

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Daniel Lezcano]

On 11/09/2018 20:42, Chris Brandt wrote:
> On Tuesday, September 11, 2018 1, Rob Herring wrote:
>> Well before we get to initcalls, the kernel calls the arch specific
>> time_init() which (on ARM) calls of_clk_init (for all the reasons
>> above) and then timer_probe(). When timer_probe returns, it is
>> expected that you have setup a clocksource and clockevent. If you
>> haven't, then at some point before we get to initcalls we should hang
>> because we're not getting any timer interrupts and time is not
>> advancing.
> 
> What I get now is:
> 
> [    0.000000] timer_probe: no matching timers found
> ...
> ...
>  [    0.000000] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
> ...
> ...
> 
> 
> But then later on in boot, I'll get (with my subsys_initcall timer fix)
> 
> ...
> ...
> [    0.000000] SCSI subsystem initialized
> [    0.000000] usbcore: registered new interface driver usbfs
> [    0.000000] usbcore: registered new interface driver hub
> [    0.000000] usbcore: registered new device driver usb
> [    0.000000] clocksource: ostm: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 28958491609 ns
> [    0.000619] sched_clock: 32 bits at 66MHz, resolution 15ns, wraps every 32537631224ns
> [    0.008599] ostm: used for clocksource
> [    0.018926] ostm: used for clock events
> [    0.133339] clocksource: Switched to clocksource ostm
> [    0.821474] NET: Registered protocol family 2
> [    0.840624] tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 4096 bytes)
> [    0.850549] TCP established hash table entries: 1024 (order: 0, 4096 bytes)
> ...
> ...
> 
> 
> 
>> Maybe you
>> just get lucky and it works as long as no thread blocks (e.g. on a
>> msleep).
> 
> You're right. If I put in a msleep() before my timer is up and running, it hangs.
> 
> As far as I can tell, nothing before device_initcall seems to call anything like msleep.
> 
>> If things changed and you can setup a timer in an initcall,
>> then why are folks still trying to do things like early platform
>> drivers. Regular drivers would work and we should be able to
>> completely remove CLK_OF_DECLARE and TIMER_OF_DECLARE.
> 
> I wonder if the reason is because you can't assign a priority to your 
> driver when you declare it with xxx_initcall( ). So, your driver ends up 
> in the same table as all the other drivers and you are not guaranteed the
> order in which they probe. So, the answer was to make a NEW table and 
> register it using TIMER_OF_DECLARE and CLOCK_OF_DECLARE.
> 
> I wonder they just didn't make a clock_initcall() and timer_initcall() 
> instead.

What happens if you place the clk_init() before board_time_init() ? in
arch/sh/kernel/time.c

void __init time_init(void)
{
        if (board_time_init)
                board_time_init();

        clk_init();

        late_time_init = sh_late_time_init;
}



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Geert Uytterhoeven]

Hi Daniel,

On Thu, Sep 13, 2018 at 3:17 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 11/09/2018 20:42, Chris Brandt wrote:
> > On Tuesday, September 11, 2018 1, Rob Herring wrote:
> >> Well before we get to initcalls, the kernel calls the arch specific
> >> time_init() which (on ARM) calls of_clk_init (for all the reasons
> >> above) and then timer_probe(). When timer_probe returns, it is
> >> expected that you have setup a clocksource and clockevent. If you
> >> haven't, then at some point before we get to initcalls we should hang
> >> because we're not getting any timer interrupts and time is not
> >> advancing.
> >
> > What I get now is:
> >
> > [    0.000000] timer_probe: no matching timers found
> > ...
> > ...
> >  [    0.000000] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
> > ...
> > ...
> >
> >
> > But then later on in boot, I'll get (with my subsys_initcall timer fix)
> >
> > ...
> > ...
> > [    0.000000] SCSI subsystem initialized
> > [    0.000000] usbcore: registered new interface driver usbfs
> > [    0.000000] usbcore: registered new interface driver hub
> > [    0.000000] usbcore: registered new device driver usb
> > [    0.000000] clocksource: ostm: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 28958491609 ns
> > [    0.000619] sched_clock: 32 bits at 66MHz, resolution 15ns, wraps every 32537631224ns
> > [    0.008599] ostm: used for clocksource
> > [    0.018926] ostm: used for clock events
> > [    0.133339] clocksource: Switched to clocksource ostm
> > [    0.821474] NET: Registered protocol family 2
> > [    0.840624] tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 4096 bytes)
> > [    0.850549] TCP established hash table entries: 1024 (order: 0, 4096 bytes)
> > ...
> > ...
> >
> >
> >
> >> Maybe you
> >> just get lucky and it works as long as no thread blocks (e.g. on a
> >> msleep).
> >
> > You're right. If I put in a msleep() before my timer is up and running, it hangs.
> >
> > As far as I can tell, nothing before device_initcall seems to call anything like msleep.
> >
> >> If things changed and you can setup a timer in an initcall,
> >> then why are folks still trying to do things like early platform
> >> drivers. Regular drivers would work and we should be able to
> >> completely remove CLK_OF_DECLARE and TIMER_OF_DECLARE.
> >
> > I wonder if the reason is because you can't assign a priority to your
> > driver when you declare it with xxx_initcall( ). So, your driver ends up
> > in the same table as all the other drivers and you are not guaranteed the
> > order in which they probe. So, the answer was to make a NEW table and
> > register it using TIMER_OF_DECLARE and CLOCK_OF_DECLARE.
> >
> > I wonder they just didn't make a clock_initcall() and timer_initcall()
> > instead.
>
> What happens if you place the clk_init() before board_time_init() ? in
> arch/sh/kernel/time.c

Nothing, as Chris is using an ARM platform ;-)

The clock driver is drivers/clk/renesas/renesas-cpg-mssr.c, which is a
platform_driver registered from subsys_initcall().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

On Thursday, September 13, 2018, Geert Uytterhoeven wrote:
> > > I wonder they just didn't make a clock_initcall() and timer_initcall()
> > > instead.
> >
> > What happens if you place the clk_init() before board_time_init() ? in
> > arch/sh/kernel/time.c
> 
> Nothing, as Chris is using an ARM platform ;-)
> 
> The clock driver is drivers/clk/renesas/renesas-cpg-mssr.c, which is a
> platform_driver registered from subsys_initcall().

Just FYI, for the heck of it, I tried and hacked in registering the 
clock driver using CLK_OF_DECLARE since that happens before the 
TIMER_OF_DECLARE timers are probed.

But, I got this result:

[    0.000000] Driver 'renesas-cpg-mssr' was unable to register with bus_type 'platform' because the bus was not initialized.


Chris

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Geert Uytterhoeven]

Hi Chris,

On Thu, Sep 13, 2018 at 4:54 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Thursday, September 13, 2018, Geert Uytterhoeven wrote:
> > > > I wonder they just didn't make a clock_initcall() and timer_initcall()
> > > > instead.
> > >
> > > What happens if you place the clk_init() before board_time_init() ? in
> > > arch/sh/kernel/time.c
> >
> > Nothing, as Chris is using an ARM platform ;-)
> >
> > The clock driver is drivers/clk/renesas/renesas-cpg-mssr.c, which is a
> > platform_driver registered from subsys_initcall().
>
> Just FYI, for the heck of it, I tried and hacked in registering the
> clock driver using CLK_OF_DECLARE since that happens before the
> TIMER_OF_DECLARE timers are probed.
>
> But, I got this result:
>
> [    0.000000] Driver 'renesas-cpg-mssr' was unable to register with bus_type 'platform' because the bus was not initialized.

Indeed, you cannot register a platform device from CLK_OF_DECLARE().
Instead, you have to operate on the passed struct device_node pointer,
cfr. the old RZ/A1 clock driver.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

On Friday, September 14, 2018, Geert Uytterhoeven wrote:
> > Just FYI, for the heck of it, I tried and hacked in registering the
> > clock driver using CLK_OF_DECLARE since that happens before the
> > TIMER_OF_DECLARE timers are probed.
> >
> > But, I got this result:
> >
> > [    0.000000] Driver 'renesas-cpg-mssr' was unable to register with
> bus_type 'platform' because the bus was not initialized.
> 
> Indeed, you cannot register a platform device from CLK_OF_DECLARE().
> Instead, you have to operate on the passed struct device_node pointer,
> cfr. the old RZ/A1 clock driver.


How about this proposal:

I leave the current OSTM timer driver as it is today with 
TIMER_OF_DECLARE.

But, I modify the clock driver so it registers a mini driver with 
CLK_OF_DECLARE that can enable individual HW module clocks using 
clk_register_fixed_rate. Once those modules/clocks are enabled, they are enabled 
forever.

Also, later on when the full platform driver is probed, for any of those
early clocks that were created, it basically ignores them.


To use this early clock, you add this to your board's .dts file as such:


/* Special Early CPG clocks */
/ {
		cpg_early: clock-controller@early {
		#clock-cells = <2>;
		compatible = "renesas,r7s9210-cpg-mssr-early";
	};
};
	
/* High resolution System tick timers */
&ostm0 {
	status = "okay";
	clocks = <&cpg_early CPG_MOD 36>;	/* replace .dtsi setting */
	power-domains = <&cpg_early>;		/* replace .dtsi setting */
};
&ostm1 {
	status = "okay";
	clocks = <&cpg_early CPG_MOD 35>;	/* replace .dtsi setting */
	power-domains = <&cpg_early>;		/* replace .dtsi setting */
};


I've coded this up and it works fine.

Note that instead of duplicating DT node entries, the driver simply 
references the full "renesas,r7s9210-cpg-mssr" node for things such as 
parent clock and register location.


Chris

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Geert Uytterhoeven]

Hi Chris,

CC linux-clk

On Mon, Sep 17, 2018 at 8:57 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, September 14, 2018, Geert Uytterhoeven wrote:
> > > Just FYI, for the heck of it, I tried and hacked in registering the
> > > clock driver using CLK_OF_DECLARE since that happens before the
> > > TIMER_OF_DECLARE timers are probed.
> > >
> > > But, I got this result:
> > >
> > > [    0.000000] Driver 'renesas-cpg-mssr' was unable to register with
> > bus_type 'platform' because the bus was not initialized.
> >
> > Indeed, you cannot register a platform device from CLK_OF_DECLARE().
> > Instead, you have to operate on the passed struct device_node pointer,
> > cfr. the old RZ/A1 clock driver.
>
> How about this proposal:
>
> I leave the current OSTM timer driver as it is today with
> TIMER_OF_DECLARE.
>
> But, I modify the clock driver so it registers a mini driver with
> CLK_OF_DECLARE that can enable individual HW module clocks using
> clk_register_fixed_rate. Once those modules/clocks are enabled, they are enabled
> forever.
>
> Also, later on when the full platform driver is probed, for any of those
> early clocks that were created, it basically ignores them.
>
>
> To use this early clock, you add this to your board's .dts file as such:
>
>
> /* Special Early CPG clocks */
> / {
>                 cpg_early: clock-controller@early {
>                 #clock-cells = <2>;
>                 compatible = "renesas,r7s9210-cpg-mssr-early";
>         };
> };
>
> /* High resolution System tick timers */
> &ostm0 {
>         status = "okay";
>         clocks = <&cpg_early CPG_MOD 36>;       /* replace .dtsi setting */
>         power-domains = <&cpg_early>;           /* replace .dtsi setting */
> };
> &ostm1 {
>         status = "okay";
>         clocks = <&cpg_early CPG_MOD 35>;       /* replace .dtsi setting */
>         power-domains = <&cpg_early>;           /* replace .dtsi setting */
> };
>
>
> I've coded this up and it works fine.

While I don't doubt this works fine, your DT is no longer describing
hardware, but also software policy.

I think the proper solution, maximizing code reuse, is to:
  - Split off early clocks from cpg_mssr_info.core_clks[] and .mod_clk[] into
    cpg_mssr_info.early_core_clks[] and .early_mod_clks[],
  - Split off early handling from cpg_mssr_probe(), to be called
      a. from CLK_OF_DECLARE() in r7s9210-cpg-mssr.c, OR
      b. from cpg_mssr_probe() if !cpg_mssr_info.early_core_clks.

BTW, this will also be needed for migrating other CA9-based SoCs to
renesas-cpg-mssr.c, as these don't have an ARM architectured timer,
just like RZ/A2.

Ideally (in the long term), Linux should learn to track dependencies properly,
so it initialized all components in the required order, automatically.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

Hi Geert,

On Tuesday, September 18, 2018, linux-renesas-soc-owner@vger.kernel.org wrote:
> > I've coded this up and it works fine.
> 
> While I don't doubt this works fine, your DT is no longer describing
> hardware, but also software policy.
> 
> I think the proper solution, maximizing code reuse, is to:
>   - Split off early clocks from cpg_mssr_info.core_clks[] and .mod_clk[]
> into
>     cpg_mssr_info.early_core_clks[] and .early_mod_clks[],

This is where I got into trouble.
I originally just tried to register all the core clocks in the early 
init. But then I had issues when the platform probe came in later and 
wanted to do the same thing.

For example, the clock tree for OSTM is:
  EXTAL -> PLL -> P1C -> OSTM

Of course there are other non-early module that use the P1C clock.

Do you think it would be OK if I just registers all the core clock in 
early init, then just pass back the clk pointers to cpg_mssr_probe later 
(to let the platform driver manage them)?


Chris

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Geert Uytterhoeven]

Hi Chris,

On Tue, Sep 18, 2018 at 1:55 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tuesday, September 18, 2018, linux-renesas-soc-owner@vger.kernel.org wrote:
> > > I've coded this up and it works fine.
> >
> > While I don't doubt this works fine, your DT is no longer describing
> > hardware, but also software policy.
> >
> > I think the proper solution, maximizing code reuse, is to:
> >   - Split off early clocks from cpg_mssr_info.core_clks[] and .mod_clk[]
> > into
> >     cpg_mssr_info.early_core_clks[] and .early_mod_clks[],
>
> This is where I got into trouble.
> I originally just tried to register all the core clocks in the early
> init. But then I had issues when the platform probe came in later and
> wanted to do the same thing.
>
> For example, the clock tree for OSTM is:
>   EXTAL -> PLL -> P1C -> OSTM
>
> Of course there are other non-early module that use the P1C clock.
>
> Do you think it would be OK if I just registers all the core clock in
> early init, then just pass back the clk pointers to cpg_mssr_probe later
> (to let the platform driver manage them)?

Just move EXTAL, PLL, and P1C from cpg_mssr_info.core_clks[] to
.early_core_clks[], and move OSTM[01] from .mod_clks[] to
.early_mod_clks[]?

Then the early init from CLK_OF_DECLARE() will just register the
early clocks, and cpg_mssr_probe() can take care of the remaining parts?

Does that make sense?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

Hi Geert,

On Tuesday, September 18, 2018, Geert Uytterhoeven wrote:
> Then the early init from CLK_OF_DECLARE() will just register the
> early clocks, and cpg_mssr_probe() can take care of the remaining parts?

What is not clear to me is what goes in DT????

I will have this in .dtsi for cpg_mssr_probe():

	cpg: clock-controller@fcfe0020 {
		compatible = "renesas,r7s9210-cpg-mssr";
		reg = <0xfcfe0010 0x455>;  /* ----------- FCFE0010 - FCFE0465 */
		clocks = <&extal_clk>;
		clock-names = "extal";


But, I also need /something/ for CLK_OF_DECLARE().
That means a second DT node (which means 2 different devices, 2 
different drivers)

What do you see the .dtsi and .dts looking like?

Thanks,
Chris

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Geert Uytterhoeven]

Hi Chris,

On Tue, Sep 18, 2018 at 5:04 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tuesday, September 18, 2018, Geert Uytterhoeven wrote:
> > Then the early init from CLK_OF_DECLARE() will just register the
> > early clocks, and cpg_mssr_probe() can take care of the remaining parts?
>
> What is not clear to me is what goes in DT????
>
> I will have this in .dtsi for cpg_mssr_probe():
>
>         cpg: clock-controller@fcfe0020 {
>                 compatible = "renesas,r7s9210-cpg-mssr";
>                 reg = <0xfcfe0010 0x455>;  /* ----------- FCFE0010 - FCFE0465 */
>                 clocks = <&extal_clk>;
>                 clock-names = "extal";
>
>
> But, I also need /something/ for CLK_OF_DECLARE().
> That means a second DT node (which means 2 different devices, 2
> different drivers)
>
> What do you see the .dtsi and .dts looking like?

The part using CLK_OF_DECLARE() is not a platform driver. It does not
operate on a device (struct platform_device), but on a device node (struct
device_node). Hence it would match against the same DT node, but map it
using of_iomap().  So you just need the existing "renesas,r7s9210-cpg-mssr"
node.

Please have a look at e.g. "mediatek,mt2712-topckgen".

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

Hi Geert,

On Tuesday, September 18, 2018 1, Geert Uytterhoeven wrote:
> > What do you see the .dtsi and .dts looking like?
> 
> The part using CLK_OF_DECLARE() is not a platform driver. It does not
> operate on a device (struct platform_device), but on a device node
> (struct
> device_node). Hence it would match against the same DT node, but map it
> using of_iomap().  So you just need the existing "renesas,r7s9210-cpg-
> mssr"
> node.

So...I tried that...and it doesn't work.

Basically, this:
CLK_OF_DECLARE(cpg_mstp_early_clks, "renesas,r7s9210-cpg-mssr",
               rza2_cpg_mssr_early_init);

But, what happens is that rza2_cpg_mssr_early_init gets called because 
it find a match against "renesas,r7s9210-cpg-mssr". But later after 
cpg_mssr_init gets call, cpg_mssr_probe never gets called. I assume that is 
because device "renesas,r7s9210-cpg-mssr" has already been matched to a 
driver.


> Please have a look at e.g. "mediatek,mt2712-topckgen".

One thing I don't understand is that in the early init, it registers a 
of_clk_add_provider. But then later in the probe, it register 
of_clk_add_provider again (on the same DT node). I guess you can do that????


So I see what the mediatek is doing, but I can't seem to reproduce it. I
must be missing something.

Chris

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Geert Uytterhoeven]

Hi Chris,

On Tue, Sep 18, 2018 at 5:52 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tuesday, September 18, 2018 1, Geert Uytterhoeven wrote:
> > > What do you see the .dtsi and .dts looking like?
> >
> > The part using CLK_OF_DECLARE() is not a platform driver. It does not
> > operate on a device (struct platform_device), but on a device node
> > (struct
> > device_node). Hence it would match against the same DT node, but map it
> > using of_iomap().  So you just need the existing "renesas,r7s9210-cpg-
> > mssr"
> > node.
>
> So...I tried that...and it doesn't work.
>
> Basically, this:
> CLK_OF_DECLARE(cpg_mstp_early_clks, "renesas,r7s9210-cpg-mssr",
>                rza2_cpg_mssr_early_init);
>
> But, what happens is that rza2_cpg_mssr_early_init gets called because
> it find a match against "renesas,r7s9210-cpg-mssr". But later after
> cpg_mssr_init gets call, cpg_mssr_probe never gets called. I assume that is
> because device "renesas,r7s9210-cpg-mssr" has already been matched to a
> driver.
>
> > Please have a look at e.g. "mediatek,mt2712-topckgen".
>
> One thing I don't understand is that in the early init, it registers a
> of_clk_add_provider. But then later in the probe, it register
> of_clk_add_provider again (on the same DT node). I guess you can do that????

Yeah, I noticed that, too.
It just adds the same xlate method and data pointer to the list.
So it's harmless, but unneeded.

> So I see what the mediatek is doing, but I can't seem to reproduce it. I
> must be missing something.

It's using CLK_OF_DECLARE_DRIVER(), which clears OF_POPULATED:

    /*
     * Use this macro when you have a driver that requires two initialization
     * routines, one at of_clk_init(), and one at platform device probe
     */
    #define CLK_OF_DECLARE_DRIVER(name, compat, fn) \
            static void __init name##_of_clk_init_driver(struct
device_node *np) \
            {                                                               \
                    of_node_clear_flag(np, OF_POPULATED);                   \
                    fn(np);                                                 \
            }                                                               \
            OF_DECLARE_1(clk, name, compat, name##_of_clk_init_driver)

Sorry for failing to tell you. I did know about that flag, but only remembered
due to your problem report.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Chris Brandt]

Hi Geert,

On Tuesday, September 18, 2018 1, Geert Uytterhoeven wrote:
> > So I see what the mediatek is doing, but I can't seem to reproduce it.
> I
> > must be missing something.
> 
> It's using CLK_OF_DECLARE_DRIVER(), which clears OF_POPULATED:

Yup, that's what I was missing.
Works now.
Thanks!

So at this point, I guess I'll move away from patching the timer driver 
and go back to focusing on the clock driver.


Chris

[PATCH 2/2] dt-bindings: timer: ostm: Add R7S9210 support

[Chris Brandt]

The R7S9210 belongs to the RZ/A2 SoC series

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 Documentation/devicetree/bindings/timer/renesas,ostm.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/timer/renesas,ostm.txt b/Documentation/devicetree/bindings/timer/renesas,ostm.txt
index be3ae0fdf775..81a78f8bcf17 100644
--- a/Documentation/devicetree/bindings/timer/renesas,ostm.txt
+++ b/Documentation/devicetree/bindings/timer/renesas,ostm.txt
@@ -9,7 +9,8 @@ Channels are independent from each other.
 Required Properties:
 
   - compatible: must be one or more of the following:
-    - "renesas,r7s72100-ostm" for the r7s72100 OSTM
+    - "renesas,r7s72100-ostm" for the R7S72100 (RZ/A1) OSTM
+    - "renesas,r7s9210-ostm" for the R7S9210 (RZ/A2) OSTM
     - "renesas,ostm" for any OSTM
 		This is a fallback for the above renesas,*-ostm entries
 
-- 
2.16.1