Message-ID: <2a3c54e8-46fc-48f9-8c01-f3bb0c4907af@ti.com>
Date: Fri, 8 Nov 2024 17:49:54 +0530
From: "Anwar, Md Danish" <a0501179@...com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
        MD Danish Anwar
	<danishanwar@...com>, <conor+dt@...nel.org>,
        <krzk+dt@...nel.org>, <robh@...nel.org>, <ssantosh@...nel.org>,
        <nm@...com>, Vignesh Raghavendra
	<vigneshr@...com>
CC: <devicetree@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <s-anna@...com>, <kristo@...nel.org>,
        <srk@...com>, Roger Quadros <rogerq@...nel.org>
Subject: Re: [PATCH 1/2] dt-bindings: soc: ti: pruss: Add clocks for ICSSG

Hi Krzysztof,

On 11/7/2024 5:51 PM, Krzysztof Kozlowski wrote:
> On 07/11/2024 12:58, MD Danish Anwar wrote:
>>
>>
>> On 07/11/24 5:16 pm, MD Danish Anwar wrote:
>>>
>>>
>>> On 07/11/24 5:14 pm, Krzysztof Kozlowski wrote:
>>>> On 07/11/2024 12:36, MD Danish Anwar wrote:
>>>>>
>>>>>
>>>>> On 07/11/24 5:01 pm, Krzysztof Kozlowski wrote:
>>>>>> On 07/11/2024 11:45, MD Danish Anwar wrote:
>>>>>>> Add clocks, assigned-clocks and assigned-clock-parents for ICSSG
>>>>>>
>>>>>> Why? We see what you are doing from the diff, no point to repeat it. I
>>>>>> don't understand why you are doing it.
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: MD Danish Anwar <danishanwar@...com>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/soc/ti/ti,pruss.yaml          | 11 +++++++++++
>>>>>>>  1 file changed, 11 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
>>>>>>> index 3cb1471cc6b6..cf4c5884d8be 100644
>>>>>>> --- a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
>>>>>>> @@ -92,6 +92,17 @@ properties:
>>>>>>>      description: |
>>>>>>>        This property is as per sci-pm-domain.txt.
>>>>>>>  
>>>>>>> +  clocks:
>>>>>>> +    items:
>>>>>>> +      - description: ICSSG_CORE Clock
>>>>>>> +      - description: ICSSG_ICLK Clock
>>>>>>> +
>>>>>>> +  assigned-clocks:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  assigned-clock-parents:
>>>>>>> +    maxItems: 1
>>>>>>
>>>>>> Why? This is really not needed, so you need to explain why you are doing
>>>>>> things differently than entire Linux kernel / DT bindings.
>>>>>>
>>>>>
>>>>> I need to add this to the device tree node
>>>>>
>>>>> +		clocks = <&k3_clks 81 0>,  /* icssg0_core_clk */
>>>>> +			 <&k3_clks 81 20>; /* icssg0_iclk */
>>>>> +		assigned-clocks = <&k3_clks 81 0>;
>>>>> +		assigned-clock-parents = <&k3_clks 81 2>;
>>>>>
>>>>> But without the above change in the binding I am getting below errors
>>>>> while running dtbs check.
>>>>>
>>>>> /workdir/arch/arm64/boot/dts/ti/k3-am642-evm-nand.dtb: icssg@...00000:
>>>>> 'assigned-clock-parents', 'assigned-clocks' do not match any of the
>>>>> regexes: '^(pru|rtu|txpru)@[0-9a-f]+$', '^pa-stats@[a-f0-9]+$',
>>>>> 'cfg@[a-f0-9]+$', 'iep@[a-f0-9]+$', 'interrupt-controller@[a-f0-9]+$',
>>>>> 'mdio@[a-f0-9]+$', 'memories@[a-f0-9]+$', 'mii-g-rt@[a-f0-9]+$',
>>>>> 'mii-rt@[a-f0-9]+$', 'pinctrl-[0-9]+'
>>>>> +/workdir/arch/arm64/boot/dts/ti/k3-am642-evm-nand.dtb: icssg@...80000:
>>>>> 'anyOf' conditional failed, one must be fixed:
>>>>>
>>>>> To fix this warning I added these in the binding and the warnings were
>>>>> fixed.
>>>>
>>>> nah, cannot reproduce. Just be sure you work on recent kernel (last time
>>>> you were sending it on some ancient stuff) and your packages are
>>>> updated, including dt schema and other kernel dependencies.
>>>>

The purpose of this series is to add 'assigned-clock-parents',
'assigned-clocks' to the DT node. Initially I was only trying to add
these two nodes to DT and at that time I got the above error. I also got
 the below error as well

/home/danish/workspace/linux-next/arch/arm64/boot/dts/ti/k3-am642-evm.dtb:
icssg@...00000: 'anyOf' conditional failed, one must be fixed:
        'clocks' is a required property
        '#clock-cells' is a required property
        from schema $id: http://devicetree.org/schemas/clock/clock.yaml#


To fix this I added 'assigned-clock-parents', 'assigned-clocks' to the
binding and at this time I got only the below error,

/home/danish/workspace/linux-next/arch/arm64/boot/dts/ti/k3-am642-evm.dtb:
icssg@...00000: 'anyOf' conditional failed, one must be fixed:
        'clocks' is a required property
        '#clock-cells' is a required property
        from schema $id: http://devicetree.org/schemas/clock/clock.yaml#

So to fix this, I added clocks to the binding as well as DT and after
that all the errors got resolved and I posted the series.

>>>
>>> I have posted this series on the latest kernel. Base commit
>>> 5b913f5d7d7fe0f567dea8605f21da6eaa1735fb
>>>
>>> Let me check if the schema is up to date or not. I will re test and
>>> reply. Thanks for pointing it out.
>>>
>>
>> Krzysztof, I re-checked.
>> I am on the latest kernel (commit
>> 5b913f5d7d7fe0f567dea8605f21da6eaa1735fb (tag: next-20241106,
>> origin/master, origin/HEAD)) and I am using the lastest dtschema v2024.9
>>
>> ❯ python3 -m pip list|grep 'dtschema'
>> dtschema                      2024.9
>>
>> Still I am getting the below dtbs check errors while running `make
>> CHECK_DTBS=y ti/k3-am642-evm.dtb` without the binding change.
>>
>> Let me know if I am missing something else.
>>
>> /home/danish/workspace/linux-next/arch/arm64/boot/dts/ti/k3-am642-evm.dtb:
>> icssg@...00000: 'assigned-clock-parents', 'assigned-clocks', 'clocks' do
> 
> Wait, what? That's different error. You have clocks documented. To
> remind: we talk about previous error so only, *only* assigned-clocks.
> 

I agree. This is a different error. I encountered this error when I
dropped the binding patch of this series and tested only the DT patch.

When you commented on Binding patch mentioning it's not needed, I
thought you were referring to the entire diff. So I dropped the patch
and tested the DT patch only. And at this time I got this error.

>> not match any of the regexes: '^(pru|rtu|txpru)@[0-9a-f]+$',
>> '^pa-stats@[a-f0-9]+$', 'cfg@[a-f0-9]+$', 'iep@[a-f0-9]+$',
>> 'interrupt-controller@[a-f0-9]+$', 'mdio@[a-f0-9]+$',
>> 'memories@[a-f0-9]+$', 'mii-g-rt@[a-f0-9]+$', 'mii-rt@[a-f0-9]+$',
>> 'pinctrl-[0-9]+'
>> 	from schema $id: http://devicetree.org/schemas/soc/ti/ti,pruss.yaml#
>> /home/danish/workspace/linux-next/arch/arm64/boot/dts/ti/k3-am642-evm.dtb:
>> icssg@...00000: 'assigned-clock-parents', 'assigned-clocks', 'clocks' do
>> not match any of the regexes: '^(pru|rtu|txpru)@[0-9a-f]+$',
>> '^pa-stats@[a-f0-9]+$', 'cfg@[a-f0-9]+$', 'iep@[a-f0-9]+$',
>> 'interrupt-controller@[a-f0-9]+$', 'mdio@[a-f0-9]+$',
>> 'memories@[a-f0-9]+$', 'mii-g-rt@[a-f0-9]+$', 'mii-rt@[a-f0-9]+$',
>> 'pinctrl-[0-9]+'
>> 	from schema $id: http://devicetree.org/schemas/soc/ti/ti,pruss.yaml#
>> /home/danish/workspace/linux-next/arch/arm64/boot/dts/ti/k3-am642-evm.dtb:
>> icssg@...80000: 'assigned-clock-parents', 'assigned-clocks', 'clocks' do
>> not match any of the regexes: '^(pru|rtu|txpru)@[0-9a-f]+$',
>> '^pa-stats@[a-f0-9]+$', 'cfg@[a-f0-9]+$', 'iep@[a-f0-9]+$',
>> 'interrupt-controller@[a-f0-9]+$', 'mdio@[a-f0-9]+$',
>> 'memories@[a-f0-9]+$', 'mii-g-rt@[a-f0-9]+$', 'mii-rt@[a-f0-9]+$',
>> 'pinctrl-[0-9]+'
>> 	from schema $id: http://devicetree.org/schemas/soc/ti/ti,pruss.yaml#
>> /home/danish/workspace/linux-next/arch/arm64/boot/dts/ti/k3-am642-evm.dtb:
>> icssg@...80000: 'assigned-clock-parents', 'assigned-clocks', 'clocks' do
>> not match any of the regexes: '^(pru|rtu|txpru)@[0-9a-f]+$',
>> '^pa-stats@[a-f0-9]+$', 'cfg@[a-f0-9]+$', 'iep@[a-f0-9]+$',
> 
> I don't understand these, either.  All of them have clocks. What are you
> testing? You add clocks to DTS but not to the binding? What would be the
> point of that test?
> 

I did some more testing. Turns out just adding clocks to dt binding is
enough. Clocks will need to be added to binding however
'assigned-clock-parents', 'assigned-clocks' are not needed in the binding.

I will drop the 'assigned-clock-parents', 'assigned-clocks' from
dt-binding and only keep below diff. Where as for DT patch (2/2) - I
will keep it as it is.

diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
index 3cb1471cc6b6..12350409d154 100644
--- a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
+++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
@@ -92,6 +92,11 @@ properties:
     description: |
       This property is as per sci-pm-domain.txt.

+  clocks:
+    items:
+      - description: ICSSG_CORE Clock
+      - description: ICSSG_ICLK Clock
+
 patternProperties:

   memories@[a-f0-9]+$:

Let me know if this looks ok to you. Thanks for your feedback.

> Best regards,
> Krzysztof
> 

-- 
Thanks and Regards,
Md Danish Anwar

Powered by blists - more mailing lists