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
