| Message ID | 20210910111529.12539-4-tanureal@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 777a2cbbaf1c6685ace7e2ce846796e9425ab320 |
| Headers | show |
| Series | [v2,1/4] spi: amd: Refactor code to use less spi_master_get_devdata | expand |
On Fri, Sep 10, 2021 at 12:15:29PM +0100, Lucas Tanure wrote: > Return from a write-only transfer without waiting for > it to finish > But wait before a new transfer as the previous may > still happening and also wait before reading the data > from the FIFO > > Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com> > --- > -static void amd_spi_execute_opcode(struct amd_spi *amd_spi) > +static int amd_spi_execute_opcode(struct amd_spi *amd_spi) > { > + int ret; > + > + ret = amd_spi_busy_wait(amd_spi); > + if (ret) > + return ret; > + > /* Set ExecuteOpCode bit in the CTRL0 register */ > amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG, AMD_SPI_EXEC_CMD, AMD_SPI_EXEC_CMD); > - amd_spi_busy_wait(amd_spi); > + > + return 0; > } > > static int amd_spi_master_setup(struct spi_device *spi) > @@ -178,6 +185,7 @@ static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi, > amd_spi_clear_fifo_ptr(amd_spi); > /* Execute command */ > amd_spi_execute_opcode(amd_spi); > + amd_spi_busy_wait(amd_spi); Surely the previous transfer can't still be happening if this if unconditional? Should this not be gated on rx_len? Thanks, Charles > /* Read data from FIFO to receive buffer */ > for (i = 0; i < rx_len; i++) > buf[i] = amd_spi_readreg8(amd_spi, AMD_SPI_FIFO_BASE + tx_len + i); > -- > 2.33.0 >
On 9/10/21 1:42 PM, Charles Keepax wrote: > On Fri, Sep 10, 2021 at 12:15:29PM +0100, Lucas Tanure wrote: >> Return from a write-only transfer without waiting for >> it to finish >> But wait before a new transfer as the previous may >> still happening and also wait before reading the data >> from the FIFO >> >> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com> >> --- >> -static void amd_spi_execute_opcode(struct amd_spi *amd_spi) >> +static int amd_spi_execute_opcode(struct amd_spi *amd_spi) >> { >> + int ret; >> + >> + ret = amd_spi_busy_wait(amd_spi); >> + if (ret) >> + return ret; >> + >> /* Set ExecuteOpCode bit in the CTRL0 register */ >> amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG, AMD_SPI_EXEC_CMD, AMD_SPI_EXEC_CMD); >> - amd_spi_busy_wait(amd_spi); >> + >> + return 0; >> } >> >> static int amd_spi_master_setup(struct spi_device *spi) >> @@ -178,6 +185,7 @@ static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi, >> amd_spi_clear_fifo_ptr(amd_spi); >> /* Execute command */ >> amd_spi_execute_opcode(amd_spi); >> + amd_spi_busy_wait(amd_spi); > > Surely the previous transfer can't still be happening if this if > unconditional? Should this not be gated on rx_len? > > Thanks, > Charles > >> /* Read data from FIFO to receive buffer */ >> for (i = 0; i < rx_len; i++) >> buf[i] = amd_spi_readreg8(amd_spi, AMD_SPI_FIFO_BASE + tx_len + i); >> -- >> 2.33.0 >> This is executed inside an xfer->rx_buf not null if, so it`s gated in a read transfer and not for a write transfer only
On Fri, Sep 10, 2021 at 02:47:32PM +0100, Lucas tanure wrote: > On 9/10/21 1:42 PM, Charles Keepax wrote: > >On Fri, Sep 10, 2021 at 12:15:29PM +0100, Lucas Tanure wrote: > >>Return from a write-only transfer without waiting for > >>it to finish > >>But wait before a new transfer as the previous may > >>still happening and also wait before reading the data > >>from the FIFO > >> > >>Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com> > >>--- > >> static int amd_spi_master_setup(struct spi_device *spi) > >>@@ -178,6 +185,7 @@ static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi, > >> amd_spi_clear_fifo_ptr(amd_spi); > >> /* Execute command */ > >> amd_spi_execute_opcode(amd_spi); > >>+ amd_spi_busy_wait(amd_spi); > > > >Surely the previous transfer can't still be happening if this if > >unconditional? Should this not be gated on rx_len? > > > >Thanks, > >Charles > > > >> /* Read data from FIFO to receive buffer */ > >> for (i = 0; i < rx_len; i++) > >> buf[i] = amd_spi_readreg8(amd_spi, AMD_SPI_FIFO_BASE + tx_len + i); > >>-- > >>2.33.0 > >> > This is executed inside an xfer->rx_buf not null if, so it`s gated > in a read transfer and not for a write transfer only > And so it is, sorry should have looked at more than just the git context there. Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com> Thanks, Charles
diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c index 97838b57871c..4b3ac7aceaf6 100644 --- a/drivers/spi/spi-amd.c +++ b/drivers/spi/spi-amd.c @@ -115,11 +115,18 @@ static int amd_spi_busy_wait(struct amd_spi *amd_spi) return 0; } -static void amd_spi_execute_opcode(struct amd_spi *amd_spi) +static int amd_spi_execute_opcode(struct amd_spi *amd_spi) { + int ret; + + ret = amd_spi_busy_wait(amd_spi); + if (ret) + return ret; + /* Set ExecuteOpCode bit in the CTRL0 register */ amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG, AMD_SPI_EXEC_CMD, AMD_SPI_EXEC_CMD); - amd_spi_busy_wait(amd_spi); + + return 0; } static int amd_spi_master_setup(struct spi_device *spi) @@ -178,6 +185,7 @@ static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi, amd_spi_clear_fifo_ptr(amd_spi); /* Execute command */ amd_spi_execute_opcode(amd_spi); + amd_spi_busy_wait(amd_spi); /* Read data from FIFO to receive buffer */ for (i = 0; i < rx_len; i++) buf[i] = amd_spi_readreg8(amd_spi, AMD_SPI_FIFO_BASE + tx_len + i);
Return from a write-only transfer without waiting for it to finish But wait before a new transfer as the previous may still happening and also wait before reading the data from the FIFO Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com> --- Changes in v2: Add wait before read data New explanation drivers/spi/spi-amd.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)