Skip to content

Conversation

@VynDragon
Copy link
Contributor

This introduces clock control for bl60x.

Reason for kconfig.bflb: other soc will also be in there:

# Copyright (c) 2025 MASSDRIVER EI (massdriver.space)
# SPDX-License-Identifier: Apache-2.0

config CLOCK_CONTROL_BOUFFALOLAB_BL61X
	bool "Bouffalolab BL61x Clock Control Driver"
	default y
	depends on DT_HAS_BFLB_BL61X_CLOCK_CONTROLLER_ENABLED

config CLOCK_CONTROL_BOUFFALOLAB_BL60X
	bool "Bouffalolab BL60x Clock Control Driver"
	default y
	depends on DT_HAS_BFLB_BL60X_CLOCK_CONTROLLER_ENABLED

config CLOCK_CONTROL_BOUFFALOLAB_BL70X
	bool "Bouffalolab BL70x Clock Control Driver"
	default y
	depends on DT_HAS_BFLB_BL70X_CLOCK_CONTROLLER_ENABLED

Reason for deferred: sadly couldnt make it work in the hybrid version due to conflict between SDK init and zephyr init, this is to avoid submitting removing dependency to SDK and this at the same time and be a really big PR.

Some of the updates to other things (like syscon) will come later.

@VynDragon VynDragon requested a review from nandojve May 19, 2025 21:16
@VynDragon VynDragon force-pushed the bl60x_clock_control branch 3 times, most recently from fdfd823 to fac145a Compare May 20, 2025 00:34
@nandojve nandojve self-assigned this May 20, 2025
@VynDragon VynDragon force-pushed the bl60x_clock_control branch 2 times, most recently from 0bc6690 to 7936c17 Compare May 20, 2025 14:18
@VynDragon VynDragon marked this pull request as ready for review May 20, 2025 14:58
/* 32 Mhz Oscillator: 0
* crystal: 1
* PLL and 32M: 2
* PLL and crystal: 3
*/
Copy link
Member

Choose a reason for hiding this comment

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

o.O this looks familiar to the other PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same code. Still havent figured how to share it well between the drivers. force temporary 32m sysclk with on and off, or maybe a function like irq_lock, idk.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. maybe create a header file in include/zephyr/drivers/clock_control for these functions so that it can be reused in other drivers like the recently added syscon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, i will make a separate commit for that later to update both this and the syscon driver.

@VynDragon VynDragon force-pushed the bl60x_clock_control branch 3 times, most recently from d73161c to 94c05d6 Compare May 24, 2025 01:24
@VynDragon VynDragon force-pushed the bl60x_clock_control branch 3 times, most recently from adf1133 to 3505958 Compare May 27, 2025 11:09
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds Bouffalolab BL60x clock control, including device tree bindings, Kconfig entries, and driver registration.

  • Enable CLOCK_CONTROL for BL60x SoC and add Kconfig entry for the BL60x clock driver
  • Introduce common and BL60x-specific dt-bindings headers and YAML files
  • Update the BL60x device tree (dtsi) and register the new driver in CMakeLists

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
soc/bflb/bl60x/Kconfig Select CLOCK_CONTROL for BL60x SoC
include/zephyr/dt-bindings/clock/bflb_clock_common.h New common clock ID definitions and frequency macro
include/zephyr/dt-bindings/clock/bflb_bl60x_clock.h BL60x-specific clock ID mappings
dts/riscv/bflb/bl60x.dtsi Add fixed-clock nodes and clock-controller node
dts/bindings/clock/*.yaml New YAML bindings for controller, root, PLL, and BCLK
drivers/clock_control/Kconfig.bflb Add CLOCK_CONTROL_BOUFFALOLAB_BL60X config
drivers/clock_control/Kconfig Source the new BL60x Kconfig.bflb file
drivers/clock_control/CMakeLists.txt Register clock_control_bl60x.c under the new config
Comments suppressed due to low confidence (6)

dts/riscv/bflb/bl60x.dtsi:3

  • There are now two differing copyright headers in this file; please consolidate into a single, correct line covering both the original ATL Electronics and the new MASSDRIVER ownership.
* Copyright (c) 2024-2025 MASSDRIVER EI (massdriver.space)

dts/bindings/clock/bflb,bl60x-pll.yaml:20

  • [nitpick] The clock-cells entry 'select' is ambiguous; consider renaming it to something more descriptive (e.g., 'pll-rate' or 'output-index').
  - select

drivers/clock_control/CMakeLists.txt:57

  • No tests for the new BL60x clock controller are included; consider adding a basic unit or integration test under tests/drivers/clock_control to verify proper initialization and behavior.
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_BOUFFALOLAB_BL60X   clock_control_bl60x.c)

include/zephyr/dt-bindings/clock/bflb_clock_common.h:18

  • The MHZ macro may not be defined in this header. Consider including the common DT macros header (e.g., <zephyr/dt-bindings/dt-common.h>) or using an explicit frequency literal.
#define BFLB_RC32M_FREQUENCY MHZ(32)

include/zephyr/dt-bindings/clock/bflb_bl60x_clock.h:18

  • [nitpick] Found mixed tabs and spaces in indentation for the PLL definitions; please use spaces consistently to match the repository style.
#define BL60X_PLL_48MHz		0

dts/riscv/bflb/bl60x.dtsi:131

  • [nitpick] Entries in the phandle list wrap mid-sentence and indentation is inconsistent; align the list items vertically for better readability.
clocks = <&clk_rc32m>, <&clk_crystal>, <&clk_root>, <&clk_bclk>,

@VynDragon VynDragon force-pushed the bl60x_clock_control branch from c54f350 to 4327a7c Compare June 15, 2025 13:31
nandojve
nandojve previously approved these changes Jun 16, 2025
@nandojve nandojve requested a review from ycsin June 16, 2025 18:30
}
}
} else {
/* do nothing and make sonarqube happy */
Copy link
Member

Choose a reason for hiding this comment

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

I find only writing to the return value when important

It is kinda important here - we tried to handle the input and we can't, so the input is invalid and we set the return value accordingly

ycsin
ycsin previously approved these changes Jun 20, 2025
Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

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

some nits, LGTM in general. Hopefully later PRs will factor out some common code that can be reused between drivers.

}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

nits: I think it's better to set ret = -EINVAL; in the else case here and clock_control_bl60x_off()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default negative logic will stay.

nandojve
nandojve previously approved these changes Jun 20, 2025
@nandojve nandojve added this to the v4.2.0 milestone Jun 20, 2025
} else if (tmp == 0) {
return MHZ(48) / (hclk_div + 1);
}
return -EINVAL;
Copy link
Contributor

Choose a reason for hiding this comment

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

function returns an unsigned int...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be unreacheable code anyway, done.

}
return CLOCK_CONTROL_STATUS_OFF;
default:
return -EINVAL;
Copy link
Contributor

Choose a reason for hiding this comment

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

function returns a clock_control_status - I think you want CLOCK_CONTROL_STATUS_UNKNOWN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduced by review... Done.

@VynDragon VynDragon dismissed stale reviews from ycsin and nandojve via 8402d50 June 20, 2025 09:42
@VynDragon VynDragon force-pushed the bl60x_clock_control branch from 5e95040 to 8402d50 Compare June 20, 2025 09:42
@VynDragon VynDragon requested review from kartben, nandojve and ycsin June 20, 2025 09:43
This introduces a clock_control driver for bl60x

Signed-off-by: Camille BAUD <[email protected]>
This enables the clock_control driver build on bl60x.
It is currently deferred init, due to being incompatible
with current SDK-based boot, to avoid later giant PR.

Signed-off-by: Camille BAUD <[email protected]>
@VynDragon VynDragon force-pushed the bl60x_clock_control branch from 8402d50 to dce0078 Compare June 20, 2025 09:50
@kartben kartben dismissed their stale review June 20, 2025 09:54

addressed - thanks!

@sonarqubecloud
Copy link

@kartben kartben merged commit 8c385be into zephyrproject-rtos:main Jun 21, 2025
26 checks passed
@VynDragon VynDragon deleted the bl60x_clock_control branch June 25, 2025 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants