netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] net: phy: nxp-c45-tja11xx: disable port and global interrupts
@ 2023-04-06  9:55 Radu Pirea (OSS)
  2023-04-07 14:14 ` Andrew Lunn
  0 siblings, 1 reply; 3+ messages in thread
From: Radu Pirea (OSS) @ 2023-04-06  9:55 UTC (permalink / raw)
  To: andrew, hkallweit1, linux, davem, edumazet, kuba, pabeni
  Cc: netdev, linux-kernel, Radu Pirea (OSS), stable

Disabling only the link event irq is not enough to disable the
interrupts. PTP will still be able to generate interrupts.

The interrupts are organised in a tree on the C45 TJA11XX PHYs. To
completely disable the interrupts, they are disable from the top of the
interrupt tree.

Fixes: 514def5dd339 ("phy: nxp-c45-tja11xx: add timestamping support")
CC: stable@vger.kernel.org # 5.15+
Signed-off-by: Radu Pirea (OSS) <radu-nicolae.pirea@oss.nxp.com>
---
 drivers/net/phy/nxp-c45-tja11xx.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
index 029875a59ff8..ce718a5865a4 100644
--- a/drivers/net/phy/nxp-c45-tja11xx.c
+++ b/drivers/net/phy/nxp-c45-tja11xx.c
@@ -31,6 +31,10 @@
 #define DEVICE_CONTROL_CONFIG_GLOBAL_EN	BIT(14)
 #define DEVICE_CONTROL_CONFIG_ALL_EN	BIT(13)
 
+#define VEND1_PORT_IRQ_ENABLES		0x0072
+#define PORT1_IRQ			BIT(1)
+#define GLOBAL_IRQ			BIT(0)
+
 #define VEND1_PHY_IRQ_ACK		0x80A0
 #define VEND1_PHY_IRQ_EN		0x80A1
 #define VEND1_PHY_IRQ_STATUS		0x80A2
@@ -235,7 +239,7 @@ struct nxp_c45_phy_stats {
 	u16		mask;
 };
 
-static bool nxp_c45_poll_txts(struct phy_device *phydev)
+static bool nxp_c45_poll(struct phy_device *phydev)
 {
 	return phydev->irq <= 0;
 }
@@ -448,7 +452,7 @@ static void nxp_c45_process_txts(struct nxp_c45_phy *priv,
 static long nxp_c45_do_aux_work(struct ptp_clock_info *ptp)
 {
 	struct nxp_c45_phy *priv = container_of(ptp, struct nxp_c45_phy, caps);
-	bool poll_txts = nxp_c45_poll_txts(priv->phydev);
+	bool poll_txts = nxp_c45_poll(priv->phydev);
 	struct skb_shared_hwtstamps *shhwtstamps_rx;
 	struct ptp_clock_event event;
 	struct nxp_c45_hwts hwts;
@@ -699,7 +703,7 @@ static void nxp_c45_txtstamp(struct mii_timestamper *mii_ts,
 		NXP_C45_SKB_CB(skb)->header = ptp_parse_header(skb, type);
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 		skb_queue_tail(&priv->tx_queue, skb);
-		if (nxp_c45_poll_txts(priv->phydev))
+		if (nxp_c45_poll(priv->phydev))
 			ptp_schedule_worker(priv->ptp_clock, 0);
 		break;
 	case HWTSTAMP_TX_OFF:
@@ -772,7 +776,7 @@ static int nxp_c45_hwtstamp(struct mii_timestamper *mii_ts,
 				 PORT_PTP_CONTROL_BYPASS);
 	}
 
-	if (nxp_c45_poll_txts(priv->phydev))
+	if (nxp_c45_poll(priv->phydev))
 		goto nxp_c45_no_ptp_irq;
 
 	if (priv->hwts_tx)
@@ -892,10 +896,12 @@ static int nxp_c45_config_intr(struct phy_device *phydev)
 {
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
 		return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
-					VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
+					VEND1_PORT_IRQ_ENABLES,
+					PORT1_IRQ | GLOBAL_IRQ);
 	else
 		return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
-					  VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
+					  VEND1_PORT_IRQ_ENABLES,
+					  PORT1_IRQ | GLOBAL_IRQ);
 }
 
 static irqreturn_t nxp_c45_handle_interrupt(struct phy_device *phydev)
@@ -1290,6 +1296,10 @@ static int nxp_c45_config_init(struct phy_device *phydev)
 	phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PORT_FUNC_ENABLES,
 			 PTP_ENABLE);
 
+	if (!nxp_c45_poll(phydev))
+		phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
+				 VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
+
 	return nxp_c45_start_op(phydev);
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH net] net: phy: nxp-c45-tja11xx: disable port and global interrupts
  2023-04-06  9:55 [PATCH net] net: phy: nxp-c45-tja11xx: disable port and global interrupts Radu Pirea (OSS)
@ 2023-04-07 14:14 ` Andrew Lunn
  2023-04-07 15:24   ` Radu Pirea (OSS)
  0 siblings, 1 reply; 3+ messages in thread
From: Andrew Lunn @ 2023-04-07 14:14 UTC (permalink / raw)
  To: Radu Pirea (OSS)
  Cc: hkallweit1, linux, davem, edumazet, kuba, pabeni, netdev,
	linux-kernel, stable

On Thu, Apr 06, 2023 at 12:55:46PM +0300, Radu Pirea (OSS) wrote:
> Disabling only the link event irq is not enough to disable the
> interrupts. PTP will still be able to generate interrupts.
> 
> The interrupts are organised in a tree on the C45 TJA11XX PHYs. To
> completely disable the interrupts, they are disable from the top of the
> interrupt tree.
> 
> Fixes: 514def5dd339 ("phy: nxp-c45-tja11xx: add timestamping support")
> CC: stable@vger.kernel.org # 5.15+
> Signed-off-by: Radu Pirea (OSS) <radu-nicolae.pirea@oss.nxp.com>
> ---
>  drivers/net/phy/nxp-c45-tja11xx.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
> index 029875a59ff8..ce718a5865a4 100644
> --- a/drivers/net/phy/nxp-c45-tja11xx.c
> +++ b/drivers/net/phy/nxp-c45-tja11xx.c
> @@ -31,6 +31,10 @@
>  #define DEVICE_CONTROL_CONFIG_GLOBAL_EN	BIT(14)
>  #define DEVICE_CONTROL_CONFIG_ALL_EN	BIT(13)
>  
> +#define VEND1_PORT_IRQ_ENABLES		0x0072
> +#define PORT1_IRQ			BIT(1)
> +#define GLOBAL_IRQ			BIT(0)

I find the PORT1 confusing there, it suggests there is a port0? What
is port0? There is no other reference to numbered ports in the driver.

> -static bool nxp_c45_poll_txts(struct phy_device *phydev)
> +static bool nxp_c45_poll(struct phy_device *phydev)
>  {
>  	return phydev->irq <= 0;
>  }

Maybe as a new patch, but phy_interrupt_is_valid() can be used here.

Maybe also extend the commit message to include a comment that
functions names are changed to reflect that all interrupts are now
disabled, not just _txts interrupts. Otherwise this rename might be
considered an unrelated change.

> @@ -448,7 +452,7 @@ static void nxp_c45_process_txts(struct nxp_c45_phy *priv,
>  static long nxp_c45_do_aux_work(struct ptp_clock_info *ptp)
>  {
>  	struct nxp_c45_phy *priv = container_of(ptp, struct nxp_c45_phy, caps);
> -	bool poll_txts = nxp_c45_poll_txts(priv->phydev);
> +	bool poll_txts = nxp_c45_poll(priv->phydev);
>  	struct skb_shared_hwtstamps *shhwtstamps_rx;
>  	struct ptp_clock_event event;
>  	struct nxp_c45_hwts hwts;
> @@ -699,7 +703,7 @@ static void nxp_c45_txtstamp(struct mii_timestamper *mii_ts,
>  		NXP_C45_SKB_CB(skb)->header = ptp_parse_header(skb, type);
>  		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>  		skb_queue_tail(&priv->tx_queue, skb);
> -		if (nxp_c45_poll_txts(priv->phydev))
> +		if (nxp_c45_poll(priv->phydev))
>  			ptp_schedule_worker(priv->ptp_clock, 0);
>  		break;
>  	case HWTSTAMP_TX_OFF:
> @@ -772,7 +776,7 @@ static int nxp_c45_hwtstamp(struct mii_timestamper *mii_ts,
>  				 PORT_PTP_CONTROL_BYPASS);
>  	}
>  
> -	if (nxp_c45_poll_txts(priv->phydev))
> +	if (nxp_c45_poll(priv->phydev))
>  		goto nxp_c45_no_ptp_irq;
>  
>  	if (priv->hwts_tx)
> @@ -892,10 +896,12 @@ static int nxp_c45_config_intr(struct phy_device *phydev)
>  {
>  	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
>  		return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> -					VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
> +					VEND1_PORT_IRQ_ENABLES,
> +					PORT1_IRQ | GLOBAL_IRQ);
>  	else
>  		return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> -					  VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
> +					  VEND1_PORT_IRQ_ENABLES,
> +					  PORT1_IRQ | GLOBAL_IRQ);
>  }
>  
>  static irqreturn_t nxp_c45_handle_interrupt(struct phy_device *phydev)
> @@ -1290,6 +1296,10 @@ static int nxp_c45_config_init(struct phy_device *phydev)
>  	phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PORT_FUNC_ENABLES,
>  			 PTP_ENABLE);
>  
> +	if (!nxp_c45_poll(phydev))
> +		phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> +				 VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
> +

It seems odd to be touching interrupt configuration outside of
nxp_c45_config_intr(). Is there a reason this cannot be part of
phydev->interrupts == PHY_INTERRUPT_ENABLED ?

	Andrew

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH net] net: phy: nxp-c45-tja11xx: disable port and global interrupts
  2023-04-07 14:14 ` Andrew Lunn
