RE: [PATCH net-next 3/3] net: phy: micrel: 1588 support for LAN8814 phy

[Divya.Koppera]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Friday, March 4, 2022 6:36 PM
> To: Divya Koppera - I30481 <Divya.Koppera@xxxxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; hkallweit1@xxxxxxxxx; linux@xxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; richardcochran@xxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>;
> Madhuri Sripada - I34878 <Madhuri.Sripada@xxxxxxxxxxxxx>; Manohar Puri -
> I30488 <Manohar.Puri@xxxxxxxxxxxxx>
> Subject: Re: [PATCH net-next 3/3] net: phy: micrel: 1588 support for LAN8814
> phy
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > +static struct kszphy_latencies lan8814_latencies = {
> > +     .rx_10          = 0x22AA,
> > +     .tx_10          = 0x2E4A,
> > +     .rx_100         = 0x092A,
> > +     .tx_100         = 0x02C1,
> > +     .rx_1000        = 0x01AD,
> > +     .tx_1000        = 0x00C9,
> > +};
> 
> Seems odd to use hex here. Are these the defaults? At minimum, you need to
> add these to the binding document, making it clear what defaults are used.
> Also, what are the unit here?

Yes Andrew, these are default values. Richard too mentioned about this as below
"However, DTS is probably the wrong place.  The linuxptp user space stack has configuration variables for this purpose:"
I will check regarding this and will come with fix in next patch if its applicable.

> 
> > +     /* Make sure the PHY is not broken. Read idle error count,
> > +      * and reset the PHY if it is maxed out.
> > +      */
> > +     regval = phy_read(phydev, MII_STAT1000);
> > +     if ((regval & 0xFF) == 0xFF) {
> > +             phy_init_hw(phydev);
> > +             phydev->link = 0;
> > +             if (phydev->drv->config_intr && phy_interrupt_is_valid(phydev))
> > +                     phydev->drv->config_intr(phydev);
> > +             return genphy_config_aneg(phydev);
> > +     }
> 
> Is this related to PTP? Or is the PHY broken in general? This looks like it should
> be something submitted to stable.
>

Previously lan8814 phy used kszphy_read_status, we have reused the same function and added new
 Changes related to latency with new function lan8814_read_status.
 
> > +static int lan8814_config_init(struct phy_device *phydev) {
> > +     int val;
> > +
> > +     /* Reset the PHY */
> > +     val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
> > +     val |= LAN8814_QSGMII_SOFT_RESET_BIT;
> > +     lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET,
> > + val);
> > +
> > +     /* Disable ANEG with QSGMII PCS Host side */
> > +     val = lanphy_read_page_reg(phydev, 5,
> LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
> > +     val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
> > +     lanphy_write_page_reg(phydev, 5,
> > + LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val);
> > +
> > +     /* MDI-X setting for swap A,B transmit */
> > +     val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP);
> > +     val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK;
> > +     val |= LAN8814_ALIGN_TX_A_B_SWAP;
> > +     lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val);
> 
> This does not look related to PTP. If David has not ready merged this, i would
> of said you should of submitted this as a separate patch.
> 

This code already present in lan8814 phy code. I think due to movement of function from up to down.
This change reflected here.

> > +static void lan8814_parse_latency(struct phy_device *phydev) {
> > +     const struct device_node *np = phydev->mdio.dev.of_node;
> > +     struct kszphy_priv *priv = phydev->priv;
> > +     struct kszphy_latencies *latency = &priv->latencies;
> > +     u32 val;
> > +
> > +     if (!of_property_read_u32(np, "lan8814,latency_rx_10", &val))
> > +             latency->rx_10 = val;
> > +     if (!of_property_read_u32(np, "lan8814,latency_tx_10", &val))
> > +             latency->tx_10 = val;
> > +     if (!of_property_read_u32(np, "lan8814,latency_rx_100", &val))
> > +             latency->rx_100 = val;
> > +     if (!of_property_read_u32(np, "lan8814,latency_tx_100", &val))
> > +             latency->tx_100 = val;
> > +     if (!of_property_read_u32(np, "lan8814,latency_rx_1000", &val))
> > +             latency->rx_1000 = val;
> > +     if (!of_property_read_u32(np, "lan8814,latency_tx_1000", &val))
> > +             latency->tx_1000 = val;
> 
> Are range checks need here? You are reading a u32, but PHY registers are
> generally 16 bit.
> 

Yes this is mistake. May we will need to give fix for this in next patch.

>     Andrew