Re: [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  wrote:
> 
> > From: Gregory CLEMENT 
> > 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

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

[Vivien Didelot]

Hi Gregory,

Gregory CLEMENT  writes:

> Hi David,
>  
>  On ven., janv. 20 2017, David Miller  wrote:
>
>> From: Gregory CLEMENT 
>> 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

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

[Gregory CLEMENT]

Hi David,
 
 On ven., janv. 20 2017, David Miller  wrote:

> From: Gregory CLEMENT 
> 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

Re: [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 0x0141, 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

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

[David Miller]

From: Gregory CLEMENT 
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.

Re: [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

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

[Gregory CLEMENT]

Hi Andrew,
 
 On jeu., janv. 19 2017, Andrew Lunn  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

RE: [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:and...@lunn.ch] 
Sent: Thursday, January 19, 2017 2:06 PM
To: Gregory CLEMENT 
Cc: Vivien Didelot ; Florian Fainelli 
; netdev@vger.kernel.org; linux-ker...@vger.kernel.org; 
David S. Miller ; Jason Cooper ; 
Sebastian Hesselbarth ; Thomas Petazzoni 
; linux-arm-ker...@lists.infradead.org; 
Nadav Haklai ; Wilson Ding ; Kostya 
Porotchkin ; Joe Zhou ; Jon Pannell 

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

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

[Vivien Didelot]

Hi Gregory,

Vivien Didelot  writes:

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

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

  Vivien

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

[Vivien Didelot]

Hi Gregory,

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

Re: [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

[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