mbox series

[net-next,RFC,0/4] net: dsa: Add Airoha AN8855 support

Message ID 20241021130209.15660-1-ansuelsmth@gmail.com (mailing list archive)
Headers show
Series net: dsa: Add Airoha AN8855 support | expand

Message

Christian Marangi Oct. 21, 2024, 1:01 p.m. UTC 
This small series add the initial support for the Airoha AN8855 Switch.

It's a 5 port Gigabit Switch with SGMII/HSGMII upstream port.

This is starting to get in the wild and there are already some router
having this switch chip.

It's conceptually similar to mediatek switch but register and bits
are different. And there is that massive Hell that is the PCS
configuration.
Saddly for that part we have absolutely NO documentation currently.

There is this special thing where PHY needs to be calibrated with values
from the switch efuse. (the thing have a whole cpu timer and MCU)

Some cleanup API are used and one extra patch for mdio_mutex_nested is
introduced. As suggested some time ago, the use of such API is limited
to scoped variants and not the guard ones.

Posting as RFC as I expect in later version to add additional feature
but this is already working and upstream-ready. So this is really to
have a review of the very basic features and if I missed anything in
recent implementation of DSA.

Christian Marangi (4):
  net: mdio: implement mdio_mutex_nested guard() variant
  dt-bindings: net: dsa: Add Airoha AN8855 Gigabit Switch documentation
  net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver
  net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY

 .../bindings/net/dsa/airoha,an8855.yaml       |  146 ++
 MAINTAINERS                                   |   11 +
 drivers/net/dsa/Kconfig                       |    9 +
 drivers/net/dsa/Makefile                      |    1 +
 drivers/net/dsa/an8855.c                      | 2008 +++++++++++++++++
 drivers/net/dsa/an8855.h                      |  492 ++++
 drivers/net/phy/Kconfig                       |    5 +
 drivers/net/phy/Makefile                      |    1 +
 drivers/net/phy/air_an8855.c                  |  187 ++
 include/linux/mdio.h                          |    4 +
 10 files changed, 2864 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/airoha,an8855.yaml
 create mode 100644 drivers/net/dsa/an8855.c
 create mode 100644 drivers/net/dsa/an8855.h
 create mode 100644 drivers/net/phy/air_an8855.c

Comments

Vladimir Oltean Oct. 21, 2024, 1:36 p.m. UTC | #1 
On Mon, Oct 21, 2024 at 03:01:55PM +0200, Christian Marangi wrote:
> It's conceptually similar to mediatek switch but register and bits
> are different.

Is it impractical to use struct regmap_field to abstract those
differences away and reuse the mt7530 driver's control flow? What is the
relationship between the Airoha and Mediatek IP anyway? The mt7530
maintainers should also be consulted w.r.t. whether code sharing is in
the common interest (I copied them).
Christian Marangi Oct. 21, 2024, 1:39 p.m. UTC | #2 
On Mon, Oct 21, 2024 at 04:36:05PM +0300, Vladimir Oltean wrote:
> On Mon, Oct 21, 2024 at 03:01:55PM +0200, Christian Marangi wrote:
> > It's conceptually similar to mediatek switch but register and bits
> > are different.
> 
> Is it impractical to use struct regmap_field to abstract those
> differences away and reuse the mt7530 driver's control flow? What is the
> relationship between the Airoha and Mediatek IP anyway? The mt7530
> maintainers should also be consulted w.r.t. whether code sharing is in
> the common interest (I copied them).

Some logic are similar for ATU or VLAN handling but then they added bits
in the middle of the register and moved some in other place.

Happy of being contradicted but from what I checked adapting the mtk
code would introduce lots of condition and wrapper and I feel it would
be actually worse than keeping the 2 codebase alone.

Would love some help by mt7530 to catch some very common case.
Vladimir Oltean Oct. 21, 2024, 1:50 p.m. UTC | #3 
On Mon, Oct 21, 2024 at 03:39:26PM +0200, Christian Marangi wrote:
> On Mon, Oct 21, 2024 at 04:36:05PM +0300, Vladimir Oltean wrote:
> > On Mon, Oct 21, 2024 at 03:01:55PM +0200, Christian Marangi wrote:
> > > It's conceptually similar to mediatek switch but register and bits
> > > are different.
> > 
> > Is it impractical to use struct regmap_field to abstract those
> > differences away and reuse the mt7530 driver's control flow? What is the
> > relationship between the Airoha and Mediatek IP anyway? The mt7530
> > maintainers should also be consulted w.r.t. whether code sharing is in
> > the common interest (I copied them).
> 
> Some logic are similar for ATU or VLAN handling but then they added bits
> in the middle of the register and moved some in other place.
> 
> Happy of being contradicted but from what I checked adapting the mtk
> code would introduce lots of condition and wrapper and I feel it would
> be actually worse than keeping the 2 codebase alone.
> 
> Would love some help by mt7530 to catch some very common case.

As long as the control flow is reasonably similar, the REG_FIELD() macro
is able to deal with register fields which have moved from one place to
another between hardware variants.
Russell King (Oracle) Oct. 21, 2024, 2:08 p.m. UTC | #4 
On Mon, Oct 21, 2024 at 03:01:58PM +0200, Christian Marangi wrote:
> +static int an8855_mii_read32(struct mii_bus *bus, u8 phy_id, u32 reg, u32 *val)
> +{
> +	u16 lo, hi;
> +	int ret;
> +
> +	ret = bus->write(bus, phy_id, 0x1f, 0x4);
> +	ret = bus->write(bus, phy_id, 0x10, 0);
> +
> +	ret = bus->write(bus, phy_id, 0x15, ((reg >> 16) & 0xFFFF));

These assignments above are useless on their own. What if one of them
fails?

Please also consider __mdiobus_write() and __mdiobus_read() which will
check that the bus lock is held, and also give the ability to trace
the bus activity.

> +	ret = bus->write(bus, phy_id, 0x16, (reg & 0xFFFF));
> +	if (ret < 0) {
> +		dev_err_ratelimited(&bus->dev,
> +				    "failed to read an8855 register\n");
> +		return ret;
> +	}
> +
> +	lo = bus->read(bus, phy_id, 0x18);
> +	hi = bus->read(bus, phy_id, 0x17);

What if one of these fails, and the negative value gets clamped to a
u16?

> +
> +	ret = bus->write(bus, phy_id, 0x1f, 0);
> +	if (ret < 0) {
> +		dev_err_ratelimited(&bus->dev,
> +				    "failed to read an8855 register\n");
> +		return ret;
> +	}
> +
> +	*val = (hi << 16) | (lo & 0xffff);
> +
> +	return 0;
> +}
> +
> +static int an8855_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> +{
> +	struct an8855_priv *priv = ctx;
> +	struct mii_bus *bus = priv->bus;
> +	int ret;
> +
> +	scoped_guard(mdio_mutex_nested, &bus->mdio_lock)
> +		ret = an8855_mii_read32(bus, priv->phy_base,
> +					reg, val);

I'm really not a fan of these non-C like code structures that make the
code harder to review, and can add (and already have resulted in) bugs,
but everyone to their own. Al Viro found a whole new class of bug caused
by these magic things. I'd much prefer explicit C code that can be read
and reviewed over "let the compiler do magic" stuff.

> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int an8855_mii_write32(struct mii_bus *bus, u8 phy_id, u32 reg, u32 val)
> +{
> +	int ret;
> +
> +	ret = bus->write(bus, phy_id, 0x1f, 0x4);
> +	ret = bus->write(bus, phy_id, 0x10, 0);
> +
> +	ret = bus->write(bus, phy_id, 0x11, ((reg >> 16) & 0xFFFF));
> +	ret = bus->write(bus, phy_id, 0x12, (reg & 0xFFFF));
> +
> +	ret = bus->write(bus, phy_id, 0x13, ((val >> 16) & 0xFFFF));
> +	ret = bus->write(bus, phy_id, 0x14, (val & 0xFFFF));

Same as above.

> +
> +	ret = bus->write(bus, phy_id, 0x1f, 0);
> +	if (ret < 0)
> +		dev_err_ratelimited(&bus->dev,
> +				    "failed to write an8855 register\n");
> +
> +	return 0;
> +}

...

> +static int an8855_set_mac_eee(struct dsa_switch *ds, int port,
> +			      struct ethtool_keee *eee)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	u32 reg;
> +	int ret;
> +
> +	if (eee->eee_enabled) {
> +		ret = regmap_read(priv->regmap, AN8855_PMCR_P(port), &reg);
> +		if (ret)
> +			return ret;
> +		if (reg & AN8855_PMCR_FORCE_MODE) {
> +			switch (reg & AN8855_PMCR_FORCE_SPEED) {
> +			case AN8855_PMCR_FORCE_SPEED_1000:
> +				reg |= AN8855_PMCR_FORCE_EEE1G;
> +				break;
> +			case AN8855_PMCR_FORCE_SPEED_100:
> +				reg |= AN8855_PMCR_FORCE_EEE100;
> +				break;
> +			default:
> +				break;
> +			}
> +			ret = regmap_write(priv->regmap, AN8855_PMCR_P(port), reg);
> +			if (ret)
> +				return ret;

What logic are you trying to implement here? It looks like you're
forcing EEE to be enabled here if AN8855_PMCR_FORCE_MODE was set, which,
reading the code in link_{up,down} it always will be when the link
happens to be down when the user configures EEE. This makes no sense.

EEE is supposed to be enabled as a result of the PHY's negotiation with
the link partner. There shouldn't be any forcing.

> +		}
> +		if (eee->tx_lpi_enabled) {
> +			ret = regmap_set_bits(priv->regmap, AN8855_PMEEECR_P(port),
> +					      AN8855_LPI_MODE_EN);
> +			if (ret)
> +				return ret;
> +		} else {
> +			ret = regmap_clear_bits(priv->regmap, AN8855_PMEEECR_P(port),
> +						AN8855_LPI_MODE_EN);
> +			if (ret)
> +				return ret;
> +		}

Maybe:

		ret = regmap_update_bits(priv->regmap, AN8855_PMEEECR_P(port),
					 AN8855_LPI_MODE_EN,
					 eee->tx_lpi_enabled ?
					   AN8855_LPI_MODE_EN : 0);
		if (ret)
			return ret;

> +	} else {
> +		ret = regmap_clear_bits(priv->regmap, AN8855_PMCR_P(port),
> +					AN8855_PMCR_FORCE_EEE1G |
> +					AN8855_PMCR_FORCE_EEE100);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_clear_bits(priv->regmap, AN8855_PMEEECR_P(port),
> +					AN8855_LPI_MODE_EN);
> +		if (ret)
> +			return ret;

This probably needs to interact with the link up/down state.

Really, I need to find the time to sort out adding EEE stuff to phylink
that is keyed from phylib's implementation, but not at the moment.

> +	}
> +
> +	return 0;
> +}
> +
> +static int an8855_get_mac_eee(struct dsa_switch *ds, int port,
> +			      struct ethtool_keee *eee)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	u32 reg;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, AN8855_PMEEECR_P(port), &reg);
> +	if (ret)
> +		return ret;
> +	eee->tx_lpi_enabled = reg & AN8855_LPI_MODE_EN;

