[PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Sjoerd Simons]

clk_get on a disabled clock node will return EPROBE_DEFER, which can
cause drivers to be deferred forever if such clocks are referenced in
their clocks property.

Update the various disabled external clock nodes to default to a
frequency of 0, but don't disable them to prevent this.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

---

 arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
 arch/arm/boot/dts/r8a7791-porter.dts  | 1 +
 arch/arm/boot/dts/r8a7791.dtsi        | 5 +----
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts
index 1adf877..da59c28 100644
--- a/arch/arm/boot/dts/r8a7791-koelsch.dts
+++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
@@ -660,6 +660,7 @@
 };
 
 &pcie_bus_clk {
+	clock-frequency = <100000000>;
 	status = "okay";
 };
 
diff --git a/arch/arm/boot/dts/r8a7791-porter.dts b/arch/arm/boot/dts/r8a7791-porter.dts
index 9554d13..19b257e 100644
--- a/arch/arm/boot/dts/r8a7791-porter.dts
+++ b/arch/arm/boot/dts/r8a7791-porter.dts
@@ -413,6 +413,7 @@
 };
 
 &pcie_bus_clk {
+	clock-frequency = <100000000>;
 	status = "okay";
 };
 
diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index 8693888..676df63 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -1104,8 +1104,7 @@
 		pcie_bus_clk: pcie_bus {
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
-			clock-frequency = <100000000>;
-			status = "disabled";
+			clock-frequency = <0>;
 		};
 
 		/* External SCIF clock */
@@ -1114,7 +1113,6 @@
 			#clock-cells = <0>;
 			/* This value must be overridden by the board. */
 			clock-frequency = <0>;
-			status = "disabled";
 		};
 
 		/* External USB clock - can be overridden by the board */
@@ -1130,7 +1128,6 @@
 			#clock-cells = <0>;
 			/* This value must be overridden by the board. */
 			clock-frequency = <0>;
-			status = "disabled";
 		};
 
 		/* Special CPG clocks */
-- 
2.8.0.rc3

Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Geert Uytterhoeven]

CC Mike, Stephen, linux-clk

On Wed, Apr 6, 2016 at 2:52 PM, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> clk_get on a disabled clock node will return EPROBE_DEFER, which can
> cause drivers to be deferred forever if such clocks are referenced in
> their clocks property.

Is this a side effect of commit 3e5dd6f6e690048d ("clk: Ignore disabled DT
clock providers")?

> Update the various disabled external clock nodes to default to a
> frequency of 0, but don't disable them to prevent this.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>
> ---
>
>  arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
>  arch/arm/boot/dts/r8a7791-porter.dts  | 1 +
>  arch/arm/boot/dts/r8a7791.dtsi        | 5 +----
>  3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts
> index 1adf877..da59c28 100644
> --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> @@ -660,6 +660,7 @@
>  };
>
>  &pcie_bus_clk {
> +       clock-frequency = <100000000>;
>         status = "okay";
>  };
>
> diff --git a/arch/arm/boot/dts/r8a7791-porter.dts b/arch/arm/boot/dts/r8a7791-porter.dts
> index 9554d13..19b257e 100644
> --- a/arch/arm/boot/dts/r8a7791-porter.dts
> +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> @@ -413,6 +413,7 @@
>  };
>
>  &pcie_bus_clk {
> +       clock-frequency = <100000000>;
>         status = "okay";
>  };
>
> diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
> index 8693888..676df63 100644
> --- a/arch/arm/boot/dts/r8a7791.dtsi
> +++ b/arch/arm/boot/dts/r8a7791.dtsi
> @@ -1104,8 +1104,7 @@
>                 pcie_bus_clk: pcie_bus {
>                         compatible = "fixed-clock";
>                         #clock-cells = <0>;
> -                       clock-frequency = <100000000>;
> -                       status = "disabled";
> +                       clock-frequency = <0>;
>                 };
>
>                 /* External SCIF clock */
> @@ -1114,7 +1113,6 @@
>                         #clock-cells = <0>;
>                         /* This value must be overridden by the board. */
>                         clock-frequency = <0>;
> -                       status = "disabled";
>                 };
>
>                 /* External USB clock - can be overridden by the board */
> @@ -1130,7 +1128,6 @@
>                         #clock-cells = <0>;
>                         /* This value must be overridden by the board. */
>                         clock-frequency = <0>;
> -                       status = "disabled";
>                 };
>
>                 /* Special CPG clocks */
> --
> 2.8.0.rc3

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] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Geert Uytterhoeven]

CC Mike, Stephen, linux-clk (this time with the new Mike)

On Wed, Apr 6, 2016 at 2:52 PM, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> clk_get on a disabled clock node will return EPROBE_DEFER, which can
> cause drivers to be deferred forever if such clocks are referenced in
> their clocks property.

Is this a side effect of commit 3e5dd6f6e690048d ("clk: Ignore disabled DT
clock providers")?

> Update the various disabled external clock nodes to default to a
> frequency of 0, but don't disable them to prevent this.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>
> ---
>
>  arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
>  arch/arm/boot/dts/r8a7791-porter.dts  | 1 +
>  arch/arm/boot/dts/r8a7791.dtsi        | 5 +----
>  3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts
> index 1adf877..da59c28 100644
> --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> @@ -660,6 +660,7 @@
>  };
>
>  &pcie_bus_clk {
> +       clock-frequency = <100000000>;
>         status = "okay";
>  };
>
> diff --git a/arch/arm/boot/dts/r8a7791-porter.dts b/arch/arm/boot/dts/r8a7791-porter.dts
> index 9554d13..19b257e 100644
> --- a/arch/arm/boot/dts/r8a7791-porter.dts
> +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> @@ -413,6 +413,7 @@
>  };
>
>  &pcie_bus_clk {
> +       clock-frequency = <100000000>;
>         status = "okay";
>  };
>
> diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
> index 8693888..676df63 100644
> --- a/arch/arm/boot/dts/r8a7791.dtsi
> +++ b/arch/arm/boot/dts/r8a7791.dtsi
> @@ -1104,8 +1104,7 @@
>                 pcie_bus_clk: pcie_bus {
>                         compatible = "fixed-clock";
>                         #clock-cells = <0>;
> -                       clock-frequency = <100000000>;
> -                       status = "disabled";
> +                       clock-frequency = <0>;
>                 };
>
>                 /* External SCIF clock */
> @@ -1114,7 +1113,6 @@
>                         #clock-cells = <0>;
>                         /* This value must be overridden by the board. */
>                         clock-frequency = <0>;
> -                       status = "disabled";
>                 };
>
>                 /* External USB clock - can be overridden by the board */
> @@ -1130,7 +1128,6 @@
>                         #clock-cells = <0>;
>                         /* This value must be overridden by the board. */
>                         clock-frequency = <0>;
> -                       status = "disabled";
>                 };
>
>                 /* Special CPG clocks */
> --
> 2.8.0.rc3

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] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Sjoerd Simons]

