Re: [PATCH net-next] net: macb: add support for 1000BASE-X autonegotiation to PCS

quoted

quoted

@@ -1025,13 +1034,15 @@ static int macb_mii_probe(struct net_device *dev)
  	struct macb *bp = netdev_priv(dev);
  	bp->phylink_sgmii_pcs.ops = &macb_phylink_pcs_ops;
+	bp->phylink_sgmii_pcs.poll = true;

Why is this needed for 1000base-x?

Without this, the hardware does not detect link state changes after
initialization. I am assuming that in the normal SGMII use-case with an
external onboard PHY etc., those state changes either don't have or are
signaled by monitoring the PHY, so that support for this wasn't needed. With
removable modules, fiber, etc., state changes are suddenly both possible and
otherwise unsignaled. It is possible -- likely, actually -- that there is
some better way to do this by properly monitoring an interrupt somewhere,
but I am working without documentation so am not sure how to implement that
and was trying to keep the patch short. Polling worked well enough as an
alternative.

This does not sound quiet correct. A PCS has two options.

1) As you say, it implements interrupts, and calls
phylink_pcs_change() on each interrupt to let phylink know something
happened.

2) You set poll true, and phylink will poll the PCS every second.

A PHY generally only reports on its media side changing state. I also
don't think phylink will poll the PCS if the PHY changes state.

I don't see anywhere macb calls phylink_pcs_change(), so polling does
seem correct.

Please break this patch up. You can add polling in a patch of its own,
with the commit message explaining why it is needed.

     Andrew