* Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks [not found] <1459947173-6664-1-git-send-email-sjoerd.simons@collabora.co.uk> @ 2016-04-06 13:09 ` Geert Uytterhoeven 2016-04-06 13:11 ` Geert Uytterhoeven 1 sibling, 0 replies; 6+ messages in thread From: Geert Uytterhoeven @ 2016-04-06 13:09 UTC (permalink / raw) To: Sjoerd Simons Cc: Simon Horman, linux-renesas-soc, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Mike Turquette, Stephen Boyd, linux-clk 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks [not found] <1459947173-6664-1-git-send-email-sjoerd.simons@collabora.co.uk> 2016-04-06 13:09 ` [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks Geert Uytterhoeven @ 2016-04-06 13:11 ` Geert Uytterhoeven 2016-04-06 13:37 ` Sjoerd Simons 1 sibling, 1 reply; 6+ messages in thread From: Geert Uytterhoeven @ 2016-04-06 13:11 UTC (permalink / raw) To: Sjoerd Simons Cc: Simon Horman, linux-renesas-soc, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Michael Turquette, Stephen Boyd, linux-clk 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks 2016-04-06 13:11 ` Geert Uytterhoeven @ 2016-04-06 13:37 ` Sjoerd Simons 2016-04-07 23:21 ` Stephen Boyd 0 siblings, 1 reply; 6+ messages in thread From: Sjoerd Simons @ 2016-04-06 13:37 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Simon Horman, linux-renesas-soc, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Michael Turquette, Stephen Boyd, linux-clk 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks 2016-04-06 13:37 ` Sjoerd Simons @ 2016-04-07 23:21 ` Stephen Boyd 2016-04-08 10:50 ` Sjoerd Simons 0 siblings, 1 reply; 6+ messages in thread From: Stephen Boyd @ 2016-04-07 23:21 UTC (permalink / raw) To: Sjoerd Simons Cc: Geert Uytterhoeven, Simon Horman, linux-renesas-soc, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Michael Turquette, linux-clk 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks 2016-04-07 23:21 ` Stephen Boyd @ 2016-04-08 10:50 ` Sjoerd Simons 2016-04-14 0:19 ` Stephen Boyd 0 siblings, 1 reply; 6+ messages in thread From: Sjoerd Simons @ 2016-04-08 10:50 UTC (permalink / raw) To: Stephen Boyd Cc: Geert Uytterhoeven, Simon Horman, linux-renesas-soc, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Michael Turquette, linux-clk 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks 2016-04-08 10:50 ` Sjoerd Simons @ 2016-04-14 0:19 ` Stephen Boyd 0 siblings, 0 replies; 6+ messages in thread From: Stephen Boyd @ 2016-04-14 0:19 UTC (permalink / raw) To: Sjoerd Simons Cc: Geert Uytterhoeven, Simon Horman, linux-renesas-soc, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Michael Turquette, linux-clk 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-04-14 0:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1459947173-6664-1-git-send-email-sjoerd.simons@collabora.co.uk>
2016-04-06 13:09 ` [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks Geert Uytterhoeven
2016-04-06 13:11 ` Geert Uytterhoeven
2016-04-06 13:37 ` Sjoerd Simons
2016-04-07 23:21 ` Stephen Boyd
2016-04-08 10:50 ` Sjoerd Simons
2016-04-14 0:19 ` Stephen Boyd
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).