Re: [RFC PATCH v2 4/8] net: sparx5: add port module support

[Andrew Lunn <andrew@lunn.ch>]

> +	/* Aneg complete provides more information  */
> +	if (DEV2G5_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(value)) {
> +		if (port->conf.portmode == PHY_INTERFACE_MODE_SGMII) {
> +			/* SGMII cisco aneg */
> +			u32 spdvalue = ((lp_abil >> 10) & 3);

			u32 spdvalue = lp_abil & LPA_SGMII_SPD_MASK;

> +
> +			status->link = !!((lp_abil >> 15) == 1) && status->link;

Maybe

			status->link = !!((lp_abil & LPA_SGMII_LINK) && status->link;

> +			status->an_complete = true;
> +			status->duplex = (lp_abil >> 12) & 0x1 ?  DUPLEX_FULL : DUPLEX_HALF;

			status->duplex = (lp_abil & LPA_SGMII_FULL_DUPLEX) ?  DUPLEX_FULL : DUPLEX_HALF;


> +			if (spdvalue == LPA_SGMII_10)
> +				status->speed = SPEED_10;
> +			else if (spdvalue == LPA_SGMII_100)
> +				status->speed = SPEED_100;
> +			else
> +				status->speed = SPEED_1000;

I wonder if there is a helper for this?


> +		} else {
> +			/* Clause 37 Aneg */
> +			status->link = !((lp_abil >> 12) & 3) && status->link;
> +			status->an_complete = true;
> +			status->duplex = ((lp_abil >> 5) & 1) ? DUPLEX_FULL : DUPLEX_UNKNOWN;
> +			if ((lp_abil >> 8) & 1) /* symmetric pause */
> +				status->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
> +			if (lp_abil & (1 << 7)) /* asymmetric pause */
> +				status->pause |= MLO_PAUSE_RX;
> +		}

Please check if there are any standard #defines you can use for
this. Russell King has done some work for clause 37. Maybe there is
some code in phy_driver.c you can use? phylink_decode_sgmii_word()

> +static int sparx5_port_verify_speed(struct sparx5 *sparx5,
> +				    struct sparx5_port *port,
> +				    struct sparx5_port_config *conf)
> +{
> +	case PHY_INTERFACE_MODE_SGMII:
> +		if (conf->speed != SPEED_1000 &&
> +		    conf->speed != SPEED_100 &&
> +		    conf->speed != SPEED_10 &&
> +		    conf->speed != SPEED_2500)
> +			return sparx5_port_error(port, conf, SPX5_PERR_SPEED);

Is it really SGMII over clocked at 2500? Or 2500BaseX?

> +static int sparx5_port_fifo_sz(struct sparx5 *sparx5,
> +			       u32 portno, u32 speed)
> +{
> +	u32 sys_clk    = sparx5_clk_period(sparx5->coreclock);
> +	u32 mac_width  = 8;
> +	u32 fifo_width = 16;
> +	u32 addition   = 0;
> +	u32 mac_per    = 6400, tmp1, tmp2, tmp3;
> +	u32 taxi_dist[SPX5_PORTS_ALL] = {

const. As it is at the moment, it gets copied onto the stack, so it
can be modified. Const i guess prevents that copy?

> +		6, 8, 10, 6, 8, 10, 6, 8, 10, 6, 8, 10,
> +		4, 4, 4, 4,
> +		11, 12, 13, 14, 15, 16, 17, 18,
> +		11, 12, 13, 14, 15, 16, 17, 18,
> +		11, 12, 13, 14, 15, 16, 17, 18,
> +		11, 12, 13, 14, 15, 16, 17, 18,
> +		4, 6, 8, 4, 6, 8, 6, 8,
> +		2, 2, 2, 2, 2, 2, 2, 4, 2
> +	};

> +static int sparx5_port_fwd_urg(struct sparx5 *sparx5, u32 speed)

What is urg? 

> +static u16 sparx5_get_aneg_word(struct sparx5_port_config *conf)
> +{
> +	if (conf->portmode == PHY_INTERFACE_MODE_1000BASEX) /* cl-37 aneg */
> +		return ((1 << 14) | /* ack */
> +		((conf->pause ? 1 : 0) << 8) | /* asymmetric pause */
> +		((conf->pause ? 1 : 0) << 7) | /* symmetric pause */
> +		(1 << 5)); /* FDX only */

ADVERTISE_LPACK, ADVERTISE_PAUSE_ASYM, ADVERTISE_PAUSE_CAP, ADVERTISE_1000XFULL?

> +int sparx5_port_config(struct sparx5 *sparx5,
> +		       struct sparx5_port *port,
> +		       struct sparx5_port_config *conf)
> +{
> +	bool high_speed_dev = sparx5_is_high_speed_device(conf);
> +	int err, urgency, stop_wm;
> +
> +	err = sparx5_port_verify_speed(sparx5, port, conf);
> +	if (err)
> +		return err;
> +
> +	/* high speed device is already configured */
> +	if (!high_speed_dev)
> +		sparx5_port_config_low_set(sparx5, port, conf);
> +
> +	/* Configure flow control */
> +	err = sparx5_port_fc_setup(sparx5, port, conf);
> +	if (err)
> +		return err;
> +
> +	/* Set the DSM stop watermark */
> +	stop_wm = sparx5_port_fifo_sz(sparx5, port->portno, conf->speed);
> +	spx5_rmw(DSM_DEV_TX_STOP_WM_CFG_DEV_TX_STOP_WM_SET(stop_wm),
> +		 DSM_DEV_TX_STOP_WM_CFG_DEV_TX_STOP_WM,
> +		 sparx5,
> +		 DSM_DEV_TX_STOP_WM_CFG(port->portno));
> +
> +	/* Enable port forwarding */
> +	urgency = sparx5_port_fwd_urg(sparx5, conf->speed);
> +	spx5_rmw(QFWD_SWITCH_PORT_MODE_PORT_ENA_SET(1) |
> +		 QFWD_SWITCH_PORT_MODE_FWD_URGENCY_SET(urgency),
> +		 QFWD_SWITCH_PORT_MODE_PORT_ENA |
> +		 QFWD_SWITCH_PORT_MODE_FWD_URGENCY,
> +		 sparx5,
> +		 QFWD_SWITCH_PORT_MODE(port->portno));

What does it mean by port forwarding? By default, packets should only
go to the CPU, until the port is added to a bridge. I've not thought
much about L3, since DSA so far only has L2 switches, but i guess you
don't need to enable L3 forwarding until a route out the port has been
added?

> +/* Initialize port config to default */
> +int sparx5_port_init(struct sparx5 *sparx5,
> +		     struct sparx5_port *port,
> +		     struct sparx5_port_config *conf)
> +{
> +	/* Discard pause frame 01-80-C2-00-00-01 */
> +	spx5_wr(0xC, sparx5, ANA_CL_CAPTURE_BPDU_CFG(port->portno));

The comment is about pause frames, but the macro contain BPDU?

    Andrew