| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200611110439.GV311@earth.li>
Date: Thu, 11 Jun 2020 12:04:39 +0100
From: Jonathan McDowell <noodles@...th.li>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Russell King - ARM Linux admin <linux@...linux.org.uk>,
Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
netdev <netdev@...r.kernel.org>,
lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB
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 <noodles@...th.li> wrote:
> >
> > Update the driver to use the new PHYLINK callbacks, removing the
> > legacy adjust_link callback.
> >
> > Signed-off-by: Jonathan McDowell <noodles@...th.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.
Powered by blists - more mailing lists