linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: "Mahadevan, Girish" <girishm@codeaurora.org>,
	broonie@kernel.org, linux-kernel@vger.kernel.org,
	linux-spi@vger.kernel.org
Cc: sdharia@codeaurora.org, kramasub@codeaurora.org,
	dianders@chromium.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
Date: Tue, 22 May 2018 09:46:39 -0700	[thread overview]
Message-ID: <152700759909.210890.13296077062705155869@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <24b3ef71-18c1-1704-e324-5581fd18a998@codeaurora.org>

Quoting Mahadevan, Girish (2018-05-21 08:52:47)
> Hi Stephen
> 
> On 5/11/2018 4:30 PM, Stephen Boyd wrote:
> 
> >> +       if (mode & SPI_CPHA)
> >> +               cpha |= CPHA;
> >> +
> >> +       if (spi_slv->mode & SPI_CS_HIGH)
> >> +               demux_output_inv |= BIT(spi_slv->chip_select);
> >> +
> >> +       if (spi_slv->controller_data) {
> >> +               u32 cs_clk_delay = 0;
> >> +               u32 inter_words_delay = 0;
> >> +
> >> +               delay_params =
> >> +               (struct spi_geni_qcom_ctrl_data *) spi_slv->controller_data;
> >> +               cs_clk_delay =
> >> +               (delay_params->spi_cs_clk_delay << SPI_CS_CLK_DELAY_SHFT)
> >> +                                                       & SPI_CS_CLK_DELAY_MSK;
> >> +               inter_words_delay =
> >> +                       delay_params->spi_inter_words_delay &
> >> +                                               SPI_INTER_WORDS_DELAY_MSK;
> >> +               spi_delay_params =
> >> +               (inter_words_delay | cs_clk_delay);
> >> +       }
> >> +
> >> +       demux_sel = spi_slv->chip_select;
> >> +       mas->cur_speed_hz = spi_slv->max_speed_hz;
> > 
> > Why can't you use clk_get_rate() instead? Or call clk_set_rate() with
> > the rate you want the master clk to run at and then divide that down
> > from there?
> 
> Not sure I follow, the intention is to run the controller clock based on
> the slave's max frequency.

That's good. The problem I see is that we have to specify the max
frequency in the controller/bus node, and also in the child/slave node.
It should only need to be specified in the slave node, so making the
cur_speed_hz equal the max_speed_hz is problematic. The current speed of
the master should be determined by calling clk_get_rate().

> > 
> >> +       }
> >> +
> >> +       if (unlikely(!mas->setup)) {
> >> +               int proto = geni_se_read_proto(se);
> >> +               unsigned int major;
> >> +               unsigned int minor;
> >> +               unsigned int step;
> >> +
> >> +               if (unlikely(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);
> >> +               geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2));
> > 
> > Why 2? Drop extra parenthesis please.
> 
> This is what the hardware programming doc recommends, the parameter is
> actually the rx_rfr_watermark, something that doesn't apply to non-UART
> protocols.
> (I will add a detailed comment)

Thanks. Just wanted to make sure we don't forget what the magic number
means.

>  an else if.
> > 
> >> +                       m_param |= FRAGMENTATION;
> >> +       }
> >> +
> >> +       mas->cur_xfer = xfer;
> >> +       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);
> > 
> > This can't be combined with above m_cmd & SPI_TX_ONLY statement?
> 
> No, writing to this register should be done after we enqueue the command
> to the GENI engine(the geni_se_setup_m_cmd), but some of the transaction
> properties (length etc) should be setup before we enqueue the GENI command.

Ok.