@ 2023-04-07 15:24   ` Radu Pirea (OSS)
  0 siblings, 0 replies; 3+ messages in thread
From: Radu Pirea (OSS) @ 2023-04-07 15:24 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: hkallweit1, linux, davem, edumazet, kuba, pabeni, netdev,
	linux-kernel, stable



On 07.04.2023 17:14, Andrew Lunn wrote:
> On Thu, Apr 06, 2023 at 12:55:46PM +0300, Radu Pirea (OSS) wrote:
>> Disabling only the link event irq is not enough to disable the
>> interrupts. PTP will still be able to generate interrupts.
>>
>> The interrupts are organised in a tree on the C45 TJA11XX PHYs. To
>> completely disable the interrupts, they are disable from the top of the
>> interrupt tree.
>>
>> Fixes: 514def5dd339 ("phy: nxp-c45-tja11xx: add timestamping support")
>> CC: stable@vger.kernel.org # 5.15+
>> Signed-off-by: Radu Pirea (OSS) <radu-nicolae.pirea@oss.nxp.com>
>> ---
>>   drivers/net/phy/nxp-c45-tja11xx.c | 22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
>> index 029875a59ff8..ce718a5865a4 100644
>> --- a/drivers/net/phy/nxp-c45-tja11xx.c
>> +++ b/drivers/net/phy/nxp-c45-tja11xx.c
>> @@ -31,6 +31,10 @@
>>   #define DEVICE_CONTROL_CONFIG_GLOBAL_EN	BIT(14)
>>   #define DEVICE_CONTROL_CONFIG_ALL_EN	BIT(13)
>>   
>> +#define VEND1_PORT_IRQ_ENABLES		0x0072
>> +#define PORT1_IRQ			BIT(1)
>> +#define GLOBAL_IRQ			BIT(0)
> 
> I find the PORT1 confusing there, it suggests there is a port0? What
> is port0? There is no other reference to numbered ports in the driver.
Sometimes HW engineers starts to count from 1 :)
TJA1103 have only one port, but the things becomes complicated if we 
talk about SJA1110 phys. From the SJA1110 user manual looks like the 
VEND1_PORT_IRQ_ENABLES register is shared between the phys. I need to 
clarify this.

Maybe is not a good idea to cut the interrupts from the top of the 
interrupt tree.
I will send another patch where I will disable the PTP and link event 
interrupts from nxp_c45_config_intr callback.
> 
>> -static bool nxp_c45_poll_txts(struct phy_device *phydev)
>> +static bool nxp_c45_poll(struct phy_device *phydev)
>>   {
>>   	return phydev->irq <= 0;
>>   }
> 
> Maybe as a new patch, but phy_interrupt_is_valid() can be used here.
> 
> Maybe also extend the commit message to include a comment that
> functions names are changed to reflect that all interrupts are now
> disabled, not just _txts interrupts. Otherwise this rename might be
> considered an unrelated change.

> 
>> @@ -448,7 +452,7 @@ static void nxp_c45_process_txts(struct nxp_c45_phy *priv,
>>   static long nxp_c45_do_aux_work(struct ptp_clock_info *ptp)
>>   {
>>   	struct nxp_c45_phy *priv = container_of(ptp, struct nxp_c45_phy, caps);
>> -	bool poll_txts = nxp_c45_poll_txts(priv->phydev);
>> +	bool poll_txts = nxp_c45_poll(priv->phydev);
>>   	struct skb_shared_hwtstamps *shhwtstamps_rx;
>>   	struct ptp_clock_event event;
>>   	struct nxp_c45_hwts hwts;
>> @@ -699,7 +703,7 @@ static void nxp_c45_txtstamp(struct mii_timestamper *mii_ts,
>>   		NXP_C45_SKB_CB(skb)->header = ptp_parse_header(skb, type);
>>   		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>>   		skb_queue_tail(&priv->tx_queue, skb);
>> -		if (nxp_c45_poll_txts(priv->phydev))
>> +		if (nxp_c45_poll(priv->phydev))
>>   			ptp_schedule_worker(priv->ptp_clock, 0);
>>   		break;
>>   	case HWTSTAMP_TX_OFF:
>> @@ -772,7 +776,7 @@ static int nxp_c45_hwtstamp(struct mii_timestamper *mii_ts,
>>   				 PORT_PTP_CONTROL_BYPASS);
>>   	}
>>   
>> -	if (nxp_c45_poll_txts(priv->phydev))
>> +	if (nxp_c45_poll(priv->phydev))
>>   		goto nxp_c45_no_ptp_irq;
>>   
>>   	if (priv->hwts_tx)
>> @@ -892,10 +896,12 @@ static int nxp_c45_config_intr(struct phy_device *phydev)
>>   {
>>   	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
>>   		return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
>> -					VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
>> +					VEND1_PORT_IRQ_ENABLES,
>> +					PORT1_IRQ | GLOBAL_IRQ);
>>   	else
>>   		return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
>> -					  VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
>> +					  VEND1_PORT_IRQ_ENABLES,
>> +					  PORT1_IRQ | GLOBAL_IRQ);
>>   }
>>   
>>   static irqreturn_t nxp_c45_handle_interrupt(struct phy_device *phydev)
>> @@ -1290,6 +1296,10 @@ static int nxp_c45_config_init(struct phy_device *phydev)
>>   	phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PORT_FUNC_ENABLES,
>>   			 PTP_ENABLE);
>>   
>> +	if (!nxp_c45_poll(phydev))
>> +		phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
>> +				 VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
>> +
> 
> It seems odd to be touching interrupt configuration outside of
> nxp_c45_config_intr(). Is there a reason this cannot be part of
> phydev->interrupts == PHY_INTERRUPT_ENABLED ?
Well, these C45 TJA PHYs have the interrupts organized in a tree.
The idea in this patch was to enable any interrupt(external trigger, 
PTP, link event, etc) from anywhere, but nxp_c45_config_intr to be able 
to disable/enable them in one register write.
> 
> 	Andrew

Radu P.

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-04-07 15:24 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-06  9:55 [PATCH net] net: phy: nxp-c45-tja11xx: disable port and global interrupts Radu Pirea (OSS)
2023-04-07 14:14 ` Andrew Lunn
2023-04-07 15:24   ` Radu Pirea (OSS)

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 NNTP newsgroup(s).