linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 2/2] clk: Add Fixed MMIO clock driver
@ 2018-11-14 15:24 Janek Kotas
  2018-11-14 22:19 ` Stephen Boyd
  0 siblings, 1 reply; 6+ messages in thread
From: Janek Kotas @ 2018-11-14 15:24 UTC (permalink / raw)
  To: mturquette, sboyd, robh+dt, mark.rutland
  Cc: linux-clk, devicetree, linux-kernel

This patch adds a driver for Fixed MMIO clock.
The driver reads a clock frequency value from a single 32-bit memory
mapped register and registers it as a fixed rate clock.

It can be enabled with COMMON_CLK_FIXED_MMIO Kconfig option.

Signed-off-by: Jan Kotas <jank@cadence.com>
---
 drivers/clk/Kconfig          |  6 ++++++
 drivers/clk/Makefile         |  1 +
 drivers/clk/clk-fixed-mmio.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)
 create mode 100644 drivers/clk/clk-fixed-mmio.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 81cdb4eaca..69c7fb859c 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -283,6 +283,12 @@ config COMMON_CLK_STM32H7
 	---help---
 	  Support for stm32h7 SoC family clocks
 
+config COMMON_CLK_FIXED_MMIO
+	bool "Clock driver for Memory Mapped Fixed values"
+	depends on COMMON_CLK && OF
+	help
+	  Support for Memory Mapped IO Fixed clocks
+
 source "drivers/clk/actions/Kconfig"
 source "drivers/clk/bcm/Kconfig"
 source "drivers/clk/hisilicon/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 72be7a38cf..4e61961dc1 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_COMMON_CLK_CDCE925)	+= clk-cdce925.o
 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_COMMON_CLK_FIXED_MMIO)	+= clk-fixed-mmio.o
 obj-$(CONFIG_COMMON_CLK_GEMINI)		+= clk-gemini.o
 obj-$(CONFIG_COMMON_CLK_ASPEED)		+= clk-aspeed.o
 obj-$(CONFIG_ARCH_HIGHBANK)		+= clk-highbank.o
diff --git a/drivers/clk/clk-fixed-mmio.c b/drivers/clk/clk-fixed-mmio.c
new file mode 100644
index 0000000000..bbcadab345
--- /dev/null
+++ b/drivers/clk/clk-fixed-mmio.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Memory Mapped IO Fixed Clock driver
+ *
+ * Copyright (C) 2018 Cadence Design Systems, Inc.
+ *
+ * Authors:
+ *	Jan Kotas <jank@cadence.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+static void __init of_fixed_mmio_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	const char *clk_name = node->name;
+	void __iomem *base;
+	u32 clk_freq;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%pOFn: failed to map address\n", node);
+		return;
+	}
+
+	clk_freq = readl(base);
+	iounmap(base);
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	clk = clk_register_fixed_rate(NULL, clk_name, NULL, 0, clk_freq);
+	if (IS_ERR(clk)) {
+		pr_err("%pOFn: failed to register fixed rate clock\n", node);
+		return;
+	}
+
+	if (of_clk_add_provider(node, of_clk_src_simple_get, clk)) {
+		pr_err("%pOFn: failed to add clock provider\n", node);
+		return;
+	}
+
+	pr_info("%pOFn: registered fixed-mmio-clock at %u Hz\n",
+		node, clk_freq);
+}
+
+CLK_OF_DECLARE(fixed_mmio_clk, "fixed-mmio-clock", of_fixed_mmio_clk_setup);
-- 
2.15.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] clk: Add Fixed MMIO clock driver
  2018-11-14 15:24 [PATCH 2/2] clk: Add Fixed MMIO clock driver Janek Kotas
@ 2018-11-14 22:19 ` Stephen Boyd
  2018-11-15  8:27   ` Janek Kotas
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2018-11-14 22:19 UTC (permalink / raw)
  To: mark.rutland, mturquette, robh+dt, Janek Kotas
  Cc: linux-clk, devicetree, linux-kernel

Quoting Janek Kotas (2018-11-14 07:24:39)
> This patch adds a driver for Fixed MMIO clock.
> The driver reads a clock frequency value from a single 32-bit memory
> mapped register and registers it as a fixed rate clock.
> 
> It can be enabled with COMMON_CLK_FIXED_MMIO Kconfig option.
> 
> Signed-off-by: Jan Kotas <jank@cadence.com>

Sounds like a fine idea. Except I can see how it will be abused if there
are a handful of these fixed rate "clks" somewhere in memory that all
get populated. 

Do you really have a fixed rate clk in hardware that exposes a single
register, or do you have a set of them that some firmware is populating
into an I/O memory space that we can read the fixed rates from? If it's
the latter, I wonder why we can't just have the firmware populate the
fixed rate clks into DT itself?

> diff --git a/drivers/clk/clk-fixed-mmio.c b/drivers/clk/clk-fixed-mmio.c
> new file mode 100644
> index 0000000000..bbcadab345
> --- /dev/null
> +++ b/drivers/clk/clk-fixed-mmio.c
> @@ -0,0 +1,49 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Memory Mapped IO Fixed Clock driver
> + *
> + * Copyright (C) 2018 Cadence Design Systems, Inc.
> + *
> + * Authors:
> + *     Jan Kotas <jank@cadence.com>
> + */
> +
> +#include <linux/clk.h>

Is this include used?

> +#include <linux/clk-provider.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>

Is this include used?

> +
> +static void __init of_fixed_mmio_clk_setup(struct device_node *node)
> +{
> +       struct clk *clk;
> +       const char *clk_name = node->name;
> +       void __iomem *base;
> +       u32 clk_freq;
> +
> +       base = of_iomap(node, 0);
> +       if (!base) {
> +               pr_err("%pOFn: failed to map address\n", node);
> +               return;
> +       }
> +
> +       clk_freq = readl(base);
> +       iounmap(base);
> +       of_property_read_string(node, "clock-output-names", &clk_name);
> +
> +       clk = clk_register_fixed_rate(NULL, clk_name, NULL, 0, clk_freq);
> +       if (IS_ERR(clk)) {
> +               pr_err("%pOFn: failed to register fixed rate clock\n", node);
> +               return;
> +       }
> +
> +       if (of_clk_add_provider(node, of_clk_src_simple_get, clk)) {
> +               pr_err("%pOFn: failed to add clock provider\n", node);
> +               return;
> +       }
> +
> +       pr_info("%pOFn: registered fixed-mmio-clock at %u Hz\n",
> +               node, clk_freq);

Please no "I'm alive!" messages.

> +}
> +
> +CLK_OF_DECLARE(fixed_mmio_clk, "fixed-mmio-clock", of_fixed_mmio_clk_setup);

Any reason why this can't be a platform driver? It would make this much
less DT specific and usable on other firmwares/platforms if we used a
platform driver here.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] clk: Add Fixed MMIO clock driver
  2018-11-14 22:19 ` Stephen Boyd
