Skip to content

sc8280xp: Add DDR clock #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andersson merged 1 commit into from
Apr 11, 2023
Merged

sc8280xp: Add DDR clock #18

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions sc8280xp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,14 @@ static struct debug_mux disp1_cc = {
.div_val = 4,
};

static struct debug_mux mc_cc = {
.phys = 0x90ba000,
.size = /* 0x54 */ 0x1000,
.block_name = "mc",

.measure = measure_mccc,
};

static struct measure_clk sc8280xp_clocks[] = {
{ "gcc_aggre_noc_pcie0_tunnel_axi_clk", &gcc, 0x217 },
{ "gcc_aggre_noc_pcie1_tunnel_axi_clk", &gcc, 0x218 },
Expand Down Expand Up @@ -418,6 +426,7 @@ static struct measure_clk sc8280xp_clocks[] = {
{ "disp1_cc_mdss_vsync_clk", &gcc, 0x82, &disp1_cc, 0x17 },
{ "disp1_cc_sleep_clk", &gcc, 0x82, &disp1_cc, 0x46 },
{ "disp1_cc_xo_clk", &gcc, 0x82, &disp1_cc, 0x45 },
{ "measure_only_mccc_clk", &gcc, 0xfeedbeef, &mc_cc, 0x50 },
Copy link
Contributor

@MarijnS95 MarijnS95 Apr 5, 2023

Choose a reason for hiding this comment

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

So far downstream always defined a mux value for gcc and the current code even configures this 0xfeedbeef "mux value" to gcc before calling the custom measure() function:

https://github.com/andersson/debugcc/blob/1f2d56984ec60e6ca0a18718c75c4e593542cefc/debugcc.c#L214-L217

Are you sure this is correct, since you gave absolutely zero context in the commit message whatsoever?

Copy link
Contributor

@MarijnS95 MarijnS95 Apr 11, 2023

Choose a reason for hiding this comment

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

I guess @andersson is indifferent about this too 😉

Copy link
Collaborator

@lumag lumag May 1, 2023

Choose a reason for hiding this comment

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

@MarijnS95 @konradybcio @andersson
I see that all the downstream debugcc drivers also program some logical value for the mc_cc into gcc's debug mux. Do we know the value that should be used for sc8280xp?

Copy link
Collaborator

@lumag lumag May 1, 2023

Choose a reason for hiding this comment

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

In other words, I see why it is not required now (because for reading mccc we bypass the gcc's debug mux). But there still should be some more correct value, but without the downstream driver or docs we can not deduce it.

Copy link
Contributor

@MarijnS95 MarijnS95 May 1, 2023

Choose a reason for hiding this comment

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

In this case I'd rather see a PR that allows setting measure_clk::primary to NULL to communicate this, and just set it to "measure_only_mccc_clk", NULL, 0, ... everywhere.

Copy link
Collaborator

@lumag lumag May 1, 2023

Choose a reason for hiding this comment

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

@MarijnS95 The patch should be quite easy
debugcc.txt

However I suppose that we should fix it instead. @andersson do you possibly have a more correct value?

Copy link
Collaborator

@lumag lumag May 1, 2023

Choose a reason for hiding this comment

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

@MarijnS95 done, #20 (untested)

Copy link
Contributor

@MarijnS95 MarijnS95 May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I never said it would be hard 😉

However I suppose that we should fix it instead. @andersson do you possibly have a more correct value?

That also works, but it's weird if this value is "not needed" (and GCC shouldn't / doesn't need to be touched at all).

{}
};

Expand Down