Quoting Russell King - ARM Linux admin <li...@armlinux.org.uk>:
Hi Russel,
Thanks for your review, see also my comments and questions below.
Hi,
On Mon, Jun 24, 2019 at 04:52:47PM +0200, René van Dorst wrote:
Convert mt7530 to PHYLINK API
Signed-off-by: René van Dorst <opensou...@vdorst.com>
---
drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
drivers/net/dsa/mt7530.h | 9 ++
2 files changed, 187 insertions(+), 59 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3181e95586d6..9c5e4dd00826 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -13,7 +13,7 @@
#include <linux/of_mdio.h>
#include <linux/of_net.h>
#include <linux/of_platform.h>
-#include <linux/phy.h>
+#include <linux/phylink.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
#include <linux/reset.h>
@@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds,
int port, int sset)
return ARRAY_SIZE(mt7530_mib);
}
-static void mt7530_adjust_link(struct dsa_switch *ds, int port,
- struct phy_device *phydev)
-{
- struct mt7530_priv *priv = ds->priv;
-
- if (phy_is_pseudo_fixed_link(phydev)) {
- dev_dbg(priv->dev, "phy-mode for master device = %x\n",
- phydev->interface);
-
- /* Setup TX circuit incluing relevant PAD and driving */
- mt7530_pad_clk_setup(ds, phydev->interface);
-
- if (priv->id == ID_MT7530) {
- /* Setup RX circuit, relevant PAD and driving on the
- * host which must be placed after the setup on the
- * device side is all finished.
- */
- mt7623_pad_clk_setup(ds);
- }
- } else {
- u16 lcl_adv = 0, rmt_adv = 0;
- u8 flowctrl;
- u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
-
- switch (phydev->speed) {
- case SPEED_1000:
- mcr |= PMCR_FORCE_SPEED_1000;
- break;
- case SPEED_100:
- mcr |= PMCR_FORCE_SPEED_100;
- break;
- }
-
- if (phydev->link)
- mcr |= PMCR_FORCE_LNK;
-
- if (phydev->duplex) {
- mcr |= PMCR_FORCE_FDX;
-
- if (phydev->pause)
- rmt_adv = LPA_PAUSE_CAP;
- if (phydev->asym_pause)
- rmt_adv |= LPA_PAUSE_ASYM;
-
- lcl_adv = linkmode_adv_to_lcl_adv_t(
- phydev->advertising);
- flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
-
- if (flowctrl & FLOW_CTRL_TX)
- mcr |= PMCR_TX_FC_EN;
- if (flowctrl & FLOW_CTRL_RX)
- mcr |= PMCR_RX_FC_EN;
- }
- mt7530_write(priv, MT7530_PMCR_P(port), mcr);
- }
-}
-
static int
mt7530_cpu_port_enable(struct mt7530_priv *priv,
int port)
@@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
return 0;
}
+static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ const struct phylink_link_state *state)
+{
+ struct mt7530_priv *priv = ds->priv;
+ u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
+ PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
+
+ switch (port) {
+ case 0: /* Internal phy */
+ case 1:
+ case 2:
+ case 3:
+ case 4:
+ if (state->interface != PHY_INTERFACE_MODE_GMII)
+ goto unsupported;
+ break;
+ /* case 5: Port 5 is not supported! */
+ case 6: /* 1st cpu port */
+ if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+ state->interface != PHY_INTERFACE_MODE_TRGMII)
+ goto unsupported;
+
+ /* Setup TX circuit incluing relevant PAD and driving */
+ mt7530_pad_clk_setup(ds, state->interface);
+
+ if (priv->id == ID_MT7530) {
+ /* Setup RX circuit, relevant PAD and driving on the
+ * host which must be placed after the setup on the
+ * device side is all finished.
+ */
+ mt7623_pad_clk_setup(ds);
+ }
+ break;
+ default:
+ dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
+ return;
+ }
+
+ if (!state->an_enabled || mode == MLO_AN_FIXED) {
+ mcr |= PMCR_FORCE_MODE;
+
+ if (state->speed == SPEED_1000)
+ mcr |= PMCR_FORCE_SPEED_1000;
+ if (state->speed == SPEED_100)
+ mcr |= PMCR_FORCE_SPEED_100;
+ if (state->duplex == DUPLEX_FULL)
+ mcr |= PMCR_FORCE_FDX;
+ if (state->link || mode == MLO_AN_FIXED)
+ mcr |= PMCR_FORCE_LNK;
This should be removed - state->link is not for use in mac_config.
Even in fixed mode, the link can be brought up/down by means of a
gpio, and this should be dealt with via the mac_link_* functions.
Maybe I understand it wrong, but is it the intention that in
phylink_mac_config with modes MLO_AN_FIXED and MLO_AN_PHY the MAC is always
forces into a certain speed/mode/interface. So it never auto-negotiate because
phylink select the best configuration for you?
Also the PMCR_FORCE_LNK is only set in phylink_link_up() or can I do it here
and do nothing phylink_link_up()?
Other question:
Where does the MAC enable/disable TX and RX fits best? port_{enable,disable}?
Or only mac_config() and port_disable?
+ if (state->pause || phylink_test(state->advertising, Pause))
+ mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
+ if (state->pause & MLO_PAUSE_TX)
+ mcr |= PMCR_TX_FC_EN;
+ if (state->pause & MLO_PAUSE_RX)
+ mcr |= PMCR_RX_FC_EN;
This is clearly wrong - if any bit in state->pause is set, then we
end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause
Pause set in the advertising mask, then both are set. This doesn't
seem right - are these bits setting the advertisement, or are they
telling the MAC to use flow control?
Last one, tell the MAC to use flow control.
Qoute of the datasheet:
If you want to disable TX and RX flow control disable bit 4 and 5
MAC Control Register:
BIT(4): Force TX FC:
0: Disabled
1: Let the MAC transmit a pause frame when operating in full-duplex
mode with low internal free memory page count.
BIT(5): Force RX FC:
0: Disabled
1: Let the MA accept a pause frame when operating in full-duplex
mode.
On the current driver both bits are set in a forced-link situation.
If we always forces the MAC mode I think I always set these bits and don't
anything with the Pause modes? Is that the right way to do it?
+ }
+
+ mt7530_write(priv, MT7530_PMCR_P(port), mcr);
+
+ return;
+
+unsupported:
+ dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
+ __func__, port, state->interface, phy_modes(state->interface));
+}
+
+static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ phy_interface_t interface)
+{
+ /* Do nothing */
+}
+
+static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ phy_interface_t interface,
+ struct phy_device *phydev)
+{
+ /* Do nothing */
+}
These two are where you should be forcing the link up or down if
required (basically, inband modes should let the link come up/down
irrespective of these functions, otherwise it should be forced.)
OK
+
+static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
+ unsigned long *supported,
+ struct phylink_link_state *state)
+{
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+ switch (port) {
+ case 0: /* Internal phy */
+ case 1:
+ case 2:
+ case 3:
+ case 4:
+ if (state->interface != PHY_INTERFACE_MODE_NA &&
+ state->interface != PHY_INTERFACE_MODE_GMII)
+ goto unsupported;
+ break;
+ /* case 5: Port 5 not supported! */
+ case 6: /* 1st cpu port */
+ if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+ state->interface != PHY_INTERFACE_MODE_TRGMII)
PHY_INTERFACE_MODE_NA ?
You mean PHY_INTERFACE_MODE_NA is missing?
PHY_INTERFACE_MODE_TRGMII is a valid mode for this port.
+ goto unsupported;
+ break;
+ default:
+ linkmode_zero(supported);
+ dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
+ return;
+ }
+
+ phylink_set(mask, Autoneg);
+ phylink_set(mask, Pause);
+ phylink_set(mask, Asym_Pause);
+ phylink_set(mask, MII);
+
+ phylink_set(mask, 10baseT_Half);
+ phylink_set(mask, 10baseT_Full);
+ phylink_set(mask, 100baseT_Half);
+ phylink_set(mask, 100baseT_Full);
+ phylink_set(mask, 1000baseT_Full);
+ phylink_set(mask, 1000baseT_Half);
You seem to be missing phylink_set_port_modes() here.
OK
+
+ linkmode_and(supported, supported, mask);
+ linkmode_and(state->advertising, state->advertising, mask);
+ return;
+
+unsupported:
+ linkmode_zero(supported);
+ dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
+ __func__, state->interface, phy_modes(state->interface));
Not a good idea to print this at error level; sometimes we just probe
for support.
Eg, think about a SFP cage, and a SFP is plugged in that uses a PHY
interface mode that the MAC can't support - we detect that by the
validation failing, and printing a more meaningful message in phylink
itself.
OK, It will be removed.
+}
+
+static int
+mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
+ struct phylink_link_state *state)
+{
+ struct mt7530_priv *priv = ds->priv;
+ u32 pmsr;
+
+ if (port < 0 || port >= MT7530_NUM_PORTS)
+ return -EINVAL;
+
+ pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
+
+ state->link = (pmsr & PMSR_LINK);
+ state->an_complete = state->link;
+ state->duplex = (pmsr & PMSR_DPX) >> 1;
+
+ switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
+ case 0:
+ state->speed = SPEED_10;
+ break;
+ case PMSR_SPEED_100:
+ state->speed = SPEED_100;
+ break;
+ case PMSR_SPEED_1000:
+ state->speed = SPEED_1000;
+ break;
+ default:
+ state->speed = SPEED_UNKNOWN;
+ break;
+ }
+
+ state->pause = 0;
+ if (pmsr & PMSR_RX_FC)
+ state->pause |= MLO_PAUSE_RX;
+ if (pmsr & PMSR_TX_FC)
+ state->pause |= MLO_PAUSE_TX;
+
+ return 1;
+}
+
static const struct dsa_switch_ops mt7530_switch_ops = {
.get_tag_protocol = mtk_get_tag_protocol,
.setup = mt7530_setup,
@@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops
mt7530_switch_ops = {
.phy_write = mt7530_phy_write,
.get_ethtool_stats = mt7530_get_ethtool_stats,
.get_sset_count = mt7530_get_sset_count,
- .adjust_link = mt7530_adjust_link,
.port_enable = mt7530_port_enable,
.port_disable = mt7530_port_disable,
.port_stp_state_set = mt7530_stp_state_set,
@@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops
mt7530_switch_ops = {
.port_vlan_prepare = mt7530_port_vlan_prepare,
.port_vlan_add = mt7530_port_vlan_add,
.port_vlan_del = mt7530_port_vlan_del,
+ .phylink_validate = mt7530_phylink_validate,
+ .phylink_mac_link_state = mt7530_phylink_mac_link_state,
+ .phylink_mac_config = mt7530_phylink_mac_config,
+ .phylink_mac_link_down = mt7530_phylink_mac_link_down,
+ .phylink_mac_link_up = mt7530_phylink_mac_link_up,
};
static const struct of_device_id mt7530_of_match[] = {
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index bfac90f48102..41d9a132ac70 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
#define PMCR_FORCE_SPEED_100 BIT(2)
#define PMCR_FORCE_FDX BIT(1)
#define PMCR_FORCE_LNK BIT(0)
+#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE
#define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
PMCR_TX_EN | PMCR_RX_EN | \
@@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
PMCR_TX_FC_EN | PMCR_RX_FC_EN)
#define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100)
+#define PMSR_EEE1G BIT(7)
+#define PMSR_EEE100M BIT(6)
+#define PMSR_RX_FC BIT(5)
+#define PMSR_TX_FC BIT(4)
+#define PMSR_SPEED_1000 BIT(3)
+#define PMSR_SPEED_100 BIT(2)
+#define PMSR_DPX BIT(1)
+#define PMSR_LINK BIT(0)
/* Register for MIB */
#define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100)
--
2.20.1
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Greats,
René
