The mpll3 clock is a valid parent clock for sd_emmc and mipi_isp on
the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml.
Add it to enable proper clock parent configuration for these peripherals.
Signed-off-by: Jian Hu <jian.hu@amlogic.com>
---
.../bindings/clock/amlogic,t7-peripherals-clkc.yaml | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml
index 55bb73707d58..27cc1f331587 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml
+++ b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml
@@ -24,7 +24,7 @@ properties:
const: 1
clocks:
- minItems: 14
+ minItems: 15
items:
- description: input oscillator
- description: input sys clk
@@ -40,12 +40,13 @@ properties:
- description: input gp1 pll
- description: input mpll1
- description: input mpll2
+ - description: input mpll3
- description: external input rmii oscillator (optional)
- description: input video pll0 (optional)
- description: external pad input for rtc (optional)
clock-names:
- minItems: 14
+ minItems: 15
items:
- const: xtal
- const: sys
@@ -61,6 +62,7 @@ properties:
- const: gp1
- const: mpll1
- const: mpll2
+ - const: mpll3
- const: ext_rmii
- const: vid_pll0
- const: ext_rtc
@@ -98,6 +100,7 @@ examples:
<&gp1 1>,
<&mpll 4>,
<&mpll 6>;
+ <&mpll 8>;
clock-names = "xtal",
"sys",
"fix",
@@ -112,5 +115,6 @@ examples:
"gp1",
"mpll1",
"mpll2";
+ "mpll3";
};
};
--
2.47.1On Thu, Mar 05, 2026 at 03:43:26PM +0800, Jian Hu wrote: > The mpll3 clock is a valid parent clock for sd_emmc and mipi_isp on > the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. > Add it to enable proper clock parent configuration for these peripherals. > > Signed-off-by: Jian Hu <jian.hu@amlogic.com> > --- > .../bindings/clock/amlogic,t7-peripherals-clkc.yaml | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml > index 55bb73707d58..27cc1f331587 100644 > --- a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml > +++ b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml > @@ -24,7 +24,7 @@ properties: > const: 1 > > clocks: > - minItems: 14 > + minItems: 15 > items: > - description: input oscillator > - description: input sys clk > @@ -40,12 +40,13 @@ properties: > - description: input gp1 pll > - description: input mpll1 > - description: input mpll2 > + - description: input mpll3 Nah, ABI break. You add it to the end of the list or provide arguments on ABI impact. Best regards, Krzysztof
On 3/6/2026 4:12 PM, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On Thu, Mar 05, 2026 at 03:43:26PM +0800, Jian Hu wrote: >> The mpll3 clock is a valid parent clock for sd_emmc and mipi_isp on >> the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. >> Add it to enable proper clock parent configuration for these peripherals. >> >> Signed-off-by: Jian Hu <jian.hu@amlogic.com> >> --- >> .../bindings/clock/amlogic,t7-peripherals-clkc.yaml | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >> index 55bb73707d58..27cc1f331587 100644 >> --- a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >> +++ b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >> @@ -24,7 +24,7 @@ properties: >> const: 1 >> >> clocks: >> - minItems: 14 >> + minItems: 15 >> items: >> - description: input oscillator >> - description: input sys clk >> @@ -40,12 +40,13 @@ properties: >> - description: input gp1 pll >> - description: input mpll1 >> - description: input mpll2 >> + - description: input mpll3 > Nah, ABI break. You add it to the end of the list or provide arguments > on ABI impact. The third patch in this series enables the DT for the Amlogic T7 clock controller. The clock controller node for amlogic,t7-peripherals-clkc has not been merged upstream yet. This change modifies the clock index order, but it will not break any existing device tree since the amlogic,t7-peripherals-clkc bindings are not used by any upstream or downstream DT at this time. Therefore, it does NOT break the ABI. The last clock entry is an external pad input for RTC and it is optional. For logical consistency, it is better to place the required mpll3 entry before the optional entry. If this change does not break the ABI, could I keep it in its original logical order right after mpll2? > Best regards, > Krzysztof >
On 10/03/2026 07:51, Jian Hu wrote: > > On 3/6/2026 4:12 PM, Krzysztof Kozlowski wrote: >> [ EXTERNAL EMAIL ] >> >> On Thu, Mar 05, 2026 at 03:43:26PM +0800, Jian Hu wrote: >>> The mpll3 clock is a valid parent clock for sd_emmc and mipi_isp on >>> the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. >>> Add it to enable proper clock parent configuration for these peripherals. >>> >>> Signed-off-by: Jian Hu <jian.hu@amlogic.com> >>> --- >>> .../bindings/clock/amlogic,t7-peripherals-clkc.yaml | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >>> index 55bb73707d58..27cc1f331587 100644 >>> --- a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >>> +++ b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >>> @@ -24,7 +24,7 @@ properties: >>> const: 1 >>> >>> clocks: >>> - minItems: 14 >>> + minItems: 15 >>> items: >>> - description: input oscillator >>> - description: input sys clk >>> @@ -40,12 +40,13 @@ properties: >>> - description: input gp1 pll >>> - description: input mpll1 >>> - description: input mpll2 >>> + - description: input mpll3 >> Nah, ABI break. You add it to the end of the list or provide arguments >> on ABI impact. > > The third patch in this series enables the DT for the Amlogic T7 clock > controller. > > The clock controller node for amlogic,t7-peripherals-clkc has not been > merged upstream yet. > This change modifies the clock index order, but it will not break any > existing device tree since the > amlogic,t7-peripherals-clkc bindings are not used by any upstream or > downstream DT at this time. > > Therefore, it does NOT break the ABI. It does. Clearly visible from the diff above, because the order is the ABI. > > The last clock entry is an external pad input for RTC and it is optional. > For logical consistency, it is better to place the required mpll3 entry > before the optional entry. > > If this change does not break the ABI, could I keep it in its original > logical order right after mpll2? Change breaks the ABI and commit must explain why and the impact. Best regards, Krzysztof
On 3/10/2026 3:08 PM, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 10/03/2026 07:51, Jian Hu wrote: >> On 3/6/2026 4:12 PM, Krzysztof Kozlowski wrote: >>> [ EXTERNAL EMAIL ] >>> >>> On Thu, Mar 05, 2026 at 03:43:26PM +0800, Jian Hu wrote: >>>> The mpll3 clock is a valid parent clock for sd_emmc and mipi_isp on >>>> the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. >>>> Add it to enable proper clock parent configuration for these peripherals. >>>> >>>> Signed-off-by: Jian Hu <jian.hu@amlogic.com> >>>> --- >>>> .../bindings/clock/amlogic,t7-peripherals-clkc.yaml | 8 ++++++-- >>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >>>> index 55bb73707d58..27cc1f331587 100644 >>>> --- a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >>>> @@ -24,7 +24,7 @@ properties: >>>> const: 1 >>>> >>>> clocks: >>>> - minItems: 14 >>>> + minItems: 15 >>>> items: >>>> - description: input oscillator >>>> - description: input sys clk >>>> @@ -40,12 +40,13 @@ properties: >>>> - description: input gp1 pll >>>> - description: input mpll1 >>>> - description: input mpll2 >>>> + - description: input mpll3 >>> Nah, ABI break. You add it to the end of the list or provide arguments >>> on ABI impact. >> The third patch in this series enables the DT for the Amlogic T7 clock >> controller. >> >> The clock controller node for amlogic,t7-peripherals-clkc has not been >> merged upstream yet. >> This change modifies the clock index order, but it will not break any >> existing device tree since the >> amlogic,t7-peripherals-clkc bindings are not used by any upstream or >> downstream DT at this time. >> >> Therefore, it does NOT break the ABI. > It does. Clearly visible from the diff above, because the order is the ABI. Got it, I understand your point that changing the order break the ABI (since index order is part of the ABI). >> The last clock entry is an external pad input for RTC and it is optional. >> For logical consistency, it is better to place the required mpll3 entry >> before the optional entry. >> >> If this change does not break the ABI, could I keep it in its original >> logical order right after mpll2? > Change breaks the ABI and commit must explain why and the impact. > It does break the ABI. The mpll3 clock is one parent clock of sd_emmc and mipi_isp clock on the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. Add the mpll3 clock source for the T7 peripherals clock controller. so that sd_emmc and mipi_isp can use mpll3. That's why this patch is added. While the amlogic,t7-peripherals-clkc bindings have been merged upstream, the corresponding device tree (DT) that uses these bindings has not been merged yet. As a result, there are no real-world users or systems that would be broken by this change. So could I update the commit message like: The mpll3 clock is one parent clock of sd_emmc and mipi_isp clock on the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. Add the mpll3 clock source for the T7 peripherals clock controller. so that sd_emmc and mipi_isp can use it. For logical consistency, place the required mpll3 entry before the optional entry. This change will break the ABI, but while the amlogic,t7-peripherals-clkc bindings have been merged upstream, the corresponding DT has not been merged yet. Thus, no real users or systems are broken by this change. > Best regards, > Krzysztof
On Thu, 05 Mar 2026 15:43:26 +0800, Jian Hu wrote: > The mpll3 clock is a valid parent clock for sd_emmc and mipi_isp on > the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. > Add it to enable proper clock parent configuration for these peripherals. > > Signed-off-by: Jian Hu <jian.hu@amlogic.com> > --- > .../bindings/clock/amlogic,t7-peripherals-clkc.yaml | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: Error: Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.example.dts:40.26-27 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [scripts/Makefile.dtbs:140: Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1597: dt_binding_check] Error 2 make: *** [Makefile:248: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.kernel.org/project/devicetree/patch/20260305074328.639993-3-jian.hu@amlogic.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Hi, rob Thanks for your review. On 3/5/2026 9:45 PM, Rob Herring (Arm) wrote: > [ EXTERNAL EMAIL ] > > On Thu, 05 Mar 2026 15:43:26 +0800, Jian Hu wrote: >> The mpll3 clock is a valid parent clock for sd_emmc and mipi_isp on >> the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. >> Add it to enable proper clock parent configuration for these peripherals. >> >> Signed-off-by: Jian Hu <jian.hu@amlogic.com> >> --- >> .../bindings/clock/amlogic,t7-peripherals-clkc.yaml | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Error: Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.example.dts:40.26-27 syntax error > FATAL ERROR: Unable to parse input tree > make[2]: *** [scripts/Makefile.dtbs:140: Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.example.dtb] Error 1 > make[2]: *** Waiting for unfinished jobs.... > make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1597: dt_binding_check] Error 2 > make: *** [Makefile:248: __sub-make] Error 2 I will fix the DTS syntax issue in Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.example.dts (line 40) and include this fix in the v2 patch, along with the previously discussed updated commit description. > doc reference errors (make refcheckdocs): > > See https://patchwork.kernel.org/project/devicetree/patch/20260305074328.639993-3-jian.hu@amlogic.com > > The base for the series is generally the latest rc1. A different dependency > should be noted in *this* patch. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit after running the above command yourself. Note > that DT_SCHEMA_FILES can be set to your schema file to speed up checking > your schema. However, it must be unset to test all examples with your schema. >
On jeu. 05 mars 2026 at 15:43, Jian Hu <jian.hu@amlogic.com> wrote: > The mpll3 clock is a valid parent clock for sd_emmc and mipi_isp on > the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. > Add it to enable proper clock parent configuration for these peripherals. ... but this changes the index of the clocks after this mpll3, and those index are supposed to be stable if I'm not mistaken. It is indeed more convenient to have the optional clocks at the end as it avoids writing multiple <0> in DT when we do not have them. At the very least, your commit description should say that this change will not break any existing DT because these bindings are not used yet. I leave it to the DT folks to say if the change is OK in such case. > > Signed-off-by: Jian Hu <jian.hu@amlogic.com> > --- > .../bindings/clock/amlogic,t7-peripherals-clkc.yaml | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml > index 55bb73707d58..27cc1f331587 100644 > --- a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml > +++ b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml > @@ -24,7 +24,7 @@ properties: > const: 1 > > clocks: > - minItems: 14 > + minItems: 15 > items: > - description: input oscillator > - description: input sys clk > @@ -40,12 +40,13 @@ properties: > - description: input gp1 pll > - description: input mpll1 > - description: input mpll2 > + - description: input mpll3 > - description: external input rmii oscillator (optional) > - description: input video pll0 (optional) > - description: external pad input for rtc (optional) > > clock-names: > - minItems: 14 > + minItems: 15 > items: > - const: xtal > - const: sys > @@ -61,6 +62,7 @@ properties: > - const: gp1 > - const: mpll1 > - const: mpll2 > + - const: mpll3 > - const: ext_rmii > - const: vid_pll0 > - const: ext_rtc > @@ -98,6 +100,7 @@ examples: > <&gp1 1>, > <&mpll 4>, > <&mpll 6>; > + <&mpll 8>; > clock-names = "xtal", > "sys", > "fix", > @@ -112,5 +115,6 @@ examples: > "gp1", > "mpll1", > "mpll2"; > + "mpll3"; > }; > }; -- Jerome
On Thu, Mar 05, 2026 at 10:03:32AM +0100, Jerome Brunet wrote: > On jeu. 05 mars 2026 at 15:43, Jian Hu <jian.hu@amlogic.com> wrote: > > > The mpll3 clock is a valid parent clock for sd_emmc and mipi_isp on > > the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. > > Add it to enable proper clock parent configuration for these peripherals. > > ... but this changes the index of the clocks after this mpll3, and those > index are supposed to be stable if I'm not mistaken. > > It is indeed more convenient to have the optional clocks at the end > as it avoids writing multiple <0> in DT when we do not have them. > > At the very least, your commit description should say that this change > will not break any existing DT because these bindings are not used yet. > > I leave it to the DT folks to say if the change is OK in such case. Based on commit msg it is not OK, that's why we ask about explaining true problem and actual impact, IOW whether this did not work in the first place and authors did not bother to test it... Best regards, Krzysztof
On 3/6/2026 4:14 PM, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On Thu, Mar 05, 2026 at 10:03:32AM +0100, Jerome Brunet wrote: >> On jeu. 05 mars 2026 at 15:43, Jian Hu <jian.hu@amlogic.com> wrote: >> >>> The mpll3 clock is a valid parent clock for sd_emmc and mipi_isp on >>> the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. >>> Add it to enable proper clock parent configuration for these peripherals. >> ... but this changes the index of the clocks after this mpll3, and those >> index are supposed to be stable if I'm not mistaken. >> >> It is indeed more convenient to have the optional clocks at the end >> as it avoids writing multiple <0> in DT when we do not have them. >> >> At the very least, your commit description should say that this change >> will not break any existing DT because these bindings are not used yet. >> >> I leave it to the DT folks to say if the change is OK in such case. > Based on commit msg it is not OK, that's why we ask about explaining > true problem and actual impact, IOW whether this did not work in the > first place and authors did not bother to test it... Thank you for the clarification and explanation. mpll3 is one of the clock sources for sd_emmc, and this use case was indeed not verified in the initial version. mpll3 is typically used by the audio module and is one of the required clock sources for the audio clock driver. In practice, sd_emmc does not use mpll3. During testing, only the other clock sources of sd_emmc were verified: fdivx, gp0, hifi. I apologize for this oversight. Even though sd_emmc does not use mpll3, the clock driver should still support it as a valid clock source. Additionally, I have confirmed that all required clock sources are defined in the T7 peripheral device tree. I will update the commit message to state that this change does not affect the ABI. > Best regards, > Krzysztof >
On 3/5/2026 5:03 PM, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > On jeu. 05 mars 2026 at 15:43, Jian Hu <jian.hu@amlogic.com> wrote: > >> The mpll3 clock is a valid parent clock for sd_emmc and mipi_isp on >> the Amlogic T7 SoC, but was missing from t7-peripherals-clkc.yaml. >> Add it to enable proper clock parent configuration for these peripherals. > ... but this changes the index of the clocks after this mpll3, and those > index are supposed to be stable if I'm not mistaken. > > It is indeed more convenient to have the optional clocks at the end > as it avoids writing multiple <0> in DT when we do not have them. > > At the very least, your commit description should say that this change > will not break any existing DT because these bindings are not used yet. > > I leave it to the DT folks to say if the change is OK in such case. You are right. I will add a commit description to explain this. The clock controller node for amlogic,t7-peripherals-clkc has not been merged upstream yet. This change modifies the clock index order, but it will not break any existing device tree since the amlogic,t7-peripherals-clkc bindings are not used by any upstream or downstream DT yet. I will send a v2 with an updated commit message. >> Signed-off-by: Jian Hu <jian.hu@amlogic.com> >> --- >> .../bindings/clock/amlogic,t7-peripherals-clkc.yaml | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >> index 55bb73707d58..27cc1f331587 100644 >> --- a/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >> +++ b/Documentation/devicetree/bindings/clock/amlogic,t7-peripherals-clkc.yaml >> @@ -24,7 +24,7 @@ properties: >> const: 1 >> >> clocks: >> - minItems: 14 >> + minItems: 15 >> items: >> - description: input oscillator >> - description: input sys clk >> @@ -40,12 +40,13 @@ properties: >> - description: input gp1 pll >> - description: input mpll1 >> - description: input mpll2 >> + - description: input mpll3 >> - description: external input rmii oscillator (optional) >> - description: input video pll0 (optional) >> - description: external pad input for rtc (optional) >> >> clock-names: >> - minItems: 14 >> + minItems: 15 >> items: >> - const: xtal >> - const: sys >> @@ -61,6 +62,7 @@ properties: >> - const: gp1 >> - const: mpll1 >> - const: mpll2 >> + - const: mpll3 >> - const: ext_rmii >> - const: vid_pll0 >> - const: ext_rtc >> @@ -98,6 +100,7 @@ examples: >> <&gp1 1>, >> <&mpll 4>, >> <&mpll 6>; >> + <&mpll 8>; >> clock-names = "xtal", >> "sys", >> "fix", >> @@ -112,5 +115,6 @@ examples: >> "gp1", >> "mpll1", >> "mpll2"; >> + "mpll3"; >> }; >> }; > -- > Jerome
© 2016 - 2026 Red Hat, Inc.