[PATCH v2 0/2] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[PATCH v2 0/2] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Esben Haabendal]

While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
driver selects FSL_IFC automatically, we need the CONFIG_FSL_IFC option to
be selectable for platforms using fsl_ifc with NOR flash.
    
Fixes: ea0c0ad6b6eb ("memory: Enable compile testing for most of the drivers")
To: Krzysztof Kozlowski <krzk@kernel.org>
To: Tudor Ambarus <tudor.ambarus@linaro.org>
To: Pratyush Yadav <pratyush@kernel.org>
To: Michael Walle <mwalle@kernel.org>
To: Miquel Raynal <miquel.raynal@bootlin.com>
To: Richard Weinberger <richard@nod.at>
To: Vignesh Raghavendra <vigneshr@ti.com>
To: Michael Ellerman <mpe@ellerman.id.au>
To: Nicholas Piggin <npiggin@gmail.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Aneesh Kumar K.V <aneesh.kumar@kernel.org>
To: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-mtd@lists.infradead.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Esben Haabendal <esben@geanix.com>

Changes in v2:
- CONFIG_MTD_NAND_FSL_IFC depends on CONFIG_FSL_IFC instead of select.
- Refresh powerpc config snippet accordingly.
- Link to v1: https://lore.kernel.org/r/20240523-fsl-ifc-config-v1-1-6eff73bdc7e6@geanix.com

---
Esben Haabendal (2):
      memory: fsl_ifc: Make FSL_IFC config visible and selectable
      powerpc/configs: Update defconfig with now user-visible CONFIG_FSL_IFC

 arch/powerpc/configs/85xx-hw.config | 2 ++
 drivers/memory/Kconfig              | 2 +-
 drivers/mtd/nand/raw/Kconfig        | 3 +--
 3 files changed, 4 insertions(+), 3 deletions(-)
---
base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
change-id: 20240523-fsl-ifc-config-c877902b297e

Best regards,
-- 
Esben Haabendal <esben@geanix.com>

[PATCH v2 1/2] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Esben Haabendal]

While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
driver selects FSL_IFC automatically, we need the CONFIG_FSL_IFC option to
be selectable for platforms using fsl_ifc with NOR flash.

Fixes: ea0c0ad6b6eb ("memory: Enable compile testing for most of the drivers")
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/memory/Kconfig       | 2 +-
 drivers/mtd/nand/raw/Kconfig | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 8efdd1f97139..c82d8d8a16ea 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -167,7 +167,7 @@ config FSL_CORENET_CF
 	  represents a coherency violation.
 
 config FSL_IFC
-	bool "Freescale IFC driver" if COMPILE_TEST
+	bool "Freescale IFC driver"
 	depends on FSL_SOC || ARCH_LAYERSCAPE || SOC_LS1021A || COMPILE_TEST
 	depends on HAS_IOMEM
 
diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index cbf8ae85e1ae..614257308516 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -234,8 +234,7 @@ config MTD_NAND_FSL_IFC
 	tristate "Freescale IFC NAND controller"
 	depends on FSL_SOC || ARCH_LAYERSCAPE || SOC_LS1021A || COMPILE_TEST
 	depends on HAS_IOMEM
-	select FSL_IFC
-	select MEMORY
+	depends on FSL_IFC
 	help
 	  Various Freescale chips e.g P1010, include a NAND Flash machine
 	  with built-in hardware ECC capabilities.

-- 
2.45.1

Re: [PATCH v2 1/2] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Miquel Raynal]

Hi Esben,

esben@geanix.com wrote on Tue, 28 May 2024 14:28:52 +0200:

> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
> driver selects FSL_IFC automatically, we need the CONFIG_FSL_IFC option to
> be selectable for platforms using fsl_ifc with NOR flash.
> 
> Fixes: ea0c0ad6b6eb ("memory: Enable compile testing for most of the drivers")
> Signed-off-by: Esben Haabendal <esben@geanix.com>

Looks good to me.

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH v2 1/2] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Christophe Leroy]

Le 28/05/2024 à 14:28, Esben Haabendal a écrit :
> [Vous ne recevez pas souvent de courriers de esben@geanix.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
> driver selects FSL_IFC automatically, we need the CONFIG_FSL_IFC option to
> be selectable for platforms using fsl_ifc with NOR flash.

I don't understand.

Shall I understand :

While use of fsl_ifc driver with NAND flash is fine as the fsl_ifc_nand 
driver selects FSL_IFC automatically, ....

or

..., as the fsl_ifc_nand driver selects FSL_IFC automatically we need 
the CONFIG_FSL_IFC option to be selectable for platforms using fsl_ifc 
with NOR flash



I'm fine with the fact that you want to be able to select it when you 
use NOR flashes, allthough I can't see why, but why do you need to 
change the "select" to a "depend" ? You should be able to leave it as a 
"select" in which case patch 2 wouldn't be necessary.

Christophe



> 
> Fixes: ea0c0ad6b6eb ("memory: Enable compile testing for most of the drivers")
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>   drivers/memory/Kconfig       | 2 +-
>   drivers/mtd/nand/raw/Kconfig | 3 +--
>   2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index 8efdd1f97139..c82d8d8a16ea 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -167,7 +167,7 @@ config FSL_CORENET_CF
>            represents a coherency violation.
> 
>   config FSL_IFC
> -       bool "Freescale IFC driver" if COMPILE_TEST
> +       bool "Freescale IFC driver"
>          depends on FSL_SOC || ARCH_LAYERSCAPE || SOC_LS1021A || COMPILE_TEST
>          depends on HAS_IOMEM
> 
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index cbf8ae85e1ae..614257308516 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -234,8 +234,7 @@ config MTD_NAND_FSL_IFC
>          tristate "Freescale IFC NAND controller"
>          depends on FSL_SOC || ARCH_LAYERSCAPE || SOC_LS1021A || COMPILE_TEST
>          depends on HAS_IOMEM
> -       select FSL_IFC
> -       select MEMORY
> +       depends on FSL_IFC
>          help
>            Various Freescale chips e.g P1010, include a NAND Flash machine
>            with built-in hardware ECC capabilities.
> 
> --
> 2.45.1
> 

Re: [PATCH v2 1/2] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Krzysztof Kozlowski]

On 28/05/2024 15:15, Christophe Leroy wrote:
> 
> 
> Le 28/05/2024 à 14:28, Esben Haabendal a écrit :
>> [Vous ne recevez pas souvent de courriers de esben@geanix.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>
>> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
>> driver selects FSL_IFC automatically, we need the CONFIG_FSL_IFC option to
>> be selectable for platforms using fsl_ifc with NOR flash.
> 
> I don't understand.
> 
> Shall I understand :
> 
> While use of fsl_ifc driver with NAND flash is fine as the fsl_ifc_nand 
> driver selects FSL_IFC automatically, ....
> 
> or
> 
> ..., as the fsl_ifc_nand driver selects FSL_IFC automatically we need 
> the CONFIG_FSL_IFC option to be selectable for platforms using fsl_ifc 
> with NOR flash
> 
> 
> 
> I'm fine with the fact that you want to be able to select it when you 
> use NOR flashes, 

But users are not fine... their memory won't work if they cannot choose
it (if you meant select=choose).

> allthough I can't see why, but why do you need to 
> change the "select" to a "depend" ? You should be able to leave it as a 

As explained in v1: user-visible symbols should not be selected. Nothing
odd here.

> "select" in which case patch 2 wouldn't be necessary.


Best regards,
Krzysztof

Re: [PATCH v2 1/2] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Esben Haabendal]

Krzysztof Kozlowski <krzk@kernel.org> writes:

