Message-ID: <CAFBinCBZKw04FSUBEyuQNdwbNJLjTK6y8p-MmNBHu=dLVOnKCg@mail.gmail.com>
Date:   Tue, 16 Jan 2018 12:17:29 +0100
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     Jerome Brunet <jbrunet@...libre.com>
Cc:     Yixun Lan <yixun.lan@...ogic.com>, davem@...emloft.net,
        netdev@...r.kernel.org, ingrassia@...genesys.com,
        linus.luessing@...3.blue, Neil Armstrong <narmstrong@...libre.com>,
        khilman@...libre.com, alexandre.torgue@...com,
        linux-amlogic@...ts.infradead.org, peppe.cavallaro@...com
Subject: Re: [PATCH net-next v5 0/4] dwmac-meson8b: clock fixes for Meson8b

Hi Jerome, Hi Yixun,

On Tue, Jan 16, 2018 at 10:37 AM, Jerome Brunet <jbrunet@...libre.com> wrote:
> On Tue, 2018-01-16 at 16:25 +0800, Yixun Lan wrote:
>>
>> On 01/16/18 01:10, Martin Blumenstingl wrote:
>> > Hi Dave,
>> >
>> > this series is now successfully tested, thus we think it's ready to be
>> > applied to your net-next tree.
>> >
>> > Emiliano reported [0] that he couldn't get dwmac-meson8b to work on his
>> > Odroid-C1. This is the (hopefully) final version of this series, which
>> > was successfully tested.
>> >
>> > Due to the fact that the public S805/S905/S912 datasheets all seem to
>> > be outdated regarding the description of the PRG_ETH0 (also called
>> > PRG_ETHERNET_ADDR0) register Linus Lüssing offered to help testing with
>> > an oscilloscope and an Odroid-C1. I would like to say HUGE thanks to him
>> > at this point as he spent hours figuring out the effects of the bits
>> > that are (though to be) relevant to get Ethernet working on the
>> > Odroid-C1.
>> > We tested three scenarios, all based on version 3 of this series:
>> > 1) MPLL2 at ~500MHz, m250_div set to 1, bit 10 enabled
>> > this resulted in a clock rate twice as high as expected at the RGMII TX
>> > clock pin (250MHz instead of 125MHz for Gbit connections and 50MHz
>> > instead of 25MHz for 100Mbit/s connections). it did not change the
>> > rate at the XTAL_IN pin of PHY (which stayed consistenly at 25MHz)
>> > 2) MPLL2 at ~250MHz, m250_div set to 1, bit 10 disabled
>> > the oscilloscope shows "no clock" for the RGMII TX clock pin at it's
>> > highest resolution (and random rates at lower resolutions). XTAL_IN is
>> > still at 25MHz
>> > 3) MPLL2 at ~250MHz, m250_div set to 1, bit 10 enabled
>> > this resulted in a 125MHz signal at the RGMII TX clock pin for Gbit
>> > speeds and 25MHz for 100Mbit/s - both values are as expected. The rate
>> > on the XTAL_IN pin was at 25MHz
>> > -> boot-logs (with the PRG_ETH0 register value) and screenshots from the
>> > readings of the oscilloscope can be found at:
>> > https://metameute.de/~tux/linux/amlogic/odroidc1/ethernet/
>> >
>> > Version 4 of this series is based on the results from Linus Lüssing's
>> > help with the oscilloscope and Odroid-C1.
>> > Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
>> > only partially test this. @Emiliano: Could you please give this version
>> > a try and let me know about the results (preferably with a "Tested-by"
>> > if it works)?
>> > You obviously still need your two "ARM: dts: meson8b" patches which
>> > - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
>> > - enable Ethernet on the Odroid-C1 (according to your last thest a TX
>> >   delay of 4ns is required to make it work properly)
>> >
>> > When testing on Meson8b this also needs a fix for the MPLL clock driver:
>> > "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
>> > https://patchwork.kernel.org/patch/10131677/
>> >
>> > I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
>> > and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
>> > fine (so let's hope that this also fixes your Meson8b issue :)).
>> >
>> > changes since v4 at [4]:
>> > - dropped "RFT" status since Jerome tested this series successfully!
>> > - dropped PATCH #2 ("simplify generating the clock names"). I will
>> >   improve the whole clock registration in a separate series. since that
>> >   patch didn't really improve anything I dropped it for now
>> > - added Jerome's Acked-/Reviewed-/Tested-by's - many thanks!
>> >
>> > changes since v3 at [3]:
>> > - renamed the function PATCH #1 from meson8b_init_rgmii_clk to
>> >   meson8b_init_rgmii_tx_clk since we now know what the register bits
>> >   mean
>> > - rewrote PATCH #3 because bit 10 is a gate clock and it seems that
>> >   there is an internal fixed divide-by-2 clock. see the patch
>> >   description for a detailed explanation
>> > - updated the description of PATCH #4 and #5 as the clock we're trying
>> >   to fix is the "RGMII TX" clock (old version stated that this is the
>> >   "RGMII clock" or "PHY reference clock"). also updated the numbers in
>> >   the description now that we have the clock hierarchy right (at least
>> >   we hope so)
>> >
>> > changes since v2 at [2]:
>> > - added PATCH #2 to make the following patch easier
>> > - Emiliano reported that there's currently another bug in the
>> >   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
>> >   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
>> >   (instead of a divide by 5 or divide by 10 clock divider). This has not
>> >   been visible on GXBB and later due to the input clock which always led
>> >   to a selection of "divide by 10" (which is done internally in the IP
>> >   block, but the bit actually means "enable RGMII clock output").
>> >   PATCH #3 was added to address this issue.
>> > - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
>> >   updated and the patch itself rebased because the m25_div clock was
>> >   removed with the new PATCH #3 (so some of the statements were not
>> >   valid anymore)
>> >
>> > changes since v1 at [1]:
>> > - changed the subject of the cover-letter to indicate that this is all
>> >   about the RGMII clock
>> > - added PATCH #1 which ensures that we don't unnecessarily change the
>> >   parent clocks in RMII mode (and also makes the code easier to
>> >   understand)
>> > - changed subject of PATCH #2 (formerly PATCH #1) to state that this
>> >   is about the RGMII clock
>> > - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
>> > - replaced PATCH #3 (formerly PATCH #2) with one that sets
>> >   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
>> >   on Meson8b correctly
>> >
>> >
>> > [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
>> > [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
>> > [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
>> > [3] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005899.html
>> > [4] http://lists.infradead.org/pipermail/linux-amlogic/2018-January/006125.html
>> >
>> > Martin Blumenstingl (4):
>> >   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
>> >   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
>> >   net: stmmac: dwmac-meson8b: fix setting the RGMII TX clock on Meson8b
>> >   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
>> >
>> >  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 113 ++++++++++++---------
>> >  1 file changed, 63 insertions(+), 50 deletions(-)
>> >
>>
>>
>> Hi Martin
>>
>>  I'm having problem with this series applied.
>> I've tested on the A113D (AXG) platform, if this patch is applied, the
>> driver will choose MPLL2 as clk source, and seems it doesn't work out
>> with this clk (fail to get IP)
>>
>> grep mpll2 /sys/kernel/debug/clk/clk_summary
>>        mpll2                              1            1   249999449
>>      0 0
>>
>
> This is because the fixed pll is 1992000000 on the axg instead of 2Ghz.
is there a chance that we can have a public AXG datasheet, similar to
what Hardkernel and Khadas published for the S805, S905, S905X and
S912?
this would give us community developers a chance to know that we are
going to break something on AXG *before* a patch is published (like in
this case :( )

> As a consequence fdiv4 is 498000000.
>
> CCF can reach something closer to 250Mhz with the mpll2.
>
> The easy way to fix this would be to make the inputs of ethernet mux optional
> and not provide the MPLL2 on the axg.
>
> However, this raises a few question on the axg platform :
> 1) Why is the root pll 1992Mhz and not 2GHz ? seems a weird choice just for 4MHz
> This 1992Mhz does not seems to help generate audio freqs anymore than 2GHz
> would. I don't really get this choice. Could you help us understand Yixun ?
>
> 2) Why is ethernet not working with mpll2 on axg ? espcially since we have
> proven the rate be correct on meson8 ... is there any change on this platform we
> don't know about ?
I tried to force my Khadas VIM2 (GXM) into using MPLL2 by changing
meson-gxl.dtsi:
        clocks = <&clkc CLKID_ETH>,
-                <&clkc CLKID_FCLK_DIV2>,
+                <&xtal>,
                <&clkc CLKID_MPLL2>;

the resulting clock tree looks fine:
    fixed_pll                         4        4        0  2000000000
        0 0
      mpll2_div                      1        1        0   250000000
       0 0
         mpll2                       1        1        0   250000000
       0 0
            c9410000.ethernet#m250_sel       1        1        0
250000000          0 0
               c9410000.ethernet#m250_div       1        1        0
250000000          0 0
                  c9410000.ethernet#fixed_div2       1        1
0   125000000          0 0
                     c9410000.ethernet#rgmii_tx_en       1        1
    0   125000000          0 0

however, Ethernet is broken (I can't ping anything)

@Yixun: according to all public datasheets bit 4 is reserved
however, Mike and Kevin were told that bit 4 controls a mux between
FCLK_DIV2 and MPLL2
could you please clarify the meaning of this bit 4 on:
- Meson8b
- Meson8m2 (a confirmation that it uses the same Ethernet registers
with the same bits/meanings of these as Meson8b would be enough)
- GXBB / GXL / GXM (assuming that these are using an identical IP for PRG_ETH0)
- AXG


Regards
Martin

Powered by blists - more mailing lists