linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Janek Kotas <jank@cadence.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
	"mturquette@baylibre.com" <mturquette@baylibre.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] clk: Add Fixed MMIO clock driver
Date: Thu, 15 Nov 2018 08:27:15 +0000	[thread overview]
Message-ID: <CAE55F09-516A-4737-8F74-07BBA3AF5737@global.cadence.com> (raw)
In-Reply-To: <154223394933.88331.14659021245427374668@swboyd.mtv.corp.google.com>

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.


  reply	other threads:[~2018-11-15  8:27 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-14 15:24 Janek Kotas
2018-11-14 22:19 ` Stephen Boyd
2018-11-15  8:27   ` Janek Kotas [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAE55F09-516A-4737-8F74-07BBA3AF5737@global.cadence.com \
    --to=jank@cadence.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mturquette@baylibre.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --subject='Re: [PATCH 2/2] clk: Add Fixed MMIO clock driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).