From: Bert Vermeulen <bert@biot.com> To: Pratyush Yadav <p.yadav@ti.com> Cc: tudor.ambarus@microchip.com, miquel.raynal@bootlin.com, richard@nod.at, vigneshr@ti.com, linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic Date: Fri, 2 Oct 2020 00:22:44 +0200 [thread overview] Message-ID: <801445c9-4f59-5300-3a03-b48a3d631efe@biot.com> (raw) In-Reply-To: <20201001063421.qcjdikj2tje3jn6k@ti.com> On 10/1/20 8:34 AM, Pratyush Yadav wrote: > So using an address width of 4 here is not necessarily the right thing > to do. This change would break SMPT parsing for all flashes that use > 3-byte addressing by default because SMPT parsing can involve register > reads/writes. One such device is the Cypress S28HS flash. In fact, this > was what prompted me to write the patch [0]. > > Before that patch, how did MX25L25635F decide to use 4-byte addressing? The SoCs I'm dealing with have an SPI_ADDR_SEL pin, indicating whether it should be in 3 or 4-byte mode. The vendor's hacked-up U-Boot sets the mode accordingly, as does their BSP. It seems to me like a misfeature, and I want to just ignore it and do reasonable JEDEC things, but I have the problem that the flash chip can be in 4-byte mode by the time it gets to my spi-nor driver. > Coming out of BFPT parsing addr_width would still be 0. My guess is that > it would go into spi_nor_set_addr_width() with addr_width still 0 and > then the check for (nor->mtd.size > 0x1000000) would set it to 4. Do I > guess correctly? No, it comes out of that with addr_width=3 because the chip publishes 3_OR_4 and hence gets 3, even if that's nonsensical for a 32MB chip to publish. Certainly that's the problem, I just want to solve it in a more general case than just a fixup for this chip. > In that case maybe we can do a better job of deciding what gets priority > in the if-else chain. For example, giving addr_width from nor->info > precedence over the one configured by SFDP can solve this problem. Then > all you have to do is set the addr_width in the info struct, which is > certainly easier than adding a fixup hook. There may be a more elegant > solution to this but I haven't given it much thought. Since Tudor doesn't want the order of sfdp->info changed, how about something like this instead? +++ b/drivers/mtd/spi-nor/core.c @@ -3028,13 +3028,15 @@ static int spi_nor_set_addr_width(struct spi_nor *nor) /* already configured from SFDP */ } else if (nor->info->addr_width) { nor->addr_width = nor->info->addr_width; - } else if (nor->mtd.size > 0x1000000) { - /* enable 4-byte addressing if the device exceeds 16MiB */ - nor->addr_width = 4; } else { nor->addr_width = 3; } + if (nor->addr_width == 3 && nor->mtd.size > 0x1000000) { + /* enable 4-byte addressing if the device exceeds 16MiB */ + nor->addr_width = 4; + } + Still fixes the general case, but I'm not sure what the SMPT parsing problem is -- would this still trigger it? -- Bert Vermeulen bert@biot.com
WARNING: multiple messages have this Message-ID (diff)
From: Bert Vermeulen <bert@biot.com> To: Pratyush Yadav <p.yadav@ti.com> Cc: vigneshr@ti.com, tudor.ambarus@microchip.com, richard@nod.at, linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org, miquel.raynal@bootlin.com Subject: Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic Date: Fri, 2 Oct 2020 00:22:44 +0200 [thread overview] Message-ID: <801445c9-4f59-5300-3a03-b48a3d631efe@biot.com> (raw) In-Reply-To: <20201001063421.qcjdikj2tje3jn6k@ti.com> On 10/1/20 8:34 AM, Pratyush Yadav wrote: > So using an address width of 4 here is not necessarily the right thing > to do. This change would break SMPT parsing for all flashes that use > 3-byte addressing by default because SMPT parsing can involve register > reads/writes. One such device is the Cypress S28HS flash. In fact, this > was what prompted me to write the patch [0]. > > Before that patch, how did MX25L25635F decide to use 4-byte addressing? The SoCs I'm dealing with have an SPI_ADDR_SEL pin, indicating whether it should be in 3 or 4-byte mode. The vendor's hacked-up U-Boot sets the mode accordingly, as does their BSP. It seems to me like a misfeature, and I want to just ignore it and do reasonable JEDEC things, but I have the problem that the flash chip can be in 4-byte mode by the time it gets to my spi-nor driver. > Coming out of BFPT parsing addr_width would still be 0. My guess is that > it would go into spi_nor_set_addr_width() with addr_width still 0 and > then the check for (nor->mtd.size > 0x1000000) would set it to 4. Do I > guess correctly? No, it comes out of that with addr_width=3 because the chip publishes 3_OR_4 and hence gets 3, even if that's nonsensical for a 32MB chip to publish. Certainly that's the problem, I just want to solve it in a more general case than just a fixup for this chip. > In that case maybe we can do a better job of deciding what gets priority > in the if-else chain. For example, giving addr_width from nor->info > precedence over the one configured by SFDP can solve this problem. Then > all you have to do is set the addr_width in the info struct, which is > certainly easier than adding a fixup hook. There may be a more elegant > solution to this but I haven't given it much thought. Since Tudor doesn't want the order of sfdp->info changed, how about something like this instead? +++ b/drivers/mtd/spi-nor/core.c @@ -3028,13 +3028,15 @@ static int spi_nor_set_addr_width(struct spi_nor *nor) /* already configured from SFDP */ } else if (nor->info->addr_width) { nor->addr_width = nor->info->addr_width; - } else if (nor->mtd.size > 0x1000000) { - /* enable 4-byte addressing if the device exceeds 16MiB */ - nor->addr_width = 4; } else { nor->addr_width = 3; } + if (nor->addr_width == 3 && nor->mtd.size > 0x1000000) { + /* enable 4-byte addressing if the device exceeds 16MiB */ + nor->addr_width = 4; + } + Still fixes the general case, but I'm not sure what the SMPT parsing problem is -- would this still trigger it? -- Bert Vermeulen bert@biot.com ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2020-10-01 22:22 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-30 23:56 [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic Bert Vermeulen 2020-09-30 23:56 ` Bert Vermeulen 2020-10-01 6:34 ` Pratyush Yadav 2020-10-01 6:34 ` Pratyush Yadav 2020-10-01 14:15 ` Tudor.Ambarus 2020-10-01 14:15 ` Tudor.Ambarus 2020-10-01 22:22 ` Bert Vermeulen [this message] 2020-10-01 22:22 ` Bert Vermeulen 2020-10-02 7:50 ` David Laight 2020-10-02 7:50 ` David Laight 2020-10-04 21:12 ` Bert Vermeulen 2020-10-04 21:12 ` Bert Vermeulen 2020-10-04 21:36 ` David Laight 2020-10-04 21:36 ` David Laight 2020-10-06 23:19 ` Joel Stanley 2020-10-06 23:19 ` Joel Stanley 2020-10-06 11:03 ` Tudor.Ambarus 2020-10-06 11:03 ` Tudor.Ambarus 2020-10-06 11:19 ` Tudor.Ambarus 2020-10-06 11:19 ` Tudor.Ambarus 2020-10-06 11:40 ` Pratyush Yadav 2020-10-06 11:40 ` Pratyush Yadav
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=801445c9-4f59-5300-3a03-b48a3d631efe@biot.com \ --to=bert@biot.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=miquel.raynal@bootlin.com \ --cc=p.yadav@ti.com \ --cc=richard@nod.at \ --cc=tudor.ambarus@microchip.com \ --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.