This is fine, the MAC is responsible for LPI transmission.

> +
> +	ret = regmap_read(priv->regmap, AN8855_CKGCR, &reg);
> +	if (ret)
> +		return ret;
> +	/* Global LPI TXIDLE Threshold, default 60ms (unit 2us) */
> +	eee->tx_lpi_timer = FIELD_GET(AN8855_LPI_TXIDLE_THD_MASK, reg) / 500;

Also fine.

> +
> +	ret = regmap_read(priv->regmap, AN8855_PMSR_P(port), &reg);
> +	if (ret)
> +		return ret;
> +	eee->eee_active = reg & (AN8855_PMSR_EEE1G | AN8855_PMSR_EEE100M);

This isn't. You're overwriting the value set by
genphy_c45_ethtool_get_eee(), which has already determined whether
EEE is active from the PHY's negotiation state, and this is what
eee_active is supposed to indicate.

> +
> +	return 0;
> +}
> +
> +static u32 en8855_get_phy_flags(struct dsa_switch *ds, int port)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	u8 calibration_data[4] = { };
> +	u8 shift_sel;
> +	u32 val;
> +	int ret;
> +	int i;
> +
> +	/* PHY doesn't need calibration */
> +	if (!priv->phy_require_calib)
> +		return 0;
> +
> +	/* Read Calibration value */
> +	for (i = 0; i < sizeof(u32); i++) {
> +		ret = regmap_read(priv->regmap, AN8855_EFUSE_DATA0 +
> +				  ((3 + i + (4 * port)) * 4), &val);
> +		if (ret)
> +			return 0;
> +
> +		shift_sel = FIELD_GET(AN8855_EFUSE_R50O, val);
> +		calibration_data[i] = en8855_get_r50ohm_val(shift_sel);
> +	}
> +
> +	memcpy(&val, calibration_data, sizeof(u32));
> +	return val;

Ewwwwwww.

So you're reading from fuses, and then passing them as phy flags.
PHY flags are no longer 100% available for whatever you want to use
them for - some have standard meanings:

 *      - Bits [15:0] are free to use by the PHY driver to communicate
 *        driver specific behavior.
 *      - Bits [23:16] are currently reserved for future use.
 *      - Bits [31:24] are reserved for defining generic
 *        PHY driver behavior.

For example, PHY_F_NO_IRQ and PHY_F_RXC_ALWAYS_ON are already allocated
in the top 8 bits.

> +static void
> +an8855_phylink_mac_config(struct phylink_config *config, unsigned int mode,
> +			  const struct phylink_link_state *state)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct dsa_switch *ds = dp->ds;
> +	struct an8855_priv *priv;
> +	int port = dp->index;
> +
> +	priv = ds->priv;
> +
> +	switch (port) {
> +	case 0:
> +	case 1:
> +	case 2:
> +	case 3:
> +	case 4:
> +		return;
> +	case 5:
> +		break;
> +	default:
> +		dev_err(ds->dev, "unsupported port: %d", port);
> +		return;
> +	}
> +
> +	if (state->interface == PHY_INTERFACE_MODE_2500BASEX &&
> +	    phylink_autoneg_inband(mode))
> +		dev_err(ds->dev, "in-band negotiation unsupported");

Please check this in the PCS code.

> +
> +	regmap_update_bits(priv->regmap, AN8855_PMCR_P(port),
> +			   AN8855_PMCR_IFG_XMIT | AN8855_PMCR_MAC_MODE |
> +			   AN8855_PMCR_BACKOFF_EN | AN8855_PMCR_BACKPR_EN,
> +			   FIELD_PREP(AN8855_PMCR_IFG_XMIT, 0x1) |
> +			   AN8855_PMCR_MAC_MODE | AN8855_PMCR_BACKOFF_EN |
> +			   AN8855_PMCR_BACKPR_EN);
> +}
> +
> +static void an8855_phylink_get_caps(struct dsa_switch *ds, int port,
> +				    struct phylink_config *config)
> +{
> +	switch (port) {
> +	case 0:
> +	case 1:
> +	case 2:
> +	case 3:
> +	case 4:
> +		__set_bit(PHY_INTERFACE_MODE_GMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +		break;
> +	case 5:
> +		phy_interface_set_rgmii(config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_SGMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +			  config->supported_interfaces);
> +		break;
> +	}
> +
> +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +				   MAC_10 | MAC_100 | MAC_1000FD;
> +}
> +
> +static void
> +an8855_phylink_mac_link_down(struct phylink_config *config, unsigned int mode,
> +			     phy_interface_t interface)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct an8855_priv *priv = dp->ds->priv;
> +
> +	/* Disable TX/RX, force link down */
> +	regmap_update_bits(priv->regmap, AN8855_PMCR_P(dp->index),
> +			   AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN |
> +			   AN8855_PMCR_FORCE_MODE | AN8855_PMCR_FORCE_LNK,
> +			   AN8855_PMCR_FORCE_MODE);

Does forcing the link down prevent the AN8855_PMSR_LNK bit being set?
If not, please document that here because the current code goes against
what's documented in phylink:
 
 * If @mode is not an in-band negotiation mode (as defined by
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 * phylink_autoneg_inband()), force the link down and disable any
 * Energy Efficient Ethernet MAC configuration. ...

> +}
> +
> +static void
> +an8855_phylink_mac_link_up(struct phylink_config *config,
> +			   struct phy_device *phydev, unsigned int mode,
> +			   phy_interface_t interface, int speed, int duplex,
> +			   bool tx_pause, bool rx_pause)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct an8855_priv *priv = dp->ds->priv;
> +	int port = dp->index;
> +	u32 reg;
> +
> +	reg = regmap_read(priv->regmap, AN8855_PMCR_P(port), &reg);
> +	if (phylink_autoneg_inband(mode)) {
> +		reg &= ~AN8855_PMCR_FORCE_MODE;
> +	} else {
> +		reg |= AN8855_PMCR_FORCE_MODE | AN8855_PMCR_FORCE_LNK;
> +
> +		reg &= ~AN8855_PMCR_FORCE_SPEED;
> +		switch (speed) {
> +		case SPEED_10:
> +			reg |= AN8855_PMCR_FORCE_SPEED_10;
> +			break;
> +		case SPEED_100:
> +			reg |= AN8855_PMCR_FORCE_SPEED_100;
> +			break;
> +		case SPEED_1000:
> +			reg |= AN8855_PMCR_FORCE_SPEED_1000;
> +			break;
> +		case SPEED_2500:
> +			reg |= AN8855_PMCR_FORCE_SPEED_2500;
> +			break;
> +		case SPEED_5000:
> +			reg |= AN8855_PMCR_FORCE_SPEED_5000;
> +			break;
> +		}
> +
> +		reg &= AN8855_PMCR_FORCE_FDX;
> +		if (duplex == DUPLEX_FULL)
> +			reg |= AN8855_PMCR_FORCE_FDX;
> +
> +		reg &= AN8855_PMCR_RX_FC_EN;
> +		if (rx_pause || dsa_port_is_cpu(dp))
> +			reg |= AN8855_PMCR_RX_FC_EN;
> +
> +		reg &= AN8855_PMCR_TX_FC_EN;
> +		if (rx_pause || dsa_port_is_cpu(dp))
> +			reg |= AN8855_PMCR_TX_FC_EN;
> +
> +		/* Disable any EEE options */
> +		reg &= ~(AN8855_PMCR_FORCE_EEE5G | AN8855_PMCR_FORCE_EEE2P5G |
> +			 AN8855_PMCR_FORCE_EEE1G | AN8855_PMCR_FORCE_EEE100);
> +	}
> +
> +	reg |= AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN;
> +
> +	regmap_write(priv->regmap, AN8855_PMCR_P(port), reg);
> +}
> +
> +static void an8855_pcs_get_state(struct phylink_pcs *pcs,
> +				 struct phylink_link_state *state)
> +{
> +	struct an8855_priv *priv = container_of(pcs, struct an8855_priv, pcs);
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, AN8855_PMSR_P(AN8855_CPU_PORT), &val);
> +	if (ret < 0) {
> +		state->link = false;
> +		return;
> +	}
> +
> +	state->link = !!(val & AN8855_PMSR_LNK);
> +	state->an_complete = state->link;
> +	state->duplex = (val & AN8855_PMSR_DPX) ? DUPLEX_FULL :
> +						  DUPLEX_HALF;
> +
> +	switch (val & AN8855_PMSR_SPEED) {
> +	case AN8855_PMSR_SPEED_10:
> +		state->speed = SPEED_10;
> +		break;
> +	case AN8855_PMSR_SPEED_100:
> +		state->speed = SPEED_100;
> +		break;
> +	case AN8855_PMSR_SPEED_1000:
> +		state->speed = SPEED_1000;
> +		break;
> +	case AN8855_PMSR_SPEED_2500:
> +		state->speed = SPEED_2500;
> +		break;
> +	case AN8855_PMSR_SPEED_5000:
> +		state->speed = SPEED_5000;
> +		break;
> +	default:
> +		state->speed = SPEED_UNKNOWN;
> +		break;
> +	}
> +
> +	if (val & AN8855_PMSR_RX_FC)
> +		state->pause |= MLO_PAUSE_RX;
> +	if (val & AN8855_PMSR_TX_FC)
> +		state->pause |= MLO_PAUSE_TX;
> +}
> +
> +static int an8855_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> +			     phy_interface_t interface,
> +			     const unsigned long *advertising,
> +			     bool permit_pause_to_mac)
> +{
> +	struct an8855_priv *priv = container_of(pcs, struct an8855_priv, pcs);
> +	u32 val;
> +	int ret;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		return 0;
> +	case PHY_INTERFACE_MODE_SGMII:
> +		break;
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> +			return -EINVAL;
> +
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/*                   !!! WELCOME TO HELL !!!                   */
> +
> +	/* TX FIR - improve TX EYE */
> +	ret = regmap_update_bits(priv->regmap, AN8855_INTF_CTRL_10, GENMASK(21, 16),
> +				 FIELD_PREP(GENMASK(21, 16), 0x20));
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(priv->regmap, AN8855_INTF_CTRL_10, GENMASK(28, 24),
> +				 FIELD_PREP(GENMASK(28, 24), 0x4));
> +	if (ret)
> +		return ret;
> +	ret = regmap_set_bits(priv->regmap, AN8855_INTF_CTRL_10, BIT(29));
> +	if (ret)
> +		return ret;
> +
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x0;
> +	else
> +		val = 0xd;
> +	ret = regmap_update_bits(priv->regmap, AN8855_INTF_CTRL_11, GENMASK(5, 0),
> +				 FIELD_PREP(GENMASK(5, 0), val));
> +	if (ret)
> +		return ret;
> +	ret = regmap_set_bits(priv->regmap, AN8855_INTF_CTRL_11, BIT(6));
> +	if (ret)
> +		return ret;
> +
> +	/* RX CDR - improve RX Jitter Tolerance */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x5;
> +	else
> +		val = 0x6;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_BOT_LIM, GENMASK(26, 24),
> +				 FIELD_PREP(GENMASK(26, 24), val));
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_BOT_LIM, GENMASK(22, 20),
> +				 FIELD_PREP(GENMASK(22, 20), val));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x1;
> +	else
> +		val = 0x0;
> +	ret = regmap_update_bits(priv->regmap, AN8855_QP_DIG_MODE_CTRL_1, GENMASK(3, 2),
> +				 FIELD_PREP(GENMASK(3, 2), val));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - LPF */
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(1, 0),
> +				 FIELD_PREP(GENMASK(1, 0), 0x1));
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(4, 2),
> +				 FIELD_PREP(GENMASK(4, 2), 0x5));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(6) | BIT(7));
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(10, 8),
> +				 FIELD_PREP(GENMASK(10, 8), 0x3));
> +	if (ret)
> +		return ret;
> +	ret = regmap_set_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(29));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(12) | BIT(13));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - ICO */
> +	ret = regmap_set_bits(priv->regmap, AN8855_PLL_CTRL_4, BIT(2));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(14));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - CHP */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x6;
> +	else
> +		val = 0x4;
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(19, 16),
> +				 FIELD_PREP(GENMASK(19, 16), val));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - PFD */
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(21, 20),
> +				 FIELD_PREP(GENMASK(21, 20), 0x1));
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(25, 24),
> +				 FIELD_PREP(GENMASK(25, 24), 0x1));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(26));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - POSTDIV */
> +	ret = regmap_set_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(22));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(27));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(28));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - SDM */
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_4, BIT(3) | BIT(4));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(30));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_SS_LCPLL_PWCTL_SETTING_2,
> +				 GENMASK(17, 16),
> +				 FIELD_PREP(GENMASK(17, 16), 0x1));
> +	if (ret)
> +		return ret;
> +
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x7a000000;
> +	else
> +		val = 0x48000000;
> +	ret = regmap_write(priv->regmap, AN8855_SS_LCPLL_TDC_FLT_2, val);
> +	if (ret)
> +		return ret;
> +	ret = regmap_write(priv->regmap, AN8855_SS_LCPLL_TDC_PCW_1, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_clear_bits(priv->regmap, AN8855_SS_LCPLL_TDC_FLT_5, BIT(24));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CK_CTRL_0, BIT(8));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - SS */
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_3, GENMASK(15, 0));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_4, GENMASK(1, 0));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_3, GENMASK(31, 16));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - TDC */
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CK_CTRL_0, BIT(9));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(priv->regmap, AN8855_RG_QP_PLL_SDM_ORD, BIT(3));
> +	if (ret)
> +		return ret;
> +	ret = regmap_set_bits(priv->regmap, AN8855_RG_QP_PLL_SDM_ORD, BIT(4));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_RX_DAC_EN, GENMASK(17, 16),
> +				 FIELD_PREP(GENMASK(17, 16), 0x2));
> +	if (ret)
> +		return ret;
> +
> +	/* TCL Disable (only for Co-SIM) */
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PON_RXFEDIG_CTRL_0, BIT(12));
> +	if (ret)
> +		return ret;
> +
> +	/* TX Init */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x4;
> +	else
> +		val = 0x0;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_QP_TX_MODE_16B_EN, BIT(0));
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_TX_MODE_16B_EN,
> +				 GENMASK(31, 16),
> +				 FIELD_PREP(GENMASK(31, 16), val));
> +	if (ret)
> +		return ret;
> +
> +	/* RX Control/Init */
> +	ret = regmap_set_bits(priv->regmap, AN8855_RG_QP_RXAFE_RESERVE, BIT(11));
> +	if (ret)
> +		return ret;
> +
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x1;
> +	else
> +		val = 0x2;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_MJV_LIM,
> +				 GENMASK(5, 4),
> +				 FIELD_PREP(GENMASK(5, 4), val));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_SETVALUE,
> +				 GENMASK(28, 25),
> +				 FIELD_PREP(GENMASK(28, 25), 0x1));
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_SETVALUE,
> +				 GENMASK(31, 29),
> +				 FIELD_PREP(GENMASK(31, 29), 0x6));
> +	if (ret)
> +		return ret;
> +
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0xf;
> +	else
> +		val = 0xc;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_PR_CKREF_DIV1,
> +				 GENMASK(12, 8),
> +				 FIELD_PREP(GENMASK(12, 8), val));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_PR_KBAND_DIV_PCIE,
> +				 GENMASK(12, 8),
> +				 FIELD_PREP(GENMASK(12, 8), 0x19));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_QP_CDR_PR_KBAND_DIV_PCIE, BIT(6));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_FORCE_IBANDLPF_R_OFF,
> +				 GENMASK(12, 6),
> +				 FIELD_PREP(GENMASK(12, 6), 0x21));
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_FORCE_IBANDLPF_R_OFF,
> +				 GENMASK(17, 16),
> +				 FIELD_PREP(GENMASK(17, 16), 0x2));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_QP_CDR_FORCE_IBANDLPF_R_OFF, BIT(13));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_QP_CDR_PR_KBAND_DIV_PCIE, BIT(30));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_PR_CKREF_DIV1,
> +				 GENMASK(26, 24),
> +				 FIELD_PREP(GENMASK(26, 24), 0x4));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(priv->regmap, AN8855_RX_CTRL_26, BIT(23));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_RX_CTRL_26, BIT(24));
> +	if (ret)
> +		return ret;
> +	ret = regmap_set_bits(priv->regmap, AN8855_RX_CTRL_26, BIT(26));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_DLY_0, GENMASK(7, 0),
> +				 FIELD_PREP(GENMASK(7, 0), 0x6f));
> +	if (ret)
> +		return ret;
> +	ret = regmap_set_bits(priv->regmap, AN8855_RX_DLY_0, GENMASK(13, 8));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_42, GENMASK(12, 0),
> +				 FIELD_PREP(GENMASK(12, 0), 0x150));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_2, GENMASK(28, 16),
> +				 FIELD_PREP(GENMASK(28, 16), 0x150));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_PON_RXFEDIG_CTRL_9,
> +				 GENMASK(2, 0),
> +				 FIELD_PREP(GENMASK(2, 0), 0x1));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_8, GENMASK(27, 16),
> +				 FIELD_PREP(GENMASK(27, 16), 0x200));
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_8, GENMASK(14, 0),
> +				 FIELD_PREP(GENMASK(14, 0), 0xfff));
> +	if (ret)
> +		return ret;
> +
> +	/* Frequency meter */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x10;
> +	else
> +		val = 0x28;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_5, GENMASK(29, 10),
> +				 FIELD_PREP(GENMASK(29, 10), val));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_6, GENMASK(19, 0),
> +				 FIELD_PREP(GENMASK(19, 0), 0x64));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_7, GENMASK(19, 0),
> +				 FIELD_PREP(GENMASK(19, 0), 0x2710));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(priv->regmap, AN8855_PLL_CTRL_0, BIT(0));
> +	if (ret)
> +		return ret;
> +
> +	/* PCS Init */
> +	if (interface == PHY_INTERFACE_MODE_SGMII &&
> +	    neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED) {
> +		ret = regmap_clear_bits(priv->regmap, AN8855_QP_DIG_MODE_CTRL_0,
> +					BIT(0));
> +		if (ret)
> +			return ret;
> +		ret = regmap_clear_bits(priv->regmap, AN8855_QP_DIG_MODE_CTRL_0,
> +					GENMASK(5, 4));
> +		if (ret)
> +			return ret;

Do these really need to be done separately, or can the two be combined?

		ret = regmap_clear_bits(priv->regmap, AN8855_QP_DIG_MODE_CTRL_0,
					GENMASK(5, 4) | BIT(0));

?

> +	}
> +
> +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_HSGMII_PCS_CTROL_1, BIT(30));
> +	if (ret)
> +		return ret;
> +
> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> +		/* Set AN Ability - Interrupt */
> +		ret = regmap_set_bits(priv->regmap, AN8855_SGMII_REG_AN_FORCE_CL37, BIT(0));
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(priv->regmap, AN8855_SGMII_REG_AN_13,
> +					 GENMASK(5, 0),
> +					 FIELD_PREP(GENMASK(5, 0), 0xb));
> +		if (ret)
> +			return ret;
> +		ret = regmap_set_bits(priv->regmap, AN8855_SGMII_REG_AN_13, BIT(8));
> +		if (ret)
> +			return ret;
> +	}

