Message-ID: <CAD=FV=XigmU1LXnNbj9NYkjJmj18chBFjbctrBeOKoS=34PLXw@mail.gmail.com>
Date:   Sat, 1 Sep 2018 22:06:10 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Dilip Kota <dkota@...eaurora.org>
Cc:     Stephen Boyd <swboyd@...omium.org>,
        Mark Brown <broonie@...nel.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>,
        devicetree@...r.kernel.org,
        Girish Mahadevan <girishm@...eaurora.org>
Subject: Re: [PATCH V3] spi: spi-geni-qcom: Add SPI driver support for GENI
 based QUP

Hi,

On Fri, Aug 24, 2018 at 3:42 AM, Dilip Kota <dkota@...eaurora.org> wrote:
> From: Girish Mahadevan <girishm@...eaurora.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@...eaurora.org>
> Signed-off-by: Dilip Kota <dkota@...eaurora.org>
> ---
> Addressing all the reviewer commets given in Patchset1.
> Summerizing all the comments below:
>
>         MAKEFILE: Arrange SPI-GENI driver in alphabetical order
>         Kconfig: Mark SPI_GENI driver dependent on QCOM_GENI_SE
>         Enable SPI core auto runtime pm, and remove runtime pm calls.
>         Remove spi_geni_unprepare_message(), spi_geni_unprepare_transfer_hardware()
>         Remove likely/unlikely keywords.
>         Remove get_spi_master() and use dev_get_drvdata()
>         Move request_irq to probe()
>         Mark bus number assignment to -1 as SPI core framework will assign dynamically
>         Use devm_spi_register_master()
>         Include platform_device.h instead of of_platform.h
>         Removing macros which are used only once:
>                 #define SPI_NUM_CHIPSELECT     4
>                 #define SPI_XFER_TIMEOUT_MS    250
>         Place Register field definitions next to respective Register definitions.
>         Replace int and u32 declerations to unsigned int.
>         Remove Hex numbers in debug prints.
>         Declare mode as u16 in spi_setup_word_len()
>         Remove the labels: setup_fifo_params_exit: exit_prepare_transfer_hardware:
>         Declaring struct spi_master as spi everywhere in the file.
>         Calling spi_finalize_current_transfer() for end of transfer.
>         Hard code the SPI controller max frequency instead of reading from DTSI node.
>         Spinlock not required, removed it.
>         Removed unrequired error prints.
>         Fix KASAN error in geni_spi_isr().
>         Remove spi-geni-qcom.h
>         Remove inter words delay and CS to Clock toggle delay logic in the driver, as of now no clients are using it.
>         Will submit this logic in the next patchset.
>         Use major, minor and step macros to read from hardware version register.
>
>  .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  |   2 -
>  drivers/spi/Kconfig                                |  12 +
>  drivers/spi/Makefile                               |   1 +
>  drivers/spi/spi-geni-qcom.c                        | 678 +++++++++++++++++++++
>  4 files changed, 691 insertions(+), 2 deletions(-)

See below for comments.  In general I've tried to post patches to
address my own comments.  See the series ending at
<https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201142/1>.
>From there you can download patch files by using the "DOWNLOAD" link
at the bottom.  Yell if you have problems.  Hopefully that's useful.
I expect that you can squash many of these into your patch to give you
a leg up on v3.

NOTE: I won't promise that I made no mistakes on my fixup patches nor
that I caught everything or did everything right.  I'll plan to take a
fresh look at the whole patch when I see your v3.


> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
> @@ -60,7 +60,6 @@ Required properties:
>  - interrupts:          Must contain SPI controller interrupts.
>  - clock-names:         Must contain "se".
>  - clocks:              Serial engine core clock needed by the device.
> -- spi-max-frequency:   Specifies maximum SPI clock frequency, units - Hz.

As per Rob's feedback, please split the device tree change into a
separate patch and justify it.  Perhaps the commit message could be
something like:

---

dt-bindings: spi: Remove spi-max-frequency from qcom,geni-se controller

No other SPI controllers have a "spi-max-frequency" at the controller
level.  The normal "spi-max-frequency" property is something that is
used when defining the nodes for SPI slaves.  While it is possible
that someone might want to define a controller-level max frequency it
should be done in other ways (perhaps by keying off a compatible
string?)

---

I think in the past Mark Brown has also requested that the bindings
actually live under "Documentation/devicetree/bindings/spi/", so
perhaps you should also add a patch to your series that moves this
documentation there and changes the "soc/qcom/qcom,geni-se.txt" to
reference that.


> +static irqreturn_t geni_spi_isr(int irq, void *data);
> +
> +struct spi_geni_master {
> +       struct geni_se se;
> +       unsigned int irq;

In v1 Stephen requested that many things in this struct become
"unsigned int", but he didn't mean the "irq".  Please change this back
to an int.  As you have things right now the code "if (spi_geni->irq <
0)" you have below is a no-op.  :(

...oh, and as Stephen pointed out to me offline you don't even need to
store irq.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201065


> +       struct device *dev;
> +       unsigned int rx_fifo_depth;
> +       unsigned int tx_fifo_depth;
> +       unsigned int tx_fifo_width;
> +       unsigned int tx_wm;

Anything that is effectively the contents of a hardware register
should be of type 'u32'.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201066


> +static int get_spi_clk_cfg(unsigned int speed_hz,
> +                       struct spi_geni_master *mas,
> +                       unsigned int *clk_idx,
> +                       unsigned int *clk_div)
> +{
> +       unsigned long sclk_freq;
> +       struct geni_se *se = &mas->se;
> +       int ret;
> +
> +       ret = geni_se_clk_freq_match(&mas->se,
> +                               (speed_hz * mas->oversampling), clk_idx,
> +                               &sclk_freq, true);

Pass "false" as the last parameter after picking my geni patch to make
this work right.  For my geni patch see
<https://lkml.kernel.org/r/20180830183612.169572-2-dianders@chromium.org>.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064


> +       if (ret) {
> +               dev_err(mas->dev, "%s: Failed(%d) to find src clk for %dHz\n",
> +                                               __func__, ret, speed_hz);

As I said in previous reviews: printing __func__ is generally
discouraged for dev_xx printouts.  They've already got the device name
and that together with the string should be enough.  Remove it here
and elsewhere in this file.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201067


> +               return ret;
> +       }
> +
> +       *clk_div = ((sclk_freq / mas->oversampling) / speed_hz);

After changing exact to false, you'll want:

  *clk_div = DIV_ROUND_UP(sclk_freq, mas->oversampling * speed_hz);
  actual_hz = sclk_freq / (mas->oversampling * *clk_div);

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064


> +       if (!(*clk_div)) {
> +               dev_err(mas->dev, "%s:Err:sclk:%lu oversampling:%d speed:%u\n",
> +                       __func__, sclk_freq, mas->oversampling, speed_hz);
> +               return -EINVAL;
> +       }

After change above to use DIV_ROUND_UP() you no longer need to check
for a zero clk_div.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064


> +static void spi_setup_word_len(struct spi_geni_master *mas, u16 mode,
> +                                       unsigned int bits_per_word)
> +{
> +       unsigned int word_len, pack_words;
> +       bool msb_first = (mode & SPI_LSB_FIRST) ? false : true;
> +       struct geni_se *se = &mas->se;
> +
> +       word_len = readl_relaxed(se->base + SE_SPI_WORD_LEN);

Gratuitously using the __relaxed versions of readl() and writel() is
discouraged in Linux because it gives very little performance gain in
99.9% of the cases and it makes the code significantly harder to
reason about it.  Remove it in spi-geni-qcom.  If it is shown that a
particular read or write is in a performance critical section then
that one instance can be added back in after carefully analysis and
documentation.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201068


> +static int setup_fifo_params(struct spi_device *spi_slv,
> +                                       struct spi_master *spi)
> +{
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +       struct geni_se *se = &mas->se;
> +       unsigned int mode = spi_slv->mode;
> +       unsigned int loopback_cfg = readl_relaxed(se->base + SE_SPI_LOOPBACK);
> +       unsigned int cpol = readl_relaxed(se->base + SE_SPI_CPOL);
> +       unsigned int cpha = readl_relaxed(se->base + SE_SPI_CPHA);
> +       unsigned int demux_sel, clk_sel, m_clk_cfg, idx, div;
> +       int ret = 0;

In v1 Stephen said:

> Don't initialize variables and then overwrite them.

This is in lots of places in your code.  I've found and fixed them for you:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201070


> +       unsigned int demux_output_inv = 0;
> +
> +       loopback_cfg &= ~LOOPBACK_MSK;
> +       cpol &= ~CPOL;
> +       cpha &= ~CPHA;
> +
> +       if (mode & SPI_LOOP)
> +               loopback_cfg |= LOOPBACK_ENABLE;
> +
> +       if (mode & SPI_CPOL)
> +               cpol |= CPOL;
> +
> +       if (mode & SPI_CPHA)
> +               cpha |= CPHA;
> +
> +       if (spi_slv->mode & SPI_CS_HIGH)
> +               demux_output_inv = BIT(spi_slv->chip_select);
> +
> +       demux_sel = spi_slv->chip_select;
> +       mas->cur_speed_hz = spi_slv->max_speed_hz;
> +       mas->cur_word_len = spi_slv->bits_per_word;
> +
> +       ret = get_spi_clk_cfg(mas->cur_speed_hz, mas, &idx, &div);
> +       if (ret) {
> +               dev_err(mas->dev, "Err setting clks ret(%d) for %d\n",
> +                                                       ret, mas->cur_speed_hz);
> +               return ret;
> +       }
> +
> +       clk_sel = (idx & CLK_SEL_MSK);
> +       m_clk_cfg = ((div << CLK_DIV_SHFT) | SER_CLK_EN);

Please avoid useless parenthesis everywhere in this file.  Stephen
pointed out many of these in v1 but the correct thing to do would have
been to search for all instances and fix them, not just fix the ones
that Stephen pointed out.  I've now done it for you:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201071


> +static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
> +{
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +       struct geni_se *se = &mas->se;
> +
> +       if (!mas->setup) {
> +               unsigned int proto = geni_se_read_proto(se);
> +               unsigned int major, minor, step, ver;
> +
> +               if (proto != GENI_SE_SPI) {
> +                       dev_err(mas->dev, "Invalid proto %d\n", proto);
> +                       return -ENXIO;
> +               }
> +               mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
> +               mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se);
> +               mas->tx_fifo_width = geni_se_get_tx_fifo_width(se);
> +
> +               /*
> +                * Hardware programming guide suggests to configure
> +                * RX FIFO RFR level to fifo_depth-2.
> +                */
> +               geni_se_init(se, 0x0, mas->tx_fifo_depth - 2);
> +               mas->oversampling = 1;
> +               /* Transmit an entire FIFO worth of data per IRQ */
> +               mas->tx_wm = 1;
> +               ver = geni_se_get_qup_hw_version(se);
> +               major = GENI_SE_VERSION_MAJOR(ver);
> +               minor = GENI_SE_VERSION_MINOR(ver);
> +               step = GENI_SE_VERSION_STEP(ver);

You don't use "step".  Kill it.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201069


> +       /*
> +        * If CS change flag is set, then toggle the CS line in between
> +        * transfers and keep CS asserted after the last transfer.
> +        * Else if keep CS flag asserted in between transfers and de-assert
> +        * CS after the last message.
> +        */
> +       if (xfer->cs_change) {
> +               if (list_is_last(&xfer->transfer_list,
> +                               &spi->cur_msg->transfers))
> +                       m_param |= FRAGMENTATION;
> +       } else {
> +               if (!list_is_last(&xfer->transfer_list,
> +                               &spi->cur_msg->transfers))

In v1 Stephen said:

> Combine else and if here into an else if.

...but really I think you could just do:

/*
 * Keep the CS asserted in either of the two cases:
 * - It's not the last transfer (and cs_change is not set)
 * - It is the last transfer (and cs_change is set)
 *
 * ...AKA keep it asserted whenever the "last transfer" and cs_change
 * booleans agree.
 */
if (!!xfer->cs_change ==
    !!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
        m_param |= FRAGMENTATION;

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201072


> +       if (m_cmd & SPI_TX_ONLY) {
> +               mas->tx_rem_bytes = xfer->len;
> +               writel_relaxed(trans_len, se->base + SE_SPI_TX_TRANS_LEN);
> +       }
> +
> +       if (m_cmd & SPI_RX_ONLY) {
> +               writel_relaxed(trans_len, se->base + SE_SPI_RX_TRANS_LEN);
> +               mas->rx_rem_bytes = xfer->len;
> +       }
> +       writel_relaxed(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
> +       geni_se_setup_m_cmd(se, m_cmd, m_param);
> +       if (m_cmd & SPI_TX_ONLY)
> +               writel_relaxed(mas->tx_wm, se->base + SE_GENI_TX_WATERMARK_REG);

In v1 Stephen said:

> This can't be combined with above m_cmd & SPI_TX_ONLY statement?

In v2 I said:

> Girish said it can't.  ...but IMO if it really can't then please add a
> comment in the code so someone doesn't later go back and try to
> combine.

I don't see a comment.  An example of an appropriate comment (assuming
that the geni hardware really does need the services of a dark wizard)
would be:

/*
 * NOTE: we have to write the SE_GENI_TX_WATERMARK_REG _after_ the
 * write to SE_SPI_TRANS_CFG and the call to geni_se_setup_m_cmd()
 * because these two things perform a special incantation to summon a
 * dark wizard and we're not allowed to set the watermark until the
 * dark wizard arrives.
 */

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201073

Presumably writing to the watermark kicks off the interrupt happening
so it has to be last.  That would be something to say instead of
talking about magic.


> +static irqreturn_t geni_spi_handle_tx(struct spi_geni_master *mas)
> +{
> +       unsigned int i = 0;
> +       unsigned int tx_fifo_width = mas->tx_fifo_width / BITS_PER_BYTE;

This is super confusing.  You have two things with the same name
"tx_fifo_width" one that's in terms of bits and one that's in terms of
bytes.  ...in general it's really hard to figure out the units of all
your variables here.  For things that it's not clear you should rename
variables.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201075


> +       unsigned int max_bytes;
> +       const u8 *tx_buf;
> +       struct geni_se *se = &mas->se;
> +
> +       if (!mas->cur_xfer)
> +               return IRQ_NONE;
> +
> +       /*
> +        * For non-byte aligned bits-per-word values(e.g 9).
> +        * The FIFO packing is set to 1 SPI word per FIFO word.

In v1 Stephen said:

> Decapitalize "The"

You added a "." at the end of the first line so I guess it's not quite
as wrong as it was but it's still better grammar to just have one
sentence.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201074


> +        * Assumption is that each SPI word will be accomodated in
> +        * ceil (bits_per_word / bits_per_byte)
> +        * and the next SPI word starts at the next byte.
> +        * In such cases, we can fit 1 SPI word per FIFO word so adjust the
> +        * max byte that can be sent per IRQ accordingly.
> +        */
> +       max_bytes = (mas->tx_fifo_depth - mas->tx_wm);
> +       if (mas->tx_fifo_width % mas->cur_word_len)
> +               max_bytes *= ((mas->cur_word_len / BITS_PER_BYTE) + 1);

Even though the math should be the same (since you should you'll never
divide cur_word_len evenly), the above is clearer as:

  max_bytes *= DIV_ROUND_UP(mas->cur_word_len, BITS_PER_BYTE);

...oh, but even that is wrong.  SPI framework documents that we need
the nearest power of 2, so a 20-bit transfer will not take up 3 bytes
but will actually take up 4.

...and of course this same assumption is replicated twice in both the
tx and rx functions.  I'm just going to rewrite these two functions.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201076


> +       while (i < max_bytes) {
> +               unsigned int j;
> +               unsigned int fifo_word = 0;
> +               u8 *fifo_byte;
> +               unsigned int bytes_per_fifo = tx_fifo_width;
> +               unsigned int bytes_to_write = 0;
> +
> +               if (mas->tx_fifo_width % mas->cur_word_len)
> +                       bytes_per_fifo =
> +                               (mas->cur_word_len / BITS_PER_BYTE) + 1;
> +
> +               if (bytes_per_fifo < (max_bytes - i))
> +                       bytes_to_write = bytes_per_fifo;
> +               else
> +                       bytes_to_write = max_bytes - i;
> +
> +               fifo_byte = (u8 *)&fifo_word;
> +               for (j = 0; j < bytes_to_write; j++)
> +                       fifo_byte[j] = tx_buf[i++];

In previous reviews Stephen pointed out that he's not super keen on
this manual copy of bytes, but IMO it's not a huge deal and we can
find a better way to do it later.  ...but if someone wants to propose
a patch that makes it nicer then I won't object.


> +static irqreturn_t geni_spi_handle_rx(struct spi_geni_master *mas)
> +{
> +       unsigned int i = 0;
> +       struct geni_se *se = &mas->se;
> +       unsigned int fifo_width = mas->tx_fifo_width / BITS_PER_BYTE;

You're assuming TX and RX FIFO widths are the same.  Let's make that explicit.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201075


> +static int spi_geni_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       struct spi_master *spi;
> +       struct spi_geni_master *spi_geni;
> +       struct resource *res;
> +       struct geni_se *se;
> +
> +       spi = spi_alloc_master(&pdev->dev, sizeof(struct spi_geni_master));
> +       if (!spi) {
> +               ret = -ENOMEM;
> +               goto spi_geni_probe_err;

When spi_alloc_master() fails you don't want spi_master_put().

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078


> +       spi->prepare_transfer_hardware = spi_geni_prepare_transfer_hardware;

IMO this is wrong.  You're using prepare_transfer_hardware() to do
one-time init.  Just do it at probe.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201079


> +       init_completion(&spi_geni->xfer_done);
> +       pm_runtime_enable(&pdev->dev);
> +       ret = devm_spi_register_master(&pdev->dev, spi);
> +       if (ret)
> +               goto spi_geni_probe_unmap;

In this error case I believe you need pm_runtime_disable().

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078


> +       return ret;
> +spi_geni_probe_unmap:
> +       devm_iounmap(&pdev->dev, se->base);

The whole point of devm is that you don't need this kind of the error handling.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078



> +static int spi_geni_remove(struct platform_device *pdev)
> +{
> +       struct spi_master *spi = platform_get_drvdata(pdev);
> +       struct spi_geni_master *spi_geni = spi_master_get_devdata(spi);
> +
> +       geni_se_resources_off(&spi_geni->se);

In v2 I said:

> Why would you need to call geni_se_resources_off()?  Isn't it handled
> by pm_runtime?


> +       pm_runtime_put_noidle(&pdev->dev);

In v2 I said:

> I'm curious: why would you have a "put" here?  Won't that result in an
> imbalance since you don't exit probe with "get"?  ...or did pm_runtime
> throw me for a loop again?

> +       pm_runtime_disable(&pdev->dev);
> +       return 0;

So testing:

      cd /sys/bus/platform/drivers/geni_spi/
      echo 880000.spi > unbind
      sleep 2
      echo 880000.spi > bind

...in fact it yells a lot.  To me this means that you didn't test
spi_geni_remove() at all.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201077


> +static int __maybe_unused spi_geni_suspend(struct device *dev)
> +{
> +       if (!pm_runtime_status_suspended(dev))
> +               return -EBUSY;

I didn't have time to dig into Stephen Boyd's feedback, but please
make sure you address it here.  I agree that something seems fishy
here.

---

In v2, I said:

> I'm not sure where to comment about this, so adding it to the end:
>
> Between v1 and v2 you totally removed all the locking.  Presumably
> this is because you didn't want to hold the lock in
> handle_fifo_timeout() while waiting for the completion.  IMO taking
> the lock out was the wrong thing to do.  You should keep it, but just
> drop the lock before wait_for_completion_timeout() and add it back
> afterwards.  Specifically you _don't_ want the IRQ and timeout code
> stomping on each other.

...but still no spinlock?

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201081

---

As we've discussed offline, your current driver doesn't deal with chip
select in a correct enough way to make cros_ec_spi work.  I've applied
and fixed up your WIP patch to fix this.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201142


-Doug

Powered by blists - more mailing lists