Date:   Thu, 16 Feb 2023 12:14:38 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Colin Foster <colin.foster@in-advantage.com>
Cc:     linux-phy@lists.infradead.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Kishon Vijay Abraham I <kishon@kernel.org>,
        Vinod Koul <vkoul@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>, UNGLinuxDriver@microchip.com,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        Lee Jones <lee@kernel.org>
Subject: Re: [RFC v1 net-next 0/7] add support for ocelot external ports
Message-ID: <Y+4eLmpX9oX3JBVJ@shell.armlinux.org.uk>
References: <20230216075321.2898003-1-colin.foster@in-advantage.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230216075321.2898003-1-colin.foster@in-advantage.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Wed, Feb 15, 2023 at 11:53:14PM -0800, Colin Foster wrote:
> Part 3 will, at a minimum, add support for ports 4-7, which are
> configured to use QSGMII to an external phy (Return Of The QSGMII). With
> any luck, and some guidance, support for SGMII, SFPs, etc. will also be
> part of this series.
> 
> 
> This patch series is absolutely an RFC at this point. While all 8 copper
> ports on the VSC7512 are currently functional, I recognize there are a
> couple empty function callbacks in the last patch that likely need to be
> implemented.
> 
...
> 
> Also, with patch 7 ("net: dsa: ocelot_ext: add support for external phys")
> my basis was the function mscc_ocelot_init_ports(), but there were several
> changes I had to make for DSA / Phylink. Are my implementations of
> ocelot_ext_parse_port_node() and ocelot_ext_phylink_create() barking up
> the right tree?

DSA already creates phylink instances per DSA port, and provides many
of the phylink MAC operations to the DSA driver via the .phylink_*
operations in the dsa_switch_ops structure, and this phylink instance
should be used for managing the status and configuring the port
according to phylink's callbacks. The core felix code already makes
use of this, implementing the mac_link_down() and mac_link_up()
operations to handle when the link comes up or goes down.

I don't see why one would need to create a separate phylink instance
to support external PHYs, SFPs, etc on a DSA switch. The phylink
instance created by DSA is there for the DSA driver to make use of
for the port, and should be sufficient for this.

I think if you use the DSA-created phylink instance, then you don't
need any of patch 6. I'm not yet convinced that you need anything
from patch 7, but maybe you could explain what patch 7 provides that
the existing felix phylink implementation doesn't already provide.

I do get the impression that the use of the PCS instance in patch 7
is an attempt to work around the use of a private instance,
redirecting the pcs_config and pcs_link_up methods to the
corresponding MAC operations as a workaround for having the private
instance.

It looks like you need to hook into the mac_config(), mac_link_up()
and mac_link_down() methods at the core felix layer, so I would
suggest looking at the felix_info structure, adding methods there for
each of these, and arranging for the core felix code to forward these
calls down to the implementation as required.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!