On Mon, 2020-06-15 at 15:32 +0100, Mark Brown wrote:
> On Mon, Jun 15, 2020 at 04:05:56PM +1200, Mark Tomlinson wrote:
>
> > When needing to send/receive data in small chunks, make this interrupt
> > driven rather than waiting for a completion event for each small section
> > of data.
>
> Again was this done for a reason and if so do we understand why doing
> this from interrupt context is safe - how long can the interrupts be
> when stuffing the FIFO from interrupt context?
As I'm porting a Broadcom patch, I'm hoping someone else can add
something to this. From the history it appears there was a hard limit
(no small chunks), and this was changed to doing it in chunks with
patch 345309fa7c0c92, apparently to improve performance. I believe this
change further improves performance, but as the patch arrived without
any documentation, I'm not certain.
> > @@ -731,12 +733,14 @@ static inline u16 read_rxram_slot_u16(struct bcm_qspi
> > *qspi, int slot)
> > ((bcm_qspi_read(qspi, MSPI, msb_offset) & 0xff) << 8);
> > }
> >
> > -static void read_from_hw(struct bcm_qspi *qspi, int slots)
> > +static void read_from_hw(struct bcm_qspi *qspi)
> > {
>
> Things might be clearer if this refactoring were split out into a
> separate patch.
Done.
> > @@ -960,24 +966,21 @@ static int bcm_qspi_transfer_one(struct spi_master
> > *master,
> > struct spi_transfer *trans)
> > {
> > struct bcm_qspi *qspi = spi_master_get_devdata(master);
> > - int slots;
> > - unsigned long timeo = msecs_to_jiffies(100);
> > + unsigned long timeo = msecs_to_jiffies(1000);
>
> That's a randomly chosen value - if we're now doing the entire transfer
> then we should be trying to estimate the length of time the transfer
> will take, for a very large transfer on a slow bus it's possible that
> even a second won't be enough.
>
Again, the value came from Broadcom. Using the data length as an
estimate sounds like a good idea.
> > - complete(&qspi->mspi_done);
> > +
> > + read_from_hw(qspi);
> > +
> > + if (qspi->trans_pos.trans) {
> > + write_to_hw(qspi);
> > + } else {
> > + complete(&qspi->mspi_done);
> > + spi_finalize_current_transfer(qspi->master);
> > + }
> > +
>
> This is adding a spi_finalize_current_transfer() which we didn't have
> before, and still leaving us doing cleanup work in the driver in another
> thread. This is confused, the driver should only need to finalize the
> transfer explicitly if it returned a timeout from transfer_one() but
> nothing's changed there.
I can remove the call to spi_finalize_current_transfer() from this
patch. I'll try to check what does happen in the timeout case.
