Re: [PATCH net-next v4 06/12] net: ethernet: oa_tc6: implement internal PHY initialization

[<Parthiban.Veerasooran@microchip.com>]

Hi Andrew,

On 24/04/24 5:18 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +/* PHY Clause 22 and 29 registers base address and mask */
>> +#define OA_TC6_PHY_STD_REG_ADDR_BASE         0xFF00
>> +#define OA_TC6_PHY_STD_REG_ADDR_MASK         0x3F
> 
> Did you every find out why C29 is reference here? From the standard:
> 
> 29. System considerations for multisegment 100BASE-T networks
> 
> NOTE—This clause relates to clauses that are not recommended for new
> installations. This clause is not recommended for new
> installations. Since March 2012, maintenance changes are no longer
> being considered for this clause.
> 
> I don't think you should be referencing it in the code.
Thanks for this detailed explanation. The reason why I considered this 
is, in section 9.2 and table 7 of the OPEN Alliance document shows the 
mapping of standard registers within MMS 0 where clause 29 also listed 
as clause 22 extended registers from 0xFF20 to 0xFF3F.

https://opensig.org/download/document/OPEN_Alliance_10BASET1x_MAC-PHY_Serial_Interface_V1.1.pdf

But by considering the limitations explained by you above, I will change 
the mask as 0x1F to support only for clause 22 registers in the next 
version.
> 
>> +static int oa_tc6_mdiobus_read(struct mii_bus *bus, int addr, int regnum)
>> +{
>> +     struct oa_tc6 *tc6 = bus->priv;
>> +     u32 regval;
>> +     bool ret;
>> +
>> +     ret = oa_tc6_read_register(tc6, OA_TC6_PHY_STD_REG_ADDR_BASE |
>> +                                (regnum & OA_TC6_PHY_STD_REG_ADDR_MASK),
>> +                                &regval);
>> +     if (ret)
>> +             return -ENODEV;
> 
> In general, you should not replace an error code from a lower level
> function with a different error code. If there is a good reason to do
> this, please add a comment.
Ah ok, I will return "ret" in the next version.
> 
>> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
>> index 534ca7d1b061..769a88254285 100644
>> --- a/drivers/net/phy/microchip_t1s.c
>> +++ b/drivers/net/phy/microchip_t1s.c
>> @@ -268,6 +268,34 @@ static int lan86xx_read_status(struct phy_device *phydev)
>>        return 0;
>>   }
> 
> Please put this into a new patch.
OK, will create as a new patch in the next version.
> 
>>
>> +/* OPEN Alliance 10BASE-T1x compliance MAC-PHYs will have both C22 and
>> + * C45 registers space. If the PHY is discovered via C22 bus protocol it assumes
>> + * it uses C22 protocol and always uses C22 registers indirect access to access
>> + * C45 registers. This is because, we don't have a clean separation between
>> + * C22/C45 register space and C22/C45 MDIO bus protocols. Resulting, PHY C45
>> + * registers direct access can't be used which can save multiple SPI bus access.
>> + * To support this feature, set .read_mmd/.write_mmd in the PHY driver to call
>> + * .read_c45/.write_c45 in the OPEN Alliance framework
>> + * drivers/net/ethernet/oa_tc6.c
>> + */
>> +static int lan865x_phy_read_mmd(struct phy_device *phydev, int devnum,
>> +                             u16 regnum)
>> +{
>> +     struct mii_bus *bus = phydev->mdio.bus;
>> +     int addr = phydev->mdio.addr;
>> +
>> +     return bus->read_c45(bus, addr, devnum, regnum);
> 
> It is better to use __mdiobus_c45_read(). That will check you have the
> lock held, won't jump through a null pointer if the bus does not
> implement C45, does tracing, and increments the MDIO statistics.
OK, sure will do it in the next version. As this is also applicable for 
__mdiobus_c45_write(), I will change this as well.

Best regards,
Parthiban V
> 
>            Andrew
>