| Message ID | 20210714145408.4382-1-biju.das.jz@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Add Gigabit Ethernet driver support | expand |
On Wed, Jul 14, 2021 at 03:54:07PM +0100, Biju Das wrote: > The DMAC and EMAC blocks of Gigabit Ethernet IP is almost > similar to Ethernet AVB. With few canges in driver we can changes > support both the IP. This patch is in preparation for > supporting the same. Please break this up a bit, it is hard to review. You can put all the refactoring into helpers into one patch. But changes like > - if (priv->chip_id == RCAR_GEN2) { > + if (priv->chip_id != RCAR_GEN3) { should be in a seperate patch with an explanation. You are aiming for lots of very simple patches which are obviously correct. > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index 86a1eb0634e8..80e62ca2e3d3 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -864,7 +864,7 @@ enum GECMR_BIT { > > /* The Ethernet AVB descriptor definitions. */ > struct ravb_desc { > - __le16 ds; /* Descriptor size */ > + __le16 ds; /* Descriptor size */ > u8 cc; /* Content control MSBs (reserved) */ > u8 die_dt; /* Descriptor interrupt enable and type */ > __le32 dptr; /* Descriptor pointer */ Please put white spaces changes in a patch of its own. Andrew
Hello Sergei Shtylyov, Thanks for the feedback. > Subject: Re: [PATCH/RFC 1/2] ravb: Preparation for supporting Gigabit > Ethernet driver > > Hello! > > On 7/14/21 5:54 PM, Biju Das wrote: > > > The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to > > Ethernet AVB. With few canges in driver we can support both the IP. > > This patch is in preparation for supporting the same. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > b/drivers/net/ethernet/renesas/ravb.h > > index 86a1eb0634e8..80e62ca2e3d3 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -864,7 +864,7 @@ enum GECMR_BIT { > > > > /* The Ethernet AVB descriptor definitions. */ struct ravb_desc { > > - __le16 ds; /* Descriptor size */ > > + __le16 ds; /* Descriptor size */ > > Oops! But this should be a matter of the seperate patch if you want to > fix whitespace... OK. Will send as separate patch. > > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index 4afff320dfd0..7e6feda59f4a 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -217,6 +217,29 @@ 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_ex(struct net_device *ndev, int q) > > What does _ex() suffix mean (seems rather poor chice)? Perhaps rx would > be better? The rationale behind suffix "_ex" is "extend" the existing function to support both AVB and Gigabit ethernet IP. > > > +{ > > + struct ravb_private *priv = netdev_priv(ndev); > > + int ring_size; > > + int 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; > > +} > > + > > static void ravb_ring_free(struct net_device *ndev, int q) { > > struct ravb_private *priv = netdev_priv(ndev); > [...] > > @@ -272,26 +281,15 @@ static void ravb_ring_free(struct net_device > > *ndev, int q) } > > > > /* Format skb and descriptor buffer for Ethernet AVB */ -static void > > ravb_ring_format(struct net_device *ndev, int q) > > +static void ravb_ring_format_ex(struct net_device *ndev, int q) > > Again, what the _ex suffix mean? > > [..] > > @@ -396,7 +414,7 @@ static int ravb_ring_init(struct net_device *ndev, > > int q) } > > > > /* E-MAC init function */ > > -static void ravb_emac_init(struct net_device *ndev) > > +static void ravb_emac_init_ex(struct net_device *ndev) > > Same question for the 3rd time... :-) > > [...] > > @@ -422,29 +440,15 @@ static void ravb_emac_init(struct net_device > *ndev) > > ravb_write(ndev, ECSIPR_ICDIP | ECSIPR_MPDIP | ECSIPR_LCHNGIP, > > ECSIPR); } > > > > +static void ravb_emac_init(struct net_device *ndev) { > > + ravb_emac_init_ex(ndev); > > Hm, looks pretty useless... Will make this as a separate single factorisation patch supporting both IP's. So that Reviewing will be easier. > > > +} > > + > > /* Device init function for Ethernet AVB */ -static int > > ravb_dmac_init(struct net_device *ndev) > > +static void ravb_dmac_init_ex(struct net_device *ndev) > > 4th _ex... > > [...] > > @@ -532,7 +561,7 @@ static void ravb_rx_csum(struct sk_buff *skb) } > > > > /* Packet receive function for Ethernet AVB */ -static bool > > ravb_rx(struct net_device *ndev, int *quota, int q) > > +static bool ravb_rx_ex(struct net_device *ndev, int *quota, int q) > > { > > struct ravb_private *priv = netdev_priv(ndev); > > int entry = priv->cur_rx[q] % priv->num_rx_ring[q]; @@ -647,6 > > +676,11 @@ static bool ravb_rx(struct net_device *ndev, int *quota, int > q) > > return boguscnt <= 0; > > } > > > > +static bool ravb_rx(struct net_device *ndev, int *quota, int q) { > > + return ravb_rx_ex(ndev, quota, q); > > +} > > + > > Looks pretty useless... OK. Will make this as a separate single factorisation patch supporting both IP's. So that Reviewing will be easier. > > [...] > > @@ -920,7 +954,7 @@ static int ravb_poll(struct napi_struct *napi, int > budget) > > if (ravb_rx(ndev, "a, q)) > > goto out; > > > > - /* Processing RX Descriptor Ring */ > > + /* Processing TX Descriptor Ring */ > > Hm, looka like a missing comment fix from the patch refactoring > ravb_poll()... OK. Will send this as separate patch. > > [...] > > @@ -2059,17 +2094,22 @@ static int ravb_probe(struct platform_device > *pdev) > > if (!ndev) > > return -ENOMEM; > > > > + /* The Ether-specific entries in the device structure. */ > > + ndev->base_addr = res->start; > > + > > + chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev); > > + > > + SET_NETDEV_DEV(ndev, &pdev->dev); > > + > > + priv = netdev_priv(ndev); > > + priv->chip_id = chip_id; > > + > > ndev->features = NETIF_F_RXCSUM; > > ndev->hw_features = NETIF_F_RXCSUM; > > > > pm_runtime_enable(&pdev->dev); > > pm_runtime_get_sync(&pdev->dev); > > > > - /* The Ether-specific entries in the device structure. */ > > - ndev->base_addr = res->start; > > - > > - chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev); > > - > > What does that hunk achieve? RZ/G2L ethernet needs to de-assert the reset(in case bootloader didn't dessert). In that case Clocks need to be turned on after that. > > > if (chip_id == RCAR_GEN3) > > irq = platform_get_irq_byname(pdev, "ch22"); > > else > [...] > > @@ -2257,7 +2292,7 @@ static int ravb_remove(struct platform_device > *pdev) > > struct ravb_private *priv = netdev_priv(ndev); > > > > /* Stop PTP Clock driver */ > > - if (priv->chip_id != RCAR_GEN2) > > + if (priv->chip_id == RCAR_GEN3) > > ravb_ptp_stop(ndev); > > > > clk_disable_unprepare(priv->refclk); > > @@ -2362,7 +2397,7 @@ static int __maybe_unused ravb_resume(struct > device *dev) > > /* Request GTI loading */ > > ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); > > > > - if (priv->chip_id != RCAR_GEN2) > > + if (priv->chip_id == RCAR_GEN3) > > ravb_set_delay_mode(ndev); > > Probably, all those chip_id check fixes deserve a patch of their own... OK. Agreed, since the delays are specifics to GEN3. Regards, Biju > > MBR, Sergei