Re: [PATCH v11 3/5] dt-bindings: clock: meson: add A1 PLL and Peripherals clkcs bindings

[neil.armstrong@linaro.org]

On 27/03/2023 15:19, Dmitry Rokosov wrote:
> On Mon, Mar 27, 2023 at 02:03:25PM +0200, neil.armstrong@linaro.org wrote:
>> On 27/03/2023 13:39, Jerome Brunet wrote:
>>>
>>> On Mon 27 Mar 2023 at 13:51, Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote:
>>>
>>>> On Mon, Mar 27, 2023 at 11:51:21AM +0200, Jerome Brunet wrote:
>>>>>
>>>>> On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote:
>>>>>
>>>>>> Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
>>>>>> clock drivers.
>>>>>> Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
>>>>>> bindings and include them to MAINTAINERS.
>>>>>>
>>>>>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>>>>>> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
>>>>>> ---
>>>>>>    .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
>>>>>>    .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
>>>>>>    MAINTAINERS                                   |   1 +
>>>>>>    include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
>>>>>>    .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
>>>>>>    5 files changed, 267 insertions(+)
>>>>>>    create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>>>>>>    create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>>>
>>>>> There is two drivers (and 2 independent patches). There should be 2
>>>>> bindings patches as well.
>>>>>
>>>>
>>>> Before, in previous versions I had two versions, but it wasn't bisectable
>>>> approach.
>>>
>>> You are confusing bisectable and Rob's robot. Splitting patches is more
>>> that likely to help bisect (and patches backport) - not the other way around.
>>>
>>>> a1-clkc schema depends on a1-pll-clkc headers and vice versa.
>>>> It means dt schemas checkers will show us failure if we split them into two
>>>> patchsets.
>>>
>>> Only because you are patches are not upstream yet ...
>>>
>>>> I know, that we can use raw digits instead of CLKID names, but IMO it doesn't
>>>> look like production schema and it requires one more patchset above the
>>>> series with proper CLKID definitons usage and proper header including.
>>>>
>>>> BTW, there is an example of Rob's test bot failure found in the previous
>>>> v10 patch series due to chicken or the egg problem.
>>>> https://lore.kernel.org/all/167769997208.7087.5344356236212731922.robh@kernel.org/
>>>>
>>>> Please advise what's the best practice to resolve that..
>>>
>>> Don't use the header in your example would solve the problem and
>>> still be correct DT wise.
>>>
>>> The examples are just examples, they are not required to actually
>>> matches a real HW, as far as I know.
>>
>> Exact, you can use fake lables instead of defined:
>>
>> <&clkc_pll CLKID_FCLK_DIV2>,
>>
>> =>
>> remove "#include <dt-bindings/clock/amlogic,a1-pll-clkc.h>"
>>
>> <&clkc_pll_fclk_div2>,
>>
>> is perfectly ok and will permit have 2 separate patches.
>>
>> The dependency is only if you have a common yaml file for
>> both bindings files, but this is not the case here.
> 
> Simple removal of "#include <dt-bindings/clock/amlogic,a1-pll-clkc.h>"
> header doesn't work, dt_binding_check make rule is failed:

I never wrote you to only remove the include, adding fake labels phandles was the logical next step.

Neil

> 
> Error: Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts:28.37-38 syntax error
> FATAL ERROR: Unable to parse input tree
> 
> It happens, because 'dt_binding_check' generates simple dts example and
> tries to compile it:
> 
> cat Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts
> ===
> 
> /dts-v1/;
> /plugin/; // silence any missing phandle references
> 
> 
> /{
>      compatible = "foo";
>      model = "foo";
>      #address-cells = <1>;
>      #size-cells = <1>;
> 
> 
> 
>      example-0 {
>          #address-cells = <1>;
>          #size-cells = <1>;
> 
>          
> 
>          apb {
>              #address-cells = <2>;
>              #size-cells = <2>;
>          
>              clock-controller@800 {
>                  compatible = "amlogic,a1-clkc";
>                  reg = <0 0x800 0 0x104>;
>                  #clock-cells = <1>;
>                  clocks = <&clkc_pll CLKID_FCLK_DIV2>,
>                           <&clkc_pll CLKID_FCLK_DIV3>,
>                           <&clkc_pll CLKID_FCLK_DIV5>,
>                           <&clkc_pll CLKID_FCLK_DIV7>,
>                           <&clkc_pll CLKID_HIFI_PLL>,
>                           <&xtal>;
>                  clock-names = "fclk_div2", "fclk_div3",
>                                "fclk_div5", "fclk_div7",
>                                "hifi_pll", "xtal";
>              };
>          };
> 
>      };
> };
> ===
> 
> As you can see, header is required.
> 
> But looks like, dt binding checker is happy with the fake references hack :)
> Below there is generated example dts:
> 
> cat Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts
> ===
> 
> /dts-v1/;
> /plugin/; // silence any missing phandle references
> 
> 
> /{
>      compatible = "foo";
>      model = "foo";
>      #address-cells = <1>;
>      #size-cells = <1>;
> 
> 
> 
>      example-0 {
>          #address-cells = <1>;
>          #size-cells = <1>;
> 
>          
> 
>          apb {
>              #address-cells = <2>;
>              #size-cells = <2>;
>          
>              clock-controller@800 {
>                  compatible = "amlogic,a1-clkc";
>                  reg = <0 0x800 0 0x104>;
>                  #clock-cells = <1>;
>                  clocks = <&clkc_pll_fclk_div2>,
>                           <&clkc_pll_fclk_div3>,
>                           <&clkc_pll_fclk_div5>,
>                           <&clkc_pll_fclk_div7>,
>                           <&clkc_pll_hifi_pll>,
>                           <&xtal>;
>                  clock-names = "fclk_div2", "fclk_div3",
>                                "fclk_div5", "fclk_div7",
>                                "hifi_pll", "xtal";
>              };
>          };
> 
>      };
> };
> ===
> 
> Yep, we are able to cheat dt checkers, but we don't help dt developers
> with such example.
> May be, it's better to prepare two patches in such hierarchy:
> 
> 1) A1 PLL clkc bindings with fake references without clkc headers
> 2) A1 clkc bindings with real CLKID bindings + A1 PLL clkc bindings fix
> with real CLKID A1 clkc bindings + header.
> 
> The such approach resolves DT checkers failures and split DT bindings
> into two patchsets.
> 
> [...]
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic