[U-Boot] Converting to SPL_OF_CONTROL

I've long been trying to convert our board (AM3352) to all DM, after
knocking out lots of problems, I've run into one I can't seem to
configure my way around, when I enable SPL_OF_CONTROL.

We have MMC2 as our boot device, with no MMC1 (and hence not present
in the DTB). Once we're into full U-Boot this is all okay, we get the
board's MMC2 appearing as mmc0 and everything's fine.

My problem's during SPL, where the boot device is MMC2, the code tries
to lookup by devnum (which is 1 because it numbers from 0 at this
point), which doesn't exist. I'd expected aliases to work, but we
don't seem to lookup up by alias, only index, so I don't have any way
that I can see of overriding it.

If I make this change, then everything works (with the addition of an
an alias for mmc1 = &mmc2):

 #else

But that feels like a pretty fundamental change to make across every
board and I've no real idea if that would even be the right thing to
do, or if I should be trying to fix this some other way.

Hello Alex,

have you tried to set "u-boot,spl-boot-order" in choosen node?

Take a look into:
doc/device-tree-bindings/chosen.txt

Best regards

Johann Neuhauser 

-----Ursprüngliche Nachricht-----
Von: U-Boot [mailto:u-boot-bounces@lists.denx.de] Im Auftrag von Alex Kiernan
Gesendet: Mittwoch, 1. August 2018 12:42
An: u-boot <u-boot@lists.denx.de>; Simon Glass <sjg@chromium.org>
Betreff: [U-Boot] Converting to SPL_OF_CONTROL

I've long been trying to convert our board (AM3352) to all DM, after knocking out lots of problems, I've run into one I can't seem to configure my way around, when I enable SPL_OF_CONTROL.

We have MMC2 as our boot device, with no MMC1 (and hence not present in the DTB). Once we're into full U-Boot this is all okay, we get the board's MMC2 appearing as mmc0 and everything's fine.

My problem's during SPL, where the boot device is MMC2, the code tries to lookup by devnum (which is 1 because it numbers from 0 at this point), which doesn't exist. I'd expected aliases to work, but we don't seem to lookup up by alias, only index, so I don't have any way that I can see of overriding it.

If I make this change, then everything works (with the addition of an an alias for mmc1 = &mmc2):

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index 0b2f059570..b7544ba183 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -131,7 +131,7 @@ static int spl_mmc_find_device(struct mmc **mmcp,
u32 boot_device)
        }

 #if CONFIG_IS_ENABLED(DM_MMC)
-       err = uclass_get_device(UCLASS_MMC, mmc_dev, &dev);
+       err = uclass_get_device_by_seq(UCLASS_MMC, mmc_dev, &dev);
        if (!err)
                *mmcp = mmc_get_mmc_dev(dev);  #else

But that feels like a pretty fundamental change to make across every board and I've no real idea if that would even be the right thing to do, or if I should be trying to fix this some other way.

--
Alex Kiernan

On Wed, Aug 1, 2018 at 12:30 PM Johann Neuhauser
<jneuhauser@dh-electronics.de> wrote:
>
> Hello Alex,
>
> have you tried to set "u-boot,spl-boot-order" in choosen node?
>
> Take a look into:
> doc/device-tree-bindings/chosen.txt
>

Thanks, I totally failed to see that.

Let me have an investigate!

> Best regards
>
> Johann Neuhauser
>
> -----Ursprüngliche Nachricht-----
> Von: U-Boot [mailto:u-boot-bounces@lists.denx.de] Im Auftrag von Alex Kiernan
> Gesendet: Mittwoch, 1. August 2018 12:42
> An: u-boot <u-boot@lists.denx.de>; Simon Glass <sjg@chromium.org>
> Betreff: [U-Boot] Converting to SPL_OF_CONTROL
>
> I've long been trying to convert our board (AM3352) to all DM, after knocking out lots of problems, I've run into one I can't seem to configure my way around, when I enable SPL_OF_CONTROL.
>
> We have MMC2 as our boot device, with no MMC1 (and hence not present in the DTB). Once we're into full U-Boot this is all okay, we get the board's MMC2 appearing as mmc0 and everything's fine.
>
> My problem's during SPL, where the boot device is MMC2, the code tries to lookup by devnum (which is 1 because it numbers from 0 at this point), which doesn't exist. I'd expected aliases to work, but we don't seem to lookup up by alias, only index, so I don't have any way that I can see of overriding it.
>
> If I make this change, then everything works (with the addition of an an alias for mmc1 = &mmc2):
>
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index 0b2f059570..b7544ba183 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -131,7 +131,7 @@ static int spl_mmc_find_device(struct mmc **mmcp,
> u32 boot_device)
>         }
>
>  #if CONFIG_IS_ENABLED(DM_MMC)
> -       err = uclass_get_device(UCLASS_MMC, mmc_dev, &dev);
> +       err = uclass_get_device_by_seq(UCLASS_MMC, mmc_dev, &dev);
>         if (!err)
>                 *mmcp = mmc_get_mmc_dev(dev);  #else
>
> But that feels like a pretty fundamental change to make across every board and I've no real idea if that would even be the right thing to do, or if I should be trying to fix this some other way.
>
> --
> Alex Kiernan
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot



--
Alex Kiernan

Hi Johann

On Wed, Aug 1, 2018 at 2:18 PM Alex Kiernan <alex.kiernan@gmail.com> wrote:
>
> On Wed, Aug 1, 2018 at 12:30 PM Johann Neuhauser
> <jneuhauser@dh-electronics.de> wrote:
> >
> > Hello Alex,
> >
> > have you tried to set "u-boot,spl-boot-order" in choosen node?
> >
> > Take a look into:
> > doc/device-tree-bindings/chosen.txt
> >
>
> Thanks, I totally failed to see that.
>
> Let me have an investigate!
>

