[RFT net-next 0/2] dwmac-meson8b: clock rounding fixes for Meson8b

[RFT net-next 0/2] dwmac-meson8b: clock rounding fixes for Meson8b

[Martin Blumenstingl]

Hi Dave,

please do not apply this series until it got a Tested-by from Emiliano.


Hi Emiliano,

you reported [0] that you couldn't get dwmac-meson8b to work on your
Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
I think I was able to find a fix: it consists of two patches (which you
find in this series)

Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
only partially test this (I could only check if the clocks were
calculated correctly when using a dummy 500002394Hz input clock instead
of MPLL2).

Could you please give this series a try and let me know about the
results?
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

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 :)).


[0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html

Martin Blumenstingl (2):
  net: stmmac: dwmac-meson8b: fix setting the PHY clock on Meson8b
  net: stmmac: dwmac-meson8b: don't try to change m250_div parent's rate

 drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.15.1

[RFT net-next 1/2] net: stmmac: dwmac-meson8b: fix setting the PHY clock on Meson8b

[Martin Blumenstingl]

Meson8b only supports MPLL2 as clock input. The rate of the MPLL2 clock
set by Odroid-C1's u-boot is close to 500MHz. The exact rate is
500002394Hz, which is calculated in drivers/clk/meson/clk-mpll.c
using the following formula:
DIV_ROUND_UP_ULL((u64)parent_rate * SDM_DEN, (SDM_DEN * n2) + sdm)
Odroid-C1's u-boot configures MPLL2 with the following values:
- SDM_DEN = 16384
- SDM = 1638
- N2 = 5

The 250MHz and 25MHz clocks inside dwmac-meson8b driver are derived
from the MPLL2 clock. Due to MPLL2 running slightly faster than 500MHz
the common clock framework chooses dividers which are too big to
generate the 250MHz and 25MHz clocks. Emiliano Ingrassia observed that
the divider for the 250MHz clock was set to 0x5 which results in a clock
rate of close to 100MHz instead of 250MHz. The divider for the 25MHz
clock is set to 0x0 (which means "divide by 5") so the resulting RGMII
clock is running at 20MHz (plus a few additional Hz). The RTL8211F PHY
on Odroid-C1 however fails to operate with a 20MHz RGMII clock.

Round the divider's clock rates to prevent this issue on Meson8b. This
means we'll now end up with a clock rate of 25000120Hz (= 25MHz plus
120Hz).
This has no effect on the Meson GX SoCs since there fclk_div2 is used as
input clock, which has a rate of 1000MHz (and thus is divisible cleanly
to 250MHz and 25MHz).

Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index 4404650b32c5..c71966332387 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -144,7 +144,9 @@ static int meson8b_init_clk(struct meson8b_dwmac *dwmac)
 	dwmac->m250_div.shift = PRG_ETH0_CLK_M250_DIV_SHIFT;
 	dwmac->m250_div.width = PRG_ETH0_CLK_M250_DIV_WIDTH;
 	dwmac->m250_div.hw.init = &init;
-	dwmac->m250_div.flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO;
+	dwmac->m250_div.flags = CLK_DIVIDER_ONE_BASED |
+				CLK_DIVIDER_ALLOW_ZERO |
+				CLK_DIVIDER_ROUND_CLOSEST;
 
 	dwmac->m250_div_clk = devm_clk_register(dev, &dwmac->m250_div.hw);
 	if (WARN_ON(IS_ERR(dwmac->m250_div_clk)))
@@ -164,7 +166,8 @@ static int meson8b_init_clk(struct meson8b_dwmac *dwmac)
 	dwmac->m25_div.width = PRG_ETH0_CLK_M25_DIV_WIDTH;
 	dwmac->m25_div.table = clk_25m_div_table;
 	dwmac->m25_div.hw.init = &init;
-	dwmac->m25_div.flags = CLK_DIVIDER_ALLOW_ZERO;
+	dwmac->m25_div.flags = CLK_DIVIDER_ALLOW_ZERO |
+				CLK_DIVIDER_ROUND_CLOSEST;
 
 	dwmac->m25_div_clk = devm_clk_register(dev, &dwmac->m25_div.hw);
 	if (WARN_ON(IS_ERR(dwmac->m25_div_clk)))
-- 
2.15.1

Re: [RFT net-next 1/2] net: stmmac: dwmac-meson8b: fix setting the PHY clock on Meson8b

[Jerome Brunet]

