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

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

[Linus Walleij]

On Mon, Jun 12, 2017 at 11:02 PM, Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Geert Uytterhoeven]

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

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

[Linus Walleij]

On Thu, Jun 15, 2017 at 10:55 AM, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Linus Walleij]

On Thu, Jun 15, 2017 at 11:00 PM, Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Philipp Zabel]

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

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

[Linus Walleij]

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