Date:   Wed, 20 Nov 2019 15:13:38 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Joergen Andreasen <joergen.andreasen@...rochip.com>,
        "Allan W. Nielsen" <allan.nielsen@...rochip.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        "Y.b. Lu" <yangbo.lu@....com>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 0/2] Convert Ocelot and Felix switches to PHYLINK

On Wed, 20 Nov 2019 at 14:08, Horatiu Vultur
<horatiu.vultur@...rochip.com> wrote:
>
> The 11/19/2019 22:42, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > > Before this commit it was ok to use PHY_INTERFACE_MODE_NA but now that
> > > is not true anymore. In this case we have 4 ports that have phy and
> > > then 6 sfp ports. So I was looking to describe this in DT but without
> > > any success. If you have any advice that would be great.
> >
> > Is it the copper ports causing the trouble, or the SFP?  Ideally, you
> > should describe the SFPs as SFPs. But i don't think the driver has the
> > needed support for that yet. So you might need to use fixed-link for
> > the moment.
>
> It was both of them. So I have done few small changes to these patches.
> - first I added the phy-mode in DT on the interfaces that have a
>   phy(internal or external)
> - add a check for PHY_INTERFACE_MODE_NA before the port is probed so it
>   would not create net device if the phy mode is PHY_INTERFACE_MODE_NA
>   because in that case the phylink was not created.
>
> With these changes now only the ports that have phy are probed. This is
> the same behaviour as before these patches. I have tried to configure
> the sfp ports as fixed-links but unfortunetly it didn't work, I think
> because of some missconfiguration on MAC or SerDes, which I need to
> figure out. But I think this can be fix in a different patch.
>
> I have done few tests and they seem to work fine.
> Here are my changes.
>
> diff --git a/arch/mips/boot/dts/mscc/ocelot_pcb120.dts b/arch/mips/boot/dts/mscc/ocelot_pcb120.dts
> index 33991fd209f5..0800a86b7f16 100644
> --- a/arch/mips/boot/dts/mscc/ocelot_pcb120.dts
> +++ b/arch/mips/boot/dts/mscc/ocelot_pcb120.dts
> @@ -60,18 +60,22 @@
>
>  &port0 {
>         phy-handle = <&phy0>;
> +       phy-mode = "sgmii";
>  };
>
>  &port1 {
>         phy-handle = <&phy1>;
> +       phy-mode = "sgmii";
>  };
>
>  &port2 {
>         phy-handle = <&phy2>;
> +       phy-mode = "sgmii";
>  };
>
>  &port3 {
>         phy-handle = <&phy3>;
> +       phy-mode = "sgmii";
>  };
>
>  &port4 {
> diff --git a/arch/mips/boot/dts/mscc/ocelot_pcb123.dts b/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
> index ef852f382da8..6b0b1fb358ad 100644
> --- a/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
> +++ b/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
> @@ -47,17 +47,21 @@
>  };
>
>  &port0 {
> +       phy-mode = "sgmii";
>         phy-handle = <&phy0>;
>  };
>
>  &port1 {
> +       phy-mode = "sgmii";
>         phy-handle = <&phy1>;
>  };
>
>  &port2 {
> +       phy-mode = "sgmii";
>         phy-handle = <&phy2>;
>  };
>
>  &port3 {
> +       phy-mode = "sgmii";
>         phy-handle = <&phy3>;
>  };
> diff --git a/drivers/net/ethernet/mscc/ocelot_board.c b/drivers/net/ethernet/mscc/ocelot_board.c
> index aecaf4ef6ef4..9dad031900b5 100644
> --- a/drivers/net/ethernet/mscc/ocelot_board.c
> +++ b/drivers/net/ethernet/mscc/ocelot_board.c
> @@ -513,6 +513,10 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
>                 if (IS_ERR(regs))
>                         continue;
>
> +               of_get_phy_mode(portnp, &phy_mode);
> +               if (phy_mode == PHY_INTERFACE_MODE_NA)
> +                       continue;
> +

So this effectively reverts your own patch 4214fa1efffd ("net: mscc:
ocelot: omit error check from of_get_phy_mode")?

>                 err = ocelot_probe_port(ocelot, port, regs);
>                 if (err) {
>                         of_node_put(portnp);
> @@ -523,11 +527,7 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
>                 priv = container_of(ocelot_port, struct ocelot_port_private,
>                                     port);
>
> -               of_get_phy_mode(portnp, &phy_mode);
> -
>                 switch (phy_mode) {
> -               case PHY_INTERFACE_MODE_NA:
> -                       continue;
>                 case PHY_INTERFACE_MODE_SGMII:
>                         break;
>                 case PHY_INTERFACE_MODE_QSGMII:
> @@ -549,20 +549,7 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
>                 }
>
>                 serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
> -               if (IS_ERR(serdes)) {
> -                       err = PTR_ERR(serdes);
> -                       if (err == -EPROBE_DEFER)
> -                               dev_dbg(ocelot->dev, "deferring probe\n");

Why did you remove the probe deferral for the serdes phy?

> -                       else
> -                               dev_err(ocelot->dev,
> -                                       "missing SerDes phys for port%d\n",
> -                                       port);
> -
> -                       of_node_put(portnp);
> -                       goto out_put_ports;
> -               }
> -
> -               if (serdes) {
> +               if (!IS_ERR(serdes)) {
>                         err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET,
>                                                phy_mode);
>                         if (err) {
> --
> 2.17.1
>
>
> >
> >    Andrew
>
> --
> /Horatiu

Thanks,
-Vladimir

Powered by blists - more mailing lists