On Wed, 2016-04-06 at 15:11 +0200, Geert Uytterhoeven wrote:
> CC Mike, Stephen, linux-clk (this time with the new Mike)
> 
> On Wed, Apr 6, 2016 at 2:52 PM, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> > 
> > clk_get on a disabled clock node will return EPROBE_DEFER, which
> > can
> > cause drivers to be deferred forever if such clocks are referenced
> > in
> > their clocks property.
> Is this a side effect of commit 3e5dd6f6e690048d ("clk: Ignore
> disabled DT
> clock providers")?

Yes it seems so. Reverting that patch means that i can drop this one
and get the expected behaviour again.

Though even so I'm not sure what the convention is for clocks like
these, the r8a7791.dtsi is inconsistent, as some are disabled while
others (e.g. the audio clocks) are 0hz. Would be good to get some input
on that regardless.

> > 
> > Update the various disabled external clock nodes to default to a
> > frequency of 0, but don't disable them to prevent this.
> > 
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > 
> > ---
> > 
> >  arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
> >  arch/arm/boot/dts/r8a7791-porter.dts  | 1 +
> >  arch/arm/boot/dts/r8a7791.dtsi        | 5 +----
> >  3 files changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts
> > b/arch/arm/boot/dts/r8a7791-koelsch.dts
> > index 1adf877..da59c28 100644
> > --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> > +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> > @@ -660,6 +660,7 @@
> >  };
> > 
> >  &pcie_bus_clk {
> > +       clock-frequency = <100000000>;
> >         status = "okay";
> >  };
> > 
> > diff --git a/arch/arm/boot/dts/r8a7791-porter.dts
> > b/arch/arm/boot/dts/r8a7791-porter.dts
> > index 9554d13..19b257e 100644
> > --- a/arch/arm/boot/dts/r8a7791-porter.dts
> > +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> > @@ -413,6 +413,7 @@
> >  };
> > 
> >  &pcie_bus_clk {
> > +       clock-frequency = <100000000>;
> >         status = "okay";
> >  };
> > 
> > diff --git a/arch/arm/boot/dts/r8a7791.dtsi
> > b/arch/arm/boot/dts/r8a7791.dtsi
> > index 8693888..676df63 100644
> > --- a/arch/arm/boot/dts/r8a7791.dtsi
> > +++ b/arch/arm/boot/dts/r8a7791.dtsi
> > @@ -1104,8 +1104,7 @@
> >                 pcie_bus_clk: pcie_bus {
> >                         compatible = "fixed-clock";
> >                         #clock-cells = <0>;
> > -                       clock-frequency = <100000000>;
> > -                       status = "disabled";
> > +                       clock-frequency = <0>;
> >                 };
> > 
> >                 /* External SCIF clock */
> > @@ -1114,7 +1113,6 @@
> >                         #clock-cells = <0>;
> >                         /* This value must be overridden by the
> > board. */
> >                         clock-frequency = <0>;
> > -                       status = "disabled";
> >                 };
> > 
> >                 /* External USB clock - can be overridden by the
> > board */
> > @@ -1130,7 +1128,6 @@
> >                         #clock-cells = <0>;
> >                         /* This value must be overridden by the
> > board. */
> >                         clock-frequency = <0>;
> > -                       status = "disabled";
> >                 };
> > 
> >                 /* Special CPG clocks */
> > --
> > 2.8.0.rc3
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linu
> x-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

-- 
Sjoerd Simons
Collabora Ltd.

Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Sergei Shtylyov]

On 04/06/2016 03:52 PM, Sjoerd Simons wrote:

> clk_get on a disabled clock node will return EPROBE_DEFER, which can
> cause drivers to be deferred forever if such clocks are referenced in
> their clocks property.
>
> Update the various disabled external clock nodes to default to a
> frequency of 0, but don't disable them to prevent this.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>
> ---
>
>   arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
>   arch/arm/boot/dts/r8a7791-porter.dts  | 1 +
>   arch/arm/boot/dts/r8a7791.dtsi        | 5 +----
>   3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts
> index 1adf877..da59c28 100644
> --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> @@ -660,6 +660,7 @@
>   };
>
>   &pcie_bus_clk {
> +	clock-frequency = <100000000>;

    Hmmm, looking at the Koelsch schematics, I don't see this clock. :-/

>   	status = "okay";
>   };
>
> diff --git a/arch/arm/boot/dts/r8a7791-porter.dts b/arch/arm/boot/dts/r8a7791-porter.dts
> index 9554d13..19b257e 100644
> --- a/arch/arm/boot/dts/r8a7791-porter.dts
> +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> @@ -413,6 +413,7 @@
>   };
>
>   &pcie_bus_clk {
> +	clock-frequency = <100000000>;
>   	status = "okay";
>   };
>

    Again, looking at the Porter schematics, I don't see this clock either. :-/

> diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
> index 8693888..676df63 100644
> --- a/arch/arm/boot/dts/r8a7791.dtsi
> +++ b/arch/arm/boot/dts/r8a7791.dtsi
> @@ -1104,8 +1104,7 @@
>   		pcie_bus_clk: pcie_bus {
>   			compatible = "fixed-clock";
>   			#clock-cells = <0>;
> -			clock-frequency = <100000000>;
> -			status = "disabled";
> +			clock-frequency = <0>;

    If the clock has a good default frequency, I don't think you need to 
remove it. Otherwise you missed USB_EXTAL which is 48 MHz (and can be overridden).

[...]

MBR, Sergei

Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Sjoerd Simons]

Hey Sergei,

Thanks for your review.

On Thu, 2016-04-07 at 02:15 +0300, Sergei Shtylyov wrote:
> On 04/06/2016 03:52 PM, Sjoerd Simons wrote:
> 
> > 
> > clk_get on a disabled clock node will return EPROBE_DEFER, which
> > can
> > cause drivers to be deferred forever if such clocks are referenced
> > in
> > their clocks property.
> > 
> > Update the various disabled external clock nodes to default to a
> > frequency of 0, but don't disable them to prevent this.
> > 
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > 
> > ---
> > 
> >   arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
> >   arch/arm/boot/dts/r8a7791-porter.dts  | 1 +
> >   arch/arm/boot/dts/r8a7791.dtsi        | 5 +----
> >   3 files changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts
> > b/arch/arm/boot/dts/r8a7791-koelsch.dts
> > index 1adf877..da59c28 100644
> > --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> > +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> > @@ -660,6 +660,7 @@
> >   };
> > 
> >   &pcie_bus_clk {
> > +	clock-frequency = <100000000>;
>     Hmmm, looking at the Koelsch schematics, I don't see this clock.
> :-/

I don't have the schematics so i was simply keeping the current state.
I've added Phil Edworthy to the list as he was the one originally
enable the bus clk for Koelsh according to git. Hopefully he can
clarify :)

> >   	status = "okay";
> >   };
> > 
> > diff --git a/arch/arm/boot/dts/r8a7791-porter.dts
> > b/arch/arm/boot/dts/r8a7791-porter.dts
> > index 9554d13..19b257e 100644
> > --- a/arch/arm/boot/dts/r8a7791-porter.dts
> > +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> > @@ -413,6 +413,7 @@
> >   };
> > 
> >   &pcie_bus_clk {
> > +	clock-frequency = <100000000>;
> >   	status = "okay";
> >   };
> > 
>     Again, looking at the Porter schematics, I don't see this clock
> either. :-/

You were the one enabling this clock for Porter ;) I don't have PCIE
hardware to test with on my porter board, might be worth if you could
disable this clock and see if PCI-E still fucntions as expected (maybe
in practise it just happens to prefer the internal clock?) ?

> > 
> > diff --git a/arch/arm/boot/dts/r8a7791.dtsi
> > b/arch/arm/boot/dts/r8a7791.dtsi
> > index 8693888..676df63 100644
> > --- a/arch/arm/boot/dts/r8a7791.dtsi
> > +++ b/arch/arm/boot/dts/r8a7791.dtsi
> > @@ -1104,8 +1104,7 @@
> >   		pcie_bus_clk: pcie_bus {
> >   			compatible = "fixed-clock";
> >   			#clock-cells = <0>;
> > -			clock-frequency = <100000000>;
> > -			status = "disabled";
> > +			clock-frequency = <0>;
>     If the clock has a good default frequency, I don't think you need
> to 
> remove it. Otherwise you missed USB_EXTAL which is 48 MHz (and can be
> overridden).

I did that as it was by default disabled, so if i do enable it but
don't drop the default frequency to 0 board swithout that clock will
suddenly have it added to their dtb.

For the usb external clock I didn't touch it as it was already enabled
by default with a proper frequency, so it wouldn't hit the issue i was
trying to fix here. But i agree, both looking at other Renesas dtsis
and how all other external clocks are done in this dtsi, this node is
odd.


-- 
Sjoerd Simons
Collabora Ltd.

Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Sergei Shtylyov]

Hello.

On 04/07/2016 10:00 AM, Sjoerd Simons wrote:

> Hey Sergei,
>
> Thanks for your review.

    You're welcome. :-)

> On Thu, 2016-04-07 at 02:15 +0300, Sergei Shtylyov wrote:
>> On 04/06/2016 03:52 PM, Sjoerd Simons wrote:
>>
>>>
>>> clk_get on a disabled clock node will return EPROBE_DEFER, which
>>> can
>>> cause drivers to be deferred forever if such clocks are referenced
>>> in
>>> their clocks property.
>>>
>>> Update the various disabled external clock nodes to default to a
>>> frequency of 0, but don't disable them to prevent this.
>>>
>>> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>>>
>>> ---
>>>
>>>    arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
>>>    arch/arm/boot/dts/r8a7791-porter.dts  | 1 +
>>>    arch/arm/boot/dts/r8a7791.dtsi        | 5 +----
>>>    3 files changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts
>>> b/arch/arm/boot/dts/r8a7791-koelsch.dts
>>> index 1adf877..da59c28 100644
>>> --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
>>> +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
>>> @@ -660,6 +660,7 @@
>>>    };
>>>
>>>    &pcie_bus_clk {
>>> +	clock-frequency = <100000000>;

>>      Hmmm, looking at the Koelsch schematics, I don't see this clock.
>> :-/
>
> I don't have the schematics so i was simply keeping the current state.

    I understand. I was just surprised by what checking the code against the 
schematics revealed.

> I've added Phil Edworthy to the list as he was the one originally

    I should've CC'ed Phil myself...

> enable the bus clk for Koelsh according to git. Hopefully he can
> clarify :)

    Let's hope he'd reply...

