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

[Jonathan McDowell]

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.

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

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

> 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

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

J.

-- 
Mistakes aren't always regrets.