[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e9de0c51-01f1-4aa2-8950-cbfa54347729@collabora.com>
Date: Mon, 27 Oct 2025 11:24:07 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Conor Dooley <conor@...nel.org>
Cc: sboyd@...nel.org, mturquette@...libre.com, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, matthias.bgg@...il.com,
laura.nao@...labora.com, nfraprado@...labora.com, wenst@...omium.org,
y.oudjana@...tonmail.com, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
kernel@...labora.com
Subject: Re: [PATCH v1 6/7] dt-bindings: clock: Describe MT6685 PM/Clock IC
Clock Controller
Il 24/10/25 18:25, Conor Dooley ha scritto:
> On Fri, Oct 24, 2025 at 10:33:00AM +0200, AngeloGioacchino Del Regno wrote:
>> Add bindings to describe the SCK_TOP clock controller embedded
>> in the MT6685 IC, reachable over the SPMI bus.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>> ---
>>
>> NOTE: This does not contain any example because the MT6685 RTC
>> will be added to the mfd binding for MediaTek SPMI PMICs
>> and examples will be there.
>>
>> ** For reviewing purposes, this is how the example will look like: **
>>
>> - |
>> #include <dt-bindings/interrupt-controller/irq.h>
>> #include <dt-bindings/spmi/spmi.h>
>>
>> spmi {
>> #address-cells = <2>;
>> #size-cells = <0>;
>>
>> pmic@9 {
>> compatible = "mediatek,mt6363";
>> reg = <0x9 SPMI_USID>;
>> interrupts = <9 1 IRQ_TYPE_LEVEL_HIGH>;
>> interrupt-controller;
>> #address-cells = <1>;
>> #interrupt-cells = <3>;
>> #size-cells = <0>;
>>
>> clock-controller@514 {
>> compatible = "mediatek,mt6685-sck-top";
>> reg = <0x514>;
>> #clock-cells = <1>;
>> };
>>
>> rtc@580 {
>> compatible = "mediatek,mt6685-rtc";
>> reg = <0x580>;
>> interrupts = <9 0 IRQ_TYPE_LEVEL_HIGH>;
>> };
>> };
>> };
>>
>> .../bindings/clock/mediatek,mt6685-clock.yaml | 37 +++++++++++++++++++
>> .../dt-bindings/clock/mediatek,mt6685-clock.h | 17 +++++++++
>> 2 files changed, 54 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/mediatek,mt6685-clock.yaml
>> create mode 100644 include/dt-bindings/clock/mediatek,mt6685-clock.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt6685-clock.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt6685-clock.yaml
>> new file mode 100644
>> index 000000000000..5407ebf2f3b5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/mediatek,mt6685-clock.yaml
>> @@ -0,0 +1,37 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/mediatek,mt6685-clock.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MediaTek Clock Controller for MT6685 SPMI PM/Clock IC
>> +
>> +maintainers:
>> + - AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>> +
>> +description: |
>> + The clock architecture in MediaTek PMICs+Clock ICs is structured like below:
>> + Crystal(XO) or Internal ClockGen -->
>> + dividers -->
>> + muxes
>> + -->
>> + clock gate
>
> Is this the intended formatting? Looks weird with "dividers" being
> unaligned with the --> above it, but maybe you were just going for x
> number of spaces?
>
Yeah I was just going for x number of spaces, otherwise that may become a bit
"too long"...
>> +
>> + The device nodes provide clock gate control in different IP blocks.
>
> I think this is more understandable as "This device provides clock gate
> control", if this sck-top is only doing gating. Otherwise, not clear if
> the dividers and muxes are here or elsewhere.
-> Datasheets are incomplete (sad-face-here) <-
Most of the information here is grabbed from more than one downstream kernel
for more than one SoC/device, and assembled together.
The XO/clockgen and dividers are not in SCKTOP - those should be partially in
the "TOP" portion (yeah, there's a top and a sck-top), and partially in another
block that controls only the clockgen.
I didn't want to implement those two, even though I almost precisely know how to
do that (and I did it in some local tests), because I could only gather partial
information and I didn't feel confident in upstreaming something that I'm not
entirely sure about.
Same goes for the MUX part: there's some here, some there, one in scktop as well
(but I didn't describe it because again incomplete info, and even downstream the
only mux in scktop seems to be unused).
So yeah - apart from one mux, anything before clock gate is elsewhere... I can
change that statement to the one you proposed, looks a bit better than what I
came up with, so thanks for that :-D
>
>> +properties:
>> + compatible:
>> + const: mediatek,mt6685-sck-top
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + '#clock-cells':
>> + const: 1
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - '#clock-cells'
>> +
>> +additionalProperties: false
>> diff --git a/include/dt-bindings/clock/mediatek,mt6685-clock.h b/include/dt-bindings/clock/mediatek,mt6685-clock.h
>> new file mode 100644
>> index 000000000000..acc5e2e15ce1
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/mediatek,mt6685-clock.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>> +/*
>> + * Copyright (c) 2025 Collabora Ltd.
>> + * AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>> + */
>> +
>> +#ifndef _DT_BINDINGS_CLK_MT6685_H
>> +#define _DT_BINDINGS_CLK_MT6685_H
>> +
>> +/* SCK_TOP_CKPDN */
>> +#define CLK_RTC_SEC_MCLK 0
>> +#define CLK_RTC_EOSC32 1
>> +#define CLK_RTC_SEC_32K 2
>> +#define CLK_RTC_MCLK 3
>> +#define CLK_RTC_32K 4
>> +
>> +#endif /* _DT_BINDINGS_CLK_MT6685_H */
>> --
>> 2.51.1
>>
Powered by blists - more mailing lists