On Sat, 2017-12-23 at 18:04 +0100, Martin Blumenstingl wrote:
> Meson8b only supports MPLL2 as clock input. The rate of the MPLL2 clock
> set by Odroid-C1's u-boot is close to 500MHz. The exact rate is
> 500002394Hz, which is calculated in drivers/clk/meson/clk-mpll.c
> using the following formula:
> DIV_ROUND_UP_ULL((u64)parent_rate * SDM_DEN, (SDM_DEN * n2) + sdm)
> Odroid-C1's u-boot configures MPLL2 with the following values:
> - SDM_DEN = 16384
> - SDM = 1638
> - N2 = 5
> 
> The 250MHz and 25MHz clocks inside dwmac-meson8b driver are derived
> from the MPLL2 clock. Due to MPLL2 running slightly faster than 500MHz
> the common clock framework chooses dividers which are too big to
> generate the 250MHz and 25MHz clocks. Emiliano Ingrassia observed that
> the divider for the 250MHz clock was set to 0x5 which results in a clock
> rate of close to 100MHz instead of 250MHz. The divider for the 25MHz
> clock is set to 0x0 (which means "divide by 5") so the resulting RGMII
> clock is running at 20MHz (plus a few additional Hz). The RTL8211F PHY
> on Odroid-C1 however fails to operate with a 20MHz RGMII clock.
> 
> Round the divider's clock rates to prevent this issue on Meson8b. This
> means we'll now end up with a clock rate of 25000120Hz (= 25MHz plus
> 120Hz).
> This has no effect on the Meson GX SoCs since there fclk_div2 is used as
> input clock, which has a rate of 1000MHz (and thus is divisible cleanly
> to 250MHz and 25MHz).
> 
> Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
> Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Makes sense to add ROUND_CLOSEST (no risk if the rate is slightly over the
requested one)

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

[RFT net-next 2/2] net: stmmac: dwmac-meson8b: don't try to change m250_div parent's rate

[Martin Blumenstingl]

Trying to set the rate of m250_div's parent clock makes no sense since
it's a mux which has neither CLK_MUX_ROUND_CLOSEST nor
CLK_SET_RATE_PARENT set.
It even does harm on Meson8b SoCs where the input clock for the mux
cannot be divided down to 250MHz evenly (the parent rate is 500002394Hz)
which is why we need to use CLK_DIVIDER_ROUND_CLOSEST for the m250_div
clock. The clk-divider driver however ignores the
CLK_DIVIDER_ROUND_CLOSEST flag if CLK_SET_RATE_PARENT is set (because
it simply tries to set the best possible clock rate for the parent,
which does nothing in our case since the parent is a mux which doesn't
allow rate changes as explained above).

This fixes setting the RGMII clock on Meson8 SoCs which ended up with a
~20MHz clock instead of the expected ~25MHz.
The dwmac-meson8b driver requests a 25MHz clock rate for the m25_div
(which only supports "divide by 5" and "divide by 10") clock which is
derived from the m250_div clock. Due to clk-divider ignoring the
CLK_DIVIDER_ROUND_CLOSEST flag the resulting m250_div clock was set to
~100MHz (divider = 5) and the m25_div clock was set to ~20MHz (divider =
5) by the common clock framework (as this value is closest to 25MHz if
we would not have set CLK_DIVIDER_ROUND_CLOSEST). What we actually need
however is a rate of ~250MHz on the m250_div clock (divider = 2) and
~25MHz on the m25_div clock (divider = 10) - these are also the values
chosen by the out-of-tree vendor driver.
With this we end up with a RGMII clock of 25000120Hz (which is as close
to 25MHz we can get with an input clock of 500002394Hz).

SoCs from the Meson GX series are not affected by this change because
the input clock is FCLK_DIV2 whose rate cannot be changed. Additionally
the GX SoCs don't need to use the "closest" divider since the parent
clock is a multiple of 250MHz.

Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index c71966332387..26f41c117d63 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -135,7 +135,7 @@ static int meson8b_init_clk(struct meson8b_dwmac *dwmac)
 	snprintf(clk_name, sizeof(clk_name), "%s#m250_div", dev_name(dev));
 	init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
 	init.ops = &clk_divider_ops;
-	init.flags = CLK_SET_RATE_PARENT;
+	init.flags = 0;
 	clk_div_parents[0] = __clk_get_name(dwmac->m250_mux_clk);
 	init.parent_names = clk_div_parents;
 	init.num_parents = ARRAY_SIZE(clk_div_parents);
-- 
2.15.1

Re: [RFT net-next 2/2] net: stmmac: dwmac-meson8b: don't try to change m250_div parent's rate

[Jerome Brunet]

On Sat, 2017-12-23 at 18:04 +0100, Martin Blumenstingl wrote:
> Trying to set the rate of m250_div's parent clock makes no sense since
> it's a mux which has neither CLK_MUX_ROUND_CLOSEST nor
> CLK_SET_RATE_PARENT set.
> It even does harm on Meson8b SoCs where the input clock for the mux
> cannot be divided down to 250MHz evenly (the parent rate is 500002394Hz)

So your problem is more with the mux actually, not the divider. Instead of
removing CLK_SET_RATE_PARENT from the divider, may I suggest to put

CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT on the parent mux, and keep
CLK_SET_RATE_PARENT (with CLK_DIVIDER_ROUND_CLOSEST)  on the divS.

