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 <[email protected]>
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(-)
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)
--
2.40.1
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 <[email protected]>
> 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 <[email protected]>
-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 <[email protected]>
> > 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 <[email protected]>
> > > 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