Re: [u-boot][PATCH v2 2/4] scripts: Makefile.spl: Enable memory drivers to be built for SPL
On Tue, Oct 25, 2022 at 05:35:29PM -0600, Simon Glass wrote: > Hi Tom, > > On Thu, 20 Oct 2022 at 06:29, Tom Rini wrote: > > > > On Thu, Oct 20, 2022 at 03:23:42PM +0300, Roger Quadros wrote: > > > Hi Tom, > > > > > > On 19/10/2022 15:54, Tom Rini wrote: > > > > On Wed, Oct 19, 2022 at 11:17:35AM +0300, Roger Quadros wrote: > > > >> > > > >> > > > >> On 18/10/2022 20:40, Tom Rini wrote: > > > >>> On Thu, Oct 06, 2022 at 04:23:58PM +0300, Roger Quadros wrote: > > > We will need ti-gpmc driver for SPL. Allow memory drivers > > > do be built for SPL. > > > > > > Signed-off-by: Roger Quadros > > > --- > > > scripts/Makefile.spl | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl > > > index 3bafeb4fe9..110076b22f 100644 > > > --- a/scripts/Makefile.spl > > > +++ b/scripts/Makefile.spl > > > @@ -114,6 +114,7 @@ libs-$(CONFIG_PARTITIONS) += disk/ > > > endif > > > > > > libs-y += drivers/ > > > +libs-y += drivers/memory/ > > > libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/ > > > libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/ > > > libs-y += dts/ > > > >>> > > > >>> This ends up being the wrong approach as it then pulls in > > > >>> drivers/memory/stm32-fmc2-ebi.o on all of those platforms, in SPL, > > > >>> which > > > >>> is not what's intended. We need an SPL_MEMORY symbol and then gate the > > > >>> directory on that. > > > >>> > > > > > > I have a question about how CONFIG_SPL_MEMORY works together with > > > CONFIG_MEMORY. > > > > > > Do we use CONFIG_SPL_MEMORY only to gate the drivers/memory directory > > > inclusion? > > > Then continue to use CONFIG_MEMORY and others to enable/disable driver > > > build for both non-SPL and SPL case? > > > > > > So drivers/memory/Makefile remains as it is? > > > > Well, for consistency code should use IS_ENABLED(MEMORY) which will be > > true for CONFIG_MEMORY or CONFIG_SPL_MEMORY. > > nit: CONFIG_IS_ENABLED(MEMORY) > > Perhaps we can look at my Kconfig series so it can become > CONFIG(MEMORY) and we can drop all the SPL_TPL_ stuff in Makefiles? I think that was the part I had some reservations about, but we should re-visit that idea as I wasn't disagreeing (I believe) with making the logic easier, but I think that we need to re-evaluate how we manage the SPL/TPL/full U-Boot idea. -- Tom signature.asc Description: PGP signature
Re: [u-boot][PATCH v2 2/4] scripts: Makefile.spl: Enable memory drivers to be built for SPL
On Wed, Oct 26, 2022 at 01:41:12PM +0300, Roger Quadros wrote: > Tom, > > On 26/10/2022 02:35, Simon Glass wrote: > > Hi Tom, > > > > On Thu, 20 Oct 2022 at 06:29, Tom Rini wrote: > >> > >> On Thu, Oct 20, 2022 at 03:23:42PM +0300, Roger Quadros wrote: > >>> Hi Tom, > >>> > >>> On 19/10/2022 15:54, Tom Rini wrote: > On Wed, Oct 19, 2022 at 11:17:35AM +0300, Roger Quadros wrote: > > > > > > On 18/10/2022 20:40, Tom Rini wrote: > >> On Thu, Oct 06, 2022 at 04:23:58PM +0300, Roger Quadros wrote: > >>> We will need ti-gpmc driver for SPL. Allow memory drivers > >>> do be built for SPL. > >>> > >>> Signed-off-by: Roger Quadros > >>> --- > >>> scripts/Makefile.spl | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl > >>> index 3bafeb4fe9..110076b22f 100644 > >>> --- a/scripts/Makefile.spl > >>> +++ b/scripts/Makefile.spl > >>> @@ -114,6 +114,7 @@ libs-$(CONFIG_PARTITIONS) += disk/ > >>> endif > >>> > >>> libs-y += drivers/ > >>> +libs-y += drivers/memory/ > >>> libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/ > >>> libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/ > >>> libs-y += dts/ > >> > >> This ends up being the wrong approach as it then pulls in > >> drivers/memory/stm32-fmc2-ebi.o on all of those platforms, in SPL, > >> which > >> is not what's intended. We need an SPL_MEMORY symbol and then gate the > >> directory on that. > >> > >>> > >>> I have a question about how CONFIG_SPL_MEMORY works together with > >>> CONFIG_MEMORY. > >>> > >>> Do we use CONFIG_SPL_MEMORY only to gate the drivers/memory directory > >>> inclusion? > >>> Then continue to use CONFIG_MEMORY and others to enable/disable driver > >>> build for both non-SPL and SPL case? > >>> > >>> So drivers/memory/Makefile remains as it is? > >> > >> Well, for consistency code should use IS_ENABLED(MEMORY) which will be > >> true for CONFIG_MEMORY or CONFIG_SPL_MEMORY. > > > > nit: CONFIG_IS_ENABLED(MEMORY) > > CONFIG_IS_ENABLED* is used in code but not by any Makefile. > My question was on what basis do we gate drivers/memory inclusion in build > from Makefile. > > Can CONFIG_MEMORY and CONFIG_SPL_MEMORY co-exist or that should never happen? What we have for now in your patch is fine and we can tweak slightly if needed if other cases arise. -- Tom signature.asc Description: PGP signature
Re: [u-boot][PATCH v2 2/4] scripts: Makefile.spl: Enable memory drivers to be built for SPL
Tom, On 26/10/2022 02:35, Simon Glass wrote: > Hi Tom, > > On Thu, 20 Oct 2022 at 06:29, Tom Rini wrote: >> >> On Thu, Oct 20, 2022 at 03:23:42PM +0300, Roger Quadros wrote: >>> Hi Tom, >>> >>> On 19/10/2022 15:54, Tom Rini wrote: On Wed, Oct 19, 2022 at 11:17:35AM +0300, Roger Quadros wrote: > > > On 18/10/2022 20:40, Tom Rini wrote: >> On Thu, Oct 06, 2022 at 04:23:58PM +0300, Roger Quadros wrote: >>> We will need ti-gpmc driver for SPL. Allow memory drivers >>> do be built for SPL. >>> >>> Signed-off-by: Roger Quadros >>> --- >>> scripts/Makefile.spl | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl >>> index 3bafeb4fe9..110076b22f 100644 >>> --- a/scripts/Makefile.spl >>> +++ b/scripts/Makefile.spl >>> @@ -114,6 +114,7 @@ libs-$(CONFIG_PARTITIONS) += disk/ >>> endif >>> >>> libs-y += drivers/ >>> +libs-y += drivers/memory/ >>> libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/ >>> libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/ >>> libs-y += dts/ >> >> This ends up being the wrong approach as it then pulls in >> drivers/memory/stm32-fmc2-ebi.o on all of those platforms, in SPL, which >> is not what's intended. We need an SPL_MEMORY symbol and then gate the >> directory on that. >> >>> >>> I have a question about how CONFIG_SPL_MEMORY works together with >>> CONFIG_MEMORY. >>> >>> Do we use CONFIG_SPL_MEMORY only to gate the drivers/memory directory >>> inclusion? >>> Then continue to use CONFIG_MEMORY and others to enable/disable driver >>> build for both non-SPL and SPL case? >>> >>> So drivers/memory/Makefile remains as it is? >> >> Well, for consistency code should use IS_ENABLED(MEMORY) which will be >> true for CONFIG_MEMORY or CONFIG_SPL_MEMORY. > > nit: CONFIG_IS_ENABLED(MEMORY) CONFIG_IS_ENABLED* is used in code but not by any Makefile. My question was on what basis do we gate drivers/memory inclusion in build from Makefile. Can CONFIG_MEMORY and CONFIG_SPL_MEMORY co-exist or that should never happen? > > Perhaps we can look at my Kconfig series so it can become > CONFIG(MEMORY) and we can drop all the SPL_TPL_ stuff in Makefiles? > > Regards, > SImon cheers, -roger
Re: [u-boot][PATCH v2 2/4] scripts: Makefile.spl: Enable memory drivers to be built for SPL
Hi Tom, On Thu, 20 Oct 2022 at 06:29, Tom Rini wrote: > > On Thu, Oct 20, 2022 at 03:23:42PM +0300, Roger Quadros wrote: > > Hi Tom, > > > > On 19/10/2022 15:54, Tom Rini wrote: > > > On Wed, Oct 19, 2022 at 11:17:35AM +0300, Roger Quadros wrote: > > >> > > >> > > >> On 18/10/2022 20:40, Tom Rini wrote: > > >>> On Thu, Oct 06, 2022 at 04:23:58PM +0300, Roger Quadros wrote: > > We will need ti-gpmc driver for SPL. Allow memory drivers > > do be built for SPL. > > > > Signed-off-by: Roger Quadros > > --- > > scripts/Makefile.spl | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl > > index 3bafeb4fe9..110076b22f 100644 > > --- a/scripts/Makefile.spl > > +++ b/scripts/Makefile.spl > > @@ -114,6 +114,7 @@ libs-$(CONFIG_PARTITIONS) += disk/ > > endif > > > > libs-y += drivers/ > > +libs-y += drivers/memory/ > > libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/ > > libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/ > > libs-y += dts/ > > >>> > > >>> This ends up being the wrong approach as it then pulls in > > >>> drivers/memory/stm32-fmc2-ebi.o on all of those platforms, in SPL, which > > >>> is not what's intended. We need an SPL_MEMORY symbol and then gate the > > >>> directory on that. > > >>> > > > > I have a question about how CONFIG_SPL_MEMORY works together with > > CONFIG_MEMORY. > > > > Do we use CONFIG_SPL_MEMORY only to gate the drivers/memory directory > > inclusion? > > Then continue to use CONFIG_MEMORY and others to enable/disable driver > > build for both non-SPL and SPL case? > > > > So drivers/memory/Makefile remains as it is? > > Well, for consistency code should use IS_ENABLED(MEMORY) which will be > true for CONFIG_MEMORY or CONFIG_SPL_MEMORY. nit: CONFIG_IS_ENABLED(MEMORY) Perhaps we can look at my Kconfig series so it can become CONFIG(MEMORY) and we can drop all the SPL_TPL_ stuff in Makefiles? Regards, SImon
Re: [u-boot][PATCH v2 2/4] scripts: Makefile.spl: Enable memory drivers to be built for SPL
On Thu, Oct 20, 2022 at 03:23:42PM +0300, Roger Quadros wrote: > Hi Tom, > > On 19/10/2022 15:54, Tom Rini wrote: > > On Wed, Oct 19, 2022 at 11:17:35AM +0300, Roger Quadros wrote: > >> > >> > >> On 18/10/2022 20:40, Tom Rini wrote: > >>> On Thu, Oct 06, 2022 at 04:23:58PM +0300, Roger Quadros wrote: > We will need ti-gpmc driver for SPL. Allow memory drivers > do be built for SPL. > > Signed-off-by: Roger Quadros > --- > scripts/Makefile.spl | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl > index 3bafeb4fe9..110076b22f 100644 > --- a/scripts/Makefile.spl > +++ b/scripts/Makefile.spl > @@ -114,6 +114,7 @@ libs-$(CONFIG_PARTITIONS) += disk/ > endif > > libs-y += drivers/ > +libs-y += drivers/memory/ > libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/ > libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/ > libs-y += dts/ > >>> > >>> This ends up being the wrong approach as it then pulls in > >>> drivers/memory/stm32-fmc2-ebi.o on all of those platforms, in SPL, which > >>> is not what's intended. We need an SPL_MEMORY symbol and then gate the > >>> directory on that. > >>> > > I have a question about how CONFIG_SPL_MEMORY works together with > CONFIG_MEMORY. > > Do we use CONFIG_SPL_MEMORY only to gate the drivers/memory directory > inclusion? > Then continue to use CONFIG_MEMORY and others to enable/disable driver > build for both non-SPL and SPL case? > > So drivers/memory/Makefile remains as it is? Well, for consistency code should use IS_ENABLED(MEMORY) which will be true for CONFIG_MEMORY or CONFIG_SPL_MEMORY. -- Tom signature.asc Description: PGP signature
Re: [u-boot][PATCH v2 2/4] scripts: Makefile.spl: Enable memory drivers to be built for SPL
Hi Tom, On 19/10/2022 15:54, Tom Rini wrote: > On Wed, Oct 19, 2022 at 11:17:35AM +0300, Roger Quadros wrote: >> >> >> On 18/10/2022 20:40, Tom Rini wrote: >>> On Thu, Oct 06, 2022 at 04:23:58PM +0300, Roger Quadros wrote: We will need ti-gpmc driver for SPL. Allow memory drivers do be built for SPL. Signed-off-by: Roger Quadros --- scripts/Makefile.spl | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 3bafeb4fe9..110076b22f 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -114,6 +114,7 @@ libs-$(CONFIG_PARTITIONS) += disk/ endif libs-y += drivers/ +libs-y += drivers/memory/ libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/ libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/ libs-y += dts/ >>> >>> This ends up being the wrong approach as it then pulls in >>> drivers/memory/stm32-fmc2-ebi.o on all of those platforms, in SPL, which >>> is not what's intended. We need an SPL_MEMORY symbol and then gate the >>> directory on that. >>> I have a question about how CONFIG_SPL_MEMORY works together with CONFIG_MEMORY. Do we use CONFIG_SPL_MEMORY only to gate the drivers/memory directory inclusion? Then continue to use CONFIG_MEMORY and others to enable/disable driver build for both non-SPL and SPL case? So drivers/memory/Makefile remains as it is? >> >> That's right. I'll fix it up. Will wait for your comments on the rest >> of the series before re-spin. > > Everything else is fine, I was about to merge it (with a > %s/DM_MEMORY/MEMORY) when I saw the stm32 platforms increasing in size > and dug in. > cheers, -roger
Re: [u-boot][PATCH v2 2/4] scripts: Makefile.spl: Enable memory drivers to be built for SPL
On Wed, Oct 19, 2022 at 11:17:35AM +0300, Roger Quadros wrote: > > > On 18/10/2022 20:40, Tom Rini wrote: > > On Thu, Oct 06, 2022 at 04:23:58PM +0300, Roger Quadros wrote: > >> We will need ti-gpmc driver for SPL. Allow memory drivers > >> do be built for SPL. > >> > >> Signed-off-by: Roger Quadros > >> --- > >> scripts/Makefile.spl | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl > >> index 3bafeb4fe9..110076b22f 100644 > >> --- a/scripts/Makefile.spl > >> +++ b/scripts/Makefile.spl > >> @@ -114,6 +114,7 @@ libs-$(CONFIG_PARTITIONS) += disk/ > >> endif > >> > >> libs-y += drivers/ > >> +libs-y += drivers/memory/ > >> libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/ > >> libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/ > >> libs-y += dts/ > > > > This ends up being the wrong approach as it then pulls in > > drivers/memory/stm32-fmc2-ebi.o on all of those platforms, in SPL, which > > is not what's intended. We need an SPL_MEMORY symbol and then gate the > > directory on that. > > > > That's right. I'll fix it up. Will wait for your comments on the rest > of the series before re-spin. Everything else is fine, I was about to merge it (with a %s/DM_MEMORY/MEMORY) when I saw the stm32 platforms increasing in size and dug in. -- Tom signature.asc Description: PGP signature
Re: [u-boot][PATCH v2 2/4] scripts: Makefile.spl: Enable memory drivers to be built for SPL
On 18/10/2022 20:40, Tom Rini wrote: > On Thu, Oct 06, 2022 at 04:23:58PM +0300, Roger Quadros wrote: >> We will need ti-gpmc driver for SPL. Allow memory drivers >> do be built for SPL. >> >> Signed-off-by: Roger Quadros >> --- >> scripts/Makefile.spl | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl >> index 3bafeb4fe9..110076b22f 100644 >> --- a/scripts/Makefile.spl >> +++ b/scripts/Makefile.spl >> @@ -114,6 +114,7 @@ libs-$(CONFIG_PARTITIONS) += disk/ >> endif >> >> libs-y += drivers/ >> +libs-y += drivers/memory/ >> libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/ >> libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/ >> libs-y += dts/ > > This ends up being the wrong approach as it then pulls in > drivers/memory/stm32-fmc2-ebi.o on all of those platforms, in SPL, which > is not what's intended. We need an SPL_MEMORY symbol and then gate the > directory on that. > That's right. I'll fix it up. Will wait for your comments on the rest of the series before re-spin. cheers, -roger
Re: [u-boot][PATCH v2 2/4] scripts: Makefile.spl: Enable memory drivers to be built for SPL
On Thu, Oct 06, 2022 at 04:23:58PM +0300, Roger Quadros wrote: > We will need ti-gpmc driver for SPL. Allow memory drivers > do be built for SPL. > > Signed-off-by: Roger Quadros > --- > scripts/Makefile.spl | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl > index 3bafeb4fe9..110076b22f 100644 > --- a/scripts/Makefile.spl > +++ b/scripts/Makefile.spl > @@ -114,6 +114,7 @@ libs-$(CONFIG_PARTITIONS) += disk/ > endif > > libs-y += drivers/ > +libs-y += drivers/memory/ > libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/ > libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/ > libs-y += dts/ This ends up being the wrong approach as it then pulls in drivers/memory/stm32-fmc2-ebi.o on all of those platforms, in SPL, which is not what's intended. We need an SPL_MEMORY symbol and then gate the directory on that. -- Tom signature.asc Description: PGP signature
[u-boot][PATCH v2 2/4] scripts: Makefile.spl: Enable memory drivers to be built for SPL
We will need ti-gpmc driver for SPL. Allow memory drivers do be built for SPL. Signed-off-by: Roger Quadros --- scripts/Makefile.spl | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 3bafeb4fe9..110076b22f 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -114,6 +114,7 @@ libs-$(CONFIG_PARTITIONS) += disk/ endif libs-y += drivers/ +libs-y += drivers/memory/ libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/ libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/ libs-y += dts/ -- 2.17.1
