[2/2,v4] clk: Add Gemini SoC clock controller

Message ID	20170524082044.8473-1-linus.walleij@linaro.org (mailing list archive)
State	Superseded
Headers	show Return-Path: <linux-clk-owner@kernel.org> From: Linus Walleij <linus.walleij@linaro.org> To: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@codeaurora.org>, linux-clk@vger.kernel.org Cc: Janos Laube <janos.dev@gmail.com>, Paulius Zaleckas <paulius.zaleckas@gmail.com>, linux-arm-kernel@lists.infradead.org, Hans Ulli Kroll <ulli.kroll@googlemail.com>, Florian Fainelli <f.fainelli@gmail.com>, Linus Walleij <linus.walleij@linaro.org> Subject: [PATCH 2/2 v4] clk: Add Gemini SoC clock controller Date: Wed, 24 May 2017 10:20:44 +0200 Message-Id: <20170524082044.8473-1-linus.walleij@linaro.org> Sender: linux-clk-owner@vger.kernel.org Precedence: bulk

Linus Walleij May 24, 2017, 8:20 a.m. UTC

The Cortina Systems Gemini (SL3516/CS3516) has an on-chip clock
controller that derive all clocks from a single crystal, using some
documented and some undocumented PLLs, half dividers, counters and
gates. This is a best attempt to construct a clock driver for the
clocks so at least we can gate off unused hardware and driver the
PCI bus clock.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Mike/Stephen: please merge this into the clk subsystem once
you're happy with it. Sorry for the back-and-forth. I will
deal with the ARM SoC DTS landing orthogonally.

ChangeLog v3->v4:
- No changes, just resending with separate DT header file.
ChangeLog v2->v3:
- Augment driver to probe directly from the system controller.
- Mike/Stephen: when you're happy with this please ACK it so I
  can merge it through ARM SoC. This is needed because of
  the mess of #include <dt-bindings/*> from the DT bindings
  that is then used all over the DTS files.
ChangeLog v1->v2:
- Move the clock controller to be part of the syscon node. No
  need for a separate child node for this.
---
 drivers/clk/Kconfig      |   7 +
 drivers/clk/Makefile     |   1 +
 drivers/clk/clk-gemini.c | 357 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 365 insertions(+)
 create mode 100644 drivers/clk/clk-gemini.c

Stephen Boyd June 1, 2017, 7:02 a.m. UTC | #1

On 05/24, Linus Walleij wrote:
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 36cfea38135f..69178fd58ac8 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -126,6 +126,13 @@ config COMMON_CLK_CS2000_CP
>  	help
>  	  If you say yes here you get support for the CS2000 clock multiplier.
>  
> +config COMMON_CLK_GEMINI
> +	bool "Clock driver for Cortina Systems Gemini SoC"
> +	select MFD_SYSCON

Is there an appropriate depends on ARCH_FOO || COMPILE_TEST we
can put here? Just so that this isn't exposed to people who dont'
like seeing options they don't care about but we still get
compile testing coverage.

> +	---help---
> +	  This driver supports the SoC clocks on the Cortina Systems Gemini
> +	  platform, also known as SL3516 or CS3516.
> +
>  config COMMON_CLK_S2MPS11
>  	tristate "Clock driver for S2MPS1X/S5M8767 MFD"
>  	depends on MFD_SEC_CORE || COMPILE_TEST
> diff --git a/drivers/clk/clk-gemini.c b/drivers/clk/clk-gemini.c
> new file mode 100644
> index 000000000000..5a0de8415f91
> --- /dev/null
> +++ b/drivers/clk/clk-gemini.c
> @@ -0,0 +1,357 @@
> +/*
> + * Cortina Gemini Clock Controller driver
> + * Copyright (c) 2017 Linus Walleij <linus.walleij@linaro.org>
> + */
> +
> +#define pr_fmt(fmt) "clk-gemini: " fmt
> +
> +#include <linux/clkdev.h>

Used?

> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <dt-bindings/clock/cortina,gemini-clock.h>
> +
> +/* Globally visible clocks */
> +static DEFINE_SPINLOCK(gemini_clk_lock);

Include spinlock.h?

