If we're using geni to manage the chip select line (don't do it--use a
GPIO!) and we happen to get a timeout waiting for the chip select
command to be completed, no errors are printed even though things
might not be in the best shape. Let's add a print.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20201217142842.v3.4.I666b37646de9652cef438ac7c2c6c2053367fc6b@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
If we get a timeout sending then this happens:
spi_transfer_one_message()
->transfer_one() AKA spi_geni_transfer_one()
setup_fifo_xfer()
mas->cur_xfer = non-NULL
spi_transfer_wait() => TIMES OUT
if (msg->status != -EINPROGRESS)
goto out
if (ret != 0 ...)
spi_set_cs()
->set_cs AKA spi_geni_set_cs()
# mas->cur_xfer is non-NULL
The above happens _before_ the SPI core calls ->handle_err() AKA
handle_fifo_timeout().
Unfortunately that won't work so well on geni. If we got a timeout
transferring then it's likely that our interrupt handler is blocked,
but we need that same interrupt handler to run and the command channel
to be unblocked in order to adjust the chip select. Trying to set the
chip select doesn't crash us but ends up confusing our state machine
and leads to messages like: Premature done. rx_rem = 32 bpw8
Let's just drop the chip select request in this case. We can detect
the case because cur_xfer is non-NULL--it would have been set to NULL
in the interrupt handler if the previous transfer had finished. Sure,
we might leave the chip select in the wrong state but it's likely it
was going to fail anyway and this avoids getting the driver even more
confused about what it's doing.
The SPI core in general assumes that setting chip select is a simple
operation that doesn't fail. Yet another reason to just reconfigure
the chip select line as GPIOs.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20201217142842.v3.3.I07afdedcc49655c5d26880f8df9170aac5792378@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
If we got a timeout when trying to send an abort command then it means
that we just got 3 timeouts in a row:
1. The original timeout that caused handle_fifo_timeout() to be
called.
2. A one second timeout waiting for the cancel command to finish.
3. A one second timeout waiting for the abort command to finish.
SPI is clocked by the controller, so nothing (aside from a hardware
fault or a totally broken sequencer) should be causing the actual
commands to fail in hardware. However, even though the hardware
itself is not expected to fail (and it'd be hard to predict how we
should handle things if it did), it's easy to hit the timeout case by
simply blocking our interrupt handler from running for a long period
of time. Obviously the system is in pretty bad shape if a interrupt
handler is blocked for > 2 seconds, but there are certainly bugs (even
bugs in other unrelated drivers) that can make this happen.
Let's make things a bit more robust against this case. If we fail to
abort we'll set a flag and then we'll block all future transfers until
we have no more interrupts pending.
Fixes: 561de45f72 ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20201217142842.v3.2.Ibade998ed587e070388b4bf58801f1107a40eb53@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
In commit 7ba9bdcb91 ("spi: spi-geni-qcom: Don't keep a local state
variable") we changed handle_fifo_timeout() so that we set
"mas->cur_xfer" to NULL to make absolutely sure that we don't mess
with the buffers from the previous transfer in the timeout case.
Unfortunately, this caused the IRQ handler to dereference NULL in some
cases. One case:
CPU0 CPU1
---- ----
setup_fifo_xfer()
geni_se_setup_m_cmd()
<hardware starts transfer>
<transfer completes in hardware>
<hardware sets M_RX_FIFO_WATERMARK_EN in m_irq>
...
handle_fifo_timeout()
spin_lock_irq(mas->lock)
mas->cur_xfer = NULL
geni_se_cancel_m_cmd()
spin_unlock_irq(mas->lock)
geni_spi_isr()
spin_lock(mas->lock)
if (m_irq & M_RX_FIFO_WATERMARK_EN)
geni_spi_handle_rx()
mas->cur_xfer NULL dereference!
tl;dr: Seriously delayed interrupts for RX/TX can lead to timeout
handling setting mas->cur_xfer to NULL.
Let's check for the NULL transfer in the TX and RX cases and reset the
watermark or clear out the fifo respectively to put the hardware back
into a sane state.
NOTE: things still could get confused if we get timeouts all the way
through handle_fifo_timeout() and then start a new transfer because
interrupts from the old transfer / cancel / abort could still be
pending. A future patch will help this corner case.
Fixes: 561de45f72 ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20201217142842.v3.1.I99ee04f0cb823415df59bd4f550d6ff5756e43d6@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
Collected patches from the two series below and associated tags so they
can be merged in one pile through the spi tree. Merry December!
SPI: https://lore.kernel.org/r/20201202214935.1114381-1-swboyd@chromium.org
cros-ec: https://lore.kernel.org/r/20201203011649.1405292-1-swboyd@chromium.org
Cc: Akash Asthana <akashast@codeaurora.org>
Cc: Simon Glass <sjg@chromium.org>
Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Alexandru M Stan <amstan@chromium.org>
Stephen Boyd (3):
platform/chrome: cros_ec_spi: Don't overwrite spi::mode
platform/chrome: cros_ec_spi: Drop bits_per_word assignment
spi: spi-geni-qcom: Use the new method of gpio CS control
drivers/platform/chrome/cros_ec_spi.c | 2 --
drivers/spi/spi-geni-qcom.c | 1 +
2 files changed, 1 insertion(+), 2 deletions(-)
base-commit: b650545978
--
https://chromeos.dev
Let's set the 'use_gpio_descriptors' field so that we use the new way of
requesting the CS GPIOs in the core. This allows us to avoid having to
configure the CS pins in "output" mode with an 'output-enable' pinctrl
setting.
Cc: Akash Asthana <akashast@codeaurora.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Cc: Alexandru M Stan <amstan@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20201204193540.3047030-4-swboyd@chromium.org
Signed-off-by: Mark Brown <broonie@kernel.org>
spi_geni_remove() accesses the driver's private data after calling
spi_unregister_master() even though that function releases the last
reference on the spi_master and thereby frees the private data.
Moreover, since commit 1a9e489e61 ("spi: spi-geni-qcom: Use OPP API to
set clk/perf state"), spi_geni_probe() leaks the spi_master allocation
if the calls to dev_pm_opp_set_clkname() or dev_pm_opp_of_add_table()
fail.
Fix by switching over to the new devm_spi_alloc_master() helper which
keeps the private data accessible until the driver has unbound and also
avoids the spi_master leak on probe.
Fixes: 561de45f72 ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: <stable@vger.kernel.org> # v4.20+: 5e844cc37a: spi: Introduce device-managed SPI controller allocation
Cc: <stable@vger.kernel.org> # v4.20+
Cc: Rajendra Nayak <rnayak@codeaurora.org>
Cc: Girish Mahadevan <girishm@codeaurora.org>
Link: https://lore.kernel.org/r/dfa1d8c41b8acdfad87ec8654cd124e6e3cb3f31.1607286887.git.lukas@wunner.de
Signed-off-by: Mark Brown <broonie@kernel.org>
If we're sending bytes over SPI, we know the FIFO is empty at the
start of the transfer. There's no reason to wait for the interrupt
telling us to start--we can just start right away. Then if we
transmit everything in one swell foop we don't even need to bother
listening for TX interrupts.
In a test of "flashrom -p ec -r /tmp/foo.bin" interrupts were reduced
from ~30560 to ~29730, about a 3% savings.
This patch looks bigger than it is because I moved a few functions
rather than adding a forward declaration. The only actual change to
geni_spi_handle_tx() was to make it return a bool indicating if there
is more to tx.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Link: https://lore.kernel.org/r/20200912111716.1.Ied5e843fad0d6b733a1fb8bcfb364dd2fa889eb3@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
We always toggle the chip select manually in spi-geni-qcom so that we
can properly implement the Linux API. There's no reason to program
this to the hardware on every transfer. Program it once at init and
be done with it.
This saves some part of a microsecond of overhead on each transfer.
While not really noticeable on any real world benchmarks, we might as
well save the time.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Link: https://lore.kernel.org/r/20200912140730.2.I33e571179986850b4ec17042e813d0b08fb1b9c1@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
In commit 902481a78e ("spi: spi-geni-qcom: Actually use our FIFO") I
explained that the maximum size we could program the FIFO was
"mas->tx_fifo_depth - 3" but that I chose "mas->tx_fifo_depth()"
because I was worried about decreased bandwidth.
Since that time:
* All the interconnect patches have landed, making things run at the
proper speed.
* I've done more measurements.
This lets me confirm that there's really no downside of using the FIFO
more. Specifically I did "flashrom -p ec -r /tmp/foo.bin" on a
Chromebook and averaged over several runs.
Before: It took 6.66 seconds and 59669 interrupts fired.
After: It took 6.66 seconds and 47992 interrupts fired.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Link: https://lore.kernel.org/r/20200912140730.1.Ie67fa32009b94702d56232c064f1d89065ee8836@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
dev_pm_opp_of_remove_table() doesn't report any errors when it fails to
find the OPP table with error -ENODEV (i.e. OPP table not present for
the device). And we can call dev_pm_opp_of_remove_table()
unconditionally here.
While at it, create a new label and put clkname on errors.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Link: https://lore.kernel.org/r/ea0864d41277e61fa31d304fbd4cf9af6b314269.1598594714.git.viresh.kumar@linaro.org
Signed-off-by: Mark Brown <broonie@kernel.org>
A fairly quiet release for SPI, nothing really going on in the core
although there's been quite a bit of driver related activity. This pull
request includes the addition of some shared code in drivers/memory for
the Renesas RPC-IF which is used by a newly added SPI driver, the memory
subsystem doesn't seem to have a fixed maintainer at the minute and this
seemed like the most sensible way to get that hardware supported.
- Quite a few cleanups and optimizations for the Altera, Qualcomm GENI,
sun6i and lantiq drivers.
- Several more GPIO descriptor conversions.
- Move the Cadence QuadSPI driver from drivers/mtd to drivers/spi.
- New support for Mediatek MT8192 and Renesas RPC-IF, R8A7742 and
R8A774e1.
-----BEGIN PGP SIGNATURE-----
iQFHBAABCgAxFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAl8oBFoTHGJyb29uaWVA
a2VybmVsLm9yZwAKCRAk1otyXVSH0P2UB/47NlDsAadOiOB0ZzPKdn4Nr5BLvUza
TP63gAsxD4BErzgf/p/0D8qUTZPzmlfA833gj9hkIavJlTmv3fGrH0XeHg2OuCJb
yhyw9pE7MPpnARVOJHz1MZkAT/dY+sF3TaUXysmymlvs6CLqRnsLTOwK2f92SeGH
ygpC4J1tfD961xWmv2Zt8wNuQXtH+JePLXeZFSx2ZkpToFT56QO6kFpWjVfDquY4
/F8bc7qyXX6FcsBYAG6Ly35OSxbPazVLkTaDWQr5V4TAD0DXw0NOyvxg2OccqE31
y1jjx3hn98sN73m/VLzlEOer4w9313K5BOISaN0z7TQSQ55XRZKe4EQj
=K1PB
-----END PGP SIGNATURE-----
Merge tag 'spi-v5.9' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi
Pull spi updates from Mark Brown:
"A fairly quiet release for SPI, nothing really going on in the core
although there's been quite a bit of driver related activity.
This includes the addition of some shared code in drivers/memory for
the Renesas RPC-IF which is used by a newly added SPI driver, the
memory subsystem doesn't seem to have a fixed maintainer at the minute
and this seemed like the most sensible way to get that hardware
supported.
- Quite a few cleanups and optimizations for the Altera, Qualcomm
GENI, sun6i and lantiq drivers.
- Several more GPIO descriptor conversions.
- Move the Cadence QuadSPI driver from drivers/mtd to drivers/spi.
- New support for Mediatek MT8192 and Renesas RPC-IF, R8A7742 and
R8A774e1"
* tag 'spi-v5.9' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi: (119 commits)
dt-bindings: lpspi: New property in document DT bindings for LPSPI
spi: lpspi: fix using CS discontinuously on i.MX8DXLEVK
spi: lpspi: remove unused fsl_lpspi->chipselect
spi: lpspi: Fix kernel warning dump when probe fail after calling spi_register
spi: rockchip: Fix error in SPI slave pio read
spi: rockchip: Support 64-location deep FIFOs
spi: rockchip: Config spi rx dma burst size depend on xfer length
spi: spi-topcliff-pch: drop call to wakeup-disable
spi: spidev: Align buffers for DMA
spi: correct kernel-doc inconsistency
spi: sun4i: update max transfer size reported
spi: imx: enable runtime pm support
spi: update bindings for MT8192 SoC
spi: mediatek: add spi support for mt8192 IC
spi: Add bindings for Lightning Mountain SoC
spi: lantiq: Add support to Lightning Mountain SoC
spi: lantiq: Move interrupt configuration to SoC specific data structure
spi: lantiq: Add fifo size bit mask in SoC specific data structure
spi: lantiq: Add support to acknowledge interrupt
spi: lantiq: Move interrupt control register offesets to SoC specific data structure
...
There's a bunch of overhead in spi-geni-qcom's prepare_message. Get
rid of it. Before this change spi_geni_prepare_message() took around
14.5 us. After this change, spi_geni_prepare_message() takes about
1.75 us (as measured by ftrace).
What's here:
* We're always in FIFO mode, so no need to call it for every transfer.
This avoids a whole ton of readl/writel calls.
* We don't need to write a whole pile of config registers if the mode
isn't changing. Cache the last mode and only do the work if needed.
* For several registers we were trying to do read/modify/write, but
there was no reason. The registers only have one thing in them, so
just write them.
Acked-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20200701174506.3.I2b3d7aeb1ea622335482cce60c58d2f8381e61dd@changeid
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
In the patch ("spi: spi-geni-qcom: Avoid clock setting if not needed")
we avoid a whole pile of clock code. As part of that, we should have
restored the clock at runtime resume. Do that.
It turns out that, at least with today's configurations, this doesn't
actually matter. That's because none of the current device trees have
an OPP table for geni SPI yet. That makes dev_pm_opp_set_rate(dev, 0)
a no-op. This is why it wasn't noticed in the testing of the original
patch. It's still a good idea to fix, though.
Reviewed-by: Rajendra Nayak <rnayak@codeaurora.org>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Mark Brown <broonie@kernel.org>
Link: https://lore.kernel.org/r/20200709074037.v2.1.I0b701fc23eca911a5bde4ae4fa7f97543d7f960e@changeid
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Every SPI transfer could have a different clock rate. The
spi-geni-qcom controller code to deal with this was never very well
optimized and has always had a lot of code plus some calls into the
clk framework which, at the very least, would grab a mutex. However,
until recently, the overhead wasn't _too_ much. That changed with
commit 0e3b8a81f5 ("spi: spi-geni-qcom: Add interconnect support")
we're now calling geni_icc_set_bw(), which leads to a bunch of math
plus:
geni_icc_set_bw()
icc_set_bw()
apply_constraints()
qcom_icc_set()
qcom_icc_bcm_voter_commit()
rpmh_invalidate()
rpmh_write_batch()
...and those rpmh commands can be a bit beefy if you call them too
often.
We already know what speed we were running at before, so if we see
that nothing has changed let's avoid the whole pile of code.
On my hardware, this made spi_geni_prepare_message() drop down from
~145 us down to ~14 us.
NOTE: Potentially it might also make sense to add some code into the
interconnect framework to avoid executing so much code when bandwidth
isn't changing, but even if we did that we still want to short circuit
here to save the extra math / clock calls.
Acked-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Akash Asthana<akashast@codeaurora.org>
Fixes: 0e3b8a81f5 ("spi: spi-geni-qcom: Add interconnect support")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20200701174506.1.Icfdcee14649fc0a6c38e87477b28523d4e60bab3@changeid
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
In commit 0e3b8a81f5 ("spi: spi-geni-qcom: Add interconnect
support") the spi_geni_runtime_suspend() and spi_geni_runtime_resume()
became a bit slower. Measuring on my hardware I see numbers in the
hundreds of microseconds now.
Let's use autosuspend to help avoid some of the overhead. Now if
we're doing a bunch of transfers we won't need to be constantly
chruning.
The number 250 ms for the autosuspend delay was picked a bit
arbitrarily, so if someone has measurements showing a better value we
could easily change this.
Fixes: 0e3b8a81f5 ("spi: spi-geni-qcom: Add interconnect support")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Akash Asthana<akashast@codeaurora.org>
Link: https://lore.kernel.org/r/20200701174506.2.I9b8f6bb1e7e6d8847e2ed2cf854ec55678db427f@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
Setting the chip select on the Qualcomm geni SPI controller isn't
exactly cheap. Let's cache the current setting and avoid setting the
chip select if it's already right.
Using "flashrom" to read or write the EC firmware on a Chromebook
shows roughly a 25% reduction in interrupts and a 15% speedup.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20200626151946.1.I06134fd669bf91fd387dc6ecfe21d44c202bd412@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
geni spi needs to express a perforamnce state requirement on CX
depending on the frequency of the clock rates. Use OPP table from
DT to register with OPP framework and use dev_pm_opp_set_rate() to
set the clk/perf state.
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Acked-by: Mark Brown <broonie@kernel.org>
Cc: Alok Chauhan <alokc@codeaurora.org>
Cc: Akash Asthana <akashast@codeaurora.org>
Cc: linux-spi@vger.kernel.org
Link: https://lore.kernel.org/r/1592222564-13556-3-git-send-email-rnayak@codeaurora.org
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Get the interconnect paths for SPI based Serial Engine device
and vote according to the current bus speed of the driver.
Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Akash Asthana <akashast@codeaurora.org>
Link: https://lore.kernel.org/r/1592908737-7068-7-git-send-email-akashast@codeaurora.org
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
There is code for adjusting the clock both in setup_fifo_params()
(called from prepare_message()) and in setup_fifo_xfer() (called from
transfer_one()). The code is the same. Abstract it out to a shared
function.
This is a no-op cleanup patch. The only change is to the error string
if we fail to set the clock. Since the two paths has marginally
different error messages I picked the clean one.
Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Akash Asthana <akashast@codeaurora.org>
Link: https://lore.kernel.org/r/1592908737-7068-6-git-send-email-akashast@codeaurora.org
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
We only need to test for these counters being non-zero when we see the
end of a transfer. If we're doing a CS change then they will already be
zero. This implies that we don't need to set these to 0 if we're
cancelling an in flight transfer too, because we only care to test these
counters when the 'DONE' bit is set in the hardware and we've set them
to non-zero for a transfer.
This is a non-functional change, just cleanup to consolidate code.
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20200620022233.64716-3-swboyd@chromium.org
Signed-off-by: Mark Brown <broonie@kernel.org>
The definition of SPI_FULL_DUPLEX (3) is really SPI_TX_ONLY (1) ORed
with SPI_RX_ONLY (2). Let's drop the define and simplify the code here a
bit by collapsing the setting of 'm_cmd' into conditions that are the
same.
This is a non-functional change, just cleanup to consolidate code.
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20200620022233.64716-2-swboyd@chromium.org
Signed-off-by: Mark Brown <broonie@kernel.org>
The variable "cur_mcmd" kept track of our current state (idle, xfer,
cs, cancel). We don't really need it, so get rid of it. Instead:
* Use separate condition variables for "chip select done", "cancel
done", and "abort done". This is important so that if a "done"
comes through (perhaps some previous interrupt finally came through)
it can't confuse the cancel/abort function.
* Use the "done" interrupt only for when a chip select or transfer is
done and we can tell the difference by looking at whether "cur_xfer"
is NULL.
This is mostly a no-op change. However, it is possible it could fix
an issue where a super delayed interrupt for a cancel command could
have confused our waiting for an abort command.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20200618080459.v4.5.Ib1e6855405fc9c99916ab7c7dee84d73a8bf3d68@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
The geni hardware has a FIFO that can hold up to 64 bytes (it has 16
entries that can hold 4 bytes each), at least on the two SoCs I tested
(sdm845 and sc7180). We configured our RX Watermark to 0, which
basically meant we got an interrupt as soon as the first 4 bytes
showed up in the FIFO. Tracing the IRQ handler showed that we often
only read 4 or 8 bytes per IRQ handler.
I tried setting the RX Watermark to "fifo size - 2" but that just got
me a bunch of overrun errors reported. Setting it to "fifo size - 3"
seemed to work great, though. This made me worried that we'd start
getting overruns if we had long interrupt latency, but that doesn't
appear to be the case and delays inserted in the IRQ handler while
using "fifo size - 3" didn't cause any errors. Presumably there is
some interaction with the poorly-documented RFR (ready for receive)
level means that "fifo size - 3" is the max. We are the SPI master,
so it makes sense that there would be no problems with overruns, the
master should just stop clocking.
Despite "fifo size - 3" working, I chose "fifo size / 2" (8 entries =
32 bytes) which gives us a little extra time to get to the interrupt
handler and should reduce dead time on the SPI wires. With this
setting, I often saw the IRQ handler handle 40 bytes but sometimes up
to 56 if we had bad interrupt latency.
Testing by running "flashrom -p ec -r" on a Chromebook saw interrupts
from the SPI driver cut roughly in half. Time was roughly the same.
Fixes: 561de45f72 ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20200618080459.v4.4.I988281f7c6ee0ed00325559bfce7539f403da69e@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
>From reading the #defines it seems like we should shout if we ever see
one of these error bits. Let's do so. This doesn't do anything
functional except print a yell in the log if the error bits are seen.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20200618080459.v4.3.Id8bebdbdb4d2ed9468634343a7e6207d6cffff8a@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
If you added a bit of a delay (like a trace_printk) into the ISR for
the spi-geni-qcom driver, you would suddenly start seeing some errors
spit out. The problem was that, though the ISR itself held a lock,
other parts of the driver didn't always grab the lock.
One example race was this:
CPU0 CPU1
---- ----
spi_geni_set_cs()
mas->cur_mcmd = CMD_CS;
geni_se_setup_m_cmd(...)
wait_for_completion_timeout(&xfer_done);
<INTERRUPT>
geni_spi_isr()
complete(&xfer_done);
<wakeup>
pm_runtime_put(mas->dev);
... // back to SPI core
spi_geni_transfer_one()
setup_fifo_xfer()
mas->cur_mcmd = CMD_XFER;
mas->cur_cmd = CMD_NONE; // bad!
return IRQ_HANDLED;
Let's fix this. Before we start messing with hardware, we'll grab the
lock to make sure that the IRQ handler from some previous command has
really finished. We don't need to hold the lock unless we're in a
state where more interrupts can come in, but we at least need to make
sure the previous IRQ is done. This lock is used exclusively to
prevent the IRQ handler and non-IRQ from stomping on each other. The
SPI core handles all other mutual exclusion.
As part of this, we change the way that the IRQ handler detects
spurious interrupts. Previously we checked for our state variable
being set to IRQ_NONE, but that was done outside the spinlock. We
could move it into the spinlock, but instead let's just change it to
look for the lack of any IRQ status bits being set. This can be done
outside the lock--the hardware certainly isn't grabbing or looking at
the spinlock when it updates its status register.
It's possible that this will fix real (but very rare) errors seen in
the field that look like:
irq ...: nobody cared (try booting with the "irqpoll" option)
NOTE: an alternate strategy considered here was to always make the
complete() / spi_finalize_current_transfer() the very last thing in
our IRQ handler. With such a change you could consider that we could
be "lockless". In that case, though, we'd have to be very careful w/
memory barriers so we made sure we didn't have any bugs with weakly
ordered memory. Using spinlocks makes the driver much easier to
understand.
Fixes: 561de45f72 ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20200618080459.v4.2.I752ebdcfd5e8bf0de06d66e767b8974932b3620e@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
The driver locks its locks in two places.
In the first usage of the lock the function doing the locking already
has a sleeping call and thus we know we can't be called from interrupt
context. That means we can use the "spin_lock_irq" variant of the
function.
In the second usage of the lock the function is the interrupt handler
and we know interrupt handlers are called with interrupts disabled.
That means we can use the "spin_lock" variant of the function.
This patch is expected to be a no-op and is just a cleanup / slight
optimization.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20200616034044.v3.1.Ic50cccdf27d42420a63485082f8b5bf86ed1a2b6@changeid
Signed-off-by: Mark Brown <broonie@kernel.org>
This driver doesn't call any DT functions like of_get_property(). Remove
the of.h include as it isn't used.
Cc: Girish Mahadevan <girishm@codeaurora.org>
Cc: Dilip Kota <dkota@codeaurora.org>
Cc: Alok Chauhan <alokc@codeaurora.org>
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20200204191206.97036-4-swboyd@chromium.org
Signed-off-by: Mark Brown <broonie@kernel.org>
Some lines are long here. Use a struct dev pointer to shorten lines and
simplify code. The clk_get() call can fail because of EPROBE_DEFER
problems too, so just remove the error print message because it isn't
useful.
Cc: Girish Mahadevan <girishm@codeaurora.org>
Cc: Dilip Kota <dkota@codeaurora.org>
Cc: Alok Chauhan <alokc@codeaurora.org>
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20200204191206.97036-3-swboyd@chromium.org
Signed-off-by: Mark Brown <broonie@kernel.org>
We don't need to force IRQF_TRIGGER_HIGH here as the DT or ACPI tables
should take care of this for us. Just use 0 instead so that we use the
flags from the firmware.
Cc: Girish Mahadevan <girishm@codeaurora.org>
Cc: Dilip Kota <dkota@codeaurora.org>
Cc: Alok Chauhan <alokc@codeaurora.org>
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20200204191206.97036-2-swboyd@chromium.org
Signed-off-by: Mark Brown <broonie@kernel.org>
Use devm_platform_ioremap_resource() to simplify the code a bit.
This is detected by coccinelle.
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Link: https://lore.kernel.org/r/20190904135918.25352-14-yuehaibing@huawei.com
Signed-off-by: Mark Brown <broonie@kernel.org>
We don't need dev_err() messages when platform_get_irq() fails now that
platform_get_irq() prints an error message itself when something goes
wrong. Let's remove these prints with a simple semantic patch.
// <smpl>
@@
expression ret;
struct platform_device *E;
@@
ret =
(
platform_get_irq(E, ...)
|
platform_get_irq_byname(E, ...)
);
if ( \( ret < 0 \| ret <= 0 \) )
{
(
-if (ret != -EPROBE_DEFER)
-{ ...
-dev_err(...);
-... }
|
...
-dev_err(...);
)
...
}
// </smpl>
While we're here, remove braces on if statements that only have one
statement (manually).
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-spi@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20190730181557.90391-42-swboyd@chromium.org
Signed-off-by: Mark Brown <broonie@kernel.org>
We don't need this forward declaration. Move the function to where it
needed so we can drop it and shave some lines of code.
CC: Girish Mahadevan <girishm@codeaurora.org>
CC: Dilip Kota <dkota@codeaurora.org>
CC: Alok Chauhan <alokc@codeaurora.org>
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
We only use this completion when we're doing something that isn't a
message transfer. For example, changing CS or aborting/canceling a
command. All of those situations properly reinitialize the completion
before sending the GENI the special command to change CS or cancel, etc.
Given that, let's remove the initialization here.
Cc: Girish Mahadevan <girishm@codeaurora.org>
Cc: Dilip Kota <dkota@codeaurora.org>
Cc: Alok Chauhan <alokc@codeaurora.org>
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
Re-arrange existing APIs in probe function to
avoid using goto and remove redundant variables.
Signed-off-by: Alok Chauhan <alokc@codeaurora.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
fixed the nitpicks.
Signed-off-by: Alok Chauhan <alokc@codeaurora.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
This driver supports GENI based SPI Controller in the Qualcomm SOCs. The
Qualcomm Generic Interface (GENI) is a programmable module supporting a
wide range of serial interfaces including SPI. This driver supports SPI
operations using FIFO mode of transfer.
Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
Signed-off-by: Dilip Kota <dkota@codeaurora.org>
Signed-off-by: Alok Chauhan <alokc@codeaurora.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
