linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Sowjanya Komatineni <skomatineni@nvidia.com>
Cc: thierry.reding@gmail.com, jonathanh@nvidia.com,
	robh+dt@kernel.org, linux-spi@vger.kernel.org,
	linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v1 3/7] spi: qspi-tegra: Add support for Tegra210 QSPI controller
Date: Fri, 4 Dec 2020 22:46:48 +0000	[thread overview]
Message-ID: <20201204224648.GI4558@sirena.org.uk> (raw)
In-Reply-To: <df8d6f25-c8cc-3b41-e4df-8e26c9b93475@nvidia.com>

[-- Attachment #1: Type: text/plain, Size: 3659 bytes --]

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.

> > 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2020-12-04 22:47 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-01 21:12 [PATCH v1 0/7] Add Tegra QSPI driver Sowjanya Komatineni
2020-12-01 21:12 ` [PATCH v1 1/7] MAINTAINERS: Add Tegra QSPI driver section Sowjanya Komatineni
2020-12-01 21:12 ` [PATCH v1 2/7] dt-bindings: spi: Add Tegra QSPI device tree binding Sowjanya Komatineni
2020-12-09 17:26   ` Rob Herring
2020-12-09 20:28     ` Sowjanya Komatineni
2020-12-10 18:08       ` Sowjanya Komatineni
2020-12-01 21:12 ` [PATCH v1 3/7] spi: qspi-tegra: Add support for Tegra210 QSPI controller Sowjanya Komatineni
2020-12-02  0:09   ` kernel test robot
2020-12-02 17:27   ` Mark Brown
2020-12-02 19:17     ` Sowjanya Komatineni
2020-12-04  0:22       ` Sowjanya Komatineni
2020-12-04 18:52         ` Mark Brown
2020-12-04 21:04           ` Sowjanya Komatineni
2020-12-04 22:46             ` Mark Brown [this message]
2020-12-05  4:10               ` Sowjanya Komatineni
     [not found]                 ` <70f69d4c-8a99-3893-76ea-7860eedb11fa@nvidia.com>
2020-12-08 12:00                   ` Mark Brown
2020-12-04 12:26       ` Thierry Reding
2020-12-04 12:17     ` Thierry Reding
2020-12-04 12:11   ` Thierry Reding
2020-12-04 22:42     ` Sowjanya Komatineni
2020-12-04 14:41   ` Jon Hunter
2020-12-04 22:34     ` Sowjanya Komatineni
2020-12-06 18:16   ` Lukas Wunner
2020-12-08  0:14     ` Sowjanya Komatineni
2020-12-08  7:44       ` Lukas Wunner
2020-12-01 21:12 ` [PATCH v1 4/7] spi: qspi-tegra: Add Tegra186 and Tegra194 QSPI compatibles Sowjanya Komatineni
2020-12-02 17:06   ` Mark Brown
2020-12-01 21:12 ` [PATCH v1 5/7] arm64: tegra: Enable QSPI on Jetson Nano Sowjanya Komatineni
2020-12-01 21:12 ` [PATCH v1 6/7] arm64: tegra: Add QSPI nodes on Tegra194 Sowjanya Komatineni
2020-12-01 21:12 ` [PATCH v1 7/7] arm64: tegra: Enable QSPI on Jetson Xavier NX Sowjanya Komatineni

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201204224648.GI4558@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=jonathanh@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=skomatineni@nvidia.com \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).