On 8/23/21 3:14 AM, Andrew Lunn wrote:
>> Just to clarify, this means I should set the physical port number in the
>> reg field of the device tree? i.e.:
>>
>> port@4 {
>> reg = <6>; /* <--- instead of <4>? */
>> label = "cpu";
>> ethernet = <&fec1>;
>> phy-mode = "rgmii-id";
>> fixed-link {
>> speed = <1000>;
>> full-duplex;
>> pause;
>> };
>> };
>
> Yes, this is fine.
>
>>>> +static int rtl8365mb_port_enable(struct dsa_switch *ds, int port,
>>>> + struct phy_device *phy)
>>>> +{
>>>> + struct realtek_smi *smi = ds->priv;
>>>> + int val;
>>>> +
>>>> + if (dsa_is_user_port(ds, port)) {
>>>> + /* Power up the internal PHY and restart autonegotiation */
>>>> + val = rtl8365mb_phy_read(smi, port, MII_BMCR);
>>>> + if (val < 0)
>>>> + return val;
>>>> +
>>>> + val &= ~BMCR_PDOWN;
>>>> + val |= BMCR_ANRESTART;
>>>> +
>>>> + return rtl8365mb_phy_write(smi, port, MII_BMCR, val);
>>>> + }
>>>
>>> There should not be any need to do this. phylib should do it. In
>>> generally, you should not need to touch any PHY registers, you just
>>> need to allow access to the PHY registers.
>>
>> Will phylib also the opposite when the interface is administratively put
>> down (e.g. ip link set dev swp2 down)? The switch doesn't seem to have
>> any other handle for stopping the flow of packets from a port except to
>> power down the internal PHY, hence why I put this in for port_disable.
>> For port_enable I just did the opposite but I can see why it's not
>> necessary.
>
> When the MAC and PHY are attached phy_attach_direct() gets called,
> which calls phy_resume(phydev); The generic implementation clears the
> BMCR_PDOWN bit.
>
> phy_detach() will suspend the PHY, which sets the BMCR_PDOWN bit.
>
> But there are two different schemes for doing this. Some MAC drivers
> connect the PHY during probe. Others do it at open. DSA does it at
> probe. So this is generic feature is not going to work for you. You
> might want to put some printk() in phy_suspend and phy_resume to check
> that.
Thanks, I will double check.
>
> So, setting/clearing the PDOWN bit does seems reasonable. But please
> document in the functions why you are doing this. Also, don't restart
> autoneg. That really should be up to phylib, and it should be
> triggered by phylink_start() which should be called directly after
> port_enable().
Understood. I'll review the necessity of these calls and add a comment
in v2 if I keep them.
>
> Andrew
>
