Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

From: Sergey Suloev
Date: Thu Apr 05 2018 - 05:59:52 EST


On 04/05/2018 12:19 PM, Maxime Ripard wrote:
On Wed, Apr 04, 2018 at 02:35:14PM +0300, Sergey Suloev wrote:
On 04/04/2018 09:50 AM, Maxime Ripard wrote:
On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote:
There is no need to handle 3/4 empty interrupt as the maximum
supported transfer length in PIO mode is equal to FIFO depth,
i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs.

Changes in v3:
1) Restored processing of 3/4 FIFO full interrupt.

Signed-off-by: Sergey Suloev <ssuloev@xxxxxxxxxxxxx>
---
drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 78acc1f..c09ad10 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
{
- return SUN6I_MAX_XFER_SIZE - 1;
+ struct spi_master *master = spi->master;
+ struct sun6i_spi *sspi = spi_master_get_devdata(master);
+
+ return sspi->fifo_depth;
Doesn't that effectively revert 3288d5cb40c0 ?

Why do you need to do so?
Need what?
Why do you need to revert that change.

Change is supposed to restrict max transfer size for PIO mode otherwise it
will fail.
The maximum transfer length = FIFO depth for PIO mode.
The point of that patch was precisely to allow to send more data than
the FIFO. You're breaking that behaviour without any justification,
and this is not ok.

I am sorry, but you can't. That's a hardware limitation.
}
static int sun6i_spi_prepare_message(struct spi_master *master,
@@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
int ret = 0;
u32 reg;
- if (tfr->len > SUN6I_MAX_XFER_SIZE)
- return -EINVAL;
+ /* A zero length transfer never finishes if programmed
+ in the hardware */
Improper comment style here. Please make sure to run checkpatch before
sending your patches.
ok
+ if (!tfr->len)
+ return 0;
Can that case even happen?
Not sure, just for safety.
+ /* Don't support transfer larger than the FIFO */
+ if (tfr->len > sspi->fifo_depth)
+ return -EMSGSIZE;
You're changing the return type, why?
As  a more appropriate one
The fact that it's more appropriate should be justified.

reinit_completion(&sspi->done);
sspi->tx_buf = tfr->tx_buf;
@@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
*/
trig_level = sspi->fifo_depth / 4 * 3;
sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
- (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
- (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
+ (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
@@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
/* Enable the interrupts */
- sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
SUN6I_INT_CTL_RF_RDY);
- if (tx_len > sspi->fifo_depth)
- sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
This would also need to be explained in your commit log.
What exactly and in what way ?
You should explain, at least:
A) What is the current behaviour
B) Why that is a problem, or what problem does it cause
C) What solution you implement and why you think it's justified

/* Start the transfer */
reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
@@ -376,7 +381,9 @@ out:
static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
{
struct sun6i_spi *sspi = dev_id;
- u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
+ u32 status;
+
+ status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
Why is this change needed?
A minor one, for readability.
That's arguable, and you should have a single logical change per
patch. So this doesn't belong in this one.

Maxime



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel