linux-kernel - Re: [PATCH 2/2] spi: spi-qcom-qspi: Add DMA mode support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0b182a36-0254-6720-4a35-f9e617c12797@quicinc.com>
Date:   Thu, 6 Apr 2023 20:23:21 +0530
From:   Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>
To:     Mark Brown <broonie@...nel.org>
CC:     <agross@...nel.org>, <andersson@...nel.org>,
        <konrad.dybcio@...aro.org>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <vkoul@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-spi@...r.kernel.org>,
        <quic_msavaliy@...cinc.com>, <dianders@...omium.org>,
        <mka@...omium.org>, <swboyd@...omium.org>,
        <quic_vtanuku@...cinc.com>
Subject: Re: [PATCH 2/2] spi: spi-qcom-qspi: Add DMA mode support

All reviewers,

Thank you very much for your time and review...

While I am addressing other comments, below are some responses...


On 4/4/2023 11:47 PM, Mark Brown wrote:
> On Tue, Apr 04, 2023 at 11:33:20PM +0530, Vijaya Krishna Nivarthi wrote:
>
> A few quick review comments, mostly coding style though.
>
>> +struct qspi_cmd_desc {
>> +	uint32_t data_address;
>> +	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;
>> +	//------------------------//
> What does this mean?

That separates the part of cmd_desc that is visible to the HW and the 
part that is required by the SW after xfer is complete.
I can add a comment in v2?


>
>> +	uint8_t *bounce_src;
>> +	uint8_t *bounce_dst;
>> +	uint32_t bounce_length;
>> +};
>> +
>> +#define QSPI_MAX_NUM_DESC 5
>>   struct qspi_xfer {
> Missing blank line after the define...


Will address in v2

>
>> +	for (ii = 0; ii < sgt->nents; ii++)
>> +		sg_total_len += sg_dma_len(sgt->sgl + ii);
> Why are we calling the iterator ii here?


Calling it ii helps in finding iterator more easily in code.

should I stick to i in v2?

>
>> +	if (ctrl->xfer.dir == QSPI_READ)
>> +		byte_ptr = (uint8_t *)xfer->rx_buf;
>> +	else
>> +		byte_ptr = (uint8_t *)xfer->tx_buf;
> If we need to cast to or from void * there's some sort of problem.


the tx_buf is a const void*

in v2 I will cast for tx_buf only?

>
>> +/* Switch to DMA if transfer length exceeds this */
>> +#define QSPI_MAX_BYTES_FIFO 64
>> +#define NO_TX_DATA_DELAY_FOR_DMA 1
>> +#define DMA_CONDITIONAL (xfer->len > QSPI_MAX_BYTES_FIFO)
>> +
> DMA_CONDITIONAL absolutely should not be a define, this just makes
> things harder to read.  Just have everything call can_dma() when needed.


Will address in v2

>
>> +#if NO_TX_DATA_DELAY_FOR_DMA
>> +		mstr_cfg &= ~(TX_DATA_OE_DELAY_MSK | TX_DATA_DELAY_MSK);
>> +#endif
> Why would we add extra delays if we don't need them, might someone set
> this and if so when?


I believe its used when some slave devices need a delay between clock 
and data.

Its configured as 1 for PIO_MODE(FIFO) right now.

For DMA_MODE we are not using same, both seem to work for DMA.

>
>> +	if (ctrl->xfer_mode == QSPI_FIFO) {
>> +	} else if (ctrl->xfer_mode == QSPI_DMA) {
>>   	}
> This should be a switch statement.


Will address in v2