| 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: <Y+4Q3PDlj+lVQAPx@shell.armlinux.org.uk>
Date: Thu, 16 Feb 2023 11:17:48 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Colin Foster <colin.foster@...advantage.com>
Cc: linux-phy@...ts.infradead.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org,
Kishon Vijay Abraham I <kishon@...nel.org>,
Vinod Koul <vkoul@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
"David S. Miller" <davem@...emloft.net>,
Florian Fainelli <f.fainelli@...il.com>,
Andrew Lunn <andrew@...n.ch>, UNGLinuxDriver@...rochip.com,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Claudiu Manoil <claudiu.manoil@....com>,
Vladimir Oltean <vladimir.oltean@....com>,
Lee Jones <lee@...nel.org>
Subject: Re: [RFC v1 net-next 7/7] net: dsa: ocelot_ext: add support for
external phys
Hi Colin,
On Wed, Feb 15, 2023 at 11:53:21PM -0800, Colin Foster wrote:
> +static const struct phylink_mac_ops ocelot_ext_phylink_ops = {
> + .validate = phylink_generic_validate,
There is no need to set this anymore.
> + .mac_config = ocelot_ext_phylink_mac_config,
> + .mac_link_down = ocelot_ext_phylink_mac_link_down,
> + .mac_link_up = ocelot_ext_phylink_mac_link_up,
> +};
> +
> +static void ocelot_ext_pcs_get_state(struct phylink_pcs *pcs,
> + struct phylink_link_state *state)
> +{
> + struct ocelot_ext_port_priv *port_priv =
> + phylink_pcs_to_ocelot_port(pcs);
> +
> + /* TODO: Determine state from hardware? */
> +}
> +
> +static int ocelot_ext_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> + phy_interface_t interface,
> + const unsigned long *advertising,
> + bool permit_pause_to_mac)
> +{
> + struct ocelot_ext_port_priv *port_priv =
> + phylink_pcs_to_ocelot_port(pcs);
> +
> + switch (interface) {
> + case PHY_INTERFACE_MODE_QSGMII:
> + ocelot_ext_phylink_mac_config(&port_priv->phylink_config, mode,
> + NULL);
Why are you calling a "mac" operation from a "pcs" operation? If this
PCS is attached to the same phylink instance as the MAC, you'll get
the .mac_config method called along with the .pcs_config, so calling
one from the other really isn't necessary.
> + break;
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static void ocelot_ext_pcs_an_restart(struct phylink_pcs *pcs)
> +{
> + /* TODO: Restart autonegotiaion process */
> +}
> +
> +static void ocelot_ext_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
> + phy_interface_t interface, int speed,
> + int duplex)
> +{
> + struct ocelot_ext_port_priv *port_priv =
> + phylink_pcs_to_ocelot_port(pcs);
> +
> + ocelot_ext_phylink_mac_link_up(&port_priv->phylink_config, NULL, mode,
> + interface, speed, duplex, false, false);
Same here... and I fail to see why any of these need to be implemented
or what the purpose of providing this pcs code is.
> +}
> +
> +static const struct phylink_pcs_ops ocelot_ext_pcs_ops = {
> + .pcs_get_state = ocelot_ext_pcs_get_state,
> + .pcs_config = ocelot_ext_pcs_config,
> + .pcs_an_restart = ocelot_ext_pcs_an_restart,
> + .pcs_link_up = ocelot_ext_pcs_link_up,
> };
>
> +static int ocelot_ext_parse_port_node(struct ocelot *ocelot,
> + struct device_node *ports_node,
> + phy_interface_t phy_mode, int port)
> +{
> + struct ocelot_ext_port_priv *ocelot_ext_port_priv;
> + struct felix *felix = ocelot_to_felix(ocelot);
> + struct ocelot_ext_priv *ocelot_ext_priv;
> +
> + ocelot_ext_priv = felix_to_ocelot_ext_priv(felix);
> +
> + ocelot_ext_port_priv = devm_kzalloc(ocelot->dev,
> + sizeof(*ocelot_ext_port_priv),
> + GFP_KERNEL);
> + if (!ocelot_ext_port_priv)
> + return -ENOMEM;
> +
> + ocelot_ext_port_priv->ocelot = ocelot;
> + ocelot_ext_port_priv->chip_port = port;
> + ocelot_ext_port_priv->pcs.ops = &ocelot_ext_pcs_ops;
> +
> + if (!felix->pcs)
> + felix->pcs = devm_kcalloc(ocelot->dev, felix->info->num_ports,
> + sizeof(struct phylink_pcs *),
> + GFP_KERNEL);
> +
> + if (!felix->pcs)
> + return -ENOMEM;
> +
> + felix->pcs[port] = &ocelot_ext_port_priv->pcs;
> +
> + ocelot_ext_priv->port_priv[port] = ocelot_ext_port_priv;
> +
> + ocelot_ext_port_priv->node = of_node_get(ports_node);
> +
> + return 0;
> +}
> +
> +static int ocelot_ext_phylink_create(struct ocelot *ocelot, int port)
> +{
> + struct ocelot_ext_port_priv *ocelot_ext_port_priv;
> + struct felix *felix = ocelot_to_felix(ocelot);
> + struct ocelot_ext_priv *ocelot_ext_priv;
> + struct device *dev = ocelot->dev;
> + struct ocelot_port *ocelot_port;
> + struct device_node *portnp;
> + phy_interface_t phy_mode;
> + struct phylink *phylink;
> + int err;
> +
> + ocelot_ext_priv = felix_to_ocelot_ext_priv(felix);
> + ocelot_port = ocelot->ports[port];
> + ocelot_ext_port_priv = ocelot_ext_priv->port_priv[port];
> +
> + if (!ocelot_ext_port_priv)
> + return 0;
> +
> + portnp = ocelot_ext_port_priv->node;
> + phy_mode = ocelot_port->phy_mode;
> +
> + /* Break out early if we're internal...? */
> + if (phy_mode == PHY_INTERFACE_MODE_INTERNAL)
> + return 0;
> +
> + if (phy_mode == PHY_INTERFACE_MODE_QSGMII)
> + ocelot_port_rmwl(ocelot_port, 0,
> + DEV_CLOCK_CFG_MAC_TX_RST |
> + DEV_CLOCK_CFG_MAC_RX_RST,
> + DEV_CLOCK_CFG);
> +
> + if (phy_mode != PHY_INTERFACE_MODE_INTERNAL) {
> + struct phy *serdes = of_phy_get(portnp, NULL);
> +
> + if (IS_ERR(serdes)) {
> + err = PTR_ERR(serdes);
> + dev_err_probe(dev, err,
> + "missing SerDes phys for port %d\n",
> + port);
> + return err;
> + }
> +
> + err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET, phy_mode);
> + of_phy_put(serdes);
> + if (err) {
> + dev_err(dev,
> + "Could not set SerDes mode on port %d: %pe\n",
> + port, ERR_PTR(err));
> + return err;
> + }
> + }
> +
> + ocelot_ext_port_priv->phylink_config.dev = dev;
> + ocelot_ext_port_priv->phylink_config.type = PHYLINK_DEV;
> + ocelot_ext_port_priv->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
> + MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD;
> +
> + __set_bit(ocelot_port->phy_mode,
> + ocelot_ext_port_priv->phylink_config.supported_interfaces);
> +
> + phylink = phylink_create(&ocelot_ext_port_priv->phylink_config,
> + of_fwnode_handle(portnp),
> + phy_mode, &ocelot_ext_phylink_ops);
I'm confused. DSA already sets up a phylink instance per port, so why
do you need another one?
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists