In the patch ("spi: spi-qcom-qspi: Fallback to PIO for xfers that
aren't multiples of 4 bytes") we detect reads that we can't handle
properly and fallback to PIO mode. While that's correct behavior, we
can do better by adding "spi_controller_mem_ops" for our
controller. Once we do this then the caller will give us a transfer
that's a multiple of 4-bytes so we can DMA.
Fixes: b5762d95607e ("spi: spi-qcom-qspi: Add DMA mode support")
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/spi/spi-qcom-qspi.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
index 39b4d8a8107a..b2bbcfd93637 100644
--- a/drivers/spi/spi-qcom-qspi.c
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -659,6 +659,30 @@ static irqreturn_t qcom_qspi_irq(int irq, void *dev_id)
return ret;
}
+static int qcom_qspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
+{
+ /*
+ * If qcom_qspi_can_dma() is going to return false we don't need to
+ * adjust anything.
+ */
+ if (op->data.nbytes <= QSPI_MAX_BYTES_FIFO)
+ return 0;
+
+ /*
+ * When reading, the transfer needs to be a multiple of 4 bytes so
+ * shrink the transfer if that's not true. The caller will then do a
+ * second transfer to finish things up.
+ */
+ if (op->data.dir == SPI_MEM_DATA_IN && (op->data.nbytes & 0x3))
+ op->data.nbytes &= ~0x3;
+
+ return 0;
+}
+
+static const struct spi_controller_mem_ops qcom_qspi_mem_ops = {
+ .adjust_op_size = qcom_qspi_adjust_op_size,
+};
+
static int qcom_qspi_probe(struct platform_device *pdev)
{
int ret;
@@ -743,6 +767,7 @@ static int qcom_qspi_probe(struct platform_device *pdev)
if (of_property_read_bool(pdev->dev.of_node, "iommus"))
master->can_dma = qcom_qspi_can_dma;
master->auto_runtime_pm = true;
+ master->mem_ops = &qcom_qspi_mem_ops;
ret = devm_pm_opp_set_clkname(&pdev->dev, "core");
if (ret)
--
2.41.0.487.g6d72f3e995-goog
On Tue, Jul 25, 2023 at 11:02:27AM -0700, Douglas Anderson wrote:
> In the patch ("spi: spi-qcom-qspi: Fallback to PIO for xfers that
> aren't multiples of 4 bytes") we detect reads that we can't handle
> properly and fallback to PIO mode. While that's correct behavior, we
> can do better by adding "spi_controller_mem_ops" for our
> controller. Once we do this then the caller will give us a transfer
> that's a multiple of 4-bytes so we can DMA.
This is more of an optimisation for the case where we're using flash -
if someone has hung some other hardware off the controller (which seems
reasonable enough if they don't need it for flash) then we'll not use
the mem_ops.
On Tue, Jul 25, 2023 at 11:30:30AM -0700, Doug Anderson wrote:
> Note that it's pretty likely someone wouldn't use this SPI controller
> for anything other than SPI flash. While it's theoretically possible,
> the controller is intended for SPI flash (supports dual and quad SPI
> modes) and is only half duplex.
TBH most devices are half duplex so it's not *that* big a restriction,
and dual/quad mode are obviously attractive if you need to transfer
large quantities of data.
Hi,
On Tue, Jul 25, 2023 at 11:24 AM Mark Brown <[email protected]> wrote:
>
> On Tue, Jul 25, 2023 at 11:02:27AM -0700, Douglas Anderson wrote:
> > In the patch ("spi: spi-qcom-qspi: Fallback to PIO for xfers that
> > aren't multiples of 4 bytes") we detect reads that we can't handle
> > properly and fallback to PIO mode. While that's correct behavior, we
> > can do better by adding "spi_controller_mem_ops" for our
> > controller. Once we do this then the caller will give us a transfer
> > that's a multiple of 4-bytes so we can DMA.
>
> This is more of an optimisation for the case where we're using flash -
> if someone has hung some other hardware off the controller (which seems
> reasonable enough if they don't need it for flash) then we'll not use
> the mem_ops.
Right. That's why I also have the first patch in the series too. The
first patch ensures correctness and the second patch makes things more
optimal for when we're using flash. Do you want me to re-submit the
patch with wording that makes this more obvious?
Note that it's pretty likely someone wouldn't use this SPI controller
for anything other than SPI flash. While it's theoretically possible,
the controller is intended for SPI flash (supports dual and quad SPI
modes) and is only half duplex.
-Doug
On 7/25/2023 11:32 PM, Douglas Anderson wrote:
> In the patch ("spi: spi-qcom-qspi: Fallback to PIO for xfers that
> aren't multiples of 4 bytes") we detect reads that we can't handle
> properly and fallback to PIO mode. While that's correct behavior, we
> can do better by adding "spi_controller_mem_ops" for our
> controller. Once we do this then the caller will give us a transfer
> that's a multiple of 4-bytes so we can DMA.
>
> Fixes: b5762d95607e ("spi: spi-qcom-qspi: Add DMA mode support")
> Signed-off-by: Douglas Anderson <[email protected]>
I checked with a couple of folks here and seemingly the POR for QSPI
controller is for storage device only, so in all likelihood we should be
having a spi-flash at the other end.
For other devices there is QUP anyway.
Hence personally I am happy with this change.
Thank you...
Reviewed-by: Vijaya Krishna Nivarthi <[email protected]>
> ---
>
> drivers/spi/spi-qcom-qspi.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
> index 39b4d8a8107a..b2bbcfd93637 100644
> --- a/drivers/spi/spi-qcom-qspi.c
> +++ b/drivers/spi/spi-qcom-qspi.c
> @@ -659,6 +659,30 @@ static irqreturn_t qcom_qspi_irq(int irq, void *dev_id)
> return ret;
> }
>
> +static int qcom_qspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> +{
> + /*
> + * If qcom_qspi_can_dma() is going to return false we don't need to
> + * adjust anything.
> + */
> + if (op->data.nbytes <= QSPI_MAX_BYTES_FIFO)
> + return 0;
> +
> + /*
> + * When reading, the transfer needs to be a multiple of 4 bytes so
> + * shrink the transfer if that's not true. The caller will then do a
> + * second transfer to finish things up.
> + */
> + if (op->data.dir == SPI_MEM_DATA_IN && (op->data.nbytes & 0x3))
> + op->data.nbytes &= ~0x3;
> +
> + return 0;
> +}
> +
> +static const struct spi_controller_mem_ops qcom_qspi_mem_ops = {
> + .adjust_op_size = qcom_qspi_adjust_op_size,
> +};
> +
> static int qcom_qspi_probe(struct platform_device *pdev)
> {
> int ret;
> @@ -743,6 +767,7 @@ static int qcom_qspi_probe(struct platform_device *pdev)
> if (of_property_read_bool(pdev->dev.of_node, "iommus"))
> master->can_dma = qcom_qspi_can_dma;
> master->auto_runtime_pm = true;
> + master->mem_ops = &qcom_qspi_mem_ops;
>
> ret = devm_pm_opp_set_clkname(&pdev->dev, "core");
> if (ret)