Messages in this thread | | | Date | Thu, 5 Nov 2020 18:49:52 +0530 | From | Pratyush Yadav <> | Subject | Re: [PATCH 0/3] mtd: Make sure UBIFS does not do multi-pass page programming on flashes that don't support it |
| |
On 05/11/20 05:51PM, Vignesh Raghavendra wrote: > > > On 11/3/20 6:15 PM, Pratyush Yadav wrote: > > On 03/11/20 05:05PM, Vignesh Raghavendra wrote: > >> > >> > >> On 11/1/20 3:14 AM, Richard Weinberger wrote: > >>> On Tue, Oct 27, 2020 at 12:24 PM Pratyush Yadav <p.yadav@ti.com> wrote: > >>>>> [0] https://lore.kernel.org/linux-mtd/20201005153138.6437-1-p.yadav@ti.com/ > >>>> > >>>> Ping. Any comments on the series? > >>> > >>> From the UBIFS point of view I'd like to avoid as many device specific > >>> settings as possible. > >>> We check already for NOR flash, checking for NOR *and* SPI_NOR_NO_MULTI_PASS_PP > >>> feels a bit clumsy. > >>> > >>> Tudor, what do you think about SPI_NOR_NO_MULTI_PASS_PP? > >>> This kind of NOR seems to be a little NAND'ish. Maybe we can hide this detail > >>> in the mtd framework? > >>> > >> > >> Agree with Richard. I don't see need for SPI_NOR_NO_MULTI_PASS_PP. From > >> MTD point of view setting mtd->writesize to be equal to pagesize should > >> be enough. Its upto clients of MTD devices to ensure there is no multi > >> pass programming within a "writesize" block. > > > > That is what I initially thought too but then I realized that multi-pass > > programming is completely different from page-size programming. Instead > > of writing 4 bytes twice, you can zero out the entire page in one single > > operation. You would be compliant with the write size requirement but > > you still do multi-pass programming because you did not erase the page > > before this operation. > > > > Right... > > > It is also not completely correct to say the Cypress S28 flash has a > > write size of 256. You _can_ write one byte if you want. You just can't > > write to that page again without erasing it first. For example, if a > > file system only wants to write 128 bytes on a page, it can do so > > without having to write the whole page. It just needs to make sure it > > doesn't write to it again without erasing first. > > > > As per documentation: > mtd_info::writesize: "In case of ECC-ed NOR it is of ECC block size" > > This means, it is block on which ECC is calculated on ECC-ed NOR and > thus needs to be erased every time before being updated. > > Looking at flash datasheet, this seems to be 16 bytes. > > So mtd->writesize = 16 and not 256 (or pagesize)
Ok.
> Also, It does not imply length of data being written has to be multiple > of it. At least NAND subsystem does not seem to care that during writes > len < mtd->writesize[1].
Ok.
> > nor_erase_prepare() was written to handle quirks of some specific > > devices. Not every device starts filling zeroes from the end of a page. > > So we have device-specific code in UBIFS already. You will obviously > > need device-specific settings to have control over that code. > > > > UBIFS intends to be robust against rogue power cuts and therefore would > need to ensure some consistency during erase which explains flash > specific quirk here.
Yes. There is no arguing if this is needed. The only question is how to skip it on flashes that don't support doing this.
> > One might argue that we should move nor_erase_prepare() out of UBIFS. > > But requiring a flash to start erasing from the start of the page is a > > UBIFS-specific requirement. Other users of a flash might not care about > > it at all. > > > > Yes. But I don't see much harm done. > > > And so we have ourselves a bit of a conundrum. Adding > > SPI_NOR_NO_MULTI_PASS_PP is IMHO the least disruptive answer. If the > > file system wants to do multi-pass page programming on NOR flashes, how > > else do we tell it not to do it for this specific flash? > > > > I see don't see need for SPI_NOR_NO_MULTI_PASS_PP as > SPI_NOR_NO_MULTI_PASS_PP is implied within a ECC block and writesize is > supposed to represent the same.
Ok. So we can control the execution of nor_erase_prepare() with mtd->writesize. Will re-roll. Thanks.
> >> If this is not clear in the current documentation of struct mtd, then > >> that can be updated. > > > > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L4166
-- Regards, Pratyush Yadav Texas Instruments India
| |