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@xxxxxxxxxx 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@xxxxxxxxxxxxx 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@xxxxxxxxxx>    
>>>>>
>>>>> Tested-by: Guenter Roeck <linux@xxxxxxxxxxxx>    
>>>>
>>>> Tested-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>  
>>>
>>> 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@xxxxxxxxxx/
>>
> 
> Thanks,
> Miquèl

cheers,
-roger