CLK_OF_DECLARE advice required

CLK_OF_DECLARE advice required

[Phil Elwell]

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

Re: CLK_OF_DECLARE advice required

[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"

Re: CLK_OF_DECLARE advice required

[Stefan Wahren]

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

Re: CLK_OF_DECLARE advice required

[Phil Elwell]

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

Re: CLK_OF_DECLARE advice required

[Stephen Warren]

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.

Re: CLK_OF_DECLARE advice required

[Stephen Boyd]

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

Re: CLK_OF_DECLARE advice required

[Phil Elwell]

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

Re: CLK_OF_DECLARE advice required

[Stephen Boyd]

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

Re: CLK_OF_DECLARE advice required

[Phil Elwell]

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

Re: CLK_OF_DECLARE advice required

[Stephen Boyd]

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

Re: CLK_OF_DECLARE advice required

[Geert Uytterhoeven]

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

Re: CLK_OF_DECLARE advice required

[Phil Elwell]

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

Re: CLK_OF_DECLARE advice required

[Stephen Boyd]

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