LinuxLists.cc - [RESEND 0/2] clk: Support for Palmas clk32kg and clk32kgaudio clocks

2014-06-27 06:01:23

Subject: [RESEND 0/2] clk: Support for Palmas clk32kg and clk32kgaudio clocks

Hi Mike,

This is a resend of the v2 version of the palmas clock driver which seamingly
missed the 3.16 merge window. I have added Nishanth's Reviewed-by tag to the
patches.

Changes since v1:
- binding documentation and driver has been separated based on Nishanth Menon's
comment

v2 of the series:
https://lkml.org/lkml/2014/5/6/323

v1 of the driver can be found:
https://lkml.org/lkml/2014/4/3/104

Palmas class of devices can provide 32K clock(s) to be used by other devices
on the board. Depending on the actual device the provided clocks can be:
CLK32K_KG and CLK32K_KGAUDIO
or only one:
CLK32K_KG (TPS659039 for example)

Use separate compatible flags for the two 32K clock.
A system which needs or have only one of the 32k clock from
Palmas will need to add node(s) for each clock as separate section
in the dts file.
The two compatible property is:
"ti,palmas-clk32kg" for clk32kg clock
"ti,palmas-clk32kgaudio" for clk32kgaudio clock

Apart from the register control of the clocks - which is done via
the clock API there is a posibility to enable the external sleep
control. In this way the clock can be enabled/disabled on demand by the
user of the clock.

Regards,
Peter
---
Peter Ujfalusi (2):
dt/bindings: Binding documentation for Palmas clk32kg and clk32kgaudio
clocks
clk: Add driver for Palmas clk32kg and clk32kgaudio clocks

.../bindings/clock/clk-palmas-clk32kg-clocks.txt | 35 +++
drivers/clk/Kconfig | 7 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-palmas.c | 307 +++++++++++++++++++++
include/dt-bindings/mfd/palmas.h | 18 ++
5 files changed, 368 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/clk-palmas-clk32kg-clocks.txt
create mode 100644 drivers/clk/clk-palmas.c
create mode 100644 include/dt-bindings/mfd/palmas.h

--
2.0.0

2014-06-27 06:01:31

by Peter Ujfalusi

[permalink] [raw]

Subject: [RESEND 2/2] clk: Add driver for Palmas clk32kg and clk32kgaudio clocks

Palmas class of devices can provide 32K clock(s) to be used by other devices
on the board. Depending on the actual device the provided clocks can be:
CLK32K_KG and CLK32K_KGAUDIO
or only one:
CLK32K_KG (TPS659039 for example)

Use separate compatible flags for the two 32K clock.
A system which needs or have only one of the 32k clock from
Palmas will need to add node(s) for each clock as separate section
in the dts file.
The two compatible property is:
"ti,palmas-clk32kg" for clk32kg clock
"ti,palmas-clk32kgaudio" for clk32kgaudio clock

Apart from the register control of the clocks - which is done via
the clock API there is a posibility to enable the external sleep
control. In this way the clock can be enabled/disabled on demand by the
user of the clock.

See the documentation for more details.

Signed-off-by: Peter Ujfalusi <[email protected]>
Reviewed-by: Nishanth Menon <[email protected]>
---
drivers/clk/Kconfig | 7 ++
drivers/clk/Makefile | 1 +
drivers/clk/clk-palmas.c | 307 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 315 insertions(+)
create mode 100644 drivers/clk/clk-palmas.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 9f9c5ae5359b..cfd3af7b2cbd 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -102,6 +102,13 @@ config COMMON_CLK_KEYSTONE
Supports clock drivers for Keystone based SOCs. These SOCs have local
a power sleep control module that gate the clock to the IPs and PLLs.

+config COMMON_CLK_PALMAS
+ tristate "Clock driver for TI Palmas devices"
+ depends on MFD_PALMAS
+ ---help---
+ This driver supports TI Palmas devices 32KHz output KG and KG_AUDIO
+ using common clock framework.
+
source "drivers/clk/qcom/Kconfig"

