| Message ID | 20230725110226.2.Id4a39804e01e4a06dae9b73fd2a5194c4c7ea453@changeid (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] spi: spi-qcom-qspi: Fallback to PIO for xfers that aren't multiples of 4 bytes | expand |
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.
Hi, On Tue, Jul 25, 2023 at 11:24 AM Mark Brown <broonie@kernel.org> 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 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.
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 <dianders@chromium.org> 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 <quic_vnivarth@quicinc.com> > --- > > 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)
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)
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 <dianders@chromium.org> --- drivers/spi/spi-qcom-qspi.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)