| Message ID | e0ea692698171f9c69b80a70607a55805d249c4a.1714046812.git.siyanteng@loongson.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | stmmac: Add Loongson platform support | expand |
> [PATCH net-next v12 06/15] net: stmmac: dwmac-loongson: Split up the platform data initialization Please convert the subject to being more specific, like this: net: stmmac: dwmac-loongson: Detach GMAC-specific platform data init On Thu, Apr 25, 2024 at 09:04:37PM +0800, Yanteng Si wrote: > Based on IP core classification, loongson has two types of network What is the real company name? At least start the name with the capital letter. * everywhere > devices: GMAC and GNET. GMAC's ip_core id is 0x35/0x37, while GNET's > ip_core id is 0x37/0x10. s/ip_core/IP-core Once again the IP-core ID isn't _hex_, but a number of the format: "v+Major.Minor" so use the _real_ IP-core version number everywhere. Note mentioning that some of your GNET device has the GMAC_VERSION.SNPSVER hardwired to 0x10 is completely redundant in this and many other context. The only place where it's relevant is the patch(es) where you have the Snps ID override. > > Device tables: > > device type pci_id snps_id channel > ls2k1000 gmac 7a03 0x35/0x37 1 > ls7a1000 gmac 7a03 0x35/0x37 1 > ls2k2000 gnet 7a13 0x10 8 > ls7a2000 gnet 7a13 0x37 1 s/gmac/GMAC s/gnet/GNET s/pci_id/PCI Dev ID s/snsp_id/Synopys Version s/channels/DMA-channels s/ls2k/LS2K s/ls7a/LS7A * everywhere > > The GMAC device only has a MAC chip inside and needs an > external PHY chip; > > To later distinguish 8-channel gnet devices from single-channel > gnet/gmac devices, move rx_queues_to_use loongson_default_data > to loongson_dwmac_probe(). Also move mac_interface to > loongson_default_data(). Again. This is only a part of the reason why you need this change. The main reason is to provide the two-leveled platform data init functions: fist one is the common method initializing the data common for both GMAC and GNET, second one is the device-specific data initializer. To sum up I would change the commit log to something like this: "Loongson delivers two types of the network devices: Loongson GMAC and Loongson GNET in the framework of four CPU/Chipsets revisions: Chip Network PCI Dev ID Synopys Version DMA-channel LS2K1000 CPU GMAC 0x7a03 v3.50a/v3.73a 1 LS7A1000 Chipset GMAC 0x7a03 v3.50a/v3.73a 1 LS2K2000 CPU GNET 0x7a13 v3.73a 8 LS7A2000 Chipset GNET 0x7a13 v3.73a 1 The driver currently supports the chips with the Loongson GMAC network device. As a preparation before adding the Loongson GNET support detach the Loongson GMAC-specific platform data initializations to the loongson_gmac_data() method and preserve the common settings in the loongson_default_data(). While at it drop the return value statement from the loongson_default_data() method as redundant." > > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > --- > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 20 ++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > index 4e0838db4259..904e288d0be0 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > @@ -11,22 +11,20 @@ > > #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 > > -static int loongson_default_data(struct plat_stmmacenet_data *plat) > +static void loongson_default_data(struct plat_stmmacenet_data *plat) > { > plat->clk_csr = 2; /* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */ > plat->has_gmac = 1; > plat->force_sf_dma_mode = 1; > > + plat->mac_interface = PHY_INTERFACE_MODE_GMII; > + I double-checked this part in my HW and in the databooks. DW GMAC with _RGMII_ PHY-interfaces can't be equipped with a PCS (STMMAC driver is wrong in considering otherwise at least in the Auto-negotiation part). PCS is only available for the RTI, RTBI and SGMII interfaces. You can double-check that by checking out the DMA_HW_FEATURE.PCSSEL flag state. I'll be surprised if it's set in your case. If it isn't then either drop the plat_stmmacenet_data::mac_interface initialization or (as Russell suggested) initialize it with PHY_INTERFACE_MODE_NA. But do that in a separate pre-requisite patch! > /* Set default value for unicast filter entries */ > plat->unicast_filter_entries = 1; > > /* Set the maxmtu to a default of JUMBO_LEN */ > plat->maxmtu = JUMBO_LEN; > > - /* Set default number of RX and TX queues to use */ > - plat->tx_queues_to_use = 1; > - plat->rx_queues_to_use = 1; > - > /* Disable Priority config by default */ > plat->tx_queues_cfg[0].use_prio = false; > plat->rx_queues_cfg[0].use_prio = false; > @@ -41,6 +39,12 @@ static int loongson_default_data(struct plat_stmmacenet_data *plat) > plat->dma_cfg->pblx8 = true; > > plat->multicast_filter_bins = 256; > +} > + > +static int loongson_gmac_data(struct plat_stmmacenet_data *plat) > +{ > + loongson_default_data(plat); > + > return 0; > } > > @@ -109,11 +113,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > } > > plat->phy_interface = phy_mode; > - plat->mac_interface = PHY_INTERFACE_MODE_GMII; > > pci_set_master(pdev); > > - loongson_default_data(plat); > + loongson_gmac_data(plat); > pci_enable_msi(pdev); > memset(&res, 0, sizeof(res)); > res.addr = pcim_iomap_table(pdev)[0]; > @@ -138,6 +141,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > goto err_disable_msi; > } > > + plat->tx_queues_to_use = 1; > + plat->rx_queues_to_use = 1; > + You can freely move this to loongson_gmac_data() method. And then, in the patch adding the GNET-support, you'll be able to provide these fields initialization in the loongson_gnet_data() method together with the plat->tx_queues_cfg[*].coe_unsupported flag init. Thus the probe() method will get to be smaller and easier to read, and the loongson_*_data() method will be more coherent. -Serge(y) > ret = stmmac_dvr_probe(&pdev->dev, plat, &res); > if (ret) > goto err_disable_msi; > -- > 2.31.4 >
在 2024/5/4 02:08, Serge Semin 写道: >> [PATCH net-next v12 06/15] net: stmmac: dwmac-loongson: Split up the platform data initialization > Please convert the subject to being more specific, like this: > > net: stmmac: dwmac-loongson: Detach GMAC-specific platform data init > > On Thu, Apr 25, 2024 at 09:04:37PM +0800, Yanteng Si wrote: >> Based on IP core classification, loongson has two types of network > What is the real company name? At least start the name with the > capital letter. > * everywhere OK, LOONGSON > >> devices: GMAC and GNET. GMAC's ip_core id is 0x35/0x37, while GNET's >> ip_core id is 0x37/0x10. > s/ip_core/IP-core > > Once again the IP-core ID isn't _hex_, but a number of the format: > "v+Major.Minor" > so use the _real_ IP-core version number everywhere. Note mentioning > that some of your GNET device has the GMAC_VERSION.SNPSVER hardwired > to 0x10 is completely redundant in this and many other context. The > only place where it's relevant is the patch(es) where you have the > Snps ID override. OK. > >> Device tables: >> >> device type pci_id snps_id channel >> ls2k1000 gmac 7a03 0x35/0x37 1 >> ls7a1000 gmac 7a03 0x35/0x37 1 >> ls2k2000 gnet 7a13 0x10 8 >> ls7a2000 gnet 7a13 0x37 1 > s/gmac/GMAC > s/gnet/GNET > s/pci_id/PCI Dev ID > s/snsp_id/Synopys Version > s/channels/DMA-channels > s/ls2k/LS2K > s/ls7a/LS7A > > * everywhere OK. > >> The GMAC device only has a MAC chip inside and needs an >> external PHY chip; >> >> To later distinguish 8-channel gnet devices from single-channel >> gnet/gmac devices, move rx_queues_to_use loongson_default_data >> to loongson_dwmac_probe(). Also move mac_interface to >> loongson_default_data(). > Again. This is only a part of the reason why you need this change. > The main reason is to provide the two-leveled platform data init > functions: fist one is the common method initializing the data common > for both GMAC and GNET, second one is the device-specific data > initializer. > > To sum up I would change the commit log to something like this: > > "Loongson delivers two types of the network devices: Loongson GMAC and > Loongson GNET in the framework of four CPU/Chipsets revisions: > > Chip Network PCI Dev ID Synopys Version DMA-channel > LS2K1000 CPU GMAC 0x7a03 v3.50a/v3.73a 1 > LS7A1000 Chipset GMAC 0x7a03 v3.50a/v3.73a 1 > LS2K2000 CPU GNET 0x7a13 v3.73a 8 > LS7A2000 Chipset GNET 0x7a13 v3.73a 1 > > The driver currently supports the chips with the Loongson GMAC network > device. As a preparation before adding the Loongson GNET support > detach the Loongson GMAC-specific platform data initializations to the > loongson_gmac_data() method and preserve the common settings in the > loongson_default_data(). > > While at it drop the return value statement from the > loongson_default_data() method as redundant." OK, Thanks! > >> Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> >> Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> >> Signed-off-by: Yanteng Si <siyanteng@loongson.cn> >> --- >> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 20 ++++++++++++------- >> 1 file changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> index 4e0838db4259..904e288d0be0 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> @@ -11,22 +11,20 @@ >> >> #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 >> >> -static int loongson_default_data(struct plat_stmmacenet_data *plat) >> +static void loongson_default_data(struct plat_stmmacenet_data *plat) >> { >> plat->clk_csr = 2; /* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */ >> plat->has_gmac = 1; >> plat->force_sf_dma_mode = 1; >> >> + plat->mac_interface = PHY_INTERFACE_MODE_GMII; >> + > I double-checked this part in my HW and in the databooks. DW GMAC with > _RGMII_ PHY-interfaces can't be equipped with a PCS (STMMAC driver is > wrong in considering otherwise at least in the Auto-negotiation part). > PCS is only available for the RTI, RTBI and SGMII interfaces. > > You can double-check that by checking out the DMA_HW_FEATURE.PCSSEL > flag state. I'll be surprised if it's set in your case. If it isn't > then either drop the plat_stmmacenet_data::mac_interface > initialization or (as Russell suggested) initialize it with > PHY_INTERFACE_MODE_NA. But do that in a separate pre-requisite patch! OK. > >> /* Set default value for unicast filter entries */ >> plat->unicast_filter_entries = 1; >> >> /* Set the maxmtu to a default of JUMBO_LEN */ >> plat->maxmtu = JUMBO_LEN; >> >> - /* Set default number of RX and TX queues to use */ >> - plat->tx_queues_to_use = 1; >> - plat->rx_queues_to_use = 1; >> - >> /* Disable Priority config by default */ >> plat->tx_queues_cfg[0].use_prio = false; >> plat->rx_queues_cfg[0].use_prio = false; >> @@ -41,6 +39,12 @@ static int loongson_default_data(struct plat_stmmacenet_data *plat) >> plat->dma_cfg->pblx8 = true; >> >> plat->multicast_filter_bins = 256; >> +} >> + >> +static int loongson_gmac_data(struct plat_stmmacenet_data *plat) >> +{ >> + loongson_default_data(plat); >> + >> return 0; >> } >> >> @@ -109,11 +113,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id >> } >> >> plat->phy_interface = phy_mode; >> - plat->mac_interface = PHY_INTERFACE_MODE_GMII; >> >> pci_set_master(pdev); >> >> - loongson_default_data(plat); >> + loongson_gmac_data(plat); >> pci_enable_msi(pdev); >> memset(&res, 0, sizeof(res)); >> res.addr = pcim_iomap_table(pdev)[0]; >> @@ -138,6 +141,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id >> goto err_disable_msi; >> } >> >> + plat->tx_queues_to_use = 1; >> + plat->rx_queues_to_use = 1; >> + > You can freely move this to loongson_gmac_data() method. And then, in > the patch adding the GNET-support, you'll be able to provide these fields > initialization in the loongson_gnet_data() method together with the > plat->tx_queues_cfg[*].coe_unsupported flag init. Thus the probe() > method will get to be smaller and easier to read, and the > loongson_*_data() method will be more coherent. As you said, at first glance, putting them in loongson_gnet_data() method is fine, but in LS2K2000: plat->rx_queues_to_use = CHANNEL_NUM; // CHANNEL_NUM = 8; plat->tx_queues_to_use = CHANNEL_NUM; So we need to distinguish between them. At the same time, we have to distinguish between LS2K2000 in probe() method. Why not put them inside probe, which will save a lot of duplicate code, like this: struct stmmac_resources res; struct loongson_data *ld; ... memset(&res, 0, sizeof(res)); res.addr = pcim_iomap_table(pdev)[0]; ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; switch (ld->gmac_verion) { case LOONGSON_DWMAC_CORE_1_00: plat->rx_queues_to_use = CHANNEL_NUM; plat->tx_queues_to_use = CHANNEL_NUM; /* Only channel 0 supports checksum, * so turn off checksum to enable multiple channels. */ for (i = 1; i < CHANNEL_NUM; i++) plat->tx_queues_cfg[i].coe_unsupported = 1; ret = loongson_dwmac_config_msi(pdev, plat, &res, np); break; default: /* 0x35 device and 0x37 device. */ plat->tx_queues_to_use = 1; plat->rx_queues_to_use = 1; ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); break; } if (ret) goto err_disable_device; What do you think? Of course, if you insist, I'm willing to repeat this in the loongson_gnet_data() method. Thanks, Yanteng
On Mon, May 13, 2024 at 7:07 PM Yanteng Si <siyanteng@loongson.cn> wrote: > > > 在 2024/5/4 02:08, Serge Semin 写道: > >> [PATCH net-next v12 06/15] net: stmmac: dwmac-loongson: Split up the platform data initialization > > Please convert the subject to being more specific, like this: > > > > net: stmmac: dwmac-loongson: Detach GMAC-specific platform data init > > > > On Thu, Apr 25, 2024 at 09:04:37PM +0800, Yanteng Si wrote: > >> Based on IP core classification, loongson has two types of network > > What is the real company name? At least start the name with the > > capital letter. > > * everywhere > OK, LOONGSON > > > >> devices: GMAC and GNET. GMAC's ip_core id is 0x35/0x37, while GNET's > >> ip_core id is 0x37/0x10. > > s/ip_core/IP-core > > > > Once again the IP-core ID isn't _hex_, but a number of the format: > > "v+Major.Minor" > > so use the _real_ IP-core version number everywhere. Note mentioning > > that some of your GNET device has the GMAC_VERSION.SNPSVER hardwired > > to 0x10 is completely redundant in this and many other context. The > > only place where it's relevant is the patch(es) where you have the > > Snps ID override. > OK. > > > >> Device tables: > >> > >> device type pci_id snps_id channel > >> ls2k1000 gmac 7a03 0x35/0x37 1 > >> ls7a1000 gmac 7a03 0x35/0x37 1 > >> ls2k2000 gnet 7a13 0x10 8 > >> ls7a2000 gnet 7a13 0x37 1 > > s/gmac/GMAC > > s/gnet/GNET > > s/pci_id/PCI Dev ID > > s/snsp_id/Synopys Version > > s/channels/DMA-channels > > s/ls2k/LS2K > > s/ls7a/LS7A > > > > * everywhere > OK. > > > >> The GMAC device only has a MAC chip inside and needs an > >> external PHY chip; > >> > >> To later distinguish 8-channel gnet devices from single-channel > >> gnet/gmac devices, move rx_queues_to_use loongson_default_data > >> to loongson_dwmac_probe(). Also move mac_interface to > >> loongson_default_data(). > > Again. This is only a part of the reason why you need this change. > > The main reason is to provide the two-leveled platform data init > > functions: fist one is the common method initializing the data common > > for both GMAC and GNET, second one is the device-specific data > > initializer. > > > > To sum up I would change the commit log to something like this: > > > > "Loongson delivers two types of the network devices: Loongson GMAC and > > Loongson GNET in the framework of four CPU/Chipsets revisions: > > > > Chip Network PCI Dev ID Synopys Version DMA-channel > > LS2K1000 CPU GMAC 0x7a03 v3.50a/v3.73a 1 > > LS7A1000 Chipset GMAC 0x7a03 v3.50a/v3.73a 1 > > LS2K2000 CPU GNET 0x7a13 v3.73a 8 > > LS7A2000 Chipset GNET 0x7a13 v3.73a 1 > > > > The driver currently supports the chips with the Loongson GMAC network > > device. As a preparation before adding the Loongson GNET support > > detach the Loongson GMAC-specific platform data initializations to the > > loongson_gmac_data() method and preserve the common settings in the > > loongson_default_data(). > > > > While at it drop the return value statement from the > > loongson_default_data() method as redundant." > OK, Thanks! > > > >> Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > >> Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > >> Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > >> --- > >> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 20 ++++++++++++------- > >> 1 file changed, 13 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> index 4e0838db4259..904e288d0be0 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> @@ -11,22 +11,20 @@ > >> > >> #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 > >> > >> -static int loongson_default_data(struct plat_stmmacenet_data *plat) > >> +static void loongson_default_data(struct plat_stmmacenet_data *plat) > >> { > >> plat->clk_csr = 2; /* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */ > >> plat->has_gmac = 1; > >> plat->force_sf_dma_mode = 1; > >> > >> + plat->mac_interface = PHY_INTERFACE_MODE_GMII; > >> + > > I double-checked this part in my HW and in the databooks. DW GMAC with > > _RGMII_ PHY-interfaces can't be equipped with a PCS (STMMAC driver is > > wrong in considering otherwise at least in the Auto-negotiation part). > > PCS is only available for the RTI, RTBI and SGMII interfaces. > > > > You can double-check that by checking out the DMA_HW_FEATURE.PCSSEL > > flag state. I'll be surprised if it's set in your case. If it isn't > > then either drop the plat_stmmacenet_data::mac_interface > > initialization or (as Russell suggested) initialize it with > > PHY_INTERFACE_MODE_NA. But do that in a separate pre-requisite patch! > OK. > > > >> /* Set default value for unicast filter entries */ > >> plat->unicast_filter_entries = 1; > >> > >> /* Set the maxmtu to a default of JUMBO_LEN */ > >> plat->maxmtu = JUMBO_LEN; > >> > >> - /* Set default number of RX and TX queues to use */ > >> - plat->tx_queues_to_use = 1; > >> - plat->rx_queues_to_use = 1; > >> - > >> /* Disable Priority config by default */ > >> plat->tx_queues_cfg[0].use_prio = false; > >> plat->rx_queues_cfg[0].use_prio = false; > >> @@ -41,6 +39,12 @@ static int loongson_default_data(struct plat_stmmacenet_data *plat) > >> plat->dma_cfg->pblx8 = true; > >> > >> plat->multicast_filter_bins = 256; > >> +} > >> + > >> +static int loongson_gmac_data(struct plat_stmmacenet_data *plat) > >> +{ > >> + loongson_default_data(plat); > >> + > >> return 0; > >> } > >> > >> @@ -109,11 +113,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > >> } > >> > >> plat->phy_interface = phy_mode; > >> - plat->mac_interface = PHY_INTERFACE_MODE_GMII; > >> > >> pci_set_master(pdev); > >> > >> - loongson_default_data(plat); > >> + loongson_gmac_data(plat); > >> pci_enable_msi(pdev); > >> memset(&res, 0, sizeof(res)); > >> res.addr = pcim_iomap_table(pdev)[0]; > >> @@ -138,6 +141,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > >> goto err_disable_msi; > >> } > >> > >> + plat->tx_queues_to_use = 1; > >> + plat->rx_queues_to_use = 1; > >> + > > You can freely move this to loongson_gmac_data() method. And then, in > > the patch adding the GNET-support, you'll be able to provide these fields > > initialization in the loongson_gnet_data() method together with the > > plat->tx_queues_cfg[*].coe_unsupported flag init. Thus the probe() > > method will get to be smaller and easier to read, and the > > loongson_*_data() method will be more coherent. > > As you said, at first glance, putting them in loongson_gnet_data() > method is fine, > > but in LS2K2000: > > plat->rx_queues_to_use = CHANNEL_NUM; // CHANNEL_NUM = 8; > plat->tx_queues_to_use = CHANNEL_NUM; > > So we need to distinguish between them. At the same time, we have to > distinguish > > between LS2K2000 in probe() method. Why not put them inside probe, which > will > > save a lot of duplicate code, like this: > > struct stmmac_resources res; > struct loongson_data *ld; > > ... > > memset(&res, 0, sizeof(res)); > res.addr = pcim_iomap_table(pdev)[0]; > ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > > switch (ld->gmac_verion) { > case LOONGSON_DWMAC_CORE_1_00: > plat->rx_queues_to_use = CHANNEL_NUM; > plat->tx_queues_to_use = CHANNEL_NUM; > > /* Only channel 0 supports checksum, > * so turn off checksum to enable multiple channels. > */ > for (i = 1; i < CHANNEL_NUM; i++) > plat->tx_queues_cfg[i].coe_unsupported = 1; > > ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > break; > default: /* 0x35 device and 0x37 device. */ > plat->tx_queues_to_use = 1; > plat->rx_queues_to_use = 1; > > ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > break; > } > if (ret) > goto err_disable_device; > > > What do you think? > > > Of course, if you insist, I'm willing to repeat this in the > > loongson_gnet_data() method. In my opinion, it is the most elegant method by reading gmac_version only once in the probe function, and save it in loongson_data for other functions to use. So, it is good for me to put the switch-case in the probe function. Huacai > > > > Thanks, > > Yanteng >
On Mon, May 13, 2024 at 07:07:38PM +0800, Yanteng Si wrote: > > 在 2024/5/4 02:08, Serge Semin 写道: > > > [PATCH net-next v12 06/15] net: stmmac: dwmac-loongson: Split up the platform data initialization > > Please convert the subject to being more specific, like this: > > > > net: stmmac: dwmac-loongson: Detach GMAC-specific platform data init > > > > On Thu, Apr 25, 2024 at 09:04:37PM +0800, Yanteng Si wrote: > > > Based on IP core classification, loongson has two types of network > > What is the real company name? At least start the name with the > > capital letter. > > * everywhere > OK, LOONGSON > > > > > devices: GMAC and GNET. GMAC's ip_core id is 0x35/0x37, while GNET's > > > ip_core id is 0x37/0x10. > > s/ip_core/IP-core > > > > Once again the IP-core ID isn't _hex_, but a number of the format: > > "v+Major.Minor" > > so use the _real_ IP-core version number everywhere. Note mentioning > > that some of your GNET device has the GMAC_VERSION.SNPSVER hardwired > > to 0x10 is completely redundant in this and many other context. The > > only place where it's relevant is the patch(es) where you have the > > Snps ID override. > OK. > > > > > Device tables: > > > > > > device type pci_id snps_id channel > > > ls2k1000 gmac 7a03 0x35/0x37 1 > > > ls7a1000 gmac 7a03 0x35/0x37 1 > > > ls2k2000 gnet 7a13 0x10 8 > > > ls7a2000 gnet 7a13 0x37 1 > > s/gmac/GMAC > > s/gnet/GNET > > s/pci_id/PCI Dev ID > > s/snsp_id/Synopys Version > > s/channels/DMA-channels > > s/ls2k/LS2K > > s/ls7a/LS7A > > > > * everywhere > OK. > > > > > The GMAC device only has a MAC chip inside and needs an > > > external PHY chip; > > > > > > To later distinguish 8-channel gnet devices from single-channel > > > gnet/gmac devices, move rx_queues_to_use loongson_default_data > > > to loongson_dwmac_probe(). Also move mac_interface to > > > loongson_default_data(). > > Again. This is only a part of the reason why you need this change. > > The main reason is to provide the two-leveled platform data init > > functions: fist one is the common method initializing the data common > > for both GMAC and GNET, second one is the device-specific data > > initializer. > > > > To sum up I would change the commit log to something like this: > > > > "Loongson delivers two types of the network devices: Loongson GMAC and > > Loongson GNET in the framework of four CPU/Chipsets revisions: > > > > Chip Network PCI Dev ID Synopys Version DMA-channel > > LS2K1000 CPU GMAC 0x7a03 v3.50a/v3.73a 1 > > LS7A1000 Chipset GMAC 0x7a03 v3.50a/v3.73a 1 > > LS2K2000 CPU GNET 0x7a13 v3.73a 8 > > LS7A2000 Chipset GNET 0x7a13 v3.73a 1 > > > > The driver currently supports the chips with the Loongson GMAC network > > device. As a preparation before adding the Loongson GNET support > > detach the Loongson GMAC-specific platform data initializations to the > > loongson_gmac_data() method and preserve the common settings in the > > loongson_default_data(). > > > > While at it drop the return value statement from the > > loongson_default_data() method as redundant." > OK, Thanks! > > > > > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > > > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > > > --- > > > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 20 ++++++++++++------- > > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > index 4e0838db4259..904e288d0be0 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > @@ -11,22 +11,20 @@ > > > #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 > > > -static int loongson_default_data(struct plat_stmmacenet_data *plat) > > > +static void loongson_default_data(struct plat_stmmacenet_data *plat) > > > { > > > plat->clk_csr = 2; /* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */ > > > plat->has_gmac = 1; > > > plat->force_sf_dma_mode = 1; > > > + plat->mac_interface = PHY_INTERFACE_MODE_GMII; > > > + > > I double-checked this part in my HW and in the databooks. DW GMAC with > > _RGMII_ PHY-interfaces can't be equipped with a PCS (STMMAC driver is > > wrong in considering otherwise at least in the Auto-negotiation part). > > PCS is only available for the RTI, RTBI and SGMII interfaces. > > > > You can double-check that by checking out the DMA_HW_FEATURE.PCSSEL > > flag state. I'll be surprised if it's set in your case. If it isn't > > then either drop the plat_stmmacenet_data::mac_interface > > initialization or (as Russell suggested) initialize it with > > PHY_INTERFACE_MODE_NA. But do that in a separate pre-requisite patch! > OK. > > > > > /* Set default value for unicast filter entries */ > > > plat->unicast_filter_entries = 1; > > > /* Set the maxmtu to a default of JUMBO_LEN */ > > > plat->maxmtu = JUMBO_LEN; > > > - /* Set default number of RX and TX queues to use */ > > > - plat->tx_queues_to_use = 1; > > > - plat->rx_queues_to_use = 1; > > > - > > > /* Disable Priority config by default */ > > > plat->tx_queues_cfg[0].use_prio = false; > > > plat->rx_queues_cfg[0].use_prio = false; > > > @@ -41,6 +39,12 @@ static int loongson_default_data(struct plat_stmmacenet_data *plat) > > > plat->dma_cfg->pblx8 = true; > > > plat->multicast_filter_bins = 256; > > > +} > > > + > > > +static int loongson_gmac_data(struct plat_stmmacenet_data *plat) > > > +{ > > > + loongson_default_data(plat); > > > + > > > return 0; > > > } > > > @@ -109,11 +113,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > } > > > plat->phy_interface = phy_mode; > > > - plat->mac_interface = PHY_INTERFACE_MODE_GMII; > > > pci_set_master(pdev); > > > - loongson_default_data(plat); > > > + loongson_gmac_data(plat); > > > pci_enable_msi(pdev); > > > memset(&res, 0, sizeof(res)); > > > res.addr = pcim_iomap_table(pdev)[0]; > > > @@ -138,6 +141,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > goto err_disable_msi; > > > } > > > + plat->tx_queues_to_use = 1; > > > + plat->rx_queues_to_use = 1; > > > + > > You can freely move this to loongson_gmac_data() method. And then, in > > the patch adding the GNET-support, you'll be able to provide these fields > > initialization in the loongson_gnet_data() method together with the > > plat->tx_queues_cfg[*].coe_unsupported flag init. Thus the probe() > > method will get to be smaller and easier to read, and the > > loongson_*_data() method will be more coherent. > > As you said, at first glance, putting them in loongson_gnet_data() method is > fine, > > but in LS2K2000: > > plat->rx_queues_to_use = CHANNEL_NUM; // CHANNEL_NUM = 8; > plat->tx_queues_to_use = CHANNEL_NUM; > > So we need to distinguish between them. At the same time, we have to > distinguish > > between LS2K2000 in probe() method. Why not put them inside probe, which > will > > save a lot of duplicate code, like this: > > struct stmmac_resources res; > struct loongson_data *ld; > > ... > > memset(&res, 0, sizeof(res)); > res.addr = pcim_iomap_table(pdev)[0]; > ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > > switch (ld->gmac_verion) { > case LOONGSON_DWMAC_CORE_1_00: > plat->rx_queues_to_use = CHANNEL_NUM; > plat->tx_queues_to_use = CHANNEL_NUM; > > /* Only channel 0 supports checksum, > * so turn off checksum to enable multiple channels. > */ > for (i = 1; i < CHANNEL_NUM; i++) > plat->tx_queues_cfg[i].coe_unsupported = 1; > > ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > break; > default: /* 0x35 device and 0x37 device. */ > plat->tx_queues_to_use = 1; > plat->rx_queues_to_use = 1; > > ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > break; > } > if (ret) > goto err_disable_device; > > > What do you think? > > > Of course, if you insist, I'm willing to repeat this in the > > loongson_gnet_data() method. Not necessarily. As Huacai earlier suggested you can keep the Loongson ID in the platform private data and have it utilized in the local sub-functions/routines. Like this: struct loongson_data { u32 loongson_id; }; static int loongson_gmac_data(struct pci_dev *pdev, struct plat_stmmacenet_data *plat) { struct loongson_data *ld = plat->bsp_priv; ... plat->rx_queues_to_use = 1; plat->tx_queues_to_use = 1; return 0; } static int loongson_gnet_data(struct pci_dev *pdev, struct plat_stmmacenet_data *plat) { struct loongson_data *ld = plat->bsp_priv; ... if (ld->loongson_id == DWMAC_CORE_LS2K2000) { plat->rx_queues_to_use = 8; plat->tx_queues_to_use = 8; /* Tx COE is supported by the channel 0 only */ for (i = 1; i < 8; i++) plat->tx_queues_cfg[i].coe_unsupported = 1; } else { plat->rx_queues_to_use = 1; plat->tx_queues_to_use = 1; } return 0; } static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct loongson_data *ld; ... ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); if (!ld) return -ENOMEM; ... ld->loongson_id = readl(res.addr + GMAC_VERSION) & 0xff; plat->bsp_priv = ld; ... if (ld->loongson_id == DWMAC_CORE_LS2K2000) ret = loongson_dwmac_config_msi(pdev, plat, &res); else ret = loongson_dwmac_config_plat(pdev, plat, &res); if (ret) return ret; ... } It's not "a lot" duplication code. Just two if-else statements, which is fine. But the data-init methods will get to be fully coherent. It's much more important. * Note switch-case is redundant since you have a single case in there, so if-else would be more than enough. -Serge(y) > > > > Thanks, > > Yanteng >
Serge Semin <fancer.lancer@gmail.com> 于2024年5月13日周一 22:05写道： > > > > > > > /* Set default value for unicast filter entries */ > > > > plat->unicast_filter_entries = 1; > > > > /* Set the maxmtu to a default of JUMBO_LEN */ > > > > plat->maxmtu = JUMBO_LEN; > > > > - /* Set default number of RX and TX queues to use */ > > > > - plat->tx_queues_to_use = 1; > > > > - plat->rx_queues_to_use = 1; > > > > - > > > > /* Disable Priority config by default */ > > > > plat->tx_queues_cfg[0].use_prio = false; > > > > plat->rx_queues_cfg[0].use_prio = false; > > > > @@ -41,6 +39,12 @@ static int loongson_default_data(struct plat_stmmacenet_data *plat) > > > > plat->dma_cfg->pblx8 = true; > > > > plat->multicast_filter_bins = 256; > > > > +} > > > > + > > > > +static int loongson_gmac_data(struct plat_stmmacenet_data *plat) > > > > +{ > > > > + loongson_default_data(plat); > > > > + > > > > return 0; > > > > } > > > > @@ -109,11 +113,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > } > > > > plat->phy_interface = phy_mode; > > > > - plat->mac_interface = PHY_INTERFACE_MODE_GMII; > > > > pci_set_master(pdev); > > > > - loongson_default_data(plat); > > > > + loongson_gmac_data(plat); > > > > pci_enable_msi(pdev); > > > > memset(&res, 0, sizeof(res)); > > > > res.addr = pcim_iomap_table(pdev)[0]; > > > > @@ -138,6 +141,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > goto err_disable_msi; > > > > } > > > > + plat->tx_queues_to_use = 1; > > > > + plat->rx_queues_to_use = 1; > > > > + > > > You can freely move this to loongson_gmac_data() method. And then, in > > > the patch adding the GNET-support, you'll be able to provide these fields > > > initialization in the loongson_gnet_data() method together with the > > > plat->tx_queues_cfg[*].coe_unsupported flag init. Thus the probe() > > > method will get to be smaller and easier to read, and the > > > loongson_*_data() method will be more coherent. > > > > As you said, at first glance, putting them in loongson_gnet_data() method is > > fine, > > > > but in LS2K2000: > > > > plat->rx_queues_to_use = CHANNEL_NUM; // CHANNEL_NUM = 8; > > plat->tx_queues_to_use = CHANNEL_NUM; > > > > So we need to distinguish between them. At the same time, we have to > > distinguish > > > > between LS2K2000 in probe() method. Why not put them inside probe, which > > will > > > > save a lot of duplicate code, like this: > > > > struct stmmac_resources res; > > struct loongson_data *ld; > > > > ... > > > > memset(&res, 0, sizeof(res)); > > res.addr = pcim_iomap_table(pdev)[0]; > > ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > > > > switch (ld->gmac_verion) { > > case LOONGSON_DWMAC_CORE_1_00: > > plat->rx_queues_to_use = CHANNEL_NUM; > > plat->tx_queues_to_use = CHANNEL_NUM; > > > > /* Only channel 0 supports checksum, > > * so turn off checksum to enable multiple channels. > > */ > > for (i = 1; i < CHANNEL_NUM; i++) > > plat->tx_queues_cfg[i].coe_unsupported = 1; > > > > ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > > break; > > default: /* 0x35 device and 0x37 device. */ > > plat->tx_queues_to_use = 1; > > plat->rx_queues_to_use = 1; > > > > ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > break; > > } > > if (ret) > > goto err_disable_device; > > > > > > What do you think? > > > > > > Of course, if you insist, I'm willing to repeat this in the > > > > loongson_gnet_data() method. > > Not necessarily. As Huacai earlier suggested you can keep the Loongson > ID in the platform private data and have it utilized in the local > sub-functions/routines. Like this: > > struct loongson_data { > u32 loongson_id; > }; > > static int loongson_gmac_data(struct pci_dev *pdev, > struct plat_stmmacenet_data *plat) > { > struct loongson_data *ld = plat->bsp_priv; > > ... > > plat->rx_queues_to_use = 1; > plat->tx_queues_to_use = 1; > > return 0; > } > > static int loongson_gnet_data(struct pci_dev *pdev, > struct plat_stmmacenet_data *plat) > { > struct loongson_data *ld = plat->bsp_priv; > > ... > > if (ld->loongson_id == DWMAC_CORE_LS2K2000) { I did the test and found that at this point in time: loongson_id = 0, it has not been initialized yet. > > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct loongson_data *ld; > ... > > ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > if (!ld) > return -ENOMEM; > ... > ld->loongson_id = readl(res.addr + GMAC_VERSION) & 0xff; > plat->bsp_priv = ld; > ... > if (ld->loongson_id == DWMAC_CORE_LS2K2000) > ret = loongson_dwmac_config_msi(pdev, plat, &res); > else > ret = loongson_dwmac_config_plat(pdev, plat, &res); I'll change loongson_dwmac_config_legacy to loongson_dwmac_config_plat in the next version. And using if-else. > > It's not "a lot" duplication code. Just two if-else statements, which > is fine. But the data-init methods will get to be fully coherent. It's > much more important. Yes, I agree with you, but it looks like we still need to do the following again inside loongson_gnet_data() : memset(&res, 0, sizeof(res)); res.addr = pcim_iomap_table(pdev)[0]; ld->loongson_id = readl(res.addr + GMAC_VERSION) & 0xff; > > * Note switch-case is redundant since you have a single case in there, > so if-else would be more than enough. I see, Your single case is great！ Thanks, Yanteng
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index 4e0838db4259..904e288d0be0 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c @@ -11,22 +11,20 @@ #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 -static int loongson_default_data(struct plat_stmmacenet_data *plat) +static void loongson_default_data(struct plat_stmmacenet_data *plat) { plat->clk_csr = 2; /* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */ plat->has_gmac = 1; plat->force_sf_dma_mode = 1; + plat->mac_interface = PHY_INTERFACE_MODE_GMII; + /* Set default value for unicast filter entries */ plat->unicast_filter_entries = 1; /* Set the maxmtu to a default of JUMBO_LEN */ plat->maxmtu = JUMBO_LEN; - /* Set default number of RX and TX queues to use */ - plat->tx_queues_to_use = 1; - plat->rx_queues_to_use = 1; - /* Disable Priority config by default */ plat->tx_queues_cfg[0].use_prio = false; plat->rx_queues_cfg[0].use_prio = false; @@ -41,6 +39,12 @@ static int loongson_default_data(struct plat_stmmacenet_data *plat) plat->dma_cfg->pblx8 = true; plat->multicast_filter_bins = 256; +} + +static int loongson_gmac_data(struct plat_stmmacenet_data *plat) +{ + loongson_default_data(plat); + return 0; } @@ -109,11 +113,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id } plat->phy_interface = phy_mode; - plat->mac_interface = PHY_INTERFACE_MODE_GMII; pci_set_master(pdev); - loongson_default_data(plat); + loongson_gmac_data(plat); pci_enable_msi(pdev); memset(&res, 0, sizeof(res)); res.addr = pcim_iomap_table(pdev)[0]; @@ -138,6 +141,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id goto err_disable_msi; } + plat->tx_queues_to_use = 1; + plat->rx_queues_to_use = 1; + ret = stmmac_dvr_probe(&pdev->dev, plat, &res); if (ret) goto err_disable_msi;