| Message ID | 20180829132954.64862-2-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | clocksource: ostm: Add support for RZ/A2 | expand |
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/
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
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
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
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
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
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.
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
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).
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
[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
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 >
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
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
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
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
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
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; }
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
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
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
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
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
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
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
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
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
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
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
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
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);
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(-)