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@xxxxxxxxxxxxxxx # 5.15+
> Signed-off-by: Radu Pirea (OSS) <radu-nicolae.pirea@xxxxxxxxxxx>
> ---
> 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
