RE: [PATCH v2 1/5] net: macb: add phylink support

[Parshuram Raju Thombare <pthombar@cadence.com>]

Hi Russel,

Thanks for review comments.

>On Wed, Jun 19, 2019 at 09:40:36AM +0100, Parshuram Thombare wrote:
>

>> +	bitmap_and(supported, supported, mask,
>__ETHTOOL_LINK_MODE_MASK_NBITS);
>
>> +	bitmap_and(state->advertising, state->advertising, mask,
>
>> +		   __ETHTOOL_LINK_MODE_MASK_NBITS);
>
>
>
>Consider using linkmode_and() here.
>
Ok

>> +static int gem_phylink_mac_link_state(struct phylink_config *pl_config,
>
>> +				      struct phylink_link_state *state)
>
>> +	state->speed = bp->speed;
>
>> +	state->duplex = bp->duplex;
>
>> +	state->link = bp->link;
>
>
>
>You can't read from the hardware what the actual MAC is doing?

As  mostly PHY mode was used in driver and this method is called only for
in band mode, I added this is just as place holder and used in next patch set
where SGMII support is added. Should I remove it from this patch ?


>> +static void gem_mac_config(struct phylink_config *pl_config, unsigned int
>mode,
>> +			   const struct phylink_link_state *state)
>
>> +{
>> +	if (bp->speed != state->speed ||
>
>> +	    bp->duplex != state->duplex) {
>
>Please read the updated phylink documentation - state->{speed,duplex}
>are not always valid depending on the negotiation mode.

At least for PHY and FIXED mode I see mac_config is called after state is updated in phylink_resolve().
In case of IN BAND mode, I see mac_config may not get called after state is updated in mac_link_state()
method. Are you suggesting to configure MAC here only for FIXED and PHY mode ?


>> +	bp->pl_config.type = PHYLINK_NETDEV;
>
>> +	bp->pl = phylink_create(&bp->pl_config, of_fwnode_handle(np),
>
>> +				bp->phy_interface, &gem_phylink_ops);
>
>> +	if (IS_ERR(bp->pl)) {
>
>> +		netdev_err(dev,
>
>> +			   "error creating PHYLINK: %ld\n", PTR_ERR(bp->pl));
>
>> +		return PTR_ERR(bp->pl);
>
>>  	}
>
>At this point bp->pl can never be NULL.

phylink_create() does return failure also. I think this comment is not for above snippet.

>
>> -	if (!dev->phydev) {
>
>> +	if (!bp->pl) {
>
>
>
>So this check is unnecessary.
>

Ok, I will remove this check.

>> -	if (dev->phydev)
>
>> -		phy_stop(dev->phydev);
>
>> +	if (bp->pl)
>
>> +		phylink_stop(bp->pl);
>
>
>
>Ditto.

Ok, I will remove this redundant check.


>> +	if (!bp->pl)
>
>> +		return -ENOTSUPP;
>
>
>
>Ditto.

Ok, I will remove this redundant check.

>
>> +	if (!bp->pl)
>
>> +		return -ENOTSUPP;
>
>
>
>Ditto.

Ok, I will remove this redundant check.

>
>> +	if (!bp->pl)
>
>>  		return -ENODEV;
>
>
>
>Ditto.

Ok, I will remove this redundant check.


>> @@ -4183,13 +4219,12 @@ static int macb_probe(struct platform_device
>*pdev)
>
>>  	struct clk *tsu_clk = NULL;
>
>>  	unsigned int queue_mask, num_queues;
>> +	phy_mode = of_get_phy_mode(np);
>
>> +	if (phy_mode < 0)
>
>>  		/* not found in DT, MII by default */
>
>>  		bp->phy_interface = PHY_INTERFACE_MODE_MII;
>
>>  	else
>
>> -		bp->phy_interface = err;
>
>> +		bp->phy_interface = phy_mode;
>
>The phy interface mode is managed by phylink - and there are phys out
>there that dynamically change their link mode.  You may wish to update
>the link mode in your mac_config() implementation too.
>
Ok, I will modify mac_config to check phy_mode and program MAC accordingly.


>> +	if (dev->phydev)
>
>> +		phy_attached_info(dev->phydev);[] 
>
>phylink already prints information about the attached phy, why do we
>need another print here?
>

Ok, I will remove this.

>> -		phy_stop(netdev->phydev);
>> -		phy_suspend(netdev->phydev);
>> +		phylink_stop(bp->pl);
>> +		if (netdev->phydev)
>> +			phy_suspend(netdev->phydev);
>
>When the attached phy is stopped, the state machine suspends the phy.
>Why do we need an explicit call to phy_suspend() here, bypassing
>phylink?
>

Here I am just trying to keep functionality unchanged, just replacing 
Phylib API's with phylink API's.

>> -		phy_resume(netdev->phydev);
>
>> -		phy_init_hw(netdev->phydev);
>
>> -		phy_start(netdev->phydev);
>
>> +		if (netdev->phydev) {
>
>> +			phy_resume(netdev->phydev);
>
>> +			phy_init_hw(netdev->phydev);
>
>> +		}
>
>> +		phylink_start(bp->pl);
>
>
>
>When the phy is started, the phy state machine will resume the phy.
>Same question as above.
>

Here I am just trying to keep functionality unchanged, just replacing 
Phylib API's with phylink API's.


Regards,
Parshuram Thombare