[2/2] net: dsa: qca8k: introduce SGMII configuration options (2024)

The QCA8337(N) has an SGMII port which can operate in MAC, PHY or BASE-Xmode depending on what it's connected to (e.g. CPU vs external PHY orSFP). At present the driver does no configuration of this port even ifit is selected.Add support for making sure the SGMII is enabled if it's in use, anddevice tree support for configuring the connection details.Signed-off-by: Jonathan McDowell <noodles@earth.li>--- drivers/net/dsa/qca8k.c | 44 ++++++++++++++++++++++++++++++++++++++++- drivers/net/dsa/qca8k.h | 12 +++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-)

On Fri, 5 Jun 2020 19:10:58 +0100Jonathan McDowell <noodles@earth.li> 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.> > Signed-off-by: Jonathan McDowell <noodles@earth.li>> ---> drivers/net/dsa/qca8k.c | 44 ++++++++++++++++++++++++++++++++++++++++-> drivers/net/dsa/qca8k.h | 12 +++++++++++> 2 files changed, 55 insertions(+), 1 deletion(-)> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c> index 9f4205b4439b..5b7979aca9b9 100644> --- a/drivers/net/dsa/qca8k.c> +++ b/drivers/net/dsa/qca8k.c> @@ -418,6 +418,40 @@ qca8k_mib_init(struct qca8k_priv *priv)> mutex_unlock(&priv->reg_mutex);> }> > +static int> +qca8k_setup_sgmii(struct qca8k_priv *priv)> +{> +const char *mode;> +u32 val;> +> +val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);> +> +val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |> +QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;> +> +if (of_property_read_bool(priv->dev->of_node, "sgmii-delay"))> +val |= QCA8K_SGMII_CLK125M_DELAY;> +> +if (of_property_read_string(priv->dev->of_node, "sgmii-mode", &mode)) {> +val &= ~QCA8K_SGMII_MODE_CTRL_MASK;> +> +if (!strcasecmp(mode, "basex")) {> +val |= QCA8K_SGMII_MODE_CTRL_BASEX;> +} else if (!strcasecmp(mode, "mac")) {> +val |= QCA8K_SGMII_MODE_CTRL_MAC;> +} else if (!strcasecmp(mode, "phy")) {> +val |= QCA8K_SGMII_MODE_CTRL_PHY;> +} else {> +pr_err("Unrecognised SGMII mode %s\n", mode);> +return -EINVAL;> +}> +}There is no sgmii-mode device tree property documented. You shouldinfere this settings from the existing device tree bindings, ie look atphy-mode. You can use of_get_phy_mode function, or something fromof_mdio.c, or better yet change the api in this driver to use the newphylink API.Marek> +> +qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);> +> +return 0;> +}> +> static int> qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)> {> @@ -458,7 +492,8 @@ qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)> break;> case PHY_INTERFACE_MODE_SGMII:> qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);> -break;> +> +return qca8k_setup_sgmii(priv);> default:> pr_err("xMII mode %d not supported\n", mode);> return -EINVAL;> @@ -661,6 +696,13 @@ qca8k_setup(struct dsa_switch *ds)> if (ret)> return ret;> > +if (of_property_read_bool(priv->dev->of_node,> + "disable-serdes-autoneg")) {> +mask = qca8k_read(priv, QCA8K_REG_PWS) |> + QCA8K_PWS_SERDES_AEN_DIS;> +qca8k_write(priv, QCA8K_REG_PWS, mask);> +}> +> /* Initialize CPU port pad mode (xMII type, delays...) */> ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, &phy_mode);> if (ret) {> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h> index 42d6ea24eb14..cd97c212f3f8 100644> --- a/drivers/net/dsa/qca8k.h> +++ b/drivers/net/dsa/qca8k.h> @@ -36,6 +36,8 @@> #define QCA8K_MAX_DELAY3> #define QCA8K_PORT_PAD_RGMII_RX_DELAY_ENBIT(24)> #define QCA8K_PORT_PAD_SGMII_ENBIT(7)> +#define QCA8K_REG_PWS0x010> +#define QCA8K_PWS_SERDES_AEN_DISBIT(7)> #define QCA8K_REG_MODULE_EN0x030> #define QCA8K_MODULE_EN_MIBBIT(0)> #define QCA8K_REG_MIB0x034> @@ -77,6 +79,16 @@> #define QCA8K_PORT_HDR_CTRL_ALL2> #define QCA8K_PORT_HDR_CTRL_MGMT1> #define QCA8K_PORT_HDR_CTRL_NONE0> +#define QCA8K_REG_SGMII_CTRL0x0e0> +#define QCA8K_SGMII_EN_PLLBIT(1)> +#define QCA8K_SGMII_EN_RXBIT(2)> +#define QCA8K_SGMII_EN_TXBIT(3)> +#define QCA8K_SGMII_EN_SDBIT(4)> +#define QCA8K_SGMII_CLK125M_DELAYBIT(7)> +#define QCA8K_SGMII_MODE_CTRL_MASK(BIT(22) | BIT(23))> +#define QCA8K_SGMII_MODE_CTRL_BASEX0> +#define QCA8K_SGMII_MODE_CTRL_PHYBIT(22)> +#define QCA8K_SGMII_MODE_CTRL_MACBIT(23)> > /* EEE control registers */> #define QCA8K_REG_EEE_CTRL0x100

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 JonathanIt 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 or1000BaseX. phylink will tell you what mode to configure it to. e.g. Afibre SFP module will want 1000BaseX. A copper SFP module will wantSGMII. A switch is likely to want 1000BaseX. A PHY is likely to wantSGMII. So remove the "sgmii-mode" property and configure it as phylinkis requesting.What exactly does sgmii-delay do?> +#define QCA8K_REG_SGMII_CTRL0x0e0> +#define QCA8K_SGMII_EN_PLLBIT(1)> +#define QCA8K_SGMII_EN_RXBIT(2)> +#define QCA8K_SGMII_EN_TXBIT(3)> +#define QCA8K_SGMII_EN_SDBIT(4)> +#define QCA8K_SGMII_CLK125M_DELAYBIT(7)> +#define QCA8K_SGMII_MODE_CTRL_MASK(BIT(22) | BIT(23))> +#define QCA8K_SGMII_MODE_CTRL_BASEX0> +#define QCA8K_SGMII_MODE_CTRL_PHYBIT(22)> +#define QCA8K_SGMII_MODE_CTRL_MACBIT(23)I guess these are not really bits. You cannot combineQCA8K_SGMII_MODE_CTRL_MAC and QCA8K_SGMII_MODE_CTRL_PHY. So it makesmore 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

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 theinitial 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 itto, 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 ifit's talking to an SGMII MAC, i.e. a CPU, or an SGMII PHY, i.e. anexternal PHY, or in BaseX mode for an SFP. I couldn't figure out a wayin the current framework to automatically work out if I wanted PHY orMAC mode. For the port tagged CPU I can assume MAC mode, but a port thatdoesn't have that might still be attached to the CPU rather than anexternal PHY.> What exactly does sgmii-delay do?As per the device tree documentation update I sent it delays the SGMIIclock by 2ns. From the data sheet:SGMII_SEL_CLK125Msgmii_clk125m_rx_delay is delayed by 2ns> > +#define QCA8K_REG_SGMII_CTRL0x0e0> > +#define QCA8K_SGMII_EN_PLLBIT(1)> > +#define QCA8K_SGMII_EN_RXBIT(2)> > +#define QCA8K_SGMII_EN_TXBIT(3)> > +#define QCA8K_SGMII_EN_SDBIT(4)> > +#define QCA8K_SGMII_CLK125M_DELAYBIT(7)> > +#define QCA8K_SGMII_MODE_CTRL_MASK(BIT(22) | BIT(23))> > +#define QCA8K_SGMII_MODE_CTRL_BASEX0> > +#define QCA8K_SGMII_MODE_CTRL_PHYBIT(22)> > +#define QCA8K_SGMII_MODE_CTRL_MACBIT(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 needset, but that works too.J.

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; MAINTAINERScontains everything that is sensible, but Andrew is suggesting thatyou 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 Linustree?"> > 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 sidesof SGMII "System side" and "Media side". System side is where you'rereceiving the results of AN from a PHY. Media side is where you'retelling the partner what you want it to do.Media side is only useful if you're connected to another MAC, andunless you have a requirement for it, I would suggest not implementingthat - you could come up with something using fixed-link, or it mayneed some other model if the settings need to change. That depends onthe 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_CLK125Msgmii_clk125m_rx_delay is delayed by 2nsThis sounds like a new world of RGMII delay pain but for SGMII. Thereis no mention of "delay" in the SGMII v1.8 specification, so I guessit's something the vendor is doing. Is this device capable ofrecovering the clock from a single serdes pair carrying the data,or does it always require the separate clock?> > > +#define QCA8K_REG_SGMII_CTRL0x0e0> > > +#define QCA8K_SGMII_EN_PLLBIT(1)> > > +#define QCA8K_SGMII_EN_RXBIT(2)> > > +#define QCA8K_SGMII_EN_TXBIT(3)> > > +#define QCA8K_SGMII_EN_SDBIT(4)> > > +#define QCA8K_SGMII_CLK125M_DELAYBIT(7)> > > +#define QCA8K_SGMII_MODE_CTRL_MASK(BIT(22) | BIT(23))> > > +#define QCA8K_SGMII_MODE_CTRL_BASEX0> > > +#define QCA8K_SGMII_MODE_CTRL_PHYBIT(22)> > > +#define QCA8K_SGMII_MODE_CTRL_MACBIT(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 twobit field.

On Sat, Jun 06, 2020 at 09:37:41AM +0100, Russell King - ARM Linux admin wrote:> 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.Sure, and I appreciate the pointer to appropriate people who mightprovide helpful comments.> Note that I haven't seen your patches.I'll make sure to copy you on v2.> > > 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?"Ta. I'll hold off on a v2 until after -rc1 drops.> > > 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.So the device in question is a 7 port stand alone switch chip. There's asingle SGMII port which is configurable between port 0 + 6 (they canalso be configure up as RGMII, while the remaining 5 ports have theirown phys).It sounds like there's a strong preference to try and auto configurethings as much as possible, so I should assume the CPU port is in MACmode, and anything not tagged as a CPU port is talking to a PHY/BASEX.I assume I can use PHY_INTERFACE_MODE_1000BASEX on thephylink_mac_config call to choose BASEX?> > > 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_CLK125Msgmii_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?Pass, but I think I might be able to get away without having toconfigure that for the moment.I'll go away and roll a v2 moving qca8k over to phylink and then usingthat to auto select the appropriate SGMII mode. Thanks for the feedback.J.

On Sat, Jun 06, 2020 at 11:59:09AM +0100, Jonathan McDowell wrote:> So the device in question is a 7 port stand alone switch chip. There's a> single SGMII port which is configurable between port 0 + 6 (they can> also be configure up as RGMII, while the remaining 5 ports have their> own phys).> > It sounds like there's a strong preference to try and auto configure> things as much as possible, so I should assume the CPU port is in MAC> mode, and anything not tagged as a CPU port is talking to a PHY/BASEX.> > I assume I can use PHY_INTERFACE_MODE_1000BASEX on the> phylink_mac_config call to choose BASEX?Yes, but from what you've mentioned above, I think I need to ensure thatthere's a proper understanding here.1000BASE-X is the IEEE 802.3 defined 1G single lane Serdes protocol.SGMII is different; it's a vendor derivative of 1000BASE-X which hasbecome a de-facto standard.Both are somewhat compatible with each other; SGMII brings with itadditional data replication to achieve 100M and 10M speeds, whilekeeping the link running at 1.25Gbaud. In both cases, there is a16-bit "configuration" word that is passed between the partners.1000BASE-X uses this configuration word to advertise the abilities ofeach end, which is limited to duplex and pause modes only. This youget by specifying the phy-mode="1000base-x" andmanaged="in-band-status" in DT.SGMII uses this configuration word for the media side to inform thesystem side which mode it wishes to operate the link: the speed andduplex. Some vendors extend it to include EEE parameters as well,or pause modes. You get this via phy-mode="sgmii" andmanaged="in-band-status" in DT.Then there are variants where the configuration word is not present.In this case, the link has to be manually configured, and without theconfiguration word, SGMII operating at 1G is compatible with1000base-X operating at 1G. Fixed-link can be used for this, althoughfixed-link will always report that the link is up at the moment; thatmay change in the future, it's something that is being looked into atthe moment.

> > > > 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?"> > Ta. I'll hold off on a v2 until after -rc1 drops.You can post at the moment, but you need to put RFC in the subjectline, just to make it clear you are only interested in comments. Andrew

On Sat, Jun 06, 2020 at 02:43:56PM +0100, Russell King - ARM Linux admin wrote:> On Sat, Jun 06, 2020 at 11:59:09AM +0100, Jonathan McDowell wrote:> > So the device in question is a 7 port stand alone switch chip. There's a> > single SGMII port which is configurable between port 0 + 6 (they can> > also be configure up as RGMII, while the remaining 5 ports have their> > own phys).> > > > It sounds like there's a strong preference to try and auto configure> > things as much as possible, so I should assume the CPU port is in MAC> > mode, and anything not tagged as a CPU port is talking to a PHY/BASEX.> > > > I assume I can use PHY_INTERFACE_MODE_1000BASEX on the> > phylink_mac_config call to choose BASEX?> > Yes, but from what you've mentioned above, I think I need to ensure that> there's a proper understanding here.> > 1000BASE-X is the IEEE 802.3 defined 1G single lane Serdes protocol.> SGMII is different; it's a vendor derivative of 1000BASE-X which has> become a de-facto standard.> > Both are somewhat compatible with each other; SGMII brings with it> additional data replication to achieve 100M and 10M speeds, while> keeping the link running at 1.25Gbaud. In both cases, there is a> 16-bit "configuration" word that is passed between the partners.> > 1000BASE-X uses this configuration word to advertise the abilities of> each end, which is limited to duplex and pause modes only. This you> get by specifying the phy-mode="1000base-x" and> managed="in-band-status" in DT.> > SGMII uses this configuration word for the media side to inform the> system side which mode it wishes to operate the link: the speed and> duplex. Some vendors extend it to include EEE parameters as well,> or pause modes. You get this via phy-mode="sgmii" and> managed="in-band-status" in DT.> > Then there are variants where the configuration word is not present.> In this case, the link has to be manually configured, and without the> configuration word, SGMII operating at 1G is compatible with> 1000base-X operating at 1G. Fixed-link can be used for this, although> fixed-link will always report that the link is up at the moment; that> may change in the future, it's something that is being looked into at> the moment.The hardware I'm using has the switch connected to the CPU via the SGMIIlink, and all instances I can find completely disable inbandconfiguration for that case. However the data sheet has an SGMII controlregister which allows configuration of the various auto-negotiationparameters (as well as whether we're base-x or sgmii) so I think thefull flexibility is there.I've got an initial port over to using phylink and picking up theparameters that way (avoiding any device tree option changes) that seemsto be working, but I'll do a bit more testing before sending out a v2RFC.J.

Hi Jonathan,url: https://github.com/0day-ci/linux/commits/Jonathan-McDowell/net-dsa-qca8k-Add-SGMII-configuration-options/20200606-021254base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-nextconfig: i386-randconfig-m021-20200618 (attached as .config)compiler: gcc-9 (Debian 9.3.0-13) 9.3.0If you fix the issue, kindly add following tag as appropriateReported-by: kernel test robot <lkp@intel.com>Reported-by: Dan Carpenter <dan.carpenter@oracle.com>smatch warnings:drivers/net/dsa/qca8k.c:438 qca8k_setup_sgmii() error: uninitialized symbol 'mode'.# https://github.com/0day-ci/linux/commit/27dd896d27e5048d2c402879fb04f6e23536ea72git remote add linux-review https://github.com/0day-ci/linuxgit remote update linux-reviewgit checkout 27dd896d27e5048d2c402879fb04f6e23536ea72vim +/mode +438 drivers/net/dsa/qca8k.c27dd896d27e5048 Jonathan McDowell 2020-06-05 421 static int27dd896d27e5048 Jonathan McDowell 2020-06-05 422 qca8k_setup_sgmii(struct qca8k_priv *priv)27dd896d27e5048 Jonathan McDowell 2020-06-05 423 {27dd896d27e5048 Jonathan McDowell 2020-06-05 424 const char *mode; ^^^^27dd896d27e5048 Jonathan McDowell 2020-06-05 425 u32 val;27dd896d27e5048 Jonathan McDowell 2020-06-05 426 27dd896d27e5048 Jonathan McDowell 2020-06-05 427 val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);27dd896d27e5048 Jonathan McDowell 2020-06-05 428 27dd896d27e5048 Jonathan McDowell 2020-06-05 429 val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |27dd896d27e5048 Jonathan McDowell 2020-06-05 430 QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;27dd896d27e5048 Jonathan McDowell 2020-06-05 431 27dd896d27e5048 Jonathan McDowell 2020-06-05 432 if (of_property_read_bool(priv->dev->of_node, "sgmii-delay"))27dd896d27e5048 Jonathan McDowell 2020-06-05 433 val |= QCA8K_SGMII_CLK125M_DELAY;27dd896d27e5048 Jonathan McDowell 2020-06-05 434 27dd896d27e5048 Jonathan McDowell 2020-06-05 435 if (of_property_read_string(priv->dev->of_node, "sgmii-mode", &mode)) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^This if condition is reversed.27dd896d27e5048 Jonathan McDowell 2020-06-05 436 val &= ~QCA8K_SGMII_MODE_CTRL_MASK;27dd896d27e5048 Jonathan McDowell 2020-06-05 437 27dd896d27e5048 Jonathan McDowell 2020-06-05 @438 if (!strcasecmp(mode, "basex")) {27dd896d27e5048 Jonathan McDowell 2020-06-05 439 val |= QCA8K_SGMII_MODE_CTRL_BASEX;27dd896d27e5048 Jonathan McDowell 2020-06-05 440 } else if (!strcasecmp(mode, "mac")) {27dd896d27e5048 Jonathan McDowell 2020-06-05 441 val |= QCA8K_SGMII_MODE_CTRL_MAC;27dd896d27e5048 Jonathan McDowell 2020-06-05 442 } else if (!strcasecmp(mode, "phy")) {27dd896d27e5048 Jonathan McDowell 2020-06-05 443 val |= QCA8K_SGMII_MODE_CTRL_PHY;27dd896d27e5048 Jonathan McDowell 2020-06-05 444 } else {27dd896d27e5048 Jonathan McDowell 2020-06-05 445 pr_err("Unrecognised SGMII mode %s\n", mode);27dd896d27e5048 Jonathan McDowell 2020-06-05 446 return -EINVAL;27dd896d27e5048 Jonathan McDowell 2020-06-05 447 }27dd896d27e5048 Jonathan McDowell 2020-06-05 448 }27dd896d27e5048 Jonathan McDowell 2020-06-05 449 27dd896d27e5048 Jonathan McDowell 2020-06-05 450 qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);27dd896d27e5048 Jonathan McDowell 2020-06-05 451 27dd896d27e5048 Jonathan McDowell 2020-06-05 452 return 0;27dd896d27e5048 Jonathan McDowell 2020-06-05 453 }---0-DAY CI Kernel Test Service, Intel Corporationhttps://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Patch

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.cindex 9f4205b4439b..5b7979aca9b9 100644--- a/drivers/net/dsa/qca8k.c+++ b/drivers/net/dsa/qca8k.c@@ -418,6 +418,40 @@  qca8k_mib_init(struct qca8k_priv *priv) mutex_unlock(&priv->reg_mutex); } +static int+qca8k_setup_sgmii(struct qca8k_priv *priv)+{+const char *mode;+u32 val;++val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);++val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |+QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;++if (of_property_read_bool(priv->dev->of_node, "sgmii-delay"))+val |= QCA8K_SGMII_CLK125M_DELAY;++if (of_property_read_string(priv->dev->of_node, "sgmii-mode", &mode)) {+val &= ~QCA8K_SGMII_MODE_CTRL_MASK;++if (!strcasecmp(mode, "basex")) {+val |= QCA8K_SGMII_MODE_CTRL_BASEX;+} else if (!strcasecmp(mode, "mac")) {+val |= QCA8K_SGMII_MODE_CTRL_MAC;+} else if (!strcasecmp(mode, "phy")) {+val |= QCA8K_SGMII_MODE_CTRL_PHY;+} else {+pr_err("Unrecognised SGMII mode %s\n", mode);+return -EINVAL;+}+}++qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);++return 0;+}+ static int qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode) {@@ -458,7 +492,8 @@  qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode) break; case PHY_INTERFACE_MODE_SGMII: qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);-break;++return qca8k_setup_sgmii(priv); default: pr_err("xMII mode %d not supported\n", mode); return -EINVAL;@@ -661,6 +696,13 @@  qca8k_setup(struct dsa_switch *ds) if (ret) return ret; +if (of_property_read_bool(priv->dev->of_node,+ "disable-serdes-autoneg")) {+mask = qca8k_read(priv, QCA8K_REG_PWS) |+ QCA8K_PWS_SERDES_AEN_DIS;+qca8k_write(priv, QCA8K_REG_PWS, mask);+}+ /* Initialize CPU port pad mode (xMII type, delays...) */ ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, &phy_mode); if (ret) {diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.hindex 42d6ea24eb14..cd97c212f3f8 100644--- a/drivers/net/dsa/qca8k.h+++ b/drivers/net/dsa/qca8k.h@@ -36,6 +36,8 @@  #define QCA8K_MAX_DELAY3 #define QCA8K_PORT_PAD_RGMII_RX_DELAY_ENBIT(24) #define QCA8K_PORT_PAD_SGMII_ENBIT(7)+#define QCA8K_REG_PWS0x010+#define QCA8K_PWS_SERDES_AEN_DISBIT(7) #define QCA8K_REG_MODULE_EN0x030 #define QCA8K_MODULE_EN_MIBBIT(0) #define QCA8K_REG_MIB0x034@@ -77,6 +79,16 @@  #define QCA8K_PORT_HDR_CTRL_ALL2 #define QCA8K_PORT_HDR_CTRL_MGMT1 #define QCA8K_PORT_HDR_CTRL_NONE0+#define QCA8K_REG_SGMII_CTRL0x0e0+#define QCA8K_SGMII_EN_PLLBIT(1)+#define QCA8K_SGMII_EN_RXBIT(2)+#define QCA8K_SGMII_EN_TXBIT(3)+#define QCA8K_SGMII_EN_SDBIT(4)+#define QCA8K_SGMII_CLK125M_DELAYBIT(7)+#define QCA8K_SGMII_MODE_CTRL_MASK(BIT(22) | BIT(23))+#define QCA8K_SGMII_MODE_CTRL_BASEX0+#define QCA8K_SGMII_MODE_CTRL_PHYBIT(22)+#define QCA8K_SGMII_MODE_CTRL_MACBIT(23) /* EEE control registers */ #define QCA8K_REG_EEE_CTRL0x100