Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <1549911467-2465-1-git-send-email-jhugo@codeaurora.org>
References: <1549911467-2465-1-git-send-email-jhugo@codeaurora.org>
Cc:     bjorn.andersson@linaro.org, georgi.djakov@linaro.org,
        linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, Jeffrey Hugo <jhugo@codeaurora.org>
From:   Stephen Boyd <sboyd@kernel.org>
Subject: Re: [PATCH v1] clk: Probe defer clk_get() on orphans
To:     Jeffrey Hugo <jhugo@codeaurora.org>, mturquette@baylibre.com
Message-ID: <155598074927.15276.327474594450313045@swboyd.mtv.corp.google.com>
User-Agent: alot/0.8
Date:   Mon, 22 Apr 2019 17:52:29 -0700
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Jeffrey Hugo (2019-02-11 10:57:47)
> If a parent to a clock comes from outside that clock's provider, the pare=
nt
> may not be present at the time the clock is registered (ie the parent com=
es
> 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.
>=20
> 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 t=
he
> 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.
>=20
> An orphan clock is a clock that is missing a parent or some other ancesto=
r.
> 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.
>=20
> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
> ---
>=20
> This is based upon the "Rewrite clk parent handling" series at [1], and a=
ssumes
> that the suspected missing line commented on at [2] is added.
>=20
> The idea for this solution came from [3] and [4].
>=20
> [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.