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
