On Thu, Jun 01, 2023 at 11:17:52PM +0900, Atsushi Nemoto wrote: > On 2023/06/01 22:36, Andrew Lunn wrote: > >> But in this case, LINK_ST is also connected to MII_RXLINK pin of > >> the MAC module in SoC. MII_RXLINK also expects low-active signal input. > >> (though I only wrote about LED, sorry) > > > > This is why the Commit message should contain the answer to 'Why?'. > > The code tells us 'What', but without knowing the 'Why', it is hard to > > say if you are doing the right or wrong thing. > > Yes, then how about this? > --- > The LINK_ST is active-high by default. This can be inverted by the > GE_LNK_STAT_INV_REG register, meaning that link up is indicated by > setting LINK_ST low. O.K. So far. But the rest still does not explain why. So i suggest adding.... One application of this PHY is connecting it to the PRU in TI Sitara SoC. This MAC requires a hardware indication of link, rather than the usual phylib callback via adjust link. And this input is active-low. Add a property to indicate that LINK_ST be configured active low. > Please look at a block diagram in this page: > https://software-dl.ti.com/processor-sdk-linux/esd/docs/latest/linux/Foundational_Components/PRU-ICSS/Linux_Drivers/PRU-ICSS_Ethernet.html We are now getting close to having all the pieces of the puzzle to decide if this is the right or wrong way to do this. This appears to be the 'Vendor Crap' driver. You are patching mainline here, so it is the mainline PRU driver we care about: https://lore.kernel.org/netdev/20230424053233.2338782-1-danishanwar@xxxxxx/T/ Looking at the device tree binding, it uses the standard phy-handle, phy-mode properties. There is also emac_adjust_link() which is used by phylib to tell the MAC driver the link is down. So you now need to convince me this change is actually needed in mainline. Thanks Andrew
