Skip to content

Conversation

@rsalveti
Copy link
Contributor

Restructuring the Zephyr build system to make it easier to support new targets.

Also replacing printks with SYS_LOG, as it can be managed via configs (and disabled when needed).

Tested on 96boards Carbon and Nitrogen.

Move the board specific configurations into its own header file, which
can now be created per board, once it gets tested and validated by the
bootloader.

Signed-off-by: Ricardo Salveti <ricardo.salveti@linaro.org>
Signed-off-by: Ricardo Salveti <ricardo.salveti@linaro.org>
@rsalveti
Copy link
Contributor Author

@d3zd3z mind taking a look?

printk("Bootloader chain: 0x%x\n", rsp.br_image_addr);
vt = (struct vector_table *)(rsp.br_image_addr +
rsp.br_hdr->ih_hdr_size);
vt = (struct vector_table *)(rsp.br_image_addr + TEXT_SECTION_OFFSET);
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect this to work. The bootloader should be built with a TEXT_SECTION_OFFSET of 0. Only the programs booted should themselves have it set.

Copy link
Member

Choose a reason for hiding this comment

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

This really needs to be the value read out of the header, which will then work with however it got built. There isn't really a reason to code this value into the bootloader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the vector table of the app itself, right? That offset will depend not on the header, but on CONFIG_TEXT_SECTION_OFFSET, used by the Zephyr app.

Not sure how to best align both things here, which is why I had it defined per device.

Copy link
Member

Choose a reason for hiding this comment

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

It only depends on the value in the header. I can't think of any reason to define this and not use the header. The header is part of the application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense need to make sure the header then gets the right value, will revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

printk("Setting vector table to %p\n", vt);

/* Not all targets set the VTOR, so just set it. */
_scs_relocate_vector_table((void *) vt);
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this can be removed, now that we're going to be setting this properly in Zephyr.

#define FLASH_AREA_IMAGE_SCRATCH_SIZE 0x20000

/* Same offset as used by the Zephyr app (CONFIG_TEXT_SECTION_OFFSET) */
#define TEXT_SECTION_OFFSET 0x100
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be 0x200 for K64F

*/

#define FLASH_DRIVER_NAME "NRF5_FLASH"
#define FLASH_ALIGN 1
Copy link
Member

Choose a reason for hiding this comment

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

Make sure this is really right. Having it too small can excessively wear the flash device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, same value as used by mynewt, so fine to have that now.

.fa_id = FLASH_AREA_IMAGE_0,
.fa_off = 0x20000,
.fa_size = 0x20000,
.fa_off = FLASH_AREA_IMAGE_0_OFFSET,
Copy link
Member

Choose a reason for hiding this comment

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

I think this is good for now. It is likely that we will bring in the partition table code from mynewt into mcuboot, which will make all of this quite different, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just wanted to improve it a bit enough to remove the hardcoded values. The best to do here is parsing mynewt's partition table, to avoid duplicating info.

printk("Bootloader chain: 0x%x\n", rsp.br_image_addr);
vt = (struct vector_table *)(rsp.br_image_addr +
rsp.br_hdr->ih_hdr_size);
vt = (struct vector_table *)(rsp.br_image_addr + TEXT_SECTION_OFFSET);
Copy link
Member

Choose a reason for hiding this comment

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

It only depends on the value in the header. I can't think of any reason to define this and not use the header. The header is part of the application.

CONFIG_HEAP_MEM_POOL_SIZE=16384

CONFIG_FLASH=y
CONFIG_SOC_FLASH_STM32F4=y
Copy link
Member

Choose a reason for hiding this comment

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

Did you do something to Zephyr to make this work? When I did it, there were two flash devices defined for the target so Kconfig picked neither.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's probably because of Linaro/zephyr@942edd4. I'm hoping the upstream version to be merged soon, then we will have the right fix at the right place.

Make it generic and let the SoC itself handle the FLASH driver support
as part of the Zephyr build system.

Signed-off-by: Ricardo Salveti <ricardo.salveti@linaro.org>
Bluetooth is enabled by default with some boards, so just make sure that
it gets disabled here to avoid having a large bootloader firmware.

Signed-off-by: Ricardo Salveti <ricardo.salveti@linaro.org>
Avoid warning when using memcmp.

Signed-off-by: Ricardo Salveti <ricardo.salveti@linaro.org>
Easier to manage and can be easily disabled via config.

Signed-off-by: Ricardo Salveti <ricardo.salveti@linaro.org>
@d3zd3z
Copy link
Member

d3zd3z commented Jan 19, 2017

Looks good to me. I'd like to get a +1 from a mynewt person, because of the image_rsa change.

@ccollins476ad
Copy link
Collaborator

It looks good to me, too. Thanks Ricardo.

@ccollins476ad ccollins476ad merged commit 7cf3d9e into mcu-tools:master Jan 20, 2017
ccollins476ad added a commit that referenced this pull request Jan 20, 2017
Merge remote-tracking branch 'rsalveti/zephyr-updates'

* rsalveti/zephyr-updates:
  zephyr: use SYS_LOG instead of printk
  image_rsa: include missing string.h
  boot/zephyr/prj.conf: disable bluetooth support by default
  boot/zephyr/prj.conf: remove SOC_FLASH_STM32F4, make it generic
  zephyr: targets: add support for 96b_nitrogen
  zephyr: restructure the build process to use board config files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants