| Message ID | 20210923140813.13541-7-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add Gigabit Ethernet driver support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 11 of 11 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 88 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On 9/23/21 5:08 PM, Biju Das wrote: > R-Car AVB-DMAC has 4 Transmit start Request queues, whereas > RZ/G2L has only 1 Transmit start Request queue(Best Effort) > > Add a multi_tsrq hw feature bit to struct ravb_hw_info to enable > this only for R-Car. This will allow us to add single TSRQ support for > RZ/G2L. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > drivers/net/ethernet/renesas/ravb.h | 1 + > drivers/net/ethernet/renesas/ravb_main.c | 12 ++++++++++-- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index bb92469d770e..c043ee555be4 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -1006,6 +1006,7 @@ struct ravb_hw_info { > unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has multiple irqs */ > unsigned no_gptp:1; /* AVB-DMAC does not support gPTP feature */ > unsigned ccc_gac:1; /* AVB-DMAC has gPTP support active in config mode */ > + unsigned multi_tsrq:1; /* AVB-DMAC has MULTI TSRQ */ Maybe 'single_tx_q' instead? > }; > > struct ravb_private { > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 8663d83507a0..d37d73f6d984 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -776,11 +776,17 @@ static void ravb_rcv_snd_enable(struct net_device *ndev) > /* function for waiting dma process finished */ > static int ravb_stop_dma(struct net_device *ndev) > { > + struct ravb_private *priv = netdev_priv(ndev); > + const struct ravb_hw_info *info = priv->info; > int error; > > /* Wait for stopping the hardware TX process */ > - error = ravb_wait(ndev, TCCR, > - TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0); > + if (info->multi_tsrq) > + error = ravb_wait(ndev, TCCR, > + TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0); > + else > + error = ravb_wait(ndev, TCCR, TCCR_TSRQ0, 0); Aren't the TSRQ1/2/3 bits reserved on RZ/G2L? If so, this new flag adds a little value, I think... unless you plan to use this flag further in the series? MBR, Sergei
Hi Sergei, Thanks for the feedback. > Subject: Re: [RFC/PATCH 06/18] ravb: Add multi_tsrq to struct ravb_hw_info > > On 9/23/21 5:08 PM, Biju Das wrote: > > > R-Car AVB-DMAC has 4 Transmit start Request queues, whereas RZ/G2L has > > only 1 Transmit start Request queue(Best Effort) > > > > Add a multi_tsrq hw feature bit to struct ravb_hw_info to enable this > > only for R-Car. This will allow us to add single TSRQ support for > > RZ/G2L. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > drivers/net/ethernet/renesas/ravb.h | 1 + > > drivers/net/ethernet/renesas/ravb_main.c | 12 ++++++++++-- > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > b/drivers/net/ethernet/renesas/ravb.h > > index bb92469d770e..c043ee555be4 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -1006,6 +1006,7 @@ struct ravb_hw_info { > > unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has multiple > irqs */ > > unsigned no_gptp:1; /* AVB-DMAC does not support gPTP > feature */ > > unsigned ccc_gac:1; /* AVB-DMAC has gPTP support active in > config mode */ > > + unsigned multi_tsrq:1; /* AVB-DMAC has MULTI TSRQ */ > > Maybe 'single_tx_q' instead? Since it is called transmit start request queue, it is better to be named as single_tsrq to match with hardware manual and I will update the comment with "GbEthernet DMAC has single TSRQ" Please let me know are you ok with it. Other wise I would like to use existing name. > > > }; > > > > struct ravb_private { > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index 8663d83507a0..d37d73f6d984 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -776,11 +776,17 @@ static void ravb_rcv_snd_enable(struct > > net_device *ndev) > > /* function for waiting dma process finished */ static int > > ravb_stop_dma(struct net_device *ndev) { > > + struct ravb_private *priv = netdev_priv(ndev); > > + const struct ravb_hw_info *info = priv->info; > > int error; > > > > /* Wait for stopping the hardware TX process */ > > - error = ravb_wait(ndev, TCCR, > > - TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0); > > + if (info->multi_tsrq) > > + error = ravb_wait(ndev, TCCR, > > + TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | > TCCR_TSRQ3, 0); > > + else > > + error = ravb_wait(ndev, TCCR, TCCR_TSRQ0, 0); > > Aren't the TSRQ1/2/3 bits reserved on RZ/G2L? If so, this new flag adds > a little value, I think... unless you plan to use this flag further in the > series? It will be confusing for RZ/G2L users. HW manual does not describes TSRQ1/2/3 and we are writing undocumented registers which is reserved. Tomorrow it can happen that this reserved bits(90% it will not happen) will be used for describing something else. It is unsafe to use reserved bits. Are you agreeing with this? Regards, Biju > > MBR, Sergei
Hi Sergei, > Subject: RE: [RFC/PATCH 06/18] ravb: Add multi_tsrq to struct ravb_hw_info > > Hi Sergei, > > Thanks for the feedback. > > > Subject: Re: [RFC/PATCH 06/18] ravb: Add multi_tsrq to struct > > ravb_hw_info > > > > On 9/23/21 5:08 PM, Biju Das wrote: > > > > > R-Car AVB-DMAC has 4 Transmit start Request queues, whereas RZ/G2L > > > has only 1 Transmit start Request queue(Best Effort) > > > > > > Add a multi_tsrq hw feature bit to struct ravb_hw_info to enable > > > this only for R-Car. This will allow us to add single TSRQ support > > > for RZ/G2L. > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- > > > drivers/net/ethernet/renesas/ravb.h | 1 + > > > drivers/net/ethernet/renesas/ravb_main.c | 12 ++++++++++-- > > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > > b/drivers/net/ethernet/renesas/ravb.h > > > index bb92469d770e..c043ee555be4 100644 > > > --- a/drivers/net/ethernet/renesas/ravb.h > > > +++ b/drivers/net/ethernet/renesas/ravb.h > > > @@ -1006,6 +1006,7 @@ struct ravb_hw_info { > > > unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has multiple > > irqs */ > > > unsigned no_gptp:1; /* AVB-DMAC does not support gPTP > > feature */ > > > unsigned ccc_gac:1; /* AVB-DMAC has gPTP support active in > > config mode */ > > > + unsigned multi_tsrq:1; /* AVB-DMAC has MULTI TSRQ */ > > > > Maybe 'single_tx_q' instead? > > Since it is called transmit start request queue, it is better to be named > as single_tsrq to match with hardware manual and I will update the comment > with "GbEthernet DMAC has single TSRQ" > Please let me know are you ok with it. Other wise I would like to use > existing name. On the next revision as you proposed for [1], I will use a u32 tsrq, instead of bit, there by we can avoid a check. https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210923140813.13541-12-biju.das.jz@bp.renesas.com/ > > > > > > }; > > > > > > struct ravb_private { > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > > b/drivers/net/ethernet/renesas/ravb_main.c > > > index 8663d83507a0..d37d73f6d984 100644 > > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > > @@ -776,11 +776,17 @@ static void ravb_rcv_snd_enable(struct > > > net_device *ndev) > > > /* function for waiting dma process finished */ static int > > > ravb_stop_dma(struct net_device *ndev) { > > > + struct ravb_private *priv = netdev_priv(ndev); > > > + const struct ravb_hw_info *info = priv->info; > > > int error; > > > > > > /* Wait for stopping the hardware TX process */ > > > - error = ravb_wait(ndev, TCCR, > > > - TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0); > > > + if (info->multi_tsrq) > > > + error = ravb_wait(ndev, TCCR, > > > + TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | > > TCCR_TSRQ3, 0); > > > + else > > > + error = ravb_wait(ndev, TCCR, TCCR_TSRQ0, 0); > > > > Aren't the TSRQ1/2/3 bits reserved on RZ/G2L? If so, this new flag > > adds a little value, I think... unless you plan to use this flag > > further in the series? > > It will be confusing for RZ/G2L users. HW manual does not describes > TSRQ1/2/3 and we are writing undocumented registers which is reserved. > > Tomorrow it can happen that this reserved bits(90% it will not happen) > will be used for describing something else. > > It is unsafe to use reserved bits. Are you agreeing with this? As per the above discussion, we can replace the above check as you proposed for [1] error = ravb_wait(ndev, TCCR, info->tsrq, 0); [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210923140813.13541-12-biju.das.jz@bp.renesas.com/ regards, Biju
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index bb92469d770e..c043ee555be4 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -1006,6 +1006,7 @@ struct ravb_hw_info { unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has multiple irqs */ unsigned no_gptp:1; /* AVB-DMAC does not support gPTP feature */ unsigned ccc_gac:1; /* AVB-DMAC has gPTP support active in config mode */ + unsigned multi_tsrq:1; /* AVB-DMAC has MULTI TSRQ */ }; struct ravb_private { diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 8663d83507a0..d37d73f6d984 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -776,11 +776,17 @@ static void ravb_rcv_snd_enable(struct net_device *ndev) /* function for waiting dma process finished */ static int ravb_stop_dma(struct net_device *ndev) { + struct ravb_private *priv = netdev_priv(ndev); + const struct ravb_hw_info *info = priv->info; int error; /* Wait for stopping the hardware TX process */ - error = ravb_wait(ndev, TCCR, - TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0); + if (info->multi_tsrq) + error = ravb_wait(ndev, TCCR, + TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0); + else + error = ravb_wait(ndev, TCCR, TCCR_TSRQ0, 0); + if (error) return error; @@ -2088,6 +2094,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = { .tx_counters = 1, .multi_irqs = 1, .ccc_gac = 1, + .multi_tsrq = 1, }; static const struct ravb_hw_info ravb_gen2_hw_info = { @@ -2106,6 +2113,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = { .stats_len = ARRAY_SIZE(ravb_gstrings_stats), .max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1, .aligned_tx = 1, + .multi_tsrq = 1, }; static const struct ravb_hw_info rgeth_hw_info = {
R-Car AVB-DMAC has 4 Transmit start Request queues, whereas RZ/G2L has only 1 Transmit start Request queue(Best Effort) Add a multi_tsrq hw feature bit to struct ravb_hw_info to enable this only for R-Car. This will allow us to add single TSRQ support for RZ/G2L. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/net/ethernet/renesas/ravb.h | 1 + drivers/net/ethernet/renesas/ravb_main.c | 12 ++++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-)