diff mbox series

[net-next,v3,6/8] net: phy: add calibration callbacks to phy_driver

Message ID 20231114105600.1012056-7-romain.gantois@bootlin.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: qualcomm: ipqess: introduce Qualcomm IPQESS driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
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: 1597 this patch: 1597
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/build_clang success Errors and warnings before: 1188 this patch: 1188
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 No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1634 this patch: 1634
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 40 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc fail Errors and warnings before: 0 this patch: 2
netdev/source_inline success Was 0 now: 0

Commit Message

Romain Gantois Nov. 14, 2023, 10:55 a.m. UTC
The IPQESS integrated Ethernet switch found in the IPQ4019 SoC requires
calibration of the PHY link when its ports are brought up. This calibration
procedure requires knowledge of precise timings and vendor-specific
registers on both the PHY and MAC side.

The existing PHY abstraction layer does not allow coordinating this kind of
calibration operation between MAC drivers and PHY drivers. As a
consequence, PHY-specific calibration information has to be included in
Ethernet drivers, since it has to schedule the entire calibration procedure
on it's own.

Add two callbacks that extend the PHY abstraction layer to allow MAC
drivers to start and stop PHY calibration runs in a PHY-model-independent
manner.

Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
 include/linux/phy.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Andrew Lunn Nov. 14, 2023, 7:14 p.m. UTC | #1
> +static inline
> +int phy_start_calibration(struct phy_device *phydev)
> +{
> +	if (!(phydev->drv &&
> +	      phydev->drv->calibration_start &&
> +	      phydev->drv->calibration_stop))
> +		return -EOPNOTSUPP;
> +
> +	return phydev->drv->calibration_start(phydev);
> +}
> +
> +static inline
> +int phy_stop_calibration(struct phy_device *phydev)
> +{
> +	if (!(phydev->drv &&
> +	      phydev->drv->calibration_stop))
> +		return -EOPNOTSUPP;
> +
> +	return phydev->drv->calibration_stop(phydev);
> +}
> +

What is the locking model?

     Andrew
Andrew Lunn Nov. 14, 2023, 7:20 p.m. UTC | #2
On Tue, Nov 14, 2023 at 11:55:56AM +0100, Romain Gantois wrote:
> The IPQESS integrated Ethernet switch found in the IPQ4019 SoC requires
> calibration of the PHY link when its ports are brought up. This calibration
> procedure requires knowledge of precise timings and vendor-specific
> registers on both the PHY and MAC side.
> 
> The existing PHY abstraction layer does not allow coordinating this kind of
> calibration operation between MAC drivers and PHY drivers. As a
> consequence, PHY-specific calibration information has to be included in
> Ethernet drivers, since it has to schedule the entire calibration procedure
> on it's own.
> 
> Add two callbacks that extend the PHY abstraction layer to allow MAC
> drivers to start and stop PHY calibration runs in a PHY-model-independent
> manner.

When adding new APIs, you need to add an example of both sides of
it. We can then decide if the API makes sense. I don't see a PHY
driver implementing this API.


    Andrew

---
pw-bot: cr
Romain Gantois Nov. 15, 2023, 3:31 p.m. UTC | #3
On Tue, 14 Nov 2023, Andrew Lunn wrote:

> > +static inline
> > +int phy_start_calibration(struct phy_device *phydev)
> > +{
> > +	if (!(phydev->drv &&
> > +	      phydev->drv->calibration_start &&
> > +	      phydev->drv->calibration_stop))
> > +		return -EOPNOTSUPP;
> > +
> > +	return phydev->drv->calibration_start(phydev);
> > +}
> > +
> > +static inline
> > +int phy_stop_calibration(struct phy_device *phydev)
> > +{
> > +	if (!(phydev->drv &&
> > +	      phydev->drv->calibration_stop))
> > +		return -EOPNOTSUPP;
> > +
> > +	return phydev->drv->calibration_stop(phydev);
> > +}
> > +
> 
> What is the locking model?
> 
>      Andrew
> 
This driver currently uses an atomic flag to make sure that the calibration 
doesn't run twice. It doesn't acquire any locks before calling 
phy_start_calibration(), which is a mistake.

I think a good locking model for this would be similar to the one used for 
phy_cable_test. The phy_start_calibration() and phy_stop_calibration() wrappers 
would acquire a lock on the PHY device and then test phydev->state, to check for 
an ongoing calibration. A new enum member such as PHY_CALIB could be defined for 
this purpose. The lock would be released by the phylib wrapper once the 
phy_driver callback returns.