> +static struct clk *gemini_clks[GEMINI_NUM_CLKS];
> +static struct clk_onecell_data gemini_clk_data;
> +
> +#define GEMINI_GLOBAL_STATUS 0x04
> +#define PLL_OSC_SEL BIT(30)
> +#define AHBSPEED_SHIFT (15)
> +#define AHBSPEED_MASK 0x07
> +#define CPU_AHB_RATIO_SHIFT (18)
> +#define CPU_AHB_RATIO_MASK 0x03
> +
> +#define GEMINI_GLOBAL_PLL_CONTROL 0x08
> +
> +#define GEMINI_GLOBAL_MISC_CONTROL 0x30
> +#define PCI_CLK_66MHZ BIT(18)
> +#define PCI_CLK_OE BIT(17)
> +
> +#define GEMINI_GLOBAL_CLOCK_CONTROL 0x34
> +#define PCI_CLKRUN_EN BIT(16)
> +#define TVC_HALFDIV_SHIFT (24)
> +#define TVC_HALFDIV_MASK 0x1f
> +#define SECURITY_CLK_SEL BIT(29)
> +
> +#define GEMINI_GLOBAL_PCI_DLL_CONTROL 0x44
> +#define PCI_DLL_BYPASS BIT(31)
> +#define PCI_DLL_TAP_SEL_MASK 0x1f

Can these get some tabs to align up the numbers in each line? My
eyes can't easily read the values from the defines.

> +
> +struct gemini_gate_data {

This one doesn't get kernel doc? :(

> +	u8 bit_idx;
> +	const char *name;
> +	const char *parent_name;
> +	unsigned long flags;
> +};
> +
> +/**
> + * struct clk_gemini_pci - Gemini PCI clock
> + * @hw: corresponding clock hardware entry
> + * @map: regmap to access the registers
> + * @rate: current rate
> + */
> +struct clk_gemini_pci {
> +	struct clk_hw hw;
> +	struct regmap *map;
> +	unsigned long rate;
> +};
> +
> +/*
> + * FIXME: some clocks are marked as CLK_IGNORE_UNUSED: this is
> + * because their kernel drivers lack proper clock handling so we
> + * need to avoid them being gated off by default. Remove this as
> + * the drivers get fixed to handle clocks properly.
> + *
> + * The DDR controller may never have a driver, but certainly must
> + * not be gated off.

You can use CLK_IS_CRITICAL for that then.

> + */
> +static const struct gemini_gate_data gemini_gates[] __initconst = {
> +	{ 1, "security-gate", "secdiv", 0 },
> +	{ 2, "gmac0-gate", "ahb", 0 },
> +	{ 3, "gmac1-gate", "ahb", 0 },
> +	{ 4, "sata0-gate", "ahb", 0 },
> +	{ 5, "sata1-gate", "ahb", 0 },
> +	{ 6, "usb0-gate", "ahb", 0 },
> +	{ 7, "usb1-gate", "ahb", 0 },
> +	{ 8, "ide-gate", "ahb", 0 },
> +	{ 9, "pci-gate", "ahb", 0 },
> +	{ 10, "ddr-gate", "ahb", CLK_IGNORE_UNUSED },
> +	{ 11, "flash-gate", "ahb", CLK_IGNORE_UNUSED },
> +	{ 12, "tvc-gate", "ahb", 0 },
> +	{ 13, "boot-gate", "apb", 0 },
> +};
> +
[..]
> +
> +static void gemini_pci_disable(struct clk_hw *hw)
> +{
> +	struct clk_gemini_pci *pciclk = to_pciclk(hw);
> +
> +	regmap_update_bits(pciclk->map,

The regmap is atomic right? Just checking to make sure we aren't
taking a mutex instead of a spinlock inside these ops.

> +			   GEMINI_GLOBAL_MISC_CONTROL,
> +			   PCI_CLK_OE, 0);
> +	regmap_update_bits(pciclk->map, GEMINI_GLOBAL_CLOCK_CONTROL,
> +			   PCI_CLKRUN_EN, 0);

And regmap writes can't fail right?

> +}
> +
[..]
> +
> +static struct clk *gemini_pci_clk_setup(const char *name,

Would need to be marked __init if this doesn't become a platform
driver.

> +					const char *parent_name,
> +					struct regmap *map)
> +{
> +	struct clk_gemini_pci *pciclk;
> +	struct clk_init_data init;
> +	struct clk *clk;
> +
> +	pciclk = kzalloc(sizeof(*pciclk), GFP_KERNEL);
> +	if (!pciclk)
> +		return ERR_PTR(-ENOMEM);
> +
> +	init.name = name;
> +	init.ops = &gemini_pci_clk_ops;
> +	init.flags = 0;
> +	init.parent_names = (parent_name ? &parent_name : NULL);
> +	init.num_parents = (parent_name ? 1 : 0);

There's always a parent name though?

> +	pciclk->map = map;
> +	pciclk->hw.init = &init;
> +
> +	clk = clk_register(NULL, &pciclk->hw);
> +	if (IS_ERR(clk))
> +		kfree(pciclk);
> +
> +	return clk;
> +}
> +
> +static void __init gemini_cc_init(struct device_node *np)
> +{
> +	void __iomem *base;
> +	struct regmap *map;
> +	struct clk *clk;
> +	unsigned int mult, div;
> +	unsigned long freq;
> +	u32 val;
> +	int ret;
> +	int i;
> +
> +	/* Remap the system controller for the exclusive register */
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		pr_err("no memory base\n");
> +		return;
> +	}
> +	map = syscon_node_to_regmap(np);
> +	if (IS_ERR(map)) {
> +		pr_err("no syscon regmap\n");
> +		return;
> +	}
> +
> +	/* RTC clock 32768 Hz */
> +	clk = clk_register_fixed_rate(NULL, "rtc", NULL, CLK_IGNORE_UNUSED,
> +				      32768);
> +	gemini_clks[GEMINI_CLK_RTC] = clk;
> +
> +	ret = regmap_read(map, GEMINI_GLOBAL_STATUS, &val);
> +	if (ret) {
> +		pr_err("failed to read global status register\n");
> +		return;
> +	}
> +
> +	/*
> +	 * XTAL is the crystal oscillator, 60 or 30 MHz selected from
> +	 * strap pin E6
> +	 */
> +	if (val & PLL_OSC_SEL)
> +		freq = 30000000;
> +	else
> +		freq = 60000000;
> +	clk = clk_register_fixed_rate(NULL, "xtal", NULL, CLK_IGNORE_UNUSED,

The CLK_IGNORE_UNUSED is not really meaningful because fixed rate
clks don't have enable/disable ops. Please remove the flag from
all fixed rate clk registrations.

> +				      freq);
> +	pr_info("main crystal @%lu MHz\n", (freq/1000000));

