Hoi
> > Hi,
> >
> > How do you generate your patches? Easiest is:
> >
> > git config sendemail.to barebox@xxxxxxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxx>
Sent: Thursday, January 19, 2023 09:59
To: Ahmad Fatoum
Cc: SCHNEIDER Johannes; barebox@xxxxxxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > 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 |
>
>
>
