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