| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6171c1c3-55ba-4f74-ae60-764820cf1caf@quicinc.com>
Date: Tue, 30 Jan 2024 11:21:45 +0800
From: Haixu Cui <quic_haixcui@...cinc.com>
To: Harald Mommer <Harald.Mommer@...nsynergy.com>,
<virtio-dev@...ts.oasis-open.org>, Mark Brown <broonie@...nel.org>,
"Viresh
Kumar" <viresh.kumar@...aro.org>, <linux-spi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
CC: <quic_ztu@...cinc.com>, Matti Moell <Matti.Moell@...nsynergy.com>,
"Mikhail Golubev" <Mikhail.Golubev@...nsynergy.com>
Subject: Re: [RFC PATCH v2 3/3] SPI: Add virtio SPI driver (V10 draft
specification).
On 1/4/2024 9:01 PM, Harald Mommer wrote:
> +static int virtio_spi_one_transfer(struct virtio_spi_req *spi_req,
> + struct spi_controller *ctrl,
> + struct spi_message *msg,
> + struct spi_transfer *xfer)
> +{
> + struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
> + struct device *dev = &priv->vdev->dev;
> + struct spi_device *spi = msg->spi;
> + struct spi_transfer_head *th;
> + struct scatterlist sg_out_head, sg_out_payload;
> + struct scatterlist sg_in_result, sg_in_payload;
> + struct scatterlist *sgs[4];
> + unsigned int outcnt = 0u;
> + unsigned int incnt = 0u;
> + int ret;
> +
> + th = &spi_req->transfer_head;
> +
> + /* Fill struct spi_transfer_head */
> + th->chip_select_id = spi_get_chipselect(spi, 0);
> + th->bits_per_word = spi->bits_per_word;
> + /*
> + * Got comment: "The virtio spec for cs_change is*not* what the Linux
> + * cs_change field does, this will not do the right thing."
> + * TODO: Understand/discuss this, still unclear what may be wrong here
> + */
> + th->cs_change = xfer->cs_change;
> + th->tx_nbits = xfer->tx_nbits;
> + th->rx_nbits = xfer->rx_nbits;
> + th->reserved[0] = 0;
> + th->reserved[1] = 0;
> + th->reserved[2] = 0;
> +
> + BUILD_BUG_ON(VIRTIO_SPI_CPHA != SPI_CPHA);
> + BUILD_BUG_ON(VIRTIO_SPI_CPOL != SPI_CPOL);
> + BUILD_BUG_ON(VIRTIO_SPI_CS_HIGH != SPI_CS_HIGH);
> + BUILD_BUG_ON(VIRTIO_SPI_MODE_LSB_FIRST != SPI_LSB_FIRST);
> +
> + th->mode = cpu_to_le32(spi->mode & (SPI_LSB_FIRST | SPI_CS_HIGH |
> + SPI_CPOL | SPI_CPHA));
> + if ((spi->mode & SPI_LOOP) != 0)
> + th->mode |= cpu_to_le32(VIRTIO_SPI_MODE_LOOP);
> +
> + th->freq = cpu_to_le32(xfer->speed_hz);
> +
> + ret = spi_delay_to_ns(&xfer->word_delay, xfer);
> + if (ret < 0) {
> + dev_warn(dev, "Cannot convert word_delay\n");
> + goto msg_done;
> + }
> + th->word_delay_ns = cpu_to_le32((u32)ret);
> +
> + ret = spi_delay_to_ns(&xfer->delay, xfer);
> + if (ret < 0) {
> + dev_warn(dev, "Cannot convert delay\n");
> + goto msg_done;
> + }
> + th->cs_setup_ns = cpu_to_le32((u32)ret);
> + th->cs_delay_hold_ns = cpu_to_le32((u32)ret);
> +
> + /* This is the "off" time when CS has to be deasserted for a moment */
> + ret = spi_delay_to_ns(&xfer->cs_change_delay, xfer);
> + if (ret < 0) {
> + dev_warn(dev, "Cannot convert cs_change_delay\n");
> + goto msg_done;
> + }
> + th->cs_change_delay_inactive_ns = cpu_to_le32((u32)ret);
Hi Harald,
spi_device structure has three cs timing members, which will also
affect the cs timing.
struct spi_device {
...
struct spi_delay cs_setup;
struct spi_delay cs_hold;
struct spi_delay cs_inactive;
...
}
These three values should also be passed to the backend, for the
controller to control cs lines.
spi_device->cs_setup is the delay before cs asserted,
spi_device->cs_hold with spi_transfer->delay work before cs deasserted,
and spi_device->cs_inactive with spi_transfer->cs_change_delay take
effect at the stage after cs deasserted.
Here is the diagram
. . . . . . . . . .
Delay + A + + B + + C + D + E + F + A +
. . . . . . . . . .
___. . . . . . .___.___. .
CS# |___.______.____.____.___.___| . |___._____________
. . . . . . . . . .
. . . . . . . . . .
SCLK__.___.___NNN_____NNN__.___.___.___.___.___.___NNN_______
NOTE: 1st transfer has two words, the delay betweent these two words are
'B' in the diagram.
A => struct spi_device -> cs_setup
B => max{struct spi_transfer -> word_delay,
struct spi_device -> word_delay}
Note: spi_device and spi_transfer both have word_delay, Linux
choose the bigger one, refer to _spi_xfer_word_delay_update
function
C => struct spi_transfer -> delay
D => struct spi_device -> cs_hold
E => struct spi_device -> cs_inactive
F => struct spi_transfer -> cs_change_delay
So the corresponding relationship:
A <===> cs_setup_ns (after CS asserted)
B <===> word_delay_ns (no matter with CS)
C+D <===> cs_delay_hold_ns (before CS deasserted)
E+F <===> cs_change_delay_inactive_ns (after CS deasserted, these two
values also recommend in Linux driver to be added up)
Best Regards
Haixu
> +
> + /* Set buffers */
> + spi_req->tx_buf = xfer->tx_buf;
> + spi_req->rx_buf = xfer->rx_buf;
> +
> + /* Prepare sending of virtio message */
> + init_completion(&spi_req->completion);
> +
> + sg_init_one(&sg_out_head, &spi_req->transfer_head,
> + sizeof(struct spi_transfer_head));
> + sgs[outcnt] = &sg_out_head;
> + outcnt++;
> +
> + if (spi_req->tx_buf) {
> + sg_init_one(&sg_out_payload, spi_req->tx_buf, xfer->len);
> + sgs[outcnt] = &sg_out_payload;
> + outcnt++;
> + }
> +
> + if (spi_req->rx_buf) {
> + sg_init_one(&sg_in_payload, spi_req->rx_buf, xfer->len);
> + sgs[outcnt + incnt] = &sg_in_payload;
> + incnt++;
> + }
> +
> + sg_init_one(&sg_in_result, &spi_req->result,
> + sizeof(struct spi_transfer_result));
> + sgs[outcnt + incnt] = &sg_in_result;
> + incnt++;
> +
> + ret = virtqueue_add_sgs(priv->vq, sgs, outcnt, incnt, spi_req,
> + GFP_KERNEL);
> +
> +msg_done:
> + if (ret)
> + msg->status = ret;
> +
> + return ret;
> +}
Powered by blists - more mailing lists