Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 5 Jun 2020 20:38:43 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Jonathan McDowell <noodles@earth.li>,
        Russell King <rmk+kernel@armlinux.org.uk>
Cc:     Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration
 options
Message-ID: <20200605183843.GB1006885@lunn.ch>
References: <cover.1591380105.git.noodles@earth.li>
 <8ddd76e484e1bedd12c87ea0810826b60e004a65.1591380105.git.noodles@earth.li>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8ddd76e484e1bedd12c87ea0810826b60e004a65.1591380105.git.noodles@earth.li>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

Hi Jonathan

It is good to include Russell King in Cc: for patches like this.

Also, netdev is closed at the moment, so please post patches as RFC.

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.

What exactly does sgmii-delay do?

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

   Andrew