| Message ID | 027b4ee29d4d7c8a22d2f5c551f5c21ced3fb046.1706601050.git.siyanteng@loongson.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | stmmac: Add Loongson platform support | expand |
On Tue, Jan 30, 2024 at 04:48:18PM +0800, Yanteng Si wrote: > Add Loongson GNET (GMAC with PHY) support, override > stmmac_priv.synopsys_id with 0x37. Please add more details of all the device capabilities: supported speeds, duplexness, IP-core version, DMA-descriptors type (normal/enhanced), MTL Tx/Rx FIFO size, Perfect and Hash-based MAC Filter tables size, L3/L4 filters availability, VLAN hash table filter, PHY-interface (GMII, RGMII, etc), EEE support, AV-feature/Multi-channels support, IEEE 1588 Timestamp support, Magic Frame support, Remote Wake-up support, IP Checksum, Tx/Rx TCP/IP Checksum, Mac Management Counters (MMC), SMA/MDIO interface, > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > --- > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 44 +++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > index 3b3578318cc1..584f7322bd3e 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > @@ -318,6 +318,8 @@ static struct mac_device_info *loongson_setup(void *apriv) > if (!mac) > return NULL; > > + priv->synopsys_id = 0x37; /*Overwrite custom IP*/ > + Please add a more descriptive comment _above_ the subjected line. In particular note why the override is needed, what is the real DW GMAC IP-core version and what is the original value the statement above overrides. > ld = priv->plat->bsp_priv; > mac->dma = &ld->dwlgmac_dma_ops; > > @@ -350,6 +352,46 @@ static struct mac_device_info *loongson_setup(void *apriv) > return mac; > } > > +static int loongson_gnet_data(struct pci_dev *pdev, > + struct plat_stmmacenet_data *plat) > +{ > + loongson_default_data(pdev, plat); > + > + plat->multicast_filter_bins = 256; > + > + plat->mdio_bus_data->phy_mask = ~(u32)BIT(2); > + > + plat->phy_addr = 2; > + plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; Are you sure PHY-interface is supposed to be defined as "internal"? > + > + plat->bsp_priv = &pdev->dev; > + > + plat->dma_cfg->pbl = 32; > + plat->dma_cfg->pblx8 = true; > + > + plat->clk_ref_rate = 125000000; > + plat->clk_ptp_rate = 125000000; > + > + return 0; > +} > + > +static int loongson_gnet_config(struct pci_dev *pdev, > + struct plat_stmmacenet_data *plat, > + struct stmmac_resources *res, > + struct device_node *np) > +{ > + int ret; > + > + ret = loongson_dwmac_config_legacy(pdev, plat, res, np); Again. This will be moved to the probe() method in one of the next patches leaving loongson_gnet_config() empty. What was the problem with doing that right away with no intermediate change? > + > + return ret; > +} > + > +static struct stmmac_pci_info loongson_gnet_pci_info = { > + .setup = loongson_gnet_data, > + .config = loongson_gnet_config, > +}; > + > static int loongson_dwmac_probe(struct pci_dev *pdev, > const struct pci_device_id *id) > { > @@ -516,9 +558,11 @@ static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend, > loongson_dwmac_resume); > > #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 > +#define PCI_DEVICE_ID_LOONGSON_GNET 0x7a13 > > static const struct pci_device_id loongson_dwmac_id_table[] = { > { PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) }, > + { PCI_DEVICE_DATA(LOONGSON, GNET, &loongson_gnet_pci_info) }, After this the driver is supposed to correctly handle the Loongson GNET devices. Based on the patches introduced further it isn't. Please consider re-arranging the changes (see my comments in the further patches). -Serge(y) > {} > }; > MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table); > -- > 2.31.4 >
在 2024/3/14 16:27, Yanteng Si 写道: > 在 2024/2/6 04:58, Serge Semin 写道: > > On Tue, Jan 30, 2024 at 04:48:18PM +0800, Yanteng Si wrote: > >> Add Loongson GNET (GMAC with PHY) support, override > >> stmmac_priv.synopsys_id with 0x37. > > Please add more details of all the device capabilities: supported > > speeds, duplexness, IP-core version, DMA-descriptors type > > (normal/enhanced), MTL Tx/Rx FIFO size, Perfect and Hash-based MAC > > Filter tables size, L3/L4 filters availability, VLAN hash table > > filter, PHY-interface (GMII, RGMII, etc), EEE support, > > AV-feature/Multi-channels support, IEEE 1588 Timestamp support, Magic > > Frame support, Remote Wake-up support, IP Checksum, Tx/Rx TCP/IP > > Checksum, Mac Management Counters (MMC), SMA/MDIO interface, > The gnet (2k2000) of 0x10 supports full-duplex and half-duplex at 1000/100/10M. > The gnet of 0x37 (i.e. the gnet of 7a2000) supports 1000/100/10M full duplex. > > The gnet with 0x10 has 8 DMA channels, except for channel 0, which does not > support sending hardware checksums. > > Supported AV features are Qav, Qat, and Qas, and other features should be > consistent with the 3.73 IP. > > > > >> Signed-off-by: Yanteng Si<siyanteng@loongson.cn> > >> Signed-off-by: Feiyang Chen<chenfeiyang@loongson.cn> > >> Signed-off-by: Yinggang Gu<guyinggang@loongson.cn> > >> --- > >> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 44 +++++++++++++++++++ > >> 1 file changed, 44 insertions(+) > >> > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> index 3b3578318cc1..584f7322bd3e 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> @@ -318,6 +318,8 @@ static struct mac_device_info *loongson_setup(void *apriv) > >> if (!mac) > >> return NULL; > >> > >> + priv->synopsys_id = 0x37; /*Overwrite custom IP*/ > >> + > > Please add a more descriptive comment _above_ the subjected line. In > > particular note why the override is needed, what is the real DW GMAC > > IP-core version and what is the original value the statement above > > overrides. > > The IP-core version of the gnet device on the loongson 2k2000 is 0x10, which is > a custom IP. > > Compared to 0x37, we have split some of the dma registers into two (tx and rx). > After overwriting stmmac_dma_ops.dma_interrupt() and stmmac_dma_ops.init_chan(), > the logic is consistent with 0x37, > > so we overwrite synopsys_id to 0x37. > > >> ld = priv->plat->bsp_priv; > >> mac->dma = &ld->dwlgmac_dma_ops; > >> > >> @@ -350,6 +352,46 @@ static struct mac_device_info *loongson_setup(void *apriv) > >> return mac; > >> } > >> > >> +static int loongson_gnet_data(struct pci_dev *pdev, > >> + struct plat_stmmacenet_data *plat) > >> +{ > >> + loongson_default_data(pdev, plat); > >> + > >> + plat->multicast_filter_bins = 256; > >> + > >> + plat->mdio_bus_data->phy_mask = ~(u32)BIT(2); > >> + > >> + plat->phy_addr = 2; > >> + plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; > > Are you sure PHY-interface is supposed to be defined as "internal"? > > Yes, because the gnet hardware has a integrated PHY, so we set it to internal， > > Correspondingly, our gmac hardware PHY is external. > > >> + > >> + plat->bsp_priv = &pdev->dev; > >> + > >> + plat->dma_cfg->pbl = 32; > >> + plat->dma_cfg->pblx8 = true; > >> + > >> + plat->clk_ref_rate = 125000000; > >> + plat->clk_ptp_rate = 125000000; > >> + > >> + return 0; > >> +} > >> + > >> +static int loongson_gnet_config(struct pci_dev *pdev, > >> + struct plat_stmmacenet_data *plat, > >> + struct stmmac_resources *res, > >> + struct device_node *np) > >> +{ > >> + int ret; > >> + > >> + ret = loongson_dwmac_config_legacy(pdev, plat, res, np); > > Again. This will be moved to the probe() method in one of the next > > patches leaving loongson_gnet_config() empty. What was the problem > > with doing that right away with no intermediate change? > > No problem. My original intention is to break the patches down into smaller pieces. > > In the next version, I will try to re-break them based on your comments. > > > > >> + > >> + return ret; > >> +} > >> + > >> +static struct stmmac_pci_info loongson_gnet_pci_info = { > >> + .setup = loongson_gnet_data, > >> + .config = loongson_gnet_config, > >> +}; > >> + > >> static int loongson_dwmac_probe(struct pci_dev *pdev, > >> const struct pci_device_id *id) > >> { > >> @@ -516,9 +558,11 @@ static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend, > >> loongson_dwmac_resume); > >> > >> #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 > >> +#define PCI_DEVICE_ID_LOONGSON_GNET 0x7a13 > >> > >> static const struct pci_device_id loongson_dwmac_id_table[] = { > >> { PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) }, > >> + { PCI_DEVICE_DATA(LOONGSON, GNET, &loongson_gnet_pci_info) }, > > After this the driver is supposed to correctly handle the Loongson > > GNET devices. Based on the patches introduced further it isn't. > > Please consider re-arranging the changes (see my comments in the > > further patches). > > OK. > > > Thanks, > > Yanteng > > > > > > -Serge(y) > > > >> {} > >> }; > >> MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table); > >> -- > >> 2.31.4 > >>
On Thu, Mar 14, 2024 at 09:12:49PM +0800, Yanteng Si wrote: > > 在 2024/3/14 16:27, Yanteng Si 写道: > > 在 2024/2/6 04:58, Serge Semin 写道: > > > On Tue, Jan 30, 2024 at 04:48:18PM +0800, Yanteng Si wrote: > > >> Add Loongson GNET (GMAC with PHY) support, override > > >> stmmac_priv.synopsys_id with 0x37. > > > Please add more details of all the device capabilities: supported > > > speeds, duplexness, IP-core version, DMA-descriptors type > > > (normal/enhanced), MTL Tx/Rx FIFO size, Perfect and Hash-based MAC > > > Filter tables size, L3/L4 filters availability, VLAN hash table > > > filter, PHY-interface (GMII, RGMII, etc), EEE support, > > > AV-feature/Multi-channels support, IEEE 1588 Timestamp support, Magic > > > Frame support, Remote Wake-up support, IP Checksum, Tx/Rx TCP/IP > > > Checksum, Mac Management Counters (MMC), SMA/MDIO interface, > > The gnet (2k2000) of 0x10 supports full-duplex and half-duplex at 1000/100/10M. > > The gnet of 0x37 (i.e. the gnet of 7a2000) supports 1000/100/10M full duplex. > > > > The gnet with 0x10 has 8 DMA channels, except for channel 0, which does not > > support sending hardware checksums. > > > > Supported AV features are Qav, Qat, and Qas, and other features should be > > consistent with the 3.73 IP. Just list all of these features in the commit message referring to the respective controller. Like this: "There are two types of them Loongson GNET controllers: Loongson 2k2000 GNET and the rest of the Loongson GNETs (like presented on the 7a2000 SoC). All of them of the DW GMAC 3.73a IP-core with the next features: Speeds, DMA-descriptors type (normal/enhanced), MTL Tx/Rx FIFO size, Perfect and Hash-based MAC Filter tables size, L3/L4 filters availability, VLAN hash table filter, PHY-interface (GMII, RGMII, etc), EEE support, IEEE 1588 Timestamp support, Magic Frame support, Remote Wake-up support, IP Checksum, Tx/Rx TCP/IP Checksum, Mac Management Counters (MMC), SMA/MDIO interface. The difference is that the Loongson 2k2000 GNET controller supports 8 DMA-channels, AV features (Qav, Qat, and Qas) and half-duplex link, meanwhile the rest of the GNETs don't have these capabilities available." > > > > > > > >> Signed-off-by: Yanteng Si<siyanteng@loongson.cn> > > >> Signed-off-by: Feiyang Chen<chenfeiyang@loongson.cn> > > >> Signed-off-by: Yinggang Gu<guyinggang@loongson.cn> > > >> --- > > >> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 44 +++++++++++++++++++ > > >> 1 file changed, 44 insertions(+) > > >> > > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > >> index 3b3578318cc1..584f7322bd3e 100644 > > >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > >> @@ -318,6 +318,8 @@ static struct mac_device_info *loongson_setup(void *apriv) > > >> if (!mac) > > >> return NULL; > > >> >> + priv->synopsys_id = 0x37; /*Overwrite custom IP*/ > > >> + > > > Please add a more descriptive comment _above_ the subjected line. In > > > particular note why the override is needed, what is the real DW GMAC > > > IP-core version and what is the original value the statement above > > > overrides. > > > > The IP-core version of the gnet device on the loongson 2k2000 is 0x10, which is > > a custom IP. > > > > Compared to 0x37, we have split some of the dma registers into two (tx and rx). > > After overwriting stmmac_dma_ops.dma_interrupt() and stmmac_dma_ops.init_chan(), > > the logic is consistent with 0x37, > > > > so we overwrite synopsys_id to 0x37. Yeah, something like that: /* The original IP-core version is 0x37 in all Loongson GNET * (2k2000 and 7a2000), but the GNET HW designers have changed the * GMAC_VERSION.SNPSVER field to the custom 0x10 value on the Loongson * 2k2000 MAC to emphasize the differences: multiple DMA-channels, AV * feature and GMAC_INT_STATUS CSR flags layout. Get back the * original value so the correct HW-interface would be * selected. */ > > > > >> ld = priv->plat->bsp_priv; > > >> mac->dma = &ld->dwlgmac_dma_ops; > > >> >> @@ -350,6 +352,46 @@ static struct mac_device_info > > *loongson_setup(void *apriv) > > >> return mac; > > >> } > > >> >> +static int loongson_gnet_data(struct pci_dev *pdev, > > >> + struct plat_stmmacenet_data *plat) > > >> +{ > > >> + loongson_default_data(pdev, plat); > > >> + > > >> + plat->multicast_filter_bins = 256; > > >> + > > >> + plat->mdio_bus_data->phy_mask = ~(u32)BIT(2); > > >> + > > >> + plat->phy_addr = 2; > > >> + plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; > > > Are you sure PHY-interface is supposed to be defined as "internal"? > > > > Yes, because the gnet hardware has a integrated PHY, so we set it to internal， > > Why do you need the phy_addr set to 2 then? Is PHY still discoverable on the subordinate MDIO-bus? kdoc in "include/linux/phy.h" defines the PHY_INTERFACE_MODE_INTERNAL mode as for a case of the MAC and PHY being combined. IIUC it's reserved for a case when you can't determine actual interface between the MAC and PHY. Is it your case? Are you sure the interface between MAC and PHY isn't something like GMII/RGMII/etc? -Serge(y) > > Correspondingly, our gmac hardware PHY is external. > > > > >> + > > >> + plat->bsp_priv = &pdev->dev; > > >> + > > >> + plat->dma_cfg->pbl = 32; > > >> + plat->dma_cfg->pblx8 = true; > > >> + > > >> + plat->clk_ref_rate = 125000000; > > >> + plat->clk_ptp_rate = 125000000; > > >> + > > >> + return 0; > > >> +} > > >> + > > >> +static int loongson_gnet_config(struct pci_dev *pdev, > > >> + struct plat_stmmacenet_data *plat, > > >> + struct stmmac_resources *res, > > >> + struct device_node *np) > > >> +{ > > >> + int ret; > > >> + > > >> + ret = loongson_dwmac_config_legacy(pdev, plat, res, np); > > > Again. This will be moved to the probe() method in one of the next > > > patches leaving loongson_gnet_config() empty. What was the problem > > > with doing that right away with no intermediate change? > > > > No problem. My original intention is to break the patches down into smaller pieces. > > > > In the next version, I will try to re-break them based on your comments. > > > > > > > >> + > > >> + return ret; > > >> +} > > >> + > > >> +static struct stmmac_pci_info loongson_gnet_pci_info = { > > >> + .setup = loongson_gnet_data, > > >> + .config = loongson_gnet_config, > > >> +}; > > >> + > > >> static int loongson_dwmac_probe(struct pci_dev *pdev, > > >> const struct pci_device_id *id) > > >> { > > >> @@ -516,9 +558,11 @@ static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend, > > >> loongson_dwmac_resume); > > >> >> #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 > > >> +#define PCI_DEVICE_ID_LOONGSON_GNET 0x7a13 > > >> >> static const struct pci_device_id loongson_dwmac_id_table[] = > > { > > >> { PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) }, > > >> + { PCI_DEVICE_DATA(LOONGSON, GNET, &loongson_gnet_pci_info) }, > > > After this the driver is supposed to correctly handle the Loongson > > > GNET devices. Based on the patches introduced further it isn't. > > > Please consider re-arranging the changes (see my comments in the > > > further patches). > > > > OK. > > > > > > Thanks, > > > > Yanteng > > > > > > > > > > -Serge(y) > > > > > >> {} > > >> }; > > >> MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table); > > >> -- >> 2.31.4 > > >> >
在 2024/3/19 23:03, Serge Semin 写道: > On Thu, Mar 14, 2024 at 09:12:49PM +0800, Yanteng Si wrote: >> 在 2024/3/14 16:27, Yanteng Si 写道: >>> 在 2024/2/6 04:58, Serge Semin 写道: >>>> On Tue, Jan 30, 2024 at 04:48:18PM +0800, Yanteng Si wrote: >>>>> Add Loongson GNET (GMAC with PHY) support, override >>>>> stmmac_priv.synopsys_id with 0x37. >>>> Please add more details of all the device capabilities: supported >>>> speeds, duplexness, IP-core version, DMA-descriptors type >>>> (normal/enhanced), MTL Tx/Rx FIFO size, Perfect and Hash-based MAC >>>> Filter tables size, L3/L4 filters availability, VLAN hash table >>>> filter, PHY-interface (GMII, RGMII, etc), EEE support, >>>> AV-feature/Multi-channels support, IEEE 1588 Timestamp support, Magic >>>> Frame support, Remote Wake-up support, IP Checksum, Tx/Rx TCP/IP >>>> Checksum, Mac Management Counters (MMC), SMA/MDIO interface, >>> The gnet (2k2000) of 0x10 supports full-duplex and half-duplex at 1000/100/10M. >>> The gnet of 0x37 (i.e. the gnet of 7a2000) supports 1000/100/10M full duplex. >>> >>> The gnet with 0x10 has 8 DMA channels, except for channel 0, which does not >>> support sending hardware checksums. >>> >>> Supported AV features are Qav, Qat, and Qas, and other features should be >>> consistent with the 3.73 IP. > Just list all of these features in the commit message referring to the > respective controller. Like this: > "There are two types of them Loongson GNET controllers: > Loongson 2k2000 GNET and the rest of the Loongson GNETs (like > presented on the 7a2000 SoC). All of them of the DW GMAC 3.73a > IP-core with the next features: > Speeds, DMA-descriptors type (normal/enhanced), MTL Tx/Rx FIFO size, > Perfect and Hash-based MAC Filter tables size, L3/L4 filters availability, > VLAN hash table filter, PHY-interface (GMII, RGMII, etc), EEE support, > IEEE 1588 Timestamp support, Magic Frame support, Remote Wake-up support, > IP Checksum, Tx/Rx TCP/IP Checksum, Mac Management Counters (MMC), > SMA/MDIO interface. > > The difference is that the Loongson 2k2000 GNET controller supports 8 > DMA-channels, AV features (Qav, Qat, and Qas) and half-duplex link, > meanwhile the rest of the GNETs don't have these capabilities > available." OK, Thanks! > >>>>> Signed-off-by: Yanteng Si<siyanteng@loongson.cn> >>>>> Signed-off-by: Feiyang Chen<chenfeiyang@loongson.cn> >>>>> Signed-off-by: Yinggang Gu<guyinggang@loongson.cn> >>>>> --- >>>>> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 44 +++++++++++++++++++ >>>>> 1 file changed, 44 insertions(+) >>>>> >>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>>>> index 3b3578318cc1..584f7322bd3e 100644 >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >>>>> @@ -318,6 +318,8 @@ static struct mac_device_info *loongson_setup(void *apriv) >>>>> if (!mac) >>>>> return NULL; >>>>> >> + priv->synopsys_id = 0x37; /*Overwrite custom IP*/ >>>>> + >>>> Please add a more descriptive comment _above_ the subjected line. In >>>> particular note why the override is needed, what is the real DW GMAC >>>> IP-core version and what is the original value the statement above >>>> overrides. >>> The IP-core version of the gnet device on the loongson 2k2000 is 0x10, which is >>> a custom IP. >>> >>> Compared to 0x37, we have split some of the dma registers into two (tx and rx). >>> After overwriting stmmac_dma_ops.dma_interrupt() and stmmac_dma_ops.init_chan(), >>> the logic is consistent with 0x37, >>> >>> so we overwrite synopsys_id to 0x37. > Yeah, something like that: > /* The original IP-core version is 0x37 in all Loongson GNET > * (2k2000 and 7a2000), but the GNET HW designers have changed the > * GMAC_VERSION.SNPSVER field to the custom 0x10 value on the Loongson > * 2k2000 MAC to emphasize the differences: multiple DMA-channels, AV > * feature and GMAC_INT_STATUS CSR flags layout. Get back the > * original value so the correct HW-interface would be > * selected. > */ OK, Thanks! >>>>> ld = priv->plat->bsp_priv; >>>>> mac->dma = &ld->dwlgmac_dma_ops; >>>>> >> @@ -350,6 +352,46 @@ static struct mac_device_info >>> *loongson_setup(void *apriv) >>>>> return mac; >>>>> } >>>>> >> +static int loongson_gnet_data(struct pci_dev *pdev, >>>>> + struct plat_stmmacenet_data *plat) >>>>> +{ >>>>> + loongson_default_data(pdev, plat); >>>>> + >>>>> + plat->multicast_filter_bins = 256; >>>>> + >>>>> + plat->mdio_bus_data->phy_mask = ~(u32)BIT(2); >>>>> + >>>>> + plat->phy_addr = 2; >>>>> + plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; >>>> Are you sure PHY-interface is supposed to be defined as "internal"? >>> Yes, because the gnet hardware has a integrated PHY, so we set it to internal， >>> > Why do you need the phy_addr set to 2 then? Is PHY still discoverable > on the subordinate MDIO-bus? > > kdoc in "include/linux/phy.h" defines the PHY_INTERFACE_MODE_INTERNAL > mode as for a case of the MAC and PHY being combined. IIUC it's > reserved for a case when you can't determine actual interface between > the MAC and PHY. Is it your case? Are you sure the interface between > MAC and PHY isn't something like GMII/RGMII/etc? Please allow me to consult with our hardware engineers before replying to you. :) Thanks, Yanteng > > -Serge(y) > >>> Correspondingly, our gmac hardware PHY is external. >>> >>>>> + >>>>> + plat->bsp_priv = &pdev->dev; >>>>> + >>>>> + plat->dma_cfg->pbl = 32; >>>>> + plat->dma_cfg->pblx8 = true; >>>>> + >>>>> + plat->clk_ref_rate = 125000000; >>>>> + plat->clk_ptp_rate = 125000000; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int loongson_gnet_config(struct pci_dev *pdev, >>>>> + struct plat_stmmacenet_data *plat, >>>>> + struct stmmac_resources *res, >>>>> + struct device_node *np) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + ret = loongson_dwmac_config_legacy(pdev, plat, res, np); >>>> Again. This will be moved to the probe() method in one of the next >>>> patches leaving loongson_gnet_config() empty. What was the problem >>>> with doing that right away with no intermediate change? >>> No problem. My original intention is to break the patches down into smaller pieces. >>> >>> In the next version, I will try to re-break them based on your comments. >>> >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static struct stmmac_pci_info loongson_gnet_pci_info = { >>>>> + .setup = loongson_gnet_data, >>>>> + .config = loongson_gnet_config, >>>>> +}; >>>>> + >>>>> static int loongson_dwmac_probe(struct pci_dev *pdev, >>>>> const struct pci_device_id *id) >>>>> { >>>>> @@ -516,9 +558,11 @@ static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend, >>>>> loongson_dwmac_resume); >>>>> >> #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 >>>>> +#define PCI_DEVICE_ID_LOONGSON_GNET 0x7a13 >>>>> >> static const struct pci_device_id loongson_dwmac_id_table[] = >>> { >>>>> { PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) }, >>>>> + { PCI_DEVICE_DATA(LOONGSON, GNET, &loongson_gnet_pci_info) }, >>>> After this the driver is supposed to correctly handle the Loongson >>>> GNET devices. Based on the patches introduced further it isn't. >>>> Please consider re-arranging the changes (see my comments in the >>>> further patches). >>> OK. >>> >>> >>> Thanks, >>> >>> Yanteng >>> >>> >>>> -Serge(y) >>>> >>>>> {} >>>>> }; >>>>> MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table); >>>>> -- >> 2.31.4 >>>>>
On Wed, Mar 20, 2024 at 06:23:14PM +0800, Yanteng Si wrote: > > 在 2024/3/19 23:03, Serge Semin 写道: > > On Thu, Mar 14, 2024 at 09:12:49PM +0800, Yanteng Si wrote: > > > 在 2024/3/14 16:27, Yanteng Si 写道: > > > > 在 2024/2/6 04:58, Serge Semin 写道: > > > > > On Tue, Jan 30, 2024 at 04:48:18PM +0800, Yanteng Si wrote: > > > > > > Add Loongson GNET (GMAC with PHY) support, override > > > > > > stmmac_priv.synopsys_id with 0x37. > > > > > Please add more details of all the device capabilities: supported > > > > > speeds, duplexness, IP-core version, DMA-descriptors type > > > > > (normal/enhanced), MTL Tx/Rx FIFO size, Perfect and Hash-based MAC > > > > > Filter tables size, L3/L4 filters availability, VLAN hash table > > > > > filter, PHY-interface (GMII, RGMII, etc), EEE support, > > > > > AV-feature/Multi-channels support, IEEE 1588 Timestamp support, Magic > > > > > Frame support, Remote Wake-up support, IP Checksum, Tx/Rx TCP/IP > > > > > Checksum, Mac Management Counters (MMC), SMA/MDIO interface, > > > > The gnet (2k2000) of 0x10 supports full-duplex and half-duplex at 1000/100/10M. > > > > The gnet of 0x37 (i.e. the gnet of 7a2000) supports 1000/100/10M full duplex. > > > > > > > > The gnet with 0x10 has 8 DMA channels, except for channel 0, which does not > > > > support sending hardware checksums. > > > > > > > > Supported AV features are Qav, Qat, and Qas, and other features should be > > > > consistent with the 3.73 IP. > > Just list all of these features in the commit message referring to the > > respective controller. Like this: > > "There are two types of them Loongson GNET controllers: > > Loongson 2k2000 GNET and the rest of the Loongson GNETs (like > > presented on the 7a2000 SoC). All of them of the DW GMAC 3.73a > > IP-core with the next features: > > Speeds, DMA-descriptors type (normal/enhanced), MTL Tx/Rx FIFO size, > > Perfect and Hash-based MAC Filter tables size, L3/L4 filters availability, > > VLAN hash table filter, PHY-interface (GMII, RGMII, etc), EEE support, > > IEEE 1588 Timestamp support, Magic Frame support, Remote Wake-up support, > > IP Checksum, Tx/Rx TCP/IP Checksum, Mac Management Counters (MMC), > > SMA/MDIO interface. > > > > The difference is that the Loongson 2k2000 GNET controller supports 8 > > DMA-channels, AV features (Qav, Qat, and Qas) and half-duplex link, > > meanwhile the rest of the GNETs don't have these capabilities > > available." > OK, Thanks! Please don't forget to replace the next text: "... Speeds, DMA-descriptors type (normal/enhanced), MTL Tx/Rx FIFO size, Perfect and Hash-based MAC Filter tables size, L3/L4 filters availability, VLAN hash table filter, PHY-interface (GMII, RGMII, etc), EEE support, IEEE 1588 Timestamp support, Magic Frame support, Remote Wake-up support, IP Checksum, Tx/Rx TCP/IP Checksum, Mac Management Counters (MMC), SMA/MDIO interface." with the actual device capabilities, like: Speeds - 10/100/1000Mbps, DMA-descriptors type - Normal DMA descriptors or Enhanced DMA descriptors, MTL Tx/Rx FIFO size - X Kb MTL Tx FIFO, Y Kb MTL Rx FIFO, Perfect and Hash-based MAC Filter tables size - X MAC addresses, X-entries hash address filter, etc -Serge(y) > > > > > > > > Signed-off-by: Yanteng Si<siyanteng@loongson.cn> > > > > > > Signed-off-by: Feiyang Chen<chenfeiyang@loongson.cn> > > > > > > Signed-off-by: Yinggang Gu<guyinggang@loongson.cn> > > > > > > --- > > > > > > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 44 +++++++++++++++++++ > > > > > > 1 file changed, 44 insertions(+) > > > > > > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > > > > index 3b3578318cc1..584f7322bd3e 100644 > > > > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > > > > @@ -318,6 +318,8 @@ static struct mac_device_info *loongson_setup(void *apriv) > > > > > > if (!mac) > > > > > > return NULL; > > > > > > >> + priv->synopsys_id = 0x37; /*Overwrite custom IP*/ > > > > > > + > > > > > Please add a more descriptive comment _above_ the subjected line. In > > > > > particular note why the override is needed, what is the real DW GMAC > > > > > IP-core version and what is the original value the statement above > > > > > overrides. > > > > The IP-core version of the gnet device on the loongson 2k2000 is 0x10, which is > > > > a custom IP. > > > > > > > > Compared to 0x37, we have split some of the dma registers into two (tx and rx). > > > > After overwriting stmmac_dma_ops.dma_interrupt() and stmmac_dma_ops.init_chan(), > > > > the logic is consistent with 0x37, > > > > > > > > so we overwrite synopsys_id to 0x37. > > Yeah, something like that: > > /* The original IP-core version is 0x37 in all Loongson GNET > > * (2k2000 and 7a2000), but the GNET HW designers have changed the > > * GMAC_VERSION.SNPSVER field to the custom 0x10 value on the Loongson > > * 2k2000 MAC to emphasize the differences: multiple DMA-channels, AV > > * feature and GMAC_INT_STATUS CSR flags layout. Get back the > > * original value so the correct HW-interface would be > > * selected. > > */ > OK, Thanks! > > > > > > ld = priv->plat->bsp_priv; > > > > > > mac->dma = &ld->dwlgmac_dma_ops; > > > > > > >> @@ -350,6 +352,46 @@ static struct mac_device_info > > > > *loongson_setup(void *apriv) > > > > > > return mac; > > > > > > } > > > > > > >> +static int loongson_gnet_data(struct pci_dev *pdev, > > > > > > + struct plat_stmmacenet_data *plat) > > > > > > +{ > > > > > > + loongson_default_data(pdev, plat); > > > > > > + > > > > > > + plat->multicast_filter_bins = 256; > > > > > > + > > > > > > + plat->mdio_bus_data->phy_mask = ~(u32)BIT(2); > > > > > > + > > > > > > + plat->phy_addr = 2; > > > > > > + plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; > > > > > Are you sure PHY-interface is supposed to be defined as "internal"? > > > > Yes, because the gnet hardware has a integrated PHY, so we set it to internal， > > > > > > Why do you need the phy_addr set to 2 then? Is PHY still discoverable > > on the subordinate MDIO-bus? > > > > kdoc in "include/linux/phy.h" defines the PHY_INTERFACE_MODE_INTERNAL > > mode as for a case of the MAC and PHY being combined. IIUC it's > > reserved for a case when you can't determine actual interface between > > the MAC and PHY. Is it your case? Are you sure the interface between > > MAC and PHY isn't something like GMII/RGMII/etc? > > Please allow me to consult with our hardware engineers before replying to > you. :) > > > Thanks, > > Yanteng > > > > > -Serge(y) > > > > > > Correspondingly, our gmac hardware PHY is external. > > > > > > > > > > + > > > > > > + plat->bsp_priv = &pdev->dev; > > > > > > + > > > > > > + plat->dma_cfg->pbl = 32; > > > > > > + plat->dma_cfg->pblx8 = true; > > > > > > + > > > > > > + plat->clk_ref_rate = 125000000; > > > > > > + plat->clk_ptp_rate = 125000000; > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +static int loongson_gnet_config(struct pci_dev *pdev, > > > > > > + struct plat_stmmacenet_data *plat, > > > > > > + struct stmmac_resources *res, > > > > > > + struct device_node *np) > > > > > > +{ > > > > > > + int ret; > > > > > > + > > > > > > + ret = loongson_dwmac_config_legacy(pdev, plat, res, np); > > > > > Again. This will be moved to the probe() method in one of the next > > > > > patches leaving loongson_gnet_config() empty. What was the problem > > > > > with doing that right away with no intermediate change? > > > > No problem. My original intention is to break the patches down into smaller pieces. > > > > > > > > In the next version, I will try to re-break them based on your comments. > > > > > > > > > > + > > > > > > + return ret; > > > > > > +} > > > > > > + > > > > > > +static struct stmmac_pci_info loongson_gnet_pci_info = { > > > > > > + .setup = loongson_gnet_data, > > > > > > + .config = loongson_gnet_config, > > > > > > +}; > > > > > > + > > > > > > static int loongson_dwmac_probe(struct pci_dev *pdev, > > > > > > const struct pci_device_id *id) > > > > > > { > > > > > > @@ -516,9 +558,11 @@ static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend, > > > > > > loongson_dwmac_resume); > > > > > > >> #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 > > > > > > +#define PCI_DEVICE_ID_LOONGSON_GNET 0x7a13 > > > > > > >> static const struct pci_device_id loongson_dwmac_id_table[] = > > > > { > > > > > > { PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) }, > > > > > > + { PCI_DEVICE_DATA(LOONGSON, GNET, &loongson_gnet_pci_info) }, > > > > > After this the driver is supposed to correctly handle the Loongson > > > > > GNET devices. Based on the patches introduced further it isn't. > > > > > Please consider re-arranging the changes (see my comments in the > > > > > further patches). > > > > OK. > > > > > > > > > > > > Thanks, > > > > > > > > Yanteng > > > > > > > > > > > > > -Serge(y) > > > > > > > > > > > {} > > > > > > }; > > > > > > MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table); > > > > > > -- >> 2.31.4 > > > > > > >
在 2024/3/19 23:03, Serge Semin 写道: >>>>> >> +static int loongson_gnet_data(struct pci_dev *pdev, >>>>> + struct plat_stmmacenet_data *plat) >>>>> +{ >>>>> + loongson_default_data(pdev, plat); >>>>> + >>>>> + plat->multicast_filter_bins = 256; >>>>> + >>>>> + plat->mdio_bus_data->phy_mask = ~(u32)BIT(2); >>>>> + >>>>> + plat->phy_addr = 2; >>>>> + plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; >>>> Are you sure PHY-interface is supposed to be defined as "internal"? >>> Yes, because the gnet hardware has a integrated PHY, so we set it to internal， >>> > Why do you need the phy_addr set to 2 then? Is PHY still discoverable > on the subordinate MDIO-bus? Because the default return value of gnet's mdio is 0xffff, when scanning for phy, if the return value is not 0, it will be assumed that the phy for that address exists. Not specifying an address will cause all addresses' phy to be detected, and the lowest address' phy will be selected by default. so then, the network is unavailable. > > kdoc in "include/linux/phy.h" defines the PHY_INTERFACE_MODE_INTERNAL > mode as for a case of the MAC and PHY being combined. IIUC it's > reserved for a case when you can't determine actual interface between > the MAC and PHY. Is it your case? Are you sure the interface between > MAC and PHY isn't something like GMII/RGMII/etc? Hmmm. the interface between MAC and PHY is GMII, so let's use PHY_INTERFACE_MODE_GMII? Thanks, Yanteng
> Because the default return value of gnet's mdio is 0xffff, when scanning for > phy, > > if the return value is not 0, it will be assumed that the phy for that > address exists. That is not correct. The MDIO bus has a pull up on the data line. If there is no device at a given address, that pull up results in 0xffff being read. phylib understands this, it knows that a read with the value of 0xffff probably means there is no device at that address. So it will not create a device. > Not specifying an address will cause all addresses' phy to be detected, and > the > > lowest address' phy will be selected by default. so then, the network is > unavailable. Do you have multiple PHYs on the bus? If there is only one PHY the first PHY should be the PHY you want, and phy_find_first() will do what you need. However, if there are multiple PHYs on the bus, you really should use a phandle in DT to point to the correct PHY. Andrew
On Thu, Mar 21, 2024 at 05:13:45PM +0800, Yanteng Si wrote: > > 在 2024/3/19 23:03, Serge Semin 写道: > > > > > > >> +static int loongson_gnet_data(struct pci_dev *pdev, > > > > > > + struct plat_stmmacenet_data *plat) > > > > > > +{ > > > > > > + loongson_default_data(pdev, plat); > > > > > > + > > > > > > + plat->multicast_filter_bins = 256; > > > > > > + > > > > > > + plat->mdio_bus_data->phy_mask = ~(u32)BIT(2); > > > > > > + > > > > > > + plat->phy_addr = 2; > > > > > > + plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; > > > > > Are you sure PHY-interface is supposed to be defined as "internal"? > > > > Yes, because the gnet hardware has a integrated PHY, so we set it to internal， > > > > > ... > > kdoc in "include/linux/phy.h" defines the PHY_INTERFACE_MODE_INTERNAL > > mode as for a case of the MAC and PHY being combined. IIUC it's > > reserved for a case when you can't determine actual interface between > > the MAC and PHY. Is it your case? Are you sure the interface between > > MAC and PHY isn't something like GMII/RGMII/etc? > Hmmm. the interface between MAC and PHY is GMII, so let's use > > PHY_INTERFACE_MODE_GMII? If MAC<->PHY interface is GMII, then PHY_INTERFACE_MODE_GMII should be used to indicate that. -Serge(y) > > > Thanks, > > Yanteng >
在 2024/3/21 22:55, Andrew Lunn 写道: >> Because the default return value of gnet's mdio is 0xffff, when scanning for >> phy, >> >> if the return value is not 0, it will be assumed that the phy for that >> address exists. > That is not correct. The MDIO bus has a pull up on the data line. If > there is no device at a given address, that pull up results in 0xffff > being read. phylib understands this, it knows that a read with the > value of 0xffff probably means there is no device at that address. So > it will not create a device. OK. I've got it. > >> Not specifying an address will cause all addresses' phy to be detected, and >> the >> >> lowest address' phy will be selected by default. so then, the network is >> unavailable. > Do you have multiple PHYs on the bus? If there is only one PHY the Only one. > first PHY should be the PHY you want, and phy_find_first() will do static int loongson_gnet_data(struct pci_dev *pdev, struct plat_stmmacenet_data *plat) { struct net_device *ndev = dev_get_drvdata(&pdev->dev); struct stmmac_priv *priv = netdev_priv(ndev); struct phy_device *phydev; phydev = phy_find_first(priv->mii); plat->phy_addr = phydev->mdio.addr; If I understand your comment correctly, this doesn't seem to work because the mii hasn't been initialized at this point. So, how about plat->phy_addr = -1; ? Thanks, Yanteng
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index 3b3578318cc1..584f7322bd3e 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c @@ -318,6 +318,8 @@ static struct mac_device_info *loongson_setup(void *apriv) if (!mac) return NULL; + priv->synopsys_id = 0x37; /*Overwrite custom IP*/ + ld = priv->plat->bsp_priv; mac->dma = &ld->dwlgmac_dma_ops; @@ -350,6 +352,46 @@ static struct mac_device_info *loongson_setup(void *apriv) return mac; } +static int loongson_gnet_data(struct pci_dev *pdev, + struct plat_stmmacenet_data *plat) +{ + loongson_default_data(pdev, plat); + + plat->multicast_filter_bins = 256; + + plat->mdio_bus_data->phy_mask = ~(u32)BIT(2); + + plat->phy_addr = 2; + plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL; + + plat->bsp_priv = &pdev->dev; + + plat->dma_cfg->pbl = 32; + plat->dma_cfg->pblx8 = true; + + plat->clk_ref_rate = 125000000; + plat->clk_ptp_rate = 125000000; + + return 0; +} + +static int loongson_gnet_config(struct pci_dev *pdev, + struct plat_stmmacenet_data *plat, + struct stmmac_resources *res, + struct device_node *np) +{ + int ret; + + ret = loongson_dwmac_config_legacy(pdev, plat, res, np); + + return ret; +} + +static struct stmmac_pci_info loongson_gnet_pci_info = { + .setup = loongson_gnet_data, + .config = loongson_gnet_config, +}; + static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -516,9 +558,11 @@ static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend, loongson_dwmac_resume); #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 +#define PCI_DEVICE_ID_LOONGSON_GNET 0x7a13 static const struct pci_device_id loongson_dwmac_id_table[] = { { PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) }, + { PCI_DEVICE_DATA(LOONGSON, GNET, &loongson_gnet_pci_info) }, {} }; MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table);