| Message ID | 20230410124856.287753-1-radu-nicolae.pirea@oss.nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: phy: nxp-c45-tja11xx: fix the PTP interrupt enabling/disabling | expand |
On Mon, 10 Apr 2023 15:48:56 +0300 Radu Pirea (OSS) wrote: > - if (phydev->interrupts == PHY_INTERRUPT_ENABLED) > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { > + phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, PTP_IRQS, PTP_IRQS); Isn't the third argument supposed to be the address? Am I missing something or this patch was no tested properly? Also why ignore the return value? > return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, > VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT); > - else > + } else { > + phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, PTP_IRQS, PTP_IRQS); > return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, > VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT); > + }
On 13.04.2023 06:44, Jakub Kicinski wrote: > On Mon, 10 Apr 2023 15:48:56 +0300 Radu Pirea (OSS) wrote: >> - if (phydev->interrupts == PHY_INTERRUPT_ENABLED) >> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { >> + phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, PTP_IRQS, PTP_IRQS); > > Isn't the third argument supposed to be the address? > Am I missing something or this patch was no tested properly? Yes, you are right. Thank you for this catch. I discovered this fix based on a driver code review and it did not trigger any issues. I just wanted to be sure if the PTP irqs are left in an inconsistent state, they are disabled from the kill switch. I will send a v2. > > Also why ignore the return value? This register might not be present on every PHY, that's why the return value is ignored. Radu P. [...]
> > Also why ignore the return value? > This register might not be present on every PHY, that's why the return value > is ignored. Please document that, otherwise you might see people add code to check the return value. Or better, still, differentiate between the different versions, and only touch it when it does exist. Andtrew
diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c index 5813b07242ce..4d7f4cb05f89 100644 --- a/drivers/net/phy/nxp-c45-tja11xx.c +++ b/drivers/net/phy/nxp-c45-tja11xx.c @@ -63,6 +63,9 @@ #define VEND1_PORT_ABILITIES 0x8046 #define PTP_ABILITY BIT(3) +#define VEND1_PORT_FUNC_IRQ_EN 0x807A +#define PTP_IRQS BIT(3) + #define VEND1_PORT_INFRA_CONTROL 0xAC00 #define PORT_INFRA_CONTROL_EN BIT(14) @@ -890,12 +893,15 @@ static int nxp_c45_start_op(struct phy_device *phydev) static int nxp_c45_config_intr(struct phy_device *phydev) { - if (phydev->interrupts == PHY_INTERRUPT_ENABLED) + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { + phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, PTP_IRQS, PTP_IRQS); return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT); - else + } else { + phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, PTP_IRQS, PTP_IRQS); return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT); + } } static irqreturn_t nxp_c45_handle_interrupt(struct phy_device *phydev)
.config_intr() handles only the link event interrupt and should also disable/enable the PTP interrupt. Even if the PTP irqs bit is toggled unconditionally, it is safe. This interrupt acts as a global switch for all PTP irqs. By default, this bit is set. 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 | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)