From: Serge Semin <fancer.lancer@gmail.com>
To: Yanteng Si <siyanteng@loongson.cn>
Cc: andrew@lunn.ch, hkallweit1@gmail.com, peppe.cavallaro@st.com,
alexandre.torgue@foss.st.com, joabreu@synopsys.com,
Jose.Abreu@synopsys.com, chenhuacai@kernel.org,
linux@armlinux.org.uk, guyinggang@loongson.cn,
netdev@vger.kernel.org, chris.chenfeiyang@gmail.com,
siyanteng01@gmail.com
Subject: Re: [PATCH net-next v12 13/15] net: stmmac: dwmac-loongson: Add Loongson GNET support
Date: Mon, 6 May 2024 13:39:24 +0300 [thread overview]
Message-ID: <jkjgjraqvih4zu7wvqykerq5wisgkhqf2n2pouha7qhfoeif7v@tkwyx53dfrdw> (raw)
In-Reply-To: <c97cb15ab77fb9dfdd281640f48dcfc08c6988c0.1714046812.git.siyanteng@loongson.cn>
On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote:
> ...
>
> +static int loongson_dwmac_config_msi(struct pci_dev *pdev,
> + struct plat_stmmacenet_data *plat,
> + struct stmmac_resources *res,
> + struct device_node *np)
> +{
> + int i, ret, vecs;
> +
> + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1);
> + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI);
> + if (ret < 0) {
> + dev_info(&pdev->dev,
> + "MSI enable failed, Fallback to legacy interrupt\n");
> + return loongson_dwmac_config_legacy(pdev, plat, res, np);
> + }
> +
> + res->irq = pci_irq_vector(pdev, 0);
> + res->wol_irq = 0;
> +
> + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx |
> + * --------- ----- -------- -------- ... -------- --------
> + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 |
> + */
> + for (i = 0; i < CHANNEL_NUM; i++) {
> + res->rx_irq[CHANNEL_NUM - 1 - i] =
> + pci_irq_vector(pdev, 1 + i * 2);
> + res->tx_irq[CHANNEL_NUM - 1 - i] =
> + pci_irq_vector(pdev, 2 + i * 2);
> + }
> +
> + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> +
> + return 0;
> +}
> +
> ...
> static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> struct plat_stmmacenet_data *plat;
> int ret, i, bus_id, phy_mode;
> struct stmmac_pci_info *info;
> struct stmmac_resources res;
> + struct loongson_data *ld;
> struct device_node *np;
>
> np = dev_of_node(&pdev->dev);
> @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> return -ENOMEM;
>
> plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL);
> - if (!plat->dma_cfg) {
> - ret = -ENOMEM;
> - goto err_put_node;
> - }
> + if (!plat->dma_cfg)
> + return -ENOMEM;
> +
> + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL);
> + if (!ld)
> + return -ENOMEM;
>
> /* Enable pci device */
> ret = pci_enable_device(pdev);
> @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> plat->phy_interface = phy_mode;
> }
>
> - pci_enable_msi(pdev);
> + plat->bsp_priv = ld;
> + plat->setup = loongson_dwmac_setup;
> + ld->dev = &pdev->dev;
> +
> 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;
>
> - plat->tx_queues_to_use = 1;
> - plat->rx_queues_to_use = 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);
> + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np);
> + break;
> + }
>
Let's now talk about this change.
First of all, one more time. You can't miss the return value check
because if any of the IRQ config method fails then the driver won't
work! The first change that introduces the problem is in the patch
[PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy
Second, as I already mentioned in another message sent to this patch
you are missing the PCI MSI IRQs freeing in the cleanup-on-error path
and in the device/driver remove() function. It's definitely wrong.
Thirdly, you said that the node-pointer is now optional and introduced
the patch
[PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support
If so and the DT-based setting up isn't mandatory then I would
suggest to proceed with the entire so called legacy setups only if the
node-pointer has been found, otherwise the pure PCI-based setup would
be performed. So the patches 10-13 (in your v12 order) would look
like this:
1. Patch 10 introduces the two types of the configs - DT and PCI plus
the bus_id initialized based on the PCI domain and device ID.
[PATCH net-next v13 10/15] net: stmmac: dwmac-loongson: Add DT-less GMAC PCI-device support
The DT and PCI config functions can look like this:
static int loongson_dwmac_config_dt(struct pci_dev *pdev,
struct plat_stmmacenet_data *plat,
struct stmmac_resources *res)
{
struct device_node *np = dev_of_node(&pdev->dev);
int ret;
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;
}
ret = of_alias_get_id(np, "ethernet");
if (ret >= 0)
plat->bus_id = ret;
res->irq = of_irq_get_byname(np, "macirq");
if (res->irq < 0) {
dev_err(&pdev->dev, "IRQ macirq not found\n");
return -ENODEV;
}
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");
res->wol_irq = res->irq;
}
res->lpi_irq = of_irq_get_byname(np, "eth_lpi");
if (res->lpi_irq < 0) {
dev_err(&pdev->dev, "IRQ eth_lpi not found\n");
return -ENODEV;
}
return 0;
}
static int loongson_dwmac_config_pci(struct pci_dev *pdev,
struct plat_stmmacenet_data *plat,
struct stmmac_resources *res)
{
res.irq = pdev->irq;
return 0;
}
...
static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
...
if (dev_of_node(&pdev->dev))
ret = loongson_dwmac_dt_config(pdev, plat, res);
else
ret = loongson_dwmac_pci_config(pdev, plat, res);
if (ret)
goto err_disable_msi;
...
}
2. Patch 11 introduces the stmmac_pci_info structure, makes the
stmmac_pci_info::setup() callback called in the probe() function and
assigns the loongson_gmac_data() method pointer to the GMAC info data.
[PATCH net-next v13 11/15] net: stmmac: dwmac-loongson: Introduce PCI device info data
3. Patch 12 can be preserved as is (but see my notes regarding moving
a part of it to the patch 13).
[PATCH net-next v13 12/15] net: stmmac: dwmac-loongson: Add flag disabling AN-less 1Gbps setup
4. Patch 13 introduces the GNET support as it's mainly done in your
patch (see my notes in there though)
[PATCH net-next v13 13/15] net: stmmac: dwmac-loongson: Add Loongson GNET support
but the loongson_dwmac_config_pci() method would now look as follows:
static int loongson_dwmac_config_pci(struct pci_dev *pdev,
struct plat_stmmacenet_data *plat,
struct stmmac_resources *res)
{
int i, ret, vecs;
/* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx |
* --------- ----- -------- -------- ... -------- --------
* IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 |
*/
vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1);
ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_LEGACY);
if (ret < 0) {
dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n");
return ret;
} else if (ret >= vecs) {
for (i = 0; i < CHANNEL_NUM; i++) {
res->rx_irq[CHANNEL_NUM - 1 - i] =
pci_irq_vector(pdev, 1 + i * 2);
res->tx_irq[CHANNEL_NUM - 1 - i] =
pci_irq_vector(pdev, 2 + i * 2);
}
plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
} else {
dev_warn(&pdev->dev, "Fall back to PCIe INTx IRQs\n");
}
res->irq = pci_irq_vector(pdev, 0);
return 0;
}
What do you think?
-Serge(y)
next prev parent reply other threads:[~2024-05-06 10:39 UTC|newest]
Thread overview: 81+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 13:01 [PATCH net-next v12 00/15] stmmac: Add Loongson platform support Yanteng Si
2024-04-25 13:01 ` [PATCH net-next v12 01/15] net: stmmac: Move the atds flag to the stmmac_dma_cfg structure Yanteng Si
2024-05-02 19:10 ` Serge Semin
2024-05-09 12:44 ` Yanteng Si
2024-04-25 13:01 ` [PATCH net-next v12 02/15] net: stmmac: Add multi-channel support Yanteng Si
2024-05-02 22:02 ` Serge Semin
2024-05-10 10:13 ` Yanteng Si
2024-05-13 9:45 ` Serge Semin
2024-05-13 9:49 ` Yanteng Si
2024-04-25 13:01 ` [PATCH net-next v12 03/15] net: stmmac: Export dwmac1000_dma_ops Yanteng Si
2024-05-03 10:27 ` Serge Semin
2024-05-13 9:46 ` Yanteng Si
2024-04-25 13:04 ` [PATCH net-next v12 04/15] net: stmmac: dwmac-loongson: Drop useless platform data Yanteng Si
2024-05-03 10:55 ` Serge Semin
2024-05-03 14:47 ` Serge Semin
2024-05-13 9:47 ` Yanteng Si
2024-05-13 9:46 ` Yanteng Si
2024-04-25 13:04 ` [PATCH net-next v12 05/15] net: stmmac: dwmac-loongson: Use PCI_DEVICE_DATA() macro for device identification Yanteng Si
2024-05-03 13:43 ` Serge Semin
2024-05-13 9:50 ` Yanteng Si
2024-04-25 13:04 ` [PATCH net-next v12 06/15] net: stmmac: dwmac-loongson: Split up the platform data initialization Yanteng Si
2024-05-03 18:08 ` Serge Semin
2024-05-13 11:07 ` Yanteng Si
2024-05-13 12:42 ` Huacai Chen
2024-05-13 14:04 ` Serge Semin
2024-05-18 10:38 ` yanteng si
2024-04-25 13:06 ` [PATCH net-next v12 07/15] net: stmmac: dwmac-loongson: Add ref and ptp clocks for Loongson Yanteng Si
2024-05-03 18:21 ` Serge Semin
2024-05-09 13:01 ` Yanteng Si
2024-04-25 13:06 ` [PATCH net-next v12 08/15] net: stmmac: dwmac-loongson: Add phy mask for Loongson GMAC Yanteng Si
2024-05-03 18:28 ` Serge Semin
2024-05-13 10:23 ` Yanteng Si
2024-04-25 13:06 ` [PATCH net-next v12 09/15] net: stmmac: dwmac-loongson: Add phy_interface " Yanteng Si
2024-04-25 14:36 ` Russell King (Oracle)
2024-04-26 10:16 ` Yanteng Si
2024-04-26 11:00 ` Russell King (Oracle)
2024-05-03 21:01 ` Serge Semin
2024-05-07 8:22 ` Russell King (Oracle)
2024-04-25 13:10 ` [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support Yanteng Si
2024-05-04 20:46 ` Serge Semin
2024-05-13 10:49 ` Yanteng Si
2024-04-25 13:10 ` [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy Yanteng Si
2024-05-04 21:28 ` Serge Semin
2024-05-13 10:12 ` Yanteng Si
2024-04-25 13:10 ` [PATCH net-next v12 12/15] net: stmmac: dwmac-loongson: Fixed failure to set network speed to 1000 Yanteng Si
2024-05-04 22:13 ` Serge Semin
2024-05-13 10:16 ` Yanteng Si
2024-04-25 13:11 ` [PATCH net-next v12 13/15] net: stmmac: dwmac-loongson: Add Loongson GNET support Yanteng Si
2024-04-26 5:12 ` Yanteng Si
2024-05-05 21:50 ` Serge Semin
2024-05-17 8:12 ` Yanteng Si
2024-05-17 9:48 ` Serge Semin
2024-05-17 11:14 ` yanteng si
2024-05-06 10:39 ` Serge Semin [this message]
2024-05-07 13:35 ` Yanteng Si
2024-05-08 14:38 ` Serge Semin
2024-05-08 14:58 ` Huacai Chen
2024-05-08 15:10 ` Serge Semin
2024-05-09 8:57 ` Yanteng Si
2024-05-13 10:56 ` Serge Semin
2024-05-13 13:26 ` Huacai Chen
2024-05-13 16:11 ` Serge Semin
2024-05-14 4:58 ` Huacai Chen
2024-05-14 11:33 ` Serge Semin
2024-05-14 12:53 ` Huacai Chen
2024-05-15 8:40 ` Serge Semin
2024-05-15 13:55 ` Huacai Chen
2024-05-17 8:42 ` Yanteng Si
2024-05-17 9:07 ` Serge Semin
2024-05-17 10:37 ` Yanteng Si
2024-05-17 16:37 ` Serge Semin
2024-05-18 10:47 ` yanteng si
2024-04-25 13:11 ` [PATCH net-next v12 14/15] net: stmmac: dwmac-loongson: Move disable_force flag to _gnet_date Yanteng Si
2024-05-05 21:53 ` Serge Semin
2024-05-13 10:20 ` Yanteng Si
2024-04-25 13:11 ` [PATCH net-next v12 15/15] net: stmmac: dwmac-loongson: Add loongson module author Yanteng Si
2024-05-06 2:12 ` Huacai Chen
2024-05-06 4:44 ` Serge Semin
2024-04-25 13:19 ` [PATCH net-next v12 00/15] stmmac: Add Loongson platform support Serge Semin
2024-04-26 4:55 ` Yanteng Si
2024-04-26 11:51 ` Serge Semin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=jkjgjraqvih4zu7wvqykerq5wisgkhqf2n2pouha7qhfoeif7v@tkwyx53dfrdw \
--to=fancer.lancer@gmail.com \
--cc=Jose.Abreu@synopsys.com \
--cc=alexandre.torgue@foss.st.com \
--cc=andrew@lunn.ch \
--cc=chenhuacai@kernel.org \
--cc=chris.chenfeiyang@gmail.com \
--cc=guyinggang@loongson.cn \
--cc=hkallweit1@gmail.com \
--cc=joabreu@synopsys.com \
--cc=linux@armlinux.org.uk \
--cc=netdev@vger.kernel.org \
--cc=peppe.cavallaro@st.com \
--cc=siyanteng01@gmail.com \
--cc=siyanteng@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).