I suppose it would a accomplish the same thing with one added benefits for
meson8b :

If the bootloader did not set the mpll2 to the correct rate, it will now be done
thanks to rate propagation.

If I missed anything, feel free to point it out.

Cheers 

> which is why we need to use CLK_DIVIDER_ROUND_CLOSEST for the m250_div
> clock. The clk-divider driver however ignores the
> CLK_DIVIDER_ROUND_CLOSEST flag if CLK_SET_RATE_PARENT is set (because
> it simply tries to set the best possible clock rate for the parent,
> which does nothing in our case since the parent is a mux which doesn't
> allow rate changes as explained above).
> 
> This fixes setting the RGMII clock on Meson8 SoCs which ended up with a
> ~20MHz clock instead of the expected ~25MHz.
> The dwmac-meson8b driver requests a 25MHz clock rate for the m25_div
> (which only supports "divide by 5" and "divide by 10") clock which is
> derived from the m250_div clock. Due to clk-divider ignoring the
> CLK_DIVIDER_ROUND_CLOSEST flag the resulting m250_div clock was set to
> ~100MHz (divider = 5) and the m25_div clock was set to ~20MHz (divider =
> 5) by the common clock framework (as this value is closest to 25MHz if
> we would not have set CLK_DIVIDER_ROUND_CLOSEST). What we actually need
> however is a rate of ~250MHz on the m250_div clock (divider = 2) and
> ~25MHz on the m25_div clock (divider = 10) - these are also the values
> chosen by the out-of-tree vendor driver.
> With this we end up with a RGMII clock of 25000120Hz (which is as close
> to 25MHz we can get with an input clock of 500002394Hz).
> 
> SoCs from the Meson GX series are not affected by this change because
> the input clock is FCLK_DIV2 whose rate cannot be changed. Additionally
> the GX SoCs don't need to use the "closest" divider since the parent
> clock is a multiple of 250MHz.
> 
> Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> index c71966332387..26f41c117d63 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> @@ -135,7 +135,7 @@ static int meson8b_init_clk(struct meson8b_dwmac *dwmac)
>  	snprintf(clk_name, sizeof(clk_name), "%s#m250_div", dev_name(dev));
>  	init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
>  	init.ops = &clk_divider_ops;
> -	init.flags = CLK_SET_RATE_PARENT;
> +	init.flags = 0;
>  	clk_div_parents[0] = __clk_get_name(dwmac->m250_mux_clk);
>  	init.parent_names = clk_div_parents;
>  	init.num_parents = ARRAY_SIZE(clk_div_parents);

Re: [RFT net-next 2/2] net: stmmac: dwmac-meson8b: don't try to change m250_div parent's rate

[Jerome Brunet]

On Sat, 2017-12-23 at 18:40 +0100, Jerome Brunet wrote:
> > Trying to set the rate of m250_div's parent clock makes no sense since
> > it's a mux which has neither CLK_MUX_ROUND_CLOSEST nor
> > CLK_SET_RATE_PARENT set.
> > It even does harm on Meson8b SoCs where the input clock for the mux
> > cannot be divided down to 250MHz evenly (the parent rate is 500002394Hz)
> 
> So your problem is more with the mux actually, not the divider. Instead of
> removing CLK_SET_RATE_PARENT from the divider, may I suggest to put
> 
> CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT on the parent mux, and keep

Thinking about it, you don't even need CLK_SET_RATE_NO_REPARENT. Just let rate
propagation figure out the best combination 

> CLK_SET_RATE_PARENT (with CLK_DIVIDER_ROUND_CLOSEST)  on the divS.
> 
> I suppose it would a accomplish the same thing with one added benefits for
> meson8b :
> 
> If the bootloader did not set the mpll2 to the correct rate, it will now be done
> thanks to rate propagation.
> 
> If I missed anything, feel free to point it out.
> 
> Cheers 

Re: [RFT net-next 2/2] net: stmmac: dwmac-meson8b: don't try to change m250_div parent's rate

[Martin Blumenstingl]

Hi Jerome,

On Sat, Dec 23, 2017 at 6:40 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sat, 2017-12-23 at 18:04 +0100, Martin Blumenstingl wrote:
>> Trying to set the rate of m250_div's parent clock makes no sense since
>> it's a mux which has neither CLK_MUX_ROUND_CLOSEST nor
>> CLK_SET_RATE_PARENT set.
>> It even does harm on Meson8b SoCs where the input clock for the mux
>> cannot be divided down to 250MHz evenly (the parent rate is 500002394Hz)
>
> So your problem is more with the mux actually, not the divider. Instead of
> removing CLK_SET_RATE_PARENT from the divider, may I suggest to put
>
> CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT on the parent mux, and keep
> CLK_SET_RATE_PARENT (with CLK_DIVIDER_ROUND_CLOSEST)  on the divS.
I just gave this a try, here's the result:
       mpll2                              1            1   127488329
       0 0
         c9410000.ethernet#m250_sel           1            1
