| Message ID | 20230724112934.2637802-1-danishanwar@ti.com |
|---|---|
| Headers | show |
| Series | Introduce ICSSG based ethernet Driver | expand |
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 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.
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