Skip to content

arch: Add support to imx7d m4 core and colibri_imx7d_m4 board #6367

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

Merged
merged 4 commits into from
Apr 11, 2018

Conversation

diegosueiro
Copy link
Contributor

board/colibri_imx7d_m4: added board support to toradex colibri imx7d single board computer

This change basically adds support of a popular IMX7D processors from NXP, which has a ARM Cortex A7 Core and a Cortex M4 Core, this change is targeted to the second one and now is possible to select it as a supported SoC, Toradex has some attractive single board computers with industrial grade using this processor and this change also the basic support of the board and the NXP supplied HAL from FreeRTOS.

This support is intended to increase the options of heterogeneous processors in Zephyr SoC portfolio, adds the SoC and Board template is the first step only, so we divided the change in two steps, first provide a functional SoC support to perform build and image generation to be flashed into differtent RAMs (imx7d m4 core does not has built in flash) and the serial drivers and the next step is the IPM driver to communicate m4 core with a7 core, this approach was chosen to keep the changes not too large and hard to verify.

This changed was validated as follows:

  • build the hello world sample under build folder with: $cmake -DBOARD=colibri_imx7d_m4 .. and make.

  • the generated image can be loaded through a Jlink probe or with A7 running Linux + uBoot using the fatload command

More details can e found in the boards/arm/colibri_imx7d_m4/doc/colibri_imx7d_m4.rst file.

This is a rework of the PR #5330
Signed-off-by: Diego Sueiro [email protected]

@diegosueiro diegosueiro changed the title Add support to imx7d m4 core and colibri_imx7d_m4 board arch: Add support to imx7d m4 core and colibri_imx7d_m4 board Feb 25, 2018
@codecov-io
Copy link

codecov-io commented Feb 25, 2018

Codecov Report

Merging #6367 into master will increase coverage by 2.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6367      +/-   ##
==========================================
+ Coverage   55.63%   57.88%   +2.24%     
==========================================
  Files         462      491      +29     
  Lines       44567    52404    +7837     
  Branches     8296     9829    +1533     
==========================================
+ Hits        24795    30332    +5537     
- Misses      16380    17883    +1503     
- Partials     3392     4189     +797
Impacted Files Coverage Δ
subsys/net/ip/ipv4.c 73.8% <0%> (-7.53%) ⬇️
include/net/net_pkt.h 83.24% <0%> (-2.39%) ⬇️
kernel/init.c 79.2% <0%> (-1.5%) ⬇️
drivers/ethernet/eth_native_posix.c 27.27% <0%> (-0.73%) ⬇️
include/crc16.h 100% <0%> (ø) ⬆️
tests/kernel/tickless/tickless/src/main.c 94% <0%> (ø)
...nel/threads/custom_data/custom_data_api/src/main.c 95.23% <0%> (ø)
tests/kernel/context/src/main.c 82.17% <0%> (ø)
tests/net/dhcpv4/src/main.c 13.75% <0%> (ø)
tests/net/net_pkt/src/main.c 66.25% <0%> (ø)
... and 51 more

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 dbb2264...f3cdf58. Read the comment docs.

@carlescufi
Copy link
Member

@diegosueiro #3065 and #5960 FYI

@diegosueiro
Copy link
Contributor Author

@carlescufi,

@diegosueiro #3065 and #5960 FYI

Thanks for the tip.


The i.MX7 SoC is a Hybrid multi-core processor composed by Single/Dual Cortex A7
core and Single Cortex M4 core.
The Zephry was ported to run on the M4 core and the final goal is to have
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to:

Zephyr was ported to run on the M4 core.  In a later release, it will also communicate
with the A7 core (running Linux) via RPmsg.

===================

The Colibri iMX7D Computer on Module with Colibri Evaluation Board
was tested with following pinmux controller configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

with the following

System Clock
============

The M4 Core is configured to run at 200 MHz clock.
Copy link
Contributor

Choose a reason for hiding this comment

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

at a 200 MHz clock speed.

Serial Port
===========

The iMX7D SoC has seven UARTs. Two is configured for the console and the
Copy link
Contributor

Choose a reason for hiding this comment

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

Two are configured


The Colibri iMX7D doesn't have QSPI flash for the M4 and it needs to be started by
the A7 core. The A7 core is responsible to load the M4 binary application into the
RAM, put the M4 in reset, set the M4 Program Counter and Stack Pointer and get the
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comma:

and Stack Pointer, and get the



Get the SP and PC from firmware binary: ``./get-pc-sp.sh zephyr.bin``
.. code-block:: console
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, blank line and indentation

Plug in the J-Link into the board and PC and run the J-Link command line tool:

.. code-block:: console
/usr/bin/JLinkExe -device Cortex-M4 -if JTAG -speed 4000 -autoconnect 1 -jtagconf -1,-1 -jlinkscriptfile iMX7D_Connect_CortexM4.JLinkScript
Copy link
Contributor

Choose a reason for hiding this comment

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

and here: blank line and indentation

/usr/bin/JLinkExe -device Cortex-M4 -if JTAG -speed 4000 -autoconnect 1 -jtagconf -1,-1 -jlinkscriptfile iMX7D_Connect_CortexM4.JLinkScript

The following steps are necessary to run the zephyr.bin:
1 Put the M4 core in reset
Copy link
Contributor

Choose a reason for hiding this comment

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

for a numbered list, add a blank line before the list, and use a period after the number:

The following steps are necessary to run the zephyr.bin:

1. Put the M4 core in reset
2. Load the binary ...

Issue the following commands inside J-Link commander:

.. code-block:: console
w4 0x3039000C 0xAC
Copy link
Contributor

Choose a reason for hiding this comment

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

blank line and indenting

- `J-Link iMX7D Instructions`_


.. zephyr-app-commands::
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems out of place? Does it below above in the debugging section?

@galak galak requested a review from MaureenHelm February 26, 2018 15:16
@galak galak added area: ARM ARM (32-bit) Architecture platform: NXP NXP labels Feb 26, 2018
@diegosueiro diegosueiro requested a review from nashif as a code owner February 26, 2018 17:51
@diegosueiro
Copy link
Contributor Author

@dbkinder,

Thanks for the documentation review. I already addressed your comments.

Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

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

Thanks @diegosueiro for picking this back up!

Please split up the changes into separate commits:

  1. Import the nxp freertos bsp
  2. Add the imx uart driver shim
  3. Add the imx7d m4 soc
  4. Add the colibri_imx7d_m4 board

cc: @MarekNovakNXP @PetrLukas @stanislav-poboril

int
default 200000000

config RDC_IMX
Copy link
Member

Choose a reason for hiding this comment

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

New configs shouldn't be introduced in a defconfig file. It looks like you're using it to compile in ext/hal/nxp/imx/drivers/rdc_semaphore.c, but are you using anything from this file yet?

* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without modification,
* are permitted provided that the following conditions are met:
Copy link
Member

Choose a reason for hiding this comment

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

This file and anything else from the nxp freertos bsp needs to go into ext/

@@ -0,0 +1,42 @@
iMX7D Port
Copy link
Member

Choose a reason for hiding this comment

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

Move this file to ext/hal/nxp/imx/README

Dependencies:
This source code depends on headers and sources from zephyr:
arch/arm/core/
include/arch/arm/
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these paths are right. Unless I'm forgetting something, the only dependency should be on ext/hal/cmsis

@@ -6,6 +6,7 @@ zephyr_sources_if_kconfig(uart_gecko.c)
zephyr_sources_if_kconfig(uart_mcux.c)
zephyr_sources_if_kconfig(uart_mcux_lpuart.c)
zephyr_sources_if_kconfig(uart_mcux_lpsci.c)
zephyr_sources_if_kconfig(uart_imx.c)
Copy link
Member

Choose a reason for hiding this comment

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

Move this change up so it's (mostly) in alpha order.