127488329          0 0
            c9410000.ethernet#m250_div           1            1
127488329          0 0
               c9410000.ethernet#m25_div           1            1
25497666          0 0

> I suppose it would a accomplish the same thing with one added benefits for
> meson8b :
>
> If the bootloader did not set the mpll2 to the correct rate, it will now be done
> thanks to rate propagation.
indeed, mpll2 is set to 0 by the bootloader on my board
with that change we'll now also set the rate "correctly" (see below)

> If I missed anything, feel free to point it out.
it seems however that clk-mpll incorrectly calculates the register values
with the mpll2 register values from Emiliano we can get to 25000120Hz
however, I guess we need to have a look at the clk-mpll driver because:
- by letting the common clock framework set the rate of mpll2 we get
25497666Hz (which means we're off by by ~500kHz, instead of 120Hz)
- according to the datasheet the allowed range of the mpll2 clock is
250..637MHz - 127488329Hz is what we get from the common clock
framework

Jerome: any idea why that is (more theoretical number games below though :))?

just a reminder from the other patch - these are the values used for
the mpll2 clock:
- parent rate = 2550MHz
- SDM_DEN = 16384
- SDM = 1638
- N2 = 5
= (parent rate 2550000000 * SDM_DEN 16384) / ((SDM_DEN 16384 * N2 5) +
1638) = 500002394Hz

using an SDM of 1639 gives us a 499996410Hz mpll2 clock
with all the dividers we get down to a RGMII clock of 24999821Hz which
is 179Hz off the desired 25MHz
in other words: the mpll2 values set by Odroid-C1's u-boot are "best",
so if we try to set mpll2's rate through the common clock framework we
should try to get the same results (else we would just make the result
worse)

> Cheers
>
>> which is why we need to use CLK_DIVIDER_ROUND_CLOSEST for the m250_div
>> clock. The clk-divider driver however ignores the
>> CLK_DIVIDER_ROUND_CLOSEST flag if CLK_SET_RATE_PARENT is set (because
>> it simply tries to set the best possible clock rate for the parent,
>> which does nothing in our case since the parent is a mux which doesn't
>> allow rate changes as explained above).
>>
>> This fixes setting the RGMII clock on Meson8 SoCs which ended up with a
>> ~20MHz clock instead of the expected ~25MHz.
>> The dwmac-meson8b driver requests a 25MHz clock rate for the m25_div
>> (which only supports "divide by 5" and "divide by 10") clock which is
>> derived from the m250_div clock. Due to clk-divider ignoring the
>> CLK_DIVIDER_ROUND_CLOSEST flag the resulting m250_div clock was set to
>> ~100MHz (divider = 5) and the m25_div clock was set to ~20MHz (divider =
>> 5) by the common clock framework (as this value is closest to 25MHz if
>> we would not have set CLK_DIVIDER_ROUND_CLOSEST). What we actually need
>> however is a rate of ~250MHz on the m250_div clock (divider = 2) and
>> ~25MHz on the m25_div clock (divider = 10) - these are also the values
>> chosen by the out-of-tree vendor driver.
>> With this we end up with a RGMII clock of 25000120Hz (which is as close
>> to 25MHz we can get with an input clock of 500002394Hz).
>>
>> SoCs from the Meson GX series are not affected by this change because
>> the input clock is FCLK_DIV2 whose rate cannot be changed. Additionally
>> the GX SoCs don't need to use the "closest" divider since the parent
>> clock is a multiple of 250MHz.
>>
>> Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> index c71966332387..26f41c117d63 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> @@ -135,7 +135,7 @@ static int meson8b_init_clk(struct meson8b_dwmac *dwmac)
>>       snprintf(clk_name, sizeof(clk_name), "%s#m250_div", dev_name(dev));
>>       init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
>>       init.ops = &clk_divider_ops;
>> -     init.flags = CLK_SET_RATE_PARENT;
>> +     init.flags = 0;
>>       clk_div_parents[0] = __clk_get_name(dwmac->m250_mux_clk);
>>       init.parent_names = clk_div_parents;
>>       init.num_parents = ARRAY_SIZE(clk_div_parents);
>

Regards
Martin

Re: [RFT net-next 2/2] net: stmmac: dwmac-meson8b: don't try to change m250_div parent's rate

[Jerome Brunet]

