From mboxrd@z Thu Jan 1 00:00:00 1970 From: ingrassia@epigenesys.com (Emiliano Ingrassia) Date: Tue, 21 Nov 2017 16:36:13 +0100 Subject: [PATCH 2/4] ARM: dts: meson8b: extending ethernet controller description In-Reply-To: References: <20170929191021.GA20840@ingrassia.epigenesys.com> Message-ID: <20171121153613.GA16818@ingrassia.epigenesys.com> To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org Hi Martin, sorry for my very late response! On Sat, Sep 30, 2017 at 04:09:48PM +0200, Martin Blumenstingl wrote: > Hi Emiliano, > > On Fri, Sep 29, 2017 at 9:10 PM, Emiliano Ingrassia > wrote: > > Hi Martin, > > > > thanks for the review! > > > > On Thu, Sep 28, 2017 at 11:41:48PM +0200, Martin Blumenstingl wrote: > >> Hi Emiliano, > >> > >> On Wed, Sep 27, 2017 at 11:39 PM, Emiliano Ingrassia > >> wrote: > >> > This patch adds ethernet controller pin description and extend its > >> > attributes in the relative node. > >> > > >> > Signed-off-by: Emiliano Ingrassia > >> > --- > >> > > >> > This patch corrects the meson8b-dwmac reg attributes updated by the previous > >> > 2/4 patch (450a483abe07f8d903c6cb74091592743975a8eb). > >> > The second addresses range, taken from S805 (aka Meson8b) SoC manual, > >> > was not correct. > >> > > >> > Please, apply this patch and discard the previous > >> > (450a483abe07f8d903c6cb74091592743975a8eb). > >> > > >> > arch/arm/boot/dts/meson8b.dtsi | 40 ++++++++++++++++++++++++++++++++++++++-- > >> > 1 file changed, 38 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi > >> > index bc278da7df0d..816bc9188f44 100644 > >> > --- a/arch/arm/boot/dts/meson8b.dtsi > >> > +++ b/arch/arm/boot/dts/meson8b.dtsi > >> > @@ -154,12 +154,48 @@ > >> > #gpio-cells = <2>; > >> > gpio-ranges = <&pinctrl_cbus 0 0 130>; > >> > }; > >> > + > >> > + eth_rgmii_pins: eth-rgmii { > >> > + mux { > >> > + groups = "eth_tx_clk", > >> > + "eth_tx_en", > >> > + "eth_txd1_0", > >> > + "eth_txd1_1", > >> > + "eth_txd0_0", > >> > + "eth_txd0_1", > >> > + "eth_rx_clk", > >> > + "eth_rx_dv", > >> > + "eth_rxd1", > >> > + "eth_rxd0", > >> > + "eth_mdio_en", > >> > + "eth_mdc", > >> > + "eth_ref_clk", > >> > + "eth_txd2", > >> > + "eth_txd3"; > >> > + function = "ethernet"; > >> > + }; > >> > + }; > >> > }; > >> > }; > >> > > >> > ðmac { > >> > - clocks = <&clkc CLKID_ETH>; > >> > - clock-names = "stmmaceth"; > >> > + compatible = "amlogic,meson8b-dwmac", "snps,dwmac-3.70a", "snps,dwmac"; > >> without a reg property this passes 0xc1108108 (as defined in > >> meson.dtsi) to the meson8b-dwmac driver. > >> are you sure that this shouldn't be 0xc1108140 (like in your initial patch)? > >> 0xc1108108 translates to 0x2050 (calculation formula: (0xc1108108 - > >> cbus base addr 0xc1100000) / 4) which is used in Amlogic's u-boot > >> sources, for example [0] > >> > > > > Yes, I know. This was the intention. > OK, this is interesting > After some research I agree with you: the correct ethernet register address is 0xc1108140. > >> currently the meson8b-dwmac driver is writing to the old register > >> location which probably does nothing. > without your change the meson6-dwmac driver is used. when I wrote the > meson8b-dwmac driver I documented the following behavior (see [0]): > "This worked for many boards because the bootloader programs the > PRG_ETHERNET registers correctly. Additionally the meson6-dwmac driver > only sets bit 1 of PRG_ETHERNET_ADDR0 which (according to the > datasheet) is only used during reset." > > > Actually, changing the second addresses range from 0xc1108108 to > > 0xc1108140 leads to an unusable ethernet controller. > Odroid-C1's u-boot writes 0x7d21 to 0xc1108140 (= (0x2050 * 4) + cbus > base), see [1] > have you tried to verify that writing 0x7d21 (= the value used in the > Odroid-C1 u-boot sources) at the end of meson8b_init_prg_eth makes it > work for you again? if it does then we are not setting the register > values correctly (which may simply be related to the clock setup - > either the internal clock in the meson8b-dwmac driver or the "other" > ethernet clock) Actually, writing 0x7d21 at the end of the initialization procedure leads to a working ethernet controller. Consider that I'm using MPLL2 as clock source for both "clkin0" and "clkin1" because, as stated in S805 SoC manual, "M8Baby internal clock source is mp2_clk_out only.". Investigating dwmac-meson8b.c, a possible error lies in the use of prg_ethernet_addr0 register bits 9-7. Infact, they are used as field for CLK_M250_DIV value, which it seems to me incorrect. From the SoC manual, those bits should be set as mpll2 clock rate divided by 250*1000*1000. Then, I guess that the correct divider value for 25 Mhz PHY clock is internally derived. Best regards, Emiliano > > >> if above statement is true then you are relying on the bootloader to > >> set up 0xc1108140 correctly. > > > > OK, sure this is bad! Those addresses are documented in S805 SoC manual > > and we should set up them correctly. > > However, checking the Odroid-C2 device tree I couldn't find > > the PRG_ETHERNET_ADDR{0,1} addresses, which are documented in S905 SoC > > manual. > > Probably I'm missing something, but don't we have the same situation on > > that board? > I'm not sure if I understand you correctly: > - the S805 datasheet mentioned that the addresses of the > PRG_ETHERNET_ADDR{0,1} registers are 0x2050 and 0x2051 > - we assume that the PRG_ETHERNET_ADDR{0,1} address from the S905 > datasheet is wrong, see [4] > - Mike got feedback from Amlogic confirming that the documentation in > the S905 datasheet is not fully correct, see [2] and [5]. if they use > the same IP block in Meson8b (like, due to the identical register > description in the S805 and S905 datasheets) then the S805 datasheet > is also wrong > > >> the reason why I wrote this meson8b-dwmac driver is because I had a > >> GXBB board with RGMII PHY but u-boot configured the register to RMII > >> mode -> ethernet wasn't working. > > > > Meson8b refers to S805 SoC while GXBB should refer to S905 SoC. > > At first look it seemed to me that dwmac-meson8b driver correctly > > support dwmac on Meson8b or we should extend the driver to better > > support it? > I added compatible strings for both SoCs (see [3]) because the > register description in both datasheets is identical. dwmac-meson8b > works fine on all known GXBB/GXL/GXM devices > so it's the Meson8b compatibility that has to be improved (if it has > to be improved, but for that we have to figure out the clock setup > too). > > >> you could verify this by zeroing both (0xc1108108 and 0xc1108140) in > >> u-boot or at the start of the meson8b-dwmac driver and see if ethernet > >> still works for you > >> > > > > I'll check it. > thank you! > please also see also my comment above regarding 0x7d21 from > Odroid-C1's u-boot sources > > >> > + > >> > + interrupts = , > >> > + ; > >> > + interrupt-names = "macirq", > >> > + "eth_lpi"; > >> did you receive one of the eth_lpi interrupts? if it works for you > >> then we should try to add this to meson-gx.dtsi as well > >> I also wonder if we should configure it in meson8b.dtsi or meson.dtsi > >> > > > > Needs more testing. > > > >> > + > >> > + clock-names = "stmmaceth", "clkin0", "clkin1"; > >> > + clocks = <&clkc CLKID_ETH>, > >> > + <&clkc CLKID_FCLK_DIV2>, > >> > + <&clkc CLKID_MPLL2>; > >> > + > >> > + resets = <&reset RESET_ETHERNET>; > >> > + reset-names = "stmmaceth"; > >> I'm not sure if this works: > >> our reset controller implements a reset pulse (write bit, IP block > >> executes a reset and clears the bit again) > >> stmmac on the other hand manually asserts and deasserts the reset line > >> (which is not implemented by our reset driver), see [1] > >> > > > > OK, I'll check and eventually fix this. > as a small hint: on Meson GX (GXBB and newer) we currently do not > configure the reset line > if it's needed on Meson8b then you could implement it in a separate patch > > >> > + > >> > + rx-fifo-depth=<4000>; > >> > + tx-fifo-depth=<2000>; > >> could you please add spaces around "=" and some info to the commit > >> message why this is necessary and where you got these values from > >> > > > > Those are optional attributes documented in > > Documentation/devicetree/bindings/net/stmmac.txt. > > The values were taken from S805 SoC manual, ch. 22 (ETHERNET MAC). > ok, then these are identical on GXBB and newer (according to the datasheets) > I wonder if the stmmac driver auto-detects the correct value when > these are not set. otherwise we should also configure these on the GX > SoCs > > >> > }; > >> > > >> > &hwrng { > >> > -- > >> > 2.14.1 > >> > > >> > > >> > _______________________________________________ > >> > linux-amlogic mailing list > >> > linux-amlogic at lists.infradead.org > >> > http://lists.infradead.org/mailman/listinfo/linux-amlogic > >> > >> looking forward to proper ethernet support on Meson8/Meson8b! > >> > > > > OK, thanks for your suggestions! > > > >> > >> Regards, > >> Martin > >> > > > > Regards, > > > > Emiliano > > > >> > >> [0] https://github.com/hardkernel/u-boot/blob/odroidc-v2011.03/board/hardkernel/odroidc/odroidc-eth.c#L29 > >> [1] https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c#L4123 > > > Regards, > Martin > > > [0] http://lists.infradead.org/pipermail/linux-amlogic/2016-August/000764.html > [1] https://github.com/hardkernel/u-boot/blob/odroidc-v2011.03/board/hardkernel/odroidc/odroidc-eth.c#L59 > [2] http://lists.infradead.org/pipermail/linux-amlogic/2016-July/000645.html > [3] https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c#L330 > [4] http://lists.infradead.org/pipermail/linux-amlogic/2016-September/001055.html > [5] http://lists.infradead.org/pipermail/linux-amlogic/2016-July/000646.html