Hi Vladimir, Sorry for the delayed response. Too much work at this time of the year (ah, Decembers)..( On Fri, Dec 08, 2023 at 06:33:43PM +0200, Vladimir Oltean wrote: > On Fri, Dec 08, 2023 at 05:11:20PM +0300, Serge Semin wrote: > > My idea was to reuse the mdio_device which has already been created > > either by means of the MDIO-bus OF-subnode or by means of the MDIO-bus > > board_info infrastructure (can be utilized in the SJA1105 or Wangxun > > Tx GBE). The xpcs_create() method then either probes the device on the MDIO > > bus and gets ID from there, or just uses the custom IDs based on the > > OF compatible match table or on the platform_data. If no MDIO-device > > was created my patchset is supposed to preserve the previous > > semantics: create MDIO-device, probe the device on the MDIO-bus, get > > device IDs from there. See the next patch for more details: > > https://lore.kernel.org/netdev/20231205103559.9605-11-fancer.lancer@xxxxxxxxx/ > > > > > That was attempted a while ago by > > > Sean Anderson with the Lynx PCS. Are you aware of the fact that even in > > > the good case in which binding the driver actually works, the user can > > > then come along and unbind it from the PCS device, and phylink isn't > > > prepared to handle that, so it will crash the kernel upon the next > > > phylink_pcs call? > > > > To be honest I didn't consider the driver bind/unbind option. But my > > case a bit different. DW XPCS MDIO-device is supposed to be created > > automatically by means of the DW XPCS MI driver from the DT-nodes > > hierarchy like this: > > mdio@1f05d000 { > > compatible = "snps,dw-xpcs-mi"; > > reg = <0 0x1f05d000 0 0x1000>; > > > > xgmac_pcs: ethernet-pcs@0 { > > compatible = "snps,dw-xpcs"; > > reg = <0>; > > }; > > }; > > The platform-device is created for the mdio@1f05d000 node for which > > the DW XPCS MI driver is loaded, which calls the > > devm_of_mdiobus_register() in the probe() method which registers the > > MDIO-bus and then creates the MDIO-device from the ethernet-pcs@0 > > node. The DW XPCS MDIO-device driver is attached to that MDIO-device > > then. In such model the PCS can be supplied to the DW *MAC via the > > "pcs-handle = &xgmac_pcs" property. > > > > Regarding the current semantics it's preserved in the framework of the > > xpcs_create_byaddr() method (former xpcs_create_mdiodev()) by means of > > the next code snippet: > > if (mdiobus_is_registered_device(bus, addr)) { > > mdiodev = bus->mdio_map[addr]; > > mdio_device_get(mdiodev); > > } else { > > mdiodev = mdio_device_create(bus, addr); > > if (IS_ERR(mdiodev)) > > return ERR_CAST(mdiodev); > > } > > Device can be automatically created if before registering the MDIO-bus > > the xpcs_create_byaddr() caller registered the MDIO-device board info > > by means of the mdiobus_register_board_info() method. In addition to > > that it's now possible to supply some custom data (custom device IDs > > in my implementation) to the XPCS driver by means of the > > mdio_board_info.platform_data field. See the next patch for > > reference: > > https://lore.kernel.org/netdev/20231205103559.9605-14-fancer.lancer@xxxxxxxxx > > > > So what the difference with the Lynx PCS is that in my case the > > MDIO-device is created automatically as a result of the DW XPCS MI > > MDIO-bus registration. Additionally I implemented the MDIO-device > > creation based on the MDIO-board-info, thus there won't be need in the > > calling mdio_device_create() on each xpcs_create_mdiodev() invocation. > > The later part isn't that important in the framework of this > > conversation, but just so you be aware. > > It's not really different, though. You can connect to the Lynx PCS both > ways, see dpaa2_pcs_create() which also searches for a pcs-handle before > calling lynx_pcs_create_fwnode(). Ah, right. Lynx PCS also implements the fwnode-based PCS descriptor creation. > What's subtly different is that we > don't (yet) have "fsl,lynx-pcs" compatible strings in the device tree. > So the MDIO controller will register the PCS devices as struct phy_device > (which still have an underlying struct mdio_device). The PCS layer > connects to the underlying struct mdio_device, and the phy_device on top > remains unconnected to any phylib/phylink MAC driver. That is confusing, > I should really get to adding those compatible strings to suppress the > phy_device creation. It hasn't been confirmed yet here https://lore.kernel.org/netdev/na6krkoco7pmsl62dfuj2xlrvpsnod74ptpfyy6gv7dzwmowga@mzsiknjian2i/ but AFAICS it is wrong to have a PCS device registered as PHY by any means: unmasking address in mii_bus->phy_mask or having the of_mdiobus_child_is_phy() returned true for a DT-node. So right, a specific compatible should be added to the PCS DT-nodes. > > > Regarding the driver bind/unbind. As I said I didn't actually consider > > that option. On the other hand my DW XPCS MDIO-device driver doesn't > > do actual probe() or remove(). The only implemented thing is the > > of_device_id table, which is used to assign PCS and PMA IDs if > > required based on the DT compatible property. So I can easily drop any > > MDIO device-driver part and parse the of_device_id table right in the > > xpcs_create_bynode(). From that perspective my implementation won't > > differ much from the Lynx PCS design. The only difference will be is > > the way the MDIO-bus is created and registered. In case of Lynx PCS > > the bus is created by the MAC-driver itself. > > Nope, not true. Follow the pcs-handle in arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi. Ah, right, I missed that case. I was referring to the cases when the Lynx PCS MDIO node is defined as a sub-node of the MAC node. > > > In my case DW XPCS MI is currently created in the framework of the > > separate platform driver. Do you think it would be better to follow > > the Lynx design pattern in order to get rid from the possibility of > > the DW XPCS MI driver being unbound behind the STMMAC+XPCS couple > > back? > > I think you actually pointed out a flaw in the Lynx PCS design too. > Actually, it is a larger flaw in the kernel. You can also unbind the > MDIO bus which holds the phy_device, and phylib (and therefore also > phylink) won't expect that either, so it will crash. > > > In this case the Dw MAC DT-node hierarchy would look like this: > > > > xgmac: ethernet@1f054000 { > > compatible = "snps,dwxgmac"; > > reg = <0 0x1f054000 0 0x4000>; > > reg-names = "stmmaceth"; > > ranges; > > > > ... > > > > pcs-handle = &xgmac_pcs; > > > > // DW XPCS MI to access the DW XPCS attached to the device > > mdio@1f05d000 { > > compatible = "snps,dwmac-mi"; > > reg = <0 0x1f05d000 0 0x1000>; > > > > xgmac_pcs: ethernet-pcs@0 { > > compatible = "snps,dw-xpcs"; > > reg = <0>; > > }; > > }; > > > > // Normal MDIO-bus to access external PHYs (it's also called > > // as SMA - Station Management Agent - by Synopsys) > > mdio { > > compatible = "snps,dwmac-mdio"; > > #address-cells = <1>; > > #size-cells = <0>; > > }; > > }; > > > > I actually thought to use that hardware description pattern instead, > > but after some meditation around that I decided that having the DW > > XPCS device defined separately from the DW MAC node seemed better at > > least from the code separation point of view. Now I think that it > > wasn't the best decision. DW XPCS is always attached to the DW XGMAC > > controller. So it would be more correct having it defined as a > > sub-node. It would also helped to avoid the platform device driver > > bind/unbind problem. > > > > What do you think? Should I re-design my patchset to be supporting the > > design above? (After having conversion with you I am more inclined to > > do that now than to stick with the currently implemented solution.) > > I think that the placement of the "mdio" node as lateral vs subordinate > to the "ethernet" node would have fixed the issue by mistake. We should > be looking at it as a structural problem of the kernel instead. Don't > let it influence what you believe should be the correct design. Ok. Thanks for clarification. I won't move the PCS device DT-node to being subordinate of the MAC DT-node then. Although after some more digging into the problem I figured out that the solution still needs to be re-designed a bit. Currently I have the DW XPCS device represented as the nodes hierarchy: mdio@1f05d000 { compatible = "snps,dwmac-mi"; reg = <0 0x1f05d000 0 0x1000>; xgmac_pcs: ethernet-pcs@0 { compatible = "snps,dw-xpcs"; reg = <0>; }; }; When I introduced such representation I assumed that it was possible to have more than one DW XPCS devices accessible over the same MCI/APB interface with for instance some static offset. But it turned out I was wrong again. DW XPCS HW databook explicitly states that port_id_i[4:0] input signal is specific to the MDIO interface only. That signal is the MDIO-bus address of the device and it doesn't exist for the DW XPCS devices mapped to the system IO-memory via the MCI/APB buses. So there can't be more than one XPCS device accessible over the same MCI/APB port and the correct way to represent the DW XPCS device is just: xgmac_pcs: ethernet-pcs@0 { compatible = "snps,dw-xpcs"; reg = <0 0x1f05d000 0 0x1000>; }; with no superior MI node. I'll re-design my patchset to support the device representation above then: just create a hidden MDIO-bus in the DW XPCS driver probe method and directly register the XPCS-device on it. The patch for the MDIO-bus subsystem will be gone after that. > > > > The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least > > > tear down the whole thing when the PCS is unbound, which is saner than > > > crashing the kernel. I don't see the equivalent protection mechanism here? > > > > You are right. I don't have any equivalent protection here. Thanks for > > suggesting a solution. > > I think that a device link between the "ethernet" device and the "mdio" > device (controller, parent of the PHY or PCS), if the Ethernet is not a > parent of the MDIO controller, could also solve that. But it would also > require ACK from PHY maintainers, who may have grander plans to address > this snag. Ok. I'll add it in v2. Let's see what the maintainers think about that. Thanks for all your comments in my patchset regard. They are really helpful. -Serge(y) > > > > Can't the xpcs continue to live without a bound driver? Having a > > > compatible string in the OF description is perfectly fine though, > > > and should absolutely not preclude that. > > > > As I explained above Dw XPCS device can live without a bound driver > > because the DW XPCS MDIO-driver doesn't do much but merely gets to be > > bound based on the of_device_id table. In my case the problem is in > > the DW XPCS MI driver which indeed can be detached. Please see my > > long-read text above. > > Yeah, common design, common problem.
