| Message ID | 1428432239-4114-5-git-send-email-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, 07 Apr 2015, Maxime Ripard wrote: > On Tue, Apr 07, 2015 at 07:43:59PM +0100, Lee Jones wrote: > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > .../devicetree/bindings/clock/clock-bindings.txt | 38 ++++++++++++++++++++++ > > 1 file changed, 38 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt > > index 06fc6d5..daf3323 100644 > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt > > @@ -44,6 +44,44 @@ For example: > > clocks by index. The names should reflect the clock output signal > > names for the device. > > > > +clock-always-on: Some hardware contains bunches of clocks which must never be > > + turned off. If drivers a) fail to obtain a reference to any > > + of these or b) give up a previously obtained reference > > + during suspend, the common clk framework will attempt to > > + disable them and a platform can fail irrecoverably as a > > + result. Usually the only way to recover from these failures > > + is to reboot. > > + > > + To avoid either of these two scenarios from catastrophically > > + disabling an otherwise perfectly healthy running system, > > + clocks can be identified as always-on using this property > > + from inside a clocksource's node. > > Isn't "clocksource" here way too similar to, I don't know, the > clocksource framework? Wouln't clock provider be better? Yes, I think you're right. Stephen also mentioned it. > > + > > + This property is not to be abused. It is only to be used to > > + protect platforms from being crippled by gated clocks, not > > + as a convenience function to avoid using the framework > > + correctly inside device drivers. > > Disregarding what's stated here, I'm pretty sure that this will > actually happen. Where do you place the cursor? That's up to Mike. > Should we create a new driver for our RAM controller, or do we want to > use clock-always-on? I would say that if all the driver did was to enable clocks, then you should use this instead. This binding was designed specifically for that purpose. However, if the aforementioned driver clock can be safely gated, then it should not be an always-on clock. > Do we really want to enforce this if we ever gain a driver that would > actually be able to manage its clock (like do we want the CPU clock to > never *ever* be gated just because we don't have a cpuidle/hotplug > driver yet?) As I've just mentioned, if a clock 'can' be turned off, this binding should never be used. Situations where using always-on as a stop-gap due to a lack of current functionality is what the paragraph above is trying to mitigate. > Have you seen the numerous NAK on such approach Mike did? I haven't, but the folks reviewing previous versions have. Do you have something specific in mind that you'd like to bring to my attention?
On Wed, 08 Apr 2015, Maxime Ripard wrote: > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote: > > > > + > > > > + This property is not to be abused. It is only to be used to > > > > + protect platforms from being crippled by gated clocks, not > > > > + as a convenience function to avoid using the framework > > > > + correctly inside device drivers. > > > > > > Disregarding what's stated here, I'm pretty sure that this will > > > actually happen. Where do you place the cursor? > > > > That's up to Mike. > > Except that Mike won't review any of the DT changes, so he won't be > able to refrain users from using it. Let alone out-of-tree DTs using a > mainline kernel. Ideally Mike should be Cc'ed on patches using clock bindings, but if he isn't the DT guys are smart enough to either make the right decisions themselves (Rob has Acked these bindings already, so will be on the lookout for misuse, I'm sure), or ask for Mike's help. > > > Should we create a new driver for our RAM controller, or do we want to > > > use clock-always-on? > > > > I would say that if all the driver did was to enable clocks, then you > > should use this instead. This binding was designed specifically for > > that purpose. > > > > However, if the aforementioned driver clock can be safely gated, then > > it should not be an always-on clock. > > Yeah, of course, I understand the original intent of it, but that > argument, which might very well be true at one point in time, might > not be true anymore two or three releases later. Why? The H/W isn't going to change in two or three releases. The clocks designated as 'always-on' will have to be on forever, or synonymously, 'always'. > And that driver might actually rely on the fact that the clock is shut > down, which won't be the case. I think you are missing the point of this binding. The driver can never rely on that in this use-case. If the clock is off, there is no device driver, period. > Introducing a DT interface solely by refering to the current state of > a driver is a bit odd. I'm not sure I get your point. This binding has nothing to do with drivers. > > > Do we really want to enforce this if we ever gain a driver that would > > > actually be able to manage its clock (like do we want the CPU clock to > > > never *ever* be gated just because we don't have a cpuidle/hotplug > > > driver yet?) > > > > As I've just mentioned, if a clock 'can' be turned off, this binding > > should never be used. Situations where using always-on as a stop-gap > > due to a lack of current functionality is what the paragraph above is > > trying to mitigate. > > But it's not really what this property is about. What this property > describes is that these clocks should never be gated. Any point in > time during the life of the system AND with in any kernel version. You got it, that's correct -- these clocks should never be gated. So why would that ever change? If that is likely (or even possible) to change in the future then this binding should not be used. To reiterate; this binding should be used on ungatable clocks only. Non-negotiable, non-changeable either by the introduction of new functionality/support or kernel version. > > > Have you seen the numerous NAK on such approach Mike did? > > > > I haven't, but the folks reviewing previous versions have. Do you > > have something specific in mind that you'd like to bring to my > > attention? > > Unfortunately, I haven't been able to dig out such mails. But it's why > we ended up with clock protection code in various clock drivers > including: > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484 > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154 > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320 > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183 > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504 > > Which is much more flexible, since you won't have to modify the DT to > change which clocks are to be left enabled, as well as way easier to > debug if you ever have to remove that property from the DT. You're right, you don't have the change the DT in these cases. You have to write new C code, which is _less_ flexible. So all these platforms are adding their own hand-rolled version of this binding, adding more duplication and cruft to the kernel. Instead they can use this 'always-on' and we can consolidate and strip it all out.
On Wed, 08 Apr 2015, Maxime Ripard wrote: > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote: > > On Wed, 08 Apr 2015, Maxime Ripard wrote: > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote: > > > > > > + > > > > > > + This property is not to be abused. It is only to be used to > > > > > > + protect platforms from being crippled by gated clocks, not > > > > > > + as a convenience function to avoid using the framework > > > > > > + correctly inside device drivers. > > > > > > > > > > Disregarding what's stated here, I'm pretty sure that this will > > > > > actually happen. Where do you place the cursor? > > > > > > > > That's up to Mike. > > > > > > Except that Mike won't review any of the DT changes, so he won't be > > > able to refrain users from using it. Let alone out-of-tree DTs using a > > > mainline kernel. > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if > > he isn't the DT guys are smart enough to either make the right > > decisions themselves (Rob has Acked these bindings already, so will be > > on the lookout for misuse, I'm sure), or ask for Mike's help. > > Yeah, right, as if this strategy really worked in the past.... > > Do we really want to look at even the DT bindings that have actually > been reviewed by maintainers that got merged? > > They don't have time for that, which is totally fine, but we really > should bury our head in the sand by actually thinking they will review > every single DT-related patch. > > Using that as an argument is just plain denial of what really happened > for the past 4 years. I agree that it's a problem, but this is a process problem and has nothing to do with this set. If you have a problem with the current process and have a better alternative, submit your thoughts to the DT list. Rejecting all new bindings because you are frightened that they will be used in a manner that they were not intended is not the way to go though. > > > > > Should we create a new driver for our RAM controller, or do we want to > > > > > use clock-always-on? > > > > > > > > I would say that if all the driver did was to enable clocks, then you > > > > should use this instead. This binding was designed specifically for > > > > that purpose. > > > > > > > > However, if the aforementioned driver clock can be safely gated, then > > > > it should not be an always-on clock. > > > > > > Yeah, of course, I understand the original intent of it, but that > > > argument, which might very well be true at one point in time, might > > > not be true anymore two or three releases later. > > > > Why? The H/W isn't going to change in two or three releases. The > > clocks designated as 'always-on' will have to be on forever, or > > synonymously, 'always'. > > > > > And that driver might actually rely on the fact that the clock is shut > > > down, which won't be the case. > > > > I think you are missing the point of this binding. The driver can > > never rely on that in this use-case. If the clock is off, there is no > > device driver, period. > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the > PM guys will be happy to hear that. > > And they are not device drivers, are not mandatory in the system, and > it's usually a good thing to keep the CPU running whenever you don't > have such drivers. > > > > Introducing a DT interface solely by refering to the current state of > > > a driver is a bit odd. > > > > I'm not sure I get your point. This binding has nothing to do with > > drivers. > > It's all about drivers. Or rather all about missing drivers. I think you are going to have to be more forthcoming with your issues with this binding, because I'm struggling to understand what your problem with it is. You have already pointed me to vendors which have a genuine/valid need for it. But instead you'd prefer they hand-roll their own implementations over multiple lines of C code (each). IMHO, that's madness. > > > > > Do we really want to enforce this if we ever gain a driver that would > > > > > actually be able to manage its clock (like do we want the CPU clock to > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug > > > > > driver yet?) > > > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding > > > > should never be used. Situations where using always-on as a stop-gap > > > > due to a lack of current functionality is what the paragraph above is > > > > trying to mitigate. > > > > > > But it's not really what this property is about. What this property > > > describes is that these clocks should never be gated. Any point in > > > time during the life of the system AND with in any kernel version. > > > > You got it, that's correct -- these clocks should never be gated. > > > > So why would that ever change? If that is likely (or even possible) > > to change in the future then this binding should not be used. > > > > To reiterate; this binding should be used on ungatable clocks only. > > Non-negotiable, non-changeable either by the introduction of new > > functionality/support or kernel version. > > I'm pretty sure that if that patch gets merged, by the end of the > year, there will be "incorrect" users by your standards. It's possible to abuse any binding. I don't see why you are so offended of this one in particular. > If you introduce a feature, you should expect people to use > it. If not, what's the point? By your own admission, there are genuine users for this binding and I expect people to use it. > > > > > Have you seen the numerous NAK on such approach Mike did? > > > > > > > > I haven't, but the folks reviewing previous versions have. Do you > > > > have something specific in mind that you'd like to bring to my > > > > attention? > > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why > > > we ended up with clock protection code in various clock drivers > > > including: > > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484 > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154 > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320 > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183 > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504 > > > > > > Which is much more flexible, since you won't have to modify the DT to > > > change which clocks are to be left enabled, as well as way easier to > > > debug if you ever have to remove that property from the DT. > > > > You're right, you don't have the change the DT in these cases. You > > have to write new C code, which is _less_ flexible. > > I'm sorry to learn that you never heard of that stable-DT thing. > > And a bit sorry to see that a maintainer is really seeing C as not > flexible. You're putting words in my mouth. I didn't say C was not flexible. I'm referencing the original DT pros i.e. it is possible to supply a different configuration without the need to compile the kernel. That's certainly true in this case. We can provide a clk-provider and tag it as always-on, all without re-compile. > > So all these platforms are adding their own hand-rolled version of > > this binding, adding more duplication and cruft to the kernel. > > Instead they can use this 'always-on' and we can consolidate and strip > > it all out. > > Except that all these platforms are actually not implementing a > binding, ie not an interface with the DT they are bound to. Each and > every of these platforms can change that list whenever they wish, just > by sending a single one-liner patch (just like the DT, really.). > > Which is not something that you can achieve with a DT binding. Once again, can you give me more information about why you have such a problem with this binding. I wish for it to be stable/ABI, I wish for it never to be removed, I envisage it will always be needed, so what's the problem? Do you have a vested interest that I am missing? Perhaps an example of possible calamity will help convince me that you're not completely wrong and blowing everything out of proportion for no good reason.
On Wed, 22 Apr 2015, Maxime Ripard wrote: > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote: > > On Wed, 08 Apr 2015, Maxime Ripard wrote: > > > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote: > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote: > > > > > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote: > > > > > > > > + > > > > > > > > + This property is not to be abused. It is only to be used to > > > > > > > > + protect platforms from being crippled by gated clocks, not > > > > > > > > + as a convenience function to avoid using the framework > > > > > > > > + correctly inside device drivers. > > > > > > > > > > > > > > Disregarding what's stated here, I'm pretty sure that this will > > > > > > > actually happen. Where do you place the cursor? > > > > > > > > > > > > That's up to Mike. > > > > > > > > > > Except that Mike won't review any of the DT changes, so he won't be > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a > > > > > mainline kernel. > > > > > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if > > > > he isn't the DT guys are smart enough to either make the right > > > > decisions themselves (Rob has Acked these bindings already, so will be > > > > on the lookout for misuse, I'm sure), or ask for Mike's help. > > > > > > Yeah, right, as if this strategy really worked in the past.... > > > > > > Do we really want to look at even the DT bindings that have actually > > > been reviewed by maintainers that got merged? > > > > > > They don't have time for that, which is totally fine, but we really > > > should bury our head in the sand by actually thinking they will review > > > every single DT-related patch. > > > > > > Using that as an argument is just plain denial of what really happened > > > for the past 4 years. > > > > I agree that it's a problem, but this is a process problem and has > > nothing to do with this set. If you have a problem with the current > > process and have a better alternative, submit your thoughts to the DT > > list. Rejecting all new bindings because you are frightened that they > > will be used in a manner that they were not intended is not the way to > > go though. > > I'm not saying that this binding should not go in because of a process > issue. > > I'm saying that discarding arguments against your binding by adding > restrictions that cannot be enforced is not reasonable. I'm open to constructive suggestions/alternatives. Hand rolling this stuff in C per vendor is not of of them. > > > > > > > Should we create a new driver for our RAM controller, or do we want to > > > > > > > use clock-always-on? > > > > > > > > > > > > I would say that if all the driver did was to enable clocks, then you > > > > > > should use this instead. This binding was designed specifically for > > > > > > that purpose. > > > > > > > > > > > > However, if the aforementioned driver clock can be safely gated, then > > > > > > it should not be an always-on clock. > > > > > > > > > > Yeah, of course, I understand the original intent of it, but that > > > > > argument, which might very well be true at one point in time, might > > > > > not be true anymore two or three releases later. > > > > > > > > Why? The H/W isn't going to change in two or three releases. The > > > > clocks designated as 'always-on' will have to be on forever, or > > > > synonymously, 'always'. > > > > > > > > > And that driver might actually rely on the fact that the clock is shut > > > > > down, which won't be the case. > > > > > > > > I think you are missing the point of this binding. The driver can > > > > never rely on that in this use-case. If the clock is off, there is no > > > > device driver, period. > > > > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the > > > PM guys will be happy to hear that. > > > > > > And they are not device drivers, are not mandatory in the system, and > > > it's usually a good thing to keep the CPU running whenever you don't > > > have such drivers. > > > > > > > > Introducing a DT interface solely by refering to the current state of > > > > > a driver is a bit odd. > > > > > > > > I'm not sure I get your point. This binding has nothing to do with > > > > drivers. > > > > > > It's all about drivers. Or rather all about missing drivers. > > > > I think you are going to have to be more forthcoming with your issues > > with this binding, because I'm struggling to understand what your > > problem with it is. You have already pointed me to vendors which have > > a genuine/valid need for it. But instead you'd prefer they hand-roll > > their own implementations over multiple lines of C code (each). > > I told you already. > > If you have that property, there's absolutely no way to do any kind of > clock management in the future. > > It might be fine for your use case, but see my point about the > unreasonable restriction. People are going to use it for clocks that > just don't have a driver *yet*, and when that driver will be merged we > will end up with a driver that (for example) makes the assumption that > the clock has been shut down to reset the IP, that might or might not > be the case, depending on how old the DT is exactly. There is a need for this binding, but as you say, it must not be abused. So how to we get people not to use it willy-nilly? IMO, if people choose to ignore the stark warning in the documentation then that's they're lookout. I guess you'd like to wrap them in more cotton wool than I would. That's fine too, but how. > This will be even a bigger madness if you ask me. > > > > > > > > Do we really want to enforce this if we ever gain a driver that would > > > > > > > actually be able to manage its clock (like do we want the CPU clock to > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug > > > > > > > driver yet?) > > > > > > > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding > > > > > > should never be used. Situations where using always-on as a stop-gap > > > > > > due to a lack of current functionality is what the paragraph above is > > > > > > trying to mitigate. > > > > > > > > > > But it's not really what this property is about. What this property > > > > > describes is that these clocks should never be gated. Any point in > > > > > time during the life of the system AND with in any kernel version. > > > > > > > > You got it, that's correct -- these clocks should never be gated. > > > > > > > > So why would that ever change? If that is likely (or even possible) > > > > to change in the future then this binding should not be used. > > > > > > > > To reiterate; this binding should be used on ungatable clocks only. > > > > Non-negotiable, non-changeable either by the introduction of new > > > > functionality/support or kernel version. > > > > > > I'm pretty sure that if that patch gets merged, by the end of the > > > year, there will be "incorrect" users by your standards. > > > > It's possible to abuse any binding. I don't see why you are so > > offended of this one in particular. > > I'm not offended, I just tried to push the same kind of patches two > years ago, with Mike pushing back, and actually came to see that he > was right a few monthes down the road. Well this was suggested by Mike. I even have his Ack already. So I guess he too has changed the error of his ways. :) > And yeah, your point that any binding can be abused is true. This one > is only so easy to abuse it's not even funny. > > > > If you introduce a feature, you should expect people to use > > > it. If not, what's the point? > > > > By your own admission, there are genuine users for this binding and I > > expect people to use it. > > The only thing that we really disagree upon is that whether that > restriction will really be followed. You expect people to, I > don't. It's the fundamental disagreement we have, that really prevent > any purely technical discussion. > > Maybe we can try to address that before moving forward? I'd like that. If a firm warning isn't good enough, then what will be? > > > > > > > Have you seen the numerous NAK on such approach Mike did? > > > > > > > > > > > > I haven't, but the folks reviewing previous versions have. Do you > > > > > > have something specific in mind that you'd like to bring to my > > > > > > attention? > > > > > > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why > > > > > we ended up with clock protection code in various clock drivers > > > > > including: > > > > > > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484 > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154 > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320 > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183 > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504 > > > > > > > > > > Which is much more flexible, since you won't have to modify the DT to > > > > > change which clocks are to be left enabled, as well as way easier to > > > > > debug if you ever have to remove that property from the DT. > > > > > > > > You're right, you don't have the change the DT in these cases. You > > > > have to write new C code, which is _less_ flexible. > > > > > > I'm sorry to learn that you never heard of that stable-DT thing. > > > > > > And a bit sorry to see that a maintainer is really seeing C as not > > > flexible. > > > > You're putting words in my mouth. I didn't say C was not flexible. > > > > I'm referencing the original DT pros i.e. it is possible to supply a > > different configuration without the need to compile the kernel. > > That's certainly true in this case. We can provide a clk-provider and > > tag it as always-on, all without re-compile. > > > > > > So all these platforms are adding their own hand-rolled version of > > > > this binding, adding more duplication and cruft to the kernel. > > > > Instead they can use this 'always-on' and we can consolidate and strip > > > > it all out. > > > > > > Except that all these platforms are actually not implementing a > > > binding, ie not an interface with the DT they are bound to. Each and > > > every of these platforms can change that list whenever they wish, just > > > by sending a single one-liner patch (just like the DT, really.). > > > > > > Which is not something that you can achieve with a DT binding. > > > > Once again, can you give me more information about why you have such a > > problem with this binding. I wish for it to be stable/ABI, I wish for > > it never to be removed, I envisage it will always be needed, so what's > > the problem? > > > > Do you have a vested interest that I am missing? > > > > Perhaps an example of possible calamity will help convince me that > > you're not completely wrong and blowing everything out of proportion > > for no good reason. > > Let's say you've introduced such a clock in kernel 4.0 for the memory > clock. > > At some point down the road, you create a ddrfreq driver (if that ever > exists). You have a new driver, which will manage the clock. > > In that driver, for some reason, you have to shutdown the clock to > reset the DDR controller. Of course that also means that you will be > removing the clk-always-on property from your DT. > > You will have in your driver something like: > > /* Reset our controller */ > clk_disable(clk); > clk_enable(clk): > > And then, you expect your controller to be in its out-of-reset > state. Which will be the case with a new DT, and not with the old one, > probably creating all kind of very entertaining issues to debug. > > All of this wouldn't be the case if you had this inside the kernel, > since (hopefully) the kernel is consistent with itself. Surely you must have realised already that DTBs are more tightly coupled to kernel versions than we would have initially liked? It's naive to assume that old DTBs will 'just-work' with newer kernels. Wrong decisions related to DT are being made daily. Adding mistakenly or foolhardily adding 'clk-always-on' to a DTS is not going to be the sole cause of breakage somewhere down the line. Pushing back on the acceptance of this binding based on idealistic, possibly already out-of-date premise is just frustrating. This useful binding should be accepted and people should not abuse it. If they do and the vendor Maintainer's review and accept then they have no foundation for recourse. Would you prefer it if I made the warning starker?
On Wed, 29 Apr 2015, Geert Uytterhoeven wrote: > On Wed, Apr 29, 2015 at 4:17 PM, Lee Jones <lee.jones@linaro.org> wrote: > > This useful binding should be accepted and people should not abuse > > it. If they do and the vendor Maintainer's review and accept then > > they have no foundation for recourse. > > > > Would you prefer it if I made the warning starker? > > Perhaps you can add a check for "clock-always-on" to scripts/checkpatch.pl, > to make people aware they should think twice? That's a really nice idea! Maxime, would that make you happ(y|ier)? > Nah, people don't read documentation, don't run checkpatch, ignore compiler > warnings, etc... :) You can only wrap people in so much cotton wool. If they then decide to ignore all the warnings and jump, woe betide them.
On Wed, 29 Apr 2015, Sascha Hauer wrote: > On Wed, Apr 29, 2015 at 03:17:51PM +0100, Lee Jones wrote: > > On Wed, 22 Apr 2015, Maxime Ripard wrote: > > > > > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote: > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote: > > > > > > > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote: > > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote: > > > > > > > > > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote: > > > > > > > > > > + > > > > > > > > > > + This property is not to be abused. It is only to be used to > > > > > > > > > > + protect platforms from being crippled by gated clocks, not > > > > > > > > > > + as a convenience function to avoid using the framework > > > > > > > > > > + correctly inside device drivers. > > > > > > > > > > > > > > > > > > Disregarding what's stated here, I'm pretty sure that this will > > > > > > > > > actually happen. Where do you place the cursor? > > > > > > > > > > > > > > > > That's up to Mike. > > > > > > > > > > > > > > Except that Mike won't review any of the DT changes, so he won't be > > > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a > > > > > > > mainline kernel. > > > > > > > > > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if > > > > > > he isn't the DT guys are smart enough to either make the right > > > > > > decisions themselves (Rob has Acked these bindings already, so will be > > > > > > on the lookout for misuse, I'm sure), or ask for Mike's help. > > > > > > > > > > Yeah, right, as if this strategy really worked in the past.... > > > > > > > > > > Do we really want to look at even the DT bindings that have actually > > > > > been reviewed by maintainers that got merged? > > > > > > > > > > They don't have time for that, which is totally fine, but we really > > > > > should bury our head in the sand by actually thinking they will review > > > > > every single DT-related patch. > > > > > > > > > > Using that as an argument is just plain denial of what really happened > > > > > for the past 4 years. > > > > > > > > I agree that it's a problem, but this is a process problem and has > > > > nothing to do with this set. If you have a problem with the current > > > > process and have a better alternative, submit your thoughts to the DT > > > > list. Rejecting all new bindings because you are frightened that they > > > > will be used in a manner that they were not intended is not the way to > > > > go though. > > > > > > I'm not saying that this binding should not go in because of a process > > > issue. > > > > > > I'm saying that discarding arguments against your binding by adding > > > restrictions that cannot be enforced is not reasonable. > > > > I'm open to constructive suggestions/alternatives. > > > > Hand rolling this stuff in C per vendor is not of of them. > > > > > > > > > > > Should we create a new driver for our RAM controller, or do we want to > > > > > > > > > use clock-always-on? > > > > > > > > > > > > > > > > I would say that if all the driver did was to enable clocks, then you > > > > > > > > should use this instead. This binding was designed specifically for > > > > > > > > that purpose. > > > > > > > > > > > > > > > > However, if the aforementioned driver clock can be safely gated, then > > > > > > > > it should not be an always-on clock. > > > > > > > > > > > > > > Yeah, of course, I understand the original intent of it, but that > > > > > > > argument, which might very well be true at one point in time, might > > > > > > > not be true anymore two or three releases later. > > > > > > > > > > > > Why? The H/W isn't going to change in two or three releases. The > > > > > > clocks designated as 'always-on' will have to be on forever, or > > > > > > synonymously, 'always'. > > > > > > > > > > > > > And that driver might actually rely on the fact that the clock is shut > > > > > > > down, which won't be the case. > > > > > > > > > > > > I think you are missing the point of this binding. The driver can > > > > > > never rely on that in this use-case. If the clock is off, there is no > > > > > > device driver, period. > > > > > > > > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the > > > > > PM guys will be happy to hear that. > > > > > > > > > > And they are not device drivers, are not mandatory in the system, and > > > > > it's usually a good thing to keep the CPU running whenever you don't > > > > > have such drivers. > > > > > > > > > > > > Introducing a DT interface solely by refering to the current state of > > > > > > > a driver is a bit odd. > > > > > > > > > > > > I'm not sure I get your point. This binding has nothing to do with > > > > > > drivers. > > > > > > > > > > It's all about drivers. Or rather all about missing drivers. > > > > > > > > I think you are going to have to be more forthcoming with your issues > > > > with this binding, because I'm struggling to understand what your > > > > problem with it is. You have already pointed me to vendors which have > > > > a genuine/valid need for it. But instead you'd prefer they hand-roll > > > > their own implementations over multiple lines of C code (each). > > > > > > I told you already. > > > > > > If you have that property, there's absolutely no way to do any kind of > > > clock management in the future. > > > > > > It might be fine for your use case, but see my point about the > > > unreasonable restriction. People are going to use it for clocks that > > > just don't have a driver *yet*, and when that driver will be merged we > > > will end up with a driver that (for example) makes the assumption that > > > the clock has been shut down to reset the IP, that might or might not > > > be the case, depending on how old the DT is exactly. > > > > There is a need for this binding, but as you say, it must not be > > abused. So how to we get people not to use it willy-nilly? > > > > IMO, if people choose to ignore the stark warning in the documentation > > then that's they're lookout. I guess you'd like to wrap them in more > > cotton wool than I would. That's fine too, but how. > > > > > This will be even a bigger madness if you ask me. > > > > > > > > > > > > Do we really want to enforce this if we ever gain a driver that would > > > > > > > > > actually be able to manage its clock (like do we want the CPU clock to > > > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug > > > > > > > > > driver yet?) > > > > > > > > > > > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding > > > > > > > > should never be used. Situations where using always-on as a stop-gap > > > > > > > > due to a lack of current functionality is what the paragraph above is > > > > > > > > trying to mitigate. > > > > > > > > > > > > > > But it's not really what this property is about. What this property > > > > > > > describes is that these clocks should never be gated. Any point in > > > > > > > time during the life of the system AND with in any kernel version. > > > > > > > > > > > > You got it, that's correct -- these clocks should never be gated. > > > > > > > > > > > > So why would that ever change? If that is likely (or even possible) > > > > > > to change in the future then this binding should not be used. > > > > > > > > > > > > To reiterate; this binding should be used on ungatable clocks only. > > > > > > Non-negotiable, non-changeable either by the introduction of new > > > > > > functionality/support or kernel version. > > > > > > > > > > I'm pretty sure that if that patch gets merged, by the end of the > > > > > year, there will be "incorrect" users by your standards. > > > > > > > > It's possible to abuse any binding. I don't see why you are so > > > > offended of this one in particular. > > > > > > I'm not offended, I just tried to push the same kind of patches two > > > years ago, with Mike pushing back, and actually came to see that he > > > was right a few monthes down the road. > > > > Well this was suggested by Mike. I even have his Ack already. So I > > guess he too has changed the error of his ways. :) > > > > > And yeah, your point that any binding can be abused is true. This one > > > is only so easy to abuse it's not even funny. > > > > > > > > If you introduce a feature, you should expect people to use > > > > > it. If not, what's the point? > > > > > > > > By your own admission, there are genuine users for this binding and I > > > > expect people to use it. > > > > > > The only thing that we really disagree upon is that whether that > > > restriction will really be followed. You expect people to, I > > > don't. It's the fundamental disagreement we have, that really prevent > > > any purely technical discussion. > > > > > > Maybe we can try to address that before moving forward? > > > > I'd like that. > > > > If a firm warning isn't good enough, then what will be? > > > > > > > > > > > Have you seen the numerous NAK on such approach Mike did? > > > > > > > > > > > > > > > > I haven't, but the folks reviewing previous versions have. Do you > > > > > > > > have something specific in mind that you'd like to bring to my > > > > > > > > attention? > > > > > > > > > > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why > > > > > > > we ended up with clock protection code in various clock drivers > > > > > > > including: > > > > > > > > > > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484 > > > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154 > > > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320 > > > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183 > > > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504 > > > > > > > > > > > > > > Which is much more flexible, since you won't have to modify the DT to > > > > > > > change which clocks are to be left enabled, as well as way easier to > > > > > > > debug if you ever have to remove that property from the DT. > > > > > > > > > > > > You're right, you don't have the change the DT in these cases. You > > > > > > have to write new C code, which is _less_ flexible. > > > > > > > > > > I'm sorry to learn that you never heard of that stable-DT thing. > > > > > > > > > > And a bit sorry to see that a maintainer is really seeing C as not > > > > > flexible. > > > > > > > > You're putting words in my mouth. I didn't say C was not flexible. > > > > > > > > I'm referencing the original DT pros i.e. it is possible to supply a > > > > different configuration without the need to compile the kernel. > > > > That's certainly true in this case. We can provide a clk-provider and > > > > tag it as always-on, all without re-compile. > > > > > > > > > > So all these platforms are adding their own hand-rolled version of > > > > > > this binding, adding more duplication and cruft to the kernel. > > > > > > Instead they can use this 'always-on' and we can consolidate and strip > > > > > > it all out. > > > > > > > > > > Except that all these platforms are actually not implementing a > > > > > binding, ie not an interface with the DT they are bound to. Each and > > > > > every of these platforms can change that list whenever they wish, just > > > > > by sending a single one-liner patch (just like the DT, really.). > > > > > > > > > > Which is not something that you can achieve with a DT binding. > > > > > > > > Once again, can you give me more information about why you have such a > > > > problem with this binding. I wish for it to be stable/ABI, I wish for > > > > it never to be removed, I envisage it will always be needed, so what's > > > > the problem? > > > > > > > > Do you have a vested interest that I am missing? > > > > > > > > Perhaps an example of possible calamity will help convince me that > > > > you're not completely wrong and blowing everything out of proportion > > > > for no good reason. > > > > > > Let's say you've introduced such a clock in kernel 4.0 for the memory > > > clock. > > > > > > At some point down the road, you create a ddrfreq driver (if that ever > > > exists). You have a new driver, which will manage the clock. > > > > > > In that driver, for some reason, you have to shutdown the clock to > > > reset the DDR controller. Of course that also means that you will be > > > removing the clk-always-on property from your DT. > > > > > > You will have in your driver something like: > > > > > > /* Reset our controller */ > > > clk_disable(clk); > > > clk_enable(clk): > > > > > > And then, you expect your controller to be in its out-of-reset > > > state. Which will be the case with a new DT, and not with the old one, > > > probably creating all kind of very entertaining issues to debug. > > > > > > All of this wouldn't be the case if you had this inside the kernel, > > > since (hopefully) the kernel is consistent with itself. > > > > Surely you must have realised already that DTBs are more tightly > > coupled to kernel versions than we would have initially liked? It's > > naive to assume that old DTBs will 'just-work' with newer kernels. > > > > Wrong decisions related to DT are being made daily. Adding mistakenly > > or foolhardily adding 'clk-always-on' to a DTS is not going to be the > > sole cause of breakage somewhere down the line. > > It may not be the sole cause of breakage, but one we can avoid. > > > > > Pushing back on the acceptance of this binding based on idealistic, > > possibly already out-of-date premise is just frustrating. > > > > This useful binding should be accepted and people should not abuse > > it. If they do and the vendor Maintainer's review and accept then > > they have no foundation for recourse. > > Maxime gave a good example for a clock which looks like a candidate for > clk-always-on but needs to be turned off in a later kernel revision. In the example Maxime gave, I'm struggling to understand how you might enable such a framework whist still using an old DT. If you want to make use of new features/drivers/enablement/power-saving you must be in a position to edit your platform code/Device Tree. > Could you elaborate why you still want to have the clk-always-on in the > device tree instead of in the kernel where it can be removed when > necessary? What's your problem with enabling the critical clocks using > clk_prepare_enable like other SoCs already do? I've explained what my issues are already. I'm not a fan of hand-rolling and duplicating code which can be consolidated and make generic. Call me old fashioned. :)
Quoting Lee Jones (2015-04-29 09:07:13) > On Wed, 29 Apr 2015, Sascha Hauer wrote: > > > On Wed, Apr 29, 2015 at 03:17:51PM +0100, Lee Jones wrote: > > > On Wed, 22 Apr 2015, Maxime Ripard wrote: > > > > > > > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote: > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote: > > > > > > > > > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote: > > > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote: > > > > > > > > > > > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote: > > > > > > > > > > > + > > > > > > > > > > > + This property is not to be abused. It is only to be used to > > > > > > > > > > > + protect platforms from being crippled by gated clocks, not > > > > > > > > > > > + as a convenience function to avoid using the framework > > > > > > > > > > > + correctly inside device drivers. > > > > > > > > > > > > > > > > > > > > Disregarding what's stated here, I'm pretty sure that this will > > > > > > > > > > actually happen. Where do you place the cursor? > > > > > > > > > > > > > > > > > > That's up to Mike. > > > > > > > > > > > > > > > > Except that Mike won't review any of the DT changes, so he won't be > > > > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a > > > > > > > > mainline kernel. > > > > > > > > > > > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if > > > > > > > he isn't the DT guys are smart enough to either make the right > > > > > > > decisions themselves (Rob has Acked these bindings already, so will be > > > > > > > on the lookout for misuse, I'm sure), or ask for Mike's help. > > > > > > > > > > > > Yeah, right, as if this strategy really worked in the past.... > > > > > > > > > > > > Do we really want to look at even the DT bindings that have actually > > > > > > been reviewed by maintainers that got merged? > > > > > > > > > > > > They don't have time for that, which is totally fine, but we really > > > > > > should bury our head in the sand by actually thinking they will review > > > > > > every single DT-related patch. > > > > > > > > > > > > Using that as an argument is just plain denial of what really happened > > > > > > for the past 4 years. > > > > > > > > > > I agree that it's a problem, but this is a process problem and has > > > > > nothing to do with this set. If you have a problem with the current > > > > > process and have a better alternative, submit your thoughts to the DT > > > > > list. Rejecting all new bindings because you are frightened that they > > > > > will be used in a manner that they were not intended is not the way to > > > > > go though. > > > > > > > > I'm not saying that this binding should not go in because of a process > > > > issue. > > > > > > > > I'm saying that discarding arguments against your binding by adding > > > > restrictions that cannot be enforced is not reasonable. > > > > > > I'm open to constructive suggestions/alternatives. > > > > > > Hand rolling this stuff in C per vendor is not of of them. > > > > > > > > > > > > > Should we create a new driver for our RAM controller, or do we want to > > > > > > > > > > use clock-always-on? > > > > > > > > > > > > > > > > > > I would say that if all the driver did was to enable clocks, then you > > > > > > > > > should use this instead. This binding was designed specifically for > > > > > > > > > that purpose. > > > > > > > > > > > > > > > > > > However, if the aforementioned driver clock can be safely gated, then > > > > > > > > > it should not be an always-on clock. > > > > > > > > > > > > > > > > Yeah, of course, I understand the original intent of it, but that > > > > > > > > argument, which might very well be true at one point in time, might > > > > > > > > not be true anymore two or three releases later. > > > > > > > > > > > > > > Why? The H/W isn't going to change in two or three releases. The > > > > > > > clocks designated as 'always-on' will have to be on forever, or > > > > > > > synonymously, 'always'. > > > > > > > > > > > > > > > And that driver might actually rely on the fact that the clock is shut > > > > > > > > down, which won't be the case. > > > > > > > > > > > > > > I think you are missing the point of this binding. The driver can > > > > > > > never rely on that in this use-case. If the clock is off, there is no > > > > > > > device driver, period. > > > > > > > > > > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the > > > > > > PM guys will be happy to hear that. > > > > > > > > > > > > And they are not device drivers, are not mandatory in the system, and > > > > > > it's usually a good thing to keep the CPU running whenever you don't > > > > > > have such drivers. > > > > > > > > > > > > > > Introducing a DT interface solely by refering to the current state of > > > > > > > > a driver is a bit odd. > > > > > > > > > > > > > > I'm not sure I get your point. This binding has nothing to do with > > > > > > > drivers. > > > > > > > > > > > > It's all about drivers. Or rather all about missing drivers. > > > > > > > > > > I think you are going to have to be more forthcoming with your issues > > > > > with this binding, because I'm struggling to understand what your > > > > > problem with it is. You have already pointed me to vendors which have > > > > > a genuine/valid need for it. But instead you'd prefer they hand-roll > > > > > their own implementations over multiple lines of C code (each). > > > > > > > > I told you already. > > > > > > > > If you have that property, there's absolutely no way to do any kind of > > > > clock management in the future. > > > > > > > > It might be fine for your use case, but see my point about the > > > > unreasonable restriction. People are going to use it for clocks that > > > > just don't have a driver *yet*, and when that driver will be merged we > > > > will end up with a driver that (for example) makes the assumption that > > > > the clock has been shut down to reset the IP, that might or might not > > > > be the case, depending on how old the DT is exactly. > > > > > > There is a need for this binding, but as you say, it must not be > > > abused. So how to we get people not to use it willy-nilly? > > > > > > IMO, if people choose to ignore the stark warning in the documentation > > > then that's they're lookout. I guess you'd like to wrap them in more > > > cotton wool than I would. That's fine too, but how. > > > > > > > This will be even a bigger madness if you ask me. > > > > > > > > > > > > > > Do we really want to enforce this if we ever gain a driver that would > > > > > > > > > > actually be able to manage its clock (like do we want the CPU clock to > > > > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug > > > > > > > > > > driver yet?) > > > > > > > > > > > > > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding > > > > > > > > > should never be used. Situations where using always-on as a stop-gap > > > > > > > > > due to a lack of current functionality is what the paragraph above is > > > > > > > > > trying to mitigate. > > > > > > > > > > > > > > > > But it's not really what this property is about. What this property > > > > > > > > describes is that these clocks should never be gated. Any point in > > > > > > > > time during the life of the system AND with in any kernel version. > > > > > > > > > > > > > > You got it, that's correct -- these clocks should never be gated. > > > > > > > > > > > > > > So why would that ever change? If that is likely (or even possible) > > > > > > > to change in the future then this binding should not be used. > > > > > > > > > > > > > > To reiterate; this binding should be used on ungatable clocks only. > > > > > > > Non-negotiable, non-changeable either by the introduction of new > > > > > > > functionality/support or kernel version. > > > > > > > > > > > > I'm pretty sure that if that patch gets merged, by the end of the > > > > > > year, there will be "incorrect" users by your standards. > > > > > > > > > > It's possible to abuse any binding. I don't see why you are so > > > > > offended of this one in particular. > > > > > > > > I'm not offended, I just tried to push the same kind of patches two > > > > years ago, with Mike pushing back, and actually came to see that he > > > > was right a few monthes down the road. > > > > > > Well this was suggested by Mike. I even have his Ack already. So I > > > guess he too has changed the error of his ways. :) > > > > > > > And yeah, your point that any binding can be abused is true. This one > > > > is only so easy to abuse it's not even funny. > > > > > > > > > > If you introduce a feature, you should expect people to use > > > > > > it. If not, what's the point? > > > > > > > > > > By your own admission, there are genuine users for this binding and I > > > > > expect people to use it. > > > > > > > > The only thing that we really disagree upon is that whether that > > > > restriction will really be followed. You expect people to, I > > > > don't. It's the fundamental disagreement we have, that really prevent > > > > any purely technical discussion. > > > > > > > > Maybe we can try to address that before moving forward? > > > > > > I'd like that. > > > > > > If a firm warning isn't good enough, then what will be? > > > > > > > > > > > > > Have you seen the numerous NAK on such approach Mike did? > > > > > > > > > > > > > > > > > > I haven't, but the folks reviewing previous versions have. Do you > > > > > > > > > have something specific in mind that you'd like to bring to my > > > > > > > > > attention? > > > > > > > > > > > > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why > > > > > > > > we ended up with clock protection code in various clock drivers > > > > > > > > including: > > > > > > > > > > > > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484 > > > > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154 > > > > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320 > > > > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183 > > > > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504 > > > > > > > > > > > > > > > > Which is much more flexible, since you won't have to modify the DT to > > > > > > > > change which clocks are to be left enabled, as well as way easier to > > > > > > > > debug if you ever have to remove that property from the DT. > > > > > > > > > > > > > > You're right, you don't have the change the DT in these cases. You > > > > > > > have to write new C code, which is _less_ flexible. > > > > > > > > > > > > I'm sorry to learn that you never heard of that stable-DT thing. > > > > > > > > > > > > And a bit sorry to see that a maintainer is really seeing C as not > > > > > > flexible. > > > > > > > > > > You're putting words in my mouth. I didn't say C was not flexible. > > > > > > > > > > I'm referencing the original DT pros i.e. it is possible to supply a > > > > > different configuration without the need to compile the kernel. > > > > > That's certainly true in this case. We can provide a clk-provider and > > > > > tag it as always-on, all without re-compile. > > > > > > > > > > > > So all these platforms are adding their own hand-rolled version of > > > > > > > this binding, adding more duplication and cruft to the kernel. > > > > > > > Instead they can use this 'always-on' and we can consolidate and strip > > > > > > > it all out. > > > > > > > > > > > > Except that all these platforms are actually not implementing a > > > > > > binding, ie not an interface with the DT they are bound to. Each and > > > > > > every of these platforms can change that list whenever they wish, just > > > > > > by sending a single one-liner patch (just like the DT, really.). > > > > > > > > > > > > Which is not something that you can achieve with a DT binding. > > > > > > > > > > Once again, can you give me more information about why you have such a > > > > > problem with this binding. I wish for it to be stable/ABI, I wish for > > > > > it never to be removed, I envisage it will always be needed, so what's > > > > > the problem? > > > > > > > > > > Do you have a vested interest that I am missing? > > > > > > > > > > Perhaps an example of possible calamity will help convince me that > > > > > you're not completely wrong and blowing everything out of proportion > > > > > for no good reason. > > > > > > > > Let's say you've introduced such a clock in kernel 4.0 for the memory > > > > clock. > > > > > > > > At some point down the road, you create a ddrfreq driver (if that ever > > > > exists). You have a new driver, which will manage the clock. > > > > > > > > In that driver, for some reason, you have to shutdown the clock to > > > > reset the DDR controller. Of course that also means that you will be > > > > removing the clk-always-on property from your DT. > > > > > > > > You will have in your driver something like: > > > > > > > > /* Reset our controller */ > > > > clk_disable(clk); > > > > clk_enable(clk): > > > > > > > > And then, you expect your controller to be in its out-of-reset > > > > state. Which will be the case with a new DT, and not with the old one, > > > > probably creating all kind of very entertaining issues to debug. > > > > > > > > All of this wouldn't be the case if you had this inside the kernel, > > > > since (hopefully) the kernel is consistent with itself. > > > > > > Surely you must have realised already that DTBs are more tightly > > > coupled to kernel versions than we would have initially liked? It's > > > naive to assume that old DTBs will 'just-work' with newer kernels. > > > > > > Wrong decisions related to DT are being made daily. Adding mistakenly > > > or foolhardily adding 'clk-always-on' to a DTS is not going to be the > > > sole cause of breakage somewhere down the line. > > > > It may not be the sole cause of breakage, but one we can avoid. > > > > > > > > Pushing back on the acceptance of this binding based on idealistic, > > > possibly already out-of-date premise is just frustrating. > > > > > > This useful binding should be accepted and people should not abuse > > > it. If they do and the vendor Maintainer's review and accept then > > > they have no foundation for recourse. > > > > Maxime gave a good example for a clock which looks like a candidate for > > clk-always-on but needs to be turned off in a later kernel revision. > > In the example Maxime gave, I'm struggling to understand how you might > enable such a framework whist still using an old DT. If you want to > make use of new features/drivers/enablement/power-saving you must be > in a position to edit your platform code/Device Tree. Not true. The DT sticklers out there will point out that DT should describe the hardware. We have clock bindings for this. Whether or not Linux controls a given clk node for now or later on is irrelevant to the correctness of the binding and the dts/dtb. You could deploy a fully realized dtb on a factory image for a device, but later update the device in the field by replacing two loadable kernel modules: 1) the clock provider driver 2) the clock consumer driver This requires no change to DT and would allow for a clock consumer driver to ignore aggressive clock gating in the factory image (clock provider driver calls clk_get, clk_prepare_enable) and then later on receive and update to your kernel modules that enabled aggressive power management (clock provider only registers the clk, clock consumer driver calls clk_get, clk_prepare_enable). > > > Could you elaborate why you still want to have the clk-always-on in the > > device tree instead of in the kernel where it can be removed when > > necessary? What's your problem with enabling the critical clocks using > > clk_prepare_enable like other SoCs already do? > > I've explained what my issues are already. I'm not a fan of > hand-rolling and duplicating code which can be consolidated and make > generic. Call me old fashioned. :) I agree with Lee that the open code stuff isn't great, but I'm less and less convinced that the DT method is the way to go. Maxime has done a good job of reminding me why I always pushed back on this type of approach in the past. Having a clock provider driver call clk_get, clk_prepare_enable on a clk is just fine. I think what needs to be done is to look at the platforms that open code this and find out how to replace this with some common code that any clock provider driver can call. Perhaps we can: 1) use the struct clk_ops.init callback (which is used very little) and pass in a generic function to handle this case, or 2) we can create a new per-clk flag which is used by __clk_init to call clk_prepare_enable, or 3) we can add a new generic function like clk_register which sets any specified defaults (clk_set_defaults), but using C code and not DT I would need to look at the drivers that open code their clk_prepare_enable calls for non-Linux devices and see what similarities exist. But clearly the DT element of Lee's approach is causing some push back, so we should consider if there is a less controversial way to do this (and a way that benefits non-DT platforms as well). I do think Lee's idea of consolidating around a single solution to a common problem is a great idea, but maybe not by using Devicetree. Regards, Mike > > -- > Lee Jones > Linaro STMicroelectronics Landing Team Lead > Linaro.org │ Open source software for ARM SoCs > Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, 29 Apr 2015, Maxime Ripard wrote: > On Wed, Apr 29, 2015 at 03:17:51PM +0100, Lee Jones wrote: > > On Wed, 22 Apr 2015, Maxime Ripard wrote: > > > > > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote: > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote: > > > > > > > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote: > > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote: > > > > > > > > > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote: > > > > > > > > > > + > > > > > > > > > > + This property is not to be abused. It is only to be used to > > > > > > > > > > + protect platforms from being crippled by gated clocks, not > > > > > > > > > > + as a convenience function to avoid using the framework > > > > > > > > > > + correctly inside device drivers. > > > > > > > > > > > > > > > > > > Disregarding what's stated here, I'm pretty sure that this will > > > > > > > > > actually happen. Where do you place the cursor? > > > > > > > > > > > > > > > > That's up to Mike. > > > > > > > > > > > > > > Except that Mike won't review any of the DT changes, so he won't be > > > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a > > > > > > > mainline kernel. > > > > > > > > > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if > > > > > > he isn't the DT guys are smart enough to either make the right > > > > > > decisions themselves (Rob has Acked these bindings already, so will be > > > > > > on the lookout for misuse, I'm sure), or ask for Mike's help. > > > > > > > > > > Yeah, right, as if this strategy really worked in the past.... > > > > > > > > > > Do we really want to look at even the DT bindings that have actually > > > > > been reviewed by maintainers that got merged? > > > > > > > > > > They don't have time for that, which is totally fine, but we really > > > > > should bury our head in the sand by actually thinking they will review > > > > > every single DT-related patch. > > > > > > > > > > Using that as an argument is just plain denial of what really happened > > > > > for the past 4 years. > > > > > > > > I agree that it's a problem, but this is a process problem and has > > > > nothing to do with this set. If you have a problem with the current > > > > process and have a better alternative, submit your thoughts to the DT > > > > list. Rejecting all new bindings because you are frightened that they > > > > will be used in a manner that they were not intended is not the way to > > > > go though. > > > > > > I'm not saying that this binding should not go in because of a process > > > issue. > > > > > > I'm saying that discarding arguments against your binding by adding > > > restrictions that cannot be enforced is not reasonable. > > > > I'm open to constructive suggestions/alternatives. > > > > Hand rolling this stuff in C per vendor is not of of them. > > I'm sorry, but ruling out alternatives that work for everyone (and > actually work better) just because you don't want to edit a C file is > not really constructive either. > > > > > > > > > > Should we create a new driver for our RAM controller, or do we want to > > > > > > > > > use clock-always-on? > > > > > > > > > > > > > > > > I would say that if all the driver did was to enable clocks, then you > > > > > > > > should use this instead. This binding was designed specifically for > > > > > > > > that purpose. > > > > > > > > > > > > > > > > However, if the aforementioned driver clock can be safely gated, then > > > > > > > > it should not be an always-on clock. > > > > > > > > > > > > > > Yeah, of course, I understand the original intent of it, but that > > > > > > > argument, which might very well be true at one point in time, might > > > > > > > not be true anymore two or three releases later. > > > > > > > > > > > > Why? The H/W isn't going to change in two or three releases. The > > > > > > clocks designated as 'always-on' will have to be on forever, or > > > > > > synonymously, 'always'. > > > > > > > > > > > > > And that driver might actually rely on the fact that the clock is shut > > > > > > > down, which won't be the case. > > > > > > > > > > > > I think you are missing the point of this binding. The driver can > > > > > > never rely on that in this use-case. If the clock is off, there is no > > > > > > device driver, period. > > > > > > > > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the > > > > > PM guys will be happy to hear that. > > > > > > > > > > And they are not device drivers, are not mandatory in the system, and > > > > > it's usually a good thing to keep the CPU running whenever you don't > > > > > have such drivers. > > > > > > > > > > > > Introducing a DT interface solely by refering to the current state of > > > > > > > a driver is a bit odd. > > > > > > > > > > > > I'm not sure I get your point. This binding has nothing to do with > > > > > > drivers. > > > > > > > > > > It's all about drivers. Or rather all about missing drivers. > > > > > > > > I think you are going to have to be more forthcoming with your issues > > > > with this binding, because I'm struggling to understand what your > > > > problem with it is. You have already pointed me to vendors which have > > > > a genuine/valid need for it. But instead you'd prefer they hand-roll > > > > their own implementations over multiple lines of C code (each). > > > > > > I told you already. > > > > > > If you have that property, there's absolutely no way to do any kind of > > > clock management in the future. > > > > > > It might be fine for your use case, but see my point about the > > > unreasonable restriction. People are going to use it for clocks that > > > just don't have a driver *yet*, and when that driver will be merged we > > > will end up with a driver that (for example) makes the assumption that > > > the clock has been shut down to reset the IP, that might or might not > > > be the case, depending on how old the DT is exactly. > > > > There is a need for this binding, > > That's your opinion. Several people already disagreed on this. > > Now, what we probably need is a generic way to flag the clocks as > supposed to be enabled. The fact that this information should be in > the DT is a different story. > > > but as you say, it must not be abused. So how to we get people not > > to use it willy-nilly? > > > > IMO, if people choose to ignore the stark warning in the documentation > > then that's they're lookout. I guess you'd like to wrap them in more > > cotton wool than I would. That's fine too, but how. > > You've chosen to ignore all our warnings. Fine. Assume that other > people will ignore yours. > > This is not about whatever you put in the documentation or checkpatch, > there will be people and/or maintainers that will go against that. > > The only way to prevent any abuse of the binding, is not to have any > binding, really. > > > > This will be even a bigger madness if you ask me. > > > > > > > > > > > > Do we really want to enforce this if we ever gain a driver that would > > > > > > > > > actually be able to manage its clock (like do we want the CPU clock to > > > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug > > > > > > > > > driver yet?) > > > > > > > > > > > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding > > > > > > > > should never be used. Situations where using always-on as a stop-gap > > > > > > > > due to a lack of current functionality is what the paragraph above is > > > > > > > > trying to mitigate. > > > > > > > > > > > > > > But it's not really what this property is about. What this property > > > > > > > describes is that these clocks should never be gated. Any point in > > > > > > > time during the life of the system AND with in any kernel version. > > > > > > > > > > > > You got it, that's correct -- these clocks should never be gated. > > > > > > > > > > > > So why would that ever change? If that is likely (or even possible) > > > > > > to change in the future then this binding should not be used. > > > > > > > > > > > > To reiterate; this binding should be used on ungatable clocks only. > > > > > > Non-negotiable, non-changeable either by the introduction of new > > > > > > functionality/support or kernel version. > > > > > > > > > > I'm pretty sure that if that patch gets merged, by the end of the > > > > > year, there will be "incorrect" users by your standards. > > > > > > > > It's possible to abuse any binding. I don't see why you are so > > > > offended of this one in particular. > > > > > > I'm not offended, I just tried to push the same kind of patches two > > > years ago, with Mike pushing back, and actually came to see that he > > > was right a few monthes down the road. > > > > Well this was suggested by Mike. I even have his Ack already. So I > > guess he too has changed the error of his ways. :) > > I wonder why it's not even merged yet then if you have the maintainers > Acked-by, and want to ignore any other review. > > > > And yeah, your point that any binding can be abused is true. This one > > > is only so easy to abuse it's not even funny. > > > > > > > > If you introduce a feature, you should expect people to use > > > > > it. If not, what's the point? > > > > > > > > By your own admission, there are genuine users for this binding and I > > > > expect people to use it. > > > > > > The only thing that we really disagree upon is that whether that > > > restriction will really be followed. You expect people to, I > > > don't. It's the fundamental disagreement we have, that really prevent > > > any purely technical discussion. > > > > > > Maybe we can try to address that before moving forward? > > > > I'd like that. > > > > If a firm warning isn't good enough, then what will be? > > > > > > > > > > > Have you seen the numerous NAK on such approach Mike did? > > > > > > > > > > > > > > > > I haven't, but the folks reviewing previous versions have. Do you > > > > > > > > have something specific in mind that you'd like to bring to my > > > > > > > > attention? > > > > > > > > > > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why > > > > > > > we ended up with clock protection code in various clock drivers > > > > > > > including: > > > > > > > > > > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484 > > > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154 > > > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320 > > > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183 > > > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504 > > > > > > > > > > > > > > Which is much more flexible, since you won't have to modify the DT to > > > > > > > change which clocks are to be left enabled, as well as way easier to > > > > > > > debug if you ever have to remove that property from the DT. > > > > > > > > > > > > You're right, you don't have the change the DT in these cases. You > > > > > > have to write new C code, which is _less_ flexible. > > > > > > > > > > I'm sorry to learn that you never heard of that stable-DT thing. > > > > > > > > > > And a bit sorry to see that a maintainer is really seeing C as not > > > > > flexible. > > > > > > > > You're putting words in my mouth. I didn't say C was not flexible. > > > > > > > > I'm referencing the original DT pros i.e. it is possible to supply a > > > > different configuration without the need to compile the kernel. > > > > That's certainly true in this case. We can provide a clk-provider and > > > > tag it as always-on, all without re-compile. > > > > > > > > > > So all these platforms are adding their own hand-rolled version of > > > > > > this binding, adding more duplication and cruft to the kernel. > > > > > > Instead they can use this 'always-on' and we can consolidate and strip > > > > > > it all out. > > > > > > > > > > Except that all these platforms are actually not implementing a > > > > > binding, ie not an interface with the DT they are bound to. Each and > > > > > every of these platforms can change that list whenever they wish, just > > > > > by sending a single one-liner patch (just like the DT, really.). > > > > > > > > > > Which is not something that you can achieve with a DT binding. > > > > > > > > Once again, can you give me more information about why you have such a > > > > problem with this binding. I wish for it to be stable/ABI, I wish for > > > > it never to be removed, I envisage it will always be needed, so what's > > > > the problem? > > > > > > > > Do you have a vested interest that I am missing? > > > > > > > > Perhaps an example of possible calamity will help convince me that > > > > you're not completely wrong and blowing everything out of proportion > > > > for no good reason. > > > > > > Let's say you've introduced such a clock in kernel 4.0 for the memory > > > clock. > > > > > > At some point down the road, you create a ddrfreq driver (if that ever > > > exists). You have a new driver, which will manage the clock. > > > > > > In that driver, for some reason, you have to shutdown the clock to > > > reset the DDR controller. Of course that also means that you will be > > > removing the clk-always-on property from your DT. > > > > > > You will have in your driver something like: > > > > > > /* Reset our controller */ > > > clk_disable(clk); > > > clk_enable(clk): > > > > > > And then, you expect your controller to be in its out-of-reset > > > state. Which will be the case with a new DT, and not with the old one, > > > probably creating all kind of very entertaining issues to debug. > > > > > > All of this wouldn't be the case if you had this inside the kernel, > > > since (hopefully) the kernel is consistent with itself. > > > > Surely you must have realised already that DTBs are more tightly > > coupled to kernel versions than we would have initially liked? > > Just to get things straight. I'm *not* one proponent of the "DT as an > ABI" rule. > > BUT, if you're designing a generic property, then you're also > designing it for platforms that have chose to have that stability. And > we do have some of those in the tree. > > And of course, that's assuming that the ABI stability is never going > to be a thing (which might or might not happen). > > > It's naive to assume that old DTBs will 'just-work' with newer > > kernels. > > Like I said, it does work on some mainline ARM platforms. > > > Wrong decisions related to DT are being made daily. Adding > > mistakenly or foolhardily adding 'clk-always-on' to a DTS is not > > going to be the sole cause of breakage somewhere down the line. > > I'm not sure that saying that accepting bindings because some other > bindings are just as bad is a good argument, especially on bindings > that do impact all the platforms. > > > Pushing back on the acceptance of this binding based on idealistic, > > possibly already out-of-date premise is just frustrating. > > Don't worry, it's just as frustrating on the other end. Showing you > exactly why it's going to be an issue with you simply ignoring by > saying something close to "not my use case" is just as frustrating. > > > This useful binding should be accepted and people should not abuse > > it. If they do and the vendor Maintainer's review and accept then > > they have no foundation for recourse. > > And in the end, there will be more and more bloated and / or poor code > in the kernel, hurting it as a whole. > > > Would you prefer it if I made the warning starker? > > The only kind of warning that would be noticed by anyone is a runtime > warning. I'm not sure this is a reasonable option :) Does Sascha's antidote patch change your opinion? We can use DT to declare critical clocks, and in the rare case of the introduction of a new DDRFreq-like feature, which doesn't adapt the DT will still be able to unlock the criticalness of the clock and use it as expected?
On Fri, 01 May 2015, Sascha Hauer wrote: > On Thu, Apr 30, 2015 at 10:57:22AM +0100, Lee Jones wrote: > > On Wed, 29 Apr 2015, Maxime Ripard wrote: > > > > > On Wed, Apr 29, 2015 at 03:17:51PM +0100, Lee Jones wrote: > > > > On Wed, 22 Apr 2015, Maxime Ripard wrote: > > > > > > > > > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote: > > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote: > > > > > > > > > > > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote: > > > > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote: > > > > > > > > > > > > > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote: > > > > > > > > > > > > + > > > > > > > > > > > > + This property is not to be abused. It is only to be used to > > > > > > > > > > > > + protect platforms from being crippled by gated clocks, not > > > > > > > > > > > > + as a convenience function to avoid using the framework > > > > > > > > > > > > + correctly inside device drivers. > > > > > > > > > > > > > > > > > > > > > > Disregarding what's stated here, I'm pretty sure that this will > > > > > > > > > > > actually happen. Where do you place the cursor? > > > > > > > > > > > > > > > > > > > > That's up to Mike. > > > > > > > > > > > > > > > > > > Except that Mike won't review any of the DT changes, so he won't be > > > > > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a > > > > > > > > > mainline kernel. > > > > > > > > > > > > > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if > > > > > > > > he isn't the DT guys are smart enough to either make the right > > > > > > > > decisions themselves (Rob has Acked these bindings already, so will be > > > > > > > > on the lookout for misuse, I'm sure), or ask for Mike's help. > > > > > > > > > > > > > > Yeah, right, as if this strategy really worked in the past.... > > > > > > > > > > > > > > Do we really want to look at even the DT bindings that have actually > > > > > > > been reviewed by maintainers that got merged? > > > > > > > > > > > > > > They don't have time for that, which is totally fine, but we really > > > > > > > should bury our head in the sand by actually thinking they will review > > > > > > > every single DT-related patch. > > > > > > > > > > > > > > Using that as an argument is just plain denial of what really happened > > > > > > > for the past 4 years. > > > > > > > > > > > > I agree that it's a problem, but this is a process problem and has > > > > > > nothing to do with this set. If you have a problem with the current > > > > > > process and have a better alternative, submit your thoughts to the DT > > > > > > list. Rejecting all new bindings because you are frightened that they > > > > > > will be used in a manner that they were not intended is not the way to > > > > > > go though. > > > > > > > > > > I'm not saying that this binding should not go in because of a process > > > > > issue. > > > > > > > > > > I'm saying that discarding arguments against your binding by adding > > > > > restrictions that cannot be enforced is not reasonable. > > > > > > > > I'm open to constructive suggestions/alternatives. > > > > > > > > Hand rolling this stuff in C per vendor is not of of them. > > > > > > I'm sorry, but ruling out alternatives that work for everyone (and > > > actually work better) just because you don't want to edit a C file is > > > not really constructive either. > > > > > > > > > > > > > > Should we create a new driver for our RAM controller, or do we want to > > > > > > > > > > > use clock-always-on? > > > > > > > > > > > > > > > > > > > > I would say that if all the driver did was to enable clocks, then you > > > > > > > > > > should use this instead. This binding was designed specifically for > > > > > > > > > > that purpose. > > > > > > > > > > > > > > > > > > > > However, if the aforementioned driver clock can be safely gated, then > > > > > > > > > > it should not be an always-on clock. > > > > > > > > > > > > > > > > > > Yeah, of course, I understand the original intent of it, but that > > > > > > > > > argument, which might very well be true at one point in time, might > > > > > > > > > not be true anymore two or three releases later. > > > > > > > > > > > > > > > > Why? The H/W isn't going to change in two or three releases. The > > > > > > > > clocks designated as 'always-on' will have to be on forever, or > > > > > > > > synonymously, 'always'. > > > > > > > > > > > > > > > > > And that driver might actually rely on the fact that the clock is shut > > > > > > > > > down, which won't be the case. > > > > > > > > > > > > > > > > I think you are missing the point of this binding. The driver can > > > > > > > > never rely on that in this use-case. If the clock is off, there is no > > > > > > > > device driver, period. > > > > > > > > > > > > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the > > > > > > > PM guys will be happy to hear that. > > > > > > > > > > > > > > And they are not device drivers, are not mandatory in the system, and > > > > > > > it's usually a good thing to keep the CPU running whenever you don't > > > > > > > have such drivers. > > > > > > > > > > > > > > > > Introducing a DT interface solely by refering to the current state of > > > > > > > > > a driver is a bit odd. > > > > > > > > > > > > > > > > I'm not sure I get your point. This binding has nothing to do with > > > > > > > > drivers. > > > > > > > > > > > > > > It's all about drivers. Or rather all about missing drivers. > > > > > > > > > > > > I think you are going to have to be more forthcoming with your issues > > > > > > with this binding, because I'm struggling to understand what your > > > > > > problem with it is. You have already pointed me to vendors which have > > > > > > a genuine/valid need for it. But instead you'd prefer they hand-roll > > > > > > their own implementations over multiple lines of C code (each). > > > > > > > > > > I told you already. > > > > > > > > > > If you have that property, there's absolutely no way to do any kind of > > > > > clock management in the future. > > > > > > > > > > It might be fine for your use case, but see my point about the > > > > > unreasonable restriction. People are going to use it for clocks that > > > > > just don't have a driver *yet*, and when that driver will be merged we > > > > > will end up with a driver that (for example) makes the assumption that > > > > > the clock has been shut down to reset the IP, that might or might not > > > > > be the case, depending on how old the DT is exactly. > > > > > > > > There is a need for this binding, > > > > > > That's your opinion. Several people already disagreed on this. > > > > > > Now, what we probably need is a generic way to flag the clocks as > > > supposed to be enabled. The fact that this information should be in > > > the DT is a different story. > > > > > > > but as you say, it must not be abused. So how to we get people not > > > > to use it willy-nilly? > > > > > > > > IMO, if people choose to ignore the stark warning in the documentation > > > > then that's they're lookout. I guess you'd like to wrap them in more > > > > cotton wool than I would. That's fine too, but how. > > > > > > You've chosen to ignore all our warnings. Fine. Assume that other > > > people will ignore yours. > > > > > > This is not about whatever you put in the documentation or checkpatch, > > > there will be people and/or maintainers that will go against that. > > > > > > The only way to prevent any abuse of the binding, is not to have any > > > binding, really. > > > > > > > > This will be even a bigger madness if you ask me. > > > > > > > > > > > > > > > > Do we really want to enforce this if we ever gain a driver that would > > > > > > > > > > > actually be able to manage its clock (like do we want the CPU clock to > > > > > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug > > > > > > > > > > > driver yet?) > > > > > > > > > > > > > > > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding > > > > > > > > > > should never be used. Situations where using always-on as a stop-gap > > > > > > > > > > due to a lack of current functionality is what the paragraph above is > > > > > > > > > > trying to mitigate. > > > > > > > > > > > > > > > > > > But it's not really what this property is about. What this property > > > > > > > > > describes is that these clocks should never be gated. Any point in > > > > > > > > > time during the life of the system AND with in any kernel version. > > > > > > > > > > > > > > > > You got it, that's correct -- these clocks should never be gated. > > > > > > > > > > > > > > > > So why would that ever change? If that is likely (or even possible) > > > > > > > > to change in the future then this binding should not be used. > > > > > > > > > > > > > > > > To reiterate; this binding should be used on ungatable clocks only. > > > > > > > > Non-negotiable, non-changeable either by the introduction of new > > > > > > > > functionality/support or kernel version. > > > > > > > > > > > > > > I'm pretty sure that if that patch gets merged, by the end of the > > > > > > > year, there will be "incorrect" users by your standards. > > > > > > > > > > > > It's possible to abuse any binding. I don't see why you are so > > > > > > offended of this one in particular. > > > > > > > > > > I'm not offended, I just tried to push the same kind of patches two > > > > > years ago, with Mike pushing back, and actually came to see that he > > > > > was right a few monthes down the road. > > > > > > > > Well this was suggested by Mike. I even have his Ack already. So I > > > > guess he too has changed the error of his ways. :) > > > > > > I wonder why it's not even merged yet then if you have the maintainers > > > Acked-by, and want to ignore any other review. > > > > > > > > And yeah, your point that any binding can be abused is true. This one > > > > > is only so easy to abuse it's not even funny. > > > > > > > > > > > > If you introduce a feature, you should expect people to use > > > > > > > it. If not, what's the point? > > > > > > > > > > > > By your own admission, there are genuine users for this binding and I > > > > > > expect people to use it. > > > > > > > > > > The only thing that we really disagree upon is that whether that > > > > > restriction will really be followed. You expect people to, I > > > > > don't. It's the fundamental disagreement we have, that really prevent > > > > > any purely technical discussion. > > > > > > > > > > Maybe we can try to address that before moving forward? > > > > > > > > I'd like that. > > > > > > > > If a firm warning isn't good enough, then what will be? > > > > > > > > > > > > > > > Have you seen the numerous NAK on such approach Mike did? > > > > > > > > > > > > > > > > > > > > I haven't, but the folks reviewing previous versions have. Do you > > > > > > > > > > have something specific in mind that you'd like to bring to my > > > > > > > > > > attention? > > > > > > > > > > > > > > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why > > > > > > > > > we ended up with clock protection code in various clock drivers > > > > > > > > > including: > > > > > > > > > > > > > > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484 > > > > > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154 > > > > > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320 > > > > > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183 > > > > > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504 > > > > > > > > > > > > > > > > > > Which is much more flexible, since you won't have to modify the DT to > > > > > > > > > change which clocks are to be left enabled, as well as way easier to > > > > > > > > > debug if you ever have to remove that property from the DT. > > > > > > > > > > > > > > > > You're right, you don't have the change the DT in these cases. You > > > > > > > > have to write new C code, which is _less_ flexible. > > > > > > > > > > > > > > I'm sorry to learn that you never heard of that stable-DT thing. > > > > > > > > > > > > > > And a bit sorry to see that a maintainer is really seeing C as not > > > > > > > flexible. > > > > > > > > > > > > You're putting words in my mouth. I didn't say C was not flexible. > > > > > > > > > > > > I'm referencing the original DT pros i.e. it is possible to supply a > > > > > > different configuration without the need to compile the kernel. > > > > > > That's certainly true in this case. We can provide a clk-provider and > > > > > > tag it as always-on, all without re-compile. > > > > > > > > > > > > > > So all these platforms are adding their own hand-rolled version of > > > > > > > > this binding, adding more duplication and cruft to the kernel. > > > > > > > > Instead they can use this 'always-on' and we can consolidate and strip > > > > > > > > it all out. > > > > > > > > > > > > > > Except that all these platforms are actually not implementing a > > > > > > > binding, ie not an interface with the DT they are bound to. Each and > > > > > > > every of these platforms can change that list whenever they wish, just > > > > > > > by sending a single one-liner patch (just like the DT, really.). > > > > > > > > > > > > > > Which is not something that you can achieve with a DT binding. > > > > > > > > > > > > Once again, can you give me more information about why you have such a > > > > > > problem with this binding. I wish for it to be stable/ABI, I wish for > > > > > > it never to be removed, I envisage it will always be needed, so what's > > > > > > the problem? > > > > > > > > > > > > Do you have a vested interest that I am missing? > > > > > > > > > > > > Perhaps an example of possible calamity will help convince me that > > > > > > you're not completely wrong and blowing everything out of proportion > > > > > > for no good reason. > > > > > > > > > > Let's say you've introduced such a clock in kernel 4.0 for the memory > > > > > clock. > > > > > > > > > > At some point down the road, you create a ddrfreq driver (if that ever > > > > > exists). You have a new driver, which will manage the clock. > > > > > > > > > > In that driver, for some reason, you have to shutdown the clock to > > > > > reset the DDR controller. Of course that also means that you will be > > > > > removing the clk-always-on property from your DT. > > > > > > > > > > You will have in your driver something like: > > > > > > > > > > /* Reset our controller */ > > > > > clk_disable(clk); > > > > > clk_enable(clk): > > > > > > > > > > And then, you expect your controller to be in its out-of-reset > > > > > state. Which will be the case with a new DT, and not with the old one, > > > > > probably creating all kind of very entertaining issues to debug. > > > > > > > > > > All of this wouldn't be the case if you had this inside the kernel, > > > > > since (hopefully) the kernel is consistent with itself. > > > > > > > > Surely you must have realised already that DTBs are more tightly > > > > coupled to kernel versions than we would have initially liked? > > > > > > Just to get things straight. I'm *not* one proponent of the "DT as an > > > ABI" rule. > > > > > > BUT, if you're designing a generic property, then you're also > > > designing it for platforms that have chose to have that stability. And > > > we do have some of those in the tree. > > > > > > And of course, that's assuming that the ABI stability is never going > > > to be a thing (which might or might not happen). > > > > > > > It's naive to assume that old DTBs will 'just-work' with newer > > > > kernels. > > > > > > Like I said, it does work on some mainline ARM platforms. > > > > > > > Wrong decisions related to DT are being made daily. Adding > > > > mistakenly or foolhardily adding 'clk-always-on' to a DTS is not > > > > going to be the sole cause of breakage somewhere down the line. > > > > > > I'm not sure that saying that accepting bindings because some other > > > bindings are just as bad is a good argument, especially on bindings > > > that do impact all the platforms. > > > > > > > Pushing back on the acceptance of this binding based on idealistic, > > > > possibly already out-of-date premise is just frustrating. > > > > > > Don't worry, it's just as frustrating on the other end. Showing you > > > exactly why it's going to be an issue with you simply ignoring by > > > saying something close to "not my use case" is just as frustrating. > > > > > > > This useful binding should be accepted and people should not abuse > > > > it. If they do and the vendor Maintainer's review and accept then > > > > they have no foundation for recourse. > > > > > > And in the end, there will be more and more bloated and / or poor code > > > in the kernel, hurting it as a whole. > > > > > > > Would you prefer it if I made the warning starker? > > > > > > The only kind of warning that would be noticed by anyone is a runtime > > > warning. I'm not sure this is a reasonable option :) > > > > Does Sascha's antidote patch change your opinion? We can use DT to > > declare critical clocks, and in the rare case of the introduction of a > > new DDRFreq-like feature, which doesn't adapt the DT will still be > > able to unlock the criticalness of the clock and use it as expected? > > Honestly I'm not very fond of declaring these in the device tree, but I know why you guys are saying that, but I'd like you to understand the reasons for me pushing for this. Rather than be being deliberately obtuse, I'm thinking of the mess that not having this stuff in DT will cause for clock implementations like ours, which describe more of a framework than a description. The providers in drivers/clock/st are blissfully ignorant of platform specifics. Per-platform configuration is described in DT. So we'd have 2 options to use a C-only based API; 1) duplicate platform information in drivers/clk/st, or 2) supply a vendor specific st,critical-clocks binding, pull out those references then run them though the aforementioned framework. It is my opinion that neither of those methods are desirable. As an aside, we also have 9 add_provider() and 9 clock_register() calls, where we would need to consider calling the new critical_clock() API from/after. Please tell me that you understand and agree, or please provide me with a suggestion to combat the issues I currently face. > naming them 'critical-clocks' rather than 'always-on' seems more > acceptable for me. It leaves a way out for the user to turn the clock > off later as it only means "you may turn them off when you know what you > are doing". With the introduction of your latest patch we are no longer in the peril previously described, thus if a platform does separate their DT from its associated kernel, it won't be the end of the world (or cost a couple mWh) if a clock becomes controllable in a subsequent kernel version.
On Thu, 07 May 2015, Maxime Ripard wrote: > On Fri, May 01, 2015 at 07:44:05AM +0100, Lee Jones wrote: > > > > Does Sascha's antidote patch change your opinion? We can use DT to > > > > declare critical clocks, and in the rare case of the introduction of a > > > > new DDRFreq-like feature, which doesn't adapt the DT will still be > > > > able to unlock the criticalness of the clock and use it as expected? > > > > > > Honestly I'm not very fond of declaring these in the device tree, but > > > > I know why you guys are saying that, but I'd like you to understand > > the reasons for me pushing for this. Rather than be being deliberately > > obtuse, I'm thinking of the mess that not having this stuff in DT will > > cause for clock implementations like ours, which describe more of a > > framework than a description. > > The DT should dictate our implementation, not the other way around. I > know that we are pretty bad at doing this, and that there's some clear > abstraction violations already widely used, but really, using this > kind of argument is pretty bad. I guess then you haven't correctly understood my argument, as that's exactly what's happened. We have a DT implementation which accurately describes the clock architecture on each of our platforms. The associated C code in drivers/clk/ is written to extract the information from it, the hardware description and register the clocks properly. What makes you think differently? > The DT can (and is) shared between several OS and bootloaders, what if > the *BSDs or barebox, or whatever, guys come up with the exact same > argument to make a completely different binding? > > We'd end up either in a deadlock, or forcing our solution down the > throat to some other system. I'm not sure any of these outcomes is > something we want. Not sure I understand why this is different from any other binding? > > The providers in drivers/clock/st are blissfully ignorant of platform > > specifics. Per-platform configuration is described in DT. > > Maybe they just need a small amount of education then. Easy to say (and implement), but that means duplicating the hardware description in DT, which is not a design win. > > So we'd have 2 options to use a C-only based API; 1) duplicate > > platform information in drivers/clk/st, or 2) supply a vendor > > specific st,critical-clocks binding, pull out those references then > > run them though the aforementioned framework. It is my opinion that > > neither of those methods are desirable. > > 3) have a generic solution for this in the clock framework, like Mike > suggested. Did you actually read and understand the points here? If not, just say so and I'll figure out a way to explain the issues better. 3) is not an alternative to 1) and 2). Instead 1) and 2) imply 3). I *want* to have a generic solution, and have made several passes at writing one. The question here is; what does that look like? Some people don't like the idea of having it in DT due to possible abuse of the property. But we can't have anything only in C because our clock implementation (rightly) doesn't know or (shouldn't have to) care about platform specifics. Instead all platform description is in DT, where it should be. So to specify critical-clocks we need either 1) or 2) above to pull the info out and send to 3).
diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt index 06fc6d5..daf3323 100644 --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt @@ -44,6 +44,44 @@ For example: clocks by index. The names should reflect the clock output signal names for the device. +clock-always-on: Some hardware contains bunches of clocks which must never be + turned off. If drivers a) fail to obtain a reference to any + of these or b) give up a previously obtained reference + during suspend, the common clk framework will attempt to + disable them and a platform can fail irrecoverably as a + result. Usually the only way to recover from these failures + is to reboot. + + To avoid either of these two scenarios from catastrophically + disabling an otherwise perfectly healthy running system, + clocks can be identified as always-on using this property + from inside a clocksource's node. + + This property is not to be abused. It is only to be used to + protect platforms from being crippled by gated clocks, not + as a convenience function to avoid using the framework + correctly inside device drivers. + + Expected values are hardware clock indices. If the + clock-indices property (see below) is used, then supplied + values must correspond to one of the listed identifiers. + Using the clock-indices example below, hardware clock <2> + is missing, therefore it is considered invalid to then + list clock <2> as an always-on clock. + +For example: + + oscillator { + #clock-cells = <1>; + clock-output-names = "ckil", "ckih"; + clock-always-on = <0>, <1>; + }; + +- this node defines a device with two clock outputs, just as in the + example above. The only difference being that 'ckil' and 'ckih' + are now identified as an always-on clocks, so the framework will + know to never attempt to gate them. + clock-indices: If the identifying number for the clocks in the node is not linear from zero, then this allows the mapping of identifiers into the clock-output-names array.
Signed-off-by: Lee Jones <lee.jones@linaro.org> --- .../devicetree/bindings/clock/clock-bindings.txt | 38 ++++++++++++++++++++++ 1 file changed, 38 insertions(+)