| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZqjoR-c9b3fjWoRa@lore-desk>
Date: Tue, 30 Jul 2024 15:19:03 +0200
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Arınç ÜNAL <arinc.unal@...nc9.com>
Cc: netdev@...r.kernel.org, daniel@...rotopia.org, dqfext@...il.com,
sean.wang@...iatek.com, andrew@...n.ch, f.fainelli@...il.com,
olteanv@...il.com, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, matthias.bgg@...il.com,
angelogioacchino.delregno@...labora.com,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, lorenzo.bianconi83@...il.com,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
devicetree@...r.kernel.org, upstream@...oha.com
Subject: Re: [PATCH net-next 2/2] net: dsa: mt7530: Add EN7581 support
> On 30/07/2024 10:46, Lorenzo Bianconi wrote:
> > Introduce support for the DSA built-in switch available on the EN7581
> > development board. EN7581 support is similar to MT7988 one except
> > it requires to set MT7530_FORCE_MODE bit in MT753X_PMCR_P register
> > for on cpu port.
> >
> > Tested-by: Benjamin Larsson <benjamin.larsson@...exis.eu>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> > ---
> > drivers/net/dsa/mt7530-mmio.c | 1 +
> > drivers/net/dsa/mt7530.c | 38 +++++++++++++++++++++++++++++++----
> > drivers/net/dsa/mt7530.h | 16 ++++++++++-----
> > 3 files changed, 46 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/dsa/mt7530-mmio.c b/drivers/net/dsa/mt7530-mmio.c
> > index b74a230a3f13..10dc49961f15 100644
> > --- a/drivers/net/dsa/mt7530-mmio.c
> > +++ b/drivers/net/dsa/mt7530-mmio.c
> > @@ -11,6 +11,7 @@
> > #include "mt7530.h"
> > static const struct of_device_id mt7988_of_match[] = {
> > + { .compatible = "airoha,en7581-switch", .data = &mt753x_table[ID_EN7581], },
> > { .compatible = "mediatek,mt7988-switch", .data = &mt753x_table[ID_MT7988], },
> > { /* sentinel */ },
> > };
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index ec18e68bf3a8..8adc4561c5b2 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -1152,7 +1152,8 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
> > * the MT7988 SoC. Trapped frames will be forwarded to the CPU port that
> > * is affine to the inbound user port.
> > */
> > - if (priv->id == ID_MT7531 || priv->id == ID_MT7988)
> > + if (priv->id == ID_MT7531 || priv->id == ID_MT7988 ||
> > + priv->id == ID_EN7581)
> > mt7530_set(priv, MT7531_CFC, MT7531_CPU_PMAP(BIT(port)));
> > /* CPU port gets connected to all user ports of
> > @@ -2207,7 +2208,7 @@ mt7530_setup_irq(struct mt7530_priv *priv)
> > return priv->irq ? : -EINVAL;
> > }
> > - if (priv->id == ID_MT7988)
> > + if (priv->id == ID_MT7988 || priv->id == ID_EN7581)
> > priv->irq_domain = irq_domain_add_linear(np, MT7530_NUM_PHYS,
> > &mt7988_irq_domain_ops,
> > priv);
> > @@ -2766,7 +2767,7 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
> > {
> > switch (port) {
> > /* Ports which are connected to switch PHYs. There is no MII pinout. */
> > - case 0 ... 3:
> > + case 0 ... 4:
>
> Please create a new function, such as en7581_mac_port_get_caps().
ack, I will do in v2.
>
> > __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > config->supported_interfaces);
> > @@ -2850,6 +2851,23 @@ mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > }
> > }
> > +static void
> > +en7581_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > + phy_interface_t interface)
> > +{
> > + /* BIT(31-27): reserved
> > + * BIT(26): TX_CRC_EN: enable(0)/disable(1) CRC insertion
> > + * BIT(25): RX_CRC_EN: enable(0)/disable(1) CRC insertion
> > + * Since the bits above have a different meaning with respect to the
> > + * one described in mt7530.h, set default values.
> > + */
> > + mt7530_clear(ds->priv, MT753X_PMCR_P(port), MT7531_FORCE_MODE_MASK);
> > + if (dsa_is_cpu_port(ds, port)) {
> > + /* enable MT7530_FORCE_MODE on cpu port */
> > + mt7530_set(ds->priv, MT753X_PMCR_P(port), MT7530_FORCE_MODE);
> > + }
> > +}
>
> This seems to undo "Clear link settings and enable force mode to force link
> down on all ports until they're enabled later." on mt7531_setup_common()
> and redo it only for the CPU port. It should be so that force mode is
> enabled on all ports. You could position the diff below as a patch before
> this patch. It introduces the MT753X_FORCE_MODE() macro to choose the
> correct constant for the switch model.
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index ec18e68bf3a8..4915264c460f 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2438,8 +2438,10 @@ mt7530_setup(struct dsa_switch *ds)
> /* Clear link settings and enable force mode to force link down
> * on all ports until they're enabled later.
> */
> - mt7530_rmw(priv, MT753X_PMCR_P(i), PMCR_LINK_SETTINGS_MASK |
> - MT7530_FORCE_MODE, MT7530_FORCE_MODE);
> + mt7530_rmw(priv, MT753X_PMCR_P(i),
> + PMCR_LINK_SETTINGS_MASK |
> + MT753X_FORCE_MODE(priv->id),
> + MT753X_FORCE_MODE(priv->id));
> /* Disable forwarding by default on all ports */
> mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
> @@ -2550,8 +2552,10 @@ mt7531_setup_common(struct dsa_switch *ds)
> /* Clear link settings and enable force mode to force link down
> * on all ports until they're enabled later.
> */
> - mt7530_rmw(priv, MT753X_PMCR_P(i), PMCR_LINK_SETTINGS_MASK |
> - MT7531_FORCE_MODE_MASK, MT7531_FORCE_MODE_MASK);
> + mt7530_rmw(priv, MT753X_PMCR_P(i),
> + PMCR_LINK_SETTINGS_MASK |
> + MT753X_FORCE_MODE(priv->id),
> + MT753X_FORCE_MODE(priv->id));
> /* Disable forwarding by default on all ports */
> mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 28592123070b..d47d1ce511ba 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -355,6 +355,10 @@ enum mt7530_vlan_port_acc_frm {
> MT7531_FORCE_MODE_TX_FC | \
> MT7531_FORCE_MODE_EEE100 | \
> MT7531_FORCE_MODE_EEE1G)
> +#define MT753X_FORCE_MODE(id) ((id == ID_MT7531 || \
> + id == ID_MT7988) ? \
> + MT7531_FORCE_MODE_MASK : \
> + MT7530_FORCE_MODE)
> #define PMCR_LINK_SETTINGS_MASK (PMCR_MAC_TX_EN | PMCR_MAC_RX_EN | \
> PMCR_FORCE_EEE1G | \
> PMCR_FORCE_EEE100 | \
ack, fine. I will merge this change in v2.
>
> > +
> > static struct phylink_pcs *
> > mt753x_phylink_mac_select_pcs(struct phylink_config *config,
> > phy_interface_t interface)
> > @@ -2880,7 +2898,8 @@ mt753x_phylink_mac_config(struct phylink_config *config, unsigned int mode,
> > priv = ds->priv;
> > - if ((port == 5 || port == 6) && priv->info->mac_port_config)
> > + if ((port == 5 || port == 6 || priv->id == ID_EN7581) &&
> > + priv->info->mac_port_config)
> > priv->info->mac_port_config(ds, port, mode, state->interface);
> > /* Are we connected to external phy */
> > @@ -3220,6 +3239,17 @@ const struct mt753x_info mt753x_table[] = {
> > .phy_write_c45 = mt7531_ind_c45_phy_write,
> > .mac_port_get_caps = mt7988_mac_port_get_caps,
> > },
> > + [ID_EN7581] = {
> > + .id = ID_EN7581,
> > + .pcs_ops = &mt7530_pcs_ops,
> > + .sw_setup = mt7988_setup,
> > + .phy_read_c22 = mt7531_ind_c22_phy_read,
> > + .phy_write_c22 = mt7531_ind_c22_phy_write,
> > + .phy_read_c45 = mt7531_ind_c45_phy_read,
> > + .phy_write_c45 = mt7531_ind_c45_phy_write,
> > + .mac_port_get_caps = mt7988_mac_port_get_caps,
> > + .mac_port_config = en7581_mac_config,
> > + },
>
> Let me lend a hand; you can apply this diff on top of this patch.
Thx :)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index b18b98a53a7d..f5766d8ae360 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2768,6 +2768,28 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
> static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
> struct phylink_config *config)
> +{
> + switch (port) {
> + /* Ports which are connected to switch PHYs. There is no MII pinout. */
> + case 0 ... 3:
> + __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> + config->supported_interfaces);
> +
> + config->mac_capabilities |= MAC_10 | MAC_100 | MAC_1000FD;
> + break;
> +
> + /* Port 6 is connected to SoC's XGMII MAC. There is no MII pinout. */
> + case 6:
> + __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> + config->supported_interfaces);
> +
> + config->mac_capabilities |= MAC_10000FD;
> + break;
> + }
> +}
> +
> +static void en7581_mac_port_get_caps(struct dsa_switch *ds, int port,
> + struct phylink_config *config)
> {
> switch (port) {
> /* Ports which are connected to switch PHYs. There is no MII pinout. */
> @@ -2855,23 +2877,6 @@ mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> }
> }
> -static void
> -en7581_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> - phy_interface_t interface)
> -{
> - /* BIT(31-27): reserved
> - * BIT(26): TX_CRC_EN: enable(0)/disable(1) CRC insertion
> - * BIT(25): RX_CRC_EN: enable(0)/disable(1) CRC insertion
> - * Since the bits above have a different meaning with respect to the
> - * one described in mt7530.h, set default values.
> - */
> - mt7530_clear(ds->priv, MT753X_PMCR_P(port), MT7531_FORCE_MODE_MASK);
> - if (dsa_is_cpu_port(ds, port)) {
> - /* enable MT7530_FORCE_MODE on cpu port */
> - mt7530_set(ds->priv, MT753X_PMCR_P(port), MT7530_FORCE_MODE);
> - }
> -}
> -
> static struct phylink_pcs *
> mt753x_phylink_mac_select_pcs(struct phylink_config *config,
> phy_interface_t interface)
> @@ -2902,8 +2907,7 @@ mt753x_phylink_mac_config(struct phylink_config *config, unsigned int mode,
> priv = ds->priv;
> - if ((port == 5 || port == 6 || priv->id == ID_EN7581) &&
> - priv->info->mac_port_config)
> + if ((port == 5 || port == 6) && priv->info->mac_port_config)
> priv->info->mac_port_config(ds, port, mode, state->interface);
> /* Are we connected to external phy */
> @@ -3251,8 +3255,7 @@ const struct mt753x_info mt753x_table[] = {
> .phy_write_c22 = mt7531_ind_c22_phy_write,
> .phy_read_c45 = mt7531_ind_c45_phy_read,
> .phy_write_c45 = mt7531_ind_c45_phy_write,
> - .mac_port_get_caps = mt7988_mac_port_get_caps,
> - .mac_port_config = en7581_mac_config,
> + .mac_port_get_caps = en7581_mac_port_get_caps,
> },
> };
> EXPORT_SYMBOL_GPL(mt753x_table);
>
> I don't know this hardware so please make sure the comments on
> en7581_mac_port_get_caps() are correct. I didn't compile this so please
> make sure it works.
ack, I tested it and it works fine.
Regards,
Lorenzo
>
> By the way, is this supposed to be AN7581? There's EN7580 but no EN7581 on
> the Airoha website.
>
> https://www.airoha.com/products/y1cQz8EpjIKhbK61
>
> Arınç
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists