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:

quoted

+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.

quoted

+
+     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

quoted

+}
+
+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.

quoted

+
+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.

quoted

+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 ?

quoted

+}
+
+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