From: <Tudor.Ambarus@microchip.com> To: <michael@walle.cc> Cc: <p.yadav@ti.com>, <miquel.raynal@bootlin.com>, <richard@nod.at>, <vigneshr@ti.com>, <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3() Date: Tue, 19 Jul 2022 08:30:58 +0000 [thread overview] Message-ID: <3095ef77-a90b-4d10-4891-a4309e3900d9@microchip.com> (raw) In-Reply-To: <1a1b1bf3f8ea7a27afb6e85e0ecabbbc@walle.cc> 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.
WARNING: multiple messages have this Message-ID (diff)
From: <Tudor.Ambarus@microchip.com> To: <michael@walle.cc> Cc: <p.yadav@ti.com>, <miquel.raynal@bootlin.com>, <richard@nod.at>, <vigneshr@ti.com>, <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3() Date: Tue, 19 Jul 2022 08:30:58 +0000 [thread overview] Message-ID: <3095ef77-a90b-4d10-4891-a4309e3900d9@microchip.com> (raw) In-Reply-To: <1a1b1bf3f8ea7a27afb6e85e0ecabbbc@walle.cc> 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. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2022-07-19 8:31 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-10 14:02 [PATCH 0/2] introduce SNOR_ID3() Michael Walle 2022-05-10 14:02 ` Michael Walle 2022-05-10 14:02 ` [PATCH 1/2] mtd: spi-nor: " Michael Walle 2022-05-10 14:02 ` Michael Walle 2022-06-05 15:00 ` Tom Fitzhenry 2022-06-05 15:00 ` Tom Fitzhenry 2022-07-12 7:23 ` Pratyush Yadav 2022-07-15 12:19 ` Biju Das 2022-07-15 12:19 ` Biju Das 2022-07-12 7:23 ` Pratyush Yadav 2022-07-19 5:57 ` Tudor.Ambarus 2022-07-19 5:57 ` Tudor.Ambarus 2022-07-19 7:07 ` Michael Walle 2022-07-19 7:07 ` Michael Walle 2022-07-19 7:33 ` Tudor.Ambarus 2022-07-19 7:33 ` Tudor.Ambarus 2022-07-19 7:57 ` Michael Walle 2022-07-19 7:57 ` Michael Walle 2022-07-19 8:30 ` Tudor.Ambarus [this message] 2022-07-19 8:30 ` Tudor.Ambarus 2022-07-28 3:24 ` Tudor.Ambarus 2022-07-28 3:24 ` Tudor.Ambarus 2022-07-28 13:12 ` Michael Walle 2022-07-28 13:12 ` Michael Walle 2022-07-28 13:31 ` Tudor.Ambarus 2022-07-28 13:31 ` Tudor.Ambarus 2022-07-28 13:56 ` Michael Walle 2022-07-28 13:56 ` Michael Walle 2022-05-10 14:02 ` [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm Michael Walle 2022-05-10 14:02 ` Michael Walle 2022-07-12 8:40 ` Tudor.Ambarus 2022-07-12 8:40 ` Tudor.Ambarus 2022-07-18 7:21 ` Michael Walle 2022-07-18 7:21 ` Michael Walle 2022-07-18 7:25 ` Michael Walle 2022-07-18 7:25 ` Michael Walle 2022-07-19 6:00 ` Tudor.Ambarus 2022-07-19 6:00 ` Tudor.Ambarus 2022-07-19 7:02 ` Michael Walle 2022-07-19 7:02 ` Michael Walle 2022-07-19 7:24 ` Tudor.Ambarus 2022-07-19 7:24 ` Tudor.Ambarus 2022-05-10 14:03 ` [PATCH 0/2] mtd: spi-nor: introduce SNOR_ID3() Michael Walle 2022-05-10 14:03 ` Michael Walle
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3095ef77-a90b-4d10-4891-a4309e3900d9@microchip.com \ --to=tudor.ambarus@microchip.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=michael@walle.cc \ --cc=miquel.raynal@bootlin.com \ --cc=p.yadav@ti.com \ --cc=richard@nod.at \ --cc=vigneshr@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.