| Message ID | 20230724112934.2637802-1-danishanwar@ti.com |
|---|---|
| Headers | show |
| Series | Introduce ICSSG based ethernet Driver | expand |
On Mon, Jul 24, 2023 at 04:59:27PM +0530, MD Danish Anwar wrote: > Add icssg_config.h / .c and icssg_classifier.c files. These are firmware > configuration and classification related files. These will be used by > ICSSG ethernet driver. > > Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> Hi Danish, some feedback from my side. ... > diff --git a/drivers/net/ethernet/ti/icssg_classifier.c b/drivers/net/ethernet/ti/icssg_classifier.c ... > +void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac) This function appears to be unused. Perhaps it would be better placed in a later patch? Or perhaps not, if it makes it hard to split up the patches nicely. In which case, perhaps the __maybe_unused annotation could be added, temporarily. Flagged by clang-16 W=1, and gcc=12 W=1 builds. Likewise for other issues flagged below regarding function declarations/definitions. > +{ > + regmap_write(miig_rt, offs[slice].mac0, (u32)(mac[0] | mac[1] << 8 | > + mac[2] << 16 | mac[3] << 24)); > + regmap_write(miig_rt, offs[slice].mac1, (u32)(mac[4] | mac[5] << 8)); > +} > + > +/* disable all RX traffic */ > +void icssg_class_disable(struct regmap *miig_rt, int slice) This function is only used in this file. Please consider making it static. ... > +void icssg_class_default(struct regmap *miig_rt, int slice, bool allmulti) This function also appears to be unused. ... > +/* required for SAV check */ > +void icssg_ft1_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac_addr) This function also appears to be unused. ... > diff --git a/drivers/net/ethernet/ti/icssg_config.c b/drivers/net/ethernet/ti/icssg_config.c ... > +void icssg_config_ipg(struct prueth_emac *emac) This function is also only used in this file. ... > +static void icssg_init_emac_mode(struct prueth *prueth) > +{ > + /* When the device is configured as a bridge and it is being brought > + * back to the emac mode, the host mac address has to be set as 0. > + */ > + u8 mac[ETH_ALEN] = { 0 }; > + > + if (prueth->emacs_initialized) > + return; > + > + regmap_update_bits(prueth->miig_rt, FDB_GEN_CFG1, > + SMEM_VLAN_OFFSET_MASK, 0); > + regmap_write(prueth->miig_rt, FDB_GEN_CFG2, 0); > + /* Clear host MAC address */ > + icssg_class_set_host_mac_addr(prueth->miig_rt, mac); icssg_class_set_host_mac_addr() is defined in icssg_classifier.c but used here in icssg_config.c. Please consider providing a declaration of this function, ideally in a .h file. ... > +int emac_set_port_state(struct prueth_emac *emac, > + enum icssg_port_state_cmd cmd) This function also appears to be unused. ... > +void icssg_config_set_speed(struct prueth_emac *emac) Ditto. ...
Hi Simon, On 25/07/23 12:55 pm, Simon Horman wrote: > On Mon, Jul 24, 2023 at 04:59:27PM +0530, MD Danish Anwar wrote: >> Add icssg_config.h / .c and icssg_classifier.c files. These are firmware >> configuration and classification related files. These will be used by >> ICSSG ethernet driver. >> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >> Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Hi Danish, > > some feedback from my side. > Thanks for the feedback. > ... > >> diff --git a/drivers/net/ethernet/ti/icssg_classifier.c b/drivers/net/ethernet/ti/icssg_classifier.c > > ... > >> +void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac) > > This function appears to be unused. > Perhaps it would be better placed in a later patch? > > Or perhaps not, if it makes it hard to split up the patches nicely. > In which case, perhaps the __maybe_unused annotation could be added, > temporarily. > Due to splitting the patch into 8-9 patches, I had to introduce these helper APIs earlier. All these APIs are helper APIs, they will be used in patch 6 (Introduce ICSSG Prueth driver). I had this concern that some APIs which will be used later but introduced earlier can create some warnings, before splitting the patches. I had raised this concern in [1] and asked Jakub if it would be OK to introduce these APIs earlier. Jakub said it would be fine [2], so I went ahead with this approach. It will make very hard to break patches if these APIs are introduced and used in same patch. > Flagged by clang-16 W=1, and gcc=12 W=1 builds. > Likewise for other issues flagged below regarding > function declarations/definitions. > >> +{ >> + regmap_write(miig_rt, offs[slice].mac0, (u32)(mac[0] | mac[1] << 8 | >> + mac[2] << 16 | mac[3] << 24)); >> + regmap_write(miig_rt, offs[slice].mac1, (u32)(mac[4] | mac[5] << 8)); >> +} >> + >> +/* disable all RX traffic */ >> +void icssg_class_disable(struct regmap *miig_rt, int slice) > > This function is only used in this file. > Please consider making it static. > This is later used in icssg_prueth.c, that is why this is not static. > ... > >> +void icssg_class_default(struct regmap *miig_rt, int slice, bool allmulti) > > This function also appears to be unused. > This is later used in icssg_prueth.c > ... > >> +/* required for SAV check */ >> +void icssg_ft1_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac_addr) > > This function also appears to be unused. > This is later used in icssg_prueth.c > ... > >> diff --git a/drivers/net/ethernet/ti/icssg_config.c b/drivers/net/ethernet/ti/icssg_config.c > > ... > >> +void icssg_config_ipg(struct prueth_emac *emac) > > This function is also only used in this file. > This is later used in icssg_prueth.c > ... > >> +static void icssg_init_emac_mode(struct prueth *prueth) >> +{ >> + /* When the device is configured as a bridge and it is being brought >> + * back to the emac mode, the host mac address has to be set as 0. >> + */ >> + u8 mac[ETH_ALEN] = { 0 }; >> + >> + if (prueth->emacs_initialized) >> + return; >> + >> + regmap_update_bits(prueth->miig_rt, FDB_GEN_CFG1, >> + SMEM_VLAN_OFFSET_MASK, 0); >> + regmap_write(prueth->miig_rt, FDB_GEN_CFG2, 0); >> + /* Clear host MAC address */ >> + icssg_class_set_host_mac_addr(prueth->miig_rt, mac); > This is later used in icssg_prueth.c > icssg_class_set_host_mac_addr() is defined in icssg_classifier.c > but used here in icssg_config.c. > > Please consider providing a declaration of this function, > ideally in a .h file. > The declaration of this function is added later(in patch 6) in icssg_prueth.h > ... > >> +int emac_set_port_state(struct prueth_emac *emac, >> + enum icssg_port_state_cmd cmd) > > This function also appears to be unused. > > ... > >> +void icssg_config_set_speed(struct prueth_emac *emac) > > Ditto. > Both these APIs are later used in icssg_prueth.c > ... [1] https://lore.kernel.org/all/17cd1e70-73bc-78d5-7e9d-7b133d6f464b@ti.com/ [2] https://lore.kernel.org/all/20230720081741.0c32d5e6@kernel.org/
On Tue, Jul 25, 2023 at 01:10:30PM +0530, Md Danish Anwar wrote: > Hi Simon, > > On 25/07/23 12:55 pm, Simon Horman wrote: > > On Mon, Jul 24, 2023 at 04:59:27PM +0530, MD Danish Anwar wrote: > >> Add icssg_config.h / .c and icssg_classifier.c files. These are firmware > >> configuration and classification related files. These will be used by > >> ICSSG ethernet driver. > >> > >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > >> Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > Hi Danish, > > > > some feedback from my side. > > > > Thanks for the feedback. > > > ... > > > >> diff --git a/drivers/net/ethernet/ti/icssg_classifier.c b/drivers/net/ethernet/ti/icssg_classifier.c > > > > ... > > > >> +void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac) > > > > This function appears to be unused. > > Perhaps it would be better placed in a later patch? > > > > Or perhaps not, if it makes it hard to split up the patches nicely. > > In which case, perhaps the __maybe_unused annotation could be added, > > temporarily. > > > > Due to splitting the patch into 8-9 patches, I had to introduce these helper > APIs earlier. All these APIs are helper APIs, they will be used in patch 6 > (Introduce ICSSG Prueth driver). > > I had this concern that some APIs which will be used later but introduced > earlier can create some warnings, before splitting the patches. > > I had raised this concern in [1] and asked Jakub if it would be OK to introduce > these APIs earlier. Jakub said it would be fine [2], so I went ahead with this > approach. > > It will make very hard to break patches if these APIs are introduced and used > in same patch. Thanks, I understand. In that case my suggestion is to, temporarily, add __maybe_unused, which will allow static analysis tools to work more cleanly over the series. It is just a suggestion, not a hard requirement. Probably something along those lines applies to all the review I provided in my previous email. Please use your discretion here.
On 25/07/23 1:14 pm, Simon Horman wrote: > On Tue, Jul 25, 2023 at 01:10:30PM +0530, Md Danish Anwar wrote: >> Hi Simon, >> >> On 25/07/23 12:55 pm, Simon Horman wrote: >>> On Mon, Jul 24, 2023 at 04:59:27PM +0530, MD Danish Anwar wrote: >>>> Add icssg_config.h / .c and icssg_classifier.c files. These are firmware >>>> configuration and classification related files. These will be used by >>>> ICSSG ethernet driver. >>>> >>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >>>> Reviewed-by: Andrew Lunn <andrew@lunn.ch> >>> >>> Hi Danish, >>> >>> some feedback from my side. >>> >> >> Thanks for the feedback. >> >>> ... >>> >>>> diff --git a/drivers/net/ethernet/ti/icssg_classifier.c b/drivers/net/ethernet/ti/icssg_classifier.c >>> >>> ... >>> >>>> +void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac) >>> >>> This function appears to be unused. >>> Perhaps it would be better placed in a later patch? >>> >>> Or perhaps not, if it makes it hard to split up the patches nicely. >>> In which case, perhaps the __maybe_unused annotation could be added, >>> temporarily. >>> >> >> Due to splitting the patch into 8-9 patches, I had to introduce these helper >> APIs earlier. All these APIs are helper APIs, they will be used in patch 6 >> (Introduce ICSSG Prueth driver). >> >> I had this concern that some APIs which will be used later but introduced >> earlier can create some warnings, before splitting the patches. >> >> I had raised this concern in [1] and asked Jakub if it would be OK to introduce >> these APIs earlier. Jakub said it would be fine [2], so I went ahead with this >> approach. >> >> It will make very hard to break patches if these APIs are introduced and used >> in same patch. > > Thanks, I understand. > > In that case my suggestion is to, temporarily, add __maybe_unused, > which will allow static analysis tools to work more cleanly over the > series. It is just a suggestion, not a hard requirement. > > Probably something along those lines applies to all the > review I provided in my previous email. Please use your discretion here. For now I think I will leave it as it is. Let reviewers review all other patches. Let's see if there are any other comments on all the patches in this series. If there are any more comments on other patches, then while re-spinning next revision I will keep this in mind and try to add __maybe_unused tags in all APIs that are used later. The idea behind splitting the patches was to get them reviewed individually as it is quite difficult to get one big patch reviewed as explained by Jakub. And these warnings were expected. If there are any other comments on this series, I will try to address all of them together in next revision. Meanwhile, Please let me know if you have any comments on other patches in this series.
On Mon, 24 Jul 2023 16:59:30 +0530 MD Danish Anwar wrote: > drivers/net/ethernet/ti/Kconfig | 13 + > drivers/net/ethernet/ti/Makefile | 3 + > drivers/net/ethernet/ti/icssg_prueth.c | 1831 ++++++++++++++++++++++++ > drivers/net/ethernet/ti/icssg_prueth.h | 48 + Please create a sub-directory for the driver. > +static int prueth_ndev_add_tx_napi(struct prueth_emac *emac) > +{ > + struct prueth *prueth = emac->prueth; > + int i, ret; > + > + for (i = 0; i < emac->tx_ch_num; i++) { > + struct prueth_tx_chn *tx_chn = &emac->tx_chns[i]; > + > + netif_napi_add_tx_weight(emac->ndev, &tx_chn->napi_tx, > + emac_napi_tx_poll, NAPI_POLL_WEIGHT); Skip specifying weight, please. > +/** > + * emac_ndo_start_xmit - EMAC Transmit function > + * @skb: SKB pointer > + * @ndev: EMAC network adapter > + * > + * Called by the system to transmit a packet - we queue the packet in > + * EMAC hardware transmit queue > + * Doesn't wait for completion we'll check for TX completion in > + * emac_tx_complete_packets(). > + * > + * Return: enum netdev_tx > + */ > +static enum netdev_tx emac_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev) > +{ > + struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc; > + struct prueth_emac *emac = netdev_priv(ndev); > + struct netdev_queue *netif_txq; > + struct prueth_tx_chn *tx_chn; > + dma_addr_t desc_dma, buf_dma; > + int i, ret = 0, q_idx; > + void **swdata; > + u32 pkt_len; > + u32 *epib; > + > + pkt_len = skb_headlen(skb); > + q_idx = skb_get_queue_mapping(skb); > + > + tx_chn = &emac->tx_chns[q_idx]; > + netif_txq = netdev_get_tx_queue(ndev, q_idx); > + > + /* Map the linear buffer */ > + buf_dma = dma_map_single(tx_chn->dma_dev, skb->data, pkt_len, DMA_TO_DEVICE); > + if (dma_mapping_error(tx_chn->dma_dev, buf_dma)) { > + netdev_err(ndev, "tx: failed to map skb buffer\n"); > + ret = NETDEV_TX_BUSY; Drop it if it can't be mapped and return OK. What's going to re-enable the queue in this case? > + goto drop_stop_q; > + } > + > + first_desc = k3_cppi_desc_pool_alloc(tx_chn->desc_pool); > + if (!first_desc) { > + netdev_dbg(ndev, "tx: failed to allocate descriptor\n"); > + dma_unmap_single(tx_chn->dma_dev, buf_dma, pkt_len, DMA_TO_DEVICE); > + ret = NETDEV_TX_BUSY; > + goto drop_stop_q_busy; > + } > + > + cppi5_hdesc_init(first_desc, CPPI5_INFO0_HDESC_EPIB_PRESENT, > + PRUETH_NAV_PS_DATA_SIZE); > + cppi5_hdesc_set_pkttype(first_desc, 0); > + epib = first_desc->epib; > + epib[0] = 0; > + epib[1] = 0; > + > + /* set dst tag to indicate internal qid at the firmware which is at > + * bit8..bit15. bit0..bit7 indicates port num for directed > + * packets in case of switch mode operation > + */ > + cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8))); > + k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma); > + cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len); > + swdata = cppi5_hdesc_get_swdata(first_desc); > + *swdata = skb; > + > + if (!skb_is_nonlinear(skb)) > + goto tx_push; Why the goto? The loop won't be entered. > + /* Handle the case where skb is fragmented in pages */ > + cur_desc = first_desc; > + for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { > + skb_frag_t *frag = &skb_shinfo(skb)->frags[i]; > + u32 frag_size = skb_frag_size(frag); > + > + next_desc = k3_cppi_desc_pool_alloc(tx_chn->desc_pool); > + if (!next_desc) { > + netdev_err(ndev, > + "tx: failed to allocate frag. descriptor\n"); > + ret = NETDEV_TX_BUSY; > + goto drop_free_descs; > + } > + > + buf_dma = skb_frag_dma_map(tx_chn->dma_dev, frag, 0, frag_size, > + DMA_TO_DEVICE); > + if (dma_mapping_error(tx_chn->dma_dev, buf_dma)) { > + netdev_err(ndev, "tx: Failed to map skb page\n"); > + k3_cppi_desc_pool_free(tx_chn->desc_pool, next_desc); > + ret = NETDEV_TX_BUSY; > + goto drop_free_descs; this label frees the skb, you can't return BUSY > + } > + > + cppi5_hdesc_reset_hbdesc(next_desc); > + k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma); > + cppi5_hdesc_attach_buf(next_desc, > + buf_dma, frag_size, buf_dma, frag_size); > + > + desc_dma = k3_cppi_desc_pool_virt2dma(tx_chn->desc_pool, > + next_desc); > + k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &desc_dma); > + cppi5_hdesc_link_hbdesc(cur_desc, desc_dma); > + > + pkt_len += frag_size; > + cur_desc = next_desc; > + } > + WARN_ON(pkt_len != skb->len); WARN_ON_ONCE() if at all > + > +tx_push: > + /* report bql before sending packet */ > + netdev_tx_sent_queue(netif_txq, pkt_len); > + > + cppi5_hdesc_set_pktlen(first_desc, pkt_len); > + desc_dma = k3_cppi_desc_pool_virt2dma(tx_chn->desc_pool, first_desc); > + /* cppi5_desc_dump(first_desc, 64); */ > + > + skb_tx_timestamp(skb); /* SW timestamp if SKBTX_IN_PROGRESS not set */ > + ret = k3_udma_glue_push_tx_chn(tx_chn->tx_chn, first_desc, desc_dma); > + if (ret) { > + netdev_err(ndev, "tx: push failed: %d\n", ret); > + goto drop_free_descs; > + } > + > + if (k3_cppi_desc_pool_avail(tx_chn->desc_pool) < MAX_SKB_FRAGS) { > + netif_tx_stop_queue(netif_txq); > + /* Barrier, so that stop_queue visible to other cpus */ > + smp_mb__after_atomic(); > + > + if (k3_cppi_desc_pool_avail(tx_chn->desc_pool) >= > + MAX_SKB_FRAGS) MAX_FRAGS + 1? > + netif_tx_wake_queue(netif_txq); > + } > + > + return NETDEV_TX_OK; > +static int emac_napi_rx_poll(struct napi_struct *napi_rx, int budget) > +{ > + struct prueth_emac *emac = prueth_napi_to_emac(napi_rx); > + int rx_flow = PRUETH_RX_FLOW_DATA; > + int flow = PRUETH_MAX_RX_FLOWS; > + int num_rx = 0; > + int cur_budget; > + int ret; > + > + while (flow--) { > + cur_budget = budget - num_rx; > + > + while (cur_budget--) { > + ret = emac_rx_packet(emac, flow); > + if (ret) > + break; > + num_rx++; > + } > + > + if (num_rx >= budget) > + break; > + } > + > + if (num_rx < budget) { > + napi_complete(napi_rx); Prefer using napi_complete_done() > + enable_irq(emac->rx_chns.irq[rx_flow]); > + } > + > + return num_rx; > +} > +static void emac_ndo_tx_timeout(struct net_device *ndev, unsigned int txqueue) > +{ > + struct prueth_emac *emac = netdev_priv(ndev); > + > + if (netif_msg_tx_err(emac)) > + netdev_err(ndev, "xmit timeout"); Core already prints something, you can drop this. > + ndev->stats.tx_errors++; > +} > +static void emac_ndo_set_rx_mode_work(struct work_struct *work) > +{ > + struct prueth_emac *emac = container_of(work, struct prueth_emac, rx_mode_work); > + struct net_device *ndev = emac->ndev; > + bool promisc, allmulti; > + > + if (!netif_running(ndev)) > + return; > + > + promisc = ndev->flags & IFF_PROMISC; > + allmulti = ndev->flags & IFF_ALLMULTI; > + emac_set_port_state(emac, ICSSG_EMAC_PORT_UC_FLOODING_DISABLE); > + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_DISABLE); > + > + if (promisc) { > + emac_set_port_state(emac, ICSSG_EMAC_PORT_UC_FLOODING_ENABLE); > + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE); > + return; > + } > + > + if (allmulti) { > + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE); > + return; > + } > + > + if (!netdev_mc_empty(ndev)) { > + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE); > + return; > + } > +} There's no need for locking in this work? > + netif_napi_add(ndev, &emac->napi_rx, > + emac_napi_rx_poll); nit: fits on a line > +static struct platform_driver prueth_driver = { > + .probe = prueth_probe, > + .remove = prueth_remove, Please use .remove_new (which has a void return).
On Tue, Jul 25, 2023 at 01:28:21PM +0530, Md Danish Anwar wrote: > On 25/07/23 1:14 pm, Simon Horman wrote: > > On Tue, Jul 25, 2023 at 01:10:30PM +0530, Md Danish Anwar wrote: > >> Hi Simon, > >> > >> On 25/07/23 12:55 pm, Simon Horman wrote: > >>> On Mon, Jul 24, 2023 at 04:59:27PM +0530, MD Danish Anwar wrote: > >>>> Add icssg_config.h / .c and icssg_classifier.c files. These are firmware > >>>> configuration and classification related files. These will be used by > >>>> ICSSG ethernet driver. > >>>> > >>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > >>>> Reviewed-by: Andrew Lunn <andrew@lunn.ch> > >>> > >>> Hi Danish, > >>> > >>> some feedback from my side. > >>> > >> > >> Thanks for the feedback. > >> > >>> ... > >>> > >>>> diff --git a/drivers/net/ethernet/ti/icssg_classifier.c b/drivers/net/ethernet/ti/icssg_classifier.c > >>> > >>> ... > >>> > >>>> +void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac) > >>> > >>> This function appears to be unused. > >>> Perhaps it would be better placed in a later patch? > >>> > >>> Or perhaps not, if it makes it hard to split up the patches nicely. > >>> In which case, perhaps the __maybe_unused annotation could be added, > >>> temporarily. > >>> > >> > >> Due to splitting the patch into 8-9 patches, I had to introduce these helper > >> APIs earlier. All these APIs are helper APIs, they will be used in patch 6 > >> (Introduce ICSSG Prueth driver). > >> > >> I had this concern that some APIs which will be used later but introduced > >> earlier can create some warnings, before splitting the patches. > >> > >> I had raised this concern in [1] and asked Jakub if it would be OK to introduce > >> these APIs earlier. Jakub said it would be fine [2], so I went ahead with this > >> approach. > >> > >> It will make very hard to break patches if these APIs are introduced and used > >> in same patch. > > > > Thanks, I understand. > > > > In that case my suggestion is to, temporarily, add __maybe_unused, > > which will allow static analysis tools to work more cleanly over the > > series. It is just a suggestion, not a hard requirement. > > > > Probably something along those lines applies to all the > > review I provided in my previous email. Please use your discretion here. > > For now I think I will leave it as it is. Let reviewers review all other > patches. Let's see if there are any other comments on all the patches in this > series. If there are any more comments on other patches, then while re-spinning > next revision I will keep this in mind and try to add __maybe_unused tags in > all APIs that are used later. Sure, that sounds reasonable. > The idea behind splitting the patches was to get them reviewed individually as > it is quite difficult to get one big patch reviewed as explained by Jakub. And > these warnings were expected. If there are any other comments on this series, I > will try to address all of them together in next revision. Yes, I understand. Thanks for splitting things up into multiple patches. I know that is a lot of work. But it is very helpful. > Meanwhile, Please let me know if you have any comments on other patches > in this series. Will do, but I nothing to add at this time.
Hi Jakub, On 26/07/23 9:39 am, Jakub Kicinski wrote: > On Mon, 24 Jul 2023 16:59:30 +0530 MD Danish Anwar wrote: >> drivers/net/ethernet/ti/Kconfig | 13 + >> drivers/net/ethernet/ti/Makefile | 3 + >> drivers/net/ethernet/ti/icssg_prueth.c | 1831 ++++++++++++++++++++++++ >> drivers/net/ethernet/ti/icssg_prueth.h | 48 + > > Please create a sub-directory for the driver. > >> +static int prueth_ndev_add_tx_napi(struct prueth_emac *emac) >> +{ >> + struct prueth *prueth = emac->prueth; >> + int i, ret; >> + >> + for (i = 0; i < emac->tx_ch_num; i++) { >> + struct prueth_tx_chn *tx_chn = &emac->tx_chns[i]; >> + >> + netif_napi_add_tx_weight(emac->ndev, &tx_chn->napi_tx, >> + emac_napi_tx_poll, NAPI_POLL_WEIGHT); > > Skip specifying weight, please. > Sure, Will change this to 'netif_napi_add_tx(emac->ndev, &tx_chn-> emac_napi_tx_poll);' >> +/** >> + * emac_ndo_start_xmit - EMAC Transmit function >> + * @skb: SKB pointer >> + * @ndev: EMAC network adapter >> + * >> + * Called by the system to transmit a packet - we queue the packet in >> + * EMAC hardware transmit queue >> + * Doesn't wait for completion we'll check for TX completion in >> + * emac_tx_complete_packets(). >> + * >> + * Return: enum netdev_tx >> + */ >> +static enum netdev_tx emac_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev) >> +{ >> + struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc; >> + struct prueth_emac *emac = netdev_priv(ndev); >> + struct netdev_queue *netif_txq; >> + struct prueth_tx_chn *tx_chn; >> + dma_addr_t desc_dma, buf_dma; >> + int i, ret = 0, q_idx; >> + void **swdata; >> + u32 pkt_len; >> + u32 *epib; >> + >> + pkt_len = skb_headlen(skb); >> + q_idx = skb_get_queue_mapping(skb); >> + >> + tx_chn = &emac->tx_chns[q_idx]; >> + netif_txq = netdev_get_tx_queue(ndev, q_idx); >> + >> + /* Map the linear buffer */ >> + buf_dma = dma_map_single(tx_chn->dma_dev, skb->data, pkt_len, DMA_TO_DEVICE); >> + if (dma_mapping_error(tx_chn->dma_dev, buf_dma)) { >> + netdev_err(ndev, "tx: failed to map skb buffer\n"); >> + ret = NETDEV_TX_BUSY; > > Drop it if it can't be mapped and return OK. What's going to re-enable > the queue in this case? > Sure. I will drop the packet and return NETDEV_TX_OK. >> + goto drop_stop_q; >> + } >> + >> + first_desc = k3_cppi_desc_pool_alloc(tx_chn->desc_pool); >> + if (!first_desc) { >> + netdev_dbg(ndev, "tx: failed to allocate descriptor\n"); >> + dma_unmap_single(tx_chn->dma_dev, buf_dma, pkt_len, DMA_TO_DEVICE); >> + ret = NETDEV_TX_BUSY; >> + goto drop_stop_q_busy; >> + } >> + >> + cppi5_hdesc_init(first_desc, CPPI5_INFO0_HDESC_EPIB_PRESENT, >> + PRUETH_NAV_PS_DATA_SIZE); >> + cppi5_hdesc_set_pkttype(first_desc, 0); >> + epib = first_desc->epib; >> + epib[0] = 0; >> + epib[1] = 0; >> + >> + /* set dst tag to indicate internal qid at the firmware which is at >> + * bit8..bit15. bit0..bit7 indicates port num for directed >> + * packets in case of switch mode operation >> + */ >> + cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8))); >> + k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma); >> + cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len); >> + swdata = cppi5_hdesc_get_swdata(first_desc); >> + *swdata = skb; >> + >> + if (!skb_is_nonlinear(skb)) >> + goto tx_push; > > Why the goto? The loop won't be entered. > skb_is_nonlinear() will return true when skb is fragmented i.e. skb_shinfo(skb)->nr_frags > 0. Makes sense to drop the if condition. As for non-fragmented skb, skb_shinfo(skb)->nr_frags = 0 and we won't enter for loop and will eventually reach at label tx_push. I will drop the if condition. >> + /* Handle the case where skb is fragmented in pages */ >> + cur_desc = first_desc; >> + for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { >> + skb_frag_t *frag = &skb_shinfo(skb)->frags[i]; >> + u32 frag_size = skb_frag_size(frag); >> + >> + next_desc = k3_cppi_desc_pool_alloc(tx_chn->desc_pool); >> + if (!next_desc) { >> + netdev_err(ndev, >> + "tx: failed to allocate frag. descriptor\n"); >> + ret = NETDEV_TX_BUSY; >> + goto drop_free_descs; >> + } >> + >> + buf_dma = skb_frag_dma_map(tx_chn->dma_dev, frag, 0, frag_size, >> + DMA_TO_DEVICE); >> + if (dma_mapping_error(tx_chn->dma_dev, buf_dma)) { >> + netdev_err(ndev, "tx: Failed to map skb page\n"); >> + k3_cppi_desc_pool_free(tx_chn->desc_pool, next_desc); >> + ret = NETDEV_TX_BUSY; >> + goto drop_free_descs; > > this label frees the skb, you can't return BUSY > Sure. Will return OK here. >> + } >> + >> + cppi5_hdesc_reset_hbdesc(next_desc); >> + k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma); >> + cppi5_hdesc_attach_buf(next_desc, >> + buf_dma, frag_size, buf_dma, frag_size); >> + >> + desc_dma = k3_cppi_desc_pool_virt2dma(tx_chn->desc_pool, >> + next_desc); >> + k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &desc_dma); >> + cppi5_hdesc_link_hbdesc(cur_desc, desc_dma); >> + >> + pkt_len += frag_size; >> + cur_desc = next_desc; >> + } >> + WARN_ON(pkt_len != skb->len); > > WARN_ON_ONCE() if at all > Sure. >> + >> +tx_push: >> + /* report bql before sending packet */ >> + netdev_tx_sent_queue(netif_txq, pkt_len); >> + >> + cppi5_hdesc_set_pktlen(first_desc, pkt_len); >> + desc_dma = k3_cppi_desc_pool_virt2dma(tx_chn->desc_pool, first_desc); >> + /* cppi5_desc_dump(first_desc, 64); */ >> + >> + skb_tx_timestamp(skb); /* SW timestamp if SKBTX_IN_PROGRESS not set */ >> + ret = k3_udma_glue_push_tx_chn(tx_chn->tx_chn, first_desc, desc_dma); >> + if (ret) { >> + netdev_err(ndev, "tx: push failed: %d\n", ret); >> + goto drop_free_descs; >> + } >> + >> + if (k3_cppi_desc_pool_avail(tx_chn->desc_pool) < MAX_SKB_FRAGS) { >> + netif_tx_stop_queue(netif_txq); >> + /* Barrier, so that stop_queue visible to other cpus */ >> + smp_mb__after_atomic(); >> + >> + if (k3_cppi_desc_pool_avail(tx_chn->desc_pool) >= >> + MAX_SKB_FRAGS) > > MAX_FRAGS + 1? > I think MAX_SKB_FRAGS is OK. If the available pool = MAX_SKB_FRAGS we should be able to wake the queue. >> + netif_tx_wake_queue(netif_txq); >> + } >> + >> + return NETDEV_TX_OK; > > >> +static int emac_napi_rx_poll(struct napi_struct *napi_rx, int budget) >> +{ >> + struct prueth_emac *emac = prueth_napi_to_emac(napi_rx); >> + int rx_flow = PRUETH_RX_FLOW_DATA; >> + int flow = PRUETH_MAX_RX_FLOWS; >> + int num_rx = 0; >> + int cur_budget; >> + int ret; >> + >> + while (flow--) { >> + cur_budget = budget - num_rx; >> + >> + while (cur_budget--) { >> + ret = emac_rx_packet(emac, flow); >> + if (ret) >> + break; >> + num_rx++; >> + } >> + >> + if (num_rx >= budget) >> + break; >> + } >> + >> + if (num_rx < budget) { >> + napi_complete(napi_rx); > > Prefer using napi_complete_done() > Sure. >> + enable_irq(emac->rx_chns.irq[rx_flow]); >> + } >> + >> + return num_rx; >> +} > >> +static void emac_ndo_tx_timeout(struct net_device *ndev, unsigned int txqueue) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + >> + if (netif_msg_tx_err(emac)) >> + netdev_err(ndev, "xmit timeout"); > > Core already prints something, you can drop this. > Sure, I will drop this print. >> + ndev->stats.tx_errors++; >> +} > >> +static void emac_ndo_set_rx_mode_work(struct work_struct *work) >> +{ >> + struct prueth_emac *emac = container_of(work, struct prueth_emac, rx_mode_work); >> + struct net_device *ndev = emac->ndev; >> + bool promisc, allmulti; >> + >> + if (!netif_running(ndev)) >> + return; >> + >> + promisc = ndev->flags & IFF_PROMISC; >> + allmulti = ndev->flags & IFF_ALLMULTI; >> + emac_set_port_state(emac, ICSSG_EMAC_PORT_UC_FLOODING_DISABLE); >> + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_DISABLE); >> + >> + if (promisc) { >> + emac_set_port_state(emac, ICSSG_EMAC_PORT_UC_FLOODING_ENABLE); >> + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE); >> + return; >> + } >> + >> + if (allmulti) { >> + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE); >> + return; >> + } >> + >> + if (!netdev_mc_empty(ndev)) { >> + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE); >> + return; >> + } >> +} > > There's no need for locking in this work? > No I don't think any lock is required here. emac_set_port_state() aquires lock before updating port status. Also emac_ndo_set_rx_mode_work() is scheduled by a singlethreaded workqueue. >> + netif_napi_add(ndev, &emac->napi_rx, >> + emac_napi_rx_poll); > > nit: fits on a line Sure I will move it to one line. > >> +static struct platform_driver prueth_driver = { >> + .probe = prueth_probe, >> + .remove = prueth_remove, > > Please use .remove_new (which has a void return). Sure I will use .remove_new instead of .remove Please let me know if this looks ok to you. I will try to address these and send next revision.
On Wed, 26 Jul 2023 16:01:23 +0530 Md Danish Anwar wrote: > I think MAX_SKB_FRAGS is OK. If the available pool = MAX_SKB_FRAGS we should be > able to wake the queue. MAX_SKB_FRAGS only counts frags and you also need space to map the head, no? In general we advise to wait until there's at least 2 * MAX_SKB_FRAGS to avoid frequent sleep/wake cycles. But IDK how long your queue is, maybe it's too much. > No I don't think any lock is required here. emac_set_port_state() aquires lock > before updating port status. Also emac_ndo_set_rx_mode_work() is scheduled by a > singlethreaded workqueue. if (netif_running()) outside of any locks is usually a red flag, but if you're confident it's fine it's fine :)
On Wed, 26 Jul 2023 09:42:17 +0200 Simon Horman wrote: > Thanks for splitting things up into multiple patches. > I know that is a lot of work. But it is very helpful. +1, definitely much easier to review now!
On 7/26/2023 1:12 PM, Simon Horman wrote: > On Tue, Jul 25, 2023 at 01:28:21PM +0530, Md Danish Anwar wrote: >> On 25/07/23 1:14 pm, Simon Horman wrote: >>> On Tue, Jul 25, 2023 at 01:10:30PM +0530, Md Danish Anwar wrote: >>>> Hi Simon, >>>> >>>> On 25/07/23 12:55 pm, Simon Horman wrote: >>>>> On Mon, Jul 24, 2023 at 04:59:27PM +0530, MD Danish Anwar wrote: >>>>>> Add icssg_config.h / .c and icssg_classifier.c files. These are firmware >>>>>> configuration and classification related files. These will be used by >>>>>> ICSSG ethernet driver. >>>>>> >>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >>>>>> Reviewed-by: Andrew Lunn <andrew@lunn.ch> >>>>> >>>>> Hi Danish, >>>>> >>>>> some feedback from my side. >>>>> >>>> >>>> Thanks for the feedback. >>>> >>>>> ... >>>>> >>>>>> diff --git a/drivers/net/ethernet/ti/icssg_classifier.c b/drivers/net/ethernet/ti/icssg_classifier.c >>>>> >>>>> ... >>>>> >>>>>> +void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac) >>>>> >>>>> This function appears to be unused. >>>>> Perhaps it would be better placed in a later patch? >>>>> >>>>> Or perhaps not, if it makes it hard to split up the patches nicely. >>>>> In which case, perhaps the __maybe_unused annotation could be added, >>>>> temporarily. >>>>> >>>> >>>> Due to splitting the patch into 8-9 patches, I had to introduce these helper >>>> APIs earlier. All these APIs are helper APIs, they will be used in patch 6 >>>> (Introduce ICSSG Prueth driver). >>>> >>>> I had this concern that some APIs which will be used later but introduced >>>> earlier can create some warnings, before splitting the patches. >>>> >>>> I had raised this concern in [1] and asked Jakub if it would be OK to introduce >>>> these APIs earlier. Jakub said it would be fine [2], so I went ahead with this >>>> approach. >>>> >>>> It will make very hard to break patches if these APIs are introduced and used >>>> in same patch. >>> >>> Thanks, I understand. >>> >>> In that case my suggestion is to, temporarily, add __maybe_unused, >>> which will allow static analysis tools to work more cleanly over the >>> series. It is just a suggestion, not a hard requirement. >>> >>> Probably something along those lines applies to all the >>> review I provided in my previous email. Please use your discretion here. >> >> For now I think I will leave it as it is. Let reviewers review all other >> patches. Let's see if there are any other comments on all the patches in this >> series. If there are any more comments on other patches, then while re-spinning >> next revision I will keep this in mind and try to add __maybe_unused tags in >> all APIs that are used later. > > Sure, that sounds reasonable. > I will be adding __maybe_unused tags to all the helper APIs introduced before the main driver patch. In the main driver patch I will be removing all those __maybe_unused tags and all the helper APIs will be back to their original name (without __maybe_unused tags) >> The idea behind splitting the patches was to get them reviewed individually as >> it is quite difficult to get one big patch reviewed as explained by Jakub. And >> these warnings were expected. If there are any other comments on this series, I >> will try to address all of them together in next revision. > > Yes, I understand. > Thanks for splitting things up into multiple patches. > I know that is a lot of work. But it is very helpful. > >> Meanwhile, Please let me know if you have any comments on other patches >> in this series. > > Will do, but I nothing to add at this time.
On 7/26/2023 9:07 PM, Jakub Kicinski wrote: > On Wed, 26 Jul 2023 16:01:23 +0530 Md Danish Anwar wrote: >> I think MAX_SKB_FRAGS is OK. If the available pool = MAX_SKB_FRAGS we should be >> able to wake the queue. > > MAX_SKB_FRAGS only counts frags and you also need space to map the head, no? > > In general we advise to wait until there's at least 2 * MAX_SKB_FRAGS > to avoid frequent sleep/wake cycles. But IDK how long your queue is, > maybe it's too much. > >> No I don't think any lock is required here. emac_set_port_state() aquires lock >> before updating port status. Also emac_ndo_set_rx_mode_work() is scheduled by a >> singlethreaded workqueue. > > if (netif_running()) outside of any locks is usually a red flag, but if > you're confident it's fine it's fine :) Sure Jakub. I will keep these as it is.
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 v11 of the patch series [v1]. This version of the patchset addresses comments made on v10. There series doesn't have any dependency. 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/ 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 | 3 + drivers/net/ethernet/ti/icssg_classifier.c | 367 ++++ drivers/net/ethernet/ti/icssg_config.c | 450 ++++ drivers/net/ethernet/ti/icssg_config.h | 200 ++ drivers/net/ethernet/ti/icssg_ethtool.c | 154 ++ drivers/net/ethernet/ti/icssg_mii_cfg.c | 120 ++ drivers/net/ethernet/ti/icssg_mii_rt.h | 151 ++ drivers/net/ethernet/ti/icssg_prueth.c | 1919 +++++++++++++++++ drivers/net/ethernet/ti/icssg_prueth.h | 259 +++ drivers/net/ethernet/ti/icssg_queues.c | 38 + drivers/net/ethernet/ti/icssg_stats.c | 57 + drivers/net/ethernet/ti/icssg_stats.h | 158 ++ drivers/net/ethernet/ti/icssg_switch_map.h | 234 ++ 15 files changed, 4307 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml create mode 100644 drivers/net/ethernet/ti/icssg_classifier.c create mode 100644 drivers/net/ethernet/ti/icssg_config.c create mode 100644 drivers/net/ethernet/ti/icssg_config.h create mode 100644 drivers/net/ethernet/ti/icssg_ethtool.c create mode 100644 drivers/net/ethernet/ti/icssg_mii_cfg.c create mode 100644 drivers/net/ethernet/ti/icssg_mii_rt.h create mode 100644 drivers/net/ethernet/ti/icssg_prueth.c create mode 100644 drivers/net/ethernet/ti/icssg_prueth.h create mode 100644 drivers/net/ethernet/ti/icssg_queues.c create mode 100644 drivers/net/ethernet/ti/icssg_stats.c create mode 100644 drivers/net/ethernet/ti/icssg_stats.h create mode 100644 drivers/net/ethernet/ti/icssg_switch_map.h