>>>    	status = "okay";
>>>    };
>>>
>>> diff --git a/arch/arm/boot/dts/r8a7791-porter.dts
>>> b/arch/arm/boot/dts/r8a7791-porter.dts
>>> index 9554d13..19b257e 100644
>>> --- a/arch/arm/boot/dts/r8a7791-porter.dts
>>> +++ b/arch/arm/boot/dts/r8a7791-porter.dts
>>> @@ -413,6 +413,7 @@
>>>    };
>>>
>>>    &pcie_bus_clk {
>>> +	clock-frequency = <100000000>;
>>>    	status = "okay";
>>>    };
>>>
>>      Again, looking at the Porter schematics, I don't see this clock
>> either. :-/
>
> You were the one enabling this clock for Porter ;) I don't have PCIE

    Yes, of course. I don't remember the gory details already -- I might have 
blindly copied what was in the Koelsch's .dts.

> hardware to test with on my porter board, might be worth if you could
> disable this clock and see if PCI-E still fucntions as expected (maybe
> in practise it just happens to prefer the internal clock?) ?

    I know the PCIe driver does require this clock in order to function -- it 
would be the same eternal -EPROBE_DEFER condition you'd already described;
see drivers/pci/host/pcie-rcar.c yourself...

>>> diff --git a/arch/arm/boot/dts/r8a7791.dtsi
>>> b/arch/arm/boot/dts/r8a7791.dtsi
>>> index 8693888..676df63 100644
>>> --- a/arch/arm/boot/dts/r8a7791.dtsi
>>> +++ b/arch/arm/boot/dts/r8a7791.dtsi
>>> @@ -1104,8 +1104,7 @@
>>>    		pcie_bus_clk: pcie_bus {
>>>    			compatible = "fixed-clock";
>>>    			#clock-cells = <0>;
>>> -			clock-frequency = <100000000>;
>>> -			status = "disabled";
>>> +			clock-frequency = <0>;
>>      If the clock has a good default frequency, I don't think you need
>> to
>> remove it. Otherwise you missed USB_EXTAL which is 48 MHz (and can be
>> overridden).
>
> I did that as it was by default disabled, so if i do enable it but
> don't drop the default frequency to 0 board swithout that clock will
> suddenly have it added to their dtb.

    Zero frequency is hardly any better then the default...

> For the usb external clock I didn't touch it as it was already enabled
> by default with a proper frequency, so it wouldn't hit the issue i was
> trying to fix here. But i agree, both looking at other Renesas dtsis
> and how all other external clocks are done in this dtsi, this node is
> odd.

    It's not that odd given that the R-Car gen2 manual specifically says it 
should be 48 MHz.
    Looking into the manual again, the PCIe bus clock must be the onne 
presented on the CICREF[PN]1_SATA/PCIe input signals and it indeed should be 
100 MHz... So Phil was correct.

MBR, Sergei

Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Stephen Boyd]

On 04/06, Sjoerd Simons wrote:
> On Wed, 2016-04-06 at 15:11 +0200, Geert Uytterhoeven wrote:
> > CC Mike, Stephen, linux-clk (this time with the new Mike)
> > 
> > On Wed, Apr 6, 2016 at 2:52 PM, Sjoerd Simons
> > <sjoerd.simons@collabora.co.uk> wrote:
> > > 
> > > clk_get on a disabled clock node will return EPROBE_DEFER, which
> > > can
> > > cause drivers to be deferred forever if such clocks are referenced
> > > in
> > > their clocks property.
> > Is this a side effect of commit 3e5dd6f6e690048d ("clk: Ignore
> > disabled DT
> > clock providers")?
> 
> Yes it seems so. Reverting that patch means that i can drop this one
> and get the expected behaviour again.

The DT is broken then? Is it possible to mark these status =
"okay" so that things work again?

> 
> Though even so I'm not sure what the convention is for clocks like
> these, the r8a7791.dtsi is inconsistent, as some are disabled while
> others (e.g. the audio clocks) are 0hz. Would be good to get some input
> on that regardless.
> 

What's the question here?

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

Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Sjoerd Simons]

