| Message ID | cover.1714046812.git.siyanteng@loongson.cn (mailing list archive) |
|---|---|
| Headers | show |
| Series | stmmac: Add Loongson platform support | expand |
Yanteng, On Thu, Apr 25, 2024 at 09:01:53PM +0800, Yanteng Si wrote: > v12: > * The biggest change is the re-splitting of patches. > * Add a "gmac_version" in loongson_data, then we only > read it once in the _probe(). > * Drop Serge's patch. > * Rebase to the latest code state. > * Fixed the gnet commit message. V11 review hasn't finished yet. You posted a question to me just four hours ago, waited for an answer a tiny bit and decided to submit v12. Really, what the rush for? Do you expect the reviewer to react in an instant? Please understand, the review process isn't a quick-road process. The most of the maintainers and reviewers have their own jobs and can't react just at the moment you want it or need it. It's better to collect all the review comments, wait for all questions being answered (ping the person you need if you waited long enough) and resubmit the series with all the notes taken into account. Needlessly rushing and spamming out the maintainers inboxes with your series containing just a part of the requested changes, won't help you much but will likely irritate the reviewers. What do you expect me to do now? Move on with v11 review? Copy my questions to v12 and continue the discussion here? By not waiting for all the discussions done you made the my life harder. What was the point? Sigh... -Serge(y) > > v11: > * Break loongson_phylink_get_caps(), fix bad logic. > * Remove a unnecessary ";". > * Remove some unnecessary "{}". > * add a blank. > * Move the code of fix _force_1000 to patch 6/6. > > The main changes occur in these two functions: > loongson_dwmac_probe(); > loongson_dwmac_setup(); > > v10: > As Andrew's comment: > * Add a #define for the 0x37. > * Add a #define for Port Select. > > others: > * Pick Serge's patch, This patch resulted from the process > of reviewing our patch set. > * Based on Serge's patch, modify our loongson_phylink_get_caps(). > * Drop patch 3/6, we need mac_interface. > * Adjusted the code layout of gnet patch. > * Corrected several errata in commit message. > * Move DISABLE_FORCE flag to loongson_gnet_data(). > > v9: > We have not provided a detailed list of equipment for a long time, > and I apologize for this. During this period, I have collected some > information and now present it to you, hoping to alleviate the pressure > of review. > > 1. IP core > We now have two types of IP cores, one is 0x37, similar to dwmac1000; > The other is 0x10. Compared to 0x37, we split several DMA registers > from one to two, and it is not worth adding a new entry for this. > According to Serge's comment, we made these devices work by overwriting > priv->synopsys_id = 0x37 and mac->dma = <LS_dma_ops>. > > 1.1. Some more detailed information > The number of DMA channels for 0x37 is 1; The number of DMA channels > for 0x10 is 8. Except for channel 0, otherchannels do not support > sending hardware checksums. Supported AV features are Qav, Qat, and Qas, > and the rest are consistent with 3.73. > > 2. DEVICE > We have two types of devices, > one is GMAC, which only has a MAC chip inside and needs an external PHY > chip; > the other is GNET, which integrates both MAC and PHY chips inside. > > 2.1. Some more detailed information > GMAC device: LS7A1000, LS2K1000, these devices do not support any pause > mode. > gnet device: LS7A2000, LS2K2000, the chip connection between the mac and > phy of these devices is not normal and requires two rounds of > negotiation; LS7A2000 does not support half-duplex and > multi-channel; > to enable multi-channel on LS2K2000, you need to turn off > hardware checksum. > **Note**: Only the LS2K2000's IP core is 0x10, while the IP cores of other > devices are 0x37. > > 3. TABLE > > device type pci_id ip_core > ls7a1000 gmac 7a03 0x35/0x37 > ls2k1000 gmac 7a03 0x35/0x37 > ls7a2000 gnet 7a13 0x37 > ls2k2000 gnet 7a13 0x10 > ----------------------------------------------- > Changes: > > * passed the CI > <https://github.com/linux-netdev/nipa/blob/main/tests/patch/checkpatch > /checkpatch.sh> > * reverse xmas tree order. > * Silence build warning. > * Re-split the patch. > * Add more detailed commit message. > * Add more code comment. > * Reduce modification of generic code. > * using the GNET-specific prefix. > * define a new macro for the GNET MAC. > * Use an easier way to overwrite mac. > * Removed some useless printk. > > > v8: > * The biggest change is according to Serge's comment in the previous > edition: > Seeing the patch in the current state would overcomplicate the generic > code and the only functions you need to update are > dwmac_dma_interrupt() > dwmac1000_dma_init_channel() > you can have these methods re-defined with all the Loongson GNET > specifics in the low-level platform driver (dwmac-loongson.c). After > that you can just override the mac_device_info.dma pointer with a > fixed stmmac_dma_ops descriptor. Here is what should be done for that: > > 1. Keep the Patch 4/9 with my comments fixed. First it will be partly > useful for your GNET device. Second in general it's a correct > implementation of the normal DW GMAC v3.x multi-channels feature and > will be useful for the DW GMACs with that feature enabled. > > 2. Create the Loongson GNET-specific > stmmac_dma_ops.dma_interrupt() > stmmac_dma_ops.init_chan() > methods in the dwmac-loongson.c driver. Don't forget to move all the > Loongson-specific macros from dwmac_dma.h to dwmac-loongson.c. > > 3. Create a Loongson GNET-specific platform setup method with the next > semantics: > + allocate stmmac_dma_ops instance and initialize it with > dwmac1000_dma_ops. > + override the stmmac_dma_ops.{dma_interrupt, init_chan} with > the pointers to the methods defined in 2. > + allocate mac_device_info instance and initialize the > mac_device_info.dma field with a pointer to the new > stmmac_dma_ops instance. > + call dwmac1000_setup() or initialize mac_device_info in a way > it's done in dwmac1000_setup() (the later might be better so you > wouldn't need to export the dwmac1000_setup() function). > + override stmmac_priv.synopsys_id with a correct value. > > 4. Initialize plat_stmmacenet_data.setup() with the pointer to the > method created in 3. > > * Others: > Re-split the patch. > Passed checkpatch.pl test. > > v7: > * Refer to andrew's suggestion: > - Add DMA_INTR_ENA_NIE_RX and DMA_INTR_ENA_NIE_TX #define's, etc. > > * Others: > - Using --subject-prefix="PATCH net-next vN" to indicate that the > patches are for the networking tree. > - Rebase to the latest networking tree: > <git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git> > > > v6: > > * Refer to Serge's suggestion: > - Add new platform feature flag: > include/linux/stmmac.h: > +#define STMMAC_FLAG_HAS_LGMAC BIT(13) > > - Add the IRQs macros specific to the Loongson Multi-channels GMAC: > drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h: > +#define DMA_INTR_ENA_NIE_LOONGSON 0x00060000 /* ...*/ > #define DMA_INTR_ENA_NIE 0x00010000 /* Normal Summary */ > ... > > - Drop all of redundant changes that don't require the > prototypes being converted to accepting the stmmac_priv > pointer. > > * Refer to andrew's suggestion: > - Drop white space changes. > - break patch up into lots of smaller parts. > Some small patches have been put into another series as a preparation > see <https://lore.kernel.org/loongarch/cover.1702289232.git.siyanteng@loongson.cn/T/#t> > > *note* : This series of patches relies on the three small patches above. > * others > - Drop irq_flags changes. > - Changed patch order. > > > v4 -> v5: > > * Remove an ugly and useless patch (fix channel number). > * Remove the non-standard dma64 driver code, and also remove > the HWIF entries, since the associated custom callbacks no > longer exist. > * Refer to Serge's suggestion: Update the dwmac1000_dma.c to > support the multi-DMA-channels controller setup. > > See: > v4: <https://lore.kernel.org/loongarch/cover.1692696115.git.chenfeiyang@loongson.cn/> > v3: <https://lore.kernel.org/loongarch/cover.1691047285.git.chenfeiyang@loongson.cn/> > v2: <https://lore.kernel.org/loongarch/cover.1690439335.git.chenfeiyang@loongson.cn/> > v1: <https://lore.kernel.org/loongarch/cover.1689215889.git.chenfeiyang@loongson.cn/> > > Yanteng Si (15): > net: stmmac: Move the atds flag to the stmmac_dma_cfg structure > net: stmmac: Add multi-channel support > net: stmmac: Export dwmac1000_dma_ops > net: stmmac: dwmac-loongson: Drop useless platform data > net: stmmac: dwmac-loongson: Use PCI_DEVICE_DATA() macro for device > identification > net: stmmac: dwmac-loongson: Split up the platform data initialization > net: stmmac: dwmac-loongson: Add ref and ptp clocks for Loongson > net: stmmac: dwmac-loongson: Add phy mask for Loongson GMAC > net: stmmac: dwmac-loongson: Add phy_interface for Loongson GMAC > net: stmmac: dwmac-loongson: Add full PCI support > net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > net: stmmac: dwmac-loongson: Fixed failure to set network speed to > 1000. > net: stmmac: dwmac-loongson: Add Loongson GNET support > net: stmmac: dwmac-loongson: Move disable_force flag to _gnet_date > net: stmmac: dwmac-loongson: Add loongson module author > > drivers/net/ethernet/stmicro/stmmac/common.h | 1 + > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 519 ++++++++++++++++-- > .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 4 +- > .../ethernet/stmicro/stmmac/dwmac1000_dma.c | 35 +- > .../ethernet/stmicro/stmmac/dwmac100_dma.c | 2 +- > .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 2 +- > .../net/ethernet/stmicro/stmmac/dwmac_dma.h | 20 +- > .../net/ethernet/stmicro/stmmac/dwmac_lib.c | 30 +- > .../ethernet/stmicro/stmmac/dwxgmac2_dma.c | 2 +- > drivers/net/ethernet/stmicro/stmmac/hwif.h | 5 +- > .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 6 + > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +- > include/linux/stmmac.h | 2 + > 13 files changed, 536 insertions(+), 103 deletions(-) > > -- > 2.31.4 >
Hi Serge, 在 2024/4/25 21:19, Serge Semin 写道: >> v12: >> * The biggest change is the re-splitting of patches. >> * Add a "gmac_version" in loongson_data, then we only >> read it once in the _probe(). >> * Drop Serge's patch. >> * Rebase to the latest code state. >> * Fixed the gnet commit message. > V11 review hasn't finished yet. You posted a question to me just four > hours ago, waited for an answer a tiny bit and decided to submit v12. > Really, what the rush for? Do you expect the reviewer to react in an > instant? I'm sorry. It's my fault. I did this because I didn't want to repeat the v8 process, we talked about v8 for two months, after I collected all the comments and changed the code, a lot of changes happened, and I seemed to misunderstand the comments about patch splitting, which made v9-v11 look bad. v12 is actually still based on v8, but it's just resplit the patches again, maybe it's easier to review, > > Please understand, the review process isn't a quick-road process. The > most of the maintainers and reviewers have their own jobs and can't > react just at the moment you want it or need it. It's better to Yes, I quite agree with you. In fact, we have been working together happily for almost a year. I appreciate your patience. With your help, this patch set has gotten better and better since the beginning. > collect all the review comments, wait for all questions being answered > (ping the person you need if you waited long enough) and resubmit the Yes, I understand, because I also do some kernel document translation in my spare time, and I understand this very well. > series with all the notes taken into account. Needlessly rushing and > spamming out the maintainers inboxes with your series containing just > a part of the requested changes, won't help you much but will likely > irritate the reviewers. Ok, I will reduce the frequency of my emails unless all comments are clearly answered. > > What do you expect me to do now? Move on with v11 review? Copy my > questions to v12 and continue the discussion here? By not waiting for > all the discussions done you made the my life harder. What was the > point? Sigh... v11 is not much different from v12, except that it removes your patch and then resplits the patch, which improves the review efficiency to some extent. loongson_dwmac_config_multi_msi() is the only comment left that didn't end in v11. I originally wanted to include this question in the cover letter of v12, but I did send it in a hurry and lost it. I'm sorry about that, let me copy this question to v12. Thanks, Yanteng
On Fri, Apr 26, 2024 at 12:55:34PM +0800, Yanteng Si wrote: > Hi Serge, > > 在 2024/4/25 21:19, Serge Semin 写道: > > > v12: > > > * The biggest change is the re-splitting of patches. > > > * Add a "gmac_version" in loongson_data, then we only > > > read it once in the _probe(). > > > * Drop Serge's patch. > > > * Rebase to the latest code state. > > > * Fixed the gnet commit message. > > V11 review hasn't finished yet. You posted a question to me just four > > hours ago, waited for an answer a tiny bit and decided to submit v12. > > Really, what the rush for? Do you expect the reviewer to react in an > > instant? > > I'm sorry. It's my fault. > > > I did this because I didn't want to repeat the v8 process, we talked about > v8 for > > two months, after I collected all the comments and changed the code, a lot > of > > changes happened, and I seemed to misunderstand the comments about patch > > splitting, which made v9-v11 look bad. > > > v12 is actually still based on v8, but it's just resplit the patches again, > maybe > > it's easier to review, > > > > > Please understand, the review process isn't a quick-road process. The > > most of the maintainers and reviewers have their own jobs and can't > > react just at the moment you want it or need it. It's better to > > Yes, I quite agree with you. In fact, we have been working together happily > for > > almost a year. I appreciate your patience. With your help, this patch set > has > > gotten better and better since the beginning. > > > collect all the review comments, wait for all questions being answered > > (ping the person you need if you waited long enough) and resubmit the > > Yes, I understand, because I also do some kernel document translation in my > > spare time, and I understand this very well. > > > series with all the notes taken into account. Needlessly rushing and > > spamming out the maintainers inboxes with your series containing just > > a part of the requested changes, won't help you much but will likely > > irritate the reviewers. > Ok, I will reduce the frequency of my emails unless all comments are clearly > answered. Not all emails, but just the series _resubmissions_. It's much better not to rush, make sure you got all the comments correctly and resubmit only then. Should you have any doubts, just ask and wait for some time (day-two-three). It will be easier and much less troublesome to fully clarify things first and then send out a close-to-what-was-requested patchset - less emails traffic in the inboxes, a discussion concentrated in a single place in the LKML archive, more happy reviewers/maintainers - less meticulous notes.) > > > > What do you expect me to do now? Move on with v11 review? Copy my > > questions to v12 and continue the discussion here? By not waiting for > > all the discussions done you made the my life harder. What was the > > point? Sigh... > > v11 is not much different from v12, except that it removes your patch and > then > > resplits the patch, which improves the review efficiency to some extent. > > > loongson_dwmac_config_multi_msi() is the only comment left that didn't end > in > > v11. I originally wanted to include this question in the cover letter of > v12, but I > > did send it in a hurry and lost it. I'm sorry about that, let me copy > this question > > to v12. Ok. Thanks for re-submitting the MSI-related discussion to the respective patch. I'll revise the rest of the v11 discussions anyway so not to miss the notes gist and get back to reviewing your series in 2-4 days. -Serge(y) > > > Thanks, > > Yanteng > >
v12: * The biggest change is the re-splitting of patches. * Add a "gmac_version" in loongson_data, then we only read it once in the _probe(). * Drop Serge's patch. * Rebase to the latest code state. * Fixed the gnet commit message. v11: * Break loongson_phylink_get_caps(), fix bad logic. * Remove a unnecessary ";". * Remove some unnecessary "{}". * add a blank. * Move the code of fix _force_1000 to patch 6/6. The main changes occur in these two functions: loongson_dwmac_probe(); loongson_dwmac_setup(); v10: As Andrew's comment: * Add a #define for the 0x37. * Add a #define for Port Select. others: * Pick Serge's patch, This patch resulted from the process of reviewing our patch set. * Based on Serge's patch, modify our loongson_phylink_get_caps(). * Drop patch 3/6, we need mac_interface. * Adjusted the code layout of gnet patch. * Corrected several errata in commit message. * Move DISABLE_FORCE flag to loongson_gnet_data(). v9: We have not provided a detailed list of equipment for a long time, and I apologize for this. During this period, I have collected some information and now present it to you, hoping to alleviate the pressure of review. 1. IP core We now have two types of IP cores, one is 0x37, similar to dwmac1000; The other is 0x10. Compared to 0x37, we split several DMA registers from one to two, and it is not worth adding a new entry for this. According to Serge's comment, we made these devices work by overwriting priv->synopsys_id = 0x37 and mac->dma = <LS_dma_ops>. 1.1. Some more detailed information The number of DMA channels for 0x37 is 1; The number of DMA channels for 0x10 is 8. Except for channel 0, otherchannels do not support sending hardware checksums. Supported AV features are Qav, Qat, and Qas, and the rest are consistent with 3.73. 2. DEVICE We have two types of devices, one is GMAC, which only has a MAC chip inside and needs an external PHY chip; the other is GNET, which integrates both MAC and PHY chips inside. 2.1. Some more detailed information GMAC device: LS7A1000, LS2K1000, these devices do not support any pause mode. gnet device: LS7A2000, LS2K2000, the chip connection between the mac and phy of these devices is not normal and requires two rounds of negotiation; LS7A2000 does not support half-duplex and multi-channel; to enable multi-channel on LS2K2000, you need to turn off hardware checksum. **Note**: Only the LS2K2000's IP core is 0x10, while the IP cores of other devices are 0x37. 3. TABLE device type pci_id ip_core ls7a1000 gmac 7a03 0x35/0x37 ls2k1000 gmac 7a03 0x35/0x37 ls7a2000 gnet 7a13 0x37 ls2k2000 gnet 7a13 0x10 ----------------------------------------------- Changes: * passed the CI <https://github.com/linux-netdev/nipa/blob/main/tests/patch/checkpatch /checkpatch.sh> * reverse xmas tree order. * Silence build warning. * Re-split the patch. * Add more detailed commit message. * Add more code comment. * Reduce modification of generic code. * using the GNET-specific prefix. * define a new macro for the GNET MAC. * Use an easier way to overwrite mac. * Removed some useless printk. v8: * The biggest change is according to Serge's comment in the previous edition: Seeing the patch in the current state would overcomplicate the generic code and the only functions you need to update are dwmac_dma_interrupt() dwmac1000_dma_init_channel() you can have these methods re-defined with all the Loongson GNET specifics in the low-level platform driver (dwmac-loongson.c). After that you can just override the mac_device_info.dma pointer with a fixed stmmac_dma_ops descriptor. Here is what should be done for that: 1. Keep the Patch 4/9 with my comments fixed. First it will be partly useful for your GNET device. Second in general it's a correct implementation of the normal DW GMAC v3.x multi-channels feature and will be useful for the DW GMACs with that feature enabled. 2. Create the Loongson GNET-specific stmmac_dma_ops.dma_interrupt() stmmac_dma_ops.init_chan() methods in the dwmac-loongson.c driver. Don't forget to move all the Loongson-specific macros from dwmac_dma.h to dwmac-loongson.c. 3. Create a Loongson GNET-specific platform setup method with the next semantics: + allocate stmmac_dma_ops instance and initialize it with dwmac1000_dma_ops. + override the stmmac_dma_ops.{dma_interrupt, init_chan} with the pointers to the methods defined in 2. + allocate mac_device_info instance and initialize the mac_device_info.dma field with a pointer to the new stmmac_dma_ops instance. + call dwmac1000_setup() or initialize mac_device_info in a way it's done in dwmac1000_setup() (the later might be better so you wouldn't need to export the dwmac1000_setup() function). + override stmmac_priv.synopsys_id with a correct value. 4. Initialize plat_stmmacenet_data.setup() with the pointer to the method created in 3. * Others: Re-split the patch. Passed checkpatch.pl test. v7: * Refer to andrew's suggestion: - Add DMA_INTR_ENA_NIE_RX and DMA_INTR_ENA_NIE_TX #define's, etc. * Others: - Using --subject-prefix="PATCH net-next vN" to indicate that the patches are for the networking tree. - Rebase to the latest networking tree: <git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git> v6: * Refer to Serge's suggestion: - Add new platform feature flag: include/linux/stmmac.h: +#define STMMAC_FLAG_HAS_LGMAC BIT(13) - Add the IRQs macros specific to the Loongson Multi-channels GMAC: drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h: +#define DMA_INTR_ENA_NIE_LOONGSON 0x00060000 /* ...*/ #define DMA_INTR_ENA_NIE 0x00010000 /* Normal Summary */ ... - Drop all of redundant changes that don't require the prototypes being converted to accepting the stmmac_priv pointer. * Refer to andrew's suggestion: - Drop white space changes. - break patch up into lots of smaller parts. Some small patches have been put into another series as a preparation see <https://lore.kernel.org/loongarch/cover.1702289232.git.siyanteng@loongson.cn/T/#t> *note* : This series of patches relies on the three small patches above. * others - Drop irq_flags changes. - Changed patch order. v4 -> v5: * Remove an ugly and useless patch (fix channel number). * Remove the non-standard dma64 driver code, and also remove the HWIF entries, since the associated custom callbacks no longer exist. * Refer to Serge's suggestion: Update the dwmac1000_dma.c to support the multi-DMA-channels controller setup. See: v4: <https://lore.kernel.org/loongarch/cover.1692696115.git.chenfeiyang@loongson.cn/> v3: <https://lore.kernel.org/loongarch/cover.1691047285.git.chenfeiyang@loongson.cn/> v2: <https://lore.kernel.org/loongarch/cover.1690439335.git.chenfeiyang@loongson.cn/> v1: <https://lore.kernel.org/loongarch/cover.1689215889.git.chenfeiyang@loongson.cn/> Yanteng Si (15): net: stmmac: Move the atds flag to the stmmac_dma_cfg structure net: stmmac: Add multi-channel support net: stmmac: Export dwmac1000_dma_ops net: stmmac: dwmac-loongson: Drop useless platform data net: stmmac: dwmac-loongson: Use PCI_DEVICE_DATA() macro for device identification net: stmmac: dwmac-loongson: Split up the platform data initialization net: stmmac: dwmac-loongson: Add ref and ptp clocks for Loongson net: stmmac: dwmac-loongson: Add phy mask for Loongson GMAC net: stmmac: dwmac-loongson: Add phy_interface for Loongson GMAC net: stmmac: dwmac-loongson: Add full PCI support net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy net: stmmac: dwmac-loongson: Fixed failure to set network speed to 1000. net: stmmac: dwmac-loongson: Add Loongson GNET support net: stmmac: dwmac-loongson: Move disable_force flag to _gnet_date net: stmmac: dwmac-loongson: Add loongson module author drivers/net/ethernet/stmicro/stmmac/common.h | 1 + .../ethernet/stmicro/stmmac/dwmac-loongson.c | 519 ++++++++++++++++-- .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 4 +- .../ethernet/stmicro/stmmac/dwmac1000_dma.c | 35 +- .../ethernet/stmicro/stmmac/dwmac100_dma.c | 2 +- .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 2 +- .../net/ethernet/stmicro/stmmac/dwmac_dma.h | 20 +- .../net/ethernet/stmicro/stmmac/dwmac_lib.c | 30 +- .../ethernet/stmicro/stmmac/dwxgmac2_dma.c | 2 +- drivers/net/ethernet/stmicro/stmmac/hwif.h | 5 +- .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 6 + .../net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +- include/linux/stmmac.h | 2 + 13 files changed, 536 insertions(+), 103 deletions(-)