Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On Wed, Feb 21, 2024 at 11:57:51AM +0530, Sumit Garg wrote: > On Tue, 20 Feb 2024 at 21:02, Marek Vasut wrote: > > On 2/20/24 14:10, Sumit Garg wrote: > > > Also, enable reset driver which is a prerequisite for PCIe support. > > Commit message needs to be fixed. > > Let me reiterate the header here too. > > Enable PCIe/NVMe support. Also, enable the reset driver which > is a prerequisite for PCIe support. With the commit message improved .. On Wed, Feb 21, 2024 at 11:59:10AM +0530, Sumit Garg wrote: > On Tue, 20 Feb 2024 at 21:34, Fabio Estevam wrote: > > On Tue, Feb 20, 2024 at 10:51 AM Sumit Garg wrote: > > > diff --git a/configs/verdin-imx8mp_defconfig > > > b/configs/verdin-imx8mp_defconfig > > > index 22b8a334dfa..d8bd644322b 100644 > > > --- a/configs/verdin-imx8mp_defconfig > > > +++ b/configs/verdin-imx8mp_defconfig > > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > > > CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > > > CONFIG_IMX_WATCHDOG=y > > > CONFIG_HEXDUMP=y > > > +CONFIG_DM_RESET=y > > > +CONFIG_RESET_IMX=y > > > +CONFIG_PCI=y > > > +CONFIG_PCIE_DW_IMX8=y > > > +CONFIG_PHY_IMX8M_PCIE=y > > > +CONFIG_CMD_PCI=y > > > +CONFIG_NVME=y > > > +CONFIG_NVME_PCI=y > > > +CONFIG_CMD_NVME=y > > > > Please don't group all these new config options at the end of the file. > That sounds better, I will do that for v2. ... and this change, feel free to add in v2 Acked-by: Francesco Dolcini Francesco
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On Wed, 2024-02-21 at 15:31 +0100, Francesco Dolcini wrote: > On Wed, Feb 21, 2024 at 09:18:51AM +, Marcel Ziswiler wrote: > > On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote: > > > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: > > > > Also, enable reset driver which is a prerequisite for PCIe support. > > > > > > > > Signed-off-by: Sumit Garg > > > > --- > > > > configs/verdin-imx8mp_defconfig | 9 + > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/configs/verdin-imx8mp_defconfig > > > > b/configs/verdin-imx8mp_defconfig > > > > index 22b8a334dfa..d8bd644322b 100644 > > > > --- a/configs/verdin-imx8mp_defconfig > > > > +++ b/configs/verdin-imx8mp_defconfig > > > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > > > > CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > > > > CONFIG_IMX_WATCHDOG=y > > > > CONFIG_HEXDUMP=y > > > > +CONFIG_DM_RESET=y > > > > +CONFIG_RESET_IMX=y > > > > +CONFIG_PCI=y > > > > +CONFIG_PCIE_DW_IMX8=y > > > > +CONFIG_PHY_IMX8M_PCIE=y > > > > +CONFIG_CMD_PCI=y > > > > +CONFIG_NVME=y > > > > +CONFIG_NVME_PCI=y > > > > +CONFIG_CMD_NVME=y > > > > > > This will increase the u-boot proper size > > > > Yes, I checked and it is actually slightly more than 32 K. > > > > > and marginally increase the > > > boot time (because of a bigger binary to be read from the eMMC). > > > > That was also my concern. > > > > > Apart of that do you expect any other impact on those changes? SPL > > > binary size should not be affected, correct? > > > > > > Asking this out loudly to confirm that nothing unexpected is going to > > > happen because of these changes. > > > > Other than that I actually gave it a quick try and PCIe/NVMe does indeed > > work and the regular boot is not > > affected (other than the slight size and boot time increase, of course). > > > > > For my curiosity, care to share what's the use case? Do you plan to have > > > the OS stored into an NVME device? > > > > For us the question is basically whether that use case does mandate > > enforcing such changes for each and every customer. Plus the regular > > expected maintenance effort any such change brings with it, of course. > > Marcel, given all you wrote here I would personally be fine > on having this enabled in the verdin_imx8mp defconfig. > > What's your idea? Are you good with it? Yes, I do agree. > Francesco Cheers Marcel
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On Wed, Feb 21, 2024 at 09:18:51AM +, Marcel Ziswiler wrote: > On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote: > > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: > > > Also, enable reset driver which is a prerequisite for PCIe support. > > > > > > Signed-off-by: Sumit Garg > > > --- > > > configs/verdin-imx8mp_defconfig | 9 + > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/configs/verdin-imx8mp_defconfig > > > b/configs/verdin-imx8mp_defconfig > > > index 22b8a334dfa..d8bd644322b 100644 > > > --- a/configs/verdin-imx8mp_defconfig > > > +++ b/configs/verdin-imx8mp_defconfig > > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > > > CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > > > CONFIG_IMX_WATCHDOG=y > > > CONFIG_HEXDUMP=y > > > +CONFIG_DM_RESET=y > > > +CONFIG_RESET_IMX=y > > > +CONFIG_PCI=y > > > +CONFIG_PCIE_DW_IMX8=y > > > +CONFIG_PHY_IMX8M_PCIE=y > > > +CONFIG_CMD_PCI=y > > > +CONFIG_NVME=y > > > +CONFIG_NVME_PCI=y > > > +CONFIG_CMD_NVME=y > > > > This will increase the u-boot proper size > > Yes, I checked and it is actually slightly more than 32 K. > > > and marginally increase the > > boot time (because of a bigger binary to be read from the eMMC). > > That was also my concern. > > > Apart of that do you expect any other impact on those changes? SPL > > binary size should not be affected, correct? > > > > Asking this out loudly to confirm that nothing unexpected is going to > > happen because of these changes. > > Other than that I actually gave it a quick try and PCIe/NVMe does indeed work > and the regular boot is not > affected (other than the slight size and boot time increase, of course). > > > For my curiosity, care to share what's the use case? Do you plan to have > > the OS stored into an NVME device? > > For us the question is basically whether that use case does mandate > enforcing such changes for each and every customer. Plus the regular > expected maintenance effort any such change brings with it, of course. Marcel, given all you wrote here I would personally be fine on having this enabled in the verdin_imx8mp defconfig. What's your idea? Are you good with it? Francesco
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On 2/21/24 14:15, Adam Ford wrote: On Wed, Feb 21, 2024 at 6:27 AM Marek Vasut wrote: On 2/21/24 13:12, Sumit Garg wrote: On Wed, 21 Feb 2024 at 16:11, Marek Vasut wrote: On 2/21/24 10:18, Marcel Ziswiler wrote: Hi Sumit On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote: Hello Sumit, On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: Also, enable reset driver which is a prerequisite for PCIe support. Signed-off-by: Sumit Garg --- configs/verdin-imx8mp_defconfig | 9 + 1 file changed, 9 insertions(+) diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig index 22b8a334dfa..d8bd644322b 100644 --- a/configs/verdin-imx8mp_defconfig +++ b/configs/verdin-imx8mp_defconfig @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 CONFIG_IMX_WATCHDOG=y CONFIG_HEXDUMP=y +CONFIG_DM_RESET=y +CONFIG_RESET_IMX=y +CONFIG_PCI=y +CONFIG_PCIE_DW_IMX8=y +CONFIG_PHY_IMX8M_PCIE=y +CONFIG_CMD_PCI=y +CONFIG_NVME=y +CONFIG_NVME_PCI=y +CONFIG_CMD_NVME=y This will increase the u-boot proper size Yes, I checked and it is actually slightly more than 32 K. and marginally increase the boot time (because of a bigger binary to be read from the eMMC). That was also my concern. Apart of that do you expect any other impact on those changes? SPL binary size should not be affected, correct? Asking this out loudly to confirm that nothing unexpected is going to happen because of these changes. Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not affected (other than the slight size and boot time increase, of course). For my curiosity, care to share what's the use case? Do you plan to have the OS stored into an NVME device? For us the question is basically whether that use case does mandate enforcing such changes for each and every customer. Plus the regular expected maintenance effort any such change brings with it, of course. You can always enable this support on MX8MP EVK, it has M2 slot and this would add build coverage of this code too, without impacting Verdin. I would have chosen that as the base platform to enable but unfortunately I don't have that at my desk. However, if someone is willing to test this patch-set on MX8MP EVK then I am happy to extend corresponding defconfig too. I think we can surely find a platform on which this can be enabled by default and tested by people. My development machine crashed (my fault for running a pre-release OS),but I have an i.MX8M Plus and an NVMe drive that I can test once I get my machine running again. I can test right away, although I'll wait for V2 with all the fixes.
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On Wed, Feb 21, 2024 at 6:27 AM Marek Vasut wrote: > > On 2/21/24 13:12, Sumit Garg wrote: > > On Wed, 21 Feb 2024 at 16:11, Marek Vasut wrote: > >> > >> On 2/21/24 10:18, Marcel Ziswiler wrote: > >>> Hi Sumit > >>> > >>> On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote: > Hello Sumit, > > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: > > Also, enable reset driver which is a prerequisite for PCIe support. > > > > Signed-off-by: Sumit Garg > > --- > >configs/verdin-imx8mp_defconfig | 9 + > >1 file changed, 9 insertions(+) > > > > diff --git a/configs/verdin-imx8mp_defconfig > > b/configs/verdin-imx8mp_defconfig > > index 22b8a334dfa..d8bd644322b 100644 > > --- a/configs/verdin-imx8mp_defconfig > > +++ b/configs/verdin-imx8mp_defconfig > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > >CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > >CONFIG_IMX_WATCHDOG=y > >CONFIG_HEXDUMP=y > > +CONFIG_DM_RESET=y > > +CONFIG_RESET_IMX=y > > +CONFIG_PCI=y > > +CONFIG_PCIE_DW_IMX8=y > > +CONFIG_PHY_IMX8M_PCIE=y > > +CONFIG_CMD_PCI=y > > +CONFIG_NVME=y > > +CONFIG_NVME_PCI=y > > +CONFIG_CMD_NVME=y > > This will increase the u-boot proper size > >>> > >>> Yes, I checked and it is actually slightly more than 32 K. > >>> > and marginally increase the > boot time (because of a bigger binary to be read from the eMMC). > >>> > >>> That was also my concern. > >>> > Apart of that do you expect any other impact on those changes? SPL > binary size should not be affected, correct? > > Asking this out loudly to confirm that nothing unexpected is going to > happen because of these changes. > >>> > >>> Other than that I actually gave it a quick try and PCIe/NVMe does indeed > >>> work and the regular boot is not > >>> affected (other than the slight size and boot time increase, of course). > >>> > For my curiosity, care to share what's the use case? Do you plan to have > the OS stored into an NVME device? > >>> > >>> For us the question is basically whether that use case does mandate > >>> enforcing such changes for each and every > >>> customer. Plus the regular expected maintenance effort any such change > >>> brings with it, of course. > >> > >> You can always enable this support on MX8MP EVK, it has M2 slot and this > >> would add build coverage of this code too, without impacting Verdin. > > > > I would have chosen that as the base platform to enable but > > unfortunately I don't have that at my desk. However, if someone is > > willing to test this patch-set on MX8MP EVK then I am happy to extend > > corresponding defconfig too. > > I think we can surely find a platform on which this can be enabled by > default and tested by people. My development machine crashed (my fault for running a pre-release OS),but I have an i.MX8M Plus and an NVMe drive that I can test once I get my machine running again. adam
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On 2/21/24 13:12, Sumit Garg wrote: On Wed, 21 Feb 2024 at 16:11, Marek Vasut wrote: On 2/21/24 10:18, Marcel Ziswiler wrote: Hi Sumit On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote: Hello Sumit, On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: Also, enable reset driver which is a prerequisite for PCIe support. Signed-off-by: Sumit Garg --- configs/verdin-imx8mp_defconfig | 9 + 1 file changed, 9 insertions(+) diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig index 22b8a334dfa..d8bd644322b 100644 --- a/configs/verdin-imx8mp_defconfig +++ b/configs/verdin-imx8mp_defconfig @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 CONFIG_IMX_WATCHDOG=y CONFIG_HEXDUMP=y +CONFIG_DM_RESET=y +CONFIG_RESET_IMX=y +CONFIG_PCI=y +CONFIG_PCIE_DW_IMX8=y +CONFIG_PHY_IMX8M_PCIE=y +CONFIG_CMD_PCI=y +CONFIG_NVME=y +CONFIG_NVME_PCI=y +CONFIG_CMD_NVME=y This will increase the u-boot proper size Yes, I checked and it is actually slightly more than 32 K. and marginally increase the boot time (because of a bigger binary to be read from the eMMC). That was also my concern. Apart of that do you expect any other impact on those changes? SPL binary size should not be affected, correct? Asking this out loudly to confirm that nothing unexpected is going to happen because of these changes. Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not affected (other than the slight size and boot time increase, of course). For my curiosity, care to share what's the use case? Do you plan to have the OS stored into an NVME device? For us the question is basically whether that use case does mandate enforcing such changes for each and every customer. Plus the regular expected maintenance effort any such change brings with it, of course. You can always enable this support on MX8MP EVK, it has M2 slot and this would add build coverage of this code too, without impacting Verdin. I would have chosen that as the base platform to enable but unfortunately I don't have that at my desk. However, if someone is willing to test this patch-set on MX8MP EVK then I am happy to extend corresponding defconfig too. I think we can surely find a platform on which this can be enabled by default and tested by people.
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On Wed, 21 Feb 2024 at 16:11, Marek Vasut wrote: > > On 2/21/24 08:55, Francesco Dolcini wrote: > > Hello Sumit, > > > > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: > >> Also, enable reset driver which is a prerequisite for PCIe support. > >> > >> Signed-off-by: Sumit Garg > >> --- > >> configs/verdin-imx8mp_defconfig | 9 + > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/configs/verdin-imx8mp_defconfig > >> b/configs/verdin-imx8mp_defconfig > >> index 22b8a334dfa..d8bd644322b 100644 > >> --- a/configs/verdin-imx8mp_defconfig > >> +++ b/configs/verdin-imx8mp_defconfig > >> @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > >> CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > >> CONFIG_IMX_WATCHDOG=y > >> CONFIG_HEXDUMP=y > >> +CONFIG_DM_RESET=y > >> +CONFIG_RESET_IMX=y > >> +CONFIG_PCI=y > >> +CONFIG_PCIE_DW_IMX8=y > >> +CONFIG_PHY_IMX8M_PCIE=y > >> +CONFIG_CMD_PCI=y > >> +CONFIG_NVME=y > >> +CONFIG_NVME_PCI=y > >> +CONFIG_CMD_NVME=y > > > > This will increase the u-boot proper size and marginally increase the > > boot time (because of a bigger binary to be read from the eMMC). > > > > Apart of that do you expect any other impact on those changes? SPL > > binary size should not be affected, correct? > > > > Asking this out loudly to confirm that nothing unexpected is going to > > happen because of these changes. > > > > For my curiosity, care to share what's the use case? Do you plan to have > > the OS stored into an NVME device? > > Boot OS images from NVMe is useful if eMMC size no longer cuts it > (because debug symbols or to collect logs over long periods of time). Yeah especially if one wants to run a fully fledged Linux distro image. -Sumit
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On Wed, 21 Feb 2024 at 16:11, Marek Vasut wrote: > > On 2/21/24 10:18, Marcel Ziswiler wrote: > > Hi Sumit > > > > On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote: > >> Hello Sumit, > >> > >> On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: > >>> Also, enable reset driver which is a prerequisite for PCIe support. > >>> > >>> Signed-off-by: Sumit Garg > >>> --- > >>> configs/verdin-imx8mp_defconfig | 9 + > >>> 1 file changed, 9 insertions(+) > >>> > >>> diff --git a/configs/verdin-imx8mp_defconfig > >>> b/configs/verdin-imx8mp_defconfig > >>> index 22b8a334dfa..d8bd644322b 100644 > >>> --- a/configs/verdin-imx8mp_defconfig > >>> +++ b/configs/verdin-imx8mp_defconfig > >>> @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > >>> CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > >>> CONFIG_IMX_WATCHDOG=y > >>> CONFIG_HEXDUMP=y > >>> +CONFIG_DM_RESET=y > >>> +CONFIG_RESET_IMX=y > >>> +CONFIG_PCI=y > >>> +CONFIG_PCIE_DW_IMX8=y > >>> +CONFIG_PHY_IMX8M_PCIE=y > >>> +CONFIG_CMD_PCI=y > >>> +CONFIG_NVME=y > >>> +CONFIG_NVME_PCI=y > >>> +CONFIG_CMD_NVME=y > >> > >> This will increase the u-boot proper size > > > > Yes, I checked and it is actually slightly more than 32 K. > > > >> and marginally increase the > >> boot time (because of a bigger binary to be read from the eMMC). > > > > That was also my concern. > > > >> Apart of that do you expect any other impact on those changes? SPL > >> binary size should not be affected, correct? > >> > >> Asking this out loudly to confirm that nothing unexpected is going to > >> happen because of these changes. > > > > Other than that I actually gave it a quick try and PCIe/NVMe does indeed > > work and the regular boot is not > > affected (other than the slight size and boot time increase, of course). > > > >> For my curiosity, care to share what's the use case? Do you plan to have > >> the OS stored into an NVME device? > > > > For us the question is basically whether that use case does mandate > > enforcing such changes for each and every > > customer. Plus the regular expected maintenance effort any such change > > brings with it, of course. > > You can always enable this support on MX8MP EVK, it has M2 slot and this > would add build coverage of this code too, without impacting Verdin. I would have chosen that as the base platform to enable but unfortunately I don't have that at my desk. However, if someone is willing to test this patch-set on MX8MP EVK then I am happy to extend corresponding defconfig too. -Sumit
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
Hi, On Wed, 21 Feb 2024 at 10:19, Marcel Ziswiler wrote: > > Hi Sumit > > On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote: > > Hello Sumit, > > > > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: > > > Also, enable reset driver which is a prerequisite for PCIe support. > > > > > > Signed-off-by: Sumit Garg > > > --- > > > configs/verdin-imx8mp_defconfig | 9 + > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/configs/verdin-imx8mp_defconfig > > > b/configs/verdin-imx8mp_defconfig > > > index 22b8a334dfa..d8bd644322b 100644 > > > --- a/configs/verdin-imx8mp_defconfig > > > +++ b/configs/verdin-imx8mp_defconfig > > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > > > CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > > > CONFIG_IMX_WATCHDOG=y > > > CONFIG_HEXDUMP=y > > > +CONFIG_DM_RESET=y > > > +CONFIG_RESET_IMX=y > > > +CONFIG_PCI=y > > > +CONFIG_PCIE_DW_IMX8=y > > > +CONFIG_PHY_IMX8M_PCIE=y > > > +CONFIG_CMD_PCI=y > > > +CONFIG_NVME=y > > > +CONFIG_NVME_PCI=y > > > +CONFIG_CMD_NVME=y > > > > This will increase the u-boot proper size > > Yes, I checked and it is actually slightly more than 32 K. > > > and marginally increase the > > boot time (because of a bigger binary to be read from the eMMC). > > That was also my concern. > > > Apart of that do you expect any other impact on those changes? SPL > > binary size should not be affected, correct? > > > > Asking this out loudly to confirm that nothing unexpected is going to > > happen because of these changes. > > Other than that I actually gave it a quick try and PCIe/NVMe does indeed work > and the regular boot is not > affected (other than the slight size and boot time increase, of course). > > > For my curiosity, care to share what's the use case? Do you plan to have > > the OS stored into an NVME device? > > For us the question is basically whether that use case does mandate enforcing > such changes for each and every > customer. Plus the regular expected maintenance effort any such change brings > with it, of course. @Francesco that's correct, we have the OS stored on NVME SSD. @Marcel wrt enforcing it, we can always fallback to have a config fragment for our customers builds. That's already the case, we fine tune the config and don't use U-Boot defconfig out of the box for shipping to end users. > > Francesco > > Cheers > > Marcel Cheers, -- Fathi Boudra
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On 2/21/24 10:18, Marcel Ziswiler wrote: Hi Sumit On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote: Hello Sumit, On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: Also, enable reset driver which is a prerequisite for PCIe support. Signed-off-by: Sumit Garg --- configs/verdin-imx8mp_defconfig | 9 + 1 file changed, 9 insertions(+) diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig index 22b8a334dfa..d8bd644322b 100644 --- a/configs/verdin-imx8mp_defconfig +++ b/configs/verdin-imx8mp_defconfig @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 CONFIG_IMX_WATCHDOG=y CONFIG_HEXDUMP=y +CONFIG_DM_RESET=y +CONFIG_RESET_IMX=y +CONFIG_PCI=y +CONFIG_PCIE_DW_IMX8=y +CONFIG_PHY_IMX8M_PCIE=y +CONFIG_CMD_PCI=y +CONFIG_NVME=y +CONFIG_NVME_PCI=y +CONFIG_CMD_NVME=y This will increase the u-boot proper size Yes, I checked and it is actually slightly more than 32 K. and marginally increase the boot time (because of a bigger binary to be read from the eMMC). That was also my concern. Apart of that do you expect any other impact on those changes? SPL binary size should not be affected, correct? Asking this out loudly to confirm that nothing unexpected is going to happen because of these changes. Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not affected (other than the slight size and boot time increase, of course). For my curiosity, care to share what's the use case? Do you plan to have the OS stored into an NVME device? For us the question is basically whether that use case does mandate enforcing such changes for each and every customer. Plus the regular expected maintenance effort any such change brings with it, of course. You can always enable this support on MX8MP EVK, it has M2 slot and this would add build coverage of this code too, without impacting Verdin.
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On 2/21/24 08:55, Francesco Dolcini wrote: Hello Sumit, On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: Also, enable reset driver which is a prerequisite for PCIe support. Signed-off-by: Sumit Garg --- configs/verdin-imx8mp_defconfig | 9 + 1 file changed, 9 insertions(+) diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig index 22b8a334dfa..d8bd644322b 100644 --- a/configs/verdin-imx8mp_defconfig +++ b/configs/verdin-imx8mp_defconfig @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 CONFIG_IMX_WATCHDOG=y CONFIG_HEXDUMP=y +CONFIG_DM_RESET=y +CONFIG_RESET_IMX=y +CONFIG_PCI=y +CONFIG_PCIE_DW_IMX8=y +CONFIG_PHY_IMX8M_PCIE=y +CONFIG_CMD_PCI=y +CONFIG_NVME=y +CONFIG_NVME_PCI=y +CONFIG_CMD_NVME=y This will increase the u-boot proper size and marginally increase the boot time (because of a bigger binary to be read from the eMMC). Apart of that do you expect any other impact on those changes? SPL binary size should not be affected, correct? Asking this out loudly to confirm that nothing unexpected is going to happen because of these changes. For my curiosity, care to share what's the use case? Do you plan to have the OS stored into an NVME device? Boot OS images from NVMe is useful if eMMC size no longer cuts it (because debug symbols or to collect logs over long periods of time).
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On Wed, 21 Feb 2024 at 15:06, Fathi Boudra wrote: > > Hi, > > On Wed, 21 Feb 2024 at 10:19, Marcel Ziswiler > wrote: > > > > Hi Sumit > > > > On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote: > > > Hello Sumit, > > > > > > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: > > > > Also, enable reset driver which is a prerequisite for PCIe support. > > > > > > > > Signed-off-by: Sumit Garg > > > > --- > > > > configs/verdin-imx8mp_defconfig | 9 + > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/configs/verdin-imx8mp_defconfig > > > > b/configs/verdin-imx8mp_defconfig > > > > index 22b8a334dfa..d8bd644322b 100644 > > > > --- a/configs/verdin-imx8mp_defconfig > > > > +++ b/configs/verdin-imx8mp_defconfig > > > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > > > > CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > > > > CONFIG_IMX_WATCHDOG=y > > > > CONFIG_HEXDUMP=y > > > > +CONFIG_DM_RESET=y > > > > +CONFIG_RESET_IMX=y > > > > +CONFIG_PCI=y > > > > +CONFIG_PCIE_DW_IMX8=y > > > > +CONFIG_PHY_IMX8M_PCIE=y > > > > +CONFIG_CMD_PCI=y > > > > +CONFIG_NVME=y > > > > +CONFIG_NVME_PCI=y > > > > +CONFIG_CMD_NVME=y > > > > > > This will increase the u-boot proper size > > > > Yes, I checked and it is actually slightly more than 32 K. > > > > > and marginally increase the > > > boot time (because of a bigger binary to be read from the eMMC). > > > > That was also my concern. > > > > > Apart of that do you expect any other impact on those changes? SPL > > > binary size should not be affected, correct? > > > > > > Asking this out loudly to confirm that nothing unexpected is going to > > > happen because of these changes. > > > > Other than that I actually gave it a quick try and PCIe/NVMe does indeed > > work and the regular boot is not > > affected (other than the slight size and boot time increase, of course). Thanks for taking the time to test it. I suppose I can take that as a Tested-by tag. > > > > > For my curiosity, care to share what's the use case? Do you plan to have > > > the OS stored into an NVME device? > > > > For us the question is basically whether that use case does mandate > > enforcing such changes for each and every > > customer. > > Plus the regular expected maintenance effort any such change brings with > > it, of course. >From a PCIe maintenance point of view, I can add myself as a maintainer for drivers added by this patch-set. > > @Francesco that's correct, we have the OS stored on NVME SSD. > @Marcel wrt enforcing it, we can always fallback to have a config > fragment for our customers builds. > That's already the case, we fine tune the config and don't use U-Boot > defconfig out of the box for shipping to end users. > Marcel, Francesco, If you folks still have worries about updating the defconfig then I can make it as a config fragment for mainline too. I suppose that should work for other boards too based on the iMX8MP SoC. -Sumit > > > Francesco > > > > Cheers > > > > Marcel > > Cheers, > -- > Fathi Boudra
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
Hi Sumit On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote: > Hello Sumit, > > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: > > Also, enable reset driver which is a prerequisite for PCIe support. > > > > Signed-off-by: Sumit Garg > > --- > > configs/verdin-imx8mp_defconfig | 9 + > > 1 file changed, 9 insertions(+) > > > > diff --git a/configs/verdin-imx8mp_defconfig > > b/configs/verdin-imx8mp_defconfig > > index 22b8a334dfa..d8bd644322b 100644 > > --- a/configs/verdin-imx8mp_defconfig > > +++ b/configs/verdin-imx8mp_defconfig > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > > CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > > CONFIG_IMX_WATCHDOG=y > > CONFIG_HEXDUMP=y > > +CONFIG_DM_RESET=y > > +CONFIG_RESET_IMX=y > > +CONFIG_PCI=y > > +CONFIG_PCIE_DW_IMX8=y > > +CONFIG_PHY_IMX8M_PCIE=y > > +CONFIG_CMD_PCI=y > > +CONFIG_NVME=y > > +CONFIG_NVME_PCI=y > > +CONFIG_CMD_NVME=y > > This will increase the u-boot proper size Yes, I checked and it is actually slightly more than 32 K. > and marginally increase the > boot time (because of a bigger binary to be read from the eMMC). That was also my concern. > Apart of that do you expect any other impact on those changes? SPL > binary size should not be affected, correct? > > Asking this out loudly to confirm that nothing unexpected is going to > happen because of these changes. Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not affected (other than the slight size and boot time increase, of course). > For my curiosity, care to share what's the use case? Do you plan to have > the OS stored into an NVME device? For us the question is basically whether that use case does mandate enforcing such changes for each and every customer. Plus the regular expected maintenance effort any such change brings with it, of course. > Francesco Cheers Marcel
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
Hello Sumit, On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote: > Also, enable reset driver which is a prerequisite for PCIe support. > > Signed-off-by: Sumit Garg > --- > configs/verdin-imx8mp_defconfig | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig > index 22b8a334dfa..d8bd644322b 100644 > --- a/configs/verdin-imx8mp_defconfig > +++ b/configs/verdin-imx8mp_defconfig > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > CONFIG_IMX_WATCHDOG=y > CONFIG_HEXDUMP=y > +CONFIG_DM_RESET=y > +CONFIG_RESET_IMX=y > +CONFIG_PCI=y > +CONFIG_PCIE_DW_IMX8=y > +CONFIG_PHY_IMX8M_PCIE=y > +CONFIG_CMD_PCI=y > +CONFIG_NVME=y > +CONFIG_NVME_PCI=y > +CONFIG_CMD_NVME=y This will increase the u-boot proper size and marginally increase the boot time (because of a bigger binary to be read from the eMMC). Apart of that do you expect any other impact on those changes? SPL binary size should not be affected, correct? Asking this out loudly to confirm that nothing unexpected is going to happen because of these changes. For my curiosity, care to share what's the use case? Do you plan to have the OS stored into an NVME device? Francesco
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On Tue, 20 Feb 2024 at 21:34, Fabio Estevam wrote: > > On Tue, Feb 20, 2024 at 10:51 AM Sumit Garg wrote: > > > > Also, enable reset driver which is a prerequisite for PCIe support. > > > > Signed-off-by: Sumit Garg > > --- > > configs/verdin-imx8mp_defconfig | 9 + > > 1 file changed, 9 insertions(+) > > > > diff --git a/configs/verdin-imx8mp_defconfig > > b/configs/verdin-imx8mp_defconfig > > index 22b8a334dfa..d8bd644322b 100644 > > --- a/configs/verdin-imx8mp_defconfig > > +++ b/configs/verdin-imx8mp_defconfig > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > > CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > > CONFIG_IMX_WATCHDOG=y > > CONFIG_HEXDUMP=y > > +CONFIG_DM_RESET=y > > +CONFIG_RESET_IMX=y > > +CONFIG_PCI=y > > +CONFIG_PCIE_DW_IMX8=y > > +CONFIG_PHY_IMX8M_PCIE=y > > +CONFIG_CMD_PCI=y > > +CONFIG_NVME=y > > +CONFIG_NVME_PCI=y > > +CONFIG_CMD_NVME=y > > Please don't group all these new config options at the end of the file. > > Use 'make savedefconfig' and then 'cp defconfig > configs/verdin-imx8mp_defconfig' to properly > add these new config options. That sounds better, I will do that for v2. -Sumit
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On Tue, 20 Feb 2024 at 21:02, Marek Vasut wrote: > > On 2/20/24 14:10, Sumit Garg wrote: > > Also, enable reset driver which is a prerequisite for PCIe support. > > Commit message needs to be fixed. Let me reiterate the header here too. Enable PCIe/NVMe support. Also, enable the reset driver which is a prerequisite for PCIe support. -Sumit
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On Tue, Feb 20, 2024 at 10:51 AM Sumit Garg wrote: > > Also, enable reset driver which is a prerequisite for PCIe support. > > Signed-off-by: Sumit Garg > --- > configs/verdin-imx8mp_defconfig | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig > index 22b8a334dfa..d8bd644322b 100644 > --- a/configs/verdin-imx8mp_defconfig > +++ b/configs/verdin-imx8mp_defconfig > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67 > CONFIG_USB_GADGET_PRODUCT_NUM=0x4000 > CONFIG_IMX_WATCHDOG=y > CONFIG_HEXDUMP=y > +CONFIG_DM_RESET=y > +CONFIG_RESET_IMX=y > +CONFIG_PCI=y > +CONFIG_PCIE_DW_IMX8=y > +CONFIG_PHY_IMX8M_PCIE=y > +CONFIG_CMD_PCI=y > +CONFIG_NVME=y > +CONFIG_NVME_PCI=y > +CONFIG_CMD_NVME=y Please don't group all these new config options at the end of the file. Use 'make savedefconfig' and then 'cp defconfig configs/verdin-imx8mp_defconfig' to properly add these new config options.
Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support
On 2/20/24 14:10, Sumit Garg wrote: Also, enable reset driver which is a prerequisite for PCIe support. Commit message needs to be fixed.
