From: Serge Semin <fancer.lancer@gmail.com>
To: Yanteng Si <siyanteng@loongson.cn>
Cc: Huacai Chen <chenhuacai@kernel.org>,
andrew@lunn.ch, hkallweit1@gmail.com, peppe.cavallaro@st.com,
alexandre.torgue@foss.st.com, joabreu@synopsys.com,
Jose.Abreu@synopsys.com, 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: Fri, 17 May 2024 19:37:47 +0300 [thread overview]
Message-ID: <ikmwqzwplbnorwrao6afj6t4iksgo4t7jk6to65pnmtqgmalkv@gnrv5cskqlsb> (raw)
In-Reply-To: <c09237c6-6661-4744-a9d3-7c3443f2820c@loongson.cn>
On Fri, May 17, 2024 at 06:37:50PM +0800, Yanteng Si wrote:
> Hi Serge,
>
> 在 2024/5/17 17:07, Serge Semin 写道:
> > On Fri, May 17, 2024 at 04:42:51PM +0800, Yanteng Si wrote:
> > > Hi Huacai, Serge,
> > >
> > > 在 2024/5/15 21:55, Huacai Chen 写道:
> > > > > > > Once again about the naming. From the retrospective point of view the
> > > > > > > so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs
> > > > > > > look similar because these are just the level-type signals connected
> > > > > > > to the system IRQ controller. But when it comes to the PCI_Express_,
> > > > > > > the implementation is completely different. The PCIe INTx is just the
> > > > > > > PCIe TLPs of special type, like MSI. Upon receiving these special
> > > > > > > messages the PCIe host controller delivers the IRQ up to the
> > > > > > > respective system IRQ controller. So in order to avoid the confusion
> > > > > > > between the actual legacy PCI INTx, PCI Express INTx and the just
> > > > > > > platform IRQs, it's better to emphasize the actual way of the IRQs
> > > > > > > delivery. In this case it's the later method.
> > > > > > You are absolutely right, and I think I found a method to use your
> > > > > > framework to solve our problems:
> > > > > >
> > > > > > static int loongson_dwmac_config_irqs(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 = plat->rx_queues_to_use + plat->tx_queues_to_use + 1;
> > > > > > ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_INTX);
> > > > > > if (ret < 0) {
> > > > > > dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n");
> > > > > > return ret;
> > > > > > }
> > > > > > if (ret >= vecs) {
> > > > > > for (i = 0; i < plat->rx_queues_to_use; i++) {
> > > > > > res->rx_irq[CHANNELS_NUM - 1 - i] =
> > > > > > pci_irq_vector(pdev, 1 + i * 2);
> > > > > > }
> > > > > > for (i = 0; i < plat->tx_queues_to_use; i++) {
> > > > > > res->tx_irq[CHANNELS_NUM - 1 - i] =
> > > > > > pci_irq_vector(pdev, 2 + i * 2);
> > > > > > }
> > > > > >
> > > > > > plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> > > > > > }
> > > > > >
> > > > > > res->irq = pci_irq_vector(pdev, 0);
> > > > > >
> > > > > > if (np) {
> > > > > > 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;
> > > > > > }
> > > > > >
> > > > > > If your agree, Yanteng can use this method in V13, then avoid furthur changes.
> > > > > Since yesterday I have been too relaxed sitting back to explain in
> > > > > detail the problems with the code above. Shortly speaking, no to the
> > > > > method designed as above.
> > > > This function is copy-paste from your version which you suggest to
> > > > Yanteng, and plus the fallback parts for DT. If you don't want to
> > > > discuss it any more, we can discuss after V13.
> > My conclusion is the same. no to _your_ (Huacai) version of the code.
> > I suggest to Huacai dig dipper in the function semantic and find out
> > the problems it has. Meanwhile I'll keep relaxing...
> >
> > > > BTW, we cannot remove "res->wol_irq = res->irq", because Loongson
> > > > GMAC/GNET indeed supports WoL.
> > > Okay, I will not drop it in v13.
> > Apparently Huacai isn't well familiar with what he is reviewing. Once
> > again the initialization is useless. Drop it.
>
> Hmm, to be honest, I'm still a little confused about this.
>
> When we first designed the driver, we looked at intel,See:
>
> $: vim drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c +953
>
> static int stmmac_config_single_msi(struct pci_dev *pdev,
> struct plat_stmmacenet_data *plat,
> struct stmmac_resources *res)
> {
> int ret;
>
> ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> if (ret < 0) {
> dev_info(&pdev->dev, "%s: Single IRQ enablement failed\n",
> __func__);
> return ret;
> }
>
> res->irq = pci_irq_vector(pdev, 0);
> res->wol_irq = res->irq;
>
> Why can't we do this?
>
> Intel Patch thread link <https://lore.kernel.org/netdev/20210316121823.18659-5-weifeng.voon@intel.com/>
First of all the Intel' STMMAC patches isn't something what can be
referred to as a good-practice example. A significant part of the
mess in the plat_stmmacenet_data structure is their doing.
Secondly as I already said several times initializing res->wol_irq
with res->irq is _useless_. It's because of the way the WoL IRQ line
is requested:
stmmac_request_irq_single(struct net_device *dev)
{
...
if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) {
ret = request_irq(priv->wol_irq, stmmac_interrupt,
IRQF_SHARED, dev->name, dev);
...
}
...
}
stmmac_request_irq_multi_msi(struct net_device *dev)
{
...
if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) {
int_name = priv->int_name_wol;
sprintf(int_name, "%s:%s", dev->name, "wol");
ret = request_irq(priv->wol_irq,
stmmac_mac_interrupt,
0, int_name, dev);
...
}
...
}
See, even if you initialize priv->wol_irq with dev->irq (res->irq) it
will have the same effect as if you had it left uninitialized
(pre-initialized with zero). So from both maintainability and
readability points of view it's better to avoid a redundant code
especially if it causes an ill coding practice reproduction.
Interestingly to note that having res->wol_irq initialized with
res->irq had been required before another Intel' commit:
8532f613bc78 ("net: stmmac: introduce MSI Interrupt routines for mac, safety, RX & TX")
(submitted sometime around the commit you are referring to).
In that commit Intel' developers themself fixed the semantics in the
STMMAC core driver, but didn't bother with fixing the platform drivers
and even the Intel' DWMAC PCI driver has been left with that redundant
line of the code. Sigh...
>
>
> >
> > > All right. I have been preparing v13 and will send it as soon as possible.
> > >
> > > Let's continue the discussion in v13. Of course, I will copy the part that
> > > has
> > >
> > > not received a clear reply to v13.
> > >
> > Note the merge window has been opened and the 'net-next' tree is now
> > closed. So either you submit your series as RFC or wait for the window
> > being closed.
> >
> Ok, if I'm fast enough, I'll send an RFC to talk about msi and legacy.
It's up to you. But please be aware, I'll be busy next week with my
own patches cooking up. So I won't be able to actively participate in
your patches review.
-Serge(y)
>
>
> Thanks,
>
> Yanteng
>
next prev parent reply other threads:[~2024-05-17 16:37 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
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 [this message]
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=ikmwqzwplbnorwrao6afj6t4iksgo4t7jk6to65pnmtqgmalkv@gnrv5cskqlsb \
--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).