-
Notifications
You must be signed in to change notification settings - Fork 15
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.
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
Conversation
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
| { "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 }, |
There was a problem hiding this comment.
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:
Are you sure this is correct, since you gave absolutely zero context in the commit message whatsoever?
There was a problem hiding this comment.
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 😉
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarijnS95 done, #20 (untested)
There was a problem hiding this comment.
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).
No description provided.