Eh?

> +
> +	/* Rate Adaption - GMII path config. */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		ret = regmap_clear_bits(priv->regmap, AN8855_RATE_ADP_P0_CTRL_0, BIT(31));
> +		if (ret)
> +			return ret;
> +	} else {
> +		if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> +			ret = regmap_set_bits(priv->regmap, AN8855_MII_RA_AN_ENABLE, BIT(0));
> +			if (ret)
> +				return ret;
> +		} else {
> +			ret = regmap_set_bits(priv->regmap, AN8855_RG_AN_SGMII_MODE_FORCE,
> +					      BIT(0));
> +			if (ret)
> +				return ret;
> +			ret = regmap_clear_bits(priv->regmap, AN8855_RG_AN_SGMII_MODE_FORCE,
> +						GENMASK(5, 4));
> +			if (ret)
> +				return ret;
> +
> +			ret = regmap_clear_bits(priv->regmap, AN8855_RATE_ADP_P0_CTRL_0,
> +						GENMASK(3, 0));
> +			if (ret)
> +				return ret;
> +		}
> +
> +		ret = regmap_set_bits(priv->regmap, AN8855_RATE_ADP_P0_CTRL_0, BIT(28));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_set_bits(priv->regmap, AN8855_RG_RATE_ADAPT_CTRL_0, BIT(0));
> +	if (ret)
> +		return ret;
> +	ret = regmap_set_bits(priv->regmap, AN8855_RG_RATE_ADAPT_CTRL_0, BIT(4));
> +	if (ret)
> +		return ret;
> +	ret = regmap_set_bits(priv->regmap, AN8855_RG_RATE_ADAPT_CTRL_0, GENMASK(27, 26));
> +	if (ret)
> +		return ret;
> +
> +	/* Disable AN */
> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> +		ret = regmap_set_bits(priv->regmap, AN8855_SGMII_REG_AN0,
> +				      AN8855_SGMII_AN_ENABLE);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regmap_clear_bits(priv->regmap, AN8855_SGMII_REG_AN0,
> +					AN8855_SGMII_AN_ENABLE);
> +		if (ret)
> +			return ret;
> +	}

Again, using regmap_update_bits() with a mask and value is probably more
readable here. This looks like it's twiddling a standard BMCR_ANENABLE
bit.

> +
> +	if (interface == PHY_INTERFACE_MODE_SGMII &&
> +	    neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED) {
> +		ret = regmap_set_bits(priv->regmap, AN8855_PHY_RX_FORCE_CTRL_0, BIT(4));
> +		if (ret)
> +			return ret;
> +	}

Eh?

> +
> +	/* Force Speed */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX ||
> +	    neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> +		if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +			val = 0x3;
> +		else
> +			val = 0x2;
> +		ret = regmap_set_bits(priv->regmap, AN8855_SGMII_STS_CTRL_0, BIT(2));
> +		if (ret)
> +			return ret;
> +		ret = regmap_update_bits(priv->regmap, AN8855_SGMII_STS_CTRL_0,
> +					 GENMASK(5, 4),
> +					 FIELD_PREP(GENMASK(5, 4), val));
> +		if (ret)
> +			return ret;
> +	}

I don't understand why speed should be forced if inband is enabled or
we're using 2500base-X.

> +
> +	/* bypass flow control to MAC */
> +	ret = regmap_write(priv->regmap, AN8855_MSG_RX_LIK_STS_0, 0x01010107);
> +	if (ret)
> +		return ret;
> +	ret = regmap_write(priv->regmap, AN8855_MSG_RX_LIK_STS_2, 0x00000EEF);
> +	if (ret)
> +		return ret;

Overall, I'd like more comments about what stuff is doing, especially
the AN stuff. I can't make head nor tail of e.g. is there any
advertisement being programmed or not.

Also, given that this will be called _on its own_ if the user requests
the inband advertisement to be changed, we don't want to unnecessarily
disrupt the link. What would happen if this code runs when the link is
up?

