* CLK_OF_DECLARE advice required @ 2017-05-30 12:23 Phil Elwell 2017-05-31 15:27 ` Stephen Warren 0 siblings, 1 reply; 13+ messages in thread From: Phil Elwell @ 2017-05-30 12:23 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd, linux-clk, linux-rpi-kernel, linux-kernel, Eric Anholt Hi, I've run into a problem using the fixed-factor clock on Raspberry Pi and I'd like some advice before I submit a patch. Some context: the aim is to use a standard UART and some external circuitry as a MIDI interface. This would be straightforward except that Linux doesn't recognise the required 31.25KHz as a valid UART baud rate. Rhe workaround is to declare the UART clock such that the reported rate differs from the actual rate. If one sets the reported rate to be (actual*38400)/31250 then requesting a 38400 baud rate will result in an actual 31250 baud signal. The fixed-factor-clock device is perfect for such a deception, but when one is inserted between the clock source and the UART the probe function of the UART always returns -EPROBE_DEFER because its input clock isn't available. This is due to an initialisation order problem that requires code changes to resolve. fixed-factor-clock has two ways to initialise and register its clock. One is via a standard module probe method, and the other is through the CLK_OF_DECLARE macro, adding a registration to the __clk_of_table section. of_clk_init walks the list of clocks, calling the initialisation functions for nodes that either have no parent (input) clock or where the parent clock is ready. The init function itself has no return value, so is assumed to succeed. In the event that the parent never becomes ready, the initialisation function is called anyway and the clock node is marked as being populated, preventing the probe function from ever being called. In this MIDI application using UART 1 on a Raspberry Pi, the parent clock is provided by the clk-bcm2835-aux driver which registers using a probe function. Because regular platform drivers are probed long after of_clk_init returns, the parent clock for the fixed-factor clock doesn't become available early enough, resulting in a failing call to the initialisation and no subsequent probe, even after the parent finally becomes available. My questions are: 1. Should all system clock drivers use OF_CLK_DECLARE? Doing so would probably avoid this problem, but further initialisation order dependencies may require more drivers to be initialised early. 2. Why does the clock initialisation hook registered by OF_CLK_DECLARE not return any indication of success? If it did, and if the OF_POPULATED flag was only set after successful initialisation then the normal retrying of a deferred probe would also avoid this problem. 3. Would adding the OF_CLK_DECLARE hook prevent the use of the devm_ managed functions like devm_kzalloc? If not, why doesn't fixed-factor-clock use it? Thank you for your time, Phil ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-05-30 12:23 CLK_OF_DECLARE advice required Phil Elwell @ 2017-05-31 15:27 ` Stephen Warren 2017-05-31 15:58 ` Stefan Wahren 0 siblings, 1 reply; 13+ messages in thread From: Stephen Warren @ 2017-05-31 15:27 UTC (permalink / raw) To: Phil Elwell Cc: Michael Turquette, Stephen Boyd, linux-clk, linux-rpi-kernel, linux-kernel, Eric Anholt On 05/30/2017 06:23 AM, Phil Elwell wrote: > Hi, > > I've run into a problem using the fixed-factor clock on Raspberry Pi and I'd > like some advice before I submit a patch. > > Some context: the aim is to use a standard UART and some external circuitry > as a MIDI interface. This would be straightforward except that Linux doesn't > recognise the required 31.25KHz as a valid UART baud rate. Rhe workaround is > to declare the UART clock such that the reported rate differs from the actual > rate. If one sets the reported rate to be (actual*38400)/31250 then > requesting a 38400 baud rate will result in an actual 31250 baud signal. This sounds like the wrong approach. Forcing the port to use a different clock rate than the user requests would prevent anyone from using that port for its standard purpose; it'd turn what should be a runtime decision into a compile-time decision. Are you sure there's no way to simply select the correct baud rate on the port? I see plenty of references to configuring custom baud rates under Linux when I search Google, e.g.: > https://stackoverflow.com/questions/12646324/how-to-set-a-custom-baud-rate-on-linux "How to set a custom baud rate on Linux?" > https://sourceware.org/ml/libc-help/2009-06/msg00016.html "Re: Terminal interface and non-standard baudrates" ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-05-31 15:27 ` Stephen Warren @ 2017-05-31 15:58 ` Stefan Wahren 2017-05-31 16:28 ` Phil Elwell 0 siblings, 1 reply; 13+ messages in thread From: Stefan Wahren @ 2017-05-31 15:58 UTC (permalink / raw) To: Stephen Warren, Phil Elwell Cc: Michael Turquette, Stephen Boyd, linux-kernel, linux-rpi-kernel, linux-clk Am 31.05.2017 um 17:27 schrieb Stephen Warren: > On 05/30/2017 06:23 AM, Phil Elwell wrote: >> Hi, >> >> I've run into a problem using the fixed-factor clock on Raspberry Pi >> and I'd >> like some advice before I submit a patch. >> >> Some context: the aim is to use a standard UART and some external >> circuitry >> as a MIDI interface. This would be straightforward except that Linux >> doesn't >> recognise the required 31.25KHz as a valid UART baud rate. Rhe >> workaround is >> to declare the UART clock such that the reported rate differs from >> the actual >> rate. If one sets the reported rate to be (actual*38400)/31250 then >> requesting a 38400 baud rate will result in an actual 31250 baud signal. > > This sounds like the wrong approach. Forcing the port to use a > different clock rate than the user requests would prevent anyone from > using that port for its standard purpose; it'd turn what should be a > runtime decision into a compile-time decision. > > Are you sure there's no way to simply select the correct baud rate on > the port? I see plenty of references to configuring custom baud rates > under Linux when I search Google, e.g.: > >> https://stackoverflow.com/questions/12646324/how-to-set-a-custom-baud-rate-on-linux >> > "How to set a custom baud rate on Linux?" > >> https://sourceware.org/ml/libc-help/2009-06/msg00016.html > "Re: Terminal interface and non-standard baudrates" > I remember this gist from Peter Hurley: https://gist.github.com/peterhurley/fbace59b55d87306a5b8 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-05-31 15:58 ` Stefan Wahren @ 2017-05-31 16:28 ` Phil Elwell 2017-05-31 16:47 ` Stephen Warren 2017-06-01 6:39 ` Stephen Boyd 0 siblings, 2 replies; 13+ messages in thread From: Phil Elwell @ 2017-05-31 16:28 UTC (permalink / raw) To: Stefan Wahren, Stephen Warren Cc: Michael Turquette, Stephen Boyd, linux-kernel, linux-rpi-kernel, linux-clk On 31/05/2017 16:58, Stefan Wahren wrote: > Am 31.05.2017 um 17:27 schrieb Stephen Warren: >> On 05/30/2017 06:23 AM, Phil Elwell wrote: >>> Hi, >>> >>> I've run into a problem using the fixed-factor clock on Raspberry Pi >>> and I'd >>> like some advice before I submit a patch. >>> >>> Some context: the aim is to use a standard UART and some external >>> circuitry >>> as a MIDI interface. This would be straightforward except that Linux >>> doesn't >>> recognise the required 31.25KHz as a valid UART baud rate. Rhe >>> workaround is >>> to declare the UART clock such that the reported rate differs from >>> the actual >>> rate. If one sets the reported rate to be (actual*38400)/31250 then >>> requesting a 38400 baud rate will result in an actual 31250 baud signal. >> >> This sounds like the wrong approach. Forcing the port to use a >> different clock rate than the user requests would prevent anyone from >> using that port for its standard purpose; it'd turn what should be a >> runtime decision into a compile-time decision. >> >> Are you sure there's no way to simply select the correct baud rate on >> the port? I see plenty of references to configuring custom baud rates >> under Linux when I search Google, e.g.: >> >>> https://stackoverflow.com/questions/12646324/how-to-set-a-custom-baud-rate-on-linux >>> >> "How to set a custom baud rate on Linux?" >> >>> https://sourceware.org/ml/libc-help/2009-06/msg00016.html >> "Re: Terminal interface and non-standard baudrates" >> > > I remember this gist from Peter Hurley: > > https://gist.github.com/peterhurley/fbace59b55d87306a5b8 Thank you, Stephen and Stephan. Stephen - the clock scaling is applied by a DT overlay so it effectively a runtime setting, but I take your point about the elegance of the solution. Stefan - anybaud looks promising, although I would have preferred for users to continue to use the existing user-space tools - kernel changes can be deployed more easily. For my edification, can you pretend for a moment that the application was a valid one and answer any of my original questions?: 1. Should all system clock drivers use OF_CLK_DECLARE? Doing so would probably avoid this problem, but further initialisation order dependencies may require more drivers to be initialised early. 2. Why does the clock initialisation hook registered by OF_CLK_DECLARE not return any indication of success? If it did, and if the OF_POPULATED flag was only set after successful initialisation then the normal retrying of a deferred probe would also avoid this problem. 3. Would adding the OF_CLK_DECLARE hook prevent the use of the devm_ managed functions like devm_kzalloc? If not, why doesn't fixed-factor-clock use it? Thanks, Phil ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-05-31 16:28 ` Phil Elwell @ 2017-05-31 16:47 ` Stephen Warren 2017-06-01 6:39 ` Stephen Boyd 1 sibling, 0 replies; 13+ messages in thread From: Stephen Warren @ 2017-05-31 16:47 UTC (permalink / raw) To: Phil Elwell, Stefan Wahren Cc: Michael Turquette, Stephen Boyd, linux-kernel, linux-rpi-kernel, linux-clk On 05/31/2017 10:28 AM, Phil Elwell wrote: > On 31/05/2017 16:58, Stefan Wahren wrote: >> Am 31.05.2017 um 17:27 schrieb Stephen Warren: >>> On 05/30/2017 06:23 AM, Phil Elwell wrote: >>>> Hi, >>>> >>>> I've run into a problem using the fixed-factor clock on Raspberry Pi >>>> and I'd >>>> like some advice before I submit a patch. >>>> >>>> Some context: the aim is to use a standard UART and some external >>>> circuitry >>>> as a MIDI interface. This would be straightforward except that Linux >>>> doesn't >>>> recognise the required 31.25KHz as a valid UART baud rate. Rhe >>>> workaround is >>>> to declare the UART clock such that the reported rate differs from >>>> the actual >>>> rate. If one sets the reported rate to be (actual*38400)/31250 then >>>> requesting a 38400 baud rate will result in an actual 31250 baud signal. >>> >>> This sounds like the wrong approach. Forcing the port to use a >>> different clock rate than the user requests would prevent anyone from >>> using that port for its standard purpose; it'd turn what should be a >>> runtime decision into a compile-time decision. >>> >>> Are you sure there's no way to simply select the correct baud rate on >>> the port? I see plenty of references to configuring custom baud rates >>> under Linux when I search Google, e.g.: >>> >>>> https://stackoverflow.com/questions/12646324/how-to-set-a-custom-baud-rate-on-linux >>>> >>> "How to set a custom baud rate on Linux?" >>> >>>> https://sourceware.org/ml/libc-help/2009-06/msg00016.html >>> "Re: Terminal interface and non-standard baudrates" >>> >> >> I remember this gist from Peter Hurley: >> >> https://gist.github.com/peterhurley/fbace59b55d87306a5b8 > > Thank you, Stephen and Stephan. Stephen - the clock scaling is applied by a DT overlay so > it effectively a runtime setting, but I take your point about the elegance of the solution. > Stefan - anybaud looks promising, although I would have preferred for users to continue to > use the existing user-space tools - kernel changes can be deployed more easily. > > For my edification, can you pretend for a moment that the application was a valid one and > answer any of my original questions?: > > 1. Should all system clock drivers use OF_CLK_DECLARE? Doing so would probably > avoid this problem, but further initialisation order dependencies may > require more drivers to be initialised early. > > 2. Why does the clock initialisation hook registered by OF_CLK_DECLARE not > return any indication of success? If it did, and if the OF_POPULATED flag > was only set after successful initialisation then the normal retrying of > a deferred probe would also avoid this problem. > > 3. Would adding the OF_CLK_DECLARE hook prevent the use of the devm_ managed > functions like devm_kzalloc? If not, why doesn't fixed-factor-clock use it? Sorry, I don't know the answers to these questions; I expect the clock subsystem maintainers will have to chime in. My only general comment is that probe deferral is the typical mechanism to handle driver/device/object dependencies, but I have no idea how that would interact with static initialization hooks like OF_CLK_DECLARE. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-05-31 16:28 ` Phil Elwell 2017-05-31 16:47 ` Stephen Warren @ 2017-06-01 6:39 ` Stephen Boyd 2017-06-01 8:26 ` Phil Elwell 1 sibling, 1 reply; 13+ messages in thread From: Stephen Boyd @ 2017-06-01 6:39 UTC (permalink / raw) To: Phil Elwell Cc: Stefan Wahren, Stephen Warren, Michael Turquette, linux-kernel, linux-rpi-kernel, linux-clk On 05/31, Phil Elwell wrote: > On 31/05/2017 16:58, Stefan Wahren wrote: > > Am 31.05.2017 um 17:27 schrieb Stephen Warren: > >> On 05/30/2017 06:23 AM, Phil Elwell wrote: > >>> Hi, > >>> > >>> I've run into a problem using the fixed-factor clock on Raspberry Pi > >>> and I'd > >>> like some advice before I submit a patch. > >>> > >>> Some context: the aim is to use a standard UART and some external > >>> circuitry > >>> as a MIDI interface. This would be straightforward except that Linux > >>> doesn't > >>> recognise the required 31.25KHz as a valid UART baud rate. Rhe > >>> workaround is > >>> to declare the UART clock such that the reported rate differs from > >>> the actual > >>> rate. If one sets the reported rate to be (actual*38400)/31250 then > >>> requesting a 38400 baud rate will result in an actual 31250 baud signal. > >> > >> This sounds like the wrong approach. Forcing the port to use a > >> different clock rate than the user requests would prevent anyone from > >> using that port for its standard purpose; it'd turn what should be a > >> runtime decision into a compile-time decision. > >> > >> Are you sure there's no way to simply select the correct baud rate on > >> the port? I see plenty of references to configuring custom baud rates > >> under Linux when I search Google, e.g.: > >> > >>> https://stackoverflow.com/questions/12646324/how-to-set-a-custom-baud-rate-on-linux > >>> > >> "How to set a custom baud rate on Linux?" > >> > >>> https://sourceware.org/ml/libc-help/2009-06/msg00016.html > >> "Re: Terminal interface and non-standard baudrates" > >> > > > > I remember this gist from Peter Hurley: > > > > https://gist.github.com/peterhurley/fbace59b55d87306a5b8 > > Thank you, Stephen and Stephan. Stephen - the clock scaling is applied by a DT overlay so > it effectively a runtime setting, but I take your point about the elegance of the solution. > Stefan - anybaud looks promising, although I would have preferred for users to continue to > use the existing user-space tools - kernel changes can be deployed more easily. > > For my edification, can you pretend for a moment that the application was a valid one and > answer any of my original questions?: > > 1. Should all system clock drivers use OF_CLK_DECLARE? Doing so would probably > avoid this problem, but further initialisation order dependencies may > require more drivers to be initialised early. No. CLK_OF_DECLARE() is only there to register clks early for things that need them early, i.e. interrupts and timers. Otherwise they should be plain drivers (platform or some other bus). If the same node has both then we have CLK_OF_DECLARE for that. > > 2. Why does the clock initialisation hook registered by OF_CLK_DECLARE not > return any indication of success? If it did, and if the OF_POPULATED flag > was only set after successful initialisation then the normal retrying of > a deferred probe would also avoid this problem. Historical raisins. Honestly if it fails the whole system should probably panic because we aren't going to get interrupts or schedule properly. Of course, we have whole drivers that register with CLK_OF_DECLARE() though when they should really be a driver that can do probe defer, etc., so making a change isn't really feasible right now. > > 3. Would adding the OF_CLK_DECLARE hook prevent the use of the devm_ managed > functions like devm_kzalloc? If not, why doesn't fixed-factor-clock use it? > If you use CLK_OF_DECLARE() then you don't get a struct device to pass to devm functions and thus you can't use them. I don't follow the question. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-06-01 6:39 ` Stephen Boyd @ 2017-06-01 8:26 ` Phil Elwell 2017-06-02 22:34 ` Stephen Boyd 0 siblings, 1 reply; 13+ messages in thread From: Phil Elwell @ 2017-06-01 8:26 UTC (permalink / raw) To: Stephen Boyd Cc: Stefan Wahren, Stephen Warren, Michael Turquette, linux-kernel, linux-rpi-kernel, linux-clk On 01/06/2017 07:39, Stephen Boyd wrote: > On 05/31, Phil Elwell wrote: >> On 31/05/2017 16:58, Stefan Wahren wrote: >>> Am 31.05.2017 um 17:27 schrieb Stephen Warren: >>>> On 05/30/2017 06:23 AM, Phil Elwell wrote: >>>>> Hi, >>>>> >>>>> I've run into a problem using the fixed-factor clock on Raspberry Pi >>>>> and I'd >>>>> like some advice before I submit a patch. >>>>> >>>>> Some context: the aim is to use a standard UART and some external >>>>> circuitry >>>>> as a MIDI interface. This would be straightforward except that Linux >>>>> doesn't >>>>> recognise the required 31.25KHz as a valid UART baud rate. Rhe >>>>> workaround is >>>>> to declare the UART clock such that the reported rate differs from >>>>> the actual >>>>> rate. If one sets the reported rate to be (actual*38400)/31250 then >>>>> requesting a 38400 baud rate will result in an actual 31250 baud signal. >>>> >>>> This sounds like the wrong approach. Forcing the port to use a >>>> different clock rate than the user requests would prevent anyone from >>>> using that port for its standard purpose; it'd turn what should be a >>>> runtime decision into a compile-time decision. >>>> >>>> Are you sure there's no way to simply select the correct baud rate on >>>> the port? I see plenty of references to configuring custom baud rates >>>> under Linux when I search Google, e.g.: >>>> >>>>> https://stackoverflow.com/questions/12646324/how-to-set-a-custom-baud-rate-on-linux >>>>> >>>> "How to set a custom baud rate on Linux?" >>>> >>>>> https://sourceware.org/ml/libc-help/2009-06/msg00016.html >>>> "Re: Terminal interface and non-standard baudrates" >>>> >>> >>> I remember this gist from Peter Hurley: >>> >>> https://gist.github.com/peterhurley/fbace59b55d87306a5b8 >> >> Thank you, Stephen and Stephan. Stephen - the clock scaling is applied by a DT overlay so >> it effectively a runtime setting, but I take your point about the elegance of the solution. >> Stefan - anybaud looks promising, although I would have preferred for users to continue to >> use the existing user-space tools - kernel changes can be deployed more easily. >> >> For my edification, can you pretend for a moment that the application was a valid one and >> answer any of my original questions?: >> >> 1. Should all system clock drivers use OF_CLK_DECLARE? Doing so would probably >> avoid this problem, but further initialisation order dependencies may >> require more drivers to be initialised early. > > No. CLK_OF_DECLARE() is only there to register clks early for > things that need them early, i.e. interrupts and timers. > Otherwise they should be plain drivers (platform or some other > bus). If the same node has both then we have > CLK_OF_DECLARE for that. The problem with fixed-factor-clock is something like a Priority Inversion. CLK_OF_DECLARE doesn't say that a driver can be probed early, it says that _must_ be probed early. There is no fallback to platform device probing - setting the OF_POPULATED flag prevents that. In my example the parent clock and the consumer are regular platform devices, but having this tiny little gasket in between probed from of_clk_init breaks what ought to be a run-of-the-mill device instantiation. It would be better if the intermediate driver could adapt to the environment in which it finds itself, inheriting the "earlyness" of its consumer, but that would require proper dependency information which Device Tree doesn't capture in a way which is easy to make use of (phandles being integers that can be embedded in vectors in subsystem-specific ways). >> 2. Why does the clock initialisation hook registered by OF_CLK_DECLARE not >> return any indication of success? If it did, and if the OF_POPULATED flag >> was only set after successful initialisation then the normal retrying of >> a deferred probe would also avoid this problem. > > Historical raisins. Honestly if it fails the whole system should > probably panic because we aren't going to get interrupts or > schedule properly. Of course, we have whole drivers that register > with CLK_OF_DECLARE() though when they should really be a driver > that can do probe defer, etc., so making a change isn't really > feasible right now. That's the conclusion I had come to, that the current situation isn't ideal but that fixing it is non-trivial. >> 3. Would adding the OF_CLK_DECLARE hook prevent the use of the devm_ managed >> functions like devm_kzalloc? If not, why doesn't fixed-factor-clock use it? >> > > If you use CLK_OF_DECLARE() then you don't get a struct device to > pass to devm functions and thus you can't use them. I don't > follow the question. You don't need to evaluate the "else" it the condition is true, so you can ignore the follow-up question. I was just confirming that if I did modify the bcm2835 clock drivers to register with CLK_OF_DECLARE that I would have to strip out the devm functions and revert to old-fashioned clean-up-on-exit code. Thanks - you've confirmed my suspicions; the problem remains though as a bear trap to catch the unwary. Phil ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-06-01 8:26 ` Phil Elwell @ 2017-06-02 22:34 ` Stephen Boyd 2017-06-05 16:24 ` Phil Elwell 0 siblings, 1 reply; 13+ messages in thread From: Stephen Boyd @ 2017-06-02 22:34 UTC (permalink / raw) To: Phil Elwell Cc: Stefan Wahren, Stephen Warren, Michael Turquette, linux-kernel, linux-rpi-kernel, linux-clk On 06/01, Phil Elwell wrote: > On 01/06/2017 07:39, Stephen Boyd wrote: > > On 05/31, Phil Elwell wrote: > >> For my edification, can you pretend for a moment that the application was a valid one and > >> answer any of my original questions?: > >> > >> 1. Should all system clock drivers use OF_CLK_DECLARE? Doing so would probably > >> avoid this problem, but further initialisation order dependencies may > >> require more drivers to be initialised early. > > > > No. CLK_OF_DECLARE() is only there to register clks early for > > things that need them early, i.e. interrupts and timers. > > Otherwise they should be plain drivers (platform or some other > > bus). If the same node has both then we have > > CLK_OF_DECLARE for that. > > The problem with fixed-factor-clock is something like a Priority Inversion. CLK_OF_DECLARE > doesn't say that a driver can be probed early, it says that _must_ be probed early. There is > no fallback to platform device probing - setting the OF_POPULATED flag prevents that. In my > example the parent clock and the consumer are regular platform devices, but having this tiny > little gasket in between probed from of_clk_init breaks what ought to be a run-of-the-mill > device instantiation. It would be better if the intermediate driver could adapt to the > environment in which it finds itself, inheriting the "earlyness" of its consumer, but that > would require proper dependency information which Device Tree doesn't capture in a way which > is easy to make use of (phandles being integers that can be embedded in vectors in > subsystem-specific ways). You sort of lost me here. The clk framework has support for "orphans" which means that clks can be registered in any order, i.e. the fixed factor clk could register first and be orphaned until the parent platform device driver probes and registers that clk at which point we'll fix up the tree. So nothing goes wrong and really the orphan design helps us here and in other situations. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-06-02 22:34 ` Stephen Boyd @ 2017-06-05 16:24 ` Phil Elwell 2017-06-05 20:13 ` Stephen Boyd 0 siblings, 1 reply; 13+ messages in thread From: Phil Elwell @ 2017-06-05 16:24 UTC (permalink / raw) To: Stephen Boyd Cc: Stefan Wahren, Stephen Warren, Michael Turquette, linux-kernel, linux-rpi-kernel, linux-clk On 02/06/2017 23:34, Stephen Boyd wrote:> On 06/01, Phil Elwell wrote: >> On 01/06/2017 07:39, Stephen Boyd wrote: >>> On 05/31, Phil Elwell wrote: >>>> For my edification, can you pretend for a moment that the application was a valid one and >>>> answer any of my original questions?: >>>> >>>> 1. Should all system clock drivers use OF_CLK_DECLARE? Doing so would probably >>>> avoid this problem, but further initialisation order dependencies may >>>> require more drivers to be initialised early. >>> >>> No. CLK_OF_DECLARE() is only there to register clks early for >>> things that need them early, i.e. interrupts and timers. >>> Otherwise they should be plain drivers (platform or some other >>> bus). If the same node has both then we have >>> CLK_OF_DECLARE for that. >> >> The problem with fixed-factor-clock is something like a Priority Inversion. CLK_OF_DECLARE >> doesn't say that a driver can be probed early, it says that _must_ be probed early. There is >> no fallback to platform device probing - setting the OF_POPULATED flag prevents that. In my >> example the parent clock and the consumer are regular platform devices, but having this tiny >> little gasket in between probed from of_clk_init breaks what ought to be a run-of-the-mill >> device instantiation. It would be better if the intermediate driver could adapt to the >> environment in which it finds itself, inheriting the "earlyness" of its consumer, but that >> would require proper dependency information which Device Tree doesn't capture in a way which >> is easy to make use of (phandles being integers that can be embedded in vectors in >> subsystem-specific ways). > > You sort of lost me here. The clk framework has support for > "orphans" which means that clks can be registered in any order, > i.e. the fixed factor clk could register first and be orphaned > until the parent platform device driver probes and registers that > clk at which point we'll fix up the tree. So nothing goes wrong > and really the orphan design helps us here and in other > situations. That sounds great, but it doesn't match my experience. Let me restate my observations with a bit more detail. In this scenario there three devices in a dependency chain: clock -> fixed-factor->clock -> uart. The Fixed Factor Clock is declared with OF_CLK_DECLARE, while the two platform drivers use normal probe functions. 1) of_clk_init() calls encounter FFC in the list of clocks to initialise and calls parent_ready on the device node. 2) The parent clock has not been initialised, so of_clk_get returns -EPROBE_DEFER. 3) Steps 1 and 2 repeat until no progress is made, at which point the force flag is set for one last iteration. This time the parent_ready check is skipped and the code calls indirectly into _of_fixed_factor_clk_setup(). 4) The FFC setup calls of_clk_get_parent_name, which returns a NULL that ends up referred to by the parent_names field of clk_init_data structure indirectly passed to clk_hw_register and clk_register. 5) In clk_register, the parent name is copied with kstrdup, which returns NULL for a NULL input. clk_register sees this as an allocation failure and returns -ENOMEM. 6) _of_fixed_factor_clock_setup returns -ENOMEM, bypassing of_clk_add_provider, but the wrapper function registered with CLK_OF_DECLARE has a void return, so the failure is lost. 7) Back in of_clk_init, which is ignorant of the failure, the OF_POPULATED flag of the FFC node has already been set, preventing the node from subsequently being probed in the usual way. 8) When the downstream uart is probed, devm_clk_get returns -EPROBE_DEFER every time, resulting in a non-functioning UART. Is this behaviour as intended? I can see that the NULL parent name in steps 4 and 5 could be handled more gracefully, but the end result would be the same. Where and how is the "orphan" clock concept supposed to help, and what needs to be fixed in this case? Thanks, Phil ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-06-05 16:24 ` Phil Elwell @ 2017-06-05 20:13 ` Stephen Boyd 2017-06-06 7:22 ` Geert Uytterhoeven 2017-06-06 8:49 ` Phil Elwell 0 siblings, 2 replies; 13+ messages in thread From: Stephen Boyd @ 2017-06-05 20:13 UTC (permalink / raw) To: Phil Elwell Cc: Stefan Wahren, Stephen Warren, Michael Turquette, linux-kernel, linux-rpi-kernel, linux-clk On 06/05, Phil Elwell wrote: > That sounds great, but it doesn't match my experience. Let me restate my > observations with a bit more detail. > > In this scenario there three devices in a dependency chain: > > clock -> fixed-factor->clock -> uart. > > The Fixed Factor Clock is declared with OF_CLK_DECLARE, while the two platform > drivers use normal probe functions. > > 1) of_clk_init() calls encounter FFC in the list of clocks to initialise and > calls parent_ready on the device node. > > 2) The parent clock has not been initialised, so of_clk_get returns > -EPROBE_DEFER. > > 3) Steps 1 and 2 repeat until no progress is made, at which point the force > flag is set for one last iteration. This time the parent_ready check is skipped > and the code calls indirectly into _of_fixed_factor_clk_setup(). > > 4) The FFC setup calls of_clk_get_parent_name, which returns a NULL that ends > up referred to by the parent_names field of clk_init_data structure indirectly > passed to clk_hw_register and clk_register. That's bad. Does "clock" in this scenario have a clock-output-names property so we can find the name of the parent of the fixed factor clock? That way we can describe the fixed factor to "clock" linkage. Without that, things won't ever work. > > 5) In clk_register, the parent name is copied with kstrdup, which returns NULL > for a NULL input. clk_register sees this as an allocation failure and returns > -ENOMEM. > > 6) _of_fixed_factor_clock_setup returns -ENOMEM, bypassing of_clk_add_provider, > but the wrapper function registered with CLK_OF_DECLARE has a void return, so > the failure is lost. Yep. We've already failed earlier. > > 7) Back in of_clk_init, which is ignorant of the failure, the OF_POPULATED flag > of the FFC node has already been set, preventing the node from subsequently > being probed in the usual way. > > 8) When the downstream uart is probed, devm_clk_get returns -EPROBE_DEFER every > time, resulting in a non-functioning UART. > > Is this behaviour as intended? I can see that the NULL parent name in steps 4 > and 5 could be handled more gracefully, but the end result would be the same. > > Where and how is the "orphan" clock concept supposed to help, and what needs to > be fixed in this case? > The orphan concept helps here because of_clk_init() eventually forces the registration of the fixed factor clock even though the fixed factor's parent has not been registered yet. As you've determined though, that isn't working properly because the fixed factor code is failing to get a name for the parent. Using the clock-output-names property would fix that though. Also, it may be appropriate to move the fixed factor clock registration into the UART driver. It would depend on where the fixed factor is situated inside the SoC, but it could be argued that if the factor is near or embedded inside the UART hardware then the UART driver should register the fixed factor clock as well as the UART clock. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-06-05 20:13 ` Stephen Boyd @ 2017-06-06 7:22 ` Geert Uytterhoeven 2017-06-06 20:49 ` Stephen Boyd 2017-06-06 8:49 ` Phil Elwell 1 sibling, 1 reply; 13+ messages in thread From: Geert Uytterhoeven @ 2017-06-06 7:22 UTC (permalink / raw) To: Stephen Boyd Cc: Phil Elwell, Stefan Wahren, Stephen Warren, Michael Turquette, linux-kernel, linux-rpi-kernel, linux-clk Hi Stephen, On Mon, Jun 5, 2017 at 10:13 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 06/05, Phil Elwell wrote: >> That sounds great, but it doesn't match my experience. Let me restate my >> observations with a bit more detail. >> >> In this scenario there three devices in a dependency chain: >> >> clock -> fixed-factor->clock -> uart. >> >> The Fixed Factor Clock is declared with OF_CLK_DECLARE, while the two platform >> drivers use normal probe functions. >> >> 1) of_clk_init() calls encounter FFC in the list of clocks to initialise and >> calls parent_ready on the device node. >> >> 2) The parent clock has not been initialised, so of_clk_get returns >> -EPROBE_DEFER. >> >> 3) Steps 1 and 2 repeat until no progress is made, at which point the force >> flag is set for one last iteration. This time the parent_ready check is skipped >> and the code calls indirectly into _of_fixed_factor_clk_setup(). >> >> 4) The FFC setup calls of_clk_get_parent_name, which returns a NULL that ends >> up referred to by the parent_names field of clk_init_data structure indirectly >> passed to clk_hw_register and clk_register. > > That's bad. Does "clock" in this scenario have a > clock-output-names property so we can find the name of the parent > of the fixed factor clock? That way we can describe the fixed > factor to "clock" linkage. Without that, things won't ever work. >> Is this behaviour as intended? I can see that the NULL parent name in steps 4 >> and 5 could be handled more gracefully, but the end result would be the same. >> >> Where and how is the "orphan" clock concept supposed to help, and what needs to >> be fixed in this case? >> > > The orphan concept helps here because of_clk_init() eventually > forces the registration of the fixed factor clock even though the > fixed factor's parent has not been registered yet. As you've > determined though, that isn't working properly because the fixed > factor code is failing to get a name for the parent. Using the > clock-output-names property would fix that though. Isn't clock-output-names deprecated for clocks with a single clock output? 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-06-06 7:22 ` Geert Uytterhoeven @ 2017-06-06 20:49 ` Stephen Boyd 0 siblings, 0 replies; 13+ messages in thread From: Stephen Boyd @ 2017-06-06 20:49 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Phil Elwell, Stefan Wahren, Stephen Warren, Michael Turquette, linux-kernel, linux-rpi-kernel, linux-clk On 06/06, Geert Uytterhoeven wrote: > Hi Stephen, > > On Mon, Jun 5, 2017 at 10:13 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > On 06/05, Phil Elwell wrote: > >> That sounds great, but it doesn't match my experience. Let me restate my > >> observations with a bit more detail. > >> > >> In this scenario there three devices in a dependency chain: > >> > >> clock -> fixed-factor->clock -> uart. > >> > >> The Fixed Factor Clock is declared with OF_CLK_DECLARE, while the two platform > >> drivers use normal probe functions. > >> > >> 1) of_clk_init() calls encounter FFC in the list of clocks to initialise and > >> calls parent_ready on the device node. > >> > >> 2) The parent clock has not been initialised, so of_clk_get returns > >> -EPROBE_DEFER. > >> > >> 3) Steps 1 and 2 repeat until no progress is made, at which point the force > >> flag is set for one last iteration. This time the parent_ready check is skipped > >> and the code calls indirectly into _of_fixed_factor_clk_setup(). > >> > >> 4) The FFC setup calls of_clk_get_parent_name, which returns a NULL that ends > >> up referred to by the parent_names field of clk_init_data structure indirectly > >> passed to clk_hw_register and clk_register. > > > > That's bad. Does "clock" in this scenario have a > > clock-output-names property so we can find the name of the parent > > of the fixed factor clock? That way we can describe the fixed > > factor to "clock" linkage. Without that, things won't ever work. > > >> Is this behaviour as intended? I can see that the NULL parent name in steps 4 > >> and 5 could be handled more gracefully, but the end result would be the same. > >> > >> Where and how is the "orphan" clock concept supposed to help, and what needs to > >> be fixed in this case? > >> > > > > The orphan concept helps here because of_clk_init() eventually > > forces the registration of the fixed factor clock even though the > > fixed factor's parent has not been registered yet. As you've > > determined though, that isn't working properly because the fixed > > factor code is failing to get a name for the parent. Using the > > clock-output-names property would fix that though. > > Isn't clock-output-names deprecated for clocks with a single clock > output? > Yes. I'd prefer we don't have any clock-output-names in dts. In this case, it's pretty much required though, until we get to a point where we can describe parent child relationships through another means besides strings. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: CLK_OF_DECLARE advice required 2017-06-05 20:13 ` Stephen Boyd 2017-06-06 7:22 ` Geert Uytterhoeven @ 2017-06-06 8:49 ` Phil Elwell 1 sibling, 0 replies; 13+ messages in thread From: Phil Elwell @ 2017-06-06 8:49 UTC (permalink / raw) To: Stephen Boyd Cc: Stefan Wahren, Stephen Warren, Michael Turquette, linux-kernel, linux-rpi-kernel, linux-clk On 05/06/2017 21:13, Stephen Boyd wrote: > On 06/05, Phil Elwell wrote: >> That sounds great, but it doesn't match my experience. Let me restate my >> observations with a bit more detail. >> >> In this scenario there three devices in a dependency chain: >> >> clock -> fixed-factor->clock -> uart. >> >> The Fixed Factor Clock is declared with OF_CLK_DECLARE, while the two platform >> drivers use normal probe functions. >> >> 1) of_clk_init() calls encounter FFC in the list of clocks to initialise and >> calls parent_ready on the device node. >> >> 2) The parent clock has not been initialised, so of_clk_get returns >> -EPROBE_DEFER. >> >> 3) Steps 1 and 2 repeat until no progress is made, at which point the force >> flag is set for one last iteration. This time the parent_ready check is skipped >> and the code calls indirectly into _of_fixed_factor_clk_setup(). >> >> 4) The FFC setup calls of_clk_get_parent_name, which returns a NULL that ends >> up referred to by the parent_names field of clk_init_data structure indirectly >> passed to clk_hw_register and clk_register. > > That's bad. Does "clock" in this scenario have a > clock-output-names property so we can find the name of the parent > of the fixed factor clock? That way we can describe the fixed > factor to "clock" linkage. Without that, things won't ever work. No - the clock provider is bcm2835-aux-clk, as declared by bcm283x.dts: https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/+/master/arch/arm/boot/dts/bcm283x.dtsi#462 If I patch it to include a "clock-output-names" property with "aux-uart" as the first entry then the FFC registers correctly, even though the source clock hasn't been initialised yet. >> 5) In clk_register, the parent name is copied with kstrdup, which returns NULL >> for a NULL input. clk_register sees this as an allocation failure and returns >> -ENOMEM. >> >> 6) _of_fixed_factor_clock_setup returns -ENOMEM, bypassing of_clk_add_provider, >> but the wrapper function registered with CLK_OF_DECLARE has a void return, so >> the failure is lost. > > Yep. We've already failed earlier. > >> >> 7) Back in of_clk_init, which is ignorant of the failure, the OF_POPULATED flag >> of the FFC node has already been set, preventing the node from subsequently >> being probed in the usual way. >> >> 8) When the downstream uart is probed, devm_clk_get returns -EPROBE_DEFER every >> time, resulting in a non-functioning UART. >> >> Is this behaviour as intended? I can see that the NULL parent name in steps 4 >> and 5 could be handled more gracefully, but the end result would be the same. >> >> Where and how is the "orphan" clock concept supposed to help, and what needs to >> be fixed in this case? >> > > The orphan concept helps here because of_clk_init() eventually > forces the registration of the fixed factor clock even though the > fixed factor's parent has not been registered yet. As you've > determined though, that isn't working properly because the fixed > factor code is failing to get a name for the parent. Using the > clock-output-names property would fix that though. > Also, it may be appropriate to move the fixed factor clock > registration into the UART driver. It would depend on where the > fixed factor is situated inside the SoC, but it could be argued > that if the factor is near or embedded inside the UART hardware > then the UART driver should register the fixed factor clock as > well as the UART clock. I take your point, but I'm trying to use a standard UART and a standard fixed-factor clock to get non-standard results in what has become a learning exercise. Thanks again - this has been very useful. Phil ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-06-06 20:49 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-30 12:23 CLK_OF_DECLARE advice required Phil Elwell 2017-05-31 15:27 ` Stephen Warren 2017-05-31 15:58 ` Stefan Wahren 2017-05-31 16:28 ` Phil Elwell 2017-05-31 16:47 ` Stephen Warren 2017-06-01 6:39 ` Stephen Boyd 2017-06-01 8:26 ` Phil Elwell 2017-06-02 22:34 ` Stephen Boyd 2017-06-05 16:24 ` Phil Elwell 2017-06-05 20:13 ` Stephen Boyd 2017-06-06 7:22 ` Geert Uytterhoeven 2017-06-06 20:49 ` Stephen Boyd 2017-06-06 8:49 ` Phil Elwell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).