[PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[PATCH] 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 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 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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
 

---
base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
change-id: 20240523-fsl-ifc-config-c877902b297e

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

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Krzysztof Kozlowski]

On 23/05/2024 15:58, Esben Haabendal wrote:
> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
> driver selects FSL_IFC automatically, we need the option to be selectable
> for platforms using fsl_ifc with NOR flash.
> 

Which driver is that? Which DTS?

Best regards,
Krzysztof

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Esben Haabendal]

Krzysztof Kozlowski <krzk@kernel.org> writes:

> On 23/05/2024 15:58, Esben Haabendal wrote:
>> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
>> driver selects FSL_IFC automatically, we need the option to be selectable
>> for platforms using fsl_ifc with NOR flash.
>
> Which driver is that?

This is drivers/memory/fsl_ifc.o driver. It is for Integrated Flash
Controller (IFC) from NXP. It is used in various Layerscape socs.

> Which DTS?

It is for "fsl,ifc" compatible devices.

/Esben

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Krzysztof Kozlowski]

On 23/05/2024 16:32, Esben Haabendal wrote:
> Krzysztof Kozlowski <krzk@kernel.org> writes:
> 
>> On 23/05/2024 15:58, Esben Haabendal wrote:
>>> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
>>> driver selects FSL_IFC automatically, we need the option to be selectable
>>> for platforms using fsl_ifc with NOR flash.
>>
>> Which driver is that?
> 
> This is drivers/memory/fsl_ifc.o driver. It is for Integrated Flash
> Controller (IFC) from NXP. It is used in various Layerscape socs.

? I know that, I mean the NOR flash working here.

> 
>> Which DTS?
> 
> It is for "fsl,ifc" compatible devices.

That's not a NOR flash.

Best regards,
Krzysztof

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Esben Haabendal]

Krzysztof Kozlowski <krzk@kernel.org> writes:

> On 23/05/2024 16:32, Esben Haabendal wrote:
>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>> 
>>> On 23/05/2024 15:58, Esben Haabendal wrote:
>>>> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
>>>> driver selects FSL_IFC automatically, we need the option to be selectable
>>>> for platforms using fsl_ifc with NOR flash.
>>>
>>> Which driver is that?
>> 
>> This is drivers/memory/fsl_ifc.o driver. It is for Integrated Flash
>> Controller (IFC) from NXP. It is used in various Layerscape socs.
>
> ? I know that, I mean the NOR flash working here.

Ah, sorry. The NOR flash I am using here is a Spansion S29AL016J.

>>> Which DTS?
>> 
>> It is for "fsl,ifc" compatible devices.
>
> That's not a NOR flash.

Nope.  The binding used for the NOR flash is "cfi-flash".

/Esben

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Krzysztof Kozlowski]

On 24/05/2024 10:47, Esben Haabendal wrote:
> Krzysztof Kozlowski <krzk@kernel.org> writes:
> 
>> On 23/05/2024 16:32, Esben Haabendal wrote:
>>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>>>
>>>> On 23/05/2024 15:58, Esben Haabendal wrote:
>>>>> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
>>>>> driver selects FSL_IFC automatically, we need the option to be selectable
>>>>> for platforms using fsl_ifc with NOR flash.
>>>>
>>>> Which driver is that?
>>>
>>> This is drivers/memory/fsl_ifc.o driver. It is for Integrated Flash
>>> Controller (IFC) from NXP. It is used in various Layerscape socs.
>>
>> ? I know that, I mean the NOR flash working here.
> 
> Ah, sorry. The NOR flash I am using here is a Spansion S29AL016J.
> 
>>>> Which DTS?
>>>
>>> It is for "fsl,ifc" compatible devices.
>>
>> That's not a NOR flash.
> 
> Nope.  The binding used for the NOR flash is "cfi-flash".

And now let's get back to my original question. I asked for driver, not
device, and for DTS not compatible.

Best regards,
Krzysztof

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Esben Haabendal]

Krzysztof Kozlowski <krzk@kernel.org> writes:

> On 24/05/2024 10:47, Esben Haabendal wrote:
>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>> 
>>> On 23/05/2024 16:32, Esben Haabendal wrote:
>>>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>>>>
>>>>> On 23/05/2024 15:58, Esben Haabendal wrote:
>>>>>> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
>>>>>> driver selects FSL_IFC automatically, we need the option to be selectable
>>>>>> for platforms using fsl_ifc with NOR flash.
>>>>>
>>>>> Which driver is that?
>>>>
>>>> This is drivers/memory/fsl_ifc.o driver. It is for Integrated Flash
>>>> Controller (IFC) from NXP. It is used in various Layerscape socs.
>>>
>>> ? I know that, I mean the NOR flash working here.
>> 
>> Ah, sorry. The NOR flash I am using here is a Spansion S29AL016J.
>> 
>>>>> Which DTS?
>>>>
>>>> It is for "fsl,ifc" compatible devices.
>>>
>>> That's not a NOR flash.
>> 
>> Nope.  The binding used for the NOR flash is "cfi-flash".
>
> And now let's get back to my original question. I asked for driver, not
> device, and for DTS not compatible.

You got me really confused now. I am not sure what you are asking me
for, and why.

I am sending a patch which changes to Kconfig for a memory controller
driver. The change is AFAICS quite similar to commit be34f45f0d4a
("memory: omap-gpmc: Make OMAP_GPMC config visible and selectable").

As for the NOR flash that in this situation is attached to the IFC
controller, it is (as mentioned) Spansion S29AL016J. It is handled by
the drivers/mtd/maps/physmap.o driver (CONFIG_MTD_PHYSMAP +
CONFIG_MTD_PHYSMAP_OF)

The DTS used to specify the NOR flash is

&ifc {
        status = "okay";
        #address-cells = <2>;
        #size-cells = <1>;
        ranges = <0x0 0x0 0x0 0x60000000 0x00200000>;

        nor_flash: nor@0,0 {
                #address-cells = <1>;
                #size-cells = <1>;
                compatible = "cfi-flash";
                reg = <0x0 0x0 0x200000>;
                bank-width = <2>;

                partition@0 {
                        reg = <0x000000 0x0f0000>;
                        label = "boot0";
                };
        };
};

So as mentioned, it is a quite standard "cfi-flash" compatible device
declaration.

Do you think there is a way that ti enable CONFIG_FSL_IFC for my
situation without the config being visible? Something like automatically
selecting CONFIG_FSL_IFC when CONFIG_MTD_PHYSMAP and
CONFIG_MTD_PHYSMAP_OF is enabled? Will that not include a risk of
pulling in CONFIG_FSL_IFC in some cases where it is not desired?

Something like

config MTD_PHYSMAP
	tristate "Flash device in physical memory map"
	depends on MTD_CFI || MTD_JEDECPROBE || MTD_ROM || MTD_RAM || MTD_LPDDR
        select FSL_IFC if SOC_LS1021A

But that looks like a road somewhere not so nice. The generic
MTD_PHYSMAP being littered with architecture specific selects.

Or is there something completely obvious (not for me then) that I am
missing?

/Esben

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Krzysztof Kozlowski]

On 27/05/2024 08:55, Esben Haabendal wrote:
> Krzysztof Kozlowski <krzk@kernel.org> writes:
> 
>> On 24/05/2024 10:47, Esben Haabendal wrote:
>>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>>>
>>>> On 23/05/2024 16:32, Esben Haabendal wrote:
>>>>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>>>>>
>>>>>> On 23/05/2024 15:58, Esben Haabendal wrote:
>>>>>>> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
>>>>>>> driver selects FSL_IFC automatically, we need the option to be selectable
>>>>>>> for platforms using fsl_ifc with NOR flash.
>>>>>>
>>>>>> Which driver is that?
>>>>>
>>>>> This is drivers/memory/fsl_ifc.o driver. It is for Integrated Flash
>>>>> Controller (IFC) from NXP. It is used in various Layerscape socs.
>>>>
>>>> ? I know that, I mean the NOR flash working here.
>>>
>>> Ah, sorry. The NOR flash I am using here is a Spansion S29AL016J.
>>>
>>>>>> Which DTS?
>>>>>
>>>>> It is for "fsl,ifc" compatible devices.
>>>>
>>>> That's not a NOR flash.
>>>
>>> Nope.  The binding used for the NOR flash is "cfi-flash".
>>
>> And now let's get back to my original question. I asked for driver, not
>> device, and for DTS not compatible.
> 
> You got me really confused now. I am not sure what you are asking me
> for, and why.
> 
> I am sending a patch which changes to Kconfig for a memory controller
> driver. The change is AFAICS quite similar to commit be34f45f0d4a
> ("memory: omap-gpmc: Make OMAP_GPMC config visible and selectable").
> 
> As for the NOR flash that in this situation is attached to the IFC
> controller, it is (as mentioned) Spansion S29AL016J. It is handled by
> the drivers/mtd/maps/physmap.o driver (CONFIG_MTD_PHYSMAP +
> CONFIG_MTD_PHYSMAP_OF)

Thanks.

> 
> The DTS used to specify the NOR flash is

Upstream DTS. I don't care about downstream.

> 
> &ifc {
>         status = "okay";
>         #address-cells = <2>;
>         #size-cells = <1>;
>         ranges = <0x0 0x0 0x0 0x60000000 0x00200000>;
> 
>         nor_flash: nor@0,0 {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 compatible = "cfi-flash";
>                 reg = <0x0 0x0 0x200000>;
>                 bank-width = <2>;
> 
>                 partition@0 {
>                         reg = <0x000000 0x0f0000>;
>                         label = "boot0";
>                 };
>         };
> };
> 
> So as mentioned, it is a quite standard "cfi-flash" compatible device
> declaration.
> 
> Do you think there is a way that ti enable CONFIG_FSL_IFC for my
> situation without the config being visible? Something like automatically
> selecting CONFIG_FSL_IFC when CONFIG_MTD_PHYSMAP and
> CONFIG_MTD_PHYSMAP_OF is enabled? Will that not include a risk of
> pulling in CONFIG_FSL_IFC in some cases where it is not desired?
> 
> Something like
> 
> config MTD_PHYSMAP
> 	tristate "Flash device in physical memory map"
> 	depends on MTD_CFI || MTD_JEDECPROBE || MTD_ROM || MTD_RAM || MTD_LPDDR
>         select FSL_IFC if SOC_LS1021A
> 
> But that looks like a road somewhere not so nice. The generic
> MTD_PHYSMAP being littered with architecture specific selects.
> 
> Or is there something completely obvious (not for me then) that I am
> missing?

If there is no user for such option, why changing it? To clarify:
whatever is out of tree, does not really exist...

Best regards,
Krzysztof

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Esben Haabendal]

Krzysztof Kozlowski <krzk@kernel.org> writes:

> On 27/05/2024 08:55, Esben Haabendal wrote:
>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>> 
>>> On 24/05/2024 10:47, Esben Haabendal wrote:
>>>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>>>>
>>>>> On 23/05/2024 16:32, Esben Haabendal wrote:
>>>>>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>>>>>>
>>>>>>> On 23/05/2024 15:58, Esben Haabendal wrote:
>>>>>>>> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
>>>>>>>> driver selects FSL_IFC automatically, we need the option to be selectable
>>>>>>>> for platforms using fsl_ifc with NOR flash.
>>>>>>>
>>>>>>> Which driver is that?
>>>>>>
>>>>>> This is drivers/memory/fsl_ifc.o driver. It is for Integrated Flash
>>>>>> Controller (IFC) from NXP. It is used in various Layerscape socs.
>>>>>
>>>>> ? I know that, I mean the NOR flash working here.
>>>>
>>>> Ah, sorry. The NOR flash I am using here is a Spansion S29AL016J.
>>>>
>>>>>>> Which DTS?
>>>>>>
>>>>>> It is for "fsl,ifc" compatible devices.
>>>>>
>>>>> That's not a NOR flash.
>>>>
>>>> Nope.  The binding used for the NOR flash is "cfi-flash".
>>>
>>> And now let's get back to my original question. I asked for driver, not
>>> device, and for DTS not compatible.
>> 
>> You got me really confused now. I am not sure what you are asking me
>> for, and why.
>> 
>> I am sending a patch which changes to Kconfig for a memory controller
>> driver. The change is AFAICS quite similar to commit be34f45f0d4a
>> ("memory: omap-gpmc: Make OMAP_GPMC config visible and selectable").
>> 
>> As for the NOR flash that in this situation is attached to the IFC
>> controller, it is (as mentioned) Spansion S29AL016J. It is handled by
>> the drivers/mtd/maps/physmap.o driver (CONFIG_MTD_PHYSMAP +
>> CONFIG_MTD_PHYSMAP_OF)
>
> Thanks.
>
>> 
>> The DTS used to specify the NOR flash is
>
> Upstream DTS. I don't care about downstream.

Upstream DTS is arch/arm/boot/dts/nxp/ls/ls1021a.dtsi

Which has

                ifc: memory-controller@1530000 {
                        compatible = "fsl,ifc";
                        reg = <0x0 0x1530000 0x0 0x10000>;
                        interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
                        status = "disabled";
                };

The NOR flash is not specified in upstream DTS, as it is board and not
SoC specific.

I assume it is not considered bad practise to keep downstream DTS out of
tree.

>> &ifc {
>>         status = "okay";
>>         #address-cells = <2>;
>>         #size-cells = <1>;
>>         ranges = <0x0 0x0 0x0 0x60000000 0x00200000>;
>> 
>>         nor_flash: nor@0,0 {
>>                 #address-cells = <1>;
>>                 #size-cells = <1>;
>>                 compatible = "cfi-flash";
>>                 reg = <0x0 0x0 0x200000>;
>>                 bank-width = <2>;
>> 
>>                 partition@0 {
>>                         reg = <0x000000 0x0f0000>;
>>                         label = "boot0";
>>                 };
>>         };
>> };
>> 
>> So as mentioned, it is a quite standard "cfi-flash" compatible device
>> declaration.
>> 
>> Do you think there is a way that ti enable CONFIG_FSL_IFC for my
>> situation without the config being visible? Something like automatically
>> selecting CONFIG_FSL_IFC when CONFIG_MTD_PHYSMAP and
>> CONFIG_MTD_PHYSMAP_OF is enabled? Will that not include a risk of
>> pulling in CONFIG_FSL_IFC in some cases where it is not desired?
>> 
>> Something like
>> 
>> config MTD_PHYSMAP
>> 	tristate "Flash device in physical memory map"
>> 	depends on MTD_CFI || MTD_JEDECPROBE || MTD_ROM || MTD_RAM || MTD_LPDDR
>>         select FSL_IFC if SOC_LS1021A
>> 
>> But that looks like a road somewhere not so nice. The generic
>> MTD_PHYSMAP being littered with architecture specific selects.
>> 
>> Or is there something completely obvious (not for me then) that I am
>> missing?
>
> If there is no user for such option, why changing it? To clarify:
> whatever is out of tree, does not really exist...

I don't have anything out of tree other than the DTS. Is the kernel only
supposed to support in-tree DTS? I thought that keeping (dtb)
device-tree in bootloader was very much considered a supported setup.

Or are you saying that I should send patches with all downstream
DTS files, even if it is for hardware that is not generally available?

/Esben

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Esben Haabendal]

Esben Haabendal <esben@geanix.com> writes:

> Krzysztof Kozlowski <krzk@kernel.org> writes:
>
>> On 27/05/2024 08:55, Esben Haabendal wrote:
>>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>>> 
>>>> On 24/05/2024 10:47, Esben Haabendal wrote:
>>>>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>>>>>
>>>>>> On 23/05/2024 16:32, Esben Haabendal wrote:
>>>>>>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>>>>>>>
>>>>>>>> On 23/05/2024 15:58, Esben Haabendal wrote:
>>>>>>>>> While use of fsl_ifc driver with NAND flash is fine, as the fsl_ifc_nand
>>>>>>>>> driver selects FSL_IFC automatically, we need the option to be selectable
>>>>>>>>> for platforms using fsl_ifc with NOR flash.
>>>>>>>>
>>>>>>>> Which driver is that?
>>>>>>>
>>>>>>> This is drivers/memory/fsl_ifc.o driver. It is for Integrated Flash
>>>>>>> Controller (IFC) from NXP. It is used in various Layerscape socs.
>>>>>>
>>>>>> ? I know that, I mean the NOR flash working here.
>>>>>
>>>>> Ah, sorry. The NOR flash I am using here is a Spansion S29AL016J.
>>>>>
>>>>>>>> Which DTS?
>>>>>>>
>>>>>>> It is for "fsl,ifc" compatible devices.
>>>>>>
>>>>>> That's not a NOR flash.
>>>>>
>>>>> Nope.  The binding used for the NOR flash is "cfi-flash".
>>>>
>>>> And now let's get back to my original question. I asked for driver, not
>>>> device, and for DTS not compatible.
>>> 
>>> You got me really confused now. I am not sure what you are asking me
>>> for, and why.
>>> 
>>> I am sending a patch which changes to Kconfig for a memory controller
>>> driver. The change is AFAICS quite similar to commit be34f45f0d4a
>>> ("memory: omap-gpmc: Make OMAP_GPMC config visible and selectable").
>>> 
>>> As for the NOR flash that in this situation is attached to the IFC
>>> controller, it is (as mentioned) Spansion S29AL016J. It is handled by
>>> the drivers/mtd/maps/physmap.o driver (CONFIG_MTD_PHYSMAP +
>>> CONFIG_MTD_PHYSMAP_OF)
>>
>> Thanks.
>>
>>> 
>>> The DTS used to specify the NOR flash is
>>
>> Upstream DTS. I don't care about downstream.
>
> Upstream DTS is arch/arm/boot/dts/nxp/ls/ls1021a.dtsi
>
> Which has
>
>                 ifc: memory-controller@1530000 {
>                         compatible = "fsl,ifc";
>                         reg = <0x0 0x1530000 0x0 0x10000>;
>                         interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
>                         status = "disabled";
>                 };
>
> The NOR flash is not specified in upstream DTS, as it is board and not
> SoC specific.
>
> I assume it is not considered bad practise to keep downstream DTS out of
> tree.
>
>>> &ifc {
>>>         status = "okay";
>>>         #address-cells = <2>;
>>>         #size-cells = <1>;
>>>         ranges = <0x0 0x0 0x0 0x60000000 0x00200000>;
>>> 
>>>         nor_flash: nor@0,0 {
>>>                 #address-cells = <1>;
>>>                 #size-cells = <1>;
>>>                 compatible = "cfi-flash";
>>>                 reg = <0x0 0x0 0x200000>;
>>>                 bank-width = <2>;
>>> 
>>>                 partition@0 {
>>>                         reg = <0x000000 0x0f0000>;
>>>                         label = "boot0";
>>>                 };
>>>         };
>>> };

Ok, I seem to still be confused as to what you want from me. If you are
saying that the kernel is not supposed to care about out-of-tree DTS
(and thereby any bootloader provided DTB), I would like to bring your
attention to arch/arm/boot/dts/nxp/ls/ls1021a-twr.dts in upstream:

&ifc {
        #address-cells = <2>;
        #size-cells = <1>;
        /* NOR Flash on board */
        ranges = <0x0 0x0 0x0 0x60000000 0x08000000>;
        status = "okay";

        nor@0,0 {
                #address-cells = <1>;
                #size-cells = <1>;
                compatible = "cfi-flash";
                reg = <0x0 0x0 0x8000000>;
                big-endian;
                bank-width = <2>;
                device-width = <1>;
        };
};

This is basically identical to what I am using, and it will have the
same problem. It requires CONFIG_FSL_IFC enabled, which is not possible
(without enabling CONFIG_MTD_NAND_FSL_IFC). This board does not have
NAND, so I don't think it is reasonable to require keeping support for
it enabled to be able to use NOR flash.

/Esben

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Krzysztof Kozlowski]

On 27/05/2024 09:47, Esben Haabendal wrote:
> 
> Ok, I seem to still be confused as to what you want from me. If you are
> saying that the kernel is not supposed to care about out-of-tree DTS
> (and thereby any bootloader provided DTB), I would like to bring your
> attention to arch/arm/boot/dts/nxp/ls/ls1021a-twr.dts in upstream:
> 
> &ifc {
>         #address-cells = <2>;
>         #size-cells = <1>;
>         /* NOR Flash on board */
>         ranges = <0x0 0x0 0x0 0x60000000 0x08000000>;
>         status = "okay";
> 
>         nor@0,0 {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 compatible = "cfi-flash";
>                 reg = <0x0 0x0 0x8000000>;
>                 big-endian;
>                 bank-width = <2>;
>                 device-width = <1>;
>         };
> };
> 

I don't understand why it took so many emails to answer that (my first)
question... Sounds good, however you did not update the existing select.
Drivers are not supposed to select user-visible symbols (leads to
issues), so you need to change it to depends and update defconfigs.




Best regards,
Krzysztof

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Esben Haabendal]

Krzysztof Kozlowski <krzk@kernel.org> writes:

> On 27/05/2024 09:47, Esben Haabendal wrote:
>> 
>> Ok, I seem to still be confused as to what you want from me. If you are
>> saying that the kernel is not supposed to care about out-of-tree DTS
>> (and thereby any bootloader provided DTB), I would like to bring your
>> attention to arch/arm/boot/dts/nxp/ls/ls1021a-twr.dts in upstream:
>> 
>> &ifc {
>>         #address-cells = <2>;
>>         #size-cells = <1>;
>>         /* NOR Flash on board */
>>         ranges = <0x0 0x0 0x0 0x60000000 0x08000000>;
>>         status = "okay";
>> 
>>         nor@0,0 {
>>                 #address-cells = <1>;
>>                 #size-cells = <1>;
>>                 compatible = "cfi-flash";
>>                 reg = <0x0 0x0 0x8000000>;
>>                 big-endian;
>>                 bank-width = <2>;
>>                 device-width = <1>;
>>         };
>> };
>> 
>
> I don't understand why it took so many emails to answer that (my first)
> question...

Because I did not understand the question. Primarely because I was (and
is) surprised that out-of-tree DTS is not supported. I was convinced
that out-of-tree DTS was the right way for hardware which is not
commonly available.

> Sounds good, however you did not update the existing select.
> Drivers are not supposed to select user-visible symbols (leads to
> issues), so you need to change it to depends and update defconfigs.

Do you wan this split into multiple commits, or a single commit changing
the Kconfig to make FSL_IFC user-visible, and changing select of it to
DEPENDS, and updating the related defconfig(s)?

/Esben

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Krzysztof Kozlowski]

On 28/05/2024 12:03, Esben Haabendal wrote:
> Krzysztof Kozlowski <krzk@kernel.org> writes:
> 
>> On 27/05/2024 09:47, Esben Haabendal wrote:
>>>
>>> Ok, I seem to still be confused as to what you want from me. If you are
>>> saying that the kernel is not supposed to care about out-of-tree DTS
>>> (and thereby any bootloader provided DTB), I would like to bring your
>>> attention to arch/arm/boot/dts/nxp/ls/ls1021a-twr.dts in upstream:
>>>
>>> &ifc {
>>>         #address-cells = <2>;
>>>         #size-cells = <1>;
>>>         /* NOR Flash on board */
>>>         ranges = <0x0 0x0 0x0 0x60000000 0x08000000>;
>>>         status = "okay";
>>>
>>>         nor@0,0 {
>>>                 #address-cells = <1>;
>>>                 #size-cells = <1>;
>>>                 compatible = "cfi-flash";
>>>                 reg = <0x0 0x0 0x8000000>;
>>>                 big-endian;
>>>                 bank-width = <2>;
>>>                 device-width = <1>;
>>>         };
>>> };
>>>
>>
>> I don't understand why it took so many emails to answer that (my first)
>> question...
> 
> Because I did not understand the question. Primarely because I was (and
> is) surprised that out-of-tree DTS is not supported. I was convinced
> that out-of-tree DTS was the right way for hardware which is not
> commonly available.

Even some non-GA hardware could, and IMHO should, be upstreamed, at
least some parts of it. This gives the user/upstreamer reason to do
changes. Otherwise you might get questions for contributions: why you
are doing and why this is worth?

Downstream or any fork is not really answer to such questions, because
they are allowed to make whatever stupid choices they want (not saying
it was done here, but in general), which should not be a reason to do
anything upstream. If downstream creates wrong DTS files, shall we
create wrong device drivers or bindings for them? No.


> 
>> Sounds good, however you did not update the existing select.
>> Drivers are not supposed to select user-visible symbols (leads to
>> issues), so you need to change it to depends and update defconfigs.
> 
> Do you wan this split into multiple commits, or a single commit changing
> the Kconfig to make FSL_IFC user-visible, and changing select of it to
> DEPENDS, and updating the related defconfig(s)?

One commit for Kconfigs (nand and memory), additional commits for
defconfigs, one per each arch, because defconfigs might go via arch tree.


Best regards,
Krzysztof

Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and selectable

[Esben Haabendal]

Krzysztof Kozlowski <krzk@kernel.org> writes:

> On 28/05/2024 12:03, Esben Haabendal wrote:
>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>> 
>>> On 27/05/2024 09:47, Esben Haabendal wrote:
>>>>
>>>> Ok, I seem to still be confused as to what you want from me. If you are
>>>> saying that the kernel is not supposed to care about out-of-tree DTS
>>>> (and thereby any bootloader provided DTB), I would like to bring your
>>>> attention to arch/arm/boot/dts/nxp/ls/ls1021a-twr.dts in upstream:
>>>>
>>>> &ifc {
>>>>         #address-cells = <2>;
>>>>         #size-cells = <1>;
>>>>         /* NOR Flash on board */
>>>>         ranges = <0x0 0x0 0x0 0x60000000 0x08000000>;
>>>>         status = "okay";
>>>>
>>>>         nor@0,0 {
>>>>                 #address-cells = <1>;
>>>>                 #size-cells = <1>;
>>>>                 compatible = "cfi-flash";
>>>>                 reg = <0x0 0x0 0x8000000>;
>>>>                 big-endian;
>>>>                 bank-width = <2>;
>>>>                 device-width = <1>;
>>>>         };
>>>> };
>>>>
>>>
>>> I don't understand why it took so many emails to answer that (my first)
>>> question...
>> 
>> Because I did not understand the question. Primarely because I was (and
>> is) surprised that out-of-tree DTS is not supported. I was convinced
>> that out-of-tree DTS was the right way for hardware which is not
>> commonly available.
>
> Even some non-GA hardware could, and IMHO should, be upstreamed, at
> least some parts of it. This gives the user/upstreamer reason to do
> changes. Otherwise you might get questions for contributions: why you
> are doing and why this is worth?
>
> Downstream or any fork is not really answer to such questions, because
> they are allowed to make whatever stupid choices they want (not saying
> it was done here, but in general), which should not be a reason to do
> anything upstream. If downstream creates wrong DTS files, shall we
> create wrong device drivers or bindings for them? No.

Of-course not. But to be fair, in this case, there has not been any new
bindings created, and no creative usage of existing bindings. Only
straight forward usage of existing drivers (IFC and physmap MTD driver),
which I am surprised no one else had seen the need to fix in upstream.

But do you/we really want to open the flood gates for DTS maintenance of
a gazillion embedded boards that only a few in the world have an
interest in? I suspect it would create quite a lot of patches to flow
through maintainers, with very litle benefit to most people. But for me
(as in the companies doing such these kind of embedded hardware), it is
a clear benefit to put upstream DTS files.

But what about boards where device-tree is created/modified dynamically
by bootloader? Is that also not really supported?

>>> Sounds good, however you did not update the existing select.
>>> Drivers are not supposed to select user-visible symbols (leads to
>>> issues), so you need to change it to depends and update defconfigs.
>> 
>> Do you wan this split into multiple commits, or a single commit changing
>> the Kconfig to make FSL_IFC user-visible, and changing select of it to
>> DEPENDS, and updating the related defconfig(s)?
>
> One commit for Kconfigs (nand and memory), additional commits for
> defconfigs, one per each arch, because defconfigs might go via arch tree.

Sounds good. I will send a v2 with that.

/Esben