Hello! Here's the set of 2 patches against the 'spi-nor/next' branch of 'linux-mtd.git' repo. We're untangling the S25FS512S being taken for S25FL512S by the SPI NOR code due to omitting the family ID byte in the latter chip's ID. I'm proposing to use the full 6-byte JEDEC ID for both these chips. [1/2] mtd: spi-nor: add Spansion S25FS512S ID [2/2] mtd: spi-nor: refine Spansion S25FL512S ID MBR, Sergei
Spansion S25FS512S flash is currently misdetected as S25FL512S since the latter uses 5-byte JEDEC ID, while the 6th ID byte (family ID) is different on those chips. Add the 6-byte S25FS512S ID before S25FL512S ID in order not to break the existing S25FS512S users. Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- drivers/mtd/spi-nor/spi-nor.c | 1 + 1 file changed, 1 insertion(+) Index: linux-mtd/drivers/mtd/spi-nor/spi-nor.c =================================================================== --- linux-mtd.orig/drivers/mtd/spi-nor/spi-nor.c +++ linux-mtd/drivers/mtd/spi-nor/spi-nor.c @@ -1887,6 +1887,7 @@ static const struct flash_info spi_nor_i { "s25sl064p", INFO(0x010216, 0x4d00, 64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, + { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, { "s25fl512s", INFO(0x010220, 0x4d00, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, { "s70fl01gs", INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) }, { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) },
Spansion S25FL512S ID is erroneously using 5-byte JEDEC ID, while the chip family ID is stored in the 6th byte. Due to using only 5-byte ID, it's also covering S25FS512S and now that we have added 6-byte ID for that chip, we can convert S25FL512S to using a proper 6-byte ID as well... Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- drivers/mtd/spi-nor/spi-nor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux-mtd/drivers/mtd/spi-nor/spi-nor.c =================================================================== --- linux-mtd.orig/drivers/mtd/spi-nor/spi-nor.c +++ linux-mtd/drivers/mtd/spi-nor/spi-nor.c @@ -1887,8 +1887,8 @@ static const struct flash_info spi_nor_i { "s25sl064p", INFO(0x010216, 0x4d00, 64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, - { "s25fl512s", INFO(0x010220, 0x4d00, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, { "s70fl01gs", INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) }, { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) }, { "s25sl12801", INFO(0x012018, 0x0301, 64 * 1024, 256, 0) },
Hi, Sergei, On 01/16/2019 07:49 PM, Sergei Shtylyov wrote: > Hello! > > Here's the set of 2 patches against the 'spi-nor/next' branch of 'linux-mtd.git' > repo. We're untangling the S25FS512S being taken for S25FL512S by the SPI NOR code > due to omitting the family ID byte in the latter chip's ID. I'm proposing to use > the full 6-byte JEDEC ID for both these chips. The patch set is looking good. If these patches are based on Cyrille's suggestion from [1], we can give him credit by adding: Suggested-by: Cyrille Pitchen <cyrille.pitchen@microchip.com> We'll need his approval if that's the case. If you're going to resubmit, it's a good time to get rid of the 80 char limit checkpatch warning. I would recommend this way of formatting entries, because it has fewer lines: { "mx25u12835f", INFO(0xc22538, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, Of course, if this is not the case, and you came with this proposal without knowing about Cyrille's suggestion, I'm not going to ask you to resubmit just for this little cosmetic change, just say. Thanks, ta [1] https://patchwork.kernel.org/patch/10595835/ ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hello! On 01/21/2019 11:28 AM, Tudor.Ambarus@microchip.com wrote: >> Here's the set of 2 patches against the 'spi-nor/next' branch of 'linux-mtd.git' >> repo. We're untangling the S25FS512S being taken for S25FL512S by the SPI NOR code >> due to omitting the family ID byte in the latter chip's ID. I'm proposing to use >> the full 6-byte JEDEC ID for both these chips. > > The patch set is looking good. If these patches are based on Cyrille's > suggestion from [1], we can give him credit by adding: No, I even had trouble finding where Cyrille was suggesting that. :-) BTW, looking at his message, I feel somewhat perplexed: "Maybe you can do the magic by placing a new INFO6() entry for the S25FL512S, just before the legacy INFO() entry for S25FL512S. Indeed entries are tested in the order they appear inside the spi_nor_ids[] array from spi_nor_read_id(), so the INFO6() entry would be tested 1st, trying to match 6 bytes, then the INFO() entry only trying to match 3 bytes." Did he actually mean adding a new INFO6() entry for S25FS512S? Else that sentence doesn't make a lot of sense to me. :-) Also INFO() entry matches 5 bytes, not 3. Anyway, I'm suggesting to get rid of the INFO() entry for S25FL512S altogether -- and that's not a part of his suggestion... perhaps it's too risky; I'm not sure why 5-byte matching was used for S25FL512S in the 1st place. > Suggested-by: Cyrille Pitchen <cyrille.pitchen@microchip.com> > We'll need his approval if that's the case. [...] > Of course, if this is not the case, and you came with this proposal without > knowing about Cyrille's suggestion, Well, I remember you (or somebody else) pointing to that patchwork entry... however, the idea I got was from the Renesas BSP patch. > I'm not going to ask you to resubmit just > for this little cosmetic change, just say. Thank you. :-) > Thanks, > ta > > [1] https://patchwork.kernel.org/patch/10595835/ MBR, Sergei ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 01/16/2019 07:51 PM, Sergei Shtylyov wrote: > Spansion S25FS512S flash is currently misdetected as S25FL512S since the > latter uses 5-byte JEDEC ID, while the 6th ID byte (family ID) is different > on those chips. Add the 6-byte S25FS512S ID before S25FL512S ID in order > not to break the existing S25FS512S users. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > > --- > drivers/mtd/spi-nor/spi-nor.c | 1 + > 1 file changed, 1 insertion(+) > > Index: linux-mtd/drivers/mtd/spi-nor/spi-nor.c > =================================================================== > --- linux-mtd.orig/drivers/mtd/spi-nor/spi-nor.c > +++ linux-mtd/drivers/mtd/spi-nor/spi-nor.c > @@ -1887,6 +1887,7 @@ static const struct flash_info spi_nor_i > { "s25sl064p", INFO(0x010216, 0x4d00, 64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, > { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, > { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > + { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > { "s25fl512s", INFO(0x010220, 0x4d00, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > { "s70fl01gs", INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) }, > { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) }, > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 01/16/2019 07:53 PM, Sergei Shtylyov wrote: > Spansion S25FL512S ID is erroneously using 5-byte JEDEC ID, while the chip > family ID is stored in the 6th byte. Due to using only 5-byte ID, it's also > covering S25FS512S and now that we have added 6-byte ID for that chip, we > can convert S25FL512S to using a proper 6-byte ID as well... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > > --- > drivers/mtd/spi-nor/spi-nor.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Index: linux-mtd/drivers/mtd/spi-nor/spi-nor.c > =================================================================== > --- linux-mtd.orig/drivers/mtd/spi-nor/spi-nor.c > +++ linux-mtd/drivers/mtd/spi-nor/spi-nor.c > @@ -1887,8 +1887,8 @@ static const struct flash_info spi_nor_i > { "s25sl064p", INFO(0x010216, 0x4d00, 64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, > { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, > { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > - { "s25fl512s", INFO(0x010220, 0x4d00, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > { "s70fl01gs", INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) }, > { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) }, > { "s25sl12801", INFO(0x012018, 0x0301, 64 * 1024, 256, 0) }, > > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Wed, 2019-01-16 at 17:53:25 UTC, Sergei Shtylyov wrote: > Spansion S25FL512S ID is erroneously using 5-byte JEDEC ID, while the chip > family ID is stored in the 6th byte. Due to using only 5-byte ID, it's also > covering S25FS512S and now that we have added 6-byte ID for that chip, we > can convert S25FL512S to using a proper 6-byte ID as well... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> Applied to http://git.infradead.org/linux-mtd.git spi-nor/next, thanks. Boris ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Wed, 2019-01-16 at 17:51:56 UTC, Sergei Shtylyov wrote: > Spansion S25FS512S flash is currently misdetected as S25FL512S since the > latter uses 5-byte JEDEC ID, while the 6th ID byte (family ID) is different > on those chips. Add the 6-byte S25FS512S ID before S25FL512S ID in order > not to break the existing S25FS512S users. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> Applied to http://git.infradead.org/linux-mtd.git spi-nor/next, thanks. Boris ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Sergei, On Wed, Jan 16, 2019 at 6:53 PM Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > Spansion S25FL512S ID is erroneously using 5-byte JEDEC ID, while the chip > family ID is stored in the 6th byte. Due to using only 5-byte ID, it's also > covering S25FS512S and now that we have added 6-byte ID for that chip, we > can convert S25FL512S to using a proper 6-byte ID as well... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> This is now commit a2126b0a010905e5 ("mtd: spi-nor: refine Spansion S25FL512S ID"), and turns out to cause a regression on r8a7791/koelsch. Dmesg diff before/after: -m25p80 spi0.0: s25fl512s (65536 Kbytes) -3 fixed-partitions partitions found on MTD device spi0.0 -Creating 3 MTD partitions on "spi0.0": -0x000000000000-0x000000080000 : "loader" -0x000000080000-0x000000600000 : "user" -0x000000600000-0x000004000000 : "flash" +m25p80 spi0.0: unrecognized JEDEC id bytes: 01, 02, 20 As the (old) U-Boot on my Koelsch keeps many module clocks enabled, I typically merge in my topic/renesas-debug branch, which makes sure all non-critical module clocks are disabled early during boot, to catch drivers not properly implementing Runtime PM. However, it turns out this has some impact on JEDEC ID detection: - When module clocks are left untouched, spi_nor_read_id() reads 0x01:0x02:0x20:0x4d:0x00:0x80. - When my debug code has disabled module clocks during early boot, The last byte is 0x00. Before the above commit, only the first 5 bytes were compared, and the last byte was ignored, thus not causing problems. When comparing all 6 bytes, detection fails if the last byte is 0x00. I believe mainline U-Boot for R-Car Gen2 boards doesn't keep the QSPI module clock enabled, so this commit may breaks those boards. To be investigated more (e.g. with a logic analyzer)... > --- linux-mtd.orig/drivers/mtd/spi-nor/spi-nor.c > +++ linux-mtd/drivers/mtd/spi-nor/spi-nor.c > @@ -1887,8 +1887,8 @@ static const struct flash_info spi_nor_i > { "s25sl064p", INFO(0x010216, 0x4d00, 64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, > { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, > { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > - { "s25fl512s", INFO(0x010220, 0x4d00, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > { "s70fl01gs", INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) }, > { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) }, > { "s25sl12801", INFO(0x012018, 0x0301, 64 * 1024, 256, 0) }, Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi, Geert, On 03/05/2019 04:05 PM, Geert Uytterhoeven wrote: > Hi Sergei, > > On Wed, Jan 16, 2019 at 6:53 PM Sergei Shtylyov > <sergei.shtylyov@cogentembedded.com> wrote: >> Spansion S25FL512S ID is erroneously using 5-byte JEDEC ID, while the chip >> family ID is stored in the 6th byte. Due to using only 5-byte ID, it's also >> covering S25FS512S and now that we have added 6-byte ID for that chip, we >> can convert S25FL512S to using a proper 6-byte ID as well... >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > This is now commit a2126b0a010905e5 ("mtd: spi-nor: refine Spansion > S25FL512S ID"), and turns out to cause a regression on r8a7791/koelsch. > Dmesg diff before/after: > > -m25p80 spi0.0: s25fl512s (65536 Kbytes) > -3 fixed-partitions partitions found on MTD device spi0.0 > -Creating 3 MTD partitions on "spi0.0": > -0x000000000000-0x000000080000 : "loader" > -0x000000080000-0x000000600000 : "user" > -0x000000600000-0x000004000000 : "flash" > +m25p80 spi0.0: unrecognized JEDEC id bytes: 01, 02, 20 > > As the (old) U-Boot on my Koelsch keeps many module clocks enabled, I > typically merge in my topic/renesas-debug branch, which makes sure all > non-critical module clocks are disabled early during boot, to catch > drivers not properly implementing Runtime PM. > > However, it turns out this has some impact on JEDEC ID detection: > - When module clocks are left untouched, spi_nor_read_id() reads > 0x01:0x02:0x20:0x4d:0x00:0x80. > - When my debug code has disabled module clocks during early boot, > The last byte is 0x00. > > Before the above commit, only the first 5 bytes were compared, and the > last byte was ignored, thus not causing problems. > When comparing all 6 bytes, detection fails if the last byte is 0x00. > We'll have to understand why just the 6th byte is different. > I believe mainline U-Boot for R-Car Gen2 boards doesn't keep the QSPI > module clock enabled, so this commit may breaks those boards. Who provides the flash's input clock and at what frequency? Cheers, ta > > To be investigated more (e.g. with a logic analyzer)... > >> --- linux-mtd.orig/drivers/mtd/spi-nor/spi-nor.c >> +++ linux-mtd/drivers/mtd/spi-nor/spi-nor.c >> @@ -1887,8 +1887,8 @@ static const struct flash_info spi_nor_i >> { "s25sl064p", INFO(0x010216, 0x4d00, 64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, >> { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, >> { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >> + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >> { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >> - { "s25fl512s", INFO(0x010220, 0x4d00, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >> { "s70fl01gs", INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) }, >> { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) }, >> { "s25sl12801", INFO(0x012018, 0x0301, 64 * 1024, 256, 0) }, > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 03/05/2019 04:05 PM, Geert Uytterhoeven wrote: > +m25p80 spi0.0: unrecognized JEDEC id bytes: 01, 02, 20 side note: first 3 id bytes are correct, we should print all id_len bytes on error. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Tue, Mar 5, 2019 at 3:05 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Jan 16, 2019 at 6:53 PM Sergei Shtylyov > <sergei.shtylyov@cogentembedded.com> wrote: > > Spansion S25FL512S ID is erroneously using 5-byte JEDEC ID, while the chip > > family ID is stored in the 6th byte. Due to using only 5-byte ID, it's also > > covering S25FS512S and now that we have added 6-byte ID for that chip, we > > can convert S25FL512S to using a proper 6-byte ID as well... > > > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > This is now commit a2126b0a010905e5 ("mtd: spi-nor: refine Spansion > S25FL512S ID"), and turns out to cause a regression on r8a7791/koelsch. > Dmesg diff before/after: > > -m25p80 spi0.0: s25fl512s (65536 Kbytes) > -3 fixed-partitions partitions found on MTD device spi0.0 > -Creating 3 MTD partitions on "spi0.0": > -0x000000000000-0x000000080000 : "loader" > -0x000000080000-0x000000600000 : "user" > -0x000000600000-0x000004000000 : "flash" > +m25p80 spi0.0: unrecognized JEDEC id bytes: 01, 02, 20 > > As the (old) U-Boot on my Koelsch keeps many module clocks enabled, I > typically merge in my topic/renesas-debug branch, which makes sure all > non-critical module clocks are disabled early during boot, to catch > drivers not properly implementing Runtime PM. > > However, it turns out this has some impact on JEDEC ID detection: > - When module clocks are left untouched, spi_nor_read_id() reads > 0x01:0x02:0x20:0x4d:0x00:0x80. > - When my debug code has disabled module clocks during early boot, > The last byte is 0x00. > > Before the above commit, only the first 5 bytes were compared, and the > last byte was ignored, thus not causing problems. > When comparing all 6 bytes, detection fails if the last byte is 0x00. > > I believe mainline U-Boot for R-Car Gen2 boards doesn't keep the QSPI > module clock enabled, so this commit may breaks those boards. > > To be investigated more (e.g. with a logic analyzer)... The FLASH returns the correct data, but it is not received correctly. When reading 16 bytes, thus including the ASCII model at offset 6/7: - actual: 01 02 20 4d 00 80 47 31 82 ff ff ff ff ff ff ff - received: 01 02 20 4d 00 00 8e 63 05 ff ff ff ff ff ff fe When retrying the operation, data is received correctly. This happens due to a Runtime PM related bug in the initialization code of the spi-rspi driver: if the module clock is not running (disabled by my debug code, the boot loader, or the clk_disable_unused late initcall), the SPI controller is not initialized properly. The first transfer still manages to read some correct data, which used to be sufficient to identify the S25FL512S FLASH chip, before the sixth byte was also considered. Will send a quick fix, and a proper solution requiring more refactoring later. Sorry for the noise... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
From: Boris Brezillon <boris.brezillon@bootlin.com> Make use of the spi-mem direct mapping API to let advanced controllers optimize read/write operations when they support direct mapping. Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Tested-by: Yogesh Narayan Gaur <yogeshnarayan.gaur@nxp.com> --- This is a leftover unmerged patch of the "spi: spi-mem: Add a direct mapping API" series, atop of the 'mtd/next' branch of the MTD 'linux.git' repo. Changes in v4: - Add Yogesh's T-b and my S-o-b Changes in v3: - Make nor->read/write() functional before the direct mappings have been created - Add Miquel's R-b Changes in v2: - Rename the dirmap fields - Return directly after calling dirmap_read/write() and let the spi-nor framework call us again if those functions returned less than the requested length drivers/mtd/devices/m25p80.c | 102 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 99 insertions(+), 3 deletions(-) Index: linux/drivers/mtd/devices/m25p80.c =================================================================== --- linux.orig/drivers/mtd/devices/m25p80.c +++ linux/drivers/mtd/devices/m25p80.c @@ -31,8 +31,70 @@ struct m25p { struct spi_mem *spimem; struct spi_nor spi_nor; + struct { + struct spi_mem_dirmap_desc *rdesc; + struct spi_mem_dirmap_desc *wdesc; + } dirmap; }; +static int m25p_create_write_dirmap(struct m25p *flash) +{ + struct spi_nor *nor = &flash->spi_nor; + struct spi_mem_dirmap_info info = { + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->program_opcode, 1), + SPI_MEM_OP_ADDR(nor->addr_width, 0, 1), + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_DATA_OUT(0, NULL, 1)), + .offset = 0, + .length = flash->spi_nor.mtd.size, + }; + struct spi_mem_op *op = &info.op_tmpl; + + /* get transfer protocols. */ + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); + op->dummy.buswidth = op->addr.buswidth; + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto); + + if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second) + op->addr.nbytes = 0; + + flash->dirmap.wdesc = spi_mem_dirmap_create(flash->spimem, &info); + if (IS_ERR(flash->dirmap.wdesc)) + return PTR_ERR(flash->dirmap.wdesc); + + return 0; +} + +static int m25p_create_read_dirmap(struct m25p *flash) +{ + struct spi_nor *nor = &flash->spi_nor; + struct spi_mem_dirmap_info info = { + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1), + SPI_MEM_OP_ADDR(nor->addr_width, 0, 1), + SPI_MEM_OP_DUMMY(nor->read_dummy, 1), + SPI_MEM_OP_DATA_IN(0, NULL, 1)), + .offset = 0, + .length = flash->spi_nor.mtd.size, + }; + struct spi_mem_op *op = &info.op_tmpl; + + /* get transfer protocols. */ + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); + op->dummy.buswidth = op->addr.buswidth; + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); + + /* convert the dummy cycles to the number of bytes */ + op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8; + + flash->dirmap.rdesc = spi_mem_dirmap_create(flash->spimem, &info); + if (IS_ERR(flash->dirmap.rdesc)) + return PTR_ERR(flash->dirmap.rdesc); + + return 0; +} + static int m25p80_read_reg(struct spi_nor *nor, u8 code, u8 *val, int len) { struct m25p *flash = nor->priv; @@ -92,6 +154,9 @@ static ssize_t m25p80_write(struct spi_n SPI_MEM_OP_DATA_OUT(len, buf, 1)); int ret; + if (flash->dirmap.wdesc) + return spi_mem_dirmap_write(flash->dirmap.wdesc, to, len, buf); + /* get transfer protocols. */ op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); @@ -128,6 +193,9 @@ static ssize_t m25p80_read(struct spi_no size_t remaining = len; int ret; + if (flash->dirmap.rdesc) + return spi_mem_dirmap_read(flash->dirmap.rdesc, from, len, buf); + /* get transfer protocols. */ op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); @@ -238,19 +306,47 @@ static int m25p_probe(struct spi_mem *sp if (ret) return ret; - return mtd_device_register(&nor->mtd, data ? data->parts : NULL, - data ? data->nr_parts : 0); + ret = m25p_create_write_dirmap(flash); + if (ret) + return ret; + + ret = m25p_create_read_dirmap(flash); + if (ret) + goto err_destroy_write_dirmap; + + ret = mtd_device_register(&nor->mtd, data ? data->parts : NULL, + data ? data->nr_parts : 0); + if (ret) + goto err_destroy_read_dirmap; + + return 0; + +err_destroy_read_dirmap: + spi_mem_dirmap_destroy(flash->dirmap.rdesc); + +err_destroy_write_dirmap: + spi_mem_dirmap_destroy(flash->dirmap.wdesc); + + return ret; } static int m25p_remove(struct spi_mem *spimem) { struct m25p *flash = spi_mem_get_drvdata(spimem); + int ret; spi_nor_restore(&flash->spi_nor); /* Clean up MTD stuff. */ - return mtd_device_unregister(&flash->spi_nor.mtd); + ret = mtd_device_unregister(&flash->spi_nor.mtd); + if (ret) + return ret; + + spi_mem_dirmap_destroy(flash->dirmap.rdesc); + spi_mem_dirmap_destroy(flash->dirmap.wdesc); + + return 0; } static void m25p_shutdown(struct spi_mem *spimem) ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi, On 01-Jun-19 12:01 AM, Sergei Shtylyov wrote: [...] > > drivers/mtd/devices/m25p80.c | 102 +++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 99 insertions(+), 3 deletions(-) > > Index: linux/drivers/mtd/devices/m25p80.c > =================================================================== > --- linux.orig/drivers/mtd/devices/m25p80.c > +++ linux/drivers/mtd/devices/m25p80.c > @@ -31,8 +31,70 @@ > struct m25p { > struct spi_mem *spimem; > struct spi_nor spi_nor; > + struct { > + struct spi_mem_dirmap_desc *rdesc; > + struct spi_mem_dirmap_desc *wdesc; > + } dirmap; > }; > > +static int m25p_create_write_dirmap(struct m25p *flash) > +{ > + struct spi_nor *nor = &flash->spi_nor; > + struct spi_mem_dirmap_info info = { > + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->program_opcode, 1), > + SPI_MEM_OP_ADDR(nor->addr_width, 0, 1), > + SPI_MEM_OP_NO_DUMMY, > + SPI_MEM_OP_DATA_OUT(0, NULL, 1)), > + .offset = 0, > + .length = flash->spi_nor.mtd.size, > + }; > + struct spi_mem_op *op = &info.op_tmpl; > + > + /* get transfer protocols. */ > + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); > + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); > + op->dummy.buswidth = op->addr.buswidth; > + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto); > + > + if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second) > + op->addr.nbytes = 0; > + > + flash->dirmap.wdesc = spi_mem_dirmap_create(flash->spimem, &info); Could you change this and elsewhere to devm_spi_mem_dirmap_create() and thus simplify error handling? Regards Vignesh > + if (IS_ERR(flash->dirmap.wdesc)) > + return PTR_ERR(flash->dirmap.wdesc); > + > + return 0; > +} > + > +static int m25p_create_read_dirmap(struct m25p *flash) > +{ > + struct spi_nor *nor = &flash->spi_nor; > + struct spi_mem_dirmap_info info = { > + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1), > + SPI_MEM_OP_ADDR(nor->addr_width, 0, 1), > + SPI_MEM_OP_DUMMY(nor->read_dummy, 1), > + SPI_MEM_OP_DATA_IN(0, NULL, 1)), > + .offset = 0, > + .length = flash->spi_nor.mtd.size, > + }; > + struct spi_mem_op *op = &info.op_tmpl; > + > + /* get transfer protocols. */ > + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); > + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); > + op->dummy.buswidth = op->addr.buswidth; > + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); > + > + /* convert the dummy cycles to the number of bytes */ > + op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8; > + > + flash->dirmap.rdesc = spi_mem_dirmap_create(flash->spimem, &info); > + if (IS_ERR(flash->dirmap.rdesc)) > + return PTR_ERR(flash->dirmap.rdesc); > + > + return 0; > +} > + > static int m25p80_read_reg(struct spi_nor *nor, u8 code, u8 *val, int len) > { > struct m25p *flash = nor->priv; > @@ -92,6 +154,9 @@ static ssize_t m25p80_write(struct spi_n > SPI_MEM_OP_DATA_OUT(len, buf, 1)); > int ret; > > + if (flash->dirmap.wdesc) > + return spi_mem_dirmap_write(flash->dirmap.wdesc, to, len, buf); > + > /* get transfer protocols. */ > op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); > op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); > @@ -128,6 +193,9 @@ static ssize_t m25p80_read(struct spi_no > size_t remaining = len; > int ret; > > + if (flash->dirmap.rdesc) > + return spi_mem_dirmap_read(flash->dirmap.rdesc, from, len, buf); > + > /* get transfer protocols. */ > op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); > op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); > @@ -238,19 +306,47 @@ static int m25p_probe(struct spi_mem *sp > if (ret) > return ret; > > - return mtd_device_register(&nor->mtd, data ? data->parts : NULL, > - data ? data->nr_parts : 0); > + ret = m25p_create_write_dirmap(flash); > + if (ret) > + return ret; > + > + ret = m25p_create_read_dirmap(flash); > + if (ret) > + goto err_destroy_write_dirmap; > + > + ret = mtd_device_register(&nor->mtd, data ? data->parts : NULL, > + data ? data->nr_parts : 0); > + if (ret) > + goto err_destroy_read_dirmap; > + > + return 0; > + > +err_destroy_read_dirmap: > + spi_mem_dirmap_destroy(flash->dirmap.rdesc); > + > +err_destroy_write_dirmap: > + spi_mem_dirmap_destroy(flash->dirmap.wdesc); > + > + return ret; > } > > > static int m25p_remove(struct spi_mem *spimem) > { > struct m25p *flash = spi_mem_get_drvdata(spimem); > + int ret; > > spi_nor_restore(&flash->spi_nor); > > /* Clean up MTD stuff. */ > - return mtd_device_unregister(&flash->spi_nor.mtd); > + ret = mtd_device_unregister(&flash->spi_nor.mtd); > + if (ret) > + return ret; > + > + spi_mem_dirmap_destroy(flash->dirmap.rdesc); > + spi_mem_dirmap_destroy(flash->dirmap.wdesc); > + > + return 0; > } > > static void m25p_shutdown(struct spi_mem *spimem) > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
The 'mtd' local variable is initialized but this value is never used, thus kill that initializer. Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- Thi patch is atop of the 'mtd/next' branch of the MTD 'linux.git' repo. drivers/mtd/chips/gen_probe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux/drivers/mtd/chips/gen_probe.c =================================================================== --- linux.orig/drivers/mtd/chips/gen_probe.c +++ linux/drivers/mtd/chips/gen_probe.c @@ -20,7 +20,7 @@ static int genprobe_new_chip(struct map_ struct mtd_info *mtd_do_chip_probe(struct map_info *map, struct chip_probe *cp) { - struct mtd_info *mtd = NULL; + struct mtd_info *mtd; struct cfi_private *cfi; /* First probe the map to see if we have CFI stuff there. */ ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 04/07/19 1:56 AM, Sergei Shtylyov wrote: > The 'mtd' local variable is initialized but this value is never used, > thus kill that initializer. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > Thi patch is atop of the 'mtd/next' branch of the MTD 'linux.git' repo. Merged into git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next Thanks! > > drivers/mtd/chips/gen_probe.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Index: linux/drivers/mtd/chips/gen_probe.c > =================================================================== > --- linux.orig/drivers/mtd/chips/gen_probe.c > +++ linux/drivers/mtd/chips/gen_probe.c > @@ -20,7 +20,7 @@ static int genprobe_new_chip(struct map_ > > struct mtd_info *mtd_do_chip_probe(struct map_info *map, struct chip_probe *cp) > { > - struct mtd_info *mtd = NULL; > + struct mtd_info *mtd; > struct cfi_private *cfi; > > /* First probe the map to see if we have CFI stuff there. */ > -- Regards Vignesh ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
cfi_udelay() open-codes DIV_ROUND_UP(), violating the kernel coding style while doing this... Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- This patch is atop of the 'mtd/next' branch of the MTD 'linux.git' repo. drivers/mtd/chips/cfi_util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux/drivers/mtd/chips/cfi_util.c =================================================================== --- linux.orig/drivers/mtd/chips/cfi_util.c +++ linux/drivers/mtd/chips/cfi_util.c @@ -26,7 +26,7 @@ void cfi_udelay(int us) { if (us >= 1000) { - msleep((us+999)/1000); + msleep(DIV_ROUND_UP(us, 1000)); } else { udelay(us); cond_resched(); ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Sergei, On Tue, Sep 17, 2019 at 9:28 PM Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > cfi_udelay() open-codes DIV_ROUND_UP(), violating the kernel coding style Perhaps "violating" sounds a bit too harsh? > while doing this... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hello! On 09/17/2019 10:43 PM, Geert Uytterhoeven wrote: >> cfi_udelay() open-codes DIV_ROUND_UP(), violating the kernel coding style > > Perhaps "violating" sounds a bit too harsh? Hm, indeed, scripts/checkpatch.pl doesn't complain on this line. Do you have other ideas how to call this? Or just omit this? >> while doing this... >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> TY! > Gr{oetje,eeting}s, > > Geert MBR, Sergei ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Sergei, On Tue, Sep 17, 2019 at 9:53 PM Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > On 09/17/2019 10:43 PM, Geert Uytterhoeven wrote: > >> cfi_udelay() open-codes DIV_ROUND_UP(), violating the kernel coding style > > > > Perhaps "violating" sounds a bit too harsh? > > Hm, indeed, scripts/checkpatch.pl doesn't complain on this line. Do you have > other ideas how to call this? Or just omit this? Just "use the existing helper, instead of open-coding the same operation"? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Sergei, On 18/09/19 2:53 AM, Geert Uytterhoeven wrote: > Hi Sergei, > > On Tue, Sep 17, 2019 at 9:53 PM Sergei Shtylyov > <sergei.shtylyov@cogentembedded.com> wrote: >> On 09/17/2019 10:43 PM, Geert Uytterhoeven wrote: >>>> cfi_udelay() open-codes DIV_ROUND_UP(), violating the kernel coding style >>> >>> Perhaps "violating" sounds a bit too harsh? >> >> Hm, indeed, scripts/checkpatch.pl doesn't complain on this line. Do you have >> other ideas how to call this? Or just omit this? > scripts/checkpatch.pl --strict will complain about this. > Just "use the existing helper, instead of open-coding the same operation"? I agree with Geert. This driver file predates checkpatch and therefore does not follow all kernel coding styles. But its good to replace open-coding with available helper macro. Also, please don't post new patches in reply to an existing thread. This patch appears in-reply-to ("mtd: devices: m25p80: Use the spi-mem dirmap API") which is unrelated to current patch. -- Regards Vignesh ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 09/18/2019 08:45 AM, Vignesh Raghavendra wrote: >>>>> cfi_udelay() open-codes DIV_ROUND_UP(), violating the kernel coding style >>>> >>>> Perhaps "violating" sounds a bit too harsh? >>> >>> Hm, indeed, scripts/checkpatch.pl doesn't complain on this line. Do you have >>> other ideas how to call this? Or just omit this? >> > > scripts/checkpatch.pl --strict will complain about this. > >> Just "use the existing helper, instead of open-coding the same operation"? > > I agree with Geert. This driver file predates checkpatch and therefore > does not follow all kernel coding styles. But its good to replace > open-coding with available helper macro. > > Also, please don't post new patches in reply to an existing thread. This I don't think I did this. It wasn't intended anyway, sorry... > patch appears in-reply-to ("mtd: devices: m25p80: Use the spi-mem dirmap > API") which is unrelated to current patch. Indeed. MBR, Sergei ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Make use of the spi-mem direct mapping API to let advanced controllers optimize read/write operations when they support direct mapping. Based on the original patch by Boris Brezillon <boris.brezillon@bootlin.com>. Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- This patch is against the 'spi-nor/next' branch of the MTD 'linux.git' repo. drivers/mtd/spi-nor/spi-nor.c | 79 ++++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 5 ++ 2 files changed, 84 insertions(+) Index: linux/drivers/mtd/spi-nor/spi-nor.c =================================================================== --- linux.orig/drivers/mtd/spi-nor/spi-nor.c +++ linux/drivers/mtd/spi-nor/spi-nor.c @@ -2562,6 +2562,14 @@ static int spi_nor_read(struct mtd_info if (ret) return ret; + if (nor->dirmap.rdesc) { + ret = spi_mem_dirmap_read(nor->dirmap.rdesc, from, len, buf); + if (ret < 0) + goto read_err; + *retlen += ret; + goto done; + } + while (len) { loff_t addr = from; @@ -2582,6 +2590,7 @@ static int spi_nor_read(struct mtd_info from += ret; len -= ret; } +done: ret = 0; read_err: @@ -2686,6 +2695,14 @@ static int spi_nor_write(struct mtd_info if (ret) return ret; + if (nor->dirmap.wdesc) { + ret = spi_mem_dirmap_write(nor->dirmap.wdesc, to, len, buf); + if (ret < 0) + goto write_err; + *retlen += ret; + goto done; + } + for (i = 0; i < len; ) { ssize_t written; loff_t addr = to + i; @@ -2723,6 +2740,8 @@ static int spi_nor_write(struct mtd_info *retlen += written; i += written; } +done: + ret = 0; write_err: spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE); @@ -4991,6 +5010,58 @@ int spi_nor_scan(struct spi_nor *nor, co } EXPORT_SYMBOL_GPL(spi_nor_scan); +static int spi_nor_create_read_dirmap(struct spi_nor *nor) +{ + struct spi_mem_dirmap_info info = { + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1), + SPI_MEM_OP_ADDR(nor->addr_width, 0, 1), + SPI_MEM_OP_DUMMY(nor->read_dummy, 1), + SPI_MEM_OP_DATA_IN(0, NULL, 1)), + .offset = 0, + .length = nor->mtd.size, + }; + struct spi_mem_op *op = &info.op_tmpl; + + /* get transfer protocols. */ + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); + op->dummy.buswidth = op->addr.buswidth; + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); + + /* convert the dummy cycles to the number of bytes */ + op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8; + + nor->dirmap.rdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem, + &info); + return PTR_ERR_OR_ZERO(nor->dirmap.rdesc); +} + +static int spi_nor_create_write_dirmap(struct spi_nor *nor) +{ + struct spi_mem_dirmap_info info = { + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->program_opcode, 1), + SPI_MEM_OP_ADDR(nor->addr_width, 0, 1), + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_DATA_OUT(0, NULL, 1)), + .offset = 0, + .length = nor->mtd.size, + }; + struct spi_mem_op *op = &info.op_tmpl; + + /* get transfer protocols. */ + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); + op->dummy.buswidth = op->addr.buswidth; + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto); + + if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second) + op->addr.nbytes = 0; + + nor->dirmap.wdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem, + &info); + return PTR_ERR_OR_ZERO(nor->dirmap.wdesc); +} + static int spi_nor_probe(struct spi_mem *spimem) { struct spi_device *spi = spimem->spi; @@ -5052,6 +5123,14 @@ static int spi_nor_probe(struct spi_mem return -ENOMEM; } + ret = spi_nor_create_read_dirmap(nor); + if (ret) + return ret; + + ret = spi_nor_create_write_dirmap(nor); + if (ret) + return ret; + return mtd_device_register(&nor->mtd, data ? data->parts : NULL, data ? data->nr_parts : 0); } Index: linux/include/linux/mtd/spi-nor.h =================================================================== --- linux.orig/include/linux/mtd/spi-nor.h +++ linux/include/linux/mtd/spi-nor.h @@ -611,6 +611,11 @@ struct spi_nor { int (*clear_sr_bp)(struct spi_nor *nor); struct spi_nor_flash_parameter params; + struct { + struct spi_mem_dirmap_desc *rdesc; + struct spi_mem_dirmap_desc *wdesc; + } dirmap; + void *priv; }; ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Fri, 25 Oct 2019 23:26:42 +0300 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > Make use of the spi-mem direct mapping API to let advanced controllers > optimize read/write operations when they support direct mapping. > > Based on the original patch by Boris Brezillon <boris.brezillon@bootlin.com>. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > This patch is against the 'spi-nor/next' branch of the MTD 'linux.git' repo. > > drivers/mtd/spi-nor/spi-nor.c | 79 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/mtd/spi-nor.h | 5 ++ > 2 files changed, 84 insertions(+) > > Index: linux/drivers/mtd/spi-nor/spi-nor.c > =================================================================== > --- linux.orig/drivers/mtd/spi-nor/spi-nor.c > +++ linux/drivers/mtd/spi-nor/spi-nor.c > @@ -2562,6 +2562,14 @@ static int spi_nor_read(struct mtd_info > if (ret) > return ret; > > + if (nor->dirmap.rdesc) { > + ret = spi_mem_dirmap_read(nor->dirmap.rdesc, from, len, buf); This spi_mem_dirmap_read() call should be moved to spi_nor_spimem_read_data(). > + if (ret < 0) > + goto read_err; > + *retlen += ret; > + goto done; > + } > + > while (len) { > loff_t addr = from; > > @@ -2582,6 +2590,7 @@ static int spi_nor_read(struct mtd_info > from += ret; > len -= ret; > } > +done: > ret = 0; > > read_err: > @@ -2686,6 +2695,14 @@ static int spi_nor_write(struct mtd_info > if (ret) > return ret; > > + if (nor->dirmap.wdesc) { > + ret = spi_mem_dirmap_write(nor->dirmap.wdesc, to, len, buf); Same here, this should be moved to spi_nor_spimem_write_data(). BTW, I wonder how this can work since write_enable() is called in the below for loop (which you skip). Is your SPI controller sending the WE cmd automatically? > + if (ret < 0) > + goto write_err; > + *retlen += ret; > + goto done; > + } > + > for (i = 0; i < len; ) { > ssize_t written; > loff_t addr = to + i; > @@ -2723,6 +2740,8 @@ static int spi_nor_write(struct mtd_info > *retlen += written; > i += written; > } > +done: > + ret = 0; > > write_err: > spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE); > @@ -4991,6 +5010,58 @@ int spi_nor_scan(struct spi_nor *nor, co > } > EXPORT_SYMBOL_GPL(spi_nor_scan); > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hello! On 10/26/2019 10:36 AM, Boris Brezillon wrote: >> Make use of the spi-mem direct mapping API to let advanced controllers >> optimize read/write operations when they support direct mapping. >> >> Based on the original patch by Boris Brezillon <boris.brezillon@bootlin.com>. >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >> --- >> This patch is against the 'spi-nor/next' branch of the MTD 'linux.git' repo. >> >> drivers/mtd/spi-nor/spi-nor.c | 79 ++++++++++++++++++++++++++++++++++++++++++ >> include/linux/mtd/spi-nor.h | 5 ++ >> 2 files changed, 84 insertions(+) >> >> Index: linux/drivers/mtd/spi-nor/spi-nor.c >> =================================================================== >> --- linux.orig/drivers/mtd/spi-nor/spi-nor.c >> +++ linux/drivers/mtd/spi-nor/spi-nor.c >> @@ -2562,6 +2562,14 @@ static int spi_nor_read(struct mtd_info >> if (ret) >> return ret; >> >> + if (nor->dirmap.rdesc) { >> + ret = spi_mem_dirmap_read(nor->dirmap.rdesc, from, len, buf); > > This spi_mem_dirmap_read() call should be moved to > spi_nor_spimem_read_data(). > >> + if (ret < 0) >> + goto read_err; >> + *retlen += ret; >> + goto done; >> + } >> + >> while (len) { >> loff_t addr = from; >> >> @@ -2582,6 +2590,7 @@ static int spi_nor_read(struct mtd_info >> from += ret; >> len -= ret; >> } >> +done: >> ret = 0; >> >> read_err: >> @@ -2686,6 +2695,14 @@ static int spi_nor_write(struct mtd_info >> if (ret) >> return ret; >> >> + if (nor->dirmap.wdesc) { >> + ret = spi_mem_dirmap_write(nor->dirmap.wdesc, to, len, buf); > > Same here, this should be moved to spi_nor_spimem_write_data(). BTW, I > wonder how this can work since write_enable() is called in the below for > loop (which you skip). Is your SPI controller sending the WE cmd > automatically? Probably not, it just doesn't have a write dirmap, only read, so this part didn't get tested... :-) >> + if (ret < 0) >> + goto write_err; >> + *retlen += ret; >> + goto done; >> + } >> + >> for (i = 0; i < len; ) { >> ssize_t written; >> loff_t addr = to + i; [...] MBR, Sergei ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Make use of the spi-mem direct mapping API to let advanced controllers optimize read/write operations when they support direct mapping. Based on the original patch by Boris Brezillon <boris.brezillon@bootlin.com>. Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- Changes in version 2: - moved the spi_mem_dirmap_{read|write}() calls from spi_nor_{read|write}() to spi_nor_spimem_{read|write}_data(). drivers/mtd/spi-nor/spi-nor.c | 125 +++++++++++++++++++++++++++++++++--------- include/linux/mtd/spi-nor.h | 5 + 2 files changed, 104 insertions(+), 26 deletions(-) Index: linux/drivers/mtd/spi-nor/spi-nor.c =================================================================== --- linux.orig/drivers/mtd/spi-nor/spi-nor.c +++ linux/drivers/mtd/spi-nor/spi-nor.c @@ -305,22 +305,28 @@ static ssize_t spi_nor_spimem_xfer_data( static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from, size_t len, u8 *buf) { - struct spi_mem_op op = - SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1), - SPI_MEM_OP_ADDR(nor->addr_width, from, 1), - SPI_MEM_OP_DUMMY(nor->read_dummy, 1), - SPI_MEM_OP_DATA_IN(len, buf, 1)); - - /* get transfer protocols. */ - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); - op.dummy.buswidth = op.addr.buswidth; - op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); + if (!nor->dirmap.rdesc) { + struct spi_mem_op op = + SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1), + SPI_MEM_OP_ADDR(nor->addr_width, from, 1), + SPI_MEM_OP_DUMMY(nor->read_dummy, 1), + SPI_MEM_OP_DATA_IN(len, buf, 1)); + + /* get transfer protocols. */ + op.cmd.buswidth = + spi_nor_get_protocol_inst_nbits(nor->read_proto); + op.addr.buswidth = + spi_nor_get_protocol_addr_nbits(nor->read_proto); + op.dummy.buswidth = op.addr.buswidth; + op.data.buswidth = + spi_nor_get_protocol_data_nbits(nor->read_proto); - /* convert the dummy cycles to the number of bytes */ - op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; + /* convert the dummy cycles to the number of bytes */ + op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; - return spi_nor_spimem_xfer_data(nor, &op); + return spi_nor_spimem_xfer_data(nor, &op); + } + return spi_mem_dirmap_read(nor->dirmap.rdesc, from, len, buf); } /** @@ -354,20 +360,27 @@ static ssize_t spi_nor_read_data(struct static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to, size_t len, const u8 *buf) { - struct spi_mem_op op = - SPI_MEM_OP(SPI_MEM_OP_CMD(nor->program_opcode, 1), - SPI_MEM_OP_ADDR(nor->addr_width, to, 1), - SPI_MEM_OP_NO_DUMMY, - SPI_MEM_OP_DATA_OUT(len, buf, 1)); - - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); - op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto); + if (!nor->dirmap.wdesc) { + struct spi_mem_op op = + SPI_MEM_OP(SPI_MEM_OP_CMD(nor->program_opcode, 1), + SPI_MEM_OP_ADDR(nor->addr_width, to, 1), + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_DATA_OUT(len, buf, 1)); - if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second) - op.addr.nbytes = 0; + op.cmd.buswidth = + spi_nor_get_protocol_inst_nbits(nor->write_proto); + op.addr.buswidth = + spi_nor_get_protocol_addr_nbits(nor->write_proto); + op.data.buswidth = + spi_nor_get_protocol_data_nbits(nor->write_proto); + + if (nor->program_opcode == SPINOR_OP_AAI_WP && + nor->sst_write_second) + op.addr.nbytes = 0; - return spi_nor_spimem_xfer_data(nor, &op); + return spi_nor_spimem_xfer_data(nor, &op); + } + return spi_mem_dirmap_write(nor->dirmap.wdesc, to, len, buf); } /** @@ -4979,6 +4992,58 @@ int spi_nor_scan(struct spi_nor *nor, co } EXPORT_SYMBOL_GPL(spi_nor_scan); +static int spi_nor_create_read_dirmap(struct spi_nor *nor) +{ + struct spi_mem_dirmap_info info = { + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1), + SPI_MEM_OP_ADDR(nor->addr_width, 0, 1), + SPI_MEM_OP_DUMMY(nor->read_dummy, 1), + SPI_MEM_OP_DATA_IN(0, NULL, 1)), + .offset = 0, + .length = nor->mtd.size, + }; + struct spi_mem_op *op = &info.op_tmpl; + + /* get transfer protocols. */ + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); + op->dummy.buswidth = op->addr.buswidth; + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); + + /* convert the dummy cycles to the number of bytes */ + op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8; + + nor->dirmap.rdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem, + &info); + return PTR_ERR_OR_ZERO(nor->dirmap.rdesc); +} + +static int spi_nor_create_write_dirmap(struct spi_nor *nor) +{ + struct spi_mem_dirmap_info info = { + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->program_opcode, 1), + SPI_MEM_OP_ADDR(nor->addr_width, 0, 1), + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_DATA_OUT(0, NULL, 1)), + .offset = 0, + .length = nor->mtd.size, + }; + struct spi_mem_op *op = &info.op_tmpl; + + /* get transfer protocols. */ + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); + op->dummy.buswidth = op->addr.buswidth; + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto); + + if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second) + op->addr.nbytes = 0; + + nor->dirmap.wdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem, + &info); + return PTR_ERR_OR_ZERO(nor->dirmap.wdesc); +} + static int spi_nor_probe(struct spi_mem *spimem) { struct spi_device *spi = spimem->spi; @@ -5040,6 +5105,14 @@ static int spi_nor_probe(struct spi_mem return -ENOMEM; } + ret = spi_nor_create_read_dirmap(nor); + if (ret) + return ret; + + ret = spi_nor_create_write_dirmap(nor); + if (ret) + return ret; + return mtd_device_register(&nor->mtd, data ? data->parts : NULL, data ? data->nr_parts : 0); } Index: linux/include/linux/mtd/spi-nor.h =================================================================== --- linux.orig/include/linux/mtd/spi-nor.h +++ linux/include/linux/mtd/spi-nor.h @@ -602,6 +602,11 @@ struct spi_nor { int (*clear_sr_bp)(struct spi_nor *nor); struct spi_nor_flash_parameter params; + struct { + struct spi_mem_dirmap_desc *rdesc; + struct spi_mem_dirmap_desc *wdesc; + } dirmap; + void *priv; }; ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 11/07/2019 11:49 PM, Sergei Shtylyov wrote: > Make use of the spi-mem direct mapping API to let advanced controllers > optimize read/write operations when they support direct mapping. > > Based on the original patch by Boris Brezillon <boris.brezillon@bootlin.com>. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- Oope, forgot to mention the base. Was generated again mtd/fixes by mistake, should still apply to spi-nor/next with only small offsets. > Changes in version 2: > - moved the spi_mem_dirmap_{read|write}() calls from spi_nor_{read|write}() to > spi_nor_spimem_{read|write}_data(). > > drivers/mtd/spi-nor/spi-nor.c | 125 +++++++++++++++++++++++++++++++++--------- > include/linux/mtd/spi-nor.h | 5 + > 2 files changed, 104 insertions(+), 26 deletions(-) [...] MBR, Sergei ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 11/07/2019 11:49 PM, Sergei Shtylyov wrote: > Make use of the spi-mem direct mapping API to let advanced controllers > optimize read/write operations when they support direct mapping. > > Based on the original patch by Boris Brezillon <boris.brezillon@bootlin.com>. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- Oops, forgot to mention the base. Was generated again mtd/fixes by mistake, should still apply to spi-nor/next with only small offsets. > Changes in version 2: > - moved the spi_mem_dirmap_{read|write}() calls from spi_nor_{read|write}() to > spi_nor_spimem_{read|write}_data(). > > drivers/mtd/spi-nor/spi-nor.c | 125 +++++++++++++++++++++++++++++++++--------- > include/linux/mtd/spi-nor.h | 5 + > 2 files changed, 104 insertions(+), 26 deletions(-) [...] MBR, Sergei ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Thu, 7 Nov 2019 23:49:12 +0300 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > Make use of the spi-mem direct mapping API to let advanced controllers > optimize read/write operations when they support direct mapping. > > Based on the original patch by Boris Brezillon <boris.brezillon@bootlin.com>. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > Changes in version 2: > - moved the spi_mem_dirmap_{read|write}() calls from spi_nor_{read|write}() to > spi_nor_spimem_{read|write}_data(). > > drivers/mtd/spi-nor/spi-nor.c | 125 +++++++++++++++++++++++++++++++++--------- > include/linux/mtd/spi-nor.h | 5 + > 2 files changed, 104 insertions(+), 26 deletions(-) > > Index: linux/drivers/mtd/spi-nor/spi-nor.c > =================================================================== > --- linux.orig/drivers/mtd/spi-nor/spi-nor.c > +++ linux/drivers/mtd/spi-nor/spi-nor.c > @@ -305,22 +305,28 @@ static ssize_t spi_nor_spimem_xfer_data( > static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from, > size_t len, u8 *buf) > { > - struct spi_mem_op op = > - SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1), > - SPI_MEM_OP_ADDR(nor->addr_width, from, 1), > - SPI_MEM_OP_DUMMY(nor->read_dummy, 1), > - SPI_MEM_OP_DATA_IN(len, buf, 1)); > - > - /* get transfer protocols. */ > - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); > - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); > - op.dummy.buswidth = op.addr.buswidth; > - op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); > + if (!nor->dirmap.rdesc) { > + struct spi_mem_op op = > + SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1), > + SPI_MEM_OP_ADDR(nor->addr_width, from, 1), > + SPI_MEM_OP_DUMMY(nor->read_dummy, 1), > + SPI_MEM_OP_DATA_IN(len, buf, 1)); > + > + /* get transfer protocols. */ > + op.cmd.buswidth = > + spi_nor_get_protocol_inst_nbits(nor->read_proto); > + op.addr.buswidth = > + spi_nor_get_protocol_addr_nbits(nor->read_proto); > + op.dummy.buswidth = op.addr.buswidth; > + op.data.buswidth = > + spi_nor_get_protocol_data_nbits(nor->read_proto); > > - /* convert the dummy cycles to the number of bytes */ > - op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; > + /* convert the dummy cycles to the number of bytes */ > + op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; > > - return spi_nor_spimem_xfer_data(nor, &op); > + return spi_nor_spimem_xfer_data(nor, &op); > + } > + return spi_mem_dirmap_read(nor->dirmap.rdesc, from, len, buf); Can we put the spi_mem_dirmap_read() in the if() branch instead of having an extra level of indentation for the most complex block. if (nor->dirmap.rdesc) return spi_mem_dirmap_read(nor->dirmap.rdesc, from, len, buf); ... (same comment applies to the write path BTW). With this addressed, you can add Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> Thanks, Boris ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hello! On 11/09/2019 10:35 PM, Boris Brezillon wrote: >> Make use of the spi-mem direct mapping API to let advanced controllers >> optimize read/write operations when they support direct mapping. >> >> Based on the original patch by Boris Brezillon <boris.brezillon@bootlin.com>. >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >> --- >> Changes in version 2: >> - moved the spi_mem_dirmap_{read|write}() calls from spi_nor_{read|write}() to >> spi_nor_spimem_{read|write}_data(). >> >> drivers/mtd/spi-nor/spi-nor.c | 125 +++++++++++++++++++++++++++++++++--------- >> include/linux/mtd/spi-nor.h | 5 + >> 2 files changed, 104 insertions(+), 26 deletions(-) >> >> Index: linux/drivers/mtd/spi-nor/spi-nor.c >> =================================================================== >> --- linux.orig/drivers/mtd/spi-nor/spi-nor.c >> +++ linux/drivers/mtd/spi-nor/spi-nor.c >> @@ -305,22 +305,28 @@ static ssize_t spi_nor_spimem_xfer_data( >> static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from, >> size_t len, u8 *buf) >> { >> - struct spi_mem_op op = >> - SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1), >> - SPI_MEM_OP_ADDR(nor->addr_width, from, 1), >> - SPI_MEM_OP_DUMMY(nor->read_dummy, 1), >> - SPI_MEM_OP_DATA_IN(len, buf, 1)); >> - >> - /* get transfer protocols. */ >> - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); >> - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); >> - op.dummy.buswidth = op.addr.buswidth; >> - op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); >> + if (!nor->dirmap.rdesc) { >> + struct spi_mem_op op = >> + SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1), >> + SPI_MEM_OP_ADDR(nor->addr_width, from, 1), >> + SPI_MEM_OP_DUMMY(nor->read_dummy, 1), >> + SPI_MEM_OP_DATA_IN(len, buf, 1)); >> + >> + /* get transfer protocols. */ >> + op.cmd.buswidth = >> + spi_nor_get_protocol_inst_nbits(nor->read_proto); >> + op.addr.buswidth = >> + spi_nor_get_protocol_addr_nbits(nor->read_proto); >> + op.dummy.buswidth = op.addr.buswidth; >> + op.data.buswidth = >> + spi_nor_get_protocol_data_nbits(nor->read_proto); >> >> - /* convert the dummy cycles to the number of bytes */ >> - op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; >> + /* convert the dummy cycles to the number of bytes */ >> + op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; >> >> - return spi_nor_spimem_xfer_data(nor, &op); >> + return spi_nor_spimem_xfer_data(nor, &op); >> + } >> + return spi_mem_dirmap_read(nor->dirmap.rdesc, from, len, buf); > > Can we put the spi_mem_dirmap_read() in the if() branch instead of > having an extra level of indentation for the most complex block. Have you noticed a complex variable initializer in that block? Do you want it to be done on the simple branch as well? > if (nor->dirmap.rdesc) > return spi_mem_dirmap_read(nor->dirmap.rdesc, from, > len, buf); > > ... > > (same comment applies to the write path BTW). > > With this addressed, you can add > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> Thank you. :-) > > Thanks, > > Boris MBR, Sergei ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
The driver calls le32_to_cpu() to convert the little-endian tables to a CPU endianness, where le32_to_cpus() should have been called. Was going to use that one... and then discovered a whole array converter, le32_to_cpu_array()! :-) Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- This patch is against the 'spi-nor/next' branch of the MTD 'linux.git' repo plus couple of SPI NOR patches posted earlier... drivers/mtd/spi-nor/spi-nor.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) Index: linux/drivers/mtd/spi-nor/spi-nor.c =================================================================== --- linux.orig/drivers/mtd/spi-nor/spi-nor.c +++ linux/drivers/mtd/spi-nor/spi-nor.c @@ -3626,8 +3626,7 @@ static int spi_nor_parse_bfpt(struct spi return err; /* Fix endianness of the BFPT DWORDs. */ - for (i = 0; i < BFPT_DWORD_MAX; i++) - bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]); + le32_to_cpu_array(bfpt.dwords, BFPT_DWORD_MAX); /* Number of address bytes. */ switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) { @@ -4085,7 +4084,7 @@ static int spi_nor_parse_smpt(struct spi u32 *smpt; size_t len; u32 addr; - int i, ret; + int ret; /* Read the Sector Map Parameter Table. */ len = smpt_header->length * sizeof(*smpt); @@ -4099,8 +4098,7 @@ static int spi_nor_parse_smpt(struct spi goto out; /* Fix endianness of the SMPT DWORDs. */ - for (i = 0; i < smpt_header->length; i++) - smpt[i] = le32_to_cpu(smpt[i]); + le32_to_cpu_array(smpt, smpt_header->length); sector_map = spi_nor_get_map_in_use(nor, smpt, smpt_header->length); if (IS_ERR(sector_map)) { @@ -4193,8 +4191,7 @@ static int spi_nor_parse_4bait(struct sp goto out; /* Fix endianness of the 4BAIT DWORDs. */ - for (i = 0; i < SFDP_4BAIT_DWORD_MAX; i++) - dwords[i] = le32_to_cpu(dwords[i]); + le32_to_cpu_array(dwords, SFDP_4BAIT_DWORD_MAX); /* * Compute the subset of (Fast) Read commands for which the 4-byte ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Saturday, February 1, 2020 10:55:08 PM EET Sergei Shtylyov wrote: > The driver calls le32_to_cpu() to convert the little-endian tables > to a CPU endianness, where le32_to_cpus() should have been called. > Was going to use that one... and then discovered a whole array > converter, le32_to_cpu_array()! > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > This patch is against the 'spi-nor/next' branch of the MTD 'linux.git' repo > plus couple of SPI NOR patches posted earlier... > > drivers/mtd/spi-nor/spi-nor.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) Applied to spi-nor/next. Thanks. ta ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/