> On 28/05/2024 15:15, Christophe Leroy wrote:
>> Le 28/05/2024 à 14:28, Esben Haabendal a écrit :
>>> [Vous ne recevez pas souvent de courriers de esben@geanix.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
>>> driver selects FSL_IFC automatically, we need the CONFIG_FSL_IFC option to
>>> be selectable for platforms using fsl_ifc with NOR flash.
>> 
>> I don't understand.
>> 
>> Shall I understand :
>> 
>> While use of fsl_ifc driver with NAND flash is fine as the fsl_ifc_nand 
>> driver selects FSL_IFC automatically, ....
>> 
>> or
>> 
>> ..., as the fsl_ifc_nand driver selects FSL_IFC automatically we need 
>> the CONFIG_FSL_IFC option to be selectable for platforms using fsl_ifc 
>> with NOR flash
>> 
>> 
>> 
>> I'm fine with the fact that you want to be able to select it when you 
>> use NOR flashes, 
>
> But users are not fine... their memory won't work if they cannot choose
> it (if you meant select=choose).

Exactly. The IFC controller supports both NAND and NOR flashes. Since
commit ea0c0ad6b6eb ("memory: Enable compile testing for most of the drivers"),
it has not been possible to use IFC controller for NOR flashes without
selecting the IFC NAND driver (CONFIG_MTD_NAND_FSL_IFC), which is
obviously not how it is supposed to be.

/Esben

[PATCH v2 2/2] powerpc/configs: Update defconfig with now user-visible CONFIG_FSL_IFC

[Esben Haabendal]

With CONFIG_FSL_IFC now being user-visible, and thus changed from a select
to depends in CONFIG_MTD_NAND_FSL_IFC, the dependencies needs to be
selected in config snippets.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 arch/powerpc/configs/85xx-hw.config | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/configs/85xx-hw.config b/arch/powerpc/configs/85xx-hw.config
index 524db76f47b7..8aff83217397 100644
--- a/arch/powerpc/configs/85xx-hw.config
+++ b/arch/powerpc/configs/85xx-hw.config
@@ -24,6 +24,7 @@ CONFIG_FS_ENET=y
 CONFIG_FSL_CORENET_CF=y
 CONFIG_FSL_DMA=y
 CONFIG_FSL_HV_MANAGER=y
+CONFIG_FSL_IFC=y
 CONFIG_FSL_PQ_MDIO=y
 CONFIG_FSL_RIO=y
 CONFIG_FSL_XGMAC_MDIO=y
@@ -58,6 +59,7 @@ CONFIG_INPUT_FF_MEMLESS=m
 CONFIG_MARVELL_PHY=y
 CONFIG_MDIO_BUS_MUX_GPIO=y
 CONFIG_MDIO_BUS_MUX_MMIOREG=y
+CONFIG_MEMORY=y
 CONFIG_MMC_SDHCI_OF_ESDHC=y
 CONFIG_MMC_SDHCI_PLTFM=y
 CONFIG_MMC_SDHCI=y

-- 
2.45.1

Re: [PATCH v2 2/2] powerpc/configs: Update defconfig with now user-visible CONFIG_FSL_IFC

[Krzysztof Kozlowski]

On 28/05/2024 14:28, Esben Haabendal wrote:
> With CONFIG_FSL_IFC now being user-visible, and thus changed from a select
> to depends in CONFIG_MTD_NAND_FSL_IFC, the dependencies needs to be
> selected in config snippets.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  arch/powerpc/configs/85xx-hw.config | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/powerpc/configs/85xx-hw.config b/arch/powerpc/configs/85xx-hw.config
> index 524db76f47b7..8aff83217397 100644
> --- a/arch/powerpc/configs/85xx-hw.config
> +++ b/arch/powerpc/configs/85xx-hw.config
> @@ -24,6 +24,7 @@ CONFIG_FS_ENET=y
>  CONFIG_FSL_CORENET_CF=y
>  CONFIG_FSL_DMA=y
>  CONFIG_FSL_HV_MANAGER=y
> +CONFIG_FSL_IFC=y

Does not look like placed according to config order. This is not
alphabetically sorted, but as Kconfig creates it (make savedefconfig).

>  CONFIG_FSL_PQ_MDIO=y
>  CONFIG_FSL_RIO=y


You also missed to update second defconfig - arm64.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] powerpc/configs: Update defconfig with now user-visible CONFIG_FSL_IFC

[Esben Haabendal]

Krzysztof Kozlowski <krzk@kernel.org> writes:

