[1/4] clk: imx: clk-imx8mn Fix nand and spi clock parent

The osc_24m is the clock-output-name and not the one that
is used as internal name reference from the strcmp. The clock
that use osc_24m, will not be able to reparent it as they should.
We need anyway register the osc_24m clock fixed factor in the clock
tree.

Fixes: 710c4ffb890 ("clk: imx: clk-imx8mn add gpmi nand clocks")
Fixes: 85b1c11989c ("clk: imx: Add ECSPI to iMX8MN")
Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
 drivers/clk/imx/clk-imx8mn.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
  

Hi Michael,

On Sun, Jul 7, 2024 at 5:20 AM Michael Trimarchi
<michael@amarulasolutions.com> wrote:

> -static const char *imx8mn_ecspi1_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> +static const char *imx8mn_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",

The kernel uses osc_24m. Why does U-Boot need to be different?

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
  

Hi

On Wed, Jul 10, 2024 at 3:34 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Michael,
>
> On Sun, Jul 7, 2024 at 5:20 AM Michael Trimarchi
> <michael@amarulasolutions.com> wrote:
>
> > -static const char *imx8mn_ecspi1_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > +static const char *imx8mn_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
>
> The kernel uses osc_24m. Why does U-Boot need to be different?

clk dump

It's how uboot is registering the clk in dts. My idea now was not to
change it to not impact other architectures.
I have a lot of changes but I don't want to address regression and fix
and test before send more patches

MIchael
  

On Wed, Jul 10, 2024 at 10:39 AM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:

> clk dump
>
> It's how uboot is registering the clk in dts. My idea now was not to
> change it to not impact other architectures.
> I have a lot of changes but I don't want to address regression and fix
> and test before send more patches

Ok, I see that 'clock-osc-24m' is already used in U-Boot by the other
peripherals.

Your change looks correct.

Ideally, we should use 'osc_24m' in the future to align with the
kernel, but that's a different task.

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
  

HI Fabio

On Wed, Jul 10, 2024 at 3:43 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Wed, Jul 10, 2024 at 10:39 AM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
>
> > clk dump
> >
> > It's how uboot is registering the clk in dts. My idea now was not to
> > change it to not impact other architectures.
> > I have a lot of changes but I don't want to address regression and fix
> > and test before send more patches
>
> Ok, I see that 'clock-osc-24m' is already used in U-Boot by the other
> peripherals.
>
> Your change looks correct.
>
> Ideally, we should use 'osc_24m' in the future to align with the
> kernel, but that's a different task.

I will rephrase the commit message

Michael
  

On Thu, Jul 11, 2024 at 2:56 AM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:

> I will rephrase the commit message

It looks good as is.

Applied all, thanks.

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
  

On Sun, Jul 7, 2024 at 3:45 AM Michael Trimarchi
<michael@amarulasolutions.com> wrote:
>
> The osc_24m is the clock-output-name and not the one that
> is used as internal name reference from the strcmp. The clock
> that use osc_24m, will not be able to reparent it as they should.
> We need anyway register the osc_24m clock fixed factor in the clock
> tree.
>
> Fixes: 710c4ffb890 ("clk: imx: clk-imx8mn add gpmi nand clocks")
> Fixes: 85b1c11989c ("clk: imx: Add ECSPI to iMX8MN")
> Cc: Marek Vasut <marex@denx.de>
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
>  drivers/clk/imx/clk-imx8mn.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> index ed9e16d7c1..bfd1677520 100644
> --- a/drivers/clk/imx/clk-imx8mn.c
> +++ b/drivers/clk/imx/clk-imx8mn.c
> @@ -57,15 +57,15 @@ static const char *imx8mn_usdhc2_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
>                                            "sys_pll3_out", "sys_pll1_266m", "audio_pll2_out", "sys_pll1_100m", };
>
>  #if CONFIG_IS_ENABLED(DM_SPI)
> -static const char *imx8mn_ecspi1_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> +static const char *imx8mn_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
>                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
>                                            "sys_pll2_250m", "audio_pll2_out", };
>
> -static const char *imx8mn_ecspi2_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> +static const char *imx8mn_ecspi2_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
>                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
>                                            "sys_pll2_250m", "audio_pll2_out", };
>
> -static const char *imx8mn_ecspi3_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> +static const char *imx8mn_ecspi3_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
>                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
>                                            "sys_pll2_250m", "audio_pll2_out", };
>  #endif
> @@ -105,7 +105,7 @@ static const char *imx8mn_usdhc3_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
>  static const char *imx8mn_qspi_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sys_pll2_333m", "sys_pll2_500m",
>                                            "audio_pll2_out", "sys_pll1_266m", "sys_pll3_out", "sys_pll1_100m", };
>
> -static const char * const imx8mn_nand_sels[] = {"osc_24m", "sys_pll2_500m", "audio_pll1_out",
> +static const char * const imx8mn_nand_sels[] = {"clock-osc-24m", "sys_pll2_500m", "audio_pll1_out",
>                                                 "sys_pll1_400m", "audio_pll2_out", "sys_pll3_out",
>                                                 "sys_pll2_250m", "video_pll_out", };
>
> @@ -119,7 +119,9 @@ static const char * const imx8mn_usb_phy_sels[] = {"clock-osc-24m", "sys_pll1_10
>
>  static int imx8mn_clk_probe(struct udevice *dev)
>  {
> +       struct clk osc_24m_clk;
>         void __iomem *base;
> +       int ret;
>
>         base = (void *)ANATOP_BASE_ADDR;
>
> @@ -238,6 +240,11 @@ static int imx8mn_clk_probe(struct udevice *dev)
>         clk_dm(IMX8MN_SYS_PLL2_1000M,
>                imx_clk_fixed_factor("sys_pll2_1000m", "sys_pll2_out", 1, 1));
>

I know it's late, but I didn't get around to testing my Nano board for
a while.  Sorry for the delayed feedback...

> +       ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> +       if (ret)
> +               return ret;
> +       clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> +

These four lines appear to have introduced a regression on the
imx8mn-beacon board.  In the SPL phase, I get an error message
indicating it cannot find the i2c clk, then access to the PMIC fails.
If I remove these four lines, the error message disappears, and the
PMIC is happy again.

I have confirmed that CLK, and SPL_CLK are both defined.   I checked
the u-boot-spl.dtb and it shows clock-osc-24m with clock-output-names
= "osc_24m"

I have also tried to mark the 24m clock as with bootph-pre-ram;:
&{/clock-osc-24m} {
  bootph-pre-ram;
};

Unfortunately, nothing is helping, and I am not sure what else to try.
clk_fixed_rate.o is built into my SPL, so I don't think it's a config
issue.  I need to the PMIC to increase the voltage since we run the
Nano in overdrive mode to run the LPDDR4 at the highest speed.

Might anyone have any suggestions?

adam

>         base = dev_read_addr_ptr(dev);
>         if (!base)
>                 return -EINVAL;
> --
> 2.43.0
>

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
  

Hi Adam

On Mon, Nov 4, 2024 at 5:11 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Sun, Jul 7, 2024 at 3:45 AM Michael Trimarchi
> <michael@amarulasolutions.com> wrote:
> >
> > The osc_24m is the clock-output-name and not the one that
> > is used as internal name reference from the strcmp. The clock
> > that use osc_24m, will not be able to reparent it as they should.
> > We need anyway register the osc_24m clock fixed factor in the clock
> > tree.
> >
> > Fixes: 710c4ffb890 ("clk: imx: clk-imx8mn add gpmi nand clocks")
> > Fixes: 85b1c11989c ("clk: imx: Add ECSPI to iMX8MN")
> > Cc: Marek Vasut <marex@denx.de>
> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > ---
> >  drivers/clk/imx/clk-imx8mn.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> > index ed9e16d7c1..bfd1677520 100644
> > --- a/drivers/clk/imx/clk-imx8mn.c
> > +++ b/drivers/clk/imx/clk-imx8mn.c
> > @@ -57,15 +57,15 @@ static const char *imx8mn_usdhc2_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> >                                            "sys_pll3_out", "sys_pll1_266m", "audio_pll2_out", "sys_pll1_100m", };
> >
> >  #if CONFIG_IS_ENABLED(DM_SPI)
> > -static const char *imx8mn_ecspi1_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > +static const char *imx8mn_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> >                                            "sys_pll2_250m", "audio_pll2_out", };
> >
> > -static const char *imx8mn_ecspi2_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > +static const char *imx8mn_ecspi2_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> >                                            "sys_pll2_250m", "audio_pll2_out", };
> >
> > -static const char *imx8mn_ecspi3_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > +static const char *imx8mn_ecspi3_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> >                                            "sys_pll2_250m", "audio_pll2_out", };
> >  #endif
> > @@ -105,7 +105,7 @@ static const char *imx8mn_usdhc3_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> >  static const char *imx8mn_qspi_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sys_pll2_333m", "sys_pll2_500m",
> >                                            "audio_pll2_out", "sys_pll1_266m", "sys_pll3_out", "sys_pll1_100m", };
> >
> > -static const char * const imx8mn_nand_sels[] = {"osc_24m", "sys_pll2_500m", "audio_pll1_out",
> > +static const char * const imx8mn_nand_sels[] = {"clock-osc-24m", "sys_pll2_500m", "audio_pll1_out",
> >                                                 "sys_pll1_400m", "audio_pll2_out", "sys_pll3_out",
> >                                                 "sys_pll2_250m", "video_pll_out", };
> >
> > @@ -119,7 +119,9 @@ static const char * const imx8mn_usb_phy_sels[] = {"clock-osc-24m", "sys_pll1_10
> >
> >  static int imx8mn_clk_probe(struct udevice *dev)
> >  {
> > +       struct clk osc_24m_clk;
> >         void __iomem *base;
> > +       int ret;
> >
> >         base = (void *)ANATOP_BASE_ADDR;
> >
> > @@ -238,6 +240,11 @@ static int imx8mn_clk_probe(struct udevice *dev)
> >         clk_dm(IMX8MN_SYS_PLL2_1000M,
> >                imx_clk_fixed_factor("sys_pll2_1000m", "sys_pll2_out", 1, 1));
> >
>
> I know it's late, but I didn't get around to testing my Nano board for
> a while.  Sorry for the delayed feedback...
>
> > +       ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> > +       if (ret)
> > +               return ret;
> > +       clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> > +
>
> These four lines appear to have introduced a regression on the
> imx8mn-beacon board.  In the SPL phase, I get an error message
> indicating it cannot find the i2c clk, then access to the PMIC fails.
> If I remove these four lines, the error message disappears, and the
> PMIC is happy again.
>
> I have confirmed that CLK, and SPL_CLK are both defined.   I checked
> the u-boot-spl.dtb and it shows clock-osc-24m with clock-output-names
> = "osc_24m"
>
> I have also tried to mark the 24m clock as with bootph-pre-ram;:
> &{/clock-osc-24m} {
>   bootph-pre-ram;
> };
>
> Unfortunately, nothing is helping, and I am not sure what else to try.
> clk_fixed_rate.o is built into my SPL, so I don't think it's a config
> issue.  I need to the PMIC to increase the voltage since we run the
> Nano in overdrive mode to run the LPDDR4 at the highest speed.
>
> Might anyone have any suggestions?
>

Not yet. I have a similar setup but I don't set pmic, so I think that
the symbol is not find
in SPL and it just fail clock registration. Is that possible?

If (ret)

put here a print

Michael
> adam
>
> >         base = dev_read_addr_ptr(dev);
> >         if (!base)
> >                 return -EINVAL;
> > --
> > 2.43.0
> >
  

On Mon, Nov 4, 2024 at 10:29 AM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi Adam
>
> On Mon, Nov 4, 2024 at 5:11 PM Adam Ford <aford173@gmail.com> wrote:
> >
> > On Sun, Jul 7, 2024 at 3:45 AM Michael Trimarchi
> > <michael@amarulasolutions.com> wrote:
> > >
> > > The osc_24m is the clock-output-name and not the one that
> > > is used as internal name reference from the strcmp. The clock
> > > that use osc_24m, will not be able to reparent it as they should.
> > > We need anyway register the osc_24m clock fixed factor in the clock
> > > tree.
> > >
> > > Fixes: 710c4ffb890 ("clk: imx: clk-imx8mn add gpmi nand clocks")
> > > Fixes: 85b1c11989c ("clk: imx: Add ECSPI to iMX8MN")
> > > Cc: Marek Vasut <marex@denx.de>
> > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > ---
> > >  drivers/clk/imx/clk-imx8mn.c | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> > > index ed9e16d7c1..bfd1677520 100644
> > > --- a/drivers/clk/imx/clk-imx8mn.c
> > > +++ b/drivers/clk/imx/clk-imx8mn.c
> > > @@ -57,15 +57,15 @@ static const char *imx8mn_usdhc2_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > >                                            "sys_pll3_out", "sys_pll1_266m", "audio_pll2_out", "sys_pll1_100m", };
> > >
> > >  #if CONFIG_IS_ENABLED(DM_SPI)
> > > -static const char *imx8mn_ecspi1_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > +static const char *imx8mn_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > >
> > > -static const char *imx8mn_ecspi2_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > +static const char *imx8mn_ecspi2_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > >
> > > -static const char *imx8mn_ecspi3_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > +static const char *imx8mn_ecspi3_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > >  #endif
> > > @@ -105,7 +105,7 @@ static const char *imx8mn_usdhc3_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > >  static const char *imx8mn_qspi_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sys_pll2_333m", "sys_pll2_500m",
> > >                                            "audio_pll2_out", "sys_pll1_266m", "sys_pll3_out", "sys_pll1_100m", };
> > >
> > > -static const char * const imx8mn_nand_sels[] = {"osc_24m", "sys_pll2_500m", "audio_pll1_out",
> > > +static const char * const imx8mn_nand_sels[] = {"clock-osc-24m", "sys_pll2_500m", "audio_pll1_out",
> > >                                                 "sys_pll1_400m", "audio_pll2_out", "sys_pll3_out",
> > >                                                 "sys_pll2_250m", "video_pll_out", };
> > >
> > > @@ -119,7 +119,9 @@ static const char * const imx8mn_usb_phy_sels[] = {"clock-osc-24m", "sys_pll1_10
> > >
> > >  static int imx8mn_clk_probe(struct udevice *dev)
> > >  {
> > > +       struct clk osc_24m_clk;
> > >         void __iomem *base;
> > > +       int ret;
> > >
> > >         base = (void *)ANATOP_BASE_ADDR;
> > >
> > > @@ -238,6 +240,11 @@ static int imx8mn_clk_probe(struct udevice *dev)
> > >         clk_dm(IMX8MN_SYS_PLL2_1000M,
> > >                imx_clk_fixed_factor("sys_pll2_1000m", "sys_pll2_out", 1, 1));
> > >
> >
> > I know it's late, but I didn't get around to testing my Nano board for
> > a while.  Sorry for the delayed feedback...
> >
> > > +       ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> > > +       if (ret)
> > > +               return ret;
> > > +       clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> > > +
> >
> > These four lines appear to have introduced a regression on the
> > imx8mn-beacon board.  In the SPL phase, I get an error message
> > indicating it cannot find the i2c clk, then access to the PMIC fails.
> > If I remove these four lines, the error message disappears, and the
> > PMIC is happy again.
> >
> > I have confirmed that CLK, and SPL_CLK are both defined.   I checked
> > the u-boot-spl.dtb and it shows clock-osc-24m with clock-output-names
> > = "osc_24m"
> >
> > I have also tried to mark the 24m clock as with bootph-pre-ram;:
> > &{/clock-osc-24m} {
> >   bootph-pre-ram;
> > };
> >
> > Unfortunately, nothing is helping, and I am not sure what else to try.
> > clk_fixed_rate.o is built into my SPL, so I don't think it's a config
> > issue.  I need to the PMIC to increase the voltage since we run the
> > Nano in overdrive mode to run the LPDDR4 at the highest speed.
> >
> > Might anyone have any suggestions?
> >
>
> Not yet. I have a similar setup but I don't set pmic, so I think that
> the symbol is not find
> in SPL and it just fail clock registration. Is that possible?

It does appear that the clock registration failed.   Since the system
does an early return, the I2C clocks among others are also not
registered which appears to cause the failure.

I changed the registration around a little:

ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
if (!ret)
        clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));

This method won't return prematurely, so the I2C and other clocks can
be registered, and it appears to address the problem.
Another alternative might be to move this clock registration until
after all the rest of the clocks are done.

adam
>
> If (ret)
>
> put here a print
>
> Michael
> > adam
> >
> > >         base = dev_read_addr_ptr(dev);
> > >         if (!base)
> > >                 return -EINVAL;
> > > --
> > > 2.43.0
> > >
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
  

Hi Adam

On Mon, Nov 4, 2024 at 6:01 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Nov 4, 2024 at 10:29 AM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
> >
> > Hi Adam
> >
> > On Mon, Nov 4, 2024 at 5:11 PM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > On Sun, Jul 7, 2024 at 3:45 AM Michael Trimarchi
> > > <michael@amarulasolutions.com> wrote:
> > > >
> > > > The osc_24m is the clock-output-name and not the one that
> > > > is used as internal name reference from the strcmp. The clock
> > > > that use osc_24m, will not be able to reparent it as they should.
> > > > We need anyway register the osc_24m clock fixed factor in the clock
> > > > tree.
> > > >
> > > > Fixes: 710c4ffb890 ("clk: imx: clk-imx8mn add gpmi nand clocks")
> > > > Fixes: 85b1c11989c ("clk: imx: Add ECSPI to iMX8MN")
> > > > Cc: Marek Vasut <marex@denx.de>
> > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > ---
> > > >  drivers/clk/imx/clk-imx8mn.c | 15 +++++++++++----
> > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> > > > index ed9e16d7c1..bfd1677520 100644
> > > > --- a/drivers/clk/imx/clk-imx8mn.c
> > > > +++ b/drivers/clk/imx/clk-imx8mn.c
> > > > @@ -57,15 +57,15 @@ static const char *imx8mn_usdhc2_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > > >                                            "sys_pll3_out", "sys_pll1_266m", "audio_pll2_out", "sys_pll1_100m", };
> > > >
> > > >  #if CONFIG_IS_ENABLED(DM_SPI)
> > > > -static const char *imx8mn_ecspi1_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > +static const char *imx8mn_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > >
> > > > -static const char *imx8mn_ecspi2_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > +static const char *imx8mn_ecspi2_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > >
> > > > -static const char *imx8mn_ecspi3_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > +static const char *imx8mn_ecspi3_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > >  #endif
> > > > @@ -105,7 +105,7 @@ static const char *imx8mn_usdhc3_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > > >  static const char *imx8mn_qspi_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sys_pll2_333m", "sys_pll2_500m",
> > > >                                            "audio_pll2_out", "sys_pll1_266m", "sys_pll3_out", "sys_pll1_100m", };
> > > >
> > > > -static const char * const imx8mn_nand_sels[] = {"osc_24m", "sys_pll2_500m", "audio_pll1_out",
> > > > +static const char * const imx8mn_nand_sels[] = {"clock-osc-24m", "sys_pll2_500m", "audio_pll1_out",
> > > >                                                 "sys_pll1_400m", "audio_pll2_out", "sys_pll3_out",
> > > >                                                 "sys_pll2_250m", "video_pll_out", };
> > > >
> > > > @@ -119,7 +119,9 @@ static const char * const imx8mn_usb_phy_sels[] = {"clock-osc-24m", "sys_pll1_10
> > > >
> > > >  static int imx8mn_clk_probe(struct udevice *dev)
> > > >  {
> > > > +       struct clk osc_24m_clk;
> > > >         void __iomem *base;
> > > > +       int ret;
> > > >
> > > >         base = (void *)ANATOP_BASE_ADDR;
> > > >
> > > > @@ -238,6 +240,11 @@ static int imx8mn_clk_probe(struct udevice *dev)
> > > >         clk_dm(IMX8MN_SYS_PLL2_1000M,
> > > >                imx_clk_fixed_factor("sys_pll2_1000m", "sys_pll2_out", 1, 1));
> > > >
> > >
> > > I know it's late, but I didn't get around to testing my Nano board for
> > > a while.  Sorry for the delayed feedback...
> > >
> > > > +       ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> > > > +       if (ret)
> > > > +               return ret;
> > > > +       clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> > > > +
> > >
> > > These four lines appear to have introduced a regression on the
> > > imx8mn-beacon board.  In the SPL phase, I get an error message
> > > indicating it cannot find the i2c clk, then access to the PMIC fails.
> > > If I remove these four lines, the error message disappears, and the
> > > PMIC is happy again.
> > >
> > > I have confirmed that CLK, and SPL_CLK are both defined.   I checked
> > > the u-boot-spl.dtb and it shows clock-osc-24m with clock-output-names
> > > = "osc_24m"
> > >
> > > I have also tried to mark the 24m clock as with bootph-pre-ram;:
> > > &{/clock-osc-24m} {
> > >   bootph-pre-ram;
> > > };
> > >
> > > Unfortunately, nothing is helping, and I am not sure what else to try.
> > > clk_fixed_rate.o is built into my SPL, so I don't think it's a config
> > > issue.  I need to the PMIC to increase the voltage since we run the
> > > Nano in overdrive mode to run the LPDDR4 at the highest speed.
> > >
> > > Might anyone have any suggestions?
> > >
> >
> > Not yet. I have a similar setup but I don't set pmic, so I think that
> > the symbol is not find
> > in SPL and it just fail clock registration. Is that possible?
>
> It does appear that the clock registration failed.   Since the system
> does an early return, the I2C clocks among others are also not
> registered which appears to cause the failure.
>
> I changed the registration around a little:
>
> ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> if (!ret)
>         clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
>

Seems that is not in the image spl so it can not find it. Are you sure
that you can inside
the SPL

Michael

> This method won't return prematurely, so the I2C and other clocks can
> be registered, and it appears to address the problem.
> Another alternative might be to move this clock registration until
> after all the rest of the clocks are done.
>
> adam
> >
> > If (ret)
> >
> > put here a print
> >
> > Michael
> > > adam
> > >
> > > >         base = dev_read_addr_ptr(dev);
> > > >         if (!base)
> > > >                 return -EINVAL;
> > > > --
> > > > 2.43.0
> > > >
> >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > michael@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > info@amarulasolutions.com
> > www.amarulasolutions.com
  

On Mon, Nov 4, 2024 at 11:04 AM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi Adam
>
> On Mon, Nov 4, 2024 at 6:01 PM Adam Ford <aford173@gmail.com> wrote:
> >
> > On Mon, Nov 4, 2024 at 10:29 AM Michael Nazzareno Trimarchi
> > <michael@amarulasolutions.com> wrote:
> > >
> > > Hi Adam
> > >
> > > On Mon, Nov 4, 2024 at 5:11 PM Adam Ford <aford173@gmail.com> wrote:
> > > >
> > > > On Sun, Jul 7, 2024 at 3:45 AM Michael Trimarchi
> > > > <michael@amarulasolutions.com> wrote:
> > > > >
> > > > > The osc_24m is the clock-output-name and not the one that
> > > > > is used as internal name reference from the strcmp. The clock
> > > > > that use osc_24m, will not be able to reparent it as they should.
> > > > > We need anyway register the osc_24m clock fixed factor in the clock
> > > > > tree.
> > > > >
> > > > > Fixes: 710c4ffb890 ("clk: imx: clk-imx8mn add gpmi nand clocks")
> > > > > Fixes: 85b1c11989c ("clk: imx: Add ECSPI to iMX8MN")
> > > > > Cc: Marek Vasut <marex@denx.de>
> > > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > > ---
> > > > >  drivers/clk/imx/clk-imx8mn.c | 15 +++++++++++----
> > > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> > > > > index ed9e16d7c1..bfd1677520 100644
> > > > > --- a/drivers/clk/imx/clk-imx8mn.c
> > > > > +++ b/drivers/clk/imx/clk-imx8mn.c
> > > > > @@ -57,15 +57,15 @@ static const char *imx8mn_usdhc2_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > > > >                                            "sys_pll3_out", "sys_pll1_266m", "audio_pll2_out", "sys_pll1_100m", };
> > > > >
> > > > >  #if CONFIG_IS_ENABLED(DM_SPI)
> > > > > -static const char *imx8mn_ecspi1_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > +static const char *imx8mn_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > >
> > > > > -static const char *imx8mn_ecspi2_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > +static const char *imx8mn_ecspi2_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > >
> > > > > -static const char *imx8mn_ecspi3_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > +static const char *imx8mn_ecspi3_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > >  #endif
> > > > > @@ -105,7 +105,7 @@ static const char *imx8mn_usdhc3_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > > > >  static const char *imx8mn_qspi_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sys_pll2_333m", "sys_pll2_500m",
> > > > >                                            "audio_pll2_out", "sys_pll1_266m", "sys_pll3_out", "sys_pll1_100m", };
> > > > >
> > > > > -static const char * const imx8mn_nand_sels[] = {"osc_24m", "sys_pll2_500m", "audio_pll1_out",
> > > > > +static const char * const imx8mn_nand_sels[] = {"clock-osc-24m", "sys_pll2_500m", "audio_pll1_out",
> > > > >                                                 "sys_pll1_400m", "audio_pll2_out", "sys_pll3_out",
> > > > >                                                 "sys_pll2_250m", "video_pll_out", };
> > > > >
> > > > > @@ -119,7 +119,9 @@ static const char * const imx8mn_usb_phy_sels[] = {"clock-osc-24m", "sys_pll1_10
> > > > >
> > > > >  static int imx8mn_clk_probe(struct udevice *dev)
> > > > >  {
> > > > > +       struct clk osc_24m_clk;
> > > > >         void __iomem *base;
> > > > > +       int ret;
> > > > >
> > > > >         base = (void *)ANATOP_BASE_ADDR;
> > > > >
> > > > > @@ -238,6 +240,11 @@ static int imx8mn_clk_probe(struct udevice *dev)
> > > > >         clk_dm(IMX8MN_SYS_PLL2_1000M,
> > > > >                imx_clk_fixed_factor("sys_pll2_1000m", "sys_pll2_out", 1, 1));
> > > > >
> > > >
> > > > I know it's late, but I didn't get around to testing my Nano board for
> > > > a while.  Sorry for the delayed feedback...
> > > >
> > > > > +       ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +       clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> > > > > +
> > > >
> > > > These four lines appear to have introduced a regression on the
> > > > imx8mn-beacon board.  In the SPL phase, I get an error message
> > > > indicating it cannot find the i2c clk, then access to the PMIC fails.
> > > > If I remove these four lines, the error message disappears, and the
> > > > PMIC is happy again.
> > > >
> > > > I have confirmed that CLK, and SPL_CLK are both defined.   I checked
> > > > the u-boot-spl.dtb and it shows clock-osc-24m with clock-output-names
> > > > = "osc_24m"
> > > >
> > > > I have also tried to mark the 24m clock as with bootph-pre-ram;:
> > > > &{/clock-osc-24m} {
> > > >   bootph-pre-ram;
> > > > };
> > > >
> > > > Unfortunately, nothing is helping, and I am not sure what else to try.
> > > > clk_fixed_rate.o is built into my SPL, so I don't think it's a config
> > > > issue.  I need to the PMIC to increase the voltage since we run the
> > > > Nano in overdrive mode to run the LPDDR4 at the highest speed.
> > > >
> > > > Might anyone have any suggestions?
> > > >
> > >
> > > Not yet. I have a similar setup but I don't set pmic, so I think that
> > > the symbol is not find
> > > in SPL and it just fail clock registration. Is that possible?
> >
> > It does appear that the clock registration failed.   Since the system
> > does an early return, the I2C clocks among others are also not
> > registered which appears to cause the failure.
> >
> > I changed the registration around a little:
> >
> > ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> > if (!ret)
> >         clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> >
>
> Seems that is not in the image spl so it can not find it. Are you sure
> that you can inside
> the SPL

I am not sure I understand what you are asking, but the clock node
appears in spl/u-boot-spl.dtb:

 clock-osc-24m {
                compatible = "fixed-clock";
                #clock-cells = <0x00>;
                clock-frequency = <0x16e3600>;
                clock-output-names = "osc_24m";
                phandle = <0x18>;
        };

Since the clock-output-names is "osc_24m" I wonder if clk_get_by_name
is the right function.  Looking at other uses of this function, it
looks like it's looking for a 'clock-names' parameter and not
clock-output-names.

adam
>
> Michael
>
> > This method won't return prematurely, so the I2C and other clocks can
> > be registered, and it appears to address the problem.
> > Another alternative might be to move this clock registration until
> > after all the rest of the clocks are done.
> >
> > adam
> > >
> > > If (ret)
> > >
> > > put here a print
> > >
> > > Michael
> > > > adam
> > > >
> > > > >         base = dev_read_addr_ptr(dev);
> > > > >         if (!base)
> > > > >                 return -EINVAL;
> > > > > --
> > > > > 2.43.0
> > > > >
> > >
> > >
> > >
> > > --
> > > Michael Nazzareno Trimarchi
> > > Co-Founder & Chief Executive Officer
> > > M. +39 347 913 2170
> > > michael@amarulasolutions.com
> > > __________________________________
> > >
> > > Amarula Solutions BV
> > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > T. +31 (0)85 111 9172
> > > info@amarulasolutions.com
> > > www.amarulasolutions.com
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
  

Hi

On Mon, Nov 4, 2024 at 6:17 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Nov 4, 2024 at 11:04 AM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
> >
> > Hi Adam
> >
> > On Mon, Nov 4, 2024 at 6:01 PM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > On Mon, Nov 4, 2024 at 10:29 AM Michael Nazzareno Trimarchi
> > > <michael@amarulasolutions.com> wrote:
> > > >
> > > > Hi Adam
> > > >
> > > > On Mon, Nov 4, 2024 at 5:11 PM Adam Ford <aford173@gmail.com> wrote:
> > > > >
> > > > > On Sun, Jul 7, 2024 at 3:45 AM Michael Trimarchi
> > > > > <michael@amarulasolutions.com> wrote:
> > > > > >
> > > > > > The osc_24m is the clock-output-name and not the one that
> > > > > > is used as internal name reference from the strcmp. The clock
> > > > > > that use osc_24m, will not be able to reparent it as they should.
> > > > > > We need anyway register the osc_24m clock fixed factor in the clock
> > > > > > tree.
> > > > > >
> > > > > > Fixes: 710c4ffb890 ("clk: imx: clk-imx8mn add gpmi nand clocks")
> > > > > > Fixes: 85b1c11989c ("clk: imx: Add ECSPI to iMX8MN")
> > > > > > Cc: Marek Vasut <marex@denx.de>
> > > > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > > > ---
> > > > > >  drivers/clk/imx/clk-imx8mn.c | 15 +++++++++++----
> > > > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> > > > > > index ed9e16d7c1..bfd1677520 100644
> > > > > > --- a/drivers/clk/imx/clk-imx8mn.c
> > > > > > +++ b/drivers/clk/imx/clk-imx8mn.c
> > > > > > @@ -57,15 +57,15 @@ static const char *imx8mn_usdhc2_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > > > > >                                            "sys_pll3_out", "sys_pll1_266m", "audio_pll2_out", "sys_pll1_100m", };
> > > > > >
> > > > > >  #if CONFIG_IS_ENABLED(DM_SPI)
> > > > > > -static const char *imx8mn_ecspi1_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > +static const char *imx8mn_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > > >
> > > > > > -static const char *imx8mn_ecspi2_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > +static const char *imx8mn_ecspi2_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > > >
> > > > > > -static const char *imx8mn_ecspi3_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > +static const char *imx8mn_ecspi3_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > > >  #endif
> > > > > > @@ -105,7 +105,7 @@ static const char *imx8mn_usdhc3_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > > > > >  static const char *imx8mn_qspi_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sys_pll2_333m", "sys_pll2_500m",
> > > > > >                                            "audio_pll2_out", "sys_pll1_266m", "sys_pll3_out", "sys_pll1_100m", };
> > > > > >
> > > > > > -static const char * const imx8mn_nand_sels[] = {"osc_24m", "sys_pll2_500m", "audio_pll1_out",
> > > > > > +static const char * const imx8mn_nand_sels[] = {"clock-osc-24m", "sys_pll2_500m", "audio_pll1_out",
> > > > > >                                                 "sys_pll1_400m", "audio_pll2_out", "sys_pll3_out",
> > > > > >                                                 "sys_pll2_250m", "video_pll_out", };
> > > > > >
> > > > > > @@ -119,7 +119,9 @@ static const char * const imx8mn_usb_phy_sels[] = {"clock-osc-24m", "sys_pll1_10
> > > > > >
> > > > > >  static int imx8mn_clk_probe(struct udevice *dev)
> > > > > >  {
> > > > > > +       struct clk osc_24m_clk;
> > > > > >         void __iomem *base;
> > > > > > +       int ret;
> > > > > >
> > > > > >         base = (void *)ANATOP_BASE_ADDR;
> > > > > >
> > > > > > @@ -238,6 +240,11 @@ static int imx8mn_clk_probe(struct udevice *dev)
> > > > > >         clk_dm(IMX8MN_SYS_PLL2_1000M,
> > > > > >                imx_clk_fixed_factor("sys_pll2_1000m", "sys_pll2_out", 1, 1));
> > > > > >
> > > > >
> > > > > I know it's late, but I didn't get around to testing my Nano board for
> > > > > a while.  Sorry for the delayed feedback...
> > > > >
> > > > > > +       ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> > > > > > +       if (ret)
> > > > > > +               return ret;
> > > > > > +       clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> > > > > > +
> > > > >
> > > > > These four lines appear to have introduced a regression on the
> > > > > imx8mn-beacon board.  In the SPL phase, I get an error message
> > > > > indicating it cannot find the i2c clk, then access to the PMIC fails.
> > > > > If I remove these four lines, the error message disappears, and the
> > > > > PMIC is happy again.
> > > > >
> > > > > I have confirmed that CLK, and SPL_CLK are both defined.   I checked
> > > > > the u-boot-spl.dtb and it shows clock-osc-24m with clock-output-names
> > > > > = "osc_24m"
> > > > >
> > > > > I have also tried to mark the 24m clock as with bootph-pre-ram;:
> > > > > &{/clock-osc-24m} {
> > > > >   bootph-pre-ram;
> > > > > };
> > > > >
> > > > > Unfortunately, nothing is helping, and I am not sure what else to try.
> > > > > clk_fixed_rate.o is built into my SPL, so I don't think it's a config
> > > > > issue.  I need to the PMIC to increase the voltage since we run the
> > > > > Nano in overdrive mode to run the LPDDR4 at the highest speed.
> > > > >
> > > > > Might anyone have any suggestions?
> > > > >
> > > >
> > > > Not yet. I have a similar setup but I don't set pmic, so I think that
> > > > the symbol is not find
> > > > in SPL and it just fail clock registration. Is that possible?
> > >
> > > It does appear that the clock registration failed.   Since the system
> > > does an early return, the I2C clocks among others are also not
> > > registered which appears to cause the failure.
> > >
> > > I changed the registration around a little:
> > >
> > > ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> > > if (!ret)
> > >         clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> > >
> >
> > Seems that is not in the image spl so it can not find it. Are you sure
> > that you can inside
> > the SPL
>
> I am not sure I understand what you are asking, but the clock node
> appears in spl/u-boot-spl.dtb:
>
>  clock-osc-24m {
>                 compatible = "fixed-clock";
>                 #clock-cells = <0x00>;
>                 clock-frequency = <0x16e3600>;
>                 clock-output-names = "osc_24m";
>                 phandle = <0x18>;
>         };
>
> Since the clock-output-names is "osc_24m" I wonder if clk_get_by_name
> is the right function.  Looking at other uses of this function, it
> looks like it's looking for a 'clock-names' parameter and not
> clock-output-names.

Yes, it's the right answer. That is strange because it works in uboot
image. Is the node the same in uboot-dtb?

Michael

>
> adam
> >
> > Michael
> >
> > > This method won't return prematurely, so the I2C and other clocks can
> > > be registered, and it appears to address the problem.
> > > Another alternative might be to move this clock registration until
> > > after all the rest of the clocks are done.
> > >
> > > adam
> > > >
> > > > If (ret)
> > > >
> > > > put here a print
> > > >
> > > > Michael
> > > > > adam
> > > > >
> > > > > >         base = dev_read_addr_ptr(dev);
> > > > > >         if (!base)
> > > > > >                 return -EINVAL;
> > > > > > --
> > > > > > 2.43.0
> > > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Michael Nazzareno Trimarchi
> > > > Co-Founder & Chief Executive Officer
> > > > M. +39 347 913 2170
> > > > michael@amarulasolutions.com
> > > > __________________________________
> > > >
> > > > Amarula Solutions BV
> > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > > T. +31 (0)85 111 9172
> > > > info@amarulasolutions.com
> > > > www.amarulasolutions.com
> >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > michael@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > info@amarulasolutions.com
> > www.amarulasolutions.com
  

On Mon, Nov 4, 2024 at 11:22 AM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi
>
> On Mon, Nov 4, 2024 at 6:17 PM Adam Ford <aford173@gmail.com> wrote:
> >
> > On Mon, Nov 4, 2024 at 11:04 AM Michael Nazzareno Trimarchi
> > <michael@amarulasolutions.com> wrote:
> > >
> > > Hi Adam
> > >
> > > On Mon, Nov 4, 2024 at 6:01 PM Adam Ford <aford173@gmail.com> wrote:
> > > >
> > > > On Mon, Nov 4, 2024 at 10:29 AM Michael Nazzareno Trimarchi
> > > > <michael@amarulasolutions.com> wrote:
> > > > >
> > > > > Hi Adam
> > > > >
> > > > > On Mon, Nov 4, 2024 at 5:11 PM Adam Ford <aford173@gmail.com> wrote:
> > > > > >
> > > > > > On Sun, Jul 7, 2024 at 3:45 AM Michael Trimarchi
> > > > > > <michael@amarulasolutions.com> wrote:
> > > > > > >
> > > > > > > The osc_24m is the clock-output-name and not the one that
> > > > > > > is used as internal name reference from the strcmp. The clock
> > > > > > > that use osc_24m, will not be able to reparent it as they should.
> > > > > > > We need anyway register the osc_24m clock fixed factor in the clock
> > > > > > > tree.
> > > > > > >
> > > > > > > Fixes: 710c4ffb890 ("clk: imx: clk-imx8mn add gpmi nand clocks")
> > > > > > > Fixes: 85b1c11989c ("clk: imx: Add ECSPI to iMX8MN")
> > > > > > > Cc: Marek Vasut <marex@denx.de>
> > > > > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > > > > ---
> > > > > > >  drivers/clk/imx/clk-imx8mn.c | 15 +++++++++++----
> > > > > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> > > > > > > index ed9e16d7c1..bfd1677520 100644
> > > > > > > --- a/drivers/clk/imx/clk-imx8mn.c
> > > > > > > +++ b/drivers/clk/imx/clk-imx8mn.c
> > > > > > > @@ -57,15 +57,15 @@ static const char *imx8mn_usdhc2_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > > > > > >                                            "sys_pll3_out", "sys_pll1_266m", "audio_pll2_out", "sys_pll1_100m", };
> > > > > > >
> > > > > > >  #if CONFIG_IS_ENABLED(DM_SPI)
> > > > > > > -static const char *imx8mn_ecspi1_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > > +static const char *imx8mn_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > > > >
> > > > > > > -static const char *imx8mn_ecspi2_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > > +static const char *imx8mn_ecspi2_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > > > >
> > > > > > > -static const char *imx8mn_ecspi3_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > > +static const char *imx8mn_ecspi3_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > > > >  #endif
> > > > > > > @@ -105,7 +105,7 @@ static const char *imx8mn_usdhc3_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > > > > > >  static const char *imx8mn_qspi_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sys_pll2_333m", "sys_pll2_500m",
> > > > > > >                                            "audio_pll2_out", "sys_pll1_266m", "sys_pll3_out", "sys_pll1_100m", };
> > > > > > >
> > > > > > > -static const char * const imx8mn_nand_sels[] = {"osc_24m", "sys_pll2_500m", "audio_pll1_out",
> > > > > > > +static const char * const imx8mn_nand_sels[] = {"clock-osc-24m", "sys_pll2_500m", "audio_pll1_out",
> > > > > > >                                                 "sys_pll1_400m", "audio_pll2_out", "sys_pll3_out",
> > > > > > >                                                 "sys_pll2_250m", "video_pll_out", };
> > > > > > >
> > > > > > > @@ -119,7 +119,9 @@ static const char * const imx8mn_usb_phy_sels[] = {"clock-osc-24m", "sys_pll1_10
> > > > > > >
> > > > > > >  static int imx8mn_clk_probe(struct udevice *dev)
> > > > > > >  {
> > > > > > > +       struct clk osc_24m_clk;
> > > > > > >         void __iomem *base;
> > > > > > > +       int ret;
> > > > > > >
> > > > > > >         base = (void *)ANATOP_BASE_ADDR;
> > > > > > >
> > > > > > > @@ -238,6 +240,11 @@ static int imx8mn_clk_probe(struct udevice *dev)
> > > > > > >         clk_dm(IMX8MN_SYS_PLL2_1000M,
> > > > > > >                imx_clk_fixed_factor("sys_pll2_1000m", "sys_pll2_out", 1, 1));
> > > > > > >
> > > > > >
> > > > > > I know it's late, but I didn't get around to testing my Nano board for
> > > > > > a while.  Sorry for the delayed feedback...
> > > > > >
> > > > > > > +       ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> > > > > > > +       if (ret)
> > > > > > > +               return ret;
> > > > > > > +       clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> > > > > > > +
> > > > > >
> > > > > > These four lines appear to have introduced a regression on the
> > > > > > imx8mn-beacon board.  In the SPL phase, I get an error message
> > > > > > indicating it cannot find the i2c clk, then access to the PMIC fails.
> > > > > > If I remove these four lines, the error message disappears, and the
> > > > > > PMIC is happy again.
> > > > > >
> > > > > > I have confirmed that CLK, and SPL_CLK are both defined.   I checked
> > > > > > the u-boot-spl.dtb and it shows clock-osc-24m with clock-output-names
> > > > > > = "osc_24m"
> > > > > >
> > > > > > I have also tried to mark the 24m clock as with bootph-pre-ram;:
> > > > > > &{/clock-osc-24m} {
> > > > > >   bootph-pre-ram;
> > > > > > };
> > > > > >
> > > > > > Unfortunately, nothing is helping, and I am not sure what else to try.
> > > > > > clk_fixed_rate.o is built into my SPL, so I don't think it's a config
> > > > > > issue.  I need to the PMIC to increase the voltage since we run the
> > > > > > Nano in overdrive mode to run the LPDDR4 at the highest speed.
> > > > > >
> > > > > > Might anyone have any suggestions?
> > > > > >
> > > > >
> > > > > Not yet. I have a similar setup but I don't set pmic, so I think that
> > > > > the symbol is not find
> > > > > in SPL and it just fail clock registration. Is that possible?
> > > >
> > > > It does appear that the clock registration failed.   Since the system
> > > > does an early return, the I2C clocks among others are also not
> > > > registered which appears to cause the failure.
> > > >
> > > > I changed the registration around a little:
> > > >
> > > > ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> > > > if (!ret)
> > > >         clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> > > >
> > >
> > > Seems that is not in the image spl so it can not find it. Are you sure
> > > that you can inside
> > > the SPL
> >
> > I am not sure I understand what you are asking, but the clock node
> > appears in spl/u-boot-spl.dtb:
> >
> >  clock-osc-24m {
> >                 compatible = "fixed-clock";
> >                 #clock-cells = <0x00>;
> >                 clock-frequency = <0x16e3600>;
> >                 clock-output-names = "osc_24m";
> >                 phandle = <0x18>;
> >         };
> >
> > Since the clock-output-names is "osc_24m" I wonder if clk_get_by_name
> > is the right function.  Looking at other uses of this function, it
> > looks like it's looking for a 'clock-names' parameter and not
> > clock-output-names.
>
> Yes, it's the right answer. That is strange because it works in uboot
> image. Is the node the same in uboot-dtb?

The U-Boot phase device tree looks like this:

 clock-osc-24m {
                compatible = "fixed-clock";
                #clock-cells = <0x00>;
                clock-frequency = <0x16e3600>;
                clock-output-names = "osc_24m";
                bootph-pre-ram;
                bootph-all;
                phandle = <0x18>;
        };

>
> Michael
>
> >
> > adam
> > >
> > > Michael
> > >
> > > > This method won't return prematurely, so the I2C and other clocks can
> > > > be registered, and it appears to address the problem.
> > > > Another alternative might be to move this clock registration until
> > > > after all the rest of the clocks are done.
> > > >
> > > > adam
> > > > >
> > > > > If (ret)
> > > > >
> > > > > put here a print
> > > > >
> > > > > Michael
> > > > > > adam
> > > > > >
> > > > > > >         base = dev_read_addr_ptr(dev);
> > > > > > >         if (!base)
> > > > > > >                 return -EINVAL;
> > > > > > > --
> > > > > > > 2.43.0
> > > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Michael Nazzareno Trimarchi
> > > > > Co-Founder & Chief Executive Officer
> > > > > M. +39 347 913 2170
> > > > > michael@amarulasolutions.com
> > > > > __________________________________
> > > > >
> > > > > Amarula Solutions BV
> > > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > > > T. +31 (0)85 111 9172
> > > > > info@amarulasolutions.com
> > > > > www.amarulasolutions.com
> > >
> > >
> > >
> > > --
> > > Michael Nazzareno Trimarchi
> > > Co-Founder & Chief Executive Officer
> > > M. +39 347 913 2170
> > > michael@amarulasolutions.com
> > > __________________________________
> > >
> > > Amarula Solutions BV
> > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > T. +31 (0)85 111 9172
> > > info@amarulasolutions.com
> > > www.amarulasolutions.com
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
  

On Mon, Nov 4, 2024 at 11:24 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Nov 4, 2024 at 11:22 AM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
> >
> > Hi
> >
> > On Mon, Nov 4, 2024 at 6:17 PM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > On Mon, Nov 4, 2024 at 11:04 AM Michael Nazzareno Trimarchi
> > > <michael@amarulasolutions.com> wrote:
> > > >
> > > > Hi Adam
> > > >
> > > > On Mon, Nov 4, 2024 at 6:01 PM Adam Ford <aford173@gmail.com> wrote:
> > > > >
> > > > > On Mon, Nov 4, 2024 at 10:29 AM Michael Nazzareno Trimarchi
> > > > > <michael@amarulasolutions.com> wrote:
> > > > > >
> > > > > > Hi Adam
> > > > > >
> > > > > > On Mon, Nov 4, 2024 at 5:11 PM Adam Ford <aford173@gmail.com> wrote:
> > > > > > >
> > > > > > > On Sun, Jul 7, 2024 at 3:45 AM Michael Trimarchi
> > > > > > > <michael@amarulasolutions.com> wrote:
> > > > > > > >
> > > > > > > > The osc_24m is the clock-output-name and not the one that
> > > > > > > > is used as internal name reference from the strcmp. The clock
> > > > > > > > that use osc_24m, will not be able to reparent it as they should.
> > > > > > > > We need anyway register the osc_24m clock fixed factor in the clock
> > > > > > > > tree.
> > > > > > > >
> > > > > > > > Fixes: 710c4ffb890 ("clk: imx: clk-imx8mn add gpmi nand clocks")
> > > > > > > > Fixes: 85b1c11989c ("clk: imx: Add ECSPI to iMX8MN")
> > > > > > > > Cc: Marek Vasut <marex@denx.de>
> > > > > > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > > > > > ---
> > > > > > > >  drivers/clk/imx/clk-imx8mn.c | 15 +++++++++++----
> > > > > > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> > > > > > > > index ed9e16d7c1..bfd1677520 100644
> > > > > > > > --- a/drivers/clk/imx/clk-imx8mn.c
> > > > > > > > +++ b/drivers/clk/imx/clk-imx8mn.c
> > > > > > > > @@ -57,15 +57,15 @@ static const char *imx8mn_usdhc2_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > > > > > > >                                            "sys_pll3_out", "sys_pll1_266m", "audio_pll2_out", "sys_pll1_100m", };
> > > > > > > >
> > > > > > > >  #if CONFIG_IS_ENABLED(DM_SPI)
> > > > > > > > -static const char *imx8mn_ecspi1_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > > > +static const char *imx8mn_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > > > > >
> > > > > > > > -static const char *imx8mn_ecspi2_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > > > +static const char *imx8mn_ecspi2_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > > > > >
> > > > > > > > -static const char *imx8mn_ecspi3_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > > > +static const char *imx8mn_ecspi3_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll1_40m",
> > > > > > > >                                            "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
> > > > > > > >                                            "sys_pll2_250m", "audio_pll2_out", };
> > > > > > > >  #endif
> > > > > > > > @@ -105,7 +105,7 @@ static const char *imx8mn_usdhc3_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sy
> > > > > > > >  static const char *imx8mn_qspi_sels[] = {"clock-osc-24m", "sys_pll1_400m", "sys_pll2_333m", "sys_pll2_500m",
> > > > > > > >                                            "audio_pll2_out", "sys_pll1_266m", "sys_pll3_out", "sys_pll1_100m", };
> > > > > > > >
> > > > > > > > -static const char * const imx8mn_nand_sels[] = {"osc_24m", "sys_pll2_500m", "audio_pll1_out",
> > > > > > > > +static const char * const imx8mn_nand_sels[] = {"clock-osc-24m", "sys_pll2_500m", "audio_pll1_out",
> > > > > > > >                                                 "sys_pll1_400m", "audio_pll2_out", "sys_pll3_out",
> > > > > > > >                                                 "sys_pll2_250m", "video_pll_out", };
> > > > > > > >
> > > > > > > > @@ -119,7 +119,9 @@ static const char * const imx8mn_usb_phy_sels[] = {"clock-osc-24m", "sys_pll1_10
> > > > > > > >
> > > > > > > >  static int imx8mn_clk_probe(struct udevice *dev)
> > > > > > > >  {
> > > > > > > > +       struct clk osc_24m_clk;
> > > > > > > >         void __iomem *base;
> > > > > > > > +       int ret;
> > > > > > > >
> > > > > > > >         base = (void *)ANATOP_BASE_ADDR;
> > > > > > > >
> > > > > > > > @@ -238,6 +240,11 @@ static int imx8mn_clk_probe(struct udevice *dev)
> > > > > > > >         clk_dm(IMX8MN_SYS_PLL2_1000M,
> > > > > > > >                imx_clk_fixed_factor("sys_pll2_1000m", "sys_pll2_out", 1, 1));
> > > > > > > >
> > > > > > >
> > > > > > > I know it's late, but I didn't get around to testing my Nano board for
> > > > > > > a while.  Sorry for the delayed feedback...
> > > > > > >
> > > > > > > > +       ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> > > > > > > > +       if (ret)
> > > > > > > > +               return ret;
> > > > > > > > +       clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> > > > > > > > +
> > > > > > >
> > > > > > > These four lines appear to have introduced a regression on the
> > > > > > > imx8mn-beacon board.  In the SPL phase, I get an error message
> > > > > > > indicating it cannot find the i2c clk, then access to the PMIC fails.
> > > > > > > If I remove these four lines, the error message disappears, and the
> > > > > > > PMIC is happy again.
> > > > > > >
> > > > > > > I have confirmed that CLK, and SPL_CLK are both defined.   I checked
> > > > > > > the u-boot-spl.dtb and it shows clock-osc-24m with clock-output-names
> > > > > > > = "osc_24m"
> > > > > > >
> > > > > > > I have also tried to mark the 24m clock as with bootph-pre-ram;:
> > > > > > > &{/clock-osc-24m} {
> > > > > > >   bootph-pre-ram;
> > > > > > > };
> > > > > > >
> > > > > > > Unfortunately, nothing is helping, and I am not sure what else to try.
> > > > > > > clk_fixed_rate.o is built into my SPL, so I don't think it's a config
> > > > > > > issue.  I need to the PMIC to increase the voltage since we run the
> > > > > > > Nano in overdrive mode to run the LPDDR4 at the highest speed.
> > > > > > >
> > > > > > > Might anyone have any suggestions?
> > > > > > >
> > > > > >
> > > > > > Not yet. I have a similar setup but I don't set pmic, so I think that
> > > > > > the symbol is not find
> > > > > > in SPL and it just fail clock registration. Is that possible?
> > > > >
> > > > > It does appear that the clock registration failed.   Since the system
> > > > > does an early return, the I2C clocks among others are also not
> > > > > registered which appears to cause the failure.
> > > > >
> > > > > I changed the registration around a little:
> > > > >
> > > > > ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
> > > > > if (!ret)
> > > > >         clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));
> > > > >
> > > >
> > > > Seems that is not in the image spl so it can not find it. Are you sure
> > > > that you can inside
> > > > the SPL
> > >
> > > I am not sure I understand what you are asking, but the clock node
> > > appears in spl/u-boot-spl.dtb:
> > >
> > >  clock-osc-24m {
> > >                 compatible = "fixed-clock";
> > >                 #clock-cells = <0x00>;
> > >                 clock-frequency = <0x16e3600>;
> > >                 clock-output-names = "osc_24m";
> > >                 phandle = <0x18>;
> > >         };
> > >
> > > Since the clock-output-names is "osc_24m" I wonder if clk_get_by_name
> > > is the right function.  Looking at other uses of this function, it
> > > looks like it's looking for a 'clock-names' parameter and not
> > > clock-output-names.
> >
> > Yes, it's the right answer. That is strange because it works in uboot
> > image. Is the node the same in uboot-dtb?
>
> The U-Boot phase device tree looks like this:
>
>  clock-osc-24m {
>                 compatible = "fixed-clock";
>                 #clock-cells = <0x00>;
>                 clock-frequency = <0x16e3600>;
>                 clock-output-names = "osc_24m";
>                 bootph-pre-ram;
>                 bootph-all;
>                 phandle = <0x18>;
>         };
>

I did some more investigating, and  with the device tree debugging
turned on, I get the following:

U-Boot SPL 2025.01-rc1-00172-g14c2ad85629c-dirty (Nov 09 2024 - 17:10:26 -0600)
clk_set_defaults(i2c1grp)
clk_set_default_parents: could not read assigned-clock-parents for 970690
clk_set_defaults(i2c@30a20000)
clk_set_default_parents: could not read assigned-clock-parents for 970f30
clk_set_defaults(clock-controller@30380000)
clk_set_default_parents: could not read assigned-clock-parents for 970ab8
clk_set_defaults(clock-osc-24m)
clk_set_default_parents: could not read assigned-clock-parents for 9700f8
clk_set_defaults(clock-osc-24m)
clk_set_default_parents: could not read assigned-clock-parents for 9700f8
clk_get_by_name_nodev(node=92a30c, name=osc_24m, clk=96fca8)
clk_get_by_index_tail: Node 'clock-controller@30380000', property
'clocks', failed to request CLK index 1: -22
clk_get_by_index_tail: uclass_get_device_by_of_offset failed: err=-22
Failed to get i2c clk
clk_set_defaults(wdoggrp)
clk_set_default_parents: could not read assigned-clock-parents for 970858
clk_set_defaults(watchdog@30280000)
clk_set_default_parents: could not read assigned-clock-parents for 970500
WDT:   Not starting watchdog@30280000
clk_set_defaults(clock-controller@30380000)
clk_set_default_parents: could not read assigned-clock-parents for 970ab8
clk_get_by_name_nodev(node=92a30c, name=osc_24m, clk=96fd38)
clk_get_by_index_tail: Node 'clock-controller@30380000', property
'clocks', failed to request CLK index 1: -22
Failed to find clock node. Check device tree
Trying to boot from BOOTROM
Boot Stage: Recovery boot

-------

If I modify the code so that it doesn't return early and just skips
the clock registration phase like:
ret = clk_get_by_name(dev, "osc_24m", &osc_24m_clk);
if (!ret)
  clk_dm(IMX8MN_CLK_24M, dev_get_clk_ptr(osc_24m_clk.dev));

------


The above work-around appears to work, and the i2c appears to enable
the clock-osc-24m clock.:

U-Boot SPL 2025.01-rc1-00172-g14c2ad85629c-dirty (Nov 09 2024 - 17:16:06 -0600)
clk_set_defaults(i2c1grp)
clk_set_default_parents: could not read assigned-clock-parents for 970690
clk_set_defaults(i2c@30a20000)
clk_set_default_parents: could not read assigned-clock-parents for 970f30
clk_set_defaults(clock-controller@30380000)
clk_set_default_parents: could not read assigned-clock-parents for 970ab8
clk_set_defaults(clock-osc-24m)
clk_set_default_parents: could not read assigned-clock-parents for 9700f8
clk_set_defaults(clock-osc-24m)
clk_set_default_parents: could not read assigned-clock-parents for 9700f8
clk_get_by_name_nodev(node=92a2fc, name=osc_24m, clk=96fca8)
clk_get_by_index_tail: Node 'clock-controller@30380000', property
'clocks', failed to request CLK index 1: -22
clk_set_defaults(clock-controller@30380000)
clk_set_default_parents: could not read assigned-clock-parents for 970ab8
clk_of_xlate_default(clk=9712a8)
clk_request(dev=970ab8, clk=9712a8)
clk_enable(clk=9712a8 name=clock-controller@30380000)
clk_enable(clk=975180 name=i2c1)
clk_enable(clk=970190 name=clock-osc-24m)

I am not sure what the difference is between osc_24m and clock-osc-24m.

clk_get_rate(clk=9712a8)
clk_get_rate(clk=976980)
clk_get_parent_rate(clk=976980)
clk_get_parent(clk=976980)
clk_get_rate(clk=975180)
clk_get_parent_rate(clk=975180)
clk_get_parent(clk=975180)
clk_get_rate(clk=970190)
clk_set_defaults(pmicirqgrp)
clk_set_default_parents: could not read assigned-clock-parents for 970728
clk_set_defaults(pmic@4b)
clk_set_default_parents: could not read assigned-clock-parents for 970ff8
clk_set_defaults(wdoggrp)
clk_set_default_parents: could not read assigned-clock-parents for 970858
clk_set_defaults(watchdog@30280000)
clk_set_default_parents: could not read assigned-clock-parents for 970500
WDT:   Not starting watchdog@30280000
Trying to boot from BOOTROM

I am not really sure why, and / I was hoping someone might have an
idea.  If not, I might send a patch for this because without it, the
imx8mn_beacon board doesn't run at the proper voltage for the LPDDR4
controller.


adam
> >
> > Michael
> >
> > >
> > > adam
> > > >
> > > > Michael
> > > >
> > > > > This method won't return prematurely, so the I2C and other clocks can
> > > > > be registered, and it appears to address the problem.
> > > > > Another alternative might be to move this clock registration until
> > > > > after all the rest of the clocks are done.
> > > > >
> > > > > adam
> > > > > >
> > > > > > If (ret)
> > > > > >
> > > > > > put here a print
> > > > > >
> > > > > > Michael
> > > > > > > adam
> > > > > > >
> > > > > > > >         base = dev_read_addr_ptr(dev);
> > > > > > > >         if (!base)
> > > > > > > >                 return -EINVAL;
> > > > > > > > --
> > > > > > > > 2.43.0
> > > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Michael Nazzareno Trimarchi
> > > > > > Co-Founder & Chief Executive Officer
> > > > > > M. +39 347 913 2170
> > > > > > michael@amarulasolutions.com
> > > > > > __________________________________
> > > > > >
> > > > > > Amarula Solutions BV
> > > > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > > > > T. +31 (0)85 111 9172
> > > > > > info@amarulasolutions.com
> > > > > > www.amarulasolutions.com
> > > >
> > > >
> > > >
> > > > --
> > > > Michael Nazzareno Trimarchi
> > > > Co-Founder & Chief Executive Officer
> > > > M. +39 347 913 2170
> > > > michael@amarulasolutions.com
> > > > __________________________________
> > > >
> > > > Amarula Solutions BV
> > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > > T. +31 (0)85 111 9172
> > > > info@amarulasolutions.com
> > > > www.amarulasolutions.com
> >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > michael@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > info@amarulasolutions.com
> > www.amarulasolutions.com

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
  

Hi Adam,

On Sat, Nov 9, 2024 at 8:23 PM Adam Ford <aford173@gmail.com> wrote:

> I did some more investigating, and  with the device tree debugging
> turned on, I get the following:
...
> clk_set_defaults(clock-controller@30380000)
> clk_set_default_parents: could not read assigned-clock-parents for 970ab8
> clk_get_by_name_nodev(node=92a30c, name=osc_24m, clk=96fd38)
> clk_get_by_index_tail: Node 'clock-controller@30380000', property
> 'clocks', failed to request CLK index 1: -22
> Failed to find clock node. Check device tree
> Trying to boot from BOOTROM
> Boot Stage: Recovery boot

Could you please test Marek's patch?

https://patchwork.ozlabs.org/project/uboot/patch/20241122015858.557561-1-marex@denx.de/

I plan to apply it for 2025.01 to fix this regression.

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
  

On Fri, Nov 22, 2024 at 6:38 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Adam,
>
> On Sat, Nov 9, 2024 at 8:23 PM Adam Ford <aford173@gmail.com> wrote:
>
> > I did some more investigating, and  with the device tree debugging
> > turned on, I get the following:
> ...
> > clk_set_defaults(clock-controller@30380000)
> > clk_set_default_parents: could not read assigned-clock-parents for 970ab8
> > clk_get_by_name_nodev(node=92a30c, name=osc_24m, clk=96fd38)
> > clk_get_by_index_tail: Node 'clock-controller@30380000', property
> > 'clocks', failed to request CLK index 1: -22
> > Failed to find clock node. Check device tree
> > Trying to boot from BOOTROM
> > Boot Stage: Recovery boot
>
> Could you please test Marek's patch?
>
> https://patchwork.ozlabs.org/project/uboot/patch/20241122015858.557561-1-marex@denx.de/

Sure.  Do you want me to test it against a specific branch or the
master mainline branch?

adam
>
> I plan to apply it for 2025.01 to fix this regression.

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
  

On Fri, Nov 22, 2024 at 10:11 AM Adam Ford <aford173@gmail.com> wrote:

> Sure.  Do you want me to test it against a specific branch or the
> master mainline branch?

Master branch, please.

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
  

On Fri, Nov 22, 2024 at 7:17 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Fri, Nov 22, 2024 at 10:11 AM Adam Ford <aford173@gmail.com> wrote:
>
> > Sure.  Do you want me to test it against a specific branch or the
> > master mainline branch?
>
> Master branch, please.

I should  have a response for you by Monday.

adam

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.