| Message ID | 20170826073311.25612-5-clabbe.montjoie@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Corentin
I think we have now all agreed this is an mdio-mux, plus it is also an
MII mux. We should represent that in device tree. This patchset does
this. However, as it is now, the mux structure in DT is ignored. All
it does is search for the phy-is-integrated flags and goes on that.
I made the comment that the device tree representation cannot be
implemented using an MDIO mux driver, because of driver loading
issues. However, the core of the MDIO mux code is just a library,
symbols exported as GPL, free for anything to use.
What i think should happen is the mdio-mux is implemented inside the
MAC driver, using the mux-core as a library. The device tree structure
of a mix is then reflected within Linux. The mux switch callback is
implemented within the MAC driver. So it can reset the MAC when the
mux is switched. The 'phy-is-integrated' property is then no longer
needed.
I would suggest a binding something like:
emac: ethernet@1c0b000 {
compatible = "allwinner,sun8i-h3-emac";
syscon = <&syscon>;
reg = <0x01c0b000 0x104>;
interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
interrupt-names = "macirq";
resets = <&ccu RST_BUS_EMAC>;
reset-names = "stmmaceth";
clocks = <&ccu CLK_BUS_EMAC>;
clock-names = "stmmaceth";
#address-cells = <1>;
#size-cells = <0>;
phy-handle = <&int_mii_phy>;
phy-mode = "mii";
allwinner,leds-active-low;
mdio: mdio {
#address-cells = <1>;
#size-cells = <0>;
}
mdio-mux {
#address-cells = <1>;
#size-cells = <0>;
mdio@0 {
reg = <0>;
#address-cells = <1>;
#size-cells = <0>;
int_mii_phy: ethernet-phy@1 {
reg = <1>;
clocks = <&ccu CLK_BUS_EPHY>;
resets = <&ccu RST_BUS_EPHY>;
};
};
ext_mdio: mdio@0 {
#address-cells = <1>;
#size-cells = <0>;
ext_rgmii_phy: ethernet-phy@1 {
reg = <1>;
};
};
};
};
Andrew
On Sat, Aug 26, 2017 at 11:20:51PM +0200, Andrew Lunn wrote: > Hi Corentin > > I think we have now all agreed this is an mdio-mux, plus it is also an > MII mux. We should represent that in device tree. This patchset does > this. However, as it is now, the mux structure in DT is ignored. All > it does is search for the phy-is-integrated flags and goes on that. > > I made the comment that the device tree representation cannot be > implemented using an MDIO mux driver, because of driver loading > issues. However, the core of the MDIO mux code is just a library, > symbols exported as GPL, free for anything to use. > > What i think should happen is the mdio-mux is implemented inside the > MAC driver, using the mux-core as a library. The device tree structure > of a mix is then reflected within Linux. The mux switch callback is > implemented within the MAC driver. So it can reset the MAC when the > mux is switched. The 'phy-is-integrated' property is then no longer > needed. It is stilll needed because some settings (allwinner,leds-active-low for example) are only for integrated phy. > > I would suggest a binding something like: > > emac: ethernet@1c0b000 { > compatible = "allwinner,sun8i-h3-emac"; > syscon = <&syscon>; > reg = <0x01c0b000 0x104>; > interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "macirq"; > resets = <&ccu RST_BUS_EMAC>; > reset-names = "stmmaceth"; > clocks = <&ccu CLK_BUS_EMAC>; > clock-names = "stmmaceth"; > #address-cells = <1>; > #size-cells = <0>; > > phy-handle = <&int_mii_phy>; > phy-mode = "mii"; > allwinner,leds-active-low; > > mdio: mdio { > #address-cells = <1>; > #size-cells = <0>; > } > > mdio-mux { > #address-cells = <1>; > #size-cells = <0>; > > mdio@0 { > reg = <0>; > #address-cells = <1>; > #size-cells = <0>; > > int_mii_phy: ethernet-phy@1 { > reg = <1>; > clocks = <&ccu CLK_BUS_EPHY>; > resets = <&ccu RST_BUS_EPHY>; > }; > }; > ext_mdio: mdio@0 { > #address-cells = <1>; > #size-cells = <0>; > > ext_rgmii_phy: ethernet-phy@1 { > reg = <1>; > }; > }; > }; > }; > I am trying to do that but I get: dwmac-sun8i 1c30000.ethernet: Error: Failed to find reg for child /soc/ethernet@1c30000/mdio dwmac-sun8i 1c30000.ethernet: Error: Failed to find reg for child /soc/ethernet@1c30000/mdio-mux dwmac-sun8i 1c30000.ethernet: Error: No acceptable child buses found So it seems that mdio_mux_init() must be run on mdio-mux and not on emac node. But in the current state it cannot be done. Do you agree that another mdio_mux_init() must be written ? (taking a of_node (in our case: mdio-mux) instead of a device) Or do I miss something ? Regards
> Do you agree that another mdio_mux_init() must be written ? (taking > a of_node (in our case: mdio-mux) instead of a device) Yes, you are correct, it is using dev->of_node. So you need to refactor the code a little, to allow an of_node to be passed. Andrew
On Sat, Aug 26, 2017 at 11:20:51PM +0200, Andrew Lunn wrote: > Hi Corentin > > I think we have now all agreed this is an mdio-mux, plus it is also an > MII mux. We should represent that in device tree. This patchset does > this. However, as it is now, the mux structure in DT is ignored. All > it does is search for the phy-is-integrated flags and goes on that. > > I made the comment that the device tree representation cannot be > implemented using an MDIO mux driver, because of driver loading > issues. However, the core of the MDIO mux code is just a library, > symbols exported as GPL, free for anything to use. > > What i think should happen is the mdio-mux is implemented inside the > MAC driver, using the mux-core as a library. The device tree structure > of a mix is then reflected within Linux. The mux switch callback is > implemented within the MAC driver. So it can reset the MAC when the > mux is switched. The 'phy-is-integrated' property is then no longer > needed. > > I would suggest a binding something like: This is looks better to me, but... > emac: ethernet@1c0b000 { > compatible = "allwinner,sun8i-h3-emac"; > syscon = <&syscon>; > reg = <0x01c0b000 0x104>; > interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "macirq"; > resets = <&ccu RST_BUS_EMAC>; > reset-names = "stmmaceth"; > clocks = <&ccu CLK_BUS_EMAC>; > clock-names = "stmmaceth"; > #address-cells = <1>; > #size-cells = <0>; > > phy-handle = <&int_mii_phy>; > phy-mode = "mii"; > allwinner,leds-active-low; > > mdio: mdio { > #address-cells = <1>; > #size-cells = <0>; > } Why do you need this node still? > > mdio-mux { > #address-cells = <1>; > #size-cells = <0>; > > mdio@0 { > reg = <0>; > #address-cells = <1>; > #size-cells = <0>; > > int_mii_phy: ethernet-phy@1 { > reg = <1>; > clocks = <&ccu CLK_BUS_EPHY>; > resets = <&ccu RST_BUS_EPHY>; > }; > }; > ext_mdio: mdio@0 { > #address-cells = <1>; > #size-cells = <0>; > > ext_rgmii_phy: ethernet-phy@1 { > reg = <1>; > }; > }; > }; > }; > > Andrew
On Thu, Aug 31, 2017 at 03:18:03PM -0500, Rob Herring wrote: > On Sat, Aug 26, 2017 at 11:20:51PM +0200, Andrew Lunn wrote: > > Hi Corentin > > > > I think we have now all agreed this is an mdio-mux, plus it is also an > > MII mux. We should represent that in device tree. This patchset does > > this. However, as it is now, the mux structure in DT is ignored. All > > it does is search for the phy-is-integrated flags and goes on that. > > > > I made the comment that the device tree representation cannot be > > implemented using an MDIO mux driver, because of driver loading > > issues. However, the core of the MDIO mux code is just a library, > > symbols exported as GPL, free for anything to use. > > > > What i think should happen is the mdio-mux is implemented inside the > > MAC driver, using the mux-core as a library. The device tree structure > > of a mix is then reflected within Linux. The mux switch callback is > > implemented within the MAC driver. So it can reset the MAC when the > > mux is switched. The 'phy-is-integrated' property is then no longer > > needed. > > > > I would suggest a binding something like: > > This is looks better to me, but... > > > emac: ethernet@1c0b000 { > > compatible = "allwinner,sun8i-h3-emac"; > > syscon = <&syscon>; > > reg = <0x01c0b000 0x104>; > > interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>; > > interrupt-names = "macirq"; > > resets = <&ccu RST_BUS_EMAC>; > > reset-names = "stmmaceth"; > > clocks = <&ccu CLK_BUS_EMAC>; > > clock-names = "stmmaceth"; > > #address-cells = <1>; > > #size-cells = <0>; > > > > phy-handle = <&int_mii_phy>; > > phy-mode = "mii"; > > allwinner,leds-active-low; > > > > mdio: mdio { > > #address-cells = <1>; > > #size-cells = <0>; > > } > > Why do you need this node still? Hi Rob It might not be needed, depending on how it is implemented. But: Documentation/devicetree/bindings/net/mdio-mux.txt It is normal for an mdio bus mux to have a phandle back to the parent mdio bus. Also, i think the stmmac driver will only instantiate the mdio bus if there is a node for it in the device tree. Andrew
On Thu, Aug 31, 2017 at 3:59 PM, Andrew Lunn <andrew@lunn.ch> wrote: > On Thu, Aug 31, 2017 at 03:18:03PM -0500, Rob Herring wrote: >> On Sat, Aug 26, 2017 at 11:20:51PM +0200, Andrew Lunn wrote: >> > Hi Corentin >> > >> > I think we have now all agreed this is an mdio-mux, plus it is also an >> > MII mux. We should represent that in device tree. This patchset does >> > this. However, as it is now, the mux structure in DT is ignored. All >> > it does is search for the phy-is-integrated flags and goes on that. >> > >> > I made the comment that the device tree representation cannot be >> > implemented using an MDIO mux driver, because of driver loading >> > issues. However, the core of the MDIO mux code is just a library, >> > symbols exported as GPL, free for anything to use. >> > >> > What i think should happen is the mdio-mux is implemented inside the >> > MAC driver, using the mux-core as a library. The device tree structure >> > of a mix is then reflected within Linux. The mux switch callback is >> > implemented within the MAC driver. So it can reset the MAC when the >> > mux is switched. The 'phy-is-integrated' property is then no longer >> > needed. >> > >> > I would suggest a binding something like: >> >> This is looks better to me, but... >> >> > emac: ethernet@1c0b000 { >> > compatible = "allwinner,sun8i-h3-emac"; >> > syscon = <&syscon>; >> > reg = <0x01c0b000 0x104>; >> > interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>; >> > interrupt-names = "macirq"; >> > resets = <&ccu RST_BUS_EMAC>; >> > reset-names = "stmmaceth"; >> > clocks = <&ccu CLK_BUS_EMAC>; >> > clock-names = "stmmaceth"; >> > #address-cells = <1>; >> > #size-cells = <0>; >> > >> > phy-handle = <&int_mii_phy>; >> > phy-mode = "mii"; >> > allwinner,leds-active-low; >> > >> > mdio: mdio { >> > #address-cells = <1>; >> > #size-cells = <0>; >> > } >> >> Why do you need this node still? > > Hi Rob > > It might not be needed, depending on how it is implemented. But: > > Documentation/devicetree/bindings/net/mdio-mux.txt > > It is normal for an mdio bus mux to have a phandle back to the parent > mdio bus. Also, i think the stmmac driver will only instantiate the > mdio bus if there is a node for it in the device tree. You don't have a phandle to the parent mdio bus though. I think we should allow for both case. The mux could be within the bus hierarchy. Depends where the controls are. Another alternative someone will try sooner or later is using the new mux control binding. Rob
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c index 675a09629d85..c353e5bcb3c1 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c @@ -41,14 +41,14 @@ * This value is used for disabling properly EMAC * and used as a good starting value in case of the * boot process(uboot) leave some stuff. - * @internal_phy: Does the MAC embed an internal PHY + * @soc_has_internal_phy: Does the MAC embed an internal PHY * @support_mii: Does the MAC handle MII * @support_rmii: Does the MAC handle RMII * @support_rgmii: Does the MAC handle RGMII */ struct emac_variant { u32 default_syscon_value; - int internal_phy; + bool soc_has_internal_phy; bool support_mii; bool support_rmii; bool support_rgmii; @@ -75,7 +75,7 @@ struct sunxi_priv_data { static const struct emac_variant emac_variant_h3 = { .default_syscon_value = 0x58000, - .internal_phy = PHY_INTERFACE_MODE_MII, + .soc_has_internal_phy = true, .support_mii = true, .support_rmii = true, .support_rgmii = true @@ -83,20 +83,20 @@ static const struct emac_variant emac_variant_h3 = { static const struct emac_variant emac_variant_v3s = { .default_syscon_value = 0x38000, - .internal_phy = PHY_INTERFACE_MODE_MII, + .soc_has_internal_phy = true, .support_mii = true }; static const struct emac_variant emac_variant_a83t = { .default_syscon_value = 0, - .internal_phy = 0, + .soc_has_internal_phy = false, .support_mii = true, .support_rgmii = true }; static const struct emac_variant emac_variant_a64 = { .default_syscon_value = 0, - .internal_phy = 0, + .soc_has_internal_phy = false, .support_mii = true, .support_rmii = true, .support_rgmii = true @@ -648,7 +648,7 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv) "Current syscon value is not the default %x (expect %x)\n", val, reg); - if (gmac->variant->internal_phy) { + if (gmac->variant->soc_has_internal_phy) { if (!gmac->use_internal_phy) { /* switch to external PHY interface */ reg &= ~H3_EPHY_SELECT; @@ -933,7 +933,7 @@ static int sun8i_dwmac_probe(struct platform_device *pdev) } plat_dat->interface = of_get_phy_mode(dev->of_node); - if (plat_dat->interface == gmac->variant->internal_phy) { + if (of_property_read_bool(plat_dat->phy_node, "phy-is-integrated")) { dev_info(&pdev->dev, "Will use internal PHY\n"); gmac->use_internal_phy = true; gmac->ephy_clk = of_clk_get(plat_dat->phy_node, 0);