[PATCH v2 1/1] ARM: i.MX8M: add PCA9450 PMIC on rev-b EVK

[PATCH v2 1/1] ARM: i.MX8M: add PCA9450 PMIC on rev-b EVK

[Johannes Schneider]

Configure and setup the PMIC found on rev-b EVKs.
The code is algiend with how imx8mn-evk handles both
PMIC variants: pca9450 vs bd71837

Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
---
 arch/arm/boards/nxp-imx8mm-evk/lowlevel.c | 51 +++++++++++++++--------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
index 6132df53ec..409554c2d5 100644
--- a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
+++ b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
@@ -16,6 +16,7 @@
 #include <mach/iomux-mx8mm.h>
 #include <mach/imx8m-ccm-regs.h>
 #include <mfd/bd71837.h>
+#include <mfd/pca9450.h>
 #include <mach/xload.h>
 #include <soc/imx8m/ddr.h>
 #include <image-metadata.h>
@@ -38,6 +39,25 @@ static void setup_uart(void)
 	putc_ll('>');
 }
 
+static struct pmic_config pca9450_cfg[] = {
+	/* BUCKxOUT_DVS0/1 control BUCK123 output */
+	{ PCA9450_BUCK123_DVS, 0x29 },
+	/*
+	 * increase VDD_SOC to typical value 0.95V before first
+	 * DRAM access, set DVS1 to 0.85v for suspend.
+	 * Enable DVS control through PMIC_STBY_REQ and
+	 * set B1_ENMODE=1 (ON by PMIC_ON_REQ=H)
+	 */
+	{ PCA9450_BUCK1OUT_DVS0, 0x1C },
+	/* Set DVS1 to 0.85v for suspend */
+	/* Enable DVS control through PMIC_STBY_REQ and set B1_ENMODE=1 (ON by PMIC_ON_REQ=H) */
+	{ PCA9450_BUCK1OUT_DVS1, 0x14 },
+	{ PCA9450_BUCK1CTRL, 0x59 },
+
+	/* set WDOG_B_CFG to cold reset */
+	{ PCA9450_RESET_CTRL, 0xA1 },
+};
+
 static struct pmic_config bd71837_cfg[] = {
 	/* decrease RESET key long push time from the default 10s to 10ms */
 	{ BD718XX_PWRONCONFIG1, 0x0 },
@@ -51,21 +71,6 @@ static struct pmic_config bd71837_cfg[] = {
 	{ BD718XX_REGLOCK, 0x11 },
 };
 
-static void power_init_board(void)
-{
-	struct pbl_i2c *i2c;
-
-	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
-	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
-
-	imx8mm_early_clock_init();
-	imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
-
-	i2c = imx8m_i2c_early_init(IOMEM(MX8MQ_I2C1_BASE_ADDR));
-
-	pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
-}
-
 extern struct dram_timing_info imx8mm_evk_dram_timing;
 
 static void start_atf(void)
@@ -78,8 +83,20 @@ static void start_atf(void)
 	if (current_el() != 3)
 		return;
 
-	power_init_board();
-	imx8mm_ddr_init(&imx8mm_evk_dram_timing, DRAM_TYPE_LPDDR4);
+	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
+	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
+
+	imx8mm_early_clock_init();
+	imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
+
+	i2c = imx8m_i2c_early_init(IOMEM(MX8MM_I2C1_BASE_ADDR));
+
+	imx8mm_ddr_init(&imx8mm_evk_lpddr4_timing, DRAM_TYPE_LPDDR4);
+	if (i2c_dev_probe(i2c, 0x25, true) == 0) {
+		pmic_configure(i2c, 0x25, pca9450_cfg, ARRAY_SIZE(pca9450_cfg));
+	} else {
+		pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
+	}
 
 	imx8mm_load_and_start_image_via_tfa();
 }
-- 
2.25.1

[PATCH v2 1/1] ARM: i.MX8M: fix typo in function call

[Johannes Schneider]

Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
---
 arch/arm/boards/nxp-imx8mp-evk/lowlevel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
