Message ID | 20210722141351.13668-10-biju.das.jz@bp.renesas.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | Add Gigabit Ethernet driver support | expand |
Hello! On 7/22/21 5:13 PM, Biju Das wrote: > Extended descriptor support in RX is available for R-Car where as it > is a normal descriptor for RZ/G2L. Factorise ravb_ring_free function > so that it can support later SoC. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/net/ethernet/renesas/ravb.h | 5 +++ > drivers/net/ethernet/renesas/ravb_main.c | 49 ++++++++++++++++-------- > 2 files changed, 37 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index a474ed68db22..3a9cf6e8671a 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -988,7 +988,12 @@ enum ravb_chip_id { > RCAR_GEN3, > }; > > +struct ravb_ops { > + void (*ring_free)(struct net_device *ndev, int q); Hmm, why not store it right in the *struct* ravb_drv_data? > +}; > + > struct ravb_drv_data { > + const struct ravb_ops *ravb_ops; > netdev_features_t net_features; > netdev_features_t net_hw_features; > const char (*gstrings_stats)[ETH_GSTRING_LEN]; > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 4ef2565534d2..a3b8b243fd54 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -247,30 +247,39 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only) > } > > /* Free skb's and DMA buffers for Ethernet AVB */ > -static void ravb_ring_free(struct net_device *ndev, int q) > +static void ravb_ring_free_rx(struct net_device *ndev, int q) How about ravb_rx_ring_free() instead? > { > struct ravb_private *priv = netdev_priv(ndev); > - int num_tx_desc = priv->num_tx_desc; > int ring_size; > int i; > > - if (priv->rx_ring[q]) { > - for (i = 0; i < priv->num_rx_ring[q]; i++) { > - struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i]; > + for (i = 0; i < priv->num_rx_ring[q]; i++) { > + struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i]; > > - if (!dma_mapping_error(ndev->dev.parent, > - le32_to_cpu(desc->dptr))) > - dma_unmap_single(ndev->dev.parent, > - le32_to_cpu(desc->dptr), > - RX_BUF_SZ, > - DMA_FROM_DEVICE); > - } > - ring_size = sizeof(struct ravb_ex_rx_desc) * > - (priv->num_rx_ring[q] + 1); > - dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q], > - priv->rx_desc_dma[q]); > - priv->rx_ring[q] = NULL; > + if (!dma_mapping_error(ndev->dev.parent, > + le32_to_cpu(desc->dptr))) > + dma_unmap_single(ndev->dev.parent, > + le32_to_cpu(desc->dptr), > + RX_BUF_SZ, > + DMA_FROM_DEVICE); > } > + ring_size = sizeof(struct ravb_ex_rx_desc) * > + (priv->num_rx_ring[q] + 1); > + dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q], > + priv->rx_desc_dma[q]); > + priv->rx_ring[q] = NULL; Couldn't this be moved into the new ravb_ring_free(), like the initial NULL check? > +} > + > +static void ravb_ring_free(struct net_device *ndev, int q) > +{ > + struct ravb_private *priv = netdev_priv(ndev); > + const struct ravb_drv_data *info = priv->info; > + int num_tx_desc = priv->num_tx_desc; > + int ring_size; > + int i; > + > + if (priv->rx_ring[q]) > + info->ravb_ops->ring_free(ndev, q); ... here? [...] MBR, Sergei
Hi Sergei, Thanks for the feedback. > Subject: Re: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free > function > > Hello! > > On 7/22/21 5:13 PM, Biju Das wrote: > > > Extended descriptor support in RX is available for R-Car where as it > > is a normal descriptor for RZ/G2L. Factorise ravb_ring_free function > > so that it can support later SoC. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/net/ethernet/renesas/ravb.h | 5 +++ > > drivers/net/ethernet/renesas/ravb_main.c | 49 > > ++++++++++++++++-------- > > 2 files changed, 37 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > b/drivers/net/ethernet/renesas/ravb.h > > index a474ed68db22..3a9cf6e8671a 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -988,7 +988,12 @@ enum ravb_chip_id { > > RCAR_GEN3, > > }; > > > > +struct ravb_ops { > > + void (*ring_free)(struct net_device *ndev, int q); > > Hmm, why not store it right in the *struct* ravb_drv_data? OK. > > > +}; > > + > > struct ravb_drv_data { > > + const struct ravb_ops *ravb_ops; > > netdev_features_t net_features; > > netdev_features_t net_hw_features; > > const char (*gstrings_stats)[ETH_GSTRING_LEN]; > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index 4ef2565534d2..a3b8b243fd54 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -247,30 +247,39 @@ static int ravb_tx_free(struct net_device *ndev, > > int q, bool free_txed_only) } > > > > /* Free skb's and DMA buffers for Ethernet AVB */ -static void > > ravb_ring_free(struct net_device *ndev, int q) > > +static void ravb_ring_free_rx(struct net_device *ndev, int q) > > How about ravb_rx_ring_free() instead? Agreed. > > > { > > struct ravb_private *priv = netdev_priv(ndev); > > - int num_tx_desc = priv->num_tx_desc; > > int ring_size; > > int i; > > > > - if (priv->rx_ring[q]) { > > - for (i = 0; i < priv->num_rx_ring[q]; i++) { > > - struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i]; > > + for (i = 0; i < priv->num_rx_ring[q]; i++) { > > + struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i]; > > > > - if (!dma_mapping_error(ndev->dev.parent, > > - le32_to_cpu(desc->dptr))) > > - dma_unmap_single(ndev->dev.parent, > > - le32_to_cpu(desc->dptr), > > - RX_BUF_SZ, > > - DMA_FROM_DEVICE); > > - } > > - ring_size = sizeof(struct ravb_ex_rx_desc) * > > - (priv->num_rx_ring[q] + 1); > > - dma_free_coherent(ndev->dev.parent, ring_size, priv- > >rx_ring[q], > > - priv->rx_desc_dma[q]); > > - priv->rx_ring[q] = NULL; > > + if (!dma_mapping_error(ndev->dev.parent, > > + le32_to_cpu(desc->dptr))) > > + dma_unmap_single(ndev->dev.parent, > > + le32_to_cpu(desc->dptr), > > + RX_BUF_SZ, > > + DMA_FROM_DEVICE); > > } > > + ring_size = sizeof(struct ravb_ex_rx_desc) * > > + (priv->num_rx_ring[q] + 1); > > + dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q], > > + priv->rx_desc_dma[q]); > > + priv->rx_ring[q] = NULL; > > Couldn't this be moved into the new ravb_ring_free(), like the initial > NULL check? For RZ/G2L, it is priv->rgeth_rx_ring, that is the reason. I can move the initial NULL check here, so the generic ravb_ring_free does not differentiate between priv->rx_ring and priv->rgeth_rx_ring. See below. > > > +} > > + > > +static void ravb_ring_free(struct net_device *ndev, int q) { > > + struct ravb_private *priv = netdev_priv(ndev); > > + const struct ravb_drv_data *info = priv->info; > > + int num_tx_desc = priv->num_tx_desc; > > + int ring_size; > > + int i; > > + > > + if (priv->rx_ring[q]) > > + info->ravb_ops->ring_free(ndev, q); > > ... here? It will be just info->ravb_ops->ring_free(ndev, q); And NULL check will be handled respective rx helper function. What do you think? Cheers, Biju > > [...] > > MBR, Sergei
Hi Sergei, > Subject: RE: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free > function > > Hi Sergei, > > Thanks for the feedback. > > > Subject: Re: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free > > function > > > > Hello! > > > > On 7/22/21 5:13 PM, Biju Das wrote: > > > > > Extended descriptor support in RX is available for R-Car where as it > > > is a normal descriptor for RZ/G2L. Factorise ravb_ring_free function > > > so that it can support later SoC. > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > drivers/net/ethernet/renesas/ravb.h | 5 +++ > > > drivers/net/ethernet/renesas/ravb_main.c | 49 > > > ++++++++++++++++-------- > > > 2 files changed, 37 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > > b/drivers/net/ethernet/renesas/ravb.h > > > index a474ed68db22..3a9cf6e8671a 100644 > > > --- a/drivers/net/ethernet/renesas/ravb.h > > > +++ b/drivers/net/ethernet/renesas/ravb.h > > > @@ -988,7 +988,12 @@ enum ravb_chip_id { > > > RCAR_GEN3, > > > }; > > > > > > +struct ravb_ops { > > > + void (*ring_free)(struct net_device *ndev, int q); > > > > Hmm, why not store it right in the *struct* ravb_drv_data? > > OK. > > > > > > +}; > > > + > > > struct ravb_drv_data { > > > + const struct ravb_ops *ravb_ops; > > > netdev_features_t net_features; > > > netdev_features_t net_hw_features; > > > const char (*gstrings_stats)[ETH_GSTRING_LEN]; > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > > b/drivers/net/ethernet/renesas/ravb_main.c > > > index 4ef2565534d2..a3b8b243fd54 100644 > > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > > @@ -247,30 +247,39 @@ static int ravb_tx_free(struct net_device > > > *ndev, int q, bool free_txed_only) } > > > > > > /* Free skb's and DMA buffers for Ethernet AVB */ -static void > > > ravb_ring_free(struct net_device *ndev, int q) > > > +static void ravb_ring_free_rx(struct net_device *ndev, int q) > > > > How about ravb_rx_ring_free() instead? > Agreed. > > > > > > { > > > struct ravb_private *priv = netdev_priv(ndev); > > > - int num_tx_desc = priv->num_tx_desc; > > > int ring_size; > > > int i; > > > > > > - if (priv->rx_ring[q]) { > > > - for (i = 0; i < priv->num_rx_ring[q]; i++) { > > > - struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i]; > > > + for (i = 0; i < priv->num_rx_ring[q]; i++) { > > > + struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i]; > > > > > > - if (!dma_mapping_error(ndev->dev.parent, > > > - le32_to_cpu(desc->dptr))) > > > - dma_unmap_single(ndev->dev.parent, > > > - le32_to_cpu(desc->dptr), > > > - RX_BUF_SZ, > > > - DMA_FROM_DEVICE); > > > - } > > > - ring_size = sizeof(struct ravb_ex_rx_desc) * > > > - (priv->num_rx_ring[q] + 1); > > > - dma_free_coherent(ndev->dev.parent, ring_size, priv- > > >rx_ring[q], > > > - priv->rx_desc_dma[q]); > > > - priv->rx_ring[q] = NULL; > > > + if (!dma_mapping_error(ndev->dev.parent, > > > + le32_to_cpu(desc->dptr))) > > > + dma_unmap_single(ndev->dev.parent, > > > + le32_to_cpu(desc->dptr), > > > + RX_BUF_SZ, > > > + DMA_FROM_DEVICE); > > > } > > > + ring_size = sizeof(struct ravb_ex_rx_desc) * > > > + (priv->num_rx_ring[q] + 1); > > > + dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q], > > > + priv->rx_desc_dma[q]); > > > + priv->rx_ring[q] = NULL; > > > > Couldn't this be moved into the new ravb_ring_free(), like the > > initial NULL check? > > For RZ/G2L, it is priv->rgeth_rx_ring, that is the reason. > > I can move the initial NULL check here, so the generic ravb_ring_free does > not differentiate between > priv->rx_ring and priv->rgeth_rx_ring. See below. > > > > > > +} > > > + > > > +static void ravb_ring_free(struct net_device *ndev, int q) { > > > + struct ravb_private *priv = netdev_priv(ndev); > > > + const struct ravb_drv_data *info = priv->info; > > > + int num_tx_desc = priv->num_tx_desc; > > > + int ring_size; > > > + int i; > > > + > > > + if (priv->rx_ring[q]) > > > + info->ravb_ops->ring_free(ndev, q); > > > > ... here? > > It will be just info->ravb_ops->ring_free(ndev, q); And NULL check > will be handled respective rx helper function. I will be sending next set of patches incorporating the above comments within couple of days. Regards, Biju
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index a474ed68db22..3a9cf6e8671a 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -988,7 +988,12 @@ enum ravb_chip_id { RCAR_GEN3, }; +struct ravb_ops { + void (*ring_free)(struct net_device *ndev, int q); +}; + struct ravb_drv_data { + const struct ravb_ops *ravb_ops; netdev_features_t net_features; netdev_features_t net_hw_features; const char (*gstrings_stats)[ETH_GSTRING_LEN]; diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 4ef2565534d2..a3b8b243fd54 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -247,30 +247,39 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only) } /* Free skb's and DMA buffers for Ethernet AVB */ -static void ravb_ring_free(struct net_device *ndev, int q) +static void ravb_ring_free_rx(struct net_device *ndev, int q) { struct ravb_private *priv = netdev_priv(ndev); - int num_tx_desc = priv->num_tx_desc; int ring_size; int i; - if (priv->rx_ring[q]) { - for (i = 0; i < priv->num_rx_ring[q]; i++) { - struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i]; + for (i = 0; i < priv->num_rx_ring[q]; i++) { + struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i]; - if (!dma_mapping_error(ndev->dev.parent, - le32_to_cpu(desc->dptr))) - dma_unmap_single(ndev->dev.parent, - le32_to_cpu(desc->dptr), - RX_BUF_SZ, - DMA_FROM_DEVICE); - } - ring_size = sizeof(struct ravb_ex_rx_desc) * - (priv->num_rx_ring[q] + 1); - dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q], - priv->rx_desc_dma[q]); - priv->rx_ring[q] = NULL; + if (!dma_mapping_error(ndev->dev.parent, + le32_to_cpu(desc->dptr))) + dma_unmap_single(ndev->dev.parent, + le32_to_cpu(desc->dptr), + RX_BUF_SZ, + DMA_FROM_DEVICE); } + ring_size = sizeof(struct ravb_ex_rx_desc) * + (priv->num_rx_ring[q] + 1); + dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q], + priv->rx_desc_dma[q]); + priv->rx_ring[q] = NULL; +} + +static void ravb_ring_free(struct net_device *ndev, int q) +{ + struct ravb_private *priv = netdev_priv(ndev); + const struct ravb_drv_data *info = priv->info; + int num_tx_desc = priv->num_tx_desc; + int ring_size; + int i; + + if (priv->rx_ring[q]) + info->ravb_ops->ring_free(ndev, q); if (priv->tx_ring[q]) { ravb_tx_free(ndev, q, false); @@ -1987,7 +1996,12 @@ static int ravb_mdio_release(struct ravb_private *priv) return 0; } +static const struct ravb_ops ravb_gen3_ops = { + .ring_free = ravb_ring_free_rx, +}; + static const struct ravb_drv_data ravb_gen3_data = { + .ravb_ops = &ravb_gen3_ops, .net_features = NETIF_F_RXCSUM, .net_hw_features = NETIF_F_RXCSUM, .gstrings_stats = ravb_gstrings_stats, @@ -2001,6 +2015,7 @@ static const struct ravb_drv_data ravb_gen3_data = { }; static const struct ravb_drv_data ravb_gen2_data = { + .ravb_ops = &ravb_gen3_ops, .net_features = NETIF_F_RXCSUM, .net_hw_features = NETIF_F_RXCSUM, .gstrings_stats = ravb_gstrings_stats,