diff mbox

[1/3] clk: ls1021a: new platform clock driver

Message ID 20170222150349.16790-2-alexander.stein@systec-electronic.com (mailing list archive)
State Changes Requested
Delegated to: Stephen Boyd
Headers show

Commit Message

Alexander Stein Feb. 22, 2017, 3:03 p.m. UTC 
This driver currently only implements the QSPI divider register
SCFG_QSPI_CFG.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/clk/Makefile      |  1 +
 drivers/clk/clk-ls1021a.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100644 drivers/clk/clk-ls1021a.c

Comments

Stephen Boyd April 7, 2017, 7:33 p.m. UTC | #1 
On 02/22, Alexander Stein wrote:
> This driver currently only implements the QSPI divider register
> SCFG_QSPI_CFG.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

Nobody reviewed. Sorry it fell down in my queue and I forgot.

> ---
>  drivers/clk/Makefile      |  1 +
>  drivers/clk/clk-ls1021a.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
>  create mode 100644 drivers/clk/clk-ls1021a.c
> 
> diff --git a/drivers/clk/clk-ls1021a.c b/drivers/clk/clk-ls1021a.c
> new file mode 100644
> index 0000000..2f64806
> --- /dev/null
> +++ b/drivers/clk/clk-ls1021a.c
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (C) 2017 SYS TEC electronic GmbH
> + * Alexander Stein <alexander.stein@systec-electronic.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/io.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +
> +static const struct clk_div_table qspi_cfg_div_table[] = {
> +	{ 0x0, 256 }, { 0x1, 64 }, { 0x2, 32 }, { 0x3, 24 },
> +	{ 0x4,  20 }, { 0x5, 15 }, { 0x6, 12 }, { 0x7,  8 },
> +	{ 0 },
> +};
> +
> +static void __init scfg_qspi_cfg_ls1021a_init(struct device_node *np)
> +{
> +	const char *parent_name;
> +	const char *name;
> +	void __iomem *base;
> +	struct clk *parent_clk;
> +	struct clk *clk;
> +	struct resource res;

Unused? but should be used!

> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		pr_warn("Failed to map address range for node %s\n", np->name);

We don't typically need any sort of error message.

> +		return;
> +	}
> +
> +	parent_clk = of_clk_get(np, 0);
> +	if (IS_ERR(parent_clk)) {
> +		pr_warn("Failed to get clock for node %s\n", np->name);
> +		return;
> +	}
> +
> +	/* Register the input clock under the desired name. */
> +	parent_name = __clk_get_name(parent_clk);
> +
> +	if (of_property_read_string(np, "clock-output-names", &name))
> +		name = np->name;

Please just hardcode it unless you really need different names
from DT. We're trying to move away from clock-output-names for
clk tree description as it's inflexible in the face of DT ABI.

> +
> +	/* Works only as those 4 bits (Bits 28-31 big endian) do not cross byte boundary */

This line is too long, but also what's going on? Some sort of
64-bit register here?

> +	clk = clk_register_divider_table(NULL, name, parent_name,
> +				   0, base,
> +				   4, 4, 0, qspi_cfg_div_table, NULL);
> +	if (IS_ERR(clk)) {
> +		pr_warn("Failed to register divider table clock (%ld)\n", PTR_ERR(clk));
> +		return;
> +	}
> +	of_clk_add_provider(np, of_clk_src_simple_get, clk);

Can you please use the clk_hw based provider and divider
registration APIs?

> +}
> +CLK_OF_DECLARE(scfg_qspi_cfg_ls1021a, "fsl,scfg-qspi-cfg-ls1021a", scfg_qspi_cfg_ls1021a_init);

