On 10.02.2021 03:39, Andrew Lunn wrote:
+static inline u32 enet_read(struct bcm4908_enet *enet, u16 offset) +{ + return readl(enet->base + offset); +}No inline functions in C files please. Let the compiler decide.
According to the kernel's coding style (coding-style.rst) inline should *not* be used for 3+ LOC functions in general. According to that, I should only fix my enet_maskset() which isn't 1 LOC indeed. If that Documentation is outdated and/or inaccurate, could you propose a change for it, please? That rule comes from 2006 (a771f2b82aa2), so I understand it may need updating. We should have that officially documented though, to avoid per-tree or per-maintainer rules for stuff like this. Personally I don't have enough compiler knowledge to propose and/or discuss such stuff. That's why I prefer following Documentation written by smarter ones ;)
+static int bcm4908_dma_alloc_buf_descs(struct bcm4908_enet *enet, + struct bcm4908_enet_dma_ring *ring) +{ + int size = ring->length * sizeof(struct bcm4908_enet_dma_ring_bd); + struct device *dev = enet->dev; + + ring->cpu_addr = dma_alloc_coherent(dev, size, &ring->dma_addr, GFP_KERNEL); + if (!ring->cpu_addr) + return -ENOMEM; + + if (((uintptr_t)ring->cpu_addr) & (0x40 - 1)) { + dev_err(dev, "Invalid DMA ring alignment\n"); + goto err_free_buf_descs; + } + + ring->slots = kzalloc(ring->length * sizeof(*ring->slots), GFP_KERNEL); + if (!ring->slots) + goto err_free_buf_descs; + + memset(ring->cpu_addr, 0, size);It looks like dma_alloc_coherent() will perform a clear. See __dma_alloc_from_coherent()
Thanks!
+static void bcm4908_enet_dma_reset(struct bcm4908_enet *enet) +{ + struct bcm4908_enet_dma_ring *rings[] = { &enet->rx_ring, &enet->tx_ring }; + int i; + + /* Disable the DMA controller and channel */ + for (i = 0; i < ARRAY_SIZE(rings); i++) + enet_write(enet, rings[i]->cfg_block + ENET_DMA_CH_CFG, 0); + enet_maskset(enet, ENET_DMA_CONTROLLER_CFG, ENET_DMA_CTRL_CFG_MASTER_EN, 0);Is there a need to wait for any in flight DMA transfers to complete before you go further? Or is that what bcm4908_enet_dma_rx_ring_disable() is doing?
bcm4908_enet_dma_rx_ring_disable() checks for DMA to "confirm" it got stopped.
+ + /* Reset channels state */ + for (i = 0; i < ARRAY_SIZE(rings); i++) { + struct bcm4908_enet_dma_ring *ring = rings[i]; + + enet_write(enet, ring->st_ram_block + ENET_DMA_CH_STATE_RAM_BASE_DESC_PTR, 0); + enet_write(enet, ring->st_ram_block + ENET_DMA_CH_STATE_RAM_STATE_DATA, 0); + enet_write(enet, ring->st_ram_block + ENET_DMA_CH_STATE_RAM_DESC_LEN_STATUS, 0); + enet_write(enet, ring->st_ram_block + ENET_DMA_CH_STATE_RAM_DESC_BASE_BUFPTR, 0); + } +} + +static void bcm4908_enet_dma_tx_ring_ensable(struct bcm4908_enet *enet, + struct bcm4908_enet_dma_ring *ring)enable not ensable?
Absolutely :)
+static int bcm4908_enet_open(struct net_device *netdev) +{ + struct bcm4908_enet *enet = netdev_priv(netdev); + struct device *dev = enet->dev; + int err; + + err = request_irq(netdev->irq, bcm4908_enet_irq_handler, 0, "enet", enet); + if (err) { + dev_err(dev, "Failed to request IRQ %d: %d\n", netdev->irq, err); + return err; + } + + bcm4908_enet_gmac_init(enet); + bcm4908_enet_dma_reset(enet); + bcm4908_enet_dma_init(enet); + + enet_umac_set(enet, UMAC_CMD, CMD_TX_EN | CMD_RX_EN); + + enet_set(enet, ENET_DMA_CONTROLLER_CFG, ENET_DMA_CTRL_CFG_MASTER_EN); + enet_maskset(enet, ENET_DMA_CONTROLLER_CFG, ENET_DMA_CTRL_CFG_FLOWC_CH1_EN, 0); + bcm4908_enet_dma_rx_ring_enable(enet, &enet->rx_ring); + + napi_enable(&enet->napi); + netif_carrier_on(netdev); + netif_start_queue(netdev); + + bcm4908_enet_intrs_ack(enet); + bcm4908_enet_intrs_on(enet); + + return 0; +}No PHY handling? It would be normal to connect the phy in open.
I believe so, this controller is integrated into SoC and is always connected to the (internal) switch port. It uses a fixed link.