> 
> > 
> > More things can be unsigned?
> > 
> >> +               fifo_byte = (u8 *)&fifo_word;
> >> +               for (j = 0; j < bytes_to_write; j++)
> >> +                       fifo_byte[j] = tx_buf[i++];
> > 
> > Why are we doing all this work to fill in a fifo byte at a time? tx_buf
> > should be a buffer of bytes that we can iowrite32_rep() with directly.
> > And then we could just run iowrite32_rep() with some count of bytes to
> > write? I suppose we may run into problems with unaligned size buffers,
> > but it sounds like that doesn't happen?
> 
> I did this for 2 reasons.
> 1.The core can handle different byte order transmissions (e.g MSB
> first), I am quite honestly not sure how the client's will setup the
> data buffer in these cases.(for bigger word sizes , 16/32)
> [we plan to support these]
> 2. For non-byte aligned word sizes (e,g 9), I am not enabling FIFO word
> packing (meaning 1 SPI-WORD/FIFO-WORD in these cases).

Alright. I'm not certain how the incoming buffer looks when clients
request certain modes. Have you tested out non-byte aligned word size
transfers?

> 
> 
> >> +++ b/include/linux/spi/spi-geni-qcom.h
> >> @@ -0,0 +1,14 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > 
> > Why?
> > 
> >> +/*
> >> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#ifndef __SPI_GENI_QCOM_HEADER___
> >> +#define __SPI_GENI_QCOM_HEADER___
> >> +
> >> +struct spi_geni_qcom_ctrl_data {
> > 
> > What's the point of this header file and structure? This driver supports
> > board files? Isn't everything DT now?
> 
> The intention was to allow a client to specify slave specific timing
> requirements, e.g CS-CLK delay (based on the slave's data sheet).
> So that the client drivers could setup these delays and pass it in part
> of the controller_data member of the spi_device data structure.
> The header file was meant to expose these timing params that the client
> could specify. I honestly didn't know how else a client could specify
> these to the controller driver.

Do you mean spi-rx-delay-us and spi-tx-delay-us properties? Those are
documented but don't seem to be used. There's also the delay_usecs part
of the spi_transfer structure, which may be what you're talking about.

  reply	other threads:[~2018-05-22 16:46 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-03 21:34 [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP Girish Mahadevan
2018-05-03 23:38 ` Mark Brown
2018-05-07 21:40   ` Mahadevan, Girish
     [not found]   ` <0c26e96c-85ad-c2a2-9abd-33096d76008b@codeaurora.org>
2018-05-17  7:21     ` Mark Brown
2018-05-21 21:45       ` Mahadevan, Girish
2018-05-22 17:32         ` Mark Brown
2018-05-11 22:30 ` Stephen Boyd
2018-05-21 15:52   ` Mahadevan, Girish
2018-05-22 16:46     ` Stephen Boyd [this message]
2018-05-22 17:30       ` Mark Brown
2018-05-24 16:25         ` Mahadevan, Girish
2018-05-24 16:29           ` Mark Brown
     [not found]             ` <28d8ab5fdeb34e52eba7ca771a17bc06@codeaurora.org>
2018-08-03 12:18               ` dkota
2018-08-09 18:03                 ` Doug Anderson
2018-08-09 18:24                   ` Trent Piepho
2018-08-09 19:37                     ` Doug Anderson
2018-08-10 18:43                       ` Trent Piepho
2018-08-10 10:52                   ` Mark Brown
2018-08-10 15:40                     ` Doug Anderson
2018-08-10 16:13                       ` Mark Brown
2018-08-10 16:27                         ` Doug Anderson
2018-08-10 16:43                           ` Mark Brown
2018-08-10 16:47                             ` Doug Anderson
2018-08-10 16:29                         ` dkota
2018-08-10 16:46                           ` Mark Brown
2018-08-14  9:00                             ` dkota
2018-08-14 15:03                               ` Mark Brown
2018-08-17 10:36                                 ` dkota
2018-08-17 14:59                                   ` Mark Brown
2018-08-24 11:00                                     ` dkota
2018-08-10 16:49                           ` Doug Anderson
2018-08-10 17:55                           ` Trent Piepho
2018-06-08 23:34 ` Matthias Kaehlcke

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=152700759909.210890.13296077062705155869@swboyd.mtv.corp.google.com \
    --to=swboyd@chromium.org \
    --cc=broonie@kernel.org \
    --cc=dianders@chromium.org \
    --cc=girishm@codeaurora.org \
    --cc=kramasub@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=sdharia@codeaurora.org \
    /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).