From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
To: Doug Anderson <dianders@chromium.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>,
Olof Johansson <olof@lixom.net>,
Yuvaraj Kumar C D <yuvaraj.cd@gmail.com>,
Mark Brown <broonie@kernel.org>,
linux-samsung-soc <linux-samsung-soc@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RESEND PATCH 2/2] ARM: dts: Add tps65090 FET constraints on Peach Pit and Pi
Date: Mon, 11 Aug 2014 19:47:18 +0200 [thread overview]
Message-ID: <53E901A6.2020902@collabora.co.uk> (raw)
In-Reply-To: <CAD=FV=XRmNkxXoxWaEfxFADrTgBrOPVX746woxiZoKck1ajwgA@mail.gmail.com>
Hello Doug,
On 08/11/2014 05:57 PM, Doug Anderson wrote:
> Javier,
>
> On Mon, Aug 11, 2014 at 4:38 AM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>> Both Exynos5420 Peach Pit and Exynos5800 Peach Pi boards
>> have a tps65090 PMU that has a number of switches (FETs)
>> that are just on/off devices but they do have a current
>> limit and the output voltage of the switch is ramped up
>> within a controlled slope.
>>
>> After the switch is turned on, a safety timer is started
>> and before this timer times out the output voltage must
>> have reached the input voltage. Otherwise the switch is
>> turned off expecting an overload condition.
>>
>> So using the maximum output voltage slew rate and the timer
>> minimum and maximum timeouts, a voltage constraints can be
>> expressed as bounded limits for the timeout. That is what
>> is used in the board schematics and should be in the DT too.
>
> I don't understand this, but if you and Mark are happy with it...
>
> ...I'm also not 100% certain what the above description has to do with
> this change, but I'll admit to having only skimmed some of the earlier
> conversations.
>
As I stated before, I wrongly assumed from where the constraints in the
schematics came from. From the latest doc I now know that there is a
"recommended operating conditions table". I'll fix it in v2, sorry for the
confusion...
>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>> arch/arm/boot/dts/exynos5420-peach-pit.dts | 14 ++++++++++++++
>> arch/arm/boot/dts/exynos5800-peach-pi.dts | 14 ++++++++++++++
>> 2 files changed, 28 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> index d8710c1..eefafe6 100644
>> --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> @@ -386,27 +386,41 @@
>> };
>> tps65090_fet1: fet1 {
>> regulator-name = "vcd_led";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <1700000>;
>
> This is almost certainly wrong. Your max is smaller than your min.
> Perhaps you want an extra 0.
>
>> };
>> tps65090_fet2: fet2 {
>> regulator-name = "video_mid";
>> + regulator-min-microvolt = <4500000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet3: fet3 {
>> regulator-name = "wwan_r";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet4: fet4 {
>> regulator-name = "sdcard";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet5: fet5 {
>> regulator-name = "camout";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> };
>> tps65090_fet6: fet6 {
>> regulator-name = "lcd_vdd";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> };
>> tps65090_fet7: fet7 {
>> regulator-name = "video_mid_1a";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_ldo1: ldo1 {
>> diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> index 07b29b7..5c38bc0 100644
>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> @@ -384,27 +384,41 @@
>> };
>> tps65090_fet1: fet1 {
>> regulator-name = "vcd_led";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <1700000>;
>
> Here, too.
>
>> };
>> tps65090_fet2: fet2 {
>> regulator-name = "video_mid";
>> + regulator-min-microvolt = <4500000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet3: fet3 {
>> regulator-name = "wwan_r";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet4: fet4 {
>> regulator-name = "sdcard";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet5: fet5 {
>> regulator-name = "camout";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> };
>> tps65090_fet6: fet6 {
>> regulator-name = "lcd_vdd";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> };
>> tps65090_fet7: fet7 {
>> regulator-name = "video_mid_1a";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_ldo1: ldo1 {
>
> Other than 1.7V vs. 17V, this matches what I see in the tps65090
> specifications. Technically I think that this should also be applied
> to other tps65090 users in mainline since it's a property shared among
> every user of tps65090. That means exynos5250-snow and
> tegra114-dalmore. I'd be tempted to say that it belongs in source
> code or in a dts fragment as well.
>
Agreed, now is clear from the table that these are operating conditions related
to the PMIC and is not a per board value. So since these are constants, they
should be added to the driver source code and not in the DTS. I'll do this in v2
as well.
> -Doug
>
Best regards,
Javier
WARNING: multiple messages have this Message-ID (diff)
From: javier.martinez@collabora.co.uk (Javier Martinez Canillas)
To: linux-arm-kernel@lists.infradead.org
Subject: [RESEND PATCH 2/2] ARM: dts: Add tps65090 FET constraints on Peach Pit and Pi
Date: Mon, 11 Aug 2014 19:47:18 +0200 [thread overview]
Message-ID: <53E901A6.2020902@collabora.co.uk> (raw)
In-Reply-To: <CAD=FV=XRmNkxXoxWaEfxFADrTgBrOPVX746woxiZoKck1ajwgA@mail.gmail.com>
Hello Doug,
On 08/11/2014 05:57 PM, Doug Anderson wrote:
> Javier,
>
> On Mon, Aug 11, 2014 at 4:38 AM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>> Both Exynos5420 Peach Pit and Exynos5800 Peach Pi boards
>> have a tps65090 PMU that has a number of switches (FETs)
>> that are just on/off devices but they do have a current
>> limit and the output voltage of the switch is ramped up
>> within a controlled slope.
>>
>> After the switch is turned on, a safety timer is started
>> and before this timer times out the output voltage must
>> have reached the input voltage. Otherwise the switch is
>> turned off expecting an overload condition.
>>
>> So using the maximum output voltage slew rate and the timer
>> minimum and maximum timeouts, a voltage constraints can be
>> expressed as bounded limits for the timeout. That is what
>> is used in the board schematics and should be in the DT too.
>
> I don't understand this, but if you and Mark are happy with it...
>
> ...I'm also not 100% certain what the above description has to do with
> this change, but I'll admit to having only skimmed some of the earlier
> conversations.
>
As I stated before, I wrongly assumed from where the constraints in the
schematics came from. From the latest doc I now know that there is a
"recommended operating conditions table". I'll fix it in v2, sorry for the
confusion...
>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>> arch/arm/boot/dts/exynos5420-peach-pit.dts | 14 ++++++++++++++
>> arch/arm/boot/dts/exynos5800-peach-pi.dts | 14 ++++++++++++++
>> 2 files changed, 28 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> index d8710c1..eefafe6 100644
>> --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> @@ -386,27 +386,41 @@
>> };
>> tps65090_fet1: fet1 {
>> regulator-name = "vcd_led";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <1700000>;
>
> This is almost certainly wrong. Your max is smaller than your min.
> Perhaps you want an extra 0.
>
>> };
>> tps65090_fet2: fet2 {
>> regulator-name = "video_mid";
>> + regulator-min-microvolt = <4500000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet3: fet3 {
>> regulator-name = "wwan_r";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet4: fet4 {
>> regulator-name = "sdcard";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet5: fet5 {
>> regulator-name = "camout";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> };
>> tps65090_fet6: fet6 {
>> regulator-name = "lcd_vdd";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> };
>> tps65090_fet7: fet7 {
>> regulator-name = "video_mid_1a";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_ldo1: ldo1 {
>> diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> index 07b29b7..5c38bc0 100644
>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> @@ -384,27 +384,41 @@
>> };
>> tps65090_fet1: fet1 {
>> regulator-name = "vcd_led";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <1700000>;
>
> Here, too.
>
>> };
>> tps65090_fet2: fet2 {
>> regulator-name = "video_mid";
>> + regulator-min-microvolt = <4500000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet3: fet3 {
>> regulator-name = "wwan_r";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet4: fet4 {
>> regulator-name = "sdcard";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_fet5: fet5 {
>> regulator-name = "camout";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> };
>> tps65090_fet6: fet6 {
>> regulator-name = "lcd_vdd";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> };
>> tps65090_fet7: fet7 {
>> regulator-name = "video_mid_1a";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <5500000>;
>> regulator-always-on;
>> };
>> tps65090_ldo1: ldo1 {
>
> Other than 1.7V vs. 17V, this matches what I see in the tps65090
> specifications. Technically I think that this should also be applied
> to other tps65090 users in mainline since it's a property shared among
> every user of tps65090. That means exynos5250-snow and
> tegra114-dalmore. I'd be tempted to say that it belongs in source
> code or in a dts fragment as well.
>
Agreed, now is clear from the table that these are operating conditions related
to the PMIC and is not a per board value. So since these are constants, they
should be added to the driver source code and not in the DTS. I'll do this in v2
as well.
> -Doug
>
Best regards,
Javier
next prev parent reply other threads:[~2014-08-11 17:47 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-11 11:38 [RESEND PATCH 1/2] ARM: dts: Improve Peach Pit and Pi power scheme Javier Martinez Canillas
2014-08-11 11:38 ` Javier Martinez Canillas
2014-08-11 11:38 ` [RESEND PATCH 2/2] ARM: dts: Add tps65090 FET constraints on Peach Pit and Pi Javier Martinez Canillas
2014-08-11 11:38 ` Javier Martinez Canillas
2014-08-11 15:57 ` Doug Anderson
2014-08-11 15:57 ` Doug Anderson
2014-08-11 16:02 ` Mark Brown
2014-08-11 16:02 ` Mark Brown
2014-08-11 17:31 ` Javier Martinez Canillas
2014-08-11 17:31 ` Javier Martinez Canillas
2014-08-11 17:47 ` Javier Martinez Canillas [this message]
2014-08-11 17:47 ` Javier Martinez Canillas
[not found] ` <1407757091-18730-1-git-send-email-javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
2014-08-11 15:46 ` [RESEND PATCH 1/2] ARM: dts: Improve Peach Pit and Pi power scheme Doug Anderson
2014-08-11 15:46 ` Doug Anderson
2014-08-11 15:46 ` Doug Anderson
2014-08-18 18:27 ` Kukjin Kim
2014-08-18 18:27 ` Kukjin Kim
2014-08-18 21:43 ` Javier Martinez Canillas
2014-08-18 21:43 ` Javier Martinez Canillas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53E901A6.2020902@collabora.co.uk \
--to=javier.martinez@collabora.co.uk \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dianders@chromium.org \
--cc=kgene.kim@samsung.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=olof@lixom.net \
--cc=yuvaraj.cd@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.