Messages in this thread | | | Subject | Re: [PATCH v1] clk: Probe defer clk_get() on orphans | From | Jeffrey Hugo <> | Date | Tue, 30 Apr 2019 14:52:40 -0600 |
| |
On 4/22/2019 6:52 PM, Stephen Boyd wrote: > Quoting Jeffrey Hugo (2019-02-11 10:57:47) >> If a parent to a clock comes from outside that clock's provider, the parent >> may not be present at the time the clock is registered (ie the parent comes >> from another driver that has not yet probed). The clock can still be >> registered, and a reference to it obtained, however that clock may not be >> fully functional - ie get_rate might return an invalid value. >> >> This has been a problem that has resulted in the UART console breaking on >> some Qualcomm SoCs, as the UART baud rate is based on a clock that is the >> child of XO. Due to the large chain of dependencies, its possible that the >> RPM has not provided XO by the time that the UART driver probes, gets the >> baud rate clock, and calls get_rate - which returns 0 and results in a bad >> configuration. >> >> An orphan clock is a clock that is missing a parent or some other ancestor. >> Since the parent is defined, we can assume that it is expected to appear at >> some point in a properly configured system (all bets are off if a required >> driver is not compiled, etc), and it is unlikely that the clock can be >> properly consumed during the time the clock is an orphan. Therefore, >> return EPROBE_DEFER for orphan clocks so that consumers wait until the >> parent chain is established, and proper clock operation can occur. >> >> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org> >> --- >> >> This is based upon the "Rewrite clk parent handling" series at [1], and assumes >> that the suspected missing line commented on at [2] is added. >> >> The idea for this solution came from [3] and [4]. >> >> [1] https://lore.kernel.org/lkml/20190129061021.94775-1-sboyd@kernel.org/T/#u >> [2] https://lkml.org/lkml/2019/2/11/1634 >> [3] https://lkml.org/lkml/2019/2/6/382 >> [4] https://lkml.org/lkml/2015/12/27/209 > > There have been multiple attempts over the years to support probe defer > for clks that don't have parents. If you search the kernel mailing list > archives I'm sure you'll come across them (for example > https://patchwork.kernel.org/patch/6313051/). That's why we have the > first part of the code to indicate if a clk is an orphan or not, i.e. > commit e6500344edbb ("clk: track the orphan status of clocks and their > children"), but not this patch that you've sent. > > There are a couple requirements that we need to make sure we don't break > first. > > 1. clk_get() should work for clks on the orphan list if that clk is > parented to something that will never be registered with the framework > > 2. We need a way for drivers to express that the parent of a clk > won't exist > > 3. Critical clks need to turn clks on even if they'll never get > parents registered > > We've had problems in the past > (http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/343007.html) > where bootloaders configure clks in certain ways that the kernel doesn't > care to even consider as possible. In these cases we either need to let > clk_set_rate() reparent them when consumers are ready or we need to > convert drivers that are forcing on clks early to use the > CLK_IS_CRITICAL flag to turn on clks even if they're never going to find > their parents. > > Last time we tried to do this in 2015 (wow so many years ago!) we were > blocked on not having the critical clks infrastructure and the legacy > sunxi clk driver needed to convert to DT and critical clks flag to keep > working. I think we could have done it a year or two ago, because sunxi > moved to a new design, but then we got more use-cases where clks may > never get the parent they're currently configured for in the bootloader > and then the kernel would never hand out the clk to consumers and the > clk_set_rate() case would fail. > > To fix that last part, I'm proposing we introduce the .get_parent_hw() > op and then rely on drivers to tell the framework that the parent is > there either with a direct pointer reference or by knowing that the > DT/firmware is telling us the parent is valid. If we just rely on string > names and a u8 to indicate parents then we don't have enough information > to figure out how the parent is provided and if it will ever appear at > some point in the future. Once we have a way to describe this through > DT/firmware then we're able to indicate the clk is an orphan when that's > actually the case vs. when the clk is configured in hardware for > something that we won't know about. You can see this work in the > clk-parent-rewrite series in clk.git. > > There's also one more problem, which is what we do with clks that we've > handed out to consumers and then the driver for the parent of that clk > is removed and the parent is unregistered. Right now, we move these clks > to the orphan list and set the clk_nodrv_ops on the parent that's > unregistered. We probably need to set clk_nodrv_ops on all the children > that get orphaned, and remove the cached clk_core pointer in all the > clk_core::parents members (even ones that aren't currently using it!), > and stash away the original clk_ops so we can restore them later when > the clk is properly reparented if the parent comes back. It's a lot of > book keeping to remove the dangling references and let it come back > later. I haven't started on this part, but it's on my radar. >
Ugh. Ok. Much more work to be done. I guess we live with the fake XO in GCC hack for a while yet then.
-- Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
| |