| Message ID | 2f46eee5-35c1-291a-52e9-132ced5749a9@atmel.com |
|---|---|
| State | Superseded |
| Headers | show |
On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: > Hi Brian, > > this is the spi-nor PR for 4.11. There are few new controller drivers and > support to some memory parts. Otherwise about the spi-nor framework itself, > the main update is the support of the 4-byte address instruction set, which > is stateless, as opposed to the 4-byte address mode, which is stateful hence > creates issues with some bootloaders when spurious reboot occurs. > > Best regards, > > Cyrille > > The following changes since commit 7ce7d89f48834cefece7804d38fc5d85382edf77: > > Linux 4.10-rc1 (2016-12-25 16:13:08 -0800) > > are available in the git repository at: > > git://github.com/spi-nor/linux.git tags/spi-nor/for-4.11 > > for you to fetch changes up to dc12bcccadafb5441170e6b7c8a438c91d4f385b: > > mtd: spi-nor: cqspi: remove redundant dead code on error return check (2017-02-02 10:42:36 +0100) > > ---------------------------------------------------------------- > This pull request contains the following notable changes: > - add support to the 4-byte address instruction set. > - add support to new memory parts. > - add support to S3AN memories. > - add support to the Intel SPI controller. > - add support to the Aspeed AST2400 and AST2550 controllers. > - fix max SPI transfer and message sizes in m25p80_read(). > - fix the Candence QSPI driver. > - fix the Freescale QSPI driver. > > ---------------------------------------------------------------- > Colin Ian King (1): > mtd: spi-nor: cqspi: remove redundant dead code on error return check > > Cyrille Pitchen (5): > mtd: spi-nor: improve macronix_quad_enable() > mtd: spi-nor: remove WARN_ONCE() message in spi_nor_write() > mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes > mtd: spi-nor: add a stateless method to support memory size above 128Mib > Merge tag 'ib-mfd-mtd-v4.11' of git://git.kernel.org/.../lee/mfd > > Cédric Le Goater (4): > mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC > mtd: aspeed: add memory controllers for the Aspeed AST2400 SoC > mtd: spi-nor: bindings for the Aspeed memory controllers > mtd: aspeed: fix compile warning in aspeed_smc_read_from_ahb() > > Guochun Mao (1): > Documentation: mtk-quadspi: update DT bindings > > Heiner Kallweit (1): > mtd: m25p80: consider max message size in m25p80_read I cannot say I'm super-happy about this particular patch, still ... > Kamal Dasu (1): > mtd: spi-nor: Add support for gd25q16 > > Marek Vasut (1): > mtd: spi-nor: cqspi: Fix build on arches missing readsl/writesl > > Mika Westerberg (3): > spi-nor: Add support for Intel SPI serial flash controller > mfd: lpc_ich: Add support for SPI serial flash host controller > mfd: lpc_ich: Add support for Intel Apollo Lake SoC > > Ricardo Ribalda Delgado (2): > mtd: spi-nor: Add support for S3AN spi-nor devices > mtd: spi-nor: Fix S3AN addressing calculation > > Uwe Kleine-König (1): > mtd: spi-nor: add dt support for Everspin MRAMs > > Victor Shyba (1): > mtd: spi-nor: Add lock/unlock support for f25l32pa > > Yunhui Cui (2): > mtd:fsl-quadspi:use the property fields of SPI-NOR > mtd: fsl-quadspi: Rename SEQID_QUAD_READ to SEQID_READ > > Documentation/devicetree/bindings/mtd/aspeed-smc.txt | 51 +++++ > Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 2 + > Documentation/devicetree/bindings/mtd/mtk-quadspi.txt | 8 +- > Documentation/mtd/intel-spi.txt | 88 +++++++++ > drivers/mfd/lpc_ich.c | 131 +++++++++++++ > drivers/mtd/devices/m25p80.c | 9 +- > drivers/mtd/devices/serial_flash_cmds.h | 7 - > drivers/mtd/devices/st_spi_fsm.c | 28 +-- > drivers/mtd/spi-nor/Kconfig | 32 +++- > drivers/mtd/spi-nor/Makefile | 3 + > drivers/mtd/spi-nor/aspeed-smc.c | 758 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/mtd/spi-nor/cadence-quadspi.c | 10 +- > drivers/mtd/spi-nor/fsl-quadspi.c | 48 ++--- > drivers/mtd/spi-nor/intel-spi-platform.c | 57 ++++++ > drivers/mtd/spi-nor/intel-spi.c | 777 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/mtd/spi-nor/intel-spi.h | 24 +++ > drivers/mtd/spi-nor/spi-nor.c | 271 ++++++++++++++++++++++++--- > drivers/spi/spi-bcm-qspi.c | 6 +- > include/linux/mfd/lpc_ich.h | 3 + > include/linux/mtd/spi-nor.h | 34 +++- > include/linux/platform_data/intel-spi.h | 31 +++ > 21 files changed, 2278 insertions(+), 100 deletions(-) > create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt > create mode 100644 Documentation/mtd/intel-spi.txt > create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c > create mode 100644 drivers/mtd/spi-nor/intel-spi-platform.c > create mode 100644 drivers/mtd/spi-nor/intel-spi.c > create mode 100644 drivers/mtd/spi-nor/intel-spi.h > create mode 100644 include/linux/platform_data/intel-spi.h >
Am 08.02.2017 um 02:09 schrieb Marek Vasut: > On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: >> Hi Brian, >> >> this is the spi-nor PR for 4.11. There are few new controller drivers and >> support to some memory parts. Otherwise about the spi-nor framework itself, >> the main update is the support of the 4-byte address instruction set, which >> is stateless, as opposed to the 4-byte address mode, which is stateful hence >> creates issues with some bootloaders when spurious reboot occurs. >> >> Best regards, >> >> Cyrille >> >> The following changes since commit 7ce7d89f48834cefece7804d38fc5d85382edf77: >> >> Linux 4.10-rc1 (2016-12-25 16:13:08 -0800) >> >> are available in the git repository at: >> >> git://github.com/spi-nor/linux.git tags/spi-nor/for-4.11 >> >> for you to fetch changes up to dc12bcccadafb5441170e6b7c8a438c91d4f385b: >> >> mtd: spi-nor: cqspi: remove redundant dead code on error return check (2017-02-02 10:42:36 +0100) >> >> ---------------------------------------------------------------- >> This pull request contains the following notable changes: >> - add support to the 4-byte address instruction set. >> - add support to new memory parts. >> - add support to S3AN memories. >> - add support to the Intel SPI controller. >> - add support to the Aspeed AST2400 and AST2550 controllers. >> - fix max SPI transfer and message sizes in m25p80_read(). >> - fix the Candence QSPI driver. >> - fix the Freescale QSPI driver. >> >> ---------------------------------------------------------------- >> Colin Ian King (1): >> mtd: spi-nor: cqspi: remove redundant dead code on error return check >> >> Cyrille Pitchen (5): >> mtd: spi-nor: improve macronix_quad_enable() >> mtd: spi-nor: remove WARN_ONCE() message in spi_nor_write() >> mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes >> mtd: spi-nor: add a stateless method to support memory size above 128Mib >> Merge tag 'ib-mfd-mtd-v4.11' of git://git.kernel.org/.../lee/mfd >> >> Cédric Le Goater (4): >> mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC >> mtd: aspeed: add memory controllers for the Aspeed AST2400 SoC >> mtd: spi-nor: bindings for the Aspeed memory controllers >> mtd: aspeed: fix compile warning in aspeed_smc_read_from_ahb() >> >> Guochun Mao (1): >> Documentation: mtk-quadspi: update DT bindings >> >> Heiner Kallweit (1): >> mtd: m25p80: consider max message size in m25p80_read > > I cannot say I'm super-happy about this particular patch, still ... > W/o it at least the fsl-espi driver is broken for transfers >= 64kB. >> Kamal Dasu (1): >> mtd: spi-nor: Add support for gd25q16 >> >> Marek Vasut (1): >> mtd: spi-nor: cqspi: Fix build on arches missing readsl/writesl >> >> Mika Westerberg (3): >> spi-nor: Add support for Intel SPI serial flash controller >> mfd: lpc_ich: Add support for SPI serial flash host controller >> mfd: lpc_ich: Add support for Intel Apollo Lake SoC >> >> Ricardo Ribalda Delgado (2): >> mtd: spi-nor: Add support for S3AN spi-nor devices >> mtd: spi-nor: Fix S3AN addressing calculation >> >> Uwe Kleine-König (1): >> mtd: spi-nor: add dt support for Everspin MRAMs >> >> Victor Shyba (1): >> mtd: spi-nor: Add lock/unlock support for f25l32pa >> >> Yunhui Cui (2): >> mtd:fsl-quadspi:use the property fields of SPI-NOR >> mtd: fsl-quadspi: Rename SEQID_QUAD_READ to SEQID_READ >> >> Documentation/devicetree/bindings/mtd/aspeed-smc.txt | 51 +++++ >> Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 2 + >> Documentation/devicetree/bindings/mtd/mtk-quadspi.txt | 8 +- >> Documentation/mtd/intel-spi.txt | 88 +++++++++ >> drivers/mfd/lpc_ich.c | 131 +++++++++++++ >> drivers/mtd/devices/m25p80.c | 9 +- >> drivers/mtd/devices/serial_flash_cmds.h | 7 - >> drivers/mtd/devices/st_spi_fsm.c | 28 +-- >> drivers/mtd/spi-nor/Kconfig | 32 +++- >> drivers/mtd/spi-nor/Makefile | 3 + >> drivers/mtd/spi-nor/aspeed-smc.c | 758 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/mtd/spi-nor/cadence-quadspi.c | 10 +- >> drivers/mtd/spi-nor/fsl-quadspi.c | 48 ++--- >> drivers/mtd/spi-nor/intel-spi-platform.c | 57 ++++++ >> drivers/mtd/spi-nor/intel-spi.c | 777 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/mtd/spi-nor/intel-spi.h | 24 +++ >> drivers/mtd/spi-nor/spi-nor.c | 271 ++++++++++++++++++++++++--- >> drivers/spi/spi-bcm-qspi.c | 6 +- >> include/linux/mfd/lpc_ich.h | 3 + >> include/linux/mtd/spi-nor.h | 34 +++- >> include/linux/platform_data/intel-spi.h | 31 +++ >> 21 files changed, 2278 insertions(+), 100 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt >> create mode 100644 Documentation/mtd/intel-spi.txt >> create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c >> create mode 100644 drivers/mtd/spi-nor/intel-spi-platform.c >> create mode 100644 drivers/mtd/spi-nor/intel-spi.c >> create mode 100644 drivers/mtd/spi-nor/intel-spi.h >> create mode 100644 include/linux/platform_data/intel-spi.h >> > >
Hi all, Le 08/02/2017 à 06:52, Heiner Kallweit a écrit : > Am 08.02.2017 um 02:09 schrieb Marek Vasut: >> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: [...] >>> Heiner Kallweit (1): >>> mtd: m25p80: consider max message size in m25p80_read >> >> I cannot say I'm super-happy about this particular patch, still ... >> > W/o it at least the fsl-espi driver is broken for transfers >= 64kB. > We asked Mark Brown about the need to check the limits for the size of SPI transfers and messages and I've understood his answer correctly, he told us we should check those limits: http://patchwork.ozlabs.org/patch/688176/ "> Does this imply that we have to fix every single driver this way ?" "Ideally. Any SPI driver that has an upper limit really ought to export it, and any driver that might bump into one of those limits should check. A lot of drivers are either unlikely to be deployed in a system that has relevant limits or tend to only do short enough transfers though - if systems are currently working it's just neatening things up really" Then the discussion ended there. I didn't see any comment after that so I just thought the discussion was over and everybody agreed. Besides, this patch doesn't introduce the check of SPI controller limits but only extend this check. What I mean is that the m25p80 driver already checks the maximum transfer size. This check was introduced by commit 95193796256c ("mtd: m25p80: read in spi_max_transfer_size chunks") Then if we already check the SPI max transfer size, I guess it make sense to also test other limits such as the SPI max message size. Finally, I think it would be very difficult and dirty to try to fix this bug only from the SPI controller driver. If m25p80_read() tries to read too many data based on what the SPI controller can actually read in a single operation, we would have to split the (Fast) Read command into two (Fast) Read commands, incrementing the address bytes as needed before sending the second command to the SPI bus. Please note that this split is already handled by spi_nor_read() when nor->read() returns a size lower than its len parameter. However, if we try to fix the issue at the SPI controller driver side, once we start to use the SPI API with spi_transfer and spi_message structures, we loose protocol info. SPI transfers and messages only carry stream of data/bytes, hence we loose the SPI flash protocol split: instruction | address | mode | dummy | data. When using the regular SPI API, the SPI controller driver should not try to recover that protocol split. It should even not be aware that the SPI device is indeed a SPI flash. Also there is no mean to recover the protocol split out without duplicating many parts of the spi-nor framework: - 3-byte or 4-byte address ? - how many dummy cycle bytes ? => this number depends on both the instruction op code and the memory manufacturer. Also I don't think it's a good think when SPI controller drivers by-pass or duplicate the selection of the right flash parameters, op codes and so on. When they do so, they do it so it works with very specific memory parts and they just don't care about some other memory parts that work slightly differently. Then it becomes a mess to maintain such drivers. With Heiner's patch, the issue is fixed by changing only one line and the bandwidth performance of read operations should not be impacted. I don't think his solution is that dirty as compared to the only other proposed solution: fix it in the SPI controller driver. The SPI controller driver could also implement its own version of spi_flash_read() mainly duplicating the code of m25p80_read() but it doesn't sound like a cleaner fix, IMHO. Best regards, Cyrille
+ Mark, as the disagreement touches his area Hi, On Wed, Feb 08, 2017 at 11:36:10AM +0100, Cyrille Pitchen wrote: > Le 08/02/2017 à 06:52, Heiner Kallweit a écrit : > > Am 08.02.2017 um 02:09 schrieb Marek Vasut: > >> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: > [...] > >>> Heiner Kallweit (1): > >>> mtd: m25p80: consider max message size in m25p80_read > >> > >> I cannot say I'm super-happy about this particular patch, still ... > >> > > W/o it at least the fsl-espi driver is broken for transfers >= 64kB. > > > > We asked Mark Brown about the need to check the limits for the size of SPI > transfers and messages and I've understood his answer correctly, he told us > we should check those limits: > > http://patchwork.ozlabs.org/patch/688176/ > > "> Does this imply that we have to fix every single driver this way ?" > > "Ideally. Any SPI driver that has an upper limit really ought to export > it, and any driver that might bump into one of those limits should > check. A lot of drivers are either unlikely to be deployed in a system > that has relevant limits or tend to only do short enough transfers > though - if systems are currently working it's just neatening things up > really" > > Then the discussion ended there. I didn't see any comment after that so I > just thought the discussion was over and everybody agreed. I thought we more or less settled on this a while ago. But I could be wrong. > Besides, this patch doesn't introduce the check of SPI controller limits > but only extend this check. What I mean is that the m25p80 driver already > checks the maximum transfer size. This check was introduced by commit > > 95193796256c ("mtd: m25p80: read in spi_max_transfer_size chunks") > > Then if we already check the SPI max transfer size, I guess it make sense > to also test other limits such as the SPI max message size. Yes, if this patch is wrong, then I'd expect commit 95193796256c was also wrong. The central disagreement seems to be on the existence of upper limits for transfer and/or message size. Marek seemed to suggest elsewhere that there is usually no upper limit (or that "upper limits" are usually just driver bugs that are being worked around). I don't disagree that often these might just be driver bugs, but I *do* disagree that there are never limits. I think Mark (? or somebody else, not reading too closely) suggested that there are just few drivers that tickle the limits. One example that shows both sides of this coin: 5185a81c02d4 "spi: rockchip: limit transfers to (64K - 1) bytes" The hardware has a hard limit on transfers, at least, which can only be 64K. For some reason there's an off-by-one error (probably a driver bug?), so I cheated and made it one smaller. [...] (snip the rest about trying to split spi_messages within the SPI driver; I agree that would be awful.) But one question: *why* exactly is the transfer or message limit so fixed? If a SPI controller allows manual control over the CS line, then why would anybody care if a sequence of data gets split up into multiple HW sequences (e.g., for a long spi_transfer, why can't we split it into 2 sub-transfers, while keeping the CS asserted?). For the above rockchip SPI controller, AIUI we could actually support the "transfer limit" by doing this. And in fact, it could probably be done within the SPI core code, instead either the SPI driver or the MTD driver. Now, in this case, *transfer* vs. *message* seems to have a difference, in whether or not you have guarantees about the CS line. The CS must stay asserted (barring the 'cs_change' flag) between transfers, but not between messages. So the one thing I can see that might support Marek's disagreement here: does the FSL ESPI hardware support controlling the CS line? It seems like currently, the driver just uses a few fixed settings for its CSMODE register fields, whereas it might be able to support ->transfer_one() (and flexible CS assertion, and splitting large transactions into smaller chunks) if you handled that differently... Heiner (or others), any thoughts on the above? I might be very mistaken, especially since I'm not intimately familiar with a lot of SPI drivers. Regardless, I think this patch is OK, given the established practice, and we should take it. We might want to push back to handle this better in the SPI layer, if at all possible. One thing I do object to: why is this change marked for stable? It's not our fault you broke your duct-taped-together SPI driver in 4.9. I don't think backporting features is the way to satisfy the -stable requirements. Maybe some reverts would be the right way... Brian
On Wed, Feb 08, 2017 at 11:09:39AM -0800, Brian Norris wrote: > But one question: *why* exactly is the transfer or message limit so > fixed? If a SPI controller allows manual control over the CS line, then > why would anybody care if a sequence of data gets split up into multiple > HW sequences (e.g., for a long spi_transfer, why can't we split it into > 2 sub-transfers, while keeping the CS asserted?). Not every device will provide software control of the chip select line so it's possible you might hit limits there. If a device does allow software control then yes, we should be handling this transparently in the core and splitting up longer transfers into chunks the device can handle and in fact this is already done in spi_split_transfers_maxsize(). We're missing masking that out in spi_max_transfer_size() though so clients won't know if we can handle things, it's overly conservative.
On 8 February 2017 at 20:09, Brian Norris <computersforpeace@gmail.com> wrote: > + Mark, as the disagreement touches his area > > Hi, > > On Wed, Feb 08, 2017 at 11:36:10AM +0100, Cyrille Pitchen wrote: >> Le 08/02/2017 à 06:52, Heiner Kallweit a écrit : >> > Am 08.02.2017 um 02:09 schrieb Marek Vasut: >> >> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: >> [...] >> >>> Heiner Kallweit (1): >> >>> mtd: m25p80: consider max message size in m25p80_read [...] > One example that shows both sides of this coin: > > 5185a81c02d4 "spi: rockchip: limit transfers to (64K - 1) bytes" > > The hardware has a hard limit on transfers, at least, which can only be > 64K. For some reason there's an off-by-one error (probably a driver > bug?), so I cheated and made it one smaller. > > [...] > > (snip the rest about trying to split spi_messages within the SPI driver; > I agree that would be awful.) > > But one question: *why* exactly is the transfer or message limit so > fixed? If a SPI controller allows manual control over the CS line, then > why would anybody care if a sequence of data gets split up into multiple > HW sequences (e.g., for a long spi_transfer, why can't we split it into > 2 sub-transfers, while keeping the CS asserted?). > > For the above rockchip SPI controller, AIUI we could actually support > the "transfer limit" by doing this. And in fact, it could probably be > done within the SPI core code, instead either the SPI driver or the MTD > driver. Yes, this is something repeatedly open-coded in SPI drivers, unfortunately. In this case limiting the transfer size to 64k rather than doing another clone of the manual CS handling actually looks reasonable. It's not like anything but mtd_debug normally triggers that limit. > > Now, in this case, *transfer* vs. *message* seems to have a difference, > in whether or not you have guarantees about the CS line. The CS must > stay asserted (barring the 'cs_change' flag) between transfers, but not > between messages. > > So the one thing I can see that might support Marek's disagreement here: > does the FSL ESPI hardware support controlling the CS line? It seems > like currently, the driver just uses a few fixed settings for its CSMODE > register fields, whereas it might be able to support ->transfer_one() > (and flexible CS assertion, and splitting large transactions into > smaller chunks) if you handled that differently... It was argued that while this is the case with most SPI controllers it is not the case with FSL ESPI. I do not recall anybody producing any datasheet/manual supporting this, though. It's not like this is a confidential piece of hardware, right? Thanks Michal
Am 08.02.2017 um 20:09 schrieb Brian Norris: > + Mark, as the disagreement touches his area > > Hi, > > On Wed, Feb 08, 2017 at 11:36:10AM +0100, Cyrille Pitchen wrote: >> Le 08/02/2017 à 06:52, Heiner Kallweit a écrit : >>> Am 08.02.2017 um 02:09 schrieb Marek Vasut: >>>> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: >> [...] >>>>> Heiner Kallweit (1): >>>>> mtd: m25p80: consider max message size in m25p80_read >>>> >>>> I cannot say I'm super-happy about this particular patch, still ... >>>> >>> W/o it at least the fsl-espi driver is broken for transfers >= 64kB. >>> >> >> We asked Mark Brown about the need to check the limits for the size of SPI >> transfers and messages and I've understood his answer correctly, he told us >> we should check those limits: >> >> http://patchwork.ozlabs.org/patch/688176/ >> >> "> Does this imply that we have to fix every single driver this way ?" >> >> "Ideally. Any SPI driver that has an upper limit really ought to export >> it, and any driver that might bump into one of those limits should >> check. A lot of drivers are either unlikely to be deployed in a system >> that has relevant limits or tend to only do short enough transfers >> though - if systems are currently working it's just neatening things up >> really" >> >> Then the discussion ended there. I didn't see any comment after that so I >> just thought the discussion was over and everybody agreed. > > I thought we more or less settled on this a while ago. But I could be > wrong. > >> Besides, this patch doesn't introduce the check of SPI controller limits >> but only extend this check. What I mean is that the m25p80 driver already >> checks the maximum transfer size. This check was introduced by commit >> >> 95193796256c ("mtd: m25p80: read in spi_max_transfer_size chunks") >> >> Then if we already check the SPI max transfer size, I guess it make sense >> to also test other limits such as the SPI max message size. > > Yes, if this patch is wrong, then I'd expect commit 95193796256c was > also wrong. > > The central disagreement seems to be on the existence of upper limits > for transfer and/or message size. Marek seemed to suggest elsewhere that > there is usually no upper limit (or that "upper limits" are usually just > driver bugs that are being worked around). I don't disagree that often > these might just be driver bugs, but I *do* disagree that there are > never limits. I think Mark (? or somebody else, not reading too closely) > suggested that there are just few drivers that tickle the limits. > > One example that shows both sides of this coin: > > 5185a81c02d4 "spi: rockchip: limit transfers to (64K - 1) bytes" > > The hardware has a hard limit on transfers, at least, which can only be > 64K. For some reason there's an off-by-one error (probably a driver > bug?), so I cheated and made it one smaller. > > [...] > > (snip the rest about trying to split spi_messages within the SPI driver; > I agree that would be awful.) > > But one question: *why* exactly is the transfer or message limit so > fixed? If a SPI controller allows manual control over the CS line, then > why would anybody care if a sequence of data gets split up into multiple > HW sequences (e.g., for a long spi_transfer, why can't we split it into > 2 sub-transfers, while keeping the CS asserted?). > > For the above rockchip SPI controller, AIUI we could actually support > the "transfer limit" by doing this. And in fact, it could probably be > done within the SPI core code, instead either the SPI driver or the MTD > driver. > > Now, in this case, *transfer* vs. *message* seems to have a difference, > in whether or not you have guarantees about the CS line. The CS must > stay asserted (barring the 'cs_change' flag) between transfers, but not > between messages. > > So the one thing I can see that might support Marek's disagreement here: > does the FSL ESPI hardware support controlling the CS line? It seems > like currently, the driver just uses a few fixed settings for its CSMODE > register fields, whereas it might be able to support ->transfer_one() > (and flexible CS assertion, and splitting large transactions into > smaller chunks) if you handled that differently... > > Heiner (or others), any thoughts on the above? I might be very mistaken, > especially since I'm not intimately familiar with a lot of SPI drivers. > FSL ESPI has hardware-controlled CS. You program the length of transfer (max. 64K) and when starting the transfer HW will assert CS and after having transferred the programmed number of bytes CS is deasserted internally. And because of course this question came up: No, you can't use pinmux to turn the CS line into a GPIO. It's only possible to change the complete block of SPI signal to GPIOs. But then we would end up with bitbanging, loose the HW FIFO etc. Heiner > Regardless, I think this patch is OK, given the established practice, > and we should take it. We might want to push back to handle this better > in the SPI layer, if at all possible. > > One thing I do object to: why is this change marked for stable? It's not > our fault you broke your duct-taped-together SPI driver in 4.9. I don't > think backporting features is the way to satisfy the -stable > requirements. Maybe some reverts would be the right way... > > Brian >
Am 08.02.2017 um 20:40 schrieb Michal Suchanek: > On 8 February 2017 at 20:09, Brian Norris <computersforpeace@gmail.com> wrote: >> + Mark, as the disagreement touches his area >> >> Hi, >> >> On Wed, Feb 08, 2017 at 11:36:10AM +0100, Cyrille Pitchen wrote: >>> Le 08/02/2017 à 06:52, Heiner Kallweit a écrit : >>>> Am 08.02.2017 um 02:09 schrieb Marek Vasut: >>>>> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: >>> [...] >>>>>> Heiner Kallweit (1): >>>>>> mtd: m25p80: consider max message size in m25p80_read > [...] >> One example that shows both sides of this coin: >> >> 5185a81c02d4 "spi: rockchip: limit transfers to (64K - 1) bytes" >> >> The hardware has a hard limit on transfers, at least, which can only be >> 64K. For some reason there's an off-by-one error (probably a driver >> bug?), so I cheated and made it one smaller. >> >> [...] >> >> (snip the rest about trying to split spi_messages within the SPI driver; >> I agree that would be awful.) >> >> But one question: *why* exactly is the transfer or message limit so >> fixed? If a SPI controller allows manual control over the CS line, then >> why would anybody care if a sequence of data gets split up into multiple >> HW sequences (e.g., for a long spi_transfer, why can't we split it into >> 2 sub-transfers, while keeping the CS asserted?). >> >> For the above rockchip SPI controller, AIUI we could actually support >> the "transfer limit" by doing this. And in fact, it could probably be >> done within the SPI core code, instead either the SPI driver or the MTD >> driver. > > Yes, this is something repeatedly open-coded in SPI drivers, unfortunately. > > In this case limiting the transfer size to 64k rather than doing > another clone of the manual CS handling actually looks reasonable. > It's not like anything but mtd_debug normally triggers that limit. > >> >> Now, in this case, *transfer* vs. *message* seems to have a difference, >> in whether or not you have guarantees about the CS line. The CS must >> stay asserted (barring the 'cs_change' flag) between transfers, but not >> between messages. >> >> So the one thing I can see that might support Marek's disagreement here: >> does the FSL ESPI hardware support controlling the CS line? It seems >> like currently, the driver just uses a few fixed settings for its CSMODE >> register fields, whereas it might be able to support ->transfer_one() >> (and flexible CS assertion, and splitting large transactions into >> smaller chunks) if you handled that differently... > > It was argued that while this is the case with most SPI controllers it > is not the case with FSL ESPI. I do not recall anybody producing any > datasheet/manual supporting this, though. It's not like this is a > confidential piece of hardware, right? > The datasheet (here called Reference Manual) is available on the NXP (ex Freescale) website, you just have to register. http://www.nxp.com/products/microcontrollers-and-processors/power-architecture-processors/qoriq-platforms/p-series/qoriq-p1010-and-p1014-low-power-communications-processors-with-trust-architecture:P1010?tab=Documentation_Tab > Thanks > > Michal >
On Wed, Feb 08, 2017 at 08:40:17PM +0100, Michal Suchanek wrote: > Yes, this is something repeatedly open-coded in SPI drivers, unfortunately. > In this case limiting the transfer size to 64k rather than doing > another clone of the manual CS handling actually looks reasonable. > It's not like anything but mtd_debug normally triggers that limit. Any driver that's open coding this is buggy and should be fixed, it should mainly be legacy drivers - some of the older drivers have a lot of problems.
On Wed, Feb 08, 2017 at 07:22:36PM +0000, Mark Brown wrote: > On Wed, Feb 08, 2017 at 11:09:39AM -0800, Brian Norris wrote: > > > But one question: *why* exactly is the transfer or message limit so > > fixed? If a SPI controller allows manual control over the CS line, then > > why would anybody care if a sequence of data gets split up into multiple > > HW sequences (e.g., for a long spi_transfer, why can't we split it into > > 2 sub-transfers, while keeping the CS asserted?). > > Not every device will provide software control of the chip select line > so it's possible you might hit limits there. If a device does allow > software control then yes, we should be handling this transparently in > the core and splitting up longer transfers into chunks the device can > handle Thanks for the info. Then it sounds like that really should be the dividing line; driver-visible limitations ideally should only happen when CS is not software-controllable. (As Heiner points out, it seems FSL ESPI is one such case.) All other cases can be handled by some form of splitting, in the SPI core. So that actually sounds like drivers like m25p80 should never have to account for spi_max_transfer_size(), but instead should have to account for only spi_max_message_size(). But that's all only a "should" -- nobody has done the work to do this properly, and so right now, we need to check for both. > and in fact this is already done in spi_split_transfers_maxsize(). > > We're missing masking that out in spi_max_transfer_size() though so > clients won't know if we can handle things, it's overly conservative. Huh, I've never noticed spi_split_transfers_maxsize(). That looks like work for it got dropped on the floor somewhere though; nobody is using it yet, and it's over a year since its introduction. Brian
On Wed, Feb 08, 2017 at 12:39:22PM -0800, Brian Norris wrote: > So that actually sounds like drivers like m25p80 should never have to > account for spi_max_transfer_size(), but instead should have to account > for only spi_max_message_size(). You might have both depending on how the hardware is structured, some things support multiple transfers while controlling the chip select internally. > > and in fact this is already done in spi_split_transfers_maxsize(). > > We're missing masking that out in spi_max_transfer_size() though so > > clients won't know if we can handle things, it's overly conservative. > Huh, I've never noticed spi_split_transfers_maxsize(). That looks like > work for it got dropped on the floor somewhere though; nobody is using > it yet, and it's over a year since its introduction. Yeah, the optimisation for rpi ground to a halt. We should probably just unconditionally do that in the core if something declares a maximum transfer size.
On Wed, Feb 08, 2017 at 11:09:39AM -0800, Brian Norris wrote: > On Wed, Feb 08, 2017 at 11:36:10AM +0100, Cyrille Pitchen wrote: > > Le 08/02/2017 à 06:52, Heiner Kallweit a écrit : > > > Am 08.02.2017 um 02:09 schrieb Marek Vasut: > > >> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: > > [...] > > >>> Heiner Kallweit (1): > > >>> mtd: m25p80: consider max message size in m25p80_read > > >> > > >> I cannot say I'm super-happy about this particular patch, still ... > > >> > > > W/o it at least the fsl-espi driver is broken for transfers >= 64kB. > > > [...] > One thing I do object to: why is this change marked for stable? It's not > our fault you broke your duct-taped-together SPI driver in 4.9. I don't > think backporting features is the way to satisfy the -stable > requirements. Maybe some reverts would be the right way... OK, I've reviewed the rest of the pull request as well as I care for. I still have this one objection: I don't think this belongs in -stable. Am I crazy? Or else Cyrille, can you remove the stable tag and resend? Or I can go rewrite your history for you and do this... Brian P.S. Also, this stuck out at me in the diffs, and checkpatch complained too. Not a real big deal: WARNING:BLOCK_COMMENT_STYLE: Block comments should align the * on each line #42: FILE: drivers/mtd/spi-nor/spi-nor.c:83: + * ATMEL flashes) + */ total: 0 errors, 1 warnings, 264 lines checked
Am 10.02.2017 um 03:46 schrieb Brian Norris: > On Wed, Feb 08, 2017 at 11:09:39AM -0800, Brian Norris wrote: >> On Wed, Feb 08, 2017 at 11:36:10AM +0100, Cyrille Pitchen wrote: >>> Le 08/02/2017 à 06:52, Heiner Kallweit a écrit : >>>> Am 08.02.2017 um 02:09 schrieb Marek Vasut: >>>>> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: >>> [...] >>>>>> Heiner Kallweit (1): >>>>>> mtd: m25p80: consider max message size in m25p80_read >>>>> >>>>> I cannot say I'm super-happy about this particular patch, still ... >>>>> >>>> W/o it at least the fsl-espi driver is broken for transfers >= 64kB. >>>> > [...] > >> One thing I do object to: why is this change marked for stable? It's not >> our fault you broke your duct-taped-together SPI driver in 4.9. I don't >> think backporting features is the way to satisfy the -stable >> requirements. Maybe some reverts would be the right way... > The patch was submitted in Aug 2016 already and there was also an inquiry to get it into 4.9: http://lists.infradead.org/pipermail/linux-mtd/2016-September/069363.html But due to whatever circumstances nothing happened and there was no response. > OK, I've reviewed the rest of the pull request as well as I care for. > I still have this one objection: I don't think this belongs in -stable. > > Am I crazy? Or else Cyrille, can you remove the stable tag and resend? > Or I can go rewrite your history for you and do this... > > Brian > > P.S. Also, this stuck out at me in the diffs, and checkpatch complained > too. Not a real big deal: > > WARNING:BLOCK_COMMENT_STYLE: Block comments should align the * on each > line > #42: FILE: drivers/mtd/spi-nor/spi-nor.c:83: > + * ATMEL flashes) > + */ > > total: 0 errors, 1 warnings, 264 lines checked > >
Le 10/02/2017 à 03:46, Brian Norris a écrit : > On Wed, Feb 08, 2017 at 11:09:39AM -0800, Brian Norris wrote: >> On Wed, Feb 08, 2017 at 11:36:10AM +0100, Cyrille Pitchen wrote: >>> Le 08/02/2017 à 06:52, Heiner Kallweit a écrit : >>>> Am 08.02.2017 um 02:09 schrieb Marek Vasut: >>>>> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: >>> [...] >>>>>> Heiner Kallweit (1): >>>>>> mtd: m25p80: consider max message size in m25p80_read >>>>> >>>>> I cannot say I'm super-happy about this particular patch, still ... >>>>> >>>> W/o it at least the fsl-espi driver is broken for transfers >= 64kB. >>>> > [...] > >> One thing I do object to: why is this change marked for stable? It's not >> our fault you broke your duct-taped-together SPI driver in 4.9. I don't >> think backporting features is the way to satisfy the -stable >> requirements. Maybe some reverts would be the right way... > > OK, I've reviewed the rest of the pull request as well as I care for. > I still have this one objection: I don't think this belongs in -stable. > > Am I crazy? Or else Cyrille, can you remove the stable tag and resend? > Or I can go rewrite your history for you and do this... > No problem, I'm rewriting the history to prepare a new PR :) So 2 modifications: 1 - remove the stable tag from "mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes" 2 - fix * alignment in comment in "mtd: spi-nor: Add support for S3AN spi-nor devices" Best regards, Cyrille > Brian > > P.S. Also, this stuck out at me in the diffs, and checkpatch complained > too. Not a real big deal: > > WARNING:BLOCK_COMMENT_STYLE: Block comments should align the * on each > line > #42: FILE: drivers/mtd/spi-nor/spi-nor.c:83: > + * ATMEL flashes) > + */ > > total: 0 errors, 1 warnings, 264 lines checked > >
On 02/10/2017 07:39 AM, Heiner Kallweit wrote: > Am 10.02.2017 um 03:46 schrieb Brian Norris: >> On Wed, Feb 08, 2017 at 11:09:39AM -0800, Brian Norris wrote: >>> On Wed, Feb 08, 2017 at 11:36:10AM +0100, Cyrille Pitchen wrote: >>>> Le 08/02/2017 à 06:52, Heiner Kallweit a écrit : >>>>> Am 08.02.2017 um 02:09 schrieb Marek Vasut: >>>>>> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: >>>> [...] >>>>>>> Heiner Kallweit (1): >>>>>>> mtd: m25p80: consider max message size in m25p80_read >>>>>> >>>>>> I cannot say I'm super-happy about this particular patch, still ... >>>>>> >>>>> W/o it at least the fsl-espi driver is broken for transfers >= 64kB. >>>>> >> [...] >> >>> One thing I do object to: why is this change marked for stable? It's not >>> our fault you broke your duct-taped-together SPI driver in 4.9. I don't >>> think backporting features is the way to satisfy the -stable >>> requirements. Maybe some reverts would be the right way... >> > The patch was submitted in Aug 2016 already and there was also an inquiry > to get it into 4.9: > http://lists.infradead.org/pipermail/linux-mtd/2016-September/069363.html > > But due to whatever circumstances nothing happened and there was no > response. Is that a bugfix ?
Am 10.02.2017 um 14:07 schrieb Marek Vasut: > On 02/10/2017 07:39 AM, Heiner Kallweit wrote: >> Am 10.02.2017 um 03:46 schrieb Brian Norris: >>> On Wed, Feb 08, 2017 at 11:09:39AM -0800, Brian Norris wrote: >>>> On Wed, Feb 08, 2017 at 11:36:10AM +0100, Cyrille Pitchen wrote: >>>>> Le 08/02/2017 à 06:52, Heiner Kallweit a écrit : >>>>>> Am 08.02.2017 um 02:09 schrieb Marek Vasut: >>>>>>> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: >>>>> [...] >>>>>>>> Heiner Kallweit (1): >>>>>>>> mtd: m25p80: consider max message size in m25p80_read >>>>>>> >>>>>>> I cannot say I'm super-happy about this particular patch, still ... >>>>>>> >>>>>> W/o it at least the fsl-espi driver is broken for transfers >= 64kB. >>>>>> >>> [...] >>> >>>> One thing I do object to: why is this change marked for stable? It's not >>>> our fault you broke your duct-taped-together SPI driver in 4.9. I don't >>>> think backporting features is the way to satisfy the -stable >>>> requirements. Maybe some reverts would be the right way... >>> >> The patch was submitted in Aug 2016 already and there was also an inquiry >> to get it into 4.9: >> http://lists.infradead.org/pipermail/linux-mtd/2016-September/069363.html >> >> But due to whatever circumstances nothing happened and there was no >> response. > > Is that a bugfix ? > Certain changes to the fsl-espi driver and the patch we're talking about here were supposed to go into the same kernel release. The driver changes made it to mainline months ago, the m25p80 patch not. So yes, we have a bug in 4.9, transfers >= 64k fail on fsl-espi.
On 02/10/2017 06:56 PM, Heiner Kallweit wrote: > Am 10.02.2017 um 14:07 schrieb Marek Vasut: >> On 02/10/2017 07:39 AM, Heiner Kallweit wrote: >>> Am 10.02.2017 um 03:46 schrieb Brian Norris: >>>> On Wed, Feb 08, 2017 at 11:09:39AM -0800, Brian Norris wrote: >>>>> On Wed, Feb 08, 2017 at 11:36:10AM +0100, Cyrille Pitchen wrote: >>>>>> Le 08/02/2017 à 06:52, Heiner Kallweit a écrit : >>>>>>> Am 08.02.2017 um 02:09 schrieb Marek Vasut: >>>>>>>> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote: >>>>>> [...] >>>>>>>>> Heiner Kallweit (1): >>>>>>>>> mtd: m25p80: consider max message size in m25p80_read >>>>>>>> >>>>>>>> I cannot say I'm super-happy about this particular patch, still ... >>>>>>>> >>>>>>> W/o it at least the fsl-espi driver is broken for transfers >= 64kB. >>>>>>> >>>> [...] >>>> >>>>> One thing I do object to: why is this change marked for stable? It's not >>>>> our fault you broke your duct-taped-together SPI driver in 4.9. I don't >>>>> think backporting features is the way to satisfy the -stable >>>>> requirements. Maybe some reverts would be the right way... >>>> >>> The patch was submitted in Aug 2016 already and there was also an inquiry >>> to get it into 4.9: >>> http://lists.infradead.org/pipermail/linux-mtd/2016-September/069363.html >>> >>> But due to whatever circumstances nothing happened and there was no >>> response. >> >> Is that a bugfix ? >> > Certain changes to the fsl-espi driver and the patch we're talking about > here were supposed to go into the same kernel release. > The driver changes made it to mainline months ago, the m25p80 patch not. > So yes, we have a bug in 4.9, transfers >= 64k fail on fsl-espi. Maybe the fsl-espi change should be reverted for 4.9 ...
Hi Brian, this is the spi-nor PR for 4.11. There are few new controller drivers and support to some memory parts. Otherwise about the spi-nor framework itself, the main update is the support of the 4-byte address instruction set, which is stateless, as opposed to the 4-byte address mode, which is stateful hence creates issues with some bootloaders when spurious reboot occurs. Best regards, Cyrille The following changes since commit 7ce7d89f48834cefece7804d38fc5d85382edf77: Linux 4.10-rc1 (2016-12-25 16:13:08 -0800) are available in the git repository at: git://github.com/spi-nor/linux.git tags/spi-nor/for-4.11 for you to fetch changes up to dc12bcccadafb5441170e6b7c8a438c91d4f385b: mtd: spi-nor: cqspi: remove redundant dead code on error return check (2017-02-02 10:42:36 +0100) ---------------------------------------------------------------- This pull request contains the following notable changes: - add support to the 4-byte address instruction set. - add support to new memory parts. - add support to S3AN memories. - add support to the Intel SPI controller. - add support to the Aspeed AST2400 and AST2550 controllers. - fix max SPI transfer and message sizes in m25p80_read(). - fix the Candence QSPI driver. - fix the Freescale QSPI driver. ---------------------------------------------------------------- Colin Ian King (1): mtd: spi-nor: cqspi: remove redundant dead code on error return check Cyrille Pitchen (5): mtd: spi-nor: improve macronix_quad_enable() mtd: spi-nor: remove WARN_ONCE() message in spi_nor_write() mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes mtd: spi-nor: add a stateless method to support memory size above 128Mib Merge tag 'ib-mfd-mtd-v4.11' of git://git.kernel.org/.../lee/mfd Cédric Le Goater (4): mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC mtd: aspeed: add memory controllers for the Aspeed AST2400 SoC mtd: spi-nor: bindings for the Aspeed memory controllers mtd: aspeed: fix compile warning in aspeed_smc_read_from_ahb() Guochun Mao (1): Documentation: mtk-quadspi: update DT bindings Heiner Kallweit (1): mtd: m25p80: consider max message size in m25p80_read Kamal Dasu (1): mtd: spi-nor: Add support for gd25q16 Marek Vasut (1): mtd: spi-nor: cqspi: Fix build on arches missing readsl/writesl Mika Westerberg (3): spi-nor: Add support for Intel SPI serial flash controller mfd: lpc_ich: Add support for SPI serial flash host controller mfd: lpc_ich: Add support for Intel Apollo Lake SoC Ricardo Ribalda Delgado (2): mtd: spi-nor: Add support for S3AN spi-nor devices mtd: spi-nor: Fix S3AN addressing calculation Uwe Kleine-König (1): mtd: spi-nor: add dt support for Everspin MRAMs Victor Shyba (1): mtd: spi-nor: Add lock/unlock support for f25l32pa Yunhui Cui (2): mtd:fsl-quadspi:use the property fields of SPI-NOR mtd: fsl-quadspi: Rename SEQID_QUAD_READ to SEQID_READ Documentation/devicetree/bindings/mtd/aspeed-smc.txt | 51 +++++ Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 2 + Documentation/devicetree/bindings/mtd/mtk-quadspi.txt | 8 +- Documentation/mtd/intel-spi.txt | 88 +++++++++ drivers/mfd/lpc_ich.c | 131 +++++++++++++ drivers/mtd/devices/m25p80.c | 9 +- drivers/mtd/devices/serial_flash_cmds.h | 7 - drivers/mtd/devices/st_spi_fsm.c | 28 +-- drivers/mtd/spi-nor/Kconfig | 32 +++- drivers/mtd/spi-nor/Makefile | 3 + drivers/mtd/spi-nor/aspeed-smc.c | 758 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/mtd/spi-nor/cadence-quadspi.c | 10 +- drivers/mtd/spi-nor/fsl-quadspi.c | 48 ++--- drivers/mtd/spi-nor/intel-spi-platform.c | 57 ++++++ drivers/mtd/spi-nor/intel-spi.c | 777 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/mtd/spi-nor/intel-spi.h | 24 +++ drivers/mtd/spi-nor/spi-nor.c | 271 ++++++++++++++++++++++++--- drivers/spi/spi-bcm-qspi.c | 6 +- include/linux/mfd/lpc_ich.h | 3 + include/linux/mtd/spi-nor.h | 34 +++- include/linux/platform_data/intel-spi.h | 31 +++ 21 files changed, 2278 insertions(+), 100 deletions(-) create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt create mode 100644 Documentation/mtd/intel-spi.txt create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c create mode 100644 drivers/mtd/spi-nor/intel-spi-platform.c create mode 100644 drivers/mtd/spi-nor/intel-spi.c create mode 100644 drivers/mtd/spi-nor/intel-spi.h create mode 100644 include/linux/platform_data/intel-spi.h