Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220404040153.1509966-1-gch981213@gmail.com> <20220404040153.1509966-3-gch981213@gmail.com>
 <20220404095937.20089db7@xps13> <CAJsYDVK6ya7FR90CtAjbpbF-_+c0GVnsKsN=1wYaoBFx=ysUtQ@mail.gmail.com>
 <20220404122824.63a1c857@xps13>
In-Reply-To: <20220404122824.63a1c857@xps13>
From:   Chuanhong Guo <gch981213@gmail.com>
Date:   Mon, 4 Apr 2022 18:54:17 +0800
Message-ID: <CAJsYDVJXH0MUB2wPydzqFgF8Y82HsUY7FLu1duCOCyVO-oXZqw@mail.gmail.com>
Subject: Re: [PATCH v2 2/5] spi: add driver for MTK SPI NAND Flash Interface
To:     Miquel Raynal <miquel.raynal@bootlin.com>
Cc:     linux-spi@vger.kernel.org, Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Roger Quadros <rogerq@kernel.org>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        Cai Huoqing <cai.huoqing@linux.dev>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Colin Ian King <colin.king@intel.com>,
        Wolfram Sang <wsa+renesas@sang-engineering.com>,
        Paul Cercueil <paul@crapouillou.net>,
        Pratyush Yadav <p.yadav@ti.com>, Yu Kuai <yukuai3@huawei.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-arm-kernel@lists.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@lists.infradead.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:NAND FLASH SUBSYSTEM" <linux-mtd@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi!

On Mon, Apr 4, 2022 at 6:28 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> [...]
> > > > +
> > > > +     // This driver ignores any ECC capability configured by user or
> > > > +     // requested by the nand chip because the BootROM and MTK bootloader
> > > > +     // expects the page format to be the exact one as calculated in
> > > > +     // setup_pagefmt.
> > >
> > > I don't like this :)
> > >
> > > I understand that the boot partition might have specific constraints,
> > > but other partitions (or if we don't use the NAND to boot?) should
> > > probably be usable with other ECC schemes.
> >
> > In this controller, the ECC step size is fixed and it can only change
> > ECC strength.
>
> That's fine.
>
> > The calculated ECC correction capability is the max
> > possible one supported by the controller.
> > I still want the default behavior to match the boot partition
> > requirement,
>
> That is okay, but that does not mean you can only support this one.
>
> > because we can't just tell end-users to customize
> > their dts by taking apart their device and figure out which flash
> > is used.
>
> They don't have to do so. In theory they should not request anything,
> the core would take care of all of that. But they can request specific
> values by using the DT and you must follow them in the driver.
>
> On his side the core is responsible of telling you which strength
> should be used otherwise and the driver is expected to use it.

The core provided ecc strength may be smaller than the
calculated one. e.g. A nand chip may only have a requirement
of 8/512bits ECC. But if it has a 2k+128 pagesize, this
controller can do 12/512bits ECC and the bootrom expects the
latter.

> You should take the user requirements first. If there are no
> user inputs, you should in theory look at the device's requirements.

I'll take the user requirements if there is one. If there isn't, I'll
follow the calculated strength instead of the device requirement
so that user doesn't have to specify a custom strength in dt.

> [...]
> > > > +static struct nand_ecc_engine_ops mtk_snfi_ecc_engine_ops = {
> > > > +     .init_ctx = mtk_snand_ecc_init_ctx,
> > > > +     .prepare_io_req = mtk_snand_ecc_prepare_io_req,
> > > > +     .finish_io_req = mtk_snand_ecc_finish_io_req,
> > >
> > > I believe you need to take care of the bounce buffer in the exit path?
> >
> > No. The buffer should be left there for non-ecc spi-mem operations.
>
> AFAIR you initialize the buffer in the ECC part, so if it must be used
> without ECC you should probably allocate it for the SPI controller.

I did. the setup_pagefmt is called once with the minimal page+oob size
in probe.

> In
> any way, you need to free that memory at some point (when removing the
> driver).

I was using the devm api for this allocation so kernel should take
care of that.

I'll change the DMA to use streamed API in the next version to avoid
an extra memory copy in reading, and the allocated buffer will be
freed in remove().

-- 
Regards,
Chuanhong Guo