Messages in this thread | | | Date | Fri, 20 Jan 2023 16:01:11 +0200 | From | Matti Vaittinen <> | Subject | Re: [PATCH v2 3/3] ARM: dts: imx7d-remarkable2: Enable the rohm,bd71815 |
| |
On 1/20/23 11:17, Alistair Francis wrote: > On Tue, Jan 10, 2023 at 5:13 AM Matti Vaittinen > <mazziesaccount@gmail.com> wrote: >> >> ma 9. tammik. 2023 klo 14.32 Alistair Francis (alistair23@gmail.com) kirjoitti: >>> >>> On Tue, Dec 20, 2022 at 11:17 PM Marco Felsch <m.felsch@pengutronix.de> wrote: >>>> >>>> Hi Alistair, >>>> >>>> thanks for your patch, please see below. >>>> >>>> On 22-12-20, Alistair Francis wrote: >>>>> Add support for the rohm,bd71815 power controller controller for the >>>>> reMarkable 2. >>>>> >>>>> Signed-off-by: Alistair Francis <alistair@alistair23.me> >>>>> --- >>>>> arch/arm/boot/dts/imx7d-remarkable2.dts | 159 ++++++++++++++++++++++++ >>>>> 1 file changed, 159 insertions(+) >>>>> >>>>> diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts b/arch/arm/boot/dts/imx7d-remarkable2.dts >>>>> index a138b292ec6a..4387d30d6180 100644 >>>>> --- a/arch/arm/boot/dts/imx7d-remarkable2.dts >>>>> +++ b/arch/arm/boot/dts/imx7d-remarkable2.dts >>>>> @@ -92,6 +92,10 @@ wifi_pwrseq: wifi_pwrseq { >>>>> }; >>>>> }; >>>>> >>>>> +&cpu0 { >>>>> + cpu-supply = <&buck1_reg>; >>>>> +}; >>>>> + >>>>> &clks { >>>>> assigned-clocks = <&clks IMX7D_CLKO2_ROOT_SRC>, >>>>> <&clks IMX7D_CLKO2_ROOT_DIV>; >>>>> @@ -119,6 +123,148 @@ wacom_digitizer: digitizer@9 { >>>>> }; >>>>> }; >>>>> >>>>> +&i2c2 { >>>>> + clock-frequency = <100000>; >>>> >>>> We can set this now to 400kHz since the driver can handle quirks now >>>> internal. >>> >>> After a discussion on the first patch I'm going to leave this as is. >>> Let me know if you do want me to change it. >>> >>>> >>>>> + pinctrl-names = "default", "sleep"; >>>>> + pinctrl-0 = <&pinctrl_i2c2>; >>>>> + pinctrl-1 = <&pinctrl_i2c2>; >>>> >>>> Do we need a "sleep" config here since this will be the same >>>> configuration as the "default" one. >>> >>> Nope, dropped. >>> >>>> >>>>> + status = "okay"; >>>>> + >>>>> + bd71815: pmic@4b { >>>>> + compatible = "rohm,bd71815"; >>>>> + reg = <0x4b>; >>>>> + pinctrl-names = "default"; >>>>> + pinctrl-0 = <&pinctrl_bd71815>; >>>>> + interrupt-parent = <&gpio6>; /* PMIC_INT_B GPIO6_IO16 */ >>>>> + interrupts = <16 IRQ_TYPE_LEVEL_LOW>; >>>>> + gpio-controller; >>>>> + clocks = <&clks IMX7D_CLKO2_ROOT_SRC>; >>>>> + clock-output-names = "bd71815-32k-out"; >>>>> + #clock-cells = <0>; >>>>> + #gpio-cells = <1>; >>>>> + >>>>> + regulators { >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + >>>>> + buck1_reg: regulator@0 { >>>>> + reg = <0>; >>>>> + regulator-compatible = "buck1"; >>>>> + regulator-min-microvolt = <800000>; >>>>> + regulator-max-microvolt = <2000000>; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + regulator-ramp-delay = <1250>; >>>>> + }; >>>>> + >>>>> + buck2_reg: regulator@1 { >>>>> + reg = <1>; >>>>> + regulator-compatible = "buck2"; >>>>> + regulator-min-microvolt = <800000>; >>>>> + regulator-max-microvolt = <2000000>; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + regulator-ramp-delay = <1250>; >>>>> + }; >>>>> + >>>>> + buck3_reg: regulator@2 { >>>>> + reg = <2>; >>>>> + regulator-compatible = "buck3"; >>>>> + regulator-min-microvolt = <1200000>; >>>>> + regulator-max-microvolt = <2700000>; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + }; >>>>> + >>>>> + buck4_reg: regulator@3 { >>>>> + reg = <3>; >>>>> + regulator-compatible = "buck4"; >>>>> + regulator-min-microvolt = <1100000>; >>>>> + regulator-max-microvolt = <1850000>; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + }; >>>>> + >>>>> + buck5_reg: regulator@4 { >>>>> + reg = <4>; >>>>> + regulator-compatible = "buck5"; >>>>> + regulator-min-microvolt = <1800000>; >>>>> + regulator-max-microvolt = <3300000>; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + }; >>>>> + >>>>> + ldo1_reg: regulator@5 { >>>>> + reg = <5>; >>>>> + regulator-compatible = "ldo1"; >>>>> + regulator-min-microvolt = <800000>; >>>>> + regulator-max-microvolt = <3300000>; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + }; >>>>> + >>>>> + ldo2_reg: regulator@6 { >>>>> + reg = <6>; >>>>> + regulator-compatible = "ldo2"; >>>>> + regulator-min-microvolt = <800000>; >>>>> + regulator-max-microvolt = <3300000>; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + }; >>>>> + >>>>> + ldo3_reg: regulator@7 { >>>>> + reg = <7>; >>>>> + regulator-compatible = "ldo3"; >>>>> + regulator-min-microvolt = <800000>; >>>>> + regulator-max-microvolt = <3300000>; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + }; >>>>> + >>>>> + ldo4_reg: regulator@8 { >>>>> + reg = <8>; >>>>> + regulator-compatible = "ldo4"; >>>>> + regulator-min-microvolt = <800000>; >>>>> + regulator-max-microvolt = <3300000>; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + }; >>>>> + >>>>> + ldo5_reg: regulator@9 { >>>>> + reg = <9>; >>>>> + regulator-compatible = "ldo5"; >>>>> + regulator-min-microvolt = <800000>; >>>>> + regulator-max-microvolt = <3300000>; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + }; >>>>> + >>>>> + dvref_reg: regulator@a { >>>>> + reg = <0xa>; >>>>> + regulator-compatible = "dvref"; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + }; >>>>> + >>>>> + lpsr_reg: regulator@b { >>>>> + reg = <0xb>; >>>>> + regulator-compatible = "lpsr"; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + }; >>>>> + >>>>> + wled_reg: regulator@c { >>>>> + reg = <0xc>; >>>>> + regulator-compatible = "wled"; >>>>> + regulator-min-microamp = <10>; >>>>> + regulator-max-microamp = <25000>; >>>>> + regulator-boot-on; >>>>> + regulator-always-on; >>>>> + }; >>>> >>>> Note: You have marked all regulators as always on, this is rather >>>> suboptimal due to power consumption. >>> >>> Good point. The power consumption is already pretty bad, as mainline >>> doesn't support the i.MX deep sleep. As I know this works I'd like to >>> keep it as is. I'll test to see if removing them doesn't break >>> anything and then send a follow up patch. >> >> I'd like to suggest also seeing what removing the "regulator-boot-on" >> causes. As far as I understand, the "regulator-boot-on" mainly (only?) >> intended to be used in situations where the regulator state can not be >> reliably read from the hardware at the boot-up. AFAIR, unlike the >> BD71837 and BD71847, >> the BD71815 does not have this limitation. (This is just my >> understanding - which is based on some age-old discussions with Mark >> Brown - hence this comment is just a suggestion to try this while >> testing, not a comment intended to be demanding for a change) > > I have tried removing both "regulator-boot-on" and "regulator-always-on". > > After one boot to userspace (where the display didn't work correctly) > the device now seems to not power up at all. Even u-boot is failing to > start. I'm not sure if the regulators are now in some strange state or > if something is fried.
Ouch. This does not sound good at all. Sorry for a bad advice. :(
I can only assume the "power dependencies" are not modelled correctly - Eg. some required power-rails are not marked to be used by devices that actually need them. This is probably being worked around using the regulator-boot-on / regulator-always-on - which will cause the Linux to turn on these rails when the driver is load. My understanding is that this is not the intended use-case for the regulator-boot-on flag though. Also, as was said - the side-effect of always-on is that there will be no power-saving options - which is still better for an user than a fried device would be :|
> > I'm currently waiting for the battery to discharge to see if that helps. >
/me keeps fingers crossed. You wouldn't guess how hard it was to type this!
Yours, -- Matti
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |