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
