[net-next,v13,11/16] net: dsa: mt7530: use external PCS driver
Commit Message
Implement regmap access wrappers, for now only to be used by the
pcs-mtk driver.
Make use of external PCS driver and drop the reduntant implementation
in mt7530.c.
As a nice side effect the SGMII registers can now also more easily be
inspected for debugging via /sys/kernel/debug/regmap.
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Tested-by: Bjørn Mork <bjorn@mork.no>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
drivers/net/dsa/Kconfig | 1 +
drivers/net/dsa/mt7530.c | 277 ++++++++++-----------------------------
drivers/net/dsa/mt7530.h | 47 +------
3 files changed, 71 insertions(+), 254 deletions(-)
Comments
Hi,
have tested Parts 1-11 this on BananaPi-R3 (mt7986) with 1000Base-X Fibre SFP (no 2g5 available yet) on gmac1 and lan4 (mt7531 p5)
Tested-by: Frank Wunderlich <frank-w@public-files.de>
Thx Daniel for working on SFP support :)
regards Frank
On 9.03.2023 13:57, Daniel Golle wrote:
> Implement regmap access wrappers, for now only to be used by the
> pcs-mtk driver.
> Make use of external PCS driver and drop the reduntant implementation
> in mt7530.c.
> As a nice side effect the SGMII registers can now also more easily be
> inspected for debugging via /sys/kernel/debug/regmap.
>
> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Tested-by: Bjørn Mork <bjorn@mork.no>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> Tested-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> drivers/net/dsa/Kconfig | 1 +
> drivers/net/dsa/mt7530.c | 277 ++++++++++-----------------------------
> drivers/net/dsa/mt7530.h | 47 +------
> 3 files changed, 71 insertions(+), 254 deletions(-)
>
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index f6f3b43dfb06..6b45fa8b6907 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -38,6 +38,7 @@ config NET_DSA_MT7530
> tristate "MediaTek MT7530 and MT7531 Ethernet switch support"
> select NET_DSA_TAG_MTK
> select MEDIATEK_GE_PHY
> + select PCS_MTK_LYNXI
> help
> This enables support for the MediaTek MT7530 and MT7531 Ethernet
> switch chips. Multi-chip module MT7530 in MT7621AT, MT7621DAT,
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3a15015bc409..582ba30374c8 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -14,6 +14,7 @@
> #include <linux/of_mdio.h>
> #include <linux/of_net.h>
> #include <linux/of_platform.h>
> +#include <linux/pcs/pcs-mtk-lynxi.h>
> #include <linux/phylink.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> @@ -2567,128 +2568,11 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
> return 0;
> }
>
> -static void mt7531_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
> - phy_interface_t interface, int speed, int duplex)
> -{
> - struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
> - int port = pcs_to_mt753x_pcs(pcs)->port;
> - unsigned int val;
> -
> - /* For adjusting speed and duplex of SGMII force mode. */
> - if (interface != PHY_INTERFACE_MODE_SGMII ||
> - phylink_autoneg_inband(mode))
> - return;
> -
> - /* SGMII force mode setting */
> - val = mt7530_read(priv, MT7531_SGMII_MODE(port));
> - val &= ~MT7531_SGMII_IF_MODE_MASK;
> -
> - switch (speed) {
> - case SPEED_10:
> - val |= MT7531_SGMII_FORCE_SPEED_10;
> - break;
> - case SPEED_100:
> - val |= MT7531_SGMII_FORCE_SPEED_100;
> - break;
> - case SPEED_1000:
> - val |= MT7531_SGMII_FORCE_SPEED_1000;
> - break;
> - }
> -
> - /* MT7531 SGMII 1G force mode can only work in full duplex mode,
> - * no matter MT7531_SGMII_FORCE_HALF_DUPLEX is set or not.
> - *
> - * The speed check is unnecessary as the MAC capabilities apply
> - * this restriction. --rmk
> - */
> - if ((speed == SPEED_10 || speed == SPEED_100) &&
> - duplex != DUPLEX_FULL)
> - val |= MT7531_SGMII_FORCE_HALF_DUPLEX;
> -
> - mt7530_write(priv, MT7531_SGMII_MODE(port), val);
> -}
> -
> static bool mt753x_is_mac_port(u32 port)
> {
> return (port == 5 || port == 6);
> }
>
> -static int mt7531_sgmii_setup_mode_force(struct mt7530_priv *priv, u32 port,
> - phy_interface_t interface)
> -{
> - u32 val;
> -
> - if (!mt753x_is_mac_port(port))
> - return -EINVAL;
> -
> - mt7530_set(priv, MT7531_QPHY_PWR_STATE_CTRL(port),
> - MT7531_SGMII_PHYA_PWD);
> -
> - val = mt7530_read(priv, MT7531_PHYA_CTRL_SIGNAL3(port));
> - val &= ~MT7531_RG_TPHY_SPEED_MASK;
> - /* Setup 2.5 times faster clock for 2.5Gbps data speeds with 10B/8B
> - * encoding.
> - */
> - val |= (interface == PHY_INTERFACE_MODE_2500BASEX) ?
> - MT7531_RG_TPHY_SPEED_3_125G : MT7531_RG_TPHY_SPEED_1_25G;
> - mt7530_write(priv, MT7531_PHYA_CTRL_SIGNAL3(port), val);
> -
> - mt7530_clear(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_ENABLE);
> -
> - /* MT7531 SGMII 1G and 2.5G force mode can only work in full duplex
> - * mode, no matter MT7531_SGMII_FORCE_HALF_DUPLEX is set or not.
> - */
> - mt7530_rmw(priv, MT7531_SGMII_MODE(port),
> - MT7531_SGMII_IF_MODE_MASK | MT7531_SGMII_REMOTE_FAULT_DIS,
> - MT7531_SGMII_FORCE_SPEED_1000);
> -
> - mt7530_write(priv, MT7531_QPHY_PWR_STATE_CTRL(port), 0);
> -
> - return 0;
> -}
> -
> -static int mt7531_sgmii_setup_mode_an(struct mt7530_priv *priv, int port,
> - phy_interface_t interface)
> -{
> - if (!mt753x_is_mac_port(port))
> - return -EINVAL;
> -
> - mt7530_set(priv, MT7531_QPHY_PWR_STATE_CTRL(port),
> - MT7531_SGMII_PHYA_PWD);
> -
> - mt7530_rmw(priv, MT7531_PHYA_CTRL_SIGNAL3(port),
> - MT7531_RG_TPHY_SPEED_MASK, MT7531_RG_TPHY_SPEED_1_25G);
> -
> - mt7530_set(priv, MT7531_SGMII_MODE(port),
> - MT7531_SGMII_REMOTE_FAULT_DIS |
> - MT7531_SGMII_SPEED_DUPLEX_AN);
> -
> - mt7530_rmw(priv, MT7531_PCS_SPEED_ABILITY(port),
> - MT7531_SGMII_TX_CONFIG_MASK, 1);
> -
> - mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_ENABLE);
> -
> - mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_RESTART);
> -
> - mt7530_write(priv, MT7531_QPHY_PWR_STATE_CTRL(port), 0);
> -
> - return 0;
> -}
> -
> -static void mt7531_pcs_an_restart(struct phylink_pcs *pcs)
> -{
> - struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
> - int port = pcs_to_mt753x_pcs(pcs)->port;
> - u32 val;
> -
> - /* Only restart AN when AN is enabled */
> - val = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
> - if (val & MT7531_SGMII_AN_ENABLE) {
> - val |= MT7531_SGMII_AN_RESTART;
> - mt7530_write(priv, MT7531_PCS_CONTROL_1(port), val);
> - }
> -}
> -
> static int
> mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> phy_interface_t interface)
> @@ -2711,11 +2595,11 @@ mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> phydev = dp->slave->phydev;
> return mt7531_rgmii_setup(priv, port, interface, phydev);
> case PHY_INTERFACE_MODE_SGMII:
> - return mt7531_sgmii_setup_mode_an(priv, port, interface);
> case PHY_INTERFACE_MODE_NA:
> case PHY_INTERFACE_MODE_1000BASEX:
> case PHY_INTERFACE_MODE_2500BASEX:
> - return mt7531_sgmii_setup_mode_force(priv, port, interface);
> + /* handled in SGMII PCS driver */
> + return 0;
> default:
> return -EINVAL;
> }
> @@ -2740,11 +2624,11 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
>
> switch (interface) {
> case PHY_INTERFACE_MODE_TRGMII:
> + return &priv->pcs[port].pcs;
> case PHY_INTERFACE_MODE_SGMII:
> case PHY_INTERFACE_MODE_1000BASEX:
> case PHY_INTERFACE_MODE_2500BASEX:
> - return &priv->pcs[port].pcs;
> -
> + return priv->ports[port].sgmii_pcs;
> default:
> return NULL;
> }
> @@ -2982,86 +2866,6 @@ static void mt7530_pcs_get_state(struct phylink_pcs *pcs,
> state->pause |= MLO_PAUSE_TX;
> }
>
> -static int
> -mt7531_sgmii_pcs_get_state_an(struct mt7530_priv *priv, int port,
> - struct phylink_link_state *state)
> -{
> - u32 status, val;
> - u16 config_reg;
> -
> - status = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
> - state->link = !!(status & MT7531_SGMII_LINK_STATUS);
> - state->an_complete = !!(status & MT7531_SGMII_AN_COMPLETE);
> - if (state->interface == PHY_INTERFACE_MODE_SGMII &&
> - (status & MT7531_SGMII_AN_ENABLE)) {
> - val = mt7530_read(priv, MT7531_PCS_SPEED_ABILITY(port));
> - config_reg = val >> 16;
> -
> - switch (config_reg & LPA_SGMII_SPD_MASK) {
> - case LPA_SGMII_1000:
> - state->speed = SPEED_1000;
> - break;
> - case LPA_SGMII_100:
> - state->speed = SPEED_100;
> - break;
> - case LPA_SGMII_10:
> - state->speed = SPEED_10;
> - break;
> - default:
> - dev_err(priv->dev, "invalid sgmii PHY speed\n");
> - state->link = false;
> - return -EINVAL;
> - }
> -
> - if (config_reg & LPA_SGMII_FULL_DUPLEX)
> - state->duplex = DUPLEX_FULL;
> - else
> - state->duplex = DUPLEX_HALF;
> - }
> -
> - return 0;
> -}
> -
> -static void
> -mt7531_sgmii_pcs_get_state_inband(struct mt7530_priv *priv, int port,
> - struct phylink_link_state *state)
> -{
> - unsigned int val;
> -
> - val = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
> - state->link = !!(val & MT7531_SGMII_LINK_STATUS);
> - if (!state->link)
> - return;
> -
> - state->an_complete = state->link;
> -
> - if (state->interface == PHY_INTERFACE_MODE_2500BASEX)
> - state->speed = SPEED_2500;
> - else
> - state->speed = SPEED_1000;
> -
> - state->duplex = DUPLEX_FULL;
> - state->pause = MLO_PAUSE_NONE;
> -}
> -
> -static void mt7531_pcs_get_state(struct phylink_pcs *pcs,
> - struct phylink_link_state *state)
> -{
> - struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
> - int port = pcs_to_mt753x_pcs(pcs)->port;
> -
> - if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> - mt7531_sgmii_pcs_get_state_an(priv, port, state);
> - return;
> - } else if ((state->interface == PHY_INTERFACE_MODE_1000BASEX) ||
> - (state->interface == PHY_INTERFACE_MODE_2500BASEX)) {
> - mt7531_sgmii_pcs_get_state_inband(priv, port, state);
> - return;
> - }
> -
> - state->link = false;
> -}
> -
> static int mt753x_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> phy_interface_t interface,
> const unsigned long *advertising,
> @@ -3081,18 +2885,57 @@ static const struct phylink_pcs_ops mt7530_pcs_ops = {
> .pcs_an_restart = mt7530_pcs_an_restart,
> };
>
> -static const struct phylink_pcs_ops mt7531_pcs_ops = {
> - .pcs_validate = mt753x_pcs_validate,
> - .pcs_get_state = mt7531_pcs_get_state,
> - .pcs_config = mt753x_pcs_config,
> - .pcs_an_restart = mt7531_pcs_an_restart,
> - .pcs_link_up = mt7531_pcs_link_up,
> +static int mt7530_regmap_read(void *context, unsigned int reg, unsigned int *val)
> +{
> + struct mt7530_priv *priv = context;
> +
> + *val = mt7530_read(priv, reg);
> + return 0;
> +};
> +
> +static int mt7530_regmap_write(void *context, unsigned int reg, unsigned int val)
> +{
> + struct mt7530_priv *priv = context;
> +
> + mt7530_write(priv, reg, val);
> + return 0;
> +};
> +
> +static int mt7530_regmap_update_bits(void *context, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + struct mt7530_priv *priv = context;
> +
> + mt7530_rmw(priv, reg, mask, val);
> + return 0;
> +};
> +
> +static const struct regmap_bus mt7531_regmap_bus = {
> + .reg_write = mt7530_regmap_write,
> + .reg_read = mt7530_regmap_read,
> + .reg_update_bits = mt7530_regmap_update_bits,
These new functions can be used for both switches, mt7530 and mt7531,
correct? If so, I believe they are supposed to be called mt753x since
88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a
new hardware").
mt753x: functions that can be used for mt7530 and mt7531 switches.
mt7530: functions specific to mt7530 switch.
mt7531: functions specific to mt7531 switch.
Arınç
On Tue, Mar 14, 2023 at 08:16:35PM +0300, Arınç ÜNAL wrote:
> On 9.03.2023 13:57, Daniel Golle wrote:
> > [...]
> > +static int mt7530_regmap_read(void *context, unsigned int reg, unsigned int *val)
> > +{
> > + struct mt7530_priv *priv = context;
> > +
> > + *val = mt7530_read(priv, reg);
> > + return 0;
> > +};
> > +
> > +static int mt7530_regmap_write(void *context, unsigned int reg, unsigned int val)
> > +{
> > + struct mt7530_priv *priv = context;
> > +
> > + mt7530_write(priv, reg, val);
> > + return 0;
> > +};
> > +
> > +static int mt7530_regmap_update_bits(void *context, unsigned int reg,
> > + unsigned int mask, unsigned int val)
> > +{
> > + struct mt7530_priv *priv = context;
> > +
> > + mt7530_rmw(priv, reg, mask, val);
> > + return 0;
> > +};
> > +
> > +static const struct regmap_bus mt7531_regmap_bus = {
> > + .reg_write = mt7530_regmap_write,
> > + .reg_read = mt7530_regmap_read,
> > + .reg_update_bits = mt7530_regmap_update_bits,
>
> These new functions can be used for both switches, mt7530 and mt7531,
> correct?
In theory, yes, they could be used on all switch ICs supported by the
mt7530.c driver. In practise they are used on MT7531 only because MT7530
and MT7621 don't have any SGMII/SerDes ports, but only MT7531 does.
> If so, I believe they are supposed to be called mt753x since
> 88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a new
> hardware").
>
> mt753x: functions that can be used for mt7530 and mt7531 switches.
> mt7530: functions specific to mt7530 switch.
> mt7531: functions specific to mt7531 switch.
Good catch, so mt7530_* is for sure not accurate. I used that naming
due to existing function names mt7530_read, mt7530_write and mt7530_rmw.
Given the situation I've explained above I think that mt753x_* would
be the best and I will change that for v14.
Thank you for reviewing!
Daniel
On 14/03/2023 20:45, Daniel Golle wrote:
> On Tue, Mar 14, 2023 at 08:16:35PM +0300, Arınç ÜNAL wrote:
>> On 9.03.2023 13:57, Daniel Golle wrote:
>>> [...]
>>> +static int mt7530_regmap_read(void *context, unsigned int reg, unsigned int *val)
>>> +{
>>> + struct mt7530_priv *priv = context;
>>> +
>>> + *val = mt7530_read(priv, reg);
>>> + return 0;
>>> +};
>>> +
>>> +static int mt7530_regmap_write(void *context, unsigned int reg, unsigned int val)
>>> +{
>>> + struct mt7530_priv *priv = context;
>>> +
>>> + mt7530_write(priv, reg, val);
>>> + return 0;
>>> +};
>>> +
>>> +static int mt7530_regmap_update_bits(void *context, unsigned int reg,
>>> + unsigned int mask, unsigned int val)
>>> +{
>>> + struct mt7530_priv *priv = context;
>>> +
>>> + mt7530_rmw(priv, reg, mask, val);
>>> + return 0;
>>> +};
>>> +
>>> +static const struct regmap_bus mt7531_regmap_bus = {
>>> + .reg_write = mt7530_regmap_write,
>>> + .reg_read = mt7530_regmap_read,
>>> + .reg_update_bits = mt7530_regmap_update_bits,
>>
>> These new functions can be used for both switches, mt7530 and mt7531,
>> correct?
>
> In theory, yes, they could be used on all switch ICs supported by the
> mt7530.c driver. In practise they are used on MT7531 only because MT7530
> and MT7621 don't have any SGMII/SerDes ports, but only MT7531 does.
>
>
>> If so, I believe they are supposed to be called mt753x since
>> 88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a new
>> hardware").
>>
>> mt753x: functions that can be used for mt7530 and mt7531 switches.
>> mt7530: functions specific to mt7530 switch.
>> mt7531: functions specific to mt7531 switch.
>
> Good catch, so mt7530_* is for sure not accurate. I used that naming
> due to existing function names mt7530_read, mt7530_write and mt7530_rmw.
I was going to send a mail to netdev mailing list to ask for opinions
whether we should rename the mt7530 DSA driver to mt753x and rename
these functions you mentioned to mt753x so it's crystal clear what code
is for what hardware. Now that we glossed over it here, I guess I can
ask it here instead.
>
> Given the situation I've explained above I think that mt753x_* would
> be the best and I will change that for v14.
>
> Thank you for reviewing!
Sounds good, cheers.
Arınç
On Tue, Mar 14, 2023 at 09:21:40PM +0300, Arınç ÜNAL wrote:
> I was going to send a mail to netdev mailing list to ask for opinions
> whether we should rename the mt7530 DSA driver to mt753x and rename these
> functions you mentioned to mt753x so it's crystal clear what code is for
> what hardware. Now that we glossed over it here, I guess I can ask it here
> instead.
My 2 cents - make your first 100 useful commits on this driver, which at
the very least produce a change in the compiled output, and then go on a
renaming spree as much as you want.
On 14/03/2023 22:53, Vladimir Oltean wrote:
> On Tue, Mar 14, 2023 at 09:21:40PM +0300, Arınç ÜNAL wrote:
>> I was going to send a mail to netdev mailing list to ask for opinions
>> whether we should rename the mt7530 DSA driver to mt753x and rename these
>> functions you mentioned to mt753x so it's crystal clear what code is for
>> what hardware. Now that we glossed over it here, I guess I can ask it here
>> instead.
>
> My 2 cents - make your first 100 useful commits on this driver, which at
> the very least produce a change in the compiled output, and then go on a
> renaming spree as much as you want.
Look, I don't ask for renaming just for the sake of renaming things. I
see a benefit which would make things clearer.
If you rather mean to, know the driver very well, by saying do 100
useful commits on the driver beforehand, that makes sense. But I think
I'm capable of managing this. I've got the time and energy.
Arınç
On Tue, Mar 14, 2023 at 11:59:40PM +0300, Arınç ÜNAL wrote:
> Look, I don't ask for renaming just for the sake of renaming things. I see a
> benefit which would make things clearer.
>
> If you rather mean to, know the driver very well, by saying do 100 useful
> commits on the driver beforehand, that makes sense. But I think I'm capable
> of managing this. I've got the time and energy.
I'm absolutely sure that you're capable of renaming mt7530 to mt753x,
that's outside the question. That change can be made without even paying
too much attention to the code, which is exactly the problem. If the
proposal is to touch mt7530_read(), mt7530_write(), mt7530_rmw()
(which it seems to be), then that's pretty much the entire driver.
Sorry for being skeptical by default, but generally such refactoring is
done by people who have the commitment to stay around when shit hits the
fan. Think of it as minimizing the time wasted by others due to that
refactoring. That could be time spent by reviewers looking at the code
being changed while trying to identify latent bugs; could be time spent
by someone who fixes a bug that doesn't backport all the way to stable
kernels because it conflicts with the refactoring. Ideally, after a
large refactoring, you would be sufficiently active to find and fix bugs
before others do, and have an eye for problematic code. Respectfully,
you still need to prove all these things. It also helps a lot if you
build a working relationship with the driver maintainers, or if you gain
their trust and become a maintainer yourself. Otherwise, more work will
just fall on the shoulders of fallback maintainers who don't have the
hardware. If there is a self-sustaining development community and they
take care of everything, I really have zero problems with large
refactoring done even by newbies. But the mt7530 maintainers have gone
pretty silent as of late, and I, as a fallback maintainer with no
hardware, have had to send 2 bug fixes to the mt7530 and 1 to the
mtk_eth_soc driver in the past month, to address the reports. Give me a
reason not to refuse more potential work :)
It's great that you have the time and energy, but with the symbolic 100
commits I just meant: start somewhere else within the driver, build the
experience, the knowledge of the development process and the appreciation
for the existing code structure.
On Wed, Mar 15, 2023 at 12:34:13AM +0200, Vladimir Oltean wrote:
> But the mt7530 maintainers have gone
> pretty silent as of late, and I, as a fallback maintainer with no
> hardware, have had to send 2 bug fixes to the mt7530 and 1 to the
> mtk_eth_soc driver in the past month, to address the reports.
That's an under-statement, and not just the DSA driver, but the
ethernet driver too. It's been years trying to get the attention
of anyone to sort out the pre-March 2020 crud in this driver, and
no one seems to care, and none of the alleged maintainers bother
replying.
I don't believe anyone who claims to be a maintainer for either the
DSA or Ethernet driver has commented on any of my patches or provided
any review.
In my mind, these drivers do not have maintainers.
What I have to resort to is spotting potential victims^wtesters on
the mailing lists to try stuff out for me - which is really not
how this should work.
It seems to me that this is the old "chuck code over the wall into
mainline, then do a runner" approach to kernel maintenance.
So, honestly, the sooner these drivers get proper maintainers, the
better - and if someone wants to step up to do that amongst those
who are involved in the current work, I'm all for that - it would
be a hell of a lot better than the current situation.
On 15.03.2023 01:34, Vladimir Oltean wrote:
> On Tue, Mar 14, 2023 at 11:59:40PM +0300, Arınç ÜNAL wrote:
>> Look, I don't ask for renaming just for the sake of renaming things. I see a
>> benefit which would make things clearer.
>>
>> If you rather mean to, know the driver very well, by saying do 100 useful
>> commits on the driver beforehand, that makes sense. But I think I'm capable
>> of managing this. I've got the time and energy.
>
> I'm absolutely sure that you're capable of renaming mt7530 to mt753x,
> that's outside the question. That change can be made without even paying
> too much attention to the code, which is exactly the problem. If the
> proposal is to touch mt7530_read(), mt7530_write(), mt7530_rmw()
> (which it seems to be), then that's pretty much the entire driver.
>
> Sorry for being skeptical by default, but generally such refactoring is
> done by people who have the commitment to stay around when shit hits the
> fan. Think of it as minimizing the time wasted by others due to that
> refactoring. That could be time spent by reviewers looking at the code
> being changed while trying to identify latent bugs; could be time spent
> by someone who fixes a bug that doesn't backport all the way to stable
> kernels because it conflicts with the refactoring. Ideally, after a
> large refactoring, you would be sufficiently active to find and fix bugs
> before others do, and have an eye for problematic code. Respectfully,
> you still need to prove all these things. It also helps a lot if you
> build a working relationship with the driver maintainers, or if you gain
> their trust and become a maintainer yourself. Otherwise, more work will
> just fall on the shoulders of fallback maintainers who don't have the
> hardware. If there is a self-sustaining development community and they
> take care of everything, I really have zero problems with large
> refactoring done even by newbies. But the mt7530 maintainers have gone
> pretty silent as of late, and I, as a fallback maintainer with no
> hardware, have had to send 2 bug fixes to the mt7530 and 1 to the
> mtk_eth_soc driver in the past month, to address the reports. Give me a
> reason not to refuse more potential work :)
Now, I can find bugs if it's something that would appear on a daily use
of the hardware, like those bugfixes you mentioned which I reported to
you. I'm not confident in fixing them myself (yet!) due to my very
slowly learning C but I'm willing to stick around for years to come so
who knows what happens in a few years. I already do keep an eye on a
very small problematic code at least.
I can be around as a maintainer to help backporting bugfixes that
wouldn't apply cleanly due to my refactoring. So I don't add more
workload to fallback maintainers like yourself. But that's all I can
promise to maintain for now, not because of availability but experience,
or rather the lack thereof.
Arınç
@@ -38,6 +38,7 @@ config NET_DSA_MT7530
tristate "MediaTek MT7530 and MT7531 Ethernet switch support"
select NET_DSA_TAG_MTK
select MEDIATEK_GE_PHY
+ select PCS_MTK_LYNXI
help
This enables support for the MediaTek MT7530 and MT7531 Ethernet
switch chips. Multi-chip module MT7530 in MT7621AT, MT7621DAT,
@@ -14,6 +14,7 @@
#include <linux/of_mdio.h>
#include <linux/of_net.h>
#include <linux/of_platform.h>
+#include <linux/pcs/pcs-mtk-lynxi.h>
#include <linux/phylink.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
@@ -2567,128 +2568,11 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
return 0;
}
-static void mt7531_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
- phy_interface_t interface, int speed, int duplex)
-{
- struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
- int port = pcs_to_mt753x_pcs(pcs)->port;
- unsigned int val;
-
- /* For adjusting speed and duplex of SGMII force mode. */
- if (interface != PHY_INTERFACE_MODE_SGMII ||
- phylink_autoneg_inband(mode))
- return;
-
- /* SGMII force mode setting */
- val = mt7530_read(priv, MT7531_SGMII_MODE(port));
- val &= ~MT7531_SGMII_IF_MODE_MASK;
-
- switch (speed) {
- case SPEED_10:
- val |= MT7531_SGMII_FORCE_SPEED_10;
- break;
- case SPEED_100:
- val |= MT7531_SGMII_FORCE_SPEED_100;
- break;
- case SPEED_1000:
- val |= MT7531_SGMII_FORCE_SPEED_1000;
- break;
- }
-
- /* MT7531 SGMII 1G force mode can only work in full duplex mode,
- * no matter MT7531_SGMII_FORCE_HALF_DUPLEX is set or not.
- *
- * The speed check is unnecessary as the MAC capabilities apply
- * this restriction. --rmk
- */
- if ((speed == SPEED_10 || speed == SPEED_100) &&
- duplex != DUPLEX_FULL)
- val |= MT7531_SGMII_FORCE_HALF_DUPLEX;
-
- mt7530_write(priv, MT7531_SGMII_MODE(port), val);
-}
-
static bool mt753x_is_mac_port(u32 port)
{
return (port == 5 || port == 6);
}
-static int mt7531_sgmii_setup_mode_force(struct mt7530_priv *priv, u32 port,
- phy_interface_t interface)
-{
- u32 val;
-
- if (!mt753x_is_mac_port(port))
- return -EINVAL;
-
- mt7530_set(priv, MT7531_QPHY_PWR_STATE_CTRL(port),
- MT7531_SGMII_PHYA_PWD);
-
- val = mt7530_read(priv, MT7531_PHYA_CTRL_SIGNAL3(port));
- val &= ~MT7531_RG_TPHY_SPEED_MASK;
- /* Setup 2.5 times faster clock for 2.5Gbps data speeds with 10B/8B
- * encoding.
- */
- val |= (interface == PHY_INTERFACE_MODE_2500BASEX) ?
- MT7531_RG_TPHY_SPEED_3_125G : MT7531_RG_TPHY_SPEED_1_25G;
- mt7530_write(priv, MT7531_PHYA_CTRL_SIGNAL3(port), val);
-
- mt7530_clear(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_ENABLE);
-
- /* MT7531 SGMII 1G and 2.5G force mode can only work in full duplex
- * mode, no matter MT7531_SGMII_FORCE_HALF_DUPLEX is set or not.
- */
- mt7530_rmw(priv, MT7531_SGMII_MODE(port),
- MT7531_SGMII_IF_MODE_MASK | MT7531_SGMII_REMOTE_FAULT_DIS,
- MT7531_SGMII_FORCE_SPEED_1000);
-
- mt7530_write(priv, MT7531_QPHY_PWR_STATE_CTRL(port), 0);
-
- return 0;
-}
-
-static int mt7531_sgmii_setup_mode_an(struct mt7530_priv *priv, int port,
- phy_interface_t interface)
-{
- if (!mt753x_is_mac_port(port))
- return -EINVAL;
-
- mt7530_set(priv, MT7531_QPHY_PWR_STATE_CTRL(port),
- MT7531_SGMII_PHYA_PWD);
-
- mt7530_rmw(priv, MT7531_PHYA_CTRL_SIGNAL3(port),
- MT7531_RG_TPHY_SPEED_MASK, MT7531_RG_TPHY_SPEED_1_25G);
-
- mt7530_set(priv, MT7531_SGMII_MODE(port),
- MT7531_SGMII_REMOTE_FAULT_DIS |
- MT7531_SGMII_SPEED_DUPLEX_AN);
-
- mt7530_rmw(priv, MT7531_PCS_SPEED_ABILITY(port),
- MT7531_SGMII_TX_CONFIG_MASK, 1);
-
- mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_ENABLE);
-
- mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_RESTART);
-
- mt7530_write(priv, MT7531_QPHY_PWR_STATE_CTRL(port), 0);
-
- return 0;
-}
-
-static void mt7531_pcs_an_restart(struct phylink_pcs *pcs)
-{
- struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
- int port = pcs_to_mt753x_pcs(pcs)->port;
- u32 val;
-
- /* Only restart AN when AN is enabled */
- val = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
- if (val & MT7531_SGMII_AN_ENABLE) {
- val |= MT7531_SGMII_AN_RESTART;
- mt7530_write(priv, MT7531_PCS_CONTROL_1(port), val);
- }
-}
-
static int
mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
phy_interface_t interface)
@@ -2711,11 +2595,11 @@ mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
phydev = dp->slave->phydev;
return mt7531_rgmii_setup(priv, port, interface, phydev);
case PHY_INTERFACE_MODE_SGMII:
- return mt7531_sgmii_setup_mode_an(priv, port, interface);
case PHY_INTERFACE_MODE_NA:
case PHY_INTERFACE_MODE_1000BASEX:
case PHY_INTERFACE_MODE_2500BASEX:
- return mt7531_sgmii_setup_mode_force(priv, port, interface);
+ /* handled in SGMII PCS driver */
+ return 0;
default:
return -EINVAL;
}
@@ -2740,11 +2624,11 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
switch (interface) {
case PHY_INTERFACE_MODE_TRGMII:
+ return &priv->pcs[port].pcs;
case PHY_INTERFACE_MODE_SGMII:
case PHY_INTERFACE_MODE_1000BASEX:
case PHY_INTERFACE_MODE_2500BASEX:
- return &priv->pcs[port].pcs;
-
+ return priv->ports[port].sgmii_pcs;
default:
return NULL;
}
@@ -2982,86 +2866,6 @@ static void mt7530_pcs_get_state(struct phylink_pcs *pcs,
state->pause |= MLO_PAUSE_TX;
}
-static int
-mt7531_sgmii_pcs_get_state_an(struct mt7530_priv *priv, int port,
- struct phylink_link_state *state)
-{
- u32 status, val;
- u16 config_reg;
-
- status = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
- state->link = !!(status & MT7531_SGMII_LINK_STATUS);
- state->an_complete = !!(status & MT7531_SGMII_AN_COMPLETE);
- if (state->interface == PHY_INTERFACE_MODE_SGMII &&
- (status & MT7531_SGMII_AN_ENABLE)) {
- val = mt7530_read(priv, MT7531_PCS_SPEED_ABILITY(port));
- config_reg = val >> 16;
-
- switch (config_reg & LPA_SGMII_SPD_MASK) {
- case LPA_SGMII_1000:
- state->speed = SPEED_1000;
- break;
- case LPA_SGMII_100:
- state->speed = SPEED_100;
- break;
- case LPA_SGMII_10:
- state->speed = SPEED_10;
- break;
- default:
- dev_err(priv->dev, "invalid sgmii PHY speed\n");
- state->link = false;
- return -EINVAL;
- }
-
- if (config_reg & LPA_SGMII_FULL_DUPLEX)
- state->duplex = DUPLEX_FULL;
- else
- state->duplex = DUPLEX_HALF;
- }
-
- return 0;
-}
-
-static void
-mt7531_sgmii_pcs_get_state_inband(struct mt7530_priv *priv, int port,
- struct phylink_link_state *state)
-{
- unsigned int val;
-
- val = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
- state->link = !!(val & MT7531_SGMII_LINK_STATUS);
- if (!state->link)
- return;
-
- state->an_complete = state->link;
-
- if (state->interface == PHY_INTERFACE_MODE_2500BASEX)
- state->speed = SPEED_2500;
- else
- state->speed = SPEED_1000;
-
- state->duplex = DUPLEX_FULL;
- state->pause = MLO_PAUSE_NONE;
-}
-
-static void mt7531_pcs_get_state(struct phylink_pcs *pcs,
- struct phylink_link_state *state)
-{
- struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
- int port = pcs_to_mt753x_pcs(pcs)->port;
-
- if (state->interface == PHY_INTERFACE_MODE_SGMII) {
- mt7531_sgmii_pcs_get_state_an(priv, port, state);
- return;
- } else if ((state->interface == PHY_INTERFACE_MODE_1000BASEX) ||
- (state->interface == PHY_INTERFACE_MODE_2500BASEX)) {
- mt7531_sgmii_pcs_get_state_inband(priv, port, state);
- return;
- }
-
- state->link = false;
-}
-
static int mt753x_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
phy_interface_t interface,
const unsigned long *advertising,
@@ -3081,18 +2885,57 @@ static const struct phylink_pcs_ops mt7530_pcs_ops = {
.pcs_an_restart = mt7530_pcs_an_restart,
};
-static const struct phylink_pcs_ops mt7531_pcs_ops = {
- .pcs_validate = mt753x_pcs_validate,
- .pcs_get_state = mt7531_pcs_get_state,
- .pcs_config = mt753x_pcs_config,
- .pcs_an_restart = mt7531_pcs_an_restart,
- .pcs_link_up = mt7531_pcs_link_up,
+static int mt7530_regmap_read(void *context, unsigned int reg, unsigned int *val)
+{
+ struct mt7530_priv *priv = context;
+
+ *val = mt7530_read(priv, reg);
+ return 0;
+};
+
+static int mt7530_regmap_write(void *context, unsigned int reg, unsigned int val)
+{
+ struct mt7530_priv *priv = context;
+
+ mt7530_write(priv, reg, val);
+ return 0;
+};
+
+static int mt7530_regmap_update_bits(void *context, unsigned int reg,
+ unsigned int mask, unsigned int val)
+{
+ struct mt7530_priv *priv = context;
+
+ mt7530_rmw(priv, reg, mask, val);
+ return 0;
+};
+
+static const struct regmap_bus mt7531_regmap_bus = {
+ .reg_write = mt7530_regmap_write,
+ .reg_read = mt7530_regmap_read,
+ .reg_update_bits = mt7530_regmap_update_bits,
+};
+
+#define MT7531_PCS_REGMAP_CONFIG(_name, _reg_base) \
+ { \
+ .name = _name, \
+ .reg_bits = 16, \
+ .val_bits = 32, \
+ .reg_stride = 4, \
+ .reg_base = _reg_base, \
+ .max_register = 0x17c, \
+ }
+
+static const struct regmap_config mt7531_pcs_config[] = {
+ MT7531_PCS_REGMAP_CONFIG("port5", MT7531_SGMII_REG_BASE(5)),
+ MT7531_PCS_REGMAP_CONFIG("port6", MT7531_SGMII_REG_BASE(6)),
};
static int
mt753x_setup(struct dsa_switch *ds)
{
struct mt7530_priv *priv = ds->priv;
+ struct regmap *regmap;
int i, ret;
/* Initialise the PCS devices */
@@ -3100,8 +2943,6 @@ mt753x_setup(struct dsa_switch *ds)
priv->pcs[i].pcs.ops = priv->info->pcs_ops;
priv->pcs[i].priv = priv;
priv->pcs[i].port = i;
- if (mt753x_is_mac_port(i))
- priv->pcs[i].pcs.poll = 1;
}
ret = priv->info->sw_setup(ds);
@@ -3116,6 +2957,16 @@ mt753x_setup(struct dsa_switch *ds)
if (ret && priv->irq)
mt7530_free_irq_common(priv);
+ if (priv->id == ID_MT7531)
+ for (i = 0; i < 2; i++) {
+ regmap = devm_regmap_init(ds->dev,
+ &mt7531_regmap_bus, priv,
+ &mt7531_pcs_config[i]);
+ priv->ports[5 + i].sgmii_pcs =
+ mtk_pcs_lynxi_create(ds->dev, regmap,
+ MT7531_PHYA_CTRL_SIGNAL3, 0);
+ }
+
return ret;
}
@@ -3211,7 +3062,7 @@ static const struct mt753x_info mt753x_table[] = {
},
[ID_MT7531] = {
.id = ID_MT7531,
- .pcs_ops = &mt7531_pcs_ops,
+ .pcs_ops = &mt7530_pcs_ops,
.sw_setup = mt7531_setup,
.phy_read_c22 = mt7531_ind_c22_phy_read,
.phy_write_c22 = mt7531_ind_c22_phy_write,
@@ -3321,7 +3172,7 @@ static void
mt7530_remove(struct mdio_device *mdiodev)
{
struct mt7530_priv *priv = dev_get_drvdata(&mdiodev->dev);
- int ret = 0;
+ int ret = 0, i;
if (!priv)
return;
@@ -3340,6 +3191,10 @@ mt7530_remove(struct mdio_device *mdiodev)
mt7530_free_irq(priv);
dsa_unregister_switch(priv->ds);
+
+ for (i = 0; i < 2; ++i)
+ mtk_pcs_lynxi_destroy(priv->ports[5 + i].sgmii_pcs);
+
mutex_destroy(&priv->reg_mutex);
}
@@ -364,47 +364,8 @@ enum mt7530_vlan_port_acc_frm {
CCR_TX_OCT_CNT_BAD)
/* MT7531 SGMII register group */
-#define MT7531_SGMII_REG_BASE 0x5000
-#define MT7531_SGMII_REG(p, r) (MT7531_SGMII_REG_BASE + \
- ((p) - 5) * 0x1000 + (r))
-
-/* Register forSGMII PCS_CONTROL_1 */
-#define MT7531_PCS_CONTROL_1(p) MT7531_SGMII_REG(p, 0x00)
-#define MT7531_SGMII_LINK_STATUS BIT(18)
-#define MT7531_SGMII_AN_ENABLE BIT(12)
-#define MT7531_SGMII_AN_RESTART BIT(9)
-#define MT7531_SGMII_AN_COMPLETE BIT(21)
-
-/* Register for SGMII PCS_SPPED_ABILITY */
-#define MT7531_PCS_SPEED_ABILITY(p) MT7531_SGMII_REG(p, 0x08)
-#define MT7531_SGMII_TX_CONFIG_MASK GENMASK(15, 0)
-#define MT7531_SGMII_TX_CONFIG BIT(0)
-
-/* Register for SGMII_MODE */
-#define MT7531_SGMII_MODE(p) MT7531_SGMII_REG(p, 0x20)
-#define MT7531_SGMII_REMOTE_FAULT_DIS BIT(8)
-#define MT7531_SGMII_IF_MODE_MASK GENMASK(5, 1)
-#define MT7531_SGMII_FORCE_DUPLEX BIT(4)
-#define MT7531_SGMII_FORCE_SPEED_MASK GENMASK(3, 2)
-#define MT7531_SGMII_FORCE_SPEED_1000 BIT(3)
-#define MT7531_SGMII_FORCE_SPEED_100 BIT(2)
-#define MT7531_SGMII_FORCE_SPEED_10 0
-#define MT7531_SGMII_SPEED_DUPLEX_AN BIT(1)
-
-enum mt7531_sgmii_force_duplex {
- MT7531_SGMII_FORCE_FULL_DUPLEX = 0,
- MT7531_SGMII_FORCE_HALF_DUPLEX = 0x10,
-};
-
-/* Fields of QPHY_PWR_STATE_CTRL */
-#define MT7531_QPHY_PWR_STATE_CTRL(p) MT7531_SGMII_REG(p, 0xe8)
-#define MT7531_SGMII_PHYA_PWD BIT(4)
-
-/* Values of SGMII SPEED */
-#define MT7531_PHYA_CTRL_SIGNAL3(p) MT7531_SGMII_REG(p, 0x128)
-#define MT7531_RG_TPHY_SPEED_MASK (BIT(2) | BIT(3))
-#define MT7531_RG_TPHY_SPEED_1_25G 0x0
-#define MT7531_RG_TPHY_SPEED_3_125G BIT(2)
+#define MT7531_SGMII_REG_BASE(p) (0x5000 + ((p) - 5) * 0x1000)
+#define MT7531_PHYA_CTRL_SIGNAL3 0x128
/* Register for system reset */
#define MT7530_SYS_CTRL 0x7000
@@ -703,13 +664,13 @@ struct mt7530_fdb {
* @pm: The matrix used to show all connections with the port.
* @pvid: The VLAN specified is to be considered a PVID at ingress. Any
* untagged frames will be assigned to the related VLAN.
- * @vlan_filtering: The flags indicating whether the port that can recognize
- * VLAN-tagged frames.
+ * @sgmii_pcs: Pointer to PCS instance for SerDes ports
*/
struct mt7530_port {
bool enable;
u32 pm;
u16 pvid;
+ struct phylink_pcs *sgmii_pcs;
};
/* Port 5 interface select definitions */