Received-SPF: pass (google.com: domain of linux-kernel+bounces-190148-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Esben Haabendal <esben@geanix.com>
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Tudor Ambarus <tudor.ambarus@linaro.org>,  Pratyush Yadav
 <pratyush@kernel.org>,  Michael Walle <mwalle@kernel.org>,
  linux-kernel@vger.kernel.org,  linux-mtd@lists.infradead.org
Subject: Re: [PATCH] memory: fsl_ifc: Make FSL_IFC config visible and
 selectable
In-Reply-To: <c045f1a4-9ddf-4c53-a69b-22ceb68a1ce8@kernel.org> (Krzysztof
	Kozlowski's message of "Mon, 27 May 2024 09:19:55 +0200")
References: <20240523-fsl-ifc-config-v1-1-6eff73bdc7e6@geanix.com>
	<979fd913-050b-445d-9ca8-0ec6906ce3ea@kernel.org>
	<87cypc38gu.fsf@geanix.com>
	<9a7f73f4-f5dc-4342-855b-08df6a839bb5@kernel.org>
	<87le3zoatn.fsf@geanix.com>
	<6c166ad5-8004-4bc4-9107-a47ba9a72161@kernel.org>
	<87ttijaglp.fsf@geanix.com>
	<c045f1a4-9ddf-4c53-a69b-22ceb68a1ce8@kernel.org>
Date: Mon, 27 May 2024 09:35:05 +0200
Message-ID: <87msobaes6.fsf@geanix.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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