static const struct uart_device_config uart_imx_dev_cfg_1 = {
.base = (uint8_t *)CONFIG_UART_IMX_UART_1_BASE_ADDRESS,
.sys_clk_freq = MHZ(100),
Copy link
Member

Choose a reason for hiding this comment

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

Why is each instance set to MHZ(100), MHZ(200), ...? Are you using sys_clk_freq anywhere?

status = "disabled";
};

uart2: uart@30890000 {
Copy link
Member

Choose a reason for hiding this comment

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

The address should be 30870000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that the right address value is 0x30890000. Looking at the i.MX7D reference manual on page 198 it mentions 0x30870000 but on page 4534 it mentions 0x30890000. I tested with 0x30870000 and couldn't get the UART working.

Copy link
Member

Choose a reason for hiding this comment

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

The cmsis header also uses 0x30890000, so it looks like you're right and there's an error in the reference manual. I'll let our docs team know.


endif # UART_IMX

config HAS_IMX_DRC
Copy link
Member

Choose a reason for hiding this comment

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

s/DRC/RDC/


zephyr_sources(
system_MCIMX7D_M4.c
startup_MCIMX7D_M4.S
Copy link
Member

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 need to build either of these. The zephyr cortex-m port already does all or most everything in startup_MCIMX7D_M4.S. The zephyr mpu driver does a lot of what's in system_MCIMX7D_M4.c, so you might just need to grab the cache init stuff and copy it to soc.c. You can do a follow up patch later to enable the existing mpu driver.





Copy link
Member

Choose a reason for hiding this comment

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

Remove the extra blank lines

Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

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

Some of the commit messages are missing a body, which will cause CI to fail.

It looks like you've merged unrelated commits. Please rebase your branch and force push to github, rather than merge.

@@ -0,0 +1,38 @@
# Kconfig - IMX M4 Core SDK
#
# Copyright (c) 2016, Freescale Semiconductor, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

Update the year and use your own copyright. If you want to assign copyright to Freescale/NXP, use the NXP copyright: Copyright (c) 2018, NXP

@@ -0,0 +1,128 @@
# Kconfig - iMX7 M4 UART
#
# Copyright (c) 2017, NXP
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as before about the copyright

select SERIAL_HAS_DRIVER
depends on HAS_IMX_HAL
help
This option enables the UART driver for NXP i.MX7
Copy link
Member

Choose a reason for hiding this comment

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

Indent with two spaces

Enable support for UART1 port in the driver. Say y here
if you want to use UART1 device.

config UART_IMX_UART_1_NAME
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the _NAME configs and get the name from DTS

((UART_Type *)(DEV_CFG(dev))->base)

/* Device data structure */
struct uart_imx_dev_data_t {
Copy link
Member

Choose a reason for hiding this comment

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

This is really a config structure, not a data structure. Can you please rename it to struct uart_imx_dev_config

uart_init_config_t initConfig = {
.baudRate = DEV_DATA(dev)->baud_rate,
.wordLength = uartWordLength8Bits,
.stopBitNum = uartStopBitNumOne,
Copy link
Member

Choose a reason for hiding this comment

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

Please fix the alignment

SYS_INIT(colibri_imx7d_init, PRE_KERNEL_1, 0);
/*******************************************************************************
* EOF
******************************************************************************/
Copy link
Member

Choose a reason for hiding this comment

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

Remove this

dbg_uart_init();
#endif /* CONFIG_UART_IMX_CONSOLE_DEBUG */

return 0;
Copy link
Member

Choose a reason for hiding this comment

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

Fix alignment

#define BOARD_I2C_IRQ_NUM I2C4_IRQn
#define BOARD_I2C_HANDLER I2C4_Handler
#define BOARD_I2C_FXAS21002_ADDR (0x20)
#define BOARD_I2C_FXOS8700_ADDR (0x1E)
Copy link
Member

Choose a reason for hiding this comment

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

What are all the macros in this file for? Most look like things we can get from dts

* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without modification,
* are permitted provided that the following conditions are met:
Copy link
Member

Choose a reason for hiding this comment

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

This file and its counterparts need to go in ext/ if we're going to use them

@diegosueiro diegosueiro force-pushed the imx7d_support branch 3 times, most recently from 1372e61 to 2ea121a Compare March 1, 2018 08:07
@diegosueiro
Copy link
Contributor Author

@MaureenHelm,

Any further feedback on this review?

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

In the commit that you import the SDK for imx - you need to update the commit message, take a look at the bottom of https://github.com/zephyrproject-rtos/zephyr/blob/master/CONTRIBUTING.rst to see examples of the format of the commit message.

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

In the import of the SDK can you maintain the dir struct of utilities/inc & utilies/src

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Can you split out the CMakeLists.txt & KConfig additions to the hal/sdk import into their own patch. The changes that add support for building a driver should be with that driver patch. For example in ext/hal/nxp/imx/drivers/CMakeLists.txt:

  • zephyr_sources_ifdef(CONFIG_UART_IMX uart_imx.c)

Should be part of the UART driver patch.

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Various code cleanups.

reg = <0x30860000 0x10000>;
interrupts = <26 0>;
interrupt-names = "status";
zephyr,irq-prio = <3>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

zephyr,irq-prio shouldnt be used, the priority value is the second cell in the interrupt prop. So remove this and have:

interrupts = <26 3>;


#ifdef CONFIG_UART_IMX_UART_2
/* We need to grasp board uart exclusively */
RDC_SetPdapAccess(RDC, rdcPdapUart2, 3 << (BOARD_DOMAIN_ID * 2), false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

need to cleanup magic number 3, probably a comment or something about why (BOARD_DOMAIN_ID * 2)

static void nxp_mcimx7_uart_config(void)
{

#ifdef CONFIG_UART_IMX_UART_2
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about all the other UARTs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment I just tested the UART_2. As soon as I test the other UARTs I'll add them.

zephyr_include_directories(.)

zephyr_sources(clock_freq.c)
zephyr_sources_ifdef(CONFIG_GPIO_IMX gpio_imx.c)
Copy link
Collaborator

Choose a reason for hiding this comment

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

CONFIG_GPIO_IMX doesnt seem to exist anywhere.

compatible = "nxp,imx-uart";
reg = <0x30860000 0x10000>;
interrupts = <26 0>;
interrupt-names = "status";
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove names, since there is only one interrupt line, its not terribly useful.

@@ -0,0 +1,18 @@
# Kconfig - Colibri iMX7D M4 board
Copy link
Collaborator

Choose a reason for hiding this comment

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

should Kconfig.new exist?

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Do we need debug console support at all? Since we have a proper uart driver and printf/scanf support in Zephyr?

Is there something else that DbgConsole is providing?

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Various comments related to memory regions.

What memory uses cases really make sense to support? Seems like the default should be running out of TCM.

label = "DDR CODE";

clock-controller;
#clocks-cells = <3>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does ddr_code have a clock-controller; and #clock-cells. They should probalby be removed (same for all the various memory nodes).

#clocks-cells = <3>;
};

ocram_pxp_code: code@00940000 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it really make sense to have pxp & edpc used for memory/code storage for Zephyr? It seems like TCM would be the default, and exposing OCRAM_S, OCRAM, and DDR would be more than enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As soon as pxp and edpc can be used for this purpose I thought that should be a good idea to have them exposed as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not opposed, but assume that PXP & EDPC are meant to normally be used by the various graphics devices/drivers that they are associated with. Also, I'd assume the performance of going across various SoC busses to those memory is going to be higher compared to TCM. It just feels rare that PXP or EDPC memories would be what one would choose, but someone could tell me otherwise.

galak
galak previously requested changes Mar 9, 2018
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Various code cleanups and questions.

#include "ccm_imx7d.h"
#include "clock_freq.h"

#define NO_DOMAIN_PERM 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about the following:

#define RDC_DOMAIN_PERM_NONE  (0x0)
#define RDC_DOMAIN_PERM_W     (0x1)
#define RDC_DOMAIN_PERM_R     (0x2)
#define RDC_DOMAIN_PERM_RW    (RDC_DOMAIN_PERM_W|RDC_DOMAIN_PERM_R)

#define RDC_DOMAIN_PERM(domain, perm) (perm << (domain * 2))

@@ -0,0 +1,48 @@
#define CONFIG_UART_IMX_UART_1_NAME NXP_IMX_UART_30860000_LABEL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like you need a fixup for prio bits:

#define CONFIG_NUM_IRQ_PRIO_BITS                ARM_V7M_NVIC_E000E100_ARM_NUM_IRQ_PRIORITY_BITS

};

&nvic {
arm,num-irqs = <127>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove arm,num-irqs - its not used, or defined anywhere.

select CPU_CORTEX_M4
select SOC_FAMILY_IMX
select CPU_HAS_SYSTICK
select CLOCK_CONTROL
Copy link
Collaborator

Choose a reason for hiding this comment

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

select CPU_HAS_FPU

config SOC_SERIES
default mcimx7_m4

config NUM_IRQ_PRIO_BITS
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove NUM_IRQ_PRIO_BITS, this should come from DT

#include "wdog_imx.h"
#include "board.h"

#define NO_DOMAIN_PERM 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, are these not also in soc.h?

@@ -0,0 +1,154 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any point importing utilities/ since I'm not sure if we'd ever use debug_console.

return 0;
}

SYS_INIT(colibri_imx7d_init, PRE_KERNEL_1, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this doesnt nothing, just remove board.c

/* The board name */
#define BOARD_NAME "IMX7_COLIBRI_M4"
#define BOARD_DOMAIN_ID (1)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need all these defines?


/* The board name */
#define BOARD_NAME "IMX7_COLIBRI_M4"
#define BOARD_DOMAIN_ID (1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think DOMAIN_ID should be a CONFIG param in the SoC and default to 1. Than board code can change the default in Kconfig.defconfig if desired.

@diegosueiro
Copy link
Contributor Author

@galak,

Any further feedback?


zephyr_sources(clock_freq.c)
zephyr_sources_ifdef(CONFIG_CLOCK_CONTROL_IMX_CCM ccm_imx7d.c)
zephyr_sources_ifdef(CONFIG_CLOCK_CONTROL_IMX_CCM ccm_analog_imx7d.c)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be also constrained by CONFIG_SOC_MCIMX7_M4, as CONFIG_SOC_MCIMX6X_M4 would add ccm_imx6sx.c and ccm_analog_imx6sx.c files instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

@@ -0,0 +1,243 @@
/*
Copy link
Collaborator

@stanislav-poboril stanislav-poboril Mar 14, 2018

Choose a reason for hiding this comment

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

The files clock_freq.h/c are coming from examples folder in FreeRTOS_BSP_1.0.1_iMX7D and the content of those files is different in FreeRTOS_BSP_1.0.1_iMX6SX. It should be renamed or moved, maybe into ext/hal/nxp/imx/devices/MCIMX7D/ folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think that the functions in this module can be the same for i.MX7 and i.MX6SX?
If so, it doesn't make sense to move those files to ext/hal/nxp/imx/devices/MCIMX7D/

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't looked at the two set of files in much depth yet, but theirs diff is quite large.
The purpose of the files (getting frequency of SoC's particular subsystems) is similar to what can be found for example in ext\hal\nxp\mcux\devices\MIMXRT1052\fsl_clock.c, which is device specific.
So I am afraid there will have to be versions of clock_freq.h/c for each SoC based on FreeRTOS BSP (KSDK 1.x).

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'll move them to ext/hal/nxp/imx/devices/MCIMX7D/.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It also looks like the proper Zephyr way is to create abstraction of the device specific clock code (in drivers/clock_control) and use it instead of accessing the external API directly in driver shims. We may consider adding clock_control module for iMX6SX/7D in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. My idea is to have this basic support upstreamed and than implement other drivers (GPIO, I2C etc) and frameworks (clock_control, pinmux etc).

@@ -0,0 +1,73 @@
# Kconfig - iMX7 M4 UART
Copy link
Collaborator

Choose a reason for hiding this comment

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

iMX6SX uses the same driver in FreeRTOS_BSP_1.0.1_iMX6SX. Not sure whether to change the iMX7 mentions to something generic. Or I can mention the other SoC once I add its support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion, all the generalization should be put in place when a new platform is going to be added.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok.

*/

#include <arm/armv7-m.dtsi>
#include <dt-bindings/clock/imx_ccm.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks unused. The header is related to drivers/clock_control/clock_control_mcux_ccm.c, which has some minimum implementation to support iMX RT1050 (which uses ext/hal/nxp/mcux). The UART shim driver uses function from clock_freq.c.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix this.

@@ -0,0 +1,146 @@
/*
Copy link
Collaborator

@stanislav-poboril stanislav-poboril Mar 14, 2018

Choose a reason for hiding this comment

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

I think the file dts/arm/nxp/nxp_imx.dtsi should be renamed to something more specific, for example dts/arm/nxp/nxp_imx7d_m4.dtsi.

(Now that we have multiple iMX SoC's, maybe also dts/arm/nxp/nxp_rt.dtsi could be renamed into something more specific, but it is not part of this pull request).

Copy link
Collaborator

Choose a reason for hiding this comment

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

agreed, this should be imx7d-m4.dtsi.


/dts-v1/;

#include <nxp/nxp_imx.dtsi>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please adjust the include after rename.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed it in my local repo and for some reason the "boards/arm: add support for colibri_imx7d_m4 board " commit was not pushed to my zephry repo fork.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I forgot the last "git rebase --continue".

#include <nxp/nxp_imx.dtsi>

/ {
model = "TORADEX Colibir IMX7D board";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo - Colibir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

endif # SERIAL

config DOMAIN_ID
int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please replace spaces by tab on this and the following line (checkpatch script fails on this).

@diegosueiro
Copy link
Contributor Author

@MaureenHelm and @galak,

What are the next steps? Is there something else that I need to do?

@MaureenHelm
Copy link
Member

Hi @diegosueiro , can you please fix the CI failures? I looked at a few and they're overflowing the flash and ram sections. You can fix this by adding sizes to boards/arm/colibri_imx7d_m4/colibri_imx7d_m4.yaml, which will filter out CI tests that don't fit. See boards/arm/frdm_kl25z/frdm_kl25z.yaml for an example.

This code component is used to add Zephyr support on iMX7 processors,
exclusively on Cortex M4 core, and to speed up the development process
it was decided to have it based on NXP FreeRTOS BSP implementation.

The source code was imported from the following folders:
    FreeRTOS_BSP_1.0.1_iMX7D/platform/drivers
    FreeRTOS_BSP_1.0.1_iMX7D/platform/devices

This source code depends on headers and sources from zephyr:
    ext/hal/cmsis

Origin: iMX7D NXP FreeRTOS BSP Peripheral Driver
License: BSD 3-Clause
URL: https://www.nxp.com/webapp/Download?colCode=FreeRTOS_iMX7D_1.0.1_LINUX&appType=license
commit: no commit hash
Purpose: The peripheral driver wraps the H/W for i.MX7 M4 core
Maintained-by: External

Signed-off-by: Diego Sueiro <[email protected]>
Adds a shim layer around the imx uart driver to adapt it to the Zephyr
serial interface.

Modem mode was introduce to control it as DCE and DTE and can be
configured in the device tree:
    modem-mode:
        type: int
        category: required
        description: Set the UART Port to modem mode 0 (dce) 1 (dte)
        generation: define

For now only the UART 2 was tested.

Signed-off-by: Diego Sueiro <[email protected]>
The i.MX7 SoC is a Hybrid multi-core processor composed by Single/Dual
Cortex A7 core and Single Cortex M4 core.

Zephyr was ported to run on the M4 core. In a later release, it will
also communicate with the A7 core (running Linux) via RPmsg.

The low level drivers come from NXP FreeRTOS BSP and are located at
ext/hal/nxp/imx. More details can be found at ext/hal/nxp/imx/README

The A7 core is responsible to load the M4 binary application into the
RAM, put the M4 in reset, set the M4 Program Counter and Stack Pointer,
and get the M4 out of reset.
The A7 can perform these steps at bootloader level after the Linux
system has booted.

The M4 can use up to 5 different RAMs. These are the memory mapping for
A7 and M4:

+---------------+-----------------+---------------------------+
| Memory Name   | Start Address   | Size                      |
+===============+=================+===========================+
| TCML          | 0x007F8000      | 32KB                      |
+---------------+-----------------+---------------------------+
| TCMU          | 0x20000000      | 32KB                      |
+---------------+-----------------+---------------------------+
| OCRAM_S       | 0x20180000      | 32KB                      |
+---------------+-----------------+---------------------------+
| OCRAM         | 0x00900000      | 128KB                     |
+---------------+-----------------+---------------------------+
| DDR           | 0x10000000      | 256MB                     |
+---------------+-----------------+---------------------------+

Signed-off-by: Diego Sueiro <[email protected]>
The Colibri iMX7D Computer on Module with Colibri Evaluation Board
configuration supports the following hardware features on the Cortex M4
Core:

+-----------+------------+-------------------------------------+
| Interface | Controller | Driver/Component                    |
+===========+============+=====================================+
| NVIC      | on-chip    | nested vector interrupt controller  |
+-----------+------------+-------------------------------------+
| SYSTICK   | on-chip    | systick                             |
+-----------+------------+-------------------------------------+
| UART      | on-chip    | serial port-polling;                |
|           |            | serial port-interrupt               |
+-----------+------------+-------------------------------------+

The default configuration can be found in the defconfig file:
boards/arm/colibri_imx7d_m4/colibri_imx7d_m4_defconfig

Other hardware features are not currently supported by the port.

Connections and IOs:

The Colibri iMX7D Computer on Module with Colibri Evaluation Board
was tested with the following pinmux controller configuration.

+---------------+-----------------+---------------------------+
| Board Name    | SoC Name        | Usage                     |
+===============+=================+===========================+
| UART_B RXD    | UART2_TXD       | UART Console              |
+---------------+-----------------+---------------------------+
| UART_B TXD    | UART2_RXD       | UART Console              |
+---------------+-----------------+---------------------------+

Signed-off-by: Diego Sueiro <[email protected]>
@diegosueiro
Copy link
Contributor Author

@MaureenHelm,

I added the memories sizes but the CI is still falling. It seems that it is not filtering by the flash size restrictions.

@MaureenHelm
Copy link
Member

I added the memories sizes but the CI is still falling. It seems that it is not filtering by the flash size restrictions.

@diegosueiro The failing tests need to have the min_flash attribute added.

@diegosueiro
Copy link
Contributor Author

diegosueiro commented Apr 9, 2018

@MaureenHelm,

@diegosueiro The failing tests need to have the min_flash attribute added.

Where should I add this? According to the documentation, min_flash and min_ram go into the tests yaml files. Is that correct?

Should I create another PR to add the min_flash for those tests?

@galak
Copy link
Collaborator

galak commented Apr 9, 2018

Where should I add this? According to the documentation, min_flash and min_ram go into the tests yaml files. Is that correct?

Should I create another PR to add the min_flash for those tests?

Its added in testcase.yaml. For example you can look at tests/crypto/ecc_dh/testcase.yaml which already has min_ram entry. For example of min_flash, take a look at tests/crypto/mbedtls/testcase.yaml.

Yes, submit the adjustments to the testcases in a separate PR.

@diegosueiro
Copy link
Contributor Author

@galak,

How do I re-run the sanitycheck for this PR as soon as the PR #7008 was merged?

@MaureenHelm
Copy link
Member

recheck

@MaureenHelm MaureenHelm dismissed galak’s stale review April 11, 2018 06:26

requested changes have been addressed

@MaureenHelm MaureenHelm merged commit 85e8eaa into zephyrproject-rtos:master Apr 11, 2018
@diegosueiro diegosueiro deleted the imx7d_support branch July 19, 2018 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARM ARM (32-bit) Architecture platform: NXP NXP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants