Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control

From: Colin Foster <colin.foster@in-advantage.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	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>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Lee Jones <lee@kernel.org>
Subject: Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control
Date: Fri, 16 Sep 2022 09:55:55 -0700	[thread overview]
Message-ID: <YySqm8t0pbH4cqR/@euler> (raw)
In-Reply-To: <20220912172109.ezilo6su5w6dihrk@skbuf>

On Mon, Sep 12, 2022 at 05:21:10PM +0000, Vladimir Oltean wrote:
> On Sun, Sep 11, 2022 at 01:02:44PM -0700, Colin Foster wrote:
> > +
> > +#define OCELOT_EXT_MEM_INIT_SLEEP_US	1000
> > +#define OCELOT_EXT_MEM_INIT_TIMEOUT_US	100000
> > +
> > +#define OCELOT_EXT_PORT_MODE_SERDES	(OCELOT_PORT_MODE_SGMII | \
> > +					 OCELOT_PORT_MODE_QSGMII)
> 
> There are places where OCELOT_EXT doesn't make too much sense, like here.
> The capabilities of the SERDES ports do not change depending on whether
> the switch is controlled externally or not. Same for the memory init
> delays. Maybe OCELOT_MEM_INIT_*, OCELOT_PORT_MODE_SERDES etc?
> 
> There are more places as well below in function names, I'll let you be
> the judge if whether ocelot is controlled externally is relevant to what
> they do in any way.
> 
> > +static int ocelot_ext_reset(struct ocelot *ocelot)

I'm looking into these changes now. I was using "ocelot_ext_" not
necessarily as an indication as "this only matters when it is controlled
externally" but rather "this is a function within ocelot_ext.c"

So there's a weird line between what constitutes "external control" vs
"generic"

There are only two things that really matter for external control:
1. The regmaps are set up in a specific way by ocelot-mfd
2. The existence of a DSA CPU port

Going by 1 only - there's basically nothing in
drivers/net/dsa/ocelot/ocelot_ext.c that is inherently "external only".
And that's kindof the point. The only thing that can be done externally
that isn't done internally would be a whole chip reset.

Going by 2 only - the simple fact that it is in drivers/net/dsa/ means
that there is a CPU port, and therefore everything in the file requires
that it is externally controlled.

Unless you're going another way, and you're not actually talking about
"function names" but instead "should this actually live in ocelot_lib"

While I don't think that's what you were directly suggesting - I like
this! ocelot_ext_reset() shouldn't exist - I should move, update, and
utilize ocelot_reset() from drivers/net/ethernet/mscc/ocelot_vsc7514.c.

The ocelot_ext function list will dwindle down to:
*_probe
*_remove
*_shutdown
*_regmap_init
*_phylink_validate