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

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]

T24gV2VkLCAyMDE2LTA0LTA2IGF0IDE1OjExICswMjAwLCBHZWVydCBVeXR0ZXJob2V2ZW4gd3Jv
dGU6Cj4gQ0MgTWlrZSwgU3RlcGhlbiwgbGludXgtY2xrICh0aGlzIHRpbWUgd2l0aCB0aGUgbmV3
IE1pa2UpCj4gCj4gT24gV2VkLCBBcHIgNiwgMjAxNiBhdCAyOjUyIFBNLCBTam9lcmQgU2ltb25z
Cj4gPHNqb2VyZC5zaW1vbnNAY29sbGFib3JhLmNvLnVrPiB3cm90ZToKPiA+IAo+ID4gY2xrX2dl
dCBvbiBhIGRpc2FibGVkIGNsb2NrIG5vZGUgd2lsbCByZXR1cm4gRVBST0JFX0RFRkVSLCB3aGlj
aAo+ID4gY2FuCj4gPiBjYXVzZSBkcml2ZXJzIHRvIGJlIGRlZmVycmVkIGZvcmV2ZXIgaWYgc3Vj
aCBjbG9ja3MgYXJlIHJlZmVyZW5jZWQKPiA+IGluCj4gPiB0aGVpciBjbG9ja3MgcHJvcGVydHku
Cj4gSXMgdGhpcyBhIHNpZGUgZWZmZWN0IG9mIGNvbW1pdCAzZTVkZDZmNmU2OTAwNDhkICgiY2xr
OiBJZ25vcmUKPiBkaXNhYmxlZCBEVAo+IGNsb2NrIHByb3ZpZGVycyIpPwoKWWVzIGl0IHNlZW1z
IHNvLiBSZXZlcnRpbmcgdGhhdCBwYXRjaCBtZWFucyB0aGF0IGkgY2FuIGRyb3AgdGhpcyBvbmUK
YW5kIGdldCB0aGUgZXhwZWN0ZWQgYmVoYXZpb3VyIGFnYWluLgoKVGhvdWdoIGV2ZW4gc28gSSdt
IG5vdCBzdXJlIHdoYXQgdGhlIGNvbnZlbnRpb24gaXMgZm9yIGNsb2NrcyBsaWtlCnRoZXNlLCB0
aGUgcjhhNzc5MS5kdHNpIGlzIGluY29uc2lzdGVudCwgYXMgc29tZSBhcmUgZGlzYWJsZWQgd2hp
bGUKb3RoZXJzIChlLmcuIHRoZSBhdWRpbyBjbG9ja3MpIGFyZSAwaHouIFdvdWxkIGJlIGdvb2Qg
dG8gZ2V0IHNvbWUgaW5wdXQKb24gdGhhdCByZWdhcmRsZXNzLgoKPiA+IAo+ID4gVXBkYXRlIHRo
ZSB2YXJpb3VzIGRpc2FibGVkIGV4dGVybmFsIGNsb2NrIG5vZGVzIHRvIGRlZmF1bHQgdG8gYQo+
ID4gZnJlcXVlbmN5IG9mIDAsIGJ1dCBkb24ndCBkaXNhYmxlIHRoZW0gdG8gcHJldmVudMKgdGhp
cy4KPiA+IAo+ID4gU2lnbmVkLW9mZi1ieTogU2pvZXJkIFNpbW9ucyA8c2pvZXJkLnNpbW9uc0Bj
b2xsYWJvcmEuY28udWs+Cj4gPiAKPiA+IC0tLQo+ID4gCj4gPiDCoGFyY2gvYXJtL2Jvb3QvZHRz
L3I4YTc3OTEta29lbHNjaC5kdHMgfCAxICsKPiA+IMKgYXJjaC9hcm0vYm9vdC9kdHMvcjhhNzc5
MS1wb3J0ZXIuZHRzwqDCoHwgMSArCj4gPiDCoGFyY2gvYXJtL2Jvb3QvZHRzL3I4YTc3OTEuZHRz
acKgwqDCoMKgwqDCoMKgwqB8IDUgKy0tLS0KPiA+IMKgMyBmaWxlcyBjaGFuZ2VkLCAzIGluc2Vy
dGlvbnMoKyksIDQgZGVsZXRpb25zKC0pCj4gPiAKPiA+IGRpZmYgLS1naXQgYS9hcmNoL2FybS9i
b290L2R0cy9yOGE3NzkxLWtvZWxzY2guZHRzCj4gPiBiL2FyY2gvYXJtL2Jvb3QvZHRzL3I4YTc3
OTEta29lbHNjaC5kdHMKPiA+IGluZGV4IDFhZGY4NzcuLmRhNTljMjggMTAwNjQ0Cj4gPiAtLS0g
YS9hcmNoL2FybS9ib290L2R0cy9yOGE3NzkxLWtvZWxzY2guZHRzCj4gPiArKysgYi9hcmNoL2Fy
bS9ib290L2R0cy9yOGE3NzkxLWtvZWxzY2guZHRzCj4gPiBAQCAtNjYwLDYgKzY2MCw3IEBACj4g
PiDCoH07Cj4gPiAKPiA+IMKgJnBjaWVfYnVzX2NsayB7Cj4gPiArwqDCoMKgwqDCoMKgwqBjbG9j
ay1mcmVxdWVuY3kgPSA8MTAwMDAwMDAwPjsKPiA+IMKgwqDCoMKgwqDCoMKgwqBzdGF0dXMgPSAi
b2theSI7Cj4gPiDCoH07Cj4gPiAKPiA+IGRpZmYgLS1naXQgYS9hcmNoL2FybS9ib290L2R0cy9y
OGE3NzkxLXBvcnRlci5kdHMKPiA+IGIvYXJjaC9hcm0vYm9vdC9kdHMvcjhhNzc5MS1wb3J0ZXIu
ZHRzCj4gPiBpbmRleCA5NTU0ZDEzLi4xOWIyNTdlIDEwMDY0NAo+ID4gLS0tIGEvYXJjaC9hcm0v
Ym9vdC9kdHMvcjhhNzc5MS1wb3J0ZXIuZHRzCj4gPiArKysgYi9hcmNoL2FybS9ib290L2R0cy9y
OGE3NzkxLXBvcnRlci5kdHMKPiA+IEBAIC00MTMsNiArNDEzLDcgQEAKPiA+IMKgfTsKPiA+IAo+
ID4gwqAmcGNpZV9idXNfY2xrIHsKPiA+ICvCoMKgwqDCoMKgwqDCoGNsb2NrLWZyZXF1ZW5jeSA9
IDwxMDAwMDAwMDA+Owo+ID4gwqDCoMKgwqDCoMKgwqDCoHN0YXR1cyA9ICJva2F5IjsKPiA+IMKg
fTsKPiA+IAo+ID4gZGlmZiAtLWdpdCBhL2FyY2gvYXJtL2Jvb3QvZHRzL3I4YTc3OTEuZHRzaQo+
ID4gYi9hcmNoL2FybS9ib290L2R0cy9yOGE3NzkxLmR0c2kKPiA+IGluZGV4IDg2OTM4ODguLjY3
NmRmNjMgMTAwNjQ0Cj4gPiAtLS0gYS9hcmNoL2FybS9ib290L2R0cy9yOGE3NzkxLmR0c2kKPiA+
ICsrKyBiL2FyY2gvYXJtL2Jvb3QvZHRzL3I4YTc3OTEuZHRzaQo+ID4gQEAgLTExMDQsOCArMTEw
NCw3IEBACj4gPiDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHBjaWVfYnVzX2Nsazog
cGNpZV9idXMgewo+ID4gwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgY29tcGF0aWJsZSA9ICJmaXhlZC1jbG9jayI7Cj4gPiDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAjY2xvY2stY2VsbHMgPSA8MD47Cj4gPiAtwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoGNsb2NrLWZyZXF1ZW5j
eSA9IDwxMDAwMDAwMDA+Owo+ID4gLcKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqBzdGF0dXMgPSAiZGlzYWJsZWQiOwo+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqBjbG9jay1mcmVxdWVuY3kgPSA8MD47Cj4gPiDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoH07Cj4gPiAKPiA+IMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgLyogRXh0ZXJuYWwgU0NJRiBjbG9jayAqLwo+ID4gQEAgLTExMTQsNyAr
MTExMyw2IEBACj4gPiDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqAjY2xvY2stY2VsbHMgPSA8MD47Cj4gPiDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqAvKiBUaGlzIHZhbHVlIG11c3QgYmUgb3ZlcnJpZGRlbiBieSB0
aGUKPiA+IGJvYXJkLiAqLwo+ID4gwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgY2xvY2stZnJlcXVlbmN5ID0gPDA+Owo+ID4gLcKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqBzdGF0dXMgPSAiZGlzYWJsZWQiOwo+ID4gwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqB9Owo+ID4gCj4gPiDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoC8qIEV4dGVybmFsIFVTQiBjbG9jayAtIGNhbiBiZSBvdmVycmlkZGVu
IGJ5IHRoZQo+ID4gYm9hcmQgKi8KPiA+IEBAIC0xMTMwLDcgKzExMjgsNiBAQAo+ID4gwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgI2Nsb2NrLWNlbGxzID0g
PDA+Owo+ID4gwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
LyogVGhpcyB2YWx1ZSBtdXN0IGJlIG92ZXJyaWRkZW4gYnkgdGhlCj4gPiBib2FyZC4gKi8KPiA+
IMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoGNsb2NrLWZy
ZXF1ZW5jeSA9IDwwPjsKPiA+IC3CoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgc3RhdHVzID0gImRpc2FibGVkIjsKPiA+IMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgfTsKPiA+IAo+ID4gwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAvKiBT
cGVjaWFsIENQRyBjbG9ja3MgKi8KPiA+IC0tCj4gPiAyLjguMC5yYzMKPiBHcntvZXRqZSxlZXRp
bmd9cywKPiAKPiDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqBHZWVydAo+IAo+IC0tCj4gR2VlcnQgVXl0dGVyaG9ldmVuIC0tIFRoZXJlJ3MgbG90cyBvZiBM
aW51eCBiZXlvbmQgaWEzMiAtLSBnZWVydEBsaW51Cj4geC1tNjhrLm9yZwo+IAo+IEluIHBlcnNv
bmFsIGNvbnZlcnNhdGlvbnMgd2l0aCB0ZWNobmljYWwgcGVvcGxlLCBJIGNhbGwgbXlzZWxmIGEK
PiBoYWNrZXIuIEJ1dAo+IHdoZW4gSSdtIHRhbGtpbmcgdG8gam91cm5hbGlzdHMgSSBqdXN0IHNh
eSAicHJvZ3JhbW1lciIgb3Igc29tZXRoaW5nCj4gbGlrZSB0aGF0Lgo+IMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAtLSBMaW51
cyBUb3J2YWxkcwoKLS0gClNqb2VyZCBTaW1vbnMKQ29sbGFib3JhIEx0ZC4K

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:
> >=20
> > On Wed, 2016-04-06 at 15:11 +0200, Geert Uytterhoeven wrote:
> > >=20
> > > CC Mike, Stephen, linux-clk (this time with the new Mike)
> > >=20
> > > On Wed, Apr 6, 2016 at 2:52 PM, Sjoerd Simons
> > > <sjoerd.simons@collabora.co.uk> wrote:
> > > >=20
> > > >=20
> > > > 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 =3D
> "okay" so that things work again?

> >=20
> >=20
> > 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.
> >=20
> 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 {
=C2=A0 clock-names =3D "internal", "external";
=C2=A0 clocks =3D <&internal, &external>
};

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

Before=C2=A03e5dd6f6e690048d ("clk: Ignore=C2=A0disabled DT=C2=A0clock prov=
iders")
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)?=C2=A0

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?


--=20
Sjoerd Simons
Collabora Ltd.

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