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.
On Thu, Feb 16, 2023 at 11:17:48AM +0000, Russell King (Oracle) wrote:
> 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.
I'll remove. Thanks.
> > +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.
Per the other email, it was my misunderstanding - probably from the
unnecessary phylink_create(). V2 will be cleaned up.
...
> > +
> > + 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?
Also in the other email, it is definitely my confusion. I'll get things
straighened out for V2, as these patches seem more complicated than they
need to be.
Thanks again!
>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
@@ -6,6 +6,7 @@
#define ocelot_to_felix(o) container_of((o), struct felix, ocelot)
#define FELIX_MAC_QUIRKS OCELOT_QUIRK_PCS_PERFORMS_RATE_ADAPTATION
+#define OCELOT_EXT_MAC_QUIRKS OCELOT_QUIRK_QSGMII_PORTS_MUST_BE_UP
#define OCELOT_PORT_MODE_NONE 0
#define OCELOT_PORT_MODE_INTERNAL BIT(0)
@@ -4,10 +4,13 @@
*/
#include <linux/mfd/ocelot.h>
+#include <linux/of_net.h>
+#include <linux/phy/phy.h>
#include <linux/phylink.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <soc/mscc/ocelot.h>
+#include <soc/mscc/ocelot_dev.h>
#include <soc/mscc/vsc7514_regs.h>
#include "felix.h"
@@ -16,20 +19,283 @@
#define OCELOT_PORT_MODE_SERDES (OCELOT_PORT_MODE_SGMII | \
OCELOT_PORT_MODE_QSGMII)
+#define phylink_config_to_ocelot_port(config) \
+ container_of(config, struct ocelot_ext_port_priv, phylink_config)
+#define phylink_pcs_to_ocelot_port(pl_pcs) \
+ container_of(pl_pcs, struct ocelot_ext_port_priv, pcs)
+
+struct ocelot_ext_port_priv {
+ struct device_node *node;
+ struct phylink_config phylink_config;
+ struct phylink *phylink;
+ struct ocelot *ocelot;
+ int chip_port;
+ struct phylink_pcs pcs;
+};
+
+struct ocelot_ext_priv {
+ struct felix felix;
+ struct ocelot_ext_port_priv *port_priv[VSC7514_NUM_PORTS];
+};
+
+static struct ocelot_ext_priv *felix_to_ocelot_ext_priv(struct felix *felix)
+{
+ return container_of(felix, struct ocelot_ext_priv, felix);
+}
+
static const u32 vsc7512_port_modes[VSC7514_NUM_PORTS] = {
OCELOT_PORT_MODE_INTERNAL,
OCELOT_PORT_MODE_INTERNAL,
OCELOT_PORT_MODE_INTERNAL,
OCELOT_PORT_MODE_INTERNAL,
- OCELOT_PORT_MODE_NONE,
- OCELOT_PORT_MODE_NONE,
- OCELOT_PORT_MODE_NONE,
- OCELOT_PORT_MODE_NONE,
- OCELOT_PORT_MODE_NONE,
- OCELOT_PORT_MODE_NONE,
- OCELOT_PORT_MODE_NONE,
+ OCELOT_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SERDES,
+ OCELOT_PORT_MODE_SGMII,
+ OCELOT_PORT_MODE_SERDES,
+};
+
+static void ocelot_ext_phylink_of_cleanup(struct ocelot *ocelot)
+{
+ struct felix *felix = ocelot_to_felix(ocelot);
+ struct ocelot_ext_priv *ocelot_ext_priv;
+ int i;
+
+ ocelot_ext_priv = felix_to_ocelot_ext_priv(felix);
+ for (i = 0; i < VSC7514_NUM_PORTS; i++) {
+ struct ocelot_ext_port_priv *port_priv;
+
+ port_priv = ocelot_ext_priv->port_priv[i];
+ if (port_priv && port_priv->node)
+ of_node_put(port_priv->node);
+ }
+}
+
+static void ocelot_ext_phylink_mac_config(struct phylink_config *config,
+ unsigned int link_an_mode,
+ const struct phylink_link_state *state)
+{
+ struct ocelot_ext_port_priv *priv =
+ phylink_config_to_ocelot_port(config);
+ struct ocelot *ocelot = priv->ocelot;
+ int port = priv->chip_port;
+
+ ocelot_phylink_mac_config(ocelot, port, link_an_mode, state);
+}
+
+static void ocelot_ext_phylink_mac_link_down(struct phylink_config *config,
+ unsigned int link_an_mode,
+ phy_interface_t interface)
+{
+ struct ocelot_ext_port_priv *priv =
+ phylink_config_to_ocelot_port(config);
+ struct ocelot *ocelot = priv->ocelot;
+ struct felix *felix = ocelot_to_felix(ocelot);
+ int port = priv->chip_port;
+
+ ocelot_phylink_mac_link_down(ocelot, port, link_an_mode, interface,
+ felix->info->quirks);
+}
+
+static void ocelot_ext_phylink_mac_link_up(struct phylink_config *config,
+ struct phy_device *phydev,
+ unsigned int link_an_mode,
+ phy_interface_t interface,
+ int speed, int duplex,
+ bool tx_pause, bool rx_pause)
+{
+ struct ocelot_ext_port_priv *priv =
+ phylink_config_to_ocelot_port(config);
+ struct ocelot *ocelot = priv->ocelot;
+ struct felix *felix = ocelot_to_felix(ocelot);
+ int port = priv->chip_port;
+
+ ocelot_phylink_mac_link_up(ocelot, port, phydev, link_an_mode,
+ interface, speed, duplex, tx_pause, rx_pause,
+ felix->info->quirks);
+}
+
+static const struct phylink_mac_ops ocelot_ext_phylink_ops = {
+ .validate = phylink_generic_validate,
+ .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);
+ 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);
+}
+
+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);
+ if (IS_ERR(phylink)) {
+ err = PTR_ERR(phylink);
+ dev_err(dev, "Could not create phylink (%pe)\n", phylink);
+ return err;
+ }
+
+ ocelot_ext_port_priv->phylink = phylink;
+
+ err = phylink_of_phy_connect(phylink, portnp, 0);
+ if (err) {
+ dev_err(dev, "Could not connect to PHY: %pe\n", ERR_PTR(err));
+ phylink_destroy(phylink);
+ ocelot_ext_port_priv->phylink = NULL;
+ return err;
+ }
+
+ return 0;
+}
+
static const struct ocelot_ops ocelot_ext_ops = {
.reset = ocelot_reset,
.wm_enc = ocelot_wm_enc,
@@ -48,6 +314,7 @@ static const char * const vsc7512_resource_names[TARGET_MAX] = {
[QS] = "qs",
[QSYS] = "qsys",
[ANA] = "ana",
+ [HSIO] = "hsio",
};
static const struct felix_info vsc7512_info = {
@@ -56,25 +323,32 @@ static const struct felix_info vsc7512_info = {
.map = vsc7514_regmap,
.ops = &ocelot_ext_ops,
.vcap = vsc7514_vcap_props,
+ .quirks = OCELOT_EXT_MAC_QUIRKS,
.num_mact_rows = 1024,
.num_ports = VSC7514_NUM_PORTS,
.num_tx_queues = OCELOT_NUM_TC,
.port_modes = vsc7512_port_modes,
+ .parse_port_node = ocelot_ext_parse_port_node,
+ .phylink_create = ocelot_ext_phylink_create,
+ .phylink_of_cleanup = ocelot_ext_phylink_of_cleanup,
};
static int ocelot_ext_probe(struct platform_device *pdev)
{
+ struct ocelot_ext_priv *ocelot_ext_priv;
struct device *dev = &pdev->dev;
struct dsa_switch *ds;
struct ocelot *ocelot;
struct felix *felix;
int err;
- felix = kzalloc(sizeof(*felix), GFP_KERNEL);
- if (!felix)
+ ocelot_ext_priv = kzalloc(sizeof(*ocelot_ext_priv), GFP_KERNEL);
+ if (!ocelot_ext_priv)
return -ENOMEM;
- dev_set_drvdata(dev, felix);
+ dev_set_drvdata(dev, ocelot_ext_priv);
+
+ felix = &ocelot_ext_priv->felix;
ocelot = &felix->ocelot;
ocelot->dev = dev;
@@ -116,28 +390,43 @@ static int ocelot_ext_probe(struct platform_device *pdev)
static int ocelot_ext_remove(struct platform_device *pdev)
{
- struct felix *felix = dev_get_drvdata(&pdev->dev);
+ struct ocelot_ext_priv *ocelot_ext_priv = dev_get_drvdata(&pdev->dev);
+ struct felix *felix;
- if (!felix)
+ if (!ocelot_ext_priv)
return 0;
+ felix = &ocelot_ext_priv->felix;
+
dsa_unregister_switch(felix->ds);
kfree(felix->ds);
- kfree(felix);
+ kfree(ocelot_ext_priv);
return 0;
}
static void ocelot_ext_shutdown(struct platform_device *pdev)
{
- struct felix *felix = dev_get_drvdata(&pdev->dev);
+ struct ocelot_ext_priv *ocelot_ext_priv = dev_get_drvdata(&pdev->dev);
+ struct ocelot_ext_port_priv *port_priv;
+ struct felix *felix;
+ int i;
- if (!felix)
+ if (!ocelot_ext_priv)
return;
+ felix = &ocelot_ext_priv->felix;
+
dsa_switch_shutdown(felix->ds);
+ for (i = 0; i < felix->info->num_ports; i++) {
+ port_priv = ocelot_ext_priv->port_priv[i];
+
+ if (port_priv && port_priv->phylink)
+ phylink_destroy(port_priv->phylink);
+ }
+
dev_set_drvdata(&pdev->dev, NULL);
}