Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Mon, 2 Oct 2023 11:30:17 +0200
From:   Miquel Raynal <miquel.raynal@bootlin.com>
To:     Nikita Shubin <nikita.shubin@maquefel.me>
Cc:     Rouven Czerwinski <r.czerwinski@pengutronix.de>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Linus Walleij <linus.walleij@linaro.org>,
        Jean Delvare <jdelvare@suse.de>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Christophe Kerello <christophe.kerello@foss.st.com>,
        Chris Packham <chris.packham@alliedtelesis.co.nz>,
        Johan Jonker <jbx6244@gmail.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org
Subject: Re: [RFC PATCH] mtd: nand: add support for ts72xx
Message-ID: <20231002113017.481aae31@xps-13>
In-Reply-To: <8bbe66a23eb5c8a2404b72d754b1bcb6f4d23867.camel@maquefel.me>
References: <20230927141532.25525-1-nikita.shubin@maquefel.me>
        <20230927172947.33106fe2@xps-13>
        <8bbe66a23eb5c8a2404b72d754b1bcb6f4d23867.camel@maquefel.me>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Nikita,

> ```
> # modprobe technologic-nand-controller
> nand: device found, Manufacturer ID: 0xec, Chip ID: 0xf1
> nand: Samsung NAND 128MiB 3,3V 8-bit
> nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
> Scanning device for bad blocks
> Bad eraseblock 137 at 0x000001120000
> Bad eraseblock 310 at 0x0000026c0000
> 3 fixed-partitions partitions found on MTD device 60000000.nand-
> controller
> Creating 3 MTD partitions on "60000000.nand-controller":
> 0x000000000000-0x000000020000 : "TS-BOOTROM"
> 0x000000020000-0x000007d20000 : "Linux"
> 0x000007d20000-0x000008000000 : "RedBoot"
> ```
>=20
> This looks like Samsung K9F1G08U0D or K9F1G08U0B.
>=20
> And the patch above totally solves my issues with READCACHE - thank
> you!
>=20

Great!

> > If you are using a Micron chip that specifies not supporting ECC
> > correction together with sequential cache reads, then it's a problem
> > that we will solve soon (you can hardcode
> > chip->controller->supported_op.cont_read to 0);
> >=20
> > Otherwise we can discuss it.
> >  =20
> > > - legacy wait implementation had no delays - only timeout and if i
> > > set=20
> > > =C2=A0 readb_poll_timeout() to something reasonable i experience speed
> > > degradation=20
> > > =C2=A0 on mtd_speedtest, can it be left as 0 ? =20
> >=20
> > Looks like the legacy implementation used cond_resched(). The delay
> > needs to be observed before first checking for the status, it's a
> > delay
> > that is described in the spec, if you get the status before you might
> > just not see a consistent value. I believe that is worth taking into
> > account in your implementation below (don't wait then delay, it's
> > not relevant). Can you share the values you've tried and the
> > performances you've got? =20
>=20
> The numbers are pretty stable, so indeed legacy is a bit faster,
> followed up by new one with zero interval.

...

> Legacy speedtest (old version of nand controller):

> mtd_speedtest: eraseblock write speed is 3793 KiB/s
> mtd_speedtest: eraseblock read speed is 3567 KiB/s
> mtd_speedtest: page write speed is 3682 KiB/s
> mtd_speedtest: page read speed is 3488 KiB/s
> mtd_speedtest: 2 page write speed is 3724 KiB/s
> mtd_speedtest: 2 page read speed is 3521 KiB/s
> mtd_speedtest: erase speed is 198709 KiB/s

...
=20
> New version with interval zero:

> mtd_speedtest: eraseblock write speed is 3685 KiB/s
> mtd_speedtest: eraseblock read speed is 3517 KiB/s
> mtd_speedtest: page write speed is 3592 KiB/s
> mtd_speedtest: page read speed is 3444 KiB/s
> mtd_speedtest: 2 page write speed is 3608 KiB/s
> mtd_speedtest: 2 page read speed is 3475 KiB/s
> mtd_speedtest: erase speed is 194499 KiB/s

Looks almost the same as above, I believe the difference is just noise
between measurements.

...

> New version with interval 100:

> mtd_speedtest: eraseblock write speed is 2722 KiB/s
> mtd_speedtest: eraseblock read speed is 2175 KiB/s
> mtd_speedtest: page write speed is 2598 KiB/s
> mtd_speedtest: page read speed is 2070 KiB/s
> mtd_speedtest: 2 page write speed is 2627 KiB/s
> mtd_speedtest: 2 page read speed is 2106 KiB/s
> mtd_speedtest: erase speed is 175851 KiB/s

...

> Also providing version with zero interval and "if (instr->delay_ns)"
> dropped - it's optional as far i understood:

It's not optional, sorry for the wrong comment, please keep it as it
is, knowing that it will be null after a wait_rdy.

> mtd_speedtest: eraseblock write speed is 3695 KiB/s
> mtd_speedtest: eraseblock read speed is 3532 KiB/s
> mtd_speedtest: page write speed is 3593 KiB/s
> mtd_speedtest: page read speed is 3457 KiB/s
> mtd_speedtest: 2 page write speed is 3640 KiB/s
> mtd_speedtest: 2 page read speed is 3488 KiB/s
> mtd_speedtest: erase speed is 195451 KiB/s

Thanks,
Miqu=C3=A8l