[RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[arinc9.unal]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

On the switch on the MT7988 SoC, there are only 4 PHYs. There's only port 6
as the CPU port, there's no port 5. Split the switch statement with a check
to enforce these for the switch on the MT7988 SoC. The internal phy-mode is
specific to MT7988 so put it for MT7988 only.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---

Daniel, this is based on the information you provided me about the switch.
I will add this to my current patch series if it looks good to you.

Arınç

---
 drivers/net/dsa/mt7530.c | 67 ++++++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 6fbbdcb5987f..f167fa135ef1 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2548,7 +2548,7 @@ 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 */
+	case 0 ... 3: /* Internal phy */
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
 		break;
@@ -2710,37 +2710,56 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	struct mt7530_priv *priv = ds->priv;
 	u32 mcr_cur, mcr_new;
 
-	switch (port) {
-	case 0 ... 4: /* Internal phy */
-		if (state->interface != PHY_INTERFACE_MODE_GMII &&
-		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
-			goto unsupported;
-		break;
-	case 5: /* Port 5, a CPU port. */
-		if (priv->p5_interface == state->interface)
+	if (priv->id == ID_MT7988) {
+		switch (port) {
+		case 0 ... 3: /* Internal phy */
+			if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
+				goto unsupported;
 			break;
+		case 6: /* Port 6, a CPU port. */
+			if (priv->p6_interface == state->interface)
+				break;
 
-		if (mt753x_mac_config(ds, port, mode, state) < 0)
+			if (mt753x_mac_config(ds, port, mode, state) < 0)
+				goto unsupported;
+
+			priv->p6_interface = state->interface;
+			break;
+		default:
 			goto unsupported;
+		}
+	} else {
+		switch (port) {
+		case 0 ... 4: /* Internal phy */
+			if (state->interface != PHY_INTERFACE_MODE_GMII)
+				goto unsupported;
+			break;
+		case 5: /* Port 5, a CPU port. */
+			if (priv->p5_interface == state->interface)
+				break;
 
-		if (priv->p5_intf_sel == P5_INTF_SEL_GMAC5 ||
-		    priv->p5_intf_sel == P5_INTF_SEL_GMAC5_SGMII)
-			priv->p5_interface = state->interface;
-		break;
-	case 6: /* Port 6, a CPU port. */
-		if (priv->p6_interface == state->interface)
+			if (mt753x_mac_config(ds, port, mode, state) < 0)
+				goto unsupported;
+
+			if (priv->p5_intf_sel == P5_INTF_SEL_GMAC5 ||
+			priv->p5_intf_sel == P5_INTF_SEL_GMAC5_SGMII)
+				priv->p5_interface = state->interface;
 			break;
+		case 6: /* Port 6, a CPU port. */
+			if (priv->p6_interface == state->interface)
+				break;
 
-		if (mt753x_mac_config(ds, port, mode, state) < 0)
-			goto unsupported;
+			if (mt753x_mac_config(ds, port, mode, state) < 0)
+				goto unsupported;
 
-		priv->p6_interface = state->interface;
-		break;
-	default:
+			priv->p6_interface = state->interface;
+			break;
+		default:
 unsupported:
-		dev_err(ds->dev, "%s: unsupported %s port: %i\n",
-			__func__, phy_modes(state->interface), port);
-		return;
+			dev_err(ds->dev, "%s: unsupported %s port: %i\n",
+				__func__, phy_modes(state->interface), port);
+			return;
+		}
 	}
 
 	mcr_cur = mt7530_read(priv, MT7530_PMCR_P(port));
-- 
2.37.2

Re: [RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[Arınç ÜNAL]

Port 6 configuration is shared so it's simpler to put a label.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 6fbbdcb5987f..009f2c0948d6 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2548,7 +2548,7 @@ 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 */
+	case 0 ... 3: /* Internal phy */
  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
  			  config->supported_interfaces);
  		break;
@@ -2710,37 +2710,50 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
  	struct mt7530_priv *priv = ds->priv;
  	u32 mcr_cur, mcr_new;
  
-	switch (port) {
-	case 0 ... 4: /* Internal phy */
-		if (state->interface != PHY_INTERFACE_MODE_GMII &&
-		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
+	if (priv->id == ID_MT7988) {
+		switch (port) {
+		case 0 ... 3: /* Internal phy */
+			if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
+				goto unsupported;
+			break;
+		case 6: /* Port 6, a CPU port. */
+			goto port6;
+		default:
  			goto unsupported;
-		break;
-	case 5: /* Port 5, a CPU port. */
-		if (priv->p5_interface == state->interface)
+		}
+	} else {
+		switch (port) {
+		case 0 ... 4: /* Internal phy */
+			if (state->interface != PHY_INTERFACE_MODE_GMII)
+				goto unsupported;
  			break;
+		case 5: /* Port 5, a CPU port. */
+			if (priv->p5_interface == state->interface)
+				break;
  
-		if (mt753x_mac_config(ds, port, mode, state) < 0)
-			goto unsupported;
+			if (mt753x_mac_config(ds, port, mode, state) < 0)
+				goto unsupported;
  
-		if (priv->p5_intf_sel == P5_INTF_SEL_GMAC5 ||
-		    priv->p5_intf_sel == P5_INTF_SEL_GMAC5_SGMII)
-			priv->p5_interface = state->interface;
-		break;
-	case 6: /* Port 6, a CPU port. */
-		if (priv->p6_interface == state->interface)
+			if (priv->p5_intf_sel == P5_INTF_SEL_GMAC5 ||
+			priv->p5_intf_sel == P5_INTF_SEL_GMAC5_SGMII)
+				priv->p5_interface = state->interface;
  			break;
+		case 6: /* Port 6, a CPU port. */
+port6:
+			if (priv->p6_interface == state->interface)
+				break;
  
-		if (mt753x_mac_config(ds, port, mode, state) < 0)
-			goto unsupported;
+			if (mt753x_mac_config(ds, port, mode, state) < 0)
+				goto unsupported;
  
-		priv->p6_interface = state->interface;
-		break;
-	default:
+			priv->p6_interface = state->interface;
+			break;
+		default:
  unsupported:
-		dev_err(ds->dev, "%s: unsupported %s port: %i\n",
-			__func__, phy_modes(state->interface), port);
-		return;
+			dev_err(ds->dev, "%s: unsupported %s port: %i\n",
+				__func__, phy_modes(state->interface), port);
+			return;
+		}
  	}
  
  	mcr_cur = mt7530_read(priv, MT7530_PMCR_P(port));

Arınç

Re: [RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[Russell King (Oracle)]

On Thu, Apr 06, 2023 at 01:04:45PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> On the switch on the MT7988 SoC, there are only 4 PHYs. There's only port 6
> as the CPU port, there's no port 5. Split the switch statement with a check
> to enforce these for the switch on the MT7988 SoC. The internal phy-mode is
> specific to MT7988 so put it for MT7988 only.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> 
> Daniel, this is based on the information you provided me about the switch.
> I will add this to my current patch series if it looks good to you.
> 
> Arınç
> 
> ---
>  drivers/net/dsa/mt7530.c | 67 ++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 6fbbdcb5987f..f167fa135ef1 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2548,7 +2548,7 @@ 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 */
> +	case 0 ... 3: /* Internal phy */
>  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>  			  config->supported_interfaces);
>  		break;
> @@ -2710,37 +2710,56 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  	struct mt7530_priv *priv = ds->priv;
>  	u32 mcr_cur, mcr_new;
>  
> -	switch (port) {
> -	case 0 ... 4: /* Internal phy */
> -		if (state->interface != PHY_INTERFACE_MODE_GMII &&
> -		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
> -			goto unsupported;
> -		break;
> -	case 5: /* Port 5, a CPU port. */
> -		if (priv->p5_interface == state->interface)
> +	if (priv->id == ID_MT7988) {
> +		switch (port) {
> +		case 0 ... 3: /* Internal phy */
> +			if (state->interface != PHY_INTERFACE_MODE_INTERNAL)

How do these end up with PHY_INTERFACE_MODE_INTERNAL ? phylib defaults
to GMII mode without something else being specified in DT.

Also note that you should *not* be validating state->interface in the
mac_config() method because it's way too late to reject it - if you get
an unsupported interface here, then that is down to the get_caps()
method being buggy. Only report interfaces in get_caps() that you are
prepared to handle in the rest of the system.

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

Re: [RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[Daniel Golle]

On Thu, Apr 06, 2023 at 12:07:01PM +0100, Russell King (Oracle) wrote:
> On Thu, Apr 06, 2023 at 01:04:45PM +0300, arinc9.unal@gmail.com wrote:
> > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > 
> > On the switch on the MT7988 SoC, there are only 4 PHYs. There's only port 6
> > as the CPU port, there's no port 5. Split the switch statement with a check
> > to enforce these for the switch on the MT7988 SoC. The internal phy-mode is
> > specific to MT7988 so put it for MT7988 only.
> > 
> > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > ---
> > 
> > Daniel, this is based on the information you provided me about the switch.
> > I will add this to my current patch series if it looks good to you.
> > 
> > Arınç
> > 
> > ---
> >  drivers/net/dsa/mt7530.c | 67 ++++++++++++++++++++++++++--------------
> >  1 file changed, 43 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index 6fbbdcb5987f..f167fa135ef1 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -2548,7 +2548,7 @@ 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 */
> > +	case 0 ... 3: /* Internal phy */
> >  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> >  			  config->supported_interfaces);
> >  		break;
> > @@ -2710,37 +2710,56 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  	struct mt7530_priv *priv = ds->priv;
> >  	u32 mcr_cur, mcr_new;
> >  
> > -	switch (port) {
> > -	case 0 ... 4: /* Internal phy */
> > -		if (state->interface != PHY_INTERFACE_MODE_GMII &&
> > -		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
> > -			goto unsupported;
> > -		break;
> > -	case 5: /* Port 5, a CPU port. */
> > -		if (priv->p5_interface == state->interface)
> > +	if (priv->id == ID_MT7988) {
> > +		switch (port) {
> > +		case 0 ... 3: /* Internal phy */
> > +			if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
> 
> How do these end up with PHY_INTERFACE_MODE_INTERNAL ? phylib defaults
> to GMII mode without something else being specified in DT.

As the link between mtk_eth_soc MAC and built-in switch as well as the
links between the built-in switch and the 4 built-in gigE PHYs are
internal, I wanted to describe them as such.

This was a reaction to the justified criticism that it doesn't make
sense to add 10GBase-KR and USXGMII as a proxy to actually indicate the
internal link between mt7530 CPU port and mtk_eth_soc gmac1 on the
MT7988, which Arınç had expressed in a review comment.

The phy-mode in the to-be-submitted DTS for MT7988 will be 'internal'
to describe the GMAC1<->MT7530 link as well as the MT7530<->gigE-PHY
links. I understand that for the built-in gigE PHYs we could just as
well us 'gmii', however, I thought that using 'internal' would be less
confusing as there is no actual GMII hardware interface.
And that's even more true for the link between GMAC1 and the built-in
switch, there are no actual USXGMII or 10GBase-KR hardware interfaces
but rather just a stateless internal link.

Sidenote: The same applies also for the link between GMAC2 and the
built-in 2500Base-T PHY. MediaTek was using 'xgmii' as PHY mode here to
program the internal muxes to connect the internal PHY with GMAC2, and
it took me a while to understand that there is no actual XGMII
interface anywhere, but they were rather just using this otherwise
unused interface mode as a flag for that...
Hence I decided to also use 'internal' PHY mode there, especially as in
this case there are several options: GMAC2 can also be connected to an
actual 1.25Gbaud/3.25Gbaud SGMII interface (pcs-mtk-lynxi again) OR an
actual 10GBase-KR/USXGMII SerDes, both can be muxed to the same pins
used to connect an external PHYs to the SoC.

Hence "phy-mode = 'internal';" will be a common occurance in mt7988.dtsi.

> 
> Also note that you should *not* be validating state->interface in the
> mac_config() method because it's way too late to reject it - if you get
> an unsupported interface here, then that is down to the get_caps()
> method being buggy. Only report interfaces in get_caps() that you are
> prepared to handle in the rest of the system.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[Arınç ÜNAL]

On 6.04.2023 14:07, Russell King (Oracle) wrote:
> On Thu, Apr 06, 2023 at 01:04:45PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> On the switch on the MT7988 SoC, there are only 4 PHYs. There's only port 6
>> as the CPU port, there's no port 5. Split the switch statement with a check
>> to enforce these for the switch on the MT7988 SoC. The internal phy-mode is
>> specific to MT7988 so put it for MT7988 only.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>
>> Daniel, this is based on the information you provided me about the switch.
>> I will add this to my current patch series if it looks good to you.
>>
>> Arınç
>>
>> ---
>>   drivers/net/dsa/mt7530.c | 67 ++++++++++++++++++++++++++--------------
>>   1 file changed, 43 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 6fbbdcb5987f..f167fa135ef1 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2548,7 +2548,7 @@ 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 */
>> +	case 0 ... 3: /* Internal phy */
>>   		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>>   			  config->supported_interfaces);
>>   		break;
>> @@ -2710,37 +2710,56 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>   	struct mt7530_priv *priv = ds->priv;
>>   	u32 mcr_cur, mcr_new;
>>   
>> -	switch (port) {
>> -	case 0 ... 4: /* Internal phy */
>> -		if (state->interface != PHY_INTERFACE_MODE_GMII &&
>> -		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
>> -			goto unsupported;
>> -		break;
>> -	case 5: /* Port 5, a CPU port. */
>> -		if (priv->p5_interface == state->interface)
>> +	if (priv->id == ID_MT7988) {
>> +		switch (port) {
>> +		case 0 ... 3: /* Internal phy */
>> +			if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
> 
> How do these end up with PHY_INTERFACE_MODE_INTERNAL ? phylib defaults
> to GMII mode without something else being specified in DT.
> 
> Also note that you should *not* be validating state->interface in the
> mac_config() method because it's way too late to reject it - if you get
> an unsupported interface here, then that is down to the get_caps()
> method being buggy. Only report interfaces in get_caps() that you are
> prepared to handle in the rest of the system.

This is already the case for all three get_caps(). The supported 
interfaces for each port are properly defined.

Though mt7988_mac_port_get_caps() clears the 
config->supported_interfaces bitmap before reporting the supported 
interfaces. I don't think this is needed as all bits in the bitmap 
should already be initialized to zero when the phylink_config structure 
is allocated.

I'm not sure if your suggestion is to make sure the supported interfaces 
are properly reported on get_caps(), or validate state->interface 
somewhere else.

Arınç

Re: [RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[Daniel Golle]

On Fri, Apr 07, 2023 at 12:43:41AM +0300, Arınç ÜNAL wrote:
> On 6.04.2023 14:07, Russell King (Oracle) wrote:
> > On Thu, Apr 06, 2023 at 01:04:45PM +0300, arinc9.unal@gmail.com wrote:
> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > 
> > > On the switch on the MT7988 SoC, there are only 4 PHYs. There's only port 6
> > > as the CPU port, there's no port 5. Split the switch statement with a check
> > > to enforce these for the switch on the MT7988 SoC. The internal phy-mode is
> > > specific to MT7988 so put it for MT7988 only.
> > > 
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > ---
> > > 
> > > Daniel, this is based on the information you provided me about the switch.
> > > I will add this to my current patch series if it looks good to you.
> > > 
> > > Arınç
> > > 
> > > ---
> > >   drivers/net/dsa/mt7530.c | 67 ++++++++++++++++++++++++++--------------
> > >   1 file changed, 43 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index 6fbbdcb5987f..f167fa135ef1 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -2548,7 +2548,7 @@ 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 */
> > > +	case 0 ... 3: /* Internal phy */
> > >   		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > >   			  config->supported_interfaces);
> > >   		break;
> > > @@ -2710,37 +2710,56 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > >   	struct mt7530_priv *priv = ds->priv;
> > >   	u32 mcr_cur, mcr_new;
> > > -	switch (port) {
> > > -	case 0 ... 4: /* Internal phy */
> > > -		if (state->interface != PHY_INTERFACE_MODE_GMII &&
> > > -		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > -			goto unsupported;
> > > -		break;
> > > -	case 5: /* Port 5, a CPU port. */
> > > -		if (priv->p5_interface == state->interface)
> > > +	if (priv->id == ID_MT7988) {
> > > +		switch (port) {
> > > +		case 0 ... 3: /* Internal phy */
> > > +			if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
> > 
> > How do these end up with PHY_INTERFACE_MODE_INTERNAL ? phylib defaults
> > to GMII mode without something else being specified in DT.
> > 
> > Also note that you should *not* be validating state->interface in the
> > mac_config() method because it's way too late to reject it - if you get
> > an unsupported interface here, then that is down to the get_caps()
> > method being buggy. Only report interfaces in get_caps() that you are
> > prepared to handle in the rest of the system.
> 
> This is already the case for all three get_caps(). The supported interfaces
> for each port are properly defined.
> 
> Though mt7988_mac_port_get_caps() clears the config->supported_interfaces
> bitmap before reporting the supported interfaces. I don't think this is
> needed as all bits in the bitmap should already be initialized to zero when
> the phylink_config structure is allocated.
> 
> I'm not sure if your suggestion is to make sure the supported interfaces are
> properly reported on get_caps(), or validate state->interface somewhere
> else.

I think what Russell meant is just there is no point in being overly
precise about permitted interface modes in mt753x_phylink_mac_config,
as this function is not meant and called too late to validate the
validity of the selected interface mode.

You change to mt7988_mac_port_get_caps looks correct to me and doing
this will already prevent mt753x_phylink_mac_config from ever being
called on MT7988 for port == 4 as well as and port == 5.

Re: [RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[Arınç ÜNAL]

On 7.04.2023 00:57, Daniel Golle wrote:
> On Fri, Apr 07, 2023 at 12:43:41AM +0300, Arınç ÜNAL wrote:
>> On 6.04.2023 14:07, Russell King (Oracle) wrote:
>>> On Thu, Apr 06, 2023 at 01:04:45PM +0300, arinc9.unal@gmail.com wrote:
>>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>
>>>> On the switch on the MT7988 SoC, there are only 4 PHYs. There's only port 6
>>>> as the CPU port, there's no port 5. Split the switch statement with a check
>>>> to enforce these for the switch on the MT7988 SoC. The internal phy-mode is
>>>> specific to MT7988 so put it for MT7988 only.
>>>>
>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>> ---
>>>>
>>>> Daniel, this is based on the information you provided me about the switch.
>>>> I will add this to my current patch series if it looks good to you.
>>>>
>>>> Arınç
>>>>
>>>> ---
>>>>    drivers/net/dsa/mt7530.c | 67 ++++++++++++++++++++++++++--------------
>>>>    1 file changed, 43 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>>> index 6fbbdcb5987f..f167fa135ef1 100644
>>>> --- a/drivers/net/dsa/mt7530.c
>>>> +++ b/drivers/net/dsa/mt7530.c
>>>> @@ -2548,7 +2548,7 @@ 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 */
>>>> +	case 0 ... 3: /* Internal phy */
>>>>    		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>>>>    			  config->supported_interfaces);
>>>>    		break;
>>>> @@ -2710,37 +2710,56 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>>>    	struct mt7530_priv *priv = ds->priv;
>>>>    	u32 mcr_cur, mcr_new;
>>>> -	switch (port) {
>>>> -	case 0 ... 4: /* Internal phy */
>>>> -		if (state->interface != PHY_INTERFACE_MODE_GMII &&
>>>> -		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
>>>> -			goto unsupported;
>>>> -		break;
>>>> -	case 5: /* Port 5, a CPU port. */
>>>> -		if (priv->p5_interface == state->interface)
>>>> +	if (priv->id == ID_MT7988) {
>>>> +		switch (port) {
>>>> +		case 0 ... 3: /* Internal phy */
>>>> +			if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
>>>
>>> How do these end up with PHY_INTERFACE_MODE_INTERNAL ? phylib defaults
>>> to GMII mode without something else being specified in DT.
>>>
>>> Also note that you should *not* be validating state->interface in the
>>> mac_config() method because it's way too late to reject it - if you get
>>> an unsupported interface here, then that is down to the get_caps()
>>> method being buggy. Only report interfaces in get_caps() that you are
>>> prepared to handle in the rest of the system.
>>
>> This is already the case for all three get_caps(). The supported interfaces
>> for each port are properly defined.
>>
>> Though mt7988_mac_port_get_caps() clears the config->supported_interfaces
>> bitmap before reporting the supported interfaces. I don't think this is
>> needed as all bits in the bitmap should already be initialized to zero when
>> the phylink_config structure is allocated.
>>
>> I'm not sure if your suggestion is to make sure the supported interfaces are
>> properly reported on get_caps(), or validate state->interface somewhere
>> else.
> 
> I think what Russell meant is just there is no point in being overly
> precise about permitted interface modes in mt753x_phylink_mac_config,
> as this function is not meant and called too late to validate the
> validity of the selected interface mode.
> 
> You change to mt7988_mac_port_get_caps looks correct to me and doing
> this will already prevent mt753x_phylink_mac_config from ever being
> called on MT7988 for port == 4 as well as and port == 5.

Ah, thanks for pointing this out Daniel. I see 
ds->ops->phylink_get_caps() is run right before phylink_create() on 
dsa_port_phylink_create(), as it should get the capabilities before 
creating an instance.

Should I remove phy_interface_zero(config->supported_interfaces); 
mt7988_mac_port_get_caps()? I'd prefer to do identical operations on 
each get_caps(), if there's no apparent reason for this to be on 
mt7988_mac_port_get_caps().

Arınç

Re: [RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[Daniel Golle]

On Fri, Apr 07, 2023 at 11:56:08AM +0300, Arınç ÜNAL wrote:
> On 7.04.2023 00:57, Daniel Golle wrote:
> > On Fri, Apr 07, 2023 at 12:43:41AM +0300, Arınç ÜNAL wrote:
> > > On 6.04.2023 14:07, Russell King (Oracle) wrote:
> > > > On Thu, Apr 06, 2023 at 01:04:45PM +0300, arinc9.unal@gmail.com wrote:
> > > > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > > 
> > > > > On the switch on the MT7988 SoC, there are only 4 PHYs. There's only port 6
> > > > > as the CPU port, there's no port 5. Split the switch statement with a check
> > > > > to enforce these for the switch on the MT7988 SoC. The internal phy-mode is
> > > > > specific to MT7988 so put it for MT7988 only.
> > > > > 
> > > > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > > ---
> > > > > 
> > > > > Daniel, this is based on the information you provided me about the switch.
> > > > > I will add this to my current patch series if it looks good to you.
> > > > > 
> > > > > Arınç
> > > > > 
> > > > > ---
> > > > >    drivers/net/dsa/mt7530.c | 67 ++++++++++++++++++++++++++--------------
> > > > >    1 file changed, 43 insertions(+), 24 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > > > index 6fbbdcb5987f..f167fa135ef1 100644
> > > > > --- a/drivers/net/dsa/mt7530.c
> > > > > +++ b/drivers/net/dsa/mt7530.c
> > > > > @@ -2548,7 +2548,7 @@ 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 */
> > > > > +	case 0 ... 3: /* Internal phy */
> > > > >    		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > > > >    			  config->supported_interfaces);
> > > > >    		break;
> > > > > @@ -2710,37 +2710,56 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > > > >    	struct mt7530_priv *priv = ds->priv;
> > > > >    	u32 mcr_cur, mcr_new;
> > > > > -	switch (port) {
> > > > > -	case 0 ... 4: /* Internal phy */
> > > > > -		if (state->interface != PHY_INTERFACE_MODE_GMII &&
> > > > > -		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > > > -			goto unsupported;
> > > > > -		break;
> > > > > -	case 5: /* Port 5, a CPU port. */
> > > > > -		if (priv->p5_interface == state->interface)
> > > > > +	if (priv->id == ID_MT7988) {
> > > > > +		switch (port) {
> > > > > +		case 0 ... 3: /* Internal phy */
> > > > > +			if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > > 
> > > > How do these end up with PHY_INTERFACE_MODE_INTERNAL ? phylib defaults
> > > > to GMII mode without something else being specified in DT.
> > > > 
> > > > Also note that you should *not* be validating state->interface in the
> > > > mac_config() method because it's way too late to reject it - if you get
> > > > an unsupported interface here, then that is down to the get_caps()
> > > > method being buggy. Only report interfaces in get_caps() that you are
> > > > prepared to handle in the rest of the system.
> > > 
> > > This is already the case for all three get_caps(). The supported interfaces
> > > for each port are properly defined.
> > > 
> > > Though mt7988_mac_port_get_caps() clears the config->supported_interfaces
> > > bitmap before reporting the supported interfaces. I don't think this is
> > > needed as all bits in the bitmap should already be initialized to zero when
> > > the phylink_config structure is allocated.
> > > 
> > > I'm not sure if your suggestion is to make sure the supported interfaces are
> > > properly reported on get_caps(), or validate state->interface somewhere
> > > else.
> > 
> > I think what Russell meant is just there is no point in being overly
> > precise about permitted interface modes in mt753x_phylink_mac_config,
> > as this function is not meant and called too late to validate the
> > validity of the selected interface mode.
> > 
> > You change to mt7988_mac_port_get_caps looks correct to me and doing
> > this will already prevent mt753x_phylink_mac_config from ever being
> > called on MT7988 for port == 4 as well as and port == 5.
> 
> Ah, thanks for pointing this out Daniel. I see ds->ops->phylink_get_caps()
> is run right before phylink_create() on dsa_port_phylink_create(), as it
> should get the capabilities before creating an instance.
> 
> Should I remove phy_interface_zero(config->supported_interfaces);
> mt7988_mac_port_get_caps()? I'd prefer to do identical operations on each
> get_caps(), if there's no apparent reason for this to be on
> mt7988_mac_port_get_caps().

Yes, sounds sane to me, please do so.

Also we could make .mac_port_config optional, as for MT7988 we actually
won't need it at all:

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index e4bb5037d3525..5efcb9897eb18 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2653,17 +2653,6 @@ static bool mt753x_is_mac_port(u32 port)
 	return (port == 5 || port == 6);
 }
 
-static int
-mt7988_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
-		  phy_interface_t interface)
-{
-	if (dsa_is_cpu_port(ds, port) &&
-	    interface == PHY_INTERFACE_MODE_INTERNAL)
-		return 0;
-
-	return -EINVAL;
-}
-
 static int
 mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
@@ -2704,6 +2693,9 @@ mt753x_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 {
 	struct mt7530_priv *priv = ds->priv;
 
+	if (!priv->info->mac_port_config)
+		return 0;
+
 	return priv->info->mac_port_config(ds, port, mode, state->interface);
 }
 
@@ -3157,7 +3149,6 @@ const struct mt753x_info mt753x_table[] = {
 		.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,
 	},
 };
 EXPORT_SYMBOL_GPL(mt753x_table);
@@ -3186,8 +3177,7 @@ mt7530_probe_common(struct mt7530_priv *priv)
 	 */
 	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 ||
-	    !priv->info->mac_port_config)
+	    !priv->info->mac_port_get_caps)
 		return -EINVAL;
 
 	priv->id = priv->info->id;

Re: [RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[Arınç ÜNAL]

On 7.04.2023 12:28, Daniel Golle wrote:
> On Fri, Apr 07, 2023 at 11:56:08AM +0300, Arınç ÜNAL wrote:
>> On 7.04.2023 00:57, Daniel Golle wrote:
>>> On Fri, Apr 07, 2023 at 12:43:41AM +0300, Arınç ÜNAL wrote:
>>>> On 6.04.2023 14:07, Russell King (Oracle) wrote:
>>>>> On Thu, Apr 06, 2023 at 01:04:45PM +0300, arinc9.unal@gmail.com wrote:
>>>>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>>>
>>>>>> On the switch on the MT7988 SoC, there are only 4 PHYs. There's only port 6
>>>>>> as the CPU port, there's no port 5. Split the switch statement with a check
>>>>>> to enforce these for the switch on the MT7988 SoC. The internal phy-mode is
>>>>>> specific to MT7988 so put it for MT7988 only.
>>>>>>
>>>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>>> ---
>>>>>>
>>>>>> Daniel, this is based on the information you provided me about the switch.
>>>>>> I will add this to my current patch series if it looks good to you.
>>>>>>
>>>>>> Arınç
>>>>>>
>>>>>> ---
>>>>>>     drivers/net/dsa/mt7530.c | 67 ++++++++++++++++++++++++++--------------
>>>>>>     1 file changed, 43 insertions(+), 24 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>>>>> index 6fbbdcb5987f..f167fa135ef1 100644
>>>>>> --- a/drivers/net/dsa/mt7530.c
>>>>>> +++ b/drivers/net/dsa/mt7530.c
>>>>>> @@ -2548,7 +2548,7 @@ 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 */
>>>>>> +	case 0 ... 3: /* Internal phy */
>>>>>>     		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>>>>>>     			  config->supported_interfaces);
>>>>>>     		break;
>>>>>> @@ -2710,37 +2710,56 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>>>>>     	struct mt7530_priv *priv = ds->priv;
>>>>>>     	u32 mcr_cur, mcr_new;
>>>>>> -	switch (port) {
>>>>>> -	case 0 ... 4: /* Internal phy */
>>>>>> -		if (state->interface != PHY_INTERFACE_MODE_GMII &&
>>>>>> -		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
>>>>>> -			goto unsupported;
>>>>>> -		break;
>>>>>> -	case 5: /* Port 5, a CPU port. */
>>>>>> -		if (priv->p5_interface == state->interface)
>>>>>> +	if (priv->id == ID_MT7988) {
>>>>>> +		switch (port) {
>>>>>> +		case 0 ... 3: /* Internal phy */
>>>>>> +			if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
>>>>>
>>>>> How do these end up with PHY_INTERFACE_MODE_INTERNAL ? phylib defaults
>>>>> to GMII mode without something else being specified in DT.
>>>>>
>>>>> Also note that you should *not* be validating state->interface in the
>>>>> mac_config() method because it's way too late to reject it - if you get
>>>>> an unsupported interface here, then that is down to the get_caps()
>>>>> method being buggy. Only report interfaces in get_caps() that you are
>>>>> prepared to handle in the rest of the system.
>>>>
>>>> This is already the case for all three get_caps(). The supported interfaces
>>>> for each port are properly defined.
>>>>
>>>> Though mt7988_mac_port_get_caps() clears the config->supported_interfaces
>>>> bitmap before reporting the supported interfaces. I don't think this is
>>>> needed as all bits in the bitmap should already be initialized to zero when
>>>> the phylink_config structure is allocated.
>>>>
>>>> I'm not sure if your suggestion is to make sure the supported interfaces are
>>>> properly reported on get_caps(), or validate state->interface somewhere
>>>> else.
>>>
>>> I think what Russell meant is just there is no point in being overly
>>> precise about permitted interface modes in mt753x_phylink_mac_config,
>>> as this function is not meant and called too late to validate the
>>> validity of the selected interface mode.
>>>
>>> You change to mt7988_mac_port_get_caps looks correct to me and doing
>>> this will already prevent mt753x_phylink_mac_config from ever being
>>> called on MT7988 for port == 4 as well as and port == 5.
>>
>> Ah, thanks for pointing this out Daniel. I see ds->ops->phylink_get_caps()
>> is run right before phylink_create() on dsa_port_phylink_create(), as it
>> should get the capabilities before creating an instance.
>>
>> Should I remove phy_interface_zero(config->supported_interfaces);
>> mt7988_mac_port_get_caps()? I'd prefer to do identical operations on each
>> get_caps(), if there's no apparent reason for this to be on
>> mt7988_mac_port_get_caps().
> 
> Yes, sounds sane to me, please do so.
> 
> Also we could make .mac_port_config optional, as for MT7988 we actually
> won't need it at all:
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index e4bb5037d3525..5efcb9897eb18 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2653,17 +2653,6 @@ static bool mt753x_is_mac_port(u32 port)
>   	return (port == 5 || port == 6);
>   }
>   
> -static int
> -mt7988_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> -		  phy_interface_t interface)
> -{
> -	if (dsa_is_cpu_port(ds, port) &&
> -	    interface == PHY_INTERFACE_MODE_INTERNAL)
> -		return 0;
> -
> -	return -EINVAL;
> -}
> -
>   static int
>   mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>   		  phy_interface_t interface)
> @@ -2704,6 +2693,9 @@ mt753x_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>   {
>   	struct mt7530_priv *priv = ds->priv;
>   
> +	if (!priv->info->mac_port_config)
> +		return 0;
> +
>   	return priv->info->mac_port_config(ds, port, mode, state->interface);
>   }
>   
> @@ -3157,7 +3149,6 @@ const struct mt753x_info mt753x_table[] = {
>   		.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,
>   	},
>   };
>   EXPORT_SYMBOL_GPL(mt753x_table);
> @@ -3186,8 +3177,7 @@ mt7530_probe_common(struct mt7530_priv *priv)
>   	 */
>   	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 ||
> -	    !priv->info->mac_port_config)
> +	    !priv->info->mac_port_get_caps)

Why split the sanity check? Isn't just removing mt7988_mac_config() and 
.mac_port_config = mt7988_mac_config enough?

Arınç

Re: [RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[Arınç ÜNAL]

On 7.04.2023 13:46, Arınç ÜNAL wrote:
> On 7.04.2023 12:28, Daniel Golle wrote:
>> On Fri, Apr 07, 2023 at 11:56:08AM +0300, Arınç ÜNAL wrote:
>>> On 7.04.2023 00:57, Daniel Golle wrote:
>>>> On Fri, Apr 07, 2023 at 12:43:41AM +0300, Arınç ÜNAL wrote:
>>>>> On 6.04.2023 14:07, Russell King (Oracle) wrote:
>>>>>> On Thu, Apr 06, 2023 at 01:04:45PM +0300, arinc9.unal@gmail.com 
>>>>>> wrote:
>>>>>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>>>>
>>>>>>> On the switch on the MT7988 SoC, there are only 4 PHYs. There's 
>>>>>>> only port 6
>>>>>>> as the CPU port, there's no port 5. Split the switch statement 
>>>>>>> with a check
>>>>>>> to enforce these for the switch on the MT7988 SoC. The internal 
>>>>>>> phy-mode is
>>>>>>> specific to MT7988 so put it for MT7988 only.
>>>>>>>
>>>>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Daniel, this is based on the information you provided me about 
>>>>>>> the switch.
>>>>>>> I will add this to my current patch series if it looks good to you.
>>>>>>>
>>>>>>> Arınç
>>>>>>>
>>>>>>> ---
>>>>>>>     drivers/net/dsa/mt7530.c | 67 
>>>>>>> ++++++++++++++++++++++++++--------------
>>>>>>>     1 file changed, 43 insertions(+), 24 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>>>>>> index 6fbbdcb5987f..f167fa135ef1 100644
>>>>>>> --- a/drivers/net/dsa/mt7530.c
>>>>>>> +++ b/drivers/net/dsa/mt7530.c
>>>>>>> @@ -2548,7 +2548,7 @@ 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 */
>>>>>>> +    case 0 ... 3: /* Internal phy */
>>>>>>>             __set_bit(PHY_INTERFACE_MODE_INTERNAL,
>>>>>>>                   config->supported_interfaces);
>>>>>>>             break;
>>>>>>> @@ -2710,37 +2710,56 @@ mt753x_phylink_mac_config(struct 
>>>>>>> dsa_switch *ds, int port, unsigned int mode,
>>>>>>>         struct mt7530_priv *priv = ds->priv;
>>>>>>>         u32 mcr_cur, mcr_new;
>>>>>>> -    switch (port) {
>>>>>>> -    case 0 ... 4: /* Internal phy */
>>>>>>> -        if (state->interface != PHY_INTERFACE_MODE_GMII &&
>>>>>>> -            state->interface != PHY_INTERFACE_MODE_INTERNAL)
>>>>>>> -            goto unsupported;
>>>>>>> -        break;
>>>>>>> -    case 5: /* Port 5, a CPU port. */
>>>>>>> -        if (priv->p5_interface == state->interface)
>>>>>>> +    if (priv->id == ID_MT7988) {
>>>>>>> +        switch (port) {
>>>>>>> +        case 0 ... 3: /* Internal phy */
>>>>>>> +            if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
>>>>>>
>>>>>> How do these end up with PHY_INTERFACE_MODE_INTERNAL ? phylib 
>>>>>> defaults
>>>>>> to GMII mode without something else being specified in DT.
>>>>>>
>>>>>> Also note that you should *not* be validating state->interface in the
>>>>>> mac_config() method because it's way too late to reject it - if 
>>>>>> you get
>>>>>> an unsupported interface here, then that is down to the get_caps()
>>>>>> method being buggy. Only report interfaces in get_caps() that you are
>>>>>> prepared to handle in the rest of the system.
>>>>>
>>>>> This is already the case for all three get_caps(). The supported 
>>>>> interfaces
>>>>> for each port are properly defined.
>>>>>
>>>>> Though mt7988_mac_port_get_caps() clears the 
>>>>> config->supported_interfaces
>>>>> bitmap before reporting the supported interfaces. I don't think 
>>>>> this is
>>>>> needed as all bits in the bitmap should already be initialized to 
>>>>> zero when
>>>>> the phylink_config structure is allocated.
>>>>>
>>>>> I'm not sure if your suggestion is to make sure the supported 
>>>>> interfaces are
>>>>> properly reported on get_caps(), or validate state->interface 
>>>>> somewhere
>>>>> else.
>>>>
>>>> I think what Russell meant is just there is no point in being overly
>>>> precise about permitted interface modes in mt753x_phylink_mac_config,
>>>> as this function is not meant and called too late to validate the
>>>> validity of the selected interface mode.
>>>>
>>>> You change to mt7988_mac_port_get_caps looks correct to me and doing
>>>> this will already prevent mt753x_phylink_mac_config from ever being
>>>> called on MT7988 for port == 4 as well as and port == 5.
>>>
>>> Ah, thanks for pointing this out Daniel. I see 
>>> ds->ops->phylink_get_caps()
>>> is run right before phylink_create() on dsa_port_phylink_create(), as it
>>> should get the capabilities before creating an instance.
>>>
>>> Should I remove phy_interface_zero(config->supported_interfaces);
>>> mt7988_mac_port_get_caps()? I'd prefer to do identical operations on 
>>> each
>>> get_caps(), if there's no apparent reason for this to be on
>>> mt7988_mac_port_get_caps().
>>
>> Yes, sounds sane to me, please do so.
>>
>> Also we could make .mac_port_config optional, as for MT7988 we actually
>> won't need it at all:
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index e4bb5037d3525..5efcb9897eb18 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2653,17 +2653,6 @@ static bool mt753x_is_mac_port(u32 port)
>>       return (port == 5 || port == 6);
>>   }
>> -static int
>> -mt7988_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>> -          phy_interface_t interface)
>> -{
>> -    if (dsa_is_cpu_port(ds, port) &&
>> -        interface == PHY_INTERFACE_MODE_INTERNAL)
>> -        return 0;
>> -
>> -    return -EINVAL;
>> -}
>> -
>>   static int
>>   mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>             phy_interface_t interface)
>> @@ -2704,6 +2693,9 @@ mt753x_mac_config(struct dsa_switch *ds, int 
>> port, unsigned int mode,
>>   {
>>       struct mt7530_priv *priv = ds->priv;
>> +    if (!priv->info->mac_port_config)
>> +        return 0;
>> +
>>       return priv->info->mac_port_config(ds, port, mode, 
>> state->interface);
>>   }
>> @@ -3157,7 +3149,6 @@ const struct mt753x_info mt753x_table[] = {
>>           .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,
>>       },
>>   };
>>   EXPORT_SYMBOL_GPL(mt753x_table);
>> @@ -3186,8 +3177,7 @@ mt7530_probe_common(struct mt7530_priv *priv)
>>        */
>>       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 ||
>> -        !priv->info->mac_port_config)
>> +        !priv->info->mac_port_get_caps)
> 
> Why split the sanity check? Isn't just removing mt7988_mac_config() and 
> .mac_port_config = mt7988_mac_config enough?

Nevermind, it is necessary. I confused the return logic. This looks good 
to me. Should I take this to my current series? It will conflict with 
sanity check changes as I also remove pad_setup from there.

Arınç

Re: [RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[Daniel Golle]

On Fri, Apr 07, 2023 at 01:50:54PM +0300, Arınç ÜNAL wrote:
> On 7.04.2023 13:46, Arınç ÜNAL wrote:
> > On 7.04.2023 12:28, Daniel Golle wrote:
> > > On Fri, Apr 07, 2023 at 11:56:08AM +0300, Arınç ÜNAL wrote:
> > > > On 7.04.2023 00:57, Daniel Golle wrote:
> > > > > On Fri, Apr 07, 2023 at 12:43:41AM +0300, Arınç ÜNAL wrote:
> > > > > > On 6.04.2023 14:07, Russell King (Oracle) wrote:
> > > > > > > On Thu, Apr 06, 2023 at 01:04:45PM +0300,
> > > > > > > arinc9.unal@gmail.com wrote:
> > > > > > > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > > > > > 
> > > > > > > > On the switch on the MT7988 SoC, there are only
> > > > > > > > 4 PHYs. There's only port 6
> > > > > > > > as the CPU port, there's no port 5. Split the
> > > > > > > > switch statement with a check
> > > > > > > > to enforce these for the switch on the MT7988
> > > > > > > > SoC. The internal phy-mode is
> > > > > > > > specific to MT7988 so put it for MT7988 only.
> > > > > > > > 
> > > > > > > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > Daniel, this is based on the information you
> > > > > > > > provided me about the switch.
> > > > > > > > I will add this to my current patch series if it looks good to you.
> > > > > > > > 
> > > > > > > > Arınç
> > > > > > > > 
> > > > > > > > ---
> > > > > > > >     drivers/net/dsa/mt7530.c | 67
> > > > > > > > ++++++++++++++++++++++++++--------------
> > > > > > > >     1 file changed, 43 insertions(+), 24 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > > > > > > index 6fbbdcb5987f..f167fa135ef1 100644
> > > > > > > > --- a/drivers/net/dsa/mt7530.c
> > > > > > > > +++ b/drivers/net/dsa/mt7530.c
> > > > > > > > @@ -2548,7 +2548,7 @@ 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 */
> > > > > > > > +    case 0 ... 3: /* Internal phy */
> > > > > > > >             __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > > > > > > >                   config->supported_interfaces);
> > > > > > > >             break;
> > > > > > > > @@ -2710,37 +2710,56 @@
> > > > > > > > mt753x_phylink_mac_config(struct dsa_switch *ds,
> > > > > > > > int port, unsigned int mode,
> > > > > > > >         struct mt7530_priv *priv = ds->priv;
> > > > > > > >         u32 mcr_cur, mcr_new;
> > > > > > > > -    switch (port) {
> > > > > > > > -    case 0 ... 4: /* Internal phy */
> > > > > > > > -        if (state->interface != PHY_INTERFACE_MODE_GMII &&
> > > > > > > > -            state->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > > > > > > -            goto unsupported;
> > > > > > > > -        break;
> > > > > > > > -    case 5: /* Port 5, a CPU port. */
> > > > > > > > -        if (priv->p5_interface == state->interface)
> > > > > > > > +    if (priv->id == ID_MT7988) {
> > > > > > > > +        switch (port) {
> > > > > > > > +        case 0 ... 3: /* Internal phy */
> > > > > > > > +            if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > > > > > 
> > > > > > > How do these end up with PHY_INTERFACE_MODE_INTERNAL
> > > > > > > ? phylib defaults
> > > > > > > to GMII mode without something else being specified in DT.
> > > > > > > 
> > > > > > > Also note that you should *not* be validating state->interface in the
> > > > > > > mac_config() method because it's way too late to
> > > > > > > reject it - if you get
> > > > > > > an unsupported interface here, then that is down to the get_caps()
> > > > > > > method being buggy. Only report interfaces in get_caps() that you are
> > > > > > > prepared to handle in the rest of the system.
> > > > > > 
> > > > > > This is already the case for all three get_caps(). The
> > > > > > supported interfaces
> > > > > > for each port are properly defined.
> > > > > > 
> > > > > > Though mt7988_mac_port_get_caps() clears the
> > > > > > config->supported_interfaces
> > > > > > bitmap before reporting the supported interfaces. I
> > > > > > don't think this is
> > > > > > needed as all bits in the bitmap should already be
> > > > > > initialized to zero when
> > > > > > the phylink_config structure is allocated.
> > > > > > 
> > > > > > I'm not sure if your suggestion is to make sure the
> > > > > > supported interfaces are
> > > > > > properly reported on get_caps(), or validate
> > > > > > state->interface somewhere
> > > > > > else.
> > > > > 
> > > > > I think what Russell meant is just there is no point in being overly
> > > > > precise about permitted interface modes in mt753x_phylink_mac_config,
> > > > > as this function is not meant and called too late to validate the
> > > > > validity of the selected interface mode.
> > > > > 
> > > > > You change to mt7988_mac_port_get_caps looks correct to me and doing
> > > > > this will already prevent mt753x_phylink_mac_config from ever being
> > > > > called on MT7988 for port == 4 as well as and port == 5.
> > > > 
> > > > Ah, thanks for pointing this out Daniel. I see
> > > > ds->ops->phylink_get_caps()
> > > > is run right before phylink_create() on dsa_port_phylink_create(), as it
> > > > should get the capabilities before creating an instance.
> > > > 
> > > > Should I remove phy_interface_zero(config->supported_interfaces);
> > > > mt7988_mac_port_get_caps()? I'd prefer to do identical
> > > > operations on each
> > > > get_caps(), if there's no apparent reason for this to be on
> > > > mt7988_mac_port_get_caps().
> > > 
> > > Yes, sounds sane to me, please do so.
> > > 
> > > Also we could make .mac_port_config optional, as for MT7988 we actually
> > > won't need it at all:
> > > 
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index e4bb5037d3525..5efcb9897eb18 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -2653,17 +2653,6 @@ static bool mt753x_is_mac_port(u32 port)
> > >       return (port == 5 || port == 6);
> > >   }
> > > -static int
> > > -mt7988_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > > -          phy_interface_t interface)
> > > -{
> > > -    if (dsa_is_cpu_port(ds, port) &&
> > > -        interface == PHY_INTERFACE_MODE_INTERNAL)
> > > -        return 0;
> > > -
> > > -    return -EINVAL;
> > > -}
> > > -
> > >   static int
> > >   mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > >             phy_interface_t interface)
> > > @@ -2704,6 +2693,9 @@ mt753x_mac_config(struct dsa_switch *ds, int
> > > port, unsigned int mode,
> > >   {
> > >       struct mt7530_priv *priv = ds->priv;
> > > +    if (!priv->info->mac_port_config)
> > > +        return 0;
> > > +
> > >       return priv->info->mac_port_config(ds, port, mode,
> > > state->interface);
> > >   }
> > > @@ -3157,7 +3149,6 @@ const struct mt753x_info mt753x_table[] = {
> > >           .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,
> > >       },
> > >   };
> > >   EXPORT_SYMBOL_GPL(mt753x_table);
> > > @@ -3186,8 +3177,7 @@ mt7530_probe_common(struct mt7530_priv *priv)
> > >        */
> > >       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 ||
> > > -        !priv->info->mac_port_config)
> > > +        !priv->info->mac_port_get_caps)
> > 
> > Why split the sanity check? Isn't just removing mt7988_mac_config() and
> > .mac_port_config = mt7988_mac_config enough?
> 
> Nevermind, it is necessary. I confused the return logic. This looks good to
> me. Should I take this to my current series? It will conflict with sanity
> check changes as I also remove pad_setup from there.

Yes please do so, I will review and test the whole series all together then
later today.

Thank you!


Daniel

Re: [RFC PATCH net-next] net: dsa: mt7530: fix port specifications for MT7988

[Daniel Golle]

On Fri, Apr 07, 2023 at 01:46:20PM +0300, Arınç ÜNAL wrote:
> On 7.04.2023 12:28, Daniel Golle wrote:
> > On Fri, Apr 07, 2023 at 11:56:08AM +0300, Arınç ÜNAL wrote:
> > > On 7.04.2023 00:57, Daniel Golle wrote:
> > > > On Fri, Apr 07, 2023 at 12:43:41AM +0300, Arınç ÜNAL wrote:
> > > > > On 6.04.2023 14:07, Russell King (Oracle) wrote:
> > > > > > On Thu, Apr 06, 2023 at 01:04:45PM +0300, arinc9.unal@gmail.com wrote:
> > > > > > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > > > > 
> > > > > > > On the switch on the MT7988 SoC, there are only 4 PHYs. There's only port 6
> > > > > > > as the CPU port, there's no port 5. Split the switch statement with a check
> > > > > > > to enforce these for the switch on the MT7988 SoC. The internal phy-mode is
> > > > > > > specific to MT7988 so put it for MT7988 only.
> > > > > > > 
> > > > > > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > Daniel, this is based on the information you provided me about the switch.
> > > > > > > I will add this to my current patch series if it looks good to you.
> > > > > > > 
> > > > > > > Arınç
> > > > > > > 
> > > > > > > ---
> > > > > > >     drivers/net/dsa/mt7530.c | 67 ++++++++++++++++++++++++++--------------
> > > > > > >     1 file changed, 43 insertions(+), 24 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > > > > > index 6fbbdcb5987f..f167fa135ef1 100644
> > > > > > > --- a/drivers/net/dsa/mt7530.c
> > > > > > > +++ b/drivers/net/dsa/mt7530.c
> > > > > > > @@ -2548,7 +2548,7 @@ 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 */
> > > > > > > +	case 0 ... 3: /* Internal phy */
> > > > > > >     		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > > > > > >     			  config->supported_interfaces);
> > > > > > >     		break;
> > > > > > > @@ -2710,37 +2710,56 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > > > > > >     	struct mt7530_priv *priv = ds->priv;
> > > > > > >     	u32 mcr_cur, mcr_new;
> > > > > > > -	switch (port) {
> > > > > > > -	case 0 ... 4: /* Internal phy */
> > > > > > > -		if (state->interface != PHY_INTERFACE_MODE_GMII &&
> > > > > > > -		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > > > > > -			goto unsupported;
> > > > > > > -		break;
> > > > > > > -	case 5: /* Port 5, a CPU port. */
> > > > > > > -		if (priv->p5_interface == state->interface)
> > > > > > > +	if (priv->id == ID_MT7988) {
> > > > > > > +		switch (port) {
> > > > > > > +		case 0 ... 3: /* Internal phy */
> > > > > > > +			if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
> > > > > > 
> > > > > > How do these end up with PHY_INTERFACE_MODE_INTERNAL ? phylib defaults
> > > > > > to GMII mode without something else being specified in DT.
> > > > > > 
> > > > > > Also note that you should *not* be validating state->interface in the
> > > > > > mac_config() method because it's way too late to reject it - if you get
> > > > > > an unsupported interface here, then that is down to the get_caps()
> > > > > > method being buggy. Only report interfaces in get_caps() that you are
> > > > > > prepared to handle in the rest of the system.
> > > > > 
> > > > > This is already the case for all three get_caps(). The supported interfaces
> > > > > for each port are properly defined.
> > > > > 
> > > > > Though mt7988_mac_port_get_caps() clears the config->supported_interfaces
> > > > > bitmap before reporting the supported interfaces. I don't think this is
> > > > > needed as all bits in the bitmap should already be initialized to zero when
> > > > > the phylink_config structure is allocated.
> > > > > 
> > > > > I'm not sure if your suggestion is to make sure the supported interfaces are
> > > > > properly reported on get_caps(), or validate state->interface somewhere
> > > > > else.
> > > > 
> > > > I think what Russell meant is just there is no point in being overly
> > > > precise about permitted interface modes in mt753x_phylink_mac_config,
> > > > as this function is not meant and called too late to validate the
> > > > validity of the selected interface mode.
> > > > 
> > > > You change to mt7988_mac_port_get_caps looks correct to me and doing
> > > > this will already prevent mt753x_phylink_mac_config from ever being
> > > > called on MT7988 for port == 4 as well as and port == 5.
> > > 
> > > Ah, thanks for pointing this out Daniel. I see ds->ops->phylink_get_caps()
> > > is run right before phylink_create() on dsa_port_phylink_create(), as it
> > > should get the capabilities before creating an instance.
> > > 
> > > Should I remove phy_interface_zero(config->supported_interfaces);
> > > mt7988_mac_port_get_caps()? I'd prefer to do identical operations on each
> > > get_caps(), if there's no apparent reason for this to be on
> > > mt7988_mac_port_get_caps().
> > 
> > Yes, sounds sane to me, please do so.
> > 
> > Also we could make .mac_port_config optional, as for MT7988 we actually
> > won't need it at all:
> > 
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index e4bb5037d3525..5efcb9897eb18 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -2653,17 +2653,6 @@ static bool mt753x_is_mac_port(u32 port)
> >   	return (port == 5 || port == 6);
> >   }
> > -static int
> > -mt7988_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > -		  phy_interface_t interface)
> > -{
> > -	if (dsa_is_cpu_port(ds, port) &&
> > -	    interface == PHY_INTERFACE_MODE_INTERNAL)
> > -		return 0;
> > -
> > -	return -EINVAL;
> > -}
> > -
> >   static int
> >   mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >   		  phy_interface_t interface)
> > @@ -2704,6 +2693,9 @@ mt753x_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >   {
> >   	struct mt7530_priv *priv = ds->priv;
> > +	if (!priv->info->mac_port_config)
> > +		return 0;
> > +
> >   	return priv->info->mac_port_config(ds, port, mode, state->interface);
> >   }
> > @@ -3157,7 +3149,6 @@ const struct mt753x_info mt753x_table[] = {
> >   		.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,
> >   	},
> >   };
> >   EXPORT_SYMBOL_GPL(mt753x_table);
> > @@ -3186,8 +3177,7 @@ mt7530_probe_common(struct mt7530_priv *priv)
> >   	 */
> >   	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 ||
> > -	    !priv->info->mac_port_config)
> > +	    !priv->info->mac_port_get_caps)
> 
> Why split the sanity check? Isn't just removing mt7988_mac_config() and
> .mac_port_config = mt7988_mac_config enough?

No, because the sanity check currently requires a pointer to a
mac_port_config function to be non-NULL. If we want to make it optional
then we'll also have to skip that in the santity check which will
otherwise fail.