Message ID | 20180814141240.9085-1-a.fatoum@pengutronix.de |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Series | net: macb: Fix regression breaking non-MDIO fixed-link PHYs | expand |
Hello Ahmad, On Tue, Aug 14, 2018 at 04:12:40PM +0200, Ahmad Fatoum wrote: > The referenced commit broke initializing macb on the EVB-KSZ9477 eval board. > There, of_mdiobus_register was called even for the fixed-link representing > the SPI-connected switch PHY, with the result that the driver attempts to > enumerate PHYs on a non-existent MDIO bus: > > libphy: MACB_mii_bus: probed > mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address > mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0 > [snip] > mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31 > macb f0028000.ethernet: broken fixed-link specification > > Cc: <stable@vger.kernel.org> > Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup") I added the people involved in 739de9a1563a to Cc. Maybe they want to comment. So not stripping the remaining part of the original mail. Best regards Uwe > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > --- > drivers/net/ethernet/cadence/macb_main.c | 26 +++++++++++++++--------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index a6c911bb5ce2..d202a03c42ed 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -481,11 +481,6 @@ static int macb_mii_probe(struct net_device *dev) > > if (np) { > if (of_phy_is_fixed_link(np)) { > - if (of_phy_register_fixed_link(np) < 0) { > - dev_err(&bp->pdev->dev, > - "broken fixed-link specification\n"); > - return -ENODEV; > - } > bp->phy_node = of_node_get(np); > } else { > bp->phy_node = of_parse_phandle(np, "phy-handle", 0); > @@ -568,7 +563,7 @@ static int macb_mii_init(struct macb *bp) > { > struct macb_platform_data *pdata; > struct device_node *np; > - int err; > + int err = -ENXIO; > > /* Enable management port */ > macb_writel(bp, NCR, MACB_BIT(MPE)); > @@ -591,10 +586,21 @@ static int macb_mii_init(struct macb *bp) > dev_set_drvdata(&bp->dev->dev, bp->mii_bus); > > np = bp->pdev->dev.of_node; > - if (pdata) > - bp->mii_bus->phy_mask = pdata->phy_mask; > + if (np && of_phy_is_fixed_link(np)) { > + if (of_phy_register_fixed_link(np) < 0) { > + dev_err(&bp->pdev->dev, > + "broken fixed-link specification\n"); > + goto err_out_free_mdiobus; > + } > + > + err = mdiobus_register(bp->mii_bus); > + } else { > + if (pdata) > + bp->mii_bus->phy_mask = pdata->phy_mask; > + > + err = of_mdiobus_register(bp->mii_bus, np); > + } > > - err = of_mdiobus_register(bp->mii_bus, np); > if (err) > goto err_out_free_mdiobus; > > @@ -606,9 +612,9 @@ static int macb_mii_init(struct macb *bp) > > err_out_unregister_bus: > mdiobus_unregister(bp->mii_bus); > +err_out_free_mdiobus: > if (np && of_phy_is_fixed_link(np)) > of_phy_deregister_fixed_link(np); > -err_out_free_mdiobus: > of_node_put(bp->phy_node); > mdiobus_free(bp->mii_bus); > err_out: > -- > 2.18.0 > > >
Hello Ahmed, Uwe, On Tue, Aug 14, 2018 at 05:58:12PM +0200, Uwe Kleine-König wrote: > Hello Ahmad, > > > On Tue, Aug 14, 2018 at 04:12:40PM +0200, Ahmad Fatoum wrote: > > The referenced commit broke initializing macb on the EVB-KSZ9477 eval board. > > There, of_mdiobus_register was called even for the fixed-link representing > > the SPI-connected switch PHY, with the result that the driver attempts to > > enumerate PHYs on a non-existent MDIO bus: > > > > libphy: MACB_mii_bus: probed > > mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address > > mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0 > > [snip] > > mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31 > > macb f0028000.ethernet: broken fixed-link specification > > > > Cc: <stable@vger.kernel.org> > > Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup") > > I added the people involved in 739de9a1563a to Cc. Maybe they want to > comment. So not stripping the remaining part of the original mail. > > Best regards > Uwe You should probably prod Andrew Lunn, he suggested that I move the fixed link code from macb_mii_init() to _probe(). Here, you're at least partially directly undoing that. (ref: https://www.mail-archive.com/netdev@vger.kernel.org/msg221018.html) > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > > --- > > drivers/net/ethernet/cadence/macb_main.c | 26 +++++++++++++++--------- > > 1 file changed, 16 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > > index a6c911bb5ce2..d202a03c42ed 100644 > > --- a/drivers/net/ethernet/cadence/macb_main.c > > +++ b/drivers/net/ethernet/cadence/macb_main.c > > @@ -481,11 +481,6 @@ static int macb_mii_probe(struct net_device *dev) > > > > if (np) { > > if (of_phy_is_fixed_link(np)) { > > - if (of_phy_register_fixed_link(np) < 0) { > > - dev_err(&bp->pdev->dev, > > - "broken fixed-link specification\n"); > > - return -ENODEV; > > - } > > bp->phy_node = of_node_get(np); > > } else { > > bp->phy_node = of_parse_phandle(np, "phy-handle", 0); > > @@ -568,7 +563,7 @@ static int macb_mii_init(struct macb *bp) > > { > > struct macb_platform_data *pdata; > > struct device_node *np; > > - int err; > > + int err = -ENXIO; > > > > /* Enable management port */ > > macb_writel(bp, NCR, MACB_BIT(MPE)); > > @@ -591,10 +586,21 @@ static int macb_mii_init(struct macb *bp) > > dev_set_drvdata(&bp->dev->dev, bp->mii_bus); > > > > np = bp->pdev->dev.of_node; > > - if (pdata) > > - bp->mii_bus->phy_mask = pdata->phy_mask; > > + if (np && of_phy_is_fixed_link(np)) { > > + if (of_phy_register_fixed_link(np) < 0) { > > + dev_err(&bp->pdev->dev, > > + "broken fixed-link specification\n"); > > + goto err_out_free_mdiobus; > > + } > > + > > + err = mdiobus_register(bp->mii_bus); > > + } else { > > + if (pdata) > > + bp->mii_bus->phy_mask = pdata->phy_mask; > > + > > + err = of_mdiobus_register(bp->mii_bus, np); > > + } > > > > - err = of_mdiobus_register(bp->mii_bus, np); > > if (err) > > goto err_out_free_mdiobus; > > > > @@ -606,9 +612,9 @@ static int macb_mii_init(struct macb *bp) > > > > err_out_unregister_bus: > > mdiobus_unregister(bp->mii_bus); > > +err_out_free_mdiobus: > > if (np && of_phy_is_fixed_link(np)) > > of_phy_deregister_fixed_link(np); > > -err_out_free_mdiobus: > > of_node_put(bp->phy_node); > > mdiobus_free(bp->mii_bus); > > err_out: > > -- > > 2.18.0 > > > > > > > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://urldefense.proofpoint.com/v2/url?u=http-3A__www.pengutronix.de_&d=DwIDAw&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=8iZb7YNOSMVIG_mTIHDL03ZcObgQI_gGlWrSewdGETA&m=IQAK1YsKs7Z2bvZxuajiSXw3asFiEztQKYkvy-LpBn8&s=XKrOhFxQshoEcDxMZVSATnJW2cbaD16mQofKdEbJVW0&e= |
On Tue, Aug 14, 2018 at 05:58:12PM +0200, Uwe Kleine-König wrote: > Hello Ahmad, > > > On Tue, Aug 14, 2018 at 04:12:40PM +0200, Ahmad Fatoum wrote: > > The referenced commit broke initializing macb on the EVB-KSZ9477 eval board. > > There, of_mdiobus_register was called even for the fixed-link representing > > the SPI-connected switch PHY, with the result that the driver attempts to > > enumerate PHYs on a non-existent MDIO bus: > > > > libphy: MACB_mii_bus: probed > > mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address > > mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0 > > [snip] > > mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31 > > macb f0028000.ethernet: broken fixed-link specification > > > > Cc: <stable@vger.kernel.org> > > Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup") > > I added the people involved in 739de9a1563a to Cc. Maybe they want to > comment. So not stripping the remaining part of the original mail. Thanks Uwe for Cc: in my. Ahmed, where is the device tree for the EVB-KSZ9477? Thanks Andrew
Hi, On Wed, Aug 15, 2018 at 3:35 AM Andrew Lunn <andrew@lunn.ch> wrote: > > On Tue, Aug 14, 2018 at 05:58:12PM +0200, Uwe Kleine-König wrote: > > Hello Ahmad, > > > > > > On Tue, Aug 14, 2018 at 04:12:40PM +0200, Ahmad Fatoum wrote: > > > The referenced commit broke initializing macb on the EVB-KSZ9477 eval board. > > > There, of_mdiobus_register was called even for the fixed-link representing > > > the SPI-connected switch PHY, with the result that the driver attempts to > > > enumerate PHYs on a non-existent MDIO bus: > > > I ran into a similar problem on v14.4 for davinci_mdio I had to patch it with [1]. The cpsw has 2 phys one phy is connected to KSZ9031 and other to ksz9897 Ethernet switch which is treated as a fixed phy with no mdio lines because of which mdio_read/write failed. This didn’t happen in v4.9.x something in core has changed ? [1] diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c index 3e84107..197baa6 100644 --- a/drivers/net/ethernet/ti/davinci_mdio.c +++ b/drivers/net/ethernet/ti/davinci_mdio.c @@ -245,6 +245,13 @@ static int davinci_mdio_read(struct mii_bus *bus, int phy_id, int phy_reg) u32 reg; int ret; + if (phy_id == 2) + return 0; + if (phy_reg & ~PHY_REG_MASK || phy_id & ~PHY_ID_MASK) return -EINVAL; @@ -289,6 +296,13 @@ static int davinci_mdio_write(struct mii_bus *bus, int phy_id, u32 reg; int ret; + if (phy_id == 2) + return 0; + if (phy_reg & ~PHY_REG_MASK || phy_id & ~PHY_ID_MASK) return -EINVAL; Cheers, --Prabhakar Lad
On 08/15/2018 04:32 AM, Andrew Lunn wrote:
> Ahmed, where is the device tree for the EVB-KSZ9477?
I've attached it [1]. It's still work-in-progress (DSA doesn't work yet for example), but Ethernet is usable with Linux v4.18 and my patch applied.
Cheers
Ahmad
[1]
=======================================
diff --git a/arch/arm/boot/dts/at91-sama5d3_xplained_ung8087.dts b/arch/arm/boot/dts/at91-sama5d3_xplained_ung8087.dts
new file mode 100644
index 000000000000..164b6a47d0bf
--- /dev/null
+++ b/arch/arm/boot/dts/at91-sama5d3_xplained_ung8087.dts
@@ -0,0 +1,393 @@
+/*
+ * at91-sama5d3_xplained_ung8087.dts - Device Tree file for the EVB-KSZ9477 board
+ *
+ * Copyright (C) 2014 Atmel,
+ * 2014 Nicolas Ferre <nicolas.ferre@atmel.com>
+ * 2018 Ahmad Fatoum <a.fatoum@pengutronix.de>
+ *
+ * Licensed under GPLv2 or later.
+ */
+/dts-v1/;
+#include "sama5d36.dtsi"
+
+/ {
+ model = "SAMA5D3 Xplained";
+ compatible = "atmel,sama5d3-xplained", "atmel,sama5d3", "atmel,sama5";
+
+ chosen {
+ bootargs = "console=ttyS0,115200";
+ };
+
+ memory {
+ reg = <0x20000000 0x10000000>;
+ };
+
+ clocks {
+ slow_xtal {
+ clock-frequency = <32768>;
+ };
+
+ main_xtal {
+ clock-frequency = <12000000>;
+ };
+ };
+
+ ahb {
+ apb {
+ mmc0: mmc@f0000000 {
+ pinctrl-0 = <&pinctrl_mmc0_clk_cmd_dat0 &pinctrl_mmc0_dat1_3 &pinctrl_mmc0_dat4_7 &pinctrl_mmc0_cd>;
+ status = "okay";
+ slot@0 {
+ reg = <0>;
+ bus-width = <8>;
+ cd-gpios = <&pioE 0 GPIO_ACTIVE_LOW>;
+ };
+ };
+
+ spi0: spi@f0004000 {
+ cs-gpios = <&pioD 13 0>, <0>, <0>, <&pioD 16 0>;
+ id = <0>;
+ status = "okay";
+ };
+
+ can0: can@f000c000 {
+ status = "okay";
+ };
+
+ i2c0: i2c@f0014000 {
+ pinctrl-0 = <&pinctrl_i2c0_pu>;
+ status = "okay";
+ };
+
+ i2c1: i2c@f0018000 {
+ status = "okay";
+
+ pmic: act8865@5b {
+ compatible = "active-semi,act8865";
+ reg = <0x5b>;
+ status = "okay";
+
+ regulators {
+ vcc_1v8_reg: DCDC_REG1 {
+ regulator-name = "VCC_1V8";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-always-on;
+ };
+
+ vcc_1v2_reg: DCDC_REG2 {
+ regulator-name = "VCC_1V2";
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ regulator-always-on;
+ };
+
+ vcc_3v3_reg: DCDC_REG3 {
+ regulator-name = "VCC_3V3";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ };
+
+ vddfuse_reg: LDO_REG1 {
+ regulator-name = "FUSE_2V5";
+ regulator-min-microvolt = <2500000>;
+ regulator-max-microvolt = <2500000>;
+ };
+
+ vddana_reg: LDO_REG2 {
+ regulator-name = "VDDANA";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ };
+ };
+ };
+ };
+
+ macb0: ethernet@f0028000 {
+ phy-mode = "rgmii";
+ gpios = <&pioB 28 GPIO_ACTIVE_LOW>;
+ reset-gpios = <&pioC 31 GPIO_ACTIVE_LOW>;
+ status = "okay";
+
+ fixed-link {
+ speed = <1000>;
+ full-duplex;
+ };
+ };
+
+ pwm0: pwm@f002c000 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_pwm0_pwmh0_0 &pinctrl_pwm0_pwmh1_0>;
+ status = "okay";
+ };
+
+ usart0: serial@f001c000 {
+ status = "okay";
+ };
+
+ usart1: serial@f0020000 {
+ pinctrl-0 = <&pinctrl_usart1 &pinctrl_usart1_rts_cts>;
+ status = "okay";
+ };
+
+ uart0: serial@f0024000 {
+ status = "okay";
+ };
+
+ mmc1: mmc@f8000000 {
+ pinctrl-0 = <&pinctrl_mmc1_clk_cmd_dat0 &pinctrl_mmc1_dat1_3 &pinctrl_mmc1_cd>;
+ status = "okay";
+ slot@0 {
+ reg = <0>;
+ bus-width = <4>;
+ cd-gpios = <&pioE 1 GPIO_ACTIVE_HIGH>;
+ };
+ };
+
+ spi1: spi@f8008000 {
+ pinctrl-0 = <&pinctrl_spi_ksz>;
+ cs-gpios = <&pioC 25 0>;
+ id = <1>;
+ status = "okay";
+
+ ksz9477: ksz9477@0 {
+ compatible = "microchip,ksz9477", "microchip,ksz9893";
+ reg = <0>;
+
+ /* Bus clock is 132 MHz. */
+ spi-max-frequency = <44000000>;
+ spi-cpha;
+ spi-cpol;
+ gpios = <&pioB 28 GPIO_ACTIVE_LOW>;
+ status = "okay";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ label = "lan0";
+ };
+
+ port@1 {
+ reg = <1>;
+ label = "lan1";
+ };
+
+ port@2 {
+ reg = <2>;
+ label = "lan2";
+ };
+
+ port@3 {
+ reg = <3>;
+ label = "lan3";
+ };
+
+ port@4 {
+ reg = <4>;
+ label = "lan4";
+ };
+
+ port@5 {
+ reg = <5>;
+ label = "cpu";
+ ethernet = <&macb0>;
+ phy-mode = "rgmii-id";
+
+ fixed-link {
+ speed = <0x3e8>;
+ full-duplex;
+ };
+ };
+
+ /* port 6 is connected to eth0 */
+ };
+ };
+ };
+
+ adc0: adc@f8018000 {
+ pinctrl-0 = <
+ &pinctrl_adc0_adtrg
+ &pinctrl_adc0_ad0
+ &pinctrl_adc0_ad1
+ &pinctrl_adc0_ad2
+ &pinctrl_adc0_ad3
+ &pinctrl_adc0_ad4
+ &pinctrl_adc0_ad5
+ &pinctrl_adc0_ad6
+ &pinctrl_adc0_ad7
+ &pinctrl_adc0_ad8
+ &pinctrl_adc0_ad9
+ >;
+ status = "okay";
+ };
+
+ i2c2: i2c@f801c000 {
+ dmas = <0>, <0>; /* Do not use DMA for i2c2 */
+ pinctrl-0 = <&pinctrl_i2c2_pu>;
+ status = "okay";
+ };
+
+ macb1: ethernet@f802c000 {
+ phy-mode = "rmii";
+ status = "disabled";
+ };
+
+ dbgu: serial@ffffee00 {
+ status = "okay";
+ };
+
+ pinctrl@fffff200 {
+ board {
+ pinctrl_i2c0_pu: i2c0_pu {
+ atmel,pins =
+ <AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>,
+ <AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
+ };
+
+ pinctrl_i2c2_pu: i2c2_pu {
+ atmel,pins =
+ <AT91_PIOA 18 AT91_PERIPH_B AT91_PINCTRL_PULL_UP>,
+ <AT91_PIOA 19 AT91_PERIPH_B AT91_PINCTRL_PULL_UP>;
+ };
+
+ pinctrl_mmc0_cd: mmc0_cd {
+ atmel,pins =
+ <AT91_PIOE 0 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP_DEGLITCH>;
+ };
+
+ pinctrl_mmc1_cd: mmc1_cd {
+ atmel,pins =
+ <AT91_PIOE 1 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP_DEGLITCH>;
+ };
+
+ pinctrl_usba_vbus: usba_vbus {
+ atmel,pins =
+ <AT91_PIOE 9 AT91_PERIPH_GPIO AT91_PINCTRL_DEGLITCH>; /* PE9, conflicts with A9 */
+ };
+
+ pinctrl_spi_ksz: spi_ksz {
+ atmel,pins =
+ <
+ AT91_PIOB 28 AT91_PERIPH_GPIO AT91_PINCTRL_DEGLITCH
+ AT91_PIOC 31 AT91_PERIPH_GPIO AT91_PINCTRL_DEGLITCH
+ >;
+ };
+ };
+ };
+
+ pmc: pmc@fffffc00 {
+ main: mainck {
+ clock-frequency = <12000000>;
+ };
+ };
+ };
+
+ ebi: ebi@10000000 {
+ pinctrl-0 = <&pinctrl_ebi_nand_addr>;
+ pinctrl-names = "default";
+ status = "okay";
+
+ nand_controller: nand-controller {
+ status = "okay";
+
+ nand@3 {
+ reg = <0x3 0x0 0x2>;
+ atmel,rb = <0>;
+ nand-bus-width = <8>;
+ nand-ecc-mode = "hw";
+ nand-ecc-strength = <4>;
+ nand-ecc-step-size = <512>;
+ nand-on-flash-bbt;
+ label = "atmel_nand";
+
+ partitions {
+ compatible = "fixed-partitions";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ at91bootstrap@0 {
+ label = "at91bootstrap";
+ reg = <0x0 0x40000>;
+ };
+
+ bootloader@40000 {
+ label = "bootloader";
+ reg = <0x40000 0x80000>;
+ };
+
+ bootloaderenv@c0000 {
+ label = "bootloader env";
+ reg = <0xc0000 0xc0000>;
+ };
+
+ dtb@180000 {
+ label = "device tree";
+ reg = <0x180000 0x80000>;
+ };
+
+ kernel@200000 {
+ label = "kernel";
+ reg = <0x200000 0x600000>;
+ };
+
+ rootfs@800000 {
+ label = "rootfs";
+ reg = <0x800000 0x0f800000>;
+ };
+ };
+ };
+ };
+ };
+
+ usb0: gadget@500000 {
+ atmel,vbus-gpio = <&pioE 9 GPIO_ACTIVE_HIGH>; /* PE9, conflicts with A9 */
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usba_vbus>;
+ status = "okay";
+ };
+
+ usb1: ohci@600000 {
+ num-ports = <3>;
+ atmel,vbus-gpio = <0
+ &pioE 3 GPIO_ACTIVE_LOW
+ &pioE 4 GPIO_ACTIVE_LOW
+ >;
+ status = "okay";
+ };
+
+ usb2: ehci@700000 {
+ status = "okay";
+ };
+ };
+
+ gpio_keys {
+ compatible = "gpio-keys";
+
+ bp3 {
+ label = "PB_USER";
+ gpios = <&pioE 29 GPIO_ACTIVE_LOW>;
+ linux,code = <0x104>;
+ gpio-key,wakeup;
+ };
+ };
+
+ leds {
+ compatible = "gpio-leds";
+
+ d2 {
+ label = "d2";
+ gpios = <&pioE 23 GPIO_ACTIVE_LOW>; /* PE23, conflicts with A23, CTS2 */
+ linux,default-trigger = "heartbeat";
+ };
+
+ d3 {
+ label = "d3";
+ gpios = <&pioE 24 GPIO_ACTIVE_HIGH>;
+ };
+ };
+};
On Thu, Aug 16, 2018 at 08:54:40AM +0200, Ahmad Fatoum wrote: > On 08/15/2018 04:32 AM, Andrew Lunn wrote: > > Ahmed, where is the device tree for the EVB-KSZ9477? > > I've attached it [1]. It's still work-in-progress (DSA doesn't work yet for example), but Ethernet is usable with Linux v4.18 and my patch applied. > Thanks. So the problem is, macb does not put phy DT nodes inside an mdio subnode. It places them directly in the MAC node. So of_mdiobus_register() is being called with the MAC np. of_mdiobus_register() then looks for children of the MAC node, assuming they are phys. But when you have a fixed phy node, it is not a phy, it does not have a reg property, and you get these warnings. There are cases when you need both fixed-phy and a mdio bus. e.g. a DSA switch hanging off MDIO. So we have a few things here... 1) A regression. We should find a fix for that. Maybe we should special case a child node called 'fixed-link' in of_mdiobus_register(). I would suggest adding a single warning if such node is found. 2) Missing functionality. Add support for an mdio container node. node = of_get_child_by_name(np, "mdio"); if (node) err = of_mdiobus_register(bp->mii_bus, node); else err = of_mdiobus_register(bp->mii_bus, np); 3) Modify the existing dts files to make use of this container. Because of backwards compatibility, we cannot force the use of it, but we can encourage it. Andrew
On 08/16/2018 04:24 PM, Andrew Lunn wrote: > 1) A regression. We should find a fix for that. Maybe we should > special case a child node called 'fixed-link' in > of_mdiobus_register(). I would suggest adding a single warning if > such node is found. I just sent out a v2, which warns if fixed-link is encountered in of_mdiobus_register(). The actual fixed-link handling remains in the macb driver, because I think it's would be out of scope for a regression fix to change where fixed-links are handled for all using drivers... > 2) Missing functionality. Add support for an mdio container node. > > node = of_get_child_by_name(np, "mdio"); > if (node) > err = of_mdiobus_register(bp->mii_bus, node); > else > err = of_mdiobus_register(bp->mii_bus, np); Done. > 3) Modify the existing dts files to make use of this container. > Because of backwards compatibility, we cannot force the use of it, > but we can encourage it. Done as well. Cheers Ahmad
On 08/15/2018 03:59 PM, Lad, Prabhakar wrote:
> This didn’t happen in v4.9.x something in core has changed ?
In my case, it was the macb driver that changed. I found the offending commit
with git-bisect, you might want to give it a try.
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index a6c911bb5ce2..d202a03c42ed 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -481,11 +481,6 @@ static int macb_mii_probe(struct net_device *dev) if (np) { if (of_phy_is_fixed_link(np)) { - if (of_phy_register_fixed_link(np) < 0) { - dev_err(&bp->pdev->dev, - "broken fixed-link specification\n"); - return -ENODEV; - } bp->phy_node = of_node_get(np); } else { bp->phy_node = of_parse_phandle(np, "phy-handle", 0); @@ -568,7 +563,7 @@ static int macb_mii_init(struct macb *bp) { struct macb_platform_data *pdata; struct device_node *np; - int err; + int err = -ENXIO; /* Enable management port */ macb_writel(bp, NCR, MACB_BIT(MPE)); @@ -591,10 +586,21 @@ static int macb_mii_init(struct macb *bp) dev_set_drvdata(&bp->dev->dev, bp->mii_bus); np = bp->pdev->dev.of_node; - if (pdata) - bp->mii_bus->phy_mask = pdata->phy_mask; + if (np && of_phy_is_fixed_link(np)) { + if (of_phy_register_fixed_link(np) < 0) { + dev_err(&bp->pdev->dev, + "broken fixed-link specification\n"); + goto err_out_free_mdiobus; + } + + err = mdiobus_register(bp->mii_bus); + } else { + if (pdata) + bp->mii_bus->phy_mask = pdata->phy_mask; + + err = of_mdiobus_register(bp->mii_bus, np); + } - err = of_mdiobus_register(bp->mii_bus, np); if (err) goto err_out_free_mdiobus; @@ -606,9 +612,9 @@ static int macb_mii_init(struct macb *bp) err_out_unregister_bus: mdiobus_unregister(bp->mii_bus); +err_out_free_mdiobus: if (np && of_phy_is_fixed_link(np)) of_phy_deregister_fixed_link(np); -err_out_free_mdiobus: of_node_put(bp->phy_node); mdiobus_free(bp->mii_bus); err_out:
The referenced commit broke initializing macb on the EVB-KSZ9477 eval board. There, of_mdiobus_register was called even for the fixed-link representing the SPI-connected switch PHY, with the result that the driver attempts to enumerate PHYs on a non-existent MDIO bus: libphy: MACB_mii_bus: probed mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0 [snip] mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31 macb f0028000.ethernet: broken fixed-link specification Cc: <stable@vger.kernel.org> Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup") Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/net/ethernet/cadence/macb_main.c | 26 +++++++++++++++--------- 1 file changed, 16 insertions(+), 10 deletions(-)