[PATCH net-next 00/15] MT7530 DSA subdriver improvements

[PATCH net-next 00/15] MT7530 DSA subdriver improvements

[Arınç ÜNAL]

Hello!

This patch series simplifies the MT7530 DSA subdriver and improves the
logic of the support for MT7530, MT7531, and the switch on the MT7988 SoC.

I have done a simple ping test to confirm basic communication on all switch
ports on the MCM and standalone MT7530 and MT7531 switch with this patch
series applied.

MT7621 Unielec, MCM MT7530:

rgmii-only-gmac0-mt7621-unielec-u7621-06-16m.dtb
gmac0-and-gmac1-mt7621-unielec-u7621-06-16m.dtb

tftpboot 0x80008000 mips-uzImage.bin; tftpboot 0x83000000 mips-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootm 0x80008000 0x83000000 0x83f00000

MT7622 Bananapi, MT7531:

gmac0-and-gmac1-mt7622-bananapi-bpi-r64.dtb

tftpboot 0x40000000 arm64-Image; tftpboot 0x45000000 arm64-rootfs.cpio.uboot; tftpboot 0x4a000000 $dtb; booti 0x40000000 0x45000000 0x4a000000

MT7623 Bananapi, standalone MT7530:

rgmii-only-gmac0-mt7623n-bananapi-bpi-r2.dtb
gmac0-and-gmac1-mt7623n-bananapi-bpi-r2.dtb

tftpboot 0x80008000 arm-zImage; tftpboot 0x83000000 arm-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootz 0x80008000 0x83000000 0x83f00000

This patch series is the continuation of the one linked below.

https://lore.kernel.org/netdev/20230522121532.86610-1-arinc.unal@arinc9.com/

Arınç

Arınç ÜNAL (15):
  net: dsa: mt7530: always trap frames to active CPU port on MT7530
  net: dsa: mt7530: use p5_interface_select as data type for p5_intf_sel
  net: dsa: mt7530: store port 5 SGMII capability of MT7531
  net: dsa: mt7530: improve comments regarding port 5 and 6
  net: dsa: mt7530: improve code path for setting up port 5
  net: dsa: mt7530: do not set priv->p5_interface on mt7530_setup_port5()
  net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled
  net: dsa: mt7530: empty default case on mt7530_setup_port5()
  net: dsa: mt7530: call port 6 setup from mt7530_mac_config()
  net: dsa: mt7530: remove pad_setup function pointer
  net: dsa: mt7530: move XTAL check to mt7530_setup()
  net: dsa: mt7530: move enabling port 6 to mt7530_setup_port6()
  net: dsa: mt7530: simplify mt7530_setup_port6() and change to void
  net: dsa: mt7530: correct port capabilities of MT7988
  net: dsa: mt7530: do not clear config->supported_interfaces

 drivers/net/dsa/mt7530-mdio.c |   7 +-
 drivers/net/dsa/mt7530.c      | 283 ++++++++++++++++---------------------
 drivers/net/dsa/mt7530.h      |  19 +--
 3 files changed, 137 insertions(+), 172 deletions(-)

[PATCH net-next 01/15] net: dsa: mt7530: always trap frames to active CPU port on MT7530

[Arınç ÜNAL]

On the MT7530 switch, the CPU_PORT field indicates which CPU port to trap
frames to, regardless of the affinity of the inbound user port.

When multiple CPU ports are in use, if the DSA conduit interface is down,
trapped frames won't be passed to the conduit interface.

To make trapping frames work including this case, implement
ds->ops->master_state_change() on this subdriver and set the CPU_PORT field
to the numerically smallest CPU port which the DSA conduit interface its
affine to is up. Introduce the active_cpu_ports field to store the
information of the active CPU ports. Correct the macros, CPU_PORT is bits 4
through 6 of the register.

Add a comment to explain frame trapping for this switch.

Currently, the driver doesn't support the use of multiple CPU ports so this
is not necessarily a bug fix.

Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 30 ++++++++++++++++++++++++++----
 drivers/net/dsa/mt7530.h |  6 ++++--
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index d27c6b70a2f6..442492d62670 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1035,10 +1035,6 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
 	mt7530_set(priv, MT7530_MFC, BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) |
 		   UNU_FFP(BIT(port)));
 
-	/* Set CPU port number */
-	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
-		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
-
 	/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
 	 * the MT7988 SoC. Trapped frames will be forwarded to the CPU port that
 	 * is affine to the inbound user port.
