Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings
From: neil . armstrong
Date: Thu Dec 01 2022 - 03:36:50 EST
On 28/11/2022 14:30, Yu Tu wrote:
Hi Jerome ,
On 2022/11/28 20:33, Jerome Brunet wrote:
[ EXTERNAL EMAIL ]
On Mon 28 Nov 2022 at 15:39, Yu Tu <yu.tu@xxxxxxxxxxx> wrote:
Hi Jerome,
Thank you for your reply.
On 2022/11/25 17:23, Jerome Brunet wrote:
[ EXTERNAL EMAIL ]
On Wed 23 Nov 2022 at 14:53, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
On 23/11/2022 14:23, Neil Armstrong wrote:
Hi,
On 23/11/2022 12:16, Yu Tu wrote:
Hi Krzysztof,
Thank you for your reply.
On 2022/11/23 18:08, Krzysztof Kozlowski wrote:
[ EXTERNAL EMAIL ]
On 23/11/2022 03:13, Yu Tu wrote:
Add the S4 PLL clock controller found and bindings in the s4 SoC family.
Signed-off-by: Yu Tu <yu.tu@xxxxxxxxxxx>
---
.../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 +
This is v5 and still bindings are here? Bindings are always separate
patches. Use subject prefixes matching the subsystem (git log --oneline
-- ...).
And this was split, wasn't it? What happened here?!?
Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history.
https://lore.kernel.or/all/1jy1v6z14n.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
Jerome was asking you to send 2 patchsets, one with :
- bindings in separate patches
- drivers in separate patches
and a second with DT changes.
Indeed, this is what was asked. It is aligned with Krzysztof's request.
According to your discussion, I still should send patches in the previous
way in series. But I'm going to change it like you suggested.
I don't know, am I getting it right?
3 people tried to explain this already and we all told you the same thing.
* 1 patchset per maintainer: clk and dt
* bindings must be dedicated patches - never mixed with driver code.
I strongly suggest that you take some time to (re)read:
* https://docs.kernel.org/process/submitting-patches.html
* https://docs.kernel.org/devicetree/bindings/submitting-patches.html
If still unclear, please take some time to look at the kernel mailing
list archive and see how others have done the same things.
Thx.
I'll change it as you suggest.But I still don't understand what you suggested in V3.
I remember discussing it with you at V3.
https://lore.kernel.or/all/1jy1v6z14n.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
">>>> Also it would be nice to split this in two series.
>>>> Bindings and drivers in one, arm64 dt in the other. These changes goes
>>>> in through different trees.
>>> At present, Bindings, DTS and drivers are three series. Do you mean to put
>>> Bindings and drivers together? If so, checkpatch.pl will report a warning.
>> Yes because patches are not in yet so there is a good reason to ignore
>> the warning. Warning will never show up on the actual tree if the
>> patches are correctly ordered.
>
> I think Binding, DTS and drivers use three series and you said two series
> is not a big problem. Three series are recommended for checkpatch.pl, I
> think it should be easy for that to separate and merge。
No - There is only 2 series. 1 for the bindings and clock drivers and
one for the DT once things are in"
Please send the following emails:
* First patchset
[PATCH V6 0/3] clk: meson: Add S4 SoC PLL and Peripheral clock controller
[PATCH v6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock controller
[PATCH v6 2/3] clk: meson: add support for Amlogic S4 SoC PLL
[PATCH v6 3/3] clk: meson: add support for Amlogic S4 SoC peripheral clock controller
1) will contain only .yaml and dt-bindings include
2) will only have drivers/clk/meson changes
3) will only have drivers/clk/meson changes
* Second patchset:
[PATCH v1 0/2] arm64: dts: meson: Add S4 SoC PLL and Peripheral clock nodes
[PATCH v1 1/2] arm64: dts: meson: add S4 Soc PLL clock controller node
[PATCH v1 2/2] arm64: dts: meson: add S4 Soc Peripheral clock controller node
1) is the patch 3 of v5 patchset
2) is the patch 4 of v5 patchset
And in the second cover letter, explain those patches comes from the previous V5 patchset
and add a link to the V6 "drivers + bindings" patchset as a dependency.
Neil
Then when the bindings + clocks patches are merged, a pull request of the bindings
can be done to me so I can merge it with DT.
MAINTAINERS | 1 +
drivers/clk/meson/Kconfig | 13 +
drivers/clk/meson/Makefile | 1 +
drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++
drivers/clk/meson/s4-pll.h | 88 ++
.../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 +
7 files changed, 1059 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
create mode 100644 drivers/clk/meson/s4-pll.c
create mode 100644 drivers/clk/meson/s4-pll.h
create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h
diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
new file mode 100644
index 000000000000..fd517e8ef14f
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic Meson S serials PLL Clock Controller
+
+maintainers:
+ - Neil Armstrong <narmstrong@xxxxxxxxxxxx>
+ - Jerome Brunet <jbrunet@xxxxxxxxxxxx>
+ - Yu Tu <yu.hu@xxxxxxxxxxx>
+
One blank line.
I will delete this, on next version patch.
+
+properties:
+ compatible:
+ const: amlogic,s4-pll-clkc
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ clock-names:
+ items:
+ - const: xtal
+
+ "#clock-cells":
+ const: 1
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ clkc_pll: clock-controller@fe008000 {
+ compatible = "amlogic,s4-pll-clkc";
+ reg = <0xfe008000 0x1e8>;
+ clocks = <&xtal>;
+ clock-names = "xtal";
+ #clock-cells = <1>;
+ };
+#endif /* __MESON_S4_PLL_H__ */
diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h
new file mode 100644
index 000000000000..345f87023886
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h
This belongs to bindings patch, not driver.
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2021 Amlogic, Inc. All rights reserved.
+ * Author: Yu Tu <yu.tu@xxxxxxxxxxx>
+ */
+
+#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H
+#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H
+
+/*
+ * CLKID index values
+ */
+
+#define CLKID_FIXED_PLL 1
+#define CLKID_FCLK_DIV2 3
Indexes start from 0 and are incremented by 1. Not by 2.
NAK.
I remember Jerome discussing this with you.You can look at this submission history.
https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@xxxxxxxxxx/
Historically we did that by only exposing part of the numbers, controlling which
clocks were part of the bindings.
But it seems this doesn't make sens anymore, maybe it would be time to put all the
clock ids in the bindings for this new SoC and break with the previous strategy.
Krzysztof and I agreed there is nothing wrong with the current
approach, I believe.
It does not prevent someone from using an un-exposed clock, sure, or
exposing it in the future if necessary.
However, I think it clearly shows that an un-exposed element is not
expected to be used by an external consumers. It should be enough to
trigger a discussion if this expectation is wrong.
So the outcome of the previous discussion was somewhere later in that
thread:
It is just a choice to not expose some IDs.
It is not tied to the implementation at all.
I think we actually follow the rules and the idea behind it.
Best regards,
Krzysztof
.
.