| Message ID | 20210802102654.5996-8-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| 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 | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: yangyingliang@huawei.com |
| 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 | success | total: 0 errors, 0 warnings, 0 checks, 39 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Mon, Aug 02, 2021 at 11:26:53AM +0100, Biju Das wrote: > R-Car Gen3 supports TX and RX clock internal delay modes, whereas R-Car > Gen2 and RZ/G2L do not support it. > Add an internal_delay hw feature bit to struct ravb_hw_info to enable this > only for R-Car Gen3. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 8/2/21 1:26 PM, Biju Das wrote: > R-Car Gen3 supports TX and RX clock internal delay modes, whereas R-Car > Gen2 and RZ/G2L do not support it. > Add an internal_delay hw feature bit to struct ravb_hw_info to enable this > only for R-Car Gen3. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> [...] OK, this one also seems uncontroversial: Reviewed-by: Sergei Shtylyov <sergei.shtylyov@gmail.com> MBR, Sergei
On 8/2/21 1:26 PM, Biju Das wrote: > R-Car Gen3 supports TX and RX clock internal delay modes, whereas R-Car > Gen2 and RZ/G2L do not support it. > Add an internal_delay hw feature bit to struct ravb_hw_info to enable this > only for R-Car Gen3. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > v2: > * Incorporated Andrew and Sergei's review comments for making it smaller patch > and provided detailed description. > --- > drivers/net/ethernet/renesas/ravb.h | 3 +++ > drivers/net/ethernet/renesas/ravb_main.c | 6 ++++-- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index 3df813b2e253..0d640dbe1eed 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -998,6 +998,9 @@ struct ravb_hw_info { > int num_tx_desc; > int stats_len; > size_t skb_sz; > + > + /* hardware features */ > + unsigned internal_delay:1; /* RAVB has internal delays */ Oops, missed it initially: RAVB? That's not a device name, according to the manuals. It seems to be the driver's name. I'd drop this comment... [...] MBR, Sergei
Hi Sergei, Thanks for the feedback > Subject: Re: [PATCH net-next v2 7/8] ravb: Add internal delay hw feature > to struct ravb_hw_info > > On 8/2/21 1:26 PM, Biju Das wrote: > > > R-Car Gen3 supports TX and RX clock internal delay modes, whereas > > R-Car > > Gen2 and RZ/G2L do not support it. > > Add an internal_delay hw feature bit to struct ravb_hw_info to enable > > this only for R-Car Gen3. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > v2: > > * Incorporated Andrew and Sergei's review comments for making it > smaller patch > > and provided detailed description. > > --- > > drivers/net/ethernet/renesas/ravb.h | 3 +++ > > drivers/net/ethernet/renesas/ravb_main.c | 6 ++++-- > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > b/drivers/net/ethernet/renesas/ravb.h > > index 3df813b2e253..0d640dbe1eed 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -998,6 +998,9 @@ struct ravb_hw_info { > > int num_tx_desc; > > int stats_len; > > size_t skb_sz; > > + > > + /* hardware features */ > > + unsigned internal_delay:1; /* RAVB has internal delays */ > > Oops, missed it initially: > RAVB? That's not a device name, according to the manuals. It seems to > be the driver's name. OK. will change it to AVB-DMAC has internal delays. Cheers, Biju
Hi Sergei, Thanks for the feedback. > Subject: Re: [PATCH net-next v2 7/8] ravb: Add internal delay hw feature > to struct ravb_hw_info > > On 8/2/21 1:26 PM, Biju Das wrote: > > > R-Car Gen3 supports TX and RX clock internal delay modes, whereas > > R-Car > > Gen2 and RZ/G2L do not support it. > > Add an internal_delay hw feature bit to struct ravb_hw_info to enable > > this only for R-Car Gen3. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > [...] > > OK, this one also seems uncontroversial: So far the comments I received 1) I have replaced NUM_TX_DESC to num_tx_desc. But you are recommending to rename it, is ravb_num_tx_desc good choice? 2) skb_sz to max_rx_len, I am ok for it, if there is no objection from others. 3) patches related to device stats. I already provided the output of ethtool -S eth0 for both R-Car and RZ/G2L. For RZ/G2L there is an "rx_queue_0_csum_offload_errors: 0", instead of "rx_queue_0_missed_errors: 0", Both uses MSC bit 6 for collecting this info. To provide correct output to the user using command "ethtool -S eth0", RZ/G2L need to have a different string LUT. Q1) Do you agree with this? Cheers, Biju > > Reviewed-by: Sergei Shtylyov <sergei.shtylyov@gmail.com> > > MBR, Sergei
On 04.08.2021 8:13, Biju Das wrote: > Hi Sergei, > > Thanks for the feedback > >> Subject: Re: [PATCH net-next v2 7/8] ravb: Add internal delay hw feature >> to struct ravb_hw_info >> >> On 8/2/21 1:26 PM, Biju Das wrote: >> >>> R-Car Gen3 supports TX and RX clock internal delay modes, whereas >>> R-Car >>> Gen2 and RZ/G2L do not support it. >>> Add an internal_delay hw feature bit to struct ravb_hw_info to enable >>> this only for R-Car Gen3. >>> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>> --- >>> v2: >>> * Incorporated Andrew and Sergei's review comments for making it >> smaller patch >>> and provided detailed description. >>> --- >>> drivers/net/ethernet/renesas/ravb.h | 3 +++ >>> drivers/net/ethernet/renesas/ravb_main.c | 6 ++++-- >>> 2 files changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/renesas/ravb.h >>> b/drivers/net/ethernet/renesas/ravb.h >>> index 3df813b2e253..0d640dbe1eed 100644 >>> --- a/drivers/net/ethernet/renesas/ravb.h >>> +++ b/drivers/net/ethernet/renesas/ravb.h >>> @@ -998,6 +998,9 @@ struct ravb_hw_info { >>> int num_tx_desc; >>> int stats_len; >>> size_t skb_sz; >>> + >>> + /* hardware features */ >>> + unsigned internal_delay:1; /* RAVB has internal delays */ >> >> Oops, missed it initially: >> RAVB? That's not a device name, according to the manuals. It seems to >> be the driver's name. > > OK. will change it to AVB-DMAC has internal delays. Please don't -- E-MAC has them, not AVB-DMAC. > Cheers, > Biju NBR, Sergei
Hi Sergei, Thanks for feedback > Subject: Re: [PATCH net-next v2 7/8] ravb: Add internal delay hw feature > to struct ravb_hw_info > > On 04.08.2021 8:13, Biju Das wrote: > > Hi Sergei, > > > > Thanks for the feedback > > > >> Subject: Re: [PATCH net-next v2 7/8] ravb: Add internal delay hw > >> feature to struct ravb_hw_info > >> > >> On 8/2/21 1:26 PM, Biju Das wrote: > >> > >>> R-Car Gen3 supports TX and RX clock internal delay modes, whereas > >>> R-Car > >>> Gen2 and RZ/G2L do not support it. > >>> Add an internal_delay hw feature bit to struct ravb_hw_info to > >>> enable this only for R-Car Gen3. > >>> > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>> --- > >>> v2: > >>> * Incorporated Andrew and Sergei's review comments for making it > >> smaller patch > >>> and provided detailed description. > >>> --- > >>> drivers/net/ethernet/renesas/ravb.h | 3 +++ > >>> drivers/net/ethernet/renesas/ravb_main.c | 6 ++++-- > >>> 2 files changed, 7 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/net/ethernet/renesas/ravb.h > >>> b/drivers/net/ethernet/renesas/ravb.h > >>> index 3df813b2e253..0d640dbe1eed 100644 > >>> --- a/drivers/net/ethernet/renesas/ravb.h > >>> +++ b/drivers/net/ethernet/renesas/ravb.h > >>> @@ -998,6 +998,9 @@ struct ravb_hw_info { > >>> int num_tx_desc; > >>> int stats_len; > >>> size_t skb_sz; > >>> + > >>> + /* hardware features */ > >>> + unsigned internal_delay:1; /* RAVB has internal delays */ > >> > >> Oops, missed it initially: > >> RAVB? That's not a device name, according to the manuals. It > >> seems to be the driver's name. > > > > OK. will change it to AVB-DMAC has internal delays. > > Please don't -- E-MAC has them, not AVB-DMAC. By looking at HW manual for R-Car AVB-DMAC (APSR register, offset:-0x08C) has TDM and RDM registers for Setting internal delay mode which can give TX clock delay up to 2.0ns and RX Clock delay 2.8ns. Please correct me, if this is not the case. Regards, Biju > > > Cheers, > > Biju > > NBR, Sergei
On 04.08.2021 8:13, Biju Das wrote: [...] >>> R-Car Gen3 supports TX and RX clock internal delay modes, whereas >>> R-Car >>> Gen2 and RZ/G2L do not support it. >>> Add an internal_delay hw feature bit to struct ravb_hw_info to enable >>> this only for R-Car Gen3. >>> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>> --- >>> v2: >>> * Incorporated Andrew and Sergei's review comments for making it >> smaller patch >>> and provided detailed description. >>> --- >>> drivers/net/ethernet/renesas/ravb.h | 3 +++ >>> drivers/net/ethernet/renesas/ravb_main.c | 6 ++++-- >>> 2 files changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/renesas/ravb.h >>> b/drivers/net/ethernet/renesas/ravb.h >>> index 3df813b2e253..0d640dbe1eed 100644 >>> --- a/drivers/net/ethernet/renesas/ravb.h >>> +++ b/drivers/net/ethernet/renesas/ravb.h >>> @@ -998,6 +998,9 @@ struct ravb_hw_info { >>> int num_tx_desc; >>> int stats_len; >>> size_t skb_sz; >>> + >>> + /* hardware features */ >>> + unsigned internal_delay:1; /* RAVB has internal delays */ >> >> Oops, missed it initially: >> RAVB? That's not a device name, according to the manuals. It seems to >> be the driver's name. > > OK. will change it to AVB-DMAC has internal delays. Please don't -- E-MAC has them, not AVB-DMAC. > Cheers, > Biju MBR, Sergei
Hi Sergei, > Subject: Re: [PATCH net-next v2 7/8] ravb: Add internal delay hw feature > to struct ravb_hw_info > > On 04.08.2021 8:13, Biju Das wrote: > > [...] > >>> R-Car Gen3 supports TX and RX clock internal delay modes, whereas > >>> R-Car > >>> Gen2 and RZ/G2L do not support it. > >>> Add an internal_delay hw feature bit to struct ravb_hw_info to > >>> enable this only for R-Car Gen3. > >>> > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>> --- > >>> v2: > >>> * Incorporated Andrew and Sergei's review comments for making it > >> smaller patch > >>> and provided detailed description. > >>> --- > >>> drivers/net/ethernet/renesas/ravb.h | 3 +++ > >>> drivers/net/ethernet/renesas/ravb_main.c | 6 ++++-- > >>> 2 files changed, 7 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/net/ethernet/renesas/ravb.h > >>> b/drivers/net/ethernet/renesas/ravb.h > >>> index 3df813b2e253..0d640dbe1eed 100644 > >>> --- a/drivers/net/ethernet/renesas/ravb.h > >>> +++ b/drivers/net/ethernet/renesas/ravb.h > >>> @@ -998,6 +998,9 @@ struct ravb_hw_info { > >>> int num_tx_desc; > >>> int stats_len; > >>> size_t skb_sz; > >>> + > >>> + /* hardware features */ > >>> + unsigned internal_delay:1; /* RAVB has internal delays */ > >> > >> Oops, missed it initially: > >> RAVB? That's not a device name, according to the manuals. It > >> seems to be the driver's name. > > > > OK. will change it to AVB-DMAC has internal delays. > > Please don't -- E-MAC has them, not AVB-DMAC. Since the register for setting internal delay mode is coming from product specific register(0x8c), I am agreeing with your statement, "E-MAC has internal delays". Cheers, Biju
On 04.08.2021 13:08, Biju Das wrote: > Hi Sergei, > > Thanks for feedback > >> Subject: Re: [PATCH net-next v2 7/8] ravb: Add internal delay hw feature >> to struct ravb_hw_info >> >> On 04.08.2021 8:13, Biju Das wrote: >>> Hi Sergei, >>> >>> Thanks for the feedback >>> >>>> Subject: Re: [PATCH net-next v2 7/8] ravb: Add internal delay hw >>>> feature to struct ravb_hw_info >>>> >>>> On 8/2/21 1:26 PM, Biju Das wrote: >>>> >>>>> R-Car Gen3 supports TX and RX clock internal delay modes, whereas >>>>> R-Car >>>>> Gen2 and RZ/G2L do not support it. >>>>> Add an internal_delay hw feature bit to struct ravb_hw_info to >>>>> enable this only for R-Car Gen3. >>>>> >>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>>>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>>>> --- >>>>> v2: >>>>> * Incorporated Andrew and Sergei's review comments for making it >>>> smaller patch >>>>> and provided detailed description. >>>>> --- >>>>> drivers/net/ethernet/renesas/ravb.h | 3 +++ >>>>> drivers/net/ethernet/renesas/ravb_main.c | 6 ++++-- >>>>> 2 files changed, 7 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/net/ethernet/renesas/ravb.h >>>>> b/drivers/net/ethernet/renesas/ravb.h >>>>> index 3df813b2e253..0d640dbe1eed 100644 >>>>> --- a/drivers/net/ethernet/renesas/ravb.h >>>>> +++ b/drivers/net/ethernet/renesas/ravb.h >>>>> @@ -998,6 +998,9 @@ struct ravb_hw_info { >>>>> int num_tx_desc; >>>>> int stats_len; >>>>> size_t skb_sz; >>>>> + >>>>> + /* hardware features */ >>>>> + unsigned internal_delay:1; /* RAVB has internal delays */ >>>> >>>> Oops, missed it initially: >>>> RAVB? That's not a device name, according to the manuals. It >>>> seems to be the driver's name. >>> >>> OK. will change it to AVB-DMAC has internal delays. >> >> Please don't -- E-MAC has them, not AVB-DMAC. > > By looking at HW manual for R-Car AVB-DMAC (APSR register, offset:-0x08C) has TDM and RDM registers for Setting internal delay mode which can give TX clock delay up to 2.0ns and RX Clock delay 2.8ns. > > Please correct me, if this is not the case. You're correct indeed -- though being counter-intuitive, APSR belongs to the AVB-DMAC block. Sorry about that. :-/ > Regards, > Biju NBR, Sergei
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index 3df813b2e253..0d640dbe1eed 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -998,6 +998,9 @@ struct ravb_hw_info { int num_tx_desc; int stats_len; size_t skb_sz; + + /* hardware features */ + unsigned internal_delay:1; /* RAVB has internal delays */ }; struct ravb_private { diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 2ac962b5b8fb..02acae4d51c1 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1939,6 +1939,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = { .num_tx_desc = NUM_TX_DESC_GEN3, .stats_len = ARRAY_SIZE(ravb_gstrings_stats), .skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1, + .internal_delay = 1, }; static const struct ravb_hw_info ravb_gen2_hw_info = { @@ -2189,7 +2190,7 @@ static int ravb_probe(struct platform_device *pdev) /* Request GTI loading */ ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); - if (priv->chip_id != RCAR_GEN2) { + if (info->internal_delay) { ravb_parse_delay_mode(np, ndev); ravb_set_delay_mode(ndev); } @@ -2362,6 +2363,7 @@ static int __maybe_unused ravb_resume(struct device *dev) { struct net_device *ndev = dev_get_drvdata(dev); struct ravb_private *priv = netdev_priv(ndev); + const struct ravb_hw_info *info = priv->info; int ret = 0; /* If WoL is enabled set reset mode to rearm the WoL logic */ @@ -2384,7 +2386,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 (info->internal_delay) ravb_set_delay_mode(ndev); /* Restore descriptor base address table */