@@ -3075,6 +3071,31 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static void
+mt753x_conduit_state_change(struct dsa_switch *ds,
+			    const struct net_device *conduit,
+			    bool operational)
+{
+	struct mt7530_priv *priv = ds->priv;
+	struct dsa_port *cpu_dp = conduit->dsa_ptr;
+
+	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
+	 * forwarded to the numerically smallest CPU port which the DSA conduit
+	 * interface its affine to is up.
+	 */
+	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
+		return;
+
+	if (operational)
+		priv->active_cpu_ports |= BIT(cpu_dp->index);
+	else
+		priv->active_cpu_ports &= ~BIT(cpu_dp->index);
+
+	if (priv->active_cpu_ports)
+		mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, CPU_EN |
+			   CPU_PORT(__ffs(priv->active_cpu_ports)));
+}
+
 static int mt7988_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
 {
 	return 0;
@@ -3130,6 +3151,7 @@ const struct dsa_switch_ops mt7530_switch_ops = {
 	.phylink_mac_link_up	= mt753x_phylink_mac_link_up,
 	.get_mac_eee		= mt753x_get_mac_eee,
 	.set_mac_eee		= mt753x_set_mac_eee,
+	.conduit_state_change	= mt753x_conduit_state_change,
 };
 EXPORT_SYMBOL_GPL(mt7530_switch_ops);
 
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 17e42d30fff4..96d610f5bcf9 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -41,8 +41,8 @@ enum mt753x_id {
 #define  UNU_FFP(x)			(((x) & 0xff) << 8)
 #define  UNU_FFP_MASK			UNU_FFP(~0)
 #define  CPU_EN				BIT(7)
-#define  CPU_PORT(x)			((x) << 4)
-#define  CPU_MASK			(0xf << 4)
+#define  CPU_PORT_MASK			GENMASK(6, 4)
+#define  CPU_PORT(x)			FIELD_PREP(CPU_PORT_MASK, x)
 #define  MIRROR_EN			BIT(3)
 #define  MIRROR_PORT(x)			((x) & 0x7)
 #define  MIRROR_MASK			0x7
@@ -760,6 +760,7 @@ struct mt753x_info {
  * @irq_domain:		IRQ domain of the switch irq_chip
  * @irq_enable:		IRQ enable bits, synced to SYS_INT_EN
  * @create_sgmii:	Pointer to function creating SGMII PCS instance(s)
+ * @active_cpu_ports:	Holding the active CPU ports
  */
 struct mt7530_priv {
 	struct device		*dev;
@@ -786,6 +787,7 @@ struct mt7530_priv {
 	struct irq_domain *irq_domain;
 	u32 irq_enable;
 	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
+	unsigned long active_cpu_ports;
 };
 
 struct mt7530_hw_vlan_entry {
-- 
2.40.1

Re: [PATCH net-next 01/15] net: dsa: mt7530: always trap frames to active CPU port on MT7530

[Russell King (Oracle)]

On Sat, Nov 18, 2023 at 03:31:51PM +0300, Arınç ÜNAL wrote:
> +	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
> +	 * forwarded to the numerically smallest CPU port which the DSA conduit
> +	 * interface its affine to is up.
> +	 */
> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> +		return;
> +
> +	if (operational)
> +		priv->active_cpu_ports |= BIT(cpu_dp->index);
> +	else
> +		priv->active_cpu_ports &= ~BIT(cpu_dp->index);
> +
> +	if (priv->active_cpu_ports)
> +		mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, CPU_EN |
> +			   CPU_PORT(__ffs(priv->active_cpu_ports)));

I would be tempted to write this as:

	mask = BIT(cpu_dp->index);

	if (operational)
		priv->active_cpu_ports |= mask;
	else
		priv->active_cpu_ports &= ~mask;

Now, what happens when active_cpu_ports is zero? Doesn't that mean there
is no active CPU port? In which case, wouldn't disabling the CPU port
direction be appropriate, such as:

	if (priv->active_cpu_ports)
		val = CPU_EN | CPU_PORT(__ffs(priv->active_cpu_ports));
	else
		val = 0;

	mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);	

?

>  struct mt7530_priv {
>  	struct device		*dev;
> @@ -786,6 +787,7 @@ struct mt7530_priv {
>  	struct irq_domain *irq_domain;
>  	u32 irq_enable;
>  	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
> +	unsigned long active_cpu_ports;

So this will be 32 or 64 bit in size. Presumably you know how many CPU
ports there can be, which looking at this code must be less than 8 as
CPU_PORT_MASK is only 3 bits in size. So, maybe use a u8, and check
that cpu_dp->index <= 7 ?

I would also suggest moving irq_enable after create_sgmii, to avoid
holes in the struct.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 01/15] net: dsa: mt7530: always trap frames to active CPU port on MT7530

[Arınç ÜNAL]

On 18.11.2023 17:34, Russell King (Oracle) wrote:
> On Sat, Nov 18, 2023 at 03:31:51PM +0300, Arınç ÜNAL wrote:
>> +	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
>> +	 * forwarded to the numerically smallest CPU port which the DSA conduit
>> +	 * interface its affine to is up.
>> +	 */
>> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
>> +		return;
>> +
>> +	if (operational)
>> +		priv->active_cpu_ports |= BIT(cpu_dp->index);
>> +	else
>> +		priv->active_cpu_ports &= ~BIT(cpu_dp->index);
>> +
>> +	if (priv->active_cpu_ports)
>> +		mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, CPU_EN |
>> +			   CPU_PORT(__ffs(priv->active_cpu_ports)));
> 
> I would be tempted to write this as:
> 
> 	mask = BIT(cpu_dp->index);
> 
> 	if (operational)
> 		priv->active_cpu_ports |= mask;
> 	else
> 		priv->active_cpu_ports &= ~mask;
> 
> Now, what happens when active_cpu_ports is zero? Doesn't that mean there
> is no active CPU port? In which case, wouldn't disabling the CPU port
> direction be appropriate, such as:
> 
> 	if (priv->active_cpu_ports)
> 		val = CPU_EN | CPU_PORT(__ffs(priv->active_cpu_ports));
> 	else
> 		val = 0;
> 
> 	mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);	
> 
> ?

In practice, it doesn't seem to matter. The CPU_EN bit enables the CPU port
defined on CPU_PORT_MASK which is used for trapping frames. No active CPU
ports would mean that all the DSA conduits are down. In that case, all the
user ports will be down also. So there won't be any traffic. But disabling
it is of course more appropriate here.

> 
>>   struct mt7530_priv {
>>   	struct device		*dev;
>> @@ -786,6 +787,7 @@ struct mt7530_priv {
>>   	struct irq_domain *irq_domain;
>>   	u32 irq_enable;
>>   	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
>> +	unsigned long active_cpu_ports;
> 
> So this will be 32 or 64 bit in size. Presumably you know how many CPU
> ports there can be, which looking at this code must be less than 8 as
> CPU_PORT_MASK is only 3 bits in size. So, maybe use a u8, and check
> that cpu_dp->index <= 7 ?

Aren't there other mechanisms to check that cpu_dp->index is a valid port?
At least with phylink_get_caps(), only ports lower than 7 will have proper
interface modes allowed.

Here's the code after you and Vladimir's review:

static void
mt753x_conduit_state_change(struct dsa_switch *ds,
			    const struct net_device *conduit,
			    bool operational)
{
	struct dsa_port *cpu_dp = conduit->dsa_ptr;
	struct mt7530_priv *priv = ds->priv;
	u8 mask;
	int val;

	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
	 * forwarded to the numerically smallest CPU port which the DSA conduit
	 * interface its affine to is up.
	 */
	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
		return;

	mask = BIT(cpu_dp->index);

	if (operational)
		priv->active_cpu_ports |= mask;
	else
		priv->active_cpu_ports &= ~mask;

	if (priv->active_cpu_ports)
		val = CPU_EN | CPU_PORT(__ffs(priv->active_cpu_ports));
	else
		val = 0;

	mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);
}

struct mt7530_priv {
	[...]
	u8 active_cpu_ports;
};

> 
> I would also suggest moving irq_enable after create_sgmii, to avoid
> holes in the struct.

Sorry, I've got no idea about this. Could you explain why would there
possibly be holes in the struct with the current ordering of the members of
the mt7530_priv structure?

Arınç

Re: [PATCH net-next 01/15] net: dsa: mt7530: always trap frames to active CPU port on MT7530

[Vladimir Oltean]

On Sat, Dec 02, 2023 at 11:29:18AM +0300, Arınç ÜNAL wrote:
> > I would be tempted to write this as:
> > 
> > 	mask = BIT(cpu_dp->index);
> > 
> > 	if (operational)
> > 		priv->active_cpu_ports |= mask;
> > 	else
> > 		priv->active_cpu_ports &= ~mask;
> > 
> > Now, what happens when active_cpu_ports is zero? Doesn't that mean there
> > is no active CPU port? In which case, wouldn't disabling the CPU port
> > direction be appropriate, such as:
> > 
> > 	if (priv->active_cpu_ports)
> > 		val = CPU_EN | CPU_PORT(__ffs(priv->active_cpu_ports));
> > 	else
> > 		val = 0;
> > 
> > 	mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);	
> > 
> > ?
> 
> In practice, it doesn't seem to matter. The CPU_EN bit enables the CPU port
> defined on CPU_PORT_MASK which is used for trapping frames. No active CPU
> ports would mean that all the DSA conduits are down. In that case, all the
> user ports will be down also. So there won't be any traffic. But disabling
> it is of course more appropriate here.

Ack, DSA takes down the user ports which are affine to a certain conduit
interface when that goes down. See the NETDEV_GOING_DOWN handling in
net/dsa/user.c.

> > 
> > >   struct mt7530_priv {
> > >   	struct device		*dev;
> > > @@ -786,6 +787,7 @@ struct mt7530_priv {
> > >   	struct irq_domain *irq_domain;
> > >   	u32 irq_enable;
> > >   	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
> > > +	unsigned long active_cpu_ports;
> > 
> > So this will be 32 or 64 bit in size. Presumably you know how many CPU
> > ports there can be, which looking at this code must be less than 8 as
> > CPU_PORT_MASK is only 3 bits in size. So, maybe use a u8, and check
> > that cpu_dp->index <= 7 ?

We picked "unsigned long" as storage because that's also the size of the
argument that __ffs() takes. But admittedly, we could have also stored a
smaller variable and promote it to unsigned long when we pass it to __ffs().

> Aren't there other mechanisms to check that cpu_dp->index is a valid port?

cpu_dp->index is guaranteed by DSA to be valid (according to the "reg"
value from the device tree and smaller than ds->num_ports). It's just a
question of balancing this kind of optimization with the possibility
that a future switch appears which has more than MT7530_NUM_PORTS (7) ports.

> At least with phylink_get_caps(), only ports lower than 7 will have proper
> interface modes allowed.
> 
> Here's the code after you and Vladimir's review:
> 
> static void
> mt753x_conduit_state_change(struct dsa_switch *ds,
> 			    const struct net_device *conduit,
> 			    bool operational)
> {
> 	struct dsa_port *cpu_dp = conduit->dsa_ptr;
> 	struct mt7530_priv *priv = ds->priv;
> 	u8 mask;
> 	int val;
> 
> 	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
> 	 * forwarded to the numerically smallest CPU port which the DSA conduit
> 	 * interface its affine to is up.
> 	 */
> 	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> 		return;
> 
> 	mask = BIT(cpu_dp->index);
> 
> 	if (operational)
> 		priv->active_cpu_ports |= mask;
> 	else
> 		priv->active_cpu_ports &= ~mask;
> 
> 	if (priv->active_cpu_ports)
> 		val = CPU_EN | CPU_PORT(__ffs(priv->active_cpu_ports));
> 	else
> 		val = 0;

You could initialize "val" with 0 at declaration time and you wouldn't
need the "else" branch.

> 
> 	mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);
> }
> 
> struct mt7530_priv {
> 	[...]
> 	u8 active_cpu_ports;
> };

Actually, looking at the code now, I don't understand why we even keep
track of the active_cpu_ports mask in the driver. We could read the
MT7530_MFC register in mt753x_conduit_state_change(), flip the bit
corresponding just to cpu_dp->index (rather than rmw all of CPU_PORT_MASK),
and write back the result. And to address Russell's concern, we could test
whether the resulting CPU_PORT_MASK portion of what we're going to write
back is all-zeroes or not, and if it is, clear the CPU_EN bit, otherwise
set it.

> 
> > 
> > I would also suggest moving irq_enable after create_sgmii, to avoid
> > holes in the struct.
> 
> Sorry, I've got no idea about this. Could you explain why would there
> possibly be holes in the struct with the current ordering of the members of
> the mt7530_priv structure?
> 
> Arınç

FWIW:

$ pahole -C mt7530_priv $KBUILD_OUTPUT/drivers/net/dsa/mt7530.o
struct mt7530_priv {
        struct device *            dev;                  /*     0     8 */
        struct dsa_switch *        ds;                   /*     8     8 */
        struct mii_bus *           bus;                  /*    16     8 */
        struct regmap *            regmap;               /*    24     8 */
        struct reset_control *     rstc;                 /*    32     8 */
        struct regulator *         core_pwr;             /*    40     8 */
        struct regulator *         io_pwr;               /*    48     8 */
        struct gpio_desc *         reset;                /*    56     8 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        const struct mt753x_info  * info;                /*    64     8 */
        unsigned int               id;                   /*    72     4 */
        bool                       mcm;                  /*    76     1 */

        /* XXX 3 bytes hole, try to pack */

        phy_interface_t            p6_interface;         /*    80     4 */
        phy_interface_t            p5_interface;         /*    84     4 */
        unsigned int               p5_intf_sel;          /*    88     4 */
        u8                         mirror_rx;            /*    92     1 */
        u8                         mirror_tx;            /*    93     1 */

        /* XXX 2 bytes hole, try to pack */

        struct mt7530_port         ports[7];             /*    96   168 */
        /* --- cacheline 4 boundary (256 bytes) was 8 bytes ago --- */
        struct mt753x_pcs          pcs[7];               /*   264   280 */
        /* --- cacheline 8 boundary (512 bytes) was 32 bytes ago --- */
        struct mutex               reg_mutex;            /*   544    32 */
        /* --- cacheline 9 boundary (576 bytes) --- */
        int                        irq;                  /*   576     4 */

        /* XXX 4 bytes hole, try to pack */

        struct irq_domain *        irq_domain;           /*   584     8 */
        u32                        irq_enable;           /*   592     4 */

        /* XXX 4 bytes hole, try to pack */

        int                        (*create_sgmii)(struct mt7530_priv *, bool); /*   600     8 */
        unsigned long              active_cpu_ports;     /*   608     8 */

        /* size: 616, cachelines: 10, members: 24 */
        /* sum members: 603, holes: 4, sum holes: 13 */
        /* last cacheline: 40 bytes */
};

It's not like this makes any practical difference, as struct mt7530_priv
isn't used from hot paths, but tidying it up is a good sign of clean,
careful development, and of understanding memory alignment.

Re: [PATCH net-next 01/15] net: dsa: mt7530: always trap frames to active CPU port on MT7530

[Arınç ÜNAL]

On 7.12.2023 20:48, Vladimir Oltean wrote:
> On Sat, Dec 02, 2023 at 11:29:18AM +0300, Arınç ÜNAL wrote:
>>>>    struct mt7530_priv {
>>>>    	struct device		*dev;
>>>> @@ -786,6 +787,7 @@ struct mt7530_priv {
>>>>    	struct irq_domain *irq_domain;
>>>>    	u32 irq_enable;
>>>>    	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
>>>> +	unsigned long active_cpu_ports;
>>>
>>> So this will be 32 or 64 bit in size. Presumably you know how many CPU
>>> ports there can be, which looking at this code must be less than 8 as
>>> CPU_PORT_MASK is only 3 bits in size. So, maybe use a u8, and check
>>> that cpu_dp->index <= 7 ?
> 
> We picked "unsigned long" as storage because that's also the size of the
> argument that __ffs() takes. But admittedly, we could have also stored a
> smaller variable and promote it to unsigned long when we pass it to __ffs().
> 
>> Aren't there other mechanisms to check that cpu_dp->index is a valid port?
> 
> cpu_dp->index is guaranteed by DSA to be valid (according to the "reg"
> value from the device tree and smaller than ds->num_ports). It's just a
> question of balancing this kind of optimization with the possibility
> that a future switch appears which has more than MT7530_NUM_PORTS (7) ports.

If this was to happen - as unlikely as I find it - I would suggest adding a
new MTXXXX_NUM_PORTS and set priv->ds->num_ports to it after checking
priv->id. I'm a maintainer here so I'll keep an eye out.

> 
>>
>> 	mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);
>> }
>>
>> struct mt7530_priv {
>> 	[...]
>> 	u8 active_cpu_ports;
>> };
> 
> Actually, looking at the code now, I don't understand why we even keep
> track of the active_cpu_ports mask in the driver. We could read the
> MT7530_MFC register in mt753x_conduit_state_change(), flip the bit
> corresponding just to cpu_dp->index (rather than rmw all of CPU_PORT_MASK),
> and write back the result. And to address Russell's concern, we could test
> whether the resulting CPU_PORT_MASK portion of what we're going to write
> back is all-zeroes or not, and if it is, clear the CPU_EN bit, otherwise
> set it.

Correct me if I'm wrong, we introduced priv->active_cpu_ports because
CPU_PORT_MASK from the MT7530_MFC register is a 3-bit mask, meant to keep a
single port number ranging from 0 to 7. There's no single bit corresponding
to cpu_dp->index on the MT7530_MFC register. So I don't see how what you
suggest here would work.

This is what I've got right now:

static void
mt753x_conduit_state_change(struct dsa_switch *ds,
			    const struct net_device *conduit,
			    bool operational)
{
	struct dsa_port *cpu_dp = conduit->dsa_ptr;
	struct mt7530_priv *priv = ds->priv;
	u8 mask;
	int val = 0;

	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
	 * forwarded to the numerically smallest CPU port which the DSA conduit
	 * interface its affine to is up.
	 */
	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
		return;

	mask = BIT(cpu_dp->index);

	if (operational)
		priv->active_cpu_ports |= mask;
	else
		priv->active_cpu_ports &= ~mask;

	if (priv->active_cpu_ports)
		val =
		    CPU_EN |
		    CPU_PORT(__ffs((unsigned long)priv->active_cpu_ports));

	mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);
}

> 
>>
>>>
>>> I would also suggest moving irq_enable after create_sgmii, to avoid
>>> holes in the struct.
>>
>> Sorry, I've got no idea about this. Could you explain why would there
>> possibly be holes in the struct with the current ordering of the members of
>> the mt7530_priv structure?
>>
>> Arınç
> 
> FWIW:
> 
> $ pahole -C mt7530_priv $KBUILD_OUTPUT/drivers/net/dsa/mt7530.o
> struct mt7530_priv {
>          struct device *            dev;                  /*     0     8 */
>          struct dsa_switch *        ds;                   /*     8     8 */
>          struct mii_bus *           bus;                  /*    16     8 */
>          struct regmap *            regmap;               /*    24     8 */
>          struct reset_control *     rstc;                 /*    32     8 */
>          struct regulator *         core_pwr;             /*    40     8 */
>          struct regulator *         io_pwr;               /*    48     8 */
>          struct gpio_desc *         reset;                /*    56     8 */
>          /* --- cacheline 1 boundary (64 bytes) --- */
>          const struct mt753x_info  * info;                /*    64     8 */
>          unsigned int               id;                   /*    72     4 */
>          bool                       mcm;                  /*    76     1 */
> 
>          /* XXX 3 bytes hole, try to pack */
> 
>          phy_interface_t            p6_interface;         /*    80     4 */
>          phy_interface_t            p5_interface;         /*    84     4 */
>          unsigned int               p5_intf_sel;          /*    88     4 */
>          u8                         mirror_rx;            /*    92     1 */
>          u8                         mirror_tx;            /*    93     1 */
> 
>          /* XXX 2 bytes hole, try to pack */
> 
>          struct mt7530_port         ports[7];             /*    96   168 */
>          /* --- cacheline 4 boundary (256 bytes) was 8 bytes ago --- */
>          struct mt753x_pcs          pcs[7];               /*   264   280 */
>          /* --- cacheline 8 boundary (512 bytes) was 32 bytes ago --- */
>          struct mutex               reg_mutex;            /*   544    32 */
>          /* --- cacheline 9 boundary (576 bytes) --- */
>          int                        irq;                  /*   576     4 */
> 
>          /* XXX 4 bytes hole, try to pack */
> 
>          struct irq_domain *        irq_domain;           /*   584     8 */
>          u32                        irq_enable;           /*   592     4 */
> 
>          /* XXX 4 bytes hole, try to pack */
> 
>          int                        (*create_sgmii)(struct mt7530_priv *, bool); /*   600     8 */
>          unsigned long              active_cpu_ports;     /*   608     8 */
> 
>          /* size: 616, cachelines: 10, members: 24 */
>          /* sum members: 603, holes: 4, sum holes: 13 */
>          /* last cacheline: 40 bytes */
> };
> 
> It's not like this makes any practical difference, as struct mt7530_priv
> isn't used from hot paths, but tidying it up is a good sign of clean,
> careful development, and of understanding memory alignment.

Got it, thanks. I've got a few patches that introduce changes to the
mt7530_priv structure. I'll make sure that, in the end, mt7530_priv won't
have any holes.

Arınç

Re: [PATCH net-next 01/15] net: dsa: mt7530: always trap frames to active CPU port on MT7530

[Vladimir Oltean]

On Sat, Nov 18, 2023 at 03:31:51PM +0300, Arınç ÜNAL wrote:
> +static void
> +mt753x_conduit_state_change(struct dsa_switch *ds,
> +			    const struct net_device *conduit,
> +			    bool operational)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	struct dsa_port *cpu_dp = conduit->dsa_ptr;

nitpick: reverse Christmas tree variable ordering

> +
> +	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
> +	 * forwarded to the numerically smallest CPU port which the DSA conduit
> +	 * interface its affine to is up.
> +	 */
> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> +		return;
> +
> +	if (operational)
> +		priv->active_cpu_ports |= BIT(cpu_dp->index);
> +	else
> +		priv->active_cpu_ports &= ~BIT(cpu_dp->index);
> +
> +	if (priv->active_cpu_ports)
> +		mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, CPU_EN |
> +			   CPU_PORT(__ffs(priv->active_cpu_ports)));
> +}

[PATCH net-next 02/15] net: dsa: mt7530: use p5_interface_select as data type for p5_intf_sel

[Arınç ÜNAL]

Use the p5_interface_select enumeration as the data type for the
p5_intf_sel field. This ensures p5_intf_sel can only take the values
defined in the p5_interface_select enumeration.

Remove the explicit assignment of 0 to P5_DISABLED as the first enum item
is automatically assigned 0.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/mt7530.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 96d610f5bcf9..1b10b70c1508 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -683,7 +683,7 @@ struct mt7530_port {
 
 /* Port 5 interface select definitions */
 enum p5_interface_select {
-	P5_DISABLED = 0,
+	P5_DISABLED,
 	P5_INTF_SEL_PHY_P0,
 	P5_INTF_SEL_PHY_P4,
 	P5_INTF_SEL_GMAC5,
@@ -776,7 +776,7 @@ struct mt7530_priv {
 	bool			mcm;
 	phy_interface_t		p6_interface;
 	phy_interface_t		p5_interface;
-	unsigned int		p5_intf_sel;
+	enum p5_interface_select p5_intf_sel;
 	u8			mirror_rx;
 	u8			mirror_tx;
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
-- 
2.40.1

Re: [PATCH net-next 02/15] net: dsa: mt7530: use p5_interface_select as data type for p5_intf_sel

[Vladimir Oltean]

On Sat, Nov 18, 2023 at 03:31:52PM +0300, Arınç ÜNAL wrote:
> Use the p5_interface_select enumeration as the data type for the
> p5_intf_sel field. This ensures p5_intf_sel can only take the values
> defined in the p5_interface_select enumeration.
> 
> Remove the explicit assignment of 0 to P5_DISABLED as the first enum item
> is automatically assigned 0.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Acked-by: Daniel Golle <daniel@makrotopia.org>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH net-next 03/15] net: dsa: mt7530: store port 5 SGMII capability of MT7531

[Arınç ÜNAL]

Introduce the p5_sgmii field to store the information for whether port 5
has got SGMII or not. Instead of reading the MT7531_TOP_SIG_SR register
multiple times, the register will be read once and the value will be
stored on the p5_sgmii field. This saves unnecessary reads of the
register.

Move the comment about MT7531AE and MT7531BE to mt7531_setup(), where the
switch is identified.

Get rid of mt7531_dual_sgmii_supported() now that priv->p5_sgmii stores the
information. Address the code where mt7531_dual_sgmii_supported() is used.

Get rid of mt7531_is_rgmii_port() which just prints the opposite of
priv->p5_sgmii.

Instead of calling mt7531_pll_setup() then returning, do not call it if
port 5 is SGMII.

Remove P5_INTF_SEL_GMAC5_SGMII. The p5_interface_select enum is supposed to
represent the mode that port 5 is being used in, not the hardware
information of port 5. Set p5_intf_sel to P5_INTF_SEL_GMAC5 instead, if
port 5 is not dsa_is_unused_port().

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/mt7530-mdio.c |  7 ++---
 drivers/net/dsa/mt7530.c      | 48 ++++++++++++-----------------------
 drivers/net/dsa/mt7530.h      |  6 +++--
 3 files changed, 22 insertions(+), 39 deletions(-)

diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
index 088533663b83..fa3ee85a99c1 100644
--- a/drivers/net/dsa/mt7530-mdio.c
+++ b/drivers/net/dsa/mt7530-mdio.c
@@ -81,17 +81,14 @@ static const struct regmap_bus mt7530_regmap_bus = {
 };
 
 static int
-mt7531_create_sgmii(struct mt7530_priv *priv, bool dual_sgmii)
+mt7531_create_sgmii(struct mt7530_priv *priv)
 {
 	struct regmap_config *mt7531_pcs_config[2] = {};
 	struct phylink_pcs *pcs;
 	struct regmap *regmap;
 	int i, ret = 0;
 
-	/* MT7531AE has two SGMII units for port 5 and port 6
-	 * MT7531BE has only one SGMII unit for port 6
-	 */
-	for (i = dual_sgmii ? 0 : 1; i < 2; i++) {
+	for (i = priv->p5_sgmii ? 0 : 1; i < 2; i++) {
 		mt7531_pcs_config[i] = devm_kzalloc(priv->dev,
 						    sizeof(struct regmap_config),
 						    GFP_KERNEL);
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 442492d62670..45c9698ad9dd 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -487,15 +487,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
 	return 0;
 }
 
-static bool mt7531_dual_sgmii_supported(struct mt7530_priv *priv)
-{
-	u32 val;
-
-	val = mt7530_read(priv, MT7531_TOP_SIG_SR);
-
-	return (val & PAD_DUAL_SGMII_EN) != 0;
-}
-
 static int
 mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
 {
@@ -510,9 +501,6 @@ mt7531_pll_setup(struct mt7530_priv *priv)
 	u32 xtal;
 	u32 val;
 
-	if (mt7531_dual_sgmii_supported(priv))
-		return;
-
 	val = mt7530_read(priv, MT7531_CREV);
 	top_sig = mt7530_read(priv, MT7531_TOP_SIG_SR);
 	hwstrap = mt7530_read(priv, MT7531_HWTRAP);
@@ -920,8 +908,6 @@ static const char *p5_intf_modes(unsigned int p5_interface)
 		return "PHY P4";
 	case P5_INTF_SEL_GMAC5:
 		return "GMAC5";
-	case P5_INTF_SEL_GMAC5_SGMII:
-		return "GMAC5_SGMII";
 	default:
 		return "unknown";
 	}
@@ -2470,6 +2456,12 @@ mt7531_setup(struct dsa_switch *ds)
 		return -ENODEV;
 	}
 
+	/* MT7531AE has got two SGMII units. One for port 5, one for port 6.
+	 * MT7531BE has got only one SGMII unit which is for port 6.
+	 */
+	val = mt7530_read(priv, MT7531_TOP_SIG_SR);
+	priv->p5_sgmii = !!(val & PAD_DUAL_SGMII_EN);
+
 	/* all MACs must be forced link-down before sw reset */
 	for (i = 0; i < MT7530_NUM_PORTS; i++)
 		mt7530_write(priv, MT7530_PMCR_P(i), MT7531_FORCE_LNK);
@@ -2479,21 +2471,18 @@ mt7531_setup(struct dsa_switch *ds)
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
 		     SYS_CTRL_REG_RST);
 
-	mt7531_pll_setup(priv);
-
-	if (mt7531_dual_sgmii_supported(priv)) {
-		priv->p5_intf_sel = P5_INTF_SEL_GMAC5_SGMII;
-
+	if (!priv->p5_sgmii) {
+		mt7531_pll_setup(priv);
+	} else {
 		/* Let ds->user_mii_bus be able to access external phy. */
 		mt7530_rmw(priv, MT7531_GPIO_MODE1, MT7531_GPIO11_RG_RXD2_MASK,
 			   MT7531_EXT_P_MDC_11);
 		mt7530_rmw(priv, MT7531_GPIO_MODE1, MT7531_GPIO12_RG_RXD3_MASK,
 			   MT7531_EXT_P_MDIO_12);
-	} else {
-		priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
 	}
-	dev_dbg(ds->dev, "P5 support %s interface\n",
-		p5_intf_modes(priv->p5_intf_sel));
+
+	if (!dsa_is_unused_port(ds, 5))
+		priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
 
 	mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK,
 		   MT7531_GPIO0_INTERRUPT);
@@ -2553,11 +2542,6 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
 	}
 }
 
-static bool mt7531_is_rgmii_port(struct mt7530_priv *priv, u32 port)
-{
-	return (port == 5) && (priv->p5_intf_sel != P5_INTF_SEL_GMAC5_SGMII);
-}
-
 static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
 				     struct phylink_config *config)
 {
@@ -2570,7 +2554,7 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
 		break;
 
 	case 5: /* 2nd cpu port supports either rgmii or sgmii/8023z */
-		if (mt7531_is_rgmii_port(priv, port)) {
+		if (!priv->p5_sgmii) {
 			phy_interface_set_rgmii(config->supported_interfaces);
 			break;
 		}
@@ -2637,7 +2621,7 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
 {
 	u32 val;
 
-	if (!mt7531_is_rgmii_port(priv, port)) {
+	if (priv->p5_sgmii) {
 		dev_err(priv->dev, "RGMII mode is not available for port %d\n",
 			port);
 		return -EINVAL;
@@ -2881,7 +2865,7 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
 
 	switch (port) {
 	case 5:
-		if (mt7531_is_rgmii_port(priv, port))
+		if (!priv->p5_sgmii)
 			interface = PHY_INTERFACE_MODE_RGMII;
 		else
 			interface = PHY_INTERFACE_MODE_2500BASEX;
@@ -3033,7 +3017,7 @@ mt753x_setup(struct dsa_switch *ds)
 		mt7530_free_irq_common(priv);
 
 	if (priv->create_sgmii) {
-		ret = priv->create_sgmii(priv, mt7531_dual_sgmii_supported(priv));
+		ret = priv->create_sgmii(priv);
 		if (ret && priv->irq)
 			mt7530_free_irq(priv);
 	}
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 1b10b70c1508..12c1731d6201 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -687,7 +687,6 @@ enum p5_interface_select {
 	P5_INTF_SEL_PHY_P0,
 	P5_INTF_SEL_PHY_P4,
 	P5_INTF_SEL_GMAC5,
-	P5_INTF_SEL_GMAC5_SGMII,
 };
 
 struct mt7530_priv;
@@ -756,6 +755,8 @@ struct mt753x_info {
  *			registers
  * @p6_interface	Holding the current port 6 interface
  * @p5_intf_sel:	Holding the current port 5 interface select
+ * @p5_sgmii:		Flag for distinguishing if port 5 of the MT7531 switch
+ *			has got SGMII
  * @irq:		IRQ number of the switch
  * @irq_domain:		IRQ domain of the switch irq_chip
  * @irq_enable:		IRQ enable bits, synced to SYS_INT_EN
@@ -777,6 +778,7 @@ struct mt7530_priv {
 	phy_interface_t		p6_interface;
 	phy_interface_t		p5_interface;
 	enum p5_interface_select p5_intf_sel;
+	bool			p5_sgmii;
 	u8			mirror_rx;
 	u8			mirror_tx;
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
@@ -786,7 +788,7 @@ struct mt7530_priv {
 	int irq;
 	struct irq_domain *irq_domain;
 	u32 irq_enable;
-	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
+	int (*create_sgmii)(struct mt7530_priv *priv);
 	unsigned long active_cpu_ports;
 };
 
-- 
2.40.1

Re: [PATCH net-next 03/15] net: dsa: mt7530: store port 5 SGMII capability of MT7531

[Vladimir Oltean]

On Sat, Nov 18, 2023 at 03:31:53PM +0300, Arınç ÜNAL wrote:
> Introduce the p5_sgmii field to store the information for whether port 5
> has got SGMII or not. Instead of reading the MT7531_TOP_SIG_SR register
> multiple times, the register will be read once and the value will be
> stored on the p5_sgmii field. This saves unnecessary reads of the
> register.
> 
> Move the comment about MT7531AE and MT7531BE to mt7531_setup(), where the
> switch is identified.
> 
> Get rid of mt7531_dual_sgmii_supported() now that priv->p5_sgmii stores the
> information. Address the code where mt7531_dual_sgmii_supported() is used.
> 
> Get rid of mt7531_is_rgmii_port() which just prints the opposite of
> priv->p5_sgmii.
> 
> Instead of calling mt7531_pll_setup() then returning, do not call it if
> port 5 is SGMII.
> 
> Remove P5_INTF_SEL_GMAC5_SGMII. The p5_interface_select enum is supposed to
> represent the mode that port 5 is being used in, not the hardware
> information of port 5. Set p5_intf_sel to P5_INTF_SEL_GMAC5 instead, if
> port 5 is not dsa_is_unused_port().
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Acked-by: Daniel Golle <daniel@makrotopia.org>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH net-next 04/15] net: dsa: mt7530: improve comments regarding port 5 and 6

[Arınç ÜNAL]

There's no logic to numerically order the CPU ports. State the port number
and its capability of being used as a CPU port instead.

Remove the irrelevant PHY muxing information from
mt7530_mac_port_get_caps(). Explain the supported MII modes instead.

Remove the out of place PHY muxing information from
mt753x_phylink_mac_config(). The function is for MT7530, MT7531, and the
switch on the MT7988 SoC but there's no PHY muxing on MT7531 or the switch
on the MT7988 SoC.

These comments were gradually introduced with the commits below.
ca366d6c889b ("net: dsa: mt7530: Convert to PHYLINK API")
38f790a80560 ("net: dsa: mt7530: Add support for port 5")
88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a new
hardware")
c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mt7530.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 45c9698ad9dd..8623742b35ee 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2520,12 +2520,16 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
 				     struct phylink_config *config)
 {
 	switch (port) {
-	case 0 ... 4: /* Internal phy */
+	/* Internal PHY */
+	case 0 ... 4:
 		__set_bit(PHY_INTERFACE_MODE_GMII,
 			  config->supported_interfaces);
 		break;
 
-	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+	/* Port 5 which can be used as a CPU port supports rgmii with delays,
+	 * mii, and gmii.
+	 */
+	case 5:
 		phy_interface_set_rgmii(config->supported_interfaces);
 		__set_bit(PHY_INTERFACE_MODE_MII,
 			  config->supported_interfaces);
@@ -2533,7 +2537,8 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
 			  config->supported_interfaces);
 		break;
 
-	case 6: /* 1st cpu port */
+	/* Port 6 which can be used as a CPU port supports rgmii and trgmii. */
+	case 6:
 		__set_bit(PHY_INTERFACE_MODE_RGMII,
 			  config->supported_interfaces);
 		__set_bit(PHY_INTERFACE_MODE_TRGMII,
@@ -2548,19 +2553,24 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
 	struct mt7530_priv *priv = ds->priv;
 
 	switch (port) {
-	case 0 ... 4: /* Internal phy */
+	/* Internal PHY */
+	case 0 ... 4:
 		__set_bit(PHY_INTERFACE_MODE_GMII,
 			  config->supported_interfaces);
 		break;
 
-	case 5: /* 2nd cpu port supports either rgmii or sgmii/8023z */
+	/* Port 5 which can be used as a CPU port supports rgmii with delays on
+	 * MT7531BE, sgmii/802.3z on MT7531AE.
+	 */
+	case 5:
 		if (!priv->p5_sgmii) {
 			phy_interface_set_rgmii(config->supported_interfaces);
 			break;
 		}
 		fallthrough;
 
-	case 6: /* 1st cpu port supports sgmii/8023z only */
+	/* Port 6 which can be used as a CPU port supports sgmii/802.3z. */
+	case 6:
 		__set_bit(PHY_INTERFACE_MODE_SGMII,
 			  config->supported_interfaces);
 		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
@@ -2579,11 +2589,13 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
 	phy_interface_zero(config->supported_interfaces);
 
 	switch (port) {
-	case 0 ... 4: /* Internal phy */
+	/* Internal PHY */
+	case 0 ... 4:
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
 		break;
 
+	/* Port 6 which can be used as a CPU port is an internal 10G port. */
 	case 6:
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
@@ -2747,12 +2759,12 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	u32 mcr_cur, mcr_new;
 
 	switch (port) {
-	case 0 ... 4: /* Internal phy */
+	case 0 ... 4:
 		if (state->interface != PHY_INTERFACE_MODE_GMII &&
 		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
 			goto unsupported;
 		break;
-	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+	case 5:
 		if (priv->p5_interface == state->interface)
 			break;
 
@@ -2762,7 +2774,7 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		if (priv->p5_intf_sel != P5_DISABLED)
 			priv->p5_interface = state->interface;
 		break;
-	case 6: /* 1st cpu port */
+	case 6:
 		if (priv->p6_interface == state->interface)
 			break;
 
-- 
2.40.1

[PATCH net-next 05/15] net: dsa: mt7530: improve code path for setting up port 5

[Arınç ÜNAL]

There're two code paths for setting up port 5:

mt7530_setup()
-> mt7530_setup_port5()

mt753x_phylink_mac_config()
-> mt753x_mac_config()
   -> mt7530_mac_config()
      -> mt7530_setup_port5()

Currently mt7530_setup_port5() from mt7530_setup() always runs. If port 5
is used as a CPU, DSA, or user port, mt7530_setup_port5() from
mt753x_phylink_mac_config() won't run. That is because priv->p5_interface
set on mt7530_setup_port5() will match state->interface on
mt753x_phylink_mac_config() which will stop running mt7530_setup_port5()
again.

Therefore, mt7530_setup_port5() will never run from
mt753x_phylink_mac_config().

Address this by not running mt7530_setup_port5() from mt7530_setup() if
port 5 is used as a CPU, DSA, or user port. This driver isn't in the
dsa_switches_apply_workarounds[] array so phylink will always be present.

For the cases of PHY muxing or the port being disabled, call
mt7530_setup_port5() from mt7530_setup(). mt7530_setup_port5() from
mt753x_phylink_mac_config() won't run when port 5 is disabled or used for
PHY muxing as port 5 won't be defined on the devicetree.

Do not set priv->p5_intf_sel to P5_DISABLED. It is already set to that when
"priv" is allocated.

Move setting the interface to a more specific location. It's supposed to be
overwritten if PHY muxing is detected.

Improve the comment which explains the process.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 8623742b35ee..069b3dfca6fa 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2308,16 +2308,15 @@ mt7530_setup(struct dsa_switch *ds)
 		return ret;
 
 	/* Setup port 5 */
-	priv->p5_intf_sel = P5_DISABLED;
-	interface = PHY_INTERFACE_MODE_NA;
-
 	if (!dsa_is_unused_port(ds, 5)) {
 		priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
-		ret = of_get_phy_mode(dsa_to_port(ds, 5)->dn, &interface);
-		if (ret && ret != -ENODEV)
-			return ret;
 	} else {
-		/* Scan the ethernet nodes. look for GMAC1, lookup used phy */
+		/* Scan the ethernet nodes. Look for GMAC1, lookup the used PHY.
+		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
+		 * is detected.
+		 */
+		interface = PHY_INTERFACE_MODE_NA;
+
 		for_each_child_of_node(dn, mac_np) {
 			if (!of_device_is_compatible(mac_np,
 						     "mediatek,eth-mac"))
@@ -2348,6 +2347,8 @@ mt7530_setup(struct dsa_switch *ds)
 			of_node_put(phy_node);
 			break;
 		}
+
+		mt7530_setup_port5(ds, interface);
 	}
 
 #ifdef CONFIG_GPIOLIB
@@ -2358,8 +2359,6 @@ mt7530_setup(struct dsa_switch *ds)
 	}
 #endif /* CONFIG_GPIOLIB */
 
-	mt7530_setup_port5(ds, interface);
-
 	/* Flush the FDB table */
 	ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL);
 	if (ret < 0)
-- 
2.40.1

Re: [PATCH net-next 05/15] net: dsa: mt7530: improve code path for setting up port 5

[Russell King (Oracle)]

On Sat, Nov 18, 2023 at 03:31:55PM +0300, Arınç ÜNAL wrote:
> There're two code paths for setting up port 5:
> 
> mt7530_setup()
> -> mt7530_setup_port5()
> 
> mt753x_phylink_mac_config()
> -> mt753x_mac_config()
>    -> mt7530_mac_config()
>       -> mt7530_setup_port5()
> 
> Currently mt7530_setup_port5() from mt7530_setup() always runs. If port 5
> is used as a CPU, DSA, or user port, mt7530_setup_port5() from
> mt753x_phylink_mac_config() won't run. That is because priv->p5_interface
> set on mt7530_setup_port5() will match state->interface on
> mt753x_phylink_mac_config() which will stop running mt7530_setup_port5()
> again.
> 
> Therefore, mt7530_setup_port5() will never run from
> mt753x_phylink_mac_config().
> 
> Address this by not running mt7530_setup_port5() from mt7530_setup() if
> port 5 is used as a CPU, DSA, or user port. This driver isn't in the
> dsa_switches_apply_workarounds[] array so phylink will always be present.
> 
> For the cases of PHY muxing or the port being disabled, call
> mt7530_setup_port5() from mt7530_setup(). mt7530_setup_port5() from
> mt753x_phylink_mac_config() won't run when port 5 is disabled or used for
> PHY muxing as port 5 won't be defined on the devicetree.

... and this should state why this needs to happen - in other words,
the commit message should state why is it critical that port 5 is
always setup.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 05/15] net: dsa: mt7530: improve code path for setting up port 5

[Arınç ÜNAL]

On 18.11.2023 17:41, Russell King (Oracle) wrote:
> On Sat, Nov 18, 2023 at 03:31:55PM +0300, Arınç ÜNAL wrote:
>> There're two code paths for setting up port 5:
>>
>> mt7530_setup()
>> -> mt7530_setup_port5()
>>
>> mt753x_phylink_mac_config()
>> -> mt753x_mac_config()
>>     -> mt7530_mac_config()
>>        -> mt7530_setup_port5()
>>
>> Currently mt7530_setup_port5() from mt7530_setup() always runs. If port 5
>> is used as a CPU, DSA, or user port, mt7530_setup_port5() from
>> mt753x_phylink_mac_config() won't run. That is because priv->p5_interface
>> set on mt7530_setup_port5() will match state->interface on
>> mt753x_phylink_mac_config() which will stop running mt7530_setup_port5()
>> again.
>>
>> Therefore, mt7530_setup_port5() will never run from
>> mt753x_phylink_mac_config().
>>
>> Address this by not running mt7530_setup_port5() from mt7530_setup() if
>> port 5 is used as a CPU, DSA, or user port. This driver isn't in the
>> dsa_switches_apply_workarounds[] array so phylink will always be present.
>>
>> For the cases of PHY muxing or the port being disabled, call
>> mt7530_setup_port5() from mt7530_setup(). mt7530_setup_port5() from
>> mt753x_phylink_mac_config() won't run when port 5 is disabled or used for
>> PHY muxing as port 5 won't be defined on the devicetree.
> 
> ... and this should state why this needs to happen - in other words,
> the commit message should state why is it critical that port 5 is
> always setup.

Actually, port 5 must not always be setup. With patch 7, I explain this
while preventing mt7530_setup_port5() from running if port 5 is disabled.

Arınç

Re: [PATCH net-next 05/15] net: dsa: mt7530: improve code path for setting up port 5

[Vladimir Oltean]

On Sat, Dec 02, 2023 at 11:36:03AM +0300, Arınç ÜNAL wrote:
> On 18.11.2023 17:41, Russell King (Oracle) wrote:
> > > For the cases of PHY muxing or the port being disabled, call
> > > mt7530_setup_port5() from mt7530_setup(). mt7530_setup_port5() from
> > > mt753x_phylink_mac_config() won't run when port 5 is disabled or used for
> > > PHY muxing as port 5 won't be defined on the devicetree.
> > 
> > ... and this should state why this needs to happen - in other words,
> > the commit message should state why is it critical that port 5 is
> > always setup.
> 
> Actually, port 5 must not always be setup. With patch 7, I explain this
> while preventing mt7530_setup_port5() from running if port 5 is disabled.
> 
> Arınç

Then change that last paragraph. You could say something like this:

To keep the cases where port 5 isn't controlled by phylink working as
before, we need to preserve the mt7530_setup_port5() call from mt7530_setup().

I think it's a case of saying too much, which sparks too many unresolved
questions in the reader's mind, which are irrelevant for the purpose of
this specific change: eliminating the overlap between DSA's setup() time
and phylink.

Re: [PATCH net-next 05/15] net: dsa: mt7530: improve code path for setting up port 5

[Arınç ÜNAL]

On 7.12.2023 21:03, Vladimir Oltean wrote:
> On Sat, Dec 02, 2023 at 11:36:03AM +0300, Arınç ÜNAL wrote:
>> On 18.11.2023 17:41, Russell King (Oracle) wrote:
>>>> For the cases of PHY muxing or the port being disabled, call
>>>> mt7530_setup_port5() from mt7530_setup(). mt7530_setup_port5() from
>>>> mt753x_phylink_mac_config() won't run when port 5 is disabled or used for
>>>> PHY muxing as port 5 won't be defined on the devicetree.
>>>
>>> ... and this should state why this needs to happen - in other words,
>>> the commit message should state why is it critical that port 5 is
>>> always setup.
>>
>> Actually, port 5 must not always be setup. With patch 7, I explain this
>> while preventing mt7530_setup_port5() from running if port 5 is disabled.
>>
>> Arınç
> 
> Then change that last paragraph. You could say something like this:
> 
> To keep the cases where port 5 isn't controlled by phylink working as
> before, we need to preserve the mt7530_setup_port5() call from mt7530_setup().
> 
> I think it's a case of saying too much, which sparks too many unresolved
> questions in the reader's mind, which are irrelevant for the purpose of
> this specific change: eliminating the overlap between DSA's setup() time
> and phylink.

Will do, thanks.

[PATCH net-next 06/15] net: dsa: mt7530: do not set priv->p5_interface on mt7530_setup_port5()

[Arınç ÜNAL]

Do not set priv->p5_interface on mt7530_setup_port5(). There isn't a case
where mt753x_phylink_mac_config() runs after mt7530_setup_port5() which
setting priv->p5_interface would prevent mt7530_setup_port5() from running
more than once.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 069b3dfca6fa..fc87ec817672 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -978,8 +978,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 	dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
 		val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
 
-	priv->p5_interface = interface;
-
 unlock_exit:
 	mutex_unlock(&priv->reg_mutex);
 }
-- 
2.40.1

Re: [PATCH net-next 06/15] net: dsa: mt7530: do not set priv->p5_interface on mt7530_setup_port5()

[Vladimir Oltean]

On Sat, Nov 18, 2023 at 03:31:56PM +0300, Arınç ÜNAL wrote:
> Do not set priv->p5_interface on mt7530_setup_port5(). There isn't a case
> where mt753x_phylink_mac_config() runs after mt7530_setup_port5() which
> setting priv->p5_interface would prevent mt7530_setup_port5() from running
> more than once.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 069b3dfca6fa..fc87ec817672 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -978,8 +978,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>  	dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
>  		val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
>  
> -	priv->p5_interface = interface;
> -
>  unlock_exit:
>  	mutex_unlock(&priv->reg_mutex);
>  }
> -- 
> 2.40.1
> 

Purely as a matter of theory, mt753x_phylink_mac_config() itself could
be called more than once, at any time there is a phylink_major_config() -
first during initial one, then upon any change of the phy_interface_t.
With the port being fixed and internal, maybe that is unlikely.

Destroying and re-creating the phylink instance could also make the
driver see more than 1 mt753x_phylink_mac_config() call. During periods
of -EPROBE_DEFER, maybe this could even happen.

I think what we need to see is a description of what the priv->p5_interface
test is really protecting against, because it certainly is uncommon in other
drivers to have this much logic to avoid mt753x_mac_config() being
called twice.

If there's nothing wrong with calling it twice and it's just an optimization,
I think that's enough of a justification for removing that extra protection.
At the same time, the optimization (and simplification) that we should
pursue is that we should remove the overlap between phylink and the
initial port setup made by the driver.

Re: [PATCH net-next 06/15] net: dsa: mt7530: do not set priv->p5_interface on mt7530_setup_port5()

[Arınç ÜNAL]

On 7.12.2023 21:18, Vladimir Oltean wrote:
> On Sat, Nov 18, 2023 at 03:31:56PM +0300, Arınç ÜNAL wrote:
>> Do not set priv->p5_interface on mt7530_setup_port5(). There isn't a case
>> where mt753x_phylink_mac_config() runs after mt7530_setup_port5() which
>> setting priv->p5_interface would prevent mt7530_setup_port5() from running
>> more than once.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 069b3dfca6fa..fc87ec817672 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -978,8 +978,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>>   	dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
>>   		val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
>>   
>> -	priv->p5_interface = interface;
>> -
>>   unlock_exit:
>>   	mutex_unlock(&priv->reg_mutex);
>>   }
>> -- 
>> 2.40.1
>>
> 
> Purely as a matter of theory, mt753x_phylink_mac_config() itself could
> be called more than once, at any time there is a phylink_major_config() -
> first during initial one, then upon any change of the phy_interface_t.
> With the port being fixed and internal, maybe that is unlikely.
> 
> Destroying and re-creating the phylink instance could also make the
> driver see more than 1 mt753x_phylink_mac_config() call. During periods
> of -EPROBE_DEFER, maybe this could even happen.
> 
> I think what we need to see is a description of what the priv->p5_interface
> test is really protecting against, because it certainly is uncommon in other
> drivers to have this much logic to avoid mt753x_mac_config() being
> called twice.
> 
> If there's nothing wrong with calling it twice and it's just an optimization,
> I think that's enough of a justification for removing that extra protection.
> At the same time, the optimization (and simplification) that we should
> pursue is that we should remove the overlap between phylink and the
> initial port setup made by the driver.

priv->p5_interface and priv->p6_interface were actually intended to apply
only for MT7531. It prevents the CPU ports to be configured again. CPU
ports are unnecessarily configured before phylink. I intend to address that
with the patch below. I'll submit it after the current patches here go
through. There's a lot to clean up in this driver.

https://github.com/arinc9/linux/commit/80efcb1870530ef5526d7feda625374b8f308369

If you can recall, this is where me and Russell mostly left off on my 30
patch series. I was supposed to run some debug info prints to make sure
that the MT7531 CPU ports are still configured as they were with
priv->info->cpu_port_config().

https://lore.kernel.org/netdev/ZHy2jQLesdYFMQtO@shell.armlinux.org.uk/

For this patch, I can change the patch log to state that priv->p5_interface
and priv->p6_interface are intended for use on the MT7531 switch.

Arınç

[PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Arınç ÜNAL]

There's no need to run all the code on mt7530_setup_port5() if port 5 is
disabled. The only case for calling mt7530_setup_port5() from
mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
defined as a port on the devicetree, therefore, it cannot be controlled by
phylink.

Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
mt7530_setup_port5().

Stop initialising the interface variable as the remaining cases will always
call mt7530_setup_port5() with it initialised.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/mt7530.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index fc87ec817672..1aab4c3f28b0 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
 		val &= ~MHWTRAP_P5_DIS;
 		break;
-	case P5_DISABLED:
-		interface = PHY_INTERFACE_MODE_NA;
-		break;
 	default:
 		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
 			priv->p5_intf_sel);
@@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
 		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
 		 * is detected.
 		 */
-		interface = PHY_INTERFACE_MODE_NA;
-
 		for_each_child_of_node(dn, mac_np) {
 			if (!of_device_is_compatible(mac_np,
 						     "mediatek,eth-mac"))
@@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
 			break;
 		}
 
-		mt7530_setup_port5(ds, interface);
+		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
+		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
+			mt7530_setup_port5(ds, interface);
 	}
 
 #ifdef CONFIG_GPIOLIB
-- 
2.40.1

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Simon Horman]

On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
> There's no need to run all the code on mt7530_setup_port5() if port 5 is
> disabled. The only case for calling mt7530_setup_port5() from
> mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
> defined as a port on the devicetree, therefore, it cannot be controlled by
> phylink.
> 
> Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
> P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
> mt7530_setup_port5().
> 
> Stop initialising the interface variable as the remaining cases will always
> call mt7530_setup_port5() with it initialised.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  drivers/net/dsa/mt7530.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index fc87ec817672..1aab4c3f28b0 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>  		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
>  		val &= ~MHWTRAP_P5_DIS;
>  		break;
> -	case P5_DISABLED:
> -		interface = PHY_INTERFACE_MODE_NA;
> -		break;
>  	default:
>  		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
>  			priv->p5_intf_sel);
> @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
>  		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
>  		 * is detected.
>  		 */
> -		interface = PHY_INTERFACE_MODE_NA;
> -
>  		for_each_child_of_node(dn, mac_np) {
>  			if (!of_device_is_compatible(mac_np,
>  						     "mediatek,eth-mac"))
> @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
>  			break;
>  		}
>  
> -		mt7530_setup_port5(ds, interface);
> +		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> +		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> +			mt7530_setup_port5(ds, interface);

Hi Arınç,

It appears that interface is now uninitialised here.

Flagged by Smatch.

>  	}
>  
>  #ifdef CONFIG_GPIOLIB
> -- 
> 2.40.1
> 

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Arınç ÜNAL]

Hi Simon.

On 21.11.2023 21:53, Simon Horman wrote:
> On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
>> There's no need to run all the code on mt7530_setup_port5() if port 5 is
>> disabled. The only case for calling mt7530_setup_port5() from
>> mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
>> defined as a port on the devicetree, therefore, it cannot be controlled by
>> phylink.
>>
>> Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
>> P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
>> mt7530_setup_port5().
>>
>> Stop initialising the interface variable as the remaining cases will always
>> call mt7530_setup_port5() with it initialised.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index fc87ec817672..1aab4c3f28b0 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>>   		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
>>   		val &= ~MHWTRAP_P5_DIS;
>>   		break;
>> -	case P5_DISABLED:
>> -		interface = PHY_INTERFACE_MODE_NA;
>> -		break;
>>   	default:
>>   		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
>>   			priv->p5_intf_sel);
>> @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
>>   		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
>>   		 * is detected.
>>   		 */
>> -		interface = PHY_INTERFACE_MODE_NA;
>> -
>>   		for_each_child_of_node(dn, mac_np) {
>>   			if (!of_device_is_compatible(mac_np,
>>   						     "mediatek,eth-mac"))
>> @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
>>   			break;
>>   		}
>>   
>> -		mt7530_setup_port5(ds, interface);
>> +		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
>> +		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
>> +			mt7530_setup_port5(ds, interface);
> 
> Hi Arınç,
> 
> It appears that interface is now uninitialised here.
> 
> Flagged by Smatch.

I'm not sure why it doesn't catch that for mt7530_setup_port5() to run
here, priv->p5_intf_sel must be either P5_INTF_SEL_PHY_P0 or
P5_INTF_SEL_PHY_P4. And for that to happen, the interface variable will be
initialised.

for_each_child_of_node(dn, mac_np) {
	if (!of_device_is_compatible(mac_np,
				     "mediatek,eth-mac"))
		continue;

	ret = of_property_read_u32(mac_np, "reg", &id);
	if (ret < 0 || id != 1)
		continue;

	phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
	if (!phy_node)
		continue;

	if (phy_node->parent == priv->dev->of_node->parent) {
		ret = of_get_phy_mode(mac_np, &interface);
		if (ret && ret != -ENODEV) {
			of_node_put(mac_np);
			of_node_put(phy_node);
			return ret;
		}
		id = of_mdio_parse_addr(ds->dev, phy_node);
		if (id == 0)
			priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
		if (id == 4)
			priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
	}
	of_node_put(mac_np);
	of_node_put(phy_node);
	break;
}

if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
     priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
	mt7530_setup_port5(ds, interface);

Arınç

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Russell King (Oracle)]

On Sat, Dec 02, 2023 at 11:45:42AM +0300, Arınç ÜNAL wrote:
> Hi Simon.
> 
> On 21.11.2023 21:53, Simon Horman wrote:
> > On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
> > > There's no need to run all the code on mt7530_setup_port5() if port 5 is
> > > disabled. The only case for calling mt7530_setup_port5() from
> > > mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
> > > defined as a port on the devicetree, therefore, it cannot be controlled by
> > > phylink.
> > > 
> > > Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
> > > P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
> > > mt7530_setup_port5().
> > > 
> > > Stop initialising the interface variable as the remaining cases will always
> > > call mt7530_setup_port5() with it initialised.
> > > 
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> > > ---
> > >   drivers/net/dsa/mt7530.c | 9 +++------
> > >   1 file changed, 3 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index fc87ec817672..1aab4c3f28b0 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
> > >   		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
> > >   		val &= ~MHWTRAP_P5_DIS;
> > >   		break;
> > > -	case P5_DISABLED:
> > > -		interface = PHY_INTERFACE_MODE_NA;
> > > -		break;
> > >   	default:
> > >   		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
> > >   			priv->p5_intf_sel);
> > > @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
> > >   		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
> > >   		 * is detected.
> > >   		 */
> > > -		interface = PHY_INTERFACE_MODE_NA;
> > > -
> > >   		for_each_child_of_node(dn, mac_np) {
> > >   			if (!of_device_is_compatible(mac_np,
> > >   						     "mediatek,eth-mac"))
> > > @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
> > >   			break;
> > >   		}
> > > -		mt7530_setup_port5(ds, interface);
> > > +		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> > > +		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> > > +			mt7530_setup_port5(ds, interface);
> > 
> > Hi Arınç,
> > 
> > It appears that interface is now uninitialised here.
> > 
> > Flagged by Smatch.
> 
> I'm not sure why it doesn't catch that for mt7530_setup_port5() to run
> here, priv->p5_intf_sel must be either P5_INTF_SEL_PHY_P0 or
> P5_INTF_SEL_PHY_P4. And for that to happen, the interface variable will be
> initialised.

It's probably due to the complexities involved in analysing the values
of variables, especially when they're in structures that are passed in.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Simon Horman]

+ Dan Carpenter <dan.carpenter@linaro.org>

On Sat, Dec 02, 2023 at 11:45:42AM +0300, Arınç ÜNAL wrote:
> Hi Simon.
> 
> On 21.11.2023 21:53, Simon Horman wrote:
> > On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
> > > There's no need to run all the code on mt7530_setup_port5() if port 5 is
> > > disabled. The only case for calling mt7530_setup_port5() from
> > > mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
> > > defined as a port on the devicetree, therefore, it cannot be controlled by
> > > phylink.
> > > 
> > > Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
> > > P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
> > > mt7530_setup_port5().
> > > 
> > > Stop initialising the interface variable as the remaining cases will always
> > > call mt7530_setup_port5() with it initialised.
> > > 
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> > > ---
> > >   drivers/net/dsa/mt7530.c | 9 +++------
> > >   1 file changed, 3 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index fc87ec817672..1aab4c3f28b0 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
> > >   		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
> > >   		val &= ~MHWTRAP_P5_DIS;
> > >   		break;
> > > -	case P5_DISABLED:
> > > -		interface = PHY_INTERFACE_MODE_NA;
> > > -		break;
> > >   	default:
> > >   		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
> > >   			priv->p5_intf_sel);
> > > @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
> > >   		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
> > >   		 * is detected.
> > >   		 */
> > > -		interface = PHY_INTERFACE_MODE_NA;
> > > -
> > >   		for_each_child_of_node(dn, mac_np) {
> > >   			if (!of_device_is_compatible(mac_np,
> > >   						     "mediatek,eth-mac"))
> > > @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
> > >   			break;
> > >   		}
> > > -		mt7530_setup_port5(ds, interface);
> > > +		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> > > +		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> > > +			mt7530_setup_port5(ds, interface);
> > 
> > Hi Arınç,
> > 
> > It appears that interface is now uninitialised here.
> > 
> > Flagged by Smatch.
> 
> I'm not sure why it doesn't catch that for mt7530_setup_port5() to run
> here, priv->p5_intf_sel must be either P5_INTF_SEL_PHY_P0 or
> P5_INTF_SEL_PHY_P4. And for that to happen, the interface variable will be
> initialised.

Yes, I see your point now. At a guess, perhaps it because:

1. It doesn't know that of_get_phy_mode will set the value of interface
2. It doesn't know if the loop will run (more than zero times)

I CCed Dan Carpenter, who is surely more knowledgeable about this than I,
in case he wants to add anything.

> for_each_child_of_node(dn, mac_np) {
> 	if (!of_device_is_compatible(mac_np,
> 				     "mediatek,eth-mac"))
> 		continue;
> 
> 	ret = of_property_read_u32(mac_np, "reg", &id);
> 	if (ret < 0 || id != 1)
> 		continue;
> 
> 	phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> 	if (!phy_node)
> 		continue;
> 
> 	if (phy_node->parent == priv->dev->of_node->parent) {
> 		ret = of_get_phy_mode(mac_np, &interface);
> 		if (ret && ret != -ENODEV) {
> 			of_node_put(mac_np);
> 			of_node_put(phy_node);
> 			return ret;
> 		}
> 		id = of_mdio_parse_addr(ds->dev, phy_node);
> 		if (id == 0)
> 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
> 		if (id == 4)
> 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
> 	}
> 	of_node_put(mac_np);
> 	of_node_put(phy_node);
> 	break;
> }
> 
> if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
>     priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> 	mt7530_setup_port5(ds, interface);
> 
> Arınç

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Dan Carpenter]

On Sat, Dec 02, 2023 at 11:45:42AM +0300, Arınç ÜNAL wrote:
> 
> I'm not sure why it doesn't catch that for mt7530_setup_port5() to run
> here, priv->p5_intf_sel must be either P5_INTF_SEL_PHY_P0 or
> P5_INTF_SEL_PHY_P4. And for that to happen, the interface variable will be
> initialised.
> 
> for_each_child_of_node(dn, mac_np) {
> 	if (!of_device_is_compatible(mac_np,
> 				     "mediatek,eth-mac"))
> 		continue;
> 
> 	ret = of_property_read_u32(mac_np, "reg", &id);
> 	if (ret < 0 || id != 1)
> 		continue;
> 
> 	phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> 	if (!phy_node)
> 		continue;
> 
> 	if (phy_node->parent == priv->dev->of_node->parent) {
> 		ret = of_get_phy_mode(mac_np, &interface);
> 		if (ret && ret != -ENODEV) {
> 			of_node_put(mac_np);
> 			of_node_put(phy_node);
> 			return ret;
> 		}
> 		id = of_mdio_parse_addr(ds->dev, phy_node);
> 		if (id == 0)
> 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
> 		if (id == 4)
> 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
> 	}
> 	of_node_put(mac_np);
> 	of_node_put(phy_node);
> 	break;
> }
> 
> if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
>     priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> 	mt7530_setup_port5(ds, interface);

Smatch doesn't know:
1) What the value of priv->p5_intf_sel is going into this function
2) We enter the for_each_child_of_node() loop
3) That if (phy_node->parent == priv->dev->of_node->parent) { is
   definitely true for one element on the list.

Looking at how Smatch parses this code, I could probably improve problem
#1 a bit.  Right now Smatch sees "struct mt7530_priv *priv = ds->priv;"
and "priv->p5_intf_sel" is unknown, but I could probably improve it to
where it says that it's in the 1-3 range.  But that doesn't help here
and it doesn't address problems 2 and 3.

It's a hard problem.

regards,
dan carpenter

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Vladimir Oltean]

On Thu, Dec 07, 2023 at 09:51:07AM +0300, Dan Carpenter wrote:
> On Sat, Dec 02, 2023 at 11:45:42AM +0300, Arınç ÜNAL wrote:
> > 
> > I'm not sure why it doesn't catch that for mt7530_setup_port5() to run
> > here, priv->p5_intf_sel must be either P5_INTF_SEL_PHY_P0 or
> > P5_INTF_SEL_PHY_P4. And for that to happen, the interface variable will be
> > initialised.
> > 
> > for_each_child_of_node(dn, mac_np) {
> > 	if (!of_device_is_compatible(mac_np,
> > 				     "mediatek,eth-mac"))
> > 		continue;
> > 
> > 	ret = of_property_read_u32(mac_np, "reg", &id);
> > 	if (ret < 0 || id != 1)
> > 		continue;
> > 
> > 	phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> > 	if (!phy_node)
> > 		continue;
> > 
> > 	if (phy_node->parent == priv->dev->of_node->parent) {
> > 		ret = of_get_phy_mode(mac_np, &interface);
> > 		if (ret && ret != -ENODEV) {
> > 			of_node_put(mac_np);
> > 			of_node_put(phy_node);
> > 			return ret;
> > 		}
> > 		id = of_mdio_parse_addr(ds->dev, phy_node);
> > 		if (id == 0)
> > 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
> > 		if (id == 4)
> > 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
> > 	}
> > 	of_node_put(mac_np);
> > 	of_node_put(phy_node);
> > 	break;
> > }
> > 
> > if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> >     priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> > 	mt7530_setup_port5(ds, interface);
> 
> Smatch doesn't know:
> 1) What the value of priv->p5_intf_sel is going into this function
> 2) We enter the for_each_child_of_node() loop
> 3) That if (phy_node->parent == priv->dev->of_node->parent) { is
>    definitely true for one element on the list.
> 
> Looking at how Smatch parses this code, I could probably improve problem
> #1 a bit.  Right now Smatch sees "struct mt7530_priv *priv = ds->priv;"
> and "priv->p5_intf_sel" is unknown, but I could probably improve it to
> where it says that it's in the 1-3 range.  But that doesn't help here
> and it doesn't address problems 2 and 3.
> 
> It's a hard problem.
> 
> regards,
> dan carpenter
> 

We could be more pragmatic about this whole sparse false positive warning,
and just move the "if" block which calls mt7530_setup_port5() right
after the priv->p5_intf_sel assignments, instead of waiting to "break;"
from the for_each_child_of_node() loop.

for_each_child_of_node(dn, mac_np) {
	if (!of_device_is_compatible(mac_np,
				     "mediatek,eth-mac"))
		continue;

	ret = of_property_read_u32(mac_np, "reg", &id);
	if (ret < 0 || id != 1)
		continue;

	phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
	if (!phy_node)
		continue;

	if (phy_node->parent == priv->dev->of_node->parent) {
		ret = of_get_phy_mode(mac_np, &interface);
		if (ret && ret != -ENODEV) {
			of_node_put(mac_np);
			of_node_put(phy_node);
			return ret;
		}
		id = of_mdio_parse_addr(ds->dev, phy_node);
		if (id == 0)
			priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
		if (id == 4)
			priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;

		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 || <---- here
		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
			mt7530_setup_port5(ds, interface);
	}
	of_node_put(mac_np);
	of_node_put(phy_node);
	break;
}

I hope it's now much clearer to sparse that "interface" is used within
the same basic block in which it also got assigned, and that determination
does not depend upon the values taken by a second variable.

Maybe it's also a bit clearer for us humans.

What would also help us humans even more is to extract the entire "dn"
handling from mt7530_setup() into a separate mt7530_setup_phy_muxing()
function, and put a good comment there about what's going on with this
PHY muxing thing.

The advantage of splitting this up is that we don't pollute mt7530_setup()
with finding the "dn" if dsa_is_unused_port(ds, 5) returns false.

Also, reducing the indentation level of for_each_child_of_node() by one
can't be bad. Maybe even by more. There's this pattern:

for_each_child_of_node(dn, mac_np) {
	// do stuff with mac_np
	break;
}

aka we only care about the first child of dn. We could find the mac_np
as the only operation inside for_each_child_of_node(), break directly,
and "do stuff with mac_np" could be done outside, further reducing the
indentation by 1 level.

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Vladimir Oltean]

On Thu, Dec 07, 2023 at 08:40:15PM +0200, Vladimir Oltean wrote:
> Also, reducing the indentation level of for_each_child_of_node() by one
> can't be bad. Maybe even by more. There's this pattern:
> 
> for_each_child_of_node(dn, mac_np) {
> 	// do stuff with mac_np
> 	break;
> }
> 
> aka we only care about the first child of dn. We could find the mac_np
> as the only operation inside for_each_child_of_node(), break directly,
> and "do stuff with mac_np" could be done outside, further reducing the
> indentation by 1 level.

I noticed just now that there is further filtering on the child OF node
by of_device_is_compatible(). In that case, of_get_compatible_child()
could be used to eliminate for_each_child_of_node() completely.

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Dan Carpenter]

On Thu, Dec 07, 2023 at 08:40:15PM +0200, Vladimir Oltean wrote:
> 
> We could be more pragmatic about this whole sparse false positive warning,
> and just move the "if" block which calls mt7530_setup_port5() right
> after the priv->p5_intf_sel assignments, instead of waiting to "break;"
> from the for_each_child_of_node() loop.
> 
> for_each_child_of_node(dn, mac_np) {
> 	if (!of_device_is_compatible(mac_np,
> 				     "mediatek,eth-mac"))
> 		continue;
> 
> 	ret = of_property_read_u32(mac_np, "reg", &id);
> 	if (ret < 0 || id != 1)
> 		continue;
> 
> 	phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> 	if (!phy_node)
> 		continue;
> 
> 	if (phy_node->parent == priv->dev->of_node->parent) {
> 		ret = of_get_phy_mode(mac_np, &interface);
> 		if (ret && ret != -ENODEV) {
> 			of_node_put(mac_np);
> 			of_node_put(phy_node);
> 			return ret;
> 		}
> 		id = of_mdio_parse_addr(ds->dev, phy_node);
> 		if (id == 0)
> 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
> 		if (id == 4)
> 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
> 
> 		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 || <---- here
> 		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> 			mt7530_setup_port5(ds, interface);

This doesn't solve the problem that Smatch doesn't know what the
original value of priv->p5_intf_sel.  And also I don't like this code
because now we call mt7530_setup_port5() on every iteration after
we find the first P5_INTF_SEL_PHY_P0.

> 	}
> 	of_node_put(mac_np);
> 	of_node_put(phy_node);
> 	break;
> }

Let's not worry too much about false positives.  We can just ignore
them.  There is always a trade off between false positives and false
negatives.  With GCC we try to get a clean run with no warnings, but
with Smatch I'm targetting the false positive ratio at "this is worth
reviewing one time."

regards,
dan carpenter

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Vladimir Oltean]

On Fri, Dec 08, 2023 at 07:23:38AM +0300, Dan Carpenter wrote:
> On Thu, Dec 07, 2023 at 08:40:15PM +0200, Vladimir Oltean wrote:
> > 
> > We could be more pragmatic about this whole sparse false positive warning,
> > and just move the "if" block which calls mt7530_setup_port5() right
> > after the priv->p5_intf_sel assignments, instead of waiting to "break;"
> > from the for_each_child_of_node() loop.
> > 
> > for_each_child_of_node(dn, mac_np) {
> > 	if (!of_device_is_compatible(mac_np,
> > 				     "mediatek,eth-mac"))
> > 		continue;
> > 
> > 	ret = of_property_read_u32(mac_np, "reg", &id);
> > 	if (ret < 0 || id != 1)
> > 		continue;
> > 
> > 	phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> > 	if (!phy_node)
> > 		continue;
> > 
> > 	if (phy_node->parent == priv->dev->of_node->parent) {
> > 		ret = of_get_phy_mode(mac_np, &interface);
> > 		if (ret && ret != -ENODEV) {
> > 			of_node_put(mac_np);
> > 			of_node_put(phy_node);
> > 			return ret;
> > 		}
> > 		id = of_mdio_parse_addr(ds->dev, phy_node);
> > 		if (id == 0)
> > 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
> > 		if (id == 4)
> > 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
> > 
> > 		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 || <---- here
> > 		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> > 			mt7530_setup_port5(ds, interface);
> 
> This doesn't solve the problem that Smatch doesn't know what the
> original value of priv->p5_intf_sel.  And also I don't like this code
> because now we call mt7530_setup_port5() on every iteration after
> we find the first P5_INTF_SEL_PHY_P0.

You seem to have not parsed the "break" from 4 lines below. There is at
most one iteration through for_each_child_of_node().

And why would the "original" value of priv->p5_intf_sel matter? Original
or modified by the "if (id == 0)" and "if (id == 4)" blocks, the code
has already executed the of_get_phy_mode(&interface) call, by the time
we reach the "if" that calls mt7530_setup_port5().

Hmm, maybe the problem, all along, was that we let the -ENODEV return
code from of_get_phy_mode() pass through? "interface" will really be
uninitialized in that case. It's not a false positive.

Instead of:

	ret = of_get_phy_mode(mac_np, &interface);
	if (ret && ret != -ENODEV) {
		...
		return ret;
	}

it should have been like this, to not complain:

	ret = of_get_phy_mode(mac_np, &interface);
	if (ret) {
		...
		return ret;
	}

> > 	}
> > 	of_node_put(mac_np);
> > 	of_node_put(phy_node);
> > 	break;
> > }

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Arınç ÜNAL]

On 8.12.2023 21:46, Vladimir Oltean wrote:
> Hmm, maybe the problem, all along, was that we let the -ENODEV return
> code from of_get_phy_mode() pass through? "interface" will really be
> uninitialized in that case. It's not a false positive.
> 
> Instead of:
> 
> 	ret = of_get_phy_mode(mac_np, &interface);
> 	if (ret && ret != -ENODEV) {
> 		...
> 		return ret;
> 	}
> 
> it should have been like this, to not complain:
> 
> 	ret = of_get_phy_mode(mac_np, &interface);
> 	if (ret) {
> 		...
> 		return ret;
> 	}
> 

I just tried this, smatch still reports "interface" as uninitialised.

$ export ARCH=mips CROSS_COMPILE=mips-linux-gnu-
$ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c

   UPD     include/config/kernel.release
   UPD     include/generated/utsrelease.h
   CHECK   scripts/mod/empty.c
   CALL    scripts/checksyscalls.sh
   CC      drivers/net/dsa/mt7530.o
   CHECK   drivers/net/dsa/mt7530.c
drivers/net/dsa/mt7530.c:217 mt7530_mii_read() warn: call of 'warn_slowpath_fmt' with non-constant format argument
drivers/net/dsa/mt7530.c:454 mt7530_setup_port6() error: uninitialized symbol 'ncpo1'.
drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
drivers/net/dsa/mt7530.c:2324 mt7530_setup() error: uninitialized symbol 'interface'.

Arınç

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Dan Carpenter]

On Sun, Dec 17, 2023 at 03:22:27PM +0300, Arınç ÜNAL wrote:
> On 8.12.2023 21:46, Vladimir Oltean wrote:
> > Hmm, maybe the problem, all along, was that we let the -ENODEV return
> > code from of_get_phy_mode() pass through? "interface" will really be
> > uninitialized in that case. It's not a false positive.
> > 
> > Instead of:
> > 
> > 	ret = of_get_phy_mode(mac_np, &interface);
> > 	if (ret && ret != -ENODEV) {
> > 		...
> > 		return ret;
> > 	}
> > 
> > it should have been like this, to not complain:
> > 
> > 	ret = of_get_phy_mode(mac_np, &interface);
> > 	if (ret) {
> > 		...
> > 		return ret;
> > 	}
> > 
> 
> I just tried this, smatch still reports "interface" as uninitialised.
> 
> $ export ARCH=mips CROSS_COMPILE=mips-linux-gnu-
> $ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
> 
>   UPD     include/config/kernel.release
>   UPD     include/generated/utsrelease.h
>   CHECK   scripts/mod/empty.c
>   CALL    scripts/checksyscalls.sh
>   CC      drivers/net/dsa/mt7530.o
>   CHECK   drivers/net/dsa/mt7530.c
> drivers/net/dsa/mt7530.c:217 mt7530_mii_read() warn: call of 'warn_slowpath_fmt' with non-constant format argument
> drivers/net/dsa/mt7530.c:454 mt7530_setup_port6() error: uninitialized symbol 'ncpo1'.
> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
> drivers/net/dsa/mt7530.c:2324 mt7530_setup() error: uninitialized symbol 'interface'.

That's so strange.

Vladimir was right that I was misreading what he said and also hadn't
noticed the break.

For me, his approach silences the warning with or without the cross
function DB.  Also of_get_phy_mode() initializes interface on all paths
so checking for -EINVAL doesn't matter as far as this warning is
concerned.

regards,
dan carpenter

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Arınç ÜNAL]

On 1/2/24 13:16, Dan Carpenter wrote:
> On Sun, Dec 17, 2023 at 03:22:27PM +0300, Arınç ÜNAL wrote:
>> On 8.12.2023 21:46, Vladimir Oltean wrote:
>>> Hmm, maybe the problem, all along, was that we let the -ENODEV return
>>> code from of_get_phy_mode() pass through? "interface" will really be
>>> uninitialized in that case. It's not a false positive.
>>>
>>> Instead of:
>>>
>>> 	ret = of_get_phy_mode(mac_np, &interface);
>>> 	if (ret && ret != -ENODEV) {
>>> 		...
>>> 		return ret;
>>> 	}
>>>
>>> it should have been like this, to not complain:
>>>
>>> 	ret = of_get_phy_mode(mac_np, &interface);
>>> 	if (ret) {
>>> 		...
>>> 		return ret;
>>> 	}
>>>
>>
>> I just tried this, smatch still reports "interface" as uninitialised.
>>
>> $ export ARCH=mips CROSS_COMPILE=mips-linux-gnu-
>> $ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
>>
>>    UPD     include/config/kernel.release
>>    UPD     include/generated/utsrelease.h
>>    CHECK   scripts/mod/empty.c
>>    CALL    scripts/checksyscalls.sh
>>    CC      drivers/net/dsa/mt7530.o
>>    CHECK   drivers/net/dsa/mt7530.c
>> drivers/net/dsa/mt7530.c:217 mt7530_mii_read() warn: call of 'warn_slowpath_fmt' with non-constant format argument
>> drivers/net/dsa/mt7530.c:454 mt7530_setup_port6() error: uninitialized symbol 'ncpo1'.
>> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
>> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
>> drivers/net/dsa/mt7530.c:2324 mt7530_setup() error: uninitialized symbol 'interface'.
> 
> That's so strange.
> 
> Vladimir was right that I was misreading what he said and also hadn't
> noticed the break.
> 
> For me, his approach silences the warning with or without the cross
> function DB.  Also of_get_phy_mode() initializes interface on all paths
> so checking for -EINVAL doesn't matter as far as this warning is
> concerned.

I must be missing something.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 7f9d63b61168..05ce3face47c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2325,7 +2325,7 @@ mt7530_setup(struct dsa_switch *ds)
  
  			if (phy_node->parent == priv->dev->of_node->parent) {
  				ret = of_get_phy_mode(mac_np, &interface);
-				if (ret && ret != -ENODEV) {
+				if (ret) {
  					of_node_put(mac_np);
  					of_node_put(phy_node);
  					return ret;

$ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
   CHECK   scripts/mod/empty.c
   CALL    scripts/checksyscalls.sh
   DESCEND objtool
   INSTALL libsubcmd_headers
   CC      drivers/net/dsa/mt7530.o
   CHECK   drivers/net/dsa/mt7530.c
drivers/net/dsa/mt7530.c:469 mt7530_pad_clk_setup() error: uninitialized symbol 'ncpo1'.
drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
drivers/net/dsa/mt7530.c:2346 mt7530_setup() error: uninitialized symbol 'interface'.

Just so you know, I intend to remove this whole PHY muxing feature once I
bring changing DSA conduit support to this subdriver. I've got two strong
reasons for this.

- Changing the DSA conduit achieves the same result with the only overhead
   being the DSA header included on every frame.

- There can't be proper dt-bindings for it as the nature of the feature
   shows that it represents an optional way to operate the hardware, it does
   not represent a hardware design. Overall, the implementation is a hack to
   make it work for specific hardware (switch must be connected to gmac1 of
   a MediaTek SoC, no PHY must be present at address 0 or 4 on the MDIO bus
   of the SoC. It should rather be configurable on userspace. Which will
   never happen as it is specific to this switch and the changing DSA
   conduit feature is the perfect substitute for this.

Let me know if you've got any suggestions that can get rid of the warning
without reworking the whole code block. Otherwise, I'm just going to ignore
it until I get rid of the whole code block.

Arınç

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Vladimir Oltean]

On Sat, Jan 06, 2024 at 08:01:25PM +0200, Arınç ÜNAL wrote:
> I must be missing something.
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 7f9d63b61168..05ce3face47c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2325,7 +2325,7 @@ mt7530_setup(struct dsa_switch *ds)
>  			if (phy_node->parent == priv->dev->of_node->parent) {
>  				ret = of_get_phy_mode(mac_np, &interface);
> -				if (ret && ret != -ENODEV) {
> +				if (ret) {
>  					of_node_put(mac_np);
>  					of_node_put(phy_node);
>  					return ret;
> 
> $ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
>   CHECK   scripts/mod/empty.c
>   CALL    scripts/checksyscalls.sh
>   DESCEND objtool
>   INSTALL libsubcmd_headers
>   CC      drivers/net/dsa/mt7530.o
>   CHECK   drivers/net/dsa/mt7530.c
> drivers/net/dsa/mt7530.c:469 mt7530_pad_clk_setup() error: uninitialized symbol 'ncpo1'.
> drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
> drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
> drivers/net/dsa/mt7530.c:2346 mt7530_setup() error: uninitialized symbol 'interface'.

Yes, well _now_ it is a false positive, probably because smatch cannot
determine that when priv->p5_intf_sel has been set to P5_INTF_SEL_PHY_P0
or P5_INTF_SEL_PHY_P4, "interface" should have been also initialized.
But it doesn't matter, you can ignore a false positive. I'm also seeing it.
Although you should check whether treating -ENODEV as a hard error is fine
and won't cause regressions.

> Just so you know, I intend to remove this whole PHY muxing feature once I
> bring changing DSA conduit support to this subdriver. I've got two strong
> reasons for this.
> - Changing the DSA conduit achieves the same result with the only overhead
>   being the DSA header included on every frame.
> 
> - There can't be proper dt-bindings for it as the nature of the feature
>   shows that it represents an optional way to operate the hardware, it does
>   not represent a hardware design. Overall, the implementation is a hack to
>   make it work for specific hardware (switch must be connected to gmac1 of
>   a MediaTek SoC, no PHY must be present at address 0 or 4 on the MDIO bus
>   of the SoC. It should rather be configurable on userspace. Which will
>   never happen as it is specific to this switch and the changing DSA
>   conduit feature is the perfect substitute for this.

Is PHY muxing a "true" switch bypass, or is it just a route through the
switch for all packets coming from GMAC5 to go to phy0 or phy4? If the
latter, I agree that dynamic conduit changing is a more flexible option,
not to mention the user space tooling is already there.

Are there existing systems that use PHY muxing? The possible problem I
see is breaking those boards which have a phy-handle on gmac5, if the
mt7530 driver is no longer going to modify its HWTRAP register.

> 
> Let me know if you've got any suggestions that can get rid of the warning
> without reworking the whole code block. Otherwise, I'm just going to ignore
> it until I get rid of the whole code block.

The obvious way would be to leave the initialization to PHY_INTERFACE_MODE_NA
there. Or to just ignore the warning.

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Arınç ÜNAL]

On 9.01.2024 17:57, Vladimir Oltean wrote:
> Yes, well _now_ it is a false positive, probably because smatch cannot
> determine that when priv->p5_intf_sel has been set to P5_INTF_SEL_PHY_P0
> or P5_INTF_SEL_PHY_P4, "interface" should have been also initialized.
> But it doesn't matter, you can ignore a false positive. I'm also seeing it.
> Although you should check whether treating -ENODEV as a hard error is fine
> and won't cause regressions.
> 
>> Just so you know, I intend to remove this whole PHY muxing feature once I
>> bring changing DSA conduit support to this subdriver. I've got two strong
>> reasons for this.
>> - Changing the DSA conduit achieves the same result with the only overhead
>>    being the DSA header included on every frame.
>>
>> - There can't be proper dt-bindings for it as the nature of the feature
>>    shows that it represents an optional way to operate the hardware, it does
>>    not represent a hardware design. Overall, the implementation is a hack to
>>    make it work for specific hardware (switch must be connected to gmac1 of
>>    a MediaTek SoC, no PHY must be present at address 0 or 4 on the MDIO bus
>>    of the SoC. It should rather be configurable on userspace. Which will
>>    never happen as it is specific to this switch and the changing DSA
>>    conduit feature is the perfect substitute for this.
> 
> Is PHY muxing a "true" switch bypass, or is it just a route through the
> switch for all packets coming from GMAC5 to go to phy0 or phy4? If the
> latter, I agree that dynamic conduit changing is a more flexible option,
> not to mention the user space tooling is already there.

It's the latter, and that's exactly what I think.

> 
> Are there existing systems that use PHY muxing? The possible problem I
> see is breaking those boards which have a phy-handle on gmac5, if the
> mt7530 driver is no longer going to modify its HWTRAP register.

Ah see, for PHY muxing, the driver actually wants the phy-handle to be put
on the SoC MAC, and the PHY to be defined on the SoC ethernet's MDIO bus.
We don't even define gmac5 as a port on the switch dt-bindings.

While none of the DTs on the Linux repository utilise this, some of the
mt7621 DTs on OpenWrt do. The change in behaviour will only be that phy0/4
will be inaccessible from the SoC MAC's network interface. I de-facto
maintain the mt7621 device tree source files there. I intend to revert it
along with adding port 5 as a CPU port so that the conduit changing feature
becomes available.

> 
>>
>> Let me know if you've got any suggestions that can get rid of the warning
>> without reworking the whole code block. Otherwise, I'm just going to ignore
>> it until I get rid of the whole code block.
> 
> The obvious way would be to leave the initialization to PHY_INTERFACE_MODE_NA
> there. Or to just ignore the warning.

I'll ignore.

Arınç

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Vladimir Oltean]

On Wed, Jan 10, 2024 at 10:26:54AM +0300, Arınç ÜNAL wrote:
> > Are there existing systems that use PHY muxing? The possible problem I
> > see is breaking those boards which have a phy-handle on gmac5, if the
> > mt7530 driver is no longer going to modify its HWTRAP register.
> 
> Ah see, for PHY muxing, the driver actually wants the phy-handle to be put
> on the SoC MAC, and the PHY to be defined on the SoC ethernet's MDIO bus.
> We don't even define gmac5 as a port on the switch dt-bindings.

I noticed that from the code already. Maybe I shouldn't have said
"gmac5" when I meant "the GMAC attached to switch port 5, aka GMAC0".
I was under the impression that you were also using this slightly
incorrect terminology, to keep a numerical association between the CPU
port number and its directly attached GMAC.

> While none of the DTs on the Linux repository utilise this, some of the
> mt7621 DTs on OpenWrt do. The change in behaviour will only be that phy0/4
> will be inaccessible from the SoC MAC's network interface. I de-facto
> maintain the mt7621 device tree source files there. I intend to revert it
> along with adding port 5 as a CPU port so that the conduit changing feature
> becomes available.

If OpenWrt kernels are always shipped in tandem with updated device
trees (i.e. no Arm SystemReady IR platforms, where the DT is provided by
U-Boot), I won't oppose to retracting features described via DT if their
platform maintainers agree in a wide enough circle that the breakage is
manageable.

BTW, besides OpenWrt, what other software is deployed on these SoCs
typically?

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Arınç ÜNAL]

On 10.01.2024 21:23, Vladimir Oltean wrote:
> On Wed, Jan 10, 2024 at 10:26:54AM +0300, Arınç ÜNAL wrote:
>>> Are there existing systems that use PHY muxing? The possible problem I
>>> see is breaking those boards which have a phy-handle on gmac5, if the
>>> mt7530 driver is no longer going to modify its HWTRAP register.
>>
>> Ah see, for PHY muxing, the driver actually wants the phy-handle to be put
>> on the SoC MAC, and the PHY to be defined on the SoC ethernet's MDIO bus.
>> We don't even define gmac5 as a port on the switch dt-bindings.
> 
> I noticed that from the code already. Maybe I shouldn't have said
> "gmac5" when I meant "the GMAC attached to switch port 5, aka GMAC0".
> I was under the impression that you were also using this slightly
> incorrect terminology, to keep a numerical association between the CPU
> port number and its directly attached GMAC.
> 
>> While none of the DTs on the Linux repository utilise this, some of the
>> mt7621 DTs on OpenWrt do. The change in behaviour will only be that phy0/4
>> will be inaccessible from the SoC MAC's network interface. I de-facto
>> maintain the mt7621 device tree source files there. I intend to revert it
>> along with adding port 5 as a CPU port so that the conduit changing feature
>> becomes available.
> 
> If OpenWrt kernels are always shipped in tandem with updated device
> trees (i.e. no Arm SystemReady IR platforms, where the DT is provided by
> U-Boot), I won't oppose to retracting features described via DT if their
> platform maintainers agree in a wide enough circle that the breakage is
> manageable.

I will see to this when the time comes.

> 
> BTW, besides OpenWrt, what other software is deployed on these SoCs
> typically?

Other than OpenWrt which is widely used for these SoCs for its ease of
flashing and upgrading, compatibility with legacy U-boot versions that
usually come with any vendor making a product out of these SoCs, I can only
talk about what I deploy to run Linux. I use mainline U-Boot along with the
device trees from the Linux repository to boot mainline Linux kernels with
Buildroot as the filesystem.

Arınç

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Vladimir Oltean]

On Thu, Jan 11, 2024 at 01:22:12PM +0300, Arınç ÜNAL wrote:
> > BTW, besides OpenWrt, what other software is deployed on these SoCs
> > typically?
> 
> Other than OpenWrt which is widely used for these SoCs for its ease of
> flashing and upgrading, compatibility with legacy U-boot versions that
> usually come with any vendor making a product out of these SoCs, I can only
> talk about what I deploy to run Linux. I use mainline U-Boot along with the
> device trees from the Linux repository to boot mainline Linux kernels with
> Buildroot as the filesystem.
> 
> Arınç

I meant what other software (operating system) _instead_ of OpenWRT.
I'm trying to figure out what other users of PHY muxing there might be.

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Frank Wunderlich]

Am 11. Januar 2024 11:31:46 MEZ schrieb Vladimir Oltean <olteanv@gmail.com>:
>On Thu, Jan 11, 2024 at 01:22:12PM +0300, Arınç ÜNAL wrote:
>> > BTW, besides OpenWrt, what other software is deployed on these SoCs
>> > typically?
>> 
>> Other than OpenWrt which is widely used for these SoCs for its ease of
>> flashing and upgrading, compatibility with legacy U-boot versions that
>> usually come with any vendor making a product out of these SoCs, I can only
>> talk about what I deploy to run Linux. I use mainline U-Boot along with the
>> device trees from the Linux repository to boot mainline Linux kernels with
>> Buildroot as the filesystem.
>> 
>> Arınç
>
>I meant what other software (operating system) _instead_ of OpenWRT.
>I'm trying to figure out what other users of PHY muxing there might be.

Hi

I (and possibly others) use debian/ubuntu and there are also archlinux images for the bpi router boards.


regards Frank

Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Arınç ÜNAL]

On 11.01.2024 13:31, Vladimir Oltean wrote:
> On Thu, Jan 11, 2024 at 01:22:12PM +0300, Arınç ÜNAL wrote:
>>> BTW, besides OpenWrt, what other software is deployed on these SoCs
>>> typically?
>>
>> Other than OpenWrt which is widely used for these SoCs for its ease of
>> flashing and upgrading, compatibility with legacy U-boot versions that
>> usually come with any vendor making a product out of these SoCs, I can only
>> talk about what I deploy to run Linux. I use mainline U-Boot along with the
>> device trees from the Linux repository to boot mainline Linux kernels with
>> Buildroot as the filesystem.
>>
>> Arınç
> 
> I meant what other software (operating system) _instead_ of OpenWRT.
> I'm trying to figure out what other users of PHY muxing there might be.

I'm not aware of any other projects that utilise PHY muxing of MT7530. It
was me spending a few months bringing the feature to OpenWrt in the first
place.

Arınç

[PATCH net-next 08/15] net: dsa: mt7530: empty default case on mt7530_setup_port5()

[Arınç ÜNAL]

There're two code paths for setting up port 5:

mt7530_setup()
-> mt7530_setup_port5()

mt753x_phylink_mac_config()
-> mt753x_mac_config()
   -> mt7530_mac_config()
      -> mt7530_setup_port5()

On the first code path, priv->p5_intf_sel is either set to
P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4 when mt7530_setup_port5() is run.

On the second code path, priv->p5_intf_sel is set to P5_INTF_SEL_GMAC5 when
mt7530_setup_port5() is run.

Empty the default case which will never run but is needed nonetheless to
handle all the remaining enumeration values.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/mt7530.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 1aab4c3f28b0..7de55cbf19a4 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -943,9 +943,7 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 		val &= ~MHWTRAP_P5_DIS;
 		break;
 	default:
-		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
-			priv->p5_intf_sel);
-		goto unlock_exit;
+		break;
 	}
 
 	/* Setup RGMII settings */
@@ -975,7 +973,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 	dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
 		val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
 
-unlock_exit:
 	mutex_unlock(&priv->reg_mutex);
 }
 
-- 
2.40.1

[PATCH net-next 09/15] net: dsa: mt7530: call port 6 setup from mt7530_mac_config()

[Arınç ÜNAL]

mt7530_pad_clk_setup() is called if port 6 is enabled. It used to do more
things than setting up port 6. That part was moved to more appropriate
locations, mt7530_setup() and mt7530_pll_setup().

Now that all it does is set up port 6, rename it to mt7530_setup_port6(),
and move it to a more appropriate location, under mt7530_mac_config().

Leave an empty mt7530_pad_clk_setup() to satisfy the pad_setup function
pointer.

This is the code path for setting up the ports before:

mt753x_phylink_mac_config()
-> mt753x_mac_config()
   -> mt7530_mac_config()
      -> mt7530_setup_port5()
-> mt753x_pad_setup()
   -> mt7530_pad_clk_setup()

This is after:

mt753x_phylink_mac_config()
-> mt753x_mac_config()
   -> mt7530_mac_config()
      -> mt7530_setup_port5()
      -> mt7530_setup_port6()

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/mt7530.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 7de55cbf19a4..ae037ff7d4c5 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -415,7 +415,7 @@ mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
 
 /* Setup port 6 interface mode and TRGMII TX circuit */
 static int
-mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
+mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 {
 	struct mt7530_priv *priv = ds->priv;
 	u32 ncpo1, ssc_delta, trgint, xtal;
@@ -487,6 +487,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
 	return 0;
 }
 
+static int
+mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
+{
+	return 0;
+}
+
 static int
 mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
 {
@@ -2608,12 +2614,15 @@ mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
 {
 	struct mt7530_priv *priv = ds->priv;
+	int ret;
 
-	/* Only need to setup port5. */
-	if (port != 5)
-		return 0;
-
-	mt7530_setup_port5(priv->ds, interface);
+	if (port == 5) {
+		mt7530_setup_port5(priv->ds, interface);
+	} else if (port == 6) {
+		ret = mt7530_setup_port6(priv->ds, interface);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
-- 
2.40.1

[PATCH net-next 10/15] net: dsa: mt7530: remove pad_setup function pointer

[Arınç ÜNAL]

The pad_setup function pointer was introduced with 88bdef8be9f6 ("net: dsa:
mt7530: Extend device data ready for adding a new hardware"). It was being
used to set up the core clock and port 6 of the MT7530 switch, and pll of
the MT7531 switch.

All of these were moved to more appropriate locations, and it was never
used for the switch on the MT7988 SoC. Therefore, this function pointer
hasn't got a use anymore. Remove it.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/mt7530.c | 36 ++----------------------------------
 drivers/net/dsa/mt7530.h |  3 ---
 2 files changed, 2 insertions(+), 37 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ae037ff7d4c5..efe5ffe3455d 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -487,18 +487,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 	return 0;
 }
 
-static int
-mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
-{
-	return 0;
-}
-
-static int
-mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
-{
-	return 0;
-}
-
 static void
 mt7531_pll_setup(struct mt7530_priv *priv)
 {
@@ -2601,14 +2589,6 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
 	}
 }
 
-static int
-mt753x_pad_setup(struct dsa_switch *ds, const struct phylink_link_state *state)
-{
-	struct mt7530_priv *priv = ds->priv;
-
-	return priv->info->pad_setup(ds, state->interface);
-}
-
 static int
 mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
@@ -2778,8 +2758,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		if (priv->p6_interface == state->interface)
 			break;
 
-		mt753x_pad_setup(ds, state);
-
 		if (mt753x_mac_config(ds, port, mode, state) < 0)
 			goto unsupported;
 
@@ -3092,11 +3070,6 @@ mt753x_conduit_state_change(struct dsa_switch *ds,
 			   CPU_PORT(__ffs(priv->active_cpu_ports)));
 }
 
-static int mt7988_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
-{
-	return 0;
-}
-
 static int mt7988_setup(struct dsa_switch *ds)
 {
 	struct mt7530_priv *priv = ds->priv;
@@ -3160,7 +3133,6 @@ const struct mt753x_info mt753x_table[] = {
 		.phy_write_c22 = mt7530_phy_write_c22,
 		.phy_read_c45 = mt7530_phy_read_c45,
 		.phy_write_c45 = mt7530_phy_write_c45,
-		.pad_setup = mt7530_pad_clk_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
 	},
@@ -3172,7 +3144,6 @@ const struct mt753x_info mt753x_table[] = {
 		.phy_write_c22 = mt7530_phy_write_c22,
 		.phy_read_c45 = mt7530_phy_read_c45,
 		.phy_write_c45 = mt7530_phy_write_c45,
-		.pad_setup = mt7530_pad_clk_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
 	},
@@ -3184,7 +3155,6 @@ 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,
-		.pad_setup = mt7531_pad_setup,
 		.cpu_port_config = mt7531_cpu_port_config,
 		.mac_port_get_caps = mt7531_mac_port_get_caps,
 		.mac_port_config = mt7531_mac_config,
@@ -3197,7 +3167,6 @@ 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,
-		.pad_setup = mt7988_pad_setup,
 		.cpu_port_config = mt7988_cpu_port_config,
 		.mac_port_get_caps = mt7988_mac_port_get_caps,
 		.mac_port_config = mt7988_mac_config,
@@ -3227,9 +3196,8 @@ mt7530_probe_common(struct mt7530_priv *priv)
 	/* Sanity check if these required device operations are filled
 	 * properly.
 	 */
-	if (!priv->info->sw_setup || !priv->info->pad_setup ||
-	    !priv->info->phy_read_c22 || !priv->info->phy_write_c22 ||
-	    !priv->info->mac_port_get_caps ||
+	if (!priv->info->sw_setup || !priv->info->phy_read_c22 ||
+	    !priv->info->phy_write_c22 || !priv->info->mac_port_get_caps ||
 	    !priv->info->mac_port_config)
 		return -EINVAL;
 
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 12c1731d6201..aabe7f6cffeb 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -704,8 +704,6 @@ struct mt753x_pcs {
  * @phy_write_c22:	Holding the way writing PHY port using C22
  * @phy_read_c45:	Holding the way reading PHY port using C45
  * @phy_write_c45:	Holding the way writing PHY port using C45
- * @pad_setup:		Holding the way setting up the bus pad for a certain
- *			MAC port
  * @phy_mode_supported:	Check if the PHY type is being supported on a certain
  *			port
  * @mac_port_validate:	Holding the way to set addition validate type for a
@@ -726,7 +724,6 @@ struct mt753x_info {
 			    int regnum);
 	int (*phy_write_c45)(struct mt7530_priv *priv, int port, int devad,
 			     int regnum, u16 val);
-	int (*pad_setup)(struct dsa_switch *ds, phy_interface_t interface);
 	int (*cpu_port_config)(struct dsa_switch *ds, int port);
 	void (*mac_port_get_caps)(struct dsa_switch *ds, int port,
 				  struct phylink_config *config);
-- 
2.40.1

[PATCH net-next 11/15] net: dsa: mt7530: move XTAL check to mt7530_setup()

[Arınç ÜNAL]

The crystal frequency concerns the switch core. The frequency should be
checked when the switch is being set up so the driver can reject the
unsupported hardware earlier and without requiring port 6 to be used.

Move it to mt7530_setup(). Drop the unnecessary function printing.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mt7530.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index efe5ffe3455d..167b340350b3 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -422,13 +422,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 
 	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
 
-	if (xtal == HWTRAP_XTAL_20MHZ) {
-		dev_err(priv->dev,
-			"%s: MT7530 with a 20MHz XTAL is not supported!\n",
-			__func__);
-		return -EINVAL;
-	}
-
 	switch (interface) {
 	case PHY_INTERFACE_MODE_RGMII:
 		trgint = 0;
@@ -2235,6 +2228,12 @@ mt7530_setup(struct dsa_switch *ds)
 		return -ENODEV;
 	}
 
+	if ((val & HWTRAP_XTAL_MASK) == HWTRAP_XTAL_20MHZ) {
+		dev_err(priv->dev,
+			"MT7530 with a 20MHz XTAL is not supported!\n");
+		return -EINVAL;
+	}
+
 	/* Reset the switch through internal reset */
 	mt7530_write(priv, MT7530_SYS_CTRL,
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
-- 
2.40.1

[PATCH net-next 12/15] net: dsa: mt7530: move enabling port 6 to mt7530_setup_port6()

[Arınç ÜNAL]

Enable port 6 only when port 6 is being used. Update the comment on
mt7530_setup() with a better explanation. Do not set MHWTRAP_MANUAL on
mt7530_setup_port5() as it's already done on mt7530_setup() beforehand.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 167b340350b3..2608b09d3295 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -420,6 +420,8 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 	struct mt7530_priv *priv = ds->priv;
 	u32 ncpo1, ssc_delta, trgint, xtal;
 
+	mt7530_clear(priv, MT7530_MHWTRAP, MHWTRAP_P6_DIS);
+
 	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
 
 	switch (interface) {
@@ -910,7 +912,7 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 
-	val |= MHWTRAP_MANUAL | MHWTRAP_P5_MAC_SEL | MHWTRAP_P5_DIS;
+	val |= MHWTRAP_P5_MAC_SEL | MHWTRAP_P5_DIS;
 	val &= ~MHWTRAP_P5_RGMII_MODE & ~MHWTRAP_PHY0_SEL;
 
 	switch (priv->p5_intf_sel) {
@@ -2250,9 +2252,11 @@ mt7530_setup(struct dsa_switch *ds)
 		mt7530_rmw(priv, MT7530_TRGMII_RD(i),
 			   RD_TAP_MASK, RD_TAP(16));
 
-	/* Enable port 6 */
+	/* Directly access the PHY registers via C_MDC/C_MDIO. The bit that
+	 * enables modifying the hardware trap must be set for this.
+	 */
 	val = mt7530_read(priv, MT7530_MHWTRAP);
-	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
+	val &= ~MHWTRAP_PHY_ACCESS;
 	val |= MHWTRAP_MANUAL;
 	mt7530_write(priv, MT7530_MHWTRAP, val);
 
-- 
2.40.1

[PATCH net-next 13/15] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Arınç ÜNAL]

This code is from before this driver was converted to phylink API. Phylink
deals with the unsupported interface cases before mt7530_setup_port6() is
run. Therefore, the default case would never run. However, it must be
defined nonetheless to handle all the remaining enumeration values, the
phy-modes.

Switch to if statement for RGMII and return which simplifies the code and
saves an indent.

Do not set P6_INTF_MODE, which is the the three least significant bits of
the MT7530_P6ECR register, to 0 for RGMII as it will already be 0 after
reset.

Read XTAL after checking for RGMII as it's only needed for the TRGMII
interface mode.

Change mt7530_setup_port6() to void now that there're no error cases left.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 100 ++++++++++++++++-----------------------
 1 file changed, 40 insertions(+), 60 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2608b09d3295..f36f240231b5 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -414,72 +414,56 @@ mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
 }
 
 /* Setup port 6 interface mode and TRGMII TX circuit */
-static int
+static void
 mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 {
 	struct mt7530_priv *priv = ds->priv;
-	u32 ncpo1, ssc_delta, trgint, xtal;
+	u32 ncpo1, ssc_delta, xtal;
 
 	mt7530_clear(priv, MT7530_MHWTRAP, MHWTRAP_P6_DIS);
 
+	if (interface == PHY_INTERFACE_MODE_RGMII)
+		return;
+
+	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK, P6_INTF_MODE(1));
+
 	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
 
-	switch (interface) {
-	case PHY_INTERFACE_MODE_RGMII:
-		trgint = 0;
-		break;
-	case PHY_INTERFACE_MODE_TRGMII:
-		trgint = 1;
+	if (xtal == HWTRAP_XTAL_25MHZ)
+		ssc_delta = 0x57;
+	else
+		ssc_delta = 0x87;
+
+	if (priv->id == ID_MT7621) {
+		/* PLL frequency: 125MHz: 1.0GBit */
+		if (xtal == HWTRAP_XTAL_40MHZ)
+			ncpo1 = 0x0640;
 		if (xtal == HWTRAP_XTAL_25MHZ)
-			ssc_delta = 0x57;
-		else
-			ssc_delta = 0x87;
-		if (priv->id == ID_MT7621) {
-			/* PLL frequency: 125MHz: 1.0GBit */
-			if (xtal == HWTRAP_XTAL_40MHZ)
-				ncpo1 = 0x0640;
-			if (xtal == HWTRAP_XTAL_25MHZ)
-				ncpo1 = 0x0a00;
-		} else { /* PLL frequency: 250MHz: 2.0Gbit */
-			if (xtal == HWTRAP_XTAL_40MHZ)
-				ncpo1 = 0x0c80;
-			if (xtal == HWTRAP_XTAL_25MHZ)
-				ncpo1 = 0x1400;
-		}
-		break;
-	default:
-		dev_err(priv->dev, "xMII interface %d not supported\n",
-			interface);
-		return -EINVAL;
+			ncpo1 = 0x0a00;
+	} else { /* PLL frequency: 250MHz: 2.0Gbit */
+		if (xtal == HWTRAP_XTAL_40MHZ)
+			ncpo1 = 0x0c80;
+		if (xtal == HWTRAP_XTAL_25MHZ)
+			ncpo1 = 0x1400;
 	}
 
-	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
-		   P6_INTF_MODE(trgint));
-
-	if (trgint) {
-		/* Disable the MT7530 TRGMII clocks */
-		core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
-
-		/* Setup the MT7530 TRGMII Tx Clock */
-		core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
-		core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
-		core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
-		core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
-		core_write(priv, CORE_PLL_GROUP4,
-			   RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN |
-			   RG_SYSPLL_BIAS_LPF_EN);
-		core_write(priv, CORE_PLL_GROUP2,
-			   RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN |
-			   RG_SYSPLL_POSDIV(1));
-		core_write(priv, CORE_PLL_GROUP7,
-			   RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) |
-			   RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
+	/* Disable the MT7530 TRGMII clocks */
+	core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
 
-		/* Enable the MT7530 TRGMII clocks */
-		core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
-	}
+	/* Setup the MT7530 TRGMII Tx Clock */
+	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
+	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
+	core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
+	core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
+	core_write(priv, CORE_PLL_GROUP4, RG_SYSPLL_DDSFBK_EN |
+		   RG_SYSPLL_BIAS_EN | RG_SYSPLL_BIAS_LPF_EN);
+	core_write(priv, CORE_PLL_GROUP2, RG_SYSPLL_EN_NORMAL |
+		   RG_SYSPLL_VODEN | RG_SYSPLL_POSDIV(1));
+	core_write(priv, CORE_PLL_GROUP7, RG_LCDDS_PCW_NCPO_CHG |
+		   RG_LCCDS_C(3) | RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
 
-	return 0;
+	/* Enable the MT7530 TRGMII clocks */
+	core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
 }
 
 static void
@@ -2597,15 +2581,11 @@ mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
 {
 	struct mt7530_priv *priv = ds->priv;
-	int ret;
 
-	if (port == 5) {
+	if (port == 5)
 		mt7530_setup_port5(priv->ds, interface);
-	} else if (port == 6) {
-		ret = mt7530_setup_port6(priv->ds, interface);
-		if (ret)
-			return ret;
-	}
+	else if (port == 6)
+		mt7530_setup_port6(priv->ds, interface);
 
 	return 0;
 }
-- 
2.40.1

[PATCH net-next 14/15] net: dsa: mt7530: correct port capabilities of MT7988

[Arınç ÜNAL]

On the switch on the MT7988 SoC, there are only 4 PHYs. That's port 0 to 3.
Set the internal phy cases to '0 ... 3'.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/mt7530.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index f36f240231b5..ca42005ff3a9 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2562,7 +2562,7 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
 
 	switch (port) {
 	/* Internal PHY */
-	case 0 ... 4:
+	case 0 ... 3:
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
 		break;
-- 
2.40.1

[PATCH net-next 15/15] net: dsa: mt7530: do not clear config->supported_interfaces

[Arınç ÜNAL]

There's no need to clear the config->supported_interfaces bitmap before
reporting the supported interfaces as all bits in the bitmap will already
be initialized to zero when the phylink_config structure is allocated.
There's no code that would change the bitmap beforehand. Remove it.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/mt7530.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ca42005ff3a9..20ae147b823e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2558,8 +2558,6 @@ 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)
 {
-	phy_interface_zero(config->supported_interfaces);
-
 	switch (port) {
 	/* Internal PHY */
 	case 0 ... 3:
-- 
2.40.1

Re: [PATCH net-next 00/15] MT7530 DSA subdriver improvements

[Arınç ÜNAL]

Can I receive reviews for patches 6, 12, 13, 14, and 15?

Thanks.
Arınç

Re: [PATCH net-next 00/15] MT7530 DSA subdriver improvements

[Vladimir Oltean]

Hi Arınç,

On Sat, Dec 02, 2023 at 11:52:13AM +0300, Arınç ÜNAL wrote:
> Can I receive reviews for patches 6, 12, 13, 14, and 15?
> 
> Thanks.
> Arınç

Sorry, I don't have time to look at this old thread anymore.

Please repost the series with the feedback you got so far addressed.
Also, you can try to split into smaller, more cohesive groups of
patches, that have higher chances of getting merged.

Re: [PATCH net-next 00/15] MT7530 DSA subdriver improvements

[Arınç ÜNAL]

On 13.12.2023 18:19, Vladimir Oltean wrote:
> Hi Arınç,
> 
> On Sat, Dec 02, 2023 at 11:52:13AM +0300, Arınç ÜNAL wrote:
>> Can I receive reviews for patches 6, 12, 13, 14, and 15?
>>
>> Thanks.
>> Arınç
> 
> Sorry, I don't have time to look at this old thread anymore.
> 
> Please repost the series with the feedback you got so far addressed.
> Also, you can try to split into smaller, more cohesive groups of
> patches, that have higher chances of getting merged.

Thanks Vladimir. I've been fighting a bad fever with sore throat. I'll
submit the first 7 patches after working on your reviews.

Thanks a lot for your attention to my patches that's been going on for more
than half a year now.

Arınç