.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: [email protected] # 5.15+
Signed-off-by: Radu Pirea (OSS) <[email protected]>
---
drivers/net/phy/nxp-c45-tja11xx.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
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)
--
2.34.1
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