> -----Original Message-----
> From: Laurentiu Tudor
> Sent: Tuesday, October 22, 2019 1:35 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>; da...@davemloft.net;
> netdev@vger.kernel.org
> Cc: Roy Pledge <roy.ple...@nxp.com>
> Subject: Re: [PATCH net-next 5/6] dpaa_eth: change DMA device
>
>
>
> On 22.10.2019 12:10, Madalin-cristian Bucur wrote:
> >> -----Original Message-----
> >> From: Laurentiu Tudor
> >> Sent: Tuesday, October 22, 2019 11:50 AM
> >> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>;
> da...@davemloft.net;
> >> netdev@vger.kernel.org
> >> Cc: Roy Pledge <roy.ple...@nxp.com>
> >> Subject: Re: [PATCH net-next 5/6] dpaa_eth: change DMA device
> >>
> >> Hello,
> >>
> >> On 21.10.2019 15:28, Madalin-cristian Bucur wrote:
> >>> The DPAA Ethernet driver is using the FMan MAC as the device for DMA
> >>> mapping. This is not actually correct, as the real DMA device is the
> >>> FMan port (the FMan Rx port for reception and the FMan Tx port for
> >>> transmission). Changing the device used for DMA mapping to the Fman
> >>> Rx and Tx port devices.
> >>>
> >>> Signed-off-by: Madalin Bucur <madalin.bu...@nxp.com>
> >>> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@nxp.com>
> >>> ---
> >>> drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 105 +++++++++++++-
> ---
> >> --------
> >>> drivers/net/ethernet/freescale/dpaa/dpaa_eth.h | 8 +-
> >>> 2 files changed, 62 insertions(+), 51 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> >> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> >>> index 8d5686d88d30..639cafaa59b8 100644
> >>> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> >>> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> >>> @@ -1335,15 +1335,15 @@ static void dpaa_fd_release(const struct
> >> net_device *net_dev,
> >>> vaddr = phys_to_virt(qm_fd_addr(fd));
> >>> sgt = vaddr + qm_fd_get_offset(fd);
> >>>
> >>> - dma_unmap_single(dpaa_bp->dev, qm_fd_addr(fd), dpaa_bp->size,
> >>> - DMA_FROM_DEVICE);
> >>> + dma_unmap_single(dpaa_bp->priv->rx_dma_dev, qm_fd_addr(fd),
> >>> + dpaa_bp->size, DMA_FROM_DEVICE);
> >>>
> >>> dpaa_release_sgt_members(sgt);
> >>>
> >>> - addr = dma_map_single(dpaa_bp->dev, vaddr, dpaa_bp->size,
> >>> - DMA_FROM_DEVICE);
> >>> - if (dma_mapping_error(dpaa_bp->dev, addr)) {
> >>> - dev_err(dpaa_bp->dev, "DMA mapping failed");
> >>> + addr = dma_map_single(dpaa_bp->priv->rx_dma_dev, vaddr,
> >>> + dpaa_bp->size, DMA_FROM_DEVICE);
> >>> + if (dma_mapping_error(dpaa_bp->priv->rx_dma_dev, addr)) {
> >>> + netdev_err(net_dev, "DMA mapping failed");
> >>> return;
> >>> }
> >>> bm_buffer_set64(&bmb, addr);
> >>> @@ -1488,7 +1488,7 @@ static int dpaa_enable_tx_csum(struct dpaa_priv
> >> *priv,
> >>>
> >>> static int dpaa_bp_add_8_bufs(const struct dpaa_bp *dpaa_bp)
> >>> {
> >>> - struct device *dev = dpaa_bp->dev;
> >>> + struct net_device *net_dev = dpaa_bp->priv->net_dev;
> >>> struct bm_buffer bmb[8];
> >>> dma_addr_t addr;
> >>> void *new_buf;
> >>> @@ -1497,16 +1497,18 @@ static int dpaa_bp_add_8_bufs(const struct
> >> dpaa_bp *dpaa_bp)
> >>> for (i = 0; i < 8; i++) {
> >>> new_buf = netdev_alloc_frag(dpaa_bp->raw_size);
> >>> if (unlikely(!new_buf)) {
> >>> - dev_err(dev, "netdev_alloc_frag() failed, size %zu\n",
> >>> - dpaa_bp->raw_size);
> >>> + netdev_err(net_dev,
> >>> + "netdev_alloc_frag() failed, size %zu\n",
> >>> + dpaa_bp->raw_size);
> >>> goto release_previous_buffs;
> >>> }
> >>> new_buf = PTR_ALIGN(new_buf, SMP_CACHE_BYTES);
> >>>
> >>> - addr = dma_map_single(dev, new_buf,
> >>> + addr = dma_map_single(dpaa_bp->priv->rx_dma_dev, new_buf,
> >>> dpaa_bp->size, DMA_FROM_DEVICE);
> >>> - if (unlikely(dma_mapping_error(dev, addr))) {
> >>> - dev_err(dpaa_bp->dev, "DMA map failed");
> >>> + if (unlikely(dma_mapping_error(dpaa_bp->priv->rx_dma_dev,
> >>> + addr))) {
> >>> + netdev_err(net_dev, "DMA map failed");
> >>> goto release_previous_buffs;
> >>> }
> >>>
> >>> @@ -1634,7 +1636,7 @@ static struct sk_buff *dpaa_cleanup_tx_fd(const
> >> struct dpaa_priv *priv,
> >>>
> >>> if (unlikely(qm_fd_get_format(fd) == qm_fd_sg)) {
> >>> nr_frags = skb_shinfo(skb)->nr_frags;
> >>> - dma_unmap_single(dev, addr,
> >>> + dma_unmap_single(priv->tx_dma_dev, addr,
> >>> qm_fd_get_offset(fd) + DPAA_SGT_SIZE,
> >>> dma_dir);
> >>>
> >>> @@ -1644,21 +1646,21 @@ static struct sk_buff
> *dpaa_cleanup_tx_fd(const
> >> struct dpaa_priv *priv,
> >>> sgt = phys_to_virt(addr + qm_fd_get_offset(fd));
> >>>
> >>> /* sgt[0] is from lowmem, was dma_map_single()-ed */
> >>> - dma_unmap_single(dev, qm_sg_addr(&sgt[0]),
> >>> + dma_unmap_single(priv->tx_dma_dev, qm_sg_addr(&sgt[0]),
> >>> qm_sg_entry_get_len(&sgt[0]), dma_dir);
> >>>
> >>> /* remaining pages were mapped with skb_frag_dma_map()
> */
> >>> for (i = 1; i <= nr_frags; i++) {
> >>> WARN_ON(qm_sg_entry_is_ext(&sgt[i]));
> >>>
> >>> - dma_unmap_page(dev, qm_sg_addr(&sgt[i]),
> >>> + dma_unmap_page(priv->tx_dma_dev, qm_sg_addr(&sgt[i]),
> >>> qm_sg_entry_get_len(&sgt[i]),
> dma_dir);
> >>> }
> >>>
> >>> /* Free the page frag that we allocated on Tx */
> >>> skb_free_frag(phys_to_virt(addr));
> >>> } else {
> >>> - dma_unmap_single(dev, addr,
> >>> + dma_unmap_single(priv->tx_dma_dev, addr,
> >>> skb_tail_pointer(skb) - (u8 *)skbh,
> dma_dir);
> >>> }
> >>>
> >>> @@ -1762,8 +1764,8 @@ static struct sk_buff *sg_fd_to_skb(const struct
> >> dpaa_priv *priv,
> >>> goto free_buffers;
> >>>
> >>> count_ptr = this_cpu_ptr(dpaa_bp->percpu_count);
> >>> - dma_unmap_single(dpaa_bp->dev, sg_addr, dpaa_bp->size,
> >>> - DMA_FROM_DEVICE);
> >>> + dma_unmap_single(dpaa_bp->priv->rx_dma_dev, sg_addr,
> >>> + dpaa_bp->size, DMA_FROM_DEVICE);
> >>> if (!skb) {
> >>> sz = dpaa_bp->size +
> >>> SKB_DATA_ALIGN(sizeof(struct
> skb_shared_info));
> >>> @@ -1853,7 +1855,6 @@ static int skb_to_contig_fd(struct dpaa_priv
> >> *priv,
> >>> int *offset)
> >>> {
> >>> struct net_device *net_dev = priv->net_dev;
> >>> - struct device *dev = net_dev->dev.parent;
> >>> enum dma_data_direction dma_dir;
> >>> unsigned char *buffer_start;
> >>> struct sk_buff **skbh;
> >>> @@ -1889,9 +1890,9 @@ static int skb_to_contig_fd(struct dpaa_priv
> >> *priv,
> >>> fd->cmd |= cpu_to_be32(FM_FD_CMD_FCO);
> >>>
> >>> /* Map the entire buffer size that may be seen by FMan, but no
> more
> >> */
> >>> - addr = dma_map_single(dev, skbh,
> >>> + addr = dma_map_single(priv->tx_dma_dev, skbh,
> >>> skb_tail_pointer(skb) - buffer_start,
> dma_dir);
> >>> - if (unlikely(dma_mapping_error(dev, addr))) {
> >>> + if (unlikely(dma_mapping_error(priv->tx_dma_dev, addr))) {
> >>> if (net_ratelimit())
> >>> netif_err(priv, tx_err, net_dev,
> >>> "dma_map_single()
> >> failed\n");
> >>> return -EINVAL;
> >>> @@ -1907,7 +1908,6 @@ static int skb_to_sg_fd(struct dpaa_priv *priv,
> >>> const enum dma_data_direction dma_dir = DMA_TO_DEVICE;
> >>> const int nr_frags = skb_shinfo(skb)->nr_frags;
> >>> struct net_device *net_dev = priv->net_dev;
> >>> - struct device *dev = net_dev->dev.parent;
> >>> struct qm_sg_entry *sgt;
> >>> struct sk_buff **skbh;
> >>> int i, j, err, sz;
> >>> @@ -1946,10 +1946,10 @@ static int skb_to_sg_fd(struct dpaa_priv
> *priv,
> >>> qm_sg_entry_set_len(&sgt[0], frag_len);
> >>> sgt[0].bpid = FSL_DPAA_BPID_INV;
> >>> sgt[0].offset = 0;
> >>> - addr = dma_map_single(dev, skb->data,
> >>> + addr = dma_map_single(priv->tx_dma_dev, skb->data,
> >>> skb_headlen(skb), dma_dir);
> >>> - if (unlikely(dma_mapping_error(dev, addr))) {
> >>> - dev_err(dev, "DMA mapping failed");
> >>> + if (unlikely(dma_mapping_error(priv->tx_dma_dev, addr))) {
> >>> + netdev_err(priv->net_dev, "DMA mapping failed");
> >>> err = -EINVAL;
> >>> goto sg0_map_failed;
> >>> }
> >>> @@ -1960,10 +1960,10 @@ static int skb_to_sg_fd(struct dpaa_priv
> *priv,
> >>> frag = &skb_shinfo(skb)->frags[i];
> >>> frag_len = skb_frag_size(frag);
> >>> WARN_ON(!skb_frag_page(frag));
> >>> - addr = skb_frag_dma_map(dev, frag, 0,
> >>> + addr = skb_frag_dma_map(priv->tx_dma_dev, frag, 0,
> >>> frag_len, dma_dir);
> >>> - if (unlikely(dma_mapping_error(dev, addr))) {
> >>> - dev_err(dev, "DMA mapping failed");
> >>> + if (unlikely(dma_mapping_error(priv->tx_dma_dev, addr))) {
> >>> + netdev_err(priv->net_dev, "DMA mapping failed");
> >>> err = -EINVAL;
> >>> goto sg_map_failed;
> >>> }
> >>> @@ -1986,10 +1986,10 @@ static int skb_to_sg_fd(struct dpaa_priv
> *priv,
> >>> skbh = (struct sk_buff **)buffer_start;
> >>> *skbh = skb;
> >>>
> >>> - addr = dma_map_single(dev, buffer_start,
> >>> + addr = dma_map_single(priv->tx_dma_dev, buffer_start,
> >>> priv->tx_headroom + DPAA_SGT_SIZE,
> >>> dma_dir);
> >>> - if (unlikely(dma_mapping_error(dev, addr))) {
> >>> - dev_err(dev, "DMA mapping failed");
> >>> + if (unlikely(dma_mapping_error(priv->tx_dma_dev, addr))) {
> >>> + netdev_err(priv->net_dev, "DMA mapping failed");
> >>> err = -EINVAL;
> >>> goto sgt_map_failed;
> >>> }
> >>> @@ -2003,7 +2003,7 @@ static int skb_to_sg_fd(struct dpaa_priv *priv,
> >>> sgt_map_failed:
> >>> sg_map_failed:
> >>> for (j = 0; j < i; j++)
> >>> - dma_unmap_page(dev, qm_sg_addr(&sgt[j]),
> >>> + dma_unmap_page(priv->tx_dma_dev, qm_sg_addr(&sgt[j]),
> >>> qm_sg_entry_get_len(&sgt[j]), dma_dir);
> >>> sg0_map_failed:
> >>> csum_failed:
> >>> @@ -2304,7 +2304,8 @@ static enum qman_cb_dqrr_result
> >> rx_default_dqrr(struct qman_portal *portal,
> >>> return qman_cb_dqrr_consume;
> >>> }
> >>>
> >>> - dma_unmap_single(dpaa_bp->dev, addr, dpaa_bp->size,
> >> DMA_FROM_DEVICE);
> >>> + dma_unmap_single(dpaa_bp->priv->rx_dma_dev, addr, dpaa_bp->size,
> >>> + DMA_FROM_DEVICE);
> >>>
> >>> /* prefetch the first 64 bytes of the frame or the SGT start
> */
> >>> vaddr = phys_to_virt(addr);
> >>> @@ -2659,7 +2660,7 @@ static inline void dpaa_bp_free_pf(const struct
> >> dpaa_bp *bp,
> >>> {
> >>> dma_addr_t addr = bm_buf_addr(bmb);
> >>>
> >>> - dma_unmap_single(bp->dev, addr, bp->size, DMA_FROM_DEVICE);
> >>> + dma_unmap_single(bp->priv->rx_dma_dev, addr, bp->size,
> >> DMA_FROM_DEVICE);
> >>>
> >>> skb_free_frag(phys_to_virt(addr));
> >>> }
> >>> @@ -2769,25 +2770,27 @@ static int dpaa_eth_probe(struct
> platform_device
> >> *pdev)
> >>> int err = 0, i, channel;
> >>> struct device *dev;
> >>>
> >>> + dev = &pdev->dev;
> >>> +
> >>> err = bman_is_probed();
> >>> if (!err)
> >>> return -EPROBE_DEFER;
> >>> if (err < 0) {
> >>> - dev_err(&pdev->dev, "failing probe due to bman probe
> >> error\n");
> >>> + dev_err(dev, "failing probe due to bman probe error\n");
> >>
> >> These changes seem unrelated.
> >
> > The &pdev->dev to dev replacement is not related directly to the
> incorrect
> > DMA mapping device but a device had to be used for the prints and
> propagating
> > &pdev->dev did not look like a good idea. Initially I had a separate
> patch for
> > this but it's superfluous to add code and remove it in another patch
> doing almost
> > nothing.
>
> Fair enough.
>
> >>> return -ENODEV;
> >>> }
> >>> err = qman_is_probed();
> >>> if (!err)
> >>> return -EPROBE_DEFER;
> >>> if (err < 0) {
> >>> - dev_err(&pdev->dev, "failing probe due to qman probe
> >> error\n");
> >>> + dev_err(dev, "failing probe due to qman probe error\n");
> >>> return -ENODEV;
> >>> }
> >>> err = bman_portals_probed();
> >>> if (!err)
> >>> return -EPROBE_DEFER;
> >>> if (err < 0) {
> >>> - dev_err(&pdev->dev,
> >>> + dev_err(dev,
> >>> "failing probe due to bman portals probe
> error\n");
> >>> return -ENODEV;
> >>> }
> >>> @@ -2795,19 +2798,11 @@ static int dpaa_eth_probe(struct
> platform_device
> >> *pdev)
> >>> if (!err)
> >>> return -EPROBE_DEFER;
> >>> if (err < 0) {
> >>> - dev_err(&pdev->dev,
> >>> + dev_err(dev,
> >>> "failing probe due to qman portals probe
> error\n");
> >>> return -ENODEV;
> >>> }
> >>>
> >>> - /* device used for DMA mapping */
> >>> - dev = pdev->dev.parent;
> >>> - err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40));
> >>> - if (err) {
> >>> - dev_err(dev, "dma_coerce_mask_and_coherent() failed\n");
> >>> - return err;
> >>> - }
> >>
> >> Why are we dropping this explicit setting of the dma mask?
> >>
> >> ---
> >> Best Regards, Laurentiu
> >
> > Hi Laurentiu, you are probably reviewing these changes with your initial
> > patch in mind that was using (incorrectly) the same (Rx) port for DMA
> > mapping of both receive and transmit traffic. Please take a second look
> at
> > the changes in this patch.
>
> My bad, didn't notice you're setting it below. Sorry for the noise.
>
> ---
> Best Regards, Laurentiu
Nevermind, I need to send everything again just to get the sender name
correctly...
Madalin
