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 <[email protected]>
---
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 <[email protected]>
+ */
+
+#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
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 <[email protected]>
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 <[email protected]>
> + */
> +
> +#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.
Thanks for the reply.
Jan
> On 14 Nov 2018, at 23:19, Stephen Boyd <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
>> + */
>> +
>> +#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.
Quoting Janek Kotas (2018-11-15 00:27:15)
> Thanks for the reply.
> Jan
>
> > On 14 Nov 2018, at 23:19, Stephen Boyd <[email protected]> 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 <[email protected]>
> >
> > 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.
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.
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!