On Thu, Apr 14, 2022 at 01:21:29AM +0300, Dmitry Baryshkov wrote: > On 13/04/2022 12:15, Johan Hovold wrote: > > On Tue, Apr 12, 2022 at 10:38:35PM +0300, Dmitry Baryshkov wrote: > >> On recent Qualcomm platforms the QMP PIPE clocks feed into a set of > >> muxes which must be parked to the "safe" source (bi_tcxo) when > >> corresponding GDSC is turned off and on again. Currently this is > >> handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src > >> clock. However the same code sequence should be applied in the > >> pcie-qcom endpoint, USB3 and UFS drivers. > > The caching of the parent is broken since set_parent() is typically not > > called before enabling the clock. > > > > This means that the above code will set the mux to its zero-initialised > > value, which currently only works by chance as the pipe clock config > > value happens to be zero. > > > > For this to work generally, you'd also need to define also the > > (default/initial) non-safe parent for each mux. Handling handover from > > the bootloader might also be tricky. > > It's not tricky at all. We can set stored_parent_cfg from gcc probe from > function. Or set statically from the config. I'll probably do the latter. That means you're just ignoring the problem. How is hard coding the initial mux configuration in any way dealing with bootloader handover? > > Furthermore, the current implementation appears to ignore locking and > > doesn't handle the case where set_parent() races with enable(). The > > former is protected by the prepare mutex and the latter by the enable > > spinlock and a driver that needs to serialise the two needs to handle > > that itself. > > Since I'm trying to remove pipe_clk usage from pcie driver itself, there > is just one user left - qmp phy. And while you are correct that there is > a race, I think we can neglect that for now. Or shift enable/disable ops > to prepare/unprepare, thus using the same mutex everywhere. Let's not add known broken code. Correctness first. Handling the muxing the prepare/unprepare might work. I even considered using those callbacks to let CCF know about the reparenting so that for example debugfs continues to reflect the actual state of things. But I'm still leaning towards this being a misguided endeavour as I've explained elsewhere. Johan
