[PATCH] clk: add LP8788 clock driver

[PATCH] clk: add LP8788 clock driver

[Kim, Milo]

LP8788 PMU consists of regulator, battery charger, RTC, ADC, backlight driver
and current sinks.
Here is additional driver, clock.

* Clock source
LP8788 clock is generated by two clock source.
One is internal oscillator. The other is attached external crystal oscillator.
LP8788 provides automatic clock source selection through the I2C register.
This operation is executed in 'prepare' of common clock driver architecture.

* Clock rate
LP8788 generates a fixed 32768Hz clock which is used for the system.

* Supported operation
.prepare: Before the clock is enabled, automatic clock source selection is done
          by register access.
.unprepare: No register for disable or remove the clock source, so do nothing.
.is_enabled
.recalc_rate: Fixed clock rate, 32.768KHz.

* clk_register_fixed_rate() vs devm_clk_register() and clkdev_add()
Fixed clock driver can be created by common clock helper function but
but LP8788 should be built as a module.
Using devm_clk_register() and clkdev_add() is more appropriate method to
implement LP8788 clock driver.

Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
 drivers/clk/Kconfig        |    7 ++
 drivers/clk/Makefile       |    1 +
 drivers/clk/clk-lp8788.c   |  186 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/mfd/lp8788.c       |    2 +
 include/linux/mfd/lp8788.h |    1 +
 5 files changed, 197 insertions(+)
 create mode 100644 drivers/clk/clk-lp8788.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index a64caef..4b5109c 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -71,6 +71,13 @@ config COMMON_CLK_AXI_CLKGEN
 	  Support for the Analog Devices axi-clkgen pcore clock generator for Xilinx
 	  FPGAs. It is commonly used in Analog Devices' reference designs.
 
+config CLK_LP8788
+	tristate "Clock driver for TI LP8788 PMU"
+	depends on MFD_LP8788
+	---help---
+	  Supports the clock subsystem of TI LP8788. It generates the 32KHz
+	  clock output.
+
 endmenu
 
 source "drivers/clk/mvebu/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 1c22f9d..06d0763 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -35,3 +35,4 @@ obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o
 obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
 obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
 obj-$(CONFIG_CLK_TWL6040)	+= clk-twl6040.o
+obj-$(CONFIG_CLK_LP8788)	+= clk-lp8788.o
diff --git a/drivers/clk/clk-lp8788.c b/drivers/clk/clk-lp8788.c
new file mode 100644
index 0000000..de56887
--- /dev/null
+++ b/drivers/clk/clk-lp8788.c
@@ -0,0 +1,186 @@
+/*
+ * TI LP8788 Clock Driver
+ *
+ * Copyright 2013 Texas Instruments
+ *
+ * Author: Milo(Woogyom) Kim <milo.kim@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/mfd/lp8788.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+
+/* Registers */
+#define LP8788_REG_STATUS		0x06
+#define LP8788_OSC_WORKING		0x10
+
+#define LP8788_REG_CLKCTRL		0xA2
+#define LP8788_CLKMODE_MASK		0x02
+#define LP8788_CLKMODE_AUTO		0X02
+
+#define CLKOUT_32KHZ		32768
+
+struct lp8788_clk {
+	struct lp8788 *lp;
+	struct clk *clk;
+	struct clk_hw hw;
+	struct clk_lookup *lookup;
+	bool enabled;
+};
+
+static struct lp8788_clk *to_lp8788_clk(struct clk_hw *hw)
+{
+	return container_of(hw, struct lp8788_clk, hw);
+}
+
+static int lp8788_clk_prepare(struct clk_hw *hw)
+{
+	struct lp8788_clk *pclk = to_lp8788_clk(hw);
+
+	/* Automatic oscillator source selection */
+	return lp8788_update_bits(pclk->lp, LP8788_REG_CLKCTRL,
+				LP8788_CLKMODE_MASK, LP8788_CLKMODE_AUTO);
+}
+
+static void lp8788_clk_unprepare(struct clk_hw *hw)
+{
+	/* Do nothing */
+	return;
+}
+
+static int lp8788_clk_is_enabled(struct clk_hw *hw)
+{
+	struct lp8788_clk *pclk = to_lp8788_clk(hw);
+
+	/*
+	 * Do not use the LP8788 register access here, unsafe locking problem
+	 * may occur during loading the driver. So stored varible is prefered.
+	 */
+
+	return pclk->enabled;
+}
+
+static unsigned long lp8788_clk_recalc_rate(struct clk_hw *hw,
+					unsigned long unused)
+{
+	return CLKOUT_32KHZ;
+}
+
+static const struct clk_ops lp8788_clk_ops = {
+	.prepare = lp8788_clk_prepare,
+	.unprepare = lp8788_clk_unprepare,
+	.is_enabled = lp8788_clk_is_enabled,
+	.recalc_rate = lp8788_clk_recalc_rate,
+};
+
+static struct clk_init_data lp8788_clk_cfg = {
+	.name = "32k_clk",
+	.ops = &lp8788_clk_ops,
+	.flags = CLK_IS_ROOT,
+};
+
+static int lp8788_clk_register(struct device *dev, struct lp8788_clk *pclk)
+{
+	struct clk_lookup *lookup;
+
+	pclk->hw.init = &lp8788_clk_cfg;
+	pclk->clk = devm_clk_register(dev, &pclk->hw);
+	if (IS_ERR(pclk->clk)) {
+		dev_err(dev, "clk register err\n");
+		return PTR_ERR(pclk->clk);
+	}
+
+	lookup = clkdev_alloc(pclk->clk, lp8788_clk_cfg.name, NULL);
+	if (!lookup) {
+		dev_err(dev, "clkdev_alloc err\n");
+		return -ENOMEM;
+	}
+	pclk->lookup = lookup;
+
+	clkdev_add(lookup);
+
+	return 0;
+}
+
+static void lp8788_clk_unregister(struct lp8788_clk *pclk)
+{
+	if (pclk->lookup)
+		clkdev_drop(pclk->lookup);
+}
+
+static int lp8788_get_clk_status(struct lp8788_clk *pclk)
+{
+	int ret;
+	u8 val;
+
+	ret = lp8788_read_byte(pclk->lp, LP8788_REG_STATUS, &val);
+	if (ret)
+		return ret;
+
+	pclk->enabled = val & LP8788_OSC_WORKING;
+
+	return 0;
+}
+
+static int lp8788_clk_probe(struct platform_device *pdev)
+{
+	struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
+	struct lp8788_clk *pclk;
+	int ret;
+
+	pclk = devm_kzalloc(&pdev->dev, sizeof(struct lp8788_clk), GFP_KERNEL);
+	if (!pclk)
+		return -ENOMEM;
+
+	pclk->lp = lp;
+	platform_set_drvdata(pdev, pclk);
+
+	ret = lp8788_get_clk_status(pclk);
+	if (ret)
+		return ret;
+
+	return lp8788_clk_register(&pdev->dev, pclk);
+}
+
+static int lp8788_clk_remove(struct platform_device *pdev)
+{
+	struct lp8788_clk *pclk = platform_get_drvdata(pdev);
+
+	lp8788_clk_unregister(pclk);
+	return 0;
+}
+
+static struct platform_driver lp8788_clk_driver = {
+	.probe = lp8788_clk_probe,
+	.remove = lp8788_clk_remove,
+	.driver		= {
+		.name	= LP8788_DEV_CLK,
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init lp8788_clk_init(void)
+{
+	return platform_driver_register(&lp8788_clk_driver);
+}
+subsys_initcall(lp8788_clk_init);
+
+static void __exit lp8788_clk_exit(void)
+{
+	platform_driver_unregister(&lp8788_clk_driver);
+}
+module_exit(lp8788_clk_exit);
+
+MODULE_DESCRIPTION("TI LP8788 Clock Driver");
+MODULE_AUTHOR("Milo Kim");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:lp8788-clk");
diff --git a/drivers/mfd/lp8788.c b/drivers/mfd/lp8788.c
index c3d3c9b..9f5596e 100644
--- a/drivers/mfd/lp8788.c
+++ b/drivers/mfd/lp8788.c
@@ -72,6 +72,8 @@ static struct resource rtc_irqs[] = {
 };
 
 static struct mfd_cell lp8788_devs[] = {
+	MFD_DEV_SIMPLE(CLK),
+
 	/* 4 bucks */
 	MFD_DEV_WITH_ID(BUCK, 1),
 	MFD_DEV_WITH_ID(BUCK, 2),
diff --git a/include/linux/mfd/lp8788.h b/include/linux/mfd/lp8788.h
index 786bf66..5f33be8 100644
--- a/include/linux/mfd/lp8788.h
+++ b/include/linux/mfd/lp8788.h
@@ -28,6 +28,7 @@
 #define LP8788_DEV_VIBRATOR	"lp8788-vibrator"
 #define LP8788_DEV_KEYLED	"lp8788-keyled"
 #define LP8788_DEV_ADC		"lp8788-adc"
+#define LP8788_DEV_CLK		"lp8788-clk"
 
 #define LP8788_NUM_BUCKS	4
 #define LP8788_NUM_DLDOS	12
-- 
1.7.9.5


Best Regards,
Milo

[PATCH] clk: add LP8788 clock driver

[Mike Turquette]

Quoting Kim, Milo (2013-03-20 17:37:14)
> LP8788 PMU consists of regulator, battery charger, RTC, ADC, backlight driver
> and current sinks.
> Here is additional driver, clock.
> 
> * Clock source
> LP8788 clock is generated by two clock source.
> One is internal oscillator. The other is attached external crystal oscillator.
> LP8788 provides automatic clock source selection through the I2C register.
> This operation is executed in 'prepare' of common clock driver architecture.
> 
> * Clock rate
> LP8788 generates a fixed 32768Hz clock which is used for the system.
> 
> * Supported operation
> .prepare: Before the clock is enabled, automatic clock source selection is done
>           by register access.
> .unprepare: No register for disable or remove the clock source, so do nothing.
> .is_enabled
> .recalc_rate: Fixed clock rate, 32.768KHz.
> 
> * clk_register_fixed_rate() vs devm_clk_register() and clkdev_add()
> Fixed clock driver can be created by common clock helper function but
> but LP8788 should be built as a module.
> Using devm_clk_register() and clkdev_add() is more appropriate method to
> implement LP8788 clock driver.
> 
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> ---
>  drivers/clk/Kconfig        |    7 ++
>  drivers/clk/Makefile       |    1 +
>  drivers/clk/clk-lp8788.c   |  186 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/mfd/lp8788.c       |    2 +
>  include/linux/mfd/lp8788.h |    1 +
>  5 files changed, 197 insertions(+)
>  create mode 100644 drivers/clk/clk-lp8788.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index a64caef..4b5109c 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -71,6 +71,13 @@ config COMMON_CLK_AXI_CLKGEN
>           Support for the Analog Devices axi-clkgen pcore clock generator for Xilinx
>           FPGAs. It is commonly used in Analog Devices' reference designs.
>  
> +config CLK_LP8788
> +       tristate "Clock driver for TI LP8788 PMU"
> +       depends on MFD_LP8788
> +       ---help---
> +         Supports the clock subsystem of TI LP8788. It generates the 32KHz
> +         clock output.
> +
>  endmenu
>  
>  source "drivers/clk/mvebu/Kconfig"
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 1c22f9d..06d0763 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -35,3 +35,4 @@ obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o
>  obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
>  obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
>  obj-$(CONFIG_CLK_TWL6040)      += clk-twl6040.o
> +obj-$(CONFIG_CLK_LP8788)       += clk-lp8788.o
> diff --git a/drivers/clk/clk-lp8788.c b/drivers/clk/clk-lp8788.c
> new file mode 100644
> index 0000000..de56887
> --- /dev/null
> +++ b/drivers/clk/clk-lp8788.c
> @@ -0,0 +1,186 @@
> +/*
> + * TI LP8788 Clock Driver
> + *
> + * Copyright 2013 Texas Instruments
> + *
> + * Author: Milo(Woogyom) Kim <milo.kim@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/mfd/lp8788.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +
> +/* Registers */
> +#define LP8788_REG_STATUS              0x06
> +#define LP8788_OSC_WORKING             0x10
> +
> +#define LP8788_REG_CLKCTRL             0xA2
> +#define LP8788_CLKMODE_MASK            0x02
> +#define LP8788_CLKMODE_AUTO            0X02
> +
> +#define CLKOUT_32KHZ           32768
> +
> +struct lp8788_clk {
> +       struct lp8788 *lp;
> +       struct clk *clk;
> +       struct clk_hw hw;
> +       struct clk_lookup *lookup;
> +       bool enabled;
> +};
> +
> +static struct lp8788_clk *to_lp8788_clk(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct lp8788_clk, hw);
> +}

Hi Milo,

Just FYI most folks are simply using a macro to do the above.  E.g:

#define to_lp8788_clk(_hw) container_of(_hw, struct lp8788_clk, hw)

> +
> +static int lp8788_clk_prepare(struct clk_hw *hw)
> +{
> +       struct lp8788_clk *pclk = to_lp8788_clk(hw);
> +
> +       /* Automatic oscillator source selection */
> +       return lp8788_update_bits(pclk->lp, LP8788_REG_CLKCTRL,
> +                               LP8788_CLKMODE_MASK, LP8788_CLKMODE_AUTO);
> +}
> +
> +static void lp8788_clk_unprepare(struct clk_hw *hw)
> +{
> +       /* Do nothing */
> +       return;
> +}
> +
> +static int lp8788_clk_is_enabled(struct clk_hw *hw)
> +{
> +       struct lp8788_clk *pclk = to_lp8788_clk(hw);
> +
> +       /*
> +        * Do not use the LP8788 register access here, unsafe locking problem
> +        * may occur during loading the driver. So stored varible is prefered.
> +        */

What unsafe locking problem?

Also have you looked into using the new .is_prepare callback?  See:
https://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=commitdiff;h=3d6ee287a3e341c88eafd0b4620b12d640b3736b;hp=30ee400614385ac49f4c9b4bc03d77ff8f07a61e

> +
> +       return pclk->enabled;

Why provide a .is_enabled callback when there are no .enable or .disable
callbacks?

Regards,
Mike

[PATCH] clk: add LP8788 clock driver

[Kim, Milo]

> Just FYI most folks are simply using a macro to do the above.  E.g:
> 
> #define to_lp8788_clk(_hw) container_of(_hw, struct lp8788_clk, hw)

OK, thanks! Will be fixed in the 2nd patch.

> > +
> > +static int lp8788_clk_is_enabled(struct clk_hw *hw)
> > +{
> > +       struct lp8788_clk *pclk = to_lp8788_clk(hw);
> > +
> > +       /*
> > +        * Do not use the LP8788 register access here, unsafe locking
> problem
> > +        * may occur during loading the driver. So stored varible is
> prefered.
> > +        */
> 
> What unsafe locking problem?

If I try to get LP8788 clock status via the remap-i2c here, then locking problem
occurs.
The 'enable_lock' is already locked and the MUTEX of REGMAP is used on reading
a register.

[    2.692443] -------------------------------------------------------
[    2.699066] swapper/0/1 is trying to acquire lock:
[    2.704132]  (&map->mutex){+.+...}, at: [<c0374ecc>] regmap_read+0x34/0x5c
[    2.711486] 
[    2.711486] but task is already holding lock:
[    2.717681]  (enable_lock){......}, at: [<c047a6c8>] clk_disable_unused_subtr
ee+0x3c/0xb4
[    2.726379] 
[    2.726379] which lock already depends on the new lock.

That's why lp8788_get_clk_status() is used on _probe().
- Read a register and store the value into local status variable.
Then local variable is used in .is_enabled().

> Also have you looked into using the new .is_prepare callback?  See:
> https://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=commitdif
> f;h=3d6ee287a3e341c88eafd0b4620b12d640b3736b;hp=30ee400614385ac49f4c9b4
> bc03d77ff8f07a61e

Not checked yet, but it looks no caller for this callback at this moment.
Will this be added inside the clk_prepare()?

> > +
> > +       return pclk->enabled;
> 
> Why provide a .is_enabled callback when there are no .enable
> or .disable
> callbacks?

LP8788 Clock is always enabled, so it needs to show the clock status.
However, I think no effect on working the clock operation without is_enabled().
Can I remove it?

Thanks,
Milo

[PATCH] clk: add LP8788 clock driver

[Mike Turquette]

Quoting Kim, Milo (2013-04-01 19:18:32)
> > Just FYI most folks are simply using a macro to do the above.  E.g:
> > 
> > #define to_lp8788_clk(_hw) container_of(_hw, struct lp8788_clk, hw)
> 
> OK, thanks! Will be fixed in the 2nd patch.
> 
> > > +
> > > +static int lp8788_clk_is_enabled(struct clk_hw *hw)
> > > +{
> > > +       struct lp8788_clk *pclk = to_lp8788_clk(hw);
> > > +
> > > +       /*
> > > +        * Do not use the LP8788 register access here, unsafe locking
> > problem
> > > +        * may occur during loading the driver. So stored varible is
> > prefered.
> > > +        */
> > 
> > What unsafe locking problem?
> 
> If I try to get LP8788 clock status via the remap-i2c here, then locking problem
> occurs.
> The 'enable_lock' is already locked and the MUTEX of REGMAP is used on reading
> a register.
> 
> [    2.692443] -------------------------------------------------------
> [    2.699066] swapper/0/1 is trying to acquire lock:
> [    2.704132]  (&map->mutex){+.+...}, at: [<c0374ecc>] regmap_read+0x34/0x5c
> [    2.711486] 
> [    2.711486] but task is already holding lock:
> [    2.717681]  (enable_lock){......}, at: [<c047a6c8>] clk_disable_unused_subtr
> ee+0x3c/0xb4
> [    2.726379] 
> [    2.726379] which lock already depends on the new lock.
> 
> That's why lp8788_get_clk_status() is used on _probe().
> - Read a register and store the value into local status variable.
> Then local variable is used in .is_enabled().
> 

I think that the new clk reentrancy patch should fix this for you.  Can
you rebase your patch onto the latest clk-next and try using remap-i2c
again?  You can that branch here:
git://git.linaro.org/people/mturquette/linux.git clk-next

> > Also have you looked into using the new .is_prepare callback?  See:
> > https://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=commitdif
> > f;h=3d6ee287a3e341c88eafd0b4620b12d640b3736b;hp=30ee400614385ac49f4c9b4
> > bc03d77ff8f07a61e
> 
> Not checked yet, but it looks no caller for this callback at this moment.
> Will this be added inside the clk_prepare()?
> 
> > > +
> > > +       return pclk->enabled;
> > 
> > Why provide a .is_enabled callback when there are no .enable
> > or .disable
> > callbacks?
> 
> LP8788 Clock is always enabled, so it needs to show the clock status.
> However, I think no effect on working the clock operation without is_enabled().
> Can I remove it?
> 

Yes you can remove it.  Since you do not provide a .disabled callback
then clk_disable_unused() will not try to disable your clock.  Do any
drivers call clk_prepare_enable on this clock?

It is also probably a good idea to set the CLK_IGNORE_UNUSED flag on
this clock (in addition to the CLK_IS_ROOT flag that you have already
set).  This way if your .unprepare callback ever does something some day
then you won't get a nasty surprise.

Regards,
Mike

> Thanks,
> Milo