index 4f24dd4cd4..3cb24df1ca 100644
--- a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
+++ b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
@@ -72,7 +72,7 @@ static void power_init_board(void)
 	imx8mp_setup_pad(MX8MP_PAD_I2C1_SCL__I2C1_SCL | I2C_PAD_CTRL);
 	imx8mp_setup_pad(MX8MP_PAD_I2C1_SDA__I2C1_SDA | I2C_PAD_CTRL);
 
-	imx8mm_early_clock_init();
+	imx8mp_early_clock_init();
 	imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
 
 	i2c = imx8m_i2c_early_init(IOMEM(MX8MP_I2C1_BASE_ADDR));
-- 
2.25.1

[PATCH v2 1/1] ARM: i.MX8M: smccc: fix firmware_atf check

[Johannes Schneider]

Replace the config-switch check with a check for the current execution
level.

Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
---
 arch/arm/mach-imx/imx8m.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/imx8m.c b/arch/arm/mach-imx/imx8m.c
index 9758525b54..bb2733f2ab 100644
--- a/arch/arm/mach-imx/imx8m.c
+++ b/arch/arm/mach-imx/imx8m.c
@@ -61,7 +61,7 @@ static int imx8m_init(const char *cputypestr)
 	pr_info("%s unique ID: %llx\n", cputypestr, imx8m_uid());
 
 	if (IS_ENABLED(CONFIG_ARM_SMCCC) &&
-	    IS_ENABLED(CONFIG_FIRMWARE_IMX8MQ_ATF)) {
+	    (current_el() == 2)) {
 		arm_smccc_smc(IMX_SIP_BUILDINFO,
 			      IMX_SIP_BUILDINFO_GET_COMMITHASH,
 			      0, 0, 0, 0, 0, 0, &res);
-- 
2.25.1

Re: [PATCH v2 1/1] ARM: i.MX8M: add PCA9450 PMIC on rev-b EVK

[Ahmad Fatoum]

Hi,

How do you generate your patches? Easiest is:

  git config sendemail.to barebox@lists.infradead.org
  git send-email -3 --annotate

This will take care to number the patches correctly.

On 11.01.23 11:01, Johannes Schneider wrote:
> Configure and setup the PMIC found on rev-b EVKs.
> The code is algiend with how imx8mn-evk handles both

aligned*

> PMIC variants: pca9450 vs bd71837

Please note which Boards you tested this on.

> 
> Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
> ---
>  arch/arm/boards/nxp-imx8mm-evk/lowlevel.c | 51 +++++++++++++++--------
>  1 file changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> index 6132df53ec..409554c2d5 100644
> --- a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> +++ b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> @@ -16,6 +16,7 @@
>  #include <mach/iomux-mx8mm.h>
>  #include <mach/imx8m-ccm-regs.h>
>  #include <mfd/bd71837.h>
> +#include <mfd/pca9450.h>
>  #include <mach/xload.h>
>  #include <soc/imx8m/ddr.h>
>  #include <image-metadata.h>
> @@ -38,6 +39,25 @@ static void setup_uart(void)
>  	putc_ll('>');
>  }
>  
> +static struct pmic_config pca9450_cfg[] = {
> +	/* BUCKxOUT_DVS0/1 control BUCK123 output */
> +	{ PCA9450_BUCK123_DVS, 0x29 },
> +	/*
> +	 * increase VDD_SOC to typical value 0.95V before first
> +	 * DRAM access, set DVS1 to 0.85v for suspend.
> +	 * Enable DVS control through PMIC_STBY_REQ and
> +	 * set B1_ENMODE=1 (ON by PMIC_ON_REQ=H)
> +	 */
> +	{ PCA9450_BUCK1OUT_DVS0, 0x1C },
> +	/* Set DVS1 to 0.85v for suspend */
> +	/* Enable DVS control through PMIC_STBY_REQ and set B1_ENMODE=1 (ON by PMIC_ON_REQ=H) */
> +	{ PCA9450_BUCK1OUT_DVS1, 0x14 },
> +	{ PCA9450_BUCK1CTRL, 0x59 },
> +
> +	/* set WDOG_B_CFG to cold reset */
> +	{ PCA9450_RESET_CTRL, 0xA1 },
> +};
> +
>  static struct pmic_config bd71837_cfg[] = {
>  	/* decrease RESET key long push time from the default 10s to 10ms */
>  	{ BD718XX_PWRONCONFIG1, 0x0 },
> @@ -51,21 +71,6 @@ static struct pmic_config bd71837_cfg[] = {
>  	{ BD718XX_REGLOCK, 0x11 },
>  };
>  
> -static void power_init_board(void)
> -{

Can you leave this function as-is? This will make the diff more
readable.

> -	struct pbl_i2c *i2c;
> -
> -	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
> -	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
> -
> -	imx8mm_early_clock_init();
> -	imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
> -
> -	i2c = imx8m_i2c_early_init(IOMEM(MX8MQ_I2C1_BASE_ADDR));
> -
> -	pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
> -}
> -
>  extern struct dram_timing_info imx8mm_evk_dram_timing;
>  
>  static void start_atf(void)
> @@ -78,8 +83,20 @@ static void start_atf(void)
>  	if (current_el() != 3)
>  		return;
>  
> -	power_init_board();
> -	imx8mm_ddr_init(&imx8mm_evk_dram_timing, DRAM_TYPE_LPDDR4);
> +	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
> +	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
> +
> +	imx8mm_early_clock_init();
> +	imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
> +
> +	i2c = imx8m_i2c_early_init(IOMEM(MX8MM_I2C1_BASE_ADDR));
> +
> +	imx8mm_ddr_init(&imx8mm_evk_lpddr4_timing, DRAM_TYPE_LPDDR4);
> +	if (i2c_dev_probe(i2c, 0x25, true) == 0) {
> +		pmic_configure(i2c, 0x25, pca9450_cfg, ARRAY_SIZE(pca9450_cfg));
> +	} else {
> +		pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
> +	}

Nitpick: You can drop the braces (Kernel coding style).

>  
>  	imx8mm_load_and_start_image_via_tfa();
>  }

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 1/1] ARM: i.MX8M: fix typo in function call

[Ahmad Fatoum]

On 11.01.23 11:01, Johannes Schneider wrote:
> Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>

Please add commit messages. Even for seemingly trivial changes, there
is always something that can be explained, e.g.

"While both functions are equivalent, it looks out-of-place to use
 the imx8mm_ variant in an i.MX8MN board's code and in future we may
 elect to configure clocking differently on the i.MX8MP."

Which brings us to:

> -	imx8mm_early_clock_init();
> +	imx8mp_early_clock_init();

Did you compile-test this? There's no imx8mp_early_clock_init defined..

>  	imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
>  
>  	i2c = imx8m_i2c_early_init(IOMEM(MX8MP_I2C1_BASE_ADDR));

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 1/1] ARM: i.MX8M: smccc: fix firmware_atf check

[Ahmad Fatoum]

On 11.01.23 11:01, Johannes Schneider wrote:
> Replace the config-switch check with a check for the current execution
> level.

Please describe your reasoning behind changes (e.g. IMX8MQ_ATF may be
deselected when building solely non-MQ boards). It's not evident otherwise,
why this is a fix.

> 
> Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
> ---
>  arch/arm/mach-imx/imx8m.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-imx/imx8m.c b/arch/arm/mach-imx/imx8m.c
> index 9758525b54..bb2733f2ab 100644
> --- a/arch/arm/mach-imx/imx8m.c
> +++ b/arch/arm/mach-imx/imx8m.c
> @@ -61,7 +61,7 @@ static int imx8m_init(const char *cputypestr)
>  	pr_info("%s unique ID: %llx\n", cputypestr, imx8m_uid());
>  
>  	if (IS_ENABLED(CONFIG_ARM_SMCCC) &&
> -	    IS_ENABLED(CONFIG_FIRMWARE_IMX8MQ_ATF)) {
> +	    (current_el() == 2)) {
>  		arm_smccc_smc(IMX_SIP_BUILDINFO,
>  			      IMX_SIP_BUILDINFO_GET_COMMITHASH,
>  			      0, 0, 0, 0, 0, 0, &res);

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 1/1] ARM: i.MX8M: add PCA9450 PMIC on rev-b EVK

[Ahmad Fatoum]

