clock tree: wrong ref_count initialization for clocks that are open in start time
Created by: yaronm-hailo
There is a problem with the current implementation of the ref_count
initialization in clock_connect_tree
here.
The code in clock_connect_tree
allows working with clocks that start enabled, but the logic of updating the ref_count
is incorrect - if a clock is enabled, the code increases the ref_count
only of the direct parent of the clock - not that of the clock itself, and not that of the parent's parents.
This is opposed to the way a clock that is enabled through set_state
is treated, where the ref_count
is updated for the clock itself and for all it's parents.
One of the implications of this is that there is no reasonable way to disable a clock that was enabled in the system start.
As an example of the problem suppose we have the following topology (taken from the documentation)
+-----------+
| CLOCK-0 |
+-+---------+
|
| +------------+
+--> CLOCK-1 |
+-+----------+
|
|
| +------------+
+---> CLOCK-2 |
| +------------+
|
| +------------+
+---> CLOCK-3 |
+------------+
Let's consider the scenario where the firmware starts with CLOCK-0
and CLOCK-1
enabled and CLOCK-2
and CLOCK-3
disabled.
This means after clock_connect_tree
is done, CLOCK-0
's ref_count
is 1, while CLOCK-1
's ref_count
is 0.
This scenario is problematic for several reasons:
- An attempt to disable
CLOCK-0
will actually disable it, even thoughCLOCK-1
is enabled and dependent on it - if we attempt to disable
CLOCK-1
,ref_count
will wrap around to 0xFFFFFFFF (perhaps this is another bug - the code will decrease the ref_count even if it is 0 - if we attempt to enable
CLOCK-2
, and then disable it -CLOCK-1
will also get disabled in the process - even though we might not have wanted it - there could have been a reason that this clock was enabled when the firmware started.
One possible solution is to increase the ref_count
of enabled clock as well as all of it's parents' ref_count
s.
This will make the behaviour unified between the clocks already enabled when the firmware starts, and clocks enabled by the firmware.
Before submitting a PR with a fix I wanted to get your opinion, perhaps there is a better solution I haven't thought of..