[PATCH v2 1/1] spl: initialize PCI before booting
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.schuchardt@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. */ + } + 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;
On Mon, Jul 24, 2023 at 10:18:41PM +0200, Heinrich Schuchardt 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.schuchardt@canonical.com
Reviewed-by: Tom Rini trini@konsulko.com
On Mon, 24 Jul 2023 at 14:19, Heinrich Schuchardt heinrich.schuchardt@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.schuchardt@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.
}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
On 25.07.23 16:51, Simon Glass wrote:
On Mon, 24 Jul 2023 at 14:19, Heinrich Schuchardt heinrich.schuchardt@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.schuchardt@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
On Tue, Jul 25, 2023 at 08:51:55AM -0600, Simon Glass wrote:
On Mon, 24 Jul 2023 at 14:19, Heinrich Schuchardt heinrich.schuchardt@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.schuchardt@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.
I think it's reasonable here to say that we failed to initialize PCI. It's still up to the specific loader to say it couldn't find a controller. This in turn will provide reasonable hints if someone has a problem. The flip side is "Oh, PCI failed but I was trying to boot from SD card anyhow which doesn't need it" (think bring-up) and having to go hack things up.
Hi Tom,
On Tue, 25 Jul 2023 at 10:37, Tom Rini trini@konsulko.com wrote:
On Tue, Jul 25, 2023 at 08:51:55AM -0600, Simon Glass wrote:
On Mon, 24 Jul 2023 at 14:19, Heinrich Schuchardt heinrich.schuchardt@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.schuchardt@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.
I think it's reasonable here to say that we failed to initialize PCI. It's still up to the specific loader to say it couldn't find a controller. This in turn will provide reasonable hints if someone has a problem. The flip side is "Oh, PCI failed but I was trying to boot from SD card anyhow which doesn't need it" (think bring-up) and having to go hack things up.
OK, so perhaps it should say 'warning' so it is clear that it might not be fatal?
I cannot imagine a failure to init PCI though. It seems pretty fatal to me.
Regards, Simon
On Tue, Jul 25, 2023 at 03:28:37PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 25 Jul 2023 at 10:37, Tom Rini trini@konsulko.com wrote:
On Tue, Jul 25, 2023 at 08:51:55AM -0600, Simon Glass wrote:
On Mon, 24 Jul 2023 at 14:19, Heinrich Schuchardt heinrich.schuchardt@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.schuchardt@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.
I think it's reasonable here to say that we failed to initialize PCI. It's still up to the specific loader to say it couldn't find a controller. This in turn will provide reasonable hints if someone has a problem. The flip side is "Oh, PCI failed but I was trying to boot from SD card anyhow which doesn't need it" (think bring-up) and having to go hack things up.
OK, so perhaps it should say 'warning' so it is clear that it might not be fatal?
That's more bytes and we're already in what should obviously be Not Great shape.
I cannot imagine a failure to init PCI though. It seems pretty fatal to me.
Working on PCI SPL support (NVMe, etc) with fall back to SD boot (so the board is easy to recover while working).
Hi Tom,
On Tue, 25 Jul 2023 at 15:39, Tom Rini trini@konsulko.com wrote:
On Tue, Jul 25, 2023 at 03:28:37PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 25 Jul 2023 at 10:37, Tom Rini trini@konsulko.com wrote:
On Tue, Jul 25, 2023 at 08:51:55AM -0600, Simon Glass wrote:
On Mon, 24 Jul 2023 at 14:19, Heinrich Schuchardt heinrich.schuchardt@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.schuchardt@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.
I think it's reasonable here to say that we failed to initialize PCI. It's still up to the specific loader to say it couldn't find a controller. This in turn will provide reasonable hints if someone has a problem. The flip side is "Oh, PCI failed but I was trying to boot from SD card anyhow which doesn't need it" (think bring-up) and having to go hack things up.
OK, so perhaps it should say 'warning' so it is clear that it might not be fatal?
That's more bytes and we're already in what should obviously be Not Great shape.
I cannot imagine a failure to init PCI though. It seems pretty fatal to me.
Working on PCI SPL support (NVMe, etc) with fall back to SD boot (so the board is easy to recover while working).
OK, I've said my piece.
Regards, Simon
On Mon, 2023-07-24 at 22:18 +0200, Heinrich Schuchardt 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.schuchardt@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. */}
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;
Reviewed-by: Mayuresh Chitale mchitale@ventanamicro.com
participants (4)
-
Heinrich Schuchardt -
mchitale＠ventanamicro.com -
Simon Glass -
Tom Rini