From: Dmitry Rokosov <ddrokosov@sberdevices.ru>
To: <neil.armstrong@linaro.org>, <jbrunet@baylibre.com>
Cc: <mturquette@baylibre.com>, <sboyd@kernel.org>,
<robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
<khilman@baylibre.com>, <martin.blumenstingl@googlemail.com>,
<jian.hu@amlogic.com>, <kernel@sberdevices.ru>,
<rockosov@gmail.com>, <linux-amlogic@lists.infradead.org>,
<linux-clk@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v11 3/5] dt-bindings: clock: meson: add A1 PLL and Peripherals clkcs bindings
Date: Mon, 27 Mar 2023 16:22:09 +0300 [thread overview]
Message-ID: <20230327132209.c64ye4ou22cq7wm6@CAB-WSD-L081021> (raw)
In-Reply-To: <20230327131927.k7uswfn6i3jqjrzv@CAB-WSD-L081021>
Don't know why, but my client deleted Jerome from To:. Sorry, added him
back.
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:
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
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
next prev parent reply other threads:[~2023-03-27 13:22 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-21 19:30 [PATCH v11 0/5] add Amlogic A1 clock controller drivers Dmitry Rokosov
2023-03-21 19:30 ` [PATCH v11 1/5] clk: meson: make pll rst bit as optional Dmitry Rokosov
2023-03-21 19:30 ` [PATCH v11 2/5] clk: meson: introduce new pll power-on sequence for A1 SoC family Dmitry Rokosov
2023-03-21 19:30 ` [PATCH v11 3/5] dt-bindings: clock: meson: add A1 PLL and Peripherals clkcs bindings Dmitry Rokosov
2023-03-27 9:51 ` Jerome Brunet
2023-03-27 10:51 ` Dmitry Rokosov
2023-03-27 11:39 ` Jerome Brunet
2023-03-27 12:03 ` neil.armstrong
2023-03-27 13:19 ` Dmitry Rokosov
2023-03-27 13:22 ` Dmitry Rokosov [this message]
2023-03-27 13:55 ` neil.armstrong
2023-03-27 14:02 ` Dmitry Rokosov
2023-03-27 13:41 ` Krzysztof Kozlowski
2023-03-27 13:46 ` Dmitry Rokosov
2023-03-27 13:59 ` Jerome Brunet
2023-03-27 15:28 ` Rob Herring
2023-03-21 19:30 ` [PATCH v11 4/5] clk: meson: a1: add Amlogic A1 PLL clock controller driver Dmitry Rokosov
2023-03-21 19:30 ` [PATCH v11 5/5] clk: meson: a1: add Amlogic A1 Peripherals " Dmitry Rokosov
2023-03-27 9:58 ` Jerome Brunet
2023-03-27 13:27 ` Dmitry Rokosov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230327132209.c64ye4ou22cq7wm6@CAB-WSD-L081021 \
--to=ddrokosov@sberdevices.ru \
--cc=devicetree@vger.kernel.org \
--cc=jbrunet@baylibre.com \
--cc=jian.hu@amlogic.com \
--cc=kernel@sberdevices.ru \
--cc=khilman@baylibre.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-amlogic@lists.infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=martin.blumenstingl@googlemail.com \
--cc=mturquette@baylibre.com \
--cc=neil.armstrong@linaro.org \
--cc=robh+dt@kernel.org \
--cc=rockosov@gmail.com \
--cc=sboyd@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).