On 11.01.23 11:17, Ahmad Fatoum wrote:
> Hi,
> 
> How do you generate your patches? Easiest is:
> 
>   git config sendemail.to barebox@lists.infradead.org
>   git send-email -3 --annotate
> 
> This will take care to number the patches correctly.
> 
> On 11.01.23 11:01, Johannes Schneider wrote:
>> Configure and setup the PMIC found on rev-b EVKs.
>> The code is algiend with how imx8mn-evk handles both
> 
> aligned*
> 
>> PMIC variants: pca9450 vs bd71837
> 
> Please note which Boards you tested this on.

Also be aware that while you now do correct PMIC setup in PBL, you still
unconditionally use a DT with only Rohm PMIC. Given that you now have
a kernel DT for evkb. Can you add it to arch/arm/dts, enable it in the
Makefile and choose one or the other DT in PBL like i.MX8MN does?

Don't forget to add fsl,imx8mm-evkb to the match list in the board.c file.

>> Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
>> ---
>>  arch/arm/boards/nxp-imx8mm-evk/lowlevel.c | 51 +++++++++++++++--------
>>  1 file changed, 34 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
>> index 6132df53ec..409554c2d5 100644
>> --- a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
>> +++ b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
>> @@ -16,6 +16,7 @@
>>  #include <mach/iomux-mx8mm.h>
>>  #include <mach/imx8m-ccm-regs.h>
>>  #include <mfd/bd71837.h>
>> +#include <mfd/pca9450.h>
>>  #include <mach/xload.h>
>>  #include <soc/imx8m/ddr.h>
>>  #include <image-metadata.h>
>> @@ -38,6 +39,25 @@ static void setup_uart(void)
>>  	putc_ll('>');
>>  }
>>  
>> +static struct pmic_config pca9450_cfg[] = {
>> +	/* BUCKxOUT_DVS0/1 control BUCK123 output */
>> +	{ PCA9450_BUCK123_DVS, 0x29 },
>> +	/*
>> +	 * increase VDD_SOC to typical value 0.95V before first
>> +	 * DRAM access, set DVS1 to 0.85v for suspend.
>> +	 * Enable DVS control through PMIC_STBY_REQ and
>> +	 * set B1_ENMODE=1 (ON by PMIC_ON_REQ=H)
>> +	 */
>> +	{ PCA9450_BUCK1OUT_DVS0, 0x1C },
>> +	/* Set DVS1 to 0.85v for suspend */
>> +	/* Enable DVS control through PMIC_STBY_REQ and set B1_ENMODE=1 (ON by PMIC_ON_REQ=H) */
>> +	{ PCA9450_BUCK1OUT_DVS1, 0x14 },
>> +	{ PCA9450_BUCK1CTRL, 0x59 },
>> +
>> +	/* set WDOG_B_CFG to cold reset */
>> +	{ PCA9450_RESET_CTRL, 0xA1 },
>> +};
>> +
>>  static struct pmic_config bd71837_cfg[] = {
>>  	/* decrease RESET key long push time from the default 10s to 10ms */
>>  	{ BD718XX_PWRONCONFIG1, 0x0 },
>> @@ -51,21 +71,6 @@ static struct pmic_config bd71837_cfg[] = {
>>  	{ BD718XX_REGLOCK, 0x11 },
>>  };
>>  
>> -static void power_init_board(void)
>> -{
> 
> Can you leave this function as-is? This will make the diff more
> readable.
> 
>> -	struct pbl_i2c *i2c;
>> -
>> -	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
>> -	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
>> -
>> -	imx8mm_early_clock_init();
>> -	imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
>> -
>> -	i2c = imx8m_i2c_early_init(IOMEM(MX8MQ_I2C1_BASE_ADDR));
>> -
>> -	pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
>> -}
>> -
>>  extern struct dram_timing_info imx8mm_evk_dram_timing;
>>  
>>  static void start_atf(void)
>> @@ -78,8 +83,20 @@ static void start_atf(void)
>>  	if (current_el() != 3)
>>  		return;
>>  
>> -	power_init_board();
>> -	imx8mm_ddr_init(&imx8mm_evk_dram_timing, DRAM_TYPE_LPDDR4);
>> +	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
>> +	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
>> +
>> +	imx8mm_early_clock_init();
>> +	imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
>> +
>> +	i2c = imx8m_i2c_early_init(IOMEM(MX8MM_I2C1_BASE_ADDR));
>> +
>> +	imx8mm_ddr_init(&imx8mm_evk_lpddr4_timing, DRAM_TYPE_LPDDR4);
>> +	if (i2c_dev_probe(i2c, 0x25, true) == 0) {
>> +		pmic_configure(i2c, 0x25, pca9450_cfg, ARRAY_SIZE(pca9450_cfg));
>> +	} else {
>> +		pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
>> +	}
> 
> Nitpick: You can drop the braces (Kernel coding style).
> 
>>  
>>  	imx8mm_load_and_start_image_via_tfa();
>>  }
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 1/1] ARM: i.MX8M: add PCA9450 PMIC on rev-b EVK

