[PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Roger Quadros]

The root of the problem is that we are selecting symbols that have
dependencies. This can cause random configurations that can fail.
The cleanest solution is to avoid using select.

This driver uses interfaces from the OMAP_GPMC driver so we have to
depend on it instead.

Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/mtd/nand/raw/Kconfig | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index 36e697456ec4..9b078e78f3fa 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -42,8 +42,7 @@ config MTD_NAND_OMAP2
 	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
 	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
 	depends on HAS_IOMEM
-	select MEMORY
-	select OMAP_GPMC
+	depends on OMAP_GPMC
 	help
 	  Support for NAND flash on Texas Instruments OMAP2, OMAP3, OMAP4
 	  and Keystone platforms.
-- 
2.17.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Miquel Raynal]

On Sat, 2022-02-19 at 19:36:00 UTC, Roger Quadros wrote:
> The root of the problem is that we are selecting symbols that have
> dependencies. This can cause random configurations that can fail.
> The cleanest solution is to avoid using select.
> 
> This driver uses interfaces from the OMAP_GPMC driver so we have to
> depend on it instead.
> 
> Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
> Signed-off-by: Roger Quadros <rogerq@kernel.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Randy Dunlap]

On 2/19/22 16:44, Guenter Roeck wrote:
> On Sat, Feb 19, 2022 at 09:36:00PM +0200, Roger Quadros wrote:
>> The root of the problem is that we are selecting symbols that have
>> dependencies. This can cause random configurations that can fail.
>> The cleanest solution is to avoid using select.
>>
>> This driver uses interfaces from the OMAP_GPMC driver so we have to
>> depend on it instead.
>>
>> Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>

Tested-by: Randy Dunlap <rdunlap@infradead.org>

Thanks.

> 
>> ---
>>  drivers/mtd/nand/raw/Kconfig | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>> index 36e697456ec4..9b078e78f3fa 100644
>> --- a/drivers/mtd/nand/raw/Kconfig
>> +++ b/drivers/mtd/nand/raw/Kconfig
>> @@ -42,8 +42,7 @@ config MTD_NAND_OMAP2
>>  	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
>>  	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>>  	depends on HAS_IOMEM
>> -	select MEMORY
>> -	select OMAP_GPMC
>> +	depends on OMAP_GPMC
>>  	help
>>  	  Support for NAND flash on Texas Instruments OMAP2, OMAP3, OMAP4
>>  	  and Keystone platforms.
>> -- 
>> 2.17.1
>>

-- 
~Randy

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Miquel Raynal]

Hi Guenter, Roger,

rdunlap@infradead.org wrote on Sat, 26 Feb 2022 22:55:28 -0800:

> On 2/19/22 16:44, Guenter Roeck wrote:
> > On Sat, Feb 19, 2022 at 09:36:00PM +0200, Roger Quadros wrote:  
> >> The root of the problem is that we are selecting symbols that have
> >> dependencies. This can cause random configurations that can fail.
> >> The cleanest solution is to avoid using select.
> >>
> >> This driver uses interfaces from the OMAP_GPMC driver so we have to
> >> depend on it instead.
> >>
> >> Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
> >> Signed-off-by: Roger Quadros <rogerq@kernel.org>  
> > 
> > Tested-by: Guenter Roeck <linux@roeck-us.net>  
> 
> Tested-by: Randy Dunlap <rdunlap@infradead.org>

Sorry for noticing that just now, but there is still a problem with
this patch: we now always compile-in the OMAP_GPMC driver whenever we
need the NAND controller, even though it is not needed. This grows the
kernel for no reason.

In fact, Roger once said:

	"We will figure out how to enable OMAP_GPMC for K3 architecture
	some other way."

It turns out this is not what was finally proposed. Could we try yet
another solution?

> 
> Thanks.
> 
> >   
> >> ---
> >>  drivers/mtd/nand/raw/Kconfig | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> >> index 36e697456ec4..9b078e78f3fa 100644
> >> --- a/drivers/mtd/nand/raw/Kconfig
> >> +++ b/drivers/mtd/nand/raw/Kconfig
> >> @@ -42,8 +42,7 @@ config MTD_NAND_OMAP2
> >>  	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
> >>  	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> >>  	depends on HAS_IOMEM
> >> -	select MEMORY
> >> -	select OMAP_GPMC
> >> +	depends on OMAP_GPMC
> >>  	help
> >>  	  Support for NAND flash on Texas Instruments OMAP2, OMAP3, OMAP4
> >>  	  and Keystone platforms.
> >> -- 
> >> 2.17.1
> >>  
> 

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Guenter Roeck]