On Thu, 2016-04-07 at 16:21 -0700, Stephen Boyd wrote:
> On 04/06, Sjoerd Simons wrote:
> > 
> > On Wed, 2016-04-06 at 15:11 +0200, Geert Uytterhoeven wrote:
> > > 
> > > CC Mike, Stephen, linux-clk (this time with the new Mike)
> > > 
> > > On Wed, Apr 6, 2016 at 2:52 PM, Sjoerd Simons
> > > <sjoerd.simons@collabora.co.uk> wrote:
> > > > 
> > > > 
> > > > clk_get on a disabled clock node will return EPROBE_DEFER,
> > > > which
> > > > can
> > > > cause drivers to be deferred forever if such clocks are
> > > > referenced
> > > > in
> > > > their clocks property.
> > > Is this a side effect of commit 3e5dd6f6e690048d ("clk: Ignore
> > > disabled DT
> > > clock providers")?
> > Yes it seems so. Reverting that patch means that i can drop this
> > one
> > and get the expected behaviour again.
> The DT is broken then? Is it possible to mark these status =
> "okay" so that things work again?

> > 
> > 
> > Though even so I'm not sure what the convention is for clocks like
> > these, the r8a7791.dtsi is inconsistent, as some are disabled while
> > others (e.g. the audio clocks) are 0hz. Would be good to get some
> > input
> > on that regardless.
> > 
> What's the question here?

So the question is how to model unconnected external clocks in device-
tree.

The dtsi we're loooking at has (in pseudo dt):

device {
  clock-names = "internal", "external";
  clocks = <&internal, &external>
};

external {
  compatible = "fixed-clock";
  clock-frequency = <12345>;
  status = "disabled";
};

Before 3e5dd6f6e690048d ("clk: Ignore disabled DT clock providers")
this apparently worked. Afterwards drivers getting all the clocks would
fail to probe with -EPROBE_DEFER.

Judging by your comment I assume this way of modelling it is broken
(and the behaviour caused by the patch is correct)? 

And as a follow-up, is modelling unconnected clocks as enabled with a
frequency of 0hz as my proposed patch does seen as the right way of
doing things?


-- 
Sjoerd Simons
Collabora Ltd.

RE: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Phil Edworthy]

Hi,

