Re: [PATCH v1 09/14] clk: msm: Add support for MSM8960's global clock controller (GCC)

[Stephen Boyd <sboyd@codeaurora.org>]

On 08/08, Mark Rutland wrote:
> Hi Stephen,
> 
> On Thu, Jul 25, 2013 at 01:43:37AM +0100, Stephen Boyd wrote:
> > Fill in the data and wire up the global clock controller to the
> > MSM clock driver. This should allow most non-multimedia device
> > drivers to control their clocks on 8960 based platforms.
> > 
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > ---
> >  .../devicetree/bindings/clock/qcom,gcc.txt         |  55 +++++++
> >  drivers/clk/msm/Kconfig                            |  10 ++
> >  drivers/clk/msm/Makefile                           |   2 +
> >  drivers/clk/msm/core.c                             |   3 +
> >  drivers/clk/msm/gcc-8960.c                         | 174 +++++++++++++++++++++
> >  drivers/clk/msm/internal.h                         |   2 +
> >  6 files changed, 246 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/qcom,gcc.txt
> >  create mode 100644 drivers/clk/msm/gcc-8960.c
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > new file mode 100644
> > index 0000000..2311e1a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > @@ -0,0 +1,55 @@
> > +MSM Global Clock Controller Binding
> > +-----------------------------------
> > +
> > +Required properties :
> > +- compatible : shall contain at least "qcom,gcc" and only one of the
> > +              following:
> > +
> > +                       "qcom,gcc-8660"
> > +                       "qcom,gcc-8960"
> > +
> > +- reg : shall contain base register location and length
> > +- clocks : shall contain clocks supplied by the clock controller
> > +
> > +Example:
> > +       clock-controller@900000 {
> > +               compatible = "qcom,gcc-8960", "qcom,gcc";
> > +               reg = <0x900000 0x4000>;
> > +
> > +               clocks {
> > +                       pxo: pxo {
> > +                               #clock-cells = <0>;
> > +                               compatible = "fixed-clock";
> > +                               clock-frequency = <27000000>;
> > +                       };
> > +
> > +                       pll8: pll8 {
> > +                               #clock-cells = <0>;
> > +                               compatible = "qcom,pll";
> > +                               clocks = <&pxo>;
> > +                       };
> > +
> > +                       vpll8: vpll8 {
> > +                               #clock-cells = <0>;
> > +                               compatible = "qcom,pll-vote";
> > +                               clocks = <&pll8>;
> > +                       };
> > +
> > +                       gsbi5_uart_rcg: gsbi5_uart_rcg {
> > +                               #clock-cells = <0>;
> > +                               compatible = "qcom,p2-mn16-clock";
> > +                               clocks = <&pxo>, <&vpll8>;
> > +                       };
> > +
> > +                       gsbi5_uart_clk: gsbi5_uart_cxc {
> > +                               #clock-cells = <0>;
> > +                               compatible = "qcom,cxc-clock";
> > +                               clocks = <&gsbi5_uart_rcg>;
> > +                       };
> > +
> > +                       gsbi5_uart_ahb: gsbi5_uart_ahb {
> > +                               #clock-cells = <0>;
> > +                               compatible = "qcom,cxc-hg-clock";
> > +                       };
> > +               };
> > +       };
> 
> I'm slightly confused by this. How is each of the clocks described in
> the clocks node related to a portion of the register set?

The registers to control clocks and determine their state are
scattered throughout the registers in the gcc (in this example
from 0x900000 to 0x903fff). If you match up gsbi5_uart_rcg with
its C struct counterpart you'll notice that there are multiple
registers used to configure the clock. It isn't as simple as one
reg property per clock even for the case where we're just
toggling a bit to turn a clock on and off either. And it isn't as
simple as saying the clock has a base register that we can offset
from because offsets are almost always different (we've tried
to correct this in future chip versions).

> 
> If the set of clocks is fixed, surely the gcc node gives you enough
> information alone, and the whole block can be modelled as a single
> provider of multiple clock outputs, or it's not fixed, and some linkage
> needs to be defined?
> 
> The code seems to imply the former, unless only a subset of clocks may
> be present? In that case, the set of clocks which might be present
> should be described in the binding.

The clock controller is hardware and the number of clock outputs
is fixed. Isn't all hardware fixed until you start talking about
FPGAs? The next minor revision of the clock controller may add
more clocks or remove clocks from that base design, but otherwise
the two are 90% the same and generally software compatible. It
isn't until we start a new generation of chips that we make major
changes to the design. Is that loose enough to qualify?

These bindings attempt to follow the regulator bindings. With
regulators there is a node for each regulator and we describe
physical characteristics of those regulators within the nodes but
we don't describe the software interface (bits, masks, shifts,
etc). I imagine we could extend these clock nodes to describe
physical characteristics such as min/max frequency or if the
bootloader has left the clocks on. Right now we're using the
nodes to describe what types of clocks there are and how the
clock tree is layed out.

Or perhaps you're talking about clock sharing? We share the clock
controller with multiple masters (processors running other OSes)
and the partitioning of the clocks is mostly predefined. We just
won't use some clocks because they're reserved for other
processors. They're still part of the same clock controller
hardware block but we don't want to control them on Linux because
we'll trample over other processors and most likely hang the
system. I wonder how this would work for hexagon and krait both
running linux on the same SoC. If all DT says is that there is a
gcc here at this address how are we supposed to know that we
shouldn't use some clock? In fact we have some clocks that are
"voteable" in the sense that each master has its own register to
vote for a clock to be on or off. The registers are all ORed
together by hardware to determine if the clock should be on or
not. I should probably rename those clocks to have a _krait or
_apps at the end so that it's clear we want to instantiate the
krait version of the clock and not the hexagon version. I suppose
the other solution there is to say we have gcc-8960-krait and
gcc-8960-hexagon so we know which voting registers to use or put
an ifdef ARCH_HEXAGON/ARCH_ARM. Is that the right solution?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation