On Mon, Mar 27, 2023 at 02:03:25PM +0200, neil.armstrong@xxxxxxxxxx wrote: > On 27/03/2023 13:39, Jerome Brunet wrote: > > > > On Mon 27 Mar 2023 at 13:51, Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxx> > > > > > Signed-off-by: Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> > > > > > --- > > > > > .../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@xxxxxxxxxx/ > > > > > > 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: 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. [...] -- Thank you, Dmitry
