| Message ID | 1407861868-20097-7-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 12, 2014 at 06:44:28PM +0200, Javier Martinez Canillas wrote: > The tps65090 PMU data manual [0] has a table that list the > "Recommended operating conditions" for each regulator. Add > the information about the FET constraints to its dtsi file. > tps65090_fet1: fet1 { > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <17000000>; > }; No, this is completely broken and exactly the sort of thing that makes doing generic device .dtsi a bad idea. We have absolutely no idea if these voltage ranges are suitable for use on a given board using the device, these are the design limits for the device but tell us nothing about the system they are deployed in.
On 08/12/2014 07:25 PM, Mark Brown wrote: > >> tps65090_fet1: fet1 { >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <17000000>; >> }; > > No, this is completely broken and exactly the sort of thing that makes > doing generic device .dtsi a bad idea. We have absolutely no idea if > these voltage ranges are suitable for use on a given board using the > device, these are the design limits for the device but tell us nothing > about the system they are deployed in. > Thanks for the explanation. I did the refactoring because I saw that there are .dtsi files for other PMICs already (twl4030, twl6030, tps65*) but now you also explained that those are broken as well. So, is adding these voltages ranges (the design limits) in the Peach Pit DTS file directly an acceptable solution? Basically what my previous patch [0] did. That matches what is in the board schematic so I assume that it's safe to use these voltage ranges for that machine. If so I'll drop this series and repost that patch fixing the typo error and commit message pointed by Doug that were already addressed in $subject. Best regards, Javier [0]: https://lkml.org/lkml/2014/8/11/204
On Tue, Aug 12, 2014 at 08:49:29PM +0200, Javier Martinez Canillas wrote: > So, is adding these voltages ranges (the design limits) in the Peach Pit DTS > file directly an acceptable solution? Basically what my previous patch [0] did. > That matches what is in the board schematic so I assume that it's safe to use > these voltage ranges for that machine. If so I'll drop this series and repost > that patch fixing the typo error and commit message pointed by Doug that were > already addressed in $subject. Well, I think the question is if you understand where those numbers come from and if they make sense. I've not seen the schematic for the board so I can't comment but it is quite unusual to see ranges listed for so many supplies, for things like SD it's obviously normal but things like video_mid are a bit more surprising. Does anything actually vary those voltages? The change where you fix up the supply mappings still makes sense of course.
Hello Mark, On 08/12/2014 11:27 PM, Mark Brown wrote: > On Tue, Aug 12, 2014 at 08:49:29PM +0200, Javier Martinez Canillas wrote: > >> So, is adding these voltages ranges (the design limits) in the Peach Pit DTS >> file directly an acceptable solution? Basically what my previous patch [0] did. > >> That matches what is in the board schematic so I assume that it's safe to use >> these voltage ranges for that machine. If so I'll drop this series and repost >> that patch fixing the typo error and commit message pointed by Doug that were >> already addressed in $subject. > > Well, I think the question is if you understand where those numbers come > from and if they make sense. I've not seen the schematic for the board > so I can't comment but it is quite unusual to see ranges listed for so > many supplies, for things like SD it's obviously normal but things like > video_mid are a bit more surprising. Does anything actually vary those > voltages? > You are right, in fact even the voltage for the SD supply (tps65090 fet4) does not change but still their constraints have to be defined, since it is used as a vmmc-supply and the series "Adding UHS support for dw_mmc driver" [0] calls mmc_regulator_get_ocrmask()/mmc_regulator_set_ocr() for mmc->supply.vmmc. For fixed regulators (like fet4), mmc_regulator_set_ocr() just skips varying the regulator voltage but still expects to be able to obtain its voltage. Since the FET is just a switch and doesn't have an output voltage, the regulator core gets its parent supply voltage but regulator_list_voltge() [1] checks that the obtained parent voltage is between the child regulator constraints. So in this particular case it makes sense to compare that the parent voltage is between the (design limits) voltage ranges for the FET but I agree that it may not make sense on other boards. Since I needed to add the ranges for fet4 I (wrongly) thought that it would make sense to add the ranges for all the other FETs in case other boards had a similar need. But now I understand your concerns about this series so I'll drop it and just post a patch that adds to the Peach Pit and Pi DTS, the constraints for the fet4 used as vmmc-supply instead of pretending that it's a common setup. > The change where you fix up the supply mappings still makes sense of > course. > Good to know, again thanks a lot for your feedback and explanations. Best regards, Javier [0]: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/265657.html [1]: https://git.kernel.org/cgit/linux/kernel/git/broonie/regulator.git/tree/drivers/regulator/core.c?h=for-next#n2209
On Wed, Aug 13, 2014 at 01:31:44PM +0200, Javier Martinez Canillas wrote: Please fix your mailer to word wrap at less than 80 columns, it makes your mails very hard to read when replying. > On 08/12/2014 11:27 PM, Mark Brown wrote: > > Well, I think the question is if you understand where those numbers come > > from and if they make sense. I've not seen the schematic for the board > > so I can't comment but it is quite unusual to see ranges listed for so > > many supplies, for things like SD it's obviously normal but things like > > video_mid are a bit more surprising. Does anything actually vary those > > voltages? > You are right, in fact even the voltage for the SD supply (tps65090 fet4) does > not change but still their constraints have to be defined, since it is used as a > vmmc-supply and the series "Adding UHS support for dw_mmc driver" [0] calls > mmc_regulator_get_ocrmask()/mmc_regulator_set_ocr() for mmc->supply.vmmc. No, that makes no sense. If the voltage isn't allowed to change why should the constraints be specifying a range of voltages for it to be set to, especially a range with more than one value in it? Please try to think about what you're doing in design terms rather than just bashing on things until you get something that works in your use case, it's important that things are understandable so that we avoid fragility and special casing. > For fixed regulators (like fet4), mmc_regulator_set_ocr() just skips varying the > regulator voltage but still expects to be able to obtain its voltage. Which can be done with regulator_get_voltage().
On 08/13/2014 02:29 PM, Mark Brown wrote: > > Please fix your mailer to word wrap at less than 80 columns, it makes > your mails very hard to read when replying. > Sorry I had my wrap length configured to 80 but just changed to 74 now. >> On 08/12/2014 11:27 PM, Mark Brown wrote: > > No, that makes no sense. If the voltage isn't allowed to change why > should the constraints be specifying a range of voltages for it to be > set to, especially a range with more than one value in it? > > Please try to think about what you're doing in design terms rather than > just bashing on things until you get something that works in your use > case, it's important that things are understandable so that we avoid > fragility and special casing. > >> For fixed regulators (like fet4), mmc_regulator_set_ocr() just skips >> varying the regulator voltage but still expects to be able to obtain >> its voltage. > > Which can be done with regulator_get_voltage(). > Indeed. I'll change mmc_regulator_get_ocrmask() in MMC core then to use regulator_can_change_voltage() to detect if the regulator is a fixed one and call regulator_get_voltage() instead of list_voltage() in that case. Do you agree that this is the correct solution? Best regards, Javier
On Wed, Aug 13, 2014 at 03:34:12PM +0200, Javier Martinez Canillas wrote: > Indeed. I'll change mmc_regulator_get_ocrmask() in MMC core then to use > regulator_can_change_voltage() to detect if the regulator is a fixed one > and call regulator_get_voltage() instead of list_voltage() in that case. > Do you agree that this is the correct solution? I would just fall back to get_voltage() if there are no voltages listed, that seems more robust.
On 08/12/2014 10:44 AM, Javier Martinez Canillas wrote: > The tps65090 PMU data manual [0] has a table that list the > "Recommended operating conditions" for each regulator. Add > the information about the FET constraints to its dtsi file. > > [0]: http://www.ti.com/lit/ds/symlink/tps65090.pdf I'm worried that this file represents the limits of the PMIC itself, whereas the DT should be representing the limits of the circuits that the various PMIC regulators are attached to on the board. For example: > diff --git a/arch/arm/boot/dts/tps65090.dtsi b/arch/arm/boot/dts/tps65090.dtsi > tps65090_fet3: fet3 { > + regulator-min-microvolt = <3000000>; > + regulator-max-microvolt = <5500000>; > }; I guess that on some boards, this output rail might be attached to devices that must run at 3.3V exactly, and on other boards it might be attached to devices that must run at 5V exactly. The DT for those two boards should each have regulator-{min,max}-microvolt set to the same value, which describes the board requirements. It feels dangerous/misleading to define the PMIC range by default. It might lead people to think that since the property already has a defined value, they don't need to think about what the correct value for their board is, and hence not change the value in their board file.
Hello Mark, On 08/13/2014 05:51 PM, Mark Brown wrote: > On Wed, Aug 13, 2014 at 03:34:12PM +0200, Javier Martinez Canillas wrote: > >> Indeed. I'll change mmc_regulator_get_ocrmask() in MMC core then to use >> regulator_can_change_voltage() to detect if the regulator is a fixed one >> and call regulator_get_voltage() instead of list_voltage() in that case. > >> Do you agree that this is the correct solution? > > I would just fall back to get_voltage() if there are no voltages listed, > that seems more robust. > Great, thanks a lot for your suggestion and all the help. Best regards, Javier
Hello Stephen, On 08/13/2014 06:16 PM, Stephen Warren wrote: > > I'm worried that this file represents the limits of the PMIC itself, > whereas the DT should be representing the limits of the circuits that > the various PMIC regulators are attached to on the board. > > For example: > >> diff --git a/arch/arm/boot/dts/tps65090.dtsi b/arch/arm/boot/dts/tps65090.dtsi > >> tps65090_fet3: fet3 { >> + regulator-min-microvolt = <3000000>; >> + regulator-max-microvolt = <5500000>; >> }; > > I guess that on some boards, this output rail might be attached to > devices that must run at 3.3V exactly, and on other boards it might be > attached to devices that must run at 5V exactly. The DT for those two > boards should each have regulator-{min,max}-microvolt set to the same > value, which describes the board requirements. > > It feels dangerous/misleading to define the PMIC range by default. It > might lead people to think that since the property already has a defined > value, they don't need to think about what the correct value for their > board is, and hence not change the value in their board file. > Yes, Mark already explained to me why this approach was broken so I've dropped the whole series. Thanks a lot for your feedback. Best regards, Javier
diff --git a/arch/arm/boot/dts/tps65090.dtsi b/arch/arm/boot/dts/tps65090.dtsi index a2ff534..60c56da 100644 --- a/arch/arm/boot/dts/tps65090.dtsi +++ b/arch/arm/boot/dts/tps65090.dtsi @@ -24,30 +24,48 @@ }; tps65090_fet1: fet1 { + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <17000000>; }; tps65090_fet2: fet2 { + regulator-min-microvolt = <4500000>; + regulator-max-microvolt = <5500000>; }; tps65090_fet3: fet3 { + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <5500000>; }; tps65090_fet4: fet4 { + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <5500000>; }; tps65090_fet5: fet5 { + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <5500000>; }; tps65090_fet6: fet6 { + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <5500000>; }; tps65090_fet7: fet7 { + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <5500000>; }; tps65090_ldo1: ldo1 { + regulator-min-microvolt = <6000000>; + regulator-max-microvolt = <17000000>; }; tps65090_ldo2: ldo2 { + regulator-min-microvolt = <6000000>; + regulator-max-microvolt = <17000000>; }; };
The tps65090 PMU data manual [0] has a table that list the "Recommended operating conditions" for each regulator. Add the information about the FET constraints to its dtsi file. [0]: http://www.ti.com/lit/ds/symlink/tps65090.pdf Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- arch/arm/boot/dts/tps65090.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)