| Message ID | 20190124154309.24987-1-marek.behun@nic.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v1,1/2] net: dsa: mv88e6xxx: Default CMODE to 1000BaseX only on 6390X | expand |
On Thu, Jan 24, 2019 at 04:43:08PM +0100, Marek Behún wrote: > Commit 787799a9d555 sets the SERDES interfaces of 6390 and 6390X to > 1000BaseX, but this is only needed on 6390X, since there are SERDES > interfaces which can be used on lower ports on 6390. > > This commit fixes this by returning to previous behaviour on 6390. > (Previous behaviour means that CMODE is not set at all if requested mode > is NA). > > This is needed on Turris MOX, where the 88e6190 is connected to CPU in > 2500BaseX mode. > > Fixes: 787799a9d555 ("net: dsa: mv88e6xxx: Default ports 9/10 6390X CMODE to 1000BaseX") > Signed-off-by: Marek Behún <marek.behun@nic.cz> > --- > drivers/net/dsa/mv88e6xxx/port.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c > index ebd26b6a93e6..ee7029f4ee22 100644 > --- a/drivers/net/dsa/mv88e6xxx/port.c > +++ b/drivers/net/dsa/mv88e6xxx/port.c > @@ -444,6 +444,8 @@ int mv88e6390_port_set_cmode(struct mv88e6xxx_chip *chip, int port, > phy_interface_t mode) > { > switch (mode) { > + case PHY_INTERFACE_MODE_NA: > + return 0; Hi Marek Although the previous behaviour might of allowed the cmode to be NA, i'm not sure that is a good idea. How do you know the port actually is using 2500BaseX, and not SGMII for example? You really should set the interface mode in the device tree file, so you know it does have the value you want. Andrew
> Hi Marek > > Although the previous behaviour might of allowed the cmode to be NA, > i'm not sure that is a good idea. How do you know the port actually is > using 2500BaseX, and not SGMII for example? > > You really should set the interface mode in the device tree file, so > you know it does have the value you want. > > Andrew Hi Andrew, yes, I wanted to set the cpu port mode in DTS at first, but if I understood the code correctly (maybe I made a mistake), the CPU port mode is not determined from DTS (since there is no phy). Am I wrong? It would be better if it was forced from DTS from the CPU port node. On Turris Mox the mode is set by pin configuration after reset. Marek
> yes, I wanted to set the cpu port mode in DTS at first, but if I > understood the code correctly (maybe I made a mistake), the CPU port > mode is not determined from DTS (since there is no phy). Am I wrong? It should work. I have boards which use it for DSA ports. I'm not sure i have a board with it for a CPU port though. Andrew
What properties does the cpu port node need to contain to force it? phy-mode = "2500base-x"; is not enough. Marek On Thu, 24 Jan 2019 17:48:05 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > yes, I wanted to set the cpu port mode in DTS at first, but if I > > understood the code correctly (maybe I made a mistake), the CPU port > > mode is not determined from DTS (since there is no phy). Am I > > wrong? > > It should work. I have boards which use it for DSA ports. I'm not sure > i have a board with it for a CPU port though. > > Andrew
On Thu, Jan 24, 2019 at 07:04:51PM +0100, Marek Behun wrote: > What properties does the cpu port node need to contain to force it? > phy-mode = "2500base-x"; is not enough. Hi Marek For DSA ports we have: phy-mode = "rgmii-txid"; fixed-link { speed = <1000>; full-duplex; }; See dsa_port_fixed_link_register_of() Andrew
On Thu, 24 Jan 2019 19:11:59 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > On Thu, Jan 24, 2019 at 07:04:51PM +0100, Marek Behun wrote: > > What properties does the cpu port node need to contain to force it? > > phy-mode = "2500base-x"; is not enough. > > Hi Marek > > For DSA ports we have: > > phy-mode = > "rgmii-txid"; fixed-link { > speed = > <1000>; full-duplex; > }; > > See dsa_port_fixed_link_register_of() > > Andrew Hi Andrew, the configuration phy-mode = "2500base-x"; fixed-link { speed = <2500>; full-duplex; }; does not work, because swphy does not support speed=2500 (only 10, 100 and 1000). managed = "in-band-status"; does not work either. If I use speed = <1000>, then the swphy is created correctly, cmode is set correctly to 2500base-x, but speed register on the port is set to 1000, and the connection does not work. The easiest way would probably be to implement swphy to support speed 2500. But I don't know what values should the simulated PHY registers contain... The function dsa_port_fixed_link_register_of creates this phy device, adjusts the link and then calls put_device(&phydev->mdio.dev); Does this mean that the phy device is immediately destroyed? Marek
On 1/24/19 11:24 AM, Marek Behun wrote: > On Thu, 24 Jan 2019 19:11:59 +0100 > Andrew Lunn <andrew@lunn.ch> wrote: > >> On Thu, Jan 24, 2019 at 07:04:51PM +0100, Marek Behun wrote: >>> What properties does the cpu port node need to contain to force it? >>> phy-mode = "2500base-x"; is not enough. >> >> Hi Marek >> >> For DSA ports we have: >> >> phy-mode = >> "rgmii-txid"; fixed-link { >> speed = >> <1000>; full-duplex; >> }; >> >> See dsa_port_fixed_link_register_of() >> >> Andrew > > Hi Andrew, > the configuration > phy-mode = "2500base-x"; > fixed-link { > speed = <2500>; > full-duplex; > }; > does not work, because swphy does not support speed=2500 (only 10, 100 > and 1000). > managed = "in-band-status"; > does not work either. > > If I use speed = <1000>, then the swphy is created correctly, cmode is > set correctly to 2500base-x, but speed register on the port is set to > 1000, and the connection does not work. > > The easiest way would probably be to implement swphy to support speed > 2500. But I don't know what values should the simulated PHY registers > contain... > > The function dsa_port_fixed_link_register_of creates this phy device, > adjusts the link and then calls put_device(&phydev->mdio.dev); > Does this mean that the phy device is immediately destroyed? Yes, we should actually migrate that code over to PHYLINK, because in PHYLINK the fixed links don't require creating a phy_device instance. This is something that has a potential of breaking a lot of people, so I have not really started doing it just yet :)
> > The function dsa_port_fixed_link_register_of creates this phy device, > > adjusts the link and then calls put_device(&phydev->mdio.dev); > > Does this mean that the phy device is immediately destroyed? > > Yes, we should actually migrate that code over to PHYLINK, because in > PHYLINK the fixed links don't require creating a phy_device instance. > This is something that has a potential of breaking a lot of people, so I > have not really started doing it just yet :) Hi Florian It also has the advantage of being easier to implement. In order to support 2.5G, i think we need C45 support, etc in the simulator. Avoiding that would be nice. Andrew
On Thu, 24 Jan 2019 11:27:54 -0800 Florian Fainelli <f.fainelli@gmail.com> wrote: > On 1/24/19 11:24 AM, Marek Behun wrote: > > On Thu, 24 Jan 2019 19:11:59 +0100 > > Andrew Lunn <andrew@lunn.ch> wrote: > > > >> On Thu, Jan 24, 2019 at 07:04:51PM +0100, Marek Behun wrote: > >>> What properties does the cpu port node need to contain to force > >>> it? phy-mode = "2500base-x"; is not enough. > >> > >> Hi Marek > >> > >> For DSA ports we have: > >> > >> phy-mode = > >> "rgmii-txid"; fixed-link { > >> speed = > >> <1000>; full-duplex; > >> }; > >> > >> See dsa_port_fixed_link_register_of() > >> > >> Andrew > > > > Hi Andrew, > > the configuration > > phy-mode = "2500base-x"; > > fixed-link { > > speed = <2500>; > > full-duplex; > > }; > > does not work, because swphy does not support speed=2500 (only 10, > > 100 and 1000). > > managed = "in-band-status"; > > does not work either. > > > > If I use speed = <1000>, then the swphy is created correctly, cmode > > is set correctly to 2500base-x, but speed register on the port is > > set to 1000, and the connection does not work. > > > > The easiest way would probably be to implement swphy to support > > speed 2500. But I don't know what values should the simulated PHY > > registers contain... > > > > The function dsa_port_fixed_link_register_of creates this phy > > device, adjusts the link and then calls > > put_device(&phydev->mdio.dev); Does this mean that the phy device > > is immediately destroyed? > > Yes, we should actually migrate that code over to PHYLINK, because in > PHYLINK the fixed links don't require creating a phy_device instance. > This is something that has a potential of breaking a lot of people, > so I have not really started doing it just yet :) phylink_create requires a net_device as first argument. what should be the device for cpu/dsa ports? Marek
On Thu, 24 Jan 2019 11:27:54 -0800 Florian Fainelli <f.fainelli@gmail.com> wrote: > On 1/24/19 11:24 AM, Marek Behun wrote: > > On Thu, 24 Jan 2019 19:11:59 +0100 > > Andrew Lunn <andrew@lunn.ch> wrote: > > > >> On Thu, Jan 24, 2019 at 07:04:51PM +0100, Marek Behun wrote: > >>> What properties does the cpu port node need to contain to force > >>> it? phy-mode = "2500base-x"; is not enough. > >> > >> Hi Marek > >> > >> For DSA ports we have: > >> > >> phy-mode = > >> "rgmii-txid"; fixed-link { > >> speed = > >> <1000>; full-duplex; > >> }; > >> > >> See dsa_port_fixed_link_register_of() > >> > >> Andrew > > > > Hi Andrew, > > the configuration > > phy-mode = "2500base-x"; > > fixed-link { > > speed = <2500>; > > full-duplex; > > }; > > does not work, because swphy does not support speed=2500 (only 10, > > 100 and 1000). > > managed = "in-band-status"; > > does not work either. > > > > If I use speed = <1000>, then the swphy is created correctly, cmode > > is set correctly to 2500base-x, but speed register on the port is > > set to 1000, and the connection does not work. > > > > The easiest way would probably be to implement swphy to support > > speed 2500. But I don't know what values should the simulated PHY > > registers contain... > > > > The function dsa_port_fixed_link_register_of creates this phy > > device, adjusts the link and then calls > > put_device(&phydev->mdio.dev); Does this mean that the phy device > > is immediately destroyed? > > Yes, we should actually migrate that code over to PHYLINK, because in > PHYLINK the fixed links don't require creating a phy_device instance. > This is something that has a potential of breaking a lot of people, > so I have not really started doing it just yet :) Can't then this patch be applied so that Turris Mox will work again? At least till the cpu/dsa port connection is converted to phylink. Marek
On 1/24/19 11:37 AM, Marek Behun wrote: > On Thu, 24 Jan 2019 11:27:54 -0800 > Florian Fainelli <f.fainelli@gmail.com> wrote: > >> On 1/24/19 11:24 AM, Marek Behun wrote: >>> On Thu, 24 Jan 2019 19:11:59 +0100 >>> Andrew Lunn <andrew@lunn.ch> wrote: >>> >>>> On Thu, Jan 24, 2019 at 07:04:51PM +0100, Marek Behun wrote: >>>>> What properties does the cpu port node need to contain to force >>>>> it? phy-mode = "2500base-x"; is not enough. >>>> >>>> Hi Marek >>>> >>>> For DSA ports we have: >>>> >>>> phy-mode = >>>> "rgmii-txid"; fixed-link { >>>> speed = >>>> <1000>; full-duplex; >>>> }; >>>> >>>> See dsa_port_fixed_link_register_of() >>>> >>>> Andrew >>> >>> Hi Andrew, >>> the configuration >>> phy-mode = "2500base-x"; >>> fixed-link { >>> speed = <2500>; >>> full-duplex; >>> }; >>> does not work, because swphy does not support speed=2500 (only 10, >>> 100 and 1000). >>> managed = "in-band-status"; >>> does not work either. >>> >>> If I use speed = <1000>, then the swphy is created correctly, cmode >>> is set correctly to 2500base-x, but speed register on the port is >>> set to 1000, and the connection does not work. >>> >>> The easiest way would probably be to implement swphy to support >>> speed 2500. But I don't know what values should the simulated PHY >>> registers contain... >>> >>> The function dsa_port_fixed_link_register_of creates this phy >>> device, adjusts the link and then calls >>> put_device(&phydev->mdio.dev); Does this mean that the phy device >>> is immediately destroyed? >> >> Yes, we should actually migrate that code over to PHYLINK, because in >> PHYLINK the fixed links don't require creating a phy_device instance. >> This is something that has a potential of breaking a lot of people, >> so I have not really started doing it just yet :) > > phylink_create requires a net_device as first argument. what should be > the device for cpu/dsa ports? We would have to somehow change that assumption or introduce some low level function for PHYLINK that are just meant to deal with the phylink object, without the netdev, that is among other things why it has not happened yet.
On 1/24/19 11:39 AM, Marek Behun wrote: > On Thu, 24 Jan 2019 11:27:54 -0800 > Florian Fainelli <f.fainelli@gmail.com> wrote: > >> On 1/24/19 11:24 AM, Marek Behun wrote: >>> On Thu, 24 Jan 2019 19:11:59 +0100 >>> Andrew Lunn <andrew@lunn.ch> wrote: >>> >>>> On Thu, Jan 24, 2019 at 07:04:51PM +0100, Marek Behun wrote: >>>>> What properties does the cpu port node need to contain to force >>>>> it? phy-mode = "2500base-x"; is not enough. >>>> >>>> Hi Marek >>>> >>>> For DSA ports we have: >>>> >>>> phy-mode = >>>> "rgmii-txid"; fixed-link { >>>> speed = >>>> <1000>; full-duplex; >>>> }; >>>> >>>> See dsa_port_fixed_link_register_of() >>>> >>>> Andrew >>> >>> Hi Andrew, >>> the configuration >>> phy-mode = "2500base-x"; >>> fixed-link { >>> speed = <2500>; >>> full-duplex; >>> }; >>> does not work, because swphy does not support speed=2500 (only 10, >>> 100 and 1000). >>> managed = "in-band-status"; >>> does not work either. >>> >>> If I use speed = <1000>, then the swphy is created correctly, cmode >>> is set correctly to 2500base-x, but speed register on the port is >>> set to 1000, and the connection does not work. >>> >>> The easiest way would probably be to implement swphy to support >>> speed 2500. But I don't know what values should the simulated PHY >>> registers contain... >>> >>> The function dsa_port_fixed_link_register_of creates this phy >>> device, adjusts the link and then calls >>> put_device(&phydev->mdio.dev); Does this mean that the phy device >>> is immediately destroyed? >> >> Yes, we should actually migrate that code over to PHYLINK, because in >> PHYLINK the fixed links don't require creating a phy_device instance. >> This is something that has a potential of breaking a lot of people, >> so I have not really started doing it just yet :) > > Can't then this patch be applied so that Turris Mox will work again? At > least till the cpu/dsa port connection is converted to phylink. How about the following hack until we can support netdev less PHYLINK instances while having the Device Tree being corrected to have the correct phy-mode = "2500base-x" property since we will need it for PHYLINK on CPU/DSA ports later on: diff --git a/net/dsa/port.c b/net/dsa/port.c index 2d7e01b23572..9ea052c30b68 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -359,6 +359,14 @@ static int dsa_port_fixed_link_register_of(struct dsa_port *dp) if (mode < 0) mode = PHY_INTERFACE_MODE_NA; phydev->interface = mode; + switch (mode) { + case PHY_INTERFACE_MODE_2500BASEX: + phydev->speed = SPEED_2500; + break; + case PHY_INTERFACE_MODE_10GKR: + phydev->speed = SPEED_10000; + break; + } genphy_config_init(phydev); genphy_read_status(phydev);
On Fri, 25 Jan 2019 10:48:38 -0800 Florian Fainelli <f.fainelli@gmail.com> wrote: > On 1/24/19 11:39 AM, Marek Behun wrote: > > On Thu, 24 Jan 2019 11:27:54 -0800 > > Florian Fainelli <f.fainelli@gmail.com> wrote: > > > >> On 1/24/19 11:24 AM, Marek Behun wrote: > >>> On Thu, 24 Jan 2019 19:11:59 +0100 > >>> Andrew Lunn <andrew@lunn.ch> wrote: > >>> > >>>> On Thu, Jan 24, 2019 at 07:04:51PM +0100, Marek Behun wrote: > >>>>> What properties does the cpu port node need to contain to force > >>>>> it? phy-mode = "2500base-x"; is not enough. > >>>> > >>>> Hi Marek > >>>> > >>>> For DSA ports we have: > >>>> > >>>> phy-mode = > >>>> "rgmii-txid"; fixed-link { > >>>> speed = > >>>> <1000>; full-duplex; > >>>> }; > >>>> > >>>> See dsa_port_fixed_link_register_of() > >>>> > >>>> Andrew > >>> > >>> Hi Andrew, > >>> the configuration > >>> phy-mode = "2500base-x"; > >>> fixed-link { > >>> speed = <2500>; > >>> full-duplex; > >>> }; > >>> does not work, because swphy does not support speed=2500 (only 10, > >>> 100 and 1000). > >>> managed = "in-band-status"; > >>> does not work either. > >>> > >>> If I use speed = <1000>, then the swphy is created correctly, > >>> cmode is set correctly to 2500base-x, but speed register on the > >>> port is set to 1000, and the connection does not work. > >>> > >>> The easiest way would probably be to implement swphy to support > >>> speed 2500. But I don't know what values should the simulated PHY > >>> registers contain... > >>> > >>> The function dsa_port_fixed_link_register_of creates this phy > >>> device, adjusts the link and then calls > >>> put_device(&phydev->mdio.dev); Does this mean that the phy device > >>> is immediately destroyed? > >> > >> Yes, we should actually migrate that code over to PHYLINK, because > >> in PHYLINK the fixed links don't require creating a phy_device > >> instance. This is something that has a potential of breaking a lot > >> of people, so I have not really started doing it just yet :) > > > > Can't then this patch be applied so that Turris Mox will work > > again? At least till the cpu/dsa port connection is converted to > > phylink. > > How about the following hack until we can support netdev less PHYLINK > instances while having the Device Tree being corrected to have the > correct phy-mode = "2500base-x" property since we will need it for > PHYLINK on CPU/DSA ports later on: > > diff --git a/net/dsa/port.c b/net/dsa/port.c > index 2d7e01b23572..9ea052c30b68 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -359,6 +359,14 @@ static int dsa_port_fixed_link_register_of(struct > dsa_port *dp) > if (mode < 0) > mode = PHY_INTERFACE_MODE_NA; > phydev->interface = mode; > + switch (mode) { > + case PHY_INTERFACE_MODE_2500BASEX: > + phydev->speed = SPEED_2500; > + break; > + case PHY_INTERFACE_MODE_10GKR: > + phydev->speed = SPEED_10000; > + break; > + } > > genphy_config_init(phydev); > genphy_read_status(phydev); But in the dts there would still have to be speed=<1000>; otherwise swphy will fail...
On 1/24/19 7:43 AM, Marek Behún wrote: > Commit 787799a9d555 sets the SERDES interfaces of 6390 and 6390X to > 1000BaseX, but this is only needed on 6390X, since there are SERDES > interfaces which can be used on lower ports on 6390. > > This commit fixes this by returning to previous behaviour on 6390. > (Previous behaviour means that CMODE is not set at all if requested mode > is NA). > > This is needed on Turris MOX, where the 88e6190 is connected to CPU in > 2500BaseX mode. > > Fixes: 787799a9d555 ("net: dsa: mv88e6xxx: Default ports 9/10 6390X CMODE to 1000BaseX") > Signed-off-by: Marek Behún <marek.behun@nic.cz> I suppose for now, this is the best way to approach that problem given the shortcomings of the fixed link support in net/dsa/port.c: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Thanks!
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c index ebd26b6a93e6..ee7029f4ee22 100644 --- a/drivers/net/dsa/mv88e6xxx/port.c +++ b/drivers/net/dsa/mv88e6xxx/port.c @@ -444,6 +444,8 @@ int mv88e6390_port_set_cmode(struct mv88e6xxx_chip *chip, int port, phy_interface_t mode) { switch (mode) { + case PHY_INTERFACE_MODE_NA: + return 0; case PHY_INTERFACE_MODE_XGMII: case PHY_INTERFACE_MODE_XAUI: case PHY_INTERFACE_MODE_RXAUI:
Commit 787799a9d555 sets the SERDES interfaces of 6390 and 6390X to 1000BaseX, but this is only needed on 6390X, since there are SERDES interfaces which can be used on lower ports on 6390. This commit fixes this by returning to previous behaviour on 6390. (Previous behaviour means that CMODE is not set at all if requested mode is NA). This is needed on Turris MOX, where the 88e6190 is connected to CPU in 2500BaseX mode. Fixes: 787799a9d555 ("net: dsa: mv88e6xxx: Default ports 9/10 6390X CMODE to 1000BaseX") Signed-off-by: Marek Behún <marek.behun@nic.cz> --- drivers/net/dsa/mv88e6xxx/port.c | 2 ++ 1 file changed, 2 insertions(+)