On Sat, 2017-12-23 at 21:00 +0100, Martin Blumenstingl wrote:
> Hi Jerome,
> 
> On Sat, Dec 23, 2017 at 6:40 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > On Sat, 2017-12-23 at 18:04 +0100, Martin Blumenstingl wrote:
> > > Trying to set the rate of m250_div's parent clock makes no sense since
> > > it's a mux which has neither CLK_MUX_ROUND_CLOSEST nor
> > > CLK_SET_RATE_PARENT set.
> > > It even does harm on Meson8b SoCs where the input clock for the mux
> > > cannot be divided down to 250MHz evenly (the parent rate is 500002394Hz)
> > 
> > So your problem is more with the mux actually, not the divider. Instead of
> > removing CLK_SET_RATE_PARENT from the divider, may I suggest to put
> > 
> > CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT on the parent mux, and keep
> > CLK_SET_RATE_PARENT (with CLK_DIVIDER_ROUND_CLOSEST)  on the divS.
> 
> I just gave this a try, here's the result:
>        mpll2                              1            1   127488329
>        0 0
>          c9410000.ethernet#m250_sel           1            1
> 127488329          0 0
>             c9410000.ethernet#m250_div           1            1
> 127488329          0 0
>                c9410000.ethernet#m25_div           1            1
> 25497666          0 0
> 
> > I suppose it would a accomplish the same thing with one added benefits for
> > meson8b :
> > 
> > If the bootloader did not set the mpll2 to the correct rate, it will now be done
> > thanks to rate propagation.
> 
> indeed, mpll2 is set to 0 by the bootloader on my board
> with that change we'll now also set the rate "correctly" (see below)
> 
> > If I missed anything, feel free to point it out.
> 
> it seems however that clk-mpll incorrectly calculates the register values
> with the mpll2 register values from Emiliano we can get to 25000120Hz
> however, I guess we need to have a look at the clk-mpll

Huuuu ! not again ! ;)

>  driver because:
> - by letting the common clock framework set the rate of mpll2 we get
> 25497666Hz (which means we're off by by ~500kHz, instead of 120Hz)

Could you dump the SDM and N2 values (devmem) that have been applied by CCF ?
to compare. If a better solution is available, I'm a bit surprised we don't find
 it. You may have a found something worth checking ...

> - according to the datasheet the allowed range of the mpll2 clock is
> 250..637MHz - 127488329Hz is what we get from the common clock
> framework

Yes, I've seen that but we are able compute out of that range and, so far, the
actual rate of clock seems coherent with the calculation. At least, when I
tested with the audio, it was the case.

> 
> Jerome: any idea why that is (more theoretical number games below though :))?
> 
> just a reminder from the other patch - these are the values used for
> the mpll2 clock:
> - parent rate = 2550MHz
> - SDM_DEN = 16384
> - SDM = 1638
> - N2 = 5
> = (parent rate 2550000000 * SDM_DEN 16384) / ((SDM_DEN 16384 * N2 5) +
> 1638) = 500002394Hz
> 
> using an SDM of 1639 gives us a 499996410Hz mpll2 clock
> with all the dividers we get down to a RGMII clock of 24999821Hz which
> is 179Hz off the desired 25MHz
> in other words: the mpll2 values set by Odroid-C1's u-boot are "best",
> so if we try to set mpll2's rate through the common clock framework we
> should try to get the same results (else we would just make the result
> worse)

Indeed, we should be able to do a lot better than 500kHz error. We should get to
the bottom of this. When testing on the S905 with audio, I got very values so I
did not question the mpll driver too much. Maybe there is something there.

Purely for debugging, from the ethernet driver, Could you try the following:
- Remove CLK_SET_RATE_PARENT from div250 to break the propagation there
- call set_rate on the mux with 500Mhz (we'll see far off we really are compared
to u-boot values and we will be able to compare)
- call set_rate on div25 as usual to get your ethernet running.

> 
> > Cheers
> > 
> > > which is why we need to use CLK_DIVIDER_ROUND_CLOSEST for the m250_div
> > > clock. The clk-divider driver however ignores the
> > > CLK_DIVIDER_ROUND_CLOSEST flag if CLK_SET_RATE_PARENT is set (because
> > > it simply tries to set the best possible clock rate for the parent,
> > > which does nothing in our case since the parent is a mux which doesn't
> > > allow rate changes as explained above).
> > > 
> > > This fixes setting the RGMII clock on Meson8 SoCs which ended up with a
> > > ~20MHz clock instead of the expected ~25MHz.
> > > The dwmac-meson8b driver requests a 25MHz clock rate for the m25_div
> > > (which only supports "divide by 5" and "divide by 10") clock which is
> > > derived from the m250_div clock. Due to clk-divider ignoring the
> > > CLK_DIVIDER_ROUND_CLOSEST flag the resulting m250_div clock was set to
> > > ~100MHz (divider = 5) and the m25_div clock was set to ~20MHz (divider =
> > > 5) by the common clock framework (as this value is closest to 25MHz if
> > > we would not have set CLK_DIVIDER_ROUND_CLOSEST). What we actually need
> > > however is a rate of ~250MHz on the m250_div clock (divider = 2) and
> > > ~25MHz on the m25_div clock (divider = 10) - these are also the values
> > > chosen by the out-of-tree vendor driver.
> > > With this we end up with a RGMII clock of 25000120Hz (which is as close
> > > to 25MHz we can get with an input clock of 500002394Hz).
> > > 
> > > SoCs from the Meson GX series are not affected by this change because
> > > the input clock is FCLK_DIV2 whose rate cannot be changed. Additionally
> > > the GX SoCs don't need to use the "closest" divider since the parent
> > > clock is a multiple of 250MHz.
> > > 
> > > Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
> > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> > > index c71966332387..26f41c117d63 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> > > @@ -135,7 +135,7 @@ static int meson8b_init_clk(struct meson8b_dwmac *dwmac)
> > >       snprintf(clk_name, sizeof(clk_name), "%s#m250_div", dev_name(dev));
> > >       init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
> > >       init.ops = &clk_divider_ops;
> > > -     init.flags = CLK_SET_RATE_PARENT;
> > > +     init.flags = 0;
> > >       clk_div_parents[0] = __clk_get_name(dwmac->m250_mux_clk);
> > >       init.parent_names = clk_div_parents;
> > >       init.num_parents = ARRAY_SIZE(clk_div_parents);
> 
> Regards
> Martin

