Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Russell King - ARM Linux admin]

On Sat, Jun 06, 2020 at 08:49:16AM +0100, Jonathan McDowell wrote:
> On Fri, Jun 05, 2020 at 08:38:43PM +0200, Andrew Lunn wrote:
> > On Fri, Jun 05, 2020 at 07:10:58PM +0100, Jonathan McDowell wrote:
> > > The QCA8337(N) has an SGMII port which can operate in MAC, PHY or BASE-X
> > > mode depending on what it's connected to (e.g. CPU vs external PHY or
> > > SFP). At present the driver does no configuration of this port even if
> > > it is selected.
> > > 
> > > Add support for making sure the SGMII is enabled if it's in use, and
> > > device tree support for configuring the connection details.
> > 
> > It is good to include Russell King in Cc: for patches like this.
> 
> No problem, I can keep him in the thread; I used get_maintainer for the
> initial set of people/lists to copy.

get_maintainer is not always "good" at selecting the right people,
especially when your patches don't match the criteria; MAINTAINERS
contains everything that is sensible, but Andrew is suggesting that
you copy me because in his opinion, you should be using phylink -
and that's something that you can't encode into a program.

Note that I haven't seen your patches.

> > Also, netdev is closed at the moment, so please post patches as RFC.
> 
> "closed"? If you mean this won't get into 5.8 then I wasn't expecting it
> to, I'm aware the merge window for that is already open.

See https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
"How often do changes from these trees make it to the mainline Linus
tree?"

> > It sounds like the hardware has a PCS which can support SGMII or
> > 1000BaseX. phylink will tell you what mode to configure it to. e.g. A
> > fibre SFP module will want 1000BaseX. A copper SFP module will want
> > SGMII. A switch is likely to want 1000BaseX. A PHY is likely to want
> > SGMII. So remove the "sgmii-mode" property and configure it as phylink
> > is requesting.
> 
> It's more than SGMII or 1000BaseX as I read it. The port can act as if
> it's talking to an SGMII MAC, i.e. a CPU, or an SGMII PHY, i.e. an
> external PHY, or in BaseX mode for an SFP. I couldn't figure out a way
> in the current framework to automatically work out if I wanted PHY or
> MAC mode. For the port tagged CPU I can assume MAC mode, but a port that
> doesn't have that might still be attached to the CPU rather than an
> external PHY.

That depends what you're connected to. Some people call the two sides
of SGMII "System side" and "Media side". System side is where you're
receiving the results of AN from a PHY. Media side is where you're
telling the partner what you want it to do.

Media side is only useful if you're connected to another MAC, and
unless you have a requirement for it, I would suggest not implementing
that - you could come up with something using fixed-link, or it may
need some other model if the settings need to change.  That depends on
the application.

> > What exactly does sgmii-delay do?
> 
> As per the device tree documentation update I sent it delays the SGMII
> clock by 2ns. From the data sheet:
> 
> SGMII_SEL_CLK125M	sgmii_clk125m_rx_delay is delayed by 2ns

This sounds like a new world of RGMII delay pain but for SGMII. There
is no mention of "delay" in the SGMII v1.8 specification, so I guess
it's something the vendor is doing. Is this device capable of
recovering the clock from a single serdes pair carrying the data,
or does it always require the separate clock?

> > > +#define QCA8K_REG_SGMII_CTRL				0x0e0
> > > +#define   QCA8K_SGMII_EN_PLL				BIT(1)
> > > +#define   QCA8K_SGMII_EN_RX				BIT(2)
> > > +#define   QCA8K_SGMII_EN_TX				BIT(3)
> > > +#define   QCA8K_SGMII_EN_SD				BIT(4)
> > > +#define   QCA8K_SGMII_CLK125M_DELAY			BIT(7)
> > > +#define   QCA8K_SGMII_MODE_CTRL_MASK			(BIT(22) | BIT(23))
> > > +#define   QCA8K_SGMII_MODE_CTRL_BASEX			0
> > > +#define   QCA8K_SGMII_MODE_CTRL_PHY			BIT(22)
> > > +#define   QCA8K_SGMII_MODE_CTRL_MAC			BIT(23)
> > 
> > I guess these are not really bits. You cannot combine
> > QCA8K_SGMII_MODE_CTRL_MAC and QCA8K_SGMII_MODE_CTRL_PHY. So it makes
> > more sense to have:
> > 
> > #define   QCA8K_SGMII_MODE_CTRL_BASEX			(0x0 << 22)
> > #define   QCA8K_SGMII_MODE_CTRL_PHY			(0x1 << 22)
> > #define   QCA8K_SGMII_MODE_CTRL_MAC			(0x2 << 22)
> 
> Sure; given there's no 0x3 choice I just went for the bits that need
> set, but that works too.

I also prefer Andrew's suggestion, as it makes it clear that it's a two
bit field.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up