| Message ID | 1411474536-22626-4-git-send-email-antoine.tenart@free-electrons.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tuesday 23 September 2014 14:15:30 Antoine Tenart wrote: > +Optional properties: > +- port-id: should be '0','1' or '2'. > +- phy-addr: MDIO address of the PHY. > +- local-mac-address: see ethernet.txt file in the same directory. > + > I believe new bindings should not use "phy-addr" properties, but instead use a phy-handle property pointing to a phy device instead. The port-id property description could be a little more verbose. What do those numbers actually mean? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arnd, On Tue, Sep 23, 2014 at 02:38:39PM +0200, Arnd Bergmann wrote: > On Tuesday 23 September 2014 14:15:30 Antoine Tenart wrote: > > +Optional properties: > > +- port-id: should be '0','1' or '2'. > > +- phy-addr: MDIO address of the PHY. > > +- local-mac-address: see ethernet.txt file in the same directory. > > + > > > > I believe new bindings should not use "phy-addr" properties, but > instead use a phy-handle property pointing to a phy device instead. That's the MDIO address of the PHY. It's a bit difficult to use a phy device as the driver uses the libphy. And that would break the platform using it. Or maybe you have something in mind? > > The port-id property description could be a little more verbose. > What do those numbers actually mean? I'll add some description. That's the Ethernet port number. I reused a property (port-id) already used by other drivers. Antoine
On Tue, Sep 23, 2014 at 04:01:13PM +0200, Antoine Tenart wrote: > Arnd, > > On Tue, Sep 23, 2014 at 02:38:39PM +0200, Arnd Bergmann wrote: > > On Tuesday 23 September 2014 14:15:30 Antoine Tenart wrote: > > > +Optional properties: > > > +- port-id: should be '0','1' or '2'. > > > +- phy-addr: MDIO address of the PHY. > > > +- local-mac-address: see ethernet.txt file in the same directory. > > > + > > > > > > > I believe new bindings should not use "phy-addr" properties, but > > instead use a phy-handle property pointing to a phy device instead. > So I had a look on other Ethernet bindings. Would you agree with something like the following? eth0: ethernet@f7b90000 { ... #address-cells = <1>; #size-cells = <0>; phy-handle = <ðphy0>; ethphy0: ethernet-phy@0 { reg = <0>; }; }; Antoine
On Tuesday 23 September 2014 16:01:13 Antoine Tenart wrote: > > On Tue, Sep 23, 2014 at 02:38:39PM +0200, Arnd Bergmann wrote: > > On Tuesday 23 September 2014 14:15:30 Antoine Tenart wrote: > > > +Optional properties: > > > +- port-id: should be '0','1' or '2'. > > > +- phy-addr: MDIO address of the PHY. > > > +- local-mac-address: see ethernet.txt file in the same directory. > > > + > > > > > > > I believe new bindings should not use "phy-addr" properties, but > > instead use a phy-handle property pointing to a phy device instead. > > That's the MDIO address of the PHY. It's a bit difficult to use a phy > device as the driver uses the libphy. And that would break the platform > using it. > > Or maybe you have something in mind? I don't understand what you mean. Doesn't libphy imply using a phy_device? What I meant was that you should be able to just call the standard of_phy_connect() and other interfaces to deal with phylib. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 23 September 2014 16:29:22 Antoine Tenart wrote: > > So I had a look on other Ethernet bindings. Would you agree with > something like the following? > > eth0: ethernet@f7b90000 { > ... > #address-cells = <1>; > #size-cells = <0>; > phy-handle = <ðphy0>; > > ethphy0: ethernet-phy@0 { > reg = <0>; > }; > }; Yes, that looks good. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 23, 2014 at 04:33:48PM +0200, Arnd Bergmann wrote: > On Tuesday 23 September 2014 16:01:13 Antoine Tenart wrote: > > > > On Tue, Sep 23, 2014 at 02:38:39PM +0200, Arnd Bergmann wrote: > > > On Tuesday 23 September 2014 14:15:30 Antoine Tenart wrote: > > > > +Optional properties: > > > > +- port-id: should be '0','1' or '2'. > > > > +- phy-addr: MDIO address of the PHY. > > > > +- local-mac-address: see ethernet.txt file in the same directory. > > > > + > > > > > > > > > > I believe new bindings should not use "phy-addr" properties, but > > > instead use a phy-handle property pointing to a phy device instead. > > > > That's the MDIO address of the PHY. It's a bit difficult to use a phy > > device as the driver uses the libphy. And that would break the platform > > using it. > > > > Or maybe you have something in mind? > > I don't understand what you mean. Doesn't libphy imply using a phy_device? > What I meant was that you should be able to just call the standard > of_phy_connect() and other interfaces to deal with phylib. > Yes sure, I read again my answer and... well I should have had a better look on the matter. Anyway, as the proposed binding seems to be OK with you, I'll update the series accordingly. Antoine
On 09/23/2014 04:37 PM, Arnd Bergmann wrote: > On Tuesday 23 September 2014 16:29:22 Antoine Tenart wrote: >> >> So I had a look on other Ethernet bindings. Would you agree with >> something like the following? >> >> eth0: ethernet@f7b90000 { >> ... >> #address-cells = <1>; >> #size-cells = <0>; >> phy-handle = <ðphy0>; >> >> ethphy0: ethernet-phy@0 { >> reg = <0>; >> }; >> }; > > Yes, that looks good. nit: ethernet-phy@0 should not be part of the ethernet controller but either part of a separate mdio-ctrl node or a separate node itself. AFAIKS, Berlin SoCs have internal PHYs, i.e. no MII/MDIO-pins exposed but only MLT-3 differential pairs. At least BG2CD also can connect its ethernet IP to a PHY capable of HDMI Ethernet Channel (HEC). So for BG2CD this has to look something like: eth0: ethernet@f7b90000 { ... phy-handle = <&hec>; }; cec: cec@f7f00baa { compatible = "marvell,berlin-cec"; reg = <0xf7f00baa 0x1234>; /* CEC unit contains HEC PHY */ hec: ethernet-phy { bla; }; } For reference, this is what we have for MVEBU SoCs with multiple ports per controller: eth: ethernet-ctrl@72000 { compatible = "marvell,orion-eth"; #address-cells = <1>; #size-cells = <0>; reg = <0x72000 0x4000>; clocks = <&gate_clk 2>; marvell,tx-checksum-limit = <1600>; status = "disabled"; ethernet-port@0 { compatible = "marvell,orion-eth-port"; reg = <0>; interrupts = <29>; /* overwrite MAC address in bootloader */ local-mac-address = [00 00 00 00 00 00]; phy-handle = <ðphy>; }; }; mdio: mdio-bus@72004 { compatible = "marvell,orion-mdio"; #address-cells = <1>; #size-cells = <0>; reg = <0x72004 0x84>; interrupts = <30>; clocks = <&gate_clk 2>; status = "disabled"; ethphy: ethernet-phy { /* set phy address in board file */ }; }; Sebastian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 23 September 2014 17:45:52 Sebastian Hesselbarth wrote: > For reference, this is what we have for MVEBU SoCs with multiple ports > per controller: > > eth: ethernet-ctrl@72000 { > compatible = "marvell,orion-eth"; > #address-cells = <1>; > #size-cells = <0>; > reg = <0x72000 0x4000>; > clocks = <&gate_clk 2>; > marvell,tx-checksum-limit = <1600>; > status = "disabled"; > > ethernet-port@0 { > compatible = "marvell,orion-eth-port"; > reg = <0>; > interrupts = <29>; > /* overwrite MAC address in bootloader */ > local-mac-address = [00 00 00 00 00 00]; > phy-handle = <ðphy>; > }; > }; > > mdio: mdio-bus@72004 { > compatible = "marvell,orion-mdio"; > #address-cells = <1>; > #size-cells = <0>; > reg = <0x72004 0x84>; > interrupts = <30>; > clocks = <&gate_clk 2>; > status = "disabled"; > ethphy: ethernet-phy { > /* set phy address in board file */ > }; > }; > But in this example, you have the same registers and the same clocks in two nodes, which are even used by the same device driver at the moment. It's not a big issue, but my feeling is that Antoine's approach was actually better because it more closely reflects the way that the hardware is built. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/23/2014 06:29 PM, Arnd Bergmann wrote: > On Tuesday 23 September 2014 17:45:52 Sebastian Hesselbarth wrote: >> For reference, this is what we have for MVEBU SoCs with multiple ports >> per controller: >> >> eth: ethernet-ctrl@72000 { >> compatible = "marvell,orion-eth"; ... >> reg = <0x72000 0x4000>; ... >> >> ethernet-port@0 { >> compatible = "marvell,orion-eth-port"; ... >> phy-handle = <ðphy>; >> }; >> }; >> >> mdio: mdio-bus@72004 { >> compatible = "marvell,orion-mdio"; ... >> reg = <0x72004 0x84>; .. >> ethphy: ethernet-phy { >> /* set phy address in board file */ >> }; >> }; > But in this example, you have the same registers and the same > clocks in two nodes, which are even used by the same device driver > at the moment. It's not a big issue, but my feeling is that Antoine's > approach was actually better because it more closely reflects > the way that the hardware is built. I was not referring to the separate mdio bus node, but putting the ethernet-phy node as a child of ethernet-ctrl. Anyway, I can live with the ethernet-phy being a child of the controller node until we discover where it is hooked up. For the internal MII PHY the controller node maybe is the only sane place to put it in. The HEC PHY will reside within the CEC IP node but that is compatible with Antoine's proposal. Sebastian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 23 September 2014 18:40:46 Sebastian Hesselbarth wrote: > On 09/23/2014 06:29 PM, Arnd Bergmann wrote: > > On Tuesday 23 September 2014 17:45:52 Sebastian Hesselbarth wrote: > >> For reference, this is what we have for MVEBU SoCs with multiple ports > >> per controller: > >> > >> eth: ethernet-ctrl@72000 { > >> compatible = "marvell,orion-eth"; > ... > >> reg = <0x72000 0x4000>; > ... > >> > >> ethernet-port@0 { > >> compatible = "marvell,orion-eth-port"; > ... > >> phy-handle = <ðphy>; > >> }; > >> }; > >> > >> mdio: mdio-bus@72004 { > >> compatible = "marvell,orion-mdio"; > ... > >> reg = <0x72004 0x84>; > .. > >> ethphy: ethernet-phy { > >> /* set phy address in board file */ > >> }; > >> }; > > > But in this example, you have the same registers and the same > > clocks in two nodes, which are even used by the same device driver > > at the moment. It's not a big issue, but my feeling is that Antoine's > > approach was actually better because it more closely reflects > > the way that the hardware is built. > > I was not referring to the separate mdio bus node, but putting the > ethernet-phy node as a child of ethernet-ctrl. Ah, got it (I think). Yes, that makes sense. The part I don't understand yet is how one uses multiple ports. pxa168_eth.c seems to be written with the assumption that only one port is ever used at a time, while mv643xx_eth.c can actually use multiple ports simultaneously. Do you think that is that a hardware limitation of pxa168_eth or a feature that nobody so far has needed from the driver? If there is only one port and we just have to know which one that is, I don't think we need the child nodes, but if one can have multiple ports operate independently then the driver will need a rework to actually be usable with that configuration. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/23/2014 07:02 PM, Arnd Bergmann wrote: > On Tuesday 23 September 2014 18:40:46 Sebastian Hesselbarth wrote: >> On 09/23/2014 06:29 PM, Arnd Bergmann wrote: >>> On Tuesday 23 September 2014 17:45:52 Sebastian Hesselbarth wrote: >>>> For reference, this is what we have for MVEBU SoCs with multiple ports >>>> per controller: >>>> >>>> eth: ethernet-ctrl@72000 { >>>> compatible = "marvell,orion-eth"; >> ... >>>> reg = <0x72000 0x4000>; >> ... >>>> >>>> ethernet-port@0 { >>>> compatible = "marvell,orion-eth-port"; >> ... >>>> phy-handle = <ðphy>; >>>> }; >>>> }; >>>> >>>> mdio: mdio-bus@72004 { >>>> compatible = "marvell,orion-mdio"; >> ... >>>> reg = <0x72004 0x84>; >> .. >>>> ethphy: ethernet-phy { >>>> /* set phy address in board file */ >>>> }; >>>> }; >> >>> But in this example, you have the same registers and the same >>> clocks in two nodes, which are even used by the same device driver >>> at the moment. It's not a big issue, but my feeling is that Antoine's >>> approach was actually better because it more closely reflects >>> the way that the hardware is built. >> >> I was not referring to the separate mdio bus node, but putting the >> ethernet-phy node as a child of ethernet-ctrl. > > Ah, got it (I think). Yes, that makes sense. > The part I don't understand yet is how one uses multiple ports. pxa168_eth.c > seems to be written with the assumption that only one port is ever used at > a time, while mv643xx_eth.c can actually use multiple ports simultaneously. > > Do you think that is that a hardware limitation of pxa168_eth or a feature > that nobody so far has needed from the driver? It has been a while I looked it up in the pxa168 datasheet, but IIRC there is only one port per controller. FWIW, there actually is also just one port per controller for Orion SoCs. The multiple ports per controller comes from the PPC system controllers, i.e. mv643xx hence the name. We just made the binding look like it is more ports available to make it backwards compatible.. although I doubt anyone is still using mv643xx anywhere. > If there is only one port and we just have to know which one that is, > I don't think we need the child nodes, but if one can have multiple > ports operate independently then the driver will need a rework > to actually be usable with that configuration. I doubt pxa168 needs port nodes at all, i.e. we have the phy-handle directly as property of the controller node. The HEC PHY node itself will be a sub-node of some future CEC node, while the (internal) MII PHY node can stay as a sub-node of the controller, e.g. (one final example to make sure we agree on the same) eth0: ethernet@f7b90000 { compatible = "marvell,pxa168-eth"; reg = <...>; #address-cells = <1>; #size-cells = <0>; phy-handle = <ðphy0>; ethphy0: ethernet-phy@0 { reg = <0>; }; }; cec0: cec@f7f00baa { compatible = "marvell,berlin-cec"; reg = <...>; #address-cells = <1>; #size-cells = <0>; hecphy0: hdmi-ethernet-phy@0 { reg = <0>; }; }; With berlin2cd-google-chromecast.dts overwriting ð0 { phy-handle = <&hecphy0>; }; Sebastian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 23 September 2014 19:31:40 Sebastian Hesselbarth wrote: > It has been a while I looked it up in the pxa168 datasheet, but IIRC > there is only one port per controller. FWIW, there actually is also > just one port per controller for Orion SoCs. The multiple ports per > controller comes from the PPC system controllers, i.e. mv643xx hence > the name. We just made the binding look like it is more ports available > to make it backwards compatible.. although I doubt anyone is still using > mv643xx anywhere. Ok, I see. > > If there is only one port and we just have to know which one that is, > > I don't think we need the child nodes, but if one can have multiple > > ports operate independently then the driver will need a rework > > to actually be usable with that configuration. > > I doubt pxa168 needs port nodes at all, i.e. we have the phy-handle > directly as property of the controller node. Ah, good. > The HEC PHY node itself will be a sub-node of some future CEC node, > while the (internal) MII PHY node can stay as a sub-node of the > controller, e.g. > > (one final example to make sure we agree on the same) > > eth0: ethernet@f7b90000 { > compatible = "marvell,pxa168-eth"; > reg = <...>; > #address-cells = <1>; > #size-cells = <0>; > phy-handle = <ðphy0>; > > ethphy0: ethernet-phy@0 { > reg = <0>; > }; > }; > > cec0: cec@f7f00baa { > compatible = "marvell,berlin-cec"; > reg = <...>; > #address-cells = <1>; > #size-cells = <0>; > > hecphy0: hdmi-ethernet-phy@0 { > reg = <0>; > }; > }; > > With berlin2cd-google-chromecast.dts overwriting > > ð0 { phy-handle = <&hecphy0>; }; Ok, now I also understood how the cec fits into this. Yes, I think this looks very good. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/marvell-pxa168.txt b/Documentation/devicetree/bindings/net/marvell-pxa168.txt new file mode 100644 index 000000000000..4177abf3dac3 --- /dev/null +++ b/Documentation/devicetree/bindings/net/marvell-pxa168.txt @@ -0,0 +1,21 @@ +* Marvell PXA168 Ethernet Controller + +Required properties: +- compatible: should be "marvell,pxa168-eth". +- reg: address and length of the register set for the device. +- interrupts: interrupt for the device. +- clocks: pointer to the clock for the device. + +Optional properties: +- port-id: should be '0','1' or '2'. +- phy-addr: MDIO address of the PHY. +- local-mac-address: see ethernet.txt file in the same directory. + +Example: + + eth0: ethernet@f7b90000 { + compatible = "marvell,pxa168-eth"; + reg = <0xf7b90000 0x10000>; + clocks = <&chip CLKID_GETH0>; + interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>; + };
This adds the binding documentation for the Marvell PXA168 Ethernet controller, following its DT support. Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- .../devicetree/bindings/net/marvell-pxa168.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/marvell-pxa168.txt