Skip to content
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

Merge in sp24 drivers/platforms #54

Open
wants to merge 56 commits into
base: main
Choose a base branch
from
Open

Conversation

naichenzhao
Copy link

No description provided.

@naichenzhao naichenzhao marked this pull request as ready for review February 14, 2025 03:46
@naichenzhao
Copy link
Author

@Fi50 @doihead @jasmangle REVIEW ME PLZ :3

@jasmangle
Copy link

jasmangle commented Feb 25, 2025

Reviewed, looks mostly good to me. There should be RCC drivers included with this as well for Bearly24 and DSP24 to replace the current clock selection struct and allow for tile reset control, but I can copy those over fairly quickly (will need to test this tomorrow, but I'll be in the lab for a bit so I should be able to). The clock select struct was made hastily as a quick fix approach to getting a simple PLL startup and shouldn't be coupled with the Intel driver, since the clock selection ultimately happens through PRCI.

Will merge ucb-bar#5 when I've tested it.

@@ -20,22 +35,55 @@ void pwm_stop(PWM_Type *PWMx, uint32_t idx) {

void pwm_set_frequency(PWM_Type *PWMx, uint32_t idx, uint32_t freq) {
// TODO: implementation
Copy link

Choose a reason for hiding this comment

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

Should we maybe remove all the TODO: implementation except where we actually need it else becomes confusing

Copy link

Choose a reason for hiding this comment

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

Would be nice to add a header comment regarding the extent PWM was tested but is kind of a cherry on top >_<

Copy link

Choose a reason for hiding this comment

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

Would be nice to clean up + comment a bit more but eh

Copy link

Choose a reason for hiding this comment

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

Same here, would be nice to comment a bit more I think but eh

reg_write8(DMA_BASE, 0);
}

void enable_Crack(){
Copy link

Choose a reason for hiding this comment

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

If we're gonna upstream this.......... should we change it >_>

Copy link

Choose a reason for hiding this comment

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

Or maybe it's just the labchip cfg that would be nice to comment on Idk

@Fi50
Copy link

Fi50 commented Feb 27, 2025

Yeah I didn't analyze too deeply, but left some small suggestions, I think once the PR is ready we should also pass it by @T-K-233 before merging into ucb-bar. I don't see us changing anything that could break a flow for anyone tho.
Agreed that might be best to replace the struct from driver/intel/pll/pll.h.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants