Message-ID: <20181119204543.GA31792@jcrouse-lnx.qualcomm.com>
Date:   Mon, 19 Nov 2018 13:45:43 -0700
From:   Jordan Crouse <jcrouse@...eaurora.org>
To:     Amit Nischal <anischal@...eaurora.org>
Cc:     Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Odelu Kukatla <okukatla@...eaurora.org>,
        Taniya Das <tdas@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 0/4] Add QCOM graphics clock controller driver for
 SDM845

On Mon, Aug 13, 2018 at 12:03:03PM +0530, Amit Nischal wrote:
> Changes in v3:
> * Modified the determine_rate() op to use the min/max rate range
>   to round the requested rate within the set_rate range. With this,
>   requested set rate will always stay within the limits.
> 
> Changes in v2:
> Addressed review comments given by Stephen: https://lkml.org/lkml/2018/6/6/294
> * Introduce clk_rcg2_gfx3d_determine_rate ops to return the best parent
>   as 'gpucc_pll0_even' and best parent rate as twice of the requested rate
>   always. This will eliminate the need of frequency table as source and
>   div-2 are fixed for gfx3d_clk_src.
>   Also modified the clk_rcg2_set_rate ops to configure the fixed source and
>   div.
> * Add support to check if requested rate falls within allowed set_rate range.
>   This will not let the source gpucc_pll0 to go out of the supported range
>   and also client can request the rate within the range.
> * Fixed comment text in probe function and added module description for GPUCC
>   driver.
> 
> The graphics clock driver depends upon the below change.
> 	https://lkml.org/lkml/2018/6/23/108
> 
> Changes in v1:
> This patch series adds support for graphics clock controller for SDM845.
> Below is the brief description for each change:
> 
> 1. For some of the GDSCs, there is requirement to enable/disable the
>    few clocks before turning on/off the gdsc power domain. This patch
>    will add support to enable/disable the clock associated with the
>    gdsc along with power domain on/off callbacks.
> 
> 2. To turn on the gpu_gx_gdsc, there is a hardware requirement to
>    turn on the root clock (GFX3D RCG) first which would be the turn
>    on signal for the gdsc along with the SW_COLLAPSE. As per the
>    current implementation of clk_rcg2_shared_ops, it clears the
>    root_enable bit in the enable() clock ops. But due to the above
>    said requirement for GFX3D shared RCG, root_enable bit would be
>    already set by gdsc driver and rcg2_shared_ops should not clear
>    the root unless the disable() is called.
> 
>    This patch add support for the same by reusing the existing
>    clk_rcg2_shared_ops and deriving "clk_rcg2_gfx3d_ops" clk_ops
>    for GFX3D clock to take care of the root set/clear requirement.
> 
> 3. Add device tree bindings for graphics clock controller for SDM845.
> 
> 4. Add graphics clock controller (GPUCC) driver for SDM845.
> 
> [v1] : https://lore.kernel.org/patchwork/project/lkml/list/?series=348697
> [v2] : https://lore.kernel.org/patchwork/project/lkml/list/?series=359012
> 
> Amit Nischal (4):
>   clk: qcom: gdsc: Add support to enable/disable the clocks with GDSC
>   clk: qcom: Add clk_rcg2_gfx3d_ops for SDM845
>   dt-bindings: clock: Introduce QCOM Graphics clock bindings
>   clk: qcom: Add graphics clock controller driver for SDM845
> 
>  .../devicetree/bindings/clock/qcom,gpucc.txt       |  18 +
>  drivers/clk/qcom/Kconfig                           |   9 +
>  drivers/clk/qcom/Makefile                          |   1 +
>  drivers/clk/qcom/clk-rcg.h                         |   1 +
>  drivers/clk/qcom/clk-rcg2.c                        |  86 +++-
>  drivers/clk/qcom/gdsc.c                            |  44 +++
>  drivers/clk/qcom/gdsc.h                            |   5 +
>  drivers/clk/qcom/gpucc-sdm845.c                    | 438 +++++++++++++++++++++
>  include/dt-bindings/clock/qcom,gpucc-sdm845.h      |  38 ++
>  9 files changed, 638 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,gpucc.txt
>  create mode 100644 drivers/clk/qcom/gpucc-sdm845.c
>  create mode 100644 include/dt-bindings/clock/qcom,gpucc-sdm845.h

This seems as good a place as any to kick off this discussion since it will
influence what these patches look like the next go around. Settle in because
this is going to get wordy...

The sdm845 GPU is comprised of two nested power domains: CX and GX. Put simply
from a CPU perspective the CX domain controls the GMU which is a microprocessor
co-located with the GPU. The GMU in turn controls the GX domain which powers the
GPU hardware. Under normal conditions the CPU powers on the CX domain and boots
the GMU and then the GMU turns on the GX and controls it under the assumption
that the GMU firmware can handle power collapse much faster than the CPU. The
GMU continues to control the GX domain up and down until we are idle for long
enough that we want to suspend the CX domain as well.

When we want to suspend the GMU we trigger a shutdown process on the GMU
which turns off GX and then the CPU turns off CX through the usual
pm_runtime_put() sequence. Under these ideal circumstances the CPU should have
no visibility into the GX at all - we should never control any headswitches or
clocks anywhere in that domain. Except....

There is one case where the CPU needs to touch the GX: due to power sequencing
requirements the GX needs to be turned off before the CX (and the CX needs to be
enabled before the GX during power up). In the very rare situation where the
GMU itself crashes and leaves the GX headswitch on the CPU needs to reach in and
turn off the GX before turning off the CX and rebooting the GMU. This is
problematic from a CPU perspective because there is no way to just go in and
hack on the register directly; we need some infrastructure set up and ready to
handle the odd use case.

We discussed this a while at LPC and Stephen had a good idea. We should keep the
GX gdsc but hack it so that the enable function does nothing (because we don't
want the GPU to turn on the headswitch ever) but leave the disable function as
is. Then we attach that genpd with dev_pm_attach_by_name() to the GMU device
and power enable/disable it at the appropriate times. Most of the time the
disable call will write to an already disabled headswitch but in that very
special situation it will actually turn off the headswitch and the world can be
right again.

So I did this and it works. At least it works for the regular case. It is
really difficult to cause the GMU to fault with the GX left on; I think I'm
going to need to hack on the firmware to test that path but I've verified that
the modified GDSC is toggling the power off before we suspend the CX so I think
thats a win.

I'll post patches against this patchset for an RFC but I mention it here because
there are a bunch of clocks defined in gpucc-sdm845.c that are not needed (
either we never call them because we don't touch GX clocks or they are parents
of uneeded clocks). It also seems like we don't need the clk_hws/clk_count
infrastructure in gdsc.c. From my testing I can safely toggle the gx gdsc off
from the CPU without 'gpu_cc_gx_gfx3d_clk_src'.

Obviously we'll need some more review and I need the actual clock experts to
weigh in but this is encouraging news because I think it lets us move ahead with
gpucc and also solve my little nagging hardware workaround issue to boot.

Look for patches shortly, in the mean time I'm going to comment to point out the
clocks I don't think we need any more so Taniya can make a new patchset.

Jordan
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists