| Message ID | 8ddd76e484e1bedd12c87ea0810826b60e004a65.1591380105.git.noodles@earth.li |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: qca8k: Add SGMII configuration options | expand |
On Fri, 5 Jun 2020 19:10:58 +0100 Jonathan 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 should infere this settings from the existing device tree bindings, ie look at phy-mode. You can use of_get_phy_mode function, or something from of_mdio.c, or better yet change the api in this driver to use the new phylink 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_DELAY 3 > #define QCA8K_PORT_PAD_RGMII_RX_DELAY_EN BIT(24) > #define QCA8K_PORT_PAD_SGMII_EN BIT(7) > +#define QCA8K_REG_PWS 0x010 > +#define QCA8K_PWS_SERDES_AEN_DIS BIT(7) > #define QCA8K_REG_MODULE_EN 0x030 > #define QCA8K_MODULE_EN_MIB BIT(0) > #define QCA8K_REG_MIB 0x034 > @@ -77,6 +79,16 @@ > #define QCA8K_PORT_HDR_CTRL_ALL 2 > #define QCA8K_PORT_HDR_CTRL_MGMT 1 > #define QCA8K_PORT_HDR_CTRL_NONE 0 > +#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) > > /* EEE control registers */ > #define QCA8K_REG_EEE_CTRL 0x100
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
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.
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.
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 might provide 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 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? > > > 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? Pass, but I think I might be able to get away without having to configure that for the moment. I'll go away and roll a v2 moving qca8k over to phylink and then using that 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 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.
> > > > 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 subject line, 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 SGMII link, and all instances I can find completely disable inband configuration for that case. However the data sheet has an SGMII control register which allows configuration of the various auto-negotiation parameters (as well as whether we're base-x or sgmii) so I think the full flexibility is there. I've got an initial port over to using phylink and picking up the parameters that way (avoiding any device tree option changes) that seems to be working, but I'll do a bit more testing before sending out a v2 RFC. J.
Hi Jonathan,
url: https://github.com/0day-ci/linux/commits/Jonathan-McDowell/net-dsa-qca8k-Add-SGMII-configuration-options/20200606-021254
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-randconfig-m021-20200618 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-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/27dd896d27e5048d2c402879fb04f6e23536ea72
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 27dd896d27e5048d2c402879fb04f6e23536ea72
vim +/mode +438 drivers/net/dsa/qca8k.c
27dd896d27e5048 Jonathan McDowell 2020-06-05 421 static int
27dd896d27e5048 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 Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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; + } + } + + 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_DELAY 3 #define QCA8K_PORT_PAD_RGMII_RX_DELAY_EN BIT(24) #define QCA8K_PORT_PAD_SGMII_EN BIT(7) +#define QCA8K_REG_PWS 0x010 +#define QCA8K_PWS_SERDES_AEN_DIS BIT(7) #define QCA8K_REG_MODULE_EN 0x030 #define QCA8K_MODULE_EN_MIB BIT(0) #define QCA8K_REG_MIB 0x034 @@ -77,6 +79,16 @@ #define QCA8K_PORT_HDR_CTRL_ALL 2 #define QCA8K_PORT_HDR_CTRL_MGMT 1 #define QCA8K_PORT_HDR_CTRL_NONE 0 +#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) /* EEE control registers */ #define QCA8K_REG_EEE_CTRL 0x100
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(-)