Skip to content

clk: adi: sc846: allow CCF unused clock cleanup + clock tree corrections - #3506

Open
qasim-ijaz wants to merge 2 commits into
adsp-6.18.31-yfrom
sc846-clk-fix
Open

clk: adi: sc846: allow CCF unused clock cleanup + clock tree corrections#3506
qasim-ijaz wants to merge 2 commits into
adsp-6.18.31-yfrom
sc846-clk-fix

Conversation

@qasim-ijaz

@qasim-ijaz qasim-ijaz commented Aug 20, 2026

Copy link
Copy Markdown

PR Description

The SC846 SoCs currently rely on the clk_ignore_unused kernel command line parameter to prevent unused clocks from being disabled (mainly as a bring up related workaround). These fixes attempt to correct the clock driver so the system can boot without that parameter + it introduces a clock tree fix to correct the parent clocks of DCLK1_x and CCLK2_x.

The following unused clocks are disabled by CCF as they have no consumers:

xspi1, sclk1_1, sclk0_1, oclk_1, lp, dclk_1, spdif,
sclk1_0, can, oclk_0, pwm, mshc, trace, and xspi0.

Related issue: #3476

PR Type

  • Bug fix (a change that fixes an issue)
  • New feature (a change that adds new functionality)
  • Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

  • I have conducted a self-review of my own code changes
  • I have compiled my changes, including the documentation
  • I have tested the changes on the relevant hardware
  • I have updated the documentation outside this repo accordingly
  • I have provided links for the relevant upstream lore

The DDR CDU gate must remain enabled while the system is running.
This was previously masked by the clk_ignore_unused kernel parameter,
which prevented unused clocks from being disabled.

Mark the DDR CDU gate as CLK_IS_CRITICAL so that it remains enabled
when unused clock cleanup is allowed to run.

Signed-off-by: Qasim Ijaz <Qasim.Ijaz@analog.com>
CCLK2_x and DCLK1_x are derived directly from their respective
CGU VCOs and divide their parent clocks by 3 and 5, respectively.
They are currently incorrectly parented to the corresponding
pllclk_half clocks.

Correct the parent clocks of CCLK2_x and DCLK1_x to use the
right CGU VCOs as specified by the HRM.

Signed-off-by: Qasim Ijaz <Qasim.Ijaz@analog.com>
@qasim-ijaz qasim-ijaz added this to ADSP Aug 20, 2026
@qasim-ijaz qasim-ijaz moved this to In Development in ADSP Aug 20, 2026
@qasim-ijaz qasim-ijaz self-assigned this Aug 20, 2026
@qasim-ijaz
qasim-ijaz requested review from a team August 20, 2026 17:24
@qasim-ijaz qasim-ijaz changed the title clk: adi: sc846: allow CCF unused clock cleanup + general clock driver cleanups clk: adi: sc846: allow CCF unused clock cleanup + clock tree corrections Aug 20, 2026
@qasim-ijaz
qasim-ijaz requested a review from pamolloy August 20, 2026 17:30
@ozan956 ozan956 added the llm review Request a review from a LLM Reviewer label Aug 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

LLM review

This series fixes two bugs introduced with the ADI SC846 clock driver: an incorrect CLK_IS_CRITICAL gap on the DDR CDU gate and a wrong parent clock for CCLK2_x/DCLK1_x.

run: 32446712956

daeef84669f2 - mark DDR CDU gate clock as critical

Marks cdu_ddr as CLK_IS_CRITICAL, consistent with how sharc0/arm0/arm1 are already handled in the same file. arch/arm64/boot/dts/adi/sc846.dtsi has no consumer node referencing ADSP_SC846_CLK_CDU_DDR, so nothing in the kernel otherwise keeps this clock enabled - this matches the commit's claim that the issue was only hidden by clk_ignore_unused.

One thing worth double-checking: none of the sibling drivers for the same CGU/CDU IP (clk-adi-sc598.c, clk-adi-sc594.c, clk-adi-sc58x.c, clk-adi-sc57x.c) mark their equivalent cdu_ddr gate critical - they all pass flag 0. Not necessarily a bug (could be legitimately SC846-specific), but a sanity check that the DDR clock domain really has no other vote on SC846 would be good.

196423f7637c - fix CCLK2_x and DCLK1_x parent clocks

Re-parents cclk2_0/cclk2_1 and dclk1_0/dclk1_1 from cguX_pllclk_half to cguX_vco. Since pllclk_half == vco/4, the old parenting produced frequencies 4x too low for these two clocks. Cross-checked against the already-upstream drivers/clk/adi/clk-adi-sc598.c, which parents its cclk2_0/cclk2_1 directly to cguX_vco with the same divide-by-3 factor - confirming the corrected topology matches the established silicon-family pattern. No public SC846 datasheet/HRM exists yet to independently confirm the DCLK1 (divide-by-5) side, so that part relies on the commit's stated HRM reference.

CI warnings

