| Message ID | 1342766789-28148-2-git-send-email-anilkumar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 20, 2012 at 12:16:26PM +0530, AnilKumar Ch wrote: > + vio_reg: regulator@1 { > + reg = <1>; > + regulator-compatible = "vio"; > + regulator-min-microvolt = <1500000>; > + regulator-max-microvolt = <3300000>; > + regulator-always-on; > + }; Every regulator here has a rather large voltage range specified with no consumers added. Are you sure these voltage ranges make sense in your design and you've not just cut'n'pasted the entire voltage range that your regulator supports without reference to what your board can do?
Hi Mark, Thanks for the review. On Fri, Jul 20, 2012 at 15:29:36, Mark Brown wrote: > On Fri, Jul 20, 2012 at 12:16:26PM +0530, AnilKumar Ch wrote: > > > + vio_reg: regulator@1 { > > + reg = <1>; > > + regulator-compatible = "vio"; > > + regulator-min-microvolt = <1500000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-always-on; > > + }; > > Every regulator here has a rather large voltage range specified with no > consumers added. Are you sure these voltage ranges make sense in your > design and you've not just cut'n'pasted the entire voltage range that > your regulator supports without reference to what your board can do? > tps65217.dtsi is a generic file to be used by the SoCs so these constraints were taken from the regulator itself. SoC specific limits can be added in SoC specific .dts file to tighten the constraints to require limit. I have tested the driver with this approach. Required consumers will be added while submitting the regulator dependent Consumers. One of the usecase I can specify here is MPU voltage control through DVFS framework. So if require, MPU consumer will be added along with the DVFS support. Thanks AnilKumar
On Fri, Jul 20, 2012 at 11:27:36AM +0000, AnilKumar, Chimata wrote: > On Fri, Jul 20, 2012 at 15:29:36, Mark Brown wrote: > > Every regulator here has a rather large voltage range specified with no > > consumers added. Are you sure these voltage ranges make sense in your > > design and you've not just cut'n'pasted the entire voltage range that > > your regulator supports without reference to what your board can do? > tps65217.dtsi is a generic file to be used by the SoCs so these constraints > were taken from the regulator itself. SoC specific limits can be added in > SoC specific .dts file to tighten the constraints to require limit. I have > tested the driver with this approach. No, this is not a sane approach. You've no idea if any of these settings are safe or sane for the board. Boards should enable things they know are safe, not remove those they know are broken.
On Fri, Jul 20, 2012 at 17:08:06, Mark Brown wrote: > On Fri, Jul 20, 2012 at 11:27:36AM +0000, AnilKumar, Chimata wrote: > > On Fri, Jul 20, 2012 at 15:29:36, Mark Brown wrote: > > > > Every regulator here has a rather large voltage range specified with no > > > consumers added. Are you sure these voltage ranges make sense in your > > > design and you've not just cut'n'pasted the entire voltage range that > > > your regulator supports without reference to what your board can do? > > > tps65217.dtsi is a generic file to be used by the SoCs so these constraints > > were taken from the regulator itself. SoC specific limits can be added in > > SoC specific .dts file to tighten the constraints to require limit. I have > > tested the driver with this approach. > > No, this is not a sane approach. You've no idea if any of these > settings are safe or sane for the board. Boards should enable things > they know are safe, not remove those they know are broken. > Unsterstood, I will send v2 with constraints updated. Regards AnilKumar
On Mon, Jul 23, 2012 at 12:36:48, AnilKumar, Chimata wrote: > On Fri, Jul 20, 2012 at 17:08:06, Mark Brown wrote: > > On Fri, Jul 20, 2012 at 11:27:36AM +0000, AnilKumar, Chimata wrote: > > > On Fri, Jul 20, 2012 at 15:29:36, Mark Brown wrote: > > > > > > Every regulator here has a rather large voltage range specified with no > > > > consumers added. Are you sure these voltage ranges make sense in your > > > > design and you've not just cut'n'pasted the entire voltage range that > > > > your regulator supports without reference to what your board can do? > > > > > tps65217.dtsi is a generic file to be used by the SoCs so these constraints > > > were taken from the regulator itself. SoC specific limits can be added in > > > SoC specific .dts file to tighten the constraints to require limit. I have > > > tested the driver with this approach. > > > > No, this is not a sane approach. You've no idea if any of these > > settings are safe or sane for the board. Boards should enable things > > they know are safe, not remove those they know are broken. > > > > Unsterstood, I will send v2 with constraints updated. > By the way, if we look at all the regulator added (DT supported) till now have the similar problem. arch/arm/boot/dts/imx6q.dtsi arch/arm/boot/dts/twl4030.dtsi arch/arm/boot/dts/twl6030.dtsi arch/arm/boot/dts/db8500.dtsi Regards AnilKumar
On Mon, Jul 23, 2012 at 01:23:50PM +0000, AnilKumar, Chimata wrote: > By the way, if we look at all the regulator added (DT supported) till now have > the similar problem. > arch/arm/boot/dts/imx6q.dtsi This is fine - the SoC contains integrated regulators which supply other bits of the Soc so we can be confident that the hookup is good just based on the silicon. > arch/arm/boot/dts/twl4030.dtsi > arch/arm/boot/dts/twl6030.dtsi These appear to have similar issues and should be fixed, at least as far as the voltage ranges go. > arch/arm/boot/dts/db8500.dtsi I'm not actually seeing anything terribly problematic here, though the regulator-name properties should really be removed as they're fairly useless and seem to be missing the point of having the property.
On 23/07/12 14:34, Mark Brown wrote: > On Mon, Jul 23, 2012 at 01:23:50PM +0000, AnilKumar, Chimata wrote: > >> By the way, if we look at all the regulator added (DT supported) till now have >> the similar problem. > >> arch/arm/boot/dts/imx6q.dtsi > > This is fine - the SoC contains integrated regulators which supply other > bits of the Soc so we can be confident that the hookup is good just > based on the silicon. > >> arch/arm/boot/dts/twl4030.dtsi >> arch/arm/boot/dts/twl6030.dtsi > > These appear to have similar issues and should be fixed, at least as far > as the voltage ranges go. > >> arch/arm/boot/dts/db8500.dtsi > > I'm not actually seeing anything terribly problematic here, though the > regulator-name properties should really be removed as they're fairly > useless and seem to be missing the point of having the property. I've missed the context of the thread, so can't comment, but I'm happy to remove the regulator-name properties from db8500.dts. Adding to my TODO.
Hi Mark, > > arch/arm/boot/dts/db8500.dtsi > > I'm not actually seeing anything terribly problematic here, though the > regulator-name properties should really be removed as they're fairly > useless and seem to be missing the point of having the property. Just looking at this now. The regulator-name property is used to populate constrains->name. Are you sure you still want them all removed? Kind regards, Lee
On Tue, Aug 28, 2012 at 12:26:07PM +0100, Lee Jones wrote: > > > arch/arm/boot/dts/db8500.dtsi > > I'm not actually seeing anything terribly problematic here, though the > > regulator-name properties should really be removed as they're fairly > > useless and seem to be missing the point of having the property. > Just looking at this now. > The regulator-name property is used to populate constrains->name. Are > you sure you still want them all removed? Yes, of course. There's no way that a generic .dtsi used for any possible board could come up with a sensible value.
On Tue, Aug 28, 2012 at 10:21:33AM -0700, Mark Brown wrote: > On Tue, Aug 28, 2012 at 12:26:07PM +0100, Lee Jones wrote: > > > > > arch/arm/boot/dts/db8500.dtsi > > > > I'm not actually seeing anything terribly problematic here, though the > > > regulator-name properties should really be removed as they're fairly > > > useless and seem to be missing the point of having the property. > > > Just looking at this now. > > > The regulator-name property is used to populate constrains->name. Are > > you sure you still want them all removed? > > Yes, of course. There's no way that a generic .dtsi used for any > possible board could come up with a sensible value. So how should constrains->name be populated then? Would you prefer regulator-names moved to the .dts file(s), or something else?
On Wed, Aug 29, 2012 at 09:31:31AM +0100, Lee Jones wrote: > On Tue, Aug 28, 2012 at 10:21:33AM -0700, Mark Brown wrote: > > > The regulator-name property is used to populate constrains->name. Are > > > you sure you still want them all removed? > > Yes, of course. There's no way that a generic .dtsi used for any > > possible board could come up with a sensible value. > So how should constrains->name be populated then? Would you prefer > regulator-names moved to the .dts file(s), or something else? Of course, yes. The sole purpose of that field is to give a board specific name to the supply. It can't usefully be set by anything except the board.
diff --git a/arch/arm/boot/dts/tps65910.dtsi b/arch/arm/boot/dts/tps65910.dtsi new file mode 100644 index 0000000..b185235 --- /dev/null +++ b/arch/arm/boot/dts/tps65910.dtsi @@ -0,0 +1,123 @@ +/* + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +/* + * Integrated Power Management Chip + * http://www.ti.com/lit/ds/symlink/tps65910.pdf + */ + +&tps { + compatible = "ti,tps65910"; + + regulators { + #address-cells = <1>; + #size-cells = <0>; + + vrtc_reg: regulator@0 { + reg = <0>; + regulator-compatible = "vrtc"; + regulator-always-on; + }; + + vio_reg: regulator@1 { + reg = <1>; + regulator-compatible = "vio"; + regulator-min-microvolt = <1500000>; + regulator-max-microvolt = <3300000>; + regulator-always-on; + }; + + vdd1_reg: regulator@2 { + reg = <2>; + regulator-compatible = "vdd1"; + regulator-min-microvolt = <600000>; + regulator-max-microvolt = <1500000>; + regulator-boot-on; + regulator-always-on; + }; + + vdd2_reg: regulator@3 { + reg = <3>; + regulator-compatible = "vdd2"; + regulator-min-microvolt = <600000>; + regulator-max-microvolt = <1500000>; + regulator-boot-on; + regulator-always-on; + }; + + vdd3_reg: regulator@4 { + reg = <4>; + regulator-compatible = "vdd3"; + regulator-always-on; + }; + + vdig1_reg: regulator@5 { + reg = <5>; + regulator-compatible = "vdig1"; + regulator-min-microvolt = <1200000>; + regulator-max-microvolt = <2700000>; + regulator-always-on; + }; + + vdig2_reg: regulator@6 { + reg = <6>; + regulator-compatible = "vdig2"; + regulator-min-microvolt = <1000000>; + regulator-max-microvolt = <1800000>; + regulator-always-on; + }; + + vpll_reg: regulator@7 { + reg = <7>; + regulator-compatible = "vpll"; + regulator-min-microvolt = <1000000>; + regulator-max-microvolt = <2500000>; + regulator-always-on; + }; + + vdac_reg: regulator@8 { + reg = <8>; + regulator-compatible = "vdac"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <2850000>; + regulator-always-on; + }; + + vaux1_reg: regulator@9 { + reg = <9>; + regulator-compatible = "vaux1"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <2850000>; + regulator-always-on; + }; + + vaux2_reg: regulator@10 { + reg = <10>; + regulator-compatible = "vaux2"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + regulator-always-on; + }; + + vaux33_reg: regulator@11 { + reg = <11>; + regulator-compatible = "vaux33"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + regulator-always-on; + }; + + vmmc_reg: regulator@12 { + reg = <12>; + regulator-compatible = "vmmc"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + regulator-always-on; + }; + }; +};
Add device tree data for tps65910 regulator by adding all the consumers necessary for AM335X-EVM. The data will be map to a regulator constraints which is required for regulator set_voltage and get_voltage calls. All tps65910 PMIC regulator constraints are placed in a seperate device tree include file (tps65910.dtsi). This patch is tested by adding the I2C slave address of TPS65910 pmic to am335x-evm.dts file (Not included in this, I2C slave addition patch will be submitted to linux-omap, where am335x-evm.dts binding file is available). Signed-off-by: AnilKumar Ch <anilkumar@ti.com> --- arch/arm/boot/dts/tps65910.dtsi | 123 +++++++++++++++++++++++++++++++++++++++ 1 files changed, 123 insertions(+), 0 deletions(-) create mode 100644 arch/arm/boot/dts/tps65910.dtsi