diff mbox series

[net-next,v7,2/9] net: stmmac: dwmac-loongson: Refactor code for loongson_dwmac_probe()

Message ID aee820a3c4293c8172edda27ad4eb9cf5eaead5e.1702990507.git.siyanteng@loongson.cn (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series stmmac: Add Loongson platform support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 20 this patch: 20
netdev/cc_maintainers warning 6 maintainers not CCed: edumazet@google.com pabeni@redhat.com kuba@kernel.org mcoquelin.stm32@gmail.com linux-stm32@st-md-mailman.stormreply.com linux-arm-kernel@lists.infradead.org
netdev/build_clang fail Errors and warnings before: 21 this patch: 21
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 20 this patch: 20
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 134 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Yanteng Si Dec. 19, 2023, 2:17 p.m. UTC 
Add a setup() function to initialize data, and simplify code for
loongson_dwmac_probe().

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  | 68 +++++++++++++------
 1 file changed, 47 insertions(+), 21 deletions(-)

Comments

Russell King (Oracle) Jan. 2, 2024, 10:37 a.m. UTC | #1 
On Tue, Dec 19, 2023 at 10:17:05PM +0800, Yanteng Si wrote:
> Add a setup() function to initialize data, and simplify code for
> loongson_dwmac_probe().

Not all changes in this patch are described.

> +static int loongson_gmac_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 = 0;
>  
> -	/* Default to phy auto-detection */
>  	plat->phy_addr = -1;
> +	plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID;

This presumably sets the default phy_interface mode?


> -	plat->bus_id = of_alias_get_id(np, "ethernet");
> -	if (plat->bus_id < 0)
> -		plat->bus_id = pci_dev_id(pdev);
> +	pci_set_master(pdev);
> +
> +	info = (struct stmmac_pci_info *)id->driver_data;
> +	ret = info->setup(pdev, plat);
> +	if (ret)
> +		goto err_disable_device;

loongson_gmac_data() gets called from here...

> +
> +	bus_id = of_alias_get_id(np, "ethernet");
> +	if (bus_id >= 0)
> +		plat->bus_id = bus_id;
>  
>  	phy_mode = device_get_phy_mode(&pdev->dev);
>  	if (phy_mode < 0) {

This gets the PHY interface mode, and errors out if it can't be found in
firmware.

> @@ -110,11 +137,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>  	}
>  
>  	plat->phy_interface = phy_mode;

So this ends up always overwriting the value written in
loongson_gmac_data(). So it seems to be that initialising
plat->phy_interface in loongson_gmac_data() is just patch noise and
serves no real purpose.

> -	plat->mac_interface = PHY_INTERFACE_MODE_GMII;

This has now gone - and is not described, and I'm left wondering what
the implication of that is on the driver. It also makes me wonder
whether loongson_gmac_data() should've been setting mac_interface
rather than phy_interface.

>  	res.wol_irq = of_irq_get_byname(np, "eth_wake_irq");
>  	if (res.wol_irq < 0) {
> -		dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n");
> +		dev_info(&pdev->dev,
> +			 "IRQ eth_wake_irq not found, using macirq\n");

Whitespace cleanups should be a separate patch.
Yanteng Si Jan. 18, 2024, 12:53 p.m. UTC | #2 
在 2024/1/2 18:37, Russell King (Oracle) 写道:
> On Tue, Dec 19, 2023 at 10:17:05PM +0800, Yanteng Si wrote:
>> Add a setup() function to initialize data, and simplify code for
>> loongson_dwmac_probe().
> Not all changes in this patch are described.
Okay, I'll re-write it in more detail.
>
>> +static int loongson_gmac_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 = 0;
>>   
>> -	/* Default to phy auto-detection */
>>   	plat->phy_addr = -1;
>> +	plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID;
> This presumably sets the default phy_interface mode?
>
>
>> -	plat->bus_id = of_alias_get_id(np, "ethernet");
>> -	if (plat->bus_id < 0)
>> -		plat->bus_id = pci_dev_id(pdev);
>> +	pci_set_master(pdev);
>> +
>> +	info = (struct stmmac_pci_info *)id->driver_data;
>> +	ret = info->setup(pdev, plat);
>> +	if (ret)
>> +		goto err_disable_device;
> loongson_gmac_data() gets called from here...
>
>> +
>> +	bus_id = of_alias_get_id(np, "ethernet");
>> +	if (bus_id >= 0)
>> +		plat->bus_id = bus_id;
>>   
>>   	phy_mode = device_get_phy_mode(&pdev->dev);
>>   	if (phy_mode < 0) {
> This gets the PHY interface mode, and errors out if it can't be found in
> firmware.
>
>> @@ -110,11 +137,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>>   	}
>>   
>>   	plat->phy_interface = phy_mode;
> So this ends up always overwriting the value written in
> loongson_gmac_data(). So it seems to be that initialising
> plat->phy_interface in loongson_gmac_data() is just patch noise and
> serves no real purpose.
>
>> -	plat->mac_interface = PHY_INTERFACE_MODE_GMII;
> This has now gone - and is not described, and I'm left wondering what
> the implication of that is on the driver. It also makes me wonder
> whether loongson_gmac_data() should've been setting mac_interface
> rather than phy_interface.

You seem to have understood this in Patch 3.

I'll re-split the patch to make the code easier to understand.

>
>>   	res.wol_irq = of_irq_get_byname(np, "eth_wake_irq");
>>   	if (res.wol_irq < 0) {
>> -		dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n");
>> +		dev_info(&pdev->dev,
>> +			 "IRQ eth_wake_irq not found, using macirq\n");
> Whitespace cleanups should be a separate patch.

OK.


Thanks,

Yanteng

>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
index 9e40c28d453a..56d1fd8c61e1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
@@ -9,7 +9,12 @@ 
 #include <linux/of_irq.h>
 #include "stmmac.h"
 
-static int loongson_default_data(struct plat_stmmacenet_data *plat)
+struct stmmac_pci_info {
+	int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat);
+};
+
+static void loongson_default_data(struct pci_dev *pdev,
+				  struct plat_stmmacenet_data *plat)
 {
 	plat->clk_csr = 2;	/* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */
 	plat->has_gmac = 1;
@@ -34,23 +39,38 @@  static int loongson_default_data(struct plat_stmmacenet_data *plat)
 
 	/* Disable RX queues routing by default */
 	plat->rx_queues_cfg[0].pkt_route = 0x0;
+}
+
+static int loongson_gmac_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 = 0;
 
-	/* Default to phy auto-detection */
 	plat->phy_addr = -1;
+	plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID;
 
 	plat->dma_cfg->pbl = 32;
 	plat->dma_cfg->pblx8 = true;
 
-	plat->multicast_filter_bins = 256;
 	return 0;
 }
 
-static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+static struct stmmac_pci_info loongson_gmac_pci_info = {
+	.setup = loongson_gmac_data,
+};
+
+static int loongson_dwmac_probe(struct pci_dev *pdev,
+				const struct pci_device_id *id)
 {
+	int ret, i, bus_id, phy_mode;
 	struct plat_stmmacenet_data *plat;
+	struct stmmac_pci_info *info;
 	struct stmmac_resources res;
 	struct device_node *np;
-	int ret, i, phy_mode;
 
 	np = dev_of_node(&pdev->dev);
 
@@ -69,22 +89,22 @@  static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 	if (!plat->mdio_bus_data)
 		return -ENOMEM;
 
+	plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg),
+				     GFP_KERNEL);
+	if (!plat->dma_cfg)
+		return -ENOMEM;
+
 	plat->mdio_node = of_get_child_by_name(np, "mdio");
 	if (plat->mdio_node) {
 		dev_info(&pdev->dev, "Found MDIO subnode\n");
 		plat->mdio_bus_data->needs_reset = true;
 	}
 
-	plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL);
-	if (!plat->dma_cfg) {
-		ret = -ENOMEM;
-		goto err_put_node;
-	}
-
 	/* Enable pci device */
 	ret = pci_enable_device(pdev);
 	if (ret) {
-		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n", __func__);
+		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n",
+			__func__);
 		goto err_put_node;
 	}
 
@@ -98,9 +118,16 @@  static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 		break;
 	}
 
-	plat->bus_id = of_alias_get_id(np, "ethernet");
-	if (plat->bus_id < 0)
-		plat->bus_id = pci_dev_id(pdev);
+	pci_set_master(pdev);
+
+	info = (struct stmmac_pci_info *)id->driver_data;
+	ret = info->setup(pdev, plat);
+	if (ret)
+		goto err_disable_device;
+
+	bus_id = of_alias_get_id(np, "ethernet");
+	if (bus_id >= 0)
+		plat->bus_id = bus_id;
 
 	phy_mode = device_get_phy_mode(&pdev->dev);
 	if (phy_mode < 0) {
@@ -110,11 +137,7 @@  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);
 	pci_enable_msi(pdev);
 	memset(&res, 0, sizeof(res));
 	res.addr = pcim_iomap_table(pdev)[0];
@@ -128,7 +151,8 @@  static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 
 	res.wol_irq = of_irq_get_byname(np, "eth_wake_irq");
 	if (res.wol_irq < 0) {
-		dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n");
+		dev_info(&pdev->dev,
+			 "IRQ eth_wake_irq not found, using macirq\n");
 		res.wol_irq = res.irq;
 	}
 
@@ -212,8 +236,10 @@  static int __maybe_unused loongson_dwmac_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend,
 			 loongson_dwmac_resume);
 
+#define PCI_DEVICE_ID_LOONGSON_GMAC	0x7a03
+
 static const struct pci_device_id loongson_dwmac_id_table[] = {
-	{ PCI_VDEVICE(LOONGSON, 0x7a03) },
+	{ PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) },
 	{}
 };
 MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table);