| 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
| ||
|
Date: Wed, 12 Apr 2023 20:58:42 +0530
From: Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>
To: Stephen Boyd <swboyd@...omium.org>, <agross@...nel.org>,
<andersson@...nel.org>, <broonie@...nel.org>,
<devicetree@...r.kernel.org>, <konrad.dybcio@...aro.org>,
<krzysztof.kozlowski+dt@...aro.org>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-spi@...r.kernel.org>, <robh+dt@...nel.org>,
<vkoul@...nel.org>
CC: <quic_msavaliy@...cinc.com>, <dianders@...omium.org>,
<mka@...omium.org>, <quic_vtanuku@...cinc.com>
Subject: Re: [PATCH 2/2] spi: spi-qcom-qspi: Add DMA mode support
Thank you for the review...
On 4/5/2023 2:15 AM, Stephen Boyd wrote:
> Quoting Vijaya Krishna Nivarthi (2023-04-04 11:03:20)
>> For performance improvement in terms of number of interrupts.
>> Code flow for DMA mode...
>> Allocate for DMA pools in probe()
>> For xfers with len > 64 bytes, return true from can_dma()
>> Thus framework creates and maps an sg table for each xfer buffer.
>> In transfer_one() enable DMA in MSTR_CONFIG.
>> Split the dma buffer of each entry of sgt into a maximum of 3 parts...
>> Unaligned head, aligned middle and tail.
>> For each part create a dma_cmd_descriptor.
>> For head and tail allocate dma_data_descriptors
>> For tx, copy the data into allocated data descriptors
>> For rx, remember the original buffers to copy from-
>> allocated descriptors after xfer is complete
>> For middle part use the aligned dma buffer.
>> Link each dma_cmd_descriptor to next.
>> Thus create a chain of descriptors.
>> Kick-off the xfer by copying 1st cmd descriptor to-
>> NEXT_DMA_DESC_ADDR register.
>> On receiving DMA_CHAIN_DONE interrupt, complete the xfer and-
>> free descriptors.
>> If timeout happens handle error by freeing descriptors.
>> In remove() free DMA pools.
>>
> Write a real commit text, and not psuedo-code for the patch. It should
> sell us on wanting to review the patch, and help us understand the
> importance of the change. The "how" comes from reading the patch
> and code itself.
Modified.
>> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>
>> ---
>> drivers/spi/spi-qcom-qspi.c | 429 ++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 398 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
>> index fab1553..64c3bec 100644
>> --- a/drivers/spi/spi-qcom-qspi.c
>> +++ b/drivers/spi/spi-qcom-qspi.c
>> @@ -13,7 +13,8 @@
>> #include <linux/pm_opp.h>
>> #include <linux/spi/spi.h>
>> #include <linux/spi/spi-mem.h>
>> -
>> +#include <linux/dmapool.h>
>> +#include <linux/dma-mapping.h>
> Sort this alphabetically like all the other headers here.
Done
>> #define QSPI_NUM_CS 2
>> #define QSPI_BYTES_PER_WORD 4
>> @@ -62,6 +63,7 @@
>> #define WR_FIFO_FULL BIT(10)
>> #define WR_FIFO_OVERRUN BIT(11)
>> #define TRANSACTION_DONE BIT(16)
>> +#define DMA_CHAIN_DONE BIT(31)
>> #define QSPI_ERR_IRQS (RESP_FIFO_UNDERRUN | HRESP_FROM_NOC_ERR | \
>> WR_FIFO_OVERRUN)
>> #define QSPI_ALL_IRQS (QSPI_ERR_IRQS | RESP_FIFO_RDY | \
>> @@ -108,6 +110,10 @@
>> #define RD_FIFO_RESET 0x0030
>> #define RESET_FIFO BIT(0)
>>
>> +#define NEXT_DMA_DESC_ADDR 0x0040
>> +#define CURRENT_DMA_DESC_ADDR 0x0044
>> +#define CURRENT_MEM_ADDR 0x0048
>> +
>> #define CUR_MEM_ADDR 0x0048
>> #define HW_VERSION 0x004c
>> #define RD_FIFO 0x0050
>> @@ -120,6 +126,22 @@ enum qspi_dir {
>> QSPI_WRITE,
>> };
>>
>> +struct qspi_cmd_desc {
>> + uint32_t data_address;
> Use u32/u8 instead of uint32_t/uint8_t
Done
>> + uint32_t next_descriptor;
>> + uint32_t direction:1;
>> + uint32_t multi_io_mode:3;
>> + uint32_t reserved1:4;
>> + uint32_t fragment:1;
>> + uint32_t reserved2:7;
>> + uint32_t length:16;
>> + //------------------------//
>> + uint8_t *bounce_src;
>> + uint8_t *bounce_dst;
>> + uint32_t bounce_length;
>> +};
>> +
>> +#define QSPI_MAX_NUM_DESC 5
>> struct qspi_xfer {
>> union {
>> const void *tx_buf;
>> @@ -137,11 +159,30 @@ enum qspi_clocks {
>> QSPI_NUM_CLKS
>> };
>>
>> +enum qspi_xfer_mode {
>> + QSPI_INVALID,
>> + QSPI_FIFO,
>> + QSPI_DMA
>> +};
>> +
>> +/* number of entries in sgt returned from spi framework that we can support */
>> +#define QSPI_QCOM_MAX_SG 5
>> +
>> struct qcom_qspi {
>> void __iomem *base;
>> struct device *dev;
>> struct clk_bulk_data *clks;
>> struct qspi_xfer xfer;
>> + struct dma_pool *dma_cmd_pool;
>> + struct dma_pool *dma_data_pool;
>> + dma_addr_t dma_cmd_desc[3*QSPI_QCOM_MAX_SG];
> Make a define for 3 and 2 and then a define for each equation?
Made defines for 3 and 2
>> + dma_addr_t dma_data_desc[2*QSPI_QCOM_MAX_SG];
>> + void *virt_cmd_desc[3*QSPI_QCOM_MAX_SG];
>> + void *virt_data_desc[2*QSPI_QCOM_MAX_SG];
>> + unsigned int n_cmd_desc;
>> + unsigned int n_data_desc;
>> + int xfer_mode;
>> + u32 iomode;
> Good use of u32
Thank you
>
>> struct icc_path *icc_path_cpu_to_qspi;
>> unsigned long last_speed;
>> /* Lock to protect data accessed by IRQs */
>> @@ -258,6 +326,186 @@ static int qcom_qspi_set_speed(struct qcom_qspi *ctrl, unsigned long speed_hz)
>> return 0;
>> }
>>
>> +/* aligned to 32 bytes, not to cross 1KB boundary */
>> +#define QSPI_ALIGN_REQ 32
>> +#define QSPI_BOUNDARY_REQ 1024
>> +
>> +int qcom_qspi_alloc_desc(struct qcom_qspi *ctrl, uint8_t *virt_ptr,
>> + dma_addr_t dma_ptr, uint32_t n_bytes)
>> +{
>> + struct qspi_cmd_desc *virt_cmd_desc, *prev;
>> + uint8_t *virt_data_desc;
>> + dma_addr_t dma_cmd_desc, dma_data_desc;
>> +
>> + if (virt_ptr && n_bytes >= QSPI_ALIGN_REQ) {
>> + dev_err(ctrl->dev,
>> + "Exiting to avert memory overwrite, n_bytes-%d\n", n_bytes);
>> + return -ENOMEM;
>> + }
>> +
>> + /* allocate for dma cmd descriptor */
>> + virt_cmd_desc = (struct qspi_cmd_desc *)dma_pool_alloc(ctrl->dma_cmd_pool,
>> + GFP_KERNEL, &dma_cmd_desc);
>> + if (!virt_cmd_desc) {
>> + dev_err(ctrl->dev,
>> + "Could not allocate for cmd_desc\n");
>> + return -ENOMEM;
>> + }
>> + ctrl->virt_cmd_desc[ctrl->n_cmd_desc] = virt_cmd_desc;
>> + ctrl->dma_cmd_desc[ctrl->n_cmd_desc] = dma_cmd_desc;
>> + ctrl->n_cmd_desc++;
>> +
>> + /* allocate for dma data descriptor if unaligned else use pre-aligned */
>> + if (virt_ptr) {
>> + virt_data_desc = (uint8_t *)dma_pool_zalloc(ctrl->dma_data_pool,
>> + GFP_KERNEL, &dma_data_desc);
>> + if (!virt_data_desc) {
>> + dev_err(ctrl->dev,
>> + "Could not allocate for data_desc\n");
>> + return -ENOMEM;
>> + }
>> + ctrl->virt_data_desc[ctrl->n_data_desc] = virt_data_desc;
>> + ctrl->dma_data_desc[ctrl->n_data_desc] = dma_data_desc;
>> + ctrl->n_data_desc++;
>> +
>> + /*
>> + * for tx copy xfer data into allocated buffer
>> + * for rx setup bounce info to copy after xfer
>> + */
>> + if (ctrl->xfer.dir == QSPI_WRITE) {
>> + memcpy(virt_data_desc, virt_ptr, n_bytes);
>> + } else {
>> + virt_cmd_desc->bounce_src = virt_data_desc;
>> + virt_cmd_desc->bounce_dst = virt_ptr;
>> + virt_cmd_desc->bounce_length = n_bytes;
>> + }
>> + } else {
>> + dma_data_desc = dma_ptr;
>> + }
>> +
>> + /* setup cmd descriptor */
>> + virt_cmd_desc->data_address = (uint32_t)(uintptr_t)(dma_data_desc);
> Why does this need to be casted?
Agreed not required, removed.
>> + virt_cmd_desc->next_descriptor = 0;
>> + virt_cmd_desc->direction = ctrl->xfer.dir;
>> + virt_cmd_desc->multi_io_mode = ctrl->iomode;
>> + virt_cmd_desc->reserved1 = 0;
>> + virt_cmd_desc->fragment = 0;
>> + virt_cmd_desc->reserved2 = 0;
>> + virt_cmd_desc->length = n_bytes;
>> +
>> + /* update previous descriptor */
>> + if (ctrl->n_cmd_desc >= 2) {
>> + prev = (ctrl->virt_cmd_desc)[ctrl->n_cmd_desc - 2];
>> + prev->next_descriptor = dma_cmd_desc;
>> + prev->fragment = 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_qspi_setup_dma_desc(struct qcom_qspi *ctrl,
>> + struct spi_transfer *xfer)
>> +{
>> + int ret;
>> + struct sg_table *sgt;
>> + unsigned int sg_total_len = 0;
>> + dma_addr_t dma_ptr_sg;
>> + unsigned int dma_len_sg;
>> + uint32_t prolog_bytes, aligned_bytes, epilog_bytes;
>> + dma_addr_t aligned_ptr;
> Don't put types in the variable name.
Renamed
>
>> + int ii;
> Why double i?
Renamed
>
>> + uint8_t *byte_ptr;
>> +
>> + if (ctrl->n_cmd_desc || ctrl->n_data_desc) {
>> + dev_err(ctrl->dev, "Remnant dma buffers cmd-%d, data-%d\n",
>> + ctrl->n_cmd_desc, ctrl->n_data_desc);
>> + return -EIO;
>> + }
>> +
>> + sgt = (ctrl->xfer.dir == QSPI_READ) ? &xfer->rx_sg : &xfer->tx_sg;
>> + if (!sgt->nents || sgt->nents > QSPI_QCOM_MAX_SG) {
>> + dev_err(ctrl->dev, "Cannot handle %d entries in scatter list\n", sgt->nents);
Powered by blists - more mailing lists