| Message ID | 20210802102654.5996-1-biju.das.jz@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Add Gigabit Ethernet driver support | expand |
On Mon, Aug 02, 2021 at 11:26:51AM +0100, Biju Das wrote: > The device stats strings for R-Car and RZ/G2L are different. > > R-Car provides 30 device stats, whereas RZ/G2L provides only 15. In > addition, RZ/G2L has stats "rx_queue_0_csum_offload_errors" instead of > "rx_queue_0_missed_errors". > > Add structure variables gstrings_stats and gstrings_size to struct > ravb_hw_info, so that subsequent SoCs can be added without any code > changes in the ravb_get_strings function. > > 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 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: > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC are > similar to the R-Car Ethernet AVB IP. With a few changes in the driver we > can support both IPs. > > Currently a runtime decision based on the chip type is used to distinguish > the HW differences between the SoC families. > > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2 and > RZ/G2L it is 2. For cases like this it is better to select the number of > TX descriptors by using a structure with a value, rather than a runtime > decision based on the chip type. > > This patch adds the num_tx_desc variable to struct ravb_hw_info and also > replaces the driver data chip type with struct ravb_hw_info by moving chip > type to it. > > 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 | 7 +++++ > drivers/net/ethernet/renesas/ravb_main.c | 38 +++++++++++++++--------- > 2 files changed, 31 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index 80e62ca2e3d3..cfb972c05b34 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -988,6 +988,11 @@ enum ravb_chip_id { > RCAR_GEN3, > }; > > +struct ravb_hw_info { > + enum ravb_chip_id chip_id; > + int num_tx_desc; I think this is rather the driver's choice, than the h/w feature... Perhaps a rename would help with that? :-) [...] MBR, Sergei
Hi Sergei, Thanks for the feedback. > Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to > driver data > > On 8/2/21 1:26 PM, Biju Das wrote: > > > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC > > are similar to the R-Car Ethernet AVB IP. With a few changes in the > > driver we can support both IPs. > > > > Currently a runtime decision based on the chip type is used to > > distinguish the HW differences between the SoC families. > > > > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2 > > and RZ/G2L it is 2. For cases like this it is better to select the > > number of TX descriptors by using a structure with a value, rather > > than a runtime decision based on the chip type. > > > > This patch adds the num_tx_desc variable to struct ravb_hw_info and > > also replaces the driver data chip type with struct ravb_hw_info by > > moving chip type to it. > > > > 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 | 7 +++++ > > drivers/net/ethernet/renesas/ravb_main.c | 38 > > +++++++++++++++--------- > > 2 files changed, 31 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > b/drivers/net/ethernet/renesas/ravb.h > > index 80e62ca2e3d3..cfb972c05b34 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -988,6 +988,11 @@ enum ravb_chip_id { > > RCAR_GEN3, > > }; > > > > +struct ravb_hw_info { > > + enum ravb_chip_id chip_id; > > + int num_tx_desc; > > I think this is rather the driver's choice, than the h/w feature... > Perhaps a rename would help with that? :-) It is consistent with current naming convention used by the driver. NUM_TX_DESC macro is replaced by num_tx_desc and the below run time decision based on chip type for H/W configuration for Gen2/Gen3 is replaced by info->num_tx_desc. priv->num_tx_desc = chip_id == RCAR_GEN2 ? NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3; Please let me know, if I am missing anything, Previously there is a suggestion to change the generic struct ravb_driver_data(which holds driver differences and HW features) with struct ravb_hw_info. Regards, Biju > > [...] > > MBR, Sergei
Hi Sergei, > Subject: RE: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to > driver data > > Hi Sergei, > > Thanks for the feedback. > > > Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to > > driver data > > > > On 8/2/21 1:26 PM, Biju Das wrote: > > > > > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC > > > are similar to the R-Car Ethernet AVB IP. With a few changes in the > > > driver we can support both IPs. > > > > > > Currently a runtime decision based on the chip type is used to > > > distinguish the HW differences between the SoC families. > > > > > > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car > > > Gen2 and RZ/G2L it is 2. For cases like this it is better to select > > > the number of TX descriptors by using a structure with a value, > > > rather than a runtime decision based on the chip type. > > > > > > This patch adds the num_tx_desc variable to struct ravb_hw_info and > > > also replaces the driver data chip type with struct ravb_hw_info by > > > moving chip type to it. > > > > > > 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 | 7 +++++ > > > drivers/net/ethernet/renesas/ravb_main.c | 38 > > > +++++++++++++++--------- > > > 2 files changed, 31 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > > b/drivers/net/ethernet/renesas/ravb.h > > > index 80e62ca2e3d3..cfb972c05b34 100644 > > > --- a/drivers/net/ethernet/renesas/ravb.h > > > +++ b/drivers/net/ethernet/renesas/ravb.h > > > @@ -988,6 +988,11 @@ enum ravb_chip_id { > > > RCAR_GEN3, > > > }; > > > > > > +struct ravb_hw_info { > > > + enum ravb_chip_id chip_id; > > > + int num_tx_desc; > > > > I think this is rather the driver's choice, than the h/w feature... > > Perhaps a rename would help with that? :-) > > It is consistent with current naming convention used by the driver. > NUM_TX_DESC macro is replaced by num_tx_desc. So the name should be ok. Indeed we are agreed to add function pointers to struct ravb_hw_info to avoid another level of indirection. If the concern is related to duplication of data(ie,priv->num_tx_desc vs info->num_tx_desc) I have a plan to remove priv->num_tx_desc with info->num_tx_desc later. Regards, Biju
Hello! On 8/2/21 1:26 PM, Biju Das wrote: > The number of queues used in retrieving device stats for R-Car is 2, > whereas for RZ/G2L it is 1. Mhm, how many RX queues are on your platform, 1? Then we don't need so specific name, just num_rx_queue. > Add the num_gstat_queue variable to struct ravb_hw_info, to add subsequent > SoCs without any code changes to the ravb_get_ethtool_stats function. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> [...] MBR, Sergei
Hi Sergei, Thanks for the feedback. > Subject: Re: [PATCH net-next v2 3/8] ravb: Add num_gstat_queue to struct > ravb_hw_info > > Hello! > > On 8/2/21 1:26 PM, Biju Das wrote: > > > The number of queues used in retrieving device stats for R-Car is 2, > > whereas for RZ/G2L it is 1. > > Mhm, how many RX queues are on your platform, 1? Then we don't need so > specific name, just num_rx_queue. There are 2 RX queues, but we provide only device stats information from first queue. R-Car = 2x15 = 30 device stats RZ/G2L = 1x15 = 15 device stats. Cheers, Biju > > > Add the num_gstat_queue variable to struct ravb_hw_info, to add > > subsequent SoCs without any code changes to the ravb_get_ethtool_stats > function. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > [...] > > MBR, Sergei
On 8/3/21 10:13 PM, Biju Das wrote: [...] >>> The number of queues used in retrieving device stats for R-Car is 2, >>> whereas for RZ/G2L it is 1. > >> >> Mhm, how many RX queues are on your platform, 1? Then we don't need so >> specific name, just num_rx_queue. > > There are 2 RX queues, but we provide only device stats information from first queue. > > R-Car = 2x15 = 30 device stats > RZ/G2L = 1x15 = 15 device stats. That's pretty strange... how the RX queue #1 is called? How many RX queues are, at all? > Cheers, > Biju MBR, Sergei
Hi Sergei, > Subject: Re: [PATCH net-next v2 3/8] ravb: Add num_gstat_queue to struct > ravb_hw_info > > On 8/3/21 10:13 PM, Biju Das wrote: > > [...] > >>> The number of queues used in retrieving device stats for R-Car is 2, > >>> whereas for RZ/G2L it is 1. > > > >> > >> Mhm, how many RX queues are on your platform, 1? Then we don't > >> need so specific name, just num_rx_queue. > > > > There are 2 RX queues, but we provide only device stats information from > first queue. > > > > R-Car = 2x15 = 30 device stats > > RZ/G2L = 1x15 = 15 device stats. > > That's pretty strange... how the RX queue #1 is called? How many RX > queues are, at all? For both R-Car and RZ/G2L, ------------------------- #define NUM_RX_QUEUE 2 #define NUM_TX_QUEUE 2 Target device stat output for RZ/G2L:- ------------------------------------ root@smarc-rzg2l:~# ethtool -S eth0 NIC statistics: rx_queue_0_current: 21852 tx_queue_0_current: 18854 rx_queue_0_dirty: 21852 tx_queue_0_dirty: 18854 rx_queue_0_packets: 21852 tx_queue_0_packets: 9427 rx_queue_0_bytes: 28224093 tx_queue_0_bytes: 1659438 rx_queue_0_mcast_packets: 498 rx_queue_0_errors: 0 rx_queue_0_crc_errors: 0 rx_queue_0_frame_errors: 0 rx_queue_0_length_errors: 0 rx_queue_0_csum_offload_errors: 0 rx_queue_0_over_errors: 0 root@smarc-rzg2l:~# Target device stat output for R-Car Gen3:- ---------------------------------------- root@hihope-rzg2m:~# ethtool -S eth0 NIC statistics: rx_queue_0_current: 34215 tx_queue_0_current: 14158 rx_queue_0_dirty: 34215 tx_queue_0_dirty: 14158 rx_queue_0_packets: 34215 tx_queue_0_packets: 14158 rx_queue_0_bytes: 38313586 tx_queue_0_bytes: 3222182 rx_queue_0_mcast_packets: 499 rx_queue_0_errors: 0 rx_queue_0_crc_errors: 0 rx_queue_0_frame_errors: 0 rx_queue_0_length_errors: 0 rx_queue_0_missed_errors: 0 rx_queue_0_over_errors: 0 rx_queue_1_current: 0 tx_queue_1_current: 0 rx_queue_1_dirty: 0 tx_queue_1_dirty: 0 rx_queue_1_packets: 0 tx_queue_1_packets: 0 rx_queue_1_bytes: 0 tx_queue_1_bytes: 0 rx_queue_1_mcast_packets: 0 rx_queue_1_errors: 0 rx_queue_1_crc_errors: 0 rx_queue_1_frame_errors: 0 rx_queue_1_length_errors: 0 rx_queue_1_missed_errors: 0 rx_queue_1_over_errors: 0 Cheers, Biju
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
On 8/3/21 8:57 AM, Biju Das wrote: >> Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to >> driver data >> >> On 8/2/21 1:26 PM, Biju Das wrote: >> >>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC >>> are similar to the R-Car Ethernet AVB IP. With a few changes in the >>> driver we can support both IPs. >>> >>> Currently a runtime decision based on the chip type is used to >>> distinguish the HW differences between the SoC families. >>> >>> The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2 >>> and RZ/G2L it is 2. For cases like this it is better to select the >>> number of TX descriptors by using a structure with a value, rather >>> than a runtime decision based on the chip type. >>> >>> This patch adds the num_tx_desc variable to struct ravb_hw_info and >>> also replaces the driver data chip type with struct ravb_hw_info by >>> moving chip type to it. >>> >>> 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 | 7 +++++ >>> drivers/net/ethernet/renesas/ravb_main.c | 38 >>> +++++++++++++++--------- >>> 2 files changed, 31 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/renesas/ravb.h >>> b/drivers/net/ethernet/renesas/ravb.h >>> index 80e62ca2e3d3..cfb972c05b34 100644 >>> --- a/drivers/net/ethernet/renesas/ravb.h >>> +++ b/drivers/net/ethernet/renesas/ravb.h >>> @@ -988,6 +988,11 @@ enum ravb_chip_id { >>> RCAR_GEN3, >>> }; >>> >>> +struct ravb_hw_info { >>> + enum ravb_chip_id chip_id; >>> + int num_tx_desc; How about leaving that field in the *struct* ravb_private? And adding the following instead: unsigned unaligned_tx: 1; >> I think this is rather the driver's choice, than the h/w feature... >> Perhaps a rename would help with that? :-) > > It is consistent with current naming convention used by the driver. NUM_TX_DESC macro is replaced by num_tx_desc and the below run time decision based on chip type for H/W configuration for Gen2/Gen3 is replaced by info->num_tx_desc. > > priv->num_tx_desc = chip_id == RCAR_GEN2 ? NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3; .. and then: priv->num_tx_desc = info->unaligned_tx ? NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3; > Please let me know, if I am missing anything, > > Previously there is a suggestion to change the generic struct ravb_driver_data (which holds driver differences and HW features) with struct ravb_hw_info. Well, my plan was to place all the hardware features supported into the *struct* ravb_hw_info and leave all the driver's software data in the *struct* ravb_private. > Regards, > Biju [...] MBR, Sergei
On 8/4/21 10:27 PM, Sergei Shtylyov wrote: >>> Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to >>> driver data >>> >>> On 8/2/21 1:26 PM, Biju Das wrote: >>> >>>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC >>>> are similar to the R-Car Ethernet AVB IP. With a few changes in the >>>> driver we can support both IPs. >>>> >>>> Currently a runtime decision based on the chip type is used to >>>> distinguish the HW differences between the SoC families. >>>> >>>> The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2 >>>> and RZ/G2L it is 2. For cases like this it is better to select the >>>> number of TX descriptors by using a structure with a value, rather >>>> than a runtime decision based on the chip type. >>>> >>>> This patch adds the num_tx_desc variable to struct ravb_hw_info and >>>> also replaces the driver data chip type with struct ravb_hw_info by >>>> moving chip type to it. >>>> >>>> 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 | 7 +++++ >>>> drivers/net/ethernet/renesas/ravb_main.c | 38 >>>> +++++++++++++++--------- >>>> 2 files changed, 31 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/renesas/ravb.h >>>> b/drivers/net/ethernet/renesas/ravb.h >>>> index 80e62ca2e3d3..cfb972c05b34 100644 >>>> --- a/drivers/net/ethernet/renesas/ravb.h >>>> +++ b/drivers/net/ethernet/renesas/ravb.h >>>> @@ -988,6 +988,11 @@ enum ravb_chip_id { >>>> RCAR_GEN3, >>>> }; >>>> >>>> +struct ravb_hw_info { >>>> + enum ravb_chip_id chip_id; >>>> + int num_tx_desc; > > How about leaving that field in the *struct* ravb_private? And adding the following instead: > > unsigned unaligned_tx: 1; Or aligned_tx, so that gen2 has it set, and gen3 has it cleared. > >>> I think this is rather the driver's choice, than the h/w feature... >>> Perhaps a rename would help with that? :-) >> >> It is consistent with current naming convention used by the driver. NUM_TX_DESC macro is replaced by num_tx_desc and the below run time decision based on chip type for H/W configuration for Gen2/Gen3 is replaced by info->num_tx_desc. >> >> priv->num_tx_desc = chip_id == RCAR_GEN2 ? NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3; > > .. and then: > > priv->num_tx_desc = info->unaligned_tx ? NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3; Sorry, mixed the values, should have been: priv->num_tx_desc = info->unaligned_tx ? NUM_TX_DESC_GEN3 : NUM_TX_DESC_GEN2; >> Please let me know, if I am missing anything, >> >> Previously there is a suggestion to change the generic struct ravb_driver_data (which holds driver differences and HW features) with struct ravb_hw_info. > > Well, my plan was to place all the hardware features supported into the *struct* ravb_hw_info and leave all > the driver's software data in the *struct* ravb_private. ... just like *struct* sh_eth_cpu_data and sh_eth_private in the sh_eth driver. >> Regards, >> Biju MBR, Sergei
On 8/2/21 1:26 PM, Biju Das wrote: > The device stats strings for R-Car and RZ/G2L are different. > > R-Car provides 30 device stats, whereas RZ/G2L provides only 15. In > addition, RZ/G2L has stats "rx_queue_0_csum_offload_errors" instead of > "rx_queue_0_missed_errors". > > Add structure variables gstrings_stats and gstrings_size to struct > ravb_hw_info, so that subsequent SoCs can be added without any code > changes in the ravb_get_strings function. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@gmail.com> [...] MBR, Sergei
Hi Biju, On Mon, Aug 2, 2021 at 12:27 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC are > similar to the R-Car Ethernet AVB IP. With a few changes in the driver we > can support both IPs. > > Currently a runtime decision based on the chip type is used to distinguish > the HW differences between the SoC families. > > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2 and > RZ/G2L it is 2. For cases like this it is better to select the number of > TX descriptors by using a structure with a value, rather than a runtime > decision based on the chip type. > > This patch adds the num_tx_desc variable to struct ravb_hw_info and also > replaces the driver data chip type with struct ravb_hw_info by moving chip > type to it. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Thanks for your patch! > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -988,6 +988,11 @@ enum ravb_chip_id { > RCAR_GEN3, > }; > > +struct ravb_hw_info { > + enum ravb_chip_id chip_id; > + int num_tx_desc; Why not "unsigned int"? ... This comment applies to a few more subsequent patches. > +}; > + > struct ravb_private { > struct net_device *ndev; > struct platform_device *pdev; > @@ -1040,6 +1045,8 @@ struct ravb_private { > unsigned txcidm:1; /* TX Clock Internal Delay Mode */ > unsigned rgmii_override:1; /* Deprecated rgmii-*id behavior */ > int num_tx_desc; /* TX descriptors per packet */ ... oh, here's the original culprit. > + > + const struct ravb_hw_info *info; > }; > Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, Thanks for the feedback. > -----Original Message----- > Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to > driver data > > Hi Biju, > > On Mon, Aug 2, 2021 at 12:27 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC > > are similar to the R-Car Ethernet AVB IP. With a few changes in the > > driver we can support both IPs. > > > > Currently a runtime decision based on the chip type is used to > > distinguish the HW differences between the SoC families. > > > > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2 > > and RZ/G2L it is 2. For cases like this it is better to select the > > number of TX descriptors by using a structure with a value, rather > > than a runtime decision based on the chip type. > > > > This patch adds the num_tx_desc variable to struct ravb_hw_info and > > also replaces the driver data chip type with struct ravb_hw_info by > > moving chip type to it. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -988,6 +988,11 @@ enum ravb_chip_id { > > RCAR_GEN3, > > }; > > > > +struct ravb_hw_info { > > + enum ravb_chip_id chip_id; > > + int num_tx_desc; > > Why not "unsigned int"? ... > This comment applies to a few more subsequent patches. To avoid signed and unsigned comparison warnings. > > > +}; > > + > > struct ravb_private { > > struct net_device *ndev; > > struct platform_device *pdev; > > @@ -1040,6 +1045,8 @@ struct ravb_private { > > unsigned txcidm:1; /* TX Clock Internal Delay Mode > */ > > unsigned rgmii_override:1; /* Deprecated rgmii-*id behavior > */ > > int num_tx_desc; /* TX descriptors per packet */ > > ... oh, here's the original culprit. Exactly, this the reason. Do you want me to change this into unsigned int? Please let me know. Regards, Biju > > > + > > + const struct ravb_hw_info *info; > > }; > > > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. > But when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds
Hi Biju, On Thu, Aug 12, 2021 at 9:26 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > -----Original Message----- > > On Mon, Aug 2, 2021 at 12:27 PM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC > > > are similar to the R-Car Ethernet AVB IP. With a few changes in the > > > driver we can support both IPs. > > > > > > Currently a runtime decision based on the chip type is used to > > > distinguish the HW differences between the SoC families. > > > > > > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2 > > > and RZ/G2L it is 2. For cases like this it is better to select the > > > number of TX descriptors by using a structure with a value, rather > > > than a runtime decision based on the chip type. > > > > > > This patch adds the num_tx_desc variable to struct ravb_hw_info and > > > also replaces the driver data chip type with struct ravb_hw_info by > > > moving chip type to it. > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Thanks for your patch! > > > > > --- a/drivers/net/ethernet/renesas/ravb.h > > > +++ b/drivers/net/ethernet/renesas/ravb.h > > > @@ -988,6 +988,11 @@ enum ravb_chip_id { > > > RCAR_GEN3, > > > }; > > > > > > +struct ravb_hw_info { > > > + enum ravb_chip_id chip_id; > > > + int num_tx_desc; > > > > Why not "unsigned int"? ... > > This comment applies to a few more subsequent patches. > > To avoid signed and unsigned comparison warnings. > > > > > > +}; > > > + > > > struct ravb_private { > > > struct net_device *ndev; > > > struct platform_device *pdev; > > > @@ -1040,6 +1045,8 @@ struct ravb_private { > > > unsigned txcidm:1; /* TX Clock Internal Delay Mode > > */ > > > unsigned rgmii_override:1; /* Deprecated rgmii-*id behavior > > */ > > > int num_tx_desc; /* TX descriptors per packet */ > > > > ... oh, here's the original culprit. > > Exactly, this the reason. > > Do you want me to change this into unsigned int? Please let me know. Up to you (or the maintainer? ;-) For new fields (in the other patches), I would use unsigned for all unsigned values. Signed values have more pitfalls related to undefined behavior. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to > driver data > > Hi Biju, > > On Thu, Aug 12, 2021 at 9:26 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > -----Original Message----- > > > On Mon, Aug 2, 2021 at 12:27 PM Biju Das > > > <biju.das.jz@bp.renesas.com> > > > wrote: > > > > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L > > > > SoC are similar to the R-Car Ethernet AVB IP. With a few changes > > > > in the driver we can support both IPs. > > > > > > > > Currently a runtime decision based on the chip type is used to > > > > distinguish the HW differences between the SoC families. > > > > > > > > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car > > > > Gen2 and RZ/G2L it is 2. For cases like this it is better to > > > > select the number of TX descriptors by using a structure with a > > > > value, rather than a runtime decision based on the chip type. > > > > > > > > This patch adds the num_tx_desc variable to struct ravb_hw_info > > > > and also replaces the driver data chip type with struct > > > > ravb_hw_info by moving chip type to it. > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > Reviewed-by: Lad Prabhakar > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > Thanks for your patch! > > > > > > > --- a/drivers/net/ethernet/renesas/ravb.h > > > > +++ b/drivers/net/ethernet/renesas/ravb.h > > > > @@ -988,6 +988,11 @@ enum ravb_chip_id { > > > > RCAR_GEN3, > > > > }; > > > > > > > > +struct ravb_hw_info { > > > > + enum ravb_chip_id chip_id; > > > > + int num_tx_desc; > > > > > > Why not "unsigned int"? ... > > > This comment applies to a few more subsequent patches. > > > > To avoid signed and unsigned comparison warnings. > > > > > > > > > +}; > > > > + > > > > struct ravb_private { > > > > struct net_device *ndev; > > > > struct platform_device *pdev; @@ -1040,6 +1045,8 @@ struct > > > > ravb_private { > > > > unsigned txcidm:1; /* TX Clock Internal Delay > Mode > > > */ > > > > unsigned rgmii_override:1; /* Deprecated rgmii-*id > behavior > > > */ > > > > int num_tx_desc; /* TX descriptors per packet > */ > > > > > > ... oh, here's the original culprit. > > > > Exactly, this the reason. > > > > Do you want me to change this into unsigned int? Please let me know. > > Up to you (or the maintainer? ;-) > > For new fields (in the other patches), I would use unsigned for all > unsigned values. Signed values have more pitfalls related to undefined > behavior. Sergei, What is your thoughts here? Please let me know. Cheers, Biju
Hi all, > Subject: RE: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to > driver data > > Hi Geert, > > Thanks for the feedback. > > > Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to > > driver data > > > > Hi Biju, > > > > On Thu, Aug 12, 2021 at 9:26 AM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > > -----Original Message----- > > > > On Mon, Aug 2, 2021 at 12:27 PM Biju Das > > > > <biju.das.jz@bp.renesas.com> > > > > wrote: > > > > > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L > > > > > SoC are similar to the R-Car Ethernet AVB IP. With a few changes > > > > > in the driver we can support both IPs. > > > > > > > > > > Currently a runtime decision based on the chip type is used to > > > > > distinguish the HW differences between the SoC families. > > > > > > > > > > The number of TX descriptors for R-Car Gen3 is 1 whereas on > > > > > R-Car > > > > > Gen2 and RZ/G2L it is 2. For cases like this it is better to > > > > > select the number of TX descriptors by using a structure with a > > > > > value, rather than a runtime decision based on the chip type. > > > > > > > > > > This patch adds the num_tx_desc variable to struct ravb_hw_info > > > > > and also replaces the driver data chip type with struct > > > > > ravb_hw_info by moving chip type to it. > > > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > > Reviewed-by: Lad Prabhakar > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > Thanks for your patch! > > > > > > > > > --- a/drivers/net/ethernet/renesas/ravb.h > > > > > +++ b/drivers/net/ethernet/renesas/ravb.h > > > > > @@ -988,6 +988,11 @@ enum ravb_chip_id { > > > > > RCAR_GEN3, > > > > > }; > > > > > > > > > > +struct ravb_hw_info { > > > > > + enum ravb_chip_id chip_id; > > > > > + int num_tx_desc; > > > > > > > > Why not "unsigned int"? ... > > > > This comment applies to a few more subsequent patches. > > > > > > To avoid signed and unsigned comparison warnings. > > > > > > > > > > > > +}; > > > > > + > > > > > struct ravb_private { > > > > > struct net_device *ndev; > > > > > struct platform_device *pdev; @@ -1040,6 +1045,8 @@ > > > > > struct ravb_private { > > > > > unsigned txcidm:1; /* TX Clock Internal Delay > > Mode > > > > */ > > > > > unsigned rgmii_override:1; /* Deprecated rgmii-*id > > behavior > > > > */ > > > > > int num_tx_desc; /* TX descriptors per > packet > > */ > > > > > > > > ... oh, here's the original culprit. > > > > > > Exactly, this the reason. > > > > > > Do you want me to change this into unsigned int? Please let me know. > > > > Up to you (or the maintainer? ;-) > > > > For new fields (in the other patches), I would use unsigned for all > > unsigned values. Signed values have more pitfalls related to > > undefined behavior. > > Sergei, What is your thoughts here? Please let me know. Here is my plan. I will split this patch into two as Andrew suggested and Then on the second patch will add as info->unaligned_tx as Sergei suggested. Now the only open point is related to the data type of "int num_tx_desc" and to align with sh_eth driver I will keep int. Regards, Biju
Hi All, I have tested without this patch and got expected results. So I am dropping this patch in the next version. Cheers, Biju > Subject: RE: [PATCH net-next v2 3/8] ravb: Add num_gstat_queue to struct > ravb_hw_info > > Hi Sergei, > > > Subject: Re: [PATCH net-next v2 3/8] ravb: Add num_gstat_queue to > > struct ravb_hw_info > > > > On 8/3/21 10:13 PM, Biju Das wrote: > > > > [...] > > >>> The number of queues used in retrieving device stats for R-Car is > > >>> 2, whereas for RZ/G2L it is 1. > > > > > >> > > >> Mhm, how many RX queues are on your platform, 1? Then we don't > > >> need so specific name, just num_rx_queue. > > > > > > There are 2 RX queues, but we provide only device stats information > > > from > > first queue. > > > > > > R-Car = 2x15 = 30 device stats > > > RZ/G2L = 1x15 = 15 device stats. > > > > That's pretty strange... how the RX queue #1 is called? How many > > RX queues are, at all? > > For both R-Car and RZ/G2L, > ------------------------- > #define NUM_RX_QUEUE 2 > #define NUM_TX_QUEUE 2 > > Target device stat output for RZ/G2L:- > ------------------------------------ > root@smarc-rzg2l:~# ethtool -S eth0 > NIC statistics: > rx_queue_0_current: 21852 > tx_queue_0_current: 18854 > rx_queue_0_dirty: 21852 > tx_queue_0_dirty: 18854 > rx_queue_0_packets: 21852 > tx_queue_0_packets: 9427 > rx_queue_0_bytes: 28224093 > tx_queue_0_bytes: 1659438 > rx_queue_0_mcast_packets: 498 > rx_queue_0_errors: 0 > rx_queue_0_crc_errors: 0 > rx_queue_0_frame_errors: 0 > rx_queue_0_length_errors: 0 > rx_queue_0_csum_offload_errors: 0 > rx_queue_0_over_errors: 0 > root@smarc-rzg2l:~# > > > Target device stat output for R-Car Gen3:- > ---------------------------------------- > root@hihope-rzg2m:~# ethtool -S eth0 > NIC statistics: > rx_queue_0_current: 34215 > tx_queue_0_current: 14158 > rx_queue_0_dirty: 34215 > tx_queue_0_dirty: 14158 > rx_queue_0_packets: 34215 > tx_queue_0_packets: 14158 > rx_queue_0_bytes: 38313586 > tx_queue_0_bytes: 3222182 > rx_queue_0_mcast_packets: 499 > rx_queue_0_errors: 0 > rx_queue_0_crc_errors: 0 > rx_queue_0_frame_errors: 0 > rx_queue_0_length_errors: 0 > rx_queue_0_missed_errors: 0 > rx_queue_0_over_errors: 0 > rx_queue_1_current: 0 > tx_queue_1_current: 0 > rx_queue_1_dirty: 0 > tx_queue_1_dirty: 0 > rx_queue_1_packets: 0 > tx_queue_1_packets: 0 > rx_queue_1_bytes: 0 > tx_queue_1_bytes: 0 > rx_queue_1_mcast_packets: 0 > rx_queue_1_errors: 0 > rx_queue_1_crc_errors: 0 > rx_queue_1_frame_errors: 0 > rx_queue_1_length_errors: 0 > rx_queue_1_missed_errors: 0 > rx_queue_1_over_errors: 0 > > Cheers, > Biju
On 8/17/21 2:24 PM, Biju Das wrote: [...] >>>>> -----Original Message----- >>>>> On Mon, Aug 2, 2021 at 12:27 PM Biju Das >>>>> <biju.das.jz@bp.renesas.com> >>>>> wrote: >>>>>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L >>>>>> SoC are similar to the R-Car Ethernet AVB IP. With a few changes >>>>>> in the driver we can support both IPs. >>>>>> >>>>>> Currently a runtime decision based on the chip type is used to >>>>>> distinguish the HW differences between the SoC families. >>>>>> >>>>>> The number of TX descriptors for R-Car Gen3 is 1 whereas on >>>>>> R-Car >>>>>> Gen2 and RZ/G2L it is 2. For cases like this it is better to >>>>>> select the number of TX descriptors by using a structure with a >>>>>> value, rather than a runtime decision based on the chip type. >>>>>> >>>>>> This patch adds the num_tx_desc variable to struct ravb_hw_info >>>>>> and also replaces the driver data chip type with struct >>>>>> ravb_hw_info by moving chip type to it. >>>>>> >>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>>>>> Reviewed-by: Lad Prabhakar >>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com> >>>>> >>>>> Thanks for your patch! >>>>> >>>>>> --- a/drivers/net/ethernet/renesas/ravb.h >>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h >>>>>> @@ -988,6 +988,11 @@ enum ravb_chip_id { >>>>>> RCAR_GEN3, >>>>>> }; >>>>>> >>>>>> +struct ravb_hw_info { >>>>>> + enum ravb_chip_id chip_id; >>>>>> + int num_tx_desc; >>>>> >>>>> Why not "unsigned int"? ... >>>>> This comment applies to a few more subsequent patches. >>>> >>>> To avoid signed and unsigned comparison warnings. >>>> >>>>> >>>>>> +}; >>>>>> + >>>>>> struct ravb_private { >>>>>> struct net_device *ndev; >>>>>> struct platform_device *pdev; @@ -1040,6 +1045,8 @@ >>>>>> struct ravb_private { >>>>>> unsigned txcidm:1; /* TX Clock Internal Delay >>> Mode >>>>> */ >>>>>> unsigned rgmii_override:1; /* Deprecated rgmii-*id >>> behavior >>>>> */ >>>>>> int num_tx_desc; /* TX descriptors per >> packet >>> */ >>>>> >>>>> ... oh, here's the original culprit. >>>> >>>> Exactly, this the reason. >>>> >>>> Do you want me to change this into unsigned int? Please let me know. >>> >>> Up to you (or the maintainer? ;-) >>> >>> For new fields (in the other patches), I would use unsigned for all >>> unsigned values. Signed values have more pitfalls related to >>> undefined behavior. >> >> Sergei, What is your thoughts here? Please let me know. > > Here is my plan. > > I will split this patch into two as Andrew suggested and If you mran changing the ravb_private::num_tx_desc to *unsigned*, it'll be a good cleanup. What's would be the 2nd part tho? > Then on the second patch will add as info->unaligned_tx as Sergei suggested. OK. > Now the only open point is related to the data type of "int num_tx_desc" > and to align with sh_eth driver I will keep int. The sh_eth driver simply doesn't have this -- it always use 1 descriptor. > Regards, > Biju MBR, Sergey
Hi Sergei, Thanks for the feedback. > Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to > driver data > > On 8/17/21 2:24 PM, Biju Das wrote: > > [...] > >>>>> -----Original Message----- > >>>>> On Mon, Aug 2, 2021 at 12:27 PM Biju Das > >>>>> <biju.das.jz@bp.renesas.com> > >>>>> wrote: > >>>>>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L > >>>>>> SoC are similar to the R-Car Ethernet AVB IP. With a few changes > >>>>>> in the driver we can support both IPs. > >>>>>> > >>>>>> Currently a runtime decision based on the chip type is used to > >>>>>> distinguish the HW differences between the SoC families. > >>>>>> > >>>>>> The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car > >>>>>> Gen2 and RZ/G2L it is 2. For cases like this it is better to > >>>>>> select the number of TX descriptors by using a structure with a > >>>>>> value, rather than a runtime decision based on the chip type. > >>>>>> > >>>>>> This patch adds the num_tx_desc variable to struct ravb_hw_info > >>>>>> and also replaces the driver data chip type with struct > >>>>>> ravb_hw_info by moving chip type to it. > >>>>>> > >>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>>>>> Reviewed-by: Lad Prabhakar > >>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>>>> > >>>>> Thanks for your patch! > >>>>> > >>>>>> --- a/drivers/net/ethernet/renesas/ravb.h > >>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h > >>>>>> @@ -988,6 +988,11 @@ enum ravb_chip_id { > >>>>>> RCAR_GEN3, > >>>>>> }; > >>>>>> > >>>>>> +struct ravb_hw_info { > >>>>>> + enum ravb_chip_id chip_id; > >>>>>> + int num_tx_desc; > >>>>> > >>>>> Why not "unsigned int"? ... > >>>>> This comment applies to a few more subsequent patches. > >>>> > >>>> To avoid signed and unsigned comparison warnings. > >>>> > >>>>> > >>>>>> +}; > >>>>>> + > >>>>>> struct ravb_private { > >>>>>> struct net_device *ndev; > >>>>>> struct platform_device *pdev; @@ -1040,6 +1045,8 @@ > >>>>>> struct ravb_private { > >>>>>> unsigned txcidm:1; /* TX Clock Internal Delay > >>> Mode > >>>>> */ > >>>>>> unsigned rgmii_override:1; /* Deprecated rgmii-*id > >>> behavior > >>>>> */ > >>>>>> int num_tx_desc; /* TX descriptors per > >> packet > >>> */ > >>>>> > >>>>> ... oh, here's the original culprit. > >>>> > >>>> Exactly, this the reason. > >>>> > >>>> Do you want me to change this into unsigned int? Please let me know. > >>> > >>> Up to you (or the maintainer? ;-) > >>> > >>> For new fields (in the other patches), I would use unsigned for all > >>> unsigned values. Signed values have more pitfalls related to > >>> undefined behavior. > >> > >> Sergei, What is your thoughts here? Please let me know. > > > > Here is my plan. > > > > I will split this patch into two as Andrew suggested and > > If you mran changing the ravb_private::num_tx_desc to *unsigned*, it'll > be a good cleanup. What's would be the 2nd part tho? OK in that case, I will split this patch into 3. First patch for adding struct ravb_hw_info to driver data and replace driver data chip type with struct ravb_hw_info Second patch for changing ravb_private::num_tx_desc from int to unsigned int. Third patch for adding aligned_tx to struct ravb_hw_info. Regards, Biju
Hello! On 18.08.2021 9:29, Biju Das wrote: [...] >>>>>>> -----Original Message----- >>>>>>> On Mon, Aug 2, 2021 at 12:27 PM Biju Das >>>>>>> <biju.das.jz@bp.renesas.com> >>>>>>> wrote: >>>>>>>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L >>>>>>>> SoC are similar to the R-Car Ethernet AVB IP. With a few changes >>>>>>>> in the driver we can support both IPs. >>>>>>>> >>>>>>>> Currently a runtime decision based on the chip type is used to >>>>>>>> distinguish the HW differences between the SoC families. >>>>>>>> >>>>>>>> The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car >>>>>>>> Gen2 and RZ/G2L it is 2. For cases like this it is better to >>>>>>>> select the number of TX descriptors by using a structure with a >>>>>>>> value, rather than a runtime decision based on the chip type. >>>>>>>> >>>>>>>> This patch adds the num_tx_desc variable to struct ravb_hw_info >>>>>>>> and also replaces the driver data chip type with struct >>>>>>>> ravb_hw_info by moving chip type to it. >>>>>>>> >>>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>>>>>>> Reviewed-by: Lad Prabhakar >>>>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com> >>>>>>> >>>>>>> Thanks for your patch! >>>>>>> >>>>>>>> --- a/drivers/net/ethernet/renesas/ravb.h >>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h >>>>>>>> @@ -988,6 +988,11 @@ enum ravb_chip_id { >>>>>>>> RCAR_GEN3, >>>>>>>> }; >>>>>>>> >>>>>>>> +struct ravb_hw_info { >>>>>>>> + enum ravb_chip_id chip_id; >>>>>>>> + int num_tx_desc; >>>>>>> >>>>>>> Why not "unsigned int"? ... >>>>>>> This comment applies to a few more subsequent patches. >>>>>> >>>>>> To avoid signed and unsigned comparison warnings. >>>>>> >>>>>>> >>>>>>>> +}; >>>>>>>> + >>>>>>>> struct ravb_private { >>>>>>>> struct net_device *ndev; >>>>>>>> struct platform_device *pdev; @@ -1040,6 +1045,8 @@ >>>>>>>> struct ravb_private { >>>>>>>> unsigned txcidm:1; /* TX Clock Internal Delay >>>>> Mode >>>>>>> */ >>>>>>>> unsigned rgmii_override:1; /* Deprecated rgmii-*id >>>>> behavior >>>>>>> */ >>>>>>>> int num_tx_desc; /* TX descriptors per >>>> packet >>>>> */ >>>>>>> >>>>>>> ... oh, here's the original culprit. >>>>>> >>>>>> Exactly, this the reason. >>>>>> >>>>>> Do you want me to change this into unsigned int? Please let me know. >>>>> >>>>> Up to you (or the maintainer? ;-) >>>>> >>>>> For new fields (in the other patches), I would use unsigned for all >>>>> unsigned values. Signed values have more pitfalls related to >>>>> undefined behavior. >>>> >>>> Sergei, What is your thoughts here? Please let me know. >>> >>> Here is my plan. >>> >>> I will split this patch into two as Andrew suggested and >> >> If you mran changing the ravb_private::num_tx_desc to *unsigned*, it'll >> be a good cleanup. What's would be the 2nd part tho? > > OK in that case, I will split this patch into 3. > > First patch for adding struct ravb_hw_info to driver data and replace > driver data chip type with struct ravb_hw_info Couldn't this be a 2nd patch?.. > Second patch for changing ravb_private::num_tx_desc from int to unsigned int. ... and this one the 1st? > Third patch for adding aligned_tx to struct ravb_hw_info. > > Regards, > Biju MBR, Sergey
Hi Sergei, > -----Original Message----- > Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to > driver data > > Hello! > > On 18.08.2021 9:29, Biju Das wrote: > > [...] > >>>>>>> -----Original Message----- > >>>>>>> On Mon, Aug 2, 2021 at 12:27 PM Biju Das > >>>>>>> <biju.das.jz@bp.renesas.com> > >>>>>>> wrote: > >>>>>>>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L > >>>>>>>> SoC are similar to the R-Car Ethernet AVB IP. With a few > >>>>>>>> changes in the driver we can support both IPs. > >>>>>>>> > >>>>>>>> Currently a runtime decision based on the chip type is used to > >>>>>>>> distinguish the HW differences between the SoC families. > >>>>>>>> > >>>>>>>> The number of TX descriptors for R-Car Gen3 is 1 whereas on > >>>>>>>> R-Car > >>>>>>>> Gen2 and RZ/G2L it is 2. For cases like this it is better to > >>>>>>>> select the number of TX descriptors by using a structure with a > >>>>>>>> value, rather than a runtime decision based on the chip type. > >>>>>>>> > >>>>>>>> This patch adds the num_tx_desc variable to struct ravb_hw_info > >>>>>>>> and also replaces the driver data chip type with struct > >>>>>>>> ravb_hw_info by moving chip type to it. > >>>>>>>> > >>>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>>>>>>> Reviewed-by: Lad Prabhakar > >>>>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>>>>>> > >>>>>>> Thanks for your patch! > >>>>>>> > >>>>>>>> --- a/drivers/net/ethernet/renesas/ravb.h > >>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h > >>>>>>>> @@ -988,6 +988,11 @@ enum ravb_chip_id { > >>>>>>>> RCAR_GEN3, > >>>>>>>> }; > >>>>>>>> > >>>>>>>> +struct ravb_hw_info { > >>>>>>>> + enum ravb_chip_id chip_id; > >>>>>>>> + int num_tx_desc; > >>>>>>> > >>>>>>> Why not "unsigned int"? ... > >>>>>>> This comment applies to a few more subsequent patches. > >>>>>> > >>>>>> To avoid signed and unsigned comparison warnings. > >>>>>> > >>>>>>> > >>>>>>>> +}; > >>>>>>>> + > >>>>>>>> struct ravb_private { > >>>>>>>> struct net_device *ndev; > >>>>>>>> struct platform_device *pdev; @@ -1040,6 +1045,8 @@ > >>>>>>>> struct ravb_private { > >>>>>>>> unsigned txcidm:1; /* TX Clock Internal > Delay > >>>>> Mode > >>>>>>> */ > >>>>>>>> unsigned rgmii_override:1; /* Deprecated rgmii-*id > >>>>> behavior > >>>>>>> */ > >>>>>>>> int num_tx_desc; /* TX descriptors per > >>>> packet > >>>>> */ > >>>>>>> > >>>>>>> ... oh, here's the original culprit. > >>>>>> > >>>>>> Exactly, this the reason. > >>>>>> > >>>>>> Do you want me to change this into unsigned int? Please let me > know. > >>>>> > >>>>> Up to you (or the maintainer? ;-) > >>>>> > >>>>> For new fields (in the other patches), I would use unsigned for > >>>>> all unsigned values. Signed values have more pitfalls related to > >>>>> undefined behavior. > >>>> > >>>> Sergei, What is your thoughts here? Please let me know. > >>> > >>> Here is my plan. > >>> > >>> I will split this patch into two as Andrew suggested and > >> > >> If you mran changing the ravb_private::num_tx_desc to *unsigned*, > >> it'll be a good cleanup. What's would be the 2nd part tho? > > > > OK in that case, I will split this patch into 3. > > > > First patch for adding struct ravb_hw_info to driver data and replace > > driver data chip type with struct ravb_hw_info > > Couldn't this be a 2nd patch?.. > > > Second patch for changing ravb_private::num_tx_desc from int to unsigned > int. > > ... and this one the 1st? > > > Third patch for adding aligned_tx to struct ravb_hw_info. Sure. Will do. Cheers, Biju
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). With a few changes in the driver we can support both IPs. Currently a runtime decision based on the chip type is used to distinguish the HW differences between the SoC families. This patch series is in preparation for supporting the RZ/G2L SoC by replacing driver data chip type with struct ravb_hw_info by moving chip type to it and also adding gstrings_stats, gstrings_size, net_hw_features, net_features, num_gstat_queue, num_tx_desc, stats_len, skb_sz variables to it. This patch also adds the feature bit for {RX, TX} clock internal delays and TX Drop counters HW features found on R-Car Gen3 to struct ravb_hw_info. This patch series is based on net-next. v1->v2: * Replaced driver data chip type with struct ravb_hw_info * Added gstrings_stats, gstrings_size, net_hw_features, net_features, num_gstat_queue, num_tx_desc, stats_len, skb_sz to struct ravb_hw_info * Added internal_delay and tx_drop_cntrs hw feature bit to struct ravb_hw_info RFC->V1 * Incorporated feedback from Andrew, Sergei, Geert and Prabhakar * https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=515525 Biju Das (8): ravb: Add struct ravb_hw_info to driver data ravb: Add skb_sz to struct ravb_hw_info ravb: Add num_gstat_queue to struct ravb_hw_info ravb: Add stats_len to struct ravb_hw_info ravb: Add gstrings_stats and gstrings_size to struct ravb_hw_info ravb: Add net_features and net_hw_features to struct ravb_hw_info ravb: Add internal delay hw feature to struct ravb_hw_info ravb: Add tx_drop_cntrs to struct ravb_hw_info drivers/net/ethernet/renesas/ravb.h | 18 +++++ drivers/net/ethernet/renesas/ravb_main.c | 91 ++++++++++++++++-------- 2 files changed, 80 insertions(+), 29 deletions(-)