@ 2018-11-15  8:27   ` Janek Kotas
  2018-11-29 22:29     ` Stephen Boyd
  0 siblings, 1 reply; 6+ messages in thread
From: Janek Kotas @ 2018-11-15  8:27 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: mark.rutland, mturquette, robh+dt, linux-clk, devicetree, linux-kernel

Thanks for the reply.
Jan

> On 14 Nov 2018, at 23:19, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> Quoting Janek Kotas (2018-11-14 07:24:39)
>> This patch adds a driver for Fixed MMIO clock.
>> The driver reads a clock frequency value from a single 32-bit memory
>> mapped register and registers it as a fixed rate clock.
>> 
>> It can be enabled with COMMON_CLK_FIXED_MMIO Kconfig option.
>> 
>> Signed-off-by: Jan Kotas <jank@cadence.com>
> 
> Sounds like a fine idea. Except I can see how it will be abused if there
> are a handful of these fixed rate "clks" somewhere in memory that all
> get populated. 
> 
> Do you really have a fixed rate clk in hardware that exposes a single
> register, or do you have a set of them that some firmware is populating
> into an I/O memory space that we can read the fixed rates from? If it's
> the latter, I wonder why we can't just have the firmware populate the
> fixed rate clks into DT itself?

The first case.
We have a single register in a fixed location which contains 
the frequency of the main system clock.
This allows us to use the same image in emulation, FPGA
and simulation without any changes.
We usually don’t use a full bootloader, just a simple wrapper,
which initializes the necessary stuff and jumps to the kernel.

> 
>> diff --git a/drivers/clk/clk-fixed-mmio.c b/drivers/clk/clk-fixed-mmio.c
>> new file mode 100644
>> index 0000000000..bbcadab345
>> --- /dev/null
>> +++ b/drivers/clk/clk-fixed-mmio.c
>> @@ -0,0 +1,49 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +/*
>> + * Memory Mapped IO Fixed Clock driver
>> + *
>> + * Copyright (C) 2018 Cadence Design Systems, Inc.
>> + *
>> + * Authors:
>> + *     Jan Kotas <jank@cadence.com>
>> + */
>> +
>> +#include <linux/clk.h>
> 
> Is this include used?
> 
>> +#include <linux/clk-provider.h>
>> +#include <linux/of_address.h>
>> +#include <linux/regmap.h>
> 
> Is this include used?

Will cleanup these, thanks.

> 
>> +
>> +static void __init of_fixed_mmio_clk_setup(struct device_node *node)
>> +{
>> +       struct clk *clk;
>> +       const char *clk_name = node->name;
>> +       void __iomem *base;
>> +       u32 clk_freq;
>> +
>> +       base = of_iomap(node, 0);
>> +       if (!base) {
>> +               pr_err("%pOFn: failed to map address\n", node);
>> +               return;
>> +       }
>> +
>> +       clk_freq = readl(base);
>> +       iounmap(base);
>> +       of_property_read_string(node, "clock-output-names", &clk_name);
>> +
>> +       clk = clk_register_fixed_rate(NULL, clk_name, NULL, 0, clk_freq);
>> +       if (IS_ERR(clk)) {
>> +               pr_err("%pOFn: failed to register fixed rate clock\n", node);
>> +               return;
>> +       }
>> +
>> +       if (of_clk_add_provider(node, of_clk_src_simple_get, clk)) {
>> +               pr_err("%pOFn: failed to add clock provider\n", node);
>> +               return;
>> +       }
>> +
>> +       pr_info("%pOFn: registered fixed-mmio-clock at %u Hz\n",
>> +               node, clk_freq);
> 
> Please no "I'm alive!" messages.

Ok, I’ll remove it.

> 
>> +}
>> +
>> +CLK_OF_DECLARE(fixed_mmio_clk, "fixed-mmio-clock", of_fixed_mmio_clk_setup);
> 
> Any reason why this can't be a platform driver? It would make this much
> less DT specific and usable on other firmwares/platforms if we used a
> platform driver here.
> 

I looked at the fixed rate clock as a reference, but I can change it to 
a platform driver, if that’s preferred.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] clk: Add Fixed MMIO clock driver
  2018-11-15  8:27   ` Janek Kotas
@ 2018-11-29 22:29     ` Stephen Boyd
       [not found]       ` <76BDCB72-CC05-41D1-93A0-6D71CBD97651@global.cadence.com>
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2018-11-29 22:29 UTC (permalink / raw)
  To: Janek Kotas
  Cc: mark.rutland, mturquette, robh+dt, linux-clk, devicetree, linux-kernel

Quoting Janek Kotas (2018-11-15 00:27:15)
> Thanks for the reply.
> Jan
> 
> > On 14 Nov 2018, at 23:19, Stephen Boyd <sboyd@kernel.org> wrote:
> > 
> > Quoting Janek Kotas (2018-11-14 07:24:39)
> >> This patch adds a driver for Fixed MMIO clock.
> >> The driver reads a clock frequency value from a single 32-bit memory
> >> mapped register and registers it as a fixed rate clock.
> >> 
> >> It can be enabled with COMMON_CLK_FIXED_MMIO Kconfig option.
> >> 
> >> Signed-off-by: Jan Kotas <jank@cadence.com>
> > 
> > Sounds like a fine idea. Except I can see how it will be abused if there
> > are a handful of these fixed rate "clks" somewhere in memory that all
> > get populated. 
> > 
> > Do you really have a fixed rate clk in hardware that exposes a single
> > register, or do you have a set of them that some firmware is populating
> > into an I/O memory space that we can read the fixed rates from? If it's
> > the latter, I wonder why we can't just have the firmware populate the
> > fixed rate clks into DT itself?
> 
> The first case.
> We have a single register in a fixed location which contains 
> the frequency of the main system clock.
> This allows us to use the same image in emulation, FPGA
> and simulation without any changes.
> We usually don’t use a full bootloader, just a simple wrapper,
> which initializes the necessary stuff and jumps to the kernel.

So the hardware team has decided to throw a frequency register in there?
Alright! Does that fixed rate clk generate its fixed rate from some
other clk? I'm curious if this fixed rate clk has a parent source.

It would also be good to make sure that any clks registered from this
driver can't be populated from regions of memory like DDR. Can you
confirm? I think it will fail, but it would be worth checking

> 
> > 
> >> diff --git a/drivers/clk/clk-fixed-mmio.c b/drivers/clk/clk-fixed-mmio.c
> >> new file mode 100644
> >> index 0000000000..bbcadab345
> >> --- /dev/null
> >> +++ b/drivers/clk/clk-fixed-mmio.c
> 
> > 
> >> +}
> >> +
> >> +CLK_OF_DECLARE(fixed_mmio_clk, "fixed-mmio-clock", of_fixed_mmio_clk_setup);
> > 
> > Any reason why this can't be a platform driver? It would make this much
> > less DT specific and usable on other firmwares/platforms if we used a
> > platform driver here.
> > 
> 
> I looked at the fixed rate clock as a reference, but I can change it to 
> a platform driver, if that’s preferred.
> 

Yes I'd prefer a platform driver unless there's some reason it can't be
done. We may need to have both in case this needs to be populated very
early, but if that isn't the case then just a platform driver for now.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] clk: Add Fixed MMIO clock driver
       [not found]       ` <76BDCB72-CC05-41D1-93A0-6D71CBD97651@global.cadence.com>
