[PATCH v3 4/6] drivers: net: add Felix DSA switch driver

[Alexandru Marginean]

On 12/15/2019 1:53 PM, Vladimir Oltean wrote:
> Hi Alex,
> 
> On Tue, 3 Dec 2019 at 18:18, Alex Marginean <alexandru.marginean at nxp.com> wrote:
>> +static int felix_port_enable(struct udevice *dev, int port,
>> +                            struct phy_device *phy)
>> +{
>> +       struct felix_priv *priv = dev_get_priv(dev);
>> +       void *base = priv->regs_base;
>> +
>> +       out_le32(base + FELIX_GMII_MAC_ENA_CFG(port),
>> +                FELIX_GMII_MAX_ENA_CFG_TX | FELIX_GMII_MAX_ENA_CFG_RX);
>> +
>> +       out_le32(base + FELIX_QSYS_SYSTEM_SW_PORT_MODE(port),
>> +                FELIX_QSYS_SYSTEM_SW_PORT_ENA |
>> +                FELIX_QSYS_SYSTEM_SW_PORT_LOSSY |
>> +                FELIX_QSYS_SYSTEM_SW_PORT_SCH(1));
>> +
>> +       if (phy)
>> +               phy_startup(phy);
>> +       return 0;
>> +}
>> +
>> +static void felix_port_disable(struct udevice *dev, int port,
>> +                              struct phy_device *phy)
>> +{
>> +       struct felix_priv *priv = dev_get_priv(dev);
>> +       void *base = priv->regs_base;
>> +
>> +       out_le32(base + FELIX_GMII_MAC_ENA_CFG(port), 0);
>> +
>> +       out_le32(base + FELIX_QSYS_SYSTEM_SW_PORT_MODE(port),
>> +                FELIX_QSYS_SYSTEM_SW_PORT_LOSSY |
>> +                FELIX_QSYS_SYSTEM_SW_PORT_SCH(1));
>> +
>> +       /*
>> +        * we don't call phy_shutdown here to avoind waiting next time we use
>> +        * the port, but the downside is that remote side will think we're
>> +        * actively processing traffic although we are not.
>> +        */
>> +}
>> --
>> 2.17.1
>>
> 
> What is the correct general procedure here, is it to call phy_startup
> so late (felix_port_enable)? I'm trying to take this driver as an
> example for sja1105, which has RGMII so PCS and no autonomous in-band
> AN like felix does. On this switch, it is too late to do phy_startup
> now.

Why is it too late?  Is it a functional problem, or you're looking to 
reduce the waiting time?

> Instead, I would need to look at phy->speed which should have
> been settled by now, and reprogram my MAC with it.
> My question is: don't you think phy_startup() and phy_shutdown()
> belong in the DSA uclass code?
> 
> Thanks,
> -Vladimir
> 

The API is similar to the one in Linux, plus I didn't want to force a 
specific PHY related behavior to drivers.  Sometimes it's fine to start 
up the PHYs at probe and then just send/receive as needed, but that 
behavior is not always acceptable.  Assume there is some other host 
connected to one of the front panel ports, if it sees link up it may 
start doing DHCP, IPv6 discovery.  I think it's generally better to keep 
links down unless the port is actually in use for the benefit of the 
remote end.  I didn't want to force this by moving the PHY calls to 
uclass code.  This approach is also similar to eth uclass, it doesn't 
handle phy calls for the driver either.

Alex