No actionable CI warnings for this range; the only entry in the job log is an informational notice about the upstream mirror target (adsp-6.18.31-y). checkpatch.pl reports no style issues for either commit.

Verification data

No SC846 HRM/datasheet is publicly listed in the ADI PDF sitemap (checked en-pdf-sitemap.xml for sc84*/sc57*/sc58*/sc59* entries). Verification was instead done by comparing the CGU/CDU topology against the already-merged sibling drivers (clk-adi-sc598.c, clk-adi-sc594.c, clk-adi-sc58x.c, clk-adi-sc57x.c) for the same IP family, and by building the full arm64 kernel + dtbs with sc846-som-ezkit_defconfig (set_arch gcc_aarch64, make -j$(nproc), make dtbs) - both build cleanly with these commits applied.

Suggested patches

No fixups are proposed; both commits are correct, minimal, and verified against sibling drivers and a full build/checkpatch run.

@sipraga sipraga left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also mentioned something about a CCLK (?) getting disabled and causing a system hang, but I don't see a fix for that here. What was that?

Comment thread drivers/clk/adi/clk-adi-sc846.c
CLK_SET_RATE_PARENT, 1, 3);
clks[ADSP_SC846_CLK_CGU1_DCLK1_1] =
clk_register_fixed_factor(NULL, "dclk1_1", "cgu1_pllclk_half",
clk_register_fixed_factor(NULL, "dclk1_1", "cgu1_vco",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagram is unnecessarily confusing in the HRM haha, I see why it was like this to begin with

@qasim-ijaz qasim-ijaz Aug 21, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, while this driver was being written during initial bring up I thought the parent clocks were pllclk_half since I was going off the diagram itself. However calculating the rate for a CCLK2_x clock like CCLK2_1 is roughly: VCO rate * (1/3), and the VCO rate for CGU1 is 3600000000 so the CCLK2_1 rate should be 1200000000 (1.2GHz) but the current CCF clk dump shows 300000000, which is wrong. My suspicions were further confirmed because the DDR test files Arturs shared a few days ago (he shared them when you asked what the ARM core rate was in the chat you might remember) also say CCLK2_1's rate is 1200 Mhz aka 1.2GHz (my changes fix all this ofc)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also explains what I saw when trying to work out the nominal ARM clock frequency.

@qasim-ijaz

qasim-ijaz commented Aug 21, 2026

Copy link
Copy Markdown
Author

You also mentioned something about a CCLK (?) getting disabled and causing a system hang, but I don't see a fix for that here. What was that?

Yes so it turns out setting the ARM to clock via the "N/A" field in the CDU causes it to act as if it was clocked by CCLK0_1 (Im not sure if its actually clocked by that but im highly suspicious of it), I ran an experiment where I ensured ARM was clocked by "N/A" (which it already is) and then I disabled CCLK0_1 in both U-Boot and Linux and the system crashes/freezes on both.

I then ran a second experiment where I reparented the ARM to a non-N/A field and then I tried to disable CCLK0_1 again and this time nothing happened. SO it seems N/A might be connected to CCLK0_1 or is itself CCLK0_1 or something, this also explains why DGM marked this as CLK_IS_CRITICAL in their workaround fix that @pamolloy shared in chat.

Anyways the right solution going forward is not to make CCLK0_1 a critical clock (thats more of a messy workaround imo), its to just stop using N/A, this is a U-Boot related fix which I am working on right now (I think that will have to be merged first before this to avoid breaking stuff)

@qasim-ijaz

qasim-ijaz commented Aug 21, 2026

Copy link
Copy Markdown
Author

From the LLM:

One thing worth double-checking: none of the sibling drivers for the same CGU/CDU IP (clk-adi-sc598.c, clk-adi-sc594.c, clk-adi-sc58x.c, clk-adi-sc57x.c) mark their equivalent cdu_ddr gate critical - they all pass flag 0. Not necessarily a bug (could be legitimately SC846-specific), but a sanity check that the DDR clock domain really has no other vote on SC846 would be good.

Not sure this is right, the SC598 marks the final DDR mux as CLK_IS_CRITICAL (and whatever parent is selects is also affected by this through prepare/enable counts increasing), the SC58X also marks the DDR CDU gate as critical, the SC594 also does the same, and the SC57X also does the same.

@nunojsa

nunojsa commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

The following unused clocks are disabled by CCF as they have no consumers:

xspi1, sclk1_1, sclk0_1, oclk_1, lp, dclk_1, spdif,
sclk1_0, can, oclk_0, pwm, mshc, trace, and xspi0.

I guess the above is the outcome of consumers to not really care about clocks and just assuming they are enabled. As XSPI is concerned, after we're done with the clock rework I definitely want the xspi driver to get and enable it's clock (and honor spi_max_speed on the flash node which means clk_set_rate() should work as expected).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llm review Request a review from a LLM Reviewer

Projects

Status: In Development

Development

Successfully merging this pull request may close these issues.

4 participants