Re: [PATCH v5 02/16] clk: tegra: Add library for the DFLL clock source (open-loop mode)

From: Mikko Perttunen
Date: Mon Oct 27 2014 - 05:36:23 EST

On 10/24/2014 06:08 PM, Vladimir Zapolskiy wrote:
Hello Mikko,

Hello Vladimir!


On 24.10.2014 17:39, Mikko Perttunen wrote:
From: Tuomas Tynkkynen <ttynkkynen@xxxxxxxxxx>

Add shared code to support the Tegra DFLL clocksource in open-loop
mode. This root clocksource is present on the Tegra124 SoCs. The
DFLL is the intended primary clock source for the fast CPU cluster.

This code is very closely based on a patch by Paul Walmsley from
December (http://comments.gmane.org/gmane.linux.ports.tegra/15273),
which in turn comes from the internal driver by originally created
by Aleksandr Frid <afrid@xxxxxxxxxx>.

Subsequent patches will add support for closed loop mode and drivers
for the Tegra124 fast CPU cluster DFLL devices, which rely on this
code.

Signed-off-by: Paul Walmsley <pwalmsley@xxxxxxxxxx>
Signed-off-by: Tuomas Tynkkynen <ttynkkynen@xxxxxxxxxx>
Signed-off-by: Mikko Perttunen <mikko.perttunen@xxxxxxxx>
---
- Style fixes
- Removed incorrect and unused DFLL_I2C_CFG_SLAVE_ADDR_MASK define
- Documented that dfll_register_clk can return -ENOMEM
- Harmonized clock operation order
- Check !soc before allocating
- Comment fixes

drivers/clk/tegra/Makefile | 1 +
drivers/clk/tegra/clk-dfll.c | 1090 ++++++++++++++++++++++++++++++++++++++++++
drivers/clk/tegra/clk-dfll.h | 55 +++
3 files changed, 1146 insertions(+)
create mode 100644 drivers/clk/tegra/clk-dfll.c
create mode 100644 drivers/clk/tegra/clk-dfll.h

diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
index f7dfb72..47320ca 100644
--- a/drivers/clk/tegra/Makefile
+++ b/drivers/clk/tegra/Makefile
@@ -1,5 +1,6 @@
obj-y += clk.o
obj-y += clk-audio-sync.o
+obj-y += clk-dfll.o
obj-y += clk-divider.o
obj-y += clk-periph.o
obj-y += clk-periph-gate.o
diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
new file mode 100644
index 0000000..405fa9d
--- /dev/null
+++ b/drivers/clk/tegra/clk-dfll.c

[snip]

+/*
+ * Runtime PM suspend/resume callbacks
+ */
+
+/**
+ * tegra_dfll_runtime_resume - enable all clocks needed by the DFLL
+ * @dev: DFLL device *
+ *
+ * Enable all clocks needed by the DFLL. Assumes that clk_prepare()
+ * has already been called on all the clocks.
+ *
+ * XXX Should also handle context restore when returning from off.
+ */
+int tegra_dfll_runtime_resume(struct device *dev)
+{
+ struct tegra_dfll *td = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_enable(td->ref_clk);
+ if (ret) {
+ dev_err(dev, "could not enable ref clock: %d\n", ret);
+ return ret;
+ }
+
+ ret = clk_enable(td->soc_clk);
+ if (ret) {

What is the correct behaviour on error? Should you fallback and try to
enable td->i2c_clk clock or may be disable right enabled above td->ref_clk?

This should probably error out and disable the previously enabled clock/clocks. Will fix.


+ dev_err(dev, "could not enable register clock: %d\n", ret);
+ return ret;
+ }
+
+ ret = clk_enable(td->i2c_clk);
+ if (ret) {

See the comment above.

+ dev_err(dev, "could not enable i2c clock: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(tegra_dfll_runtime_resume);
+
+/**
+ * tegra_dfll_runtime_suspend - disable all clocks needed by the DFLL
+ * @dev: DFLL device *
+ *
+ * Disable all clocks needed by the DFLL. Assumes that other code
+ * will later call clk_unprepare().
+ */
+int tegra_dfll_runtime_suspend(struct device *dev)
+{
+ struct tegra_dfll *td = dev_get_drvdata(dev);
+
+ clk_disable(td->ref_clk);
+ clk_disable(td->soc_clk);
+ clk_disable(td->i2c_clk);

Relevant to the comment above.

+ return 0;
+}
+EXPORT_SYMBOL(tegra_dfll_runtime_suspend);
+

[snip]

+/**
+ * dfll_register_clk - register the DFLL output clock with the clock framework
+ * @td: DFLL instance
+ *
+ * Register the DFLL's output clock with the Linux clock framework and register
+ * the DFLL driver as an OF clock provider. Returns 0 upon success or -EINVAL
+ * or -ENOMEM upon failure.
+ */
+static int dfll_register_clk(struct tegra_dfll *td)
+{
+ int ret;
+
+ dfll_clk_init_data.name = td->output_clock_name;
+ td->dfll_clk_hw.init = &dfll_clk_init_data;
+
+ td->dfll_clk = clk_register(td->dev, &td->dfll_clk_hw);
+ if (IS_ERR(td->dfll_clk)) {
+ dev_err(td->dev, "DFLL clock registration error\n");
+ return -EINVAL;
+ }
+
+ ret = of_clk_add_provider(td->dev->of_node, of_clk_src_simple_get,
+ td->dfll_clk);
+ if (ret) {
+ dev_err(td->dev, "of_clk_add_provider() failed\n");
+ goto out_unregister_clk;
+ }
+
+ return 0;
+
+out_unregister_clk:
+ clk_unregister(td->dfll_clk);

---8<---
if (ret) {
dev_err(td->dev, "of_clk_add_provider() failed\n");
clk_unregister(td->dfll_clk);
}
---8<---

version is 5 lines and one label less.

Will fix.


+ return ret;
+}
+

[snip]

+/**
+ * read_dt_param - helper function for reading required parameters from the DT
+ * @td: DFLL instance
+ * @param: DT property name
+ * @dest: output pointer for the value read
+ *
+ * Read a required numeric parameter from the DFLL device node, or complain
+ * if the property doesn't exist. Returns a boolean indicating success for
+ * easy chaining of multiple calls to this function.
+ */
+static bool read_dt_param(struct tegra_dfll *td, const char *param, u32 *dest)
+{
+ int err = of_property_read_u32(td->dev->of_node, param, dest);
+
+ if (err < 0) {
+ dev_err(td->dev, "failed to read DT parameter %s: %d\n",
+ param, err);
+ return false;
+ }
+
+ return true;
+}

^^^^ Relatively useless wrapper function used only once. You are anyway
going to check the return value again...

As you noted, this will be used more later.


+
+/**
+ * dfll_fetch_common_params - read DFLL parameters from the device tree
+ * @td: DFLL instance
+ *
+ * Read all the DT parameters that are common to both I2C and PWM operation.
+ * Returns 0 on success or -EINVAL on any failure.
+ */
+static int dfll_fetch_common_params(struct tegra_dfll *td)
+{
+ bool ok = true;
+
+ ok &= read_dt_param(td, "nvidia,droop-ctrl", &td->droop_ctrl);

See the comment above.

+ if (of_property_read_string(td->dev->of_node, "clock-output-names",
+ &td->output_clock_name)) {
+ dev_err(td->dev, "missing clock-output-names property\n");
+ ok = false;
+ }
+
+ return ok ? 0 : -EINVAL;
+}


--
With best wishes,
Vladimir


Thanks for reviewing!

Cheers,
Mikko

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/