Re: [RFT net-next 2/2] net: stmmac: dwmac-meson8b: don't try to change m250_div parent's rate

[Martin Blumenstingl]

Hi Jerome,

On Sat, Dec 23, 2017 at 9:40 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sat, 2017-12-23 at 21:00 +0100, Martin Blumenstingl wrote:
>> Hi Jerome,
>>
>> On Sat, Dec 23, 2017 at 6:40 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>> > On Sat, 2017-12-23 at 18:04 +0100, Martin Blumenstingl wrote:
>> > > Trying to set the rate of m250_div's parent clock makes no sense since
>> > > it's a mux which has neither CLK_MUX_ROUND_CLOSEST nor
>> > > CLK_SET_RATE_PARENT set.
>> > > It even does harm on Meson8b SoCs where the input clock for the mux
>> > > cannot be divided down to 250MHz evenly (the parent rate is 500002394Hz)
>> >
>> > So your problem is more with the mux actually, not the divider. Instead of
>> > removing CLK_SET_RATE_PARENT from the divider, may I suggest to put
>> >
>> > CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT on the parent mux, and keep
>> > CLK_SET_RATE_PARENT (with CLK_DIVIDER_ROUND_CLOSEST)  on the divS.
>>
>> I just gave this a try, here's the result:
>>        mpll2                              1            1   127488329
>>        0 0
>>          c9410000.ethernet#m250_sel           1            1
>> 127488329          0 0
>>             c9410000.ethernet#m250_div           1            1
>> 127488329          0 0
>>                c9410000.ethernet#m25_div           1            1
>> 25497666          0 0
>>
>> > I suppose it would a accomplish the same thing with one added benefits for
>> > meson8b :
>> >
>> > If the bootloader did not set the mpll2 to the correct rate, it will now be done
>> > thanks to rate propagation.
>>
>> indeed, mpll2 is set to 0 by the bootloader on my board
>> with that change we'll now also set the rate "correctly" (see below)
>>
>> > If I missed anything, feel free to point it out.
>>
>> it seems however that clk-mpll incorrectly calculates the register values
>> with the mpll2 register values from Emiliano we can get to 25000120Hz
>> however, I guess we need to have a look at the clk-mpll
>
> Huuuu ! not again ! ;)
>
>>  driver because:
>> - by letting the common clock framework set the rate of mpll2 we get
>> 25497666Hz (which means we're off by by ~500kHz, instead of 120Hz)
>
> Could you dump the SDM and N2 values (devmem) that have been applied by CCF ?
> to compare. If a better solution is available, I'm a bit surprised we don't find
>  it. You may have a found something worth checking ...
while calculating this with a target frequency of 500MHz manually
again I saw that there's a remainder of 10Mhz after the initial
division.
remainder * SDM_DEN = 163840000000 - this value overflows 32-bit,
things will go downhill from here... :)
long story short: here's a patch for that issue: [0]

the results with "CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT" on the mux:
    fixed_pll                             3            3  2550000000
       0 0
      mpll2                              1            1   124999851
      0 0
         c9410000.ethernet#m250_sel           1            1
124999851          0 0
            c9410000.ethernet#m250_div           1            1
124999851          0 0
               c9410000.ethernet#m25_div           1            1
24999971          0 0

this is even closer to 25MHz than the values in the vendor driver
(120Hz vs 29Hz)
I'll re-spin this series, then Emiliano "just" has to confirm that
it's also working for him (despite the dividers in the dwmac-meson8b
driver are set up different compared to the vendor driver)

>> - according to the datasheet the allowed range of the mpll2 clock is
>> 250..637MHz - 127488329Hz is what we get from the common clock
>> framework
>
> Yes, I've seen that but we are able compute out of that range and, so far, the
> actual rate of clock seems coherent with the calculation. At least, when I
> tested with the audio, it was the case.
I'm curious: ...on a GX SoCs probably?

