| Message ID | 20230727112827.3977534-1-danishanwar@ti.com |
|---|---|
| Headers | show |
| Series | Introduce ICSSG based ethernet Driver | expand |
On Thu, 27 Jul 2023 16:58:23 +0530 MD Danish Anwar wrote: > +static int emac_tx_complete_packets(struct prueth_emac *emac, int chn, > + int budget) > +{ > + struct net_device *ndev = emac->ndev; > + struct cppi5_host_desc_t *desc_tx; > + struct netdev_queue *netif_txq; > + struct prueth_tx_chn *tx_chn; > + unsigned int total_bytes = 0; > + struct sk_buff *skb; > + dma_addr_t desc_dma; > + int res, num_tx = 0; > + void **swdata; > + > + tx_chn = &emac->tx_chns[chn]; > + > + while (budget) { > + res = k3_udma_glue_pop_tx_chn(tx_chn->tx_chn, &desc_dma); > + if (res == -ENODATA) > + break; You shouldn't limit the number of serviced packets to budget for Tx NAPI. https://docs.kernel.org/next/networking/napi.html#driver-api > + skb->dev = ndev; > + if (!netif_running(skb->dev)) { > + dev_kfree_skb_any(skb); > + return 0; > + } why do you check if the interface is running? If a packet arrives, it means the interface is running.. > +drop_free_descs: > + prueth_xmit_free(tx_chn, first_desc); > +drop_stop_q: > + netif_tx_stop_queue(netif_txq); Do not stop the queue on DMA errors. If the queue is empty nothing will wake it up. Queue should only be stopped based on occupancy. > + dev_kfree_skb_any(skb); > + > + /* error */ > + ndev->stats.tx_dropped++; > + netdev_err(ndev, "tx: error: %d\n", ret); > + > + return ret;
Hi Jakub, On 29/07/23 5:54 am, Jakub Kicinski wrote: > On Thu, 27 Jul 2023 16:58:23 +0530 MD Danish Anwar wrote: >> +static int emac_tx_complete_packets(struct prueth_emac *emac, int chn, >> + int budget) >> +{ >> + struct net_device *ndev = emac->ndev; >> + struct cppi5_host_desc_t *desc_tx; >> + struct netdev_queue *netif_txq; >> + struct prueth_tx_chn *tx_chn; >> + unsigned int total_bytes = 0; >> + struct sk_buff *skb; >> + dma_addr_t desc_dma; >> + int res, num_tx = 0; >> + void **swdata; >> + >> + tx_chn = &emac->tx_chns[chn]; >> + >> + while (budget) { >> + res = k3_udma_glue_pop_tx_chn(tx_chn->tx_chn, &desc_dma); >> + if (res == -ENODATA) >> + break; > > You shouldn't limit the number of serviced packets to budget for Tx > NAPI. > > https://docs.kernel.org/next/networking/napi.html#driver-api > >> + skb->dev = ndev; >> + if (!netif_running(skb->dev)) { >> + dev_kfree_skb_any(skb); >> + return 0; >> + } > > why do you check if the interface is running? > If a packet arrives, it means the interface is running.. > Sure, I will drop this if condition. >> +drop_free_descs: >> + prueth_xmit_free(tx_chn, first_desc); >> +drop_stop_q: >> + netif_tx_stop_queue(netif_txq); > > Do not stop the queue on DMA errors. If the queue is empty nothing > will wake it up. Queue should only be stopped based on occupancy. > Sure, I will try to make sure we only stop queue based on occupancy. >> + dev_kfree_skb_any(skb); >> + >> + /* error */ >> + ndev->stats.tx_dropped++; >> + netdev_err(ndev, "tx: error: %d\n", ret); >> + >> + return ret;
Hi Jakub, On 29/07/23 5:54 am, Jakub Kicinski wrote: > On Thu, 27 Jul 2023 16:58:23 +0530 MD Danish Anwar wrote: >> +static int emac_tx_complete_packets(struct prueth_emac *emac, int chn, >> + int budget) >> +{ >> + struct net_device *ndev = emac->ndev; >> + struct cppi5_host_desc_t *desc_tx; >> + struct netdev_queue *netif_txq; >> + struct prueth_tx_chn *tx_chn; >> + unsigned int total_bytes = 0; >> + struct sk_buff *skb; >> + dma_addr_t desc_dma; >> + int res, num_tx = 0; >> + void **swdata; >> + >> + tx_chn = &emac->tx_chns[chn]; >> + >> + while (budget) { >> + res = k3_udma_glue_pop_tx_chn(tx_chn->tx_chn, &desc_dma); >> + if (res == -ENODATA) >> + break; > > You shouldn't limit the number of serviced packets to budget for Tx > NAPI. > > https://docs.kernel.org/next/networking/napi.html#driver-api Sure, I will remove budget from here, instead will service packets in while(true) {} loop. > >> + skb->dev = ndev; >> + if (!netif_running(skb->dev)) { >> + dev_kfree_skb_any(skb); >> + return 0; >> + } > > why do you check if the interface is running? > If a packet arrives, it means the interface is running.. > >> +drop_free_descs: >> + prueth_xmit_free(tx_chn, first_desc); >> +drop_stop_q: >> + netif_tx_stop_queue(netif_txq); > > Do not stop the queue on DMA errors. If the queue is empty nothing > will wake it up. Queue should only be stopped based on occupancy. There are five error handling cases in xmit(). 1. DMA Mapping the linear buffer -- If we fail to map dma, we will return NETDEV_TX_OK and goto drop_free_skb which will free the skb and drop the packet. 2. Allocating descriptor for linear buffer -- If we fail to allocate descriptor this means it is a occupancy issue and we will goto drop_stop_q_busy which will stop queue and return NETDEV_TX_BUSY. 3. Allocating descriptor when skb is fragmented. -- If we fail to allocate descriptor when skb is fragmented, we will goto drop_stop_q which will stop the queue, free the descriptor, free the skb, drop the packet and return NETDEV_TX_OK. 4. DMA mapping for fragment. -- If DMA mapping for fragment fails, we will go to drop_free_descs which will free the descriptor, free the skb, drop the packet and return NETDEV_TX_OK. 5. Tx push failed. -- If tx push fails we will goto drop_free_descs which will free the descriptor, free the skb, drop the packet and return. We will only stop queue in case 2 and 3 where we failed to allocate descriptor. In case 1, 4 and 5 we are encountering dma mapping error, so for these cases we will not stop the queue. Below will be my goto labels. drop_stop_q: netif_tx_stop_queue(netif_txq); drop_free_descs: prueth_xmit_free(tx_chn, first_desc); drop_free_skb: dev_kfree_skb_any(skb); /* error */ ndev->stats.tx_dropped++; netdev_err(ndev, "tx: error: %d\n", ret); return ret; drop_stop_q_busy: netif_tx_stop_queue(netif_txq); return NETDEV_TX_BUSY; Please let me know if this looks OK or if you have any comment on this. > >> + dev_kfree_skb_any(skb); >> + >> + /* error */ >> + ndev->stats.tx_dropped++; >> + netdev_err(ndev, "tx: error: %d\n", ret); >> + >> + return ret;
On Mon, 31 Jul 2023 16:49:59 +0530 Md Danish Anwar wrote: > There are five error handling cases in xmit(). > > 1. DMA Mapping the linear buffer -- If we fail to map dma, we will return > NETDEV_TX_OK and goto drop_free_skb which will free the skb and drop the packet. > > 2. Allocating descriptor for linear buffer -- If we fail to allocate descriptor > this means it is a occupancy issue and we will goto drop_stop_q_busy which will > stop queue and return NETDEV_TX_BUSY. > > 3. Allocating descriptor when skb is fragmented. -- If we fail to allocate > descriptor when skb is fragmented, we will goto drop_stop_q which will stop the > queue, free the descriptor, free the skb, drop the packet and return NETDEV_TX_OK. This one should be BUSY, right? goto free_desc_stop_q_busy > 4. DMA mapping for fragment. -- If DMA mapping for fragment fails, we will go > to drop_free_descs which will free the descriptor, free the skb, drop the > packet and return NETDEV_TX_OK. > > 5. Tx push failed. -- If tx push fails we will goto drop_free_descs which will > free the descriptor, free the skb, drop the packet and return. > > We will only stop queue in case 2 and 3 where we failed to allocate descriptor. > In case 1, 4 and 5 we are encountering dma mapping error, so for these cases we > will not stop the queue. > > Below will be my goto labels. > > drop_stop_q: > netif_tx_stop_queue(netif_txq); > > drop_free_descs: > prueth_xmit_free(tx_chn, first_desc); > > drop_free_skb: > dev_kfree_skb_any(skb); > > /* error */ > ndev->stats.tx_dropped++; > netdev_err(ndev, "tx: error: %d\n", ret); > > return ret; free_desc_stop_q_busy: prueth_xmit_free(tx_chn, first_desc); > drop_stop_q_busy: > netif_tx_stop_queue(netif_txq); > return NETDEV_TX_BUSY;
The Programmable Real-time Unit and Industrial Communication Subsystem Gigabit (PRU_ICSSG) is a low-latency microcontroller subsystem in the TI SoCs. This subsystem is provided for the use cases like the implementation of custom peripheral interfaces, offloading of tasks from the other processor cores of the SoC, etc. The subsystem includes many accelerators for data processing like multiplier and multiplier-accumulator. It also has peripherals like UART, MII/RGMII, MDIO, etc. Every ICSSG core includes two 32-bit load/store RISC CPU cores called PRUs. The above features allow it to be used for implementing custom firmware based peripherals like ethernet. This series adds the YAML documentation and the driver with basic EMAC support for TI AM654 Silicon Rev 2 SoC with the PRU_ICSSG Sub-system. running dual-EMAC firmware. This currently supports basic EMAC with 1Gbps and 100Mbps link. 10M and half-duplex modes are not yet supported because they require the support of an IEP, which will be added later. Advanced features like switch-dev and timestamping will be added later. This is the v12 of the patch series [v1]. This version of the patchset addresses comments made on v11. There series doesn't have any dependency. Changes from v11 to v12 : *) Rebased the series on latest net-next. *) Addressed Jakub's comments on ndo_xmit API. *) Added hooks to .get_rmon_stats for the driver. Now tx / rx bucket size and frame counts per bucket will be fetched by ethtool_rmon_stats instead of ethtool -S. *) Added __maybe_unused tags to unused config and classifier APIs in patch 2,3 and 4. These tags are later removed in patch 6. Changes from v10 to v11 : *) Rebased the series on latest net-next. *) Split the ICSSG driver introduction patch into 9 different patches as asked by Jakub. *) Introduced new patch(patch 8/10) to dump Standard network interface staticstics via ndo_get_stats64. Now certain stats that are reported by ICSSG hardware and are also part of struct rtnl_link_stats64, will be reported by ndo_get_stats64. While other stats that are not part of the struct rtnl_link_stats64 will be reported by ethtool -S. These stats are not duplicated. Changes from v9 to v10 : *) Rebased the series on latest net-next. *) Moved 'ndev prueth->emac[mac] == emac' assignment to the end of function prueth_netdev_init(). *) In unsupported phy_mode switch case instead of returning -EINVAL, store the error code in ret and 'goto free' Changes from v8 to v9 : *) Rebased the series on latest net-next. *) Fixed smatch and sparse warnings as pointed by Simon. *) Fixed leaky ndev in prueth_netdev_init() as asked by Simon. Changes from v7 to v8 : *) Rebased the series on 6.5-rc1. *) Fixed few formattings. Changes from v6 to v7 : *) Added RB tag of Rob in patch 1 of this series. *) Addressed Simon's comment on patch 2 of the series. *) Rebased patchset on next-20230428 linux-next. Changes from v5 to v6 : *) Added RB tag of Andrew Lunn in patch 2 of this series. *) Addressed Rob's comment on patch 1 of the series. *) Rebased patchset on next-20230421 linux-next. Changes from v4 to v5 : *) Re-arranged properties section in ti,icssg-prueth.yaml file. *) Added requirement for minimum one ethernet port. *) Fixed some minor formatting errors as asked by Krzysztof. *) Dropped SGMII mode from enum mii_mode as SGMII mode is not currently supported by the driver. *) Added switch-case block to handle different phy modes by ICSSG driver. Changes from v3 to v4 : *) Addressed Krzysztof's comments and fixed dt_binding_check errors in patch 1/2. *) Added interrupt-extended property in ethernet-ports properties section. *) Fixed comments in file icssg_switch_map.h according to the Linux coding style in patch 2/2. Added Documentation of structures in patch 2/2. Changes from v2 to v3 : *) Addressed Rob and Krzysztof's comments on patch 1 of this series. Fixed indentation. Removed description and pinctrl section from ti,icssg-prueth.yaml file. *) Addressed Krzysztof, Paolo, Randy, Andrew and Christophe's comments on patch 2 of this seires. *) Fixed blanklines in Kconfig and Makefile. Changed structures to const as suggested by Krzysztof. *) Fixed while loop logic in emac_tx_complete_packets() API as suggested by Paolo. Previously in the loop's last iteration 'budget' was 0 and napi_consume_skb would wrongly assume the caller is not in NAPI context Now, budget won't be zero in last iteration of loop. *) Removed inline functions addr_to_da1() and addr_to_da0() as asked by Andrew. *) Added dev_err_probe() instead of dev_err() as suggested by Christophe. *) In ti,icssg-prueth.yaml file, in the patternProperties section of ethernet-ports, kept the port name as "port" instead of "ethernet-port" as all other drivers were using "port". Will change it if is compulsory to use "ethernet-port". [v1] https://lore.kernel.org/all/20220506052433.28087-1-p-mohan@ti.com/ [v2] https://lore.kernel.org/all/20220531095108.21757-1-p-mohan@ti.com/ [v3] https://lore.kernel.org/all/20221223110930.1337536-1-danishanwar@ti.com/ [v4] https://lore.kernel.org/all/20230206060708.3574472-1-danishanwar@ti.com/ [v5] https://lore.kernel.org/all/20230210114957.2667963-1-danishanwar@ti.com/ [v6] https://lore.kernel.org/all/20230424053233.2338782-1-danishanwar@ti.com/ [v7] https://lore.kernel.org/all/20230502061650.2716736-1-danishanwar@ti.com/ [v8] https://lore.kernel.org/all/20230710053550.89160-1-danishanwar@ti.com/ [v9] https://lore.kernel.org/all/20230714094432.1834489-1-danishanwar@ti.com/ [v10] https://lore.kernel.org/all/20230719082755.3399424-1-danishanwar@ti.com/ [v11] https://lore.kernel.org/netdev/20230724112934.2637802-1-danishanwar@ti.com/ Thanks and Regards, Md Danish Anwar MD Danish Anwar (9): net: ti: icssg-prueth: Add Firmware Interface for ICSSG Ethernet driver. net: ti: icssg-prueth: Add mii helper apis and macros net: ti: icssg-prueth: Add Firmware config and classification APIs. net: ti: icssg-prueth: Add icssg queues APIs and macros dt-bindings: net: Add ICSSG Ethernet net: ti: icssg-prueth: Add ICSSG Stats net: ti: icssg-prueth: Add Standard network staticstics net: ti: icssg-prueth: Add ethtool ops for ICSSG Ethernet driver net: ti: icssg-prueth: Add Power management support Roger Quadros (1): net: ti: icssg-prueth: Add ICSSG ethernet driver .../bindings/net/ti,icssg-prueth.yaml | 184 ++ drivers/net/ethernet/ti/Kconfig | 13 + drivers/net/ethernet/ti/Makefile | 10 + .../net/ethernet/ti/icssg/icssg_classifier.c | 367 ++++ drivers/net/ethernet/ti/icssg/icssg_config.c | 450 ++++ drivers/net/ethernet/ti/icssg/icssg_config.h | 200 ++ drivers/net/ethernet/ti/icssg/icssg_ethtool.c | 188 ++ drivers/net/ethernet/ti/icssg/icssg_mii_cfg.c | 120 ++ drivers/net/ethernet/ti/icssg/icssg_mii_rt.h | 151 ++ drivers/net/ethernet/ti/icssg/icssg_prueth.c | 1901 +++++++++++++++++ drivers/net/ethernet/ti/icssg/icssg_prueth.h | 262 +++ drivers/net/ethernet/ti/icssg/icssg_queues.c | 38 + drivers/net/ethernet/ti/icssg/icssg_stats.c | 57 + drivers/net/ethernet/ti/icssg/icssg_stats.h | 158 ++ .../net/ethernet/ti/icssg/icssg_switch_map.h | 234 ++ 15 files changed, 4333 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml create mode 100644 drivers/net/ethernet/ti/icssg/icssg_classifier.c create mode 100644 drivers/net/ethernet/ti/icssg/icssg_config.c create mode 100644 drivers/net/ethernet/ti/icssg/icssg_config.h create mode 100644 drivers/net/ethernet/ti/icssg/icssg_ethtool.c create mode 100644 drivers/net/ethernet/ti/icssg/icssg_mii_cfg.c create mode 100644 drivers/net/ethernet/ti/icssg/icssg_mii_rt.h create mode 100644 drivers/net/ethernet/ti/icssg/icssg_prueth.c create mode 100644 drivers/net/ethernet/ti/icssg/icssg_prueth.h create mode 100644 drivers/net/ethernet/ti/icssg/icssg_queues.c create mode 100644 drivers/net/ethernet/ti/icssg/icssg_stats.c create mode 100644 drivers/net/ethernet/ti/icssg/icssg_stats.h create mode 100644 drivers/net/ethernet/ti/icssg/icssg_switch_map.h