From: Krzysztof Kozlowski <krzk@kernel.org>
To: Ansuel Smith <ansuelsmth@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Andy Gross <agross@kernel.org>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org
Subject: Re: [PATCH v2 16/16] dt-bindings: arm: msm: Convert kpss driver Documentation to yaml
Date: Sun, 20 Mar 2022 15:47:27 +0100 [thread overview]
Message-ID: <ae9a0e80-d9a5-320f-f1d5-9666633a6a87@kernel.org> (raw)
In-Reply-To: <YjbvpN5S77cyyN/U@Ansuel-xps.localdomain>
On 20/03/2022 10:11, Ansuel Smith wrote:
> On Sun, Mar 20, 2022 at 01:05:51PM +0100, Krzysztof Kozlowski wrote:
>> On 18/03/2022 17:08, Ansuel Smith wrote:
>>> Convert kpss-acc and kpss-gcc Documentation to yaml. Fix multiple
>>> Documentation error and provide additional example for kpss-gcc-v2.
>>>
>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
>>> ---
>>> .../bindings/arm/msm/qcom,kpss-acc.txt | 49 ----------
>>> .../bindings/arm/msm/qcom,kpss-acc.yaml | 97 +++++++++++++++++++
>>> .../bindings/arm/msm/qcom,kpss-gcc.txt | 44 ---------
>>> .../bindings/arm/msm/qcom,kpss-gcc.yaml | 63 ++++++++++++
>>> 4 files changed, 160 insertions(+), 93 deletions(-)
>>> delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
>>> create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
>>> delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
>>> create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
>>> deleted file mode 100644
>>> index 7f696362a4a1..000000000000
>>> --- a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
>>> +++ /dev/null
>>> @@ -1,49 +0,0 @@
>>> -Krait Processor Sub-system (KPSS) Application Clock Controller (ACC)
>>> -
>>> -The KPSS ACC provides clock, power domain, and reset control to a Krait CPU.
>>> -There is one ACC register region per CPU within the KPSS remapped region as
>>> -well as an alias register region that remaps accesses to the ACC associated
>>> -with the CPU accessing the region.
>>> -
>>> -PROPERTIES
>>> -
>>> -- compatible:
>>> - Usage: required
>>> - Value type: <string>
>>> - Definition: should be one of:
>>> - "qcom,kpss-acc-v1"
>>> - "qcom,kpss-acc-v2"
>>> -
>>> -- reg:
>>> - Usage: required
>>> - Value type: <prop-encoded-array>
>>> - Definition: the first element specifies the base address and size of
>>> - the register region. An optional second element specifies
>>> - the base address and size of the alias register region.
>>> -
>>> -- clocks:
>>> - Usage: required
>>> - Value type: <prop-encoded-array>
>>> - Definition: reference to the pll parents.
>>> -
>>> -- clock-names:
>>> - Usage: required
>>> - Value type: <stringlist>
>>> - Definition: must be "pll8_vote", "pxo".
>>> -
>>> -- clock-output-names:
>>> - Usage: optional
>>> - Value type: <string>
>>> - Definition: Name of the output clock. Typically acpuX_aux where X is a
>>> - CPU number starting at 0.
>>> -
>>> -Example:
>>> -
>>> - clock-controller@2088000 {
>>> - compatible = "qcom,kpss-acc-v2";
>>> - reg = <0x02088000 0x1000>,
>>> - <0x02008000 0x1000>;
>>> - clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>;
>>> - clock-names = "pll8_vote", "pxo";
>>> - clock-output-names = "acpu0_aux";
>>> - };
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
>>> new file mode 100644
>>> index 000000000000..6e8ef4f85eab
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
>>> @@ -0,0 +1,97 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-acc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Krait Processor Sub-system (KPSS) Application Clock Controller (ACC)
>>> +
>>> +maintainers:
>>> + - Ansuel Smith <ansuelsmth@gmail.com>
>>> +
>>> +description: |
>>> + The KPSS ACC provides clock, power domain, and reset control to a Krait CPU.
>>> + There is one ACC register region per CPU within the KPSS remapped region as
>>> + well as an alias register region that remaps accesses to the ACC associated
>>> + with the CPU accessing the region.
>>> +
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - qcom,kpss-acc-v1
>>> + - qcom,kpss-acc-v2
>>> +
>>> + reg:
>>> + items:
>>> + - description: Base address and size of the register region
>>> + - description: Optional base address and size of the alias register region
>>> +
>>> + clocks:
>>> + items:
>>> + - description: phandle to pll8_vote
>>> + - description: phandle to pxo_board
>>> +
>>> + clock-names:
>>> + items:
>>> + - const: pll8_vote
>>> + - const: pxo
>>> +
>>> + clock-output-names:
>>> + description: Name of the aux clock. Krait can have at most 4 cpu.
>>> + enum:
>>> + - acpu0_aux
>>> + - acpu1_aux
>>> + - acpu2_aux
>>> + - acpu3_aux
>>> +
>>> + '#clock-cells':
>>> + const: 0
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> +
>>> +allOf:
>>> + - if:
>>> + properties:
>>> + compatible:
>>> + contains:
>>
>> This is not obvious from the original bindings. Why are these required
>> only on v1? Whatr about v2? Are they allowed on v2? Why at least
>> clock-cells are not required on v2?
>>
>
> The original Documentation was a joke. I should have also write it in
> the cover letter. v2 doesn't export clocks and just expose the kpss
> regs. Doesn't provide any mux / clocks. It's really an entirely
> different implementation. Aka the Documentation was wrong from the
> start.
OK, thanks for explanation. Please mention it in the commit msg.
>
>> The previous bindings make them required which you change here. Your
>> commit does not explain this.
>>
>> Please explicitly explain in commit msg all changes to the bindings
>> which are necessary for conversion.
>>
>>> + const: qcom,kpss-acc-v1
>>> + then:
>>> + required:
>>> + - clocks
>>> + - clock-names
>>> + - clock-output-names
>>> + - '#clock-cells'
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/clock/qcom,gcc-ipq806x.h>
>>> +
>>> + clock-controller@2088000 {
>>> + compatible = "qcom,kpss-acc-v1";
>>> + reg = <0x02088000 0x1000>, <0x02008000 0x1000>;
>>> + clocks = <&gcc PLL8_VOTE>, <&pxo_board>;
>>> + clock-names = "pll8_vote", "pxo";
>>> + clock-output-names = "acpu0_aux";
>>> + #clock-cells = <0>;
>>> + };
>>> +
>>> + clock-controller@2098000 {
>>> + compatible = "qcom,kpss-acc-v1";
>>> + reg = <0x02098000 0x1000>, <0x02008000 0x1000>;
>>> + clock-output-names = "acpu1_aux";
>>> + clocks = <&gcc PLL8_VOTE>, <&pxo_board>;
>>> + clock-names = "pll8_vote", "pxo";
>>> + #clock-cells = <0>;
>>
>> It's the same example as above, no differences. Remove it.
>>
>
> Mhh this is a direct example on how the kpss-acc are defined for ipq806x
> with a configuration with 2 cpu. Tell me if I should really remove it or
> not.
Yeah, but all properties are exactly the same, there is no difference
here, so this as an example does not bring additional information.
>
>>> + };
>>> +
>>> + - |
>>> + clock-controller@f9088000 {
>>> + compatible = "qcom,kpss-acc-v2";
>>> + reg = <0xf9088000 0x1000>,
>>> + <0xf9008000 0x1000>;
>>
>> You now changed the example. This looks wrong.
>>
>
> Again the example was wrong from the start. There isn't ANY kpss v2 that
> exports clock and the driver that expose the aux clocks have just the
> compatible for v1. (that is correct)
OK, please also mention it in commit msg.
>
>>> + };
>>> +...
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
>>> deleted file mode 100644
>>> index e628758950e1..000000000000
>>> --- a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
>>> +++ /dev/null
>>> @@ -1,44 +0,0 @@
>>> -Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
>>> -
>>> -PROPERTIES
>>> -
>>> -- compatible:
>>> - Usage: required
>>> - Value type: <string>
>>> - Definition: should be one of the following. The generic compatible
>>> - "qcom,kpss-gcc" should also be included.
>>> - "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc"
>>> - "qcom,kpss-gcc-apq8064", "qcom,kpss-gcc"
>>> - "qcom,kpss-gcc-msm8974", "qcom,kpss-gcc"
>>> - "qcom,kpss-gcc-msm8960", "qcom,kpss-gcc"
>>> -
>>> -- reg:
>>> - Usage: required
>>> - Value type: <prop-encoded-array>
>>> - Definition: base address and size of the register region
>>> -
>>> -- clocks:
>>> - Usage: required
>>> - Value type: <prop-encoded-array>
>>> - Definition: reference to the pll parents.
>>> -
>>> -- clock-names:
>>> - Usage: required
>>> - Value type: <stringlist>
>>> - Definition: must be "pll8_vote", "pxo".
>>> -
>>> -- clock-output-names:
>>> - Usage: required
>>> - Value type: <string>
>>> - Definition: Name of the output clock. Typically acpu_l2_aux indicating
>>> - an L2 cache auxiliary clock.
>>> -
>>> -Example:
>>> -
>>> - l2cc: clock-controller@2011000 {
>>> - compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc";
>>> - reg = <0x2011000 0x1000>;
>>> - clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>;
>>> - clock-names = "pll8_vote", "pxo";
>>> - clock-output-names = "acpu_l2_aux";
>>> - };
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
>>> new file mode 100644
>>> index 000000000000..578e2eccb7db
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
>>> @@ -0,0 +1,63 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-gcc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
>>> +
>>> +maintainers:
>>> + - Ansuel Smith <ansuelsmth@gmail.com>
>>> +
>>> +description: |
>>> + Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
>>> + to control L2 mux (in the current implementation).
>>> +
>>> +properties:
>>> + compatible:
>>> + const: qcom,kpss-gcc
>>
>> This is wrong conversion. You removed quite a lot.
>>
>
> The driver doesn't have any support for it and we have no user for
> kpss-gcc. Again another Documentation that was pushed for the sake of it
> with no real user/no sense at all. Tell me if I should keep random
> compatible anyway. Should't be better to fix them for good and make devs
> add a specific compatible when the driver actually add support for them?
We talk here mostly about bindings, not driver, so the driver's
implementation matters less. IOW, driver does not have to implement all
compatibles from bindings. Bindings can be used in other projects,
outside of mainline kernel, so the compatibles actually might be useful.
These compatibles might be reasonable even if not implemented in the
driver, without real user.
More important question is whether these compatibles make sense. That
part I don't exactly now. You used term "random compatible", so are they
really random? To me they look like specific implementation of GCC, so
they look reasonable, but I know nothing about Qualcomm Krait and GCC.
>
>> Please split this commit into two separate conversions (acc and gcc).
>>
>
> Will do.
>
>>> +
>>> + reg:
>>> + items:
>>> + - description: Base address and size of the register region
>>
>> Just maxItems:1. No need for items and description - it's obvious.
>>
>
> Ok.
>
>>> +
>>> + clocks:
>>> + items:
>>> + - description: phandle to pll8_vote
>>> + - description: phandle to pxo_board
>>> +
>>> + clock-names:
>>> + items:
>>> + - const: pll8_vote
>>> + - const: pxo
>>> +
>>> + clock-output-names:
>>> + const: acpu_l2_aux
>>> +
>>
>>
>> Best regards,
>> Krzysztof
>
> I hope I don't look rude with my response. The Documentation for this
> stuff is really old and was pushed randomly. Guess how fun it was to
> discover that NONE of them were actually right and we have this from
> ages broken. Will for sure improve the commit description.
>
> Also considering how brake they are should I really do a direct
> conversion and then fix them? Instead of dropping them for good and
> reimplement them the correct way directly?
It depends. If the bindings are really wrong, usually we combine the
changes necessary for conversion in one commit. Tweaking compatibles to
real life or adding new compatibles - maybe better in a separate commit.
Best regards,
Krzysztof
prev parent reply other threads:[~2022-03-20 14:47 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-18 16:08 [PATCH v2 00/16] Modernize rest of the krait drivers Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 01/16] clk: introduce clk_hw_get_index_of_parent new API Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 02/16] clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 03/16] clk: qcom: gcc-ipq806x: add PXO_SRC in clk table Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 04/16] clk: qcom: clk-hfpll: use poll_timeout macro Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 05/16] clk: qcom: kpss-xcc: convert to parent data API Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 06/16] clk: qcom: clk-krait: unlock spin after mux completion Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 07/16] clk: qcom: clk-krait: add hw_parent check for div2_round_rate Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 08/16] clk: qcom: krait-cc: convert to parent_data API Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 09/16] clk: qcom: krait-cc: drop pr_info and register qsb only if needed Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 10/16] clk: qcom: krait-cc: drop hardcoded safe_sel Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 11/16] clk: qcom: krait-cc: force sec_mux to QSB Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 12/16] clk: qcom: clk-krait: add apq/ipq8064 errata workaround Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 13/16] clk: qcom: clk-krait: add enable disable ops Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 14/16] dt-bindings: clock: Convert qcom,krait-cc to yaml Ansuel Smith
2022-03-20 12:09 ` Krzysztof Kozlowski
2022-03-18 16:08 ` [PATCH v2 15/16] ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and clocks Ansuel Smith
2022-03-18 16:08 ` [PATCH v2 16/16] dt-bindings: arm: msm: Convert kpss driver Documentation to yaml Ansuel Smith
2022-03-20 12:05 ` Krzysztof Kozlowski
2022-03-20 9:11 ` Ansuel Smith
2022-03-20 14:47 ` Krzysztof Kozlowski [this message]
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=ae9a0e80-d9a5-320f-f1d5-9666633a6a87@kernel.org \
--to=krzk@kernel.org \
--cc=agross@kernel.org \
--cc=ansuelsmth@gmail.com \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=robh+dt@kernel.org \
--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