Port Zephyr to Intel S1000

[rgundi]

This patchset brings Zephyr support to Intel S1000. The GPIO, UART, I2C, DMA and I2S drivers along with interrupt controllers are currently supported.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@970b0a9). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #6842   +/-   ##
=========================================
  Coverage          ?   58.62%           
=========================================
  Files             ?      464           
  Lines             ?    47445           
  Branches          ?     8790           
=========================================
  Hits              ?    27815           
  Misses            ?    15797           
  Partials          ?     3833

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 970b0a9...6c29aba. Read the comment docs.

[mbolivar]

Please combine these into a single commit. The older shell script-based way of working is not supported anymore.

[rgundi]

Done.

[andyross]

Some random whining but nothing that I think should block merge by itself.

I do note the lack of asm2 integration. Was that not working out with xcc? Any particular problems?

[andyross]

Why not just use main() for this thread? This just looks like a waste of memory to me.

[rgundi]

This is the way it has been done for all the tests for peripherals in S1000. Just keeps "main" clean and it's easier to add and remove tests (via CMakeLists.txt).

[andyross]

This patch should be a test in the USB subsystem, or else a sample. I don't see anything board-specific here at all. Also needs a YAML descriptor and ztest integration if it lives in tests/.

[rgundi]

YAML descriptor and ztest integration will happen in a future patchset.

[andyross]

Isn't this missing a ton of stuff? I'm seeing multiple drivers in this pull requests that aren't represented here in the hardware description.

[rgundi]

This is not fully complete. We haven't finished migrating all drivers to DTS. This will also happen separately driver by driver in future patchsets.

[andyross]

This was floating around a while back, and I didn't understand it then either. Which component on Xtensa needs "__start" to be identical with the reset vector? I mean, this isn't wrong. I just don't see why we should care what the arch names its boot vectors.

Is the problem that some flashing tool is assuming that ResetVector and the ELF start vector are the same or something?

[rgundi]

Well.. on S1000, the "make debug" and "make flash" commands download the ELF file on to the beginning of SRAM (address is 0xBE000000). Upon Power on, ROM code gets executed and jumps to SRAM location 0xBE000000. So, the first piece of code that we want to be executed in SRAM needs to be placed at 0xBE000000. Since there is already a different __start in the Xtensa code, that was getting placed at that location thereby causing issues. Hence _ResetVector had to be renamed to __start to ensure it gets included in the right area.

[andyross]

FWIW: it might be a good idea to split the initialization code like this into a macro library and a per-SoC or per-board implementation. This stuff is always going to be very closely tied to the behavior of whatever bootloader coexists with Zephyr, which is going to be vendor-specific. Note that esp32 doesn't use this at all and relies on its own startup code.

Obviously we want to identify the root cause here too. I'm just saying that this kind of "hack" is going to happen again and again as this code gets applied to more hardware. Making the change this way doesn't scale, even if it was perfectly understood.

[rgundi]

I understand your point but i am unsure how to implement it. Can we take this up later?

[andyross]

Three-letter struct names are likely to collide and don't belong in drivers. Maybe "i2s_cavs_ssp" instead?

[rgundi]

sure. will make the change.

[andyross]

More broadly: why is this stuff in a header at all? It defines internal data that is only used by or included in one other file, and exposes no APIs for use by other parts of the system. Just put this in the C file.

[rgundi]

This is more of a personal choice, i feel. I wanted to keep some things in the header file to keep the size of the source file reasonable. Also, i wasn't sure if the things that i put in the header file would not be used anywhere else (except the corresponding .c). That said, i can merge the .c an .h files if you insist.

[andyross]

Style; doing this with a struct literal allows the compiler to generate better code and is more readable:

blk_cfg = {
   .block_size = blk_size,
   .source_address = (u32_t)src,
   .dest_address = (u32_t)dst,
}

[rgundi]

sure. will make the change.

[andyross]

So... start_dma() doesn't enable DMA? Is that racy? Seems like this needs a comment explaining why this is OK. Maybe "start_dma()" should be renamed "setup_dma()"?

[rgundi]

Well.. here the DMA is controlled by the peripheral. So, even if the DMA starts, unless the peripheral enables the DMA, the DMA will not complete. I can put in a comment there.

[andyross]

Any reason you can't use a k_queue for this? Among other things, I don't see any synchronization of the I2C API here, and a k_queue would give you that for free.

[rgundi]

Guess a k_queue should work here. I don't need any synchronization though. Will give the struct a try and see if things work fine.

[lpereira]

Will anything need to be done here in the future? None of these cases actually perform any operation.

[rgundi]

I initially kept it thinking i may need it for something in future. Haven't used it so far and i don't think i need it anymore. Will remove it. Will reintroduce it in future, if needed.

[lpereira]

Same here.

[rgundi]

will remove this too.

[lpereira]

Please check if channel is within bounds before taking a pointer to the chan array.

[rgundi]

ok.

[lpereira]

Same here.

[rgundi]

ok

[lpereira]

Is this port in particular capable of SMP? If so, we might want to copy the DMA callbacks to local variables before looping through blocks/tfrs.

[rgundi]

Currently this port doesn't have SMP but we may need to support it in future. DMA callbacks are already copied to local variables. Can you let me know what is not copied here?

[lpereira]

With the embedded structs here, will having a boolean in the middle create a unnecessary hole due to alignment?

[rgundi]

Well, not sure if a hole is created here. What is the size of bool in zephyr?

[lpereira]

I've checked most of these patches already, and there are just a few minor points that I'd like to have fixed before merging this.

BTW, thanks for using DT in an Xtensa port.

[SebastianBoe]

The copy-paste between this file and cmake/compiler/gcc.cmake must be resolved.

[SebastianBoe]

I would recommend organizing this as a "gcc-like" toolchain, so having COMPILER set to gcc, and modifying gcc.cmake to be generic enough to support xcc.

[rgundi]

Agree there are similarities between xcc.cmake and gcc.cmake but there are some differences as well. I prefer taking this up later. Let me know if you agree

[SebastianBoe]

No, the code must be maintainable before it goes into master.

Or else the technical debt is pushed to the next contributor to gcc.cmake, instead of where it rightly belongs.

I am aware there are differences, gcc.cmake must be made generic enough to not require the changes made.

[rgundi]

ok. i have come up with something. Please review once i push the changes.

[SebastianBoe]

This toplevel CMakeLists.txt file should not be aware of the toolchain used.

Perhaps, it would be possible to do:

set(OPTIMIZE_FOR_SIZE_FLAG "-O0")

in the toolchain file.

[rgundi]

Agreed.

[SebastianBoe]

Can it be explained why (AFAICT) the other xtensa toolchains did not require this flag, but this toolchain does?

[rgundi]

The XCC toolchain is derived from an older version of gcc and it does not understand "anonymous structs/unions". Guess it still follows the C99 standard. This flag enables that capability.

[SebastianBoe]

This should also be moved to the toolchain build script.

[rgundi]

should i employ a method similar to the one adopted for OPTIMIZE_FOR_SIZE_FLAG flag? i.e. define a new flag, something like FMS_EXTENSIONS in toolchain build script and then include it under zephyr_compile_options in the toplevel CMakeLists.txt.
Another option is to use zephyr_cc_option directly. It will fail for gcc but work for xcc. Let me know your thoughts.

[SebastianBoe]

Hi, I don't think -fms-extensions will fail for gcc actually.

I believe the cleanest solution will be to do

list(APPEND TOOLCHAIN_C_FLAGS -fms-extensions)

from the xcc toolchain build script.

[rgundi]

Well.. that was my preferred way as well in the beginning when i started providing this support. It just doesn't work. It just doesn't seem to take effect at all. That's the reason why i modified the toplevel CMakeLists.txt.

[SebastianBoe]

Right, so the reason TOOLCHAIN_C_FLAGS is taking effect on 'arm', but not on 'xtensa' is that arm is doing

zephyr_compile_options(
${TOOLCHAIN_C_FLAGS}
)

I suspect that this is undesirable, the toplevel CMakeLists.txt should be doing

zephyr_compile_options(
${TOOLCHAIN_C_FLAGS}
)

so that it is not architecture-dependent whether toolchain flags are used or not.

Anyway, for this particular PR we need to get rid of

+if(${ZEPHYR_TOOLCHAIN_VARIANT} STREQUAL "xcc")

in the toplevel CMakeLists.txt file.

This can be done by copying what 'arm' is doing, and executing

zephyr_compile_options(${TOOLCHAIN_C_FLAGS})

in arch/xtensa/CMakeLists.txt

or by adding TOOLCHAIN_C_FLAGS in the toplevel CMakeLists.txt and removing it from arch/{arm,posix}.

[rgundi]

Thanks for the info. I have modified the toplevel CMakeLists.txt and it is working fine.

[SebastianBoe]

Does the toolchain not support this flag?
Or does it use a different flag under another name?

If it is not supported then I would think the if statement was not necessary because
zephyr_ld_options is supposed to test if a flag is supported by the toolchain and omit it if not.

[rgundi]

xcc does not support this flag.

I know that zephyr_ld_options is supposed to test and omit a flag if it is not supported but i saw a few failures initially and was not really sure of the impact. I had then decided to omit it for XCC. Will revert this change now.

[SebastianBoe]

It should be possible to omit the if statement.

[rgundi]

ok

[SebastianBoe]

What the CMake code was originally expressing here was correct.

If the user has enabled STACK_CANARIES, but the toolchain does not support -fstack-protector-all, then the build system should not silently drop the protection.

[rgundi]

ok. Will revert it. Thanks for the explanation.

[SebastianBoe]

It should be possible to omit the if statement.

[rgundi]

ok

[dbkinder]

The title underline must be at least as long as the title, so add some more #######. Also, spelling should be Intel

[dbkinder]

processor chip

[dbkinder]

add a blank line before this sublist

[dbkinder]

add a blank line before this sublist too

[dbkinder]

blank line before this sublist (and fix the occurances below too)

[dbkinder]

Saying "We use" is an unexpected shift. Maybe instead, say, "We recommend using" here.

[dbkinder]

Delete the "please":

Connect the J8 header and UART chip as shown below:

[dbkinder]

Instead:

Use ``minicom`` or another terminal emulator to monitor the UART data
by following these steps:

[dbkinder]

Instead:

For debugging, you can use a flyswatter2 to connect to the S1000 CRB.

[dbkinder]

If you intended for these links to show up in the generated output, add this directive before them:

.. target-notes::

and a blank line.

[SavinayDharmappa]

@galak could you review dts support for xtensa ?

[rgundi]

@lpereira , @mbolivar : I have incorporated your suggestions. Please review.

[nashif]

recheck