| Message ID | 20160820093538.9707-2-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Saturday, August 20, 2016 11:35:35 AM CEST Martin Blumenstingl wrote: > +- reg: The first register range should be the one of the DWMAC > + controller. The second range is is for the Amlogic specific > + configuration (for example the PRG_ETHERNET register range > + on Meson8b and newer) > ... > +Example for GXBB: > + ethmac: ethernet@c9410000 { > + compatible = "amlogic,meson-gxbb-dwmac", "snps,dwmac"; > + reg = <0x0 0xc9410000 0x0 0x10000>, > + <0x0 0xc8834540 0x0 0x8>; > The address "0xc8834540" suggests that this is part of a larger register range that is used for various things, i.e. a "syscon" type of device. How about making this a syscon reference rather than a "reg" address? Arnd
On Mon, Aug 22, 2016 at 1:55 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Saturday, August 20, 2016 11:35:35 AM CEST Martin Blumenstingl wrote: >> +- reg: The first register range should be the one of the DWMAC >> + controller. The second range is is for the Amlogic specific >> + configuration (for example the PRG_ETHERNET register range >> + on Meson8b and newer) >> > ... > >> +Example for GXBB: >> + ethmac: ethernet@c9410000 { >> + compatible = "amlogic,meson-gxbb-dwmac", "snps,dwmac"; >> + reg = <0x0 0xc9410000 0x0 0x10000>, >> + <0x0 0xc8834540 0x0 0x8>; >> > > The address "0xc8834540" suggests that this is part of a larger register > range that is used for various things, i.e. a "syscon" type of device. You are right, these are part of the cbus range (which is already defined in meson-gxbb.dtsi) > How about making this a syscon reference rather than a "reg" address? The first version of my patch ([0]) used syscon_regmap_lookup_by_phandle. Maybe I did it wrong (and I should have passed the cbus syscon-node instead of defining a new one just for the 2x32bit PRG_ETHERNET registers). I am perfectly fine with either way - however it seems that some other dwmac glue implementations are also using a second set of resources (that doesn't automatically make it "correct" though). Martin
On Monday, August 22, 2016 2:04:49 PM CEST Martin Blumenstingl wrote: > On Mon, Aug 22, 2016 at 1:55 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Saturday, August 20, 2016 11:35:35 AM CEST Martin Blumenstingl wrote: > >> +- reg: The first register range should be the one of the DWMAC > >> + controller. The second range is is for the Amlogic specific > >> + configuration (for example the PRG_ETHERNET register range > >> + on Meson8b and newer) > >> > > ... > > > >> +Example for GXBB: > >> + ethmac: ethernet@c9410000 { > >> + compatible = "amlogic,meson-gxbb-dwmac", "snps,dwmac"; > >> + reg = <0x0 0xc9410000 0x0 0x10000>, > >> + <0x0 0xc8834540 0x0 0x8>; > >> > > > > The address "0xc8834540" suggests that this is part of a larger register > > range that is used for various things, i.e. a "syscon" type of device. > You are right, these are part of the cbus range (which is already > defined in meson-gxbb.dtsi) > > > How about making this a syscon reference rather than a "reg" address? > The first version of my patch ([0]) used > syscon_regmap_lookup_by_phandle. Maybe I did it wrong (and I should > have passed the cbus syscon-node instead of defining a new one just > for the 2x32bit PRG_ETHERNET registers). > I am perfectly fine with either way - however it seems that some other > dwmac glue implementations are also using a second set of resources > (that doesn't automatically make it "correct" though). It really depends on the kind of SoC. Some may have a suboptimal binding, on some others there may be a distinct register area that just contains a few additional registers for the dwmac. Arnd
On Mon, Aug 22, 2016 at 5:25 PM, Arnd Bergmann <arnd@arndb.de> wrote: > It really depends on the kind of SoC. Some may have a suboptimal > binding, on some others there may be a distinct register area that > just contains a few additional registers for the dwmac. the dwmac PHY configuration registers (2x32bit) on the GXBB SoC are part of the "periphs" region/module. This is already defined as "simple-bus" in meson-gxbb.dtsi, see [0] On Meson8b this is slightly different: there is no specific "periphs" region - there the dwmac PHY configuration registers are directly located in the cbus region at a slightly different offset than on the GXBB SoCs. In the future we might need a third memory region because the latest reference kernel contains some more PHY configuration registers on newer SoCs (GXL = S905X). Please let me know if you're OK with the dts definition in it's current state - or let me know how you would like to change it. PS: I will re-send the patches in a v3 in a few minutes because that fixes a bug during module unload. Regards, Martin [0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi#n217
On Sunday 28 August 2016, Martin Blumenstingl wrote: > On Mon, Aug 22, 2016 at 5:25 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > It really depends on the kind of SoC. Some may have a suboptimal > > binding, on some others there may be a distinct register area that > > just contains a few additional registers for the dwmac. > the dwmac PHY configuration registers (2x32bit) on the GXBB SoC are > part of the "periphs" region/module. This is already defined as > "simple-bus" in meson-gxbb.dtsi, see [0] > On Meson8b this is slightly different: there is no specific "periphs" > region - there the dwmac PHY configuration registers are directly > located in the cbus region at a slightly different offset than on the > GXBB SoCs. > > In the future we might need a third memory region because the latest > reference kernel contains some more PHY configuration registers on > newer SoCs (GXL = S905X). > > Please let me know if you're OK with the dts definition in it's > current state - or let me know how you would like to change it. > > PS: I will re-send the patches in a v3 in a few minutes because that > fixes a bug during module unload. I don't really see a good way to describe this hardware then. If it was only the first case, I'd suggest marking the periphs bus node as "compatible="simple-bus","syscon";" so you could have a reference to it, but that doesn't seem to work well in the second case, unless you can a separate DT node just for the PHY config registers there. With the third case, is there any logic at all behind the register map? Maybe someone else has a better idea for how to describe this. In general, we try to avoid overlapping "reg" properties, but I even see that the "periphs" node on gxbb has a "reg" property (is this intentional) that overlaps with the registers in its ranges, so adding another one won't make this worse than it already is. Arnd
diff --git a/Documentation/devicetree/bindings/net/meson-dwmac.txt b/Documentation/devicetree/bindings/net/meson-dwmac.txt index ec633d7..89e62dd 100644 --- a/Documentation/devicetree/bindings/net/meson-dwmac.txt +++ b/Documentation/devicetree/bindings/net/meson-dwmac.txt @@ -1,18 +1,32 @@ * Amlogic Meson DWMAC Ethernet controller The device inherits all the properties of the dwmac/stmmac devices -described in the file net/stmmac.txt with the following changes. +described in the file stmmac.txt in the current directory with the +following changes. -Required properties: +Required properties on all platforms: -- compatible: should be "amlogic,meson6-dwmac" along with "snps,dwmac" - and any applicable more detailed version number - described in net/stmmac.txt +- compatible: Depending on the platform this should be one of: + - "amlogic,meson6-dwmac" + - "amlogic,meson8b-dwmac" + - "amlogic,meson-gxbb-dwmac" + Additionally "snps,dwmac" and any applicable more + detailed version number described in net/stmmac.txt + should be used. -- reg: should contain a register range for the dwmac controller and - another one for the Amlogic specific configuration +- reg: The first register range should be the one of the DWMAC + controller. The second range is is for the Amlogic specific + configuration (for example the PRG_ETHERNET register range + on Meson8b and newer) -Example: +Required properties on Meson8b and newer: +- clock-names: Should contain the following: + - "stmmaceth" - see stmmac.txt + - "clkin0" - first parent clock of the internal mux + - "clkin1" - second parent clock of the internal mux + + +Example for Meson6: ethmac: ethernet@c9410000 { compatible = "amlogic,meson6-dwmac", "snps,dwmac"; @@ -23,3 +37,18 @@ Example: clocks = <&clk81>; clock-names = "stmmaceth"; } + +Example for GXBB: + ethmac: ethernet@c9410000 { + compatible = "amlogic,meson-gxbb-dwmac", "snps,dwmac"; + reg = <0x0 0xc9410000 0x0 0x10000>, + <0x0 0xc8834540 0x0 0x8>; + interrupts = <0 8 1>; + interrupt-names = "macirq"; + clocks = <&clkc CLKID_ETH>, + <&clkc CLKID_FCLK_DIV2>, + <&clkc CLKID_MPLL2>; + clock-names = "stmmaceth", "clkin0", "clkin1"; + phy-mode = "rgmii"; + status = "disabled"; + };