Re: [PATCH 2/2] clk: Add Fixed MMIO clock driver

[Janek Kotas]

Thanks for the reply.
Jan

> On 14 Nov 2018, at 23:19, Stephen Boyd <sboyd@xxxxxxxxxx> 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@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
>> + */
>> +
>> +#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.