[PATCH v5 0/2] Add support for the ethernet switch on the ESPRESSObin

[Gregory CLEMENT]

Hi,

This set of patches adds support for the Marvell Ethernet switch
88E6341 which is found on the ESPRESSObin. With this series the
network is usable on this board.

From now on, I am taking care of this series.

As Andrew Lunn pointed this switch is not fully compatible with the
6352. However it is neither fully compatible with the 6390. Actually
it is more a mix between this two families.

I created a new family for this switch and filled the ops structure by
selecting which seems the more appropriate functions. I rebased the
series on net-next/master which allowed me to benefit to the eeprom
functions introduced for the 6390.



While comparing the datasheet and the ops functions used, some
question came to me. They should not prevent applying this series,
but their answer would help me to have a better understanding of the
dsa subsystem.

- Are the temperature related operation still useful with dsa2 ?

Indeed the hwmon initialization is only done from dsa_probe which is
not called if we use dsa2.

- Why the setup is done differently between the 6390 and the 6352
families when the have exactly the same register?

- On the Port Controller 2, the bit PORT_CONTROL_2_MAP_DA is set for
6352 and not for 6390 whereas the same bit exists in 6360 and the
description for this bit is the same for both datasheet.


- Register PORT_ATU_CONTROL and PORT_PRI_OVERRIDE are reset on 6352
and not on 6390. While here again the registers description are
the same.

Thanks,

Gregory


Gregory CLEMENT (1):
net: dsa: mv88e6xxx: Add support for ethernet switch 88E6341

Romain Perier (1):
net: dsa: mv88e6xxx: Don't forbid MDIO I/Os for PHY addr >=
num_of_ports

drivers/net/dsa/mv88e6xxx/chip.c | 61 +++++++++++++++++++++++++++++------
drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 21 +++++++++++-
2 files changed, 72 insertions(+), 10 deletions(-)

[Gregory CLEMENT]

The Marvell 88E6341 device is single-chip, 6-port Ethernet switch with
four integrated 10/100/1000Mbps Ethernet transceivers and one high speed
SerDes interfaces. It is partially compatible with switches of family
88E6352 and switches of family 88E6390.

This commit adds an initial support for this switch by describing its
capabilities to the driver and introducing a new family.

Signed-off-by: Gregory CLEMENT <***@free-electrons.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 55 +++++++++++++++++++++++++++++++++--
drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 21 ++++++++++++-
2 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index d1960ae0a618..c73800e8105b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -664,6 +664,11 @@ static bool mv88e6xxx_6320_family(struct mv88e6xxx_chip *chip)
return chip->info->family == MV88E6XXX_FAMILY_6320;
}

