From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-05 11:06:51
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.
This patch series is aims to add functional support for Gigabit Ethernet driver
by filling all the stubs.
Ref:-
https://lore.kernel.org/linux-renesas-soc/OS0PR01MB5922240F88E5E0FD989ECDF386AC9@OS0PR01MB5922.jpnprd01.prod.outlook.com/T/#m8dee0a1b14d505d4611cad8c10e4017a30db55d6
RFC changes:
* used ALIGN macro for calculating the value for max_rx_len.
* used rx_max_buf_size instead of rx_2k_buffers feature bit.
* moved struct ravb_rx_desc *gbeth_rx_ring near to ravb_private::rx_ring
and allocating it for 1 RX queue.
* Started using gbeth_rx_ring instead of gbeth_rx_ring[q].
* renamed ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
* renamed ravb_rx_ring_free to ravb_rx_ring_free_rcar
* renamed ravb_rx_ring_format to ravb_rx_ring_format_rcar
* renamed ravb_rcar_rx to ravb_rx_rcar
* renamed "tsrq" variable
* Updated the comments
Biju Das (12):
ravb: Use ALIGN macro for max_rx_len
ravb: Add rx_max_buf_size to struct ravb_hw_info
ravb: Fillup ravb_set_features_gbeth() stub
ravb: Fillup ravb_alloc_rx_desc_gbeth() stub
ravb: Fillup ravb_rx_ring_free_gbeth() stub
ravb: Fillup ravb_rx_ring_format_gbeth() stub
ravb: Fillup ravb_rx_gbeth() stub
ravb: Add carrier_counters to struct ravb_hw_info
ravb: Add support to retrieve stats for GbEthernet
ravb: Rename "tsrq" variable
ravb: Optimize ravb_emac_init_gbeth function
ravb: Update/Add comments
drivers/net/ethernet/renesas/ravb.h | 51 +++-
drivers/net/ethernet/renesas/ravb_main.c | 349 +++++++++++++++++++++--
2 files changed, 367 insertions(+), 33 deletions(-)
--
2.17.1
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-05 11:06:53
Use ALIGN macro for calculating the value for max_rx_len.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Suggested-by: Sergey Shtylyov <redacted>
---
drivers/net/ethernet/renesas/ravb_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-05 11:06:57
R-Car AVB-DMAC has maximum 2K size on RX buffer, whereas on RZ/G2L
it is 8K. We need to allow for changing the MTU within the limit
of the maximum size of a descriptor.
Add a rx_max_buf_size variable to struct ravb_hw_info to handle
this difference.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC:
* used buffer_size instead of feature bit.
---
drivers/net/ethernet/renesas/ravb.h | 1 +
drivers/net/ethernet/renesas/ravb_main.c | 5 ++++-
2 files changed, 5 insertions(+), 1 deletion(-)
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-05 11:07:03
Fillup ravb_set_features_gbeth() function to support RZ/G2L.
Also set the net_hw_features bits supported by GbEthernet
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC changes:
* Added CSR0 initilization
* Seperated and created a new patch fro retrieving stats.
---
drivers/net/ethernet/renesas/ravb.h | 38 ++++++++++++++++++++++++
drivers/net/ethernet/renesas/ravb_main.c | 34 ++++++++++++++++++++-
2 files changed, 71 insertions(+), 1 deletion(-)
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-05 11:07:05
Fillup ravb_alloc_rx_desc_gbeth() function to support RZ/G2L.
This patch also renames ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
to be consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC->v1:
* started allocating 1 rx queue for "gbeth_rx_ring"
* Moved gbeth_rx_ring near to rx_ring in priv structure
* renamed ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
---
drivers/net/ethernet/renesas/ravb.h | 1 +
drivers/net/ethernet/renesas/ravb_main.c | 30 ++++++++++++++++++------
2 files changed, 24 insertions(+), 7 deletions(-)
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-05 11:07:09
Fillup ravb_rx_ring_free_gbeth() function to support RZ/G2L.
This patch also renames ravb_rx_ring_free to ravb_rx_ring_free_rcar
to be consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC Changes:
* moved "gbeth_rx_ring" to previous patch
* started using "gbeth_rx_ring" instead of gbeth_rx_ring[q].
* renamed ravb_rx_ring_free to ravb_rx_ring_free_rcar
---
drivers/net/ethernet/renesas/ravb_main.c | 28 ++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
@@ -236,10 +236,30 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)staticvoidravb_rx_ring_free_gbeth(structnet_device*ndev,intq){-/* Place holder */+structravb_private*priv=netdev_priv(ndev);+unsignedintring_size;+unsignedinti;++if(!priv->gbeth_rx_ring)+return;++for(i=0;i<priv->num_rx_ring[q];i++){+structravb_rx_desc*desc=&priv->gbeth_rx_ring[i];++if(!dma_mapping_error(ndev->dev.parent,+le32_to_cpu(desc->dptr)))+dma_unmap_single(ndev->dev.parent,+le32_to_cpu(desc->dptr),+GBETH_RX_BUFF_MAX,+DMA_FROM_DEVICE);+}+ring_size=sizeof(structravb_rx_desc)*(priv->num_rx_ring[q]+1);+dma_free_coherent(ndev->dev.parent,ring_size,priv->gbeth_rx_ring,+priv->rx_desc_dma[q]);+priv->gbeth_rx_ring=NULL;}-staticvoidravb_rx_ring_free(structnet_device*ndev,intq)+staticvoidravb_rx_ring_free_rcar(structnet_device*ndev,intq){structravb_private*priv=netdev_priv(ndev);unsignedintring_size;
@@ -2220,7 +2240,7 @@ static int ravb_mdio_release(struct ravb_private *priv)}staticconststructravb_hw_inforavb_gen3_hw_info={-.rx_ring_free=ravb_rx_ring_free,+.rx_ring_free=ravb_rx_ring_free_rcar,.rx_ring_format=ravb_rx_ring_format,.alloc_rx_desc=ravb_alloc_rx_desc_rcar,.receive=ravb_rcar_rx,
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-05 11:07:12
Fillup ravb_rx_ring_format_gbeth() function to support RZ/G2L.
This patch also renames ravb_rx_ring_format to ravb_rx_ring_format_rcar
to be consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC changes:
* Started using gbeth_rx_ring instead of gbeth_rx_ring[q].
* renamed ravb_rx_ring_format to ravb_rx_ring_format_rcar
---
drivers/net/ethernet/renesas/ravb.h | 1 +
drivers/net/ethernet/renesas/ravb_main.c | 34 +++++++++++++++++++++---
2 files changed, 31 insertions(+), 4 deletions(-)
@@ -327,10 +327,36 @@ static void ravb_ring_free(struct net_device *ndev, int q)staticvoidravb_rx_ring_format_gbeth(structnet_device*ndev,intq){-/* Place holder */+structravb_private*priv=netdev_priv(ndev);+structravb_rx_desc*rx_desc;+unsignedintrx_ring_size;+dma_addr_tdma_addr;+unsignedinti;++rx_ring_size=sizeof(*rx_desc)*priv->num_rx_ring[q];+memset(priv->gbeth_rx_ring,0,rx_ring_size);+/* Build RX ring buffer */+for(i=0;i<priv->num_rx_ring[q];i++){+/* RX descriptor */+rx_desc=&priv->gbeth_rx_ring[i];+rx_desc->ds_cc=cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);+dma_addr=dma_map_single(ndev->dev.parent,priv->rx_skb[q][i]->data,+GBETH_RX_BUFF_MAX,+DMA_FROM_DEVICE);+/* We just set the data size to 0 for a failed mapping which+*shouldpreventDMAfromhappening...+*/+if(dma_mapping_error(ndev->dev.parent,dma_addr))+rx_desc->ds_cc=cpu_to_le16(0);+rx_desc->dptr=cpu_to_le32(dma_addr);+rx_desc->die_dt=DT_FEMPTY;+}+rx_desc=&priv->gbeth_rx_ring[i];+rx_desc->dptr=cpu_to_le32((u32)priv->rx_desc_dma[q]);+rx_desc->die_dt=DT_LINKFIX;/* type */}-staticvoidravb_rx_ring_format(structnet_device*ndev,intq)+staticvoidravb_rx_ring_format_rcar(structnet_device*ndev,intq){structravb_private*priv=netdev_priv(ndev);structravb_ex_rx_desc*rx_desc;
@@ -2241,7 +2267,7 @@ static int ravb_mdio_release(struct ravb_private *priv)staticconststructravb_hw_inforavb_gen3_hw_info={.rx_ring_free=ravb_rx_ring_free_rcar,-.rx_ring_format=ravb_rx_ring_format,+.rx_ring_format=ravb_rx_ring_format_rcar,.alloc_rx_desc=ravb_alloc_rx_desc_rcar,.receive=ravb_rcar_rx,.set_rate=ravb_set_rate_rcar,
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-05 11:07:16
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC changes:
* renamed "rxtop_skb" to "rx_1st_skb" and moved near to rx_skb.
* removed parameter q from "ravb_get_skb_gbeth"
* renamed ravb_rcar_rx to ravb_rx_rcar
---
drivers/net/ethernet/renesas/ravb.h | 1 +
drivers/net/ethernet/renesas/ravb_main.c | 167 ++++++++++++++++++++++-
2 files changed, 163 insertions(+), 5 deletions(-)
@@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)}}+staticvoidravb_rx_csum_gbeth(structsk_buff*skb)+{+u8*hw_csum;++/* The hardware checksum is contained in sizeof(__sum16) (2) bytes+*appendedtopacketdata+*/+if(unlikely(skb->len<sizeof(__sum16)))+return;+hw_csum=skb_tail_pointer(skb)-sizeof(__sum16);++if(*hw_csum==0)+skb->ip_summed=CHECKSUM_UNNECESSARY;+else+skb->ip_summed=CHECKSUM_NONE;+}+staticvoidravb_rx_csum(structsk_buff*skb){u8*hw_csum;
@@ -735,15 +752,155 @@ static void ravb_rx_csum(struct sk_buff *skb)skb_trim(skb,skb->len-sizeof(__sum16));}+staticstructsk_buff*ravb_get_skb_gbeth(structnet_device*ndev,intentry,+structravb_rx_desc*desc)+{+structravb_private*priv=netdev_priv(ndev);+structsk_buff*skb;++skb=priv->rx_skb[RAVB_BE][entry];+priv->rx_skb[RAVB_BE][entry]=NULL;+dma_unmap_single(ndev->dev.parent,le32_to_cpu(desc->dptr),+ALIGN(GBETH_RX_BUFF_MAX,16),DMA_FROM_DEVICE);++returnskb;+}+/* Packet receive function for Gigabit Ethernet */staticboolravb_rx_gbeth(structnet_device*ndev,int*quota,intq){-/* Place holder */-returntrue;+structravb_private*priv=netdev_priv(ndev);+conststructravb_hw_info*info=priv->info;+structnet_device_stats*stats;+structravb_rx_desc*desc;+structsk_buff*skb;+dma_addr_tdma_addr;+u8desc_status;+intboguscnt;+u16pkt_len;+u8die_dt;+intentry;+intlimit;++entry=priv->cur_rx[q]%priv->num_rx_ring[q];+boguscnt=priv->dirty_rx[q]+priv->num_rx_ring[q]-priv->cur_rx[q];+stats=&priv->stats[q];++boguscnt=min(boguscnt,*quota);+limit=boguscnt;+desc=&priv->gbeth_rx_ring[entry];+while(desc->die_dt!=DT_FEMPTY){+/* Descriptor type must be checked before all other reads */+dma_rmb();+desc_status=desc->msc;+pkt_len=le16_to_cpu(desc->ds_cc)&RX_DS;++if(--boguscnt<0)+break;++/* We use 0-byte descriptors to mark the DMA mapping errors */+if(!pkt_len)+continue;++if(desc_status&MSC_MC)+stats->multicast++;++if(desc_status&(MSC_CRC|MSC_RFE|MSC_RTSF|MSC_RTLF|MSC_CEEF)){+stats->rx_errors++;+if(desc_status&MSC_CRC)+stats->rx_crc_errors++;+if(desc_status&MSC_RFE)+stats->rx_frame_errors++;+if(desc_status&(MSC_RTLF|MSC_RTSF))+stats->rx_length_errors++;+if(desc_status&MSC_CEEF)+stats->rx_missed_errors++;+}else{+die_dt=desc->die_dt&0xF0;+switch(die_dt){+caseDT_FSINGLE:+skb=ravb_get_skb_gbeth(ndev,entry,desc);+skb_put(skb,pkt_len);+skb->protocol=eth_type_trans(skb,ndev);+if(ndev->features&NETIF_F_RXCSUM)+ravb_rx_csum_gbeth(skb);+napi_gro_receive(&priv->napi[q],skb);+stats->rx_packets++;+stats->rx_bytes+=pkt_len;+break;+caseDT_FSTART:+priv->rx_1st_skb=ravb_get_skb_gbeth(ndev,entry,desc);+skb_put(priv->rx_1st_skb,pkt_len);+break;+caseDT_FMID:+skb=ravb_get_skb_gbeth(ndev,entry,desc);+skb_copy_to_linear_data_offset(priv->rx_1st_skb,+priv->rx_1st_skb->len,+skb->data,+pkt_len);+skb_put(priv->rx_1st_skb,pkt_len);+dev_kfree_skb(skb);+break;+caseDT_FEND:+skb=ravb_get_skb_gbeth(ndev,entry,desc);+skb_copy_to_linear_data_offset(priv->rx_1st_skb,+priv->rx_1st_skb->len,+skb->data,+pkt_len);+skb_put(priv->rx_1st_skb,pkt_len);+dev_kfree_skb(skb);+priv->rx_1st_skb->protocol=+eth_type_trans(priv->rx_1st_skb,ndev);+if(ndev->features&NETIF_F_RXCSUM)+ravb_rx_csum_gbeth(skb);+napi_gro_receive(&priv->napi[q],+priv->rx_1st_skb);+stats->rx_packets++;+stats->rx_bytes+=priv->rx_1st_skb->len;+break;+}+}++entry=(++priv->cur_rx[q])%priv->num_rx_ring[q];+desc=&priv->gbeth_rx_ring[entry];+}++/* Refill the RX ring buffers. */+for(;priv->cur_rx[q]-priv->dirty_rx[q]>0;priv->dirty_rx[q]++){+entry=priv->dirty_rx[q]%priv->num_rx_ring[q];+desc=&priv->gbeth_rx_ring[entry];+desc->ds_cc=cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);++if(!priv->rx_skb[q][entry]){+skb=netdev_alloc_skb(ndev,info->max_rx_len);+if(!skb)+break;+ravb_set_buffer_align(skb);+dma_addr=dma_map_single(ndev->dev.parent,+skb->data,+GBETH_RX_BUFF_MAX,+DMA_FROM_DEVICE);+skb_checksum_none_assert(skb);+/* We just set the data size to 0 for a failed mapping+*whichshouldpreventDMAfromhappening...+*/+if(dma_mapping_error(ndev->dev.parent,dma_addr))+desc->ds_cc=cpu_to_le16(0);+desc->dptr=cpu_to_le32(dma_addr);+priv->rx_skb[q][entry]=skb;+}+/* Descriptor type must be set after all the above writes */+dma_wmb();+desc->die_dt=DT_FEMPTY;+}++*quota-=limit-(++boguscnt);++returnboguscnt<=0;}/* Packet receive function for Ethernet AVB */-staticboolravb_rcar_rx(structnet_device*ndev,int*quota,intq)+staticboolravb_rx_rcar(structnet_device*ndev,int*quota,intq){structravb_private*priv=netdev_priv(ndev);conststructravb_hw_info*info=priv->info;
@@ -196,11 +196,15 @@ enum ravb_reg {MAHR=0x05c0,MALR=0x05c8,TROCR=0x0700,/* R-Car Gen3 and RZ/G2L only */+CXR41=0x0708,/* RZ/G2L only */+CXR42=0x0710,/* RZ/G2L only */CEFCR=0x0740,FRECR=0x0748,TSFRCR=0x0750,TLFRCR=0x0758,RFCR=0x0760,+CXR55=0x0768,/* RZ/G2L only */+CXR56=0x0770,/* RZ/G2L only */MAFCR=0x0778,CSR0=0x0800,/* RZ/G2L only */CSR1=0x0804,/* RZ/G2L only */
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-05 11:07:31
Rename the variable "tsrq" with "tccr_mask" as we are passing
TCCR mask to the ravb_wait() function.
There is no functional change.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Suggested-by: Sergey Shtylyov <redacted>
---
RFC changes:
* New patch.
---
drivers/net/ethernet/renesas/ravb.h | 2 +-
drivers/net/ethernet/renesas/ravb_main.c | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)
@@ -1043,7 +1043,7 @@ static int ravb_stop_dma(struct net_device *ndev)interror;/* Wait for stopping the hardware TX process */-error=ravb_wait(ndev,TCCR,info->tsrq,0);+error=ravb_wait(ndev,TCCR,info->tccr_mask,0);if(error)returnerror;
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-05 11:07:35
This patch update/add the following comments
1) Fix the typo AVB->DMAC in comment, as the code following the comment
is for GbEthernet DMAC in ravb_dmac_init_gbeth()
2) Update the comment "PAUSE prohibition"-> "EMAC Mode: PAUSE
prohibition; Duplex; TX; RX;" in ravb_emac_init_gbeth()
3) Document PFRI register bit, as it is only supported for
R-Car Gen3 and RZ/G2L.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
RFC changes:
* New patch.
---
drivers/net/ethernet/renesas/ravb.h | 2 +-
drivers/net/ethernet/renesas/ravb_main.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
@@ -588,7 +588,7 @@ static int ravb_dmac_init_gbeth(struct net_device *ndev)/* Descriptor format */ravb_ring_format(ndev,RAVB_BE);-/* Set AVB RX */+/* Set DMAC RX */ravb_write(ndev,0x60000000,RCR);/* Set Max Frame Length (RTC) */
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.
This patch series is aims to add functional support for Gigabit Ethernet driver
by filling all the stubs.
Ref:-
https://lore.kernel.org/linux-renesas-soc/OS0PR01MB5922240F88E5E0FD989ECDF386AC9@OS0PR01MB5922.jpnprd01.prod.outlook.com/T/#m8dee0a1b14d505d4611cad8c10e4017a30db55d6
RFC changes:
* used ALIGN macro for calculating the value for max_rx_len.
* used rx_max_buf_size instead of rx_2k_buffers feature bit.
* moved struct ravb_rx_desc *gbeth_rx_ring near to ravb_private::rx_ring
and allocating it for 1 RX queue.
* Started using gbeth_rx_ring instead of gbeth_rx_ring[q].
* renamed ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
* renamed ravb_rx_ring_free to ravb_rx_ring_free_rcar
* renamed ravb_rx_ring_format to ravb_rx_ring_format_rcar
* renamed ravb_rcar_rx to ravb_rx_rcar
* renamed "tsrq" variable
* Updated the comments
Biju Das (12):
ravb: Use ALIGN macro for max_rx_len
ravb: Add rx_max_buf_size to struct ravb_hw_info
ravb: Fillup ravb_set_features_gbeth() stub
ravb: Fillup ravb_alloc_rx_desc_gbeth() stub
ravb: Fillup ravb_rx_ring_free_gbeth() stub
ravb: Fillup ravb_rx_ring_format_gbeth() stub
ravb: Fillup ravb_rx_gbeth() stub
ravb: Add carrier_counters to struct ravb_hw_info
ravb: Add support to retrieve stats for GbEthernet
ravb: Rename "tsrq" variable
ravb: Optimize ravb_emac_init_gbeth function
ravb: Update/Add comments
drivers/net/ethernet/renesas/ravb.h | 51 +++-
drivers/net/ethernet/renesas/ravb_main.c | 349 +++++++++++++++++++++--
2 files changed, 367 insertions(+), 33 deletions(-)
I dodn;'t expect the patchset to be reposted so soon but I'll switch
to reviewing it insted of the previously posted 8-patch series...
MBR, Sergey
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-05 12:04:54
Hi Sergey,
Subject: Re: [RFC 00/12] Add functional support for Gigabit Ethernet
driver
On 10/5/21 2:06 PM, Biju Das wrote:
quoted
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.
This patch series is aims to add functional support for Gigabit
Ethernet driver by filling all the stubs.
Ref:-
https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
.kernel.org%2Flinux-renesas-soc%2FOS0PR01MB5922240F88E5E0FD989ECDF386A
C9%40OS0PR01MB5922.jpnprd01.prod.outlook.com%2FT%2F%23m8dee0a1b14d505d
4611cad8c10e4017a30db55d6&data=04%7C01%7Cbiju.das.jz%40bp.renesas.
com%7C880ddc38cf254b0a81fc08d987f6ea17%7C53d82571da1947e49cb4625a166a4
a2a%7C0%7C0%7C637690316835703147%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL
jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata
=WmbtErppjUTywkNet%2FtDKw9v5gqaqRlcHGjI3PZ1UN8%3D&reserved=0
RFC changes:
* used ALIGN macro for calculating the value for max_rx_len.
* used rx_max_buf_size instead of rx_2k_buffers feature bit.
* moved struct ravb_rx_desc *gbeth_rx_ring near to
ravb_private::rx_ring
quoted
and allocating it for 1 RX queue.
* Started using gbeth_rx_ring instead of gbeth_rx_ring[q].
* renamed ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
* renamed ravb_rx_ring_free to ravb_rx_ring_free_rcar
* renamed ravb_rx_ring_format to ravb_rx_ring_format_rcar
* renamed ravb_rcar_rx to ravb_rx_rcar
* renamed "tsrq" variable
* Updated the comments
Biju Das (12):
ravb: Use ALIGN macro for max_rx_len
ravb: Add rx_max_buf_size to struct ravb_hw_info
ravb: Fillup ravb_set_features_gbeth() stub
ravb: Fillup ravb_alloc_rx_desc_gbeth() stub
ravb: Fillup ravb_rx_ring_free_gbeth() stub
ravb: Fillup ravb_rx_ring_format_gbeth() stub
ravb: Fillup ravb_rx_gbeth() stub
ravb: Add carrier_counters to struct ravb_hw_info
ravb: Add support to retrieve stats for GbEthernet
ravb: Rename "tsrq" variable
ravb: Optimize ravb_emac_init_gbeth function
ravb: Update/Add comments
drivers/net/ethernet/renesas/ravb.h | 51 +++-
drivers/net/ethernet/renesas/ravb_main.c | 349
+++++++++++++++++++++--
2 files changed, 367 insertions(+), 33 deletions(-)
I dodn;'t expect the patchset to be reposted so soon but I'll switch to
reviewing it insted of the previously posted 8-patch series...
Use ALIGN macro for calculating the value for max_rx_len.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Suggested-by: Sergey Shtylyov <redacted>
R-Car AVB-DMAC has maximum 2K size on RX buffer, whereas on RZ/G2L
it is 8K. We need to allow for changing the MTU within the limit
of the maximum size of a descriptor.
Add a rx_max_buf_size variable to struct ravb_hw_info to handle
this difference.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Fillup ravb_set_features_gbeth() function to support RZ/G2L.
Also set the net_hw_features bits supported by GbEthernet
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Fillup ravb_alloc_rx_desc_gbeth() function to support RZ/G2L.
This patch also renames ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
to be consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Rename the variable "tsrq" with "tccr_mask" as we are passing
TCCR mask to the ravb_wait() function.
There is no functional change.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Suggested-by: Sergey Shtylyov <redacted>
This patch update/add the following comments
1) Fix the typo AVB->DMAC in comment, as the code following the comment
is for GbEthernet DMAC in ravb_dmac_init_gbeth()
; not needed at the end of the comment. :-)
2) Update the comment "PAUSE prohibition"-> "EMAC Mode: PAUSE
prohibition; Duplex; TX; RX;" in ravb_emac_init_gbeth()
3) Document PFRI register bit, as it is only supported for
R-Car Gen3 and RZ/G2L.
Not a good idea to do 3 different things in 1 patch... I know I said that (2) isn't worth
a separate patch but I meant that it shouldbe done as a part of a lrger ravb_emac_init_gbeth()
change. Sorry for not being clear enough...
From: Sergei Shtylyov <hidden> Date: 2021-10-05 19:41:40
On 10/5/21 2:06 PM, Biju Das wrote:
Fillup ravb_rx_ring_free_gbeth() function to support RZ/G2L.
This patch also renames ravb_rx_ring_free to ravb_rx_ring_free_rcar
to be consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Fillup ravb_rx_ring_format_gbeth() function to support RZ/G2L.
This patch also renames ravb_rx_ring_format to ravb_rx_ring_format_rcar
to be consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-06 06:50:58
Hi Sergey,
Subject: Re: [RFC 04/12] ravb: Fillup ravb_alloc_rx_desc_gbeth() stub
On 10/5/21 2:06 PM, Biju Das wrote:
quoted
Fillup ravb_alloc_rx_desc_gbeth() function to support RZ/G2L.
This patch also renames ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
to be consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
-static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
+static void *ravb_alloc_rx_desc_rcar(struct net_device *ndev, int q)
{
struct ravb_private *priv = netdev_priv(ndev);
unsigned int ring_size;
@@ -1085,16 +1092,25 @@ static int ravb_poll(struct napi_struct *napi,
int budget)
quoted
struct net_device *ndev = napi->dev;
struct ravb_private *priv = netdev_priv(ndev);
const struct ravb_hw_info *info = priv->info;
+ bool gptp = info->gptp || info->ccc_gac;
+ struct ravb_rx_desc *desc;
unsigned long flags;
int q = napi - priv->napi;
int mask = BIT(q);
int quota = budget;
+ unsigned int entry;
+ if (!gptp) {
+ entry = priv->cur_rx[q] % priv->num_rx_ring[q];
+ desc = &priv->gbeth_rx_ring[entry];
+ }
/* Processing RX Descriptor Ring */
/* Clear RX interrupt */
ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
- if (ravb_rx(ndev, "a, q))
- goto out;
+ if (gptp || desc->die_dt != DT_FEMPTY) {
+ if (ravb_rx(ndev, "a, q))
+ goto out;
+ }
Not sure I understand this new logic around the ravb_rx() call, care to
explain?
The code is simple.
If (gptp || --> means non gptp case that is Gbethernet
die_dt --> Descriptor thype
So basically the new logic is , on Gbethernet case, if descriptor is not empty, then process rx.
Regards,
Biju
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-06 06:53:32
Hi Sergei,
Subject: Re: [RFC 12/12] ravb: Update/Add comments
On 10/5/21 2:06 PM, Biju Das wrote:
quoted
This patch update/add the following comments
1) Fix the typo AVB->DMAC in comment, as the code following the comment
is for GbEthernet DMAC in ravb_dmac_init_gbeth()
; not needed at the end of the comment. :-)
quoted
2) Update the comment "PAUSE prohibition"-> "EMAC Mode: PAUSE
prohibition; Duplex; TX; RX;" in ravb_emac_init_gbeth()
3) Document PFRI register bit, as it is only supported for
R-Car Gen3 and RZ/G2L.
Not a good idea to do 3 different things in 1 patch... I know I said
that (2) isn't worth a separate patch but I meant that it shouldbe done as
a part of a lrger ravb_emac_init_gbeth() change. Sorry for not being clear
enough...
We are improving comments on Gbethernet driver, so I thought 1 patch will address
All the comments, since there is no functional change.
OK will create 3 separate patches for fixing these comments.
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-06 07:44:03
Hi Sergei,
Thanks for the feedback.
Subject: Re: [RFC 03/12] ravb: Fillup ravb_set_features_gbeth() stub
On 10/5/21 2:06 PM, Biju Das wrote:
quoted
Fillup ravb_set_features_gbeth() function to support RZ/G2L.
Also set the net_hw_features bits supported by GbEthernet
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
RZ/G2L E-MAC supports carrier counters.
Add a carrier_counter hw feature bit to struct ravb_hw_info
to add this feature only for RZ/G2L.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Sergey Shtylyov <redacted>
RZ/G2L E-MAC supports carrier counters.
Add a carrier_counter hw feature bit to struct ravb_hw_info
to add this feature only for RZ/G2L.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Sergey Shtylyov <redacted>
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-06 17:21:55
Hi Sergey,
Thanks for the feedback.
quoted hunk
Subject: Re: [RFC 08/12] ravb: Add carrier_counters to struct ravb_hw_info
On 10/5/21 2:06 PM, Biju Das wrote:
quoted
RZ/G2L E-MAC supports carrier counters.
Add a carrier_counter hw feature bit to struct ravb_hw_info to add
this feature only for RZ/G2L.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Sergey Shtylyov <redacted>
[...]
diff --git a/drivers/net/ethernet/renesas/ravb.h
b/drivers/net/ethernet/renesas/ravb.h
index 8c7b2569c7dd..899e16c5eb1a 100644
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-06 17:22:53
Hi Sergey,
Subject: Re: [RFC 08/12] ravb: Add carrier_counters to struct ravb_hw_info
On 10/6/21 7:41 PM, Sergey Shtylyov wrote:
quoted
quoted
RZ/G2L E-MAC supports carrier counters.
Add a carrier_counter hw feature bit to struct ravb_hw_info to add
this feature only for RZ/G2L.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Sergey Shtylyov <redacted>
[...]
quoted
diff --git a/drivers/net/ethernet/renesas/ravb.h
b/drivers/net/ethernet/renesas/ravb.h
index 8c7b2569c7dd..899e16c5eb1a 100644
Add support for retrieving stats information for GbEthernet.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-06 20:22:05
Hi Sergey,
Thanks for thefeedback.
Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
On 10/5/21 2:06 PM, Biju Das wrote:
quoted
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be consistent
with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]
So the TCP/UDP/ICMP checksums are not dealt with? Why enable them then?
If last 2bytes is zero, means there is no checksum error w.r.to TCP/UDP/ICMP checksums.
RZ/G2L checksum part is different from R-Car Gen3. There is no TOE block at all for R-Car Gen3.
Regards,
Biju
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be consistent
with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-07 05:49:17
Hi Sergey,
Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
On 10/6/21 11:22 PM, Biju Das wrote:
[...]
quoted
quoted
quoted
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]
}
}
+static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
+ u8 *hw_csum;
+
+ /* The hardware checksum is contained in sizeof(__sum16) (2) bytes
+ * appended to packet data
+ */
+ if (unlikely(skb->len < sizeof(__sum16)))
+ return;
+ hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
Not 32-bit? The manual says the IP checksum is stored in the first
2 bytes.
It is 16 bit. It is on last 2 bytes.
So you're saying the manual is wrong?
I am not sure which manual you are referring here.
I am referring to Rev.1.00 Sep, 2021 of RZ/G2L hardware manual and
I have shared the link[1] for you to download. Hope you are referring same manual
[1] https://www.renesas.com/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0?language=en&r=1467981
Please check the section 30.5.6.1 checksum calculation handling
And figure 30.25 the field of checksum attaching field
Also see Table 30.17 for checksum values for non-error conditions.
TCP/UDP/ICPM checksum is at last 2bytes.
quoted
quoted
quoted
+
+ if (*hw_csum == 0)
You only check the 1st byte, not the full checksum!
As I said earlier, "0" value on last 16 bit, means no checksum error.
How's that? 'hw_csum' is declared as 'u8 *'!
It is my mistake, which will be taken care in the next patch by using u16 *.
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]
}
}
+static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
+ u8 *hw_csum;
+
+ /* The hardware checksum is contained in sizeof(__sum16) (2) bytes
+ * appended to packet data
+ */
+ if (unlikely(skb->len < sizeof(__sum16)))
+ return;
+ hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
Not 32-bit? The manual says the IP checksum is stored in the first
2 bytes.
It is 16 bit. It is on last 2 bytes.
The IP checksum is at the 1st 2 bytes of the overall 4-byte checksum (coming after
the packet payload), no?
quoted
So you're saying the manual is wrong?
I am not sure which manual you are referring here.
I am referring to Rev.1.00 Sep, 2021 of RZ/G2L hardware manual and
Same here.
[...]
Please check the section 30.5.6.1 checksum calculation handling> And figure 30.25 the field of checksum attaching field
I have.
Also see Table 30.17 for checksum values for non-error conditions.
TCP/UDP/ICPM checksum is at last 2bytes.
What are you arguing with then? :-)
My point was that your code fetched the TCP/UDP/ICMP checksum ISO the IP checksum
because it subtracts sizeof(__sum16), while should probably subtract sizeof(__wsum).
quoted
quoted
quoted
quoted
+
+ if (*hw_csum == 0)
You only check the 1st byte, not the full checksum!
As I said earlier, "0" value on last 16 bit, means no checksum error.
How's that? 'hw_csum' is declared as 'u8 *'!
It is my mistake, which will be taken care in the next patch by using u16 *.
Note that this 'u16' halfword can be unaligned, that's why the current code uses get_unaligned_le16().
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-07 20:09:32
Hi Sergey,
Thanks for the feedback.
Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
On 10/7/21 8:49 AM, Biju Das wrote:
[...]
quoted
quoted
quoted
quoted
quoted
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]
}
}
+static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
+ u8 *hw_csum;
+
+ /* The hardware checksum is contained in sizeof(__sum16) (2)
bytes
quoted
quoted
quoted
quoted
quoted
+ * appended to packet data
+ */
+ if (unlikely(skb->len < sizeof(__sum16)))
+ return;
+ hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
Not 32-bit? The manual says the IP checksum is stored in the
first
2 bytes.
It is 16 bit. It is on last 2 bytes.
The IP checksum is at the 1st 2 bytes of the overall 4-byte checksum
(coming after the packet payload), no?
Sorry, I got confused with your question earlier. Now it is clear for me.
I agree the checksum part is stored in last 4bytes. Of this, the first 2 bytes IPV4 checksum
and last 2 bytes TCP/UDP/ICMP checksum.
quoted
quoted
So you're saying the manual is wrong?
I am not sure which manual you are referring here.
I am referring to Rev.1.00 Sep, 2021 of RZ/G2L hardware manual and
Same here.
[...]
quoted
Please check the section 30.5.6.1 checksum calculation handling> And
figure 30.25 the field of checksum attaching field
I have.
quoted
Also see Table 30.17 for checksum values for non-error conditions.
quoted
TCP/UDP/ICPM checksum is at last 2bytes.
What are you arguing with then? :-)
My point was that your code fetched the TCP/UDP/ICMP checksum ISO the
IP checksum because it subtracts sizeof(__sum16), while should probably
subtract sizeof(__wsum)
Agreed. My code missed IP4 checksum result. May be we need to extract 2 checksum info
from last 4 bytes. First checksum(2bytes) is IP4 header checksum and next checksum(2 bytes) for TCP/UDP/ICMP and use this info finding the non error case mentioned in Table 30.17.
For eg:-
IPV6 non error-condition --> "0xFFFF"-->IPV4HeaderCSum value and "0x0000" TCP/UDP/ICMP CSUM value
IPV4 non error-condition --> "0x0000"-->IPV4HeaderCSum value and "0x0000" TCP/UDP/ICMP CSUM value
Do you agree?
Regards,
Biju
quoted
quoted
quoted
quoted
quoted
+
+ if (*hw_csum == 0)
You only check the 1st byte, not the full checksum!
As I said earlier, "0" value on last 16 bit, means no checksum error.
How's that? 'hw_csum' is declared as 'u8 *'!
It is my mistake, which will be taken care in the next patch by using
u16 *.
Note that this 'u16' halfword can be unaligned, that's why the current
code uses get_unaligned_le16().
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-08 06:46:40
Hi Sergey,
Subject: RE: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
Hi Sergey,
Thanks for the feedback.
quoted
Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
On 10/7/21 8:49 AM, Biju Das wrote:
[...]
quoted
quoted
quoted
quoted
quoted
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]
}
}
+static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
+ u8 *hw_csum;
+
+ /* The hardware checksum is contained in sizeof(__sum16) (2)
bytes
quoted
quoted
quoted
quoted
quoted
+ * appended to packet data
+ */
+ if (unlikely(skb->len < sizeof(__sum16)))
+ return;
+ hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
Not 32-bit? The manual says the IP checksum is stored in the
first
2 bytes.
It is 16 bit. It is on last 2 bytes.
The IP checksum is at the 1st 2 bytes of the overall 4-byte
checksum (coming after the packet payload), no?
Sorry, I got confused with your question earlier. Now it is clear for me.
I agree the checksum part is stored in last 4bytes. Of this, the first 2
bytes IPV4 checksum and last 2 bytes TCP/UDP/ICMP checksum.
quoted
quoted
quoted
So you're saying the manual is wrong?
I am not sure which manual you are referring here.
I am referring to Rev.1.00 Sep, 2021 of RZ/G2L hardware manual and
Same here.
[...]
quoted
Please check the section 30.5.6.1 checksum calculation handling> And
figure 30.25 the field of checksum attaching field
I have.
quoted
Also see Table 30.17 for checksum values for non-error conditions.
quoted
TCP/UDP/ICPM checksum is at last 2bytes.
What are you arguing with then? :-)
My point was that your code fetched the TCP/UDP/ICMP checksum ISO
the IP checksum because it subtracts sizeof(__sum16), while should
probably subtract sizeof(__wsum)
Agreed. My code missed IP4 checksum result. May be we need to extract 2
checksum info from last 4 bytes. First checksum(2bytes) is IP4 header
checksum and next checksum(2 bytes) for TCP/UDP/ICMP and use this info
finding the non error case mentioned in Table 30.17.
For eg:-
IPV6 non error-condition --> "0xFFFF"-->IPV4HeaderCSum value and "0x0000"
TCP/UDP/ICMP CSUM value
IPV4 non error-condition --> "0x0000"-->IPV4HeaderCSum value and "0x0000"
TCP/UDP/ICMP CSUM value
Do you agree?
What I meant here is some thing like below, please let me know if you have any issues with
this, otherwise I would like to send the patch with below changes.
Further improvements can happen later.
Please let me know.
+/* Hardware checksum status */
+#define IPV4_RX_CSUM_OK 0x00000000
+#define IPV6_RX_CSUM_OK 0xFFFF0000
+
enum ravb_reg {
/* AVB-DMAC registers */
CCC = 0x0000,
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]
}
}
+static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
+ u8 *hw_csum;
+
+ /* The hardware checksum is contained in sizeof(__sum16) (2)
bytes
quoted
quoted
quoted
quoted
quoted
+ * appended to packet data
+ */
+ if (unlikely(skb->len < sizeof(__sum16)))
+ return;
+ hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
[...]
quoted
quoted
quoted
Please check the section 30.5.6.1 checksum calculation handling> And
figure 30.25 the field of checksum attaching field
I have.
quoted
Also see Table 30.17 for checksum values for non-error conditions.
quoted
TCP/UDP/ICPM checksum is at last 2bytes.
What are you arguing with then? :-)
My point was that your code fetched the TCP/UDP/ICMP checksum ISO
the IP checksum because it subtracts sizeof(__sum16), while should
probably subtract sizeof(__wsum)
Agreed. My code missed IP4 checksum result. May be we need to extract 2
checksum info from last 4 bytes. First checksum(2bytes) is IP4 header
checksum and next checksum(2 bytes) for TCP/UDP/ICMP and use this info
finding the non error case mentioned in Table 30.17.
For eg:-
IPV6 non error-condition --> "0xFFFF"-->IPV4HeaderCSum value and "0x0000"
TCP/UDP/ICMP CSUM value
IPV4 non error-condition --> "0x0000"-->IPV4HeaderCSum value and "0x0000"
TCP/UDP/ICMP CSUM value
Do you agree?
What I meant here is some thing like below, please let me know if you have any issues with
this, otherwise I would like to send the patch with below changes.
Further improvements can happen later.
Please let me know.
+/* Hardware checksum status */
+#define IPV4_RX_CSUM_OK 0x00000000
+#define IPV6_RX_CSUM_OK 0xFFFF0000
Mhm, this should prolly come from the IP headers...
[...]
This is not against the patch currently under investigation. :-)
+ u8 *hw_csum;
/* The hardware checksum is contained in sizeof(__sum16) (2) bytes
* appended to packet data
*/
- if (unlikely(skb->len < sizeof(__sum16)))
+ if (unlikely(skb->len < sizeof(__wsum)))
I think this usage of __wsum is valid (I remember that I suggested it). We have 2 16-bit checksums here
covered by that, not a 32-bit sum...
I don't think there's a hard-and-fast way to differentiate the valid packet just from
the 2 16-bit checksums...
[...]
quoted
quoted
quoted
quoted
quoted
quoted
quoted
+
+ if (*hw_csum == 0)
You only check the 1st byte, not the full checksum!
As I said earlier, "0" value on last 16 bit, means no checksum
error.
quoted
quoted
quoted
How's that? 'hw_csum' is declared as 'u8 *'!
It is my mistake, which will be taken care in the next patch by
using
u16 *.
That won't do it, I'm afraid...
From an IRC discuassion on IRC we concluded that we don't need to check the checksum's
value, we just need to store it for the upper layers to catch the invalid sums...
[...]
MBR, Sergey
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]
}
}
+static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
+ u8 *hw_csum;
+
+ /* The hardware checksum is contained in sizeof(__sum16) (2)
bytes
quoted
quoted
quoted
quoted
quoted
+ * appended to packet data
+ */
+ if (unlikely(skb->len < sizeof(__sum16)))
+ return;
+ hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
[...]
quoted
quoted
quoted
quoted
Please check the section 30.5.6.1 checksum calculation handling> And
figure 30.25 the field of checksum attaching field
I have.
quoted
Also see Table 30.17 for checksum values for non-error conditions.
quoted
TCP/UDP/ICPM checksum is at last 2bytes.
What are you arguing with then? :-)
My point was that your code fetched the TCP/UDP/ICMP checksum ISO
the IP checksum because it subtracts sizeof(__sum16), while should
probably subtract sizeof(__wsum)
Agreed. My code missed IP4 checksum result. May be we need to extract 2
checksum info from last 4 bytes. First checksum(2bytes) is IP4 header
checksum and next checksum(2 bytes) for TCP/UDP/ICMP and use this info
finding the non error case mentioned in Table 30.17.
For eg:-
IPV6 non error-condition --> "0xFFFF"-->IPV4HeaderCSum value and "0x0000"
TCP/UDP/ICMP CSUM value
IPV4 non error-condition --> "0x0000"-->IPV4HeaderCSum value and "0x0000"
TCP/UDP/ICMP CSUM value
Do you agree?
quoted
What I meant here is some thing like below, please let me know if you have any issues with
this, otherwise I would like to send the patch with below changes.
Further improvements can happen later.
Please let me know.
+/* Hardware checksum status */
+#define IPV4_RX_CSUM_OK 0x00000000
+#define IPV6_RX_CSUM_OK 0xFFFF0000
Mhm, this should prolly come from the IP headers...
[...]
This is not against the patch currently under investigation. :-)
quoted
+ u8 *hw_csum;
/* The hardware checksum is contained in sizeof(__sum16) (2) bytes
* appended to packet data
*/
- if (unlikely(skb->len < sizeof(__sum16)))
+ if (unlikely(skb->len < sizeof(__wsum)))
I think this usage of __wsum is valid (I remember that I suggested it). We have 2 16-bit checksums here
I meant "I don't think", of course. :-)
[...]
MBR, Sergey
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]
}
}
+static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
+ u8 *hw_csum;
+
+ /* The hardware checksum is contained in sizeof(__sum16) (2)
bytes
quoted
quoted
quoted
quoted
quoted
+ * appended to packet data
+ */
+ if (unlikely(skb->len < sizeof(__sum16)))
+ return;
+ hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
[...]
quoted
quoted
quoted
quoted
Please check the section 30.5.6.1 checksum calculation handling>
And figure 30.25 the field of checksum attaching field
I have.
quoted
Also see Table 30.17 for checksum values for non-error conditions.
quoted
TCP/UDP/ICPM checksum is at last 2bytes.
What are you arguing with then? :-)
My point was that your code fetched the TCP/UDP/ICMP checksum
ISO the IP checksum because it subtracts sizeof(__sum16), while
should probably subtract sizeof(__wsum)
Agreed. My code missed IP4 checksum result. May be we need to
extract 2 checksum info from last 4 bytes. First checksum(2bytes)
is IP4 header checksum and next checksum(2 bytes) for TCP/UDP/ICMP
and use this info finding the non error case mentioned in Table
30.17.
quoted
quoted
quoted
For eg:-
IPV6 non error-condition --> "0xFFFF"-->IPV4HeaderCSum value and
"0x0000"
quoted
quoted
quoted
TCP/UDP/ICMP CSUM value
IPV4 non error-condition --> "0x0000"-->IPV4HeaderCSum value and
"0x0000"
quoted
quoted
quoted
TCP/UDP/ICMP CSUM value
Do you agree?
quoted
What I meant here is some thing like below, please let me know if you
have any issues with this, otherwise I would like to send the patch
with below changes.
quoted
quoted
Further improvements can happen later.
Please let me know.
+/* Hardware checksum status */
+#define IPV4_RX_CSUM_OK 0x00000000
+#define IPV6_RX_CSUM_OK 0xFFFF0000
Mhm, this should prolly come from the IP headers...
[...]
This is not against the patch currently under investigation. :-)
quoted
+ u8 *hw_csum;
/* The hardware checksum is contained in sizeof(__sum16) (2)
bytes
quoted
quoted
* appended to packet data
*/
- if (unlikely(skb->len < sizeof(__sum16)))
+ if (unlikely(skb->len < sizeof(__wsum)))
I think this usage of __wsum is valid (I remember that I suggested
it). We have 2 16-bit checksums here
I meant "I don't think", of course. :-)
Ok will use 2 * sizeof(__sum16) instead and extract IPV4 header csum and TCP/UDP/ICMP csum result.
All error condition/unsupported cases will be passed to stack with CHECKSUM_NONE
and only non-error cases will be set as CHECKSUM_UNNCESSARY.
Does it sounds good to you?
Regards,
Biju
From: Sergei Shtylyov <hidden> Date: 2021-10-09 08:34:52
On 09.10.2021 11:27, Biju Das wrote:
quoted
quoted
[...]
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]
}
}
+static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
+ u8 *hw_csum;
+
+ /* The hardware checksum is contained in sizeof(__sum16) (2)
bytes
quoted
quoted
quoted
quoted
quoted
+ * appended to packet data
+ */
+ if (unlikely(skb->len < sizeof(__sum16)))
+ return;
+ hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
[...]
quoted
quoted
quoted
quoted
Please check the section 30.5.6.1 checksum calculation handling>
And figure 30.25 the field of checksum attaching field
I have.
quoted
Also see Table 30.17 for checksum values for non-error conditions.
quoted
TCP/UDP/ICPM checksum is at last 2bytes.
What are you arguing with then? :-)
My point was that your code fetched the TCP/UDP/ICMP checksum
ISO the IP checksum because it subtracts sizeof(__sum16), while
should probably subtract sizeof(__wsum)
Agreed. My code missed IP4 checksum result. May be we need to
extract 2 checksum info from last 4 bytes. First checksum(2bytes)
is IP4 header checksum and next checksum(2 bytes) for TCP/UDP/ICMP
and use this info finding the non error case mentioned in Table
30.17.
quoted
quoted
quoted
For eg:-
IPV6 non error-condition --> "0xFFFF"-->IPV4HeaderCSum value and
"0x0000"
quoted
quoted
quoted
TCP/UDP/ICMP CSUM value
IPV4 non error-condition --> "0x0000"-->IPV4HeaderCSum value and
"0x0000"
quoted
quoted
quoted
TCP/UDP/ICMP CSUM value
Do you agree?
quoted
What I meant here is some thing like below, please let me know if you
have any issues with this, otherwise I would like to send the patch
with below changes.
quoted
quoted
Further improvements can happen later.
Please let me know.
+/* Hardware checksum status */
+#define IPV4_RX_CSUM_OK 0x00000000
+#define IPV6_RX_CSUM_OK 0xFFFF0000
Mhm, this should prolly come from the IP headers...
[...]
This is not against the patch currently under investigation. :-)
quoted
+ u8 *hw_csum;
/* The hardware checksum is contained in sizeof(__sum16) (2)
bytes
quoted
quoted
* appended to packet data
*/
- if (unlikely(skb->len < sizeof(__sum16)))
+ if (unlikely(skb->len < sizeof(__wsum)))
I think this usage of __wsum is valid (I remember that I suggested
it). We have 2 16-bit checksums here
I meant "I don't think", of course. :-)
Ok will use 2 * sizeof(__sum16) instead and extract IPV4 header csum and TCP/UDP/ICMP csum result.
I'm not sure how to deal with the later...
All error condition/unsupported cases will be passed to stack with CHECKSUM_NONE
and only non-error cases will be set as CHECKSUM_UNNCESSARY.
Does it sounds good to you?
No. The networking stack needs to know about the bad checksums too.
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-09 09:41:55
Hi Sergey,
Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
On 09.10.2021 11:27, Biju Das wrote:
quoted
quoted
quoted
[...]
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Fillup ravb_rx_gbeth() function to support RZ/G2L.
This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]
}
}
+static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
+ u8 *hw_csum;
+
+ /* The hardware checksum is contained in sizeof(__sum16)
+(2)
bytes
quoted
quoted
quoted
quoted
quoted
+ * appended to packet data
+ */
+ if (unlikely(skb->len < sizeof(__sum16)))
+ return;
+ hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
[...]
quoted
quoted
quoted
quoted
Please check the section 30.5.6.1 checksum calculation handling>
And figure 30.25 the field of checksum attaching field
I have.
quoted
Also see Table 30.17 for checksum values for non-error conditions.
quoted
TCP/UDP/ICPM checksum is at last 2bytes.
What are you arguing with then? :-)
My point was that your code fetched the TCP/UDP/ICMP checksum
ISO the IP checksum because it subtracts sizeof(__sum16), while
should probably subtract sizeof(__wsum)
Agreed. My code missed IP4 checksum result. May be we need to
extract 2 checksum info from last 4 bytes. First checksum(2bytes)
is IP4 header checksum and next checksum(2 bytes) for
TCP/UDP/ICMP and use this info finding the non error case
mentioned in Table
30.17.
quoted
quoted
quoted
For eg:-
IPV6 non error-condition --> "0xFFFF"-->IPV4HeaderCSum value and
"0x0000"
quoted
quoted
quoted
TCP/UDP/ICMP CSUM value
IPV4 non error-condition --> "0x0000"-->IPV4HeaderCSum value and
"0x0000"
quoted
quoted
quoted
TCP/UDP/ICMP CSUM value
Do you agree?
quoted
What I meant here is some thing like below, please let me know if
you have any issues with this, otherwise I would like to send the
patch
with below changes.
quoted
quoted
Further improvements can happen later.
Please let me know.
+/* Hardware checksum status */
+#define IPV4_RX_CSUM_OK 0x00000000
+#define IPV6_RX_CSUM_OK 0xFFFF0000
Mhm, this should prolly come from the IP headers...
[...]
From: Biju Das <biju.das.jz@bp.renesas.com> Date: 2021-10-09 17:53:36
-----Original Message-----
From: Biju Das <biju.das.jz@bp.renesas.com>
Sent: 06 October 2021 08:44
To: Sergey Shtylyov <redacted>; David S. Miller
[off-list ref]; Jakub Kicinski [off-list ref]
Cc: Geert Uytterhoeven <geert+renesas@glider.be>; Sergey Shtylyov
[off-list ref]; Adam Ford [off-list ref]; Andrew Lunn
[off-list ref]; Yuusuke Ashizuka [off-list ref]; Yoshihiro
Shimoda [off-list ref]; netdev@vger.kernel.org; linux-
renesas-soc@vger.kernel.org; Chris Paterson [off-list ref];
Biju Das [off-list ref]; Prabhakar Mahadev Lad
[off-list ref]
Subject: RE: [RFC 03/12] ravb: Fillup ravb_set_features_gbeth() stub
Hi Sergei,
Thanks for the feedback.
quoted
Subject: Re: [RFC 03/12] ravb: Fillup ravb_set_features_gbeth() stub
On 10/5/21 2:06 PM, Biju Das wrote:
quoted
Fillup ravb_set_features_gbeth() function to support RZ/G2L.
Also set the net_hw_features bits supported by GbEthernet
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Hm, the >linux/netdev_features.h> says those are contradictory to
have both NETIF_F_HW_CSUM and NETIF_F_CSUM_MASK set...
It is a mistake from my side, I am taking out this setting. Any way below
code overrides it.
This will answer all your comments below.
I am deferring this patch and will take out RX checksum offload functionality from patch#7
Will post this 2 patches as RFC, as looks like it needs more discussions related to HW checksum.
Regards,
Biju