Re: [PATCH 2/5] mtd: rawnand: qcom: Add initial support for qspi nand

From: Md Sadre Alam <quic_mdalam@quicinc.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>,
	<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-mtd@lists.infradead.org>, <linux-arm-msm@vger.kernel.org>,
	<robh+dt@kernel.org>, <vigneshr@ti.com>, <richard@nod.at>,
	<agross@kernel.org>, <bjorn.andersson@linaro.org>,
	<quic_srichara@quicinc.com>, <qpic_varada@quicinc.com>,
	<quic_sjaganat@quicinc.com>, Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH 2/5] mtd: rawnand: qcom: Add initial support for qspi nand
Date: Tue, 28 Mar 2023 16:48:59 +0530	[thread overview]
Message-ID: <80ab07f8-fd7b-da08-f92e-20b76f5cefea@quicinc.com> (raw)
In-Reply-To: <20230327164949.465750a0@xps-13>

On 3/27/2023 8:19 PM, Miquel Raynal wrote:
> Hello,
> 
> + Mark
> 
> quic_mdalam@quicinc.com wrote on Mon, 27 Mar 2023 19:24:02 +0530:
> 
>> On 3/6/2023 8:08 PM, Miquel Raynal wrote:
>>> Hello,
>>>
>>> quic_mdalam@quicinc.com wrote on Mon, 6 Mar 2023 19:45:58 +0530:
>>>    
>>>> On 10/29/2020 2:37 PM, Boris Brezillon wrote:
>>>>> Hello,
>>>>>
>>>>> On Sat, 10 Oct 2020 11:01:39 +0530
>>>>> Md Sadre Alam <mdalam@codeaurora.org> wrote:
>>>>>    >>>> This change will add initial support for qspi (serial nand).
>>>>>>
>>>>>> QPIC Version v.2.0 onwards supports serial nand as well so this
>>>>>> change will initialize all required register to enable qspi (serial
>>>>>> nand).
>>>>>>
>>>>>> This change is supporting very basic functionality of qspi nand flash.
>>>>>>
>>>>>> 1. Reset device (Reset QSPI NAND device).
>>>>>>
>>>>>> 2. Device detection (Read id QSPI NAND device).
>>>>> Unfortunately, that's not going to work in the long term. You're
>>>>> basically hacking the raw NAND framework to make SPI NANDs fit. I do
>>>>> understand the rationale behind this decision (re-using the code for
>>>>> ECC and probably other things), but that's not going to work. So I'd
>>>>> recommend doing the following instead:
>>>>>
>>>>> 1/ implement a SPI-mem controller driver
>>>>> 2/ implement an ECC engine driver so the ECC logic can be shared
>>>>>       between the SPI controller and raw NAND controller drivers
>>>>> 3/ convert the raw NAND driver to the exec_op() interface (none of
>>>>>       this hack would have been possible if the driver was using the new
>>>>>       API)
>>>>>
>>>>> Regards,
>>>>>
>>>>> Boris
>>>>>    >>      Sorry for late reply, again started working on this feature support.  The QPIC v2 on wards there is serial nand support got added , its not a standard SPI controller
>>>>
>>>>       its QPIC controller having support for serial nand. All SPI related configuration done by QPIC hardware and its not exposed as SPI bus to the external world. Only based on
>>>>
>>>>       QPIC_SPI_CFG = 1, serial functionality will get selected. So that no need to implement as SPI-mem controller driver, since its not a SPI controller.
>>>>
>>>>      Please check the below diagram for top view of QPIC controller.
>>>
>>> One of the hard things in the Linux kernel is to make devices fit
>>> frameworks. This feature does not fit the raw NAND framework. It does
>>> not follow any of the conventions taken there. It is not gonna be
>>> accepted there. You need to expose spi-mem functionalities, even if the
>>> spi-proper features are not available. I believe your situation still
>>> fits the spi-mem abstraction.
>>>
>>> Thanks,
>>> Miquèl
>>
>>
>> I have started writing the driver code for SPI NAND. Please check the below design,
>> is this fine as per Boris suggestion.
> 
> Thanks.
> 
> 
>>             |------------------------|                      |------------------------------|                        |---------------------------------|
>>             |qcom spi nand driver    |--------------------->|common ECC engine driver      |<-----------------------|qcom raw nand driver             |
>>             |                        |                      |                              |                        |                                 |
>>             |                        |                      |drivers/mtd/nand/ecc-qcom.c   |                        |drivers/mtd/nand/raw/qcom_nand.c |
>>             |                        |                      |                              |                        |                                 |
>>             |drivers/spi/spi-qpic.c  |                      |------------------------------|                        |                                 |
>>             |                        |                      |common API file:              |                        |                                 |
>>             |                        |                      |common API: reset, read id,   |                        |                                 |
>>             |                        |--------------------->|erase, read page, write page, |<-----------------------|                                 |
>>             |------------------------|                      |bad block check etc.          |                        |                                 |
>>                                                             |                              |                        |---------------------------------|
>>                                                             |drivers/mtd/nand/raw/qpic_comm|
>>                                                             |    on.c                      |
>>                                                             |------------------------------|
>>
>>
>> Here ECC engine driver as separate file under (drivers/mtd/nand/ecc-qcom.c) and all
>> common APIs like reset, read id, erase, write page, read page, check block bad etc.
> 
> I'm not sure how generic these APIs are, please try to not put anything
> raw NAND or SPI-NAND specific in the common file. I don't want to see
> "if (raw)" or "if (spi)" conditions there.

  I agree , I will not try to put any condition like if(raw) or if(spi).

  I am doing in the below sequence.

  1) exec->op() in raw nand driver

  2) Move ecc related code to new ecc engine driver under ./drivers/mtd/nand/ecc-qcom.c
     and make raw nand driver to work with this ecc engine driver.

  3) Write separate SPI-mem driver under ./drivers/spi/spi-qpic.c

  4) Move common code under qpic_common.c under ./driver/mtd/nand/raw/qpic_common.c

> 
>> as separate file under drivers/mtd/nand/raw/qpic_common.c.APIs under ECC engine drivers
>> and qpic_common.c will be exported and used by spi-qpic.c driver (Serial NAND) and qcom_nand.c
>> (raw nand driver).
> 
> On my side, I don't have a strong opinion yet but it looks good to me.
> 
> Can you confirm you are considering switching to the ->exec_op() API in
> the raw NAND driver?

   Yes, first priority is switching to ->exec_op() API in raw nand driver.
   I have already started and sent couple of patches, will address latest comment
   send again.

> 
> Thanks,
> Miquèl

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