On Thu, Jun 11, 2020 at 11:58:43AM +0300, Vladimir Oltean wrote:
> Hi Jonathan,
>
> On Wed, 10 Jun 2020 at 23:19, Jonathan McDowell <nood...@earth.li> wrote:
> >
> > Update the driver to use the new PHYLINK callbacks, removing the
> > legacy adjust_link callback.
> >
> > Signed-off-by: Jonathan McDowell <nood...@earth.li>
...
> > static void
> > -qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
> > +qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int
> > mode,
> > + const struct phylink_link_state *state)
> > {
> > struct qca8k_priv *priv = ds->priv;
> > u32 reg;
> >
> > - /* Force fixed-link setting for CPU port, skip others. */
> > - if (!phy_is_pseudo_fixed_link(phy))
> > + switch (port) {
> > + case 0: /* 1st CPU port */
> > + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> > + state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> > + state->interface != PHY_INTERFACE_MODE_SGMII)
> > + return;
> > +
> > + reg = QCA8K_REG_PORT0_PAD_CTRL;
> > + break;
> > + case 1:
> > + case 2:
> > + case 3:
> > + case 4:
> > + case 5:
> > + /* Internal PHY, nothing to do */
> > + return;
> > + case 6: /* 2nd CPU port / external PHY */
> > + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> > + state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> > + state->interface != PHY_INTERFACE_MODE_SGMII &&
> > + state->interface != PHY_INTERFACE_MODE_1000BASEX)
> > + return;
> > +
> > + reg = QCA8K_REG_PORT6_PAD_CTRL;
> > + break;
> > + default:
> > + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__,
> > port);
> > + return;
> > + }
> > +
> > + if (port != 6 && phylink_autoneg_inband(mode)) {
> > + dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
> > + __func__);
> > + return;
> > + }
> > +
> > + switch (state->interface) {
> > + case PHY_INTERFACE_MODE_RGMII:
> > + /* RGMII mode means no delay so don't enable the delay */
> > + qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN);
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII_ID:
> > + /* RGMII_ID needs internal delay. This is enabled through
> > + * PORT5_PAD_CTRL for all ports, rather than individual port
> > + * registers
> > + */
> > + qca8k_write(priv, reg,
> > + QCA8K_PORT_PAD_RGMII_EN |
> > + QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
> > + QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
>
> 3 points here:
> - Should you prevalidate the device tree bindings that in case rgmii*
> mode are used, same delay settings are applied to all ports?
> - Can any RGMII port be connected to a PHY? If it can, won't the PHY
> enable delays too for RGMII_ID? Will the link work in that case?
> - Should you treat RGMII_TX_DELAY and RGMII_RX_DELAY independently for
> the case where there may be PCB traces?
The intent with this patch was to pull out the conversion to PHYLINK to
be stand-alone, with no functional changes, as request by Andrew. I
think there's room for some future clean-up here around the RGMII
options, but my main purpose in this patch set is to improve the SGMII
portion which my hardware uses that doesn't work with mainline.
> > +static void
> > +qca8k_phylink_validate(struct dsa_switch *ds, int port,
> > + unsigned long *supported,
> > + struct phylink_link_state *state)
> > +{
> > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > +
> > + switch (port) {
> > + case 0: /* 1st CPU port */
> > + if (state->interface != PHY_INTERFACE_MODE_NA &&
> > + state->interface != PHY_INTERFACE_MODE_RGMII &&
> > + state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> > + state->interface != PHY_INTERFACE_MODE_SGMII)
> > + goto unsupported;
> > break;
> > - case 100:
> > - reg = QCA8K_PORT_STATUS_SPEED_100;
> > + case 1:
> > + case 2:
> > + case 3:
> > + case 4:
> > + case 5:
> > + /* Internal PHY */
> > + if (state->interface != PHY_INTERFACE_MODE_NA &&
> > + state->interface != PHY_INTERFACE_MODE_GMII)
> > + goto unsupported;
> > break;
> > - case 1000:
> > - reg = QCA8K_PORT_STATUS_SPEED_1000;
> > + case 6: /* 2nd CPU port / external PHY */
> > + if (state->interface != PHY_INTERFACE_MODE_NA &&
> > + state->interface != PHY_INTERFACE_MODE_RGMII &&
> > + state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> > + state->interface != PHY_INTERFACE_MODE_SGMII &&
> > + state->interface != PHY_INTERFACE_MODE_1000BASEX)
> > + goto unsupported;
> > break;
> > default:
> > - dev_dbg(priv->dev, "port%d link speed %dMbps not
> > supported.\n",
> > - port, phy->speed);
> > + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__,
> > port);
>
> phylink has a better validation error message than this, I'd say this
> is unnecessary.
Ok.
> > +static void
> > +qca8k_phylink_mac_link_up(struct dsa_switch *ds, int port, unsigned int
> > mode,
> > + phy_interface_t interface, struct phy_device
> > *phydev,
> > + int speed, int duplex, bool tx_pause, bool
> > rx_pause)
> > +{
> > + struct qca8k_priv *priv = ds->priv;
> > + u32 reg;
> > +
> > + if (phylink_autoneg_inband(mode)) {
> > + reg = QCA8K_PORT_STATUS_LINK_AUTO;
> > + } else {
> > + switch (speed) {
> > + case SPEED_10:
> > + reg = QCA8K_PORT_STATUS_SPEED_10;
> > + break;
> > + case SPEED_100:
> > + reg = QCA8K_PORT_STATUS_SPEED_100;
> > + break;
> > + case SPEED_1000:
> > + reg = QCA8K_PORT_STATUS_SPEED_1000;
> > + break;
> > + default:
> > + reg = QCA8K_PORT_STATUS_LINK_AUTO;
> > + break;
> > + }
> > +
> > + if (duplex == DUPLEX_FULL)
> > + reg |= QCA8K_PORT_STATUS_DUPLEX;
> > +
> > + if (rx_pause | dsa_is_cpu_port(ds, port))
>
> I think it is odd to do bitwise OR on booleans.
I agree; will fix in the next spin.
J.
--
I get the feeling that I've been cheated.