Can this be a platform driver? We usually reserve CLK_OF_DECLARE
for clks that we need to get the timer or interrupt controllers
working because they need to probe so early. Otherwise use a
proper driver and we can use platform device APIs instead of OF
specific ones to map register regions and get clks.
Alexander Stein April 12, 2017, 3:01 p.m. UTC | #2 
On Friday 07 April 2017 12:33:24, Stephen Boyd wrote:
> On 02/22, Alexander Stein wrote:
> > +static const struct clk_div_table qspi_cfg_div_table[] = {
> > +	{ 0x0, 256 }, { 0x1, 64 }, { 0x2, 32 }, { 0x3, 24 },
> > +	{ 0x4,  20 }, { 0x5, 15 }, { 0x6, 12 }, { 0x7,  8 },
> > +	{ 0 },
> > +};
> > +
> > +static void __init scfg_qspi_cfg_ls1021a_init(struct device_node *np)
> > +{
> > +	const char *parent_name;
> > +	const char *name;
> > +	void __iomem *base;
> > +	struct clk *parent_clk;
> > +	struct clk *clk;
> > +	struct resource res;
> 
> Unused? but should be used!

Just copy & paste mistake. It can be removed.

> > +
> > +	base = of_iomap(np, 0);
> > +	if (!base) {
> > +		pr_warn("Failed to map address range for node %s\n", np->name);
> 
> We don't typically need any sort of error message.

Ok.

> > +		return;
> > +	}
> > +
> > +	parent_clk = of_clk_get(np, 0);
> > +	if (IS_ERR(parent_clk)) {
> > +		pr_warn("Failed to get clock for node %s\n", np->name);
> > +		return;
> > +	}
> > +
> > +	/* Register the input clock under the desired name. */
> > +	parent_name = __clk_get_name(parent_clk);
> > +
> > +	if (of_property_read_string(np, "clock-output-names", &name))
> > +		name = np->name;
> 
> Please just hardcode it unless you really need different names
> from DT. We're trying to move away from clock-output-names for
> clk tree description as it's inflexible in the face of DT ABI.

But how do you then reference this clock from another DT node if clock-output-
names is remove dfrom patch 2/3? See path 3/3. I have to admit I'm not an 
expert on DT clocks.

> > +
> > +	/* Works only as those 4 bits (Bits 28-31 big endian) do not cross 
byte
> > boundary */
> This line is too long, but also what's going on? Some sort of
> 64-bit register here?

No, this is periphery attached as big-endian on a little-endian CPU, the 
infamous LS1021A has lots (but not all) of them. Finally this needs a proper 
clk improvement to support big-endian accesses on little-endian CPUs. Don't 
look at it (in detail), yet.

> > +	clk = clk_register_divider_table(NULL, name, parent_name,
> > +				   0, base,
> > +				   4, 4, 0, qspi_cfg_div_table, NULL);
> > +	if (IS_ERR(clk)) {
> > +		pr_warn("Failed to register divider table clock (%ld)\n",
> > PTR_ERR(clk));
> > +		return;
> > +	}
> > +	of_clk_add_provider(np, of_clk_src_simple_get, clk);
> 
> Can you please use the clk_hw based provider and divider
> registration APIs?

Is there a specific reason to use clk_hw based API? Both apparently do the 
same.

> > +}
> > +CLK_OF_DECLARE(scfg_qspi_cfg_ls1021a, "fsl,scfg-qspi-cfg-ls1021a",
> > scfg_qspi_cfg_ls1021a_init);
> Can this be a platform driver? We usually reserve CLK_OF_DECLARE
> for clks that we need to get the timer or interrupt controllers
> working because they need to probe so early. Otherwise use a
> proper driver and we can use platform device APIs instead of OF
> specific ones to map register regions and get clks.

As this is a simple clock divider neither timers nor interrupts are involved. 
There should be no problem to change this to a platform driver changing the 
init function into a probe one.

Best regards,
Alexander

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd April 19, 2017, 4:39 p.m. UTC | #3 
On 04/12, Alexander Stein wrote:
> On Friday 07 April 2017 12:33:24, Stephen Boyd wrote:
> > On 02/22, Alexander Stein wrote:
> > > +		return;
> > > +	}
> > > +
> > > +	parent_clk = of_clk_get(np, 0);
> > > +	if (IS_ERR(parent_clk)) {
> > > +		pr_warn("Failed to get clock for node %s\n", np->name);
> > > +		return;
> > > +	}
> > > +
> > > +	/* Register the input clock under the desired name. */
> > > +	parent_name = __clk_get_name(parent_clk);
> > > +
> > > +	if (of_property_read_string(np, "clock-output-names", &name))
> > > +		name = np->name;
> > 
> > Please just hardcode it unless you really need different names
> > from DT. We're trying to move away from clock-output-names for
> > clk tree description as it's inflexible in the face of DT ABI.
> 
> But how do you then reference this clock from another DT node if clock-output-
> names is remove dfrom patch 2/3? See path 3/3. I have to admit I'm not an 
> expert on DT clocks.

clocks = <&phandle cells...>? I'm not sure I follow the problem
here?

clock-output-names is optional.

> 
> > > +
> > > +	/* Works only as those 4 bits (Bits 28-31 big endian) do not cross 
> byte
> > > boundary */
> > This line is too long, but also what's going on? Some sort of
> > 64-bit register here?
> 
> No, this is periphery attached as big-endian on a little-endian CPU, the 
> infamous LS1021A has lots (but not all) of them. Finally this needs a proper 
> clk improvement to support big-endian accesses on little-endian CPUs. Don't 
> look at it (in detail), yet.
> 
> > > +	clk = clk_register_divider_table(NULL, name, parent_name,
> > > +				   0, base,
> > > +				   4, 4, 0, qspi_cfg_div_table, NULL);
> > > +	if (IS_ERR(clk)) {
> > > +		pr_warn("Failed to register divider table clock (%ld)\n",
> > > PTR_ERR(clk));
> > > +		return;
> > > +	}
> > > +	of_clk_add_provider(np, of_clk_src_simple_get, clk);
> > 
> > Can you please use the clk_hw based provider and divider
> > registration APIs?
> 
> Is there a specific reason to use clk_hw based API? Both apparently do the 
> same.

