On Mon, 2020-12-21 at 00:35 +0100, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> > + /* 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;
Yes, changed to use the SGMII macroes.
>
>
> > + 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()
There are the ADVERTISE_* macroes in mii.h.
I've changed the code to use them.
>
> > +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?
Yes the SGMII mode in the serdes driver is overclocked.
Nothing in the switch driver needs changing when changing between
speeds 1G/2G5.
>
> > +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)
Changed to const.
>
> What is urg?
urg=urgency (taken directly from the name in the asic).
Another name for speed, or actually how many clockcycles can go by
before the port instance must be served.
>
> > +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?
Yes, applied.
>
> > +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?
This mean that the port is enabled in the queue system - not that it
can participate in switching. When the port joins the bridge, then
forwarding masks will enable swithcing.
I've changed the comment to "Enable port in queue system"
>
> > +/* 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?
The "0xC" is a 2 bit field that chooses what to do with the second BPDU
with the address 01-80-C2-00-00-01 (0x3 chooses the first 01-80-C2-00-
00-00, and 0x300 chooses the third 01-80-C2-00-00-02, etc..). Both
bits high means discard.
I've change the 0xC to a macro called "PAUSE_DISCARD"
>
> Andrew
Thanks
Bjarni Jonasson
Microchip
