From: Arınç ÜNAL <[email protected]>
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 <[email protected]>
---
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
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ç
On Thu, Apr 06, 2023 at 01:04:45PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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!
On Thu, Apr 06, 2023 at 12:07:01PM +0100, Russell King (Oracle) wrote:
> On Thu, Apr 06, 2023 at 01:04:45PM +0300, [email protected] wrote:
> > From: Arınç ÜNAL <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> >
> > 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!
On 6.04.2023 14:07, Russell King (Oracle) wrote:
> On Thu, Apr 06, 2023 at 01:04:45PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>>
>> 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ç
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, [email protected] wrote:
> > > From: Arınç ÜNAL <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > >
> > > 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.
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, [email protected] wrote:
>>>> From: Arınç ÜNAL <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>>
>>>> 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ç
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, [email protected] wrote:
> > > > > From: Arınç ÜNAL <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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;
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, [email protected] wrote:
>>>>>> From: Arınç ÜNAL <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>> ---
>>>>>>
>>>>>> 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ç
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, [email protected]
>>>>>> wrote:
>>>>>>> From: Arınç ÜNAL <[email protected]>
>>>>>>>
>>>>>>> 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 <[email protected]>
>>>>>>> ---
>>>>>>>
>>>>>>> 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ç
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, [email protected] wrote:
> > > > > > > From: Arınç ÜNAL <[email protected]>
> > > > > > >
> > > > > > > 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 <[email protected]>
> > > > > > > ---
> > > > > > >
> > > > > > > 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.
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,
> > > > > > > [email protected] wrote:
> > > > > > > > From: Arınç ÜNAL <[email protected]>
> > > > > > > >
> > > > > > > > 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 <[email protected]>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > 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