> +
> +	return 0;
> +}
> +
> +static void an8855_pcs_an_restart(struct phylink_pcs *pcs)
> +{
> +	struct an8855_priv *priv = container_of(pcs, struct an8855_priv, pcs);
> +
> +	regmap_set_bits(priv->regmap, AN8855_SGMII_REG_AN0,
> +			AN8855_SGMII_AN_RESTART);

Again, looks like a standard PHY BMCR.

I haven't done a full review, but these are just what I've spotted so
far.
Russell King (Oracle) Oct. 21, 2024, 2:11 p.m. UTC | #5 
On Mon, Oct 21, 2024 at 04:36:05PM +0300, Vladimir Oltean wrote:
> On Mon, Oct 21, 2024 at 03:01:55PM +0200, Christian Marangi wrote:
> > It's conceptually similar to mediatek switch but register and bits
> > are different.
> 
> Is it impractical to use struct regmap_field to abstract those
> differences away and reuse the mt7530 driver's control flow? What is the
> relationship between the Airoha and Mediatek IP anyway? The mt7530
> maintainers should also be consulted w.r.t. whether code sharing is in
> the common interest (I copied them).

That thought crossed my mind while reviewing patch 3. I compared the
PMCR and PMSR, a lot of the bits are in completely different places
between the two. I didn't check further, but I got the feeling that
would invite more complexity.
Christian Marangi Oct. 21, 2024, 2:39 p.m. UTC | #6 
On Mon, Oct 21, 2024 at 03:08:24PM +0100, Russell King (Oracle) wrote:
> On Mon, Oct 21, 2024 at 03:01:58PM +0200, Christian Marangi wrote:
> > +static int an8855_mii_read32(struct mii_bus *bus, u8 phy_id, u32 reg, u32 *val)
> > +{
> > +	u16 lo, hi;
> > +	int ret;
> > +
> > +	ret = bus->write(bus, phy_id, 0x1f, 0x4);
> > +	ret = bus->write(bus, phy_id, 0x10, 0);
> > +
> > +	ret = bus->write(bus, phy_id, 0x15, ((reg >> 16) & 0xFFFF));
> 
> These assignments above are useless on their own. What if one of them
> fails?
> 
> Please also consider __mdiobus_write() and __mdiobus_read() which will
> check that the bus lock is held, and also give the ability to trace
> the bus activity.
> 
> > +	ret = bus->write(bus, phy_id, 0x16, (reg & 0xFFFF));
> > +	if (ret < 0) {
> > +		dev_err_ratelimited(&bus->dev,
> > +				    "failed to read an8855 register\n");
> > +		return ret;
> > +	}
> > +
> > +	lo = bus->read(bus, phy_id, 0x18);
> > +	hi = bus->read(bus, phy_id, 0x17);
> 
> What if one of these fails, and the negative value gets clamped to a
> u16?
> 
> > +
> > +	ret = bus->write(bus, phy_id, 0x1f, 0);
> > +	if (ret < 0) {
> > +		dev_err_ratelimited(&bus->dev,
> > +				    "failed to read an8855 register\n");
> > +		return ret;
> > +	}
> > +
> > +	*val = (hi << 16) | (lo & 0xffff);
> > +
> > +	return 0;
> > +}
> > +
> > +static int an8855_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> > +{
> > +	struct an8855_priv *priv = ctx;
> > +	struct mii_bus *bus = priv->bus;
> > +	int ret;
> > +
> > +	scoped_guard(mdio_mutex_nested, &bus->mdio_lock)
> > +		ret = an8855_mii_read32(bus, priv->phy_base,
> > +					reg, val);
> 
> I'm really not a fan of these non-C like code structures that make the
> code harder to review, and can add (and already have resulted in) bugs,
> but everyone to their own. Al Viro found a whole new class of bug caused
> by these magic things. I'd much prefer explicit C code that can be read
> and reviewed over "let the compiler do magic" stuff.
>

Eh I hoped some of the cleanup API could be used... I received some
request to start using it. I will drop them entirely ok.

> > +
> > +	return ret < 0 ? ret : 0;
> > +}
> > +
> > +static int an8855_mii_write32(struct mii_bus *bus, u8 phy_id, u32 reg, u32 val)
> > +{
> > +	int ret;
> > +
> > +	ret = bus->write(bus, phy_id, 0x1f, 0x4);
> > +	ret = bus->write(bus, phy_id, 0x10, 0);
> > +
> > +	ret = bus->write(bus, phy_id, 0x11, ((reg >> 16) & 0xFFFF));
> > +	ret = bus->write(bus, phy_id, 0x12, (reg & 0xFFFF));
> > +
> > +	ret = bus->write(bus, phy_id, 0x13, ((val >> 16) & 0xFFFF));
> > +	ret = bus->write(bus, phy_id, 0x14, (val & 0xFFFF));
> 
> Same as above.
> 
> > +
> > +	ret = bus->write(bus, phy_id, 0x1f, 0);
> > +	if (ret < 0)
> > +		dev_err_ratelimited(&bus->dev,
> > +				    "failed to write an8855 register\n");
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int an8855_set_mac_eee(struct dsa_switch *ds, int port,
> > +			      struct ethtool_keee *eee)
> > +{
> > +	struct an8855_priv *priv = ds->priv;
> > +	u32 reg;
> > +	int ret;
> > +
> > +	if (eee->eee_enabled) {
> > +		ret = regmap_read(priv->regmap, AN8855_PMCR_P(port), &reg);
> > +		if (ret)
> > +			return ret;
> > +		if (reg & AN8855_PMCR_FORCE_MODE) {
> > +			switch (reg & AN8855_PMCR_FORCE_SPEED) {
> > +			case AN8855_PMCR_FORCE_SPEED_1000:
> > +				reg |= AN8855_PMCR_FORCE_EEE1G;
> > +				break;
> > +			case AN8855_PMCR_FORCE_SPEED_100:
> > +				reg |= AN8855_PMCR_FORCE_EEE100;
> > +				break;
> > +			default:
> > +				break;
> > +			}
> > +			ret = regmap_write(priv->regmap, AN8855_PMCR_P(port), reg);
> > +			if (ret)
> > +				return ret;
> 
> What logic are you trying to implement here? It looks like you're
> forcing EEE to be enabled here if AN8855_PMCR_FORCE_MODE was set, which,
> reading the code in link_{up,down} it always will be when the link
> happens to be down when the user configures EEE. This makes no sense.
> 
> EEE is supposed to be enabled as a result of the PHY's negotiation with
> the link partner. There shouldn't be any forcing.
> 
> > +		}
> > +		if (eee->tx_lpi_enabled) {
> > +			ret = regmap_set_bits(priv->regmap, AN8855_PMEEECR_P(port),
> > +					      AN8855_LPI_MODE_EN);
> > +			if (ret)
> > +				return ret;
> > +		} else {
> > +			ret = regmap_clear_bits(priv->regmap, AN8855_PMEEECR_P(port),
> > +						AN8855_LPI_MODE_EN);
> > +			if (ret)
> > +				return ret;
> > +		}
> 
> Maybe:
> 
> 		ret = regmap_update_bits(priv->regmap, AN8855_PMEEECR_P(port),
> 					 AN8855_LPI_MODE_EN,
> 					 eee->tx_lpi_enabled ?
> 					   AN8855_LPI_MODE_EN : 0);
> 		if (ret)
> 			return ret;
> 
> > +	} else {
> > +		ret = regmap_clear_bits(priv->regmap, AN8855_PMCR_P(port),
> > +					AN8855_PMCR_FORCE_EEE1G |
> > +					AN8855_PMCR_FORCE_EEE100);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_clear_bits(priv->regmap, AN8855_PMEEECR_P(port),
> > +					AN8855_LPI_MODE_EN);
> > +		if (ret)
> > +			return ret;
> 
> This probably needs to interact with the link up/down state.
> 
> Really, I need to find the time to sort out adding EEE stuff to phylink
> that is keyed from phylib's implementation, but not at the moment.
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int an8855_get_mac_eee(struct dsa_switch *ds, int port,
> > +			      struct ethtool_keee *eee)
> > +{
> > +	struct an8855_priv *priv = ds->priv;
> > +	u32 reg;
> > +	int ret;
> > +
> > +	ret = regmap_read(priv->regmap, AN8855_PMEEECR_P(port), &reg);
> > +	if (ret)
> > +		return ret;
> > +	eee->tx_lpi_enabled = reg & AN8855_LPI_MODE_EN;
> 
> This is fine, the MAC is responsible for LPI transmission.
> 
> > +
> > +	ret = regmap_read(priv->regmap, AN8855_CKGCR, &reg);
> > +	if (ret)
> > +		return ret;
> > +	/* Global LPI TXIDLE Threshold, default 60ms (unit 2us) */
> > +	eee->tx_lpi_timer = FIELD_GET(AN8855_LPI_TXIDLE_THD_MASK, reg) / 500;
> 
> Also fine.
> 
> > +
> > +	ret = regmap_read(priv->regmap, AN8855_PMSR_P(port), &reg);
> > +	if (ret)
> > +		return ret;
> > +	eee->eee_active = reg & (AN8855_PMSR_EEE1G | AN8855_PMSR_EEE100M);
> 
> This isn't. You're overwriting the value set by
> genphy_c45_ethtool_get_eee(), which has already determined whether
> EEE is active from the PHY's negotiation state, and this is what
> eee_active is supposed to indicate.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static u32 en8855_get_phy_flags(struct dsa_switch *ds, int port)
> > +{
> > +	struct an8855_priv *priv = ds->priv;
> > +	u8 calibration_data[4] = { };
> > +	u8 shift_sel;
> > +	u32 val;
> > +	int ret;
> > +	int i;
> > +
> > +	/* PHY doesn't need calibration */
> > +	if (!priv->phy_require_calib)
> > +		return 0;
> > +
> > +	/* Read Calibration value */
> > +	for (i = 0; i < sizeof(u32); i++) {
> > +		ret = regmap_read(priv->regmap, AN8855_EFUSE_DATA0 +
> > +				  ((3 + i + (4 * port)) * 4), &val);
> > +		if (ret)
> > +			return 0;
> > +
> > +		shift_sel = FIELD_GET(AN8855_EFUSE_R50O, val);
> > +		calibration_data[i] = en8855_get_r50ohm_val(shift_sel);
> > +	}
> > +
> > +	memcpy(&val, calibration_data, sizeof(u32));
> > +	return val;
> 
> Ewwwwwww.
> 
> So you're reading from fuses, and then passing them as phy flags.
> PHY flags are no longer 100% available for whatever you want to use
> them for - some have standard meanings:
> 

Yep this is the main reason of RFC... Any hint on how to handle this?

Actually the Switch have some special regs to access the PHY indirectly
so maybe I can do the calibration from the switch but I would really
love to find a way to pass these kind of data and handle them entirely
in the PHY driver code.

The first solution that comes to mind is shared global header in
include/linux/dsa and implement some kind of OPs or additional entry in
struct phy to declare a pointer and the size of it? Or with the global
header a struct directly? But this would be very special and just for
these case of internal PHY.

>  *      - Bits [15:0] are free to use by the PHY driver to communicate
>  *        driver specific behavior.
>  *      - Bits [23:16] are currently reserved for future use.
>  *      - Bits [31:24] are reserved for defining generic
>  *        PHY driver behavior.
> 
> For example, PHY_F_NO_IRQ and PHY_F_RXC_ALWAYS_ON are already allocated
> in the top 8 bits.
> 

I checked the DSA code and hoped I could use the full register.

> > +static void
> > +an8855_phylink_mac_config(struct phylink_config *config, unsigned int mode,
> > +			  const struct phylink_link_state *state)
> > +{
> > +	struct dsa_port *dp = dsa_phylink_to_port(config);
> > +	struct dsa_switch *ds = dp->ds;
> > +	struct an8855_priv *priv;
> > +	int port = dp->index;
> > +
> > +	priv = ds->priv;
> > +
> > +	switch (port) {
> > +	case 0:
> > +	case 1:
> > +	case 2:
> > +	case 3:
> > +	case 4:
> > +		return;
> > +	case 5:
> > +		break;
> > +	default:
> > +		dev_err(ds->dev, "unsupported port: %d", port);
> > +		return;
> > +	}
> > +
> > +	if (state->interface == PHY_INTERFACE_MODE_2500BASEX &&
> > +	    phylink_autoneg_inband(mode))
> > +		dev_err(ds->dev, "in-band negotiation unsupported");
> 
> Please check this in the PCS code.
> 
> > +
> > +	regmap_update_bits(priv->regmap, AN8855_PMCR_P(port),
> > +			   AN8855_PMCR_IFG_XMIT | AN8855_PMCR_MAC_MODE |
> > +			   AN8855_PMCR_BACKOFF_EN | AN8855_PMCR_BACKPR_EN,
> > +			   FIELD_PREP(AN8855_PMCR_IFG_XMIT, 0x1) |
> > +			   AN8855_PMCR_MAC_MODE | AN8855_PMCR_BACKOFF_EN |
> > +			   AN8855_PMCR_BACKPR_EN);
> > +}
> > +
> > +static void an8855_phylink_get_caps(struct dsa_switch *ds, int port,
> > +				    struct phylink_config *config)
> > +{
> > +	switch (port) {
> > +	case 0:
> > +	case 1:
> > +	case 2:
> > +	case 3:
> > +	case 4:
> > +		__set_bit(PHY_INTERFACE_MODE_GMII,
> > +			  config->supported_interfaces);
> > +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > +			  config->supported_interfaces);
> > +		break;
> > +	case 5:
> > +		phy_interface_set_rgmii(config->supported_interfaces);
> > +		__set_bit(PHY_INTERFACE_MODE_SGMII,
> > +			  config->supported_interfaces);
> > +		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > +			  config->supported_interfaces);
> > +		break;
> > +	}
> > +
> > +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> > +				   MAC_10 | MAC_100 | MAC_1000FD;
> > +}
> > +
> > +static void
> > +an8855_phylink_mac_link_down(struct phylink_config *config, unsigned int mode,
> > +			     phy_interface_t interface)
> > +{
> > +	struct dsa_port *dp = dsa_phylink_to_port(config);
> > +	struct an8855_priv *priv = dp->ds->priv;
> > +
> > +	/* Disable TX/RX, force link down */
> > +	regmap_update_bits(priv->regmap, AN8855_PMCR_P(dp->index),
> > +			   AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN |
> > +			   AN8855_PMCR_FORCE_MODE | AN8855_PMCR_FORCE_LNK,
> > +			   AN8855_PMCR_FORCE_MODE);
> 
> Does forcing the link down prevent the AN8855_PMSR_LNK bit being set?
> If not, please document that here because the current code goes against
> what's documented in phylink:
>  
>  * If @mode is not an in-band negotiation mode (as defined by
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  * phylink_autoneg_inband()), force the link down and disable any
>  * Energy Efficient Ethernet MAC configuration. ...
> 

Mhhh the force bit just "force" the configutation, AKA it does set the
link down. We can keep the link up and just disable TX and RX.

In the current code, I enable force bit, and clear the LINK bit if it
was set prev and disable TX and RX.

Soo with the comments, with autoneg I should just disable TX and RX
correct?

> > +}
> > +
> > +static void
> > +an8855_phylink_mac_link_up(struct phylink_config *config,
> > +			   struct phy_device *phydev, unsigned int mode,
> > +			   phy_interface_t interface, int speed, int duplex,
> > +			   bool tx_pause, bool rx_pause)
> > +{
> > +	struct dsa_port *dp = dsa_phylink_to_port(config);
> > +	struct an8855_priv *priv = dp->ds->priv;
> > +	int port = dp->index;
> > +	u32 reg;
> > +
> > +	reg = regmap_read(priv->regmap, AN8855_PMCR_P(port), &reg);
> > +	if (phylink_autoneg_inband(mode)) {
> > +		reg &= ~AN8855_PMCR_FORCE_MODE;
> > +	} else {
> > +		reg |= AN8855_PMCR_FORCE_MODE | AN8855_PMCR_FORCE_LNK;
> > +
> > +		reg &= ~AN8855_PMCR_FORCE_SPEED;
> > +		switch (speed) {
> > +		case SPEED_10:
> > +			reg |= AN8855_PMCR_FORCE_SPEED_10;
> > +			break;
> > +		case SPEED_100:
> > +			reg |= AN8855_PMCR_FORCE_SPEED_100;
> > +			break;
> > +		case SPEED_1000:
> > +			reg |= AN8855_PMCR_FORCE_SPEED_1000;
> > +			break;
> > +		case SPEED_2500:
> > +			reg |= AN8855_PMCR_FORCE_SPEED_2500;
> > +			break;
> > +		case SPEED_5000:
> > +			reg |= AN8855_PMCR_FORCE_SPEED_5000;
> > +			break;
> > +		}
> > +
> > +		reg &= AN8855_PMCR_FORCE_FDX;
> > +		if (duplex == DUPLEX_FULL)
> > +			reg |= AN8855_PMCR_FORCE_FDX;
> > +
> > +		reg &= AN8855_PMCR_RX_FC_EN;
> > +		if (rx_pause || dsa_port_is_cpu(dp))
> > +			reg |= AN8855_PMCR_RX_FC_EN;
> > +
> > +		reg &= AN8855_PMCR_TX_FC_EN;
> > +		if (rx_pause || dsa_port_is_cpu(dp))
> > +			reg |= AN8855_PMCR_TX_FC_EN;
> > +
> > +		/* Disable any EEE options */
> > +		reg &= ~(AN8855_PMCR_FORCE_EEE5G | AN8855_PMCR_FORCE_EEE2P5G |
> > +			 AN8855_PMCR_FORCE_EEE1G | AN8855_PMCR_FORCE_EEE100);
> > +	}
> > +
> > +	reg |= AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN;
> > +
> > +	regmap_write(priv->regmap, AN8855_PMCR_P(port), reg);
> > +}
> > +
> > +static void an8855_pcs_get_state(struct phylink_pcs *pcs,
> > +				 struct phylink_link_state *state)
> > +{
> > +	struct an8855_priv *priv = container_of(pcs, struct an8855_priv, pcs);
> > +	u32 val;
> > +	int ret;
> > +
> > +	ret = regmap_read(priv->regmap, AN8855_PMSR_P(AN8855_CPU_PORT), &val);
> > +	if (ret < 0) {
> > +		state->link = false;
> > +		return;
> > +	}
> > +
> > +	state->link = !!(val & AN8855_PMSR_LNK);
> > +	state->an_complete = state->link;
> > +	state->duplex = (val & AN8855_PMSR_DPX) ? DUPLEX_FULL :
> > +						  DUPLEX_HALF;
> > +
> > +	switch (val & AN8855_PMSR_SPEED) {
> > +	case AN8855_PMSR_SPEED_10:
> > +		state->speed = SPEED_10;
> > +		break;
> > +	case AN8855_PMSR_SPEED_100:
> > +		state->speed = SPEED_100;
> > +		break;
> > +	case AN8855_PMSR_SPEED_1000:
> > +		state->speed = SPEED_1000;
> > +		break;
> > +	case AN8855_PMSR_SPEED_2500:
> > +		state->speed = SPEED_2500;
> > +		break;
> > +	case AN8855_PMSR_SPEED_5000:
> > +		state->speed = SPEED_5000;
> > +		break;
> > +	default:
> > +		state->speed = SPEED_UNKNOWN;
> > +		break;
> > +	}
> > +
> > +	if (val & AN8855_PMSR_RX_FC)
> > +		state->pause |= MLO_PAUSE_RX;
> > +	if (val & AN8855_PMSR_TX_FC)
> > +		state->pause |= MLO_PAUSE_TX;
> > +}
> > +
> > +static int an8855_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> > +			     phy_interface_t interface,
> > +			     const unsigned long *advertising,
> > +			     bool permit_pause_to_mac)
> > +{
> > +	struct an8855_priv *priv = container_of(pcs, struct an8855_priv, pcs);
> > +	u32 val;
> > +	int ret;
> > +
> > +	switch (interface) {
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +		return 0;
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +		break;
> > +	case PHY_INTERFACE_MODE_2500BASEX:
> > +		if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> > +			return -EINVAL;
> > +
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*                   !!! WELCOME TO HELL !!!                   */
> > +
> > +	/* TX FIR - improve TX EYE */
> > +	ret = regmap_update_bits(priv->regmap, AN8855_INTF_CTRL_10, GENMASK(21, 16),
> > +				 FIELD_PREP(GENMASK(21, 16), 0x20));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_INTF_CTRL_10, GENMASK(28, 24),
> > +				 FIELD_PREP(GENMASK(28, 24), 0x4));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_set_bits(priv->regmap, AN8855_INTF_CTRL_10, BIT(29));
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +		val = 0x0;
> > +	else
> > +		val = 0xd;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_INTF_CTRL_11, GENMASK(5, 0),
> > +				 FIELD_PREP(GENMASK(5, 0), val));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_set_bits(priv->regmap, AN8855_INTF_CTRL_11, BIT(6));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* RX CDR - improve RX Jitter Tolerance */
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +		val = 0x5;
> > +	else
> > +		val = 0x6;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_BOT_LIM, GENMASK(26, 24),
> > +				 FIELD_PREP(GENMASK(26, 24), val));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_BOT_LIM, GENMASK(22, 20),
> > +				 FIELD_PREP(GENMASK(22, 20), val));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* PLL */
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +		val = 0x1;
> > +	else
> > +		val = 0x0;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_QP_DIG_MODE_CTRL_1, GENMASK(3, 2),
> > +				 FIELD_PREP(GENMASK(3, 2), val));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* PLL - LPF */
> > +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(1, 0),
> > +				 FIELD_PREP(GENMASK(1, 0), 0x1));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(4, 2),
> > +				 FIELD_PREP(GENMASK(4, 2), 0x5));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(6) | BIT(7));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(10, 8),
> > +				 FIELD_PREP(GENMASK(10, 8), 0x3));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_set_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(29));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(12) | BIT(13));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* PLL - ICO */
> > +	ret = regmap_set_bits(priv->regmap, AN8855_PLL_CTRL_4, BIT(2));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(14));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* PLL - CHP */
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +		val = 0x6;
> > +	else
> > +		val = 0x4;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(19, 16),
> > +				 FIELD_PREP(GENMASK(19, 16), val));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* PLL - PFD */
> > +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(21, 20),
> > +				 FIELD_PREP(GENMASK(21, 20), 0x1));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2, GENMASK(25, 24),
> > +				 FIELD_PREP(GENMASK(25, 24), 0x1));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(26));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* PLL - POSTDIV */
> > +	ret = regmap_set_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(22));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(27));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(28));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* PLL - SDM */
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_4, BIT(3) | BIT(4));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2, BIT(30));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_SS_LCPLL_PWCTL_SETTING_2,
> > +				 GENMASK(17, 16),
> > +				 FIELD_PREP(GENMASK(17, 16), 0x1));
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +		val = 0x7a000000;
> > +	else
> > +		val = 0x48000000;
> > +	ret = regmap_write(priv->regmap, AN8855_SS_LCPLL_TDC_FLT_2, val);
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_write(priv->regmap, AN8855_SS_LCPLL_TDC_PCW_1, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_SS_LCPLL_TDC_FLT_5, BIT(24));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CK_CTRL_0, BIT(8));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* PLL - SS */
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_3, GENMASK(15, 0));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_4, GENMASK(1, 0));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_3, GENMASK(31, 16));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* PLL - TDC */
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CK_CTRL_0, BIT(9));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_set_bits(priv->regmap, AN8855_RG_QP_PLL_SDM_ORD, BIT(3));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_set_bits(priv->regmap, AN8855_RG_QP_PLL_SDM_ORD, BIT(4));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_RX_DAC_EN, GENMASK(17, 16),
> > +				 FIELD_PREP(GENMASK(17, 16), 0x2));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* TCL Disable (only for Co-SIM) */
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PON_RXFEDIG_CTRL_0, BIT(12));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* TX Init */
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +		val = 0x4;
> > +	else
> > +		val = 0x0;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_QP_TX_MODE_16B_EN, BIT(0));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_TX_MODE_16B_EN,
> > +				 GENMASK(31, 16),
> > +				 FIELD_PREP(GENMASK(31, 16), val));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* RX Control/Init */
> > +	ret = regmap_set_bits(priv->regmap, AN8855_RG_QP_RXAFE_RESERVE, BIT(11));
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +		val = 0x1;
> > +	else
> > +		val = 0x2;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_MJV_LIM,
> > +				 GENMASK(5, 4),
> > +				 FIELD_PREP(GENMASK(5, 4), val));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_SETVALUE,
> > +				 GENMASK(28, 25),
> > +				 FIELD_PREP(GENMASK(28, 25), 0x1));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_SETVALUE,
> > +				 GENMASK(31, 29),
> > +				 FIELD_PREP(GENMASK(31, 29), 0x6));
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +		val = 0xf;
> > +	else
> > +		val = 0xc;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_PR_CKREF_DIV1,
> > +				 GENMASK(12, 8),
> > +				 FIELD_PREP(GENMASK(12, 8), val));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_PR_KBAND_DIV_PCIE,
> > +				 GENMASK(12, 8),
> > +				 FIELD_PREP(GENMASK(12, 8), 0x19));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_QP_CDR_PR_KBAND_DIV_PCIE, BIT(6));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_FORCE_IBANDLPF_R_OFF,
> > +				 GENMASK(12, 6),
> > +				 FIELD_PREP(GENMASK(12, 6), 0x21));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_FORCE_IBANDLPF_R_OFF,
> > +				 GENMASK(17, 16),
> > +				 FIELD_PREP(GENMASK(17, 16), 0x2));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_QP_CDR_FORCE_IBANDLPF_R_OFF, BIT(13));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_QP_CDR_PR_KBAND_DIV_PCIE, BIT(30));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_PR_CKREF_DIV1,
> > +				 GENMASK(26, 24),
> > +				 FIELD_PREP(GENMASK(26, 24), 0x4));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_set_bits(priv->regmap, AN8855_RX_CTRL_26, BIT(23));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_RX_CTRL_26, BIT(24));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_set_bits(priv->regmap, AN8855_RX_CTRL_26, BIT(26));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RX_DLY_0, GENMASK(7, 0),
> > +				 FIELD_PREP(GENMASK(7, 0), 0x6f));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_set_bits(priv->regmap, AN8855_RX_DLY_0, GENMASK(13, 8));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_42, GENMASK(12, 0),
> > +				 FIELD_PREP(GENMASK(12, 0), 0x150));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_2, GENMASK(28, 16),
> > +				 FIELD_PREP(GENMASK(28, 16), 0x150));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_PON_RXFEDIG_CTRL_9,
> > +				 GENMASK(2, 0),
> > +				 FIELD_PREP(GENMASK(2, 0), 0x1));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_8, GENMASK(27, 16),
> > +				 FIELD_PREP(GENMASK(27, 16), 0x200));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_8, GENMASK(14, 0),
> > +				 FIELD_PREP(GENMASK(14, 0), 0xfff));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Frequency meter */
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +		val = 0x10;
> > +	else
> > +		val = 0x28;
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_5, GENMASK(29, 10),
> > +				 FIELD_PREP(GENMASK(29, 10), val));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_6, GENMASK(19, 0),
> > +				 FIELD_PREP(GENMASK(19, 0), 0x64));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_7, GENMASK(19, 0),
> > +				 FIELD_PREP(GENMASK(19, 0), 0x2710));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_set_bits(priv->regmap, AN8855_PLL_CTRL_0, BIT(0));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* PCS Init */
> > +	if (interface == PHY_INTERFACE_MODE_SGMII &&
> > +	    neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED) {
> > +		ret = regmap_clear_bits(priv->regmap, AN8855_QP_DIG_MODE_CTRL_0,
> > +					BIT(0));
> > +		if (ret)
> > +			return ret;
> > +		ret = regmap_clear_bits(priv->regmap, AN8855_QP_DIG_MODE_CTRL_0,
> > +					GENMASK(5, 4));
> > +		if (ret)
> > +			return ret;
> 
> Do these really need to be done separately, or can the two be combined?
> 
> 		ret = regmap_clear_bits(priv->regmap, AN8855_QP_DIG_MODE_CTRL_0,
> 					GENMASK(5, 4) | BIT(0));
> 
> ?

I decided to separate them as much as possible following the pattern
used in SDK to make this mess more readable. If needed yes I can combine
the 2.

> 
> > +	}
> > +
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_HSGMII_PCS_CTROL_1, BIT(30));
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> > +		/* Set AN Ability - Interrupt */
> > +		ret = regmap_set_bits(priv->regmap, AN8855_SGMII_REG_AN_FORCE_CL37, BIT(0));
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_update_bits(priv->regmap, AN8855_SGMII_REG_AN_13,
> > +					 GENMASK(5, 0),
> > +					 FIELD_PREP(GENMASK(5, 0), 0xb));
> > +		if (ret)
> > +			return ret;
> > +		ret = regmap_set_bits(priv->regmap, AN8855_SGMII_REG_AN_13, BIT(8));
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> Eh?
> 
> > +
> > +	/* Rate Adaption - GMII path config. */
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX) {
> > +		ret = regmap_clear_bits(priv->regmap, AN8855_RATE_ADP_P0_CTRL_0, BIT(31));
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> > +			ret = regmap_set_bits(priv->regmap, AN8855_MII_RA_AN_ENABLE, BIT(0));
> > +			if (ret)
> > +				return ret;
> > +		} else {
> > +			ret = regmap_set_bits(priv->regmap, AN8855_RG_AN_SGMII_MODE_FORCE,
> > +					      BIT(0));
> > +			if (ret)
> > +				return ret;
> > +			ret = regmap_clear_bits(priv->regmap, AN8855_RG_AN_SGMII_MODE_FORCE,
> > +						GENMASK(5, 4));
> > +			if (ret)
> > +				return ret;
> > +
> > +			ret = regmap_clear_bits(priv->regmap, AN8855_RATE_ADP_P0_CTRL_0,
> > +						GENMASK(3, 0));
> > +			if (ret)
> > +				return ret;
> > +		}
> > +
> > +		ret = regmap_set_bits(priv->regmap, AN8855_RATE_ADP_P0_CTRL_0, BIT(28));
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	ret = regmap_set_bits(priv->regmap, AN8855_RG_RATE_ADAPT_CTRL_0, BIT(0));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_set_bits(priv->regmap, AN8855_RG_RATE_ADAPT_CTRL_0, BIT(4));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_set_bits(priv->regmap, AN8855_RG_RATE_ADAPT_CTRL_0, GENMASK(27, 26));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Disable AN */
> > +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> > +		ret = regmap_set_bits(priv->regmap, AN8855_SGMII_REG_AN0,
> > +				      AN8855_SGMII_AN_ENABLE);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		ret = regmap_clear_bits(priv->regmap, AN8855_SGMII_REG_AN0,
> > +					AN8855_SGMII_AN_ENABLE);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> Again, using regmap_update_bits() with a mask and value is probably more
> readable here. This looks like it's twiddling a standard BMCR_ANENABLE
> bit.
> 

