[PATCH V2] dt-bindings: thermal: mediatek,thermal: document AUXADC 32k clock

[PATCH V2] dt-bindings: thermal: mediatek,thermal: document AUXADC 32k clock

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

SoCs MT7981 and MT7986 include a newer thermal block (V3) that requires
enabling one more clock called AUXADC 32k. Require it in binding.

Cc: Daniel Golle <daniel@makrotopia.org>
Cc: Sam Shih <sam.shih@mediatek.com>
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Disallow "adc_32k" on other chipsets (maxItems: 2)

 .../bindings/thermal/mediatek,thermal.yaml    | 31 +++++++++++++++++--
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml
index d96a2e32bd8f..e7373d78618c 100644
--- a/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml
+++ b/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml
@@ -15,9 +15,6 @@ description:
   controls a mux in the apmixedsys register space via AHB bus accesses, so a
   phandle to the APMIXEDSYS is also needed.
 
-allOf:
-  - $ref: thermal-sensor.yaml#
-
 properties:
   compatible:
     enum:
@@ -38,14 +35,18 @@ properties:
     maxItems: 1
 
   clocks:
+    minItems: 2
     items:
       - description: Main clock needed for register access
       - description: The AUXADC clock
+      - description: AUXADC 32k clock
 
   clock-names:
+    minItems: 2
     items:
       - const: therm
       - const: auxadc
+      - const: adc_32k
 
   mediatek,auxadc:
     $ref: /schemas/types.yaml#/definitions/phandle
@@ -76,6 +77,30 @@ required:
   - mediatek,auxadc
   - mediatek,apmixedsys
 
+allOf:
+  - $ref: thermal-sensor.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mediatek,mt7981-thermal
+              - mediatek,mt7986-thermal
+    then:
+      properties:
+        clocks:
+          minItems: 3
+
+        clock-names:
+          minItems: 3
+    else:
+      properties:
+        clocks:
+          maxItems: 2
+
+        clock-names:
+          maxItems: 2
+
 unevaluatedProperties: false
 
 examples:
-- 
2.35.3

Re: [PATCH V2] dt-bindings: thermal: mediatek,thermal: document AUXADC 32k clock

[Krzysztof Kozlowski]

On 09/02/2024 06:52, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> SoCs MT7981 and MT7986 include a newer thermal block (V3) that requires
> enabling one more clock called AUXADC 32k. Require it in binding.
> 
> Cc: Daniel Golle <daniel@makrotopia.org>
> Cc: Sam Shih <sam.shih@mediatek.com>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH V2] dt-bindings: thermal: mediatek,thermal: document AUXADC 32k clock

[AngeloGioacchino Del Regno]

Il 09/02/24 06:52, Rafał Miłecki ha scritto:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> SoCs MT7981 and MT7986 include a newer thermal block (V3) that requires
> enabling one more clock called AUXADC 32k. Require it in binding.
> 
> Cc: Daniel Golle <daniel@makrotopia.org>
> Cc: Sam Shih <sam.shih@mediatek.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

So, I've made some research on this matter.. and this is a NACK.

I'll keep it short: you don't really have any 32KHz clock input for the ADC in
the infracfg clock controller (nor others).

What you're trying to say here is that you have a FastRC Oscillator input clock
(in your case, this is CLK_INFRA_ADC_FRC_CK): this clock is responsible for
providing an input clock of either 26MHz or something that is MPLL/16 (I do not
know what MPLL outputs on your platform, but those PLLs are usually giving out
"big numbers").

The FRC osc in the AUXADC will perhaps output a 32KHz clock for internal reference
and/or whatever else, but I'm not even sure that it's really going to be 32k then
internally to the IP block.

(I said "I'll keep it short" btw :-P)

Anyway.

The FRC clock is also required for the INFRA_ADC_26M clock to work correctly and,
in the clock controller driver(s) of both 7981 and 7986, that is parented to....
... surprise surprise ... INFRA_ADC_FRC_CK :-)


Finally, you do *not* need this commit, INFRA does *not* provide an adc_32k clock
to the thermal block, and you do *not* need to add the FRC clock in the devicetree
of 7981, nor 7986.

Mind you - at least in mt7986a.dtsi, there is an adc_32k clock in the thermal node,
so the right way to get your thermal node validated is not to modify the binding,
but to adhere to it by removing the extra, unneeded, clock.

Cheers!
Angelo

Re: [PATCH V2] dt-bindings: thermal: mediatek,thermal: document AUXADC 32k clock

[Rafał Miłecki]

On 9.02.2024 10:13, AngeloGioacchino Del Regno wrote:
> Il 09/02/24 06:52, Rafał Miłecki ha scritto:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> SoCs MT7981 and MT7986 include a newer thermal block (V3) that requires
>> enabling one more clock called AUXADC 32k. Require it in binding.
>>
>> Cc: Daniel Golle <daniel@makrotopia.org>
>> Cc: Sam Shih <sam.shih@mediatek.com>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> 
> So, I've made some research on this matter.. and this is a NACK.

Well, I can only thank you for the research.

Let's drop this patch. I'll sort out the rest later.

Re: [PATCH V2] dt-bindings: thermal: mediatek,thermal: document AUXADC 32k clock

[AngeloGioacchino Del Regno]

Il 12/02/24 07:11, Rafał Miłecki ha scritto:
> On 9.02.2024 10:13, AngeloGioacchino Del Regno wrote:
>> Il 09/02/24 06:52, Rafał Miłecki ha scritto:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> SoCs MT7981 and MT7986 include a newer thermal block (V3) that requires
>>> enabling one more clock called AUXADC 32k. Require it in binding.
>>>
>>> Cc: Daniel Golle <daniel@makrotopia.org>
>>> Cc: Sam Shih <sam.shih@mediatek.com>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>
>> So, I've made some research on this matter.. and this is a NACK.
> 
> Well, I can only thank you for the research.
> 
> Let's drop this patch. I'll sort out the rest later.

Thank you for bringing that up - this was necessary to clarify this and
to actually make both of us aware of the mistake in the device trees that
we have for those SoCs.

Take your time, btw, you're doing a great job.

Cheers,
Angelo