>>
>> Jerome: any idea why that is (more theoretical number games below though :))?
>>
>> just a reminder from the other patch - these are the values used for
>> the mpll2 clock:
>> - parent rate = 2550MHz
>> - SDM_DEN = 16384
>> - SDM = 1638
>> - N2 = 5
>> = (parent rate 2550000000 * SDM_DEN 16384) / ((SDM_DEN 16384 * N2 5) +
>> 1638) = 500002394Hz
>>
>> using an SDM of 1639 gives us a 499996410Hz mpll2 clock
>> with all the dividers we get down to a RGMII clock of 24999821Hz which
>> is 179Hz off the desired 25MHz
>> in other words: the mpll2 values set by Odroid-C1's u-boot are "best",
>> so if we try to set mpll2's rate through the common clock framework we
>> should try to get the same results (else we would just make the result
>> worse)
>
> Indeed, we should be able to do a lot better than 500kHz error. We should get to
> the bottom of this. When testing on the S905 with audio, I got very values so I
> did not question the mpll driver too much. Maybe there is something there.
>
> Purely for debugging, from the ethernet driver, Could you try the following:
> - Remove CLK_SET_RATE_PARENT from div250 to break the propagation there
> - call set_rate on the mux with 500Mhz (we'll see far off we really are compared
> to u-boot values and we will be able to compare)
> - call set_rate on div25 as usual to get your ethernet running.
>
>>
>> > Cheers
>> >
>> > > which is why we need to use CLK_DIVIDER_ROUND_CLOSEST for the m250_div
>> > > clock. The clk-divider driver however ignores the
>> > > CLK_DIVIDER_ROUND_CLOSEST flag if CLK_SET_RATE_PARENT is set (because
>> > > it simply tries to set the best possible clock rate for the parent,
>> > > which does nothing in our case since the parent is a mux which doesn't
>> > > allow rate changes as explained above).
>> > >
>> > > This fixes setting the RGMII clock on Meson8 SoCs which ended up with a
>> > > ~20MHz clock instead of the expected ~25MHz.
>> > > The dwmac-meson8b driver requests a 25MHz clock rate for the m25_div
>> > > (which only supports "divide by 5" and "divide by 10") clock which is
>> > > derived from the m250_div clock. Due to clk-divider ignoring the
>> > > CLK_DIVIDER_ROUND_CLOSEST flag the resulting m250_div clock was set to
>> > > ~100MHz (divider = 5) and the m25_div clock was set to ~20MHz (divider =
>> > > 5) by the common clock framework (as this value is closest to 25MHz if
>> > > we would not have set CLK_DIVIDER_ROUND_CLOSEST). What we actually need
>> > > however is a rate of ~250MHz on the m250_div clock (divider = 2) and
>> > > ~25MHz on the m25_div clock (divider = 10) - these are also the values
>> > > chosen by the out-of-tree vendor driver.
>> > > With this we end up with a RGMII clock of 25000120Hz (which is as close
>> > > to 25MHz we can get with an input clock of 500002394Hz).
>> > >
>> > > SoCs from the Meson GX series are not affected by this change because
>> > > the input clock is FCLK_DIV2 whose rate cannot be changed. Additionally
>> > > the GX SoCs don't need to use the "closest" divider since the parent
>> > > clock is a multiple of 250MHz.
>> > >
>> > > Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
>> > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> > > ---
>> > >  drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> > > index c71966332387..26f41c117d63 100644
>> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> > > @@ -135,7 +135,7 @@ static int meson8b_init_clk(struct meson8b_dwmac *dwmac)
>> > >       snprintf(clk_name, sizeof(clk_name), "%s#m250_div", dev_name(dev));
>> > >       init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
>> > >       init.ops = &clk_divider_ops;
>> > > -     init.flags = CLK_SET_RATE_PARENT;
>> > > +     init.flags = 0;
>> > >       clk_div_parents[0] = __clk_get_name(dwmac->m250_mux_clk);
>> > >       init.parent_names = clk_div_parents;
>> > >       init.num_parents = ARRAY_SIZE(clk_div_parents);
>>
>> Regards
>> Martin
>


Regards
Martin

[0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005856.html

Re: [RFT net-next 2/2] net: stmmac: dwmac-meson8b: don't try to change m250_div parent's rate

[Jerome Brunet]

On Sat, 2017-12-23 at 22:49 +0100, Martin Blumenstingl wrote:
> while calculating this with a target frequency of 500MHz manually
> again I saw that there's a remainder of 10Mhz after the initial
> division.
> remainder * SDM_DEN = 163840000000 - this value overflows 32-bit,
> things will go downhill from here... :)
> long story short: here's a patch for that issue: [0]

Thanks for the fix !

> 
> the results with "CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT" on the mux:

Actually, I don't think you need the CLK_SET_RATE_NO_REPARENT

