From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 30 Jun 2016 16:18:49 +0100 From: Mark Rutland To: Dirk Behme Cc: Dirk Behme , devicetree@vger.kernel.org, Stefano Stabellini , Michael Turquette , Stephen Boyd , Julien Grall , xen-devel@lists.xenproject.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor Message-ID: <20160630151523.GA29700@leverpostej> References: <1467282752-14053-1-git-send-email-dirk.behme@de.bosch.com> <20160630142136.GE20363@leverpostej> <57753328.9060300@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <57753328.9060300@gmail.com> List-ID: Hi, On Thu, Jun 30, 2016 at 04:56:40PM +0200, Dirk Behme wrote: > On 30.06.2016 16:21, Mark Rutland wrote: > >On Thu, Jun 30, 2016 at 12:32:32PM +0200, Dirk Behme wrote: > >>+- clocks: one or more clocks to be registered. > >>+ Xen hypervisor drivers might replace native drivers, resulting in > >>+ clocks not registered by these native drivers. To avoid that these > >>+ unregistered clocks are disabled, then, e.g. by clk_disable_unused(), > >>+ register them in the hypervisor node. > >>+ An example for this are the clocks of the serial driver. If the clocks > >>+ used by the serial hardware interface are not registered by the serial > >>+ driver the serial output might stop once clk_disable_unused() is called. > > > >This shouldn't be described in terms of the Linux implementation details > >like clk_disable_unused. Those can change at any time, and don't define > >the contract as such. > > Ok. I remove that from the description. Thanks! Great, thanks. > >What exactly is the contract here? I assume that you don't want the > >kernel to alter the state of these clocks in any way, > > I don't think so. I think what we want is that the kernel just don't > disable the (from kernel's point of view) "unused" clocks. I think > we get this from the fact that using 'clk_ignore_unused' is a > working workaround for this issue. What if the clock (or a parent thereof) is shared with another device? Surely you don't want that other driver to change the rate (changing the rate of the UART behind Xen's back)? I appreciate that clk_ignore_unused might work on platforms today, but that relies on the behaviour of drivers, which might change. It may also not work on other platforms in future, in cases like the above. > >e.g. the rate cannot be changed, they must be left always on, parent > >clocks cannot be altered if that would result in momentary jitter. > > > >I suspect it may be necessary to do more to ensure that, but I'm not > >familiar enough with the clocks subsystem to say. > > > >Are we likely to need to care about regulators, GPIOs, resets, etc? I > >worry that this may be the tip of hte iceberg > > I don't think so. If there is a user in the kernel of the clock, > fine. Then hopefully the user in the kernel knows what he is doing > with the clock and then he should do what ever he wants. I'm not sure that's true. A user of the clock may know nothing about other users. As far as I can see, nothing prevents it from changing the clock rate. While drivers in the same kernel can co-ordinate to avoid problems such as that, we can't do that if we know nothing about the user hidden by Xen. >>From looking at the Xen bug tracker, it's not clear which platforms/devices this is necessary for, so it's still not clear to me if we need to care about regulators, GPIOs, resets, and so on. Do we know which platforms in particular we need this for? > As there is a user in the kernel, we don't have to care about > 'accidentally' disabling it. > > Just let us care about the clocks there is no user. As above, I don't believe that alone is sufficient in general, though it may happen to be for a particular platform. Without information regarding the affected platform(s), it's rather difficult to tell. Thanks, Mark. From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.rutland@arm.com (Mark Rutland) Date: Thu, 30 Jun 2016 16:18:49 +0100 Subject: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor In-Reply-To: <57753328.9060300@gmail.com> References: <1467282752-14053-1-git-send-email-dirk.behme@de.bosch.com> <20160630142136.GE20363@leverpostej> <57753328.9060300@gmail.com> Message-ID: <20160630151523.GA29700@leverpostej> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi, On Thu, Jun 30, 2016 at 04:56:40PM +0200, Dirk Behme wrote: > On 30.06.2016 16:21, Mark Rutland wrote: > >On Thu, Jun 30, 2016 at 12:32:32PM +0200, Dirk Behme wrote: > >>+- clocks: one or more clocks to be registered. > >>+ Xen hypervisor drivers might replace native drivers, resulting in > >>+ clocks not registered by these native drivers. To avoid that these > >>+ unregistered clocks are disabled, then, e.g. by clk_disable_unused(), > >>+ register them in the hypervisor node. > >>+ An example for this are the clocks of the serial driver. If the clocks > >>+ used by the serial hardware interface are not registered by the serial > >>+ driver the serial output might stop once clk_disable_unused() is called. > > > >This shouldn't be described in terms of the Linux implementation details > >like clk_disable_unused. Those can change at any time, and don't define > >the contract as such. > > Ok. I remove that from the description. Thanks! Great, thanks. > >What exactly is the contract here? I assume that you don't want the > >kernel to alter the state of these clocks in any way, > > I don't think so. I think what we want is that the kernel just don't > disable the (from kernel's point of view) "unused" clocks. I think > we get this from the fact that using 'clk_ignore_unused' is a > working workaround for this issue. What if the clock (or a parent thereof) is shared with another device? Surely you don't want that other driver to change the rate (changing the rate of the UART behind Xen's back)? I appreciate that clk_ignore_unused might work on platforms today, but that relies on the behaviour of drivers, which might change. It may also not work on other platforms in future, in cases like the above. > >e.g. the rate cannot be changed, they must be left always on, parent > >clocks cannot be altered if that would result in momentary jitter. > > > >I suspect it may be necessary to do more to ensure that, but I'm not > >familiar enough with the clocks subsystem to say. > > > >Are we likely to need to care about regulators, GPIOs, resets, etc? I > >worry that this may be the tip of hte iceberg > > I don't think so. If there is a user in the kernel of the clock, > fine. Then hopefully the user in the kernel knows what he is doing > with the clock and then he should do what ever he wants. I'm not sure that's true. A user of the clock may know nothing about other users. As far as I can see, nothing prevents it from changing the clock rate. While drivers in the same kernel can co-ordinate to avoid problems such as that, we can't do that if we know nothing about the user hidden by Xen. >>From looking at the Xen bug tracker, it's not clear which platforms/devices this is necessary for, so it's still not clear to me if we need to care about regulators, GPIOs, resets, and so on. Do we know which platforms in particular we need this for? > As there is a user in the kernel, we don't have to care about > 'accidentally' disabling it. > > Just let us care about the clocks there is no user. As above, I don't believe that alone is sufficient in general, though it may happen to be for a particular platform. Without information regarding the affected platform(s), it's rather difficult to tell. Thanks, Mark. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Rutland Subject: Re: [PATCH v2] xen/arm: register clocks used by the hypervisor Date: Thu, 30 Jun 2016 16:18:49 +0100 Message-ID: <20160630151523.GA29700@leverpostej> References: <1467282752-14053-1-git-send-email-dirk.behme@de.bosch.com> <20160630142136.GE20363@leverpostej> <57753328.9060300@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <57753328.9060300@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Dirk Behme Cc: devicetree@vger.kernel.org, Dirk Behme , Michael Turquette , Stephen Boyd , Julien Grall , Stefano Stabellini , xen-devel@lists.xenproject.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org SGksCgpPbiBUaHUsIEp1biAzMCwgMjAxNiBhdCAwNDo1Njo0MFBNICswMjAwLCBEaXJrIEJlaG1l IHdyb3RlOgo+IE9uIDMwLjA2LjIwMTYgMTY6MjEsIE1hcmsgUnV0bGFuZCB3cm90ZToKPiA+T24g VGh1LCBKdW4gMzAsIDIwMTYgYXQgMTI6MzI6MzJQTSArMDIwMCwgRGlyayBCZWhtZSB3cm90ZToK PiA+PistIGNsb2Nrczogb25lIG9yIG1vcmUgY2xvY2tzIHRvIGJlIHJlZ2lzdGVyZWQuCj4gPj4r ICBYZW4gaHlwZXJ2aXNvciBkcml2ZXJzIG1pZ2h0IHJlcGxhY2UgbmF0aXZlIGRyaXZlcnMsIHJl c3VsdGluZyBpbgo+ID4+KyAgY2xvY2tzIG5vdCByZWdpc3RlcmVkIGJ5IHRoZXNlIG5hdGl2ZSBk cml2ZXJzLiBUbyBhdm9pZCB0aGF0IHRoZXNlCj4gPj4rICB1bnJlZ2lzdGVyZWQgY2xvY2tzIGFy ZSBkaXNhYmxlZCwgdGhlbiwgZS5nLiBieSBjbGtfZGlzYWJsZV91bnVzZWQoKSwKPiA+PisgIHJl Z2lzdGVyIHRoZW0gaW4gdGhlIGh5cGVydmlzb3Igbm9kZS4KPiA+PisgIEFuIGV4YW1wbGUgZm9y IHRoaXMgYXJlIHRoZSBjbG9ja3Mgb2YgdGhlIHNlcmlhbCBkcml2ZXIuIElmIHRoZSBjbG9ja3MK PiA+PisgIHVzZWQgYnkgdGhlIHNlcmlhbCBoYXJkd2FyZSBpbnRlcmZhY2UgYXJlIG5vdCByZWdp c3RlcmVkIGJ5IHRoZSBzZXJpYWwKPiA+PisgIGRyaXZlciB0aGUgc2VyaWFsIG91dHB1dCBtaWdo dCBzdG9wIG9uY2UgY2xrX2Rpc2FibGVfdW51c2VkKCkgaXMgY2FsbGVkLgo+ID4KPiA+VGhpcyBz aG91bGRuJ3QgYmUgZGVzY3JpYmVkIGluIHRlcm1zIG9mIHRoZSBMaW51eCBpbXBsZW1lbnRhdGlv biBkZXRhaWxzCj4gPmxpa2UgY2xrX2Rpc2FibGVfdW51c2VkLiBUaG9zZSBjYW4gY2hhbmdlIGF0 IGFueSB0aW1lLCBhbmQgZG9uJ3QgZGVmaW5lCj4gPnRoZSBjb250cmFjdCBhcyBzdWNoLgo+IAo+ IE9rLiBJIHJlbW92ZSB0aGF0IGZyb20gdGhlIGRlc2NyaXB0aW9uLiBUaGFua3MhCgpHcmVhdCwg dGhhbmtzLgoKPiA+V2hhdCBleGFjdGx5IGlzIHRoZSBjb250cmFjdCBoZXJlPyBJIGFzc3VtZSB0 aGF0IHlvdSBkb24ndCB3YW50IHRoZQo+ID5rZXJuZWwgdG8gYWx0ZXIgdGhlIHN0YXRlIG9mIHRo ZXNlIGNsb2NrcyBpbiBhbnkgd2F5LAo+IAo+IEkgZG9uJ3QgdGhpbmsgc28uIEkgdGhpbmsgd2hh dCB3ZSB3YW50IGlzIHRoYXQgdGhlIGtlcm5lbCBqdXN0IGRvbid0Cj4gZGlzYWJsZSB0aGUgKGZy b20ga2VybmVsJ3MgcG9pbnQgb2YgdmlldykgInVudXNlZCIgY2xvY2tzLiBJIHRoaW5rCj4gd2Ug Z2V0IHRoaXMgZnJvbSB0aGUgZmFjdCB0aGF0IHVzaW5nICdjbGtfaWdub3JlX3VudXNlZCcgaXMg YQo+IHdvcmtpbmcgd29ya2Fyb3VuZCBmb3IgdGhpcyBpc3N1ZS4KCldoYXQgaWYgdGhlIGNsb2Nr IChvciBhIHBhcmVudCB0aGVyZW9mKSBpcyBzaGFyZWQgd2l0aCBhbm90aGVyIGRldmljZT8KClN1 cmVseSB5b3UgZG9uJ3Qgd2FudCB0aGF0IG90aGVyIGRyaXZlciB0byBjaGFuZ2UgdGhlIHJhdGUg KGNoYW5naW5nIHRoZQpyYXRlIG9mIHRoZSBVQVJUIGJlaGluZCBYZW4ncyBiYWNrKT8KCkkgYXBw cmVjaWF0ZSB0aGF0IGNsa19pZ25vcmVfdW51c2VkIG1pZ2h0IHdvcmsgb24gcGxhdGZvcm1zIHRv ZGF5LCBidXQKdGhhdCByZWxpZXMgb24gdGhlIGJlaGF2aW91ciBvZiBkcml2ZXJzLCB3aGljaCBt aWdodCBjaGFuZ2UuIEl0IG1heSBhbHNvCm5vdCB3b3JrIG9uIG90aGVyIHBsYXRmb3JtcyBpbiBm dXR1cmUsIGluIGNhc2VzIGxpa2UgdGhlIGFib3ZlLgoKPiA+ZS5nLiB0aGUgcmF0ZSBjYW5ub3Qg YmUgY2hhbmdlZCwgdGhleSBtdXN0IGJlIGxlZnQgYWx3YXlzIG9uLCBwYXJlbnQKPiA+Y2xvY2tz IGNhbm5vdCBiZSBhbHRlcmVkIGlmIHRoYXQgd291bGQgcmVzdWx0IGluIG1vbWVudGFyeSBqaXR0 ZXIuCj4gPgo+ID5JIHN1c3BlY3QgaXQgbWF5IGJlIG5lY2Vzc2FyeSB0byBkbyBtb3JlIHRvIGVu c3VyZSB0aGF0LCBidXQgSSdtIG5vdAo+ID5mYW1pbGlhciBlbm91Z2ggd2l0aCB0aGUgY2xvY2tz IHN1YnN5c3RlbSB0byBzYXkuCj4gPgo+ID5BcmUgd2UgbGlrZWx5IHRvIG5lZWQgdG8gY2FyZSBh Ym91dCByZWd1bGF0b3JzLCBHUElPcywgcmVzZXRzLCBldGM/IEkKPiA+d29ycnkgdGhhdCB0aGlz IG1heSBiZSB0aGUgdGlwIG9mIGh0ZSBpY2ViZXJnCj4gCj4gSSBkb24ndCB0aGluayBzby4gSWYg dGhlcmUgaXMgYSB1c2VyIGluIHRoZSBrZXJuZWwgb2YgdGhlIGNsb2NrLAo+IGZpbmUuIFRoZW4g aG9wZWZ1bGx5IHRoZSB1c2VyIGluIHRoZSBrZXJuZWwga25vd3Mgd2hhdCBoZSBpcyBkb2luZwo+ IHdpdGggdGhlIGNsb2NrIGFuZCB0aGVuIGhlIHNob3VsZCBkbyB3aGF0IGV2ZXIgaGUgd2FudHMu CgpJJ20gbm90IHN1cmUgdGhhdCdzIHRydWUuIEEgdXNlciBvZiB0aGUgY2xvY2sgbWF5IGtub3cg bm90aGluZyBhYm91dApvdGhlciB1c2Vycy4gQXMgZmFyIGFzIEkgY2FuIHNlZSwgbm90aGluZyBw cmV2ZW50cyBpdCBmcm9tIGNoYW5naW5nIHRoZQpjbG9jayByYXRlLgoKV2hpbGUgZHJpdmVycyBp biB0aGUgc2FtZSBrZXJuZWwgY2FuIGNvLW9yZGluYXRlIHRvIGF2b2lkIHByb2JsZW1zIHN1Y2gK YXMgdGhhdCwgd2UgY2FuJ3QgZG8gdGhhdCBpZiB3ZSBrbm93IG5vdGhpbmcgYWJvdXQgdGhlIHVz ZXIgaGlkZGVuIGJ5Clhlbi4KCkZyb20gbG9va2luZyBhdCB0aGUgWGVuIGJ1ZyB0cmFja2VyLCBp dCdzIG5vdCBjbGVhciB3aGljaApwbGF0Zm9ybXMvZGV2aWNlcyB0aGlzIGlzIG5lY2Vzc2FyeSBm b3IsIHNvIGl0J3Mgc3RpbGwgbm90IGNsZWFyIHRvIG1lCmlmIHdlIG5lZWQgdG8gY2FyZSBhYm91 dCByZWd1bGF0b3JzLCBHUElPcywgcmVzZXRzLCBhbmQgc28gb24uCgpEbyB3ZSBrbm93IHdoaWNo IHBsYXRmb3JtcyBpbiBwYXJ0aWN1bGFyIHdlIG5lZWQgdGhpcyBmb3I/Cgo+IEFzIHRoZXJlIGlz IGEgdXNlciBpbiB0aGUga2VybmVsLCB3ZSBkb24ndCBoYXZlIHRvIGNhcmUgYWJvdXQKPiAnYWNj aWRlbnRhbGx5JyBkaXNhYmxpbmcgaXQuCj4gCj4gSnVzdCBsZXQgdXMgY2FyZSBhYm91dCB0aGUg Y2xvY2tzIHRoZXJlIGlzIG5vIHVzZXIuCgpBcyBhYm92ZSwgSSBkb24ndCBiZWxpZXZlIHRoYXQg YWxvbmUgaXMgc3VmZmljaWVudCBpbiBnZW5lcmFsLCB0aG91Z2ggaXQKbWF5IGhhcHBlbiB0byBi ZSBmb3IgYSBwYXJ0aWN1bGFyIHBsYXRmb3JtLiBXaXRob3V0IGluZm9ybWF0aW9uCnJlZ2FyZGlu ZyB0aGUgYWZmZWN0ZWQgcGxhdGZvcm0ocyksIGl0J3MgcmF0aGVyIGRpZmZpY3VsdCB0byB0ZWxs LgoKVGhhbmtzLApNYXJrLgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX18KWGVuLWRldmVsIG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpo dHRwOi8vbGlzdHMueGVuLm9yZy94ZW4tZGV2ZWwK