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.