>     fixed_pll                             3            3  2550000000
>        0 0
>       mpll2                              1            1   124999851
>       0 0
>          c9410000.ethernet#m250_sel           1            1
> 124999851          0 0
>             c9410000.ethernet#m250_div           1            1
> 124999851          0 0
>                c9410000.ethernet#m25_div           1            1
> 24999971          0 0
> 
> this is even closer to 25MHz than the values in the vendor driver
> (120Hz vs 29Hz)
> I'll re-spin this series, then Emiliano "just" has to confirm that
> it's also working for him (despite the dividers in the dwmac-meson8b
> driver are set up different compared to the vendor driver)

if the rate of the parent clock of mux may change, I think following part should
be changed as well

	case PHY_INTERFACE_MODE_RMII:
		/* Use the rate of the mux clock for the internal RMII PHY */
		clk_rate = clk_get_rate(dwmac->m250_mux_clk);

I think it is a bit weak anyway, as it depends on the initial settings. Do you
remember why you wrote this way ?

> 
> > > - according to the datasheet the allowed range of the mpll2 clock is
> > > 250..637MHz - 127488329Hz is what we get from the common clock
> > > framework
> > 
> > Yes, I've seen that but we are able compute out of that range and, so far, the
> > actual rate of clock seems coherent with the calculation. At least, when I
> > tested with the audio, it was the case.
> 
> I'm curious: ...on a GX SoCs probably?

Indeed, you got me !

Re: [RFT net-next 2/2] net: stmmac: dwmac-meson8b: don't try to change m250_div parent's rate

[Martin Blumenstingl]

On Sat, Dec 23, 2017 at 11:41 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sat, 2017-12-23 at 22:49 +0100, Martin Blumenstingl wrote:
>> while calculating this with a target frequency of 500MHz manually
>> again I saw that there's a remainder of 10Mhz after the initial
>> division.
>> remainder * SDM_DEN = 163840000000 - this value overflows 32-bit,
>> things will go downhill from here... :)
>> long story short: here's a patch for that issue: [0]
>
> Thanks for the fix !
>
>>
>> the results with "CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT" on the mux:
>
> Actually, I don't think you need the CLK_SET_RATE_NO_REPARENT
as I have to re-test this on the GX SoCs anyways I'll give it a go
without CLK_SET_RATE_NO_REPARENT. stay tuned...

>>     fixed_pll                             3            3  2550000000
>>        0 0
>>       mpll2                              1            1   124999851
>>       0 0
>>          c9410000.ethernet#m250_sel           1            1
>> 124999851          0 0
>>             c9410000.ethernet#m250_div           1            1
>> 124999851          0 0
>>                c9410000.ethernet#m25_div           1            1
>> 24999971          0 0
>>
>> this is even closer to 25MHz than the values in the vendor driver
>> (120Hz vs 29Hz)
>> I'll re-spin this series, then Emiliano "just" has to confirm that
>> it's also working for him (despite the dividers in the dwmac-meson8b
>> driver are set up different compared to the vendor driver)
>
> if the rate of the parent clock of mux may change, I think following part should
> be changed as well
>
>         case PHY_INTERFACE_MODE_RMII:
>                 /* Use the rate of the mux clock for the internal RMII PHY */
>                 clk_rate = clk_get_rate(dwmac->m250_mux_clk);
>
> I think it is a bit weak anyway, as it depends on the initial settings. Do you
> remember why you wrote this way ?
actually this is confusing and a no-op: I'll have to fix it with a
re-spin of this series anyways.

while trying to make sense why Ethernet wouldn't work on Emiliano's
Odroid-C1 I stumbled across this header: [0]
it explains why it doesn't matter which values are set for the
mux/dividers in RMII mode: the clocks are used to generate the "RGMII
clock"
the currrent code is a no-op since the common clock framework sets all
dividers in PRG_ETH0 to 0 - which results in a register value of
0x1800 (which used by the vendor driver for RMII)

I will add a patch which ignores the clocks in RMII mode: this is also
needed because we don't want to configure MPLL2 on Meson8b in RMII
mode

>>
>> > > - according to the datasheet the allowed range of the mpll2 clock is
>> > > 250..637MHz - 127488329Hz is what we get from the common clock
>> > > framework
>> >
>> > Yes, I've seen that but we are able compute out of that range and, so far, the
>> > actual rate of clock seems coherent with the calculation. At least, when I
>> > tested with the audio, it was the case.
>>
>> I'm curious: ...on a GX SoCs probably?
>
> Indeed, you got me !
good to know - I'll try to keep the 32-bit overflow issues in mind
when things don't work on Meson8/Meson8b even if they do work on the
GX SoCs


Regards
Martin

[0] https://github.com/endlessm/u-boot-meson/blob/345ee7eb02903f5ecb1173ffb2cd36666e44ebed/arch/arm/include/asm/arch-m8b/aml_eth_reg.h#L489