RE: [PATCH net-next] net: phy: micrel: Add callback for restoring context

[Biju Das]

Hi Paolo Abeni,

Thanks for the feedback.

> -----Original Message-----
> From: Paolo Abeni <pabeni@xxxxxxxxxx>
> Sent: 10 July 2025 14:38
> Subject: Re: [PATCH net-next] net: phy: micrel: Add callback for restoring context
> 
> On 7/7/25 4:29 PM, Biju Das wrote:
> > @@ -374,6 +376,7 @@ static struct kszphy_hw_stat kszphy_hw_stats[] = {
> > };
> >
> >  struct kszphy_type {
> > +	void (*resume)(struct phy_device *phydev);
> >  	u32 led_mode_reg;
> >  	u16 interrupt_level_mask;
> >  	u16 cable_diag_reg;
> 
> Why adding another callback? I think you could avoid it using a ksz9131-specific phy_driver->resume.

It is being called from open() as well see [1]

[1]
[    1.934177]  kszphy_resume+0x18/0xc4
[    1.934193]  phy_resume+0x3c/0x74
[    1.934207]  phy_attach_direct+0x1bc/0x398
[    1.934220]  phylink_fwnode_phy_connect+0xb0/0x130
[    1.934237]  __stmmac_open+0xe4/0x50c
[    1.934251]  stmmac_open+0x44/0xd4

> 
> > @@ -444,6 +447,7 @@ struct kszphy_priv {
> >  	bool rmii_ref_clk_sel;
> >  	bool rmii_ref_clk_sel_val;
> >  	bool clk_enable;
> > +	bool is_suspended;
> >  	u64 stats[ARRAY_SIZE(kszphy_hw_stats)];
> >  	struct kszphy_phy_stats phy_stats;
> >  };
> > @@ -491,6 +495,7 @@ static const struct kszphy_type ksz9021_type = {
> > };
> >
> >  static const struct kszphy_type ksz9131_type = {
> > +	.resume = ksz9131_restore_rgmii_delay,
> >  	.interrupt_level_mask	= BIT(14),
> >  	.disable_dll_tx_bit	= BIT(12),
> >  	.disable_dll_rx_bit	= BIT(12),
> > @@ -1387,6 +1392,12 @@ static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
> >  			      txcdll_val);
> >  }
> >
> > +static void ksz9131_restore_rgmii_delay(struct phy_device *phydev) {
> > +	if (phy_interface_is_rgmii(phydev))
> > +		ksz9131_config_rgmii_delay(phydev);
> > +}
> > +
> >  /* Silicon Errata DS80000693B
> >   *
> >   * When LEDs are configured in Individual Mode, LED1 is ON in a
> > no-link @@ -2345,6 +2356,11 @@ static int
> > kszphy_generic_suspend(struct phy_device *phydev)
> >
> >  static int kszphy_suspend(struct phy_device *phydev)  {
> > +	struct kszphy_priv *priv = phydev->priv;
> > +
> > +	if (priv)
> > +		priv->is_suspended = true;
> 
> Under which circumstances `priv` could be NULL? AFAICS it should always not NULL after probe.

On the kszphy_config_init(), there is a check for NULL. That is the reason for adding it here.

Yor right, priv cannot be NULL after probe.


> 
> > +
> >  	/* Disable PHY Interrupts */
> >  	if (phy_interrupt_is_valid(phydev)) {
> >  		phydev->interrupts = PHY_INTERRUPT_DISABLED; @@ -2381,8 +2397,17 @@
> > static void kszphy_parse_led_mode(struct phy_device *phydev)
> >
> >  static int kszphy_resume(struct phy_device *phydev)  {
> > +	struct kszphy_priv *priv = phydev->priv;
> >  	int ret;
> >
> > +	if (priv && priv->is_suspended) {
> 
> I think you can use phydev->suspended instead of adding another flag.

Yes, I can use phydev->suspended as phylink_prepare_resume() calls phy_resume()
with phydev->suspended set.
> 
> Also, can resume be invoked without the phydev being suspended?

Yes, phy_attach_direct() during open() invokes resume.

Cheers,
Biju