Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

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
>