Re: [PATCH] arm64: dts: imx8mm: Add support for emtrion emCON-MX8M Mini

From: Matti Vaittinen
Date: Tue Oct 05 2021 - 01:29:36 EST

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@xxxxxxxxxxx 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@xxxxxxxxxxx>
Reviewed by: Frank Erdrich <frank.erdrich@xxxxxxxxxxx>
---

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