| Message ID | 20210825070154.14336-1-biju.das.jz@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Add Factorisation code to support Gigabit Ethernet driver | expand |
Hello: This series was applied to netdev/net-next.git (refs/heads/master): On Wed, 25 Aug 2021 08:01:41 +0100 you wrote: > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC are > similar to the R-Car Ethernet AVB IP. > > The Gigabit Ethernet IP consists of Ethernet controller (E-MAC), Internal > TCP/IP Offload Engine (TOE) and Dedicated Direct memory access controller > (DMAC). > > [...] Here is the summary with links: - [net-next,01/13] ravb: Remove the macros NUM_TX_DESC_GEN[23] https://git.kernel.org/netdev/net-next/c/c81d894226b9 - [net-next,02/13] ravb: Add multi_irq to struct ravb_hw_info https://git.kernel.org/netdev/net-next/c/6de19fa0e9f7 - [net-next,03/13] ravb: Add no_ptp_cfg_active to struct ravb_hw_info https://git.kernel.org/netdev/net-next/c/8f27219a6191 - [net-next,04/13] ravb: Add ptp_cfg_active to struct ravb_hw_info https://git.kernel.org/netdev/net-next/c/a69a3d094de3 - [net-next,05/13] ravb: Factorise ravb_ring_free function https://git.kernel.org/netdev/net-next/c/bf46b7578404 - [net-next,06/13] ravb: Factorise ravb_ring_format function https://git.kernel.org/netdev/net-next/c/1ae22c19e75c - [net-next,07/13] ravb: Factorise ravb_ring_init function https://git.kernel.org/netdev/net-next/c/7870a41848ab - [net-next,08/13] ravb: Factorise ravb_rx function https://git.kernel.org/netdev/net-next/c/d5d95c11365b - [net-next,09/13] ravb: Factorise ravb_adjust_link function https://git.kernel.org/netdev/net-next/c/cb21104f2c35 - [net-next,10/13] ravb: Factorise ravb_set_features https://git.kernel.org/netdev/net-next/c/80f35a0df086 - [net-next,11/13] ravb: Factorise ravb_dmac_init function https://git.kernel.org/netdev/net-next/c/eb4fd127448b - [net-next,12/13] ravb: Factorise ravb_emac_init function https://git.kernel.org/netdev/net-next/c/511d74d9d86c - [net-next,13/13] ravb: Add reset support https://git.kernel.org/netdev/net-next/c/0d13a1a464a0 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Hello! On 25.08.2021 13:30, patchwork-bot+netdevbpf@kernel.org wrote: > This series was applied to netdev/net-next.git (refs/heads/master): > > On Wed, 25 Aug 2021 08:01:41 +0100 you wrote: Now this is super fast -- I didn't even have the time to promise reviewing... :-/ >> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC are >> similar to the R-Car Ethernet AVB IP. >> >> The Gigabit Ethernet IP consists of Ethernet controller (E-MAC), Internal >> TCP/IP Offload Engine (TOE) and Dedicated Direct memory access controller >> (DMAC). >> >> [...] > > Here is the summary with links: > - [net-next,01/13] ravb: Remove the macros NUM_TX_DESC_GEN[23] > https://git.kernel.org/netdev/net-next/c/c81d894226b9 > - [net-next,02/13] ravb: Add multi_irq to struct ravb_hw_info > https://git.kernel.org/netdev/net-next/c/6de19fa0e9f7 > - [net-next,03/13] ravb: Add no_ptp_cfg_active to struct ravb_hw_info > https://git.kernel.org/netdev/net-next/c/8f27219a6191 > - [net-next,04/13] ravb: Add ptp_cfg_active to struct ravb_hw_info > https://git.kernel.org/netdev/net-next/c/a69a3d094de3 > - [net-next,05/13] ravb: Factorise ravb_ring_free function > https://git.kernel.org/netdev/net-next/c/bf46b7578404 > - [net-next,06/13] ravb: Factorise ravb_ring_format function > https://git.kernel.org/netdev/net-next/c/1ae22c19e75c > - [net-next,07/13] ravb: Factorise ravb_ring_init function > https://git.kernel.org/netdev/net-next/c/7870a41848ab > - [net-next,08/13] ravb: Factorise ravb_rx function > https://git.kernel.org/netdev/net-next/c/d5d95c11365b > - [net-next,09/13] ravb: Factorise ravb_adjust_link function > https://git.kernel.org/netdev/net-next/c/cb21104f2c35 > - [net-next,10/13] ravb: Factorise ravb_set_features > https://git.kernel.org/netdev/net-next/c/80f35a0df086 > - [net-next,11/13] ravb: Factorise ravb_dmac_init function > https://git.kernel.org/netdev/net-next/c/eb4fd127448b > - [net-next,12/13] ravb: Factorise ravb_emac_init function > https://git.kernel.org/netdev/net-next/c/511d74d9d86c > - [net-next,13/13] ravb: Add reset support > https://git.kernel.org/netdev/net-next/c/0d13a1a464a0 Will have to do a post-merge review again. And I expect more issues here than in a previous patch set... > You are awesome, thank you! MBR, Sergey
On Wed, Aug 25, 2021 at 01:57:55PM +0300, Sergey Shtylyov wrote: > Hello! > > On 25.08.2021 13:30, patchwork-bot+netdevbpf@kernel.org wrote: > > > This series was applied to netdev/net-next.git (refs/heads/master): > > > > On Wed, 25 Aug 2021 08:01:41 +0100 you wrote: > Now this is super fast -- I didn't even have the time to promise > reviewing... :-/ 2 hours 30 minutes, i think. Seems like reviews are no longer wanted in netdev. Andrew
On 8/25/21 10:01 AM, Biju Das wrote: > There are some H/W differences for the gPTP feature between > R-Car Gen3, R-Car Gen2, and RZ/G2L as below. > > 1) On R-Car Gen2, gPTP support is not active in config mode. > 2) On R-Car Gen3, gPTP support is active in config mode. > 3) RZ/G2L does not support the gPTP feature. > > Add a no_ptp_cfg_active hw feature bit to struct ravb_hw_info for > handling gPTP for R-Car Gen2. > > 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 | 1 + > drivers/net/ethernet/renesas/ravb_main.c | 20 ++++++++++++-------- > 2 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index da486e06b322..9ecf1a8c3ca8 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -998,6 +998,7 @@ struct ravb_hw_info { > unsigned internal_delay:1; /* AVB-DMAC has internal delays */ > unsigned tx_counters:1; /* E-MAC has TX counters */ > unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has multiple irqs */ > + unsigned no_ptp_cfg_active:1; /* AVB-DMAC does not support gPTP active in config mode */ Isn't this better to name it 'ptp_active_cfg' -- positive, instead of negative? [...] MBR, Sergey
On 8/25/21 10:01 AM, Biju Das wrote: > The ravb_ring_format function uses an extended descriptor in RX > for R-Car compared to the normal descriptor for RZ/G2L. Factorise > RX ring buffer buildup to extend the support for later SoC. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index dc388a32496a..e52e36ccd1c6 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c [...] > @@ -321,6 +310,26 @@ static void ravb_ring_format(struct net_device *ndev, int q) > rx_desc = &priv->rx_ring[q][i]; > rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]); > rx_desc->die_dt = DT_LINKFIX; /* type */ > +} > + > +/* Format skb and descriptor buffer for Ethernet AVB */ > +static void ravb_ring_format(struct net_device *ndev, int q) > +{ > + struct ravb_private *priv = netdev_priv(ndev); > + const struct ravb_hw_info *info = priv->info; > + unsigned int num_tx_desc = priv->num_tx_desc; > + struct ravb_tx_desc *tx_desc; > + struct ravb_desc *desc; > + unsigned int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] * > + num_tx_desc; > + unsigned int i; > + > + priv->cur_rx[q] = 0; > + priv->cur_tx[q] = 0; > + priv->dirty_rx[q] = 0; > + priv->dirty_tx[q] = 0; > + > + info->rx_ring_format(ndev, q); > > memset(priv->tx_ring[q], 0, tx_ring_size); > /* Build TX ring buffer */ That's all fine but the fragment that sets up TX descriptor ring base address was left in ravb_rx_ring_formet()... [...] MBR, Sergey
Hi Sergei, > Subject: Re: [PATCH net-next 06/13] ravb: Factorise ravb_ring_format > function > > On 8/25/21 10:01 AM, Biju Das wrote: > > > The ravb_ring_format function uses an extended descriptor in RX for > > R-Car compared to the normal descriptor for RZ/G2L. Factorise RX ring > > buffer buildup to extend the support for later SoC. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index dc388a32496a..e52e36ccd1c6 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > [...] > > @@ -321,6 +310,26 @@ static void ravb_ring_format(struct net_device > *ndev, int q) > > rx_desc = &priv->rx_ring[q][i]; > > rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]); > > rx_desc->die_dt = DT_LINKFIX; /* type */ > > +} > > + > > +/* Format skb and descriptor buffer for Ethernet AVB */ static void > > +ravb_ring_format(struct net_device *ndev, int q) { > > + struct ravb_private *priv = netdev_priv(ndev); > > + const struct ravb_hw_info *info = priv->info; > > + unsigned int num_tx_desc = priv->num_tx_desc; > > + struct ravb_tx_desc *tx_desc; > > + struct ravb_desc *desc; > > + unsigned int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] > * > > + num_tx_desc; > > + unsigned int i; > > + > > + priv->cur_rx[q] = 0; > > + priv->cur_tx[q] = 0; > > + priv->dirty_rx[q] = 0; > > + priv->dirty_tx[q] = 0; > > + > > + info->rx_ring_format(ndev, q); > > > > memset(priv->tx_ring[q], 0, tx_ring_size); > > /* Build TX ring buffer */ > > That's all fine but the fragment that sets up TX descriptor ring base > address was left in ravb_rx_ring_formet()... Can you please clarify this? Which fragment in [1]? Do you see any problems with that? [1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/renesas/ravb_main.c#n286 Regards, Biju Regards, Biju
On 8/25/21 10:01 AM, Biju Das wrote: > The ravb_ring_init function uses an extended descriptor in RX for > R-Car and normal descriptor for RZ/G2L. Add a helper function > for RX ring buffer allocation to 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> Here as well... [...] Seems sane, so: Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> MBR, Sergey
On 8/25/21 10:01 AM, Biju Das wrote: > The DMAC IP on the R-Car AVB module has different initialization > parameters for RCR, TGC, TCCR, RIC0, RIC2, and TIC compared to > DMAC IP on the RZ/G2L Gigabit Ethernet module. Factorise the > ravb_dmac_init function to support the later SoC. Couldn't we resolve these differencies like the sh_eth driver does, by adding the register values into the *struct* ravb_hw_info? > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> [...] MBR, Sergey
On 8/25/21 10:01 AM, Biju Das wrote: > Reset support is present on R-Car. Let's support it, if it is > available. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 7a144b45e41d..0f85f2d97b18 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c [...] > @@ -2349,6 +2358,7 @@ static int ravb_probe(struct platform_device *pdev) > > pm_runtime_put(&pdev->dev); > pm_runtime_disable(&pdev->dev); > + reset_control_assert(rstc); > return error; > } > > @@ -2374,6 +2384,7 @@ static int ravb_remove(struct platform_device *pdev) > netif_napi_del(&priv->napi[RAVB_BE]); > ravb_mdio_release(priv); > pm_runtime_disable(&pdev->dev); > + reset_control_assert(priv->rstc); > free_netdev(ndev); > platform_set_drvdata(pdev, NULL); > Is it possible to get into/out of reset in open()/close() methods? Otherwise, looks good (I'm not much into reset h/w) Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> MBR, Sergey
Hi Sergei, Thanks for the feedback. > Subject: Re: [PATCH net-next 11/13] ravb: Factorise ravb_dmac_init > function > > On 8/25/21 10:01 AM, Biju Das wrote: > > > The DMAC IP on the R-Car AVB module has different initialization > > parameters for RCR, TGC, TCCR, RIC0, RIC2, and TIC compared to DMAC IP > > on the RZ/G2L Gigabit Ethernet module. Factorise the ravb_dmac_init > > function to support the later SoC. > > Couldn't we resolve these differencies like the sh_eth driver does, by > adding the register values into the *struct* ravb_hw_info? I will evaluate your proposal in terms of code size and data size And with the current code and share the details in next RFC patchset for supporting RZ/G2L with dmac_init function. Based on the RFC discussion, we can conclude it. Currently by looking at your proposal, I am seeing duplication of Data in R-Car Gen3 and R-Car Gen2. If statement for adding RIC3 register for RZ/G2L, which involves Exposing another hwinfo bit. Regards, Biju > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > [...] > > MBR, Sergey
Hi Sergei, Thanks for the feedback. > Subject: Re: [PATCH net-next 13/13] ravb: Add reset support > > On 8/25/21 10:01 AM, Biju Das wrote: > > > Reset support is present on R-Car. Let's support it, if it is > > available. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index 7a144b45e41d..0f85f2d97b18 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > [...] > > > @@ -2349,6 +2358,7 @@ static int ravb_probe(struct platform_device > > *pdev) > > > > pm_runtime_put(&pdev->dev); > > pm_runtime_disable(&pdev->dev); > > + reset_control_assert(rstc); > > return error; > > } > > > > @@ -2374,6 +2384,7 @@ static int ravb_remove(struct platform_device > *pdev) > > netif_napi_del(&priv->napi[RAVB_BE]); > > ravb_mdio_release(priv); > > pm_runtime_disable(&pdev->dev); > > + reset_control_assert(priv->rstc); > > free_netdev(ndev); > > platform_set_drvdata(pdev, NULL); > > > > Is it possible to get into/out of reset in open()/close() methods? No, Reason, Normally reset will be called ravb_mdio_release(priv); pm_runtime_disable(&pdev->dev); reset_control_assert(priv->rstc); After reset assert, We should not access any RAVB registers, otherwise system will hang. There is a high chance that other users(for eg:- mdio) may access ravb registers and system hangs. Regards, Biju > Otherwise, looks good (I'm not much into reset h/w) > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > MBR, Sergey