endmenu
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 567f10259029..312742c10661 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
obj-$(CONFIG_ARCH_MOXART) += clk-moxart.o
obj-$(CONFIG_ARCH_NOMADIK) += clk-nomadik.o
obj-$(CONFIG_ARCH_NSPIRE) += clk-nspire.o
+obj-$(CONFIG_COMMON_CLK_PALMAS) += clk-palmas.o
obj-$(CONFIG_CLK_PPC_CORENET) += clk-ppc-corenet.o
obj-$(CONFIG_COMMON_CLK_S2MPS11) += clk-s2mps11.o
obj-$(CONFIG_COMMON_CLK_SI5351) += clk-si5351.o
diff --git a/drivers/clk/clk-palmas.c b/drivers/clk/clk-palmas.c
new file mode 100644
index 000000000000..781630e1372b
--- /dev/null
+++ b/drivers/clk/clk-palmas.c
@@ -0,0 +1,307 @@
+/*
+ * Clock driver for Palmas device.
+ *
+ * Copyright (c) 2013, NVIDIA Corporation.
+ * Copyright (c) 2013-2014 Texas Instruments, Inc.
+ *
+ * Author: Laxman Dewangan <[email protected]>
+ * Peter Ujfalusi <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
+ * whether express or implied; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/mfd/palmas.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define PALMAS_CLOCK_DT_EXT_CONTROL_ENABLE1 1
+#define PALMAS_CLOCK_DT_EXT_CONTROL_ENABLE2 2
+#define PALMAS_CLOCK_DT_EXT_CONTROL_NSLEEP 3
+
+struct palmas_clk32k_desc {
+ const char *clk_name;
+ unsigned int control_reg;
+ unsigned int enable_mask;
+ unsigned int sleep_mask;
+ unsigned int sleep_reqstr_id;
+ int delay;
+};
+
+struct palmas_clock_info {
+ struct device *dev;
+ struct clk *clk;
+ struct clk_hw hw;
+ struct palmas *palmas;
+ struct palmas_clk32k_desc *clk_desc;
+ int ext_control_pin;
+};
+
+static inline struct palmas_clock_info *to_palmas_clks_info(struct clk_hw *hw)
+{
+ return container_of(hw, struct palmas_clock_info, hw);
+}
+
+static unsigned long palmas_clks_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ return 32768;
+}
+
+static int palmas_clks_prepare(struct clk_hw *hw)
+{
+ struct palmas_clock_info *cinfo = to_palmas_clks_info(hw);
+ int ret;
+
+ ret = palmas_update_bits(cinfo->palmas, PALMAS_RESOURCE_BASE,
+ cinfo->clk_desc->control_reg,
+ cinfo->clk_desc->enable_mask,
+ cinfo->clk_desc->enable_mask);
+ if (ret < 0)
+ dev_err(cinfo->dev, "Reg 0x%02x update failed, %d\n",
+ cinfo->clk_desc->control_reg, ret);
+ else if (cinfo->clk_desc->delay)
+ udelay(cinfo->clk_desc->delay);
+
+ return ret;
+}
+
+static void palmas_clks_unprepare(struct clk_hw *hw)
+{
+ struct palmas_clock_info *cinfo = to_palmas_clks_info(hw);
+ int ret;
+
+ /*
+ * Clock can be disabled through external pin if it is externally
+ * controlled.
+ */
+ if (cinfo->ext_control_pin)
+ return;
+
+ ret = palmas_update_bits(cinfo->palmas, PALMAS_RESOURCE_BASE,
+ cinfo->clk_desc->control_reg,
+ cinfo->clk_desc->enable_mask, 0);
+ if (ret < 0)
+ dev_err(cinfo->dev, "Reg 0x%02x update failed, %d\n",
+ cinfo->clk_desc->control_reg, ret);
+}
+
+static int palmas_clks_is_prepared(struct clk_hw *hw)
+{
+ struct palmas_clock_info *cinfo = to_palmas_clks_info(hw);
+ int ret;
+ u32 val;
+
+ if (cinfo->ext_control_pin)
+ return 1;
+
+ ret = palmas_read(cinfo->palmas, PALMAS_RESOURCE_BASE,
+ cinfo->clk_desc->control_reg, &val);
+ if (ret < 0) {
+ dev_err(cinfo->dev, "Reg 0x%02x read failed, %d\n",
+ cinfo->clk_desc->control_reg, ret);
+ return ret;
+ }
+ return !!(val & cinfo->clk_desc->enable_mask);
+}
+
+static struct clk_ops palmas_clks_ops = {
+ .prepare = palmas_clks_prepare,
+ .unprepare = palmas_clks_unprepare,
+ .is_prepared = palmas_clks_is_prepared,
+ .recalc_rate = palmas_clks_recalc_rate,
+};
+
+struct palmas_clks_of_match_data {
+ struct clk_init_data init;
+ struct palmas_clk32k_desc desc;
+};
+
+static struct palmas_clks_of_match_data palmas_of_clk32kg = {
+ .init = {
+ .name = "clk32kg",
+ .ops = &palmas_clks_ops,
+ .flags = CLK_IS_ROOT | CLK_IGNORE_UNUSED,
+ },
+ .desc = {
+ .clk_name = "clk32kg",
+ .control_reg = PALMAS_CLK32KG_CTRL,
+ .enable_mask = PALMAS_CLK32KG_CTRL_MODE_ACTIVE,
+ .sleep_mask = PALMAS_CLK32KG_CTRL_MODE_SLEEP,
+ .sleep_reqstr_id = PALMAS_EXTERNAL_REQSTR_ID_CLK32KG,
+ .delay = 200,
+ },
+};
+
+static struct palmas_clks_of_match_data palmas_of_clk32kgaudio = {
+ .init = {
+ .name = "clk32kgaudio",
+ .ops = &palmas_clks_ops,
+ .flags = CLK_IS_ROOT | CLK_IGNORE_UNUSED,
+ },
+ .desc = {
+ .clk_name = "clk32kgaudio",
+ .control_reg = PALMAS_CLK32KGAUDIO_CTRL,
+ .enable_mask = PALMAS_CLK32KG_CTRL_MODE_ACTIVE,
+ .sleep_mask = PALMAS_CLK32KG_CTRL_MODE_SLEEP,
+ .sleep_reqstr_id = PALMAS_EXTERNAL_REQSTR_ID_CLK32KGAUDIO,
+ .delay = 200,
+ },
+};
+
+static struct of_device_id palmas_clks_of_match[] = {
+ {
+ .compatible = "ti,palmas-clk32kg",
+ .data = &palmas_of_clk32kg,
+ },
+ {
+ .compatible = "ti,palmas-clk32kgaudio",
+ .data = &palmas_of_clk32kgaudio,
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(of, palmas_clks_of_match);
+
+static void palmas_clks_get_clk_data(struct platform_device *pdev,
+ struct palmas_clock_info *cinfo)
+{
+ struct device_node *node = pdev->dev.of_node;
+ unsigned int prop;
+ int ret;
+
+ ret = of_property_read_u32(node, "ti,external-sleep-control",
+ &prop);
+ if (ret)
+ return;
+
+ switch (prop) {
+ case PALMAS_CLOCK_DT_EXT_CONTROL_ENABLE1:
+ prop = PALMAS_EXT_CONTROL_ENABLE1;
+ break;
+ case PALMAS_CLOCK_DT_EXT_CONTROL_ENABLE2:
+ prop = PALMAS_EXT_CONTROL_ENABLE2;
+ break;
+ case PALMAS_CLOCK_DT_EXT_CONTROL_NSLEEP:
+ prop = PALMAS_EXT_CONTROL_NSLEEP;
+ break;
+ default:
+ dev_warn(&pdev->dev, "%s: Invalid ext control option: %u\n",
+ node->name, prop);
+ prop = 0;
+ break;
+ }
+ cinfo->ext_control_pin = prop;
+}
+
+static int palmas_clks_init_configure(struct palmas_clock_info *cinfo)
+{
+ int ret;
+
+ ret = palmas_update_bits(cinfo->palmas, PALMAS_RESOURCE_BASE,
+ cinfo->clk_desc->control_reg,
+ cinfo->clk_desc->sleep_mask, 0);
+ if (ret < 0) {
+ dev_err(cinfo->dev, "Reg 0x%02x update failed, %d\n",
+ cinfo->clk_desc->control_reg, ret);
+ return ret;
+ }
+
+ if (cinfo->ext_control_pin) {
+ ret = clk_prepare(cinfo->clk);
+ if (ret < 0) {
+ dev_err(cinfo->dev, "Clock prep failed, %d\n", ret);
+ return ret;
+ }
+
+ ret = palmas_ext_control_req_config(cinfo->palmas,
+ cinfo->clk_desc->sleep_reqstr_id,
+ cinfo->ext_control_pin, true);
+ if (ret < 0) {
+ dev_err(cinfo->dev, "Ext config for %s failed, %d\n",
+ cinfo->clk_desc->clk_name, ret);
+ return ret;
+ }
+ }
+
+ return ret;
+}
+static int palmas_clks_probe(struct platform_device *pdev)
+{
+ struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
+ struct device_node *node = pdev->dev.of_node;
+ struct palmas_clks_of_match_data *match_data;
+ const struct of_device_id *match;
+ struct palmas_clock_info *cinfo;
+ struct clk *clk;
+ int ret;
+
+ match = of_match_device(palmas_clks_of_match, &pdev->dev);
+ match_data = (struct palmas_clks_of_match_data *)match->data;
+
+ cinfo = devm_kzalloc(&pdev->dev, sizeof(*cinfo), GFP_KERNEL);
+ if (!cinfo)
+ return -ENOMEM;
+
+ palmas_clks_get_clk_data(pdev, cinfo);
+ platform_set_drvdata(pdev, cinfo);
+
+ cinfo->dev = &pdev->dev;
+ cinfo->palmas = palmas;
+
+ cinfo->clk_desc = &match_data->desc;
+ cinfo->hw.init = &match_data->init;
+ clk = devm_clk_register(&pdev->dev, &cinfo->hw);
+ if (IS_ERR(clk)) {
+ ret = PTR_ERR(clk);
+ dev_err(&pdev->dev, "Fail to register clock %s, %d\n",
+ match_data->desc.clk_name, ret);
+ return ret;
+ }
+
+ cinfo->clk = clk;
+ ret = palmas_clks_init_configure(cinfo);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Clock config failed, %d\n", ret);
+ return ret;
+ }
+
+ ret = of_clk_add_provider(node, of_clk_src_simple_get, cinfo->clk);
+ if (ret < 0)
+ dev_err(&pdev->dev, "Fail to add clock driver, %d\n", ret);
+ return ret;
+}
+
+static int palmas_clks_remove(struct platform_device *pdev)
+{
+ of_clk_del_provider(pdev->dev.of_node);
+ return 0;
+}
+
+static struct platform_driver palmas_clks_driver = {
+ .driver = {
+ .name = "palmas-clk",
+ .owner = THIS_MODULE,
+ .of_match_table = palmas_clks_of_match,
+ },
+ .probe = palmas_clks_probe,
+ .remove = palmas_clks_remove,
+};
+
+module_platform_driver(palmas_clks_driver);
+
+MODULE_DESCRIPTION("Clock driver for Palmas Series Devices");
+MODULE_ALIAS("platform:palmas-clk");
+MODULE_AUTHOR("Peter Ujfalusi <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.0.0

2014-06-27 06:01:27

by Peter Ujfalusi

[permalink] [raw]

Subject: [RESEND 1/2] dt/bindings: Binding documentation for Palmas clk32kg and clk32kgaudio clocks

2014-06-27 18:23:16

by Javier Martinez Canillas

[permalink] [raw]

Subject: Re: [RESEND 2/2] clk: Add driver for Palmas clk32kg and clk32kgaudio clocks

Hello Peter,

On Fri, Jun 27, 2014 at 8:01 AM, Peter Ujfalusi <[email protected]> wrote:
> Palmas class of devices can provide 32K clock(s) to be used by other devices
> on the board. Depending on the actual device the provided clocks can be:
> CLK32K_KG and CLK32K_KGAUDIO
> or only one:
> CLK32K_KG (TPS659039 for example)
>
> Use separate compatible flags for the two 32K clock.
> A system which needs or have only one of the 32k clock from
> Palmas will need to add node(s) for each clock as separate section
> in the dts file.
> The two compatible property is:
> "ti,palmas-clk32kg" for clk32kg clock
> "ti,palmas-clk32kgaudio" for clk32kgaudio clock
>
> Apart from the register control of the clocks - which is done via
> the clock API there is a posibility to enable the external sleep
> control. In this way the clock can be enabled/disabled on demand by the
> user of the clock.
>
> See the documentation for more details.
>
> Signed-off-by: Peter Ujfalusi <[email protected]>
> Reviewed-by: Nishanth Menon <[email protected]>
> ---
> drivers/clk/Kconfig | 7 ++
> drivers/clk/Makefile | 1 +
> drivers/clk/clk-palmas.c | 307 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 315 insertions(+)
> create mode 100644 drivers/clk/clk-palmas.c
>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 9f9c5ae5359b..cfd3af7b2cbd 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -102,6 +102,13 @@ config COMMON_CLK_KEYSTONE
> Supports clock drivers for Keystone based SOCs. These SOCs have local
> a power sleep control module that gate the clock to the IPs and PLLs.
>
> +config COMMON_CLK_PALMAS
> + tristate "Clock driver for TI Palmas devices"
> + depends on MFD_PALMAS
> + ---help---
> + This driver supports TI Palmas devices 32KHz output KG and KG_AUDIO
> + using common clock framework.
> +
> source "drivers/clk/qcom/Kconfig"
>
> endmenu
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 567f10259029..312742c10661 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
> obj-$(CONFIG_ARCH_MOXART) += clk-moxart.o
> obj-$(CONFIG_ARCH_NOMADIK) += clk-nomadik.o
> obj-$(CONFIG_ARCH_NSPIRE) += clk-nspire.o
> +obj-$(CONFIG_COMMON_CLK_PALMAS) += clk-palmas.o
> obj-$(CONFIG_CLK_PPC_CORENET) += clk-ppc-corenet.o
> obj-$(CONFIG_COMMON_CLK_S2MPS11) += clk-s2mps11.o
> obj-$(CONFIG_COMMON_CLK_SI5351) += clk-si5351.o
> diff --git a/drivers/clk/clk-palmas.c b/drivers/clk/clk-palmas.c
> new file mode 100644
> index 000000000000..781630e1372b
> --- /dev/null
> +++ b/drivers/clk/clk-palmas.c
> @@ -0,0 +1,307 @@
> +/*
> + * Clock driver for Palmas device.
> + *
> + * Copyright (c) 2013, NVIDIA Corporation.
> + * Copyright (c) 2013-2014 Texas Instruments, Inc.
> + *
> + * Author: Laxman Dewangan <[email protected]>
> + * Peter Ujfalusi <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
> + * whether express or implied; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/mfd/palmas.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define PALMAS_CLOCK_DT_EXT_CONTROL_ENABLE1 1
> +#define PALMAS_CLOCK_DT_EXT_CONTROL_ENABLE2 2
> +#define PALMAS_CLOCK_DT_EXT_CONTROL_NSLEEP 3
> +
> +struct palmas_clk32k_desc {
> + const char *clk_name;
> + unsigned int control_reg;
> + unsigned int enable_mask;
> + unsigned int sleep_mask;
> + unsigned int sleep_reqstr_id;
> + int delay;
> +};
> +
> +struct palmas_clock_info {
> + struct device *dev;
> + struct clk *clk;
> + struct clk_hw hw;
> + struct palmas *palmas;
> + struct palmas_clk32k_desc *clk_desc;
> + int ext_control_pin;
> +};
> +
> +static inline struct palmas_clock_info *to_palmas_clks_info(struct clk_hw *hw)
> +{
> + return container_of(hw, struct palmas_clock_info, hw);
> +}
> +
> +static unsigned long palmas_clks_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + return 32768;
> +}

I see that other clock drivers using a constant rate return 0 if the
clock has not been enabled. So maybe is more correct to have something
like the following?

if (__clk_is_enabled(hw->clk))
return 32768;
else
return 0;

Best regards,
Javier

2014-06-30 05:57:07

by Peter Ujfalusi

[permalink] [raw]

Subject: Re: [RESEND 2/2] clk: Add driver for Palmas clk32kg and clk32kgaudio clocks

Hi Javier,

On 06/27/2014 09:23 PM, Javier Martinez Canillas wrote:
> Hello Peter,
>
> On Fri, Jun 27, 2014 at 8:01 AM, Peter Ujfalusi <[email protected]> wrote:
>> Palmas class of devices can provide 32K clock(s) to be used by other devices
>> on the board. Depending on the actual device the provided clocks can be:
>> CLK32K_KG and CLK32K_KGAUDIO
>> or only one:
>> CLK32K_KG (TPS659039 for example)
>>
>> Use separate compatible flags for the two 32K clock.
>> A system which needs or have only one of the 32k clock from
>> Palmas will need to add node(s) for each clock as separate section
>> in the dts file.
>> The two compatible property is:
>> "ti,palmas-clk32kg" for clk32kg clock
>> "ti,palmas-clk32kgaudio" for clk32kgaudio clock
>>
>> Apart from the register control of the clocks - which is done via
>> the clock API there is a posibility to enable the external sleep
>> control. In this way the clock can be enabled/disabled on demand by the
>> user of the clock.
>>
>> See the documentation for more details.
>>
>> Signed-off-by: Peter Ujfalusi <[email protected]>
>> Reviewed-by: Nishanth Menon <[email protected]>

>> +static unsigned long palmas_clks_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + return 32768;
>> +}
>
> I see that other clock drivers using a constant rate return 0 if the
> clock has not been enabled.

and there are examples when similar fixed clock drivers returns only the clock
value, like clk-max77686. I can not find clear guidelines neither in the
documentation or around the header/c files for this.
Mike, what is the appropriate way of handling the recalc_rate?

> So maybe is more correct to have something
> like the following?
>
> if (__clk_is_enabled(hw->clk))
> return 32768;
> else
> return 0;
>
> Best regards,
> Javier
>

--
Péter

2014-07-02 04:33:54

by Mike Turquette

[permalink] [raw]

Subject: Re: [RESEND 2/2] clk: Add driver for Palmas clk32kg and clk32kgaudio clocks

Quoting Peter Ujfalusi (2014-06-29 22:56:55)
> Hi Javier,
>
> On 06/27/2014 09:23 PM, Javier Martinez Canillas wrote:
> > Hello Peter,
> >
> > On Fri, Jun 27, 2014 at 8:01 AM, Peter Ujfalusi <[email protected]> wrote:
> >> Palmas class of devices can provide 32K clock(s) to be used by other devices
> >> on the board. Depending on the actual device the provided clocks can be:
> >> CLK32K_KG and CLK32K_KGAUDIO
> >> or only one:
> >> CLK32K_KG (TPS659039 for example)
> >>
> >> Use separate compatible flags for the two 32K clock.
> >> A system which needs or have only one of the 32k clock from
> >> Palmas will need to add node(s) for each clock as separate section
> >> in the dts file.
> >> The two compatible property is:
> >> "ti,palmas-clk32kg" for clk32kg clock
> >> "ti,palmas-clk32kgaudio" for clk32kgaudio clock
> >>
> >> Apart from the register control of the clocks - which is done via
> >> the clock API there is a posibility to enable the external sleep
> >> control. In this way the clock can be enabled/disabled on demand by the
> >> user of the clock.
> >>
> >> See the documentation for more details.
> >>
> >> Signed-off-by: Peter Ujfalusi <[email protected]>
> >> Reviewed-by: Nishanth Menon <[email protected]>
>
> >> +static unsigned long palmas_clks_recalc_rate(struct clk_hw *hw,
> >> + unsigned long parent_rate)
> >> +{
> >> + return 32768;
> >> +}
> >
> > I see that other clock drivers using a constant rate return 0 if the
> > clock has not been enabled.
>
> and there are examples when similar fixed clock drivers returns only the clock
> value, like clk-max77686. I can not find clear guidelines neither in the
> documentation or around the header/c files for this.
> Mike, what is the appropriate way of handling the recalc_rate?

You are right that there are no guidelines stating, "don't do that", but
please, "don't do that" ;-)

clk_enable and clk_set_rate are entirely unrelated operations from the
perspective of the Linux clock framework, and mixing these two classes
of operations is a recipe for pain.

>
> > So maybe is more correct to have something
> > like the following?
> >
> > if (__clk_is_enabled(hw->clk))
> > return 32768;
> > else
> > return 0;

So what happens here if this is gateable clock and later on we call
clk_enable on it? The clocks rate will still be zero since
clk_enable/clk_disable do not touch the rate at all.

Regards,
Mike

> >
> > Best regards,
> > Javier
> >
>
>
> --
> Péter

2014-07-02 04:53:37

by Mike Turquette

[permalink] [raw]

Subject: Re: [RESEND 0/2] clk: Support for Palmas clk32kg and clk32kgaudio clocks

Quoting Peter Ujfalusi (2014-06-26 23:01:09)
> Hi Mike,
>
> This is a resend of the v2 version of the palmas clock driver which seamingly
> missed the 3.16 merge window. I have added Nishanth's Reviewed-by tag to the
> patches.

Thanks for the resend. Applied to clk-next.

Regards,
Mike

>
> Changes since v1:
> - binding documentation and driver has been separated based on Nishanth Menon's
> comment
>
> v2 of the series:
> https://lkml.org/lkml/2014/5/6/323
>
> v1 of the driver can be found:
> https://lkml.org/lkml/2014/4/3/104
>
> Palmas class of devices can provide 32K clock(s) to be used by other devices
> on the board. Depending on the actual device the provided clocks can be:
> CLK32K_KG and CLK32K_KGAUDIO
> or only one:
> CLK32K_KG (TPS659039 for example)
>
> Use separate compatible flags for the two 32K clock.
> A system which needs or have only one of the 32k clock from
> Palmas will need to add node(s) for each clock as separate section
> in the dts file.
> The two compatible property is:
> "ti,palmas-clk32kg" for clk32kg clock
> "ti,palmas-clk32kgaudio" for clk32kgaudio clock
>
> Apart from the register control of the clocks - which is done via
> the clock API there is a posibility to enable the external sleep
> control. In this way the clock can be enabled/disabled on demand by the
> user of the clock.
>
> Regards,
> Peter
> ---
> Peter Ujfalusi (2):
> dt/bindings: Binding documentation for Palmas clk32kg and clk32kgaudio
> clocks
> clk: Add driver for Palmas clk32kg and clk32kgaudio clocks
>
> .../bindings/clock/clk-palmas-clk32kg-clocks.txt | 35 +++
> drivers/clk/Kconfig | 7 +
> drivers/clk/Makefile | 1 +
> drivers/clk/clk-palmas.c | 307 +++++++++++++++++++++
> include/dt-bindings/mfd/palmas.h | 18 ++
> 5 files changed, 368 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/clk-palmas-clk32kg-clocks.txt
> create mode 100644 drivers/clk/clk-palmas.c
> create mode 100644 include/dt-bindings/mfd/palmas.h
>
> --
> 2.0.0
>

2014-07-02 09:16:57

by Javier Martinez Canillas

[permalink] [raw]

Subject: Re: [RESEND 2/2] clk: Add driver for Palmas clk32kg and clk32kgaudio clocks

Hello Mike,

On Wed, Jul 2, 2014 at 6:33 AM, Mike Turquette <[email protected]> wrote:
> Quoting Peter Ujfalusi (2014-06-29 22:56:55)
>> Hi Javier,
>>
>> On 06/27/2014 09:23 PM, Javier Martinez Canillas wrote:
>> > Hello Peter,
>> >
>> > On Fri, Jun 27, 2014 at 8:01 AM, Peter Ujfalusi <[email protected]> wrote:
>> >> Palmas class of devices can provide 32K clock(s) to be used by other devices
>> >> on the board. Depending on the actual device the provided clocks can be:
>> >> CLK32K_KG and CLK32K_KGAUDIO
>> >> or only one:
>> >> CLK32K_KG (TPS659039 for example)
>> >>
>> >> Use separate compatible flags for the two 32K clock.
>> >> A system which needs or have only one of the 32k clock from
>> >> Palmas will need to add node(s) for each clock as separate section
>> >> in the dts file.
>> >> The two compatible property is:
>> >> "ti,palmas-clk32kg" for clk32kg clock
>> >> "ti,palmas-clk32kgaudio" for clk32kgaudio clock
>> >>
>> >> Apart from the register control of the clocks - which is done via
>> >> the clock API there is a posibility to enable the external sleep
>> >> control. In this way the clock can be enabled/disabled on demand by the
>> >> user of the clock.
>> >>
>> >> See the documentation for more details.
>> >>
>> >> Signed-off-by: Peter Ujfalusi <[email protected]>
>> >> Reviewed-by: Nishanth Menon <[email protected]>
>>
>> >> +static unsigned long palmas_clks_recalc_rate(struct clk_hw *hw,
>> >> + unsigned long parent_rate)
>> >> +{
>> >> + return 32768;
>> >> +}
>> >
>> > I see that other clock drivers using a constant rate return 0 if the
>> > clock has not been enabled.
>>
>> and there are examples when similar fixed clock drivers returns only the clock
>> value, like clk-max77686. I can not find clear guidelines neither in the
>> documentation or around the header/c files for this.
>> Mike, what is the appropriate way of handling the recalc_rate?
>
> You are right that there are no guidelines stating, "don't do that", but
> please, "don't do that" ;-)
>
> clk_enable and clk_set_rate are entirely unrelated operations from the
> perspective of the Linux clock framework, and mixing these two classes
> of operations is a recipe for pain.
>

Thanks a lot for the clarification. I just asked since when posting a
clock driver I got feedback that recalc_rate should return 0 if the
clock was disabled and also saw some drivers doing that, e.g:
drivers/clk/clk-s2mps11.c

>>
>> > So maybe is more correct to have something
>> > like the following?
>> >
>> > if (__clk_is_enabled(hw->clk))
>> > return 32768;
>> > else
>> > return 0;
>
> So what happens here if this is gateable clock and later on we call
> clk_enable on it? The clocks rate will still be zero since
> clk_enable/clk_disable do not touch the rate at all.
>

Got it, thanks for the explanation.

> Regards,
> Mike
>

Best regards,
Javier