| Message ID | 1482943592-12556-2-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Dec 28, 2016 at 05:46:17PM +0100, Thomas Petazzoni wrote: > The Marvell PPv2 Device Tree binding was so far only used to describe > the PPv2.1 network controller, used in the Marvell Armada 375. > > A new version of this IP block, PPv2.2 is used in the Marvell Armada > 7K/8K processor. This commit extends the existing binding so that it can > also be used to describe PPv2.2 hardware. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > .../devicetree/bindings/net/marvell-pp2.txt | 66 ++++++++++++++++++---- > 1 file changed, 55 insertions(+), 11 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/marvell-pp2.txt b/Documentation/devicetree/bindings/net/marvell-pp2.txt > index aa4f423..76071f3 100644 > --- a/Documentation/devicetree/bindings/net/marvell-pp2.txt > +++ b/Documentation/devicetree/bindings/net/marvell-pp2.txt > @@ -1,17 +1,28 @@ > -* Marvell Armada 375 Ethernet Controller (PPv2) > +* Marvell Armada 375 Ethernet Controller (PPv2.1) > + Marvell Armada 7K/8K Ethernet Controller (PPv2.2) > > Required properties: > > -- compatible: should be "marvell,armada-375-pp2" > +- compatible: should be one of: > + "marvell,armada-375-pp2" > + "marvell,armada-7k-pp2" > - reg: addresses and length of the register sets for the device. > - Must contain the following register sets: > + For "marvell,armada-375-pp2", must contain the following register > + sets: > - common controller registers > - LMS registers > - In addition, at least one port register set is required. > -- clocks: a pointer to the reference clocks for this device, consequently: > - - main controller clock > - - GOP clock > -- clock-names: names of used clocks, must be "pp_clk" and "gop_clk". > + - one register area per Ethernet port > + For "marvell,armda-7k-pp2", must contain the following register > + sets: > + - common controller registers > + - per-port registers > + > +- clocks: pointers to the reference clocks for this device, consequently: > + - main controller clock (for both armada-375-pp2 and armada-7k-pp2) > + - GOP clock (for both armada-375-pp2 and armada-7k-pp2) > + - MG clock (only for armada-7k-pp2) > +- clock-names: names of used clocks, must be "pp_clk", "gop_clk" and > + "mg_clk" (the latter only for armada-7k-pp2). > > The ethernet ports are represented by subnodes. At least one port is > required. > @@ -19,8 +30,9 @@ required. > Required properties (port): > > - interrupts: interrupt for the port > -- port-id: should be '0' or '1' for ethernet ports, and '2' for the > - loopback port > +- port-id: ID of the port from the MAC point of view > +- gop-port-id: only for marvell,armada-7k-pp2, ID of the port from the > + GOP (Group Of Ports) point of view What GOP is needs a better explanation. Why doesn't 375 need this? > - phy-mode: See ethernet.txt file in the same directory > > Optional properties (port): > @@ -31,7 +43,7 @@ Optional properties (port): > then fixed link is assumed, and the 'fixed-link' property is > mandatory. > > -Example: > +Example for marvell,armada-375-pp2: > > ethernet@f0000 { > compatible = "marvell,armada-375-pp2"; > @@ -59,3 +71,35 @@ ethernet@f0000 { > phy-mode = "gmii"; > }; > }; > + > +Example for marvell,armada-7k-pp2: > + > +cpm_ethernet: ethernet@0 { > + compatible = "marvell,armada-7k-pp22"; > + reg = <0x0 0x100000>, > + <0x100000 0x80000>; > + clocks = <&cpm_syscon0 1 3>, <&cpm_syscon0 1 9>, <&cpm_syscon0 1 5>; > + clock-names = "pp_clk", "gop_clk", "gp_clk"; > + status = "disabled"; Drop status from examples. > + > + eth0: eth@0 { unit address requires a reg property. Or this can be 'eth0' instead. > + interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>; > + port-id = <0>; > + gop-port-id = <0>; > + status = "disabled"; > + }; > + > + eth1: eth@1 { > + interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>; > + port-id = <1>; > + gop-port-id = <2>; > + status = "disabled"; > + }; > + > + eth2: eth@2 { > + interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>; > + port-id = <2>; > + gop-port-id = <3>; > + status = "disabled"; > + }; > +}; > -- > 2.7.4 > > -- > 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 Wed, Dec 28, 2016 at 05:46:17PM +0100, Thomas Petazzoni wrote: > @@ -31,7 +43,7 @@ Optional properties (port): > then fixed link is assumed, and the 'fixed-link' property is > mandatory. Not directly related to this patch, but this context is wrong. The PP2 driver _requires_ a PHY. It doesn't support fixed-link in its current form. I think the DT binding describes an expectation of a future driver. The side effect is that if trying to use a fixed-link on any port, the ethernet driver fails to probe.
Hello, On Sat, 7 Jan 2017 09:32:28 +0000, Russell King - ARM Linux wrote: > On Wed, Dec 28, 2016 at 05:46:17PM +0100, Thomas Petazzoni wrote: > > @@ -31,7 +43,7 @@ Optional properties (port): > > then fixed link is assumed, and the 'fixed-link' property is > > mandatory. > > Not directly related to this patch, but this context is wrong. The > PP2 driver _requires_ a PHY. It doesn't support fixed-link in its > current form. I think the DT binding describes an expectation of > a future driver. > > The side effect is that if trying to use a fixed-link on any port, > the ethernet driver fails to probe. Correct. I will send a patch to adjust the Device Tree binding documentation accordingly. Thanks for noticing this mistake! Thomas
Hello, On Tue, 3 Jan 2017 14:18:42 -0600, Rob Herring wrote: > > +- port-id: ID of the port from the MAC point of view > > +- gop-port-id: only for marvell,armada-7k-pp2, ID of the port from the > > + GOP (Group Of Ports) point of view > > What GOP is needs a better explanation. Why doesn't 375 need this? GOP stands for "group of ports", it's one of the HW component inside the PPv2 IP. Armada 375 also has the same GOP, but we described the registers in a different way for Armada 375, with one reg entry per port: reg = <0xf0000 0xa000>, <0xc0000 0x3060>, <0xc4000 0x100>, <0xc5000 0x100>; The last two entries are the per-port registers for eth0 and eth1. For PPv2.2, we wanted to simplify a little bit the register mappings, and simply reflect the memory map of the SoC. In the SoC datasheet, there are two memory areas for the networking subsystem, which are the two areas reflected in: reg = <0x0 0x100000>, <0x100000 0x80000>; The per-port registers are inside the second register area. But by exposing the entire register area in the Device Tree binding, we allow improvements in the driver that need additional registers to be made without changing the Device Tree description of the device. > > +Example for marvell,armada-7k-pp2: > > + > > +cpm_ethernet: ethernet@0 { > > + compatible = "marvell,armada-7k-pp22"; > > + reg = <0x0 0x100000>, > > + <0x100000 0x80000>; > > + clocks = <&cpm_syscon0 1 3>, <&cpm_syscon0 1 9>, <&cpm_syscon0 1 5>; > > + clock-names = "pp_clk", "gop_clk", "gp_clk"; > > + status = "disabled"; > > Drop status from examples. OK, so I'll have to adjust this for the existing armada-375-pp2 example as well. > > + > > + eth0: eth@0 { > > unit address requires a reg property. Or this can be 'eth0' instead. Same here, the sub-nodes don't have a reg property for armada-375-pp2, so I guess this comment applies as well, right? Thanks for your feedback! Best regards, Thomas
On Thu, Feb 02, 2017 at 05:56:50PM +0100, Thomas Petazzoni wrote: > For PPv2.2, we wanted to simplify a little bit the register mappings, > and simply reflect the memory map of the SoC. In the SoC datasheet, > there are two memory areas for the networking subsystem, which are the > two areas reflected in: > > reg = <0x0 0x100000>, > <0x100000 0x80000>; > > The per-port registers are inside the second register area. But by > exposing the entire register area in the Device Tree binding, we allow > improvements in the driver that need additional registers to be made > without changing the Device Tree description of the device. Are you sure that this makes sense? You have the serdes block at 0x120000-0x125fff, which sits within that range, and that needs to be configured for SATA and PCIe, so it's not strictly just a network thing. I know from my experimentation that disabling the "serdes" by clearing the SD_EXTERNAL_CONFIG1_REG and SD_EXTERNAL_CONFIG0_REG for SATA channels prevents SATA working.
Hello, On Fri, 3 Feb 2017 16:48:08 +0000, Russell King - ARM Linux wrote: > On Thu, Feb 02, 2017 at 05:56:50PM +0100, Thomas Petazzoni wrote: > > For PPv2.2, we wanted to simplify a little bit the register mappings, > > and simply reflect the memory map of the SoC. In the SoC datasheet, > > there are two memory areas for the networking subsystem, which are the > > two areas reflected in: > > > > reg = <0x0 0x100000>, > > <0x100000 0x80000>; > > > > The per-port registers are inside the second register area. But by > > exposing the entire register area in the Device Tree binding, we allow > > improvements in the driver that need additional registers to be made > > without changing the Device Tree description of the device. > > Are you sure that this makes sense? You have the serdes block at > 0x120000-0x125fff, which sits within that range, and that needs to > be configured for SATA and PCIe, so it's not strictly just a network > thing. > > I know from my experimentation that disabling the "serdes" by clearing > the SD_EXTERNAL_CONFIG1_REG and SD_EXTERNAL_CONFIG0_REG for SATA > channels prevents SATA working. I have simply/stupidly followed the datasheet, which says: Packet processor: 0xf2000000, 0xf4000000 (i.e offset 0x0 in the CP) Networking interfaces: 0xf2100000, 0xf4100000 (i.e offset 0x100000 in the CP) IO Bridge Addr Decoding: 0xf2190000, 0xf4190000 And since there is no other block described, I assumed that the entire range 0xf2000000 to 0xf2100000 was dedicated to the "Packet processor", and that the range 0xf2100000 to 0xf2190000 was dedicated to the "Networking interfaces" (and I realize the size is 0x90000, not 0x80000). On the other hand, there are registers after 0x125fff that are really networking related. For example, the driver is currently accessing 0x12a204, which is after the SERDES related registers. I'll try to get some more information about this, but I suspect this is one case where we don't yet fully understand how all the HW works and what all registers are doing, so it's hard to do a perfect DT binding from day 1. Thomas
Hello, On Tue, 14 Feb 2017 15:25:03 +0100, Thomas Petazzoni wrote: > I'll try to get some more information about this, but I suspect this is > one case where we don't yet fully understand how all the HW works and > what all registers are doing, so it's hard to do a perfect DT binding > from day 1. So I've looked into this more closely, and after drawing a diagram of all the register areas, I came up with a much more reduced area, with only networking related registers: starting at 0x129000 for a size of 0xb000. So now the DT node looks like this: + cpm_ethernet: ethernet@0 { + compatible = "marvell,armada-7k-pp22"; + reg = <0x0 0x100000>, <0x129000 0xb000>; This will be part of my next iteration. Thanks! Thomas
diff --git a/Documentation/devicetree/bindings/net/marvell-pp2.txt b/Documentation/devicetree/bindings/net/marvell-pp2.txt index aa4f423..76071f3 100644 --- a/Documentation/devicetree/bindings/net/marvell-pp2.txt +++ b/Documentation/devicetree/bindings/net/marvell-pp2.txt @@ -1,17 +1,28 @@ -* Marvell Armada 375 Ethernet Controller (PPv2) +* Marvell Armada 375 Ethernet Controller (PPv2.1) + Marvell Armada 7K/8K Ethernet Controller (PPv2.2) Required properties: -- compatible: should be "marvell,armada-375-pp2" +- compatible: should be one of: + "marvell,armada-375-pp2" + "marvell,armada-7k-pp2" - reg: addresses and length of the register sets for the device. - Must contain the following register sets: + For "marvell,armada-375-pp2", must contain the following register + sets: - common controller registers - LMS registers - In addition, at least one port register set is required. -- clocks: a pointer to the reference clocks for this device, consequently: - - main controller clock - - GOP clock -- clock-names: names of used clocks, must be "pp_clk" and "gop_clk". + - one register area per Ethernet port + For "marvell,armda-7k-pp2", must contain the following register + sets: + - common controller registers + - per-port registers + +- clocks: pointers to the reference clocks for this device, consequently: + - main controller clock (for both armada-375-pp2 and armada-7k-pp2) + - GOP clock (for both armada-375-pp2 and armada-7k-pp2) + - MG clock (only for armada-7k-pp2) +- clock-names: names of used clocks, must be "pp_clk", "gop_clk" and + "mg_clk" (the latter only for armada-7k-pp2). The ethernet ports are represented by subnodes. At least one port is required. @@ -19,8 +30,9 @@ required. Required properties (port): - interrupts: interrupt for the port -- port-id: should be '0' or '1' for ethernet ports, and '2' for the - loopback port +- port-id: ID of the port from the MAC point of view +- gop-port-id: only for marvell,armada-7k-pp2, ID of the port from the + GOP (Group Of Ports) point of view - phy-mode: See ethernet.txt file in the same directory Optional properties (port): @@ -31,7 +43,7 @@ Optional properties (port): then fixed link is assumed, and the 'fixed-link' property is mandatory. -Example: +Example for marvell,armada-375-pp2: ethernet@f0000 { compatible = "marvell,armada-375-pp2"; @@ -59,3 +71,35 @@ ethernet@f0000 { phy-mode = "gmii"; }; }; + +Example for marvell,armada-7k-pp2: + +cpm_ethernet: ethernet@0 { + compatible = "marvell,armada-7k-pp22"; + reg = <0x0 0x100000>, + <0x100000 0x80000>; + clocks = <&cpm_syscon0 1 3>, <&cpm_syscon0 1 9>, <&cpm_syscon0 1 5>; + clock-names = "pp_clk", "gop_clk", "gp_clk"; + status = "disabled"; + + eth0: eth@0 { + interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>; + port-id = <0>; + gop-port-id = <0>; + status = "disabled"; + }; + + eth1: eth@1 { + interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>; + port-id = <1>; + gop-port-id = <2>; + status = "disabled"; + }; + + eth2: eth@2 { + interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>; + port-id = <2>; + gop-port-id = <3>; + status = "disabled"; + }; +};
The Marvell PPv2 Device Tree binding was so far only used to describe the PPv2.1 network controller, used in the Marvell Armada 375. A new version of this IP block, PPv2.2 is used in the Marvell Armada 7K/8K processor. This commit extends the existing binding so that it can also be used to describe PPv2.2 hardware. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- .../devicetree/bindings/net/marvell-pp2.txt | 66 ++++++++++++++++++---- 1 file changed, 55 insertions(+), 11 deletions(-)