[Marco Felsch]

On 23-01-11, Ahmad Fatoum wrote:
> Hi,
> 
> How do you generate your patches? Easiest is:
> 
>   git config sendemail.to barebox@lists.infradead.org
>   git send-email -3 --annotate
> 
> This will take care to number the patches correctly.
> 
> On 11.01.23 11:01, Johannes Schneider wrote:
> > Configure and setup the PMIC found on rev-b EVKs.
> > The code is algiend with how imx8mn-evk handles both
> 
> aligned*
> 
> > PMIC variants: pca9450 vs bd71837
> 
> Please note which Boards you tested this on.
> 
> > 
> > Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
> > ---
> >  arch/arm/boards/nxp-imx8mm-evk/lowlevel.c | 51 +++++++++++++++--------
> >  1 file changed, 34 insertions(+), 17 deletions(-)
> > 
> > diff --git a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> > index 6132df53ec..409554c2d5 100644
> > --- a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> > +++ b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> > @@ -16,6 +16,7 @@
> >  #include <mach/iomux-mx8mm.h>
> >  #include <mach/imx8m-ccm-regs.h>
> >  #include <mfd/bd71837.h>
> > +#include <mfd/pca9450.h>
> >  #include <mach/xload.h>
> >  #include <soc/imx8m/ddr.h>
> >  #include <image-metadata.h>
> > @@ -38,6 +39,25 @@ static void setup_uart(void)
> >  	putc_ll('>');
> >  }
> >  
> > +static struct pmic_config pca9450_cfg[] = {
> > +	/* BUCKxOUT_DVS0/1 control BUCK123 output */
> > +	{ PCA9450_BUCK123_DVS, 0x29 },
> > +	/*
> > +	 * increase VDD_SOC to typical value 0.95V before first
> > +	 * DRAM access, set DVS1 to 0.85v for suspend.
> > +	 * Enable DVS control through PMIC_STBY_REQ and
> > +	 * set B1_ENMODE=1 (ON by PMIC_ON_REQ=H)
> > +	 */
> > +	{ PCA9450_BUCK1OUT_DVS0, 0x1C },
> > +	/* Set DVS1 to 0.85v for suspend */
> > +	/* Enable DVS control through PMIC_STBY_REQ and set B1_ENMODE=1 (ON by PMIC_ON_REQ=H) */
> > +	{ PCA9450_BUCK1OUT_DVS1, 0x14 },
> > +	{ PCA9450_BUCK1CTRL, 0x59 },
> > +
> > +	/* set WDOG_B_CFG to cold reset */
> > +	{ PCA9450_RESET_CTRL, 0xA1 },
> > +};

While picking this patch I noticed that you're configuring the PMIC
differently compared to upstream u-boot. What is the reason for this?

Regards,
  Marco

> > +
> >  static struct pmic_config bd71837_cfg[] = {
> >  	/* decrease RESET key long push time from the default 10s to 10ms */
> >  	{ BD718XX_PWRONCONFIG1, 0x0 },
> > @@ -51,21 +71,6 @@ static struct pmic_config bd71837_cfg[] = {
> >  	{ BD718XX_REGLOCK, 0x11 },
> >  };
> >  
> > -static void power_init_board(void)
> > -{
> 
> Can you leave this function as-is? This will make the diff more
> readable.
> 
> > -	struct pbl_i2c *i2c;
> > -
> > -	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
> > -	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
> > -
> > -	imx8mm_early_clock_init();
> > -	imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
> > -
> > -	i2c = imx8m_i2c_early_init(IOMEM(MX8MQ_I2C1_BASE_ADDR));
> > -
> > -	pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
> > -}
> > -
> >  extern struct dram_timing_info imx8mm_evk_dram_timing;
> >  
> >  static void start_atf(void)
> > @@ -78,8 +83,20 @@ static void start_atf(void)
> >  	if (current_el() != 3)
> >  		return;
> >  
> > -	power_init_board();
> > -	imx8mm_ddr_init(&imx8mm_evk_dram_timing, DRAM_TYPE_LPDDR4);
> > +	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
> > +	imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
> > +
> > +	imx8mm_early_clock_init();
> > +	imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
> > +
> > +	i2c = imx8m_i2c_early_init(IOMEM(MX8MM_I2C1_BASE_ADDR));
> > +
> > +	imx8mm_ddr_init(&imx8mm_evk_lpddr4_timing, DRAM_TYPE_LPDDR4);
> > +	if (i2c_dev_probe(i2c, 0x25, true) == 0) {
> > +		pmic_configure(i2c, 0x25, pca9450_cfg, ARRAY_SIZE(pca9450_cfg));
> > +	} else {
> > +		pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
> > +	}
> 
> Nitpick: You can drop the braces (Kernel coding style).
> 
> >  
> >  	imx8mm_load_and_start_image_via_tfa();
> >  }
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> 
> 

Re: [PATCH v2 1/1] ARM: i.MX8M: add PCA9450 PMIC on rev-b EVK

[SCHNEIDER Johannes]

Hoi

> > Hi,
> >
> > How do you generate your patches? Easiest is:
> >
> >   git config sendemail.to barebox@lists.infradead.org
> >   git send-email -3 --annotate
> >
> > This will take care to number the patches correctly.
> >
> > On 11.01.23 11:01, Johannes Schneider wrote:
> > > Configure and setup the PMIC found on rev-b EVKs.
> > > The code is algiend with how imx8mn-evk handles both
> >
> > aligned*
> >
> > > PMIC variants: pca9450 vs bd71837
> >
> > Please note which Boards you tested this on.
> >
> > >
> > > Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
> > > ---
> > >  arch/arm/boards/nxp-imx8mm-evk/lowlevel.c | 51 +++++++++++++++--------
> > >  1 file changed, 34 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> > > index 6132df53ec..409554c2d5 100644
> > > --- a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> > > +++ b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> > > @@ -16,6 +16,7 @@
> > >  #include <mach/iomux-mx8mm.h>
> > >  #include <mach/imx8m-ccm-regs.h>
> > >  #include <mfd/bd71837.h>
> > > +#include <mfd/pca9450.h>
> > >  #include <mach/xload.h>
> > >  #include <soc/imx8m/ddr.h>
> > >  #include <image-metadata.h>
> > > @@ -38,6 +39,25 @@ static void setup_uart(void)
> > >     putc_ll('>');
> > >  }
> > >
> > > +static struct pmic_config pca9450_cfg[] = {
> > > +   /* BUCKxOUT_DVS0/1 control BUCK123 output */
> > > +   { PCA9450_BUCK123_DVS, 0x29 },
> > > +   /*
> > > +    * increase VDD_SOC to typical value 0.95V before first
> > > +    * DRAM access, set DVS1 to 0.85v for suspend.
> > > +    * Enable DVS control through PMIC_STBY_REQ and
> > > +    * set B1_ENMODE=1 (ON by PMIC_ON_REQ=H)
> > > +    */
> > > +   { PCA9450_BUCK1OUT_DVS0, 0x1C },
> > > +   /* Set DVS1 to 0.85v for suspend */
> > > +   /* Enable DVS control through PMIC_STBY_REQ and set B1_ENMODE=1 (ON by PMIC_ON_REQ=H) */
> > > +   { PCA9450_BUCK1OUT_DVS1, 0x14 },
> > > +   { PCA9450_BUCK1CTRL, 0x59 },
> > > +
> > > +   /* set WDOG_B_CFG to cold reset */
> > > +   { PCA9450_RESET_CTRL, 0xA1 },
> > > +};
>
> While picking this patch I noticed that you're configuring the PMIC
> differently compared to upstream u-boot. What is the reason for this?
>
> Regards,
>   Marco
>

very observant, the init sequence matches the 8mp and 8mn already in barebox.
uboot 8mn and barebox 8mn differ too

as far as i can see the "only" differences are related to standby?
PCA9450_BUCK1OUT_DVS1 0x10  vs  0x1c

on our project hardware the settings seemed to work fine - but standby was not tested :-s

-> should i "blindly" adopt the upstream u-boot settings?
or is it preferable to keep all three boards (mp, mn and now mm) the same?


> > > +
> > >  static struct pmic_config bd71837_cfg[] = {
> > >     /* decrease RESET key long push time from the default 10s to 10ms */
> > >     { BD718XX_PWRONCONFIG1, 0x0 },
> > > @@ -51,21 +71,6 @@ static struct pmic_config bd71837_cfg[] = {
> > >     { BD718XX_REGLOCK, 0x11 },
> > >  };
> > >
> > > -static void power_init_board(void)
> > > -{
> >
> > Can you leave this function as-is? This will make the diff more
> > readable.
> >
> > > -   struct pbl_i2c *i2c;
> > > -
> > > -   imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
> > > -   imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
> > > -
> > > -   imx8mm_early_clock_init();
> > > -   imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
> > > -
> > > -   i2c = imx8m_i2c_early_init(IOMEM(MX8MQ_I2C1_BASE_ADDR));
> > > -
> > > -   pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
> > > -}
> > > -
> > >  extern struct dram_timing_info imx8mm_evk_dram_timing;
> > >
> > >  static void start_atf(void)
> > > @@ -78,8 +83,20 @@ static void start_atf(void)
> > >     if (current_el() != 3)
> > >             return;
> > >
> > > -   power_init_board();
> > > -   imx8mm_ddr_init(&imx8mm_evk_dram_timing, DRAM_TYPE_LPDDR4);
> > > +   imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
> > > +   imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
> > > +
> > > +   imx8mm_early_clock_init();
> > > +   imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
> > > +
> > > +   i2c = imx8m_i2c_early_init(IOMEM(MX8MM_I2C1_BASE_ADDR));
> > > +
> > > +   imx8mm_ddr_init(&imx8mm_evk_lpddr4_timing, DRAM_TYPE_LPDDR4);
> > > +   if (i2c_dev_probe(i2c, 0x25, true) == 0) {
> > > +           pmic_configure(i2c, 0x25, pca9450_cfg, ARRAY_SIZE(pca9450_cfg));
> > > +   } else {
> > > +           pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
> > > +   }
> >
> > Nitpick: You can drop the braces (Kernel coding style).
> >
> > >
> > >     imx8mm_load_and_start_image_via_tfa();
> > >  }
> >

gruß
Johannes

________________________________________
From: Marco Felsch <m.felsch@pengutronix.de>
Sent: Thursday, January 19, 2023 09:59
To: Ahmad Fatoum
Cc: SCHNEIDER Johannes; barebox@lists.infradead.org
Subject: Re: [PATCH v2 1/1] ARM: i.MX8M: add PCA9450 PMIC on rev-b EVK

This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.


