netdev - Re: [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ftqxjh67a7s4iprpiuw5xxmncj3bveezf5vust7cej3kowwcvj@m7nqrxq7oe2f>
Date: Mon, 5 Feb 2024 23:58:44 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>
Cc: andrew@...n.ch, hkallweit1@...il.com, peppe.cavallaro@...com, 
	alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com, 
	chenhuacai@...ngson.cn, linux@...linux.org.uk, guyinggang@...ngson.cn, 
	netdev@...r.kernel.org, chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET
 support

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@...ngson.cn>
> Signed-off-by: Feiyang Chen <chenfeiyang@...ngson.cn>
> Signed-off-by: Yinggang Gu <guyinggang@...ngson.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
>