Please add space around that freq / 1000000.

> +
> +	/* VCO clock derived from the crystal */
> +	mult = 13 + ((val >> AHBSPEED_SHIFT) & AHBSPEED_MASK);
> +	div = 2;
> +	/* If we run on 30 Mhz crystal we have to multiply with two */

s/Mhz/MHz/

> +	if (val & PLL_OSC_SEL)
> +		mult *= 2;
> +	clk = clk_register_fixed_factor(NULL, "vco", "xtal", CLK_IGNORE_UNUSED,
> +					mult, div);

Drop ignore unused flag.

> +
> +	/* The AHB clock is always 1/3 of the VCO */
> +	clk = clk_register_fixed_factor(NULL, "ahb", "vco",
> +					CLK_IGNORE_UNUSED, 1, 3);

Drop ignore unused flag.

Also, please move to using clk_hw_register*() APIs so that clk
pointers aren't used in this provider driver.

> +	gemini_clks[GEMINI_CLK_AHB] = clk;
> +
> +	/* The APB clock is always 1/6 of the AHB */
> +	clk = clk_register_fixed_factor(NULL, "apb", "ahb",
> +					CLK_IGNORE_UNUSED, 1, 6);
> +	gemini_clks[GEMINI_CLK_APB] = clk;
> +
> +	/* CPU clock derived as a fixed ratio from the AHB clock */
> +	switch ((val >> CPU_AHB_RATIO_SHIFT) & CPU_AHB_RATIO_MASK) {
> +	case 0x0:
> +		/* 1x */
> +		mult = 1;
> +		div = 1;
> +		break;
> +	case 0x1:
> +		/* 1.5x */
> +		mult = 3;
> +		div = 2;
> +		break;
> +	case 0x2:
> +		/* 1.85x */
> +		mult = 24;
> +		div = 13;
> +		break;
> +	case 0x3:
> +		/* 2x */
> +		mult = 2;
> +		div = 1;
> +		break;

Could this be an array of mult/div pairs that we index directly?
Would save some lines and save lives! Well maybe not that last
part.

> +	}
> +	clk = clk_register_fixed_factor(NULL, "cpu", "ahb",
> +					CLK_IGNORE_UNUSED, mult, div);
> +	gemini_clks[GEMINI_CLK_CPU] = clk;
> +
> +	/* Security clock is 1:1 or 0.75 of APB */
> +	ret = regmap_read(map, GEMINI_GLOBAL_CLOCK_CONTROL, &val);
> +	if (ret) {
> +		pr_err("failed to read global clock control register\n");
> +		return;
> +	}
> +	if (val & SECURITY_CLK_SEL) {
> +		mult = 1;
> +		div = 1;
> +	} else {
> +		mult = 3;
> +		div = 4;
> +	}
> +	clk = clk_register_fixed_factor(NULL, "secdiv", "ahb",
> +					0, mult, div);
> +
> +	/*
> +	 * These are the leaf gates, at boot no clocks are gated.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(gemini_gates); i++) {
> +		const struct gemini_gate_data *gd;
> +
> +		gd = &gemini_gates[i];
> +		gemini_clks[GEMINI_CLK_GATES + i] =
> +			clk_register_gate(NULL, gd->name,

clk_hw_register_gate().

> +					  gd->parent_name,
> +					  gd->flags,
> +					  base + GEMINI_GLOBAL_CLOCK_CONTROL,
> +					  gd->bit_idx,
> +					  CLK_GATE_SET_TO_DISABLE,
> +					  &gemini_clk_lock);
> +	}
> +
> +	/*
> +	 * The TV Interface Controller has a 5-bit half divider register.
> +	 * This clock is supposed to be 27MHz as this is an exact multiple
> +	 * of PAL and NTSC frequencies. The register is undocumented :(
> +	 * FIXME: figure out the parent and how the divider works.
> +	 */
> +	mult = 1;
> +	div = ((val >> TVC_HALFDIV_SHIFT) & TVC_HALFDIV_MASK);
> +	pr_debug("TVC half divider value = %d\n", div);
> +	div += 1;
> +	clk = clk_register_fixed_rate(NULL, "tvcdiv", "xtal", 0, 27000000);
> +	gemini_clks[GEMINI_CLK_TVC] = clk;
> +
> +	/* FIXME: very unclear what the parent is */
> +	clk = gemini_pci_clk_setup("PCI", "xtal", map);
> +	gemini_clks[GEMINI_CLK_PCI] = clk;
> +
> +	/* FIXME: very unclear what the parent is */
> +	clk = clk_register_fixed_rate(NULL, "uart", "xtal", CLK_IGNORE_UNUSED,
> +				      48000000);
> +	gemini_clks[GEMINI_CLK_UART] = clk;
> +
> +	/* Register the clocks to be accessed by the device tree */
> +	gemini_clk_data.clks = gemini_clks;
> +	gemini_clk_data.clk_num = ARRAY_SIZE(gemini_clks);
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &gemini_clk_data);

