Re: [PATCHv1 1/6] net: dsa: Support internal phy on 'cpu' port

[Sebastian Reichel]

Hi Andrew,

On Wed, Jan 03, 2018 at 02:21:28PM +0100, Andrew Lunn wrote:
> On Wed, Jan 03, 2018 at 01:26:04PM +0100, Sebastian Reichel wrote:
> > This adds support for enabling the internal phy for a 'cpu' port.
> > It has been tested on GE B850v3 and B650v3, which have a built-in
> > MV88E6240 switch connected to a PCIe based network card. Without
> > this patch the link does not come up and no traffic can be routed
> > through the switch.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxxxx>
> > ---
> >  net/dsa/port.c | 26 ++++++++++++++++++++++----
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > index bb4be2679904..f99c1d34416c 100644
> > --- a/net/dsa/port.c
> > +++ b/net/dsa/port.c
> > @@ -282,6 +282,10 @@ int dsa_port_fixed_link_register_of(struct dsa_port *dp)
> >  	int mode;
> >  	int err;
> >  
> > +	mode = of_get_phy_mode(dn);
> > +	if (mode < 0)
> > +		mode = PHY_INTERFACE_MODE_NA;
> > +
> >  	if (of_phy_is_fixed_link(dn)) {
> >  		err = of_phy_register_fixed_link(dn);
> >  		if (err) {
> > @@ -292,10 +296,6 @@ int dsa_port_fixed_link_register_of(struct dsa_port *dp)
> >  		}
> >  
> >  		phydev = of_phy_find_device(dn);
> > -
> > -		mode = of_get_phy_mode(dn);
> > -		if (mode < 0)
> > -			mode = PHY_INTERFACE_MODE_NA;
> >  		phydev->interface = mode;
> >  
> >  		genphy_config_init(phydev);
> > @@ -305,6 +305,24 @@ int dsa_port_fixed_link_register_of(struct dsa_port *dp)
> >  			ds->ops->adjust_link(ds, port, phydev);
> >  
> >  		put_device(&phydev->mdio.dev);
> > +	} else if (mode == PHY_INTERFACE_MODE_INTERNAL ||
> > +		   mode == PHY_INTERFACE_MODE_NA) {
> 
> Hi Sebastian
> 
> I understand what you are trying to do, i've got boards which also
> have back-to-back PHYs for the CPU port. These boards however have the
> strapping correct, so nothing needs doing in software.

What I have is a PCIe intel network card with phy, that is wired to a
mv88e6240 switch. The network card is exposed as normal network device,
so phy is enabled when the interface is brought up. The 'cpu' port
for mv88e6240 has an integrated phy, that needs to be enabled.

Your boards must be different, since mv88e6xxx is being reset during
probe(). So even if the 'cpu' phy was enabled before driver probe(),
it would be disabled afterwards.

> But the way you are doing it is wrong. PHY_INTERFACE_MODE_NA means
> something else has already setup the interface mode, leave it alone.

Ok, I assumed, that PHY_INTERFACE_MODE_NA means "no explicit
configuration found, use implicit configuration". E.g. for
mv88e6xxx the downstream ports are not configured in DT, but
their PHY is enabled.

> PHY_INTERFACE_MODE_INTERNAL means there is some other sort of bus
> between the MAC and the PHY than the normal MII.
> 
> What you want to say is that there is a PHY on this port, and that you
> want to configure it to a given fixed configuration, probably 1000
> Full, with auto-neg turned off. This is something completely different
> to a fixed phy, which is used when there is no PHY at all.

That's why I put the new code into

if (of_phy_is_fixed_link(...)) {
    <<< old code >>>
} else {
    <<< new code >>>
}

I agree, that the function name dsa_port_fixed_link_register_of() is
a bit confusing with the added code. I actually added this to
dsa_cpu_dsa_setup() and with the rebase to current master it ended
up there.

> What state is the PHY in, if you don't have this patch? Is it powered
> down?

The phy is part of mv88e6240, which is being reset during probe.
So the phy is powered down and DSA is not functional except for
phy information of downstream ports. The PCIe network interface
does not detect a carrier.

-- Sebastian

Attachment: signature.asc
Description: PGP signature