Re: [PATCH 1/3][v2] net: phy: introduce 1000BASE-KX and 10GBASE-KR

On 22.01.2016 09:15, Shaohui Xie wrote:

_______________________________________
From: Andrew Lunn <andrew@lunn.ch>
Sent: Friday, January 22, 2016 5:12 AM
To: Shaohui Xie
Cc: Sebastian Hesselbarth; Florian Fainelli; shh.xie@gmail.com; devicetree@vger.kernel.org; netdev@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; davem@davemloft.net; Shaohui Xie
Subject: Re: [PATCH 1/3][v2] net: phy: introduce 1000BASE-KX and 10GBASE-KR

On Tue, Jan 19, 2016 at 05:00:35AM +0000, Shaohui Xie wrote:

quoted

quoted

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch]
Sent: Monday, January 18, 2016 11:15 PM
To: Shaohui Xie
Cc: Sebastian Hesselbarth; Florian Fainelli; shh.xie@gmail.com;
devicetree@vger.kernel.org; netdev@vger.kernel.org; linuxppc-
dev@lists.ozlabs.org; davem@davemloft.net; Shaohui Xie
Subject: Re: [PATCH 1/3][v2] net: phy: introduce 1000BASE-KX and 10GBASE-KR

quoted

[S.H] the fsl backplane, e.g. 10GBASE-KR, needs software to handle
link training, It's to train link partner, and trained by link partner

parallel.

quoted

But if media type is not copper, e.g. optical module, we won't need this.

So what we actually need to know is copper vs fibre?

quoted

Copper is not enough to indicate backplane, since backplane is
always copper, but copper is not always backplane.

quoted

O.K, lets try again....

[S.H]Seems I did not get your point, Sorry for the inconvenient.

quoted

If it is copper backplane you need to perform training.

quoted

Looking at the driver probe function, it is either 1000BASE-KX, no
training needed, or else it is 10GBASE-RK and training is needed.

quoted

Looking at fsl_backplane_config_aneg() you expect phydev->speed to be
set, and from the speed you then kick of either KR autoneg or KX
autoneg. Could you also start the training at this point? Use the
speed to indicate if training is needed?

  [S.H]The training cannot be started at this point, yet, because it's based on
autoneg result, only when both sides autoneg-ed to 10G-KR, then to start
the training.

Shaohui,

look, we want you to convince us why to have a generic backplane
property in the phy binding. You had a bad start by adding new
property values to a property that does not relate to your use
case at all. Your job now really is to give strong reasons _why_
and _what_ a phy driver needs to know about the backplane setup.

Your first approach was to add "10gbase-rk" or "40gbase-foo" but
now you tell us about ANEG. Of what use is the information given
by the property when ANEG tells you something different? E.g.
consider the property tells you "10g-something" but ANEG gives
you "40g"; does the property add any value to your training
decision now?

Besides the driver, generally speaking, "copper + speed" is not enough to indicate
it's backplane, for ex. "copper + 1000" does not mean it has to be 1000BASE-KX,
it could be SGMII, hence cannot use KX autoneg.

So, is it copper + speed + backplane? or speed + backplane? And out of
the set of required input, is there anything your _cannot_ determine
from other things, e.g. ANEG?

If it is backplane only, would a boolean property ("backplane-mode")
be enough for the training decision?

If putting backplane property to phy.txt is not good, I can put it to fsl specific
binding, like the second patch 2/3 did.

You seem to see vendor specific properties as a place to dump all your
waste you don't want to think about. You fail to give good reasons what
is so special about the backplane setup and now you are telling us that
it is even fsl-specific?

Sebastian

quoted

Looking at fsl_backplane_config_aneg() you expect phydev->speed to be
set, and from the speed you then kick of either KR autoneg or KX
autoneg. Could you also start the training at this point? Use the
speed to indicate if training is needed?

  [S.H]The training cannot be started at this point, yet, because it's ba=

sed on

autoneg result, only when both sides autoneg-ed to 10G-KR, then to start
the training.

Shaohui,

look, we want you to convince us why to have a generic backplane
property in the phy binding. You had a bad start by adding new
property values to a property that does not relate to your use
case at all. Your job now really is to give strong reasons _why_
and _what_ a phy driver needs to know about the backplane setup.

[S.H] The PHY driver needs to know what the backplane mode is,=20
1000BASE-KX or 10GBASE-KR, the driver parses the property for
"1000base-kx" or "10gbase-kr", the driver does use the property,
I don't understand why the property is not related to my use case?

Your first approach was to add "10gbase-rk" or "40gbase-foo" but
now you tell us about ANEG. Of what use is the information given
by the property when ANEG tells you something different? E.g.
consider the property tells you "10g-something" but ANEG gives
you "40g"; does the property add any value to your training
decision now?

[S.H] The ANEG is not a gerneric AN, it's special to 10G-KR,  KR AN can onl=
y
AN to KR if both sides support KR, it cannot give "40g" or anything differe=
nt,=20
drive needs the property to do speicific init.

Besides the driver, generally speaking, "copper + speed" is not enough to=

 indicate

it's backplane, for ex. "copper + 1000" does not mean it has to be 1000BA=

SE-KX,

it could be SGMII, hence cannot use KX autoneg.

So, is it copper + speed + backplane? or speed + backplane? And out of
the set of required input, is there anything your _cannot_ determine
from other things, e.g. ANEG?

[S.H] Backplane is enough, 1000BASE-KX means : copper + 1000 + KX stuff.
The KR AN is to check whether LP is also KR, if yes, do training, otherwise=
, do nothing.

If it is backplane only, would a boolean property ("backplane-mode")
be enough for the training decision?

[S.H] a boolean property is not enough, there are different backplane modes=
.

If putting backplane property to phy.txt is not good, I can put it to fsl=

 specific

binding, like the second patch 2/3 did.

You seem to see vendor specific properties as a place to dump all your
waste you don't want to think about. You fail to give good reasons what
is so special about the backplane setup and now you are telling us that
it is even fsl-specific?

[S.H] I don't think it's waste, I just thought it might be special to fsl.
Agreed I failed to give good reasons, but I tried hard to. :(

Thanks,
Shaohui

Sebastian