Re: [PATCH net-next V2 3/4] net: lan743x: Migrate phylib to phylink

[Raju Lakkaraju]

Hi Russell King,

Thank you for review the patches.

The 07/29/2024 10:16, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Jul 16, 2024 at 05:03:48PM +0530, Raju Lakkaraju wrote:
> > +static void lan743x_phylink_mac_link_up(struct phylink_config *config,
> > +                                     struct phy_device *phydev,
> > +                                     unsigned int link_an_mode,
> > +                                     phy_interface_t interface,
> > +                                     int speed, int duplex,
> > +                                     bool tx_pause, bool rx_pause)
> > +{
> > +     struct net_device *netdev = to_net_dev(config->dev);
> > +     struct lan743x_adapter *adapter = netdev_priv(netdev);
> > +     int mac_cr;
> > +     u8 cap;
> > +
> > +     mac_cr = lan743x_csr_read(adapter, MAC_CR);
> > +     /* Pre-initialize register bits.
> > +      * Resulting value corresponds to SPEED_10
> > +      */
> > +     mac_cr &= ~(MAC_CR_CFG_H_ | MAC_CR_CFG_L_);
> > +     if (speed == SPEED_2500)
> > +             mac_cr |= (MAC_CR_CFG_H_ | MAC_CR_CFG_L_);
> > +     else if (speed == SPEED_1000)
> > +             mac_cr |= (MAC_CR_CFG_H_);
> > +     else if (speed == SPEED_100)
> > +             mac_cr |= (MAC_CR_CFG_L_);
> 
> These parens in each of these if() sub-blocks is not required. |=
> operates the same way as = - all such operators are treated the same
> in C.

Accpeted. I will fix.

> 
> > +
> > +     lan743x_csr_write(adapter, MAC_CR, mac_cr);
> > +
> > +     lan743x_ptp_update_latency(adapter, speed);
> > +
> > +     /* Flow Control operation */
> > +     cap = 0;
> > +     if (tx_pause)
> > +             cap |= FLOW_CTRL_TX;
> > +     if (rx_pause)
> > +             cap |= FLOW_CTRL_RX;
> > +
> > +     lan743x_mac_flow_ctrl_set_enables(adapter,
> > +                                       cap & FLOW_CTRL_TX,
> > +                                       cap & FLOW_CTRL_RX);
> > +
> > +     netif_tx_wake_all_queues(to_net_dev(config->dev));
> 
> You already have "netdev", so there's no need to do the to_net_dev()
> dance again here.

Accepted. I will fix
> 
> > +}
> > +
> > +static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
> > +     .mac_config = lan743x_phylink_mac_config,
> > +     .mac_link_down = lan743x_phylink_mac_link_down,
> > +     .mac_link_up = lan743x_phylink_mac_link_up,
> > +};
> 
> I guess as there's no PCS support here, you don't support inband mode
> for 1000base-X (which is rather fundamental for it).
> 

Initially, I add PHYLINK and SFP support changes in one patch series.
Due to too many changes, I split in 2 set of patches (i.e. PHYLINK and SFP
support).
In SFP support patch series, I would like to use Synopsys Designware's XPCS
driver as PCS support. Those changes are ready to submit for code review after
this patch series accept.
Those changes support 2500basex-x, 1000base-x along with SGMII Interfaces.

> > +
> > +static int lan743x_phylink_create(struct net_device *netdev)
> > +{
> > +     struct lan743x_adapter *adapter = netdev_priv(netdev);
> > +     struct phylink *pl;
> > +
> > +     adapter->phylink_config.dev = &netdev->dev;
> > +     adapter->phylink_config.type = PHYLINK_NETDEV;
> > +     adapter->phylink_config.mac_managed_pm = false;
> > +
> > +     adapter->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
> > +             MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD;
> > +
> > +     lan743x_phy_interface_select(adapter);
> > +
> > +     switch (adapter->phy_interface) {
> > +     case PHY_INTERFACE_MODE_SGMII:
> > +             __set_bit(PHY_INTERFACE_MODE_SGMII,
> > +                       adapter->phylink_config.supported_interfaces);
> > +             __set_bit(PHY_INTERFACE_MODE_1000BASEX,
> > +                       adapter->phylink_config.supported_interfaces);
> > +             __set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > +                       adapter->phylink_config.supported_interfaces);
> > +             break;
> > +     case PHY_INTERFACE_MODE_GMII:
> > +             __set_bit(PHY_INTERFACE_MODE_GMII,
> > +                       adapter->phylink_config.supported_interfaces);
> > +             break;
> > +     case PHY_INTERFACE_MODE_MII:
> > +             __set_bit(PHY_INTERFACE_MODE_MII,
> > +                       adapter->phylink_config.supported_interfaces);
> > +             break;
> > +     default:
> > +             __set_bit(PHY_INTERFACE_MODE_RGMII,
> > +                       adapter->phylink_config.supported_interfaces);
> 
> Do you really only support RGMII and not RGMII_ID/RGMII_TXID/RGMII_RXID
> (which are normally implemented by tweaking the delays at the PHY end
> of the RGMII link) ?

Accepted.
Microchip's KSZ9131 PHY support RGMII_ID/RGMII_TXID/RGMII_RXID. I will fix.

> 
> > +static bool lan743x_phy_handle_exists(struct device_node *dn)
> > +{
> > +     dn = of_parse_phandle(dn, "phy-handle", 0);
> > +     of_node_put(dn);
> > +     if (IS_ERR(dn))
> > +             return false;
> > +
> > +     return true;
> 
> This likely doesn't work. Have you checked what the return values for
> of_parse_phandle() actually are before creating this, because to me
> this looks like you haven't - and thus what you've created is wrong.
> of_parse_phandle() doesn't return error-pointers when it fails, it
> returns NULL. Therefore, this will always return true.
> 
> We have another implementation of something very similar in the macb
> driver - see macb_phy_handle_exists(), and this one is implemented
> correctly.

Ok. 
After change, i ran the checkpatch script. it's giving follwoing warning
i.e.
"CHECK: Comparison to NULL could be written "dn"" 

Is it OK ?

> 
> > +}
> > +
> > +static int lan743x_phylink_connect(struct lan743x_adapter *adapter)
> > +{
> > +     struct device_node *dn = adapter->pdev->dev.of_node;
> > +     struct net_device *dev = adapter->netdev;
> > +     struct fixed_phy_status fphy_status = {
> > +             .link = 1,
> > +             .speed = SPEED_1000,
> > +             .duplex = DUPLEX_FULL,
> > +     };
> > +     struct phy_device *phydev;
> > +     int ret;
> > +
> > +     if (dn)
> > +             ret = phylink_of_phy_connect(adapter->phylink, dn, 0);
> > +
> > +     if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
> > +             phydev = phy_find_first(adapter->mdiobus);
> > +             if (!phydev) {
> > +                     if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
> > +                           ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
> > +                             phydev = fixed_phy_register(PHY_POLL,
> > +                                                         &fphy_status,
> > +                                                         NULL);
> > +                             if (IS_ERR(phydev)) {
> > +                                     netdev_err(dev, "No PHY/fixed_PHY found\n");
> > +                                     return PTR_ERR(phydev);
> > +                             }
> 
> Eww. Given that phylink has its own internal fixed-PHY support, can we
> not find some way to avoid the legacy fixed-PHY usage here?

Yes. I agree with you. This is very much valid suggestion.
Andrew also gave same suggestion.

Currently we don't have Device Tree support for LAN743X driver. 
For SFP support, I create the software-node an passing the paramters there.

I don't have fixed-PHY hardware setup currently.
I would like to take this as action item to fix it after SFP support commits.

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

-- 
Thanks,                                                                         
Raju