| Message ID | 20240117161602.153233-1-diogo.ivo@siemens.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for ICSSG-based Ethernet on SR1.0 devices | expand |
On Wed, 17 Jan 2024 16:14:54 +0000 Diogo Ivo wrote:
> net: ti: icssg-config: add SR1.0-specific configuration bits
FWIW looks like this patch didn't make it to the list.
Hello Diogo, On 17/01/2024 18:14, Diogo Ivo wrote: > Hello, > > This series extends the current ICSSG-based Ethernet driver to support > Silicon Revision 1.0 devices. > > Notable differences between the Silicon Revisions are that there is > no TX core in SR1.0 with this being handled by the firmware, requiring > extra DMA channels to communicate commands to the firmware (with the > firmware being different as well) and in the packet classifier. > > The motivation behind it is that a significant number of Siemens > devices containing SR1.0 silicon have been deployed in the field > and need to be supported and updated to newer kernel versions > without losing functionality. Adding SR1.0 support with all its ifdefs makes the driver more complicated than it should be. I think we need to treat SR1.0 and SR2.0 as different devices with their own independent drivers. While the data path is pretty much the same, also like in am65-cpsw-nuss.c, the initialization, firmware and other runtime logic is significantly different. How about introducing a new icssg_prueth_sr1.c and putting all the SR1 stuff there? You could still re-use the other helper files in net/ti/icssg/. It also warrants for it's own Kconfig symbol so it can be built only if required. Any common logic could still be moved to icssg_common.c and re-used in both drivers. > > This series is based on TI's 5.10 SDK [1]. > > The first version of this patch series can be found in [2]. > > [1]: https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/tree/?h=ti-linux-5.10.y > [2]: https://lore.kernel.org/all/20231219174548.3481-1-diogo.ivo@siemens.com/ > > Changes in v2: > - Addressed Krzysztof's comments on the dt-binding > - Removed explicit references to SR2.0 > - Added static keyword as indicated by the kernel test robot > > Diogo Ivo (8): > dt-bindings: net: Add support for AM65x SR1.0 in ICSSG > net: ti: icssg-config: add SR1.0-specific configuration bits > net: ti: icssg-prueth: add SR1.0-specific configuration bits > net: ti: icssg-classifier: Add support for SR1.0 > net: ti: icssg-config: Add SR1.0 configuration functions > net: ti: icssg-ethtool: Adjust channel count for SR1.0 > net: ti: iccsg-prueth: Add necessary functions for SR1.0 support > net: ti: icssg-prueth: Wire up support for SR1.0 > > .../bindings/net/ti,icssg-prueth.yaml | 29 +- > .../net/ethernet/ti/icssg/icssg_classifier.c | 113 +++- > drivers/net/ethernet/ti/icssg/icssg_config.c | 86 ++- > drivers/net/ethernet/ti/icssg/icssg_config.h | 55 ++ > drivers/net/ethernet/ti/icssg/icssg_ethtool.c | 10 +- > drivers/net/ethernet/ti/icssg/icssg_prueth.c | 556 ++++++++++++++++-- > drivers/net/ethernet/ti/icssg/icssg_prueth.h | 21 +- > 7 files changed, 788 insertions(+), 82 deletions(-) >
Hi all, thank you for your input so far. On 1/23/24 12:15, Roger Quadros wrote: > Hello Diogo, > > On 17/01/2024 18:14, Diogo Ivo wrote: >> Hello, >> >> This series extends the current ICSSG-based Ethernet driver to support >> Silicon Revision 1.0 devices. >> >> Notable differences between the Silicon Revisions are that there is >> no TX core in SR1.0 with this being handled by the firmware, requiring >> extra DMA channels to communicate commands to the firmware (with the >> firmware being different as well) and in the packet classifier. >> >> The motivation behind it is that a significant number of Siemens >> devices containing SR1.0 silicon have been deployed in the field >> and need to be supported and updated to newer kernel versions >> without losing functionality. > Adding SR1.0 support with all its ifdefs makes the driver more complicated > than it should be. > > I think we need to treat SR1.0 and SR2.0 as different devices with their > own independent drivers. While the data path is pretty much the same, > also like in am65-cpsw-nuss.c, the initialization, firmware and other > runtime logic is significantly different. > > How about introducing a new icssg_prueth_sr1.c and putting all the SR1 stuff > there? You could still re-use the other helper files in net/ti/icssg/. > It also warrants for it's own Kconfig symbol so it can be built only > if required. > Any common logic could still be moved to icssg_common.c and re-used in both drivers. Yes, that sounds like a more reasonable approach. I will refactor the code and come back with a v3, hopefully with all patches getting sent out :) Best regards, Diogo