| Message ID | 1474274696-28090-1-git-send-email-andy.yan@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 19 Sep 2016 16:44:56 +0800 Andy Yan <andy.yan@rock-chips.com> wrote: > The current rk3066a based boards(Rayeager, Bqcurie2, Marsboard) use > pwm modulate vdd_logic voltage, but the pwm is default disabled and > the pwm pin acts as a gpio before pwm regulator probed, so the pwm > regulator driver will get a zero dutycycle at probe time, so change > the initial dutycycle to zero to match pwm_regulator_init_state check. > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > > --- > > arch/arm/boot/dts/rk3066a-bqcurie2.dts | 2 +- > arch/arm/boot/dts/rk3066a-marsboard.dts | 2 +- > arch/arm/boot/dts/rk3066a-rayeager.dts | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/boot/dts/rk3066a-bqcurie2.dts b/arch/arm/boot/dts/rk3066a-bqcurie2.dts > index bc674ee..618450d 100644 > --- a/arch/arm/boot/dts/rk3066a-bqcurie2.dts > +++ b/arch/arm/boot/dts/rk3066a-bqcurie2.dts > @@ -61,7 +61,7 @@ > regulator-min-microvolt = <1200000>; > regulator-max-microvolt = <1200000>; > regulator-always-on; > - voltage-table = <1000000 100>, > + voltage-table = <1000000 0>, > <1200000 42>; So, it seems you are reversing the PWM polarity here. Are you sure you shouldn't change the 2nd entry of this table (<1200000 58>)? > status = "okay"; > }; > diff --git a/arch/arm/boot/dts/rk3066a-marsboard.dts b/arch/arm/boot/dts/rk3066a-marsboard.dts > index a2b763e..ddc680b 100644 > --- a/arch/arm/boot/dts/rk3066a-marsboard.dts > +++ b/arch/arm/boot/dts/rk3066a-marsboard.dts > @@ -59,7 +59,7 @@ > regulator-min-microvolt = <1200000>; > regulator-max-microvolt = <1200000>; > regulator-always-on; > - voltage-table = <1000000 100>, > + voltage-table = <1000000 0>, > <1200000 42>; > status = "okay"; > }; > diff --git a/arch/arm/boot/dts/rk3066a-rayeager.dts b/arch/arm/boot/dts/rk3066a-rayeager.dts > index 2536b3a..30aee99 100644 > --- a/arch/arm/boot/dts/rk3066a-rayeager.dts > +++ b/arch/arm/boot/dts/rk3066a-rayeager.dts > @@ -84,7 +84,7 @@ > regulator-min-microvolt = <1200000>; > regulator-max-microvolt = <1200000>; > regulator-always-on; > - voltage-table = <1000000 100>, > + voltage-table = <1000000 0>, > <1200000 42>; > status = "okay"; > };
On 2016年09月19日 17:25, Boris Brezillon wrote: > On Mon, 19 Sep 2016 16:44:56 +0800 > Andy Yan <andy.yan@rock-chips.com> wrote: > >> The current rk3066a based boards(Rayeager, Bqcurie2, Marsboard) use >> pwm modulate vdd_logic voltage, but the pwm is default disabled and >> the pwm pin acts as a gpio before pwm regulator probed, so the pwm >> regulator driver will get a zero dutycycle at probe time, so change >> the initial dutycycle to zero to match pwm_regulator_init_state check. >> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> >> --- >> >> arch/arm/boot/dts/rk3066a-bqcurie2.dts | 2 +- >> arch/arm/boot/dts/rk3066a-marsboard.dts | 2 +- >> arch/arm/boot/dts/rk3066a-rayeager.dts | 2 +- >> 3 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/boot/dts/rk3066a-bqcurie2.dts b/arch/arm/boot/dts/rk3066a-bqcurie2.dts >> index bc674ee..618450d 100644 >> --- a/arch/arm/boot/dts/rk3066a-bqcurie2.dts >> +++ b/arch/arm/boot/dts/rk3066a-bqcurie2.dts >> @@ -61,7 +61,7 @@ >> regulator-min-microvolt = <1200000>; >> regulator-max-microvolt = <1200000>; >> regulator-always-on; >> - voltage-table = <1000000 100>, >> + voltage-table = <1000000 0>, >> <1200000 42>; > So, it seems you are reversing the PWM polarity here. Are you sure you > shouldn't change the 2nd entry of this table (<1200000 58>)? no, 42% duty cycle give a stable 1.2v vdd_logic voltage on my board. As i explained in the commit, the pwm was default at a disabled state before regulator_register success at pwm_regulator probe, so pwm_regulator_init_state function will get a zero dutycycle from pwm_get_relative_dutycycle. I have to change the dutycycle of fist table to zero to match the following check, other wise the function will return defualt value(-EINVAL), then the pwm regulator register failed. > >> status = "okay"; >> }; >> diff --git a/arch/arm/boot/dts/rk3066a-marsboard.dts b/arch/arm/boot/dts/rk3066a-marsboard.dts >> index a2b763e..ddc680b 100644 >> --- a/arch/arm/boot/dts/rk3066a-marsboard.dts >> +++ b/arch/arm/boot/dts/rk3066a-marsboard.dts >> @@ -59,7 +59,7 @@ >> regulator-min-microvolt = <1200000>; >> regulator-max-microvolt = <1200000>; >> regulator-always-on; >> - voltage-table = <1000000 100>, >> + voltage-table = <1000000 0>, >> <1200000 42>; >> status = "okay"; >> }; >> diff --git a/arch/arm/boot/dts/rk3066a-rayeager.dts b/arch/arm/boot/dts/rk3066a-rayeager.dts >> index 2536b3a..30aee99 100644 >> --- a/arch/arm/boot/dts/rk3066a-rayeager.dts >> +++ b/arch/arm/boot/dts/rk3066a-rayeager.dts >> @@ -84,7 +84,7 @@ >> regulator-min-microvolt = <1200000>; >> regulator-max-microvolt = <1200000>; >> regulator-always-on; >> - voltage-table = <1000000 100>, >> + voltage-table = <1000000 0>, >> <1200000 42>; >> status = "okay"; >> }; > > >
On Mon, 19 Sep 2016 17:38:10 +0800 Andy Yan <andy.yan@rock-chips.com> wrote: > On 2016年09月19日 17:25, Boris Brezillon wrote: > > On Mon, 19 Sep 2016 16:44:56 +0800 > > Andy Yan <andy.yan@rock-chips.com> wrote: > > > >> The current rk3066a based boards(Rayeager, Bqcurie2, Marsboard) use > >> pwm modulate vdd_logic voltage, but the pwm is default disabled and > >> the pwm pin acts as a gpio before pwm regulator probed, so the pwm > >> regulator driver will get a zero dutycycle at probe time, so change > >> the initial dutycycle to zero to match pwm_regulator_init_state check. > >> > >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > >> > >> --- > >> > >> arch/arm/boot/dts/rk3066a-bqcurie2.dts | 2 +- > >> arch/arm/boot/dts/rk3066a-marsboard.dts | 2 +- > >> arch/arm/boot/dts/rk3066a-rayeager.dts | 2 +- > >> 3 files changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/arm/boot/dts/rk3066a-bqcurie2.dts b/arch/arm/boot/dts/rk3066a-bqcurie2.dts > >> index bc674ee..618450d 100644 > >> --- a/arch/arm/boot/dts/rk3066a-bqcurie2.dts > >> +++ b/arch/arm/boot/dts/rk3066a-bqcurie2.dts > >> @@ -61,7 +61,7 @@ > >> regulator-min-microvolt = <1200000>; > >> regulator-max-microvolt = <1200000>; > >> regulator-always-on; > >> - voltage-table = <1000000 100>, > >> + voltage-table = <1000000 0>, > >> <1200000 42>; > > So, it seems you are reversing the PWM polarity here. Are you sure you > > shouldn't change the 2nd entry of this table (<1200000 58>)? > > > no, 42% duty cycle give a stable 1.2v vdd_logic voltage on my > board. As i explained in the commit, the pwm was default at a disabled > state before regulator_register success at pwm_regulator probe, so > pwm_regulator_init_state function will get a zero dutycycle from > pwm_get_relative_dutycycle. I have to change the dutycycle of fist > table to zero to match the following check, other wise the function will > return defualt value(-EINVAL), then the pwm regulator register failed. Is 0% duty really producing a 1V output, or are you just changing it to make it work? BTW, I'm not sure we should fail when the current PWM state does not match one of the value in the voltage-table. > > > >> status = "okay"; > >> }; > >> diff --git a/arch/arm/boot/dts/rk3066a-marsboard.dts b/arch/arm/boot/dts/rk3066a-marsboard.dts > >> index a2b763e..ddc680b 100644 > >> --- a/arch/arm/boot/dts/rk3066a-marsboard.dts > >> +++ b/arch/arm/boot/dts/rk3066a-marsboard.dts > >> @@ -59,7 +59,7 @@ > >> regulator-min-microvolt = <1200000>; > >> regulator-max-microvolt = <1200000>; > >> regulator-always-on; > >> - voltage-table = <1000000 100>, > >> + voltage-table = <1000000 0>, > >> <1200000 42>; > >> status = "okay"; > >> }; > >> diff --git a/arch/arm/boot/dts/rk3066a-rayeager.dts b/arch/arm/boot/dts/rk3066a-rayeager.dts > >> index 2536b3a..30aee99 100644 > >> --- a/arch/arm/boot/dts/rk3066a-rayeager.dts > >> +++ b/arch/arm/boot/dts/rk3066a-rayeager.dts > >> @@ -84,7 +84,7 @@ > >> regulator-min-microvolt = <1200000>; > >> regulator-max-microvolt = <1200000>; > >> regulator-always-on; > >> - voltage-table = <1000000 100>, > >> + voltage-table = <1000000 0>, > >> <1200000 42>; > >> status = "okay"; > >> }; > > > > > > > >
Hi Boris: On 2016年09月19日 17:44, Boris Brezillon wrote: > On Mon, 19 Sep 2016 17:38:10 +0800 > Andy Yan <andy.yan@rock-chips.com> wrote: > >> On 2016年09月19日 17:25, Boris Brezillon wrote: >>> On Mon, 19 Sep 2016 16:44:56 +0800 >>> Andy Yan <andy.yan@rock-chips.com> wrote: >>> >>>> The current rk3066a based boards(Rayeager, Bqcurie2, Marsboard) use >>>> pwm modulate vdd_logic voltage, but the pwm is default disabled and >>>> the pwm pin acts as a gpio before pwm regulator probed, so the pwm >>>> regulator driver will get a zero dutycycle at probe time, so change >>>> the initial dutycycle to zero to match pwm_regulator_init_state check. >>>> >>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >>>> >>>> --- >>>> >>>> arch/arm/boot/dts/rk3066a-bqcurie2.dts | 2 +- >>>> arch/arm/boot/dts/rk3066a-marsboard.dts | 2 +- >>>> arch/arm/boot/dts/rk3066a-rayeager.dts | 2 +- >>>> 3 files changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/arch/arm/boot/dts/rk3066a-bqcurie2.dts b/arch/arm/boot/dts/rk3066a-bqcurie2.dts >>>> index bc674ee..618450d 100644 >>>> --- a/arch/arm/boot/dts/rk3066a-bqcurie2.dts >>>> +++ b/arch/arm/boot/dts/rk3066a-bqcurie2.dts >>>> @@ -61,7 +61,7 @@ >>>> regulator-min-microvolt = <1200000>; >>>> regulator-max-microvolt = <1200000>; >>>> regulator-always-on; >>>> - voltage-table = <1000000 100>, >>>> + voltage-table = <1000000 0>, >>>> <1200000 42>; >>> So, it seems you are reversing the PWM polarity here. Are you sure you >>> shouldn't change the 2nd entry of this table (<1200000 58>)? >> >> no, 42% duty cycle give a stable 1.2v vdd_logic voltage on my >> board. As i explained in the commit, the pwm was default at a disabled >> state before regulator_register success at pwm_regulator probe, so >> pwm_regulator_init_state function will get a zero dutycycle from >> pwm_get_relative_dutycycle. I have to change the dutycycle of fist >> table to zero to match the following check, other wise the function will >> return defualt value(-EINVAL), then the pwm regulator register failed. > Is 0% duty really producing a 1V output, or are you just changing it to > make it work? no, I'm not sure 0% duty is really producing a 1v output, maybe the 100% duty producing a 1v output, I just changing it to make it work on your Patch. Before the pwm regulator registered success, the pwm output pin act as a stable gpio at a hight or low level(depends on the soc power on state) to control a 1v output. > > BTW, I'm not sure we should fail when the current PWM state does not > match one of the value in the voltage-table. Yes, the previous version don't have the state check, so the regulator can register success. > >>> >>>> status = "okay"; >>>> }; >>>> diff --git a/arch/arm/boot/dts/rk3066a-marsboard.dts b/arch/arm/boot/dts/rk3066a-marsboard.dts >>>> index a2b763e..ddc680b 100644 >>>> --- a/arch/arm/boot/dts/rk3066a-marsboard.dts >>>> +++ b/arch/arm/boot/dts/rk3066a-marsboard.dts >>>> @@ -59,7 +59,7 @@ >>>> regulator-min-microvolt = <1200000>; >>>> regulator-max-microvolt = <1200000>; >>>> regulator-always-on; >>>> - voltage-table = <1000000 100>, >>>> + voltage-table = <1000000 0>, >>>> <1200000 42>; >>>> status = "okay"; >>>> }; >>>> diff --git a/arch/arm/boot/dts/rk3066a-rayeager.dts b/arch/arm/boot/dts/rk3066a-rayeager.dts >>>> index 2536b3a..30aee99 100644 >>>> --- a/arch/arm/boot/dts/rk3066a-rayeager.dts >>>> +++ b/arch/arm/boot/dts/rk3066a-rayeager.dts >>>> @@ -84,7 +84,7 @@ >>>> regulator-min-microvolt = <1200000>; >>>> regulator-max-microvolt = <1200000>; >>>> regulator-always-on; >>>> - voltage-table = <1000000 100>, >>>> + voltage-table = <1000000 0>, >>>> <1200000 42>; >>>> status = "okay"; >>>> }; >>> >>> >> > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hi, On Mon, Sep 19, 2016 at 1:44 AM, Andy Yan <andy.yan@rock-chips.com> wrote: > The current rk3066a based boards(Rayeager, Bqcurie2, Marsboard) use > pwm modulate vdd_logic voltage, but the pwm is default disabled and > the pwm pin acts as a gpio before pwm regulator probed, so the pwm > regulator driver will get a zero dutycycle at probe time, so change > the initial dutycycle to zero to match pwm_regulator_init_state check. > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > > --- > > arch/arm/boot/dts/rk3066a-bqcurie2.dts | 2 +- > arch/arm/boot/dts/rk3066a-marsboard.dts | 2 +- > arch/arm/boot/dts/rk3066a-rayeager.dts | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/boot/dts/rk3066a-bqcurie2.dts b/arch/arm/boot/dts/rk3066a-bqcurie2.dts > index bc674ee..618450d 100644 > --- a/arch/arm/boot/dts/rk3066a-bqcurie2.dts > +++ b/arch/arm/boot/dts/rk3066a-bqcurie2.dts > @@ -61,7 +61,7 @@ > regulator-min-microvolt = <1200000>; > regulator-max-microvolt = <1200000>; > regulator-always-on; > - voltage-table = <1000000 100>, > + voltage-table = <1000000 0>, In my opinion this isn't quite the right answer. I think that you should add a new property describing the voltage in the case that the pin is an input and you should fill that property in, like: voltage-when-input = <1000000>; Once you have this property you should ideally be able to read whether the pin is currently configured as an input or as a special function at bootup. Note that I don't actually know if this is possible with the current pinctrl API, but it does seem like the ideal way to do it since it means you'll work even if the BIOS changes (AKA: if the BIOS leaves the pin as an input you can keep the voltage the same and if the BIOS leaves the pin as PWM you can keep the voltage the same). It's also possible that you could just add a property that says "init to a certain value at bootup no matter what the BIOS left it as". As long as that voltage is the maximum (and you'll lower it later) this ought to be safe and you shouldn't risk temporarily undervolting things. Note that, if you haven't already done so, you almost certainly want to make sure your pinctrl species an "init" state in addition to a "default" state. See <https://patchwork.kernel.org/patch/7454311/>. -Doug
Am Montag, 19. September 2016, 08:15:30 CEST schrieb Doug Anderson: > Hi, > > On Mon, Sep 19, 2016 at 1:44 AM, Andy Yan <andy.yan@rock-chips.com> wrote: > > The current rk3066a based boards(Rayeager, Bqcurie2, Marsboard) use > > pwm modulate vdd_logic voltage, but the pwm is default disabled and > > the pwm pin acts as a gpio before pwm regulator probed, so the pwm > > regulator driver will get a zero dutycycle at probe time, so change > > the initial dutycycle to zero to match pwm_regulator_init_state check. > > > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > > > > --- > > > > arch/arm/boot/dts/rk3066a-bqcurie2.dts | 2 +- > > arch/arm/boot/dts/rk3066a-marsboard.dts | 2 +- > > arch/arm/boot/dts/rk3066a-rayeager.dts | 2 +- > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm/boot/dts/rk3066a-bqcurie2.dts > > b/arch/arm/boot/dts/rk3066a-bqcurie2.dts index bc674ee..618450d 100644 > > --- a/arch/arm/boot/dts/rk3066a-bqcurie2.dts > > +++ b/arch/arm/boot/dts/rk3066a-bqcurie2.dts > > @@ -61,7 +61,7 @@ > > > > regulator-min-microvolt = <1200000>; > > regulator-max-microvolt = <1200000>; > > regulator-always-on; > > > > - voltage-table = <1000000 100>, > > + voltage-table = <1000000 0>, > > In my opinion this isn't quite the right answer. I think that you > should add a new property describing the voltage in the case that the > pin is an input and you should fill that property in, like: > > voltage-when-input = <1000000>; I'd think this would be more of a pwm issue, not something the pwm-regulator should need to care about. Ideally the pwm driver should be able to return some state information even if disabled? I.e. deriving a duty-cycle value from its pin state similar to what Doug described below (it's either 0% or 100%) But right now I have a hard time understanding how the pwm could return any duty-cycle information for an input gpio to the pwm-regulator, as I assume the pwm-driver has to probe (and thus set pinctrl to the pwm function) before the pwm-regulator is able to get the pwm handle? > Once you have this property you should ideally be able to read whether > the pin is currently configured as an input or as a special function > at bootup. Note that I don't actually know if this is possible with > the current pinctrl API, but it does seem like the ideal way to do it > since it means you'll work even if the BIOS changes (AKA: if the BIOS > leaves the pin as an input you can keep the voltage the same and if > the BIOS leaves the pin as PWM you can keep the voltage the same). > > It's also possible that you could just add a property that says "init > to a certain value at bootup no matter what the BIOS left it as". As > long as that voltage is the maximum (and you'll lower it later) this > ought to be safe and you shouldn't risk temporarily undervolting > things. > > > Note that, if you haven't already done so, you almost certainly want > to make sure your pinctrl species an "init" state in addition to a > "default" state. See <https://patchwork.kernel.org/patch/7454311/>. > > -Doug
Hi, On Mon, Sep 19, 2016 at 9:15 AM, Heiko Stuebner <heiko@sntech.de> wrote: > Am Montag, 19. September 2016, 08:15:30 CEST schrieb Doug Anderson: >> Hi, >> >> On Mon, Sep 19, 2016 at 1:44 AM, Andy Yan <andy.yan@rock-chips.com> wrote: >> > The current rk3066a based boards(Rayeager, Bqcurie2, Marsboard) use >> > pwm modulate vdd_logic voltage, but the pwm is default disabled and >> > the pwm pin acts as a gpio before pwm regulator probed, so the pwm >> > regulator driver will get a zero dutycycle at probe time, so change >> > the initial dutycycle to zero to match pwm_regulator_init_state check. >> > >> > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> > >> > --- >> > >> > arch/arm/boot/dts/rk3066a-bqcurie2.dts | 2 +- >> > arch/arm/boot/dts/rk3066a-marsboard.dts | 2 +- >> > arch/arm/boot/dts/rk3066a-rayeager.dts | 2 +- >> > 3 files changed, 3 insertions(+), 3 deletions(-) >> > >> > diff --git a/arch/arm/boot/dts/rk3066a-bqcurie2.dts >> > b/arch/arm/boot/dts/rk3066a-bqcurie2.dts index bc674ee..618450d 100644 >> > --- a/arch/arm/boot/dts/rk3066a-bqcurie2.dts >> > +++ b/arch/arm/boot/dts/rk3066a-bqcurie2.dts >> > @@ -61,7 +61,7 @@ >> > >> > regulator-min-microvolt = <1200000>; >> > regulator-max-microvolt = <1200000>; >> > regulator-always-on; >> > >> > - voltage-table = <1000000 100>, >> > + voltage-table = <1000000 0>, >> >> In my opinion this isn't quite the right answer. I think that you >> should add a new property describing the voltage in the case that the >> pin is an input and you should fill that property in, like: >> >> voltage-when-input = <1000000>; > > I'd think this would be more of a pwm issue, not something the pwm-regulator > should need to care about. > > Ideally the pwm driver should be able to return some state information even if > disabled? I.e. deriving a duty-cycle value from its pin state similar to what > Doug described below (it's either 0% or 100%) > > But right now I have a hard time understanding how the pwm could return any > duty-cycle information for an input gpio to the pwm-regulator, as I assume the > pwm-driver has to probe (and thus set pinctrl to the pwm function) before the > pwm-regulator is able to get the pwm handle? Hrm, right. The PWM ought to own the pinctrl, not the regulator. Hrm. Then I guess this gets more complicated. One thing to point out, though, is that an EE I talked to said that the "voltage when input" is actually a well defined property and is unrelated to the min/max voltage. AKA: it's not guaranteed to be equal to the 50% duty cycle. ...so adding a property to the PWM regulator that includes this value is something very sane. The "voltage when input" is defined by the pile of resistors and capacitors that are used to actually make the PWM control the regulator. The "voltage when input" is super important because this is the voltage that's used at bootup (when all pins are configured as inputs, possible with a pull applied) and that's used during suspend time when the PWM stops. -Doug
On Mon, 19 Sep 2016 09:38:34 -0700 Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Mon, Sep 19, 2016 at 9:15 AM, Heiko Stuebner <heiko@sntech.de> wrote: > > Am Montag, 19. September 2016, 08:15:30 CEST schrieb Doug Anderson: > >> Hi, > >> > >> On Mon, Sep 19, 2016 at 1:44 AM, Andy Yan <andy.yan@rock-chips.com> wrote: > >> > The current rk3066a based boards(Rayeager, Bqcurie2, Marsboard) use > >> > pwm modulate vdd_logic voltage, but the pwm is default disabled and > >> > the pwm pin acts as a gpio before pwm regulator probed, so the pwm > >> > regulator driver will get a zero dutycycle at probe time, so change > >> > the initial dutycycle to zero to match pwm_regulator_init_state check. > >> > > >> > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > >> > > >> > --- > >> > > >> > arch/arm/boot/dts/rk3066a-bqcurie2.dts | 2 +- > >> > arch/arm/boot/dts/rk3066a-marsboard.dts | 2 +- > >> > arch/arm/boot/dts/rk3066a-rayeager.dts | 2 +- > >> > 3 files changed, 3 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/arch/arm/boot/dts/rk3066a-bqcurie2.dts > >> > b/arch/arm/boot/dts/rk3066a-bqcurie2.dts index bc674ee..618450d 100644 > >> > --- a/arch/arm/boot/dts/rk3066a-bqcurie2.dts > >> > +++ b/arch/arm/boot/dts/rk3066a-bqcurie2.dts > >> > @@ -61,7 +61,7 @@ > >> > > >> > regulator-min-microvolt = <1200000>; > >> > regulator-max-microvolt = <1200000>; > >> > regulator-always-on; > >> > > >> > - voltage-table = <1000000 100>, > >> > + voltage-table = <1000000 0>, > >> > >> In my opinion this isn't quite the right answer. I think that you > >> should add a new property describing the voltage in the case that the > >> pin is an input and you should fill that property in, like: > >> > >> voltage-when-input = <1000000>; > > > > I'd think this would be more of a pwm issue, not something the pwm-regulator > > should need to care about. > > > > Ideally the pwm driver should be able to return some state information even if > > disabled? I.e. deriving a duty-cycle value from its pin state similar to what > > Doug described below (it's either 0% or 100%) > > > > But right now I have a hard time understanding how the pwm could return any > > duty-cycle information for an input gpio to the pwm-regulator, as I assume the > > pwm-driver has to probe (and thus set pinctrl to the pwm function) before the > > pwm-regulator is able to get the pwm handle? > > Hrm, right. The PWM ought to own the pinctrl, not the regulator. > Hrm. Then I guess this gets more complicated. > > One thing to point out, though, is that an EE I talked to said that > the "voltage when input" is actually a well defined property and is > unrelated to the min/max voltage. AKA: it's not guaranteed to be > equal to the 50% duty cycle. ...so adding a property to the PWM > regulator that includes this value is something very sane. The > "voltage when input" is defined by the pile of resistors and > capacitors that are used to actually make the PWM control the > regulator. > > The "voltage when input" is super important because this is the > voltage that's used at bootup (when all pins are configured as inputs, > possible with a pull applied) and that's used during suspend time when > the PWM stops. Correct me if I'm wrong, but the main problem here is that, when we try to detect the initial regulator state, we ran into a "missing entry in the duty-cycle <-> voltage table" error, which then triggers an -EINVAL error preventing the PWM regulator probe to succeed. Of course, adding an entry for the 0% dutycle case would solve the issue, but I wonder if we should not allow "unknown value" at probe time, and let the regulator user set the voltage output when it claims it. Another option would be to fake a valid value in this case (choose the closest entry in the voltage table?).
Hi, On Mon, Sep 19, 2016 at 10:13 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Correct me if I'm wrong, but the main problem here is that, when we try > to detect the initial regulator state, we ran into a "missing entry in > the duty-cycle <-> voltage table" error, which then triggers an -EINVAL > error preventing the PWM regulator probe to succeed. > > Of course, adding an entry for the 0% dutycle case would solve the > issue, but I wonder if we should not allow "unknown value" at probe > time, and let the regulator user set the voltage output when it claims > it. > > Another option would be to fake a valid value in this case (choose the > closest entry in the voltage table?). If the goal is to avoid glitching the voltage at bootup, then the above doesn't really solve it. For instance, you could have: 0% - 1.5V 100% - 0.8V When configured as input: 1.1V Max useful value: 1.2V So if firmware doesn't touch the PWM regulator then that means you're booting up at 1.1V. If you read out the PWM itself, it will claim that it's running at 0%. While that's true, that doesn't mean that the voltage on the system is actually 1.5V because at boot the system had configured the pinmux as GPIO input. ...so it's actually 1.1V because the PWM isn't actually controlling the pins. So if you want to avoid glitching the line then you want to make sure that you properly init to 1.1V, _not_ to 1.5V. In the case above, it's possible that 1.5V isn't really so great for the hardware lifespan because 1.2V is the maximum useful value. You could argue that the hardware is badly designed in this case, but I have certainly seen boards designed where the maximum achievable value is higher than the maximum "safe" value. -Doug
Am Montag, 19. September 2016, 19:13:27 CEST schrieb Boris Brezillon: > On Mon, 19 Sep 2016 09:38:34 -0700 > > Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > > > On Mon, Sep 19, 2016 at 9:15 AM, Heiko Stuebner <heiko@sntech.de> wrote: > > > Am Montag, 19. September 2016, 08:15:30 CEST schrieb Doug Anderson: > > >> Hi, > > >> > > >> On Mon, Sep 19, 2016 at 1:44 AM, Andy Yan <andy.yan@rock-chips.com> wrote: > > >> > The current rk3066a based boards(Rayeager, Bqcurie2, Marsboard) use > > >> > pwm modulate vdd_logic voltage, but the pwm is default disabled and > > >> > the pwm pin acts as a gpio before pwm regulator probed, so the pwm > > >> > regulator driver will get a zero dutycycle at probe time, so change > > >> > the initial dutycycle to zero to match pwm_regulator_init_state > > >> > check. > > >> > > > >> > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > > >> > > > >> > --- > > >> > > > >> > arch/arm/boot/dts/rk3066a-bqcurie2.dts | 2 +- > > >> > arch/arm/boot/dts/rk3066a-marsboard.dts | 2 +- > > >> > arch/arm/boot/dts/rk3066a-rayeager.dts | 2 +- > > >> > 3 files changed, 3 insertions(+), 3 deletions(-) > > >> > > > >> > diff --git a/arch/arm/boot/dts/rk3066a-bqcurie2.dts > > >> > b/arch/arm/boot/dts/rk3066a-bqcurie2.dts index bc674ee..618450d > > >> > 100644 > > >> > --- a/arch/arm/boot/dts/rk3066a-bqcurie2.dts > > >> > +++ b/arch/arm/boot/dts/rk3066a-bqcurie2.dts > > >> > @@ -61,7 +61,7 @@ > > >> > > > >> > regulator-min-microvolt = <1200000>; > > >> > regulator-max-microvolt = <1200000>; > > >> > regulator-always-on; > > >> > > > >> > - voltage-table = <1000000 100>, > > >> > + voltage-table = <1000000 0>, > > >> > > >> In my opinion this isn't quite the right answer. I think that you > > >> should add a new property describing the voltage in the case that the > > >> > > >> pin is an input and you should fill that property in, like: > > >> voltage-when-input = <1000000>; > > > > > > I'd think this would be more of a pwm issue, not something the > > > pwm-regulator should need to care about. > > > > > > Ideally the pwm driver should be able to return some state information > > > even if disabled? I.e. deriving a duty-cycle value from its pin state > > > similar to what Doug described below (it's either 0% or 100%) > > > > > > But right now I have a hard time understanding how the pwm could return > > > any > > > duty-cycle information for an input gpio to the pwm-regulator, as I > > > assume the pwm-driver has to probe (and thus set pinctrl to the pwm > > > function) before the pwm-regulator is able to get the pwm handle? > > > > Hrm, right. The PWM ought to own the pinctrl, not the regulator. > > Hrm. Then I guess this gets more complicated. > > > > One thing to point out, though, is that an EE I talked to said that > > the "voltage when input" is actually a well defined property and is > > unrelated to the min/max voltage. AKA: it's not guaranteed to be > > equal to the 50% duty cycle. ...so adding a property to the PWM > > regulator that includes this value is something very sane. The > > "voltage when input" is defined by the pile of resistors and > > capacitors that are used to actually make the PWM control the > > regulator. > > > > The "voltage when input" is super important because this is the > > voltage that's used at bootup (when all pins are configured as inputs, > > possible with a pull applied) and that's used during suspend time when > > the PWM stops. > > Correct me if I'm wrong, but the main problem here is that, when we try > to detect the initial regulator state, we ran into a "missing entry in > the duty-cycle <-> voltage table" error, which then triggers an -EINVAL > error preventing the PWM regulator probe to succeed. I'm unsure about this ... i.e. according to Andy's description, the pin is muxed as gpio and set to input, which in turn results in the voltage-when- input scenario. So what happens with that voltage when the pwm-driver (= driver-core after probe) switches the pinmux to the pwm output, but that pwm itself is not running? For veyrons it was easy, as the firmware set up the pwm to what it wanted, so the state could be read back.
On Mon, 19 Sep 2016 10:22:43 -0700 Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Mon, Sep 19, 2016 at 10:13 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > Correct me if I'm wrong, but the main problem here is that, when we try > > to detect the initial regulator state, we ran into a "missing entry in > > the duty-cycle <-> voltage table" error, which then triggers an -EINVAL > > error preventing the PWM regulator probe to succeed. > > > > Of course, adding an entry for the 0% dutycle case would solve the > > issue, but I wonder if we should not allow "unknown value" at probe > > time, and let the regulator user set the voltage output when it claims > > it. > > > > Another option would be to fake a valid value in this case (choose the > > closest entry in the voltage table?). > > If the goal is to avoid glitching the voltage at bootup, then the > above doesn't really solve it. > > For instance, you could have: > > 0% - 1.5V > 100% - 0.8V > When configured as input: 1.1V > Max useful value: 1.2V > > So if firmware doesn't touch the PWM regulator then that means you're > booting up at 1.1V. Oh! I really didn't consider this case when developing the solution. I was assuming that the firmware/bootloader would configure the PWM correctly and leave it activating when booting the kernel. > > If you read out the PWM itself, it will claim that it's running at 0%. > While that's true, that doesn't mean that the voltage on the system is > actually 1.5V because at boot the system had configured the pinmux as > GPIO input. ...so it's actually 1.1V because the PWM isn't actually > controlling the pins. The PWM chip has always claimed the pins and muxed them to the PWM IP. So, this means it's broken from the beginning, and my patch is only uncovering the problem (unless the pins stay configured as input until the PWM is enabled, which I'm not sure is the case). > > So if you want to avoid glitching the line then you want to make sure > that you properly init to 1.1V, _not_ to 1.5V. > > In the case above, it's possible that 1.5V isn't really so great for > the hardware lifespan because 1.2V is the maximum useful value. You > could argue that the hardware is badly designed in this case, but I > have certainly seen boards designed where the maximum achievable value > is higher than the maximum "safe" value. > > -Doug
Hi, On Mon, Sep 19, 2016 at 10:48 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > The PWM chip has always claimed the pins and muxed them to the PWM IP. > So, this means it's broken from the beginning, and my patch is only > uncovering the problem (unless the pins stay configured as input until > the PWM is enabled, which I'm not sure is the case). Such a solution is achievable with the pinctrl APIs pretty easily. You might not be able to use the automatic "init" state but you can do something similar and switch to an "active" state once the PWM is actually turned on the first time. -Doug
On Mon, 19 Sep 2016 10:52:51 -0700 Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Mon, Sep 19, 2016 at 10:48 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > The PWM chip has always claimed the pins and muxed them to the PWM IP. > > So, this means it's broken from the beginning, and my patch is only > > uncovering the problem (unless the pins stay configured as input until > > the PWM is enabled, which I'm not sure is the case). > > Such a solution is achievable with the pinctrl APIs pretty easily. > You might not be able to use the automatic "init" state but you can do > something similar and switch to an "active" state once the PWM is > actually turned on the first time. But is it really the case here (I don't see any code requesting a specific pinmux depending on the PWM state)? Anyway, we really need to handle this case, we should define the typical voltage when the PWM is disabled. Same as what you suggested with voltage-when-input, but with a different naming (since the concept of pinmux is PWM hardware/driver specific). voltage-when-pwm-disabled = <...>;
Hi, On Mon, Sep 19, 2016 at 11:06 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Mon, 19 Sep 2016 10:52:51 -0700 > Doug Anderson <dianders@chromium.org> wrote: > >> Hi, >> >> On Mon, Sep 19, 2016 at 10:48 AM, Boris Brezillon >> <boris.brezillon@free-electrons.com> wrote: >> > The PWM chip has always claimed the pins and muxed them to the PWM IP. >> > So, this means it's broken from the beginning, and my patch is only >> > uncovering the problem (unless the pins stay configured as input until >> > the PWM is enabled, which I'm not sure is the case). >> >> Such a solution is achievable with the pinctrl APIs pretty easily. >> You might not be able to use the automatic "init" state but you can do >> something similar and switch to an "active" state once the PWM is >> actually turned on the first time. > > But is it really the case here (I don't see any code requesting a > specific pinmux depending on the PWM state)? It is not happening right now as far as I know. ...but that's a bug. > Anyway, we really need to handle this case, we should define the > typical voltage when the PWM is disabled. Same as what you suggested > with voltage-when-input, but with a different naming (since the concept > of pinmux is PWM hardware/driver specific). > > voltage-when-pwm-disabled = <...>; Voltage when disabled and voltage when input are two different states. A disabled PWM will typically either drive high or low (depending on where it was when you turned it off). Not all "disabled" states will mean that the pin is configured as an input.
On Mon, 19 Sep 2016 11:12:12 -0700 Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Mon, Sep 19, 2016 at 11:06 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Mon, 19 Sep 2016 10:52:51 -0700 > > Doug Anderson <dianders@chromium.org> wrote: > > > >> Hi, > >> > >> On Mon, Sep 19, 2016 at 10:48 AM, Boris Brezillon > >> <boris.brezillon@free-electrons.com> wrote: > >> > The PWM chip has always claimed the pins and muxed them to the PWM IP. > >> > So, this means it's broken from the beginning, and my patch is only > >> > uncovering the problem (unless the pins stay configured as input until > >> > the PWM is enabled, which I'm not sure is the case). > >> > >> Such a solution is achievable with the pinctrl APIs pretty easily. > >> You might not be able to use the automatic "init" state but you can do > >> something similar and switch to an "active" state once the PWM is > >> actually turned on the first time. > > > > But is it really the case here (I don't see any code requesting a > > specific pinmux depending on the PWM state)? > > It is not happening right now as far as I know. ...but that's a bug. Okay. > > > Anyway, we really need to handle this case, we should define the > > typical voltage when the PWM is disabled. Same as what you suggested > > with voltage-when-input, but with a different naming (since the concept > > of pinmux is PWM hardware/driver specific). > > > > voltage-when-pwm-disabled = <...>; > > Voltage when disabled and voltage when input are two different states. > A disabled PWM will typically either drive high or low (depending on > where it was when you turned it off). Not all "disabled" states will > mean that the pin is configured as an input. Well, my point was that, from the regulator PoV, a PWM is either enabled or disabled. The pinmux config when the PWM is disabled depends on the PWM driver/hardware, and I don't think it's a good idea to expose this information at the regulator level, hence the name "voltage-when-pwm-disabled". Anyway, let's stop bikeshedding, no matter the name, I think we both agree that a new DT property is needed to handle this case :-).
On Mon, 19 Sep 2016 11:12:12 -0700 Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Mon, Sep 19, 2016 at 11:06 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Mon, 19 Sep 2016 10:52:51 -0700 > > Doug Anderson <dianders@chromium.org> wrote: > > > >> Hi, > >> > >> On Mon, Sep 19, 2016 at 10:48 AM, Boris Brezillon > >> <boris.brezillon@free-electrons.com> wrote: > >> > The PWM chip has always claimed the pins and muxed them to the PWM IP. > >> > So, this means it's broken from the beginning, and my patch is only > >> > uncovering the problem (unless the pins stay configured as input until > >> > the PWM is enabled, which I'm not sure is the case). > >> > >> Such a solution is achievable with the pinctrl APIs pretty easily. > >> You might not be able to use the automatic "init" state but you can do > >> something similar and switch to an "active" state once the PWM is > >> actually turned on the first time. > > > > But is it really the case here (I don't see any code requesting a > > specific pinmux depending on the PWM state)? > > It is not happening right now as far as I know. ...but that's a bug. > > > Anyway, we really need to handle this case, we should define the > > typical voltage when the PWM is disabled. Same as what you suggested > > with voltage-when-input, but with a different naming (since the concept > > of pinmux is PWM hardware/driver specific). > > > > voltage-when-pwm-disabled = <...>; > > Voltage when disabled and voltage when input are two different states. > A disabled PWM will typically either drive high or low (depending on > where it was when you turned it off). Not all "disabled" states will > mean that the pin is configured as an input. Okay, after reading again your first answer, I think I understand why you want to differentiate the when-disabled and when-input cases. You want to use the "init" and "default" pinctrl states. The "init" state (applied at probe time) would keep the PWM pin in gpio+input mode and the "default" state (applied when the PWM device is enabled for the first time) would mux the pin to the PWM device. Your solution requires that the pwm-regulator device knows in which state the pin attached to the PWM is, which IMO is breaking the layering we have right now: a PWM-regulator is assigned a PWM device which is assigned a pin and a pinmux config. Another solution would be to expose an additional information in the pwm_state: whether the PWM is in the INIT state (probed, but not yet configured by its user) or DEFAULT state (probed and already configured by its user). But again, by doing that we also expose internal PWM details to its user, which I'm not sure will help keep the PWM API simple. Actually, I had something slightly different in mind. I thought about having two new pinctrl states ("enabled" and "disabled"). The "enabled" state (pin muxed to the PWM device) would be applied each time the PWM is enabled, and "disabled" state (gpio+input mode) would be applied each time the PWM is disabled. This way we can guarantee that even when the PWM is disabled, the PWM-driven regulator is configured to output a non-destructive voltage. Hence my suggestion to name the property 'voltage-when-pwm-disabled'.
Hi, On Mon, Sep 19, 2016 at 1:43 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Mon, 19 Sep 2016 11:12:12 -0700 > Doug Anderson <dianders@chromium.org> wrote: > >> Hi, >> >> On Mon, Sep 19, 2016 at 11:06 AM, Boris Brezillon >> <boris.brezillon@free-electrons.com> wrote: >> > On Mon, 19 Sep 2016 10:52:51 -0700 >> > Doug Anderson <dianders@chromium.org> wrote: >> > >> >> Hi, >> >> >> >> On Mon, Sep 19, 2016 at 10:48 AM, Boris Brezillon >> >> <boris.brezillon@free-electrons.com> wrote: >> >> > The PWM chip has always claimed the pins and muxed them to the PWM IP. >> >> > So, this means it's broken from the beginning, and my patch is only >> >> > uncovering the problem (unless the pins stay configured as input until >> >> > the PWM is enabled, which I'm not sure is the case). >> >> >> >> Such a solution is achievable with the pinctrl APIs pretty easily. >> >> You might not be able to use the automatic "init" state but you can do >> >> something similar and switch to an "active" state once the PWM is >> >> actually turned on the first time. >> > >> > But is it really the case here (I don't see any code requesting a >> > specific pinmux depending on the PWM state)? >> >> It is not happening right now as far as I know. ...but that's a bug. >> >> > Anyway, we really need to handle this case, we should define the >> > typical voltage when the PWM is disabled. Same as what you suggested >> > with voltage-when-input, but with a different naming (since the concept >> > of pinmux is PWM hardware/driver specific). >> > >> > voltage-when-pwm-disabled = <...>; >> >> Voltage when disabled and voltage when input are two different states. >> A disabled PWM will typically either drive high or low (depending on >> where it was when you turned it off). Not all "disabled" states will >> mean that the pin is configured as an input. > > Okay, after reading again your first answer, I think I understand why > you want to differentiate the when-disabled and when-input cases. You > want to use the "init" and "default" pinctrl states. The "init" state > (applied at probe time) would keep the PWM pin in gpio+input mode and > the "default" state (applied when the PWM device is enabled for the > first time) would mux the pin to the PWM device. > Your solution requires that the pwm-regulator device knows in which > state the pin attached to the PWM is, which IMO is breaking the > layering we have right now: a PWM-regulator is assigned a PWM device > which is assigned a pin and a pinmux config. > Another solution would be to expose an additional information in the > pwm_state: whether the PWM is in the INIT state (probed, but not yet > configured by its user) or DEFAULT state (probed and already > configured by its user). But again, by doing that we also expose > internal PWM details to its user, which I'm not sure will help keep > the PWM API simple. One note is that probably using the "init" state would not be a good idea because it would make assumptions about what state the firmware left things. Possibly a firmware update could cause a change from a PWM being left as "input" to the PWM actually being initted. ...really we should be able to detect if the PWM was left on at bootup. > Actually, I had something slightly different in mind. I thought about > having two new pinctrl states ("enabled" and "disabled"). The "enabled" > state (pin muxed to the PWM device) would be applied each time the PWM > is enabled, and "disabled" state (gpio+input mode) would be applied each > time the PWM is disabled. Adding "enabled" and "disabled" state is sane IMHO. At the moment the PWM regulator never actually puts things in "disabled" state, but I suppose it could in the future. > This way we can guarantee that even when the PWM is disabled, the > PWM-driven regulator is configured to output a non-destructive voltage. > Hence my suggestion to name the property 'voltage-when-pwm-disabled'. That's fine with me, as long as the PWM starts up as "enabled" if the pinctrl happened to be left initted by the BIOS. -Doug
+Mark I realize Mark has been out of the discussion, and what started as a DT problem actually turned into a PWM regulator discussion. Maybe we should start a new thread. On Mon, 19 Sep 2016 14:15:06 -0700 Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Mon, Sep 19, 2016 at 1:43 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Mon, 19 Sep 2016 11:12:12 -0700 > > Doug Anderson <dianders@chromium.org> wrote: > > > >> Hi, > >> > >> On Mon, Sep 19, 2016 at 11:06 AM, Boris Brezillon > >> <boris.brezillon@free-electrons.com> wrote: > >> > On Mon, 19 Sep 2016 10:52:51 -0700 > >> > Doug Anderson <dianders@chromium.org> wrote: > >> > > >> >> Hi, > >> >> > >> >> On Mon, Sep 19, 2016 at 10:48 AM, Boris Brezillon > >> >> <boris.brezillon@free-electrons.com> wrote: > >> >> > The PWM chip has always claimed the pins and muxed them to the PWM IP. > >> >> > So, this means it's broken from the beginning, and my patch is only > >> >> > uncovering the problem (unless the pins stay configured as input until > >> >> > the PWM is enabled, which I'm not sure is the case). > >> >> > >> >> Such a solution is achievable with the pinctrl APIs pretty easily. > >> >> You might not be able to use the automatic "init" state but you can do > >> >> something similar and switch to an "active" state once the PWM is > >> >> actually turned on the first time. > >> > > >> > But is it really the case here (I don't see any code requesting a > >> > specific pinmux depending on the PWM state)? > >> > >> It is not happening right now as far as I know. ...but that's a bug. > >> > >> > Anyway, we really need to handle this case, we should define the > >> > typical voltage when the PWM is disabled. Same as what you suggested > >> > with voltage-when-input, but with a different naming (since the concept > >> > of pinmux is PWM hardware/driver specific). > >> > > >> > voltage-when-pwm-disabled = <...>; > >> > >> Voltage when disabled and voltage when input are two different states. > >> A disabled PWM will typically either drive high or low (depending on > >> where it was when you turned it off). Not all "disabled" states will > >> mean that the pin is configured as an input. > > > > Okay, after reading again your first answer, I think I understand why > > you want to differentiate the when-disabled and when-input cases. You > > want to use the "init" and "default" pinctrl states. The "init" state > > (applied at probe time) would keep the PWM pin in gpio+input mode and > > the "default" state (applied when the PWM device is enabled for the > > first time) would mux the pin to the PWM device. > > Your solution requires that the pwm-regulator device knows in which > > state the pin attached to the PWM is, which IMO is breaking the > > layering we have right now: a PWM-regulator is assigned a PWM device > > which is assigned a pin and a pinmux config. > > Another solution would be to expose an additional information in the > > pwm_state: whether the PWM is in the INIT state (probed, but not yet > > configured by its user) or DEFAULT state (probed and already > > configured by its user). But again, by doing that we also expose > > internal PWM details to its user, which I'm not sure will help keep > > the PWM API simple. > > One note is that probably using the "init" state would not be a good > idea because it would make assumptions about what state the firmware > left things. Possibly a firmware update could cause a change from a > PWM being left as "input" to the PWM actually being initted. > ...really we should be able to detect if the PWM was left on at > bootup. Yep. > > > > Actually, I had something slightly different in mind. I thought about > > having two new pinctrl states ("enabled" and "disabled"). The "enabled" > > state (pin muxed to the PWM device) would be applied each time the PWM > > is enabled, and "disabled" state (gpio+input mode) would be applied each > > time the PWM is disabled. > > Adding "enabled" and "disabled" state is sane IMHO. At the moment the > PWM regulator never actually puts things in "disabled" state, but I > suppose it could in the future. > > > This way we can guarantee that even when the PWM is disabled, the > > PWM-driven regulator is configured to output a non-destructive voltage. > > Hence my suggestion to name the property 'voltage-when-pwm-disabled'. > > That's fine with me, as long as the PWM starts up as "enabled" if the > pinctrl happened to be left initted by the BIOS. Yes, that's already the case in the pwm-rockchip driver. Okay, I can try to implement what is described above, but, in the meantime, I think we should find a solution for Andy's initial problem. As I said, the problem you're describing (pins muxed to the PWM device when it should actually stay in gpio+input mode) is not new, and the old pwm-regulator and pwm-rockchip implementation (before my atomic PWM changes) were behaving the same way. What is new though, is the pwm_regulator_init_state() function [1], and it seems it's now preventing the probe of a pwm-regulator device if the initial PWM state is not described in the voltage-table. The question is, what should we do? 1/ Force users to put an entry matching this state (which means breaking DT compat) 2/ Put a valid value in drvdata->state even if it's not reflecting the real state 3/ Patch regulator core to support an "unknown-selector" return code. Mark, any opinion? Thanks, Boris [1]https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/regulator/pwm-regulator.c?id=refs/tags/next-20160922#n60
On Thu, Sep 22, 2016 at 05:12:17PM +0200, Boris Brezillon wrote: > +Mark > I realize Mark has been out of the discussion, and what started as a DT > problem actually turned into a PWM regulator discussion. > Maybe we should start a new thread. Probably, you're lucky I even looked at this - the number of irrelevant patches I get CCed on is such that I'll often delete things that look irrelevant unread. I'm unsure what the relevance is, it looks like it's mainly a discussion about pinctrl? > As I said, the problem you're describing (pins muxed to the PWM device > when it should actually stay in gpio+input mode) is not new, and the old > pwm-regulator and pwm-rockchip implementation (before my atomic PWM > changes) were behaving the same way. Why would this make any kind of sense? > What is new though, is the pwm_regulator_init_state() function [1], and > it seems it's now preventing the probe of a pwm-regulator device if the > initial PWM state is not described in the voltage-table. > The question is, what should we do? > 1/ Force users to put an entry matching this state (which means > breaking DT compat) > 2/ Put a valid value in drvdata->state even if it's not reflecting the > real state > 3/ Patch regulator core to support an "unknown-selector" return code. Could someone say what the actual problem was please? That was a very long e-mail so I might be missing something but the obvious thing seems to be to force a state since we'll be doing that when we enable anyway. Or just not have the voltage table and use it as a continuous regulator.
On Thu, 22 Sep 2016 17:47:52 +0100 Mark Brown <broonie@kernel.org> wrote: > On Thu, Sep 22, 2016 at 05:12:17PM +0200, Boris Brezillon wrote: > > +Mark > > > I realize Mark has been out of the discussion, and what started as a DT > > problem actually turned into a PWM regulator discussion. > > Maybe we should start a new thread. > > Probably, you're lucky I even looked at this - the number of irrelevant > patches I get CCed on is such that I'll often delete things that look > irrelevant unread. I'm unsure what the relevance is, it looks like it's > mainly a discussion about pinctrl? Not exactly, even if this involves pin muxing. > > > As I said, the problem you're describing (pins muxed to the PWM device > > when it should actually stay in gpio+input mode) is not new, and the old > > pwm-regulator and pwm-rockchip implementation (before my atomic PWM > > changes) were behaving the same way. > > Why would this make any kind of sense? > > > What is new though, is the pwm_regulator_init_state() function [1], and > > it seems it's now preventing the probe of a pwm-regulator device if the > > initial PWM state is not described in the voltage-table. > > > The question is, what should we do? > > > 1/ Force users to put an entry matching this state (which means > > breaking DT compat) > > 2/ Put a valid value in drvdata->state even if it's not reflecting the > > real state > > 3/ Patch regulator core to support an "unknown-selector" return code. > > Could someone say what the actual problem was please? Well, to sum-up, the discussion started because Andy notice that some boards were no longer booting, and found out that commit 87248991a1de ("regulator: pwm: Properly initialize the ->state field") was causing the regression, because ->get_voltage_sel() was returning -EINVAL, which in turn was caused by a missing entry in the voltage table. Doug also noted that Andy was expecting the pin connected to the PWM regulator to stay in a GPIO+input state until the PWM is really enabled, in order to avoid glitches. But this not the case currently, since the PWM chip claims all the pins at probe time. So, there are 2 different problems here: 1/ the board no longer boots because of commit 87248991a1de and a missing entry in the voltage table 2/ claiming the PWM pins at probe time can cause glitches I'm currently trying to solve #1, but most of the discussion in this thread was about addressing #2. > That was a very > long e-mail so I might be missing something but the obvious thing seems > to be to force a state since we'll be doing that when we enable anyway. Hm, okay, but which state should we choose? The first entry in the voltage-table? > Or just not have the voltage table and use it as a continuous regulator. Yes, but that means patching the DT, which means breaking the DT compat.
On Thu, Sep 22, 2016 at 08:13:01PM +0200, Boris Brezillon wrote: > So, there are 2 different problems here: > 1/ the board no longer boots because of commit 87248991a1de and a > missing entry in the voltage table > 2/ claiming the PWM pins at probe time can cause glitches > I'm currently trying to solve #1, but most of the discussion in this > thread was about addressing #2. Well, if you actually want the entry in the voltage table then adding it does seem the most sensible fix. > > That was a very > > long e-mail so I might be missing something but the obvious thing seems > > to be to force a state since we'll be doing that when we enable anyway. > Hm, okay, but which state should we choose? The first entry in the > voltage-table? That's why we don't do this currently. Probably the closest one if we can work out what it was trying to achieve. > > Or just not have the voltage table and use it as a continuous regulator. > Yes, but that means patching the DT, which means breaking the DT compat. It sounds like you want to fix the DT anyway though?
diff --git a/arch/arm/boot/dts/rk3066a-bqcurie2.dts b/arch/arm/boot/dts/rk3066a-bqcurie2.dts index bc674ee..618450d 100644 --- a/arch/arm/boot/dts/rk3066a-bqcurie2.dts +++ b/arch/arm/boot/dts/rk3066a-bqcurie2.dts @@ -61,7 +61,7 @@ regulator-min-microvolt = <1200000>; regulator-max-microvolt = <1200000>; regulator-always-on; - voltage-table = <1000000 100>, + voltage-table = <1000000 0>, <1200000 42>; status = "okay"; }; diff --git a/arch/arm/boot/dts/rk3066a-marsboard.dts b/arch/arm/boot/dts/rk3066a-marsboard.dts index a2b763e..ddc680b 100644 --- a/arch/arm/boot/dts/rk3066a-marsboard.dts +++ b/arch/arm/boot/dts/rk3066a-marsboard.dts @@ -59,7 +59,7 @@ regulator-min-microvolt = <1200000>; regulator-max-microvolt = <1200000>; regulator-always-on; - voltage-table = <1000000 100>, + voltage-table = <1000000 0>, <1200000 42>; status = "okay"; }; diff --git a/arch/arm/boot/dts/rk3066a-rayeager.dts b/arch/arm/boot/dts/rk3066a-rayeager.dts index 2536b3a..30aee99 100644 --- a/arch/arm/boot/dts/rk3066a-rayeager.dts +++ b/arch/arm/boot/dts/rk3066a-rayeager.dts @@ -84,7 +84,7 @@ regulator-min-microvolt = <1200000>; regulator-max-microvolt = <1200000>; regulator-always-on; - voltage-table = <1000000 100>, + voltage-table = <1000000 0>, <1200000 42>; status = "okay"; };
The current rk3066a based boards(Rayeager, Bqcurie2, Marsboard) use pwm modulate vdd_logic voltage, but the pwm is default disabled and the pwm pin acts as a gpio before pwm regulator probed, so the pwm regulator driver will get a zero dutycycle at probe time, so change the initial dutycycle to zero to match pwm_regulator_init_state check. Signed-off-by: Andy Yan <andy.yan@rock-chips.com> --- arch/arm/boot/dts/rk3066a-bqcurie2.dts | 2 +- arch/arm/boot/dts/rk3066a-marsboard.dts | 2 +- arch/arm/boot/dts/rk3066a-rayeager.dts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)