Re: [PATCH v2 1/5] clk: qcom: regmap-mux: add pipe clk implementation

From: Johan Hovold <johan@kernel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: "Andy Gross" <agross@kernel.org>,
	"Bjorn Andersson" <bjorn.andersson@linaro.org>,
	"Stephen Boyd" <swboyd@chromium.org>,
	"Michael Turquette" <mturquette@baylibre.com>,
	"Taniya Das" <quic_tdas@quicinc.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Prasad Malisetty" <quic_pmaliset@quicinc.com>,
	linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH v2 1/5] clk: qcom: regmap-mux: add pipe clk implementation
Date: Thu, 14 Apr 2022 14:26:51 +0200	[thread overview]
Message-ID: <YlgTC5RdDXC3abrI@hovoldconsulting.com> (raw)
In-Reply-To: <82c6813c-fcff-5097-56e0-0cb7aac2eac2@linaro.org>

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