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>
Douglas Anderson