Messages in this thread Patch in this message | | | From | <> | Subject | Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3() | Date | Tue, 19 Jul 2022 08:30:58 +0000 |
| |
On 7/19/22 10:57, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2022-07-19 09:33, schrieb Tudor.Ambarus@microchip.com: >> On 7/19/22 10:07, Michael Walle wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> Am 2022-07-19 07:57, schrieb Tudor.Ambarus@microchip.com: >>>> On 5/10/22 17:02, Michael Walle wrote: >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>>>> know >>>>> the content is safe >>>>> >>>>> Up until now, flashes were defined by specifying the JEDEC ID, the >>>>> sector size and the number of sectors. This can be read by parsing >>>>> the >>>>> SFDP, we don't have to specify it. Thus provide a new macro >>>>> SNOR_ID3() >>>>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. >>>>> All >>>>> new flashes which have SFDP should use this macro. >>>> >>>> I like the idea, but you need to refine it a bit. >>>> Your assumptions are true only for flashes that are compliant with >>>> SFDP >>>> revB or >>>> later because params->page_size is initialized by querying BFPT DWORD >>>> 11. I think it would be good to specify this in the comment section. >>> >>> Sure. >>> >>>> Also, I think you introduce >>>> a bug in spi_nor_select_erase() when >>>> CONFIG_MTD_SPI_NOR_USE_4K_SECTORS >>>> is not >>>> selected. wanted_size will be zero, will you select an invalid erase >>>> type? >>> >>> You mean to squeeze [1] into this one? If so, sure. >>> >>> -michael >>> >>> [1] >>> https://lore.kernel.org/linux-mtd/20220716000643.3541839-1-quic_jaehyoo@quicinc.com/ >> >> No, these are orthogonal. If you keep wanted_size to zero, then >> spi_nor_select_uniform_erase() will return NULL, doesn't it? > > No, have a look at the second condition > > if (!erase && tested_erase->size) > erase = .. > > So it will return the first non-empty slot. Thus it will > only return NULL if all the slots are empty (given the > fix is included). > > Actually, I'd have expected that to mask out an erase > type, you clear the corresponding bit in uniform_erase_type, > in which case the for loop in spi_nor_select_uniform_erase() > would have just worked. But apparently there are two differnt > mechanism here to mark an entry as unused, either the size > is zero or the bit is not set. But that is a topic for another > patch.
Right, I remember I leaned towards using just the erase mask to mask out an erase, but I have to reassess this. Here's a patch that is related and I left behind: https://patchwork.ozlabs.org/project/linux-mtd/patch/20211119081412.29732-1-alexander.sverdlin@nokia.com/
>
something else that looks wrong: drivers/mtd/spi-nor/swp.c: return nor->info->sector_size << drivers/mtd/spi-nor/swp.c: return nor->info->sector_size;
How do we progress on this? I like the SNOR_ID3 idea, but I think it should have a different form. Do you want to spend more time on this or do you think I should invest more time on this?
> -michael > >> Maybe to adapt the code to something like >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 21cefe2864ba..dd6cd852d1ef 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -2148,7 +2148,7 @@ static int spi_nor_select_erase(struct spi_nor >> *nor) >> struct spi_nor_erase_map *map = &nor->params->erase_map; >> const struct spi_nor_erase_type *erase = NULL; >> struct mtd_info *mtd = &nor->mtd; >> - u32 wanted_size = nor->info->sector_size; >> + u32 wanted_size = nor->params->sector_size; >> >> and fill nor->params->sector_size even when no SFDP >> >> Also, params->size = (u64)info->sector_size * info->n_sectors; from >> spi_nor_init_default_params() becomes superfluous. I would check >> the fields that I don't initialize in flash_info with SNOR_ID3 >> and check how I can mitigate their absence throughout the code.
| |