[v2,1/9] spl: Try to get SPL boot device via board_get_int

Message ID 20200519192340.16624-2-jagan@amarulasolutions.com
State New
Headers show
Series
  • riscv: sifive/fu540: Booting from SPI
Related show

Commit Message

Jagan Teki May 19, 2020, 7:23 p.m.
Usually, the associated board would supply spl boot device
using spl_boot_device() but some boards have board driver
that are possible to supply boot device via board_get_int
with BOARD_SPL_BOOT_DEVICE id.

This patch add support for those.

Cc: Mario Six <mario.six at gdsys.cc>
Cc: Tom Rini <trini at konsulko.com>
Cc: Simon Glass <sjg at chromium.org>
Cc: Jean-Jacques Hiblot <jjhiblot at ti.com>
Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
---
Changes for v2:
- new patch

 common/spl/spl.c | 14 +++++++++++++-
 include/board.h  |  9 +++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

Comments

Tom Rini May 20, 2020, 1:02 p.m. | #1
On Wed, May 20, 2020 at 12:53:32AM +0530, Jagan Teki wrote:

> Usually, the associated board would supply spl boot device
> using spl_boot_device() but some boards have board driver
> that are possible to supply boot device via board_get_int
> with BOARD_SPL_BOOT_DEVICE id.
> 
> This patch add support for those.
> 
> Cc: Mario Six <mario.six at gdsys.cc>
> Cc: Tom Rini <trini at konsulko.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Jean-Jacques Hiblot <jjhiblot at ti.com>
> Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> ---
> Changes for v2:
> - new patch
> 
>  common/spl/spl.c | 14 +++++++++++++-
>  include/board.h  |  9 +++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index fc5cbbbeba..a07b71b3c1 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -9,6 +9,7 @@
>  #include <common.h>
>  #include <bloblist.h>
>  #include <binman_sym.h>
> +#include <board.h>
>  #include <dm.h>
>  #include <handoff.h>
>  #include <hang.h>
> @@ -483,9 +484,20 @@ int spl_init(void)
>  #define BOOT_DEVICE_NONE 0xdeadbeef
>  #endif
>  
> +__weak u32 spl_boot_device(void)
> +{
> +	return 0;
> +}
> +
>  __weak void board_boot_order(u32 *spl_boot_list)
>  {
> -	spl_boot_list[0] = spl_boot_device();
> +	struct udevice *board;
> +
> +	if (!board_get(&board))
> +		board_get_int(board, BOARD_SPL_BOOT_DEVICE,
> +			      (int *)&spl_boot_list[0]);
> +	else
> +		spl_boot_list[0] = spl_boot_device();
>  }
>  
>  static struct spl_image_loader *spl_ll_find_loader(uint boot_device)
> diff --git a/include/board.h b/include/board.h
> index 678b652b0a..ce4eaba38d 100644
> --- a/include/board.h
> +++ b/include/board.h
> @@ -211,3 +211,12 @@ static inline int board_get_fit_loadable(struct udevice *dev, int index,
>  }
>  
>  #endif
> +
> +/**
> + * Common board unique identifier
> + *
> + * @BOARD_SPL_BOOT_DEVICE:	id to get SPL boot device.
> + */
> +enum common_ids {
> +	BOARD_SPL_BOOT_DEVICE,
> +};

I don't understand why we need this abstraction.  The intention of what
we have today is that the generic SPL framework calls out to something
to ask "what are we booted from?".  Why can the board driver not just
supply that information?  Thanks!
Jagan Teki May 20, 2020, 1:16 p.m. | #2
On Wed, May 20, 2020 at 6:32 PM Tom Rini <trini at konsulko.com> wrote:
>
> On Wed, May 20, 2020 at 12:53:32AM +0530, Jagan Teki wrote:
>
> > Usually, the associated board would supply spl boot device
> > using spl_boot_device() but some boards have board driver
> > that are possible to supply boot device via board_get_int
> > with BOARD_SPL_BOOT_DEVICE id.
> >
> > This patch add support for those.
> >
> > Cc: Mario Six <mario.six at gdsys.cc>
> > Cc: Tom Rini <trini at konsulko.com>
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Jean-Jacques Hiblot <jjhiblot at ti.com>
> > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> > ---
> > Changes for v2:
> > - new patch
> >
> >  common/spl/spl.c | 14 +++++++++++++-
> >  include/board.h  |  9 +++++++++
> >  2 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > index fc5cbbbeba..a07b71b3c1 100644
> > --- a/common/spl/spl.c
> > +++ b/common/spl/spl.c
> > @@ -9,6 +9,7 @@
> >  #include <common.h>
> >  #include <bloblist.h>
> >  #include <binman_sym.h>
> > +#include <board.h>
> >  #include <dm.h>
> >  #include <handoff.h>
> >  #include <hang.h>
> > @@ -483,9 +484,20 @@ int spl_init(void)
> >  #define BOOT_DEVICE_NONE 0xdeadbeef
> >  #endif
> >
> > +__weak u32 spl_boot_device(void)
> > +{
> > +     return 0;
> > +}
> > +
> >  __weak void board_boot_order(u32 *spl_boot_list)
> >  {
> > -     spl_boot_list[0] = spl_boot_device();
> > +     struct udevice *board;
> > +
> > +     if (!board_get(&board))
> > +             board_get_int(board, BOARD_SPL_BOOT_DEVICE,
> > +                           (int *)&spl_boot_list[0]);
> > +     else
> > +             spl_boot_list[0] = spl_boot_device();
> >  }
> >
> >  static struct spl_image_loader *spl_ll_find_loader(uint boot_device)
> > diff --git a/include/board.h b/include/board.h
> > index 678b652b0a..ce4eaba38d 100644
> > --- a/include/board.h
> > +++ b/include/board.h
> > @@ -211,3 +211,12 @@ static inline int board_get_fit_loadable(struct udevice *dev, int index,
> >  }
> >
> >  #endif
> > +
> > +/**
> > + * Common board unique identifier
> > + *
> > + * @BOARD_SPL_BOOT_DEVICE:   id to get SPL boot device.
> > + */
> > +enum common_ids {
> > +     BOARD_SPL_BOOT_DEVICE,
> > +};
>
> I don't understand why we need this abstraction.  The intention of what
> we have today is that the generic SPL framework calls out to something
> to ask "what are we booted from?".  Why can the board driver not just
> supply that information?  Thanks!

Yes, we can update boot-device on respective areas by probing board
driver and assign spl_boot_list[0] by explicitly define
spl_boot_device function, but this change bypass all these codes. Just
like how we did on SPL fit to load the concerned image via board
driver.

152781d4641e0e4c37b3a32f699cf99aeec877c8
"spl: fit: Allow the board to tell if more images must be loaded from FIT"

Jagan.
Tom Rini May 20, 2020, 2:10 p.m. | #3
On Wed, May 20, 2020 at 06:46:55PM +0530, Jagan Teki wrote:
> On Wed, May 20, 2020 at 6:32 PM Tom Rini <trini at konsulko.com> wrote:
> >
> > On Wed, May 20, 2020 at 12:53:32AM +0530, Jagan Teki wrote:
> >
> > > Usually, the associated board would supply spl boot device
> > > using spl_boot_device() but some boards have board driver
> > > that are possible to supply boot device via board_get_int
> > > with BOARD_SPL_BOOT_DEVICE id.
> > >
> > > This patch add support for those.
> > >
> > > Cc: Mario Six <mario.six at gdsys.cc>
> > > Cc: Tom Rini <trini at konsulko.com>
> > > Cc: Simon Glass <sjg at chromium.org>
> > > Cc: Jean-Jacques Hiblot <jjhiblot at ti.com>
> > > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> > > ---
> > > Changes for v2:
> > > - new patch
> > >
> > >  common/spl/spl.c | 14 +++++++++++++-
> > >  include/board.h  |  9 +++++++++
> > >  2 files changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > > index fc5cbbbeba..a07b71b3c1 100644
> > > --- a/common/spl/spl.c
> > > +++ b/common/spl/spl.c
> > > @@ -9,6 +9,7 @@
> > >  #include <common.h>
> > >  #include <bloblist.h>
> > >  #include <binman_sym.h>
> > > +#include <board.h>
> > >  #include <dm.h>
> > >  #include <handoff.h>
> > >  #include <hang.h>
> > > @@ -483,9 +484,20 @@ int spl_init(void)
> > >  #define BOOT_DEVICE_NONE 0xdeadbeef
> > >  #endif
> > >
> > > +__weak u32 spl_boot_device(void)
> > > +{
> > > +     return 0;
> > > +}
> > > +
> > >  __weak void board_boot_order(u32 *spl_boot_list)
> > >  {
> > > -     spl_boot_list[0] = spl_boot_device();
> > > +     struct udevice *board;
> > > +
> > > +     if (!board_get(&board))
> > > +             board_get_int(board, BOARD_SPL_BOOT_DEVICE,
> > > +                           (int *)&spl_boot_list[0]);
> > > +     else
> > > +             spl_boot_list[0] = spl_boot_device();
> > >  }
> > >
> > >  static struct spl_image_loader *spl_ll_find_loader(uint boot_device)
> > > diff --git a/include/board.h b/include/board.h
> > > index 678b652b0a..ce4eaba38d 100644
> > > --- a/include/board.h
> > > +++ b/include/board.h
> > > @@ -211,3 +211,12 @@ static inline int board_get_fit_loadable(struct udevice *dev, int index,
> > >  }
> > >
> > >  #endif
> > > +
> > > +/**
> > > + * Common board unique identifier
> > > + *
> > > + * @BOARD_SPL_BOOT_DEVICE:   id to get SPL boot device.
> > > + */
> > > +enum common_ids {
> > > +     BOARD_SPL_BOOT_DEVICE,
> > > +};
> >
> > I don't understand why we need this abstraction.  The intention of what
> > we have today is that the generic SPL framework calls out to something
> > to ask "what are we booted from?".  Why can the board driver not just
> > supply that information?  Thanks!
> 
> Yes, we can update boot-device on respective areas by probing board
> driver and assign spl_boot_list[0] by explicitly define
> spl_boot_device function, but this change bypass all these codes. Just
> like how we did on SPL fit to load the concerned image via board
> driver.

I still don't get it, sorry.  Why is spl_boot_device() not provided by
the "board" driver to say what to boot in this case?
Jagan Teki May 22, 2020, 4:12 p.m. | #4
On Wed, May 20, 2020 at 7:40 PM Tom Rini <trini at konsulko.com> wrote:
>
> On Wed, May 20, 2020 at 06:46:55PM +0530, Jagan Teki wrote:
> > On Wed, May 20, 2020 at 6:32 PM Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Wed, May 20, 2020 at 12:53:32AM +0530, Jagan Teki wrote:
> > >
> > > > Usually, the associated board would supply spl boot device
> > > > using spl_boot_device() but some boards have board driver
> > > > that are possible to supply boot device via board_get_int
> > > > with BOARD_SPL_BOOT_DEVICE id.
> > > >
> > > > This patch add support for those.
> > > >
> > > > Cc: Mario Six <mario.six at gdsys.cc>
> > > > Cc: Tom Rini <trini at konsulko.com>
> > > > Cc: Simon Glass <sjg at chromium.org>
> > > > Cc: Jean-Jacques Hiblot <jjhiblot at ti.com>
> > > > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> > > > ---
> > > > Changes for v2:
> > > > - new patch
> > > >
> > > >  common/spl/spl.c | 14 +++++++++++++-
> > > >  include/board.h  |  9 +++++++++
> > > >  2 files changed, 22 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > > > index fc5cbbbeba..a07b71b3c1 100644
> > > > --- a/common/spl/spl.c
> > > > +++ b/common/spl/spl.c
> > > > @@ -9,6 +9,7 @@
> > > >  #include <common.h>
> > > >  #include <bloblist.h>
> > > >  #include <binman_sym.h>
> > > > +#include <board.h>
> > > >  #include <dm.h>
> > > >  #include <handoff.h>
> > > >  #include <hang.h>
> > > > @@ -483,9 +484,20 @@ int spl_init(void)
> > > >  #define BOOT_DEVICE_NONE 0xdeadbeef
> > > >  #endif
> > > >
> > > > +__weak u32 spl_boot_device(void)
> > > > +{
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  __weak void board_boot_order(u32 *spl_boot_list)
> > > >  {
> > > > -     spl_boot_list[0] = spl_boot_device();
> > > > +     struct udevice *board;
> > > > +
> > > > +     if (!board_get(&board))
> > > > +             board_get_int(board, BOARD_SPL_BOOT_DEVICE,
> > > > +                           (int *)&spl_boot_list[0]);
> > > > +     else
> > > > +             spl_boot_list[0] = spl_boot_device();
> > > >  }
> > > >
> > > >  static struct spl_image_loader *spl_ll_find_loader(uint boot_device)
> > > > diff --git a/include/board.h b/include/board.h
> > > > index 678b652b0a..ce4eaba38d 100644
> > > > --- a/include/board.h
> > > > +++ b/include/board.h
> > > > @@ -211,3 +211,12 @@ static inline int board_get_fit_loadable(struct udevice *dev, int index,
> > > >  }
> > > >
> > > >  #endif
> > > > +
> > > > +/**
> > > > + * Common board unique identifier
> > > > + *
> > > > + * @BOARD_SPL_BOOT_DEVICE:   id to get SPL boot device.
> > > > + */
> > > > +enum common_ids {
> > > > +     BOARD_SPL_BOOT_DEVICE,
> > > > +};
> > >
> > > I don't understand why we need this abstraction.  The intention of what
> > > we have today is that the generic SPL framework calls out to something
> > > to ask "what are we booted from?".  Why can the board driver not just
> > > supply that information?  Thanks!
> >
> > Yes, we can update boot-device on respective areas by probing board
> > driver and assign spl_boot_list[0] by explicitly define
> > spl_boot_device function, but this change bypass all these codes. Just
> > like how we did on SPL fit to load the concerned image via board
> > driver.
>
> I still don't get it, sorry.  Why is spl_boot_device() not provided by
> the "board" driver to say what to boot in this case?

That means, we have to add spl_boot_device in board-uclass.c ? so-that
respective board driver shall use?

Jagan.
Tom Rini May 22, 2020, 9:25 p.m. | #5
On Fri, May 22, 2020 at 09:42:22PM +0530, Jagan Teki wrote:
> On Wed, May 20, 2020 at 7:40 PM Tom Rini <trini at konsulko.com> wrote:
> >
> > On Wed, May 20, 2020 at 06:46:55PM +0530, Jagan Teki wrote:
> > > On Wed, May 20, 2020 at 6:32 PM Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Wed, May 20, 2020 at 12:53:32AM +0530, Jagan Teki wrote:
> > > >
> > > > > Usually, the associated board would supply spl boot device
> > > > > using spl_boot_device() but some boards have board driver
> > > > > that are possible to supply boot device via board_get_int
> > > > > with BOARD_SPL_BOOT_DEVICE id.
> > > > >
> > > > > This patch add support for those.
> > > > >
> > > > > Cc: Mario Six <mario.six at gdsys.cc>
> > > > > Cc: Tom Rini <trini at konsulko.com>
> > > > > Cc: Simon Glass <sjg at chromium.org>
> > > > > Cc: Jean-Jacques Hiblot <jjhiblot at ti.com>
> > > > > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> > > > > ---
> > > > > Changes for v2:
> > > > > - new patch
> > > > >
> > > > >  common/spl/spl.c | 14 +++++++++++++-
> > > > >  include/board.h  |  9 +++++++++
> > > > >  2 files changed, 22 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > > > > index fc5cbbbeba..a07b71b3c1 100644
> > > > > --- a/common/spl/spl.c
> > > > > +++ b/common/spl/spl.c
> > > > > @@ -9,6 +9,7 @@
> > > > >  #include <common.h>
> > > > >  #include <bloblist.h>
> > > > >  #include <binman_sym.h>
> > > > > +#include <board.h>
> > > > >  #include <dm.h>
> > > > >  #include <handoff.h>
> > > > >  #include <hang.h>
> > > > > @@ -483,9 +484,20 @@ int spl_init(void)
> > > > >  #define BOOT_DEVICE_NONE 0xdeadbeef
> > > > >  #endif
> > > > >
> > > > > +__weak u32 spl_boot_device(void)
> > > > > +{
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > >  __weak void board_boot_order(u32 *spl_boot_list)
> > > > >  {
> > > > > -     spl_boot_list[0] = spl_boot_device();
> > > > > +     struct udevice *board;
> > > > > +
> > > > > +     if (!board_get(&board))
> > > > > +             board_get_int(board, BOARD_SPL_BOOT_DEVICE,
> > > > > +                           (int *)&spl_boot_list[0]);
> > > > > +     else
> > > > > +             spl_boot_list[0] = spl_boot_device();
> > > > >  }
> > > > >
> > > > >  static struct spl_image_loader *spl_ll_find_loader(uint boot_device)
> > > > > diff --git a/include/board.h b/include/board.h
> > > > > index 678b652b0a..ce4eaba38d 100644
> > > > > --- a/include/board.h
> > > > > +++ b/include/board.h
> > > > > @@ -211,3 +211,12 @@ static inline int board_get_fit_loadable(struct udevice *dev, int index,
> > > > >  }
> > > > >
> > > > >  #endif
> > > > > +
> > > > > +/**
> > > > > + * Common board unique identifier
> > > > > + *
> > > > > + * @BOARD_SPL_BOOT_DEVICE:   id to get SPL boot device.
> > > > > + */
> > > > > +enum common_ids {
> > > > > +     BOARD_SPL_BOOT_DEVICE,
> > > > > +};
> > > >
> > > > I don't understand why we need this abstraction.  The intention of what
> > > > we have today is that the generic SPL framework calls out to something
> > > > to ask "what are we booted from?".  Why can the board driver not just
> > > > supply that information?  Thanks!
> > >
> > > Yes, we can update boot-device on respective areas by probing board
> > > driver and assign spl_boot_list[0] by explicitly define
> > > spl_boot_device function, but this change bypass all these codes. Just
> > > like how we did on SPL fit to load the concerned image via board
> > > driver.
> >
> > I still don't get it, sorry.  Why is spl_boot_device() not provided by
> > the "board" driver to say what to boot in this case?
> 
> That means, we have to add spl_boot_device in board-uclass.c ? so-that
> respective board driver shall use?

Yes, or perhaps a board driver doesn't even make sense in this case and
the existing abstraction should be used as is?  This isn't a unique
problem, it's something we've been handling in SPL since the beginning.
In so far as we can now try and solve this problem with something
DM-based instead of not, it should still I believe just be the same
function call.
Jagan Teki May 25, 2020, 8:31 a.m. | #6
On Sat, May 23, 2020 at 2:55 AM Tom Rini <trini at konsulko.com> wrote:
>
> On Fri, May 22, 2020 at 09:42:22PM +0530, Jagan Teki wrote:
> > On Wed, May 20, 2020 at 7:40 PM Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Wed, May 20, 2020 at 06:46:55PM +0530, Jagan Teki wrote:
> > > > On Wed, May 20, 2020 at 6:32 PM Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Wed, May 20, 2020 at 12:53:32AM +0530, Jagan Teki wrote:
> > > > >
> > > > > > Usually, the associated board would supply spl boot device
> > > > > > using spl_boot_device() but some boards have board driver
> > > > > > that are possible to supply boot device via board_get_int
> > > > > > with BOARD_SPL_BOOT_DEVICE id.
> > > > > >
> > > > > > This patch add support for those.
> > > > > >
> > > > > > Cc: Mario Six <mario.six at gdsys.cc>
> > > > > > Cc: Tom Rini <trini at konsulko.com>
> > > > > > Cc: Simon Glass <sjg at chromium.org>
> > > > > > Cc: Jean-Jacques Hiblot <jjhiblot at ti.com>
> > > > > > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> > > > > > ---
> > > > > > Changes for v2:
> > > > > > - new patch
> > > > > >
> > > > > >  common/spl/spl.c | 14 +++++++++++++-
> > > > > >  include/board.h  |  9 +++++++++
> > > > > >  2 files changed, 22 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > > > > > index fc5cbbbeba..a07b71b3c1 100644
> > > > > > --- a/common/spl/spl.c
> > > > > > +++ b/common/spl/spl.c
> > > > > > @@ -9,6 +9,7 @@
> > > > > >  #include <common.h>
> > > > > >  #include <bloblist.h>
> > > > > >  #include <binman_sym.h>
> > > > > > +#include <board.h>
> > > > > >  #include <dm.h>
> > > > > >  #include <handoff.h>
> > > > > >  #include <hang.h>
> > > > > > @@ -483,9 +484,20 @@ int spl_init(void)
> > > > > >  #define BOOT_DEVICE_NONE 0xdeadbeef
> > > > > >  #endif
> > > > > >
> > > > > > +__weak u32 spl_boot_device(void)
> > > > > > +{
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > >  __weak void board_boot_order(u32 *spl_boot_list)
> > > > > >  {
> > > > > > -     spl_boot_list[0] = spl_boot_device();
> > > > > > +     struct udevice *board;
> > > > > > +
> > > > > > +     if (!board_get(&board))
> > > > > > +             board_get_int(board, BOARD_SPL_BOOT_DEVICE,
> > > > > > +                           (int *)&spl_boot_list[0]);
> > > > > > +     else
> > > > > > +             spl_boot_list[0] = spl_boot_device();
> > > > > >  }
> > > > > >
> > > > > >  static struct spl_image_loader *spl_ll_find_loader(uint boot_device)
> > > > > > diff --git a/include/board.h b/include/board.h
> > > > > > index 678b652b0a..ce4eaba38d 100644
> > > > > > --- a/include/board.h
> > > > > > +++ b/include/board.h
> > > > > > @@ -211,3 +211,12 @@ static inline int board_get_fit_loadable(struct udevice *dev, int index,
> > > > > >  }
> > > > > >
> > > > > >  #endif
> > > > > > +
> > > > > > +/**
> > > > > > + * Common board unique identifier
> > > > > > + *
> > > > > > + * @BOARD_SPL_BOOT_DEVICE:   id to get SPL boot device.
> > > > > > + */
> > > > > > +enum common_ids {
> > > > > > +     BOARD_SPL_BOOT_DEVICE,
> > > > > > +};
> > > > >
> > > > > I don't understand why we need this abstraction.  The intention of what
> > > > > we have today is that the generic SPL framework calls out to something
> > > > > to ask "what are we booted from?".  Why can the board driver not just
> > > > > supply that information?  Thanks!
> > > >
> > > > Yes, we can update boot-device on respective areas by probing board
> > > > driver and assign spl_boot_list[0] by explicitly define
> > > > spl_boot_device function, but this change bypass all these codes. Just
> > > > like how we did on SPL fit to load the concerned image via board
> > > > driver.
> > >
> > > I still don't get it, sorry.  Why is spl_boot_device() not provided by
> > > the "board" driver to say what to boot in this case?
> >
> > That means, we have to add spl_boot_device in board-uclass.c ? so-that
> > respective board driver shall use?
>
> Yes, or perhaps a board driver doesn't even make sense in this case and
> the existing abstraction should be used as is?  This isn't a unique
> problem, it's something we've been handling in SPL since the beginning.
> In so far as we can now try and solve this problem with something
> DM-based instead of not, it should still I believe just be the same
> function call.

I don't understand why we have individual function calls though we
have dm supported cases to get rid of those? then why would be the
case for spl fit code to pump multiple images via dm board driver?
What this patch is trying to do is fundamentally similar, like spl_fit
is using a board driver to support multiple fit images where spl is
using a board driver to support the desired boot device.

Jagan.
Tom Rini May 25, 2020, 1:15 p.m. | #7
On Mon, May 25, 2020 at 02:01:09PM +0530, Jagan Teki wrote:
> On Sat, May 23, 2020 at 2:55 AM Tom Rini <trini at konsulko.com> wrote:
> >
> > On Fri, May 22, 2020 at 09:42:22PM +0530, Jagan Teki wrote:
> > > On Wed, May 20, 2020 at 7:40 PM Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Wed, May 20, 2020 at 06:46:55PM +0530, Jagan Teki wrote:
> > > > > On Wed, May 20, 2020 at 6:32 PM Tom Rini <trini at konsulko.com> wrote:
> > > > > >
> > > > > > On Wed, May 20, 2020 at 12:53:32AM +0530, Jagan Teki wrote:
> > > > > >
> > > > > > > Usually, the associated board would supply spl boot device
> > > > > > > using spl_boot_device() but some boards have board driver
> > > > > > > that are possible to supply boot device via board_get_int
> > > > > > > with BOARD_SPL_BOOT_DEVICE id.
> > > > > > >
> > > > > > > This patch add support for those.
> > > > > > >
> > > > > > > Cc: Mario Six <mario.six at gdsys.cc>
> > > > > > > Cc: Tom Rini <trini at konsulko.com>
> > > > > > > Cc: Simon Glass <sjg at chromium.org>
> > > > > > > Cc: Jean-Jacques Hiblot <jjhiblot at ti.com>
> > > > > > > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> > > > > > > ---
> > > > > > > Changes for v2:
> > > > > > > - new patch
> > > > > > >
> > > > > > >  common/spl/spl.c | 14 +++++++++++++-
> > > > > > >  include/board.h  |  9 +++++++++
> > > > > > >  2 files changed, 22 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > > > > > > index fc5cbbbeba..a07b71b3c1 100644
> > > > > > > --- a/common/spl/spl.c
> > > > > > > +++ b/common/spl/spl.c
> > > > > > > @@ -9,6 +9,7 @@
> > > > > > >  #include <common.h>
> > > > > > >  #include <bloblist.h>
> > > > > > >  #include <binman_sym.h>
> > > > > > > +#include <board.h>
> > > > > > >  #include <dm.h>
> > > > > > >  #include <handoff.h>
> > > > > > >  #include <hang.h>
> > > > > > > @@ -483,9 +484,20 @@ int spl_init(void)
> > > > > > >  #define BOOT_DEVICE_NONE 0xdeadbeef
> > > > > > >  #endif
> > > > > > >
> > > > > > > +__weak u32 spl_boot_device(void)
> > > > > > > +{
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >  __weak void board_boot_order(u32 *spl_boot_list)
> > > > > > >  {
> > > > > > > -     spl_boot_list[0] = spl_boot_device();
> > > > > > > +     struct udevice *board;
> > > > > > > +
> > > > > > > +     if (!board_get(&board))
> > > > > > > +             board_get_int(board, BOARD_SPL_BOOT_DEVICE,
> > > > > > > +                           (int *)&spl_boot_list[0]);
> > > > > > > +     else
> > > > > > > +             spl_boot_list[0] = spl_boot_device();
> > > > > > >  }
> > > > > > >
> > > > > > >  static struct spl_image_loader *spl_ll_find_loader(uint boot_device)
> > > > > > > diff --git a/include/board.h b/include/board.h
> > > > > > > index 678b652b0a..ce4eaba38d 100644
> > > > > > > --- a/include/board.h
> > > > > > > +++ b/include/board.h
> > > > > > > @@ -211,3 +211,12 @@ static inline int board_get_fit_loadable(struct udevice *dev, int index,
> > > > > > >  }
> > > > > > >
> > > > > > >  #endif
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * Common board unique identifier
> > > > > > > + *
> > > > > > > + * @BOARD_SPL_BOOT_DEVICE:   id to get SPL boot device.
> > > > > > > + */
> > > > > > > +enum common_ids {
> > > > > > > +     BOARD_SPL_BOOT_DEVICE,
> > > > > > > +};
> > > > > >
> > > > > > I don't understand why we need this abstraction.  The intention of what
> > > > > > we have today is that the generic SPL framework calls out to something
> > > > > > to ask "what are we booted from?".  Why can the board driver not just
> > > > > > supply that information?  Thanks!
> > > > >
> > > > > Yes, we can update boot-device on respective areas by probing board
> > > > > driver and assign spl_boot_list[0] by explicitly define
> > > > > spl_boot_device function, but this change bypass all these codes. Just
> > > > > like how we did on SPL fit to load the concerned image via board
> > > > > driver.
> > > >
> > > > I still don't get it, sorry.  Why is spl_boot_device() not provided by
> > > > the "board" driver to say what to boot in this case?
> > >
> > > That means, we have to add spl_boot_device in board-uclass.c ? so-that
> > > respective board driver shall use?
> >
> > Yes, or perhaps a board driver doesn't even make sense in this case and
> > the existing abstraction should be used as is?  This isn't a unique
> > problem, it's something we've been handling in SPL since the beginning.
> > In so far as we can now try and solve this problem with something
> > DM-based instead of not, it should still I believe just be the same
> > function call.
> 
> I don't understand why we have individual function calls though we
> have dm supported cases to get rid of those? then why would be the
> case for spl fit code to pump multiple images via dm board driver?
> What this patch is trying to do is fundamentally similar, like spl_fit
> is using a board driver to support multiple fit images where spl is
> using a board driver to support the desired boot device.

Because when using DM it needs to be to improve things overall, not just
duplicate them.  We have an abstraction for "figure out what device
we've booted from" already.  I don't see how your changes are improving
the situation, just making the same abstraction with more calls.

When we're dealing with SPL we need to be even more thoughtful than
usual about size impacts.  So the DM implementation of spl_boot_device()
belongs elsewhere rather than making SPL do some checks and doing DM or
not DM.  Thanks!

Patch

diff --git a/common/spl/spl.c b/common/spl/spl.c
index fc5cbbbeba..a07b71b3c1 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -9,6 +9,7 @@ 
 #include <common.h>
 #include <bloblist.h>
 #include <binman_sym.h>
+#include <board.h>
 #include <dm.h>
 #include <handoff.h>
 #include <hang.h>
@@ -483,9 +484,20 @@  int spl_init(void)
 #define BOOT_DEVICE_NONE 0xdeadbeef
 #endif
 
+__weak u32 spl_boot_device(void)
+{
+	return 0;
+}
+
 __weak void board_boot_order(u32 *spl_boot_list)
 {
-	spl_boot_list[0] = spl_boot_device();
+	struct udevice *board;
+
+	if (!board_get(&board))
+		board_get_int(board, BOARD_SPL_BOOT_DEVICE,
+			      (int *)&spl_boot_list[0]);
+	else
+		spl_boot_list[0] = spl_boot_device();
 }
 
 static struct spl_image_loader *spl_ll_find_loader(uint boot_device)
diff --git a/include/board.h b/include/board.h
index 678b652b0a..ce4eaba38d 100644
--- a/include/board.h
+++ b/include/board.h
@@ -211,3 +211,12 @@  static inline int board_get_fit_loadable(struct udevice *dev, int index,
 }
 
 #endif
+
+/**
+ * Common board unique identifier
+ *
+ * @BOARD_SPL_BOOT_DEVICE:	id to get SPL boot device.
+ */
+enum common_ids {
+	BOARD_SPL_BOOT_DEVICE,
+};