[PATCH 2/8] clk: keystone: Add gate control clock driver

[mturquette@linaro.org (Mike Turquette)]

Quoting Mark Rutland (2013-08-13 09:53:44)
> On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote:
> > On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote:
> > > [adding dt maintainers]
> > > 
> > > On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote:
> > >> Add the driver for the clock gate control which uses PSC (Power Sleep
> > >> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and
> > >> disabling of the clocks for different IPs present in the SoC.
> > >>
> > >> Cc: Mike Turquette <mturquette@linaro.org>
> > >>
> > >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > >> ---
> > >>  .../devicetree/bindings/clock/keystone-gate.txt    |   30 ++
> > >>  drivers/clk/keystone/gate.c                        |  306 ++++++++++++++++++++
> > >>  include/linux/clk/keystone.h                       |    1 +
> > >>  3 files changed, 337 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
> > >>  create mode 100644 drivers/clk/keystone/gate.c
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> > >> new file mode 100644
> > >> index 0000000..40afef5
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> > >> @@ -0,0 +1,30 @@
> > >> +Binding for Keystone gate control driver which uses PSC controller IP.
> > >> +
> > >> +This binding uses the common clock binding[1].
> > >> +
> > >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >> +
> > >> +Required properties:
> > >> +- compatible : shall be "keystone,psc-clk".
> > > 
> > > Similarly to my comments on patch 1, this should probably be something
> > > more like "ti,keystone-psc-clock"
> > > 
> > >> +- #clock-cells : from common clock binding; shall be set to 0.
> > >> +- clocks : parent clock phandle
> > >> +- reg :        psc address address space
> > >> +
> > >> +Optional properties:
> > >> +- clock-output-names : From common clock binding to override the
> > >> +                       default output clock name
> > >> +- status : "enabled" if clock is always enabled
> > > 
> > > Huh? This is a standard property, for which ePAPR defines "okay",
> > > "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't
> > > seem to follow the standard meaning judging by the code.
> > > 
> > > It looks like this could be a boolean property
> > > ("clock-always-enabled"?), but that assumes it's necessary.
> > > 
> > > Why do you need this?
> > > 
> > I dropped this already. Forgot to update the documentation.
> 
> Ok.
> 
> > 
> > >> +- lpsc : lpsc module id, if not set defaults to zero
> > > 
> > > What's that needed for? Are there multiple clocks sharing a common
> > > register bank?
> > > 
> > >> +- pd : power domain number, if not set defaults to zero (always ON)
> > > 
> > > Please don't use abbreviations unnecessarily. "power-domain-id" or
> > > similar would be far better. What exactly does this represent? Does the
> > > clock IP itself handle power domains, or is there some external unit
> > > that controls power domains?
> > > 
> > pd is commonly used but I can expand it. This represent the power
> > domain number.
> 
> Does the the clock IP have some internal logic for handling power
> domains only covering its clocks, or is there some external power
> controller involved (i.e. do we possibly need to describe an external
> unit and a linkage to it?).

Hmm, does the clock own the power domain or does the power domain own
the clock? As you well know on OMAP the clock resides within the power
domain. So it seems to me that the better way to do this would be for
the power domain to have it's own binding and node, and then reference
the clock:

power-domain {
	...
	clocks = <&chipclk3>, <&chipclk4>;
	clock-names = "perclk", "uartclk";
	...
};

Now maybe things are different on Keystone, but if not then I don't see
why the clock binding should have anything to do with the power domain.

> 
> > 
> > >> +- gpsc : gpsc number, if not set defaults to zero
> > > 
> > > How does that interact with the lpsc property? When are these necessary?
> > > 
> > lpsc is local clock/module control where as gpsc sits on top of it. 

Similar to the above, should the gpsc have it's own binding and node
that contains the lpsc which also have their own bindings/nodes, which
finally contain the clocks? Maybe that is over engineered but I want to
consider this before the binding gets set in stone.

Regards,
Mike

> 
> Ok. I'm not sure haivng these default to zero makes much sense, as that
> could easily hide errors in dts. It might make more sense to make them
> required and error out if they aren't present.
> 
> Unless they're zero far more often?
> 
> > 
> > >> diff --git a/drivers/clk/keystone/gate.c b/drivers/clk/keystone/gate.c
> > >> new file mode 100644
> > >> index 0000000..72ac478
> > >> --- /dev/null
> > >> +++ b/drivers/clk/keystone/gate.c
> > 
> > [..]
> > 
> > >> +/**
> > >> + * clk_register_psc - register psc clock
> > >> + * @dev: device that is registering this clock
> > >> + * @name: name of this clock
> > >> + * @parent_name: name of clock's parent
> > >> + * @psc_data: platform data to configure this clock
> > >> + * @lock: spinlock used by this clock
> > >> + */
> > >> +static struct clk *clk_register_psc(struct device *dev,
> > >> +                       const char *name,
> > >> +                       const char *parent_name,
> > >> +                       struct clk_psc_data *psc_data,
> > >> +                       spinlock_t *lock)
> > >> +{
> > >> +       struct clk_init_data init;
> > >> +       struct clk_psc *psc;
> > >> +       struct clk *clk;
> > >> +
> > >> +       psc = kzalloc(sizeof(*psc), GFP_KERNEL);
> > >> +       if (!psc)
> > >> +               return ERR_PTR(-ENOMEM);
> > >> +
> > >> +       init.name = name;
> > >> +       init.ops = &clk_psc_ops;
> > >> +       init.flags = psc_data->flags;
> > >> +       init.parent_names = (parent_name ? &parent_name : NULL);
> > > 
> > > Is &parent_name not a pointer to a pointer on the stack? Is this only
> > > used once?
> > > 
> > >> +       init.num_parents = (parent_name ? 1 : 0);
> > >> +
> > >> +       psc->psc_data = psc_data;
> > >> +       psc->lock = lock;
> > >> +       psc->hw.init = &init;
> > > 
> > > That's definitely a pointer to some data on the stack, and that seems to
> > > be kept around. Is this only used for one-time initialisation, or might
> > > it be used later?
> > > 
> > Its init only.
> 
> Ok.
> 
> > 
> > >> +       data->base = psc_addr[gpsc].io_base;
> > >> +       data->lpsc = lpsc;
> > >> +       data->gpsc = gpsc;
> > >> +       data->domain = pd;
> > >> +
> > >> +       if (of_property_read_bool(node, "ti,psc-lreset"))
> > >> +               data->psc_flags |= PSC_LRESET;
> > >> +       if (of_property_read_bool(node, "ti,psc-force"))
> > >> +               data->psc_flags |= PSC_FORCE;
> > > 
> > > Neither of these were defined in the binding, and they don't appear to
> > > have been inherited form another binding. What are they for? Why are
> > > they needed?
> > > 
> > They represent the hardware support transition method needed to have
> > proper clock enable/disable sequence to work. Will update the binding
> > along with other comments which I agree.
> 
> Ok.
> 
> > 
> > >> +
> > >> +       clk = clk_register_psc(NULL, clk_name, parent_name,
> > >> +                               data, lock);
> > >> +
> > >> +       if (clk) {
> > >> +               of_clk_add_provider(node, of_clk_src_simple_get, clk);
> > >> +
> > >> +               rc = of_property_read_string(node, "status", &status);
> > >> +               if (status && !strcmp(status, "enabled"))
> > >> +                       clk_prepare_enable(clk);
> > >> +               return;
> > >> +       }
> > > 
> > > As mentioned above, this abuses the standard status property, and it's
> > > not clear why this is necessary.
> > > 
> > Actually I have removed this one from dt nodes. Looks like missed
> > to pick the right patch while posting. Have dropped this change
> > already.
> 
> Great!
> 
> Thanks,
> Mark.