Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold <johan@kernel.org>]

On Thu, Apr 21, 2022 at 02:36:05PM +0300, Dmitry Baryshkov wrote:
> On 21/04/2022 13:20, Johan Hovold wrote:
> > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > output generated by the PHY before powering on the PHY and restore the
> > default source during power down.
> > 
> > Add support for an optional pipe clock mux which will be reparented to
> > the generated pipe clock before powering on the PHY and restored to the
> > default reference source on power off.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
 
> > +static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
> > +{
> > +	struct qcom_qmp *qmp = qphy->qmp;
> > +	int ret;
> > +
> > +	ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
> > +	if (ret)
> > +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> > +
> > +
> > +	ret = clk_prepare_enable(qphy->pipe_clk);
> > +	if (ret) {
> > +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> > +		goto err_restore_parent;
> > +	}
> 
> So, what you do here is you manually set the parent of 
> GCC_PCIE_1_PIPE_CLK_SRC to PHY pipe clock right before enabling 
> GCC_PCIE_1_PIPE_CLK and set it back to XO after disabling 
> GCC_PCIE_1_PIPE_CLK.
> 
> My proposal is doing exactly the same, but doing that automatically 
> through the clock infrastructure. After removing pipe_clock handling 
> from pcie driver itself, we can be sure that nobody is playing dirty 
> tricks around the pipe_clock.

Yes, the end result is similar, but I believe handling it explicitly in
the driver is preferred for a number of reasons that I've already
mentioned. Not least because the mux needs to be updated when the PHY is
powered on, not when the GCC pipe clock is ungated.

In practise, powering on the PHY and ungating the clock happen to
coincide in time because only the PHY driver will use the GCC pipe
clock, but conceptually they are unrelated (and as the GDSC hang shows,
something in the system appears to be ungating the clock while the PHY
is powered off).

The QMP PHY driver implementation is much more straight forward and
easier to reason about than having the mux implementation spread out
over multiple clock drivers where it's not clear at all what is really
going on or why (and even debugfs will give you a false view of the
clock tree state).

> > +
> > +	return 0;
> > +
> > +err_restore_parent:
> > +	clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> > +
> > +	return ret;
> > +}
> > +
> > +static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
> > +{
> > +	struct qcom_qmp *qmp = qphy->qmp;
> > +	int ret;
> > +
> > +	clk_disable_unprepare(qphy->pipe_clk);
> > +
> > +	ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> > +	if (ret)
> > +		dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> > +}
> > +

Johan