[U-Boot] [RFC v3 03/15] dma: add bcm6348-iudma support

[Grygorii Strashko <grygorii.strashko@ti.com>]

Hi

thanks for your comments.

On 02/22/2018 02:48 PM, Álvaro Fernández Rojas wrote:
> El 22/02/2018 a las 20:50, Grygorii Strashko escribió:
>> On 02/21/2018 10:10 AM, Álvaro Fernández Rojas wrote:
>>> BCM6348 IUDMA controller is present on multiple BMIPS (BCM63xx) SoCs.
>>>
>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>> ---
>>>    v3: no changes
>>>    v2: Fix dma rx burst config and select DMA_CHANNELS.
>>>
>>>    drivers/dma/Kconfig         |   9 +
>>>    drivers/dma/Makefile        |   1 +
>>>    drivers/dma/bcm6348-iudma.c | 505 
>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 515 insertions(+)
>>>    create mode 100644 drivers/dma/bcm6348-iudma.c
>>>
>> [...]
>>> +static int bcm6348_iudma_enable(struct dma *dma)
>>> +{
>>> +    struct bcm6348_iudma_priv *priv = dev_get_priv(dma->dev);
>>> +    struct bcm6348_chan_priv *ch_priv = priv->ch_priv[dma->id];
>>> +    struct bcm6348_dma_desc *dma_desc;
>>> +    uint8_t i;
>>> +
>>> +    /* init dma rings */
>>> +    dma_desc = ch_priv->dma_ring;
>>> +    for (i = 0; i < ch_priv->dma_ring_size; i++) {
>>> +        if (bcm6348_iudma_chan_is_rx(dma->id)) {
>>> +            dma_desc->status = DMAD_ST_OWN_MASK;
>>> +            dma_desc->length = PKTSIZE_ALIGN;
>>> +            dma_desc->address = virt_to_phys(net_rx_packets[i]);
>> You are filling RX queue/ring with buffers defined in Net core.
>> Does it mean that this DMA driver will not be usable for other 
>> purposes, as
>> Net can be compiled out?
> As far as I know, and depending on the specific SoC, BCM63xx IUDMA is 
> used for Ethernet, USB (device only) and xDSL.
> So yes, in u-boot it will be used for ethernet only.
> BTW, my first attempt didn't use net_rx_packets, but I saw that in 
> pic32_eth implementation and dropped the dma specific buffers. I will 
> add them again ;).

it is really net specific :)

>>
>> Wouldn't it be reasonable to have some sort of .receive_prepare() 
>> callback in
>> DMA dma_ops, so DMA user can control which buffers to push in RX DMA 
>> channel?
>> And it also can be used in eth_ops.free_pkt() callback (see below).
> Yes, probably, but maybe we can achieve that without adding another call.
>>

I'm looking at this patch set from our HW point of view. In my case,
DMA channel can be used with different IPs (not only networking), so
it would be really great if DMA user can pass RX buffers in DMA driver -
network driver can use net_rx_packets, other drivers might use own buffers.
So hard-codding RX buffers in DMA driver looks like not a good choice.

>>> +
>>> +    /* flush cache */
>>> +    flush_dcache_range((ulong)dma_desc,
>>> +        ALIGN_END_ADDR(struct bcm6348_dma_desc, dma_desc, 1));
>> Could you clarify pls, if you do return dma_desc to RX ring here or not?
> Yes.
>>
>> if yes, wouldn't it cause potential problem on Net RX path
>>         ret = eth_get_ops(current)->recv(current, flags, &packet);
>> ^^ (1) here buffer will be received from DMA ( and pushed back to RX 
>> ring ?? )
>>
>>         flags = 0;
>>         if (ret > 0)
>>             net_process_received_packet(packet, ret);
>> ^^ (2) here it will be passed in Net stack
>>
>>         if (ret >= 0 && eth_get_ops(current)->free_pkt)
>>             eth_get_ops(current)->free_pkt(current, packet, ret);
>> ^^ at this point it should be safe to return buffer in DMA RX ring.
>>
>>         if (ret <= 0)
>>             break;
>>
>> Can DMA overwrite packet after point (1) while packet is still 
>> processed (2)?
> I don't think so, because as far as I know u-boot is not processing more 
> than one packet at once, is it?

u-boot can't process more than one packet, but dma does. if buffer returned
to DMA and there are some traffic on the line - DMA can potentially refill 
all buffers in its RX ring while u-boot still processing one packet.


> But yeah, I see your point and if it does process more than one packet 
> at once this is not the proper way to do that.
> I will use free_pkt in next version and lock the packet until it's 
> processed.


-- 
regards,
-grygorii