The problem with this is that one calibration run can access multiple 
phy_device instances at the same time, e.g. if a switch is linked to a multiport 
PHY via a PSGMII link.

So acquiring a lock on a single phy device isn't enough. Ideally, these 
calls could somehow acquire one lock on all the hardware resources of a 
multiport PHY simultaneously. From what I've seen, there is no standard kernel 
interface that allows MAC drivers to know about link-sharing between phy 
devices. I'll have to do more research on this but if you know of an existing 
interface that I can use for this, please tell me.

Best,
Andrew Lunn Nov. 15, 2023, 4:12 p.m. UTC | #4
On Wed, Nov 15, 2023 at 04:31:07PM +0100, Romain Gantois wrote:
> On Tue, 14 Nov 2023, Andrew Lunn wrote:
> 
> > > +static inline
> > > +int phy_start_calibration(struct phy_device *phydev)
> > > +{
> > > +	if (!(phydev->drv &&
> > > +	      phydev->drv->calibration_start &&
> > > +	      phydev->drv->calibration_stop))
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	return phydev->drv->calibration_start(phydev);
> > > +}
> > > +
> > > +static inline
> > > +int phy_stop_calibration(struct phy_device *phydev)
> > > +{
> > > +	if (!(phydev->drv &&
> > > +	      phydev->drv->calibration_stop))
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	return phydev->drv->calibration_stop(phydev);
> > > +}
> > > +
> > 
> > What is the locking model?
> > 
> >      Andrew
> > 
> This driver currently uses an atomic flag to make sure that the calibration 
> doesn't run twice. It doesn't acquire any locks before calling 
> phy_start_calibration(), which is a mistake.
> 
> I think a good locking model for this would be similar to the one used for 
> phy_cable_test. The phy_start_calibration() and phy_stop_calibration() wrappers 
> would acquire a lock on the PHY device and then test phydev->state, to check for 
> an ongoing calibration. A new enum member such as PHY_CALIB could be defined for 
> this purpose. The lock would be released by the phylib wrapper once the 
> phy_driver callback returns.
> 
> The problem with this is that one calibration run can access multiple 
> phy_device instances at the same time, e.g. if a switch is linked to a multiport 
> PHY via a PSGMII link.
> 
> So acquiring a lock on a single phy device isn't enough. Ideally, these 
> calls could somehow acquire one lock on all the hardware resources of a 
> multiport PHY simultaneously. From what I've seen, there is no standard kernel 
> interface that allows MAC drivers to know about link-sharing between phy 
> devices. I'll have to do more research on this but if you know of an existing 
> interface that I can use for this, please tell me.

Lets get the switch parts merged first, then we can think about this
calibration problem. I need a better understanding of the requirements
before i can suggest something.

       Andrew
diff mbox series

Patch

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3cc52826f18e..b1092b2ecee3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1142,6 +1142,13 @@  struct phy_driver {
 	int (*led_hw_control_get)(struct phy_device *dev, u8 index,
 				  unsigned long *rules);
 
+	/* @calibration_start: Start calibrating the MAC-to-PHY link. */
+	int (*calibration_start)(struct phy_device *dev);
+
+	/* @calibration_start: Finalize MAC-to-PHY link calibration
+	 * and run tests. Returns 0 if the calibration tests are successful.
+	 */
+	int (*calibration_stop)(struct phy_device *dev);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
@@ -1770,6 +1777,27 @@  int phy_start_cable_test_tdr(struct phy_device *phydev,
 }
 #endif
 
+static inline
+int phy_start_calibration(struct phy_device *phydev)
+{
+	if (!(phydev->drv &&
+	      phydev->drv->calibration_start &&
+	      phydev->drv->calibration_stop))
+		return -EOPNOTSUPP;
+
+	return phydev->drv->calibration_start(phydev);
+}
+
+static inline
+int phy_stop_calibration(struct phy_device *phydev)
+{
+	if (!(phydev->drv &&
+	      phydev->drv->calibration_stop))
+		return -EOPNOTSUPP;
+
+	return phydev->drv->calibration_stop(phydev);
+}
+
 static inline void phy_device_reset(struct phy_device *phydev, int value)
 {
 	mdio_device_reset(&phydev->mdio, value);