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

siwx917: Add clock driver #77

Merged
merged 6 commits into from
Jan 15, 2025

Conversation

jerome-pouiller
Copy link
Contributor

This driver is mostly the initial seed for further implementation of a
real clock driver.

Currently, only the DMA driver is able to properly manage the clock
source. The other drivers need to be patched in the upstream. For
these ones the clock driver still contains the series of #ifdef to
enable the clocks.

For nos the clock driver doesn't allow the user to choose the clock
source for the various peripherals. The driver hardcodes some sane
values.

@jerome-pouiller jerome-pouiller marked this pull request as ready for review December 20, 2024 16:29
@jerome-pouiller jerome-pouiller requested a review from a team as a code owner December 20, 2024 16:29
@jhedberg jhedberg changed the title Add clock driver siwx917: Add clock driver Dec 20, 2024
return 0;
}

static const struct clock_control_driver_api siwx917_clock_api = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this should be using DEVICE_API() like here:
https://github.com/zephyrproject-rtos/zephyr/blob/59c94db6f052c9d848b4cec2008152d32d8ee597/drivers/clock_control/clock_control_nrf.c#L695-L700

It's a fairly new convention, so if we're not pointing at a recent enough upstream yet, then that needs to be fixed first. Most likely the same update will need to be made to any other of our downstream 917 drivers.

help
Enable clock management on Silicon Labs SiWx917 chips. This driver
includes support for HP (High Performace), ULP (Ultra Low Power), and
ULP VBAT locks.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/locks/clocks/


switch (clockid) {
case SIWX917_CLK_ULP_UART:
*rate = 32000000;
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is a minimal driver, but would it make sense to use RSI_CLK_GetBaseClock(periph) from clock_update.c in the HAL already now, rather than hard-coding frequencies? This would force us to consider how we translate between clock_control_subsys_t and HAL PERI_CLKS_T already now.

It unfortunately seems like RSI_CLK_PeripheralClkDisable() doesn't take the same PERI_CLKS_T, it takes a different enum PERIPHERALS_CLK_T combined with a pointer to the M4CLK_Type HW block... My immediate thought is that we should make the Zephyr subsys ID into something that allows us to extract 4 things: the HW subsys (M4, ULP, UULP), the power gate, the enable/disable enum (PERIPHERALS_CLK_T/ULPPERIPHERALS_CLK_T) and the PERI_CLKS_T enum. We will need these 4 pieces of data if we want to reduce hard-coding in the future.

Feel free to punt this and do it as a later improvement, I just dislike the hard-coding of frequencies here when the HAL already keeps track of it for us.

@jerome-pouiller jerome-pouiller force-pushed the add-clock-driver branch 2 times, most recently from 87d45e9 to ea03f67 Compare January 8, 2025 16:24
@jerome-pouiller
Copy link
Contributor Author

v2:

  • rebase
  • import and use RSI_CLK_GetBaseClock()
  • fix typo in Kconfig

Note I have not introduced DEVICE_API(). I prefer to do this change in a specific PR.

@jhedberg
Copy link
Collaborator

@jerome-pouiller please update west.yml to point at the actual commit (I just merged the corresponding hal_silabs PR)

This patch tries to unify the name the devices on siwx917:
  - Currently, devices from the Ultra Low Power (ULP) domain are
    sometime numbered sometime not. Since there is at most one instance
    of each device in the ULP domain, just drop the numbering.
  - Keep numbering for device outside of ULP. The numbering sometime
    start with 0, sometime with 1. We keep this weirdness, since it is
    used in the reference manual.

Signed-off-by: Jérôme Pouiller <[email protected]>
This patch tries to unify the name the devices on siwx917:
  - All the devices in the Ultra Low Power are prefixed with ULP.
  - Currently, devices from the Ultra Low Power (ULP) domain are
    sometime numbered sometime not. Since there is at most 1 instance of
    each device in the ULP domain, just drop the numbering.

Note, we still use a reference to "udma1" in the linker file since this
reference is shared with WiseConnect.

Signed-off-by: Jérôme Pouiller <[email protected]>
This patch tries to unify the name the devices on siwx917. We tends to
name the node with their role (UART, DMA, ...) rather than with the name
of the hardware block (UDMA). So, rename "udma0" in "dma0".

Note, we still use a reference to `udma0` in the linker file since this
reference is shared with WiseConnect.

Signed-off-by: Jérôme Pouiller <[email protected]>
clock_update.c provides RSI_CLK_GetBaseClock().

Signed-off-by: Jérôme Pouiller <[email protected]>
This driver is mostly the initial seed for further implementation of a
real clock driver.

Currently, sinc ethe driver do not enable the clock sources, this driver
still contains the series of #ifdef to enable the clocks.

It also doesn't allow the user to choose the clock source for the
various peripherals. The driver hardcodes some sane values.

Signed-off-by: Jérôme Pouiller <[email protected]>
SiWx917 DMA is now able to enable the clock when required.

Signed-off-by: Jérôme Pouiller <[email protected]>
@jerome-pouiller
Copy link
Contributor Author

Done

@jhedberg jhedberg merged commit a80b4ac into SiliconLabsSoftware:main Jan 15, 2025
3 checks passed
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.

3 participants