| Message ID | 20230616135323.98215-2-radu-nicolae.pirea@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add TJA1120 support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 31 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Jun 16, 2023 at 04:53:10PM +0300, Radu Pirea (NXP OSS) wrote: > .config_intr() handles only the link event interrupt and should > disable/enable the PTP interrupt also. > > It's safe to disable/enable the PTP irq even if the egress ts irq > is disabled. This interrupt, the PTP one, acts as a global switch for all > PTP irqs. > > Fixes: 514def5dd339 ("phy: nxp-c45-tja11xx: add timestamping support") > CC: stable@vger.kernel.org # 5.15+ > Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com> Please don't mix fixes and development work in one patchset. Please post this to applying to net, not net-next. > static int nxp_c45_config_intr(struct phy_device *phydev) > { > - if (phydev->interrupts == PHY_INTERRUPT_ENABLED) > + /* The return value is ignored on purpose. It might be < 0. > + * 0x807A register is not present on SJA1110 PHYs. > + */ > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { > + phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, > + VEND1_PORT_FUNC_IRQ_EN, PTP_IRQS); > return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, > VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT); phy_set_bits_mmd() will not return an error if the register does not exist. There is no such indication for MDIO. This is going to do a read/modify/write. That read might get 0xffff, or random junk. And then the write back will be successful. The only time phy_read()/phy_write return error is when there is a problem within the bus master, like its clock gets turned off and the transfer times out. So it is good to document you are accessing a register which might not exist, but there is no need to ignore the return code. Andrew
On 16.06.2023 23:36, Andrew Lunn wrote: > On Fri, Jun 16, 2023 at 04:53:10PM +0300, Radu Pirea (NXP OSS) wrote: >> .config_intr() handles only the link event interrupt and should >> disable/enable the PTP interrupt also. >> >> It's safe to disable/enable the PTP irq even if the egress ts irq >> is disabled. This interrupt, the PTP one, acts as a global switch for all >> PTP irqs. >> >> Fixes: 514def5dd339 ("phy: nxp-c45-tja11xx: add timestamping support") >> CC: stable@vger.kernel.org # 5.15+ >> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com> > > Please don't mix fixes and development work in one patchset. Please > post this to applying to net, not net-next. Ok. I will send it to net and apply your suggestions. > >> static int nxp_c45_config_intr(struct phy_device *phydev) >> { >> - if (phydev->interrupts == PHY_INTERRUPT_ENABLED) >> + /* The return value is ignored on purpose. It might be < 0. >> + * 0x807A register is not present on SJA1110 PHYs. >> + */ >> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { >> + phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, >> + VEND1_PORT_FUNC_IRQ_EN, PTP_IRQS); >> return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, >> VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT); > > phy_set_bits_mmd() will not return an error if the register does not > exist. There is no such indication for MDIO. This is going to do a > read/modify/write. That read might get 0xffff, or random junk. And > then the write back will be successful. The only time > phy_read()/phy_write return error is when there is a problem within > the bus master, like its clock gets turned off and the transfer times > out. > > So it is good to document you are accessing a register which might not > exist, but there is no need to ignore the return code. > > Andrew
diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c index 029875a59ff8..7b213c3f4536 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,20 @@ 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) + /* The return value is ignored on purpose. It might be < 0. + * 0x807A register is not present on SJA1110 PHYs. + */ + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { + phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, + VEND1_PORT_FUNC_IRQ_EN, 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, + VEND1_PORT_FUNC_IRQ_EN, 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 disable/enable the PTP interrupt also. It's safe to disable/enable the PTP irq even if the egress ts irq is disabled. This interrupt, the PTP one, acts as a global switch for all PTP irqs. Fixes: 514def5dd339 ("phy: nxp-c45-tja11xx: add timestamping support") CC: stable@vger.kernel.org # 5.15+ Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com> --- drivers/net/phy/nxp-c45-tja11xx.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)