> On 28/05/2024 14:28, Esben Haabendal wrote:
>> With CONFIG_FSL_IFC now being user-visible, and thus changed from a select
>> to depends in CONFIG_MTD_NAND_FSL_IFC, the dependencies needs to be
>> selected in config snippets.
>> 
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>>  arch/powerpc/configs/85xx-hw.config | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/arch/powerpc/configs/85xx-hw.config b/arch/powerpc/configs/85xx-hw.config
>> index 524db76f47b7..8aff83217397 100644
>> --- a/arch/powerpc/configs/85xx-hw.config
>> +++ b/arch/powerpc/configs/85xx-hw.config
>> @@ -24,6 +24,7 @@ CONFIG_FS_ENET=y
>>  CONFIG_FSL_CORENET_CF=y
>>  CONFIG_FSL_DMA=y
>>  CONFIG_FSL_HV_MANAGER=y
>> +CONFIG_FSL_IFC=y
>
> Does not look like placed according to config order.

Correct.

> This is not alphabetically sorted, but as Kconfig creates it (make
> savedefconfig).

Are you sure about this?

It looks very much alphabetically sorted, with only two "errors"

$ diff -u 85xx-hw.config 85xx-hw.config.sorted 
--- 85xx-hw.config      2024-05-28 15:05:44.665354428 +0200
+++ 85xx-hw.config.sorted       2024-05-28 15:05:56.102019081 +0200
@@ -15,8 +15,8 @@
 CONFIG_DMADEVICES=y
 CONFIG_E1000E=y
 CONFIG_E1000=y
-CONFIG_EDAC=y
 CONFIG_EDAC_MPC85XX=y
+CONFIG_EDAC=y
 CONFIG_EEPROM_AT24=y
 CONFIG_EEPROM_LEGACY=y
 CONFIG_FB_FSL_DIU=y
@@ -71,10 +71,10 @@
 CONFIG_MTD_CMDLINE_PARTS=y
 CONFIG_MTD_NAND_FSL_ELBC=y
 CONFIG_MTD_NAND_FSL_IFC=y
-CONFIG_MTD_RAW_NAND=y
 CONFIG_MTD_PHYSMAP_OF=y
 CONFIG_MTD_PHYSMAP=y
 CONFIG_MTD_PLATRAM=y
+CONFIG_MTD_RAW_NAND=y
 CONFIG_MTD_SPI_NOR=y
 CONFIG_NETDEVICES=y
 CONFIG_NVRAM=y

I don't think that this file has ever been Kconfig sorted since it was
created back in ancient times.

And as it is merged with other config snippets using merge_into_defconfig
function. I have no idea how to use savedefconfig to maintain such a snippet.
It would require doing the reverse of the merge_into_defconfig.

>>  CONFIG_FSL_PQ_MDIO=y
>>  CONFIG_FSL_RIO=y
>
> You also missed to update second defconfig - arm64.

Argh. I thought I checked, and it did not need any changes. But it needs
to have CONFIG_FSL_IFC=y added.

I will add that for v3.

/Esben

Re: [PATCH v2 2/2] powerpc/configs: Update defconfig with now user-visible CONFIG_FSL_IFC

[Krzysztof Kozlowski]

On 28/05/2024 15:33, Esben Haabendal wrote:
> Krzysztof Kozlowski <krzk@kernel.org> writes:
> 
>> On 28/05/2024 14:28, Esben Haabendal wrote:
>>> With CONFIG_FSL_IFC now being user-visible, and thus changed from a select
>>> to depends in CONFIG_MTD_NAND_FSL_IFC, the dependencies needs to be
>>> selected in config snippets.
>>>
>>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>>> ---
>>>  arch/powerpc/configs/85xx-hw.config | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/powerpc/configs/85xx-hw.config b/arch/powerpc/configs/85xx-hw.config
>>> index 524db76f47b7..8aff83217397 100644
>>> --- a/arch/powerpc/configs/85xx-hw.config
>>> +++ b/arch/powerpc/configs/85xx-hw.config
>>> @@ -24,6 +24,7 @@ CONFIG_FS_ENET=y
>>>  CONFIG_FSL_CORENET_CF=y
>>>  CONFIG_FSL_DMA=y
>>>  CONFIG_FSL_HV_MANAGER=y
>>> +CONFIG_FSL_IFC=y
>>
>> Does not look like placed according to config order.
> 
> Correct.
> 
>> This is not alphabetically sorted, but as Kconfig creates it (make
>> savedefconfig).
> 
> Are you sure about this?
> 
> It looks very much alphabetically sorted, with only two "errors"

