>> + if (change_interface) {
>> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
>> + gem_writel(bp, NCFGR, ~GEM_BIT(SGMIIEN) &
>> + ~GEM_BIT(PCSSEL) &
>> + gem_readl(bp, NCFGR));
>> + gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) &
>> + gem_readl(bp, NCR));
>> + gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
>> + GEM_BIT(PCS_CTRL_RST));
>> + }
>I still don't think this makes much sense, splitting the interface
>configuration between here and below.
Do you mean splitting mac_config in two *_configure functions ?
This was done as per Andrew's suggestion to make code mode readable
and easy to manage by splitting MAC configuration for different interfaces.
>> + bp->phy_interface = state->interface;
>> + }
>> +
>> if (!phylink_autoneg_inband(mode) &&
>> (bp->speed != state->speed ||
>> - bp->duplex != state->duplex)) {
>> + bp->duplex != state->duplex ||
>> + change_interface)) {
>> u32 reg;
>>
>> reg = macb_readl(bp, NCFGR);
>> reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
>> if (macb_is_gem(bp))
>> reg &= ~GEM_BIT(GBE);
>> + macb_or_gem_writel(bp, NCFGR, reg);
>> +
>> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>> + gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) |
>> + GEM_BIT(PCSSEL) |
>> + gem_readl(bp, NCFGR));
>This will only be executed when we are not using inband mode, which
>basically means it's not possible to switch to SGMII in-band mode.
SGMII is used in default PHY mode. And above code is to program MAC to
select PCS and SGMII interface.
>> +
>> + if (!interface_supported) {
>> + netdev_err(dev, "Phy mode %s not supported",
>> + phy_modes(phy_mode));
>> + goto err_out_free_netdev;
>> + }
>> +
>> bp->phy_interface = phy_mode;
>> + } else {
>> + bp->phy_interface = phy_mode;
>> + }
>If bp->phy_interface is PHY_INTERFACE_MODE_SGMII here, and mac_config()
>is called with state->interface = PHY_INTERFACE_MODE_SGMII, then
>mac_config() won't configure the MAC for the interface type - is that
>intentional?
In mac_config configure MAC for non in-band mode, there is also check for
speed, duplex
changes. bp->speed and bp->duplex are initialized to SPEED_UNKNOWN and
DUPLEX_UNKNOWN
values so it is expected that for non in band mode state contains valid speed
and duplex mode
which are different from *_UNKNOWN values.
Regards,
Parshuram Thombare