And of_clk_add_hw_provider().

> +}
> +CLK_OF_DECLARE_DRIVER(gemini_cc, "cortina,gemini-syscon", gemini_cc_init);

Is there a reason why this isn't a platform driver?

Linus Walleij June 5, 2017, 1:34 p.m. UTC | #2

On Thu, Jun 1, 2017 at 9:02 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

>> +CLK_OF_DECLARE_DRIVER(gemini_cc, "cortina,gemini-syscon", gemini_cc_init);
>
> Is there a reason why this isn't a platform driver?

I can't get the platform to boot if I try, I guess because the console
UART needs the clock like super-early.

I fixed all the other issues, resending soon.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd June 5, 2017, 7:58 p.m. UTC | #3

On 06/05, Linus Walleij wrote:
> On Thu, Jun 1, 2017 at 9:02 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> >> +CLK_OF_DECLARE_DRIVER(gemini_cc, "cortina,gemini-syscon", gemini_cc_init);
> >
> > Is there a reason why this isn't a platform driver?
> 
> I can't get the platform to boot if I try, I guess because the console
> UART needs the clock like super-early.
> 
> I fixed all the other issues, resending soon.
> 

Does the console uart driver support probe defer?

Linus Walleij June 8, 2017, 12:18 p.m. UTC | #4