On 23-01-11, Ahmad Fatoum wrote:
> Hi,
>
> How do you generate your patches? Easiest is:
>
>   git config sendemail.to barebox@lists.infradead.org
>   git send-email -3 --annotate
>
> This will take care to number the patches correctly.
>
> On 11.01.23 11:01, Johannes Schneider wrote:
> > Configure and setup the PMIC found on rev-b EVKs.
> > The code is algiend with how imx8mn-evk handles both
>
> aligned*
>
> > PMIC variants: pca9450 vs bd71837
>
> Please note which Boards you tested this on.
>
> >
> > Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
> > ---
> >  arch/arm/boards/nxp-imx8mm-evk/lowlevel.c | 51 +++++++++++++++--------
> >  1 file changed, 34 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> > index 6132df53ec..409554c2d5 100644
> > --- a/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> > +++ b/arch/arm/boards/nxp-imx8mm-evk/lowlevel.c
> > @@ -16,6 +16,7 @@
> >  #include <mach/iomux-mx8mm.h>
> >  #include <mach/imx8m-ccm-regs.h>
> >  #include <mfd/bd71837.h>
> > +#include <mfd/pca9450.h>
> >  #include <mach/xload.h>
> >  #include <soc/imx8m/ddr.h>
> >  #include <image-metadata.h>
> > @@ -38,6 +39,25 @@ static void setup_uart(void)
> >     putc_ll('>');
> >  }
> >
> > +static struct pmic_config pca9450_cfg[] = {
> > +   /* BUCKxOUT_DVS0/1 control BUCK123 output */
> > +   { PCA9450_BUCK123_DVS, 0x29 },
> > +   /*
> > +    * increase VDD_SOC to typical value 0.95V before first
> > +    * DRAM access, set DVS1 to 0.85v for suspend.
> > +    * Enable DVS control through PMIC_STBY_REQ and
> > +    * set B1_ENMODE=1 (ON by PMIC_ON_REQ=H)
> > +    */
> > +   { PCA9450_BUCK1OUT_DVS0, 0x1C },
> > +   /* Set DVS1 to 0.85v for suspend */
> > +   /* Enable DVS control through PMIC_STBY_REQ and set B1_ENMODE=1 (ON by PMIC_ON_REQ=H) */
> > +   { PCA9450_BUCK1OUT_DVS1, 0x14 },
> > +   { PCA9450_BUCK1CTRL, 0x59 },
> > +
> > +   /* set WDOG_B_CFG to cold reset */
> > +   { PCA9450_RESET_CTRL, 0xA1 },
> > +};

While picking this patch I noticed that you're configuring the PMIC
differently compared to upstream u-boot. What is the reason for this?

Regards,
  Marco

> > +
> >  static struct pmic_config bd71837_cfg[] = {
> >     /* decrease RESET key long push time from the default 10s to 10ms */
> >     { BD718XX_PWRONCONFIG1, 0x0 },
> > @@ -51,21 +71,6 @@ static struct pmic_config bd71837_cfg[] = {
> >     { BD718XX_REGLOCK, 0x11 },
> >  };
> >
> > -static void power_init_board(void)
> > -{
>
> Can you leave this function as-is? This will make the diff more
> readable.
>
> > -   struct pbl_i2c *i2c;
> > -
> > -   imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
> > -   imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
> > -
> > -   imx8mm_early_clock_init();
> > -   imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
> > -
> > -   i2c = imx8m_i2c_early_init(IOMEM(MX8MQ_I2C1_BASE_ADDR));
> > -
> > -   pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
> > -}
> > -
> >  extern struct dram_timing_info imx8mm_evk_dram_timing;
> >
> >  static void start_atf(void)
> > @@ -78,8 +83,20 @@ static void start_atf(void)
> >     if (current_el() != 3)
> >             return;
> >
> > -   power_init_board();
> > -   imx8mm_ddr_init(&imx8mm_evk_dram_timing, DRAM_TYPE_LPDDR4);
> > +   imx8mm_setup_pad(IMX8MM_PAD_I2C1_SCL_I2C1_SCL);
> > +   imx8mm_setup_pad(IMX8MM_PAD_I2C1_SDA_I2C1_SDA);
> > +
> > +   imx8mm_early_clock_init();
> > +   imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
> > +
> > +   i2c = imx8m_i2c_early_init(IOMEM(MX8MM_I2C1_BASE_ADDR));
> > +
> > +   imx8mm_ddr_init(&imx8mm_evk_lpddr4_timing, DRAM_TYPE_LPDDR4);
> > +   if (i2c_dev_probe(i2c, 0x25, true) == 0) {
> > +           pmic_configure(i2c, 0x25, pca9450_cfg, ARRAY_SIZE(pca9450_cfg));
> > +   } else {
> > +           pmic_configure(i2c, 0x4b, bd71837_cfg, ARRAY_SIZE(bd71837_cfg));
> > +   }
>
> Nitpick: You can drop the braces (Kernel coding style).
>
> >
> >     imx8mm_load_and_start_image_via_tfa();
> >  }
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | https://eur02.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&data=05%7C01%7C%7C15dee1b2d380432d7f6908daf9fb861f%7C1b16ab3eb8f64fe39f3e2db7fe549f6a%7C0%7C0%7C638097155931579086%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=xtHVBfBVx5T1eSva9cJhzjNm90XisJl7Gf4hUraDUjc%3D&reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>
>
>