Re: [PATCH v1 3/7] spi: qspi-tegra: Add support for Tegra210 QSPI controller

[Sowjanya Komatineni]

On 12/4/20 2:46 PM, Mark Brown wrote:
> On Fri, Dec 04, 2020 at 01:04:46PM -0800, Sowjanya Komatineni wrote:
> > On 12/4/20 10:52 AM, Mark Brown wrote:
> > > On Thu, Dec 03, 2020 at 04:22:54PM -0800, Sowjanya Komatineni wrote:
> > > > Also unpack mode needs to manually put the bytes together from read data to
> > > > SPI core rx buffer.
> > > Could you be more explicit here, I don't know what "unpack mode" is?
> > Tegra SPI/QSPI controller support packed mode and unpacked mode based on
> > bits per word in a transfer.
> > Packed Mode: When enabled, all 32-bits of data in FIFO contains valid data
> > packets of 8-bit/16-bit/32-bit length.
> > Non packed mode: For transfers like 24-bit data for example we disable
> > packed mode and only 24-bits of FIFO data are valid and other bits are 0's.
> > So during TX for FIFO filling and during receive when FIFO data is read, SW
> > need to skip invalid bits and should align order from/to SPI core tx/rx
> > buffers.
> That's pretty surprising - is it really worth the overhead of using
> non-packed mode compared to just doing the transfer in 8 bit mode?  In
> any case it seems better to only do the memcpy() stuff in the cases
> where it's actually required since it looks like fairly obvious overhead
> otherwise, and the code could use some comments explaining why we're
> doing this.  It may actually be that the implementation is already doing
> the most sensible thing and it just needs more comments explaining why
> that's the case.

Understand the overhead but If any device specific transfers use/need 24 
bits per word, without non-packed mode we should fail the transfer.

Tegra HW has non-packed mode for such cases.

OK. Will use dma_map/unmap for packed mode transfer and for non-packed 
mode will use dma buf for fifo data and then can fill SPI core rx_buf 
with valid bytes from dma buf contents.

Sure will add comments for non-packed mode logic.

> > > This is not a good idea, attempting to reverse engineer the message and
> > > guess at the contents isn't going to be robust and if it's useful it
> > > will if nothing else lead to a bunch of duplicated code in drivers as
> > > every device that has this feature will need to reimplment it.  Instead
> > > we should extend the framework so there's explicit support for
> > > specifying transfers that are padding bytes, then there's no guesswork
> > > that can go wrong and no duplicated code between drivers.  A flag in the
> > > transfer struct might work?
> > As per QSPI spec, Dummy bytes for initial read latency are always FF's. So
> > its not like guessing the contents.
> The guesswork I was thinking of was deciding to do this rather than the
> pattern being output - the bit where the driver figures out that the
> intent of that transfer is to provide dummy bytes.
>
> > Tegra QSPI controller HW supports transferring dummy bytes (sending FF's
> > after address) based on dummy clock cycles programmed.
> > To allow Tegra QSPI HW transfer dummy bytes directly, controller driver need
> > number of dummy bytes / actual dummy clock cycles which core driver gets
> > from flash sfdp read data.
> Sure, the use case makes sense.
>
> > So, we can add flag to transfer and based on this flag if controller HW
> > supports then we can ignore filling dummy bytes in spi_mem_exec_op but
> > controller driver still needs dummy_cycles value. So probably along with
> > flag, do you agree to have dummy_cycle as part of transfer struct which can
> > be set to nor->read_dummy value?
> Yeah, or given that perhaps just skip the flag and do this by specifying
> dummy_cycles.  Or if this is always a multiple of 8 (which I guess it
> must be to do it using normal byte transfers) perhaps just have the flag
> and use the existing length field to infer the number of cycles?  I've
> not actually looked at the details at all so one or both of those
> suggestions may not actually make sense, please take them with a grain
> of salt.
>
> I'd recommend doing this as a followup to introducing the base driver,
> start off with the less efficient explicit writes and then add the API
> and add the support in the driver - that way the new API can be
> reviewed without it holding up the rest of the driver.

ok I think adding dummy_cycles to transfer is enough without flag.

If dummy cycles is 0, definitely no dummy bytes transfer.

So will get rid of code that detects dummy bytes xfer phase from list of 
transfers.


Thanks Mark.


Regards,

Sowjanya