Received-SPF: pass (google.com: domain of linux-kernel+bounces-32686-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 22 Jan 2024 13:00:37 +0300
From: Serge Semin <fancer.lancer@gmail.com>
To: Denis Kirjanov <dkirjanov@suse.de>, 
	Suraj Jaiswal <quic_jsuraj@quicinc.com>
Cc: Suraj Jaiswal <quic_jsuraj@quicinc.com>, Vinod Koul <vkoul@kernel.org>, 
	Bhupesh Sharma <bhupesh.sharma@linaro.org>, Andy Gross <agross@kernel.org>, 
	Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, 
	"David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, 
	Jakub Kicinski <kuba@kernel.org>, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Alexandre Torgue <alexandre.torgue@foss.st.com>, Jose Abreu <joabreu@synopsys.com>, 
	Maxime Coquelin <mcoquelin.stm32@gmail.com>, netdev@vger.kernel.org, linux-arm-msm@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-stm32@st-md-mailman.stormreply.com, Prasad Sodagudi <psodagud@quicinc.com>, 
	Andrew Halaney <ahalaney@redhat.com>, Rob Herring <robh@kernel.org>, kernel@quicinc.com
Subject: Re: [PATCH net-next v9 3/3] net: stmmac: Add driver support for
 DWMAC5 common safety IRQ
Message-ID: <giimpexp3qk3byb725r3ot3aund2bwmi45yrctkydatm73d5af@e36xmjf2ehvu>
References: <20240110111649.2256450-1-quic_jsuraj@quicinc.com>
 <20240110111649.2256450-4-quic_jsuraj@quicinc.com>
 <633ff61d-f73d-4221-a2fd-79f913880761@suse.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <633ff61d-f73d-4221-a2fd-79f913880761@suse.de>

On Wed, Jan 10, 2024 at 03:07:30PM +0300, Denis Kirjanov wrote:
> 
> 
> On 1/10/24 14:16, 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.
> 
> As I see .safety_feat_irq_status available not just in dwmac5 but 
> in dwxgmac2_core and that means that the subject line is not just about dwmac5

Right. Suraj, could you please fix the subject to be describing the
actual change? The commit message body more-or-less describes it
correctly.

> 
> > 
> > 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 | 41 ++++++++++++++++++-
> >  .../ethernet/stmicro/stmmac/stmmac_platform.c |  8 ++++
> >  4 files changed, 51 insertions(+), 2 deletions(-)
> > 
> > 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..e0192a282121 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;
> > @@ -3661,6 +3665,23 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
> >  		}
> >  	}
> >  
> > +	/* Request the common Safety Feature Correctible/Uncorrectible
> > +	 * Error line in case of another line is used
> > +	 */
> > +	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 sfty MSI %d (error: %d)\n",
> > +				   __func__, priv->sfty_irq, ret);
> > +			irq_err = REQ_IRQ_ERR_SFTY;
> > +			goto irq_error;
> > +		}
> > +	}
> > +
> >  	/* Request the Safety Feature Correctible Error line in
> >  	 * case of another line is used
> >  	 */
> > @@ -3798,6 +3819,21 @@ static int stmmac_request_irq_single(struct net_device *dev)
> >  		}
> >  	}
> >  
> > +	/* Request the common Safety Feature Correctible/Uncorrectible
> > +	 * Error line in case of another line is used
> > +	 */
> > +	if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) {
> > +		ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt,
> > +				  IRQF_SHARED, dev->name, dev);
> > +		if (unlikely(ret < 0)) {
> > +			netdev_err(priv->dev,
> > +				   "%s: ERROR: allocating the sfty IRQ %d (%d)\n",
> > +				   __func__, priv->sfty_irq, ret);
> > +			irq_err = REQ_IRQ_ERR_SFTY;
> > +			goto irq_error;
> > +		}
> > +	}
> > +
> >  	return 0;
> >  
> >  irq_error:
> > @@ -6022,8 +6058,8 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
> >  	if (test_bit(STMMAC_DOWN, &priv->state))
> >  		return IRQ_HANDLED;
> >  

> > -	/* Check if a fatal error happened */
> > -	if (stmmac_safety_feat_interrupt(priv))
> > +	/* Check ASP error if it isn't delivered via an individual IRQ */
> > +	if (priv->sfty_irq <= 0 && stmmac_safety_feat_interrupt(priv))

Well, I guess this is the best we can do with no IRQs handling part
refactoring.

> >  		return IRQ_HANDLED;
> >  
> >  	/* To handle Common interrupts */
> > @@ -7462,6 +7498,7 @@ 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;
> >  	for (i = 0; i < MTL_MAX_RX_QUEUES; i++)
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > index 70eadc83ca68..ab250161fd79 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > @@ -743,6 +743,14 @@ 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");
> > +	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");

s/IRQ safety IRQ/IRQ sfty
* Although I would have also converted this to just dev_dbg() since
* the IRQ line is optional and is present on a single platform you
* have.


With the subject and the log-message fixed feel free to add:

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y) 

> > +	}
> > +
> >  	stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0);
> >  
> >  	return PTR_ERR_OR_ZERO(stmmac_res->addr);
>