On Mon, Jun 5, 2017 at 9:58 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/05, Linus Walleij wrote:
>> On Thu, Jun 1, 2017 at 9:02 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>
>> >> +CLK_OF_DECLARE_DRIVER(gemini_cc, "cortina,gemini-syscon", gemini_cc_init);
>> >
>> > Is there a reason why this isn't a platform driver?
>>
>> I can't get the platform to boot if I try, I guess because the console
>> UART needs the clock like super-early.
>>
>> I fixed all the other issues, resending soon.
>>
>
> Does the console uart driver support probe defer?

It's the 8250_of.c driver.

It's such a big driver that I don't dare to say, I guess I would
have to go in and debug and patch around in the 8250 driver
if you insist.

But I'm not even sure that is the problem actually, I'd have to
analyze.

I think the timer may be more of an issue...

It is using CLOCKSOURCE_OF_DECLARE(), at least last time
I checked there was no such thing as clocksources retrying their
calls, and that cannot be regular probes because of, yeah device
core is using the timer, I think.

Yeah I looked in:
drivers/clocksource/clksrc-probe.c

It will just fail if it can't initialize the clocksource.

I don't understand what platforms can actually get their timers
up at all without the clock framework? Those hardcoding the
clock frequency in the device tree, or things like the ARM
TWD which has it encoded in hardware perhaps?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Walleij June 12, 2017, 6:21 a.m. UTC | #5

On Thu, Jun 8, 2017 at 2:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> I think the timer may be more of an issue...
>
> It is using CLOCKSOURCE_OF_DECLARE(), at least last time
> I checked there was no such thing as clocksources retrying their
> calls, and that cannot be regular probes because of, yeah device
> core is using the timer, I think.
>
> Yeah I looked in:
> drivers/clocksource/clksrc-probe.c
>
> It will just fail if it can't initialize the clocksource.
>
> I don't understand what platforms can actually get their timers
> up at all without the clock framework? Those hardcoding the
> clock frequency in the device tree, or things like the ARM
> TWD which has it encoded in hardware perhaps?

I just confirmed this to be the problem using a scratch patch for
a platform device init and some earlydebug prints:

Uncompressing Linux... done, booting the kernel.
 Booting Linux on physical CPU 0x0
start_kernel
(...)
 NR_IRQS:16 nr_irqs:16 16
 could not get PCLK
Failed to initialize '/soc/timer@43000000': -517
 clocksource_probe: no matching clocksources found
 sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every
21474836475000000ns
 Console: colour dummy device 80x30
 Calibrating delay loop...

So the deferred clock makes the whole platform hang because the timer
needs it. Without a clocksource, the delay loop cannot be calibrated, so
it hangs there.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd June 12, 2017, 9:02 p.m. UTC | #6

On 06/12, Linus Walleij wrote:
> On Thu, Jun 8, 2017 at 2:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > I think the timer may be more of an issue...
> >
> > It is using CLOCKSOURCE_OF_DECLARE(), at least last time
> > I checked there was no such thing as clocksources retrying their
> > calls, and that cannot be regular probes because of, yeah device
> > core is using the timer, I think.
> >
> > Yeah I looked in:
> > drivers/clocksource/clksrc-probe.c
> >
> > It will just fail if it can't initialize the clocksource.
> >
> > I don't understand what platforms can actually get their timers
> > up at all without the clock framework? Those hardcoding the
> > clock frequency in the device tree, or things like the ARM
> > TWD which has it encoded in hardware perhaps?
> 
> I just confirmed this to be the problem using a scratch patch for
> a platform device init and some earlydebug prints:
> 
> Uncompressing Linux... done, booting the kernel.
>  Booting Linux on physical CPU 0x0
> start_kernel
> (...)
>  NR_IRQS:16 nr_irqs:16 16
>  could not get PCLK
> Failed to initialize '/soc/timer@43000000': -517
>  clocksource_probe: no matching clocksources found
>  sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every
> 21474836475000000ns
>  Console: colour dummy device 80x30
>  Calibrating delay loop...
> 
> So the deferred clock makes the whole platform hang because the timer
> needs it. Without a clocksource, the delay loop cannot be calibrated, so
> it hangs there.
> 