Looks like so... the 2 bit are the same... (btw these are the ONLY bits
I have in documentation...)

> > +
> > +	if (interface == PHY_INTERFACE_MODE_SGMII &&
> > +	    neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED) {
> > +		ret = regmap_set_bits(priv->regmap, AN8855_PHY_RX_FORCE_CTRL_0, BIT(4));
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> Eh?
> 
> > +
> > +	/* Force Speed */
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX ||
> > +	    neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> > +		if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +			val = 0x3;
> > +		else
> > +			val = 0x2;
> > +		ret = regmap_set_bits(priv->regmap, AN8855_SGMII_STS_CTRL_0, BIT(2));
> > +		if (ret)
> > +			return ret;
> > +		ret = regmap_update_bits(priv->regmap, AN8855_SGMII_STS_CTRL_0,
> > +					 GENMASK(5, 4),
> > +					 FIELD_PREP(GENMASK(5, 4), val));
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> I don't understand why speed should be forced if inband is enabled or
> we're using 2500base-X.
> 

Again no documentation. From my test with the value set to 0 CPU port
can't send traffic.

I assume this is some kind of max speed value.

3 2500
2 1000
1 100
0 10

Maybe?

> > +
> > +	/* bypass flow control to MAC */
> > +	ret = regmap_write(priv->regmap, AN8855_MSG_RX_LIK_STS_0, 0x01010107);
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_write(priv->regmap, AN8855_MSG_RX_LIK_STS_2, 0x00000EEF);
> > +	if (ret)
> > +		return ret;
> 
> Overall, I'd like more comments about what stuff is doing, especially
> the AN stuff. I can't make head nor tail of e.g. is there any
> advertisement being programmed or not.
> 
> Also, given that this will be called _on its own_ if the user requests
> the inband advertisement to be changed, we don't want to unnecessarily
> disrupt the link. What would happen if this code runs when the link is
> up?

I will try to gather more info about all of these configuration but
honestly chance are very low...

From what I observed these values doesn't interrupt the link but maybe
this code can be moved in different OPs to better handle changing case?

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void an8855_pcs_an_restart(struct phylink_pcs *pcs)
> > +{
> > +	struct an8855_priv *priv = container_of(pcs, struct an8855_priv, pcs);
> > +
> > +	regmap_set_bits(priv->regmap, AN8855_SGMII_REG_AN0,
> > +			AN8855_SGMII_AN_RESTART);
> 
> Again, looks like a standard PHY BMCR.
> 
> I haven't done a full review, but these are just what I've spotted so
> far.
>

Thanks a lot for the initial review, I will check the EEE part better.

Again the big problem is the PCS setup with all that nightmares
values... The Documentation is empty (only AN value are there) and the
values from the SDK by compacting the sgmii(inband and forced)/hsgmii in
one single code by checking all the common piece.

This if why you have all those if condition and all... They comes from
compacting the functions, that part of code is still a blackbox more or
less currently...