@ 2018-11-30  8:58         ` Stephen Boyd
       [not found]           ` <63A29785-D1AC-4B7C-952E-3A2E5799A21D@global.cadence.com>
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2018-11-30  8:58 UTC (permalink / raw)
  To: Janek Kotas
  Cc: mark.rutland, mturquette, robh+dt, linux-clk, devicetree, linux-kernel

Quoting Janek Kotas (2018-11-30 00:52:16)
> 
>         We have a single register in a fixed location which contains 
>         the frequency of the main system clock.
>         This allows us to use the same image in emulation, FPGA
>         and simulation without any changes.
>         We usually don’t use a full bootloader, just a simple wrapper,
>         which initializes the necessary stuff and jumps to the kernel.
> 
> 
>     So the hardware team has decided to throw a frequency register in there?
>     Alright! Does that fixed rate clk generate its fixed rate from some
>     other clk? I’m curious if this fixed rate clk has a parent source.
> 
> 
> It depends, in simulation and emulation we can just generate 
> a clock source with a given frequency.
> On FPGA it’s usually an external oscillator, sometimes a fixed PLL.
> The driver is not intended to be used in full, complex SoCs.

Ok, sounds like it isn't worth modeling this then.

> 
> 
>     It would also be good to make sure that any clks registered from this
>     driver can't be populated from regions of memory like DDR. Can you
>     confirm? I think it will fail, but it would be worth checking
> 

Please try this.

>     Yes I'd prefer a platform driver unless there's some reason it can't be
>     done. We may need to have both in case this needs to be populated very
>     early, but if that isn’t the case then just a platform driver for now.
> 
> 
> I played with it a bit, and it and it looks like I need both.
> We use this clock source with some of our components, 
> and for some of them the platform driver was initialized too late.
> 

Ok. So do both then.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] clk: Add Fixed MMIO clock driver
       [not found]           ` <63A29785-D1AC-4B7C-952E-3A2E5799A21D@global.cadence.com>
@ 2018-11-30 18:56             ` Stephen Boyd
  0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2018-11-30 18:56 UTC (permalink / raw)
  To: Janek Kotas
  Cc: mark.rutland, mturquette, robh+dt, linux-clk, devicetree, linux-kernel

Quoting Janek Kotas (2018-11-30 01:57:09)
> 
> I tried it. io_remap doesn’t allow RAM to be mapped,
> even if it’s marked as a reserved memory.
> I got an error, the address couldn’t be mapped.
> 
> ARM64’s memory management has an explicit check for this here:
> https://elixir.bootlin.com/linux/latest/source/arch/arm64/mm/ioremap.c#L55
> 

Ok. Thanks!


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-11-30 18:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-14 15:24 [PATCH 2/2] clk: Add Fixed MMIO clock driver Janek Kotas
2018-11-14 22:19 ` Stephen Boyd
2018-11-15  8:27   ` Janek Kotas
2018-11-29 22:29     ` Stephen Boyd
     [not found]       ` <76BDCB72-CC05-41D1-93A0-6D71CBD97651@global.cadence.com>
2018-11-30  8:58         ` Stephen Boyd
     [not found]           ` <63A29785-D1AC-4B7C-952E-3A2E5799A21D@global.cadence.com>
2018-11-30 18:56             ` Stephen Boyd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).