On 3/4/22 07:54, Miquel Raynal wrote:
> Hi Guenter, Roger,
> 
> rdunlap@infradead.org wrote on Sat, 26 Feb 2022 22:55:28 -0800:
> 
>> On 2/19/22 16:44, Guenter Roeck wrote:
>>> On Sat, Feb 19, 2022 at 09:36:00PM +0200, Roger Quadros wrote:
>>>> The root of the problem is that we are selecting symbols that have
>>>> dependencies. This can cause random configurations that can fail.
>>>> The cleanest solution is to avoid using select.
>>>>
>>>> This driver uses interfaces from the OMAP_GPMC driver so we have to
>>>> depend on it instead.
>>>>
>>>> Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>>>
>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>
>> Tested-by: Randy Dunlap <rdunlap@infradead.org>
> 
> Sorry for noticing that just now, but there is still a problem with
> this patch: we now always compile-in the OMAP_GPMC driver whenever we
> need the NAND controller, even though it is not needed. This grows the
> kernel for no reason.
> 

Sorry, you lost me.

> In fact, Roger once said:
> 
> 	"We will figure out how to enable OMAP_GPMC for K3 architecture
> 	some other way."
> 
> It turns out this is not what was finally proposed. Could we try yet
> another solution?
> 
>>
>> Thanks.
>>
>>>    
>>>> ---
>>>>   drivers/mtd/nand/raw/Kconfig | 3 +--
>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>>>> index 36e697456ec4..9b078e78f3fa 100644
>>>> --- a/drivers/mtd/nand/raw/Kconfig
>>>> +++ b/drivers/mtd/nand/raw/Kconfig
>>>> @@ -42,8 +42,7 @@ config MTD_NAND_OMAP2
>>>>   	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
>>>>   	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>>>>   	depends on HAS_IOMEM
>>>> -	select MEMORY
>>>> -	select OMAP_GPMC
>>>> +	depends on OMAP_GPMC

No matter if select or depend, both result in OMAP_GPMC enabled
if MTD_NAND_OMAP2={m,y}, and both suggest that OMAP_GPMC is
needed for MTD_NAND_OMAP2. That conflicts with

  we now always compile-in the OMAP_GPMC driver whenever we
  need the NAND controller, even though it is not needed
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

If it isn't needed, why the dependency in the first place ?
Also, commit 4cd335dae3cf2 states "this driver depends on OMAP_GPMC
driver and uses symbols from there". Are you saying that this is not
correct ?

Guenter

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Roger Quadros]

Hi Miquel,

On 04/03/2022 17:54, Miquel Raynal wrote:
> Hi Guenter, Roger,
> 
> rdunlap@infradead.org wrote on Sat, 26 Feb 2022 22:55:28 -0800:
> 
>> On 2/19/22 16:44, Guenter Roeck wrote:
>>> On Sat, Feb 19, 2022 at 09:36:00PM +0200, Roger Quadros wrote:  
>>>> The root of the problem is that we are selecting symbols that have
>>>> dependencies. This can cause random configurations that can fail.
>>>> The cleanest solution is to avoid using select.
>>>>
>>>> This driver uses interfaces from the OMAP_GPMC driver so we have to
>>>> depend on it instead.
>>>>
>>>> Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>  
>>>
>>> Tested-by: Guenter Roeck <linux@roeck-us.net>  
>>
>> Tested-by: Randy Dunlap <rdunlap@infradead.org>
> 
> Sorry for noticing that just now, but there is still a problem with
> this patch: we now always compile-in the OMAP_GPMC driver whenever we
> need the NAND controller, even though it is not needed. This grows the
> kernel for no reason.

Sorry, I did not understand what you meant.

We no longer explicitly enable OMAP_GPMC since we dropped the "select".
This fixes all build issues that were reported recently.

MTD_NAND_OMAP2 will not be enabled if OMAP_GPMC is not since we added
the "depends on". This fixes the original build issue that we started to
fix with select initially.

> 
> In fact, Roger once said:
> 
> 	"We will figure out how to enable OMAP_GPMC for K3 architecture
> 	some other way."
> 
> It turns out this is not what was finally proposed. Could we try yet
> another solution?

This issue is still present i.e. we cannot enable MTD_NAND_OMAP2 driver on
K3 platform since OMAP_GPMC config is hidden and not select-able
by user or defconfig file.

But it is not yet a deal breaker since NAND on K3 is not yet enabled upstream.

For this I think OMAP_GPMC has to be a visible config entry and select-able
from a defconfig file as I had done initially [1].

Now we have a lot of explanation to write as to why we need to do it ;)