+static bool mv88e6xxx_6341_family(struct mv88e6xxx_chip *chip)
+{
+ return chip->info->family == MV88E6XXX_FAMILY_6341;
+}
+
static bool mv88e6xxx_6351_family(struct mv88e6xxx_chip *chip)
{
return chip->info->family == MV88E6XXX_FAMILY_6351;
@@ -1688,7 +1693,8 @@ static int _mv88e6xxx_vtu_new(struct mv88e6xxx_chip *chip, u16 vid,
: GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER;

if (mv88e6xxx_6097_family(chip) || mv88e6xxx_6165_family(chip) ||
- mv88e6xxx_6351_family(chip) || mv88e6xxx_6352_family(chip)) {
+ mv88e6xxx_6351_family(chip) || mv88e6xxx_6352_family(chip) ||
+ mv88e6xxx_6341_family(chip)) {
struct mv88e6xxx_vtu_entry vstp;

/* Adding a VTU entry requires a valid STU entry. As VSTP is not
@@ -2543,7 +2549,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
if (mv88e6xxx_6352_family(chip) || mv88e6xxx_6351_family(chip) ||
mv88e6xxx_6165_family(chip) || mv88e6xxx_6097_family(chip) ||
mv88e6xxx_6095_family(chip) || mv88e6xxx_6320_family(chip) ||
- mv88e6xxx_6185_family(chip))
+ mv88e6xxx_6185_family(chip) || mv88e6xxx_6341_family(chip))
reg = PORT_CONTROL_2_MAP_DA;

if (mv88e6xxx_6095_family(chip) || mv88e6xxx_6185_family(chip)) {
@@ -2597,7 +2603,8 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)

if (mv88e6xxx_6352_family(chip) || mv88e6xxx_6351_family(chip) ||
mv88e6xxx_6165_family(chip) || mv88e6xxx_6097_family(chip) ||
- mv88e6xxx_6320_family(chip)) {
+ mv88e6xxx_6320_family(chip) || mv88e6xxx_6341_family(chip) ||
+ mv88e6xxx_6341_family(chip)) {
/* Port ATU control: disable limiting the number of
* address database entries that this port is allowed
* to use.
@@ -3714,6 +3721,34 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
.reset = mv88e6352_g1_reset,
};

+static const struct mv88e6xxx_ops mv88e6341_ops = {
+ /* MV88E6XXX_FAMILY_6341 */
+ .get_eeprom = mv88e6xxx_g2_get_eeprom8,
+ .set_eeprom = mv88e6xxx_g2_set_eeprom8,
+ .set_switch_mac = mv88e6xxx_g2_set_switch_mac,
+ .phy_read = mv88e6xxx_g2_smi_phy_read,
+ .phy_write = mv88e6xxx_g2_smi_phy_write,
+ .port_set_link = mv88e6xxx_port_set_link,
+ .port_set_duplex = mv88e6xxx_port_set_duplex,
+ .port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
+ .port_set_speed = mv88e6390_port_set_speed,
+ .port_tag_remap = mv88e6095_port_tag_remap,
+ .port_set_frame_mode = mv88e6351_port_set_frame_mode,
+ .port_set_egress_unknowns = mv88e6351_port_set_egress_unknowns,
+ .port_set_ether_type = mv88e6351_port_set_ether_type,
+ .port_jumbo_config = mv88e6165_port_jumbo_config,
+ .port_egress_rate_limiting = mv88e6097_port_egress_rate_limiting,
+ .port_pause_config = mv88e6097_port_pause_config,
+ .stats_snapshot = mv88e6390_g1_stats_snapshot,
+ .stats_get_sset_count = mv88e6320_stats_get_sset_count,
+ .stats_get_strings = mv88e6320_stats_get_strings,
+ .stats_get_stats = mv88e6390_stats_get_stats,
+ .g1_set_cpu_port = mv88e6390_g1_set_cpu_port,
+ .g1_set_egress_port = mv88e6390_g1_set_egress_port,
+ .mgmt_rsvd2cpu = mv88e6390_g1_mgmt_rsvd2cpu,
+ .reset = mv88e6352_g1_reset,
+};
+
static const struct mv88e6xxx_ops mv88e6390_ops = {
/* MV88E6XXX_FAMILY_6390 */
.get_eeprom = mv88e6xxx_g2_get_eeprom8,
@@ -4101,6 +4136,20 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.ops = &mv88e6321_ops,
},

+ [MV88E6341] = {
+ .prod_num = PORT_SWITCH_ID_PROD_NUM_6341,
+ .family = MV88E6XXX_FAMILY_6341,
+ .name = "Marvell 88E6341",
+ .num_databases = 4096,
+ .num_ports = 6,
+ .port_base_addr = 0x10,
+ .global1_addr = 0x1b,
+ .age_time_coeff = 15000,
+ .tag_protocol = DSA_TAG_PROTO_EDSA,
+ .flags = MV88E6XXX_FLAGS_FAMILY_6341,
+ .ops = &mv88e6341_ops,
+ },
+
[MV88E6350] = {
.prod_num = PORT_SWITCH_ID_PROD_NUM_6350,
.family = MV88E6XXX_FAMILY_6351,
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 466cfdadb7bd..35e879169003 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -100,6 +100,7 @@
#define PORT_SWITCH_ID_PROD_NUM_6240 0x240
#define PORT_SWITCH_ID_PROD_NUM_6290 0x290
#define PORT_SWITCH_ID_PROD_NUM_6321 0x310
+#define PORT_SWITCH_ID_PROD_NUM_6341 0x340
#define PORT_SWITCH_ID_PROD_NUM_6352 0x352
#define PORT_SWITCH_ID_PROD_NUM_6350 0x371
#define PORT_SWITCH_ID_PROD_NUM_6351 0x375
@@ -382,7 +383,7 @@
#define GLOBAL2_EEPROM_CMD_WRITE_EN BIT(10)
#define GLOBAL2_EEPROM_CMD_ADDR_MASK 0xff
#define GLOBAL2_EEPROM_DATA 0x15
-#define GLOBAL2_EEPROM_ADDR 0x15 /* 6390 */
+#define GLOBAL2_EEPROM_ADDR 0x15 /* 6390, 6341 */
#define GLOBAL2_PTP_AVB_OP 0x16
#define GLOBAL2_PTP_AVB_DATA 0x17
#define GLOBAL2_SMI_PHY_CMD 0x18
@@ -433,6 +434,7 @@ enum mv88e6xxx_model {
MV88E6290,
MV88E6320,
MV88E6321,
+ MV88E6341,
MV88E6350,
MV88E6351,
MV88E6352,
@@ -448,6 +450,7 @@ enum mv88e6xxx_family {
MV88E6XXX_FAMILY_6165, /* 6123 6161 6165 */
MV88E6XXX_FAMILY_6185, /* 6108 6121 6122 6131 6152 6155 6182 6185 */
MV88E6XXX_FAMILY_6320, /* 6320 6321 */
+ MV88E6XXX_FAMILY_6341, /* 6141 6341 */
MV88E6XXX_FAMILY_6351, /* 6171 6175 6350 6351 */
MV88E6XXX_FAMILY_6352, /* 6172 6176 6240 6352 */
MV88E6XXX_FAMILY_6390, /* 6190 6190X 6191 6290 6390 6390X */
@@ -612,6 +615,22 @@ enum mv88e6xxx_cap {
MV88E6XXX_FLAGS_MULTI_CHIP | \
MV88E6XXX_FLAGS_PVT)

+#define MV88E6XXX_FLAGS_FAMILY_6341 \
+ (MV88E6XXX_FLAG_EEE | \
+ MV88E6XXX_FLAG_G1_ATU_FID | \
+ MV88E6XXX_FLAG_G1_VTU_FID | \
+ MV88E6XXX_FLAG_GLOBAL2 | \
+ MV88E6XXX_FLAG_G2_INT | \
+ MV88E6XXX_FLAG_G2_POT | \
+ MV88E6XXX_FLAG_STU | \
+ MV88E6XXX_FLAG_TEMP | \
+ MV88E6XXX_FLAG_TEMP_LIMIT | \
+ MV88E6XXX_FLAG_VTU | \
+ MV88E6XXX_FLAGS_IRL | \
+ MV88E6XXX_FLAGS_MULTI_CHIP | \
+ MV88E6XXX_FLAGS_PVT | \
+ MV88E6XXX_FLAGS_SERDES)
+
#define MV88E6XXX_FLAGS_FAMILY_6351 \
(MV88E6XXX_FLAG_G1_ATU_FID | \
MV88E6XXX_FLAG_G1_VTU_FID | \

[Vivien Didelot]

Post by Gregory CLEMENT
+static bool mv88e6xxx_6341_family(struct mv88e6xxx_chip *chip)
+{
+ return chip->info->family == MV88E6XXX_FAMILY_6341;
+}

I don't want to see these erronous family checks anymore, but I cannot
blame you for adding it since not all the code is moved to ops yet ;)

Post by Gregory CLEMENT
MV88E6XXX_FAMILY_6165, /* 6123 6161 6165 */
MV88E6XXX_FAMILY_6185, /* 6108 6121 6122 6131 6152 6155 6182 6185 */
MV88E6XXX_FAMILY_6320, /* 6320 6321 */
+ MV88E6XXX_FAMILY_6341, /* 6141 6341 */

Maybe I missed it, Andrew, can you confirm that 6341 is a proper Marvell
family of switch chips?

Post by Gregory CLEMENT
MV88E6XXX_FAMILY_6351, /* 6171 6175 6350 6351 */
MV88E6XXX_FAMILY_6352, /* 6172 6176 6240 6352 */
MV88E6XXX_FAMILY_6390, /* 6190 6190X 6191 6290 6390 6390X */

Otherwise the patch looks good.

Thanks,

Vivien

[Andrew Lunn]

Post by Vivien Didelot

Post by Gregory CLEMENT
+static bool mv88e6xxx_6341_family(struct mv88e6xxx_chip *chip)
+{
+ return chip->info->family == MV88E6XXX_FAMILY_6341;
+}

I don't want to see these erronous family checks anymore, but I cannot
blame you for adding it since not all the code is moved to ops yet ;)

Post by Gregory CLEMENT
MV88E6XXX_FAMILY_6165, /* 6123 6161 6165 */
MV88E6XXX_FAMILY_6185, /* 6108 6121 6122 6131 6152 6155 6182 6185 */
MV88E6XXX_FAMILY_6320, /* 6320 6321 */
+ MV88E6XXX_FAMILY_6341, /* 6141 6341 */

Maybe I missed it, Andrew, can you confirm that 6341 is a proper Marvell
family of switch chips?

My understand is that it is. Marvell have not added it to DSDT. There
is a new SDK called UMSD, also GPLv2 and BSD. They call this family
Topaz, and the 6390 is Peridot.

Andrew

[Vivien Didelot]

Hi Andrew,

Post by Andrew Lunn

Post by Vivien Didelot
Maybe I missed it, Andrew, can you confirm that 6341 is a proper Marvell
family of switch chips?

My understand is that it is. Marvell have not added it to DSDT. There
is a new SDK called UMSD, also GPLv2 and BSD. They call this family
Topaz, and the 6390 is Peridot.

At least we could rename it anytime. They don't really make sense nor or
reliable anyway...

Thanks,

Vivien

[Gregory CLEMENT]

Hi Vvien and Andrew,

Post by Andrew Lunn

Post by Vivien Didelot

Post by Gregory CLEMENT
+static bool mv88e6xxx_6341_family(struct mv88e6xxx_chip *chip)
+{
+ return chip->info->family == MV88E6XXX_FAMILY_6341;
+}

I don't want to see these erronous family checks anymore, but I cannot
blame you for adding it since not all the code is moved to ops yet ;)

If there a series about to be merged I can rebase my series on it. Else
I propose to keep it and convert the family check to ops when you will
send the series for it.

Post by Andrew Lunn

Post by Vivien Didelot

Post by Gregory CLEMENT
MV88E6XXX_FAMILY_6165, /* 6123 6161 6165 */
MV88E6XXX_FAMILY_6185, /* 6108 6121 6122 6131 6152 6155 6182 6185 */
MV88E6XXX_FAMILY_6320, /* 6320 6321 */
+ MV88E6XXX_FAMILY_6341, /* 6141 6341 */

Maybe I missed it, Andrew, can you confirm that 6341 is a proper Marvell
family of switch chips?

My understand is that it is. Marvell have not added it to DSDT. There
is a new SDK called UMSD, also GPLv2 and BSD. They call this family
Topaz, and the 6390 is Peridot.

I confirm that 6141 and 6341 are called Topaz. Actually I can add the
support for the 6141 too, it is just a matter of adding an ID and maybe
removing some ops as 6141 is a subset of 6341.

Gregory

[Vivien Didelot]

Hi Gregory,

Post by Gregory CLEMENT
If there a series about to be merged I can rebase my series on it. Else
I propose to keep it and convert the family check to ops when you will
send the series for it.

I am reworking the VTU operations, but not these port operations yet.
I will take care of converting the family checks once I'm done, don't
worry ;)

Post by Gregory CLEMENT
I confirm that 6141 and 6341 are called Topaz. Actually I can add the
support for the 6141 too, it is just a matter of adding an ID and maybe
removing some ops as 6141 is a subset of 6341.

I don't mind having a patch adding support for only one chip at a time,
that is up to you.

Thanks,

Vivien

[Andrew Lunn]

Post by Vivien Didelot
Hi Gregory,

Post by Gregory CLEMENT
If there a series about to be merged I can rebase my series on it. Else
I propose to keep it and convert the family check to ops when you will
send the series for it.

I am reworking the VTU operations, but not these port operations yet.

I'm working on them. I have a patch ready for testing on my hardware.

Andrew

[Gregory CLEMENT]

From: Romain Perier <***@free-electrons.com>

Some Marvell ethernet switches have internal ethernet transceivers with
hardcoded phy addresses. These addresses can be greater than the number
of ports or its value might be different than the associated port number.
This is for example the case for MV88E6341 that has 6 ports and internal
Port 1 to Port4 PHYs mapped at SMI addresses from 0x11 to 0x14.

This commits fixes the issue by removing the condition in MDIO callbacks.

Signed-off-by: Romain Perier <***@free-electrons.com>
Reviewed-by: Andrew Lunn <***@lunn.ch>
Signed-off-by: Gregory CLEMENT <***@free-electrons.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 6 ------
1 file changed, 6 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 987b2dbbd35a..d1960ae0a618 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2882,9 +2882,6 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
u16 val;
int err;

- if (phy >= mv88e6xxx_num_ports(chip))
- return 0xffff;
-
mutex_lock(&chip->reg_lock);
err = mv88e6xxx_phy_read(chip, phy, reg, &val);
mutex_unlock(&chip->reg_lock);
@@ -2897,9 +2894,6 @@ static int mv88e6xxx_mdio_write(struct mii_bus *bus, int phy, int reg, u16 val)
struct mv88e6xxx_chip *chip = bus->priv;
int err;

- if (phy >= mv88e6xxx_num_ports(chip))
- return 0xffff;
-
mutex_lock(&chip->reg_lock);
err = mv88e6xxx_phy_write(chip, phy, reg, val);
mutex_unlock(&chip->reg_lock);

[Vivien Didelot]

Hi Gregory,

Post by Gregory CLEMENT
Some Marvell ethernet switches have internal ethernet transceivers with
hardcoded phy addresses. These addresses can be greater than the number
of ports or its value might be different than the associated port number.
This is for example the case for MV88E6341 that has 6 ports and internal
Port 1 to Port4 PHYs mapped at SMI addresses from 0x11 to 0x14.

Isn't there an hardware table used to map the PHY addresses on such chip?

Post by Gregory CLEMENT
This commits fixes the issue by removing the condition in MDIO callbacks.

The patch is anyway still valid:

Reviewed-by: Vivien Didelot <***@savoirfairelinux.com>

Thanks,

Vivien

[Andrew Lunn]

Post by Vivien Didelot
Hi Gregory,

Post by Gregory CLEMENT
Some Marvell ethernet switches have internal ethernet transceivers with
hardcoded phy addresses. These addresses can be greater than the number
of ports or its value might be different than the associated port number.
This is for example the case for MV88E6341 that has 6 ports and internal
Port 1 to Port4 PHYs mapped at SMI addresses from 0x11 to 0x14.

Isn't there an hardware table used to map the PHY addresses on such chip?

The 6390 has something like this. But if we can avoid it, lets keep it
KISS.

Andrew

[Vivien Didelot]

Hi Andrew,

Post by Andrew Lunn

Post by Vivien Didelot
Isn't there an hardware table used to map the PHY addresses on such chip?

The 6390 has something like this. But if we can avoid it, lets keep it
KISS.

I definitely agree. I was just curious about this new chip.

Thanks,

Vivien

[Andrew Lunn]

Post by Gregory CLEMENT
While comparing the datasheet and the ops functions used, some
question came to me. They should not prevent applying this series,
but their answer would help me to have a better understanding of the
dsa subsystem.
- Are the temperature related operation still useful with dsa2 ?

No. I'm in the process of moving the code into the Marvell PHY driver,
since the sensor is in the embedded PHYs.

What ID does the embedded PHY use? The 6390 has a blank ID, where as
older device have a real ID.

Post by Gregory CLEMENT
- Why the setup is done differently between the 6390 and the 6352
families when the have exactly the same register?

EDSA on 6390 works differently to 6352, meaning it breaks. So we need
to run the 6390 with DSA tagging, not EDSA. Maybe this is the source
of the differences?

It should also be noted that the 6390 support is not yet complete. I
have a few more patches in my tree to post.

Post by Gregory CLEMENT
- On the Port Controller 2, the bit PORT_CONTROL_2_MAP_DA is set for
6352 and not for 6390 whereas the same bit exists in 6360 and the
description for this bit is the same for both datasheet.

Humm, it does look like it is missing mv88e6xxx_6390_family(chip).

Post by Gregory CLEMENT
- Register PORT_ATU_CONTROL and PORT_PRI_OVERRIDE are reset on 6352
and not on 6390. While here again the registers description are
the same.

And the same here. I've mostly been working on where the 6390 is
different. Where it is the same i've mostly ignored it so far :-)

There is also an ongoing effort to remove all these big if statements
with a list of families.

Andrew

[Jon Pannell]

+ Bob + Christine

Jon Pannell


-----Original Message-----
From: Andrew Lunn [mailto:***@lunn.ch]
Sent: Thursday, January 19, 2017 2:06 PM
To: Gregory CLEMENT <***@free-electrons.com>
Cc: Vivien Didelot <***@savoirfairelinux.com>; Florian Fainelli <***@gmail.com>; ***@vger.kernel.org; linux-***@vger.kernel.org; David S. Miller <***@davemloft.net>; Jason Cooper <***@lakedaemon.net>; Sebastian Hesselbarth <***@gmail.com>; Thomas Petazzoni <***@free-electrons.com>; linux-arm-***@lists.infradead.org; Nadav Haklai <***@marvell.com>; Wilson Ding <***@marvell.com>; Kostya Porotchkin <***@marvell.com>; Joe Zhou <***@marvell.com>; Jon Pannell <***@marvell.com>
Subject: [EXT] Re: [PATCH v5 0/2] Add support for the ethernet switch on the ESPRESSObin

External Email

----------------------------------------------------------------------

Post by Gregory CLEMENT
While comparing the datasheet and the ops functions used, some
question came to me. They should not prevent applying this series, but
their answer would help me to have a better understanding of the dsa
subsystem.
- Are the temperature related operation still useful with dsa2 ?

No. I'm in the process of moving the code into the Marvell PHY driver, since the sensor is in the embedded PHYs.

What ID does the embedded PHY use? The 6390 has a blank ID, where as older device have a real ID.

Post by Gregory CLEMENT
- Why the setup is done differently between the 6390 and the 6352
families when the have exactly the same register?

EDSA on 6390 works differently to 6352, meaning it breaks. So we need to run the 6390 with DSA tagging, not EDSA. Maybe this is the source of the differences?

It should also be noted that the 6390 support is not yet complete. I have a few more patches in my tree to post.

Post by Gregory CLEMENT
- On the Port Controller 2, the bit PORT_CONTROL_2_MAP_DA is set for
6352 and not for 6390 whereas the same bit exists in 6360 and the
description for this bit is the same for both datasheet.

Humm, it does look like it is missing mv88e6xxx_6390_family(chip).

Post by Gregory CLEMENT
- Register PORT_ATU_CONTROL and PORT_PRI_OVERRIDE are reset on 6352
and not on 6390. While here again the registers description are
the same.

And the same here. I've mostly been working on where the 6390 is different. Where it is the same i've mostly ignored it so far :-)

There is also an ongoing effort to remove all these big if statements with a list of families.

Andrew

[Gregory CLEMENT]

Hi Andrew,

Post by Andrew Lunn

Post by Gregory CLEMENT
While comparing the datasheet and the ops functions used, some
question came to me. They should not prevent applying this series,
but their answer would help me to have a better understanding of the
dsa subsystem.
- Are the temperature related operation still useful with dsa2 ?

No. I'm in the process of moving the code into the Marvell PHY driver,
since the sensor is in the embedded PHYs.
What ID does the embedded PHY use? The 6390 has a blank ID, where as
older device have a real ID.

Actually I didn't find anything related to the temperature measurement
in the datasheet I have. For the 6390 there is a dedicated datsheet for
the PHY part for the 6352 it is part of the same datasheet.

After a second look I think I don't have anything related to the PHY
part in the datasheets.

What I wanted to do was to test 6390 and 6352 temperature related
functions and to see if one of them worked. That's how I realized it was
not possible to do it with dsa2.

Post by Andrew Lunn

Post by Gregory CLEMENT
- Why the setup is done differently between the 6390 and the 6352
families when the have exactly the same register?

EDSA on 6390 works differently to 6352, meaning it breaks. So we need
to run the 6390 with DSA tagging, not EDSA. Maybe this is the source
of the differences?
It should also be noted that the 6390 support is not yet complete. I
have a few more patches in my tree to post.

Post by Gregory CLEMENT
- On the Port Controller 2, the bit PORT_CONTROL_2_MAP_DA is set for
6352 and not for 6390 whereas the same bit exists in 6360 and the
description for this bit is the same for both datasheet.

Humm, it does look like it is missing mv88e6xxx_6390_family(chip).

Post by Gregory CLEMENT
- Register PORT_ATU_CONTROL and PORT_PRI_OVERRIDE are reset on 6352
and not on 6390. While here again the registers description are
the same.

And the same here. I've mostly been working on where the 6390 is
different. Where it is the same i've mostly ignored it so far :-)
There is also an ongoing effort to remove all these big if statements
with a list of families.

Thanks for this answers I understand it a little better now.

Gregory

Post by Andrew Lunn
Andrew

[Andrew Lunn]

Post by Gregory CLEMENT
What I wanted to do was to test 6390 and 6352 temperature related
functions and to see if one of them worked. That's how I realized it was
not possible to do it with dsa2.

Hi Gregory

Probably the best thing to do is wait until the temperature code moves
into the PHY driver. It then becomes easier to test. It probably is
the same as the 6352, or like the 6390 which i've so far failed to get
working, despite the registers being the same as the 6352.

Andrew

[Andrew Lunn]

Post by Gregory CLEMENT
Actually I didn't find anything related to the temperature measurement
in the datasheet I have. For the 6390 there is a dedicated datsheet for
the PHY part for the 6352 it is part of the same datasheet.

Hi Gregory

The temperature sensor changes have landed in net-next. If you have
time, please rebase to it and do some tests. Here are the likely
outcomes:

1) Like the 6390, it does not have a valid PHY product ID. Hence the
Marvell PHY driver is not loaded. You can see the PHY ID in

