| Message ID | 20200920112914.26501-1-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| Headers | show |
| Series | spi: dw: Add full Baikal-T1 SPI Controllers support | expand |
On Sun, Sep 20, 2020 at 02:28:44PM +0300, Serge Semin wrote: > First two patches are just cleanups to simplify the DW APB SSI device > initialization a bit. We suggest to discard the IRQ threshold macro as > unused and use a ternary operator to initialize the set_cs callback > instead of assigning-and-updating it. > Then we've discovered that the n_bytes field of the driver private data is > used by the DW APB SSI IRQ handler, which requires it to be initialized This is a *huge* patch series which is a bit unweildy to review (especially given the other 10+ patch series you sent at the same time), once you start getting over 10 patches it's time to pay attention to series length and the fact that you're outlining a bunch of tangentially related areas which could have been split out easily enough. It is much better to send smaller sets of patches at once, or if you're sending a lot then to split them into smaller serieses. This will tend to make the review more approachable which will in turn tend to make things go faster, people are much more likely to put off going through a huge series.
On Sun, 20 Sep 2020 14:28:44 +0300, Serge Semin wrote: > Originally I intended to merge a dedicated Baikal-T1 System Boot SPI > Controller driver into the kernel and leave the DW APB SSI driver > untouched. But after a long discussion (see the link at the bottom of the > letter) Mark and Andy persuaded me to integrate what we developed there > into the DW APB SSI core driver to be useful for another controllers, > which may have got the same peculiarities/problems as ours: > - No IRQ. > - No DMA. > - No GPIO CS, so a native CS is utilized. > - small Tx/Rx FIFO depth. > - Automatic CS assertion/de-assertion. > - Slow system bus. > All of them have been fixed in the framework of this patchset in some > extent at least for the SPI memory operations. As I expected it wasn't > that easy and the integration took that many patches as you can see from > the subject. Though some of them are mere cleanups or weakly related with > the subject fixes, but we just couldn't leave the code as is at some > places since we were working with the DW APB SSI driver anyway. Here is > what we did to fix the original DW APB SSI driver, to make it less messy. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/9] spi: dw: Discard IRQ threshold macro commit: 07918df724f2fed02327e3cbfe58a5d5568b2cc2 [2/9] spi: dw: Initialize n_bytes before the memory barrier commit: 8225c1c9a073c323f68833d136fcf94fbc75a275 [3/9] spi: spi-dw: Remove extraneous locking commit: 0b6bfad4cee4a3d5c49e01fa00284db4b676360e [4/9] spi: dw: Clear IRQ status on DW SPI controller reset commit: a128f6ecd56a32e559889145003425b0c7d406e3 [5/9] spi: dw: Disable all IRQs when controller is unused commit: a1d5aa6f7f97b15e8fd917169239088823471741 [6/9] spi: dw: Use relaxed IO-methods to access FIFOs commit: 7e31cea7d1e0f4b683dc45c21530cd3ee82559b4 [7/9] spi: dw: Discard DW SSI chip type storages commit: 675e7c9d71cedee3988b554c47971be77e72d2db [8/9] spi: dw: Convert CS-override to DW SPI capabilities commit: cc760f3143f53ea8387cd76cffc43bdc89db9df4 [9/9] spi: dw: Add KeemBay Master capability commit: ffb7ca54c95b4c76ad8a9aa1b2b16d61df2a7139 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi Mark On Tue, Sep 29, 2020 at 03:43:51PM +0100, Mark Brown wrote: > On Sun, Sep 20, 2020 at 02:28:44PM +0300, Serge Semin wrote: > > > First two patches are just cleanups to simplify the DW APB SSI device > > initialization a bit. We suggest to discard the IRQ threshold macro as > > unused and use a ternary operator to initialize the set_cs callback > > instead of assigning-and-updating it. > > > Then we've discovered that the n_bytes field of the driver private data is > > used by the DW APB SSI IRQ handler, which requires it to be initialized > > This is a *huge* patch series which is a bit unweildy to review > (especially given the other 10+ patch series you sent at the same time), Yeah, sorry about the bulky series. If most of the changes have been more complicated than that, less inter-dependent and less directed to having the code prepared for the main alterations I would have definitely split them up in different series. But the biggest part of the patchset is just a preparation before adding the mem-ops, poll-based transfers and Baikal-T1 SPI support. So having them submitted without the main part of the patchset would be just weird. The other 10+ patches were sent months ago. I've just resent them with minor alterations to get more attention.) Anyway since they concern an absolutely different functionality (DW APB SSI DMA driver) of course I've delivered them in the framework of the different patchset. > once you start getting over 10 patches it's time to pay attention to > series length and the fact that you're outlining a bunch of tangentially > related areas which could have been split out easily enough. It is much > better to send smaller sets of patches at once, or if you're sending a > lot then to split them into smaller serieses. This will tend to make > the review more approachable which will in turn tend to make things go > faster, people are much more likely to put off going through a huge > series. I see you have already merged in the first nine patches. So would you like me to split the rest of them up into two series or it would be ok to resend (if required) them as one series seeing it's not that bulky anymore? -Sergey
On Wed, Sep 30, 2020 at 01:43:03AM +0300, Serge Semin wrote: > On Tue, Sep 29, 2020 at 03:43:51PM +0100, Mark Brown wrote: > > This is a *huge* patch series which is a bit unweildy to review > > (especially given the other 10+ patch series you sent at the same time), > Yeah, sorry about the bulky series. If most of the changes have been more > complicated than that, less inter-dependent and less directed to having the code > prepared for the main alterations I would have definitely split them up in > different series. But the biggest part of the patchset is just a preparation > before adding the mem-ops, poll-based transfers and Baikal-T1 SPI support. So > having them submitted without the main part of the patchset would be just weird. One option with things like this is to just not send everything at once - even when split into multiple series it's a huge bulk of patches in an inbox. Unless the patches are obviously from their subjects repetitive people probably aren't getting far enough in to look at the actual patches or even their sizes before deciding it looks like a lot of work and putting things off for later. > I see you have already merged in the first nine patches. So would you like me > to split the rest of them up into two series or it would be ok to resend (if > required) them as one series seeing it's not that bulky anymore? Not all of the first 9, IIRC I skipped one I had comments on. If they can be split that would probably be helpful, if there are dependencies then it's not going to buy too much.
On Wed, Sep 30, 2020 at 12:04:04PM +0100, Mark Brown wrote: > On Wed, Sep 30, 2020 at 01:43:03AM +0300, Serge Semin wrote: > > On Tue, Sep 29, 2020 at 03:43:51PM +0100, Mark Brown wrote: > > > > This is a *huge* patch series which is a bit unweildy to review > > > (especially given the other 10+ patch series you sent at the same time), > > > Yeah, sorry about the bulky series. If most of the changes have been more > > complicated than that, less inter-dependent and less directed to having the code > > prepared for the main alterations I would have definitely split them up in > > different series. But the biggest part of the patchset is just a preparation > > before adding the mem-ops, poll-based transfers and Baikal-T1 SPI support. So > > having them submitted without the main part of the patchset would be just weird. > > One option with things like this is to just not send everything at once > - even when split into multiple series it's a huge bulk of patches in an > inbox. Unless the patches are obviously from their subjects repetitive > people probably aren't getting far enough in to look at the actual > patches or even their sizes before deciding it looks like a lot of work > and putting things off for later. > > > I see you have already merged in the first nine patches. So would you like me > > to split the rest of them up into two series or it would be ok to resend (if > > required) them as one series seeing it's not that bulky anymore? > > Not all of the first 9, IIRC I skipped one I had comments on. Yes, you skipped one and I've already given you my response on your comment about it: [PATCH 02/30] spi: dw: Use ternary op to init set_cs callback So have I responded to your comment on another patch: [PATCH 11/30] spi: dw: Add DWC SSI capability . I will need a response from you about them to go further with this patchset. > If they > can be split that would probably be helpful, if there are dependencies > then it's not going to buy too much. Well, all later patches depend on the changes introduced in the previous ones in one way or another. So in any case that will be an incremental series of patchsets otherwise they most likely won't get applied cleanly on the driver source code. For now we have got 21 patch left to review: I) First two ones you've given your comments on and are mostly related to the patches you have already merged in. 1. 688c17cad5c2 spi: dw: Use ternary op to init set_cs callback 2. 17d0b3abc03d spi: dw: Add DWC SSI capability II) Refactor the DW APB SSI controller config procedure. 3. 6a436c824961 spi: dw: Detach SPI device specific CR0 config method 4. 47614d60e44c spi: dw: Update SPI bus speed in a config function 5. df64a4961801 spi: dw: Simplify the SPI bus speed config procedure 6. 1a583b130bab spi: dw: Update Rx sample delay in the config function 7. 9f205a8939a2 spi: dw: Add DW SPI controller config structure III) Refactor IRQ-based SPI transfer procedure. 8. d4fa973a3f7c spi: dw: Refactor data IO procedure 9. d998b98e3d93 spi: dw: Refactor IRQ-based SPI transfer procedure 10. 7fc419af6e67 spi: dw: Perform IRQ setup in a dedicated function 11. d3dfd997379a spi: dw: Unmask IRQs after enabling the chip 12. 6ecf589320f3 spi: dw: Discard chip enabling on DMA setup error IV) Final preparation before adding the memory operations. 13. 84a03fad452c spi: dw: De-assert chip-select on reset 14. dd0212eb5738 spi: dw: Explicitly de-assert CS on SPI transfer completion 15. d1eea0f556cf spi: dw: Move num-of retries parameter to the header file 16. 3e70e5a6c1d9 spi: dw: Add generic DW SSI status-check method v) Introduce memory and poll-based operations. 17. 52d733f30464 spi: dw: Add memory operations support 18. c2f45eb3d662 spi: dw: Introduce max mem-ops SPI bus frequency setting 19. ccf08869b6bd spi: dw: Add poll-based SPI transfers support vI) Add Baikal-T1 glue-driver 20. a536c408f7aa dt-bindings: spi: dw: Add Baikal-T1 SPI Controllers 21. 791e68755ead spi: dw: Add Baikal-T1 SPI Controller glue driver If you want I can resend the series split up as I described above. Alternatively I can collect I) - III) into a one patchset and IV) - VI) into another one. So to speak I'll do in whatever scenario you prefer. Just tell me which one is more suitable for you to review. In anyway we need to settle the issues regarding the first two patches. Please give me your answers on the comments I've left there in response to your comments.) -Sergey
On Wed, Sep 30, 2020 at 04:11:15PM +0300, Serge Semin wrote: > On Wed, Sep 30, 2020 at 12:04:04PM +0100, Mark Brown wrote: > > Not all of the first 9, IIRC I skipped one I had comments on. > Yes, you skipped one and I've already given you my response on your comment > about it: [PATCH 02/30] spi: dw: Use ternary op to init set_cs callback > So have I responded to your comment on another patch: > [PATCH 11/30] spi: dw: Add DWC SSI capability . > I will need a response from you about them to go further with this patchset. I'm not sure I saw any concrete questions with those?
Originally I intended to merge a dedicated Baikal-T1 System Boot SPI Controller driver into the kernel and leave the DW APB SSI driver untouched. But after a long discussion (see the link at the bottom of the letter) Mark and Andy persuaded me to integrate what we developed there into the DW APB SSI core driver to be useful for another controllers, which may have got the same peculiarities/problems as ours: - No IRQ. - No DMA. - No GPIO CS, so a native CS is utilized. - small Tx/Rx FIFO depth. - Automatic CS assertion/de-assertion. - Slow system bus. All of them have been fixed in the framework of this patchset in some extent at least for the SPI memory operations. As I expected it wasn't that easy and the integration took that many patches as you can see from the subject. Though some of them are mere cleanups or weakly related with the subject fixes, but we just couldn't leave the code as is at some places since we were working with the DW APB SSI driver anyway. Here is what we did to fix the original DW APB SSI driver, to make it less messy. First two patches are just cleanups to simplify the DW APB SSI device initialization a bit. We suggest to discard the IRQ threshold macro as unused and use a ternary operator to initialize the set_cs callback instead of assigning-and-updating it. Then we've discovered that the n_bytes field of the driver private data is used by the DW APB SSI IRQ handler, which requires it to be initialized before the SMP memory barrier and to be visible from another CPUs. Speaking about the SMP memory barrier. Having one right after the shared resources initialization is enough and there is no point in using the spin-lock to protect the Tx/Rx buffer pointers. The protection functionality is redundant there by the driver design. (Though I have a doubt whether the SMP memory barrier is also required there because the normal IO-methods like readl/writel implies a full memory barrier. So any memory operations performed before them are supposed to be seen by devices and another CPUs. See the patch log for details of my concern.) Thirdly we've found out that there is some confusion in the IRQs masking/unmasking/clearing in the SPI-transfer procedure. Multiple interrupts are unmasked on the SPI-transfer initialization, but just TXEI is only masked back on completion. Similarly IRQ status isn't cleared on the controller reset, which actually makes the reset being not full and errors prone in the controller probe procedure. Another very important optimization is using the IO-relaxed accessors in the dw_read_io_reg()/dw_write_io_reg() methods. Since the Tx/Rx FIFO data registers are the most frequently accessible controller resource, using relaxed accessors there will significantly improve the data read/write performance. At least on Baikal-T1 SoC such modification opens up a way to have the DW APB SSI controller working with higher SPI bus speeds, than without it. Fifthly we've made an effort to cleanup the code using the SPI-device private data - chip_data. We suggest to remove the chip type from there since it isn't used and isn't implemented right anyway. Then instead of having a bus speed, clock divider, transfer mode preserved there, and recalculating the CR0 fields of the SPI-device-specific phase, polarity and frame format each time the SPI transfer is requested, we can save it in the chip_data instance. By doing so we'll make that structure finally used as it was supposed to by design (see the spi-fsl-dspi.c, spi-pl022.c, spi-pxa2xx.c drivers for examples). Sixthly instead of having the SPI-transfer specific CR0-update callback, we suggest to implement the DW APB SSI controller capabilities approach. By doing so we can now inject the vendor-specific peculiarities in different parts of the DW APB SSI core driver (which is required to implement both SPI-transfers and the SPI memory operations). This will also make the code less confusing like defining a callback in the core driver, setting it up in the glue layer, then calling it from the core driver again. Seeing the small capabilities implementation embedded in-situ is more readable than tracking the callbacks assignments. This will concern the CS-override, Keembay master setup, DW SSI-specific CR0 registers layout capabilities. Seventhly since there are going to be two types of the transfers implemented in the DW APB SSI core driver, we need a common method to set the controller configuration like, Tx/Rx-mode, bus speed, data frame size and number of data frames to read in case of the memory operations. So we just detached the corresponding code from the SPI-transfer-one method and made it to be a part of the new dw_spi_update_config() function, which is former update_cr0(). Note that the new method will be also useful for the glue drivers, which due to the hardware design need to create their own memory operations (for instance, for the dirmap-operations provided in the Baikal-T System Boot SPI controller driver). Eighthly it is the data IO procedure and IRQ-based SPI-transfer implementation refactoring. The former one will look much simpler if the buffers initial pointers and the buffers length data utilized instead of the Tx/Rx buffers start and end pointers. The later one currently lacks of valid execution at the final stage of the SPI-transfer. So if there is no data left to send, but there is still data which needs to be received, the Tx FIFO Empty IRQ will constantly happen until all of the requested inbound data is received. So we suggest to fix that by taking the Rx FIFO Empty IRQ into account. Ninthly it's potentially errors prone to enable the DW APB SSI interrupts before enabling the chip. It specifically concerns a case if for some reason the DW APB SSI IRQs handler is executed before the controller is enabled. That will cause a part of the outbound data loss. So we suggest to reverse the order. Tenthly in order to be able to pre-initialize the Tx FIFO with data and only the start the SPI memory operations we need to have any CS de-activated. We'll fulfil that requirement by explicitly clearing the CS on the SPI transfer completion and at the explicit controller reset. Then seeing all the currently available and potentially being created types of the SPI transfers need to perform the DW APB SSI controller status register check and the errors handler procedure, we've created a common method for all of them. Eleventhly if before we've mostly had a series of fixups, cleanups and refactorings, here we've finally come to the new functionality implementation. It concerns the poll-based transfer (as Baikal-T1 System Boot SPI controller lacks a dedicated IRQ lane connected) and the SPI memory operations implementation. If the former feature is pretty much straightforward (see the patch log for details), the later one is a bit tricky. It's based on the EEPROM-read (write-then-read) and the Tx-only modes of the DW APB SSI controller, which as performing the automatic data read and write let's us to implement the faster IO procedure than using the Tx-Rx-mode-based approach. Having the memory-operations implemented that way is the best thing we can currently do to provide the errors-less SPI transfers to SPI devices with native CS attached. Note the approach utilized here to develop the SPI memory operations can be also used to create the "automatic CS toggle problem"-free(ish) SPI transfers (combine SPI-message transfers into two buffers, disable interrupts, push-pull the combined data). But we don't provide a solution in the framework of this patchset. It is a matter of a dedicated one, which we currently don't intend to spend our time on. Finally at the closure of the this patchset you'll find patches, which provide the Baikal-T1-specific DW APB SSI controllers support. The SoC has got three SPI controllers. Two of them are pretty much normal DW APB SSI interfaces: with IRQ, DMA, FIFOs of 64 words depth, 4x CSs. But the third one as being a part of the Baikal-T1 System Boot Controller has got a very limited resources: no IRQ, no DMA, only a single native chip-select and Tx/Rx FIFOs with just 8 words depth available. In order to provide a transparent initial boot code execution the System Boot SPI Controller is also utilized by an vendor-specific IP-block, which exposes an SPI flash memory direct mapping interface. Please see the corresponding patch for details. Link: https://lore.kernel.org/linux-spi/20200508093621.31619-1-Sergey.Semin@baikalelectronics.ru/ [1] "LINUX KERNEL MEMORY BARRIERS", Documentation/memory-barriers.txt, Section "KERNEL I/O BARRIER EFFECTS" Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Lars Povlsen <lars.povlsen@microchip.com> Cc: wuxu.wu <wuxu.wu@huawei.com> Cc: Feng Tang <feng.tang@intel.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: linux-spi@vger.kernel.org Cc: devicetree@vger.kernel.org Cc: linux-kernel@vger.kernel.org Serge Semin (30): spi: dw: Discard IRQ threshold macro spi: dw: Use ternary op to init set_cs callback spi: dw: Initialize n_bytes before the memory barrier Revert: spi: spi-dw: Add lock protect dw_spi rx/tx to prevent concurrent calls spi: dw: Clear IRQ status on DW SPI controller reset spi: dw: Disable all IRQs when controller is unused spi: dw: Use relaxed IO-methods to access FIFOs spi: dw: Discard DW SSI chip type storages spi: dw: Convert CS-override to DW SPI capabilities spi: dw: Add KeemBay Master capability spi: dw: Add DWC SSI capability spi: dw: Detach SPI device specific CR0 config method spi: dw: Update SPI bus speed in a config function spi: dw: Simplify the SPI bus speed config procedure spi: dw: Update Rx sample delay in the config function spi: dw: Add DW SPI controller config structure spi: dw: Refactor data IO procedure spi: dw: Refactor IRQ-based SPI transfer procedure spi: dw: Perform IRQ setup in a dedicated function spi: dw: Unmask IRQs after enabling the chip spi: dw: Discard chip enabling on DMA setup error spi: dw: De-assert chip-select on reset spi: dw: Explicitly de-assert CS on SPI transfer completion spi: dw: Move num-of retries parameter to the header file spi: dw: Add generic DW SSI status-check method spi: dw: Add memory operations support spi: dw: Introduce max mem-ops SPI bus frequency setting spi: dw: Add poll-based SPI transfers support dt-bindings: spi: dw: Add Baikal-T1 SPI Controllers spi: dw: Add Baikal-T1 SPI Controller glue driver .../bindings/spi/snps,dw-apb-ssi.yaml | 33 +- drivers/spi/Kconfig | 29 + drivers/spi/Makefile | 1 + drivers/spi/spi-dw-bt1.c | 339 +++++++++ drivers/spi/spi-dw-core.c | 642 ++++++++++++++---- drivers/spi/spi-dw-dma.c | 16 +- drivers/spi/spi-dw-mmio.c | 36 +- drivers/spi/spi-dw.h | 85 ++- 8 files changed, 960 insertions(+), 221 deletions(-) create mode 100644 drivers/spi/spi-dw-bt1.c