pon., 11 maj 2020 o 21:24 Florian Fainelli <f.fainelli@xxxxxxxxx> napisał(a):
>
>
>
> On 5/11/2020 8:07 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> >
> > This adds the driver for the MediaTek Ethernet MAC used on the MT8* SoC
> > family. For now we only support full-duplex.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > ---
>
> [snip]
>
> > +static int mtk_mac_ring_pop_tail(struct mtk_mac_ring *ring,
> > + struct mtk_mac_ring_desc_data *desc_data)
> > +{
> > + struct mtk_mac_ring_desc *desc = &ring->descs[ring->tail];
> > + unsigned int status;
> > +
> > + /* Let the device release the descriptor. */
> > + dma_rmb();
> > + status = desc->status;
> > +
> > + if (!(status & MTK_MAC_DESC_BIT_COWN))
> > + return -1;
> > +
> > + desc_data->len = status & MTK_MAC_DESC_MSK_LEN;
> > + desc_data->flags = status & ~MTK_MAC_DESC_MSK_LEN;
> > + desc_data->dma_addr = desc->data_ptr;
> > + desc_data->skb = ring->skbs[ring->tail];
> > +
> > + desc->data_ptr = 0;
> > + desc->status = MTK_MAC_DESC_BIT_COWN;
> > + if (status & MTK_MAC_DESC_BIT_EOR)
> > + desc->status |= MTK_MAC_DESC_BIT_EOR;
>
> Don't you need a dma_wmb() for the device to observe the new descriptor
> here?
>
HW has released the descriptor (set the COWN bit) and I just clear all
other bits here really. Yeah, I guess it won't hurt to make sure.
> [snip]
>
> > +static void mtk_mac_dma_unmap_tx(struct mtk_mac_priv *priv,
> > + struct mtk_mac_ring_desc_data *desc_data)
> > +{
> > + struct device *dev = mtk_mac_get_dev(priv);
> > +
> > + return dma_unmap_single(dev, desc_data->dma_addr,
> > + desc_data->len, DMA_TO_DEVICE);
>
> If you stored a pointer to the sk_buff you transmitted, then you would
> need an expensive read to the descriptor to determine the address and
> length, and you would also not be at the mercy of the HW putting
> incorrect values there.
>
You mean store the mapped addresses? Yeah I can do that but I'll still
need to read the descriptor memory to verify it was released by HW.
> sp
> > +static void mtk_mac_dma_init(struct mtk_mac_priv *priv)
> > +{
> > + struct mtk_mac_ring_desc *desc;
> > + unsigned int val;
> > + int i;
> > +
> > + priv->descs_base = (struct mtk_mac_ring_desc *)priv->ring_base;
> > +
> > + for (i = 0; i < MTK_MAC_NUM_DESCS_TOTAL; i++) {
> > + desc = &priv->descs_base[i];
> > +
> > + memset(desc, 0, sizeof(*desc));
> > + desc->status = MTK_MAC_DESC_BIT_COWN;
> > + if ((i == MTK_MAC_NUM_TX_DESCS - 1) ||
> > + (i == MTK_MAC_NUM_DESCS_TOTAL - 1))
> > + desc->status |= MTK_MAC_DESC_BIT_EOR;
> > + }
> > +
> > + mtk_mac_ring_init(&priv->tx_ring, priv->descs_base, 0);
> > + mtk_mac_ring_init(&priv->rx_ring,
> > + priv->descs_base + MTK_MAC_NUM_TX_DESCS,
> > + MTK_MAC_NUM_RX_DESCS);
> > +
> > + /* Set DMA pointers. */
> > + val = (unsigned int)priv->dma_addr;
>
> You would probably add a WARN_ON() or something that catches the upper
> 32-bits of the dma_addr being set, see my comment about the DMA mask
> setting.
>
Can it still happen if I check the return value of dma_set_mask_and_coherent()?
> [snip]
>
> > +static void mtk_mac_tx_complete_all(struct mtk_mac_priv *priv)
> > +{
> > + struct net_device *ndev = priv_to_netdev(priv);
> > + struct mtk_mac_ring *ring = &priv->tx_ring;
> > + int ret;
> > +
> > + for (;;) {
> > + mtk_mac_lock(priv);
> > +
> > + if (!mtk_mac_ring_descs_available(ring)) {
> > + mtk_mac_unlock(priv);
> > + break;
> > + }
> > +
> > + ret = mtk_mac_tx_complete_one(priv);
> > + if (ret) {
> > + mtk_mac_unlock(priv);
> > + break;
> > + }
> > +
> > + if (netif_queue_stopped(ndev))
> > + netif_wake_queue(ndev);
> > +
> > + mtk_mac_unlock(priv);
> > + }
>
> Where do you increment the net_device statistics to indicate the bytes
> and packets transmitted?
>
I don't. I use the counters provided by HW for that.
> [snip]
>
> > + mtk_mac_set_mode_rmii(priv);
> > +
> > + dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>
> Your code assumes that DMA addresses are not going to be >= 4GB so you
> should be checking this function's return code and abort here otherwise
> your driver will fail in surprisingly difficult ways to debug.
Sure, thanks.
Bart
