Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Bjorn Andersson]

On Wed 02 Nov 15:55 PDT 2016, Stephen Boyd wrote:

> On 11/02, Bjorn Andersson wrote:
> > On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote:
> > 
> > > On 10/19, Georgi Djakov wrote:
> > > > Add a driver for the A53 Clock Controller. It is a hardware block that
> > > > implements a combined mux and half integer divider functionality. It can
> > > > choose between a fixed-rate clock or the dedicated A53 PLL. The source
> > > > and the divider can be set both at the same time.
> > > > 
> > > > This is required for enabling CPU frequency scaling on platforms like
> > > > MSM8916.
> > > > 
> > > 
> > > Please Cc DT reviewers.
> > > 
> > > > Signed-off-by: Georgi Djakov <georgi.djakov@xxxxxxxxxx>
> > > > ---
> > > >  .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
> > > >  drivers/clk/qcom/Kconfig                           |   8 ++
> > > >  drivers/clk/qcom/Makefile                          |   1 +
> > > >  drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
> > > >  4 files changed, 186 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > > >  create mode 100644 drivers/clk/qcom/a53cc.c
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > > > new file mode 100644
> > > > index 000000000000..a025f062f177
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > > > @@ -0,0 +1,22 @@
> > > > +Qualcomm A53 CPU Clock Controller Binding
> > > > +------------------------------------------------
> > > > +The A53 CPU Clock Controller is hardware, which provides a combined
> > > > +mux and divider functionality for the CPU clocks. It can choose between
> > > > +a fixed rate clock and the dedicated A53 PLL.
> > > > +
> > > > +Required properties :
> > > > +- compatible : shall contain:
> > > > +
> > > > +			"qcom,a53cc"
> > > > +
> > > > +- reg : shall contain base register location and length
> > > > +	of the APCS region
> > > > +- #clock-cells : shall contain 1
> > > > +
> > > > +Example:
> > > > +
> > > > +	apcs: syscon@b011000 {
> > > > +		compatible = "qcom,a53cc", "syscon";
> > > 
> > > Why is it a syscon? Is that part used?
> > > 
> > 
> > I use the register at offset 8 for interrupting the other subsystems, so
> > this must be available as something I can poke.
> > 
> > Which makes me think that this should be described as a "simple-mfd" and
> > "syscon" with the a53cc node as a child - grabbing the regmap of the
> > syscon parent, rather then ioremapping the same region again.
> > 
> 
> That's sort of a question for DT reviewers. The register space
> certainly seems like a free for all with a tilt toward power
> management of the CPU, similar to how this was done on Krait
> based designs.
> 

Right. But this kind of mashup blocks was the reason why simple-mfd was
put in place.

> I wonder why we didn't make up some provider/consumer binding for
> the "kicking" feature used by SMD/RPM code. Then this could be a
> clock provider and a "kick" provider (haha #kick-cells) and the
> usage of syscon/regmap wouldn't be mandatory.
> 

I did consider doing that, but had enough dependencies to put in place
as it was.

I'm in favour of us inventing a kicker API and it's found outside out
use cases as well (e.g. virtio/rpmsg).

Regards,
Bjorn