diff mbox series

[11/14] drivers: net: dsa: qca8k: apply switch revision fix

Message ID 20210423014741.11858-12-ansuelsmth@gmail.com
State New
Headers show
Series Multiple improvement to qca8k stability | expand

Commit Message

Christian Marangi April 23, 2021, 1:47 a.m. UTC
qca8k require special debug value based on the switch revision.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Comments

Christian Marangi April 24, 2021, 9:18 p.m. UTC | #1
On Thu, Apr 22, 2021 at 07:02:37PM -0700, Florian Fainelli wrote:
> 

> 

> On 4/22/2021 6:47 PM, Ansuel Smith wrote:

> > qca8k require special debug value based on the switch revision.

> > 

> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

> > ---

> >  drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--

> >  1 file changed, 21 insertions(+), 2 deletions(-)

> > 

> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c

> > index 193c269d8ed3..12d2c97d1417 100644

> > --- a/drivers/net/dsa/qca8k.c

> > +++ b/drivers/net/dsa/qca8k.c

> > @@ -909,7 +909,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,

> >  {

> >  	const struct qca8k_match_data *data;

> >  	struct qca8k_priv *priv = ds->priv;

> > -	u32 reg, val;

> > +	u32 phy, reg, val;

> >  

> >  	/* get the switches ID from the compatible */

> >  	data = of_device_get_match_data(priv->dev);

> > @@ -928,7 +928,26 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,

> >  	case 3:

> >  	case 4:

> >  	case 5:

> > -		/* Internal PHY, nothing to do */

> > +		/* Internal PHY, apply revision fixup */

> > +		phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;

> > +		switch (priv->switch_revision) {

> > +		case 1:

> > +			/* For 100M waveform */

> > +			qca8k_phy_dbg_write(priv, phy, 0, 0x02ea);

> > +			/* Turn on Gigabit clock */

> > +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x68a0);

> > +			break;

> > +

> > +		case 2:

> > +			qca8k_phy_mmd_write(priv, phy, 0x7, 0x3c, 0x0);

> > +			fallthrough;

> > +		case 4:

> > +			qca8k_phy_mmd_write(priv, phy, 0x3, 0x800d, 0x803f);

> > +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x6860);

> > +			qca8k_phy_dbg_write(priv, phy, 0x5, 0x2c46);

> > +			qca8k_phy_dbg_write(priv, phy, 0x3c, 0x6000);

> > +			break;

> 

> This would be better done with a PHY driver that is specific to the

> integrated PHY found in these switches, it would provide a nice clean

> layer and would allow you to expose additional features like cable

> tests, PHY statistics/counters, etc.


I'm starting to do some work with this and a problem arised. Since these
value are based on the switch revision, how can I access these kind of
data from the phy driver? It's allowed to declare a phy driver in the
dsa directory? (The idea would be to create a qca8k dir with the dsa
driver and the dedicated internal phy driver.) This would facilitate the
use of normal qca8k_read/write (to access the switch revision from the
phy driver) using common function?

> -- 

> Florian
Heiner Kallweit April 24, 2021, 9:49 p.m. UTC | #2
On 24.04.2021 23:18, Ansuel Smith wrote:
> On Thu, Apr 22, 2021 at 07:02:37PM -0700, Florian Fainelli wrote:

>>

>>

>> On 4/22/2021 6:47 PM, Ansuel Smith wrote:

>>> qca8k require special debug value based on the switch revision.

>>>

>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

>>> ---

>>>  drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--

>>>  1 file changed, 21 insertions(+), 2 deletions(-)

>>>

>>> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c

>>> index 193c269d8ed3..12d2c97d1417 100644

>>> --- a/drivers/net/dsa/qca8k.c

>>> +++ b/drivers/net/dsa/qca8k.c

>>> @@ -909,7 +909,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,

>>>  {

>>>  	const struct qca8k_match_data *data;

>>>  	struct qca8k_priv *priv = ds->priv;

>>> -	u32 reg, val;

>>> +	u32 phy, reg, val;

>>>  

>>>  	/* get the switches ID from the compatible */

>>>  	data = of_device_get_match_data(priv->dev);

>>> @@ -928,7 +928,26 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,

>>>  	case 3:

>>>  	case 4:

>>>  	case 5:

>>> -		/* Internal PHY, nothing to do */

>>> +		/* Internal PHY, apply revision fixup */

>>> +		phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;

>>> +		switch (priv->switch_revision) {

>>> +		case 1:

>>> +			/* For 100M waveform */

>>> +			qca8k_phy_dbg_write(priv, phy, 0, 0x02ea);

>>> +			/* Turn on Gigabit clock */

>>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x68a0);

>>> +			break;

>>> +

>>> +		case 2:

>>> +			qca8k_phy_mmd_write(priv, phy, 0x7, 0x3c, 0x0);


Please replace the magic numbers with constants. Here even standard
registers are used: 0x7 = MDIO_MMD_AN, 0x3c = MDIO_AN_EEE_ADV
Effectively EEE advertisement is disabled.

>>> +			fallthrough;

>>> +		case 4:

>>> +			qca8k_phy_mmd_write(priv, phy, 0x3, 0x800d, 0x803f);

>>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x6860);

>>> +			qca8k_phy_dbg_write(priv, phy, 0x5, 0x2c46);

>>> +			qca8k_phy_dbg_write(priv, phy, 0x3c, 0x6000);

>>> +			break;

>>

>> This would be better done with a PHY driver that is specific to the

>> integrated PHY found in these switches, it would provide a nice clean

>> layer and would allow you to expose additional features like cable

>> tests, PHY statistics/counters, etc.

> 

> I'm starting to do some work with this and a problem arised. Since these

> value are based on the switch revision, how can I access these kind of

> data from the phy driver? It's allowed to declare a phy driver in the

> dsa directory? (The idea would be to create a qca8k dir with the dsa

> driver and the dedicated internal phy driver.) This would facilitate the

> use of normal qca8k_read/write (to access the switch revision from the

> phy driver) using common function?

> 


PHY drivers reside under drivers/net/phy. Not sure whether your internal
PHY's have a proper PHY ID, you could assign pseudo PHY ID's differing
per switch revision. See mv88e6xxx_mdio_read() for a similar use case.

>> -- 

>> Florian


Heiner
Florian Fainelli April 25, 2021, 1:09 a.m. UTC | #3
On 4/24/2021 2:18 PM, Ansuel Smith wrote:
> On Thu, Apr 22, 2021 at 07:02:37PM -0700, Florian Fainelli wrote:

>>

>>

>> On 4/22/2021 6:47 PM, Ansuel Smith wrote:

>>> qca8k require special debug value based on the switch revision.

>>>

>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

>>> ---

>>>  drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--

>>>  1 file changed, 21 insertions(+), 2 deletions(-)

>>>

>>> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c

>>> index 193c269d8ed3..12d2c97d1417 100644

>>> --- a/drivers/net/dsa/qca8k.c

>>> +++ b/drivers/net/dsa/qca8k.c

>>> @@ -909,7 +909,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,

>>>  {

>>>  	const struct qca8k_match_data *data;

>>>  	struct qca8k_priv *priv = ds->priv;

>>> -	u32 reg, val;

>>> +	u32 phy, reg, val;

>>>  

>>>  	/* get the switches ID from the compatible */

>>>  	data = of_device_get_match_data(priv->dev);

>>> @@ -928,7 +928,26 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,

>>>  	case 3:

>>>  	case 4:

>>>  	case 5:

>>> -		/* Internal PHY, nothing to do */

>>> +		/* Internal PHY, apply revision fixup */

>>> +		phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;

>>> +		switch (priv->switch_revision) {

>>> +		case 1:

>>> +			/* For 100M waveform */

>>> +			qca8k_phy_dbg_write(priv, phy, 0, 0x02ea);

>>> +			/* Turn on Gigabit clock */

>>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x68a0);

>>> +			break;

>>> +

>>> +		case 2:

>>> +			qca8k_phy_mmd_write(priv, phy, 0x7, 0x3c, 0x0);

>>> +			fallthrough;

>>> +		case 4:

>>> +			qca8k_phy_mmd_write(priv, phy, 0x3, 0x800d, 0x803f);

>>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x6860);

>>> +			qca8k_phy_dbg_write(priv, phy, 0x5, 0x2c46);

>>> +			qca8k_phy_dbg_write(priv, phy, 0x3c, 0x6000);

>>> +			break;

>>

>> This would be better done with a PHY driver that is specific to the

>> integrated PHY found in these switches, it would provide a nice clean

>> layer and would allow you to expose additional features like cable

>> tests, PHY statistics/counters, etc.

> 

> I'm starting to do some work with this and a problem arised. Since these

> value are based on the switch revision, how can I access these kind of

> data from the phy driver? It's allowed to declare a phy driver in the

> dsa directory? (The idea would be to create a qca8k dir with the dsa

> driver and the dedicated internal phy driver.) This would facilitate the

> use of normal qca8k_read/write (to access the switch revision from the

> phy driver) using common function?


The PHY driver should live under drivers/net/phy/ and if you need to
communicate the switch revision to the PHY driver you can use
phydev->dev_flags and implement a dsa_switch_ops::get_phy_flags()
callback and define a custom bitmask.

As far as the read/write operations if your switch implements a custom
mii_bus for the purpose of doing all of the underlying indirect register
accesses, then you should be fine. A lot of drivers do that however if
you want an example of both (communicating something to the PHY driver
and having a custom MII bus) you can look at drivers/net/dsa/bcm_sf2.c
for an example.
-- 
Florian
Christian Marangi April 25, 2021, 1:19 a.m. UTC | #4
On Sat, Apr 24, 2021 at 06:09:27PM -0700, Florian Fainelli wrote:
> 

> 

> On 4/24/2021 2:18 PM, Ansuel Smith wrote:

> > On Thu, Apr 22, 2021 at 07:02:37PM -0700, Florian Fainelli wrote:

> >>

> >>

> >> On 4/22/2021 6:47 PM, Ansuel Smith wrote:

> >>> qca8k require special debug value based on the switch revision.

> >>>

> >>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

> >>> ---

> >>>  drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--

> >>>  1 file changed, 21 insertions(+), 2 deletions(-)

> >>>

> >>> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c

> >>> index 193c269d8ed3..12d2c97d1417 100644

> >>> --- a/drivers/net/dsa/qca8k.c

> >>> +++ b/drivers/net/dsa/qca8k.c

> >>> @@ -909,7 +909,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,

> >>>  {

> >>>  	const struct qca8k_match_data *data;

> >>>  	struct qca8k_priv *priv = ds->priv;

> >>> -	u32 reg, val;

> >>> +	u32 phy, reg, val;

> >>>  

> >>>  	/* get the switches ID from the compatible */

> >>>  	data = of_device_get_match_data(priv->dev);

> >>> @@ -928,7 +928,26 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,

> >>>  	case 3:

> >>>  	case 4:

> >>>  	case 5:

> >>> -		/* Internal PHY, nothing to do */

> >>> +		/* Internal PHY, apply revision fixup */

> >>> +		phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;

> >>> +		switch (priv->switch_revision) {

> >>> +		case 1:

> >>> +			/* For 100M waveform */

> >>> +			qca8k_phy_dbg_write(priv, phy, 0, 0x02ea);

> >>> +			/* Turn on Gigabit clock */

> >>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x68a0);

> >>> +			break;

> >>> +

> >>> +		case 2:

> >>> +			qca8k_phy_mmd_write(priv, phy, 0x7, 0x3c, 0x0);

> >>> +			fallthrough;

> >>> +		case 4:

> >>> +			qca8k_phy_mmd_write(priv, phy, 0x3, 0x800d, 0x803f);

> >>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x6860);

> >>> +			qca8k_phy_dbg_write(priv, phy, 0x5, 0x2c46);

> >>> +			qca8k_phy_dbg_write(priv, phy, 0x3c, 0x6000);

> >>> +			break;

> >>

> >> This would be better done with a PHY driver that is specific to the

> >> integrated PHY found in these switches, it would provide a nice clean

> >> layer and would allow you to expose additional features like cable

> >> tests, PHY statistics/counters, etc.

> > 

> > I'm starting to do some work with this and a problem arised. Since these

> > value are based on the switch revision, how can I access these kind of

> > data from the phy driver? It's allowed to declare a phy driver in the

> > dsa directory? (The idea would be to create a qca8k dir with the dsa

> > driver and the dedicated internal phy driver.) This would facilitate the

> > use of normal qca8k_read/write (to access the switch revision from the

> > phy driver) using common function?

> 

> The PHY driver should live under drivers/net/phy/ and if you need to

> communicate the switch revision to the PHY driver you can use

> phydev->dev_flags and implement a dsa_switch_ops::get_phy_flags()

> callback and define a custom bitmask.

> 

> As far as the read/write operations if your switch implements a custom

> mii_bus for the purpose of doing all of the underlying indirect register

> accesses, then you should be fine. A lot of drivers do that however if

> you want an example of both (communicating something to the PHY driver

> and having a custom MII bus) you can look at drivers/net/dsa/bcm_sf2.c

> for an example.

> -- 

> Florian


Thanks a lot for the suggestions. Will send v2 to the net-next branch
hoping I did all the correct way.
Qingfang Deng April 25, 2021, 4:45 a.m. UTC | #5
Hi Ansuel,

On Sat, Apr 24, 2021 at 11:18:20PM +0200, Ansuel Smith wrote:
> 

> I'm starting to do some work with this and a problem arised. Since these

> value are based on the switch revision, how can I access these kind of

> data from the phy driver? It's allowed to declare a phy driver in the