Uh, ok, in general this is savedefconfig-sorted, but of course some
platforms can choose other way. At least all arm and arm64 are
savedefconfig-sorted.

Follow whatever is there already, so in this case alphabetical.

...

> 
>>>  CONFIG_FSL_PQ_MDIO=y
>>>  CONFIG_FSL_RIO=y
>>
>> You also missed to update second defconfig - arm64.
> 
> Argh. I thought I checked, and it did not need any changes. But it needs
> to have CONFIG_FSL_IFC=y added.
> 
> I will add that for v3.
> 
> /Esben

Best regards,
Krzysztof

Re: [PATCH v2 2/2] powerpc/configs: Update defconfig with now user-visible CONFIG_FSL_IFC

[Michael Ellerman]

Esben Haabendal <esben@geanix.com> writes:
> Krzysztof Kozlowski <krzk@kernel.org> writes:
>
>> On 28/05/2024 14:28, Esben Haabendal wrote:
>>> With CONFIG_FSL_IFC now being user-visible, and thus changed from a select
>>> to depends in CONFIG_MTD_NAND_FSL_IFC, the dependencies needs to be
>>> selected in config snippets.
>>> 
>>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>>> ---
>>>  arch/powerpc/configs/85xx-hw.config | 2 ++
>>>  1 file changed, 2 insertions(+)
>>> 
>>> diff --git a/arch/powerpc/configs/85xx-hw.config b/arch/powerpc/configs/85xx-hw.config
>>> index 524db76f47b7..8aff83217397 100644
>>> --- a/arch/powerpc/configs/85xx-hw.config
>>> +++ b/arch/powerpc/configs/85xx-hw.config
>>> @@ -24,6 +24,7 @@ CONFIG_FS_ENET=y
>>>  CONFIG_FSL_CORENET_CF=y
>>>  CONFIG_FSL_DMA=y
>>>  CONFIG_FSL_HV_MANAGER=y
>>> +CONFIG_FSL_IFC=y
>>
>> Does not look like placed according to config order.
>
> Correct.
>
>> This is not alphabetically sorted, but as Kconfig creates it (make
>> savedefconfig).
>
> Are you sure about this?
>
> It looks very much alphabetically sorted, with only two "errors"
>
> $ diff -u 85xx-hw.config 85xx-hw.config.sorted 
> --- 85xx-hw.config      2024-05-28 15:05:44.665354428 +0200
> +++ 85xx-hw.config.sorted       2024-05-28 15:05:56.102019081 +0200
> @@ -15,8 +15,8 @@
>  CONFIG_DMADEVICES=y
>  CONFIG_E1000E=y
>  CONFIG_E1000=y
> -CONFIG_EDAC=y
>  CONFIG_EDAC_MPC85XX=y
> +CONFIG_EDAC=y
>  CONFIG_EEPROM_AT24=y
>  CONFIG_EEPROM_LEGACY=y
>  CONFIG_FB_FSL_DIU=y
> @@ -71,10 +71,10 @@
>  CONFIG_MTD_CMDLINE_PARTS=y
>  CONFIG_MTD_NAND_FSL_ELBC=y
>  CONFIG_MTD_NAND_FSL_IFC=y
> -CONFIG_MTD_RAW_NAND=y
>  CONFIG_MTD_PHYSMAP_OF=y
>  CONFIG_MTD_PHYSMAP=y
>  CONFIG_MTD_PLATRAM=y
> +CONFIG_MTD_RAW_NAND=y
>  CONFIG_MTD_SPI_NOR=y
>  CONFIG_NETDEVICES=y
>  CONFIG_NVRAM=y
>
> I don't think that this file has ever been Kconfig sorted since it was
> created back in ancient times.
>
> And as it is merged with other config snippets using merge_into_defconfig
> function. I have no idea how to use savedefconfig to maintain such a snippet.
> It would require doing the reverse of the merge_into_defconfig.

Right. This is a config fragment, not a full config, so it's not managed
with savedefconfig.

Alphabetical order is preferable when adding new symbols.

cheers