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
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
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
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
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
Hi Stephen,
On Mon, Jun 5, 2017 at 10:13 PM, Stephen Boyd <[email protected]> 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 -- [email protected]
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
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
On 06/06, Geert Uytterhoeven wrote:
> Hi Stephen,
>
> On Mon, Jun 5, 2017 at 10:13 PM, Stephen Boyd <[email protected]> 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