Date:   Tue, 5 Oct 2021 08:29:29 +0300
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Shawn Guo <shawnguo@...nel.org>, thanushree.sreerama@...rion.com
Cc:     robh+dt@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
        festevam@...il.com, linux-imx@....com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] arm64: dts: imx8mm: Add support for emtrion emCON-MX8M
 Mini

Hi All,

Nice to see more boards added :) Thanks guys!

On 10/4/21 07:21, Shawn Guo wrote:
> On Wed, Sep 01, 2021 at 02:28:26PM +0200, thanushree.sreerama@...rion.com wrote:
>> This patch adds support for the emtrion GmbH emCON-MX8M Mini modules.
>> They are available with NXP i.MX 8M Mini equipped with 2 or 4 GB Memory.
>>
>> The devicetree imx8mm-emcon.dtsi is the common part providing all
>> module components and the basic support for the SoC. The support for the
>> avari baseboard in the developer-kit configuration is provided by the
>> emcon-avari dts files.
>>
>> Signed-off-by: Thanushree Sreerama <thanushree.sreerama@...rion.com>
>> Reviewed by: Frank Erdrich <frank.erdrich@...rion.com>
>> ---

snip

>> +&i2c3 {
>> +	clock-frequency = <400000>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_i2c3>;
>> +	status = "okay";
>> +
>> +	pmic: bd71837@4b {
> 
> bd71837: pmic@4b
> 
>> +		compatible = "rohm,bd71847";
> 
> So `bd71847: pmic@4b` maybe?
> 
>> +		reg = <0x4b>;
>> +		pinctrl-0 = <&pinctrl_pmic>;
>> +		interrupt-parent = <&gpio3>;
>> +		interrupts = <2 GPIO_ACTIVE_LOW>;

This looks fishy to me. What I would expect to see here is the
IRQ_TYPE_LEVEL_LOW - which equals to 8. (AFAIR the PMIC HW is built to 
work with level active IRQs, and the PMIC does pull the line low when 
IRQ occurs). I don't think the GPIO_ACTIVE_LOW is intended to be used as 
IRQ type specifier(?). I admit I didn't look the IRQ controller your SoC 
has though. The GPIO_ACTIVE_LOW equals to 1 - which actually equals 
IRQ_TYPE_EDGE_RISING.

I probably don't understand something as I don't see how the setup could 
produce any rising edges - because rising edges should occur only when 
we ACK the IRQ. Any explanations/education appreciated here :)

Feel free to ignore this comment if it makes no sense :)

>> +		rohm,reset-snvs-powered;
>> +
>> +		regulators {
>> +			buck1_reg: BUCK1 {
>> +				regulator-name = "BUCK1";
>> +				regulator-min-microvolt = <700000>;
>> +				regulator-max-microvolt = <1300000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +				regulator-ramp-delay = <1250>;
>> +			};
>> +
>> +			buck2_reg: BUCK2 {
>> +				regulator-name = "BUCK2";
>> +				regulator-min-microvolt = <700000>;
>> +				regulator-max-microvolt = <1300000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +				regulator-ramp-delay = <1250>;
>> +				rohm,dvs-run-voltage = <1000000>;
>> +				rohm,dvs-idle-voltage = <900000>;
>> +			};
>> +
>> +			buck3_reg: BUCK3 {
>> +				// BUCK5 in datasheet
>> +				regulator-name = "BUCK3";
>> +				regulator-min-microvolt = <700000>;
>> +				regulator-max-microvolt = <1350000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			buck4_reg: BUCK4 {
>> +				// BUCK6 in datasheet
>> +				regulator-name = "BUCK4";
>> +				regulator-min-microvolt = <3000000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			buck5_reg: BUCK5 {
>> +				// BUCK7 in datasheet
>> +				regulator-name = "BUCK5";
>> +				regulator-min-microvolt = <1605000>;
>> +				regulator-max-microvolt = <1995000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			buck6_reg: BUCK6 {
>> +				// BUCK8 in datasheet
>> +				regulator-name = "BUCK6";
>> +				regulator-min-microvolt = <800000>;
>> +				regulator-max-microvolt = <1400000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo1_reg: LDO1 {
>> +				regulator-name = "LDO1";
>> +				regulator-min-microvolt = <1600000>;
>> +				regulator-max-microvolt = <1900000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo2_reg: LDO2 {
>> +				regulator-name = "LDO2";
>> +				regulator-min-microvolt = <800000>;
>> +				regulator-max-microvolt = <900000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo3_reg: LDO3 {
>> +				regulator-name = "LDO3";
>> +				regulator-min-microvolt = <1800000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo4_reg: LDO4 {
>> +				regulator-name = "LDO4";
>> +				regulator-min-microvolt = <900000>;
>> +				regulator-max-microvolt = <1800000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo6_reg: LDO6 {
>> +				regulator-name = "LDO6";
>> +				regulator-min-microvolt = <900000>;
>> +				regulator-max-microvolt = <1800000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
> 
> You really want to have all these regulators boot-on and always-on?
Following text is more like a note to one who considers changing this 
than actual review comment.

The correctness of this is on a gray area for the BD71847. The BD71847 
regulator's enable state can be either controlled by the SW or by the HW 
state machine. When controlled by the HW state machine, the SW can't 
easily(*) read the regulator status. This could probably/perhaps warrant 
the use of the regulator-boot-on.

Following may not be relevant for a device-tree description - but is 
relevant for one who wants the setup to work:
The BD71837/47/50 have a 'feature' that if a regulator enable state is 
controlled by SW - and if the PMIC goes to SNVS state and not to READY 
state when reset occurs - then at next boot the regulators which were 
controlled by SW do not power-on. In order to avoid potential boot 
failures at reset the BD71837/47/50 regulators which are marked as 
boot-on and always-on are not taken under SW control if reset target is 
SNVS. One could think the condition in driver code is slightly wrong. It 
might not feel ideal that we require both always-on AND boot-on to be 
present in order to not take the regulator under SW control. Probably 
only one of those should be enough? Well, leaving regulators with only 
the always-on defined under the HW state machine control would break 
things with setups that want the regulators to stay on at PMICs SUSPEND 
state. So, in a sense requiring both to be present is correct - and 
having boot-on kind of makes sense.

In this particular case I see the SNVS is used as reset target state and 
both the always-on and boot-on is given to all regulators - which means 
all of the regulators are left under HW state machine control. It's good 
to know what are the implications of changing these properties :)

(*)In most cases the SW is running on i.MX8 core powered by the PMIC. 
This means the PMIC is most probably at RUN state when SW is running. 
AFAIR the current HW state can also be read from the registers. So most, 
or maybe all of the regulator enable states could probably be determined 
by the HW state even when controlled by the HW state machine. I do not 
remember if PMIC has couple of OTP configurable regulators which enable 
state can be different on different HW states depending on the OTP 
settings - or whether that configuration can be read from the PMIC. In 
any case, if we don't have boot-on and always-on, we do take regulators 
under SW control - in which case we can always read the current state. 
If we have a boot-on and always-on, we may leave regulators under HW 
state machine control - and we do not control the state by SW and we do 
not implement any complex logic for reading the state.


Best Regards
--Matti Vaittinen

Powered by blists - more mailing lists