[1] - https://lore.kernel.org/lkml/20211123102607.13002-3-rogerq@kernel.org/

> 
>>
>> Thanks.
>>
>>>   
>>>> ---
>>>>  drivers/mtd/nand/raw/Kconfig | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>>>> index 36e697456ec4..9b078e78f3fa 100644
>>>> --- a/drivers/mtd/nand/raw/Kconfig
>>>> +++ b/drivers/mtd/nand/raw/Kconfig
>>>> @@ -42,8 +42,7 @@ config MTD_NAND_OMAP2
>>>>  	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
>>>>  	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>>>>  	depends on HAS_IOMEM
>>>> -	select MEMORY
>>>> -	select OMAP_GPMC
>>>> +	depends on OMAP_GPMC
>>>>  	help
>>>>  	  Support for NAND flash on Texas Instruments OMAP2, OMAP3, OMAP4
>>>>  	  and Keystone platforms.
>>>> -- 
>>>> 2.17.1
>>>>  
>>
> 
> Thanks,
> Miquèl


cheers,
-roger

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Miquel Raynal]

Hi Roger,

rogerq@kernel.org wrote on Sat, 5 Mar 2022 00:50:14 +0200:

> Hi Miquel,
> 
> On 04/03/2022 17:54, Miquel Raynal wrote:
> > Hi Guenter, Roger,
> > 
> > rdunlap@infradead.org wrote on Sat, 26 Feb 2022 22:55:28 -0800:
> >   
> >> On 2/19/22 16:44, Guenter Roeck wrote:  
> >>> On Sat, Feb 19, 2022 at 09:36:00PM +0200, Roger Quadros wrote:    
> >>>> The root of the problem is that we are selecting symbols that have
> >>>> dependencies. This can cause random configurations that can fail.
> >>>> The cleanest solution is to avoid using select.
> >>>>
> >>>> This driver uses interfaces from the OMAP_GPMC driver so we have to
> >>>> depend on it instead.
> >>>>
> >>>> Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
> >>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>    
> >>>
> >>> Tested-by: Guenter Roeck <linux@roeck-us.net>    
> >>
> >> Tested-by: Randy Dunlap <rdunlap@infradead.org>  
> > 
> > Sorry for noticing that just now, but there is still a problem with
> > this patch: we now always compile-in the OMAP_GPMC driver whenever we
> > need the NAND controller, even though it is not needed. This grows the
> > kernel for no reason.  
> 
> Sorry, I did not understand what you meant.
> 
> We no longer explicitly enable OMAP_GPMC since we dropped the "select".
> This fixes all build issues that were reported recently.
> 
> MTD_NAND_OMAP2 will not be enabled if OMAP_GPMC is not since we added
> the "depends on". This fixes the original build issue that we started to
> fix with select initially.

Yes, this side is fine.

In the initial commit, you proposed:

--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -42,7 +42,8 @@ config MTD_NAND_OMAP2
        tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
        depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
        depends on HAS_IOMEM
+       select OMAP_GPMC if ARCH_K3

Which creates a dependency over OMAP_GPMC only for a single
architecture. Which means that other OMAP platforms do not necessarily
need OMAP_GPMC for the NAND controller to work. Now, you propose:

--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -42,8 +42,7 @@ config MTD_NAND_OMAP2
 	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
 	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
 	depends on HAS_IOMEM
	depends on OMAP_GPMC

This means any of the other OMAP architectures will compile the GPMC
driver even though they might not need it, which would unnecessarily
increase the kernel size.

Am I missing something?

> > In fact, Roger once said:
> > 
> > 	"We will figure out how to enable OMAP_GPMC for K3 architecture
> > 	some other way."
> > 
> > It turns out this is not what was finally proposed. Could we try yet
> > another solution?  
> 
> This issue is still present i.e. we cannot enable MTD_NAND_OMAP2 driver on
> K3 platform since OMAP_GPMC config is hidden and not select-able
> by user or defconfig file.
> 
> But it is not yet a deal breaker since NAND on K3 is not yet enabled upstream.
> 
> For this I think OMAP_GPMC has to be a visible config entry and select-able
> from a defconfig file as I had done initially [1].
> 
> Now we have a lot of explanation to write as to why we need to do it ;)

We certainly do :)

> [1] - https://lore.kernel.org/lkml/20211123102607.13002-3-rogerq@kernel.org/
> 

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Roger Quadros]

Hi Miquel,

On 07/03/2022 12:03, Miquel Raynal wrote:
> Hi Roger,
> 
> rogerq@kernel.org wrote on Sat, 5 Mar 2022 00:50:14 +0200:
> 
>> Hi Miquel,
>>
>> On 04/03/2022 17:54, Miquel Raynal wrote:
>>> Hi Guenter, Roger,
>>>
>>> rdunlap@infradead.org wrote on Sat, 26 Feb 2022 22:55:28 -0800:
>>>   
>>>> On 2/19/22 16:44, Guenter Roeck wrote:  
>>>>> On Sat, Feb 19, 2022 at 09:36:00PM +0200, Roger Quadros wrote:    
>>>>>> The root of the problem is that we are selecting symbols that have
>>>>>> dependencies. This can cause random configurations that can fail.
>>>>>> The cleanest solution is to avoid using select.
>>>>>>
>>>>>> This driver uses interfaces from the OMAP_GPMC driver so we have to
>>>>>> depend on it instead.
>>>>>>
>>>>>> Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>    
>>>>>
>>>>> Tested-by: Guenter Roeck <linux@roeck-us.net>    
>>>>
>>>> Tested-by: Randy Dunlap <rdunlap@infradead.org>  
>>>
>>> Sorry for noticing that just now, but there is still a problem with
>>> this patch: we now always compile-in the OMAP_GPMC driver whenever we
>>> need the NAND controller, even though it is not needed. This grows the
>>> kernel for no reason.  
>>
>> Sorry, I did not understand what you meant.
>>
>> We no longer explicitly enable OMAP_GPMC since we dropped the "select".
>> This fixes all build issues that were reported recently.
>>
>> MTD_NAND_OMAP2 will not be enabled if OMAP_GPMC is not since we added
>> the "depends on". This fixes the original build issue that we started to
>> fix with select initially.
> 
> Yes, this side is fine.
> 
> In the initial commit, you proposed:
> 
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -42,7 +42,8 @@ config MTD_NAND_OMAP2
>         tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
>         depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>         depends on HAS_IOMEM
> +       select OMAP_GPMC if ARCH_K3
> 
> Which creates a dependency over OMAP_GPMC only for a single
> architecture. Which means that other OMAP platforms do not necessarily
> need OMAP_GPMC for the NAND controller to work. Now, you propose:

No that is not true. Other platforms that need MTD_NAND_OMAP2 are
explicitly selecting OMAP_GPMC
i.e. in arch/arm/mach-omap2/Kconfig

> 
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -42,8 +42,7 @@ config MTD_NAND_OMAP2
>  	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
>  	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>  	depends on HAS_IOMEM
> 	depends on OMAP_GPMC
> 
> This means any of the other OMAP architectures will compile the GPMC
> driver even though they might not need it, which would unnecessarily
> increase the kernel size.
> 
> Am I missing something?

MTD_NAND_OMAP2 NAND controller is a submodule of the OMAP GPMC IP. So it
cannot work without OMAP_GPMC driver.

Hope this clarifies the doubts.

> 
>>> In fact, Roger once said:
>>>
>>> 	"We will figure out how to enable OMAP_GPMC for K3 architecture
>>> 	some other way."
>>>
>>> It turns out this is not what was finally proposed. Could we try yet
>>> another solution?  
>>
>> This issue is still present i.e. we cannot enable MTD_NAND_OMAP2 driver on
>> K3 platform since OMAP_GPMC config is hidden and not select-able
>> by user or defconfig file.
>>
>> But it is not yet a deal breaker since NAND on K3 is not yet enabled upstream.
>>
>> For this I think OMAP_GPMC has to be a visible config entry and select-able
>> from a defconfig file as I had done initially [1].
>>
>> Now we have a lot of explanation to write as to why we need to do it ;)
> 
> We certainly do :)
> 
>> [1] - https://lore.kernel.org/lkml/20211123102607.13002-3-rogerq@kernel.org/
>>
> 
> Thanks,
> Miquèl

cheers,
-roger

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Miquel Raynal]

Hi Roger,

rogerq@kernel.org wrote on Mon, 7 Mar 2022 14:25:48 +0200:

> Hi Miquel,
> 
> On 07/03/2022 12:03, Miquel Raynal wrote:
> > Hi Roger,
> > 
> > rogerq@kernel.org wrote on Sat, 5 Mar 2022 00:50:14 +0200:
> >   
> >> Hi Miquel,
> >>
> >> On 04/03/2022 17:54, Miquel Raynal wrote:  
> >>> Hi Guenter, Roger,
> >>>
> >>> rdunlap@infradead.org wrote on Sat, 26 Feb 2022 22:55:28 -0800:
> >>>     
> >>>> On 2/19/22 16:44, Guenter Roeck wrote:    
> >>>>> On Sat, Feb 19, 2022 at 09:36:00PM +0200, Roger Quadros wrote:      
> >>>>>> The root of the problem is that we are selecting symbols that have
> >>>>>> dependencies. This can cause random configurations that can fail.
> >>>>>> The cleanest solution is to avoid using select.
> >>>>>>
> >>>>>> This driver uses interfaces from the OMAP_GPMC driver so we have to
> >>>>>> depend on it instead.
> >>>>>>
> >>>>>> Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
> >>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>      
> >>>>>
> >>>>> Tested-by: Guenter Roeck <linux@roeck-us.net>      
> >>>>
> >>>> Tested-by: Randy Dunlap <rdunlap@infradead.org>    
> >>>
> >>> Sorry for noticing that just now, but there is still a problem with
> >>> this patch: we now always compile-in the OMAP_GPMC driver whenever we
> >>> need the NAND controller, even though it is not needed. This grows the
> >>> kernel for no reason.    
> >>
> >> Sorry, I did not understand what you meant.
> >>
> >> We no longer explicitly enable OMAP_GPMC since we dropped the "select".
> >> This fixes all build issues that were reported recently.
> >>
> >> MTD_NAND_OMAP2 will not be enabled if OMAP_GPMC is not since we added
> >> the "depends on". This fixes the original build issue that we started to
> >> fix with select initially.  
> > 
> > Yes, this side is fine.
> > 
> > In the initial commit, you proposed:
> > 
> > --- a/drivers/mtd/nand/raw/Kconfig
> > +++ b/drivers/mtd/nand/raw/Kconfig
> > @@ -42,7 +42,8 @@ config MTD_NAND_OMAP2
> >         tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
> >         depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> >         depends on HAS_IOMEM
> > +       select OMAP_GPMC if ARCH_K3
> > 
> > Which creates a dependency over OMAP_GPMC only for a single
> > architecture. Which means that other OMAP platforms do not necessarily
> > need OMAP_GPMC for the NAND controller to work. Now, you propose:  
> 
> No that is not true. Other platforms that need MTD_NAND_OMAP2 are
> explicitly selecting OMAP_GPMC
> i.e. in arch/arm/mach-omap2/Kconfig

Okay, in this case the fix is fine but we will need to clean this up in
a second time.

> > --- a/drivers/mtd/nand/raw/Kconfig
> > +++ b/drivers/mtd/nand/raw/Kconfig
> > @@ -42,8 +42,7 @@ config MTD_NAND_OMAP2
> >  	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
> >  	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> >  	depends on HAS_IOMEM
> > 	depends on OMAP_GPMC
> > 
> > This means any of the other OMAP architectures will compile the GPMC
> > driver even though they might not need it, which would unnecessarily
> > increase the kernel size.
> > 
> > Am I missing something?  
> 
> MTD_NAND_OMAP2 NAND controller is a submodule of the OMAP GPMC IP. So it
> cannot work without OMAP_GPMC driver.

I didn't remember exactly but in that case it's okay, I was just
surprised by the "select GPMC if ARCH_K3" but indeed with this
explanation it makes more sense.

> Hope this clarifies the doubts.

Yes, thanks. I will send the fix to Linus then.

Cheers,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Roger Quadros]

Hi Miquel,

