Date:   Thu, 16 Feb 2023 16:42:06 -0800
From:   Colin Foster <colin.foster@in-advantage.com>
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
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+7NXoFTBYVG3/+b@colin-ia-desktop>
References: <20230216075321.2898003-1-colin.foster@in-advantage.com>
 <Y+4eLmpX9oX3JBVJ@shell.armlinux.org.uk>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y+4eLmpX9oX3JBVJ@shell.armlinux.org.uk>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?RyUgqs6Gg/n1cX8uXgBIGLUyhZ/jZTvz6f67ryRWuQ22buN8RaAAOqlCctnm?=
 =?us-ascii?Q?lo0oQaoU+M0Guv+I4IVDHSVX7eDNWIDEO8NGgPLSjjSipqFJnaRvBcUpmCnk?=
 =?us-ascii?Q?1arHJHGxNxta4yEdbn/00fbq2RN7JV2H4VqmhEpNq8897lcOvjgHfQNF+ChE?=
 =?us-ascii?Q?74dVbJPJRQ48RhopIsF69NKIT9GvZw6Ut4tLpd/Qrwnq/yjIqzYNbTHkJ6XU?=
 =?us-ascii?Q?FALSJj6Q8vYK45adAFz/PTvH1l3acXTGIsgY1+67DtWNQcN0y9B+oMJAYvye?=
 =?us-ascii?Q?72kAnD3W3TdtTk0azE2e6Z3qUyVGjjG867FC3I6nTEfZeEUQZKdN/fwWY63o?=
 =?us-ascii?Q?4tFdpP1zzOEx7+8kJ9lCCXKDDG4SfbvG/bfXOAqwCxBDJzOcA3DDqQUnhglg?=
 =?us-ascii?Q?3HdNM1nHKIxvkL7S16awarcEiv8XHXfFMmWpeLd9KQrJG8eZN6OTOv/MJu7x?=
 =?us-ascii?Q?Q0lDkIhXxqftqgB9T7pBBERBpTFPU9dLFY7hzthX27nK0oV6L/6vXRXZLgvf?=
 =?us-ascii?Q?/RVMAszO93m4shog/HZ+xydo9Z8dIDl2sqgHtgHZdjqtQjfIMilPA6u5pWLi?=
 =?us-ascii?Q?UZuk8Di/HZx0FG6CK3HQ6ifUtUnmOl2y523oLMOsvk7DeHzBH/wG6pXMZiou?=
 =?us-ascii?Q?UNJiq6v08cwdCfRJ55iHSFvy5VxjCFVSzDxQz82ePQw8P4taDXSvdMshNb7K?=
 =?us-ascii?Q?Egr6Hg62uuH9tHpUgPJx48ImF1bF3aY0Xh9u+OOP0pfxhivJs81oPOwqEMYK?=
 =?us-ascii?Q?wvJRzWcyq1lENYX+78gDA4RnU7b9+tRlp8IUcbjKV9h/UPTmfqKtNvFu5lUZ?=
 =?us-ascii?Q?EGzFn1Fepqo0oqy9PjrRLOiIuYg+GJeQqsQyD2LLuJVpesh1RDjvkb3Vi5z9?=
 =?us-ascii?Q?JDKchT/15XMsuD09CCbyPmyhKDUdAoXECs6rvhC7fFmNpvz+Db+B1cqCEErG?=
 =?us-ascii?Q?IzIweQPB/cdO3iOZS5IeyF/zk/f0pEtVTPZjDkz+z+LrROIdsVnXOK8tDtwk?=
 =?us-ascii?Q?rfcrT2Pzq/gvfpQdH36jHyX/QuhKQRiSb4HFkt0ycEMIV/WdG82iveyeZKf3?=
 =?us-ascii?Q?3CHnxEoyxSPvxWGOyJz71gouM1wQJ77lMSo62QQnsT7EJGidX2T5t/4RHlUN?=
 =?us-ascii?Q?0vXGY07SCKKNhsYMqWNM2f4mOPI8aBqB/Xh5aaVJTN/us3rkUlHl3xnVlHjc?=
 =?us-ascii?Q?pB4iHnCUJ3jCsg3h6WSA8F1Hfn7rHp7gg3cd+W6Nkh7WplpOgWWGOFc3IJYK?=
 =?us-ascii?Q?A4wJGBCs/ikdnRXCfUxvEy/21mjusdyIkVvTSe5j8ml+8m9u5rasRe9PV2Lo?=
 =?us-ascii?Q?J3tbR0Aag6vW3qMi1/CNBIoQ9HnrmoHxmWa5ySSja0OYVqj3US8rLhR8cw3E?=
 =?us-ascii?Q?/bIFADPTMMf4N8SDY8wD0KQGFYU2smDxCbz5Xnp6jQ5Jip53J6/V5GCrwiQS?=
 =?us-ascii?Q?U6ketAC5thIRs8L4mrtKJ069BVlardvyHIanDxYtrpyKnNxHeAu8Qanq6bzQ?=
 =?us-ascii?Q?2EM8Moh7+EdKrXxdgkkP13hzx9ss+93jQFUkZXq0LdXFQclpilMYxFW/Frc1?=
 =?us-ascii?Q?LU69Lyg2GKCWaKW6gCVILbUoZCazq3HP8hFVQYoPXtRd+Ds7s6JFhpb2Wdiu?=
 =?us-ascii?Q?GcU0KClJiEObQD3OQCTT32Y=3D?=
X-OriginatorOrg: in-advantage.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ed64e078-d037-4c36-d105-08db107fcdf0
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2351.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Feb 2023 00:42:11.0996
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 48e842ca-fbd8-4633-a79d-0c955a7d3aae
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 6Ln1qlzW7v74DIIsqpe8HTN8wm/2nvgn8BvLAwPnm3Cvln188jO6HcyRJm5y25JNcyrVnOtk2anfStUAG/30Z/a+eZpilg5o7Qxelo0Jfhs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR10MB4796
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Russell,

On Thu, Feb 16, 2023 at 12:14:38PM +0000, Russell King (Oracle) wrote:
> 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.

This is essentially the feedback I was looking for. "This looks wrong"
which means I'll take a step back.

> 
> 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'll have to go through it again to remember exactly what I was up
against - it was a while ago now. All of the logic was based on the
logic in ocelot_port_phylink_create() - which is part of the vsc7514
switchdev implementation (a chip that is essentially identical, except
for an internal MIPS instead of external SPI control)

I believe the main gotcha was that control over the phy itself, by way
of phy_set_mode_ext(). That needed the 'struct device_node *portnp'


.... Keeps looking ....

Ahh, yes. Regmaps and regfields aren't initialized at the time of
dt parsing in felix. And the MDIO bus isn't allocated until after that.
That's the reason for patch 6 parse_port_node() - I need the tree node
to get MDIO access to the phy, which I don't have until I'm done parsing
the tree...

There might be a cleaner way for me to do that. I'm tiptoeing a little
bit to avoid any regressions with the felix_vsc9959 or seville_vsc9953.

> 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.

I'm not convinced I don't need PCS here, and just have things working
wrong.

The configuration looks like this:


		|------------------------------------------------|
		| CPU                                            |
		|------------------------------------------------|
            |
           SPI
            |
		|------------------------------------------------|
		| VSC7512                                        |
		|------------------------------------------------|
            ||        ||         ||         ||      |
		|-------|  |-------|  |-------|  |-------|  |
		| port4 |  | port5 |  | port6 |  | port7 |  |
		|-------|  |-------|  |-------|  |-------|  |
                      ||                            |
                    QSGMII                         MDIO
                      ||                            |
		|------------------------------------------------|
		| VSC8512                                        |
		|------------------------------------------------|
            ||        ||         ||         ||
		|-------|  |-------|  |-------|  |-------|
		| sw0p4 |  | sw0p5 |  | sw0p6 |  | sw0p7 |
		|-------|  |-------|  |-------|  |-------|


Would phylink_pcs need to get involved in the QSGMII link at all, or
should the phylnk from dsa_port_phylink_create() be all that's needed?


> 
> 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.

Yes, I'll look at how I can clean things up.

I greatly appreciate your feedback!

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