So that only seems to be implemented for rockchip, but it certainly
looks like it's solving the kind of problem I have. The big thing
you've made me realise is that board_boot_order() is a weak function
that I can override...

I'll see what I can do starting from the rockchip code.

Thanks!

> > Best regards
> >
> > Johann Neuhauser
> >
> > -----Ursprüngliche Nachricht-----
> > Von: U-Boot [mailto:u-boot-bounces@lists.denx.de] Im Auftrag von Alex Kiernan
> > Gesendet: Mittwoch, 1. August 2018 12:42
> > An: u-boot <u-boot@lists.denx.de>; Simon Glass <sjg@chromium.org>
> > Betreff: [U-Boot] Converting to SPL_OF_CONTROL
> >
> > I've long been trying to convert our board (AM3352) to all DM, after knocking out lots of problems, I've run into one I can't seem to configure my way around, when I enable SPL_OF_CONTROL.
> >
> > We have MMC2 as our boot device, with no MMC1 (and hence not present in the DTB). Once we're into full U-Boot this is all okay, we get the board's MMC2 appearing as mmc0 and everything's fine.
> >
> > My problem's during SPL, where the boot device is MMC2, the code tries to lookup by devnum (which is 1 because it numbers from 0 at this point), which doesn't exist. I'd expected aliases to work, but we don't seem to lookup up by alias, only index, so I don't have any way that I can see of overriding it.
> >
> > If I make this change, then everything works (with the addition of an an alias for mmc1 = &mmc2):
> >
> > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index 0b2f059570..b7544ba183 100644
> > --- a/common/spl/spl_mmc.c
> > +++ b/common/spl/spl_mmc.c
> > @@ -131,7 +131,7 @@ static int spl_mmc_find_device(struct mmc **mmcp,
> > u32 boot_device)
> >         }
> >
> >  #if CONFIG_IS_ENABLED(DM_MMC)
> > -       err = uclass_get_device(UCLASS_MMC, mmc_dev, &dev);
> > +       err = uclass_get_device_by_seq(UCLASS_MMC, mmc_dev, &dev);
> >         if (!err)
> >                 *mmcp = mmc_get_mmc_dev(dev);  #else
> >
> > But that feels like a pretty fundamental change to make across every board and I've no real idea if that would even be the right thing to do, or if I should be trying to fix this some other way.
> >
> > --
> > Alex Kiernan
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > https://lists.denx.de/listinfo/u-boot
>
>
>
> --
> Alex Kiernan



--
Alex Kiernan

On Wed, Aug 1, 2018 at 11:41 AM Alex Kiernan <alex.kiernan@gmail.com> wrote:
>
> I've long been trying to convert our board (AM3352) to all DM, after
> knocking out lots of problems, I've run into one I can't seem to
> configure my way around, when I enable SPL_OF_CONTROL.
>
> We have MMC2 as our boot device, with no MMC1 (and hence not present
> in the DTB). Once we're into full U-Boot this is all okay, we get the
> board's MMC2 appearing as mmc0 and everything's fine.
>
> My problem's during SPL, where the boot device is MMC2, the code tries
> to lookup by devnum (which is 1 because it numbers from 0 at this
> point), which doesn't exist. I'd expected aliases to work, but we
> don't seem to lookup up by alias, only index, so I don't have any way
> that I can see of overriding it.
>
> If I make this change, then everything works (with the addition of an
> an alias for mmc1 = &mmc2):
>
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index 0b2f059570..b7544ba183 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -131,7 +131,7 @@ static int spl_mmc_find_device(struct mmc **mmcp,
> u32 boot_device)
>         }
>
>  #if CONFIG_IS_ENABLED(DM_MMC)
> -       err = uclass_get_device(UCLASS_MMC, mmc_dev, &dev);
> +       err = uclass_get_device_by_seq(UCLASS_MMC, mmc_dev, &dev);
>         if (!err)
>                 *mmcp = mmc_get_mmc_dev(dev);
>  #else
>
> But that feels like a pretty fundamental change to make across every
> board and I've no real idea if that would even be the right thing to
> do, or if I should be trying to fix this some other way.
>

Thanks to Johan for pointing me at the Rockchip code; with that I can
get the board to use the right MMC in SPL, but I still have the same
problem, I'm booting from MMC2, but because all the numbering is by
devnum, not sequence, I end up with a board_boot_order function that's
the equivalent of this:

void board_boot_order(u32 *spl_boot_list)
{
        u32 boot_device = spl_boot_device();

        if (boot_device == BOOT_DEVICE_MMC2)
                spl_boot_list[0] = BOOT_DEVICE_MMC1;
        else
                spl_boot_list[0] = boot_device;
}

Which seems fundamentally wrong...

I'm assuming that the change I was originally playing with is a non-starter:

--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -131,7 +131,7 @@ static int spl_mmc_find_device(struct mmc **mmcp,
u32 boot_device)
        }

#if CONFIG_IS_ENABLED(DM_MMC)
-       err = uclass_get_device(UCLASS_MMC, mmc_dev, &dev);
+       err = uclass_get_device_by_seq(UCLASS_MMC, mmc_dev, &dev);
        if (!err)
                *mmcp = mmc_get_mmc_dev(dev);
#else

Obviously I could guard that change with yet another symbol, but that
feels like it's papering over the problem... anyone got a pointer as
to what the right way to fix this is?

--
Alex Kiernan