| Message ID | 551537ED.7060402@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Iain, On 27-03-15 11:58, Iain Paton wrote: > add pmic regulator definitions matching the manufacturers 3.4.x fex > file. > > Signed-off-by: Iain Paton <ipaton0@gmail.com> > --- > > As this file belongs to Hans and he decided not to use axp209.dtsi in > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315612.html > then I won't add it here. That was because using it was breaking stuff, but now that we know better which regulators are used for what, and which one we most add an always-on; property too, I would greatly prefer you to acually use axp209.dtsi, see e.g. the sun5i-a13-utoo-p66.dts file where I'm using it. Can you please respin this patch using axp209.dtsi? Also please do not add nodes for unused regulators, like the ldo-s for the csi-s. Thanks & Regards, Hans > > arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts | 68 ++++++++++++++++++++++++++ > 1 file changed, 68 insertions(+) > > diff --git a/arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts b/arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts > index 31dc2f1..f7a8fcc 100644 > --- a/arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts > +++ b/arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts > @@ -67,6 +67,15 @@ > default-state = "on"; > }; > }; > + > + reg_axp_ipsout: axp_ipsout { > + compatible = "regulator-fixed"; > + regulator-name = "axp-ipsout"; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + regulator-always-on; > + }; > + > }; > > &ahci { > @@ -97,10 +106,69 @@ > axp209: pmic@34 { > compatible = "x-powers,axp209"; > reg = <0x34>; > + interrupt-parent = <&intc>; > interrupts = <0>; > > interrupt-controller; > #interrupt-cells = <1>; > + > + acin-supply = <®_axp_ipsout>; > + vin2-supply = <®_axp_ipsout>; > + vin3-supply = <®_axp_ipsout>; > + ldo24in-supply = <®_axp_ipsout>; > + ldo3in-supply = <®_axp_ipsout>; > + > + x-powers,dcdc-freq = <1500>; > + > + regulators { > + vdd_rtc: ldo1 { > + regulator-min-microvolt = <1300000>; > + regulator-max-microvolt = <1300000>; > + regulator-always-on; > + regulator-name="vdd_rtc"; > + }; > + > + avcc: ldo2 { > + regulator-min-microvolt = <3000000>; > + regulator-max-microvolt = <3000000>; > + regulator-always-on; > + regulator-name="avcc"; > + }; > + > + vcc_csi0: ldo3 { > + regulator-min-microvolt = <2800000>; > + regulator-max-microvolt = <2800000>; > + regulator-always-on; > + regulator-name="vcc_csi0"; > + }; > + > + vcc_csi1: ldo4 { > + regulator-min-microvolt = <2800000>; > + regulator-max-microvolt = <2800000>; > + regulator-always-on; > + regulator-name="vcc_csi1"; > + }; > + > + ldo5 { > + regulator-min-microvolt = <2800000>; > + regulator-max-microvolt = <2800000>; > + regulator-name="reg_unused"; > + }; > + > + vdd_cpu: dcdc2 { > + regulator-min-microvolt = <1250000>; > + regulator-max-microvolt = <1550000>; > + regulator-always-on; > + regulator-name="vdd_cpu"; > + }; > + > + vdd_int: dcdc3 { > + regulator-min-microvolt = <1250000>; > + regulator-max-microvolt = <1250000>; > + regulator-always-on; > + regulator-name="vdd_int"; > + }; > + }; > }; > }; > >
On 27/03/15 11:02, Hans de Goede wrote: > Hi Iain, > > On 27-03-15 11:58, Iain Paton wrote: >> add pmic regulator definitions matching the manufacturers 3.4.x fex >> file. >> >> Signed-off-by: Iain Paton <ipaton0@gmail.com> >> --- >> >> As this file belongs to Hans and he decided not to use axp209.dtsi in >> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315612.html >> then I won't add it here. > > That was because using it was breaking stuff, but now that we know better > which regulators are used for what, and which one we most add an always-on; > property too, I would greatly prefer you to acually use axp209.dtsi, see > e.g. the sun5i-a13-utoo-p66.dts file where I'm using it. > > Can you please respin this patch using axp209.dtsi? > > Also please do not add nodes for unused regulators, like the ldo-s for the csi-s. You would prefer that the csi is broken on the lime then? Unlike the cubieboards, ldo3 & ldo4 really are used on the olimex boards. So it seems that as yet you still don't know enough about which regulators are used, or you wouldn't be asking for that. That's the major disadvantage of axp209.dtsi, the regulator node isn't describing the pmic at all. Instead it's describing the stuff connected to the pmic outputs, which is undeniably board specific and therefore totally unsuitable for a generic file. Leading to every board needing to override everything regardless. So no, I'm not going to respin the patch. I strongly believe using axp209.dtsi is the wrong thing to do here. If that means the patch doesn't make it in, then so be it. Rgds, Iain
Hi, On 03/28/2015 12:53 PM, Iain Paton wrote: > On 27/03/15 11:02, Hans de Goede wrote: >> Hi Iain, >> >> On 27-03-15 11:58, Iain Paton wrote: >>> add pmic regulator definitions matching the manufacturers 3.4.x fex >>> file. >>> >>> Signed-off-by: Iain Paton <ipaton0@gmail.com> >>> --- >>> >>> As this file belongs to Hans and he decided not to use axp209.dtsi in >>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315612.html >>> then I won't add it here. >> >> That was because using it was breaking stuff, but now that we know better >> which regulators are used for what, and which one we most add an always-on; >> property too, I would greatly prefer you to acually use axp209.dtsi, see >> e.g. the sun5i-a13-utoo-p66.dts file where I'm using it. >> >> Can you please respin this patch using axp209.dtsi? >> >> Also please do not add nodes for unused regulators, like the ldo-s for the csi-s. > > You would prefer that the csi is broken on the lime then? Unlike the cubieboards, > ldo3 & ldo4 really are used on the olimex boards. So your olimex boards have camera sensors attached? Because if not then csi is not used at all. > So it seems that as yet you still > don't know enough about which regulators are used, or you wouldn't be asking for > that. It would seem that you do not know what csi is for ... > That's the major disadvantage of axp209.dtsi, the regulator node isn't describing > the pmic at all. Instead it's describing the stuff connected to the pmic outputs, Not it is not, not anymore it is just a skeleton creating the regulator nodes so that you can address them by reference as is now the norm for configuring nodes in a dts. Have you actually looked at the current axp209.dtsi it does not contain any assumptions about how the regulators are used. > which is undeniably board specific and therefore totally unsuitable for a generic > file. Leading to every board needing to override everything regardless. > > So no, I'm not going to respin the patch. I strongly believe using axp209.dtsi is > the wrong thing to do here. > If that means the patch doesn't make it in, then so be it. Please take a look at sun5i-a13-utoo-p66.dts, if you do something similar like that, then ldo 3 and 4 will get disabled, please run some tests this way, then I expect that you will see that everything still works fine without them enabled, and if not you can easily enable them the same way the other regulators are enabled in sun5i-a13-utoo-p66.dts Regards, Hans
On Sat, Mar 28, 2015 at 11:53:06AM +0000, Iain Paton wrote: > On 27/03/15 11:02, Hans de Goede wrote: > > Hi Iain, > > > > On 27-03-15 11:58, Iain Paton wrote: > >> add pmic regulator definitions matching the manufacturers 3.4.x fex > >> file. > >> > >> Signed-off-by: Iain Paton <ipaton0@gmail.com> > >> --- > >> > >> As this file belongs to Hans and he decided not to use axp209.dtsi in > >> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315612.html > >> then I won't add it here. > > > > That was because using it was breaking stuff, but now that we know better > > which regulators are used for what, and which one we most add an always-on; > > property too, I would greatly prefer you to acually use axp209.dtsi, see > > e.g. the sun5i-a13-utoo-p66.dts file where I'm using it. > > > > Can you please respin this patch using axp209.dtsi? > > > > Also please do not add nodes for unused regulators, like the ldo-s for the csi-s. > > You would prefer that the csi is broken on the lime then? CSI is not usable (no driver for it), neither it is actually used by default on the lime, so disabling the regulator is the right thing to do. > Unlike the cubieboards, ldo3 & ldo4 really are used on the olimex > boards. So it seems that as yet you still don't know enough about > which regulators are used, or you wouldn't be asking for that. They are not used. They are connected to something that is not usable in itself, so leaving them enabled is just useless. > That's the major disadvantage of axp209.dtsi, the regulator node > isn't describing the pmic at all. Instead it's describing the stuff > connected to the pmic outputs, which is undeniably board specific > and therefore totally unsuitable for a generic file. Leading to > every board needing to override everything regardless. No, it's defining which regulators are provided by the regulator, and the voltage boundaries they have. It doesn't make any assumption with regards to what is connected to what, and if a particular regulator is connected to something. That's something that the board DTS should describe as accurately as possible. > So no, I'm not going to respin the patch. I strongly believe using > axp209.dtsi is the wrong thing to do here. If that means the patch > doesn't make it in, then so be it. Too bad. Maxime
On 28/03/15 23:50, Hans de Goede wrote: > So your olimex boards have camera sensors attached? Because if not then csi is not > used at all. Yes. >> So it seems that as yet you still >> don't know enough about which regulators are used, or you wouldn't be asking for >> that. > > It would seem that you do not know what csi is for ... I know quite well what it's for. My HW stops working when I do what you're asking. Rgds, Iain
On 30/03/15 23:23, Maxime Ripard wrote: > CSI is not usable (no driver for it), neither it is actually used by > default on the lime, so disabling the regulator is the right thing to > do. My userspace code works just fine, until the regulator gets turned off. Anyway, I don't have the patience Hans showed with the simplefb stuff. So consider yourselves successful in wearing me down. I'm done debating any of this. One less contributor to linux-sunxi for you to worry about, I won't bother you anymore. > Too bad. For who? Rgds, Iain
Hi, On 04-04-15 12:30, Iain Paton wrote: > On 30/03/15 23:23, Maxime Ripard wrote: > >> CSI is not usable (no driver for it), neither it is actually used by >> default on the lime, so disabling the regulator is the right thing to >> do. > > My userspace code works just fine, until the regulator gets turned off. So you've userspace code to use the csi interface? Cool! Any chance you can publish this somewhere. I've writing v4l2 drivers for the csi on the sunxi SoCs somewhere (low) on my todo list. > Anyway, I don't have the patience Hans showed with the simplefb stuff. > So consider yourselves successful in wearing me down. I'm done debating > any of this. One less contributor to linux-sunxi for you to worry about, > I won't bother you anymore. I'm sorry to hear that, it would have helped tremendously here if you had explained that you actually have a camera sensor hooked up and are using that with an upstream kernel right from the start. As I've hopefully shown when merging your u-boot patches we certainly welcome you as a contributor, and I do hope you will keep contributing in the future. As for this specific patch I'm still of the opinion that your original patch is not the right way forward, we are using axp209.dtsi with all new boards and we are slowly moving existing boards over too. Note that it is only a matter of time until someone does submit a patch which follows the standard way of doing things for sunxi boards and then your camera sensors will stop working. Recent kernel support something known as devicetree overlays which I believe is the way forward with this, what we would like to see for cases like this one is to have a patch in the main dts which works as Maxime and I requested, and then use a dts overlay to enable ldo3 and ldo4 for your specific situation. This solution should also be future proof, since as said someone will make the requested changes eventually as we plan to move all boards to using axp209.dtsi and not enabling unnecessary regulators. You will likely object to me labeling the ldo3 and ldo4 regulators as unnecessary but think this through for a moment Olimex has sold thousands of lime boards, how much of those do you think will have camera sensors attached? For the default upstream dts file we need to do what is right for the normal / default use case here. And for people who have special hardware / daughtboards attached that is exactly what devicetree overlays are designed to handle. Regards, Hans > >> Too bad. > > For who? > > Rgds, > Iain >
[adding Mark Brown and Stephen Warren to cc list] Hello, On Tue, Mar 31, 2015 at 12:23 AM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Sat, Mar 28, 2015 at 11:53:06AM +0000, Iain Paton wrote: >> On 27/03/15 11:02, Hans de Goede wrote: >> > Hi Iain, >> > >> > On 27-03-15 11:58, Iain Paton wrote: >> >> add pmic regulator definitions matching the manufacturers 3.4.x fex >> >> file. >> >> >> >> Signed-off-by: Iain Paton <ipaton0@gmail.com> >> >> --- >> >> >> >> As this file belongs to Hans and he decided not to use axp209.dtsi in >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315612.html >> >> then I won't add it here. >> > >> > That was because using it was breaking stuff, but now that we know better >> > which regulators are used for what, and which one we most add an always-on; >> > property too, I would greatly prefer you to acually use axp209.dtsi, see >> > e.g. the sun5i-a13-utoo-p66.dts file where I'm using it. >> > >> > Can you please respin this patch using axp209.dtsi? >> > >> > Also please do not add nodes for unused regulators, like the ldo-s for the csi-s. >> >> You would prefer that the csi is broken on the lime then? > > CSI is not usable (no driver for it), neither it is actually used by > default on the lime, so disabling the regulator is the right thing to > do. > >> Unlike the cubieboards, ldo3 & ldo4 really are used on the olimex >> boards. So it seems that as yet you still don't know enough about >> which regulators are used, or you wouldn't be asking for that. > > They are not used. They are connected to something that is not usable > in itself, so leaving them enabled is just useless. > >> That's the major disadvantage of axp209.dtsi, the regulator node >> isn't describing the pmic at all. Instead it's describing the stuff >> connected to the pmic outputs, which is undeniably board specific >> and therefore totally unsuitable for a generic file. Leading to >> every board needing to override everything regardless. > > No, it's defining which regulators are provided by the regulator, and > the voltage boundaries they have. It doesn't make any assumption with > regards to what is connected to what, and if a particular regulator is > connected to something. That's something that the board DTS should > describe as accurately as possible. > >> So no, I'm not going to respin the patch. I strongly believe using >> axp209.dtsi is the wrong thing to do here. If that means the patch >> doesn't make it in, then so be it. > > Too bad. > I'm not familiar with the sunxi platform but just wanted to mention that AFAIK Mark Brown thinks that generic PMIC .dtsi files describing regulators is not a good idea. At least he nacked [0] when I tried to do something similar to avoid having duplicated DTS fragments for the tps65090 PMU used in the Exynos Chromebooks. But I've added him as cc in case I misunderstood something. Stephen Warren (cc'ed too) also said that those files are not very useful [1] since are largely content free and the regulators description would have to be duplicated in the DTS anyways since that information is very board specific. Which is the point that Ian makes for not using the axp209.dtsi. The only two platforms that have included .dtsi for PMICs are sunxi and omap (i.e: twl4030.dtsi) but Mark said that at least in the case of the twl4030 pmic, the .dtsi is broken [0]. As I said, I'm not particularly interested in sunxi but I think it is good to have consistency on how things are done across the different platforms. > Maxime Best regards, Javier [0]: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/278983.html [1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/279265.html
On Sat, Apr 04, 2015 at 02:18:12PM +0200, Javier Martinez Canillas wrote: > On Tue, Mar 31, 2015 at 12:23 AM, Maxime Ripard > > No, it's defining which regulators are provided by the regulator, and > > the voltage boundaries they have. It doesn't make any assumption with > > regards to what is connected to what, and if a particular regulator is > > connected to something. That's something that the board DTS should > > describe as accurately as possible. This is broken - think about what this means. If you are defining a voltage (or any other constraint) you're saying that it's safe to use on a given board. If you provide a voltage constraint saying that the maximum allowable range for a voltage regulator is safe. That's unlikely to be true on any given board, usually only a limited set of regulators can vary voltages at runtime safely at all and then rarely over their full supported range. Similarly for other constraints, for example allowing a regulator to be disabled when there are driverless things (or drivers without regulator support or mappings for that board) relying on it is going to break. Providing a list of the regulators is safe but not really doing a huge amount. Note that I've not seen the original patch so it's possible it's doing something different.
Hi Mark, On Sat, Apr 04, 2015 at 01:33:02PM +0100, Mark Brown wrote: > On Sat, Apr 04, 2015 at 02:18:12PM +0200, Javier Martinez Canillas wrote: > > On Tue, Mar 31, 2015 at 12:23 AM, Maxime Ripard > > > > No, it's defining which regulators are provided by the regulator, and > > > the voltage boundaries they have. It doesn't make any assumption with > > > regards to what is connected to what, and if a particular regulator is > > > connected to something. That's something that the board DTS should > > > describe as accurately as possible. > > This is broken - think about what this means. If you are defining a > voltage (or any other constraint) you're saying that it's safe to use on > a given board. If you provide a voltage constraint saying that the > maximum allowable range for a voltage regulator is safe. That's > unlikely to be true on any given board, usually only a limited set of > regulators can vary voltages at runtime safely at all and then rarely > over their full supported range. Similarly for other constraints, for > example allowing a regulator to be disabled when there are driverless > things (or drivers without regulator support or mappings for that board) > relying on it is going to break. > > Providing a list of the regulators is safe but not really doing a huge > amount. > > Note that I've not seen the original patch so it's possible it's doing > something different. If this model is broken, then I don't see how the full regulator support is not broken. Our PMIC DTSI sets up the regulators with the ranges supported by the PMIC. We have two cases then: - Either the regulator is not used on the board, and it will be disabled by the framework. - Or the regulator is actually used on the board, it will be defined in the DTS, possibly with additional constraints. I really don't see how your above comments apply to that situation. What I do see though, is that if we drop the DTSI, all our boards will have to define all the PMIC regulators, even if they're not used at all on the board, just to have the right behaviour. And that's clearly a no-go for me. Maxime
Hi, On 04-04-15 14:33, Mark Brown wrote: > On Sat, Apr 04, 2015 at 02:18:12PM +0200, Javier Martinez Canillas wrote: >> On Tue, Mar 31, 2015 at 12:23 AM, Maxime Ripard > >>> No, it's defining which regulators are provided by the regulator, and >>> the voltage boundaries they have. It doesn't make any assumption with >>> regards to what is connected to what, and if a particular regulator is >>> connected to something. That's something that the board DTS should >>> describe as accurately as possible. > > This is broken - think about what this means. If you are defining a > voltage (or any other constraint) you're saying that it's safe to use on > a given board. If you provide a voltage constraint saying that the > maximum allowable range for a voltage regulator is safe. That's > unlikely to be true on any given board, usually only a limited set of > regulators can vary voltages at runtime safely at all and then rarely > over their full supported range. Similarly for other constraints, for > example allowing a regulator to be disabled when there are driverless > things (or drivers without regulator support or mappings for that board) > relying on it is going to break. Right, this is why in later revisions the pmic dtsi file does not contain any ranges, these must now all be set by the dts file. > Providing a list of the regulators is safe but not really doing a huge > amount. This (providing a list) is pretty much all the dtsi does, this is actually somewhat useful, because a regulator must be listed for it to get automatically turned off when not used. So by having the dtsi + dts files referencing only the regulators which they use we end up automatically turning off all the other regulators. FYI here is the full contents of the dtsi: /* * Copyright 2015 Chen-Yu Tsai * * Chen-Yu Tsai <wens@csie.org> * * This file is dual-licensed: you can use it either under the terms * of the GPL or the X11 license, at your option. Note that this dual * licensing only applies to this file, and not this project as a * whole. * * a) This file is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License as * published by the Free Software Foundation; either version 2 of the * License, or (at your option) any later version. * * This file is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public * License along with this file; if not, write to the Free * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, * MA 02110-1301 USA * * Or, alternatively, * * b) Permission is hereby granted, free of charge, to any person * obtaining a copy of this software and associated documentation * files (the "Software"), to deal in the Software without * restriction, including without limitation the rights to use, * copy, modify, merge, publish, distribute, sublicense, and/or * sell copies of the Software, and to permit persons to whom the * Software is furnished to do so, subject to the following * conditions: * * The above copyright notice and this permission notice shall be * included in all copies or substantial portions of the Software. * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR * OTHER DEALINGS IN THE SOFTWARE. */ /* * AXP202/209 Integrated Power Management Chip * http://www.x-powers.com/product/AXP20X.php * http://dl.linux-sunxi.org/AXP/AXP209%20Datasheet%20v1.0_cn.pdf */ &axp209 { compatible = "x-powers,axp209"; interrupt-controller; #interrupt-cells = <1>; regulators { /* Default work frequency for buck regulators */ x-powers,dcdc-freq = <1500>; reg_dcdc2: dcdc2 { regulator-name = "dcdc2"; }; reg_dcdc3: dcdc3 { regulator-name = "dcdc3"; }; reg_ldo1: ldo1 { /* LDO1 is a fixed output regulator */ regulator-always-on; regulator-min-microvolt = <1300000>; regulator-max-microvolt = <1300000>; regulator-name = "ldo1"; }; reg_ldo2: ldo2 { regulator-name = "ldo2"; }; reg_ldo3: ldo3 { regulator-name = "ldo3"; }; reg_ldo4: ldo4 { regulator-name = "ldo4"; }; reg_ldo5: ldo5 { regulator-name = "ldo5"; }; }; }; Regards, Hans
Hi, On 04-04-15 14:56, Maxime Ripard wrote: > Hi Mark, > > On Sat, Apr 04, 2015 at 01:33:02PM +0100, Mark Brown wrote: >> On Sat, Apr 04, 2015 at 02:18:12PM +0200, Javier Martinez Canillas wrote: >>> On Tue, Mar 31, 2015 at 12:23 AM, Maxime Ripard >> >>>> No, it's defining which regulators are provided by the regulator, and >>>> the voltage boundaries they have. It doesn't make any assumption with >>>> regards to what is connected to what, and if a particular regulator is >>>> connected to something. That's something that the board DTS should >>>> describe as accurately as possible. >> >> This is broken - think about what this means. If you are defining a >> voltage (or any other constraint) you're saying that it's safe to use on >> a given board. If you provide a voltage constraint saying that the >> maximum allowable range for a voltage regulator is safe. That's >> unlikely to be true on any given board, usually only a limited set of >> regulators can vary voltages at runtime safely at all and then rarely >> over their full supported range. Similarly for other constraints, for >> example allowing a regulator to be disabled when there are driverless >> things (or drivers without regulator support or mappings for that board) >> relying on it is going to break. >> >> Providing a list of the regulators is safe but not really doing a huge >> amount. >> >> Note that I've not seen the original patch so it's possible it's doing >> something different. > > If this model is broken, then I don't see how the full regulator > support is not broken. > > Our PMIC DTSI sets up the regulators with the ranges supported by the > PMIC. We have two cases then: > - Either the regulator is not used on the board, and it will be > disabled by the framework. > - Or the regulator is actually used on the board, it will be defined > in the DTS, possibly with additional constraints. Note that we've already stopped defining any constraints in the dtsi as that indeed does not make much sense, if the dts does not provide any constraints but does use / enable the regulator then: 1) That is a bug in the dts really 2) It will end up with the actual hardware constraints anyways as those are coded into the driver. >What I do see though, is that if we drop the DTSI, all our > boards will have to define all the PMIC regulators, even if they're > not used at all on the board, just to have the right behaviour. And > that's clearly a no-go for me. Right the advantage of the dtsi is that it lasts ALL regulators, so that unused ones actually get turned off, where as a dts not using the dtsi and only containing those it actually uses will result in the unused regulators being untouched (since they are not listed) rather then turned off. Regards, Hans
On Sat, Apr 04, 2015 at 02:56:45PM +0200, Maxime Ripard wrote: > On Sat, Apr 04, 2015 at 01:33:02PM +0100, Mark Brown wrote: > > This is broken - think about what this means. If you are defining a > > voltage (or any other constraint) you're saying that it's safe to use on > > a given board. If you provide a voltage constraint saying that the > > maximum allowable range for a voltage regulator is safe. That's > > unlikely to be true on any given board, usually only a limited set of > > regulators can vary voltages at runtime safely at all and then rarely > > over their full supported range. Similarly for other constraints, for > > example allowing a regulator to be disabled when there are driverless > > things (or drivers without regulator support or mappings for that board) > > relying on it is going to break. > If this model is broken, then I don't see how the full regulator > support is not broken. > Our PMIC DTSI sets up the regulators with the ranges supported by the > PMIC. We have two cases then: > - Either the regulator is not used on the board, and it will be > disabled by the framework. Or the regulator is used by some passive component on the board that doesn't appear in DT (or some driver that doesn't yet have DT support doesn't claim the regulator) and it gets switched off. Or we power it off and then later discover that we've been placing physical stress on the system. > - Or the regulator is actually used on the board, it will be defined > in the DTS, possibly with additional constraints. Especially for voltage range constraints you're assuming that every system integrator will get everything right from their initial attempt to power up Linux onwards and will anticipate future enhancements to the drivers which try to vary voltages. For any given design you can pretty much guarantee that the electrical engineers would not approve of software making use of the maximum allowable voltage variation but that is what you're proposing to enable. That is clearly not a sensible or reasonable default, the best we can do is just not vary the voltage and let the system integrator say if it's safe to do so. Just listing the regulators is less likely to be an issue, it's more just not particularly useful than anything else, though it can potentially cause long term stresses too. Providing .dtsis for reference designs can make sense, if there's a widely cloned board with lots of common design elements then reusing that .dtsi is fine but clearly that .dtsi isn't going to enable the full flexibility of the regulators since there will be constraints from that reference design. > I really don't see how your above comments apply to that > situation. What I do see though, is that if we drop the DTSI, all our > boards will have to define all the PMIC regulators, even if they're > not used at all on the board, just to have the right behaviour. And > that's clearly a no-go for me. Nothing else is safe. Remember, if we get this wrong we risk system instability and at worst physical damage to the system either in rare cases immediately due to getting something badly wrong or more likely from long term electrical stresses. If what you were proposing were safe then we should still not be enumerating everything in device trees, we should just be enabling all operations for every regulator in the system by default. The fact that you have to override this should be a warning sign that you shouldn't be doing it.
On Sat, Apr 04, 2015 at 02:58:23PM +0200, Hans de Goede wrote: > regulators { > /* Default work frequency for buck regulators */ > x-powers,dcdc-freq = <1500>; Are you sure this is safe and correct for all boards (with most regulator designs I'd expect it to be dictated by passive choice)? > > reg_dcdc2: dcdc2 { > regulator-name = "dcdc2"; > }; These names should all be removed, they're vanishingly unlikely to reflect the schematic supply name (which is what the property is there for) and this should either be the same as the regulator API comes up with by default or override any better naming it manages to provide. > reg_ldo1: ldo1 { > /* LDO1 is a fixed output regulator */ > regulator-always-on; If there's no control this should be redundant. > regulator-min-microvolt = <1300000>; > regulator-max-microvolt = <1300000>; Are you *sure* this voltage is the one used on all boards? If it's the physically fixed output voltage then it shouldn't be in the constraints at all and the driver should just know.
On Sat, Apr 04, 2015 at 03:05:44PM +0200, Hans de Goede wrote: > On 04-04-15 14:56, Maxime Ripard wrote: > >What I do see though, is that if we drop the DTSI, all our > >boards will have to define all the PMIC regulators, even if they're > >not used at all on the board, just to have the right behaviour. And > >that's clearly a no-go for me. > Right the advantage of the dtsi is that it lasts ALL regulators, > so that unused ones actually get turned off, where as a dts not using > the dtsi and only containing those it actually uses will result in > the unused regulators being untouched (since they are not listed) > rather then turned off. Like I said in reply to Maxim a .dtsi does not seem the place to do this - if it's safe to do it we should be doing it in the core code.
On Sat, Apr 04, 2015 at 07:47:16PM +0100, Mark Brown wrote: > On Sat, Apr 04, 2015 at 02:56:45PM +0200, Maxime Ripard wrote: > > On Sat, Apr 04, 2015 at 01:33:02PM +0100, Mark Brown wrote: > > > > This is broken - think about what this means. If you are defining a > > > voltage (or any other constraint) you're saying that it's safe to use on > > > a given board. If you provide a voltage constraint saying that the > > > maximum allowable range for a voltage regulator is safe. That's > > > unlikely to be true on any given board, usually only a limited set of > > > regulators can vary voltages at runtime safely at all and then rarely > > > over their full supported range. Similarly for other constraints, for > > > example allowing a regulator to be disabled when there are driverless > > > things (or drivers without regulator support or mappings for that board) > > > relying on it is going to break. > > > If this model is broken, then I don't see how the full regulator > > support is not broken. > > > Our PMIC DTSI sets up the regulators with the ranges supported by the > > PMIC. We have two cases then: > > - Either the regulator is not used on the board, and it will be > > disabled by the framework. > > Or the regulator is used by some passive component on the board that > doesn't appear in DT (or some driver that doesn't yet have DT support > doesn't claim the regulator) and it gets switched off. Or we power it > off and then later discover that we've been placing physical stress on > the system. Isn't that what the regulator-always-on property is here for? > > - Or the regulator is actually used on the board, it will be defined > > in the DTS, possibly with additional constraints. > > Especially for voltage range constraints you're assuming that every > system integrator will get everything right from their initial attempt > to power up Linux onwards and will anticipate future enhancements to the > drivers which try to vary voltages. For any given design you can pretty > much guarantee that the electrical engineers would not approve of > software making use of the maximum allowable voltage variation but that > is what you're proposing to enable. That is clearly not a sensible or > reasonable default, the best we can do is just not vary the voltage and > let the system integrator say if it's safe to do so. > > Just listing the regulators is less likely to be an issue, it's more > just not particularly useful than anything else, though it can > potentially cause long term stresses too. My bad, that's actually just what we are doing, plus defining the regulators that are at a fixed voltage and can't be disabled. > Providing .dtsis for reference designs can make sense, if there's a > widely cloned board with lots of common design elements then reusing > that .dtsi is fine but clearly that .dtsi isn't going to enable the full > flexibility of the regulators since there will be constraints from that > reference design. That PMIC is used with various SoCs, so that's not really an option. And especially when it comes to regulators, I don't think there's really a pattern that emerged yet... > > I really don't see how your above comments apply to that > > situation. What I do see though, is that if we drop the DTSI, all our > > boards will have to define all the PMIC regulators, even if they're > > not used at all on the board, just to have the right behaviour. And > > that's clearly a no-go for me. > > Nothing else is safe. Remember, if we get this wrong we risk system > instability and at worst physical damage to the system either in rare > cases immediately due to getting something badly wrong or more likely > from long term electrical stresses. If what you were proposing were > safe then we should still not be enumerating everything in device trees, > we should just be enabling all operations for every regulator in the > system by default. The fact that you have to override this should be a > warning sign that you shouldn't be doing it. Well, we're doing low level stuff and (from experience) things like a poor muxing option can cause physical damage too, and we don't have any kind of protection or policy against that. So, what do you suggest for the kernel to have a behaviour independant of what the bootloader state was, without adding a lot of churn in each and every DTS? Maxime
On Tue, Apr 07, 2015 at 12:17:57PM +0200, Maxime Ripard wrote: > On Sat, Apr 04, 2015 at 07:47:16PM +0100, Mark Brown wrote: > > Or the regulator is used by some passive component on the board that > > doesn't appear in DT (or some driver that doesn't yet have DT support > > doesn't claim the regulator) and it gets switched off. Or we power it > > off and then later discover that we've been placing physical stress on > > the system. > Isn't that what the regulator-always-on property is here for? Only if it's actually added by someone after having looked at the system... > > Providing .dtsis for reference designs can make sense, if there's a > > widely cloned board with lots of common design elements then reusing > > that .dtsi is fine but clearly that .dtsi isn't going to enable the full > > flexibility of the regulators since there will be constraints from that > > reference design. > That PMIC is used with various SoCs, so that's not really an > option. And especially when it comes to regulators, I don't think > there's really a pattern that emerged yet... I'm not sure you saw the "reference design" part of the above? > > Nothing else is safe. Remember, if we get this wrong we risk system > > instability and at worst physical damage to the system either in rare > > cases immediately due to getting something badly wrong or more likely > > from long term electrical stresses. If what you were proposing were > > safe then we should still not be enumerating everything in device trees, > > we should just be enabling all operations for every regulator in the > > system by default. The fact that you have to override this should be a > > warning sign that you shouldn't be doing it. > Well, we're doing low level stuff and (from experience) things like a > poor muxing option can cause physical damage too, and we don't have > any kind of protection or policy against that. Does the pinmux code actively go in and do things without being explicitly configured? > So, what do you suggest for the kernel to have a behaviour independant > of what the bootloader state was, without adding a lot of churn in > each and every DTS? Well, I'm fairly happy with the current state. I think having some sort of board level property saying that undescribed regulators can be enabled and disabled (which I think is all that this is really doing) might be a viable solution?
On Wed, Apr 08, 2015 at 01:35:06PM +0100, Mark Brown wrote: > On Tue, Apr 07, 2015 at 12:17:57PM +0200, Maxime Ripard wrote: > > On Sat, Apr 04, 2015 at 07:47:16PM +0100, Mark Brown wrote: > > > > Or the regulator is used by some passive component on the board that > > > doesn't appear in DT (or some driver that doesn't yet have DT support > > > doesn't claim the regulator) and it gets switched off. Or we power it > > > off and then later discover that we've been placing physical stress on > > > the system. > > > Isn't that what the regulator-always-on property is here for? > > Only if it's actually added by someone after having looked at the > system... True, but that can also be said for pretty much any DT patch. If you do something wrong in your DT and haven't looked carefuly at your datasheet/schematics, here be dragons. > > > Providing .dtsis for reference designs can make sense, if there's a > > > widely cloned board with lots of common design elements then reusing > > > that .dtsi is fine but clearly that .dtsi isn't going to enable the full > > > flexibility of the regulators since there will be constraints from that > > > reference design. > > > That PMIC is used with various SoCs, so that's not really an > > option. And especially when it comes to regulators, I don't think > > there's really a pattern that emerged yet... > > I'm not sure you saw the "reference design" part of the above? I saw that, I was just saying that this doesn't really work for us unfortunately. > > > Nothing else is safe. Remember, if we get this wrong we risk system > > > instability and at worst physical damage to the system either in rare > > > cases immediately due to getting something badly wrong or more likely > > > from long term electrical stresses. If what you were proposing were > > > safe then we should still not be enumerating everything in device trees, > > > we should just be enabling all operations for every regulator in the > > > system by default. The fact that you have to override this should be a > > > warning sign that you shouldn't be doing it. > > > Well, we're doing low level stuff and (from experience) things like a > > poor muxing option can cause physical damage too, and we don't have > > any kind of protection or policy against that. > > Does the pinmux code actively go in and do things without being > explicitly configured? You have a point :) > > So, what do you suggest for the kernel to have a behaviour > > independant of what the bootloader state was, without adding a lot > > of churn in each and every DTS? > > Well, I'm fairly happy with the current state. I think having some > sort of board level property saying that undescribed regulators can > be enabled and disabled (which I think is all that this is really > doing) might be a viable solution? It looks like configuration to me ;) Especially since the semantic of it would be to disable something that is not even declared in the DT in the first place. Maxime
On Mon, Apr 13, 2015 at 10:15:18AM +0200, Maxime Ripard wrote: > On Wed, Apr 08, 2015 at 01:35:06PM +0100, Mark Brown wrote: > > On Tue, Apr 07, 2015 at 12:17:57PM +0200, Maxime Ripard wrote: > > > On Sat, Apr 04, 2015 at 07:47:16PM +0100, Mark Brown wrote: > > > > Or the regulator is used by some passive component on the board that > > > > doesn't appear in DT (or some driver that doesn't yet have DT support > > > > doesn't claim the regulator) and it gets switched off. Or we power it > > > > off and then later discover that we've been placing physical stress on > > > > the system. > > > Isn't that what the regulator-always-on property is here for? > > Only if it's actually added by someone after having looked at the > > system... > True, but that can also be said for pretty much any DT patch. If you > do something wrong in your DT and haven't looked carefuly at your > datasheet/schematics, here be dragons. There's a difference between actively choosing to do something and having it done by default - the goal here is to make sure that if someone didn't even look at something we're as safe as possible. > > > > Providing .dtsis for reference designs can make sense, if there's a > > > > widely cloned board with lots of common design elements then reusing > > > > that .dtsi is fine but clearly that .dtsi isn't going to enable the full > > > > flexibility of the regulators since there will be constraints from that > > > > reference design. > > > That PMIC is used with various SoCs, so that's not really an > > > option. And especially when it comes to regulators, I don't think > > > there's really a pattern that emerged yet... > > I'm not sure you saw the "reference design" part of the above? > I saw that, I was just saying that this doesn't really work for us > unfortunately. I really don't understand how your explanation follows at all, sorry. How does the ability to use the same PMIC with multiple SoCs influence reference designs?
On Mon, Apr 13, 2015 at 12:44:28PM +0100, Mark Brown wrote: > On Mon, Apr 13, 2015 at 10:15:18AM +0200, Maxime Ripard wrote: > > On Wed, Apr 08, 2015 at 01:35:06PM +0100, Mark Brown wrote: > > > On Tue, Apr 07, 2015 at 12:17:57PM +0200, Maxime Ripard wrote: > > > > On Sat, Apr 04, 2015 at 07:47:16PM +0100, Mark Brown wrote: > > > > > > Or the regulator is used by some passive component on the board that > > > > > doesn't appear in DT (or some driver that doesn't yet have DT support > > > > > doesn't claim the regulator) and it gets switched off. Or we power it > > > > > off and then later discover that we've been placing physical stress on > > > > > the system. > > > > > Isn't that what the regulator-always-on property is here for? > > > > Only if it's actually added by someone after having looked at the > > > system... > > > True, but that can also be said for pretty much any DT patch. If you > > do something wrong in your DT and haven't looked carefuly at your > > datasheet/schematics, here be dragons. > > There's a difference between actively choosing to do something and > having it done by default - the goal here is to make sure that if > someone didn't even look at something we're as safe as possible. I don't think that discussion is getting anywhere. I do understand your point, but I also am under the impression that in order to allow people to get the regulators wrong, we place the maintainance burden on those who get it right... > > > > > Providing .dtsis for reference designs can make sense, if there's a > > > > > widely cloned board with lots of common design elements then reusing > > > > > that .dtsi is fine but clearly that .dtsi isn't going to enable the full > > > > > flexibility of the regulators since there will be constraints from that > > > > > reference design. > > > > > That PMIC is used with various SoCs, so that's not really an > > > > option. And especially when it comes to regulators, I don't think > > > > there's really a pattern that emerged yet... > > > > I'm not sure you saw the "reference design" part of the above? > > > I saw that, I was just saying that this doesn't really work for us > > unfortunately. > > I really don't understand how your explanation follows at all, sorry. > How does the ability to use the same PMIC with multiple SoCs influence > reference designs? Maybe we don't have the same definition for a reference design then, but to me, it's a board that is used as a starting point for other vendors to create their boards. If so, the central piece of this board is actually the SoC. Having multiple SoCs will prevent any sharing of the DTSI, won't it? And like I already said, we're not in such a case anyway. Pretty much all boards we've seen so far wire the regulators differently. Maxime
On Tue, Apr 14, 2015 at 06:27:38PM +0200, Maxime Ripard wrote: > On Mon, Apr 13, 2015 at 12:44:28PM +0100, Mark Brown wrote: > > There's a difference between actively choosing to do something and > > having it done by default - the goal here is to make sure that if > > someone didn't even look at something we're as safe as possible. > I don't think that discussion is getting anywhere. > I do understand your point, but I also am under the impression that in > order to allow people to get the regulators wrong, we place the > maintainance burden on those who get it right... Someone is going to be unhappy no matter which default we pick and it seems better that this is the person who had to type a bit more rather than the person with hardware damage (or the person who finds their board suddenly doesn't work because they changed a config option for that matter). > > I really don't understand how your explanation follows at all, sorry. > > How does the ability to use the same PMIC with multiple SoCs influence > > reference designs? > Maybe we don't have the same definition for a reference design then, > but to me, it's a board that is used as a starting point for other > vendors to create their boards. Right. > If so, the central piece of this board is actually the SoC. Having > multiple SoCs will prevent any sharing of the DTSI, won't it? If the boards have different SoCs they're probably not the same reference design, unless they're one of the fully pluggable reference designs which really don't work well with DT at the best of times. The point is that the DTSI for a reference design is for that design not for the SoC, so for example a DTSI for BeagleBone Black derivatives rather than a DTSI for the SoC or PMIC on it. > And like I already said, we're not in such a case anyway. Pretty much > all boards we've seen so far wire the regulators differently. OK, so that's the sort of case where this is going to be useful.
diff --git a/arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts b/arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts index 31dc2f1..f7a8fcc 100644 --- a/arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts +++ b/arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts @@ -67,6 +67,15 @@ default-state = "on"; }; }; + + reg_axp_ipsout: axp_ipsout { + compatible = "regulator-fixed"; + regulator-name = "axp-ipsout"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + regulator-always-on; + }; + }; &ahci { @@ -97,10 +106,69 @@ axp209: pmic@34 { compatible = "x-powers,axp209"; reg = <0x34>; + interrupt-parent = <&intc>; interrupts = <0>; interrupt-controller; #interrupt-cells = <1>; + + acin-supply = <®_axp_ipsout>; + vin2-supply = <®_axp_ipsout>; + vin3-supply = <®_axp_ipsout>; + ldo24in-supply = <®_axp_ipsout>; + ldo3in-supply = <®_axp_ipsout>; + + x-powers,dcdc-freq = <1500>; + + regulators { + vdd_rtc: ldo1 { + regulator-min-microvolt = <1300000>; + regulator-max-microvolt = <1300000>; + regulator-always-on; + regulator-name="vdd_rtc"; + }; + + avcc: ldo2 { + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <3000000>; + regulator-always-on; + regulator-name="avcc"; + }; + + vcc_csi0: ldo3 { + regulator-min-microvolt = <2800000>; + regulator-max-microvolt = <2800000>; + regulator-always-on; + regulator-name="vcc_csi0"; + }; + + vcc_csi1: ldo4 { + regulator-min-microvolt = <2800000>; + regulator-max-microvolt = <2800000>; + regulator-always-on; + regulator-name="vcc_csi1"; + }; + + ldo5 { + regulator-min-microvolt = <2800000>; + regulator-max-microvolt = <2800000>; + regulator-name="reg_unused"; + }; + + vdd_cpu: dcdc2 { + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <1550000>; + regulator-always-on; + regulator-name="vdd_cpu"; + }; + + vdd_int: dcdc3 { + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <1250000>; + regulator-always-on; + regulator-name="vdd_int"; + }; + }; }; };
add pmic regulator definitions matching the manufacturers 3.4.x fex file. Signed-off-by: Iain Paton <ipaton0@gmail.com> --- As this file belongs to Hans and he decided not to use axp209.dtsi in http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315612.html then I won't add it here. arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts | 68 ++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)