Re: [PATCH v2 1/1] spl: initialize PCI before booting
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 > > --- > 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 > #include > -#include > #include > > 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
Re: [PATCH v2 1/1] spl: initialize PCI before booting
Hi Tom, On Tue, 25 Jul 2023 at 15:39, Tom Rini 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 wrote: > > > > > > On Tue, Jul 25, 2023 at 08:51:55AM -0600, Simon Glass wrote: > > > > On Mon, 24 Jul 2023 at 14:19, 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 > > > > > --- > > > > > 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
Re: [PATCH v2 1/1] spl: initialize PCI before booting
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 wrote: > > > > On Tue, Jul 25, 2023 at 08:51:55AM -0600, Simon Glass wrote: > > > On Mon, 24 Jul 2023 at 14:19, 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 > > > > --- > > > > 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). -- Tom signature.asc Description: PGP signature
Re: [PATCH v2 1/1] spl: initialize PCI before booting
Hi Tom, On Tue, 25 Jul 2023 at 10:37, Tom Rini wrote: > > On Tue, Jul 25, 2023 at 08:51:55AM -0600, Simon Glass wrote: > > On Mon, 24 Jul 2023 at 14:19, 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 > > > --- > > > 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
Re: [PATCH v2 1/1] spl: initialize PCI before booting
On Tue, Jul 25, 2023 at 08:51:55AM -0600, Simon Glass wrote: > On Mon, 24 Jul 2023 at 14:19, 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 > > --- > > 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. -- Tom signature.asc Description: PGP signature
Re: [PATCH v2 1/1] spl: initialize PCI before booting
On 25.07.23 16:51, Simon Glass wrote: On Mon, 24 Jul 2023 at 14:19, 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 --- 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 #include -#include #include 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
Re: [PATCH v2 1/1] spl: initialize PCI before booting
On Mon, 24 Jul 2023 at 14:19, 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 > --- > 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 > #include > -#include > #include > > 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
Re: [PATCH v2 1/1] spl: initialize PCI before booting
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 Reviewed-by: Tom Rini -- Tom signature.asc Description: PGP signature
[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 --- 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 #include -#include #include 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
Re: [PATCH v2 1/1] spl: initialize PCI before booting from MMC
On Mon, Jul 24, 2023 at 07:50:02PM +0200, Heinrich Schuchardt wrote: > On 24.07.23 17:12, Tom Rini wrote: > > On Mon, Jul 24, 2023 at 12:33:08PM +0200, Heinrich Schuchardt wrote: > > > Some MMC controllers are PCI bus devices. Before calling spl_mmc_load() we > > > must bind the PCI devices. > > > > > > Signed-off-by: Heinrich Schuchardt > > > Reviewed-by: Stefan Roese > > > --- > > > v2: > > > Remove empty line at start of code block. > > > Add empty line before return. > > > --- > > > common/spl/spl_mmc.c | 4 > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c > > > index a665091b00..eb95005769 100644 > > > --- a/common/spl/spl_mmc.c > > > +++ b/common/spl/spl_mmc.c > > > @@ -7,6 +7,7 @@ > > >*/ > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -495,6 +496,9 @@ int spl_mmc_load(struct spl_image_info *spl_image, > > > int spl_mmc_load_image(struct spl_image_info *spl_image, > > > struct spl_boot_device *bootdev) > > > { > > > + if (IS_ENABLED(CONFIG_SPL_PCI)) > > > + pci_init(); > > > + > > > return spl_mmc_load(spl_image, bootdev, > > > #ifdef CONFIG_SPL_FS_LOAD_PAYLOAD_NAME > > > CONFIG_SPL_FS_LOAD_PAYLOAD_NAME, > > > > As you've posted a number of these patches now, is there a common place > > we can call pci_init() and check the return value? > > > > We could move this to board_init_r() in common/spl/spl.c. Would that be your > preference? Yes, thanks. -- Tom signature.asc Description: PGP signature
Re: [PATCH v2 1/1] spl: initialize PCI before booting from MMC
On 24.07.23 17:12, Tom Rini wrote: On Mon, Jul 24, 2023 at 12:33:08PM +0200, Heinrich Schuchardt wrote: Some MMC controllers are PCI bus devices. Before calling spl_mmc_load() we must bind the PCI devices. Signed-off-by: Heinrich Schuchardt Reviewed-by: Stefan Roese --- v2: Remove empty line at start of code block. Add empty line before return. --- common/spl/spl_mmc.c | 4 1 file changed, 4 insertions(+) diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index a665091b00..eb95005769 100644 --- a/common/spl/spl_mmc.c +++ b/common/spl/spl_mmc.c @@ -7,6 +7,7 @@ */ #include #include +#include #include #include #include @@ -495,6 +496,9 @@ int spl_mmc_load(struct spl_image_info *spl_image, int spl_mmc_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) { + if (IS_ENABLED(CONFIG_SPL_PCI)) + pci_init(); + return spl_mmc_load(spl_image, bootdev, #ifdef CONFIG_SPL_FS_LOAD_PAYLOAD_NAME CONFIG_SPL_FS_LOAD_PAYLOAD_NAME, As you've posted a number of these patches now, is there a common place we can call pci_init() and check the return value? We could move this to board_init_r() in common/spl/spl.c. Would that be your preference? Best regards Heinrich
Re: [PATCH v2 1/1] spl: initialize PCI before booting from MMC
On Mon, Jul 24, 2023 at 12:33:08PM +0200, Heinrich Schuchardt wrote: > Some MMC controllers are PCI bus devices. Before calling spl_mmc_load() we > must bind the PCI devices. > > Signed-off-by: Heinrich Schuchardt > Reviewed-by: Stefan Roese > --- > v2: > Remove empty line at start of code block. > Add empty line before return. > --- > common/spl/spl_mmc.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c > index a665091b00..eb95005769 100644 > --- a/common/spl/spl_mmc.c > +++ b/common/spl/spl_mmc.c > @@ -7,6 +7,7 @@ > */ > #include > #include > +#include > #include > #include > #include > @@ -495,6 +496,9 @@ int spl_mmc_load(struct spl_image_info *spl_image, > int spl_mmc_load_image(struct spl_image_info *spl_image, > struct spl_boot_device *bootdev) > { > + if (IS_ENABLED(CONFIG_SPL_PCI)) > + pci_init(); > + > return spl_mmc_load(spl_image, bootdev, > #ifdef CONFIG_SPL_FS_LOAD_PAYLOAD_NAME > CONFIG_SPL_FS_LOAD_PAYLOAD_NAME, As you've posted a number of these patches now, is there a common place we can call pci_init() and check the return value? -- Tom signature.asc Description: PGP signature
[PATCH v2 1/1] spl: initialize PCI before booting from MMC
Some MMC controllers are PCI bus devices. Before calling spl_mmc_load() we must bind the PCI devices. Signed-off-by: Heinrich Schuchardt Reviewed-by: Stefan Roese --- v2: Remove empty line at start of code block. Add empty line before return. --- common/spl/spl_mmc.c | 4 1 file changed, 4 insertions(+) diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index a665091b00..eb95005769 100644 --- a/common/spl/spl_mmc.c +++ b/common/spl/spl_mmc.c @@ -7,6 +7,7 @@ */ #include #include +#include #include #include #include @@ -495,6 +496,9 @@ int spl_mmc_load(struct spl_image_info *spl_image, int spl_mmc_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) { + if (IS_ENABLED(CONFIG_SPL_PCI)) + pci_init(); + return spl_mmc_load(spl_image, bootdev, #ifdef CONFIG_SPL_FS_LOAD_PAYLOAD_NAME CONFIG_SPL_FS_LOAD_PAYLOAD_NAME, -- 2.40.1