Re: [PATCH v2 02/14] net: wwan: t7xx: Add control DMA interface

["Martinez, Ricardo" <ricardo.martinez@linux.intel.com>]

On 11/1/2021 7:03 AM, Andy Shevchenko wrote:
> On Sun, Oct 31, 2021 at 08:56:23PM -0700, Ricardo Martinez wrote:
>> From: Haijun Lio <haijun.liu@mediatek.com>
>>
>> Cross Layer DMA (CLDMA) Hardware interface (HIF) enables the control
>> path of Host-Modem data transfers. CLDMA HIF layer provides a common
>> interface to the Port Layer.
>>
>> CLDMA manages 8 independent RX/TX physical channels with data flow
>> control in HW queues. CLDMA uses ring buffers of General Packet
>> Descriptors (GPD) for TX/RX. GPDs can represent multiple or single
>> data buffers (DB).
>>
>> CLDMA HIF initializes GPD rings, registers ISR handlers for CLDMA
>> interrupts, and initializes CLDMA HW registers.
>>
>> CLDMA TX flow:
>> 1. Port Layer write
>> 2. Get DB address
>> 3. Configure GPD
>> 4. Triggering processing via HW register write
>>
>> CLDMA RX flow:
>> 1. CLDMA HW sends a RX "done" to host
>> 2. Driver starts thread to safely read GPD
>> 3. DB is sent to Port layer
>> 4. Create a new buffer for GPD ring
...
>
>> +void cldma_hw_reset(void __iomem *ao_base)
>> +{
>> +	iowrite32(ioread32(ao_base + REG_INFRA_RST4_SET) | RST4_CLDMA1_SW_RST_SET,
>> +		  ao_base + REG_INFRA_RST4_SET);
>> +	iowrite32(ioread32(ao_base + REG_INFRA_RST2_SET) | RST2_CLDMA1_AO_SW_RST_SET,
>> +		  ao_base + REG_INFRA_RST2_SET);
>> +	udelay(1);
>> +	iowrite32(ioread32(ao_base + REG_INFRA_RST4_CLR) | RST4_CLDMA1_SW_RST_CLR,
>> +		  ao_base + REG_INFRA_RST4_CLR);
>> +	iowrite32(ioread32(ao_base + REG_INFRA_RST2_CLR) | RST2_CLDMA1_AO_SW_RST_CLR,
>> +		  ao_base + REG_INFRA_RST2_CLR);
> Setting and clearing are in the same order, is it okay?
> Can we do it rather symmetrical?
In this case, order does not matter.

This will be symmetrical in the next iteration.

>> +}
> ...
>
>> +	mb(); /* prevents outstanding GPD updates */
> Is there any counterpart of this barrier?

This is not needed, removing it.

...

>
>> +		ret = cldma_gpd_rx_from_queue(queue, budget, &over_budget);
>> +		if (ret == -ENODATA)
>> +			return 0;
>> +
>> +		if (ret)
>> +			return ret;
> Drop redundant blank line

The style followed is to keep a blank line after 'if' blocks.

Is that acceptable as long as it is consistent across the driver?
>
>> +			/* greedy mode */
>> +			l2_rx_int = cldma_hw_int_status(hw_info, BIT(queue->index), true);
...
>
>> +exit:
> Seems useless.

This tag is used when the PM patch is introduced later in the same series.

> +	return ret;
...