> dsa directory? (The idea would be to create a qca8k dir with the dsa

> driver and the dedicated internal phy driver.) This would facilitate the

> use of normal qca8k_read/write (to access the switch revision from the

> phy driver) using common function?


In case of different switch revision, the PHY ID should also be different.
I think you can reuse the current at803x.c PHY driver, as they seem to
share similar registers.

> 

> > -- 

> > Florian
Christian Marangi April 25, 2021, 11:59 a.m. UTC | #6
On Sun, Apr 25, 2021 at 12:45:54PM +0800, DENG Qingfang wrote:
> Hi Ansuel,
> 
> On Sat, Apr 24, 2021 at 11:18:20PM +0200, Ansuel Smith wrote:
> > 
> > I'm starting to do some work with this and a problem arised. Since these
> > value are based on the switch revision, how can I access these kind of
> > data from the phy driver? It's allowed to declare a phy driver in the
> > dsa directory? (The idea would be to create a qca8k dir with the dsa
> > driver and the dedicated internal phy driver.) This would facilitate the
> > use of normal qca8k_read/write (to access the switch revision from the
> > phy driver) using common function?
> 
> In case of different switch revision, the PHY ID should also be different.
> I think you can reuse the current at803x.c PHY driver, as they seem to
> share similar registers.
>

Is this really necessary? Every PHY has the same ID linked to the switch
id but the revision can change across the same switch id. Isn't the phy
dev flag enought to differiante one id from another? 

> > 
> > > -- 
> > > Florian
Andrew Lunn April 25, 2021, 2:33 p.m. UTC | #7
On Sun, Apr 25, 2021 at 01:59:19PM +0200, Ansuel Smith wrote:
> On Sun, Apr 25, 2021 at 12:45:54PM +0800, DENG Qingfang wrote:

> > Hi Ansuel,

> > 

> > On Sat, Apr 24, 2021 at 11:18:20PM +0200, Ansuel Smith wrote:

> > > 

> > > I'm starting to do some work with this and a problem arised. Since these

> > > value are based on the switch revision, how can I access these kind of

> > > data from the phy driver? It's allowed to declare a phy driver in the

> > > dsa directory? (The idea would be to create a qca8k dir with the dsa

> > > driver and the dedicated internal phy driver.) This would facilitate the

> > > use of normal qca8k_read/write (to access the switch revision from the

> > > phy driver) using common function?

> > 

> > In case of different switch revision, the PHY ID should also be different.

> > I think you can reuse the current at803x.c PHY driver, as they seem to

> > share similar registers.

> >

> 

> Is this really necessary? Every PHY has the same ID linked to the switch

> id but the revision can change across the same switch id. Isn't the phy

> dev flag enought to differiante one id from another? 


Just as general background information: A PHY ID generally consists of
three parts.

1) OUI - Identifies the manufacture - 22 bits
2) device - Generally 6 bits
3) revision - Generally 4 bits

The 22 bits of OUI is standardized. But the last 10 bits the vendor
can use as they wish. But generally, this is how it is used.

Loading the PHY driver is generally based on matching the OUI and
device ID. The revision is ignored. But it is available to the driver
if needed.

It could be, the switch revision is also reflected in the PHY
revision.

	Andrew
diff mbox series

Patch

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 193c269d8ed3..12d2c97d1417 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -909,7 +909,7 @@  qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 {
 	const struct qca8k_match_data *data;
 	struct qca8k_priv *priv = ds->priv;
-	u32 reg, val;
+	u32 phy, reg, val;
 
 	/* get the switches ID from the compatible */
 	data = of_device_get_match_data(priv->dev);
@@ -928,7 +928,26 @@  qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	case 3:
 	case 4:
 	case 5:
-		/* Internal PHY, nothing to do */
+		/* Internal PHY, apply revision fixup */
+		phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
+		switch (priv->switch_revision) {
+		case 1:
+			/* For 100M waveform */
+			qca8k_phy_dbg_write(priv, phy, 0, 0x02ea);
+			/* Turn on Gigabit clock */
+			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x68a0);
+			break;
+
+		case 2:
+			qca8k_phy_mmd_write(priv, phy, 0x7, 0x3c, 0x0);
+			fallthrough;
+		case 4:
+			qca8k_phy_mmd_write(priv, phy, 0x3, 0x800d, 0x803f);
+			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x6860);
+			qca8k_phy_dbg_write(priv, phy, 0x5, 0x2c46);
+			qca8k_phy_dbg_write(priv, phy, 0x3c, 0x6000);
+			break;
+		}
 		return;
 	case 6: /* 2nd CPU port / external PHY */
 		if (state->interface != PHY_INTERFACE_MODE_RGMII &&