| Message ID | 20231212115841.3800241-1-quic_jsuraj@quicinc.com |
|---|---|
| Headers | show |
| Series | Ethernet DWMAC5 fault IRQ support | expand |
On Tue, 2023-12-12 at 17:28 +0530, Suraj Jaiswal wrote: > @@ -3759,6 +3763,7 @@ static int stmmac_request_irq_single(struct net_device *dev) > struct stmmac_priv *priv = netdev_priv(dev); > enum request_irq_err irq_err; > int ret; > + char *int_name; I'm sorry to nit-pick, but please respect the reverse x-mas tree above, Otherwise LGTM! Cheers, Paolo
Hi Suraj On Tue, Dec 12, 2023 at 05:28:41PM +0530, Suraj Jaiswal wrote: > Add support to listen HW safety IRQ like ECC(error > correction code), DPP(data path parity), FSM(finite state > machine) fault in common IRQ line. > > Signed-off-by: Suraj Jaiswal <quic_jsuraj@quicinc.com> > --- > drivers/net/ethernet/stmicro/stmmac/common.h | 1 + > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++ > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++ > .../ethernet/stmicro/stmmac/stmmac_platform.c | 9 ++++++++ > 4 files changed, 34 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > index 721c1f8e892f..b9233b09b80f 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/common.h > +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > @@ -344,6 +344,7 @@ enum request_irq_err { > REQ_IRQ_ERR_ALL, > REQ_IRQ_ERR_TX, > REQ_IRQ_ERR_RX, > + REQ_IRQ_ERR_SFTY, > REQ_IRQ_ERR_SFTY_UE, > REQ_IRQ_ERR_SFTY_CE, > REQ_IRQ_ERR_LPI, > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > index 9f89acf31050..ca3d93851bed 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > @@ -31,6 +31,7 @@ struct stmmac_resources { > int wol_irq; > int lpi_irq; > int irq; > + int sfty_irq; > int sfty_ce_irq; > int sfty_ue_irq; > int rx_irq[MTL_MAX_RX_QUEUES]; > @@ -297,6 +298,7 @@ struct stmmac_priv { > void __iomem *ptpaddr; > void __iomem *estaddr; > unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)]; > + int sfty_irq; > int sfty_ce_irq; > int sfty_ue_irq; > int rx_irq[MTL_MAX_RX_QUEUES]; > @@ -305,6 +307,7 @@ struct stmmac_priv { > char int_name_mac[IFNAMSIZ + 9]; > char int_name_wol[IFNAMSIZ + 9]; > char int_name_lpi[IFNAMSIZ + 9]; > + char int_name_sfty[IFNAMSIZ + 10]; > char int_name_sfty_ce[IFNAMSIZ + 10]; > char int_name_sfty_ue[IFNAMSIZ + 10]; > char int_name_rx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 14]; > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 47de466e432c..6cf289f192a7 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -3592,6 +3592,10 @@ static void stmmac_free_irq(struct net_device *dev, > if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) > free_irq(priv->wol_irq, dev); > fallthrough; > + case REQ_IRQ_ERR_SFTY: > + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) > + free_irq(priv->sfty_irq, dev); > + fallthrough; > case REQ_IRQ_ERR_WOL: > free_irq(dev->irq, dev); > fallthrough; > @@ -3759,6 +3763,7 @@ static int stmmac_request_irq_single(struct net_device *dev) > struct stmmac_priv *priv = netdev_priv(dev); > enum request_irq_err irq_err; > int ret; > + char *int_name; See my comment below. > > ret = request_irq(dev->irq, stmmac_interrupt, > IRQF_SHARED, dev->name, dev); > @@ -3798,6 +3803,20 @@ static int stmmac_request_irq_single(struct net_device *dev) > } > } > > + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) { > + int_name = priv->int_name_sfty; > + sprintf(int_name, "%s:%s", dev->name, "safety"); > + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt, > + 0, int_name, dev); > + if (unlikely(ret < 0)) { > + netdev_err(priv->dev, > + "%s: alloc safety failed %d (error: %d)\n", > + __func__, priv->sfty_irq, ret); > + irq_err = REQ_IRQ_ERR_SFTY; > + goto irq_error; > + } > + } > + Omg, I thought this change belonged to stmmac_request_irq_multi_msi(). My bad, sorry. Please move the code above to stmmac_request_irq_multi_msi() and get back the part in stmmac_request_irq_single() as it was in v5, but instead of specifying "safety" IRQ name use "dev->name" as the rest of similar code snippets in here have: + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) { + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt, + 0, dev->name, dev); + if (unlikely(ret < 0)) { + netdev_err(priv->dev, + "%s: alloc safety failed %d (error: %d)\n", + __func__, priv->sfty_irq, ret); + irq_err = REQ_IRQ_ERR_SFTY; + goto irq_error; + } + } I guess at some point afterwards we'll need to refactor the IRQs request part of this driver: replace stmmac_request_irq_single() body with the upper part of the stmmac_request_irq_multi_msi() method and then just make the former method being called from the later one... > return 0; > > irq_error: > @@ -7462,8 +7481,10 @@ int stmmac_dvr_probe(struct device *device, > priv->dev->irq = res->irq; > priv->wol_irq = res->wol_irq; > priv->lpi_irq = res->lpi_irq; > + priv->sfty_irq = res->sfty_irq; > priv->sfty_ce_irq = res->sfty_ce_irq; > priv->sfty_ue_irq = res->sfty_ue_irq; > + Please drop this change. The code below is attached to the code above because it basically does the same but in the loop. > for (i = 0; i < MTL_MAX_RX_QUEUES; i++) > priv->rx_irq[i] = res->rx_irq[i]; > for (i = 0; i < MTL_MAX_TX_QUEUES; i++) > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index 1ffde555da47..3808a3225a7d 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -726,6 +726,15 @@ int stmmac_get_platform_resources(struct platform_device *pdev, > dev_info(&pdev->dev, "IRQ eth_lpi not found\n"); > } > > + stmmac_res->sfty_irq = > + platform_get_irq_byname_optional(pdev, "sfty"); > + Please drop this change too. It's normal to have a method call attached to the error check statement especially seeing the rest of the similar code snippets are designed that way in this function. -Serge(y) > + if (stmmac_res->sfty_irq < 0) { > + if (stmmac_res->sfty_irq == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + dev_info(&pdev->dev, "IRQ safety IRQ not found\n"); > + } > + > stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0); > > return PTR_ERR_OR_ZERO(stmmac_res->addr); > -- > 2.25.1 > >
Hi Serge, Please find commnet inline & let me know if any further action needed Thanks Suraj On 12/14/2023 8:42 PM, Serge Semin wrote: > Hi Suraj > > On Tue, Dec 12, 2023 at 05:28:41PM +0530, Suraj Jaiswal wrote: >> Add support to listen HW safety IRQ like ECC(error >> correction code), DPP(data path parity), FSM(finite state >> machine) fault in common IRQ line. >> >> Signed-off-by: Suraj Jaiswal <quic_jsuraj@quicinc.com> >> --- >> drivers/net/ethernet/stmicro/stmmac/common.h | 1 + >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++ >> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++ >> .../ethernet/stmicro/stmmac/stmmac_platform.c | 9 ++++++++ >> 4 files changed, 34 insertions(+) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h >> index 721c1f8e892f..b9233b09b80f 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/common.h >> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h >> @@ -344,6 +344,7 @@ enum request_irq_err { >> REQ_IRQ_ERR_ALL, >> REQ_IRQ_ERR_TX, >> REQ_IRQ_ERR_RX, >> + REQ_IRQ_ERR_SFTY, >> REQ_IRQ_ERR_SFTY_UE, >> REQ_IRQ_ERR_SFTY_CE, >> REQ_IRQ_ERR_LPI, >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> index 9f89acf31050..ca3d93851bed 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> @@ -31,6 +31,7 @@ struct stmmac_resources { >> int wol_irq; >> int lpi_irq; >> int irq; >> + int sfty_irq; >> int sfty_ce_irq; >> int sfty_ue_irq; >> int rx_irq[MTL_MAX_RX_QUEUES]; >> @@ -297,6 +298,7 @@ struct stmmac_priv { >> void __iomem *ptpaddr; >> void __iomem *estaddr; >> unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)]; >> + int sfty_irq; >> int sfty_ce_irq; >> int sfty_ue_irq; >> int rx_irq[MTL_MAX_RX_QUEUES]; >> @@ -305,6 +307,7 @@ struct stmmac_priv { >> char int_name_mac[IFNAMSIZ + 9]; >> char int_name_wol[IFNAMSIZ + 9]; >> char int_name_lpi[IFNAMSIZ + 9]; >> + char int_name_sfty[IFNAMSIZ + 10]; >> char int_name_sfty_ce[IFNAMSIZ + 10]; >> char int_name_sfty_ue[IFNAMSIZ + 10]; >> char int_name_rx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 14]; >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> index 47de466e432c..6cf289f192a7 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> @@ -3592,6 +3592,10 @@ static void stmmac_free_irq(struct net_device *dev, >> if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) >> free_irq(priv->wol_irq, dev); >> fallthrough; >> + case REQ_IRQ_ERR_SFTY: >> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) >> + free_irq(priv->sfty_irq, dev); >> + fallthrough; >> case REQ_IRQ_ERR_WOL: >> free_irq(dev->irq, dev); >> fallthrough; >> @@ -3759,6 +3763,7 @@ static int stmmac_request_irq_single(struct net_device *dev) >> struct stmmac_priv *priv = netdev_priv(dev); >> enum request_irq_err irq_err; >> int ret; > >> + char *int_name; > > See my comment below. > >> >> ret = request_irq(dev->irq, stmmac_interrupt, >> IRQF_SHARED, dev->name, dev); >> @@ -3798,6 +3803,20 @@ static int stmmac_request_irq_single(struct net_device *dev) >> } >> } >> > >> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) { >> + int_name = priv->int_name_sfty; >> + sprintf(int_name, "%s:%s", dev->name, "safety"); >> + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt, >> + 0, int_name, dev); >> + if (unlikely(ret < 0)) { >> + netdev_err(priv->dev, >> + "%s: alloc safety failed %d (error: %d)\n", >> + __func__, priv->sfty_irq, ret); >> + irq_err = REQ_IRQ_ERR_SFTY; >> + goto irq_error; >> + } >> + } >> + > > Omg, I thought this change belonged to stmmac_request_irq_multi_msi(). > My bad, sorry. Please move the code above to > stmmac_request_irq_multi_msi() and get back the part in > stmmac_request_irq_single() as it was in v5, but instead of specifying > "safety" IRQ name use "dev->name" as the rest of similar code snippets > in here have: > > + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) { > + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt, > + 0, dev->name, dev); > + if (unlikely(ret < 0)) { > + netdev_err(priv->dev, > + "%s: alloc safety failed %d (error: %d)\n", > + __func__, priv->sfty_irq, ret); > + irq_err = REQ_IRQ_ERR_SFTY; > + goto irq_error; > + } > + } <Suraj> We can not use "dev->name" as this is name already used by "stmmac_interrupt" @ https://elixir.bootlin.com/linux/v6.1.68/source/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c#L3655. < ret = request_irq(dev->irq, stmmac_interrupt, IRQF_SHARED, dev->name, dev); > if we are using same "dev->name" while requesting safety IRQ as well then "/proc/interrupt" will show same name eth0/eth1 for both "stmmac_interrupt" & "safety interrupt" and by looking at "/proc/interrupt" output we can not say which IRQ is for safety and which is for stmmac_interrupt. > > I guess at some point afterwards we'll need to refactor the IRQs > request part of this driver: replace stmmac_request_irq_single() body > with the upper part of the stmmac_request_irq_multi_msi() method and > then just make the former method being called from the later one... > >> return 0; >> >> irq_error: >> @@ -7462,8 +7481,10 @@ int stmmac_dvr_probe(struct device *device, >> priv->dev->irq = res->irq; >> priv->wol_irq = res->wol_irq; >> priv->lpi_irq = res->lpi_irq; >> + priv->sfty_irq = res->sfty_irq; >> priv->sfty_ce_irq = res->sfty_ce_irq; >> priv->sfty_ue_irq = res->sfty_ue_irq; > >> + > > Please drop this change. The code below is attached to the code above > because it basically does the same but in the loop. <Suraj> below loop code "for (i = 0; i < MTL_MAX_RX_QUEUES; i++) priv->rx_irq[i] = res->rx_irq[i];" is not for rx_irq array and will not help for safety irq. Let me know if I got your commnet properly . > >> for (i = 0; i < MTL_MAX_RX_QUEUES; i++) >> priv->rx_irq[i] = res->rx_irq[i]; >> for (i = 0; i < MTL_MAX_TX_QUEUES; i++) >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> index 1ffde555da47..3808a3225a7d 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> @@ -726,6 +726,15 @@ int stmmac_get_platform_resources(struct platform_device *pdev, >> dev_info(&pdev->dev, "IRQ eth_lpi not found\n"); >> } >> >> + stmmac_res->sfty_irq = >> + platform_get_irq_byname_optional(pdev, "sfty"); > >> + > > Please drop this change too. It's normal to have a method call > attached to the error check statement especially seeing the rest of > the similar code snippets are designed that way in this function. <Suraj> Do you means to remove all below code where we are printing the dev_info() message ? We added this code similar to LPM code. > > -Serge(y) > >> + if (stmmac_res->sfty_irq < 0) { >> + if (stmmac_res->sfty_irq == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> + dev_info(&pdev->dev, "IRQ safety IRQ not found\n"); >> + } >> + >> stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0); >> >> return PTR_ERR_OR_ZERO(stmmac_res->addr); >> -- >> 2.25.1 >> >>
On Mon, Dec 18, 2023 at 03:27:54PM +0530, Suraj Jaiswal wrote: > > Hi Serge, > Please find commnet inline & let me know if any further action needed > > Thanks > Suraj > > On 12/14/2023 8:42 PM, Serge Semin wrote: > > Hi Suraj > > > > On Tue, Dec 12, 2023 at 05:28:41PM +0530, Suraj Jaiswal wrote: > >> Add support to listen HW safety IRQ like ECC(error > >> correction code), DPP(data path parity), FSM(finite state > >> machine) fault in common IRQ line. > >> > >> Signed-off-by: Suraj Jaiswal <quic_jsuraj@quicinc.com> > >> --- > >> drivers/net/ethernet/stmicro/stmmac/common.h | 1 + > >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++ > >> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++ > >> .../ethernet/stmicro/stmmac/stmmac_platform.c | 9 ++++++++ > >> 4 files changed, 34 insertions(+) > >> > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > >> index 721c1f8e892f..b9233b09b80f 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/common.h > >> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > >> @@ -344,6 +344,7 @@ enum request_irq_err { > >> REQ_IRQ_ERR_ALL, > >> REQ_IRQ_ERR_TX, > >> REQ_IRQ_ERR_RX, > >> + REQ_IRQ_ERR_SFTY, > >> REQ_IRQ_ERR_SFTY_UE, > >> REQ_IRQ_ERR_SFTY_CE, > >> REQ_IRQ_ERR_LPI, > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > >> index 9f89acf31050..ca3d93851bed 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > >> @@ -31,6 +31,7 @@ struct stmmac_resources { > >> int wol_irq; > >> int lpi_irq; > >> int irq; > >> + int sfty_irq; > >> int sfty_ce_irq; > >> int sfty_ue_irq; > >> int rx_irq[MTL_MAX_RX_QUEUES]; > >> @@ -297,6 +298,7 @@ struct stmmac_priv { > >> void __iomem *ptpaddr; > >> void __iomem *estaddr; > >> unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)]; > >> + int sfty_irq; > >> int sfty_ce_irq; > >> int sfty_ue_irq; > >> int rx_irq[MTL_MAX_RX_QUEUES]; > >> @@ -305,6 +307,7 @@ struct stmmac_priv { > >> char int_name_mac[IFNAMSIZ + 9]; > >> char int_name_wol[IFNAMSIZ + 9]; > >> char int_name_lpi[IFNAMSIZ + 9]; > >> + char int_name_sfty[IFNAMSIZ + 10]; > >> char int_name_sfty_ce[IFNAMSIZ + 10]; > >> char int_name_sfty_ue[IFNAMSIZ + 10]; > >> char int_name_rx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 14]; > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >> index 47de466e432c..6cf289f192a7 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >> @@ -3592,6 +3592,10 @@ static void stmmac_free_irq(struct net_device *dev, > >> if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) > >> free_irq(priv->wol_irq, dev); > >> fallthrough; > >> + case REQ_IRQ_ERR_SFTY: > >> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) > >> + free_irq(priv->sfty_irq, dev); > >> + fallthrough; > >> case REQ_IRQ_ERR_WOL: > >> free_irq(dev->irq, dev); > >> fallthrough; > >> @@ -3759,6 +3763,7 @@ static int stmmac_request_irq_single(struct net_device *dev) > >> struct stmmac_priv *priv = netdev_priv(dev); > >> enum request_irq_err irq_err; > >> int ret; > > > >> + char *int_name; > > > > See my comment below. > > > >> > >> ret = request_irq(dev->irq, stmmac_interrupt, > >> IRQF_SHARED, dev->name, dev); > >> @@ -3798,6 +3803,20 @@ static int stmmac_request_irq_single(struct net_device *dev) > >> } > >> } > >> > > > >> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) { > >> + int_name = priv->int_name_sfty; > >> + sprintf(int_name, "%s:%s", dev->name, "safety"); > >> + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt, > >> + 0, int_name, dev); > >> + if (unlikely(ret < 0)) { > >> + netdev_err(priv->dev, > >> + "%s: alloc safety failed %d (error: %d)\n", > >> + __func__, priv->sfty_irq, ret); > >> + irq_err = REQ_IRQ_ERR_SFTY; > >> + goto irq_error; > >> + } > >> + } > >> + > > > > Omg, I thought this change belonged to stmmac_request_irq_multi_msi(). > > My bad, sorry. Please move the code above to > > stmmac_request_irq_multi_msi() and get back the part in > > stmmac_request_irq_single() as it was in v5, Please note my comment regarding the common safety IRQ being supported in both stmmac_request_irq_single() and stmmac_request_irq_multi_msi() methods. > but instead of specifying > > "safety" IRQ name use "dev->name" as the rest of similar code snippets > > in here have: > > > > + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) { > > + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt, > > + 0, dev->name, dev); > > + if (unlikely(ret < 0)) { > > + netdev_err(priv->dev, > > + "%s: alloc safety failed %d (error: %d)\n", > > + __func__, priv->sfty_irq, ret); > > + irq_err = REQ_IRQ_ERR_SFTY; > > + goto irq_error; > > + } > > + } > > <Suraj> We can not use "dev->name" as this is name already used by "stmmac_interrupt" @ https://elixir.bootlin.com/linux/v6.1.68/source/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c#L3655. It's not that much of the problem. The main idea is to convert your solution to following the _local_ coding convention. See, the rest of the IRQs in stmmac_request_irq_single() are requested with "dev->name" being specified as the IRQ name (irrespective to having such solution being not that correct). That's what I was talking about. If you want the safety IRQ to have an unique name, then please submit this patch as I suggested above and _then_, on top of it, add a new patch which would convert the entire stmmac_request_irq_single() method to creating all IRQ names as it's, for instance, done in stmmac_request_irq_multi_msi(). > < > ret = request_irq(dev->irq, stmmac_interrupt, > IRQF_SHARED, dev->name, dev); > > > if we are using same "dev->name" while requesting safety IRQ as well then "/proc/interrupt" will show same name eth0/eth1 for both "stmmac_interrupt" & "safety interrupt" and by looking at "/proc/interrupt" output we can not say which IRQ is for safety and which is for stmmac_interrupt. Thanks. I am perfectly aware of that. Please see my comment above. > > > > I guess at some point afterwards we'll need to refactor the IRQs > > request part of this driver: replace stmmac_request_irq_single() body > > with the upper part of the stmmac_request_irq_multi_msi() method and > > then just make the former method being called from the later one... > > > >> return 0; > >> > >> irq_error: > >> @@ -7462,8 +7481,10 @@ int stmmac_dvr_probe(struct device *device, > >> priv->dev->irq = res->irq; > >> priv->wol_irq = res->wol_irq; > >> priv->lpi_irq = res->lpi_irq; > >> + priv->sfty_irq = res->sfty_irq; > >> priv->sfty_ce_irq = res->sfty_ce_irq; > >> priv->sfty_ue_irq = res->sfty_ue_irq; > > > >> + > > > > Please drop this change. The code below is attached to the code above > > because it basically does the same but in the loop. > <Suraj> below loop code "for (i = 0; i < MTL_MAX_RX_QUEUES; i++) priv->rx_irq[i] = res->rx_irq[i];" is not for rx_irq array and will not help for safety irq. > Let me know if I got your commnet properly . Sorry, you didn't. My comment concerned the _empty_ line you placed between the code above and below. You shouldn't have done that. > > > >> for (i = 0; i < MTL_MAX_RX_QUEUES; i++) > >> priv->rx_irq[i] = res->rx_irq[i]; > >> for (i = 0; i < MTL_MAX_TX_QUEUES; i++) > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > >> index 1ffde555da47..3808a3225a7d 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > >> @@ -726,6 +726,15 @@ int stmmac_get_platform_resources(struct platform_device *pdev, > >> dev_info(&pdev->dev, "IRQ eth_lpi not found\n"); > >> } > >> > >> + stmmac_res->sfty_irq = > >> + platform_get_irq_byname_optional(pdev, "sfty"); > > > >> + > > > > Please drop this change too. It's normal to have a method call > > attached to the error check statement especially seeing the rest of > > the similar code snippets are designed that way in this function. > <Suraj> Do you means to remove all below code where we are printing the dev_info() message ? No. I was referring to the _empty_ line between the method above and the error check code below. It's pointless and at the very least breaks the local coding convention. -Serge(y) > We added this code similar to LPM code. > > > > -Serge(y) > > > >> + if (stmmac_res->sfty_irq < 0) { > >> + if (stmmac_res->sfty_irq == -EPROBE_DEFER) > >> + return -EPROBE_DEFER; > >> + dev_info(&pdev->dev, "IRQ safety IRQ not found\n"); > >> + } > >> + > >> stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0); > >> > >> return PTR_ERR_OR_ZERO(stmmac_res->addr); > >> -- > >> 2.25.1 > >> > >>
Hi Serge, takan care of all commnets in V7 . Please review Thanks Suraj On 12/18/2023 4:18 PM, Serge Semin wrote: > On Mon, Dec 18, 2023 at 03:27:54PM +0530, Suraj Jaiswal wrote: >> >> Hi Serge, >> Please find commnet inline & let me know if any further action needed >> >> Thanks >> Suraj >> >> On 12/14/2023 8:42 PM, Serge Semin wrote: >>> Hi Suraj >>> >>> On Tue, Dec 12, 2023 at 05:28:41PM +0530, Suraj Jaiswal wrote: >>>> Add support to listen HW safety IRQ like ECC(error >>>> correction code), DPP(data path parity), FSM(finite state >>>> machine) fault in common IRQ line. >>>> >>>> Signed-off-by: Suraj Jaiswal <quic_jsuraj@quicinc.com> >>>> --- >>>> drivers/net/ethernet/stmicro/stmmac/common.h | 1 + >>>> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++ >>>> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++ >>>> .../ethernet/stmicro/stmmac/stmmac_platform.c | 9 ++++++++ >>>> 4 files changed, 34 insertions(+) >>>> >>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h >>>> index 721c1f8e892f..b9233b09b80f 100644 >>>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h >>>> @@ -344,6 +344,7 @@ enum request_irq_err { >>>> REQ_IRQ_ERR_ALL, >>>> REQ_IRQ_ERR_TX, >>>> REQ_IRQ_ERR_RX, >>>> + REQ_IRQ_ERR_SFTY, >>>> REQ_IRQ_ERR_SFTY_UE, >>>> REQ_IRQ_ERR_SFTY_CE, >>>> REQ_IRQ_ERR_LPI, >>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>>> index 9f89acf31050..ca3d93851bed 100644 >>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>>> @@ -31,6 +31,7 @@ struct stmmac_resources { >>>> int wol_irq; >>>> int lpi_irq; >>>> int irq; >>>> + int sfty_irq; >>>> int sfty_ce_irq; >>>> int sfty_ue_irq; >>>> int rx_irq[MTL_MAX_RX_QUEUES]; >>>> @@ -297,6 +298,7 @@ struct stmmac_priv { >>>> void __iomem *ptpaddr; >>>> void __iomem *estaddr; >>>> unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)]; >>>> + int sfty_irq; >>>> int sfty_ce_irq; >>>> int sfty_ue_irq; >>>> int rx_irq[MTL_MAX_RX_QUEUES]; >>>> @@ -305,6 +307,7 @@ struct stmmac_priv { >>>> char int_name_mac[IFNAMSIZ + 9]; >>>> char int_name_wol[IFNAMSIZ + 9]; >>>> char int_name_lpi[IFNAMSIZ + 9]; >>>> + char int_name_sfty[IFNAMSIZ + 10]; >>>> char int_name_sfty_ce[IFNAMSIZ + 10]; >>>> char int_name_sfty_ue[IFNAMSIZ + 10]; >>>> char int_name_rx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 14]; >>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>> index 47de466e432c..6cf289f192a7 100644 >>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>> @@ -3592,6 +3592,10 @@ static void stmmac_free_irq(struct net_device *dev, >>>> if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) >>>> free_irq(priv->wol_irq, dev); >>>> fallthrough; >>>> + case REQ_IRQ_ERR_SFTY: >>>> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) >>>> + free_irq(priv->sfty_irq, dev); >>>> + fallthrough; >>>> case REQ_IRQ_ERR_WOL: >>>> free_irq(dev->irq, dev); >>>> fallthrough; >>>> @@ -3759,6 +3763,7 @@ static int stmmac_request_irq_single(struct net_device *dev) >>>> struct stmmac_priv *priv = netdev_priv(dev); >>>> enum request_irq_err irq_err; >>>> int ret; >>> >>>> + char *int_name; >>> >>> See my comment below. >>> >>>> >>>> ret = request_irq(dev->irq, stmmac_interrupt, >>>> IRQF_SHARED, dev->name, dev); >>>> @@ -3798,6 +3803,20 @@ static int stmmac_request_irq_single(struct net_device *dev) >>>> } >>>> } >>>> >>> >>>> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) { >>>> + int_name = priv->int_name_sfty; >>>> + sprintf(int_name, "%s:%s", dev->name, "safety"); >>>> + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt, >>>> + 0, int_name, dev); >>>> + if (unlikely(ret < 0)) { >>>> + netdev_err(priv->dev, >>>> + "%s: alloc safety failed %d (error: %d)\n", >>>> + __func__, priv->sfty_irq, ret); >>>> + irq_err = REQ_IRQ_ERR_SFTY; >>>> + goto irq_error; >>>> + } >>>> + } >>>> + >>> > >>> Omg, I thought this change belonged to stmmac_request_irq_multi_msi(). >>> My bad, sorry. Please move the code above to >>> stmmac_request_irq_multi_msi() and get back the part in >>> stmmac_request_irq_single() as it was in v5, > > Please note my comment regarding the common safety IRQ being supported > in both stmmac_request_irq_single() and stmmac_request_irq_multi_msi() > methods. > >> but instead of specifying >>> "safety" IRQ name use "dev->name" as the rest of similar code snippets >>> in here have: >>> >>> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) { >>> + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt, >>> + 0, dev->name, dev); >>> + if (unlikely(ret < 0)) { >>> + netdev_err(priv->dev, >>> + "%s: alloc safety failed %d (error: %d)\n", >>> + __func__, priv->sfty_irq, ret); >>> + irq_err = REQ_IRQ_ERR_SFTY; >>> + goto irq_error; >>> + } >>> + } >> > >> <Suraj> We can not use "dev->name" as this is name already used by "stmmac_interrupt" @ https://elixir.bootlin.com/linux/v6.1.68/source/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c#L3655. > > It's not that much of the problem. The main idea is to convert your > solution to following the _local_ coding convention. See, the rest of > the IRQs in stmmac_request_irq_single() are requested with "dev->name" > being specified as the IRQ name (irrespective to having such solution > being not that correct). That's what I was talking about. If you want > the safety IRQ to have an unique name, then please submit this patch > as I suggested above and _then_, on top of it, add a new patch which > would convert the entire stmmac_request_irq_single() method to > creating all IRQ names as it's, for instance, done in > stmmac_request_irq_multi_msi(). > >> < >> ret = request_irq(dev->irq, stmmac_interrupt, >> IRQF_SHARED, dev->name, dev); >>> > >> if we are using same "dev->name" while requesting safety IRQ as well then "/proc/interrupt" will show same name eth0/eth1 for both "stmmac_interrupt" & "safety interrupt" and by looking at "/proc/interrupt" output we can not say which IRQ is for safety and which is for stmmac_interrupt. > > Thanks. I am perfectly aware of that. Please see my comment above. > >>> >>> I guess at some point afterwards we'll need to refactor the IRQs >>> request part of this driver: replace stmmac_request_irq_single() body >>> with the upper part of the stmmac_request_irq_multi_msi() method and >>> then just make the former method being called from the later one... >>> >>>> return 0; >>>> >>>> irq_error: >>>> @@ -7462,8 +7481,10 @@ int stmmac_dvr_probe(struct device *device, >>>> priv->dev->irq = res->irq; >>>> priv->wol_irq = res->wol_irq; >>>> priv->lpi_irq = res->lpi_irq; >>>> + priv->sfty_irq = res->sfty_irq; >>>> priv->sfty_ce_irq = res->sfty_ce_irq; >>>> priv->sfty_ue_irq = res->sfty_ue_irq; >>> >>>> + >>> >>> Please drop this change. The code below is attached to the code above >>> because it basically does the same but in the loop. > >> <Suraj> below loop code "for (i = 0; i < MTL_MAX_RX_QUEUES; i++) priv->rx_irq[i] = res->rx_irq[i];" is not for rx_irq array and will not help for safety irq. >> Let me know if I got your commnet properly . > > Sorry, you didn't. My comment concerned the _empty_ line you placed > between the code above and below. You shouldn't have done that. > >>> >>>> for (i = 0; i < MTL_MAX_RX_QUEUES; i++) >>>> priv->rx_irq[i] = res->rx_irq[i]; >>>> for (i = 0; i < MTL_MAX_TX_QUEUES; i++) >>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>>> index 1ffde555da47..3808a3225a7d 100644 >>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>>> @@ -726,6 +726,15 @@ int stmmac_get_platform_resources(struct platform_device *pdev, >>>> dev_info(&pdev->dev, "IRQ eth_lpi not found\n"); >>>> } >>>> >>>> + stmmac_res->sfty_irq = >>>> + platform_get_irq_byname_optional(pdev, "sfty"); >>> >>>> + >>> >>> Please drop this change too. It's normal to have a method call >>> attached to the error check statement especially seeing the rest of >>> the similar code snippets are designed that way in this function. > >> <Suraj> Do you means to remove all below code where we are printing the dev_info() message ? > > No. I was referring to the _empty_ line between the method above and > the error check code below. It's pointless and at the very least > breaks the local coding convention. > > -Serge(y) > >> We added this code similar to LPM code. >>> >>> -Serge(y) >>> >>>> + if (stmmac_res->sfty_irq < 0) { >>>> + if (stmmac_res->sfty_irq == -EPROBE_DEFER) >>>> + return -EPROBE_DEFER; >>>> + dev_info(&pdev->dev, "IRQ safety IRQ not found\n"); >>>> + } >>>> + >>>> stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0); >>>> >>>> return PTR_ERR_OR_ZERO(stmmac_res->addr); >>>> -- >>>> 2.25.1 >>>> >>>>
Add support to listen Ethernet HW common safery IRQ for correctable and uncorrectable fault. The safety IRQ will be triggered for ECC(error correction code), DPP(data path parity, FSM(finite state machine) error. Changes since v6: - use name sfty_irq instead of safety_common_irq. Changes since v5: - Add description of ECC, DPP, FSM Changes since v4: - Fix DT_CHECKER warning - use name safety for the IRQ. Suraj Jaiswal (3): dt-bindings: net: qcom,ethqos: add binding doc for safety IRQ for sa8775p arm64: dts: qcom: sa8775p: enable safety IRQ net: stmmac: Add driver support for DWMAC5 common safety IRQ .../devicetree/bindings/net/qcom,ethqos.yaml | 9 +++++--- .../devicetree/bindings/net/snps,dwmac.yaml | 6 ++++-- arch/arm64/boot/dts/qcom/sa8775p.dtsi | 10 +++++---- drivers/net/ethernet/stmicro/stmmac/common.h | 1 + drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++ .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++ .../ethernet/stmicro/stmmac/stmmac_platform.c | 9 ++++++++ 7 files changed, 50 insertions(+), 9 deletions(-)