We're trying to split the consumer and provider APIs along struct
clk_hw and struct clk respectively. If we can have drivers only
registers clk_hw pointers and never get back anything but an
error code, then we can force consumers to always go through the
clk_get() family of APIs. Then we can easily tell who is a
provider, who is a consumer, and who is a provider + a consumer.
Right now this isn't always clear cut because clk_hw has access
to struct clk, and also clk_regsiter() returns a clk pointer, but
it doesn't really get used by anything in a provider driver,
unless provider drivers are doing something with the consumer
API.

> 
> > > +}
> > > +CLK_OF_DECLARE(scfg_qspi_cfg_ls1021a, "fsl,scfg-qspi-cfg-ls1021a",
> > > scfg_qspi_cfg_ls1021a_init);
> > Can this be a platform driver? We usually reserve CLK_OF_DECLARE
> > for clks that we need to get the timer or interrupt controllers
> > working because they need to probe so early. Otherwise use a
> > proper driver and we can use platform device APIs instead of OF
> > specific ones to map register regions and get clks.
> 
> As this is a simple clock divider neither timers nor interrupts are involved. 
> There should be no problem to change this to a platform driver changing the 
> init function into a probe one.
> 

Sounds great! Please do.
diff mbox

Patch

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 925081e..611f53f 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -26,6 +26,7 @@  obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
 obj-$(CONFIG_COMMON_CLK_CS2000_CP)	+= clk-cs2000-cp.o
 obj-$(CONFIG_ARCH_EFM32)		+= clk-efm32gg.o
 obj-$(CONFIG_ARCH_HIGHBANK)		+= clk-highbank.o
+obj-$(CONFIG_SOC_LS1021A)		+= clk-ls1021a.o
 obj-$(CONFIG_COMMON_CLK_MAX77686)	+= clk-max77686.o
 obj-$(CONFIG_ARCH_MB86S7X)		+= clk-mb86s7x.o
 obj-$(CONFIG_ARCH_MOXART)		+= clk-moxart.o
diff --git a/drivers/clk/clk-ls1021a.c b/drivers/clk/clk-ls1021a.c
new file mode 100644
index 0000000..2f64806
--- /dev/null
+++ b/drivers/clk/clk-ls1021a.c
@@ -0,0 +1,58 @@ 
+/*
+ * Copyright (C) 2017 SYS TEC electronic GmbH
+ * Alexander Stein <alexander.stein@systec-electronic.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/io.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+static const struct clk_div_table qspi_cfg_div_table[] = {
+	{ 0x0, 256 }, { 0x1, 64 }, { 0x2, 32 }, { 0x3, 24 },
+	{ 0x4,  20 }, { 0x5, 15 }, { 0x6, 12 }, { 0x7,  8 },
+	{ 0 },
+};
+
+static void __init scfg_qspi_cfg_ls1021a_init(struct device_node *np)
+{
+	const char *parent_name;
+	const char *name;
+	void __iomem *base;
+	struct clk *parent_clk;
+	struct clk *clk;
+	struct resource res;
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_warn("Failed to map address range for node %s\n", np->name);
+		return;
+	}
+
+	parent_clk = of_clk_get(np, 0);
+	if (IS_ERR(parent_clk)) {
+		pr_warn("Failed to get clock for node %s\n", np->name);
+		return;
+	}
+
+	/* Register the input clock under the desired name. */
+	parent_name = __clk_get_name(parent_clk);
+
+	if (of_property_read_string(np, "clock-output-names", &name))
+		name = np->name;
+
+	/* Works only as those 4 bits (Bits 28-31 big endian) do not cross byte boundary */
+	clk = clk_register_divider_table(NULL, name, parent_name,
+				   0, base,
+				   4, 4, 0, qspi_cfg_div_table, NULL);
+	if (IS_ERR(clk)) {
+		pr_warn("Failed to register divider table clock (%ld)\n", PTR_ERR(clk));
+		return;
+	}
+	of_clk_add_provider(np, of_clk_src_simple_get, clk);
+}
+CLK_OF_DECLARE(scfg_qspi_cfg_ls1021a, "fsl,scfg-qspi-cfg-ls1021a", scfg_qspi_cfg_ls1021a_init);