Re: [PATCH v2 1/1] spl: initialize PCI before booting

[Heinrich Schuchardt]

On 25.07.23 16:51, Simon Glass wrote:
On Mon, 24 Jul 2023 at 14:19, Heinrich Schuchardt
<heinrich.schucha...@canonical.com> wrote:

MMC, SATA, and USB may be using PCI based controllers.
Initialize the PCI sub-system before trying to boot.

Remove the initialization for NVMe that is now redundant.

Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com>
---
v2:
         Centralize the PCI initialization
---
  common/spl/spl.c      | 7 +++++++
  common/spl/spl_nvme.c | 5 -----
  2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index f09bb97781..0062f3f45d 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -800,6 +800,13 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
             IS_ENABLED(CONFIG_SPL_ATF))
                 dram_init_banksize();

+       if (CONFIG_IS_ENABLED(PCI)) {
+               ret = pci_init();
+               if (ret)
+                       puts(SPL_TPL_PROMPT "Cannot initialize PCI\n");
+               /* Don't fail. We still can try other boot methods. */

But which ones? This seems dubious to me, since there is no check (for
each loader) as to whether PCI is needed or not. If PCI cannot be set
up, we should probably return an error.

There is no caller to which we could reasonably return. The alternative 
would be to call hang() as in the code above this location. An 
unrecoverable device is a worst case scenario. So you definitively don't 
want to call hang() here.

It is typical to have a mix of loaders that need PCI and others that 
don't. E.g. NVMe will require PCI but MMC may not. In this case if PCI 
cannot be initialized, the NVMe device is not found and booting via NVMe 
fails but you still can insert an SD card to recover.

Best regards

Heinrich


+       }
+
         bootcount_inc();

         /* Dump driver model states to aid analysis */
diff --git a/common/spl/spl_nvme.c b/common/spl/spl_nvme.c
index 2af63f1dc8..c8774d67ec 100644
--- a/common/spl/spl_nvme.c
+++ b/common/spl/spl_nvme.c
@@ -7,7 +7,6 @@

  #include <common.h>
  #include <spl.h>
-#include <init.h>
  #include <nvme.h>

  static int spl_nvme_load_image(struct spl_image_info *spl_image,
@@ -15,10 +14,6 @@ static int spl_nvme_load_image(struct spl_image_info 
*spl_image,
  {
         int ret;

-       ret = pci_init();
-       if (ret < 0)
-               return ret;
-
         ret = nvme_scan_namespace();
         if (ret < 0)
                 return ret;
--
2.40.1


Regards,
Simon