On 07/03/2022 16:12, Miquel Raynal wrote:
> Hi Roger,
> 
> rogerq@kernel.org wrote on Mon, 7 Mar 2022 14:25:48 +0200:
> 
>> Hi Miquel,
>>
>> On 07/03/2022 12:03, Miquel Raynal wrote:
>>> Hi Roger,
>>>
>>> rogerq@kernel.org wrote on Sat, 5 Mar 2022 00:50:14 +0200:
>>>   
>>>> Hi Miquel,
>>>>
>>>> On 04/03/2022 17:54, Miquel Raynal wrote:  
>>>>> Hi Guenter, Roger,
>>>>>
>>>>> rdunlap@infradead.org wrote on Sat, 26 Feb 2022 22:55:28 -0800:
>>>>>     
>>>>>> On 2/19/22 16:44, Guenter Roeck wrote:    
>>>>>>> On Sat, Feb 19, 2022 at 09:36:00PM +0200, Roger Quadros wrote:      
>>>>>>>> The root of the problem is that we are selecting symbols that have
>>>>>>>> dependencies. This can cause random configurations that can fail.
>>>>>>>> The cleanest solution is to avoid using select.
>>>>>>>>
>>>>>>>> This driver uses interfaces from the OMAP_GPMC driver so we have to
>>>>>>>> depend on it instead.
>>>>>>>>
>>>>>>>> Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
>>>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>      
>>>>>>>
>>>>>>> Tested-by: Guenter Roeck <linux@roeck-us.net>      
>>>>>>
>>>>>> Tested-by: Randy Dunlap <rdunlap@infradead.org>    
>>>>>
>>>>> Sorry for noticing that just now, but there is still a problem with
>>>>> this patch: we now always compile-in the OMAP_GPMC driver whenever we
>>>>> need the NAND controller, even though it is not needed. This grows the
>>>>> kernel for no reason.    
>>>>
>>>> Sorry, I did not understand what you meant.
>>>>
>>>> We no longer explicitly enable OMAP_GPMC since we dropped the "select".
>>>> This fixes all build issues that were reported recently.
>>>>
>>>> MTD_NAND_OMAP2 will not be enabled if OMAP_GPMC is not since we added
>>>> the "depends on". This fixes the original build issue that we started to
>>>> fix with select initially.  
>>>
>>> Yes, this side is fine.
>>>
>>> In the initial commit, you proposed:
>>>
>>> --- a/drivers/mtd/nand/raw/Kconfig
>>> +++ b/drivers/mtd/nand/raw/Kconfig
>>> @@ -42,7 +42,8 @@ config MTD_NAND_OMAP2
>>>         tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
>>>         depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>>>         depends on HAS_IOMEM
>>> +       select OMAP_GPMC if ARCH_K3
>>>
>>> Which creates a dependency over OMAP_GPMC only for a single
>>> architecture. Which means that other OMAP platforms do not necessarily
>>> need OMAP_GPMC for the NAND controller to work. Now, you propose:  
>>
>> No that is not true. Other platforms that need MTD_NAND_OMAP2 are
>> explicitly selecting OMAP_GPMC
>> i.e. in arch/arm/mach-omap2/Kconfig
> 
> Okay, in this case the fix is fine but we will need to clean this up in
> a second time.

What clean up are you implying here? Those legacy platform might need
OMAP_GPMC for booting so they select it. There is nothing much we can do there.

What is left to do now is make user/defconfig files to enable OMAP_GPMC driver
so other platforms that don't need OMAP_GPMC for basic operation can still
enable them later via defconfig or manually by user.

> 
>>> --- a/drivers/mtd/nand/raw/Kconfig
>>> +++ b/drivers/mtd/nand/raw/Kconfig
>>> @@ -42,8 +42,7 @@ config MTD_NAND_OMAP2
>>>  	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
>>>  	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>>>  	depends on HAS_IOMEM
>>> 	depends on OMAP_GPMC
>>>
>>> This means any of the other OMAP architectures will compile the GPMC
>>> driver even though they might not need it, which would unnecessarily
>>> increase the kernel size.
>>>
>>> Am I missing something?  
>>
>> MTD_NAND_OMAP2 NAND controller is a submodule of the OMAP GPMC IP. So it
>> cannot work without OMAP_GPMC driver.
> 
> I didn't remember exactly but in that case it's okay, I was just
> surprised by the "select GPMC if ARCH_K3" but indeed with this
> explanation it makes more sense.
> 
>> Hope this clarifies the doubts.
> 
> Yes, thanks. I will send the fix to Linus then.
> 
> Cheers,
> Miquèl

cheers,
-roger

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Miquel Raynal]

Hi Roger,

rogerq@kernel.org wrote on Mon, 7 Mar 2022 23:05:47 +0200:

> Hi Miquel,
> 
> On 07/03/2022 16:12, Miquel Raynal wrote:
> > Hi Roger,
> > 
> > rogerq@kernel.org wrote on Mon, 7 Mar 2022 14:25:48 +0200:
> >   
> >> Hi Miquel,
> >>
> >> On 07/03/2022 12:03, Miquel Raynal wrote:  
> >>> Hi Roger,
> >>>
> >>> rogerq@kernel.org wrote on Sat, 5 Mar 2022 00:50:14 +0200:
> >>>     
> >>>> Hi Miquel,
> >>>>
> >>>> On 04/03/2022 17:54, Miquel Raynal wrote:    
> >>>>> Hi Guenter, Roger,
> >>>>>
> >>>>> rdunlap@infradead.org wrote on Sat, 26 Feb 2022 22:55:28 -0800:
> >>>>>       
> >>>>>> On 2/19/22 16:44, Guenter Roeck wrote:      
> >>>>>>> On Sat, Feb 19, 2022 at 09:36:00PM +0200, Roger Quadros wrote:        
> >>>>>>>> The root of the problem is that we are selecting symbols that have
> >>>>>>>> dependencies. This can cause random configurations that can fail.
> >>>>>>>> The cleanest solution is to avoid using select.
> >>>>>>>>
> >>>>>>>> This driver uses interfaces from the OMAP_GPMC driver so we have to
> >>>>>>>> depend on it instead.
> >>>>>>>>
> >>>>>>>> Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
> >>>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>        
> >>>>>>>
> >>>>>>> Tested-by: Guenter Roeck <linux@roeck-us.net>        
> >>>>>>
> >>>>>> Tested-by: Randy Dunlap <rdunlap@infradead.org>      
> >>>>>
> >>>>> Sorry for noticing that just now, but there is still a problem with
> >>>>> this patch: we now always compile-in the OMAP_GPMC driver whenever we
> >>>>> need the NAND controller, even though it is not needed. This grows the
> >>>>> kernel for no reason.      
> >>>>
> >>>> Sorry, I did not understand what you meant.
> >>>>
> >>>> We no longer explicitly enable OMAP_GPMC since we dropped the "select".
> >>>> This fixes all build issues that were reported recently.
> >>>>
> >>>> MTD_NAND_OMAP2 will not be enabled if OMAP_GPMC is not since we added
> >>>> the "depends on". This fixes the original build issue that we started to
> >>>> fix with select initially.    
> >>>
> >>> Yes, this side is fine.
> >>>
> >>> In the initial commit, you proposed:
> >>>
> >>> --- a/drivers/mtd/nand/raw/Kconfig
> >>> +++ b/drivers/mtd/nand/raw/Kconfig
> >>> @@ -42,7 +42,8 @@ config MTD_NAND_OMAP2
> >>>         tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
> >>>         depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> >>>         depends on HAS_IOMEM
> >>> +       select OMAP_GPMC if ARCH_K3
> >>>
> >>> Which creates a dependency over OMAP_GPMC only for a single
> >>> architecture. Which means that other OMAP platforms do not necessarily
> >>> need OMAP_GPMC for the NAND controller to work. Now, you propose:    
> >>
> >> No that is not true. Other platforms that need MTD_NAND_OMAP2 are
> >> explicitly selecting OMAP_GPMC
> >> i.e. in arch/arm/mach-omap2/Kconfig  
> > 
> > Okay, in this case the fix is fine but we will need to clean this up in
> > a second time.  
> 
> What clean up are you implying here? Those legacy platform might need
> OMAP_GPMC for booting so they select it. There is nothing much we can do there.

If all the board that need the OMAP_GPMC select it, then we should just
take care that the K3 architecture does not behave differently unless
there is a good reason to do it differently. AFAIR you told me this
architecture did not select OMAP_GPMC yet.

> What is left to do now is make user/defconfig files to enable OMAP_GPMC driver
> so other platforms that don't need OMAP_GPMC for basic operation can still
> enable them later via defconfig or manually by user.

And yes, that is also what I meant to be "cleaned up" :)

> >>> --- a/drivers/mtd/nand/raw/Kconfig
> >>> +++ b/drivers/mtd/nand/raw/Kconfig
> >>> @@ -42,8 +42,7 @@ config MTD_NAND_OMAP2
> >>>  	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
> >>>  	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> >>>  	depends on HAS_IOMEM
> >>> 	depends on OMAP_GPMC
> >>>
> >>> This means any of the other OMAP architectures will compile the GPMC
> >>> driver even though they might not need it, which would unnecessarily
> >>> increase the kernel size.
> >>>
> >>> Am I missing something?    
> >>
> >> MTD_NAND_OMAP2 NAND controller is a submodule of the OMAP GPMC IP. So it
> >> cannot work without OMAP_GPMC driver.  
> > 
> > I didn't remember exactly but in that case it's okay, I was just
> > surprised by the "select GPMC if ARCH_K3" but indeed with this
> > explanation it makes more sense.
> >   
> >> Hope this clarifies the doubts.  
> > 
> > Yes, thanks. I will send the fix to Linus then.
> > 
> > Cheers,
> > Miquèl  
> 
> cheers,
> -roger


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: omap2: Actually prevent invalid configuration and build error

[Miquel Raynal]

Hi Roger,

rogerq@kernel.org wrote on Mon, 7 Mar 2022 14:25:48 +0200:

> Hi Miquel,
> 
> On 07/03/2022 12:03, Miquel Raynal wrote:
> > Hi Roger,
> > 
> > rogerq@kernel.org wrote on Sat, 5 Mar 2022 00:50:14 +0200:
> >   
> >> Hi Miquel,
> >>
> >> On 04/03/2022 17:54, Miquel Raynal wrote:  
> >>> Hi Guenter, Roger,
> >>>
> >>> rdunlap@infradead.org wrote on Sat, 26 Feb 2022 22:55:28 -0800:
> >>>     
> >>>> On 2/19/22 16:44, Guenter Roeck wrote:    
> >>>>> On Sat, Feb 19, 2022 at 09:36:00PM +0200, Roger Quadros wrote:      
> >>>>>> The root of the problem is that we are selecting symbols that have
> >>>>>> dependencies. This can cause random configurations that can fail.
> >>>>>> The cleanest solution is to avoid using select.
> >>>>>>
> >>>>>> This driver uses interfaces from the OMAP_GPMC driver so we have to
> >>>>>> depend on it instead.
> >>>>>>
> >>>>>> Fixes: 4cd335dae3cf ("mtd: rawnand: omap2: Prevent invalid configuration and build error")
> >>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>      
> >>>>>
> >>>>> Tested-by: Guenter Roeck <linux@roeck-us.net>      
> >>>>
> >>>> Tested-by: Randy Dunlap <rdunlap@infradead.org>    
> >>>
> >>> Sorry for noticing that just now, but there is still a problem with
> >>> this patch: we now always compile-in the OMAP_GPMC driver whenever we
> >>> need the NAND controller, even though it is not needed. This grows the
> >>> kernel for no reason.    
> >>
> >> Sorry, I did not understand what you meant.
> >>
> >> We no longer explicitly enable OMAP_GPMC since we dropped the "select".
> >> This fixes all build issues that were reported recently.
> >>
> >> MTD_NAND_OMAP2 will not be enabled if OMAP_GPMC is not since we added
> >> the "depends on". This fixes the original build issue that we started to
> >> fix with select initially.  
> > 
> > Yes, this side is fine.
> > 
> > In the initial commit, you proposed:
> > 
> > --- a/drivers/mtd/nand/raw/Kconfig
> > +++ b/drivers/mtd/nand/raw/Kconfig
> > @@ -42,7 +42,8 @@ config MTD_NAND_OMAP2
> >         tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
> >         depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> >         depends on HAS_IOMEM
> > +       select OMAP_GPMC if ARCH_K3
> > 
> > Which creates a dependency over OMAP_GPMC only for a single
> > architecture. Which means that other OMAP platforms do not necessarily
> > need OMAP_GPMC for the NAND controller to work. Now, you propose:  
> 
> No that is not true. Other platforms that need MTD_NAND_OMAP2 are
> explicitly selecting OMAP_GPMC
> i.e. in arch/arm/mach-omap2/Kconfig

Ok, in this case the fix is legit, but as you said there is certainly
some clean up to do on this side in a second time.

> > --- a/drivers/mtd/nand/raw/Kconfig
> > +++ b/drivers/mtd/nand/raw/Kconfig
> > @@ -42,8 +42,7 @@ config MTD_NAND_OMAP2
> >  	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
> >  	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> >  	depends on HAS_IOMEM
> > 	depends on OMAP_GPMC
> > 
> > This means any of the other OMAP architectures will compile the GPMC
> > driver even though they might not need it, which would unnecessarily
> > increase the kernel size.
> > 
> > Am I missing something?  
> 
> MTD_NAND_OMAP2 NAND controller is a submodule of the OMAP GPMC IP. So it
> cannot work without OMAP_GPMC driver.
> 
> Hope this clarifies the doubts.

I was not sure anymore if there was a proper hardware dependency here
because of the "select GPMC if ARCH_K3" addition. But your
clarification make this more understandable now.

I will send the fix to Linus.

Cheers,
Miquèl

> >>> In fact, Roger once said:
> >>>
> >>> 	"We will figure out how to enable OMAP_GPMC for K3 architecture
> >>> 	some other way."
> >>>
> >>> It turns out this is not what was finally proposed. Could we try yet
> >>> another solution?    
> >>
> >> This issue is still present i.e. we cannot enable MTD_NAND_OMAP2 driver on
> >> K3 platform since OMAP_GPMC config is hidden and not select-able
> >> by user or defconfig file.
> >>
> >> But it is not yet a deal breaker since NAND on K3 is not yet enabled upstream.
> >>
> >> For this I think OMAP_GPMC has to be a visible config entry and select-able
> >> from a defconfig file as I had done initially [1].
> >>
> >> Now we have a lot of explanation to write as to why we need to do it ;)  
> > 
> > We certainly do :)
> >   
> >> [1] - https://lore.kernel.org/lkml/20211123102607.13002-3-rogerq@kernel.org/
> >>  
> > 
> > Thanks,
> > Miquèl  
> 
> cheers,
> -roger

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/