Hi,
On Wed, Nov 17, 2021 at 10:18:56AM +0100, Horatiu Vultur wrote:
quoted hunk ↗ jump to hunk
+static void lan966x_phylink_mac_link_state(struct phylink_config *config,
+ struct phylink_link_state *state)
+{
+}
+
+static void lan966x_phylink_mac_aneg_restart(struct phylink_config *config)
+{
+}
Since you always attach a PCS, it is not necessary to provide stubs
for these functions.
quoted hunk ↗ jump to hunk
+static int lan966x_pcs_config(struct phylink_pcs *pcs,
+ unsigned int mode,
+ phy_interface_t interface,
+ const unsigned long *advertising,
+ bool permit_pause_to_mac)
+{
+ struct lan966x_port *port = lan966x_pcs_to_port(pcs);
+ struct lan966x_port_config config;
+ int ret;
+
+ memset(&config, 0, sizeof(config));
+
+ config = port->config;
+ config.portmode = interface;
+ config.inband = phylink_autoneg_inband(mode);
+ config.autoneg = phylink_test(advertising, Autoneg);
+ if (phylink_test(advertising, Pause))
+ config.pause_adv |= ADVERTISE_1000XPAUSE;
+ if (phylink_test(advertising, Asym_Pause))
+ config.pause_adv |= ADVERTISE_1000XPSE_ASYM;
There are patches around that add
phylink_mii_c22_pcs_encode_advertisement() which will create the C22
advertisement for you. It would be good to get that patch merged so
people can use it. That should also eliminate lan966x_get_aneg_word(),
although I notice you need to set ADVERTISE_LPACK as well (can you
check that please? Hardware should be managing that bit as it should
only be set once the hardware has received the link partner's
advertisement.)
quoted hunk ↗ jump to hunk
+static void decode_cl37_word(u16 lp_abil, uint16_t ld_abil,
+ struct lan966x_port_status *status)
+{
+ status->link = !(lp_abil & ADVERTISE_RFAULT) && status->link;
+ status->an_complete = true;
+ status->duplex = (ADVERTISE_1000XFULL & lp_abil) ?
+ DUPLEX_FULL : DUPLEX_UNKNOWN;
+
+ if ((ld_abil & ADVERTISE_1000XPAUSE) &&
+ (lp_abil & ADVERTISE_1000XPAUSE)) {
+ status->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
+ } else if ((ld_abil & ADVERTISE_1000XPSE_ASYM) &&
+ (lp_abil & ADVERTISE_1000XPSE_ASYM)) {
+ status->pause |= (lp_abil & ADVERTISE_1000XPAUSE) ?
+ MLO_PAUSE_TX : 0;
+ status->pause |= (ld_abil & ADVERTISE_1000XPAUSE) ?
+ MLO_PAUSE_RX : 0;
+ } else {
+ status->pause = MLO_PAUSE_NONE;
+ }
+}
We already have phylink_decode_c37_word() which will decode this for
you, although it would need to be exported. Please re-use this code.
quoted hunk ↗ jump to hunk
+
+static void decode_sgmii_word(u16 lp_abil, struct lan966x_port_status *status)
+{
+ status->an_complete = true;
+ if (!(lp_abil & LPA_SGMII_LINK)) {
+ status->link = false;
+ return;
+ }
+
+ switch (lp_abil & LPA_SGMII_SPD_MASK) {
+ case LPA_SGMII_10:
+ status->speed = SPEED_10;
+ break;
+ case LPA_SGMII_100:
+ status->speed = SPEED_100;
+ break;
+ case LPA_SGMII_1000:
+ status->speed = SPEED_1000;
+ break;
+ default:
+ status->link = false;
+ return;
+ }
+ if (lp_abil & LPA_SGMII_FULL_DUPLEX)
+ status->duplex = DUPLEX_FULL;
+ else
+ status->duplex = DUPLEX_HALF;
+}
The above mentioned function will also handle SGMII as well.
quoted hunk ↗ jump to hunk
+int lan966x_port_pcs_set(struct lan966x_port *port,
+ struct lan966x_port_config *config)
+{
+ struct lan966x *lan966x = port->lan966x;
+ bool sgmii = false, inband_aneg = false;
+ int err;
+
+ lan966x_port_link_down(port);
+
+ if (config->inband) {
+ if (config->portmode == PHY_INTERFACE_MODE_SGMII ||
+ config->portmode == PHY_INTERFACE_MODE_QSGMII)
+ inband_aneg = true; /* Cisco-SGMII in-band-aneg */
+ else if (config->portmode == PHY_INTERFACE_MODE_1000BASEX &&
+ config->autoneg)
+ inband_aneg = true; /* Clause-37 in-band-aneg */
+
+ if (config->speed > 0) {
+ err = phy_set_speed(port->serdes, config->speed);
+ if (err)
+ return err;
+ }
+
+ } else {
+ sgmii = true; /* Phy is connnected to the MAC */
This looks weird. SGMII can be in-band as well (and technically is
in-band in its as-specified form.)
quoted hunk ↗ jump to hunk
+ }
+
+ /* Choose SGMII or 1000BaseX/2500BaseX PCS mode */
+ lan_rmw(DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA_SET(sgmii),
+ DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA,
+ lan966x, DEV_PCS1G_MODE_CFG(port->chip_port));
With the code as you have it, what this means is if we specify
SGMII + in-band, we end up configuring the port to be in 1000BaseX
mode, so it's incapable of 10 and 100M speeds. This seems incorrect.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!