| Message ID | 20230606231844.726272-1-abe.kohandel@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5b6d0b91f84cff3f28724076f93f6f9e2ef8d775 |
| Headers | show |
| Series | spi: dw: Remove misleading comment for Mount Evans SoC | expand |
On Tue, Jun 06, 2023 at 04:18:44PM -0700, Abe Kohandel wrote: > Remove a misleading comment about the DMA operations of the Intel Mount > Evans SoC's SPI Controller as requested by Serge. > > Signed-off-by: Abe Kohandel <abe.kohandel@intel.com> > Link: https://lore.kernel.org/linux-spi/20230606191333.247ucbf7h3tlooxf@mobilestation/ > Fixes: 0760d5d0e9f0 ("spi: dw: Add compatible for Intel Mount Evans SoC") Note Fixes tag normally goes first. In this case it seems redundant though. > --- > drivers/spi/spi-dw-mmio.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c > index c1d16157de61..a699ce496cc5 100644 > --- a/drivers/spi/spi-dw-mmio.c > +++ b/drivers/spi/spi-dw-mmio.c > @@ -237,14 +237,7 @@ static int dw_spi_intel_init(struct platform_device *pdev, > } > > /* > - * The Intel Mount Evans SoC's Integrated Management Complex uses the > - * SPI controller for access to a NOR SPI FLASH. However, the SoC doesn't > - * provide a mechanism to override the native chip select signal. I had nothing against this part of the comment but only about the second chunk of the text. > - * > - * This driver doesn't use DMA for memory operations when a chip select > - * override is not provided due to the native chip select timing behavior. > - * As a result no DMA configuration is done for the controller and this > - * configuration is not tested. > + * DMA-based mem ops are not configured for this device and are not tested. * Note mem-ops is just a feature of the DW APB/AHB SSI controllers * which provides a way to perform write-then-read and write-only * transfers (see Transmit only and EEPROM read transfer modes in the * hw manual). It works irrespective of whether your controller has a * DMA-engine connected or doesn't have. Modern DW SSI controllers * support Enhanced SPI modes with the extended SPI-bus width * capability. But it's a whole another story and such modes aren't * currently supported by the driver. Just a question for the sake of the discussion history. Does your platform have a DMA-engine synthesized to work with this DW SSI controller? That is does your controller has the DMA Controller Interface (handshake signals) connected to any DMA-engine on your platform? I am asking because if there is no such DMA-engine then the last part of your statement is just redundant since you can't test something which isn't supported by design. Anyway the change in this patch looks good. Thanks for submitting it. Reviewed-by: Serge Semin <fancer.lancer@gmail.com> -Serge(y) > */ > static int dw_spi_mountevans_imc_init(struct platform_device *pdev, > struct dw_spi_mmio *dwsmmio) > -- > 2.40.1 >
On 23/06/07 02:27PM, Serge Semin wrote: > On Tue, Jun 06, 2023 at 04:18:44PM -0700, Abe Kohandel wrote: > > Remove a misleading comment about the DMA operations of the Intel Mount > > Evans SoC's SPI Controller as requested by Serge. > > > > > Signed-off-by: Abe Kohandel <abe.kohandel@intel.com> > > Link: https://lore.kernel.org/linux-spi/20230606191333.247ucbf7h3tlooxf@mobilestation/ > > Fixes: 0760d5d0e9f0 ("spi: dw: Add compatible for Intel Mount Evans SoC") > > Note Fixes tag normally goes first. In this case it seems redundant > though. > Thanks, will do this in the future. > > - * The Intel Mount Evans SoC's Integrated Management Complex uses the > > - * SPI controller for access to a NOR SPI FLASH. However, the SoC doesn't > > - * provide a mechanism to override the native chip select signal. > > I had nothing against this part of the comment but only about the > second chunk of the text. Thinking about it a bit more there is nothing precluding this controller from being used for other purposes in the future. It is configured with two chip selects, only one of which is used today. I removed it to so it wouldn't become inaccurate if that happens. > > + * DMA-based mem ops are not configured for this device and are not tested. > > * Note mem-ops is just a feature of the DW APB/AHB SSI controllers > * which provides a way to perform write-then-read and write-only > * transfers (see Transmit only and EEPROM read transfer modes in the > * hw manual). It works irrespective of whether your controller has a > * DMA-engine connected or doesn't have. Modern DW SSI controllers > * support Enhanced SPI modes with the extended SPI-bus width > * capability. But it's a whole another story and such modes aren't > * currently supported by the driver. > > Just a question for the sake of the discussion history. Does your > platform have a DMA-engine synthesized to work with this DW SSI > controller? That is does your controller has the DMA Controller > Interface (handshake signals) connected to any DMA-engine on your > platform? I am asking because if there is no such DMA-engine then > the last part of your statement is just redundant since you can't test > something which isn't supported by design. The platform does have a DMA-engine synthesized but I have been having some challenges with getting it to work which may require some further quirks added to the DMA driver. One example being the system uses 40-bit addressing but the DMA-engine is only synthesized with 32-bit address capability and is meant to only target a specific region of memory where it "knowns" the upper byte of the address. Anyhow, I hope to work through some of those challenges and enable this in the future. Thanks, Abe
On Wed, Jun 07, 2023 at 08:00:48AM -0700, Abe Kohandel wrote: > On 23/06/07 02:27PM, Serge Semin wrote: > > On Tue, Jun 06, 2023 at 04:18:44PM -0700, Abe Kohandel wrote: > > > Remove a misleading comment about the DMA operations of the Intel Mount > > > Evans SoC's SPI Controller as requested by Serge. > > > > > > > > Signed-off-by: Abe Kohandel <abe.kohandel@intel.com> > > > Link: https://lore.kernel.org/linux-spi/20230606191333.247ucbf7h3tlooxf@mobilestation/ > > > Fixes: 0760d5d0e9f0 ("spi: dw: Add compatible for Intel Mount Evans SoC") > > > > Note Fixes tag normally goes first. In this case it seems redundant > > though. > > > > Thanks, will do this in the future. > > > > - * The Intel Mount Evans SoC's Integrated Management Complex uses the > > > - * SPI controller for access to a NOR SPI FLASH. However, the SoC doesn't > > > - * provide a mechanism to override the native chip select signal. > > > > I had nothing against this part of the comment but only about the > > second chunk of the text. > > Thinking about it a bit more there is nothing precluding this controller from > being used for other purposes in the future. It is configured with two chip > selects, only one of which is used today. I removed it to so it wouldn't become > inaccurate if that happens. Ok. Regarding the number of chip-selects. You could have overwritten the dw_spi.num_cs field with value 2 then in the dw_spi_mountevans_imc_init() method. Thus having a bit safer driver for your platform. > > > > + * DMA-based mem ops are not configured for this device and are not tested. > > > > * Note mem-ops is just a feature of the DW APB/AHB SSI controllers > > * which provides a way to perform write-then-read and write-only > > * transfers (see Transmit only and EEPROM read transfer modes in the > > * hw manual). It works irrespective of whether your controller has a > > * DMA-engine connected or doesn't have. Modern DW SSI controllers > > * support Enhanced SPI modes with the extended SPI-bus width > > * capability. But it's a whole another story and such modes aren't > > * currently supported by the driver. > > > > Just a question for the sake of the discussion history. Does your > > platform have a DMA-engine synthesized to work with this DW SSI > > controller? That is does your controller has the DMA Controller > > Interface (handshake signals) connected to any DMA-engine on your > > platform? I am asking because if there is no such DMA-engine then > > the last part of your statement is just redundant since you can't test > > something which isn't supported by design. > > The platform does have a DMA-engine synthesized but I have been having some > challenges with getting it to work which may require some further quirks added > to the DMA driver. The main question is whether that DMA-engine has the handshake signals connected to the DW SSI controller. If it doesn't then adding such engine support would be a great deal of challenge indeed because a software-based handshaking interface would need to be added to the DMA-engine subsystem first. Then the DW SSI driver would need to be fixed to work with that interface. Taking a FIFO-size into account and an amount of IRQs to handle on each handshaking round, the resultant performance might get to be not worth all the efforts so a simple IRQ-based transfers implementation may work better. > One example being the system uses 40-bit addressing but the > DMA-engine is only synthesized with 32-bit address capability and is meant to > only target a specific region of memory where it "knowns" the upper byte of the > address. That's a pretty much well known problem. The kernel has a solution for it: DMA-mask set for the DMA-engine device (see dma_set_mask() and dma_set_mask_and_coherent()) and SWIOTLB (formerly known as bounce buffers). Alternatively modern CPUs are normally equipped with a thing like IOMMU, which can be used to remap the limited device address space to any CPU/RAM address. -Serge(y) > > Anyhow, I hope to work through some of those challenges and enable this in the > future. > > Thanks, > Abe
On 23/06/07 06:28PM, Serge Semin wrote: > On Wed, Jun 07, 2023 at 08:00:48AM -0700, Abe Kohandel wrote: > > On 23/06/07 02:27PM, Serge Semin wrote: > > > On Tue, Jun 06, 2023 at 04:18:44PM -0700, Abe Kohandel wrote: > > > > - * The Intel Mount Evans SoC's Integrated Management Complex uses the > > > > - * SPI controller for access to a NOR SPI FLASH. However, the SoC doesn't > > > > - * provide a mechanism to override the native chip select signal. > > > > > > I had nothing against this part of the comment but only about the > > > second chunk of the text. > > > > > Thinking about it a bit more there is nothing precluding this controller from > > being used for other purposes in the future. It is configured with two chip > > selects, only one of which is used today. I removed it to so it wouldn't become > > inaccurate if that happens. > > Ok. Regarding the number of chip-selects. You could have overwritten > the dw_spi.num_cs field with value 2 then in the dw_spi_mountevans_imc_init() > method. Thus having a bit safer driver for your platform. I am currently setting dw_spi.num_cs via the num-cs property in the device tree. Is one preferred over the other? I guess setting the dw_spi.num_cs in code is safer than using the device tree. > > > > + * DMA-based mem ops are not configured for this device and are not tested. > > > > > > * Note mem-ops is just a feature of the DW APB/AHB SSI controllers > > > * which provides a way to perform write-then-read and write-only > > > * transfers (see Transmit only and EEPROM read transfer modes in the > > > * hw manual). It works irrespective of whether your controller has a > > > * DMA-engine connected or doesn't have. Modern DW SSI controllers > > > * support Enhanced SPI modes with the extended SPI-bus width > > > * capability. But it's a whole another story and such modes aren't > > > * currently supported by the driver. > > > > > > Just a question for the sake of the discussion history. Does your > > > platform have a DMA-engine synthesized to work with this DW SSI > > > controller? That is does your controller has the DMA Controller > > > Interface (handshake signals) connected to any DMA-engine on your > > > platform? I am asking because if there is no such DMA-engine then > > > the last part of your statement is just redundant since you can't test > > > something which isn't supported by design. > > > > > The platform does have a DMA-engine synthesized but I have been having some > > challenges with getting it to work which may require some further quirks added > > to the DMA driver. > > The main question is whether that DMA-engine has the handshake signals > connected to the DW SSI controller. If it doesn't then adding such > engine support would be a great deal of challenge indeed because a > software-based handshaking interface would need to be added to the > DMA-engine subsystem first. Then the DW SSI driver would need to be > fixed to work with that interface. Taking a FIFO-size into account and > an amount of IRQs to handle on each handshaking round, the resultant > performance might get to be not worth all the efforts so a simple > IRQ-based transfers implementation may work better. Oh sorry, I wasn't explicit enough. The HW handshaking signals are connected to the DW SSI controller so we should be able to take advantage of that acceleration and not have to go through the challenging steps you have outlined. > > One example being the system uses 40-bit addressing but the > > DMA-engine is only synthesized with 32-bit address capability and is meant to > > only target a specific region of memory where it "knowns" the upper byte of the > > address. > > That's a pretty much well known problem. The kernel has a solution for > it: DMA-mask set for the DMA-engine device (see dma_set_mask() and > dma_set_mask_and_coherent()) and SWIOTLB (formerly known as bounce > buffers). > > Alternatively modern CPUs are normally equipped with a thing like > IOMMU, which can be used to remap the limited device address space to > any CPU/RAM address. Thanks for all the advice Serge, much appreciated! Hopefully I can come back with a patch to enable the DMA engine for this platform in the near future. Thanks, Abe
On Tue, 06 Jun 2023 16:18:44 -0700, Abe Kohandel wrote: > Remove a misleading comment about the DMA operations of the Intel Mount > Evans SoC's SPI Controller as requested by Serge. > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: dw: Remove misleading comment for Mount Evans SoC commit: 5b6d0b91f84cff3f28724076f93f6f9e2ef8d775 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
On Wed, Jun 07, 2023 at 09:53:41AM -0700, Abe Kohandel wrote: > On 23/06/07 06:28PM, Serge Semin wrote: > > On Wed, Jun 07, 2023 at 08:00:48AM -0700, Abe Kohandel wrote: > > > On 23/06/07 02:27PM, Serge Semin wrote: > > > > On Tue, Jun 06, 2023 at 04:18:44PM -0700, Abe Kohandel wrote: > > > > > > - * The Intel Mount Evans SoC's Integrated Management Complex uses the > > > > > - * SPI controller for access to a NOR SPI FLASH. However, the SoC doesn't > > > > > - * provide a mechanism to override the native chip select signal. > > > > > > > > I had nothing against this part of the comment but only about the > > > > second chunk of the text. > > > > > > > > Thinking about it a bit more there is nothing precluding this controller from > > > being used for other purposes in the future. It is configured with two chip > > > selects, only one of which is used today. I removed it to so it wouldn't become > > > inaccurate if that happens. > > > > Ok. Regarding the number of chip-selects. You could have overwritten > > the dw_spi.num_cs field with value 2 then in the dw_spi_mountevans_imc_init() > > method. Thus having a bit safer driver for your platform. > > I am currently setting dw_spi.num_cs via the num-cs property in the device > tree. Is one preferred over the other? I guess setting the dw_spi.num_cs in > code is safer than using the device tree. Strictly speaking the "num-cs" property is supposed to be used for generic DW APB SSI devices (compatible with the snps,dw* string) only since the IP-core can be synthesized with up to 16 slave select lines and originally it was considered as impossible to auto-detect the number of lines (although it's actually possible just by test-writing to the SER register - another idea for the driver improvement ;) ). Meanwhile vendor-specific implementations of the DW APB/AHB SSI controller are synthesized with the particular parameters values including the number of slave-select signals. Thus the kernel driver can easily infer all device parameters (like reg-io-width, num-cs, etc) just based on the compatible string (that's what I did in the spi-dw-bt1.o driver). The problem is that historically nobody cared about too relaxed "num-cs" utilization and now we can't update the driver and the bindings to strictly follow that constraint. The only solution is to implement the SER-writability-based auto-detection procedure but it isn't free of risks to break some older devices (though this problem can be avoided by making it optional). To sum it up and answering to your question, it's preferable to directly initialize dw_spi.num_cs in driver if the value is known. Note though that the DW APB SSI driver currently has a limitation of GPIO-based chip-selects usage: a total number of chip-selects must not exceed a number of the native chip select lines (except AMD Pensando Elba device). It's because in order to have the controller performing the SPI transfers at least one SER flag must be set. Since we can't predict which native CS is free from being utilized on a platform (SPI devices can be instantiated from user-space), the GPIO-based lines are activated together with the corresponding SER flag. > > > > > > + * DMA-based mem ops are not configured for this device and are not tested. > > > > > > > > * Note mem-ops is just a feature of the DW APB/AHB SSI controllers > > > > * which provides a way to perform write-then-read and write-only > > > > * transfers (see Transmit only and EEPROM read transfer modes in the > > > > * hw manual). It works irrespective of whether your controller has a > > > > * DMA-engine connected or doesn't have. Modern DW SSI controllers > > > > * support Enhanced SPI modes with the extended SPI-bus width > > > > * capability. But it's a whole another story and such modes aren't > > > > * currently supported by the driver. > > > > > > > > Just a question for the sake of the discussion history. Does your > > > > platform have a DMA-engine synthesized to work with this DW SSI > > > > controller? That is does your controller has the DMA Controller > > > > Interface (handshake signals) connected to any DMA-engine on your > > > > platform? I am asking because if there is no such DMA-engine then > > > > the last part of your statement is just redundant since you can't test > > > > something which isn't supported by design. > > > > > > > > The platform does have a DMA-engine synthesized but I have been having some > > > challenges with getting it to work which may require some further quirks added > > > to the DMA driver. > > > > The main question is whether that DMA-engine has the handshake signals > > connected to the DW SSI controller. If it doesn't then adding such > > engine support would be a great deal of challenge indeed because a > > software-based handshaking interface would need to be added to the > > DMA-engine subsystem first. Then the DW SSI driver would need to be > > fixed to work with that interface. Taking a FIFO-size into account and > > an amount of IRQs to handle on each handshaking round, the resultant > > performance might get to be not worth all the efforts so a simple > > IRQ-based transfers implementation may work better. > > Oh sorry, I wasn't explicit enough. The HW handshaking signals are connected to > the DW SSI controller so we should be able to take advantage of that > acceleration and not have to go through the challenging steps you have > outlined. > > > > One example being the system uses 40-bit addressing but the > > > DMA-engine is only synthesized with 32-bit address capability and is meant to > > > only target a specific region of memory where it "knowns" the upper byte of the > > > address. > > > > That's a pretty much well known problem. The kernel has a solution for > > it: DMA-mask set for the DMA-engine device (see dma_set_mask() and > > dma_set_mask_and_coherent()) and SWIOTLB (formerly known as bounce > > buffers). > > > > Alternatively modern CPUs are normally equipped with a thing like > > IOMMU, which can be used to remap the limited device address space to > > any CPU/RAM address. > > Thanks for all the advice Serge, much appreciated! Hopefully I can come back > with a patch to enable the DMA engine for this platform in the near future. Always welcome. Feel free to ask should any question arise on that journey. -Serge(y) > > Thanks, > Abe
diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c index c1d16157de61..a699ce496cc5 100644 --- a/drivers/spi/spi-dw-mmio.c +++ b/drivers/spi/spi-dw-mmio.c @@ -237,14 +237,7 @@ static int dw_spi_intel_init(struct platform_device *pdev, } /* - * The Intel Mount Evans SoC's Integrated Management Complex uses the - * SPI controller for access to a NOR SPI FLASH. However, the SoC doesn't - * provide a mechanism to override the native chip select signal. - * - * This driver doesn't use DMA for memory operations when a chip select - * override is not provided due to the native chip select timing behavior. - * As a result no DMA configuration is done for the controller and this - * configuration is not tested. + * DMA-based mem ops are not configured for this device and are not tested. */ static int dw_spi_mountevans_imc_init(struct platform_device *pdev, struct dw_spi_mmio *dwsmmio)
Remove a misleading comment about the DMA operations of the Intel Mount Evans SoC's SPI Controller as requested by Serge. Signed-off-by: Abe Kohandel <abe.kohandel@intel.com> Link: https://lore.kernel.org/linux-spi/20230606191333.247ucbf7h3tlooxf@mobilestation/ Fixes: 0760d5d0e9f0 ("spi: dw: Add compatible for Intel Mount Evans SoC") --- drivers/spi/spi-dw-mmio.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)