by Jian Hu

[permalink] [raw]

Subject: Re: [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings

On 2019/12/12 17:57, Jerome Brunet wrote:
>
> On Fri 06 Dec 2019 at 08:40, Jian Hu <[email protected]> wrote:
>
>> Add the documentation to support Amlogic A1 PLL clock driver,
>> and add A1 PLL clock controller bindings.
>>
>> Signed-off-by: Jian Hu <[email protected]>
>> ---
>> .../bindings/clock/amlogic,a1-pll-clkc.yaml | 59 +++++++++++++++++++
>> include/dt-bindings/clock/a1-pll-clkc.h | 16 +++++
>> 2 files changed, 75 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>> create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>> new file mode 100644
>> index 000000000000..7feeef5abf1b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>> @@ -0,0 +1,59 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>
> Rob commented on the above in v1 and it remains unaddressedOK, I will fix it in the next version.
>
>> +/*
>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>> + */
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: Amlogic Meson A/C serials PLL Clock Control Unit Device Tree Bindings
>> +
>> +maintainers:
>> + - Neil Armstrong <[email protected]>
>> + - Jerome Brunet <[email protected]>
>> + - Jian Hu <[email protected]>
>> +
>> +properties:
>> + compatible:
>> + - enum:
>> + - amlogic,a1-pll-clkc
>> + "#clock-cells":
>> + const: 1
>> +
>> + reg:
>> + maxItems: 1
>> +
>> +clocks:
>> + minItems: 2
>> + maxItems: 2
>> + items:
>> + - description: Input xtal_fixpll
>> + - description: Input xtal_hifipll
>> +
>> +clock-names:
>> + minItems: 2
>> + maxItems: 2
>> + items:
>> + - const: xtal_fixpll
>> + - const: xtal_hifipll
>> +
>> +required:
>> + - compatible
>> + - "#clock-cells"
>> + - reg
>> + - clocks
>> + - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + clkc_pll: pll-clock-controller@7c80 {
>> + compatible = "amlogic,a1-pll-clkc";
>> + reg = <0 0x7c80 0 0x18c>;
>> + #clock-cells = <1>;
>> + clocks = <&clkc_periphs CLKID_XTAL_FIXPLL>,
>> + <&clkc_periphs CLKID_XTAL_HIFIPLL>;
>> + clock-names = "xtal_fixpll", "xtal_hifipll";
>> + };
>> diff --git a/include/dt-bindings/clock/a1-pll-clkc.h b/include/dt-bindings/clock/a1-pll-clkc.h
>> new file mode 100644
>> index 000000000000..58eae237e503
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/a1-pll-clkc.h
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef __A1_PLL_CLKC_H
>> +#define __A1_PLL_CLKC_H
>> +
>> +#define CLKID_FIXED_PLL 1
>> +#define CLKID_FCLK_DIV2 6
>> +#define CLKID_FCLK_DIV3 7
>> +#define CLKID_FCLK_DIV5 8
>> +#define CLKID_FCLK_DIV7 9
>> +#define CLKID_HIFI_PLL 10
>> +
>> +#endif /* __A1_PLL_CLKC_H */
>
> .
>

2019-12-13 10:40:59

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings

Hi,

On Fri, Dec 06, 2019 at 03:40:47PM +0800, Jian Hu wrote:
> Add the documentation to support Amlogic A1 PLL clock driver,
> and add A1 PLL clock controller bindings.
>
> Signed-off-by: Jian Hu <[email protected]>
> ---
> .../bindings/clock/amlogic,a1-pll-clkc.yaml | 59 +++++++++++++++++++
> include/dt-bindings/clock/a1-pll-clkc.h | 16 +++++
> 2 files changed, 75 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
>
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> new file mode 100644
> index 000000000000..7feeef5abf1b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> + */
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Amlogic Meson A/C serials PLL Clock Control Unit Device Tree Bindings
> +
> +maintainers:
> + - Neil Armstrong <[email protected]>
> + - Jerome Brunet <[email protected]>
> + - Jian Hu <[email protected]>
> +
> +properties:
> + compatible:
> + - enum:
> + - amlogic,a1-pll-clkc

I'm not sure this works, compatible shouldn't contain a list.

You can write this like:
compatible:
const: amlogic,a1-pll-clkc

> + "#clock-cells":
> + const: 1
> +
> + reg:
> + maxItems: 1
> +
> +clocks:
> + minItems: 2
> + maxItems: 2

This is redundant, it will be added automatically by the tools ...

> + items:
> + - description: Input xtal_fixpll
> + - description: Input xtal_hifipll

... When you have a list of items :)

> +
> +clock-names:
> + minItems: 2
> + maxItems: 2
> + items:
> + - const: xtal_fixpll
> + - const: xtal_hifipll

Same story here

Maxime

Attachments:

(No filename) (2.08 kB)
signature.asc (235.00 B)
Download all attachments

2019-12-20 07:04:47

by Jian Hu

[permalink] [raw]

Subject: Re: [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops

Hi jerome

On 2019/12/18 16:37, Jian Hu wrote:
>
>
> On 2019/12/17 17:29, Jerome Brunet wrote:
>>
>> On Tue 17 Dec 2019 at 09:41, Jian Hu <[email protected]> wrote:
>>
>>> On 2019/12/12 18:16, Jerome Brunet wrote:
>>>>
>>>> On Fri 06 Dec 2019 at 08:40, Jian Hu <[email protected]> wrote:
>>>>
>>>>> The A1 PLL design is different with previous SoCs. The PLL
>>>>> internal analog modules Power-on sequence is different
>>>>> with previous, and thus requires a strict register sequence to
>>>>> enable the PLL.
>>>>>
>>>>> Signed-off-by: Jian Hu <[email protected]>
>>>>> ---
>>>>>    drivers/clk/meson/clk-pll.c | 21 +++++++++++++++++++++
>>>>>    drivers/clk/meson/clk-pll.h | 1 +
>>>>>    drivers/clk/meson/parm.h    | 1 +
>>>>>    3 files changed, 23 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>>>>> index ddb1e5634739..4aff31a51589 100644
>>>>> --- a/drivers/clk/meson/clk-pll.c
>>>>> +++ b/drivers/clk/meson/clk-pll.c
>>>>> @@ -318,6 +318,23 @@ static int meson_clk_pll_enable(struct clk_hw
>>>>> *hw)
>>>>>        struct clk_regmap *clk = to_clk_regmap(hw);
>>>>>        struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>>>>    +    /*
>>>>> +     * The A1 design is different with previous SoCs.The PLL
>>>>> +     * internal analog modules Power-on sequence is different with
>>>>> +     * previous, and thus requires a strict register sequence to
>>>>> +     * enable the PLL.
>>>>
>>>> The code does something more, not completly different. This comment is
>>>> not aligned with what the code does
>>> ok, I will correct the comment.
>>>>
>>>>> +     */
>>>>> +    if (MESON_PARM_APPLICABLE(&pll->current_en)) {
>>>>> +        /* Enable the pll */
>>>>> +        meson_parm_write(clk->map, &pll->en, 1);
>>>>> +        udelay(10);
>>>>> +        /* Enable the pll self-adaption module current */
>>>>> +        meson_parm_write(clk->map, &pll->current_en, 1);
>>>>> +        udelay(40);
>>>>> +        meson_parm_write(clk->map, &pll->rst, 1);
>>>>> +        meson_parm_write(clk->map, &pll->rst, 0);
>>>>
>>>> Here you enable the PLL and self adaptation module then reset the PLL.
>>>> However:
>>>> #1 when you enter this function, the PLL should already by in reset
>>>> and disabled
>>>> #2 the code after that will reset the PLL again
>>> For A1 PLLs, There is no reset bit, It will not reset the PLL.
>>> And in V2, you mentioned PARM 'rst' can be used for one toggling, And
>>> 'rst'
>>> is used for BIT(6) in CTRL2.
>>>
>>
>> oh my ! What is it then ? Why do you need to toggle it ? What does is
>> do ?
>>
> The PLL enable flow:
>      step1: enable the PLL
>      step2: enable the self adaptation module
>      step3: reset the lock detect module, let the lock detect module
>            work，And then the PLL will work.
>
> Toggle the bit 6 in CTRL2 can reset the lock detect module.

#1 I intend to introduce a new PARM 'l_detect', and the
meson_clk_pll_enable is blow. Please help to review it.

static int meson_clk_pll_enable(struct clk_hw *hw)
{
struct clk_regmap *clk = to_clk_regmap(hw);
struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);

/* do nothing if the PLL is already enabled */
if (clk_hw_is_enabled(hw))
return 0;
/*
* Compared with the previous SoCs, self-adaption module current
* is newly added, keep the new power-on sequence to enable the
* PLL.
*/
if (MESON_PARM_APPLICABLE(&pll->current_en)) {
/* Enable the pll */
meson_parm_write(clk->map, &pll->en, 1);
udelay(10);
/* Enable the pll self-adaption module current */
meson_parm_write(clk->map, &pll->current_en, 1);
udelay(40);
/* Enable lock detect module */
meson_parm_write(clk->map, &pll->l_detect, 1);
meson_parm_write(clk->map, &pll->l_detect, 0);
goto out;
}

/* Make sure the pll is in reset */
meson_parm_write(clk->map, &pll->rst, 1);

/* Enable the pll */
meson_parm_write(clk->map, &pll->en, 1);

/* Take the pll out reset */
meson_parm_write(clk->map, &pll->rst, 0);

out:
if (meson_clk_pll_wait_lock(hw))
return -EIO;

return 0;
}

#2 Add check for PARM 'rst' when
writing to it, the same with frac.

if (MESON_PARM_APPLICABLE(&pll->rst))
meson_parm_write(clk->map, &pll->rst, 1);

And I have tested it for it, The periphs and hifi pll works well.

Or any good idea about it?

>>> Quote V2 the HIFI PLL init_regs definition：
>>>
>>>
>>> +static const struct reg_sequence a1_hifi_init_regs[] = {
>>> +    { .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
>>> +    { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
>>> +    { .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x100a1100 },
>>> +    { .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
>>> +    { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },
>>> +    { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x11f18440, .delay_us =
>>> 10 },
>>> +    { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x15f18440, .delay_us =
>>> 40 },
>>> +    { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001140 },
>>> +    { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
>>> +};
>>>
>>> So maybe another new PARM should be defined to avoid the ambiguity.
>>> What do you think about it?
>>
>> This is not the point of my comment Jian !
>>
>> I'm assuming here that you have tested your v4 before sending and that
>> it work (hopefully)
>>
> Yes, it works wells. I have tested the drivers before sending every
> patchset version.
>> The fact is that with this code, when disabled the bit behind rst
>> (whatever it is) is set. So when you get to enable the bit is already
>> set.
>> The code you sent does the same as the snip I gave you in the reply.
>>
>> Now, if your PLL is THAT different, maybe it would be best if you could
>> clearly explain how it works, what bit should be set and why. Then we
>> will be able to figure out how the driver has to be restructured.
>>
> the same as 'The PLL enable flow' above
>>>
>>>>
>>>> So if what you submited works, inserting the following should
>>>> accomplish
>>>> the same thing:
>>>>
>>>> ---8<---
>>>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>>>> index 489092dde3a6..9b38df0a7682 100644
>>>> --- a/drivers/clk/meson/clk-pll.c
>>>> +++ b/drivers/clk/meson/clk-pll.c
>>>> @@ -330,6 +330,13 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>>>           /* Enable the pll */
>>>>           meson_parm_write(clk->map, &pll->en, 1);
>>>>
>>>> +       if (MESON_PARM_APPLICABLE(&pll->current_en)) {
>>>> +               udelay(10);
>>>> +               /* Enable the pll self-adaption module current */
>>>> +               meson_parm_write(clk->map, &pll->current_en, 1);
>>>> +               udelay(40);
>>>> +       }
>>>> +
>>>>           /* Take the pll out reset */
>>>>           meson_parm_write(clk->map, &pll->rst, 0);
>>>> --->8---
>>>>
>>>>
>>>>
>>>>
>>>>> +    }
>>>>> +
>>>>>        /* do nothing if the PLL is already enabled */
>>>>>        if (clk_hw_is_enabled(hw))
>>>>>            return 0;
>>>>
>>>> In any case, nothing should be done on the clock before this check
>>>> otherwise you might just break the clock
>>>>
>>> OK, I will put the enabled check ahead.
>>>>> @@ -347,6 +364,10 @@ static void meson_clk_pll_disable(struct
>>>>> clk_hw *hw)
>>>>>          /* Disable the pll */
>>>>>        meson_parm_write(clk->map, &pll->en, 0);
>>>>> +
>>>>> +    /* Disable PLL internal self-adaption module current */
>>>>> +    if (MESON_PARM_APPLICABLE(&pll->current_en))
>>>>> +        meson_parm_write(clk->map, &pll->current_en, 0);
>>>>>    }
>>>>>      static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned
>>>>> long
>>>>> rate,
>>>>> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
>>>>> index 367efd0f6410..30f039242a65 100644
>>>>> --- a/drivers/clk/meson/clk-pll.h
>>>>> +++ b/drivers/clk/meson/clk-pll.h
>>>>> @@ -36,6 +36,7 @@ struct meson_clk_pll_data {
>>>>>        struct parm frac;
>>>>>        struct parm l;
>>>>>        struct parm rst;
>>>>> +    struct parm current_en;
>>>>>        const struct reg_sequence *init_regs;
>>>>>        unsigned int init_count;
>>>>>        const struct pll_params_table *table;
>>>>> diff --git a/drivers/clk/meson/parm.h b/drivers/clk/meson/parm.h
>>>>> index 3c9ef1b505ce..c53fb26577e3 100644
>>>>> --- a/drivers/clk/meson/parm.h
>>>>> +++ b/drivers/clk/meson/parm.h
>>>>> @@ -20,6 +20,7 @@
>>>>>        (((reg) & CLRPMASK(width, shift)) | ((val) << (shift)))
>>>>>      #define MESON_PARM_APPLICABLE(p)        (!!((p)->width))
>>>>> +#define MESON_PARM_CURRENT(p)            (!!((p)->width))
>>>>
>>>> Why do we need that ?
>>> OK, I will remove it ,and use 'MESON_PARM_APPLICABLE' instead
>>>>
>>>>>      struct parm {
>>>>>        u16    reg_off;
>>>>
>>>> .
>>>>
>>
>> .
>>