Re: [PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml

[Ansuel Smith <ansuelsmth@gmail.com>]

On Wed, Mar 23, 2022 at 10:59:39AM +0100, Krzysztof Kozlowski wrote:
> On 22/03/2022 14:26, Ansuel Smith wrote:
> > On Tue, Mar 22, 2022 at 11:07:26AM +0100, Krzysztof Kozlowski wrote:
> >> On 22/03/2022 00:15, Ansuel Smith wrote:
> >>> Convert kpss-gcc driver Documentation to yaml. Since kpss-gcc expose a
> >>> clock add the required '#clock-cells' binding while converting it.
> >>>
> >>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> >>> ---
> >>>  .../bindings/arm/msm/qcom,kpss-gcc.txt        | 44 ------------
> >>>  .../bindings/arm/msm/qcom,kpss-gcc.yaml       | 69 +++++++++++++++++++
> >>>  2 files changed, 69 insertions(+), 44 deletions(-)
> >>>  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-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..7eb852be02c1
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> >>> @@ -0,0 +1,69 @@
> >>> +# 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:
> >>> +    items:
> >>> +      - enum:
> >>> +          - qcom,kpss-gcc-ipq8064
> >>> +          - qcom,kpss-gcc-apq8064
> >>> +          - qcom,kpss-gcc-msm8974
> >>> +          - qcom,kpss-gcc-msm8960
> >>> +      - const: qcom,kpss-gcc
> >>> +      - const: syscon
> >>
> >> There was no syscon here before. This is not explained in commit msg or
> >> patch history, while I asked to document explicitly any deviation from
> >> the conversion.
> >>
> >> This is not how the process works. You keep making silent/hidden changes
> >> to the bindings and to the patch submission process. It's difficult to
> >> review and it is even more difficult to trust you that you implement
> >> what we ask for. You keep resending versions of the patchset the same
> >> day (two versions yesterday, shortly after another one) which does not
> >> give time to react and review. Plus then you hide some more changes to
> >> regular conversion without explaining them.
> >>
> >> NAK. It's really bad process. :(
> >>
> >>
> >> Best regards,
> >> Krzysztof
> > 
> > The thing is that i'm trying to fix all the mess of years of keeping bad
> > Documentation and having dts that never followed Documentation. It's
> > really nothing silent/hidden. You add review tag to a patch? That won't
> > change. The bot alert me of some bugs? I push another revision with the
> > bug fixed. 
> 
> It does not necessarily mean that bindings are bad and such changes
> should be documented.
> 
> > (I understand I should not send that much revision in the
> > same day but still considering the slow process of reviewing the c code,
> > I prefer to keep the Documentation part correct and ready)
> 
> Rob also pointed to this - sending two versions of this huge patchset
> the same day is way too much.
>

You are totally right. I understand now the problem.

> > 
> > If you notice the changes across the different patch, it's very minimal
> > and 99% of it has not changed. Nothing silent just me addressing warning
> > from the bot. About the trust issue...
> > Is it really a syscon addition that bad? Again the original
> > Documentation was just bad so why should we care to have a 100% 1:1
> > conversion if it should have been not accepted in the first place.
> 
> Does not have to be 100% but deviations should be either expected or
> explained. Bindings are used also outside of Linux kernel.
> 
> > The addition of this new syscon is because in the current dtsi it's
> > there and I assume it's there as this is a global accessor and probably
> > other driver would access the same regs (so it's also a syscon)
> 
> If these are assumptions, then they need to be checked. If these were
> new bindings, we would discuss/check the need of syscon. Now we do not
> question existing properties, because they were accepted. But syscon
> compatible was not accepted, so putting it here requires our acknowledgment.
> 

About this I have a question. If the dts already have some binding and
the Documentation doesn't have them. Should the dts have priority or the
Documentation?
In the case where we can't prove that syscon is needed (for example), can
we remove it from dts (and accept to have inconsistency while the dts
changes are merged) or we should add the extra binding to the
Documentation putting some comments about it and discussing the
inclusion? 

> The bindings are probably pure junk, so this is not merely a conversion
> how you wrote in commit msg. This is rework of the bindings. Don't hide
> rework under "conversion". Conversion is TXT->YAML without any changes...
> 

Ok, thanks for the clarification. I still think should be handled with
conversion + additional commit to add the missing part so I have to fix
my wrong commits.

> I asked about this before and the only part you added to commit msg was
> "clock-cells". And now I see syscon - so isn't it a bit surprising?
> 

You are right... I will just do the 1:1 conversion and put all these
addition to a separate commit to make them clear.

> > 
> > I understand the complain about putting too much revision... But NAK
> > this cause I'm trying to fix all this mess just because more and more
> > problems are coming up and I'm trying to fix them. It's a bit sad.
> 
> Why you cannot test your changes and fix them all before sending sixth
> version? Why the bot has to test your code, not you?
> 

I'm aksed Rob if there is a quicker way to test single Documenation and
dts but it's my fault anyway.

> > Hope you can understand that it's not my interest to push silent changes
> > or other nasty stuff. It's just me fixing the mess and trying to at
> > least have the Documentation part ready while I wait for c code review.
> Best regards,
> Krzysztof

-- 
	Ansuel