Ok. So can the certain clks that are required to get the timer
going be put into CLK_OF_DECLARE_DRIVER() and then have a regular
platform driver for the rest of the clks that aren't required for
early boot? We've been doing this sort of hybrid design lately,
so hopefully that works here too.

Linus Walleij June 14, 2017, 11:31 a.m. UTC | #7

On Mon, Jun 12, 2017 at 11:02 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/12, Linus Walleij wrote:

>> So the deferred clock makes the whole platform hang because the timer
>> needs it. Without a clocksource, the delay loop cannot be calibrated, so
>> it hangs there.
>
> Ok. So can the certain clks that are required to get the timer
> going be put into CLK_OF_DECLARE_DRIVER() and then have a regular
> platform driver for the rest of the clks that aren't required for
> early boot? We've been doing this sort of hybrid design lately,
> so hopefully that works here too.

Sure I will give it a spin!

Is there a best-in-class driver doing this I can use as inspiration?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd June 14, 2017, 3:55 p.m. UTC | #8

On 06/14, Linus Walleij wrote:
> On Mon, Jun 12, 2017 at 11:02 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 06/12, Linus Walleij wrote:
> 
> >> So the deferred clock makes the whole platform hang because the timer
> >> needs it. Without a clocksource, the delay loop cannot be calibrated, so
> >> it hangs there.
> >
> > Ok. So can the certain clks that are required to get the timer
> > going be put into CLK_OF_DECLARE_DRIVER() and then have a regular
> > platform driver for the rest of the clks that aren't required for
> > early boot? We've been doing this sort of hybrid design lately,
> > so hopefully that works here too.
> 
> Sure I will give it a spin!
> 
> Is there a best-in-class driver doing this I can use as inspiration?
> 

Great. I suppose drivers/clk/nxp/clk-lpc18xx-creg.c or
drivers/clk/axis/clk-artpec6.c would be good examples.

Linus Walleij June 15, 2017, 7:16 a.m. UTC | #9

On Mon, Jun 12, 2017 at 11:02 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> So can the certain clks that are required to get the timer
> going be put into CLK_OF_DECLARE_DRIVER() and then have a regular
> platform driver for the rest of the clks that aren't required for
> early boot? We've been doing this sort of hybrid design lately,
> so hopefully that works here too.

So I tried this hybrid approach.

It works and it doesn't work, it is very annoying actually... we get
a conflict of interest between the clock driver, the reset driver and
the device tree bindings and how Linux uses device tree.

The reason is that no less than three devices probe from the same
device tree node, essentially this is the problem:

syscon: syscon@40000000 {
      compatible = "cortina,gemini-syscon", "syscon";
      reg = <0x40000000 0x1000>;
      #clock-cells = <1>;
      #reset-cells = <1>;
};

This has already a driver in drivers/reset/reset-gemini.c
binding and probing from "cortina,gemini-syscon".

That works fine, because CLK_OF_DECLARE_DRIVER() does not
bind to the device using the device core, and syscon will always probe
itself when the first user tries to access it.

If we make the clocks bind to the platform device, the reset
controller will not probe, regressing the boot in another way, because
some drivers need their reset lines.

The natural suggestion is then to make a subnode (and thus
subdevices) in the syscon, like in my original suggestion:
http://marc.info/?l=linux-arm-kernel&m=149306019417796&w=2

> +syscon: syscon@40000000 {
> +     compatible = "cortina,gemini-syscon", "syscon", "simple-mfd";
> +     reg = <0x40000000 0x1000>;
> +
> +     clock-controller {
> +         compatible = "cortina,gemini-clock-controller";
> +         #clock-cells = <1>;
> +     };
> + };

But to that design Rob said:
http://marc.info/?l=linux-arm-kernel&m=149340388608747&w=2

>> +syscon: syscon@40000000 {
>> +     compatible = "cortina,gemini-syscon", "syscon", "simple-mfd";
>> +     reg = <0x40000000 0x1000>;
>> +
>> +     clock-controller {
>> +         compatible = "cortina,gemini-clock-controller";
>> +         #clock-cells = <1>;
>
> There's not really much reason to have a child node here. The parent can
> be the clock provider.

And this approach worked as long as we were using
CLK_OF_DECLARE_DRIVER() to probe the clocks.

So that is why it looks as it looks.

I'm stuck between a rock and a hard place here.

Ways forward:

- We can go back to the older device tree bindings. These are
  ACKed and merged but we have patched worse things
  after the fact. We add back the subnode (also for the
  reset controller then, so it looks coherent). But I don't know how
  the DT maintainers would feel about that. It's not DT's fault that
  Linux can't bind more than one driver to a single DT node.

- We can stay with CLK_OF_DECLARE_DRIVER() and things
  JustWork(TM). I.e. just apply the v5 patch and be happy,
  concluding that Linux as a whole can't do anything better
  right now.

- I can vaguely think about ways of working around the Linux
  device driver model to spawn the child nodes from the system
  controller inside the kernel without subnodes. That would
  essentially duplicate the work that "simple-mfd" is doing on
  the system controller if we kept the device nodes, and
  introduce a new MFD driver to do just this.

Ideas? Stehen, Rob, Philipp? We need to work together to find the
best way out of this...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven June 15, 2017, 8:55 a.m. UTC | #10

Hi Liinus,

On Thu, Jun 15, 2017 at 9:16 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Jun 12, 2017 at 11:02 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> So can the certain clks that are required to get the timer
>> going be put into CLK_OF_DECLARE_DRIVER() and then have a regular
>> platform driver for the rest of the clks that aren't required for
>> early boot? We've been doing this sort of hybrid design lately,
>> so hopefully that works here too.
>
> So I tried this hybrid approach.
>
> It works and it doesn't work, it is very annoying actually... we get
> a conflict of interest between the clock driver, the reset driver and
> the device tree bindings and how Linux uses device tree.
>
> The reason is that no less than three devices probe from the same
> device tree node, essentially this is the problem:
>
> syscon: syscon@40000000 {
>       compatible = "cortina,gemini-syscon", "syscon";
>       reg = <0x40000000 0x1000>;
>       #clock-cells = <1>;
>       #reset-cells = <1>;
> };
>
> This has already a driver in drivers/reset/reset-gemini.c
> binding and probing from "cortina,gemini-syscon".
>
> That works fine, because CLK_OF_DECLARE_DRIVER() does not
> bind to the device using the device core, and syscon will always probe
> itself when the first user tries to access it.
>
> If we make the clocks bind to the platform device, the reset
> controller will not probe, regressing the boot in another way, because
> some drivers need their reset lines.

If clocks and resets are provided by the same hardware module, you can
have a single (platform) driver registering both the clock and reset
controllers.
Cfr. drivers/clk/renesas/renesas-cpg-mssr.c.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Walleij June 15, 2017, 12:57 p.m. UTC | #11

On Thu, Jun 15, 2017 at 10:55 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

> If clocks and resets are provided by the same hardware module, you can
> have a single (platform) driver registering both the clock and reset
> controllers.
> Cfr. drivers/clk/renesas/renesas-cpg-mssr.c.

That is indeed an option.

So I would say, clk & reset maintainers: would you prefer that I merge the
reset control into the clock driver as well, ask Philipp to drop the pending
reset control patches from his subsystem tree and have you manage the
combined driver and bindings?

It seems to me as very ugly from a divide & conquer subsystem and file
split point of view.

I seems elegant from the "make clocks a platform device" point of view.

I am happy with either approach as long as it works.

I guess it is up to the taste of the subsystem maintainers, especially
clk.

If I get some time I might just hack this up and send the patches so
it is on the table as an alternative to the current v5 patch. Certainly it is
better than going back and augmenting the DT bindings.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd June 15, 2017, 9 p.m. UTC | #12

On 06/15, Linus Walleij wrote:
> On Thu, Jun 15, 2017 at 10:55 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> 
> > If clocks and resets are provided by the same hardware module, you can
> > have a single (platform) driver registering both the clock and reset
> > controllers.
> > Cfr. drivers/clk/renesas/renesas-cpg-mssr.c.
> 
> That is indeed an option.
> 
> So I would say, clk & reset maintainers: would you prefer that I merge the
> reset control into the clock driver as well, ask Philipp to drop the pending
> reset control patches from his subsystem tree and have you manage the
> combined driver and bindings?
> 
> It seems to me as very ugly from a divide & conquer subsystem and file
> split point of view.
> 
> I seems elegant from the "make clocks a platform device" point of view.
> 
> I am happy with either approach as long as it works.
> 
> I guess it is up to the taste of the subsystem maintainers, especially
> clk.
> 
> If I get some time I might just hack this up and send the patches so
> it is on the table as an alternative to the current v5 patch. Certainly it is
> better than going back and augmenting the DT bindings.
> 

We have quite a few clock and reset controllers that put their
drivers into the clk directory. I don't see any problem with that
approach.

Philipp Zabel June 15, 2017, 9:55 p.m. UTC | #13

Hi Linus,

On Thu, Jun 15, 2017 at 02:57:53PM +0200, Linus Walleij wrote:
> On Thu, Jun 15, 2017 at 10:55 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> 
> > If clocks and resets are provided by the same hardware module, you can
> > have a single (platform) driver registering both the clock and reset
> > controllers.
> > Cfr. drivers/clk/renesas/renesas-cpg-mssr.c.
> 
> That is indeed an option.
> 
> So I would say, clk & reset maintainers: would you prefer that I merge the
> reset control into the clock driver as well, ask Philipp to drop the pending
> reset control patches from his subsystem tree and have you manage the
> combined driver and bindings?

The reset/next pull requests are not merged into the arm-soc tree yet.
I suppose I could retract the pull requests and drop the Gemini reset
patches, if the patches in arm-soc/gemeni/dts are also dropped from
arm-soc/for-next.

> It seems to me as very ugly from a divide & conquer subsystem and file
> split point of view.
>
> I seems elegant from the "make clocks a platform device" point of view.
> 
> I am happy with either approach as long as it works.
> 
> I guess it is up to the taste of the subsystem maintainers, especially
> clk.
>
> If I get some time I might just hack this up and send the patches so
> it is on the table as an alternative to the current v5 patch. Certainly it is
> better than going back and augmenting the DT bindings.

I have a slight preference for keeping the DT bindings simple, even if
that means merging the reset controller into the clock driver.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Walleij June 16, 2017, 8:35 a.m. UTC | #14

On Thu, Jun 15, 2017 at 11:00 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> We have quite a few clock and reset controllers that put their
> drivers into the clk directory. I don't see any problem with that
> approach.

OK I'll proceed like this!

Thanks,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Walleij June 16, 2017, 8:38 a.m. UTC | #15

On Thu, Jun 15, 2017 at 11:55 PM, Philipp Zabel <pza@pengutronix.de> wrote:
> On Thu, Jun 15, 2017 at 02:57:53PM +0200, Linus Walleij wrote:

>> So I would say, clk & reset maintainers: would you prefer that I merge the
>> reset control into the clock driver as well, ask Philipp to drop the pending
>> reset control patches from his subsystem tree and have you manage the
>> combined driver and bindings?
>
> The reset/next pull requests are not merged into the arm-soc tree yet.
> I suppose I could retract the pull requests and drop the Gemini reset
> patches,

There is no need. You can simply revert the patches, it's OK that the
drivers bounce in and out of the kernel.

The clock driver will just probe instead of the reset driver (due to being
earlier in the link order... OK fragile but it works), so there
will not even be a runtime conflict.

> if the patches in arm-soc/gemeni/dts are also dropped from
> arm-soc/for-next.

There is no need for that, the bindings do not change with this approach.

> I have a slight preference for keeping the DT bindings simple, even if
> that means merging the reset controller into the clock driver.

OK then that is what we're gonna do.

I'm onto it!

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[2/2,v4] clk: Add Gemini SoC clock controller

Commit Message

Comments

Patch