* Re: drivers/of/of_mdio.c needs a small modification [not found] <c8b74845-b9e1-6d85-3947-56333b73d756@arf.net.pl> @ 2020-08-28 22:28 ` Andrew Lunn 2020-08-28 22:34 ` Adam Rudziński 0 siblings, 1 reply; 12+ messages in thread From: Andrew Lunn @ 2020-08-28 22:28 UTC (permalink / raw) To: Adam Rudziński; +Cc: robh+dt, frowand.list, f.fainelli, netdev Hi Adam > If kernel has to bring up two Ethernet interfaces, the processor has two > peripherals with functionality of MACs (in i.MX6ULL these are Fast Ethernet > Controllers, FECs), but uses a shared MDIO bus, then the kernel first probes > one MAC, enables clock for its PHY, probes MDIO bus tryng to discover _all_ > PHYs, and then probes the second MAC, and enables clock for its PHY. The > result is that the second PHY is still inactive during PHY discovery. Thus, > one Ethernet interface is not functional. What clock are you talking about? Do you have the FEC feeding a 50MHz clock to the PHY? Each FEC providing its own clock to its own PHY? And are you saying a PHY without its reference clock does not respond to MDIO reads and hence the second PHY does not probe because it has no reference clock? Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drivers/of/of_mdio.c needs a small modification 2020-08-28 22:28 ` drivers/of/of_mdio.c needs a small modification Andrew Lunn @ 2020-08-28 22:34 ` Adam Rudziński 2020-08-28 22:53 ` Andrew Lunn 0 siblings, 1 reply; 12+ messages in thread From: Adam Rudziński @ 2020-08-28 22:34 UTC (permalink / raw) To: Andrew Lunn; +Cc: robh+dt, frowand.list, f.fainelli, netdev Hi Andrew. W dniu 2020-08-29 o 00:28, Andrew Lunn pisze: > Hi Adam > >> If kernel has to bring up two Ethernet interfaces, the processor has two >> peripherals with functionality of MACs (in i.MX6ULL these are Fast Ethernet >> Controllers, FECs), but uses a shared MDIO bus, then the kernel first probes >> one MAC, enables clock for its PHY, probes MDIO bus tryng to discover _all_ >> PHYs, and then probes the second MAC, and enables clock for its PHY. The >> result is that the second PHY is still inactive during PHY discovery. Thus, >> one Ethernet interface is not functional. > What clock are you talking about? Do you have the FEC feeding a 50MHz > clock to the PHY? Each FEC providing its own clock to its own PHY? And > are you saying a PHY without its reference clock does not respond to > MDIO reads and hence the second PHY does not probe because it has no > reference clock? > > Andrew Yes, exactly. In my case the PHYs are LAN8720A, and it works this way. Maybe this problem is PHY-related. I also think that my proposition allows to make device tree look better and be less confusing, as a side-effect. Best regards, Adam ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drivers/of/of_mdio.c needs a small modification 2020-08-28 22:34 ` Adam Rudziński @ 2020-08-28 22:53 ` Andrew Lunn 2020-08-28 23:14 ` Adam Rudziński 0 siblings, 1 reply; 12+ messages in thread From: Andrew Lunn @ 2020-08-28 22:53 UTC (permalink / raw) To: Adam Rudziński; +Cc: robh+dt, frowand.list, f.fainelli, netdev On Sat, Aug 29, 2020 at 12:34:05AM +0200, Adam Rudziński wrote: > Hi Andrew. > > W dniu 2020-08-29 o 00:28, Andrew Lunn pisze: > > Hi Adam > > > > > If kernel has to bring up two Ethernet interfaces, the processor has two > > > peripherals with functionality of MACs (in i.MX6ULL these are Fast Ethernet > > > Controllers, FECs), but uses a shared MDIO bus, then the kernel first probes > > > one MAC, enables clock for its PHY, probes MDIO bus tryng to discover _all_ > > > PHYs, and then probes the second MAC, and enables clock for its PHY. The > > > result is that the second PHY is still inactive during PHY discovery. Thus, > > > one Ethernet interface is not functional. > > What clock are you talking about? Do you have the FEC feeding a 50MHz > > clock to the PHY? Each FEC providing its own clock to its own PHY? And > > are you saying a PHY without its reference clock does not respond to > > MDIO reads and hence the second PHY does not probe because it has no > > reference clock? > > > > Andrew > > Yes, exactly. In my case the PHYs are LAN8720A, and it works this way. O.K. Boards i've seen like this have both PHYs driver from the first MAC. Or the clock goes the other way, the PHY has a crystal and it feeds the FEC. I would say the correct way to solve this is to make the FEC a clock provider. It should register its clocks with the common clock framework. The MDIO bus can then request the clock from the second FEC before it scans the bus. Or we add the clock to the PHY node so it enables the clock before probing it. There are people who want this sort of framework code, to be able to support a GPIO reset, which needs releasing before probing the bus for the PHY. Anyway, post your patch, so we get a better idea what you are proposing. Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drivers/of/of_mdio.c needs a small modification 2020-08-28 22:53 ` Andrew Lunn @ 2020-08-28 23:14 ` Adam Rudziński 2020-08-29 3:29 ` Florian Fainelli 0 siblings, 1 reply; 12+ messages in thread From: Adam Rudziński @ 2020-08-28 23:14 UTC (permalink / raw) To: Andrew Lunn; +Cc: robh+dt, frowand.list, f.fainelli, netdev W dniu 2020-08-29 o 00:53, Andrew Lunn pisze: > On Sat, Aug 29, 2020 at 12:34:05AM +0200, Adam Rudziński wrote: >> Hi Andrew. >> >> W dniu 2020-08-29 o 00:28, Andrew Lunn pisze: >>> Hi Adam >>> >>>> If kernel has to bring up two Ethernet interfaces, the processor has two >>>> peripherals with functionality of MACs (in i.MX6ULL these are Fast Ethernet >>>> Controllers, FECs), but uses a shared MDIO bus, then the kernel first probes >>>> one MAC, enables clock for its PHY, probes MDIO bus tryng to discover _all_ >>>> PHYs, and then probes the second MAC, and enables clock for its PHY. The >>>> result is that the second PHY is still inactive during PHY discovery. Thus, >>>> one Ethernet interface is not functional. >>> What clock are you talking about? Do you have the FEC feeding a 50MHz >>> clock to the PHY? Each FEC providing its own clock to its own PHY? And >>> are you saying a PHY without its reference clock does not respond to >>> MDIO reads and hence the second PHY does not probe because it has no >>> reference clock? >>> >>> Andrew >> Yes, exactly. In my case the PHYs are LAN8720A, and it works this way. > O.K. Boards i've seen like this have both PHYs driver from the first > MAC. Or the clock goes the other way, the PHY has a crystal and it > feeds the FEC. > > I would say the correct way to solve this is to make the FEC a clock > provider. It should register its clocks with the common clock > framework. The MDIO bus can then request the clock from the second FEC > before it scans the bus. Or we add the clock to the PHY node so it > enables the clock before probing it. There are people who want this > sort of framework code, to be able to support a GPIO reset, which > needs releasing before probing the bus for the PHY. > > Anyway, post your patch, so we get a better idea what you are > proposing. > > Andrew Hm, this sounds reasonable, but complicated at the same time. I have spent some time searching for possible solution and never found anything teaching something similar, so I'd also speculate that it's kind of not very well documented. That doesn't mean I'm against these solutions, just that seems to be beyond capabilities of many mortals who even try to read. OK, so a patch it is. Please, let me know how to make the patch so that it was useful and as convenient as possible for you. Would you like me to use some specific code/repo/branch/... as its base? Best regards, Adam ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drivers/of/of_mdio.c needs a small modification 2020-08-28 23:14 ` Adam Rudziński @ 2020-08-29 3:29 ` Florian Fainelli 2020-08-29 8:15 ` Adam Rudziński 0 siblings, 1 reply; 12+ messages in thread From: Florian Fainelli @ 2020-08-29 3:29 UTC (permalink / raw) To: Adam Rudziński, Andrew Lunn; +Cc: robh+dt, frowand.list, netdev On 8/28/2020 4:14 PM, Adam Rudziński wrote: > W dniu 2020-08-29 o 00:53, Andrew Lunn pisze: >> On Sat, Aug 29, 2020 at 12:34:05AM +0200, Adam Rudziński wrote: >>> Hi Andrew. >>> >>> W dniu 2020-08-29 o 00:28, Andrew Lunn pisze: >>>> Hi Adam >>>> >>>>> If kernel has to bring up two Ethernet interfaces, the processor >>>>> has two >>>>> peripherals with functionality of MACs (in i.MX6ULL these are Fast >>>>> Ethernet >>>>> Controllers, FECs), but uses a shared MDIO bus, then the kernel >>>>> first probes >>>>> one MAC, enables clock for its PHY, probes MDIO bus tryng to >>>>> discover _all_ >>>>> PHYs, and then probes the second MAC, and enables clock for its >>>>> PHY. The >>>>> result is that the second PHY is still inactive during PHY >>>>> discovery. Thus, >>>>> one Ethernet interface is not functional. >>>> What clock are you talking about? Do you have the FEC feeding a 50MHz >>>> clock to the PHY? Each FEC providing its own clock to its own PHY? And >>>> are you saying a PHY without its reference clock does not respond to >>>> MDIO reads and hence the second PHY does not probe because it has no >>>> reference clock? >>>> >>>> Andrew >>> Yes, exactly. In my case the PHYs are LAN8720A, and it works this way. >> O.K. Boards i've seen like this have both PHYs driver from the first >> MAC. Or the clock goes the other way, the PHY has a crystal and it >> feeds the FEC. >> >> I would say the correct way to solve this is to make the FEC a clock >> provider. It should register its clocks with the common clock >> framework. The MDIO bus can then request the clock from the second FEC >> before it scans the bus. Or we add the clock to the PHY node so it >> enables the clock before probing it. There are people who want this >> sort of framework code, to be able to support a GPIO reset, which >> needs releasing before probing the bus for the PHY. >> >> Anyway, post your patch, so we get a better idea what you are >> proposing. >> >> Andrew > > Hm, this sounds reasonable, but complicated at the same time. I have > spent some time searching for possible solution and never found anything > teaching something similar, so I'd also speculate that it's kind of not > very well documented. That doesn't mean I'm against these solutions, > just that seems to be beyond capabilities of many mortals who even try > to read. > > OK, so a patch it is. Please, let me know how to make the patch so that > it was useful and as convenient as possible for you. Would you like me > to use some specific code/repo/branch/... as its base? This is targeting the net-next tree, see the netdev-FAQ here for details: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.rst I will be posting some patches for our ARCH_BRCMSTB platforms which require that we turn on the internal PHY's digital clock otherwise it does not respond on the MDIO bus and we cannot discover its ID and we cannot bind to a PHY driver. I will make sure to copy you so you can see if this would work for you. -- Florian ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drivers/of/of_mdio.c needs a small modification 2020-08-29 3:29 ` Florian Fainelli @ 2020-08-29 8:15 ` Adam Rudziński 2020-08-29 15:15 ` Andrew Lunn 0 siblings, 1 reply; 12+ messages in thread From: Adam Rudziński @ 2020-08-29 8:15 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn, netdev; +Cc: robh+dt, frowand.list [-- Attachment #1: Type: text/plain, Size: 11011 bytes --] W dniu 2020-08-29 o 05:29, Florian Fainelli pisze: > > On 8/28/2020 4:14 PM, Adam Rudziński wrote: >> W dniu 2020-08-29 o 00:53, Andrew Lunn pisze: >>> On Sat, Aug 29, 2020 at 12:34:05AM +0200, Adam Rudziński wrote: >>>> Hi Andrew. >>>> >>>> W dniu 2020-08-29 o 00:28, Andrew Lunn pisze: >>>>> Hi Adam >>>>> >>>>>> If kernel has to bring up two Ethernet interfaces, the processor >>>>>> has two >>>>>> peripherals with functionality of MACs (in i.MX6ULL these are >>>>>> Fast Ethernet >>>>>> Controllers, FECs), but uses a shared MDIO bus, then the kernel >>>>>> first probes >>>>>> one MAC, enables clock for its PHY, probes MDIO bus tryng to >>>>>> discover _all_ >>>>>> PHYs, and then probes the second MAC, and enables clock for its >>>>>> PHY. The >>>>>> result is that the second PHY is still inactive during PHY >>>>>> discovery. Thus, >>>>>> one Ethernet interface is not functional. >>>>> What clock are you talking about? Do you have the FEC feeding a 50MHz >>>>> clock to the PHY? Each FEC providing its own clock to its own PHY? >>>>> And >>>>> are you saying a PHY without its reference clock does not respond to >>>>> MDIO reads and hence the second PHY does not probe because it has no >>>>> reference clock? >>>>> >>>>> Andrew >>>> Yes, exactly. In my case the PHYs are LAN8720A, and it works this way. >>> O.K. Boards i've seen like this have both PHYs driver from the first >>> MAC. Or the clock goes the other way, the PHY has a crystal and it >>> feeds the FEC. >>> >>> I would say the correct way to solve this is to make the FEC a clock >>> provider. It should register its clocks with the common clock >>> framework. The MDIO bus can then request the clock from the second FEC >>> before it scans the bus. Or we add the clock to the PHY node so it >>> enables the clock before probing it. There are people who want this >>> sort of framework code, to be able to support a GPIO reset, which >>> needs releasing before probing the bus for the PHY. >>> >>> Anyway, post your patch, so we get a better idea what you are >>> proposing. >>> >>> Andrew >> >> Hm, this sounds reasonable, but complicated at the same time. I have >> spent some time searching for possible solution and never found >> anything teaching something similar, so I'd also speculate that it's >> kind of not very well documented. That doesn't mean I'm against these >> solutions, just that seems to be beyond capabilities of many mortals >> who even try to read. >> >> OK, so a patch it is. Please, let me know how to make the patch so >> that it was useful and as convenient as possible for you. Would you >> like me to use some specific code/repo/branch/... as its base? > > This is targeting the net-next tree, see the netdev-FAQ here for details: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.rst > > > I will be posting some patches for our ARCH_BRCMSTB platforms which > require that we turn on the internal PHY's digital clock otherwise it > does not respond on the MDIO bus and we cannot discover its ID and we > cannot bind to a PHY driver. I will make sure to copy you so you can > see if this would work for you. Actually, I came to conclusion, that sending a patch for discussion is not the best idea at this time, because the original code for my kernel was from here: https://github.com/SoMLabs/somlabs-linux-imx/tree/imx_4.19.35_1.1.0 commit ef35d67fb and although it still looks similar in the kernel, I've assumed that now the safest way is to just attach the original and modified files. So here they go, with some discussion below. I hope this doesn't make a mess and is still good. The essential general modification is in drivers/of/of_mdio.c include/linux/of_mdio.h and to make the driver profit on that, target-specific (well, the driver is target-specific) modification in drivers/net/ethernet/freescale/fec_main.c Modification in of_mdio.c "Original" function of_mdiobus_register contains a loop over all child nodes. /** * of_mdiobus_register - Register mii_bus and create PHYs from the device tree * @mdio: pointer to mii_bus structure * @np: pointer to device_node of MDIO bus. * * This function registers the mii_bus structure and registers a phy_device * for each child node of @np. */ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np) { (...) /* Register the MDIO bus */ rc = mdiobus_register(mdio); if (rc) return rc; /* Loop over the child nodes and register a phy_device for each phy */ for_each_available_child_of_node(np, child) { addr = of_mdio_parse_addr(&mdio->dev, child); if (addr < 0) { scanphys = true; continue; } if (of_mdiobus_child_is_phy(child)) rc = of_mdiobus_register_phy(mdio, child, addr); else rc = of_mdiobus_register_device(mdio, child, addr); if (rc == -ENODEV) dev_err(&mdio->dev, "MDIO device at address %d is missing.\n", addr); else if (rc) goto unregister; } if (!scanphys) return 0; (...) } The loop is preceeded by mdiobus_register, so this provides service only for a new MDIO bus. It is not possible to later add more child nodes. Therefore the driver has to know all the child nodes before it registers the bus. The device tree looks more or less like this: &fec2 { (...) mdio { (all PHYs here) }; (...) }; &fec1 { (some stuff, but no mdio here) }; The driver (fec_main.c) for the second FEC has to only set the pointer to the shared MDIO bus, but also cannot do anything more: if ((fep->quirks & FEC_QUIRK_SINGLE_MDIO) && fep->dev_id > 0) { /* fec1 uses fec0 mii_bus */ if (mii_cnt && fec0_mii_bus) { fep->mii_bus = fec0_mii_bus; *fec_mii_bus_share = FEC0_MII_BUS_SHARE_TRUE; mii_cnt++; return 0; } return -ENOENT; } I propose to get the loop out of of_mdiobus_register and make it a new public function (with prototype in of_mdio.h; most likely it makes sense to add checking if pointers are not NULL). /** * of_mdiobus_register - Register mii_bus and create PHYs from the device tree * @mdio: pointer to mii_bus structure * @np: pointer to device_node of MDIO bus. * * This function registers the mii_bus structure and registers a phy_device * for each child node of @np. */ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np) { (...) rc = mdiobus_register(mdio); if (rc) return rc; rc = of_mdiobus_register_children(mdio, np, &scanphys); if (rc) goto unregister; if (!scanphys) return 0; (...) } /** * of_mdiobus_register_children - Create PHYs from the device tree * @mdio: pointer to mii_bus structure * @np: pointer to device_node of MDIO bus. * @scanphys: pointer to boolean variable telling if PHYs with * empty reg property should be scanned by the calling function * or NULL if this is information is not needed * * This function registers a phy_device for each child node of @np. */ int of_mdiobus_register_children(struct mii_bus *mdio, struct device_node *np, bool *scanphys) { struct device_node *child; int addr, rc; /* Loop over the child nodes and register a phy_device for each phy */ for_each_available_child_of_node(np, child) { addr = of_mdio_parse_addr(&mdio->dev, child); if (addr < 0) { if (scanphys) { *scanphys = true; } continue; } if (of_mdiobus_child_is_phy(child)) rc = of_mdiobus_register_phy(mdio, child, addr); else rc = of_mdiobus_register_device(mdio, child, addr); if (rc == -ENODEV) dev_err(&mdio->dev, "MDIO device at address %d is missing.\n", addr); else if (rc) return rc; } return 0; } The driver would be able to add the new PHYs to the shared MDIO bus by calling of_mdiobus_register_children. Then the device tree looks like this, which is more reasonable in my opinion: &fec2 { (...) mdio { (phy for fec2 here) }; (...) }; &fec1 { (...) mdio { (phy for fec1 here) }; (...) }; The driver would be able to add the new PHYs to the shared MDIO bus by calling of_mdiobus_register_children. if ((fep->quirks & FEC_QUIRK_SINGLE_MDIO) && fep->dev_id > 0) { /* fec1 uses fec0 mii_bus */ if (mii_cnt && fec0_mii_bus) { fep->mii_bus = fec0_mii_bus; *fec_mii_bus_share = FEC0_MII_BUS_SHARE_TRUE; mii_cnt++; node = of_get_child_by_name(pdev->dev.of_node, "mdio"); return of_mdiobus_register_children(fep->mii_bus, node, NULL); } return -ENOENT; } This could be also done later "dynamically" on runtime. Best regards, Adam [-- Attachment #2: original.tar.bz2 --] [-- Type: application/octet-stream, Size: 27747 bytes --] [-- Attachment #3: proposed.tar.bz2 --] [-- Type: application/octet-stream, Size: 27900 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drivers/of/of_mdio.c needs a small modification 2020-08-29 8:15 ` Adam Rudziński @ 2020-08-29 15:15 ` Andrew Lunn 2020-08-29 15:37 ` Adam Rudziński 0 siblings, 1 reply; 12+ messages in thread From: Andrew Lunn @ 2020-08-29 15:15 UTC (permalink / raw) To: Adam Rudziński; +Cc: Florian Fainelli, netdev, robh+dt, frowand.list > The driver would be able to add the new PHYs to the shared MDIO bus by > calling of_mdiobus_register_children. Then the device tree looks like this, > which is more reasonable in my opinion: > > &fec2 { > (...) > mdio { > (phy for fec2 here) > }; > (...) > }; > > &fec1 { > (...) > mdio { > (phy for fec1 here) > }; > (...) > }; DT describes hardware, and the topology of the hardware. The hardware really is: ethernet1@83fec000 { compatible = "fsl,imx51-fec", "fsl,imx27-fec"; reg = <0x83fec000 0x4000>; interrupts = <87>; phy-mode = "mii"; phy-reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; /* GPIO2_14 */ local-mac-address = [00 04 9F 01 1B B9]; phy-supply = <®_fec_supply>; phy-handle = <ðphy1>; mdio { clock-frequency = <5000000>; ethphy1: ethernet-phy@1 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <1>; max-speed = <100>; }; ethphy2: ethernet-phy@2 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <2>; max-speed = <100>; }; }; }; ethernet2@84fec000 { compatible = "fsl,imx51-fec", "fsl,imx27-fec"; reg = <0x83fec000 0x4000>; interrupts = <87>; phy-mode = "mii"; phy-reset-gpios = <&gpio2 15 GPIO_ACTIVE_LOW>; /* GPIO2_15 */ local-mac-address = [00 04 9F 01 1B BA]; phy-supply = <®_fec_supply>; phy-handle = <ðphy2>; }; What is missing from this is clocks. The IMX has a central clock provider: clks: clock-controller@20c4000 { compatible = "fsl,imx6ul-ccm"; reg = <0x020c4000 0x4000>; interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>; #clock-cells = <1>; clocks = <&ckil>, <&osc>, <&ipp_di0>, <&ipp_di1>; clock-names = "ckil", "osc", "ipp_di0", "ipp_di1"; }; and it exports two clocks, MX6UL_CLK_ENET1_REF, MX6UL_CLK_ENET2_REF So adding the clock properties: ethernet1@83fec000 { compatible = "fsl,imx51-fec", "fsl,imx27-fec"; reg = <0x83fec000 0x4000>; interrupts = <87>; phy-mode = "mii"; phy-reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; /* GPIO2_14 */ local-mac-address = [00 04 9F 01 1B B9]; phy-supply = <®_fec_supply>; phy-handle = <ðphy1>; mdio { clock-frequency = <5000000>; ethphy1: ethernet-phy@1 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <1>; max-speed = <100>; clocks = <&clks MX6UL_CLK_ENET1_REF>; }; ethphy2: ethernet-phy@2 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <2>; max-speed = <100>; clocks = <&clks MX6UL_CLK_ENET2_REF>; }; }; }; ethernet2@84fec000 { compatible = "fsl,imx51-fec", "fsl,imx27-fec"; reg = <0x83fec000 0x4000>; interrupts = <87>; phy-mode = "mii"; phy-reset-gpios = <&gpio2 15 GPIO_ACTIVE_LOW>; /* GPIO2_15 */ local-mac-address = [00 04 9F 01 1B BA]; phy-supply = <®_fec_supply>; phy-handle = <ðphy2>; }; Also look at drivers/net/phy/micrel.c. It has code to look up a FEC clock and use it. But that code assumes the PHY responds to MDIO reads when the clock is not ticking. It sounds like your PHY does not? Please double check that. If it does not, you need to add clock code to the PHY core. Florians patchset will help with that. Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drivers/of/of_mdio.c needs a small modification 2020-08-29 15:15 ` Andrew Lunn @ 2020-08-29 15:37 ` Adam Rudziński 2020-08-29 16:00 ` Andrew Lunn 0 siblings, 1 reply; 12+ messages in thread From: Adam Rudziński @ 2020-08-29 15:37 UTC (permalink / raw) To: Andrew Lunn; +Cc: Florian Fainelli, netdev, robh+dt, frowand.list W dniu 2020-08-29 o 17:15, Andrew Lunn pisze: >> The driver would be able to add the new PHYs to the shared MDIO bus by >> calling of_mdiobus_register_children. Then the device tree looks like this, >> which is more reasonable in my opinion: >> >> &fec2 { >> (...) >> mdio { >> (phy for fec2 here) >> }; >> (...) >> }; >> >> &fec1 { >> (...) >> mdio { >> (phy for fec1 here) >> }; >> (...) >> }; > DT describes hardware, and the topology of the hardware. The hardware really is: > > ethernet1@83fec000 { > compatible = "fsl,imx51-fec", "fsl,imx27-fec"; > reg = <0x83fec000 0x4000>; > interrupts = <87>; > phy-mode = "mii"; > phy-reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; /* GPIO2_14 */ > local-mac-address = [00 04 9F 01 1B B9]; > phy-supply = <®_fec_supply>; > phy-handle = <ðphy1>; > mdio { > clock-frequency = <5000000>; > ethphy1: ethernet-phy@1 { > compatible = "ethernet-phy-ieee802.3-c22"; > reg = <1>; > max-speed = <100>; > }; > ethphy2: ethernet-phy@2 { > compatible = "ethernet-phy-ieee802.3-c22"; > reg = <2>; > max-speed = <100>; > }; > }; > }; > > ethernet2@84fec000 { > compatible = "fsl,imx51-fec", "fsl,imx27-fec"; > reg = <0x83fec000 0x4000>; > interrupts = <87>; > phy-mode = "mii"; > phy-reset-gpios = <&gpio2 15 GPIO_ACTIVE_LOW>; /* GPIO2_15 */ > local-mac-address = [00 04 9F 01 1B BA]; > phy-supply = <®_fec_supply>; > phy-handle = <ðphy2>; > }; This is true assuming that the PHYs are always and forever connected to one specific MDIO bus. This is probably reasonable. Although, in i.MX the MDIO bus of FEC1 and FEC2 shares the pins. So with this "split" description one can just comment out FEC2 and still enjoy operational ethernet (FEC1 + its PHY). This may be simpler for hardware guys, like me, who don't have that much experience with device trees. But yes, that doesn't necessarily mean it's the good way to go. > Also look at drivers/net/phy/micrel.c. It has code to look up a FEC > clock and use it. But that code assumes the PHY responds to MDIO reads > when the clock is not ticking. It sounds like your PHY does not? > Please double check that. If it does not, you need to add clock code > to the PHY core. Florians patchset will help with that. > I'm sure of that - LAN8720A needs to have the clock from FEC or external generator to be able to talk over MDIO. I can see that one way or another, it's still some modification of the source code. You know the ropes, you decide if and which one makes sense. Best regards, Adam ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drivers/of/of_mdio.c needs a small modification 2020-08-29 15:37 ` Adam Rudziński @ 2020-08-29 16:00 ` Andrew Lunn 2020-08-29 18:01 ` Adam Rudziński 0 siblings, 1 reply; 12+ messages in thread From: Andrew Lunn @ 2020-08-29 16:00 UTC (permalink / raw) To: Adam Rudziński; +Cc: Florian Fainelli, netdev, robh+dt, frowand.list > This is true assuming that the PHYs are always and forever connected to one > specific MDIO bus. This is probably reasonable. Although, in i.MX the MDIO > bus of FEC1 and FEC2 shares the pins. In general, they do not. In fact, i don't see how that can work. The FEC drive provides no mutual exclusion between MDIO operations on different MDIO controllers. So one controller could be performing a read while the other a write. If they are sharing the same pins, how do you drive the clock pin both high and low at the same time? How do you have the data pin both high impedance so the PHY can drive it, and also drive out a 0 or a 1 to perform a right? What is suspect you can do is use pinmux to connect the pins to either ethernet1 MDIO controller, or ethernet2 mdio controller. But never both. You have to decide which gets to control the bus, and the other controller is isolated. > I'm sure of that - LAN8720A needs to have the clock from FEC or external > generator to be able to talk over MDIO. O.K. Then you need to core to enable the clock before scanning the bus. Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drivers/of/of_mdio.c needs a small modification 2020-08-29 16:00 ` Andrew Lunn @ 2020-08-29 18:01 ` Adam Rudziński 2020-08-29 23:16 ` Andrew Lunn 0 siblings, 1 reply; 12+ messages in thread From: Adam Rudziński @ 2020-08-29 18:01 UTC (permalink / raw) To: Andrew Lunn; +Cc: Florian Fainelli, netdev, robh+dt, frowand.list W dniu 2020-08-29 o 18:00, Andrew Lunn pisze: >> This is true assuming that the PHYs are always and forever connected to one >> specific MDIO bus. This is probably reasonable. Although, in i.MX the MDIO >> bus of FEC1 and FEC2 shares the pins. > In general, they do not. In fact, i don't see how that can work. The > FEC drive provides no mutual exclusion between MDIO operations on > different MDIO controllers. > (...) > You have to decide which gets to control the bus, and the other > controller is isolated. Yes, true, but that was not what I meant. In i.MX6ULL MDIO buses of both FECs share the same pins, but of course only one can be active. It makes no sense to try to use both MDIO buses if the PHYs are connected to the same wires. I agree, the current state is good from this point of view. I meant that with the split description of the mdio node the mdio bus for use in the system would be selected almost automatically. Suppose that I can do the device tree "my way": &fec2 { ... mdio { phy2 ... }; ... }; &fec1 { ... mdio { phy1 ... }; ... }; This emphasizes which PHY is intended for use by which FEC, that's why it looks more natural for me. Here, at least the FEC driver will automatically use MDIO bus from FEC2 for all the PHYs. Now suppose that I want to have a cheaper version, with only one Ethernet interface on board: //&fec2 { //... // mdio { phy2 ... }; //... //}; &fec1 { ... mdio { phy1 ... }; ... }; Now, the system will use MDIO bus from FEC1. This is just less playing with the device tree. But I agree, that you don't do that frequently, so that doesn't save much effort. And, what this example doesn't show, is that the pin assignment has to be modfied - mdio pins have to get to fec1 now. This way of building the device tree results from my proposition, but it was not my motivation. Maybe it would be more useful if the device tree would be preprocessed and then compiled based on some conditions: #if defined(CONFIG_USE_FEC2) &fec2 { ... mdio { phy2 ... }; ... }; #endif #if defined(CONFIG_USE_FEC1) &fec1 { ... mdio { phy1 ... }; ... }; #endif Best regards, Adam ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drivers/of/of_mdio.c needs a small modification 2020-08-29 18:01 ` Adam Rudziński @ 2020-08-29 23:16 ` Andrew Lunn 2020-08-30 20:47 ` Adam Rudziński 0 siblings, 1 reply; 12+ messages in thread From: Andrew Lunn @ 2020-08-29 23:16 UTC (permalink / raw) To: Adam Rudziński; +Cc: Florian Fainelli, netdev, robh+dt, frowand.list > I meant that with the split description of the mdio node the mdio bus for > use in the system would be selected almost automatically. Suppose that I can > do the device tree "my way": > &fec2 { > ... > mdio { phy2 ... }; > ... > }; > &fec1 { > ... > mdio { phy1 ... }; > ... > }; > This emphasizes which PHY is intended for use by which FEC, that's why it > looks more natural for me. And it looks really wrong to me. It suggests there are two busses, and each PHY is on its own bus. When in fact there is one MDIO bus with two PHYs. Device tree should represents the real hardware, not some pseudo description. Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drivers/of/of_mdio.c needs a small modification 2020-08-29 23:16 ` Andrew Lunn @ 2020-08-30 20:47 ` Adam Rudziński 0 siblings, 0 replies; 12+ messages in thread From: Adam Rudziński @ 2020-08-30 20:47 UTC (permalink / raw) To: Andrew Lunn; +Cc: Florian Fainelli, netdev, robh+dt, frowand.list W dniu 2020-08-30 o 01:16, Andrew Lunn pisze: >> I meant that with the split description of the mdio node the mdio bus for >> use in the system would be selected almost automatically. Suppose that I can >> do the device tree "my way": >> &fec2 { >> ... >> mdio { phy2 ... }; >> ... >> }; >> &fec1 { >> ... >> mdio { phy1 ... }; >> ... >> }; >> This emphasizes which PHY is intended for use by which FEC, that's why it >> looks more natural for me. > And it looks really wrong to me. It suggests there are two busses, and > each PHY is on its own bus. When in fact there is one MDIO bus with > two PHYs. Device tree should represents the real hardware, not some > pseudo description. > > Andrew Sure, the "split" variant may cause a misleading first impression for a human reader. Similarly, one might argue that having both PHYs under one mdio node suggests that both PHYs are connected to the same FEC. One way or another, the device tree is a pretty complex thing and requires some effort to read it correctly. The important thing is if the kernel is getting the correct information. The discussion got a bit off-topic, though. I'm not advocating any particular structure of the device tree, nor I'm saying any should be supported, as long the existing standard(s) make(s) it possible to do their job(s). Getting back to the original problem, I have tried the solution with clocks defined under phy nodes, and it didn't work. eth0 was up, but eth1 again faced "fec 2188000.ethernet eth1: Unable to connect to phy". Details below, maybe it was my fault. imx6ull.dtsi defines: fec1: ethernet@2188000 { compatible = "fsl,imx6ul-fec", "fsl,imx6q-fec"; reg = <0x2188000 0x4000>; interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>; clocks = <&clks IMX6UL_CLK_ENET>, <&clks IMX6UL_CLK_ENET_AHB>, <&clks IMX6UL_CLK_ENET_PTP>, <&clks IMX6UL_CLK_ENET_REF>, <&clks IMX6UL_CLK_ENET_REF>; clock-names = "ipg", "ahb", "ptp", "enet_clk_ref", "enet_out"; stop-mode = <&gpr 0x10 3>; fsl,num-tx-queues=<1>; fsl,num-rx-queues=<1>; fsl,magic-packet; fsl,wakeup_irq = <0>; status = "disabled"; }; fec2: ethernet@20b4000 { compatible = "fsl,imx6ul-fec", "fsl,imx6q-fec"; reg = <0x20b4000 0x4000>; interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>; clocks = <&clks IMX6UL_CLK_ENET>, <&clks IMX6UL_CLK_ENET_AHB>, <&clks IMX6UL_CLK_ENET_PTP>, <&clks IMX6UL_CLK_ENET2_REF_125M>, <&clks IMX6UL_CLK_ENET2_REF_125M>; clock-names = "ipg", "ahb", "ptp", "enet_clk_ref", "enet_out"; stop-mode = <&gpr 0x10 4>; fsl,num-tx-queues=<1>; fsl,num-rx-queues=<1>; fsl,magic-packet; fsl,wakeup_irq = <0>; status = "disabled"; }; so in my top-level dts file (which includes imx6ull.dtsi) I've tried: &fec1 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_enet1>; phy-mode = "rmii"; phy-handle = <ðphy0>; status = "okay"; }; &fec2 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_enet2>, <&pinctrl_enet2_mdio>; phy-mode = "rmii"; phy-handle = <ðphy1>; status = "okay"; mdio { #address-cells = <1>; #size-cells = <0>; ethphy0: ethernet-phy@0 { reg = <0>; clocks = <&clks IMX6UL_CLK_ENET_REF>; }; ethphy1: ethernet-phy@1 { reg = <1>; clocks = <&clks IMX6UL_CLK_ENET2_REF_125M>; }; }; }; Adding compatible = "..." and max-speed = "..." didn't change anything. Please, let me know if I omitted something important in the test, and if I should repeat it with amended device tree. Best regards, Adam ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-08-30 20:47 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <c8b74845-b9e1-6d85-3947-56333b73d756@arf.net.pl> 2020-08-28 22:28 ` drivers/of/of_mdio.c needs a small modification Andrew Lunn 2020-08-28 22:34 ` Adam Rudziński 2020-08-28 22:53 ` Andrew Lunn 2020-08-28 23:14 ` Adam Rudziński 2020-08-29 3:29 ` Florian Fainelli 2020-08-29 8:15 ` Adam Rudziński 2020-08-29 15:15 ` Andrew Lunn 2020-08-29 15:37 ` Adam Rudziński 2020-08-29 16:00 ` Andrew Lunn 2020-08-29 18:01 ` Adam Rudziński 2020-08-29 23:16 ` Andrew Lunn 2020-08-30 20:47 ` Adam Rudziński
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.