/sys/bus/mdio_bus/devices/*/phy_id

If it is 0x01410000, there is no product ID. I have a workaround for
this.

2) It has a valid phy_id, but it is not known to the marvell driver.
Add an entry to the table at the bottom of drivers/net/phy/marvell.c,
and a new entry in marvell_drivers. I would copy the 1540.


3) The Marvell PHY driver does recognise it, and makes the temperature
available in /sys/class/hwmon/hwmon*/temp1_input. It always returns
-25000mC. Same problem i have with the 6390. No idea how to fix it yet.

4) The Marvell PHY driver does recognise it, and makes the temperature
available in /sys/class/hwmon/hwmon*/temp1_input. The value is O.K. It
all works :-)

Personally, i'm not betting on 4 :-)


Andrew

[Vivien Didelot]

Hi Gregory,

Post by Gregory CLEMENT
I created a new family for this switch and filled the ops structure by
selecting which seems the more appropriate functions.

We don't create families, this information comes from the Marvell
DSDT. If you don't know which switch family Marvell put the 6341 in, you
should be able to use MV88E6XXX_FAMILY_NONE safely.

Note that this is anyway not a useful information since the Marvell
chips have lots of per-models specificities... Any family related code
(e.g. the mv88e6xxx_*_family() helpers) are likely to be removed soon.

Post by Gregory CLEMENT
While comparing the datasheet and the ops functions used, some
question came to me. They should not prevent applying this series,
but their answer would help me to have a better understanding of the
dsa subsystem.
- Are the temperature related operation still useful with dsa2 ?
Indeed the hwmon initialization is only done from dsa_probe which is
not called if we use dsa2.

HWMON is not supported anymore in dsa2. Andrew is in the process of
moving its support to the PHY driver instead. However you can still test
it locally with this patch:

http://ix.io/1QvY

The temperature is accessed via ethtool -e|-E.

Post by Gregory CLEMENT
- Why the setup is done differently between the 6390 and the 6352
families when the have exactly the same register?
- On the Port Controller 2, the bit PORT_CONTROL_2_MAP_DA is set for
6352 and not for 6390 whereas the same bit exists in 6360 and the
description for this bit is the same for both datasheet.
- Register PORT_ATU_CONTROL and PORT_PRI_OVERRIDE are reset on 6352
and not on 6390. While here again the registers description are
the same.

You are totally correct here. We are in the process of moving every old
pieces of code such as this:

if (mv88e6xxx_6352_family(chip) || mv88e6xxx_6351_family(chip) ||
mv88e6xxx_6165_family(chip) || mv88e6xxx_6097_family(chip) ||
mv88e6xxx_6095_family(chip) || mv88e6xxx_6320_family(chip) ||
mv88e6xxx_6185_family(chip))
reg = PORT_CONTROL_2_MAP_DA;

into proper "library" functions defined in the correct internal SMI
device specific file (here, port.c), to be used in the mv88e6xxx_ops
structure, if a given model support the feature described in the
datasheet.

So the two registers you mentioned don't have proper ops yet.

Thanks!

Vivien

[Vivien Didelot]

Hi Gregory,

Post by Vivien Didelot
The temperature is accessed via ethtool -e|-E.

Oops, that was a silly mistake, no need to explain! ;-)

Vivien

[David Miller]

From: Gregory CLEMENT <***@free-electrons.com>
Date: Thu, 19 Jan 2017 22:49:32 +0100

Post by Gregory CLEMENT
I created a new family for this switch and filled the ops structure
by selecting which seems the more appropriate functions. I rebased
the series on net-next/master which allowed me to benefit to the
eeprom functions introduced for the 6390.

It looks like there will be at least one more respin of this series,
specifically to remove the new family as Vivien seems to object to
this.

[Gregory CLEMENT]

Hi David,

Post by David Miller
Date: Thu, 19 Jan 2017 22:49:32 +0100

Post by Gregory CLEMENT
I created a new family for this switch and filled the ops structure
by selecting which seems the more appropriate functions. I rebased
the series on net-next/master which allowed me to benefit to the
eeprom functions introduced for the 6390.

It looks like there will be at least one more respin of this series,
specifically to remove the new family as Vivien seems to object to
this.

I am about to send a new version. However about removing the new family,
I thought that with the confirmation from Andrew we could keep it.

Vivien could you confirm this?

Thanks,

Gregory

[Vivien Didelot]

Hi Gregory,

Post by Gregory CLEMENT
Hi David,

Post by David Miller
Date: Thu, 19 Jan 2017 22:49:32 +0100

Post by Gregory CLEMENT
I created a new family for this switch and filled the ops structure
by selecting which seems the more appropriate functions. I rebased
the series on net-next/master which allowed me to benefit to the
eeprom functions introduced for the 6390.

It looks like there will be at least one more respin of this series,
specifically to remove the new family as Vivien seems to object to
this.

I am about to send a new version. However about removing the new family,
I thought that with the confirmation from Andrew we could keep it.
Vivien could you confirm this?

I confirm.

Thanks!

Vivien

[Andrew Lunn]

Post by Gregory CLEMENT
Hi David,

Post by David Miller
Date: Thu, 19 Jan 2017 22:49:32 +0100

Post by Gregory CLEMENT
I created a new family for this switch and filled the ops structure
by selecting which seems the more appropriate functions. I rebased
the series on net-next/master which allowed me to benefit to the
eeprom functions introduced for the 6390.

It looks like there will be at least one more respin of this series,
specifically to remove the new family as Vivien seems to object to
this.

I am about to send a new version. However about removing the new family,
I thought that with the confirmation from Andrew we could keep it.

Hi Gregory

Yes, keep it. It will hopefully be short lived, but it is needed at
the moment.

Andrew