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

[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(-)

-- 
2.11.0

[PATCH v5 1/2] net: dsa: mv88e6xxx: Don't forbid MDIO I/Os for PHY addr >= num_of_ports

[Gregory CLEMENT]

From: Romain Perier <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 <romain.perier@free-electrons.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Gregory CLEMENT <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);
-- 
2.11.0

[PATCH v5 1/2] net: dsa: mv88e6xxx: Don't forbid MDIO I/Os for PHY addr >= num_of_ports

[Vivien Didelot]

Hi Gregory,

Gregory CLEMENT <gregory.clement@free-electrons.com> writes:

> From: Romain Perier <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.

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

> This commits fixes the issue by removing the condition in MDIO callbacks.
>
> Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

The patch is anyway still valid:

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

Thanks,

        Vivien

[PATCH v5 1/2] net: dsa: mv88e6xxx: Don't forbid MDIO I/Os for PHY addr >= num_of_ports

[Andrew Lunn]

On Thu, Jan 19, 2017 at 05:13:12PM -0500, Vivien Didelot wrote:
> Hi Gregory,
> 
> Gregory CLEMENT <gregory.clement@free-electrons.com> writes:
> 
> > From: Romain Perier <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.
> 
> 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

[PATCH v5 1/2] net: dsa: mv88e6xxx: Don't forbid MDIO I/Os for PHY addr >= num_of_ports

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> 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

[PATCH v5 2/2] net: dsa: mv88e6xxx: Add support for ethernet switch 88E6341

[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 <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 |	\
-- 
2.11.0

[PATCH v5 2/2] net: dsa: mv88e6xxx: Add support for ethernet switch 88E6341

[Vivien Didelot]

Gregory CLEMENT <gregory.clement@free-electrons.com> writes:

> +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 ;)

>  	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?

>  	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

[PATCH v5 2/2] net: dsa: mv88e6xxx: Add support for ethernet switch 88E6341

[Andrew Lunn]

On Thu, Jan 19, 2017 at 05:26:03PM -0500, Vivien Didelot wrote:
> Gregory CLEMENT <gregory.clement@free-electrons.com> writes:
> 
> > +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 ;)
> 
> >  	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

[PATCH v5 2/2] net: dsa: mv88e6xxx: Add support for ethernet switch 88E6341

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> 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

[PATCH v5 2/2] net: dsa: mv88e6xxx: Add support for ethernet switch 88E6341

[Gregory CLEMENT]

Hi Vvien and Andrew,
 
 On ven., janv. 20 2017, Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Jan 19, 2017 at 05:26:03PM -0500, Vivien Didelot wrote:
>> Gregory CLEMENT <gregory.clement@free-electrons.com> writes:
>> 
>> > +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.

>> 
>> >  	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

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v5 2/2] net: dsa: mv88e6xxx: Add support for ethernet switch 88E6341

[Vivien Didelot]

Hi Gregory,

Gregory CLEMENT <gregory.clement@free-electrons.com> writes:

> 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 ;)

> 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

[PATCH v5 2/2] net: dsa: mv88e6xxx: Add support for ethernet switch 88E6341

[Andrew Lunn]

On Fri, Jan 20, 2017 at 12:30:16PM -0500, Vivien Didelot wrote:
> Hi Gregory,
> 
> Gregory CLEMENT <gregory.clement@free-electrons.com> writes:
> 
> > 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

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

[Andrew Lunn]

> 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.

> - 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.

>   - 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).
 
> 
>   - 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

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

[Jon Pannell]

+ Bob + Christine

Jon Pannell


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

External Email

----------------------------------------------------------------------
> 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.

> - 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.

>   - 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).
 
> 
>   - 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

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

[Gregory CLEMENT]

Hi Andrew,
 
 On jeu., janv. 19 2017, Andrew Lunn <andrew@lunn.ch> wrote:

>> 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.

>
>> - 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.
>
>>   - 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).
>  
>> 
>>   - 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

>
>      Andrew

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Andrew Lunn]

> 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

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

[Andrew Lunn]

> 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

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

[Vivien Didelot]

Hi Gregory,

Gregory CLEMENT <gregory.clement@free-electrons.com> writes:

> 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.
    
> 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.

> - 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

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

[Vivien Didelot]

Hi Gregory,

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

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

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

      Vivien

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

[David Miller]

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

> 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.

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

[Gregory CLEMENT]

Hi David,
 
 On ven., janv. 20 2017, David Miller <davem@davemloft.net> wrote:

> From: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Date: Thu, 19 Jan 2017 22:49:32 +0100
>
>> 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

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Vivien Didelot]

Hi Gregory,

Gregory CLEMENT <gregory.clement@free-electrons.com> writes:

> Hi David,
>  
>  On ven., janv. 20 2017, David Miller <davem@davemloft.net> wrote:
>
>> From: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> Date: Thu, 19 Jan 2017 22:49:32 +0100
>>
>>> 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

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

[Andrew Lunn]

On Tue, Jan 24, 2017 at 05:47:06PM +0100, Gregory CLEMENT wrote:
> Hi David,
>  
>  On ven., janv. 20 2017, David Miller <davem@davemloft.net> wrote:
> 
> > From: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > Date: Thu, 19 Jan 2017 22:49:32 +0100
> >
> >> 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