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

[Álvaro Fernández Rojas]

Hi Grygorii,

El 23/02/2018 a las 17:57, Grygorii Strashko escribió:
> 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 at 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.
I just sent a new version of the patches which doesn't use 
net_rx_packets in bcm6348-iudma.
However, after several tests it turns out that dma received buffers must 
be processed as soon as possible in order to avoid flow control issues.
Considering that, releasing the buffer by using free_pkt after the 
packet is processed isn't a valid option, since it makes ethernet unusable.
That's why I decided to allocate a dynamic buffer on bcm6348-iudma, 
saving the received packets on net_rx_packets and immediately returning 
the dma descriptor to the RX ring.

However, you can still add dma_receive_prepare to dma ops if you need it 
for your HW.
>
>
>> 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,
Álvaro.