| Message ID | 20250121082536.11752-1-zhaoqunqin@loongson.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: dwmac-loongson: Add fix_soc_reset function | expand |
Hi, Qunqin, The patch itself looks good to me, but something can be improved. 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback" 2. You lack a "." at the end of the commit message. 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport to 6.12/6.13. After that you can add: Reviewed-by: Huacai Chen <chenhuacai@loongson.cn> Huacai On Tue, Jan 21, 2025 at 4:26 PM Qunqin Zhao <zhaoqunqin@loongson.cn> wrote: > > Loongson's GMAC device takes nearly two seconds to complete DMA reset, > however, the default waiting time for reset is 200 milliseconds > > Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn> > --- > .../net/ethernet/stmicro/stmmac/dwmac-loongson.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > index bfe6e2d631bd..35639d26256c 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > @@ -516,6 +516,18 @@ static int loongson_dwmac_acpi_config(struct pci_dev *pdev, > return 0; > } > > +static int loongson_fix_soc_reset(void *priv, void __iomem *ioaddr) > +{ > + u32 value = readl(ioaddr + DMA_BUS_MODE); > + > + value |= DMA_BUS_MODE_SFT_RESET; > + writel(value, ioaddr + DMA_BUS_MODE); > + > + return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value, > + !(value & DMA_BUS_MODE_SFT_RESET), > + 10000, 2000000); > +} > + > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct plat_stmmacenet_data *plat; > @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > plat->bsp_priv = ld; > plat->setup = loongson_dwmac_setup; > + plat->fix_soc_reset = loongson_fix_soc_reset; > ld->dev = &pdev->dev; > ld->loongson_id = readl(res.addr + GMAC_VERSION) & 0xff; > > > base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532 > -- > 2.43.0 >
在 1/21/25 16:25, Qunqin Zhao 写道: > Loongson's GMAC device takes nearly two seconds to complete DMA reset, > however, the default waiting time for reset is 200 milliseconds Is only GMAC like this? > > Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn> > --- > .../net/ethernet/stmicro/stmmac/dwmac-loongson.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > index bfe6e2d631bd..35639d26256c 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > @@ -516,6 +516,18 @@ static int loongson_dwmac_acpi_config(struct pci_dev *pdev, > return 0; > } > How about putting a part of the commit message here as a comment? > +static int loongson_fix_soc_reset(void *priv, void __iomem *ioaddr) > +{ > + u32 value = readl(ioaddr + DMA_BUS_MODE); > + > + value |= DMA_BUS_MODE_SFT_RESET; > + writel(value, ioaddr + DMA_BUS_MODE); > + > + return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value, > + !(value & DMA_BUS_MODE_SFT_RESET), > + 10000, 2000000); > +} > + > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct plat_stmmacenet_data *plat; > @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > plat->bsp_priv = ld; > plat->setup = loongson_dwmac_setup; > + plat->fix_soc_reset = loongson_fix_soc_reset; If only GMAC needs to be done this way, how about putting it inside the loongson_gmac_data()? Thanks, Yanteng
在 1/21/25 17:29, Huacai Chen 写道: > Hi, Qunqin, > > The patch itself looks good to me, but something can be improved. > 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback" > 2. You lack a "." at the end of the commit message. > 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport > to 6.12/6.13. Then we also need to have a fixes tag. Thanks, Yanteng
On Tue, 21 Jan 2025 17:29:37 +0800 Huacai Chen wrote: > Hi, Qunqin, > > The patch itself looks good to me, but something can be improved. > 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback" > 2. You lack a "." at the end of the commit message. > 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport > to 6.12/6.13. Please don't top post.
在 2025/1/21 下午5:29, Huacai Chen 写道: > Hi, Qunqin, > > The patch itself looks good to me, but something can be improved. > 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback" > 2. You lack a "." at the end of the commit message. > 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport > to 6.12/6.13. > > After that you can add: > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn> OK, Thanks > > > Huacai > > On Tue, Jan 21, 2025 at 4:26 PM Qunqin Zhao <zhaoqunqin@loongson.cn> wrote: >> Loongson's GMAC device takes nearly two seconds to complete DMA reset, >> however, the default waiting time for reset is 200 milliseconds >> >> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn> >> --- >> .../net/ethernet/stmicro/stmmac/dwmac-loongson.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> index bfe6e2d631bd..35639d26256c 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> @@ -516,6 +516,18 @@ static int loongson_dwmac_acpi_config(struct pci_dev *pdev, >> return 0; >> } >> >> +static int loongson_fix_soc_reset(void *priv, void __iomem *ioaddr) >> +{ >> + u32 value = readl(ioaddr + DMA_BUS_MODE); >> + >> + value |= DMA_BUS_MODE_SFT_RESET; >> + writel(value, ioaddr + DMA_BUS_MODE); >> + >> + return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value, >> + !(value & DMA_BUS_MODE_SFT_RESET), >> + 10000, 2000000); >> +} >> + >> static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> { >> struct plat_stmmacenet_data *plat; >> @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id >> >> plat->bsp_priv = ld; >> plat->setup = loongson_dwmac_setup; >> + plat->fix_soc_reset = loongson_fix_soc_reset; >> ld->dev = &pdev->dev; >> ld->loongson_id = readl(res.addr + GMAC_VERSION) & 0xff; >> >> >> base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532 >> -- >> 2.43.0 >>
在 2025/1/21 下午9:41, Yanteng Si 写道: > > 在 1/21/25 16:25, Qunqin Zhao 写道: >> Loongson's GMAC device takes nearly two seconds to complete DMA reset, >> however, the default waiting time for reset is 200 milliseconds > Is only GMAC like this? At present, this situation has only been found on GMAC. >> >> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn> >> --- >> .../net/ethernet/stmicro/stmmac/dwmac-loongson.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> index bfe6e2d631bd..35639d26256c 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> @@ -516,6 +516,18 @@ static int loongson_dwmac_acpi_config(struct >> pci_dev *pdev, >> return 0; >> } > How about putting a part of the commit message here as a comment? Sure, will do that. >> +static int loongson_fix_soc_reset(void *priv, void __iomem *ioaddr) > > >> +{ >> + u32 value = readl(ioaddr + DMA_BUS_MODE); >> + >> + value |= DMA_BUS_MODE_SFT_RESET; >> + writel(value, ioaddr + DMA_BUS_MODE); >> + >> + return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value, >> + !(value & DMA_BUS_MODE_SFT_RESET), >> + 10000, 2000000); >> +} >> + >> static int loongson_dwmac_probe(struct pci_dev *pdev, const struct >> pci_device_id *id) >> { >> struct plat_stmmacenet_data *plat; >> @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev >> *pdev, const struct pci_device_id >> plat->bsp_priv = ld; >> plat->setup = loongson_dwmac_setup; >> + plat->fix_soc_reset = loongson_fix_soc_reset; > > If only GMAC needs to be done this way, how about putting it inside > the loongson_gmac_data()? Regardless of whether this situation occurs in other devices(like gnet), this change will not have any impact on gnet, right? Thanks. > > Thanks, > > Yanteng
在 2025/1/21 下午9:47, Yanteng Si 写道: > > 在 1/21/25 17:29, Huacai Chen 写道: >> Hi, Qunqin, >> >> The patch itself looks good to me, but something can be improved. >> 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() >> callback" >> 2. You lack a "." at the end of the commit message. > >> 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport >> to 6.12/6.13. > > Then we also need to have a fixes tag. OK, thanks. > > Thanks, > > Yanteng >
Hi, Jakub, On Wed, Jan 22, 2025 at 2:11 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 21 Jan 2025 17:29:37 +0800 Huacai Chen wrote: > > Hi, Qunqin, > > > > The patch itself looks good to me, but something can be improved. > > 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback" > > 2. You lack a "." at the end of the commit message. > > 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport > > to 6.12/6.13. > > Please don't top post. I know about inline replies, but in this case I agree with the code itself so I cannot reply after the code. Huacai
在 2025/1/22 09:31, Qunqin Zhao 写道: > > 在 2025/1/21 下午9:41, Yanteng Si 写道: >> >> 在 1/21/25 16:25, Qunqin Zhao 写道: >>> Loongson's GMAC device takes nearly two seconds to complete DMA reset, >>> however, the default waiting time for reset is 200 milliseconds >> Is only GMAC like this? > At present, this situation has only been found on GMAC. >>> @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev >>> *pdev, const struct pci_device_id >>> plat->bsp_priv = ld; >>> plat->setup = loongson_dwmac_setup; >>> + plat->fix_soc_reset = loongson_fix_soc_reset; >> >> If only GMAC needs to be done this way, how about putting it inside >> the loongson_gmac_data()? > > Regardless of whether this situation occurs in other devices(like > gnet), this change will not have any impact on gnet, right? > Yeah，However, it is obvious that there is now a more suitable place for it. In the Loongson driver, `loongson_gmac_data()` and `loongson_default_data()` were designed from the beginning. When GNET support was added later, `loongson_gnet_data()` was designed. We once made great efforts to extract these codes from the `probe()` . Are we going to go back to the past? Of course, I'm not saying that I disagree with you fixing the GMAC in the `probe()`. I just think it's a bad start. After that, other people may also put similar code here, and eventually it will make the `probe` a mess. If you insist on doing this, please change the function name to `loongson_gmac_fix_reset()`, just like `loongson_gnet_fix_speed`. Thanks, Yanteng
On Wed, 22 Jan 2025 12:09:28 +0800 Huacai Chen wrote: > Subject: Re: [PATCH] net: stmmac: dwmac-loongson: Add fix_soc_reset function > > On Wed, Jan 22, 2025 at 2:11 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Tue, 21 Jan 2025 17:29:37 +0800 Huacai Chen wrote: > > > Hi, Qunqin, > > > > > > The patch itself looks good to me, but something can be improved. > > > 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback" > > > 2. You lack a "." at the end of the commit message. > > > 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport > > > to 6.12/6.13. > > > > Please don't top post. > I know about inline replies, but in this case I agree with the code > itself so I cannot reply after the code. You're not trying hard enough. The message is Subject, body and code. Reply in the place where the problem is or where the CC stable should be added.
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index bfe6e2d631bd..35639d26256c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c @@ -516,6 +516,18 @@ static int loongson_dwmac_acpi_config(struct pci_dev *pdev, return 0; } +static int loongson_fix_soc_reset(void *priv, void __iomem *ioaddr) +{ + u32 value = readl(ioaddr + DMA_BUS_MODE); + + value |= DMA_BUS_MODE_SFT_RESET; + writel(value, ioaddr + DMA_BUS_MODE); + + return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value, + !(value & DMA_BUS_MODE_SFT_RESET), + 10000, 2000000); +} + static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct plat_stmmacenet_data *plat; @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id plat->bsp_priv = ld; plat->setup = loongson_dwmac_setup; + plat->fix_soc_reset = loongson_fix_soc_reset; ld->dev = &pdev->dev; ld->loongson_id = readl(res.addr + GMAC_VERSION) & 0xff;
Loongson's GMAC device takes nearly two seconds to complete DMA reset, however, the default waiting time for reset is 200 milliseconds Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn> --- .../net/ethernet/stmicro/stmmac/dwmac-loongson.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532