Skip to content

Toolchain independence #717

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

Draft
wom-bat wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Toolchain independence #717

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5078b0b
Fix multi-line comment in apb_timer/timer.c
wom-bat Apr 22, 2026
7f958e5
Remove unused variable in i2c/libi2c.c
wom-bat Apr 22, 2026
0f02696
Mark usdhc_debug as unused
wom-bat Apr 22, 2026
16062b3
Make sure things are rebuilt when they should be
wom-bat Apr 22, 2026
c42de5f
Allow blk example to build with gcc or clang
wom-bat Apr 22, 2026
1984d81
Mark unused functions unused
wom-bat May 12, 2026
15d7615
Whitespace only changes in timer.c
wom-bat May 12, 2026
1a30591
Delete unused function
wom-bat May 12, 2026
2bb2ae4
Whitespace only fixes
wom-bat May 12, 2026
385953f
Remove CLANG default toolchain
wom-bat May 25, 2026
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
318 changes: 175 additions & 143 deletions drivers/blk/mmc/imx/usdhc.c
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions drivers/blk/virtio/block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ static void virtio_blk_print_req(struct virtio_blk_req *req)
req->type, req->reserved, req->sector, req->status);
}

static void virtio_blk_print_config(volatile struct virtio_blk_config *config)
static inline void __attribute__((unused)) virtio_blk_print_config(volatile struct virtio_blk_config *config)
{
LOG_DRIVER("capacity: 0x%lx (0x%lx bytes)\n", config->capacity, config->capacity * VIRTIO_BLK_SECTOR_SIZE);
LOG_DRIVER("size_max: 0x%x\n", config->size_max);
Expand All @@ -130,7 +130,7 @@ static void virtio_blk_print_config(volatile struct virtio_blk_config *config)
LOG_DRIVER("secure_erase_sector_alignment: 0x%x\n", config->secure_erase_sector_alignment);
}

static void virtio_blk_print_features(uint64_t features)
static inline __attribute((unused)) void virtio_blk_print_features(uint64_t features)
{
if (features & ((uint64_t)1 << VIRTIO_BLK_F_SIZE_MAX)) {
LOG_DRIVER(" VIRTIO_BLK_F_SIZE_MAX\n");
Expand Down
108 changes: 66 additions & 42 deletions drivers/timer/apb_timer/timer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,37 @@

#define LOG_APBTIMER_ERR(...) do{ sddf_dprintf("APBTIMER|ERROR: "); sddf_dprintf(__VA_ARGS__); }while(0)

#define NUM_TIMERS 2 // Adjust if synthesised with more timers. \
// Minimum = 2 due to a bug in the HDL, assumed for rest of this driver.
/*
* Adjust NUM_TIMERS if synthesised with more timers.
* Minimum = 2 due to a bug in the HDL, assumed for rest
* of this driver.
*/
#define NUM_TIMERS 2
#define APBTIMER_MAX_TICKS (UINT32_MAX)
#define APBTIMER_CLK_FREQ ((uint64_t)50000000) // 50MHz
#define NANO_INVERSE NS_IN_S

// The APB timer has an array of internal timers. Use one for long-running time measurements, use
// the other for generating interrupts at finer granularity using prescalers.
// TODO: support >2 timers; probably exposed over a different API for drivers etc?
/*
* The APB timer has an array of internal timers. Use one for
* long-running time measurements, use the other for generating
* interrupts at finer granularity using prescalers.
*
* TODO: support >2 timers; probably exposed over a different API for
* drivers etc?
*/
#define TIMER_TIMEOUT (0)
// We are conservative with IRQs in config.json. We don't use the 2nd (cmp) interrupt
// for the timekeeper, and since that IRQ isn't in config.json we must make sure the timekeeper
// is the last ID to avoid accessing the wrong IRQ.
/*
* We are conservative with IRQs in config.json. We don't use the 2nd
* (cmp) interrupt for the timekeeper, and since that IRQ isn't in
* config.json we must make sure the timekeeper is the last ID to
* avoid accessing the wrong IRQ.
*/
#define TIMER_TIMEKEEPER (1)

// NOTE: at the time of writing, the APB timer's prescaler logic is completely
// broken. We use no prescaler as a result.
/*
* NOTE: at the time of writing, the APB timer's prescaler logic is
* completely broken. We use no prescaler as a result.
*/
#define TIMEKEEPER_PRESCALER (0)

#define APBTIMER_CTRL_EN_BIT (BIT(0))
Expand All @@ -52,7 +66,10 @@

__attribute__((__section__(".device_resources"))) device_resources_t device_resources;

// Timer block implements NUM_TIMERS separate timers with back-to-back registers
/*
* Timer block implements NUM_TIMERS separate timers with back-to-back
* registers
*/
struct timer_regs {
uint32_t timer;
uint32_t ctrl;
Expand All @@ -63,7 +80,8 @@ struct timer_regs {
// Array of regs structs, not just one set!
volatile struct timer_regs *regs;

/* Keep track of how many timer overflows have occured.
/*
* Keep track of how many timer overflows have occured.
* Used as the most significant segment of ticks.
* We need to keep track of this state as the value register is only
* 32 bits as opposed to the common 64 bit timer value regsiters found
Expand All @@ -80,11 +98,12 @@ typedef struct apbtimer_timeout_conf {
} apbtimer_timeout_conf_t;

/**
* Convert the tick count of a timer to nanoseconds, given the expected prescaler
* and overflow counter. Prescaler can be set to 0 to ignore.
* Convert the tick count of a timer to nanoseconds, given the
* expected prescaler and overflow counter. Prescaler can be set to 0
* to ignore.
*
* Prescaler should be given in same format as ctrl reg - i.e. 0 = disabled (multiply
* by 1), 1 = multiply by 2, etc.
* Prescaler should be given in same format as ctrl reg - i.e. 0 =
* disabled (multiply by 1), 1 = multiply by 2, etc.
*/
static inline uint64_t tick_to_ns(uint64_t ticks, uint64_t prescaler)
{
Expand All @@ -99,8 +118,8 @@ static inline uint64_t tick_to_ns(uint64_t ticks, uint64_t prescaler)

/**
* Return number of ticks since driver startup using timekeeper timer.
* NOTE: one round of timer @ 50MHz with prescaler of (1<<3)=4 lasts for 171.8
* seconds. Time resolution = 80ns per tick.
* NOTE: one round of timer @ 50MHz with prescaler of (1<<3)=4 lasts
* for 171.8 seconds. Time resolution = 80ns per tick.
*/
static uint64_t get_time_ns(void)
{
Expand All @@ -112,27 +131,32 @@ static uint64_t get_time_ns(void)
}

/**
* Calculate the cmp value and prescaler for the timeout timer given
* a desired delay in nanoseconds. If delay exceeds capacity of timer,
* Calculate the cmp value and prescaler for the timeout timer given a
* desired delay in nanoseconds. If delay exceeds capacity of timer,
* returns maximum prescaler and cmp.
*/
static apbtimer_timeout_conf_t calculate_timeout_from_ns(uint64_t ns_delay)
{
// Convert nanoseconds to ticks with a prescaler of zero (x1)
// tick = 1 timer period = 1/f_clk = T_clk
// ticks = n. periods in delay = seconds_delay / T_clk
//
// To get ticks efficiently, precalculate 1/nano (10e-9).
// Hence, T_clk = NANO_INVERSE / F_clk
// and T_delay = seconds_delay / (NANO_INVERSE/F_clk)
// uint64_t divisor = NANO_INVERSE / APBTIMER_CLK_FREQ;
// uint64_t ticks = ns_delay / divisor;
/*
* Convert nanoseconds to ticks with a prescaler of zero (x1)
* tick = 1 timer period = 1/f_clk = T_clk
* ticks = n. periods in delay = seconds_delay / T_clk
*
* To get ticks efficiently, precalculate 1/nano (10e-9).
* Hence, T_clk = NANO_INVERSE / F_clk
* and T_delay = seconds_delay / (NANO_INVERSE/F_clk)
* uint64_t divisor = NANO_INVERSE / APBTIMER_CLK_FREQ;
* uint64_t ticks = ns_delay / divisor;
*/
uint64_t ticks = (ns_delay * APBTIMER_CLK_FREQ) / NANO_INVERSE;

uint32_t prescaler = 0;
uint32_t cmp = ticks;
// NOTE: at the time of writing, the APB timer's prescaler logic is completely
// broken. The prescaler calculator is disabled as a result.
/*
* NOTE: at the time of writing, the APB timer's prescaler logic
* is completely broken. The prescaler calculator is disabled as a
* result.
*/

// if (ticks <= UINT32_MAX) {
// // No prescaler needed
Expand Down Expand Up @@ -170,9 +194,9 @@ static inline void set_timeout_prescaler(uint8_t prescaler)
}

/**
* Set up timekeeper timer for timestamping execution.
* Use maximum prescaler, disable cmp interrupts to minimise performance
* impact from timeouts.
* Set up timekeeper timer for timestamping execution. Use maximum
* prescaler, disable cmp interrupts to minimise performance impact
* from timeouts.
*/
static inline void setup_timekeeper(void)
{
Expand All @@ -197,15 +221,15 @@ static inline void timeout_set_enable(bool enable)
}

/**
* Process timeouts from the queue using the timeout timer.
* This *differs* from most other sDDF timers because we process timeouts
* on a relative basis rather than with respect to the absolute time, as such
* a method is cumbersome and inefficient with 32 bit timers.
* Process timeouts from the queue using the timeout timer. This
* *differs* from most other sDDF timers because we process timeouts
* on a relative basis rather than with respect to the absolute time,
* as such a method is cumbersome and inefficient with 32 bit timers.
*
* Timeouts are stored using absolute time upon PPC, this function converts the
* next timeout into a relative stamp from the current time and awaits it using
* the timeout timer. Automatically sets prescaler to satisfy most granular time
* resolution.
* Timeouts are stored using absolute time upon PPC, this function
* converts the next timeout into a relative stamp from the current
* time and awaits it using the timeout timer. Automatically sets
* prescaler to satisfy most granular time resolution.
*/
static void process_timeouts(void)
{
Expand Down
2 changes: 1 addition & 1 deletion examples/blk/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ REPORT_FILE := ${BUILD_DIR}/report.txt
all: ${IMAGE_FILE}

qemu ${IMAGE_FILE} ${REPORT_FILE} clean clobber: ${BUILD_DIR}/Makefile FORCE
${MAKE} -C ${BUILD_DIR} MICROKIT_SDK=${MICROKIT_SDK} $(notdir $@)
${MAKE} -C ${BUILD_DIR} -${MAKEFLAGS} $(notdir $@)

${BUILD_DIR}/Makefile: blk.mk
mkdir -p ${BUILD_DIR}
Expand Down
21 changes: 15 additions & 6 deletions examples/blk/blk.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,29 @@ SYSTEM_FILE := blk.system
SUPPORTED_BOARDS := qemu_virt_aarch64 \
qemu_virt_riscv64 \
maaxboard \
x86_64_generic
x86_64_generic

TOP := ${SDDF}/examples/blk
CONFIGS_INCLUDE := ${TOP}
SDDF_CUSTOM_LIBC := 1

# Force rebuild if partition or NVME args change
# This is probably too crude: it forces _everything_ to be rebuilt
COMMON_CONFIG += ${PARTITION} ${NVME}

include ${SDDF}/tools/make/board/common.mk


IMAGES := blk_driver.elf client.elf blk_virt.elf serial_virt_tx.elf serial_driver.elf
CFLAGS += -Wall -Wno-unused-function -Werror -Wno-unused-command-line-argument \

@midnightveil midnightveil May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can understand wanting to remove these disabled warnings but it has nothing to do with GCC vs Clang... These should be separate commits not squished in with the rest silently.

@wom-bat wom-bat May 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is GCC-versus clang ... the arguments are not available for gcc, and the warnings they suppress need to be fixed in-line in the code instead.

@midnightveil midnightveil May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

GCC doesn't support -Wno-unused-function? but it says it does: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wunused-function (and I'm pretty sure these pre-date clang-as-default).

@wom-bat wom-bat May 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

-Wunused-command-line-argument is the one it doesn't seem to accept... and if we can get rid of them, let's go ...

@midnightveil midnightveil Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we can get rid of -Wunused-command-line-argument unfortunately, there's some issues with the Nix-packaged sysroots, and a few other cases.

-I$(SDDF)/include \
-I$(SDDF)/include/microkit \
-I$(CONFIGS_INCLUDE)
IMAGES := blk_driver.elf \
client.elf \
blk_virt.elf \
serial_virt_tx.elf \
serial_driver.elf

CFLAGS += -Wall -Werror \
-I$(SDDF)/include \
-I$(SDDF)/include/microkit \
-I$(CONFIGS_INCLUDE)

LDFLAGS := -L$(BOARD_DIR)/lib
LIBS := --start-group -lmicrokit -Tmicrokit.ld libsddf_util_debug.a --end-group
Expand Down
2 changes: 0 additions & 2 deletions i2c/libi2c.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ static int __i2c_dispatch(libi2c_conf_t *conf, i2c_addr_t address, void *buf, ui

// Create header command
i2c_cmd_t header;
i2c_err_t error = I2C_ERR_OK;
header.flag_mask = I2C_FLAG_HEAD;
header.payload.i2c_header.batch_len = num_batches;
header.payload.i2c_header.address = address;
Expand Down Expand Up @@ -103,7 +102,6 @@ static int __i2c_dispatch(libi2c_conf_t *conf, i2c_addr_t address, void *buf, ui
// No need to clean up if we fail. We just surrender pending requests
// and exit.
LOG_LIBI2C_ERR("__i2c_dispatch failed to enqueue request!\n");
error = -1;
i2c_request_abort(*conf->handle);
return -1;
}
Expand Down
24 changes: 21 additions & 3 deletions tools/make/board/common.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,21 @@ endif
ifeq ($(wildcard ${SDDF}/tools/make/board/${MICROKIT_BOARD}.mk),)
$(error No Make snippet in ${SDDF}/tools/make/board for ${MICROKIT_BOARD})
endif
TOOLCHAIN ?= clang

include ${SDDF}/tools/make/board/${MICROKIT_BOARD}.mk
include ${SDDF}/tools/make/toolchain/${TOOLCHAIN}.mk

MICROKIT_TOOL ?= $(MICROKIT_SDK)/bin/microkit

# For submakes
export SUPPORTED_BOARDS
export MICROKIT_BOARD
export MICROKIT_CONFIG
export BOARD_DIR
export MICROKIT_SDK
export TOOLCHAIN

ifneq ($(ARCH),x86_64)
DTS := $(SDDF)/dts/$(MICROKIT_BOARD).dts
DTB := $(MICROKIT_BOARD).dtb
Expand All @@ -56,11 +65,20 @@ eth_driver.elf: ${ETH_DRIV}

# Magic to ensure stuff gets recompiled if we change
# board name, or use a different Microkit etc.
CHECK_FLAGS_BOARD_HASH := .board_cflags-$(shell echo -- ${CFLAGS} ${MICROKIT_SDK} ${MICROKIT_BOARD} ${MICROKIT_CONFIG} ${SMP_CONFIG} ${BENCH_PMU_EVENTS} | shasum | sed 's/ *-//g')

COMMON_CONFIG += ${TOOLCHAIN} \
${CFLAGS} \
${MICROKIT_SDK} \
${MICROKIT_BOARD} \
${MICROKIT_CONFIG} \
${SMP_CONFIG} \
${BENCH_PMU_EVENTS}

CHECK_FLAGS_BOARD_HASH := .board_cflags-$(shell echo -- ${COMMON_CONFIG} | \
shasum | sed 's/ *-//g')

${CHECK_FLAGS_BOARD_HASH}:
-rm -f .board_cflags-*
touch $@

REBUILD_CANDIDATE_OBJECTS := $(shell find . -name '*.o')
${REBUILD_CANDIDATE_OBJECTS): .EXTRA_PREREQS = ${CHECK_FLAGS_BOARD_HASH}
.EXTRA_PREREQS = ${CHECK_FLAGS_BOARD_HASH}

@midnightveil midnightveil May 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI Peter: #715. (I suspect this might be the BSD make instead of GNU make thing again? IDK)

@wom-bat wom-bat May 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could be; I will check this.

@wom-bat wom-bat May 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We use so many GNUmake idioms I'd be surprised if BSD make builds the system at all.

@midnightveil midnightveil May 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to make make error if it's not GNUmake? That would solve this as well, I think. (I do recall us decided that we basically need GNUmake: our CI needs it explicitly)

@midnightveil midnightveil May 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correction: macOS default is GNUmake 3.8, from 2006...

@midnightveil midnightveil Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So I don't know what the difference is but @0aids was reporting that $^ was including all the dependencies from this and breaking their build.

I don't know what the difference is between them.

@wom-bat wom-bat Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you mean with 'EXTRA_PREREQS' or with Courtney's patch? Either way, we're adding a prerequisite to ensure that stuff is rebuilt; and $^ is the list of all prerequisites except the ones in EXTRA_PREREQS -- Courtney's patch will add the hash file 6to the list; this patch will not.

@midnightveil midnightveil Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, Courtney's patch. But it also included all the .h files listed in the compiler emitted .d files.

What confuses me is I don't see what the difference is because the list of dependencies in the compiler-depfile would contain the headers always but why does it only sometimes appear in $^. And when we tested this with two files one included the dependencies in the other file didn't appear in $^.

@wom-bat wom-bat Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On the first run, the .d files haven't been created, so Make doesn't know about the header files. On subsequent runs it does. A prerequisite in Make terms is any file, that if newer than a target, means the target has to be rebuilt: any header file a C file includes, if changed, means that the .o target needs rebuilding. So the complete prerequisite list for a .o file will always include the C file it is built from, and the transitive closure of header files that that C file includes directly or recursively.

There is almost never a reason to use $^ in a Makefile rule;

@midnightveil midnightveil Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, a lot of our Makefile rules do...

In some ways I think we need to redo all our Makefiles to be of better quality. At the moment they have a hodegpoge of styles and techniques, but at the same time I don't know of any golden references.

7 changes: 1 addition & 6 deletions virtio/transport/mmio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,6 @@ static bool check_magic(virtio_mmio_regs_t *regs)
return regs->MagicValue == 0x74726976;
}

static bool check_device_id(virtio_mmio_regs_t *regs, virtio_device_id_t id)
{
return regs->DeviceID == id;
}

static uint32_t get_version(virtio_mmio_regs_t *regs)
{
return regs->Version;
Expand Down Expand Up @@ -137,4 +132,4 @@ void virtio_transport_write_isr(virtio_device_handle_t *device_handle, uint32_t
volatile virtio_mmio_regs_t *regs = get_regs(device_handle->device_resources);

regs->InterruptACK = isr;
}
}
Loading