On 07 April 2016 20:14, Sergei Shtylyov wrote:
> On 04/07/2016 10:00 AM, Sjoerd Simons wrote:
> 
> > Hey Sergei,
> >
> > Thanks for your review.
> 
>     You're welcome. :-)
> 
> > On Thu, 2016-04-07 at 02:15 +0300, Sergei Shtylyov wrote:
> >> On 04/06/2016 03:52 PM, Sjoerd Simons wrote:
> >>
> >>>
> >>> clk_get on a disabled clock node will return EPROBE_DEFER, which
> >>> can
> >>> cause drivers to be deferred forever if such clocks are referenced
> >>> in
> >>> their clocks property.
> >>>
> >>> Update the various disabled external clock nodes to default to a
> >>> frequency of 0, but don't disable them to prevent this.
> >>>
> >>> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> >>>
> >>> ---
> >>>
> >>>    arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
> >>>    arch/arm/boot/dts/r8a7791-porter.dts  | 1 +
> >>>    arch/arm/boot/dts/r8a7791.dtsi        | 5 +----
> >>>    3 files changed, 3 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> b/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> index 1adf877..da59c28 100644
> >>> --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> @@ -660,6 +660,7 @@
> >>>    };
> >>>
> >>>    &pcie_bus_clk {
> >>> +	clock-frequency = <100000000>;
> 
> >>      Hmmm, looking at the Koelsch schematics, I don't see this clock.
> >> :-/
> >
> > I don't have the schematics so i was simply keeping the current state.
> 
>     I understand. I was just surprised by what checking the code against the
> schematics revealed.
> 
> > I've added Phil Edworthy to the list as he was the one originally
> 
>     I should've CC'ed Phil myself...
>
> > enable the bus clk for Koelsh according to git. Hopefully he can
> > clarify :)
> 
>     Let's hope he'd reply...
> 
> >>>    	status = "okay";
> >>>    };
> >>>
> >>> diff --git a/arch/arm/boot/dts/r8a7791-porter.dts
> >>> b/arch/arm/boot/dts/r8a7791-porter.dts
> >>> index 9554d13..19b257e 100644
> >>> --- a/arch/arm/boot/dts/r8a7791-porter.dts
> >>> +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> >>> @@ -413,6 +413,7 @@
> >>>    };
> >>>
> >>>    &pcie_bus_clk {
> >>> +	clock-frequency = <100000000>;
> >>>    	status = "okay";
> >>>    };
> >>>
> >>      Again, looking at the Porter schematics, I don't see this clock
> >> either. :-/
> >
> > You were the one enabling this clock for Porter ;) I don't have PCIE
> 
>     Yes, of course. I don't remember the gory details already -- I might have
> blindly copied what was in the Koelsch's .dts.
> 
> > hardware to test with on my porter board, might be worth if you could
> > disable this clock and see if PCI-E still fucntions as expected (maybe
> > in practise it just happens to prefer the internal clock?) ?
> 
>     I know the PCIe driver does require this clock in order to function -- it
> would be the same eternal -EPROBE_DEFER condition you'd already described;
> see drivers/pci/host/pcie-rcar.c yourself...
> 
> >>> diff --git a/arch/arm/boot/dts/r8a7791.dtsi
> >>> b/arch/arm/boot/dts/r8a7791.dtsi
> >>> index 8693888..676df63 100644
> >>> --- a/arch/arm/boot/dts/r8a7791.dtsi
> >>> +++ b/arch/arm/boot/dts/r8a7791.dtsi
> >>> @@ -1104,8 +1104,7 @@
> >>>    		pcie_bus_clk: pcie_bus {
> >>>    			compatible = "fixed-clock";
> >>>    			#clock-cells = <0>;
> >>> -			clock-frequency = <100000000>;
> >>> -			status = "disabled";
> >>> +			clock-frequency = <0>;
> >>      If the clock has a good default frequency, I don't think you need
> >> to
> >> remove it. Otherwise you missed USB_EXTAL which is 48 MHz (and can be
> >> overridden).
> >
> > I did that as it was by default disabled, so if i do enable it but
> > don't drop the default frequency to 0 board swithout that clock will
> > suddenly have it added to their dtb.
> 
>     Zero frequency is hardly any better then the default...
> 
> > For the usb external clock I didn't touch it as it was already enabled
> > by default with a proper frequency, so it wouldn't hit the issue i was
> > trying to fix here. But i agree, both looking at other Renesas dtsis
> > and how all other external clocks are done in this dtsi, this node is
> > odd.
> 
>     It's not that odd given that the R-Car gen2 manual specifically says it
> should be 48 MHz.
>     Looking into the manual again, the PCIe bus clock must be the onne
> presented on the CICREF[PN]1_SATA/PCIe input signals and it indeed should be
> 100 MHz... So Phil was correct.

PCIe always requires a 100MHz reference clock. On Koelsch, this clock is
generated by U6 and always on. Some boards may need additional hardware
to be setup in order to output the clock, hence the DT node for the clock.

I hope that clarifies the situation!
Phil

Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Stephen Boyd]

On 04/08, Sjoerd Simons wrote:
> On Thu, 2016-04-07 at 16:21 -0700, Stephen Boyd wrote:
> > On 04/06, Sjoerd Simons wrote:
> > > 
> > > Though even so I'm not sure what the convention is for clocks like
> > > these, the r8a7791.dtsi is inconsistent, as some are disabled while
> > > others (e.g. the audio clocks) are 0hz. Would be good to get some
> > > input
> > > on that regardless.
> > > 
> > What's the question here?
> 
> So the question is how to model unconnected external clocks in device-
> tree.
> 
> The dtsi we're loooking at has (in pseudo dt):
> 
> device {
>   clock-names = "internal", "external";
>   clocks = <&internal, &external>
> };
> 
> external {
>   compatible = "fixed-clock";
>   clock-frequency = <12345>;
>   status = "disabled";
> };
> 
> Before 3e5dd6f6e690048d ("clk: Ignore disabled DT clock providers")
> this apparently worked. Afterwards drivers getting all the clocks would
> fail to probe with -EPROBE_DEFER.
> 
> Judging by your comment I assume this way of modelling it is broken
> (and the behaviour caused by the patch is correct)? 
> 
> And as a follow-up, is modelling unconnected clocks as enabled with a
> frequency of 0hz as my proposed patch does seen as the right way of
> doing things?
> 

Right. In the case where the external clk is populated, I imagine
there would be a DT node describing it with the clock-frequency
property if it's a fixed rate clk. If the clk is not populated on
the board, then we would have a "ground" clk node that has a
frequency of 0. This way, if we have some mux clk that is default
connected to the external clk but can switch to some internal clk
it isn't orphaned forever. This is actually a problem for orphan
clk deferral right now.

My head starts to spin when we consider something like expansion
boards that have clk pins on them though. Hopefully for things
like that, we can populate clks with DT overlays and then change
the root hierarchy of the clk tree by swapping out the "ground"
clk for some real clk on the expansion board. The usage of
strings to describe the clk tree is probably going to get us here
though. Fun!

If we can't do this DT design because of backwards compatibility
concerns, then perhaps we need to expand the core to return a
fixed rate of 0 clk whenever clk_get() is called on a provider
that's status = "disabled"? Here's an untested patch to show that
idea.

---8<----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fb74dc1f7520..19c3777b1cea 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3085,6 +3085,17 @@ int of_clk_parent_fill(struct device_node *np, const char **parents,
 }
 EXPORT_SYMBOL_GPL(of_clk_parent_fill);
 
+static void init_disabled_clk_provider(struct device_node *np)
+{
+	struct clk *clk;
+
+	clk = clk_register_fixed_rate(NULL, of_node_full_name(np), NULL, 0, 0);
+	if (IS_ERR(clk))
+		return;
+
+	of_clk_add_provider(np, of_clk_src_simple_get, clk);
+}
+
 struct clock_provider {
 	of_clk_init_cb_t clk_init_cb;
 	struct device_node *np;
@@ -3150,9 +3161,6 @@ void __init of_clk_init(const struct of_device_id *matches)
 	for_each_matching_node_and_match(np, matches, &match) {
 		struct clock_provider *parent;
 
-		if (!of_device_is_available(np))
-			continue;
-
 		parent = kzalloc(sizeof(*parent), GFP_KERNEL);
 		if (!parent) {
 			list_for_each_entry_safe(clk_provider, next,
@@ -3165,7 +3173,18 @@ void __init of_clk_init(const struct of_device_id *matches)
 			return;
 		}
 
-		parent->clk_init_cb = match->data;
+		/*
+		 * Sometimes DT nodes are status = "disabled" but they're used
+		 * by other clk providers. In that case we make the disabled
+		 * provider return fixed rate clks with a frequency of 0 so
+		 * that nothing is orphaned and drivers can still get all
+		 * their clks.
+		 */
+		if (!of_device_is_available(np)) {
+			parent->clk_init_cb = init_disabled_clk_provider;
+		} else {
+			parent->clk_init_cb = match->data;
+		}
 		parent->np = of_node_get(np);
 		list_add_tail(&parent->node, &clk_provider_list);
 	}
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Geert Uytterhoeven]

On Wed, Apr 6, 2016 at 2:52 PM, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> clk_get on a disabled clock node will return EPROBE_DEFER, which can
> cause drivers to be deferred forever if such clocks are referenced in
> their clocks property.
>
> Update the various disabled external clock nodes to default to a
> frequency of 0, but don't disable them to prevent this.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Simon Horman]

On Tue, Apr 19, 2016 at 09:18:55AM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 6, 2016 at 2:52 PM, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> > clk_get on a disabled clock node will return EPROBE_DEFER, which can
> > cause drivers to be deferred forever if such clocks are referenced in
> > their clocks property.
> >
> > Update the various disabled external clock nodes to default to a
> > frequency of 0, but don't disable them to prevent this.
> >
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks, I will add your tag.