[PATCH 0/3] LS1021A: QSPI clock divider suport

[PATCH 0/3] LS1021A: QSPI clock divider suport

[Alexander Stein]

Hi,

first of all this is a set which is not supposed to be merged. I'll describe
later why. Therefore I also skipped any device tree binding documentation.
I'm trying to add QSPI clock divider support for LS1021A. This is a single
register with 4 bits specifying a divider in the set of 8, 12, 16, 20, 23, 32,
64 and 256.
As a prove of concept I've added 3 patches adding this feature.
1st patch:
Adding a clk_register_divider_table providing those divider set.

2nd patch:
DT node for this driver.

3rd patch:
DT node for QSPI periphery using this clock divider.

The biggest flaws in the current implementation:
* I don't like the idea of adding a driver for a specific register for a specific
  platform. Although LS1043 has the same register (even at same offset) but
  LS2080 has different valid values (e.g. 0 and 1 are reserved), different
  dividers and a different parent clock. I would like to provide this dividers
  in DT itself, but apparently there is no simple driver around
  clk_register_divider_table, no?
* The bigger problem is that the SCFG periphery containing this register is
  connected in big endian, even this is a little endian ARM CPU.
  The values in clk-ls1021a.c for divider offset is actually wrong, but works
  because this is read big endian.
  The problem is that clk_readl (used e.g. by clk-divider) and clk_writel
  assume to access by little endian, unless platform is PPC.
  I suspect adding endian awareness for those functions affects lots of
  users and seems non-trivial.

Any comments about the approach using clk_register_divider_table? How about a
specific driver providing a simple clock divider table in DT? And finally
any suggestions how to add endian awareness for clk_readl/clk_writel?

Best regards,
Alexander

Alexander Stein (3):
  clk: ls1021a: new platform clock driver
  ARM: dts: ls1021a: Add node for scfg-qspi-cfg
  ARM: dts: ls1021a: Add QSPI node

 arch/arm/boot/dts/ls1021a.dtsi | 24 +++++++++++++++++
 drivers/clk/Makefile           |  1 +
 drivers/clk/clk-ls1021a.c      | 58 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+)
 create mode 100644 drivers/clk/clk-ls1021a.c

-- 
2.10.2

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

[Alexander Stein]

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

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);
-- 
2.10.2

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

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Alexander Stein]

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

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

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] ARM: dts: ls1021a: Add node for scfg-qspi-cfg

[Alexander Stein]

This adds the node for SCFG_QSPI_CFG driver.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 arch/arm/boot/dts/ls1021a.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index 78a3d79..a2d58ef 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -176,6 +176,15 @@
 			big-endian;
 		};
 
+		scfg_qspi_cfg: scfg_qspi_cfg@157015c {
+		    compatible = "fsl,scfg-qspi-cfg-ls1021a";
+		    #clock-cells = <0>;
+		    reg = <0x0 0x157015c 0x0 0x4>;
+		    clocks = <&cga_pll1 0>;
+		    clock-output-names = "scfg-qspi-cfg";
+		    big-endian;
+		};
+
 		crypto: crypto@1700000 {
 			compatible = "fsl,sec-v5.0", "fsl,sec-v4.0";
 			fsl,sec-era = <7>;
-- 
2.10.2

[PATCH 3/3] ARM: dts: ls1021a: Add QSPI node

[Alexander Stein]

Add the QSPI node, disabled by default. Any chips and partitions must
be setup at board level.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 arch/arm/boot/dts/ls1021a.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index a2d58ef..7e412fb 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -147,6 +147,21 @@
 			big-endian;
 		};
 
+		qspi: quadspi@1550000 {
+			compatible = "fsl,ls1021a-qspi";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x0 0x1550000 0x0 0x10000>,
+				<0x0 0x40000000 0x0 0x20000000>;
+			reg-names = "QuadSPI", "QuadSPI-memory";
+			interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>;
+			clock-names = "qspi_en", "qspi";
+			clocks = <&scfg_qspi_cfg>, <&scfg_qspi_cfg>;
+			big-endian;
+			amba-base = <0x40000000>;
+			status = "disabled";
+		};
+
 		esdhc: esdhc@1560000 {
 			compatible = "fsl,esdhc";
 			reg = <0x0 0x1560000 0x0 0x10000>;
-- 
2.10.2