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), >> + ®val); >> + 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 >
