| Message ID | 20160625165013.15917-4-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On 25/06/16 18:50, Martin Blumenstingl wrote: > The Amlogic reference driver uses the "mc_val" devicetree property to > configure the PRG_ETHERNET_ADDR0 register. Unfortunately it uses magic > values for this configuration. > > According to the datasheet the PRG_ETHERNET_ADDR0 register is at address > 0xc8834108. However, the reference driver uses 0xc8834540 instead. > According to my tests, the value from the reference driver is correct. > > The updated examples are representing 0x1621 from the reference driver's > mc_val property, which is used when there is an external gbit PHY > connected. > For RMII mode PHYs mc_val 0x1800 is used in the reference driver, which > translates would translate to "do not set any of the following > properties" (as the two bits are configured automatically): > - amlogic,enable-25mhz-phy-clk > - amlogic,mp2-clock > - amlogic,tx-delay [...] > }; > + > + prg_ethernet: prg_ethernet@540 { > + compatible = "syscon"; > + reg = <0x0 0x00540 0x0 0x8>; > + }; > }; A syscon is a region containing a set of miscellaneous registers used for several reasons by several devices [1]. It this case there is really no need to define a new syscon node since those two registers are only used by your driver. [1] http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mfd/syscon.txt > hiubus: hiubus@c883c000 { > @@ -354,14 +360,14 @@ > }; > > ethmac: ethernet@c9410000 { > - compatible = "amlogic,meson6-dwmac", "snps,dwmac"; > - reg = <0x0 0xc9410000 0x0 0x10000 > - 0x0 0xc8834540 0x0 0x4>; > + compatible = "amlogic,meson8b-dwmac", "snps,dwmac"; > + reg = <0x0 0xc9410000 0x0 0x10000>; > interrupts = <0 8 1>; > interrupt-names = "macirq"; > clocks = <&clkc CLKID_ETH>; > clock-names = "stmmaceth"; > phy-mode = "rgmii"; > + amlogic,prg-ethernet = <&prg_ethernet>; > status = "disabled"; > }; > }; > -- > 2.9.0 > > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic
On Mon, Jun 27, 2016 at 11:24 AM, Carlo Caione <carlo@caione.org> wrote: > A syscon is a region containing a set of miscellaneous registers used > for several reasons by several devices [1]. It this case there is really > no need to define a new syscon node since those two registers are only > used by your driver. I can easily change it back if that's the way to go. Before I do that: could you please confirm that "mp2_clk_out" (which is controlled by PRG_ETH0/offset 0x0 bits 7-9) is not something which has to be available through the common clk framework?
On Mon, Jun 27, 2016 at 12:44 PM, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > On Mon, Jun 27, 2016 at 11:24 AM, Carlo Caione <carlo@caione.org> wrote: >> A syscon is a region containing a set of miscellaneous registers used >> for several reasons by several devices [1]. It this case there is really >> no need to define a new syscon node since those two registers are only >> used by your driver. > I can easily change it back if that's the way to go. > Before I do that: could you please confirm that "mp2_clk_out" (which > is controlled by PRG_ETH0/offset 0x0 bits 7-9) is not something which > has to be available through the common clk framework? there was just an IRC discussion with Carlo on this topic: We tried to find whether PRG_ETH0 is used to actually configure "mp2_clk_out". Carlo brought up that it could also be the case that the ethernet block simply needs to be informed about the rate of the mp2_clk_out (which is *probably* the "mpll2" clock). I'm adding Michael Turquette to this mail, maybe you can comment on this topic. If it turns out that the etthernet block just has to know about the clock rate then we have two tasks: 1. identify why the mpll2 rate returns 0 on my GXBB device 2. change my patch so the new DWMAC glue gets a reference to the mpll2 clock and then use "clk_get_rate(mpll2) / (250 * 1000000)" to configure the PRG_ETH0_MP2_CLK bits.
Hi Martin, Quoting Martin Blumenstingl (2016-06-27 04:33:49) > On Mon, Jun 27, 2016 at 12:44 PM, Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > > On Mon, Jun 27, 2016 at 11:24 AM, Carlo Caione <carlo@caione.org> wrote: > >> A syscon is a region containing a set of miscellaneous registers used > >> for several reasons by several devices [1]. It this case there is really > >> no need to define a new syscon node since those two registers are only > >> used by your driver. > > I can easily change it back if that's the way to go. > > Before I do that: could you please confirm that "mp2_clk_out" (which > > is controlled by PRG_ETH0/offset 0x0 bits 7-9) is not something which > > has to be available through the common clk framework? > there was just an IRC discussion with Carlo on this topic: > We tried to find whether PRG_ETH0 is used to actually configure > "mp2_clk_out". Carlo brought up that it could also be the case that > the ethernet block simply needs to be informed about the rate of the > mp2_clk_out (which is *probably* the "mpll2" clock). > > I'm adding Michael Turquette to this mail, maybe you can comment on this topic. > > If it turns out that the etthernet block just has to know about the > clock rate then we have two tasks: > 1. identify why the mpll2 rate returns 0 on my GXBB device This is in progress, but turns out it doesn't matter for Ethernet. Bit 4 in PRG_ETHERNET_ADDR0 control a mux clock inside of the Ethernet controller. A value of 0x0 selects fclk_div2 and a value of 0x1 selects mp2_clk_out. The bootloader programs in sets the mux to zero, or fclk_div2 as the input clock (which runs at 1GHz). > 2. change my patch so the new DWMAC glue gets a reference to the mpll2 > clock and then use "clk_get_rate(mpll2) / (250 * 1000000)" to > configure the PRG_ETH0_MP2_CLK bits. Hmm, I'm not sure about that part. Bits 7-9 is a divider that further divides the clock signal selected by bit 4. This is set to 0x4, which means we divide the 1GHz fclk_div2 down to 250MHz, which seems to be the expected value coming out of this divider. I haven't looked further to see if there is a further programmable divider to divide 250MHz down to 50MHz, or (more likely) there is simply a fixed-factor divide-by-5 that results in the 50MHz rate consumed by the PHY. Modeling this all in the mmc driver makes sense. So we would have: struct clk_mux clk_m250_sel -> struct clk_divider clk_m250_div -> struct clk_fixed_factor enet_phy_clk I don't know what the name should be for that last one, I just chose enet_phy_clk since it illustrates the point. The updated docs suggest that clk_m250_{sel,div} might be reasonable names for the mux and divider. Kevin and I just got this info from AmLogic earlier today. The next rev of documentation should correct these register definitions. Regards, Mike
Michael Turquette <mturquette@baylibre.com> writes: > Hi Martin, > > Quoting Martin Blumenstingl (2016-06-27 04:33:49) >> On Mon, Jun 27, 2016 at 12:44 PM, Martin Blumenstingl >> <martin.blumenstingl@googlemail.com> wrote: >> > On Mon, Jun 27, 2016 at 11:24 AM, Carlo Caione <carlo@caione.org> wrote: >> >> A syscon is a region containing a set of miscellaneous registers used >> >> for several reasons by several devices [1]. It this case there is really >> >> no need to define a new syscon node since those two registers are only >> >> used by your driver. >> > I can easily change it back if that's the way to go. >> > Before I do that: could you please confirm that "mp2_clk_out" (which >> > is controlled by PRG_ETH0/offset 0x0 bits 7-9) is not something which >> > has to be available through the common clk framework? >> there was just an IRC discussion with Carlo on this topic: >> We tried to find whether PRG_ETH0 is used to actually configure >> "mp2_clk_out". Carlo brought up that it could also be the case that >> the ethernet block simply needs to be informed about the rate of the >> mp2_clk_out (which is *probably* the "mpll2" clock). >> >> I'm adding Michael Turquette to this mail, maybe you can comment on this topic. >> >> If it turns out that the etthernet block just has to know about the >> clock rate then we have two tasks: >> 1. identify why the mpll2 rate returns 0 on my GXBB device > > This is in progress, but turns out it doesn't matter for Ethernet. Bit 4 > in PRG_ETHERNET_ADDR0 control a mux clock inside of the Ethernet > controller. > > A value of 0x0 selects fclk_div2 and a value of 0x1 selects mp2_clk_out. > The bootloader programs in sets the mux to zero, or fclk_div2 as the > input clock (which runs at 1GHz). > >> 2. change my patch so the new DWMAC glue gets a reference to the mpll2 >> clock and then use "clk_get_rate(mpll2) / (250 * 1000000)" to >> configure the PRG_ETH0_MP2_CLK bits. > > Hmm, I'm not sure about that part. Bits 7-9 is a divider that further > divides the clock signal selected by bit 4. This is set to 0x4, which > means we divide the 1GHz fclk_div2 down to 250MHz, which seems to be the > expected value coming out of this divider. > > I haven't looked further to see if there is a further programmable > divider to divide 250MHz down to 50MHz, or (more likely) there is simply > a fixed-factor divide-by-5 that results in the 50MHz rate consumed by > the PHY. > > Modeling this all in the mmc driver makes sense. So we would have: > > struct clk_mux clk_m250_sel -> > struct clk_divider clk_m250_div -> > struct clk_fixed_factor enet_phy_clk There's also bit 10: "Generate 25MHz clock for PHY" (which is set to 1 by the bootloaders on P200 and odroidc2) This suggests that it might not be a fixed-factor divide-by-5 but a choice between a divide-by-5 and a divide-by-10 for the PHY clock. Kevin
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts index b06bf8a..8d540ff 100644 --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts @@ -103,6 +103,10 @@ status = "okay"; pinctrl-0 = <ð_pins>; pinctrl-names = "default"; + + amlogic,enable-25mhz-phy-clk; + amlogic,mp2-clock = <MESON8B_DWMAC_MP2_CLOCK_1000MHZ>; + amlogic,tx-delay = <MESON8B_DWMAC_TX_CLK_DELAY_QUARTER_CYCLE>; }; &sd_emmc_b { diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi index 5dfd849..4574677 100644 --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi @@ -85,6 +85,10 @@ status = "okay"; pinctrl-0 = <ð_pins>; pinctrl-names = "default"; + + amlogic,enable-25mhz-phy-clk; + amlogic,mp2-clock = <MESON8B_DWMAC_MP2_CLOCK_1000MHZ>; + amlogic,tx-delay = <MESON8B_DWMAC_TX_CLK_DELAY_QUARTER_CYCLE>; }; &sd_emmc_b { diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi index 3a77829..31ae35e 100644 --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi @@ -83,6 +83,10 @@ status = "okay"; pinctrl-0 = <ð_pins>; pinctrl-names = "default"; + + amlogic,enable-25mhz-phy-clk; + amlogic,mp2-clock = <MESON8B_DWMAC_MP2_CLOCK_1000MHZ>; + amlogic,tx-delay = <MESON8B_DWMAC_TX_CLK_DELAY_QUARTER_CYCLE>; }; &sd_emmc_b { diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi index 6c23965..463c2cd 100644 --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi @@ -46,6 +46,7 @@ #include <dt-bindings/gpio/meson-gxbb-gpio.h> #include <dt-bindings/reset/amlogic,meson-gxbb-reset.h> #include <dt-bindings/clock/gxbb-clkc.h> +#include <dt-bindings/net/amlogic-meson8b-dwmac.h> / { compatible = "amlogic,meson-gxbb"; @@ -308,6 +309,11 @@ }; }; }; + + prg_ethernet: prg_ethernet@540 { + compatible = "syscon"; + reg = <0x0 0x00540 0x0 0x8>; + }; }; hiubus: hiubus@c883c000 { @@ -354,14 +360,14 @@ }; ethmac: ethernet@c9410000 { - compatible = "amlogic,meson6-dwmac", "snps,dwmac"; - reg = <0x0 0xc9410000 0x0 0x10000 - 0x0 0xc8834540 0x0 0x4>; + compatible = "amlogic,meson8b-dwmac", "snps,dwmac"; + reg = <0x0 0xc9410000 0x0 0x10000>; interrupts = <0 8 1>; interrupt-names = "macirq"; clocks = <&clkc CLKID_ETH>; clock-names = "stmmaceth"; phy-mode = "rgmii"; + amlogic,prg-ethernet = <&prg_ethernet>; status = "disabled"; }; };
The Amlogic reference driver uses the "mc_val" devicetree property to configure the PRG_ETHERNET_ADDR0 register. Unfortunately it uses magic values for this configuration. According to the datasheet the PRG_ETHERNET_ADDR0 register is at address 0xc8834108. However, the reference driver uses 0xc8834540 instead. According to my tests, the value from the reference driver is correct. The updated examples are representing 0x1621 from the reference driver's mc_val property, which is used when there is an external gbit PHY connected. For RMII mode PHYs mc_val 0x1800 is used in the reference driver, which translates would translate to "do not set any of the following properties" (as the two bits are configured automatically): - amlogic,enable-25mhz-phy-clk - amlogic,mp2-clock - amlogic,tx-delay Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts | 4 ++++ arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi | 4 ++++ arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi | 4 ++++ arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 12 +++++++++--- 4 files changed, 21 insertions(+), 3 deletions(-)