On Tue, Dec 10, 2024 at 10:56:41AM +0100, Matthias Schiffer wrote: > On Mon, 2024-12-09 at 17:14 +0100, Andrew Lunn wrote: > > > > > Not our board, but the AM62 SoC. From the datasheet: > > > > > > "TXC is delayed internally before being driven to the RGMII[x]_TXC pin. This > > > internal delay is always enabled." So enabling the TX delay on the PHY side > > > would result in a double delay. > > > > phy-mode describes the board. If the board does not have extra long > > clock lines, phy-mode should be rgmii-id. > > > > The fact the MAC is doing something which no other MAC does should be > > hidden away in the MAC driver, as much as possible. > > Isn't it kind of a philosophical question whether a delay added by the SoC > integration is part of the MAC or not? One could also argue that the MAC IP core > is always the same, with some SoCs adding the delay and others not. (I don't > know if there are actually SoCs with the same IP core that don't add a delay; > I'm just not a big fan of hiding details in the driver that could easily be > described by the Device Tree, thus making the driver more generic) It is more about, what does phy-mode = "rgmii"; mean? It means the board provides the delay via extra long clock lines. Except for when some random MAC driver has a completely different meaning, it is not documented it means something else, you have to read the sources and the mailing lists, to find out what this particularly MAC driver is doing for phy-mode = "rgmii". Do we really want that. Or should we define that phy-mode = "rgmii" means the PCB provides the delay. End of story, no exceptions. And that "rgmii-id" means the MAC/PHY pair need to provide the delay? End of story, no exceptions. > > Since the MAC driver is forcing the TX delay, it needs to take the > > value returned from of_get_phy_mode() and mask out the TX bit before > > passing it to the PHY. > > Hmm okay, this is what the similar ICSSG/PRUETH driver does. I've always found > that solution to be particularly confusing, but if that's how it's supposed to > work, I'll have to accept that. It has to be that why. If the MAC does the delay, the MAC needs to ensure the PHY does not do the delays and well, and it achieves that by setting PHY_INTERFACE_MODE to indicate the PHY should not add delays. Do you have a better idea how this can be done? > In my opinion the documentation Documentation/networking/phy.rst is not very > clear on this matter - the whole section "(RG)MII/electrical interface > considerations" talks about whether the PHY inserts the delay or not, so my > assumption was that phy-mode describes the PHY side of things and only that. > > It gets even more confusing when taking into account > Documentation/devicetree/bindings/net/ethernet-controller.yaml, which contains > comments like "RGMII with internal RX delay provided by the PHY, the MAC should > not add an RX delay in this case", which sounds like there are only the cases > "delay is added by the PHY" and "delay is added by the MAC" - the case "delay is > part of the board design, neither MAC nor PHY add it" doesn't even appear. We have tried to improve the documentation. We have also been very rigid in reviewing DT bindings, and what these things mean. But it seems like many developers don't read reviews other developers get. Go search the email archive. How many times have i had this very same conversation? Everybody gets pause wrong. Everybody gets EEE wrong. Everybody gets RGMII delays wrong, not matter how many times we tell developers they are getting it wrong.... phylink is helping with this, it takes it out of developers hands so they cannot get pause or soon EEE wrong. Andrew
