Subject: [PATCH 2/3] ARM i.MX6: Fix ethernet PLL clocks

From: [email-masked] (Dirk Behme)
Date: Tue, 8 Apr 2014 13:44:06 +0200

Hi Sascha and Shawn,

On 22.11.2012 13:35, Sascha Hauer wrote:
> On Thu, Nov 22, 2012 at 07:32:28PM +0800, Shawn Guo wrote:
>> On Thu, Nov 22, 2012 at 10:59:17AM +0100, Sascha Hauer wrote:
>>> In current code the ethernet PLL is not handled correctly. The PLL runs at 500MHz
>>> and has different outputs. Only the enet reference clock is implemented. This
>>> patch changes the PLL so that it outputs 500MHz and adds the additional outputs
>>> as dividers. This now matches the datasheet which says:
>>>
>>>> This PLL synthesizes a low jitter clock from 24 MHz reference clock.
>>>> The PLL outputs a 500 MHz clock. The reference clocks generated by this PLL are:
>>>>   ? Ref_PCIe = 125 MHz
>>>>   ? Ref_SATA = 100 MHz
>>>>   ? Ref_ethernet, which is configurable based on the PLL_ENET[1:0] register field.
>>>
>>> Signed-off-by: Sascha Hauer <[email-masked]>
>>> ---
>>>   .../devicetree/bindings/clock/imx6q-clock.txt      |    5 ++
>>>   arch/arm/mach-imx/clk-imx6q.c                      |   21 ++++++-
>>>   arch/arm/mach-imx/clk-pllv3.c                      |   63 +-------------------
>>>   3 files changed, 25 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/imx6q-clock.txt b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
>>> index bb71d4f..d77b4e6 100644
>>> --- a/Documentation/devicetree/bindings/clock/imx6q-clock.txt
>>> +++ b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
>>> @@ -198,6 +198,11 @@ clocks and IDs.
>>>      usbphy2                 183
>>>      ldb_di0_div_3_5         184
>>>      ldb_di1_div_3_5         185
>>> +   sata_ref                186
>>> +   sata_ref_100m           187
>>> +   pcie_ref                188
>>> +   pcie_ref_125m           189
>>> +   enet_ref                190
>>>
>>>   Examples:
>>>
>>> diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
>>> index 4066365..622a299 100644
>>> --- a/arch/arm/mach-imx/clk-imx6q.c
>>> +++ b/arch/arm/mach-imx/clk-imx6q.c
>>> @@ -153,6 +153,7 @@ enum mx6q_clks {
>>>      usdhc4, vdo_axi, vpu_axi, cko1, pll1_sys, pll2_bus, pll3_usb_otg,
>>>      pll4_audio, pll5_video, pll8_mlb, pll7_usb_host, pll6_enet, ssi1_ipg,
>>>      ssi2_ipg, ssi3_ipg, rom, usbphy1, usbphy2, ldb_di0_div_3_5, ldb_di1_div_3_5,
>>> +   sata_ref, sata_ref_100m, pcie_ref, pcie_ref_125m, enet_ref,
>>>      clk_max
>>>   };
>>>
>>> @@ -163,6 +164,13 @@ static enum mx6q_clks const clks_init_on[] __initconst = {
>>>      mmdc_ch0_axi, rom,
>>>   };
>>>
>>> +static struct clk_div_table clk_enet_ref_table[] = {
>>> +   { .val = 0, .div = 20, },
>>> +   { .val = 1, .div = 10, },
>>> +   { .val = 2, .div = 5, },
>>> +   { .val = 3, .div = 4, },
>>> +};
>>> +
>>>   int __init mx6q_clocks_init(void)
>>>   {
>>>      struct device_node *np;
>>> @@ -195,13 +203,22 @@ int __init mx6q_clocks_init(void)
>>>      clk[pll3_usb_otg]  = imx_clk_pllv3(IMX_PLLV3_USB,       "pll3_usb_otg", "osc", base + 0x10, 0x2000,   0x3);
>>>      clk[pll4_audio]    = imx_clk_pllv3(IMX_PLLV3_AV,        "pll4_audio",   "osc", base + 0x70, 0x2000,   0x7f);
>>>      clk[pll5_video]    = imx_clk_pllv3(IMX_PLLV3_AV,        "pll5_video",   "osc", base + 0xa0, 0x2000,   0x7f);
>>> -   clk[pll6_enet]     = imx_clk_pllv3(IMX_PLLV3_ENET,      "pll6_enet",    "osc", base + 0xe0, 0x182000, 0x3);
>>> +   clk[pll6_enet]     = imx_clk_pllv3(IMX_PLLV3_ENET,      "pll6_enet",    "osc", base + 0xe0, 0x2000,   0x3);
>>>      clk[pll7_usb_host] = imx_clk_pllv3(IMX_PLLV3_USB,       "pll7_usb_host","osc", base + 0x20, 0x2000,   0x3);
>>> -   clk[pll8_mlb]      = imx_clk_pllv3(IMX_PLLV3_MLB,       "pll8_mlb",     "osc", base + 0xd0, 0x2000,   0x0);
>>
>> It gets removed by accident?
>
> Yes, I screwed it up while rebasing. Find an updated version attached.
>
>>
>> Otherwise, for the series:
>>
>> Acked-by: Shawn Guo <[email-masked]>
>
> Thanks.
>
> Sascha
>
> 8<----------------------------------------------------
>
>  From 4de7de851c6666c4dca4f36acbc7f9b802ea7d7f Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <[email-masked]>
> Date: Wed, 21 Nov 2012 14:42:31 +0100
> Subject: [PATCH 2/3] ARM i.MX6: Fix ethernet PLL clocks
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> In current code the ethernet PLL is not handled correctly. The PLL runs at 500MHz
> and has different outputs. Only the enet reference clock is implemented. This
> patch changes the PLL so that it outputs 500MHz and adds the additional outputs
> as dividers. This now matches the datasheet which says:
>
>> This PLL synthesizes a low jitter clock from 24 MHz reference clock.
>> The PLL outputs a 500 MHz clock. The reference clocks generated by this PLL are:
>>   ? Ref_PCIe = 125 MHz
>>   ? Ref_SATA = 100 MHz
>>   ? Ref_ethernet, which is configurable based on the PLL_ENET[1:0] register field.
>
> Signed-off-by: Sascha Hauer <[email-masked]>
> ---
>   .../devicetree/bindings/clock/imx6q-clock.txt      |    5 ++
>   arch/arm/mach-imx/clk-imx6q.c                      |   20 ++++++-
>   arch/arm/mach-imx/clk-pllv3.c                      |   63 +-------------------
>   3 files changed, 25 insertions(+), 63 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/imx6q-clock.txt b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> index bb71d4f..d77b4e6 100644
> --- a/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> +++ b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> @@ -198,6 +198,11 @@ clocks and IDs.
>          usbphy2                 183
>          ldb_di0_div_3_5         184
>          ldb_di1_div_3_5         185
> +       sata_ref                186
> +       sata_ref_100m           187
> +       pcie_ref                188
> +       pcie_ref_125m           189
> +       enet_ref                190
>
>   Examples:
>
> diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
> index 4066365..a579a1c 100644
> --- a/arch/arm/mach-imx/clk-imx6q.c
> +++ b/arch/arm/mach-imx/clk-imx6q.c
> @@ -153,6 +153,7 @@ enum mx6q_clks {
>          usdhc4, vdo_axi, vpu_axi, cko1, pll1_sys, pll2_bus, pll3_usb_otg,
>          pll4_audio, pll5_video, pll8_mlb, pll7_usb_host, pll6_enet, ssi1_ipg,
>          ssi2_ipg, ssi3_ipg, rom, usbphy1, usbphy2, ldb_di0_div_3_5, ldb_di1_div_3_5,
> +       sata_ref, sata_ref_100m, pcie_ref, pcie_ref_125m, enet_ref,
>          clk_max
>   };
>
> @@ -163,6 +164,13 @@ static enum mx6q_clks const clks_init_on[] __initconst = {
>          mmdc_ch0_axi, rom,
>   };
>
> +static struct clk_div_table clk_enet_ref_table[] = {
> +       { .val = 0, .div = 20, },
> +       { .val = 1, .div = 10, },
> +       { .val = 2, .div = 5, },
> +       { .val = 3, .div = 4, },
> +};
> +
>   int __init mx6q_clocks_init(void)
>   {
>          struct device_node *np;
> @@ -195,13 +203,23 @@ int __init mx6q_clocks_init(void)
>          clk[pll3_usb_otg]  = imx_clk_pllv3(IMX_PLLV3_USB,       "pll3_usb_otg", "osc", base + 0x10, 0x2000,   0x3);
>          clk[pll4_audio]    = imx_clk_pllv3(IMX_PLLV3_AV,        "pll4_audio",   "osc", base + 0x70, 0x2000,   0x7f);
>          clk[pll5_video]    = imx_clk_pllv3(IMX_PLLV3_AV,        "pll5_video",   "osc", base + 0xa0, 0x2000,   0x7f);
> -       clk[pll6_enet]     = imx_clk_pllv3(IMX_PLLV3_ENET,      "pll6_enet",    "osc", base + 0xe0, 0x182000, 0x3);
> +       clk[pll6_enet]     = imx_clk_pllv3(IMX_PLLV3_ENET,      "pll6_enet",    "osc", base + 0xe0, 0x2000,   0x3);
>          clk[pll7_usb_host] = imx_clk_pllv3(IMX_PLLV3_USB,       "pll7_usb_host","osc", base + 0x20, 0x2000,   0x3);
>          clk[pll8_mlb]      = imx_clk_pllv3(IMX_PLLV3_MLB,       "pll8_mlb",     "osc", base + 0xd0, 0x2000,   0x0);
>
>          clk[usbphy1] = imx_clk_gate("usbphy1", "pll3_usb_otg", base + 0x10, 6);
>          clk[usbphy2] = imx_clk_gate("usbphy2", "pll7_usb_host", base + 0x20, 6);
>
> +       clk[sata_ref] = imx_clk_fixed_factor("sata_ref", "pll6_enet", 1, 5);
> +       clk[pcie_ref] = imx_clk_fixed_factor("pcie_ref", "pll6_enet", 1, 4);
> +
> +       clk[sata_ref_100m] = imx_clk_gate("sata_ref_100m", "sata_ref", base + 0xe0, 20);
> +       clk[pcie_ref_125m] = imx_clk_gate("pcie_ref_125m", "pcie_ref", base + 0xe0, 19);
> +
> +       clk[enet_ref] = clk_register_divider_table(NULL, "enet_ref", "pll6_enet", 0,
> +                       base + 0xe0, 0, 2, 0, clk_enet_ref_table,
> +                       &imx_ccm_lock);
> +
>          /*                                name              parent_name        reg       idx */
>          clk[pll2_pfd0_352m] = imx_clk_pfd("pll2_pfd0_352m", "pll2_bus",     base + 0x100, 0);
>          clk[pll2_pfd1_594m] = imx_clk_pfd("pll2_pfd1_594m", "pll2_bus",     base + 0x100, 1);
> diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
> index 36aac94..59e7433 100644
> --- a/arch/arm/mach-imx/clk-pllv3.c
> +++ b/arch/arm/mach-imx/clk-pllv3.c
> @@ -287,66 +287,7 @@ static const struct clk_ops clk_pllv3_av_ops = {
>   static unsigned long clk_pllv3_enet_recalc_rate(struct clk_hw *hw,
>                                                  unsigned long parent_rate)
>   {
> -       struct clk_pllv3 *pll = to_clk_pllv3(hw);
> -       u32 div = readl_relaxed(pll->base) & pll->div_mask;
> -
> -       switch (div) {
> -       case 0:
> -               return 25000000;
> -       case 1:
> -               return 50000000;
> -       case 2:
> -               return 100000000;
> -       case 3:
> -               return 125000000;
> -       }
> -
> -       return 0;
> -}
> -
> -static long clk_pllv3_enet_round_rate(struct clk_hw *hw, unsigned long rate,
> -                                     unsigned long *prate)
> -{
> -       if (rate >= 125000000)
> -               rate = 125000000;
> -       else if (rate >= 100000000)
> -               rate = 100000000;
> -       else if (rate >= 50000000)
> -               rate = 50000000;
> -       else
> -               rate = 25000000;
> -       return rate;
> -}
> -
> -static int clk_pllv3_enet_set_rate(struct clk_hw *hw, unsigned long rate,
> -               unsigned long parent_rate)
> -{
> -       struct clk_pllv3 *pll = to_clk_pllv3(hw);
> -       u32 val, div;
> -
> -       switch (rate) {
> -       case 25000000:
> -               div = 0;
> -               break;
> -       case 50000000:
> -               div = 1;
> -               break;
> -       case 100000000:
> -               div = 2;
> -               break;
> -       case 125000000:
> -               div = 3;
> -               break;
> -       default:
> -               return -EINVAL;
> -       }
> -
> -       val = readl_relaxed(pll->base);
> -       val &= ~pll->div_mask;
> -       val |= div;
> -       writel_relaxed(val, pll->base);
> -
> -       return 0;

I'm no expert on this, but it seems to be able to use 100MHz or 125Mhz 
enet clock, you additionally have to set the ENABLE_100M 
(CCM_ANALOG_PLL_ENET[20]) or ENABLE_125M (CCM_ANALOG_PLL_ENET[19]) bits. 
Which isn't done by above change, and even worse, it's not possible with 
switching from dedicated clk_pllv3_enet_set_rate() to the generic ones 
using the clk_enet_ref_table.

Therefore, I've seen people doing

static int clk_pllv3_enet_set_rate(struct clk_hw *hw, unsigned long rate,
                unsigned long parent_rate)
{
        struct clk_pllv3 *pll = to_clk_pllv3(hw);
        u32 val, div, phase_shift_flag = 0;

        switch (rate) {
        case 25000000:
                div = 0;
                break;
        case 50000000:
                div = 1;
                break;
        case 100000000:
                div = 2;
                phase_shift_flag = 0x1 << 20;
                break;
        case 125000000:
                div = 3;
                phase_shift_flag = 0x1 << 19;
                break;
        default:
                return -EINVAL;
        }

        val = readl_relaxed(pll->base);
        val &= ~pll->div_mask;
        val |= div;
        val |= phase_shift_flag;
        writel_relaxed(val, pll->base);

        return 0;
}

again (note the additional phase_shift_flag).

What do you think?

Best regards

Dirk