sel4 / util_libs Goto Github PK

Hi all,

Contrary to what is explained in util_libs/libethdrivers, the inclusion of imx6 platform definitions (util_libs/libethdrivers/src/plat/imx6/uboot/imx-regs.h) is creating compilation errors.

The problem occurs while compiling camkes example picotcp_single_component for imx8mq-evk.
Is the imx8mq-evk ethernet port really supported? Is there a missing commit in the main tree to fix this?
EthDriver_imx8mq.txt

From my understanding, it seems that imx-regs.h from uboot should be selectively loaded for imx6 or for imx8m*. Definitions seem really different and should certainly be stored in separated folders, I mean util_libs/libethdrivers/src/plat/imx6 and util_libs/libethdrivers/src/plat/imx8m.

Is it a current action on roadmap? What do you think about it?

GitLint action fails with obscure message about broken tool link at https://github.com/seL4/util_libs/runs/1923196863?check_suite_focus=true

13s
Run seL4/ci-actions/gitlint@master
  with:
    action_name: gitlint
/home/runner/work/_actions/seL4/ci-actions/master/js/../gitlint/steps.sh
Setting up
  Installing python 3.7
  update-alternatives: error: alternative path /usr/bin/python3.7 doesn't exist
  update-alternatives: error: alternative path /usr/bin/python3.7 doesn't exist
  update-alternatives: error: no alternatives for python3
  Python 3.8.5
  ERROR: launchpadlib 1.10.13 requires testresources, which is not installed.
  Installing gitlint tool
  Cloning https://github.com/seL4/util_libs.git@refs/pull/65/merge
  Fetching master
  Cloning https://github.com/seL4/sel4_tools.git
  Using default sel4_tools/misc/.gitlint

Checking the following commits:
fatal: Invalid revision range master..944aa3f2b3139fa3a522fa3b9ebd8066287cf3d5
Error: Action gitlint failed.

Any hint, what this error could be and where the commit ID 944aa3f2... comes from?

https://github.com/seL4/util_libs/blob/master/libethdrivers/src/lwip.c uses dma_alloc_pin() with cached=1. With the recent changes in seL4/camkes-tool@ac567a1 that make CAmkES honor the caching flag, this is broken now - there is no cached DMA pool available, just an uncached on.

Quick fix is using cached=0 everywhere, as DMA in ARM/RISC-V us usually uncached. Things might be different on x86, I'm not really an expert there... Aside from this, it seems the LWIP version here lost active maintenance some tiume back and it's unclear how well this is working anyway.

I have a CAmkES project for which I want to use SerialServer in order to multiplex access to a serial console. The hardware in use is a Zynq7000.

When running the default serial_server applications (I've tried both polling and interrupt-based), the console output is garbled after the system boots:

## Starting application at 0x006f9000 ...

ELF-loader started on CPU: ARM Ltd. Cortex-A9 r3p0
  paddr=[6f9000..aa4ab3]
No DTB passed in from boot loader.
Looking for DTB in CPIO archive...found at 7fb1b4.
Loaded DTB from 7fb1b4.
   paddr=[3c000..3efff]
ELF-loading image 'kernel'
  paddr=[0..3bfff]
  vaddr=[e0000000..e003bfff]
  virt_entry=e0000000
ELF-loading image 'capdl-loader'
  paddr=[3f000..307fff]
  vaddr=[10000..2d8fff]
  virt_entry=18794
Enabling MMU and paging
Bootstrapping kernel
Booting all finished, dropped to user space
LJdd�䄄E��Ƈ�D�$Db

Typing leads to more garbage displayed on screen. This makes me suspect it's that an error occurs when setting the baud rate in libplatsupport/src/mach/zynq/serial.c. Indeed, applying this patch fixes the issue:

diff --git a/libplatsupport/src/mach/zynq/serial.c b/libplatsupport/src/mach/zynq/serial.c
index 15e47b0..7780b6d 100644
--- a/libplatsupport/src/mach/zynq/serial.c
+++ b/libplatsupport/src/mach/zynq/serial.c
@@ -321,31 +321,31 @@ static void zynq_uart_set_baud(
 
     zynq_uart_calc_baud_divs(UART_REF_CLK, bps, &div8, &cd, &bdiv);
 
-    /* Disable the Rx path */
-    regs->cr &= ~UART_CR_RXEN;
+    ///* Disable the Rx path */
+    //regs->cr &= ~UART_CR_RXEN;
 
-    /* Disable the Tx path */
-    regs->cr &= ~UART_CR_TXEN;
+    ///* Disable the Tx path */
+    //regs->cr &= ~UART_CR_TXEN;
 
-    /* Apply the calculated values */
-    if (div8) {
-        regs->mr |= UART_MR_CLKS;
-    } else {
-        regs->mr &= ~UART_MR_CLKS;
-    }
+    ///* Apply the calculated values */
+    //if (div8) {
+    //    regs->mr |= UART_MR_CLKS;
+    //} else {
+    //    regs->mr &= ~UART_MR_CLKS;
+    //}
 
-    regs->baudgen = cd;
-    regs->bauddiv = bdiv;
+    //regs->baudgen = cd;
+    //regs->bauddiv = bdiv;
 
-    /* Reset the Tx and Rx paths */
-    regs->cr |= UART_CR_TXRES | UART_CR_RXRES;
-    while (regs->cr & (UART_CR_TXRES | UART_CR_RXRES));
+    ///* Reset the Tx and Rx paths */
+    //regs->cr |= UART_CR_TXRES | UART_CR_RXRES;
+    //while (regs->cr & (UART_CR_TXRES | UART_CR_RXRES));
 
-    /* Enable the Rx path */
-    zynq_uart_enable_rx(regs);
+    ///* Enable the Rx path */
+    //zynq_uart_enable_rx(regs);
 
-    /* Enable the Tx path */
-    zynq_uart_enable_tx(regs);
+    ///* Enable the Tx path */
+    //zynq_uart_enable_tx(regs);
 }
 
 int serial_configure(

Recompiling the application makes it work as expected (u-boot initializes the console before seL4 runs):

## Starting application at 0x006f9000 ...

ELF-loader started on CPU: ARM Ltd. Cortex-A9 r3p0
  paddr=[6f9000..aa4ab3]
No DTB passed in from boot loader.
Looking for DTB in CPIO archive...found at 7fb1b4.
Loaded DTB from 7fb1b4.
   paddr=[3c000..3efff]
ELF-loading image 'kernel'
  paddr=[0..3bfff]
  vaddr=[e0000000..e003bfff]
  virt_entry=e0000000
ELF-loading image 'capdl-loader'
  paddr=[3f000..307fff]
  vaddr=[10000..2d8fff]
  virt_entry=18794
Enabling MMU and paging
Bootstrapping kernel
Booting all finished, dropped to user space
interruptcli: Hello Serial Server!
interruptcli: hello
interruptcli: test
interruptcli: this is a test

The baud rate calculated by zynq_uart_calc_baud_divs is 115207. I don't know a lot about UART consoles, but I think that 115207 is close enough that it should still work and display sensible output.

As far as I know we don't set any non-standard clock values anywhere.

    imx_ltimer_t *imx_ltimer = data;
...
    ps_free(&imx_ltimer->ops.malloc_ops, sizeof(imx_ltimer), imx_ltimer);

I think this is freeing something of the wrong size. If I'm not mistaken sizeof(imx_ltimer) is the size of the pointer not the size of the struct. I think it should be either sizeof(imx_ltimer_t) or (preferably) sizeof *imx_ltimer.

#117 brings support for the Quartz64 board. It Seems the RK3566 is very similar to the
RK3399 used on the Rockpro64 board, so the the driver code should be unified instead of copy/paste duplicating. As a side effect, this would fix the CR/LF issues from #48 then also.

This is loosely related to RasPi3/RasPi4 driver code unification issue #61 and it would not be a surprise if there are more platforms that run into this. There is the src/mach folder where such driver could end up as an intermediate step. However, mach is considered deprecated as it does not scale too well. Having dedicates driver names makes more sense, so platform can pick what they need. In the end that is how the device tree also works.

On systems where there are PCI or PCIe buses not accessible via a bridge from bus 0, libpci does not find them.

It should allow for multiple root complexes, and set up routing for PCI buses attached other than to the first.

The NULL pointer check in gpt_init should be moved prior to the use of the pointer.

The chardev_map is used by many serial drivers in the platform code for mapping the serial registers into virtual memory.

Given the extensive use of the function it should really be documented.

It takes as a parameter a struct dev_defn, which has a size field.

It is somewhat implied (or at least it was to me from usage!) that size could be an arbitrary power-of-2 size.

However, that turns out to not be the case! It must be page-sized. Passing a non-page sized value results in a very difficult to debug error occurring (and when you are trying to map the UART registers you don't have any debug output!).

Could this function be please be documented to make it clear that only page-size sizes may be passed and/or could the chardev_map function perform that check early. That would make manually tracing the error much easier, rather than it failing about 12 functions deep otherwise.

In the allocate_register_callback function there is an assert:

    assert(curr_num == 0);

This does not seem appropriate. I'd suggest that this should return an error (in the same way that if ps_pmem_map fails an error is returned). The function has no reasonable basis on which to assert curr_num as the FDT contents are well outside the control. (By contrast the assertion assert(token != NULL) seems reasonable as this is controlled by the call to ps_fdt_walk_registers which is in the same module).

I'm not sure what the most appropriate return value would be, or what if this affects other areas as well.

flycast uses libelf.

see:
https://github.com/flyinghead/flycast/blob/master/core/deps/libelf/elf.cpp

In that file, it states that it is released under this license:

 * Australian Public Licence B (OZPLB)
 * 
 * Version 1-0
 * 
 * Copyright (c) 2004 University of New South Wales
 * 
 * All rights reserved. 
 * 
 * Developed by: Operating Systems and Distributed Systems Group (DiSy)
 *               University of New South Wales
 *               http://www.disy.cse.unsw.edu.au
 * 

• Is this the same project as the one hosted here at seL4/util_libs?
• Has it been relicensed from Australian Public Licence B to BSD 2-clause?

https://github.com/seL4/util_libs/blob/master/libelf/src/elf.c

/*
 * Copyright (c) 1999-2004 University of New South Wales
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

Fix missing licenses from the Odroid C2 network driver that was brought in with ae833d2 from @nomadeel , as the GIT action check complains now for every branch:

# MISSING LICENSES

'GPL-2.0' found in:
* libethdrivers/src/plat/odroidc2/uboot/net.h

'GPL-2.0+' found in:
* libethdrivers/src/plat/odroidc2/uboot/designware.c
* libethdrivers/src/plat/odroidc2/uboot/designware.h
* libethdrivers/src/plat/odroidc2/uboot/netdev.h
* libethdrivers/src/plat/odroidc2/uboot/phy.c
* libethdrivers/src/plat/odroidc2/uboot/phy.h
* libethdrivers/src/plat/odroidc2/uboot/realtek.c


# MISSING COPYRIGHT AND LICENSING INFORMATION

The following files have no copyright information:
* libethdrivers/src/plat/odroidc2/uboot/bitops.h
* libethdrivers/src/plat/odroidc2/uboot/config.h
* libethdrivers/src/plat/odroidc2/uboot/err.h
* libethdrivers/src/plat/odroidc2/uboot/list.h
* libethdrivers/src/plat/odroidc2/uboot/system.h


# SUMMARY

* Bad licenses:
* Deprecated licenses:
* Licenses without file extension:
* Missing licenses: GPL-2.0, GPL-2.0+
* Unused licenses:
* Used licenses: BSD-1-Clause, BSD-2-Clause, BSD-3-Clause, CC-BY-SA-4.0, GPL-2.0-only, GPL-2.0-or-later, IBM-pibs, MIT
* Read errors: 0
* Files with copyright information: 678 / 683
* Files with license information: 683 / 683

Unfortunately, your project is not compliant with version 3.0 of the REUSE Specification :-(
Error: Action license-check failed.

ps_fdt_read_path does not clearly document when the state of the cookie and initialization in cases when an error is returned.

This has led to some call-sites checking the error and then calling ps_fdt_cleanup_cookie, and other call-sites not calling ps_fdt_cleanup_cookie.

From examination of the implementation is is clear that ps_fdt_cleanup_cookie should not be called in the cases where an error is returned. So the code that does do this cleanup (such as imx/gpt.c) should be fixed to omit the call to cleanup.
The current behaviour is such that we end up with a double free, which is very difficult to debug.

In addition, to fixing the existing incorrect calls to ps_fdt_cleanup_cookie I'd suggest the following improvements:

• Update documentation of ps_fdt_read_path to make it clear when ps_fdt_cleanup_cookie should be called.
• Update implementation of ps_fdt_read_path so that the cookie output variable is only populated in the case of success (or explicitly populated will NULL on the case of failure). Such a change would make it clearer to callers whether or not to call cleanup, and would hit NULL-pointer check asserts, if ps_fdt_cleanup_cookie is called incorrectly rather than ending up with a double free that just ends up crashing with no visible output.

I've found when trying to test imx6 ethernet driver on the Nitrogen6_SoloX platform that quite often, the mdio_read implementation (https://github.com/seL4/util_libs/blob/master/libethdrivers/src/plat/imx6/enet.c#L397) will start returning all ones (0xffff) until the hardware is reset. Has anyone else experienced this or know what could cause this?

There are a lot of branches that look as if the work has been merged. Could you remove them here?

• missing-lincenses
• spdx
• spdx-syntax
• alyons-ppi
• kent/fix-style
• kent/rockpro64
• kent/remove-warning
• nomadeel/fdt-fix

in libplatsupport/src/plat/imx6/gpio.c there is

struct imx6_gpio_regs {

    uint32_t int_cfg;     /* +0x0C */
    uint32_t int_mask;    /* +0x14 */
    ....

This look broken because no field for offset 0x10 exists - and I have doubts the compiler read the comments and inserts this filed. So I wonder if this code has ever been used anywhere since it was added in a1ea096

In some implementation (rockpro64) the CR is sent after the LF, while in others (such as imx) it is sent prior to the LF.

I think it should be sent prior (as in, imx is correct). Is there a reason the rockpro64 is implemented differently?

Seems branch kent/fixup-minor-warnings was merged, so it can be removed?

Seems the Raspi 3 and 4 basically use the same timer. And some other peripherals look quite identical, too. Once the RasPi4 support is merged, we should look into reducing the amount of code duplication. Instead, factor the common parts out into a shared place (e.g. libplatsupport/src/mach/bcm) like we do int with zynq or exynos

In this code:

#ifdef CONFIG_PLAT_IMX7
    /* eanble the 24MHz source and select the oscillator as CLKSRC */
    gptcr |= (BIT(EN_24M) | (5u << CLKSRC));
#else
    gptcr |= BIT(CLKSRC);
#endif

(from libplatsupport/src/mach/imx/gpt.c) the use of the BIT macro is very confusing.

The value of CLKSRC is 6, but that field is not a single bit, rather it is a 3-bit field (bits 6, 7, 8).

Although it is the same after expansion, I think:

   gpctr |= (1u << CLKSRC)

would make the intent clearer. Or even:

#define NO_CLOCK 0
#define PERIPHERAL_CLOCK 1
#define HIGH_FREQUENCY_REFERENCE_CLOCK 2
#define EXTERNAL_CLOCK 3
....
...

 qpctr |= (PERIPHERAL_CLOCK << CLKSRC)

The i.MX timer's set_timeout function converts absolute times to relative without checking that the time is in the future. This leads to an attempt to set a very large relative timeout instead of returning ETIME.

Also, non-periodic relative timeouts that are too far into the future for a 32-bit timer should saturate to 0xffffffff so that the timeout is received early, rather than returning EINVAL.

The omap version of set_timeout handles both of these cases; I think the code can simply be copied from there.

In libplatsupport/src/plat/imx8m/clock.c

clk_t *ps_clocks[];
freq_t ps_freq_default[];

Causes compiler warnings. Making them explicitly zero size:

clk_t *ps_clocks[0];
freq_t ps_freq_default[0];

Shuts up the compiler, but I'm not sure if it is really a good fix.

The code for handling serial IRQs on Exynos does not properly clear interrupt flags:

https://github.com/seL4/util_libs/blob/master/libplatsupport/src/mach/exynos/serial.c#L360

This function should do something like *REG_PTR(d->vaddr, UINTP) = INT_TX in the uart_handle_tx_irq branch and *REG_PTR(d->vaddr, UINTP) = INT_RX in the uart_handle_rx_irq branch. See section 12.6.1.13 in the Exynos 5420 data sheet.

This bug causes a terrible performance problem on Odroid-XU.

(Moved from https://sel4.atlassian.net/browse/SELFOUR-2273)
The zynqmp uses the same serial device as the zc706, but the input clk frequency is different. This results in incorrect baud rates being set.  Currently configuring the baud is disabled and we rely on previous programs to have configured the serial correctly.

This seems to be the only repository that uses the workflow and PRs aren't being merged as a queue so it's not apparently helpful.

The relevant code is here:

https://github.com/seL4/util_libs/blob/master/libplatsupport/src/mach/exynos/spi.c#L201

The comment says to use a 180 degree phase feedback, but the code sets no phase feedback. For a 180 degree phase feedback it should be v = (0x2 << FB_CLK_SEL_SHF);. The SPI for the CAN controller on the HACMS Odroid/daughterboard platform does not work correctly without a phase feedback (90, 180, or 270 would all work). Please set the phase feedback to 180 degrees or make it configurable when the SPI is initialized.

this offset of MU_IIR and MU_IER is wrong , MU_IIR shoule 0x48 , MU_IER should 0x44
https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf page 8

Follow-up from https://github.com/seL4/util_libs/pull/112/files/b6a47d9d0b656ef77601ea9d005a26409c887a8c#r766004484

We had to include this header file here to address a nasty include problem and make these prototypes available to other C files in our driver:

int enet_mdio_read(struct enet *enet, uint16_t phy, uint16_t reg);
int enet_mdio_write(struct enet *enet, uint16_t phy, uint16_t reg, uint16_t data);

The includes should be reworked, so no header form the src are made public

The exact semantics of this don't appear to be documented anywhere that I could find.

The semantics I assume is that if we ever have a timer overflow then some code will track the overflows to provide extended timer range, however while I can find implementations using it, I don't see anything that actually checks if they received and LTIMER_OVERFLOW_EVENT and doing anything about it.

I assume that LTIMER_TIMEOUT_EVENT should be used with the callback when a timeout set with ltimer_set_timeout occurs. I can't find where in the iMX timer implementations the LTIMER_TIMEOUT_EVENT is ever passed to the callback, but I may be looking in the wrong place.

A while ago I tried to get the RPi4 into the seL4 CI due to how fragile the platform is. After some fixes/work I thought it was ready to go but then it got blocked on not working in release mode (seL4/ci-actions#278).

seL4test would just hang in release mode. What seems to be causing this is the BCM serial driver as applying this patch fixes everything:

diff --git a/libplatsupport/src/mach/bcm/serial.c b/libplatsupport/src/mach/bcm/serial.c
index 0091c15..ec77a7d 100644
--- a/libplatsupport/src/mach/bcm/serial.c
+++ b/libplatsupport/src/mach/bcm/serial.c
@@ -185,7 +185,7 @@ int uart_init(const struct dev_defn *defn, const ps_io_ops_t *ops, ps_chardevice
 
     }
 
-    uart_gpio_configure(defn->id, ops);
+    // uart_gpio_configure(defn->id, ops);
 
     uart_funcs.uart_init(defn, ops, dev);

I don't know anything about GPIO and don't really have the chance to understand it right now so I don't know why this works. In fact, I don't even remember why I tried commenting out this.

Debug mode of sel4test does not make use of the actual serial driver (which seems weird in my opinion now that I think about it) which is why this issue only came up in release mode.

If someone more familiar with GPIO could take a look at this that would be great.

The parameters for the set_timeout function are documented in platsupport/ltimer.h however the semantics are not very clear.

Specifically, what is the correct behaviour if called with a time that is in the past (and/or called with a time that is in the past by the time the callee completes, consider the function could be interrupted by a higher priority thread at any point in time).

In addition, the semantics associated with IRQs may come in earlier than requested due to implementation details seems difficult for a caller to use correctly. It seems like the caller would then need to call get_time or something to check. In my experience most timeout APIs guarantee to wait for at least the specified time, but don't make guarantees about how soon after the requested expiry the actual callback is made. I suggest that this might be more useful set of semantics to consider specifying instead.