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@xxxxxxxxxx>
>>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@xxxxxx>
>> ---
>> .../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.
>> +- 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.
>> +- 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.
>> 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.
>> + 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.
>> +
>> + 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.
Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
