| Message ID | 20221006132400.5628-3-rogerq@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Introduce MEMORY uclass and TI GPMC driver | expand |
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 <rogerq@kernel.org> > --- > 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.
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 <rogerq@kernel.org> >> --- >> 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
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 <rogerq@kernel.org> > >> --- > >> 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.
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 <rogerq@kernel.org> >>>> --- >>>> 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
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 <rogerq@kernel.org> > >>>> --- > >>>> 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.
Hi Tom, On Thu, 20 Oct 2022 at 06:29, Tom Rini <trini@konsulko.com> 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 <rogerq@kernel.org> > > >>>> --- > > >>>> 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
Tom, On 26/10/2022 02:35, Simon Glass wrote: > Hi Tom, > > On Thu, 20 Oct 2022 at 06:29, Tom Rini <trini@konsulko.com> 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 <rogerq@kernel.org> >>>>>>> --- >>>>>>> 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
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 <trini@konsulko.com> 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 <rogerq@kernel.org> > >>>>>>> --- > >>>>>>> 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.
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 <trini@konsulko.com> 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 <rogerq@kernel.org> > > > >>>> --- > > > >>>> 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.
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/
We will need ti-gpmc driver for SPL. Allow memory drivers do be built for SPL. Signed-off-by: Roger Quadros <rogerq@kernel.org> --- scripts/Makefile.spl | 1 + 1 file changed, 1 insertion(+)