Skip to content

Add Cypress PSoC 6 targets #9481

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 12 commits into from
Feb 8, 2019
Merged

Add Cypress PSoC 6 targets #9481

merged 12 commits into from
Feb 8, 2019

Conversation

vmedcy
Copy link
Contributor

@vmedcy vmedcy commented Jan 24, 2019

Description

Initial support for Cypress boards at targets/TARGET_Cypress/TARGET_PSOC6.

Added pioneer kits:

  • CY8CKIT-062-4343W
  • CY8CKIT-062-BLE
  • CY8CKIT-WIFI-BT

Added prototyping boards:

  • CY8CPROTO-062-4343W

Added connectivity modules:

  • CY8CMOD-062-4343W

The pull request is split into multiple commits:

  1. Move existing FUTURE_SEQUANA port to targets/TARGET_Cypress/TARGET_PSOC6_FUTURE

  2. Add Cypress PSoC 6 HAL, latest PDL, support for 4 board targets and 4343W connectivity module.

  3. Add mbedtls target providing alternate implementation for PSoC 6 crypto hardware.

  4. Add BLE HCI drivers for CYW43012 and CYW4343W.

  5. Add WICED connectivity drivers compiled for all supported boards and toolchains.

Pull request type

[ ] Fix
[ ] Refactor
[X] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Related pull requests

PSOC6.py updates: #9466 #9509

platform database update: ARMmbed/mbed-os-tools#110

Reviewers

@0xc0170 @lrusinowicz @orenc17

@vmedcy
Copy link
Contributor Author

vmedcy commented Jan 24, 2019

Test suite executed on CY8CPROTO_062_4343W still reports several failures, this is investigated:

mbed test -m CY8CPROTO_062_4343W -t GCC_ARM
...
mbedgt: test case results: 1 FAIL / 15 SKIPPED / 601 OK / 3 ERROR

CY8CPROTO_062_4343W_GCC_ARM_20190123.log

@ciarmcom ciarmcom requested review from 0xc0170, orenc17 and a team January 24, 2019 08:00
@ciarmcom
Copy link
Member

@vmedcy, thank you for your changes.
@0xc0170 @orenc17 @ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-pan @ARMmbed/mbed-os-maintainers please review.

@mikisch81
Copy link
Contributor

@ARMmbed/mbed-os-psa

@0xc0170 0xc0170 requested a review from ashok-rao January 24, 2019 17:11
*
********************************************************************************
* Copyright (c) 2017-2018 Cypress Semiconductor. All rights reserved.
* You may use this file only in accordance with the license, terms, conditions,
Copy link
Contributor

@0xc0170 0xc0170 Jan 24, 2019

Choose a reason for hiding this comment

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

What license for these files? It's missing in this header (it references user agreement, cant spot any in the wifi bt folder).
There were similar files already integrated, they were released under Apache 2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The low-level PSoC initialization code in Cypress port is generated by ModusToolbox MCU Configurators (unlike FUTURE_SEQUANA port which used PSoC Creator tool). The configurators are being updated to generate code with Apache license header.
I will regenerate the code with latest configurators to apply the new license. No Cypress EULA included in mbed port, this is intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xc0170: this is addressed now, all BSP code in GeneratedSource is now Apache 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xc0170: together with the latest rebase, I included the CapSense middleware:
5252d05. The code is supplied with the standard Cypress EULA, it is not Apache 2.0 licensed. Per the guidelines, the license file is added to the psoc6mw/capsense folder.

Copy link
Contributor

@lrusinowicz lrusinowicz left a comment

Choose a reason for hiding this comment

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

I'm not convinced the way it was done, i.e. copying all the files for the basic device support is the best option. This will definitely complicate maintenance in the future.
I understand you want to use newer PDL version, but I see no reason why Sequana would not work with it, if the CY8CKIT_062_BLE does. That's basically the same chip. I'm switching between these boards for testing purposes all the time. Just update PDL and be done with it.

Copy link
Contributor

@lrusinowicz lrusinowicz left a comment

Choose a reason for hiding this comment

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

I see that you have removed the "chip" layer from the vendor->family->chip->board hierarchy that I've originally implemented. This greatly increases amount of copy-pasting when one want's to add a new board, as it can be seen even in your commit.
What's motivation behind this?

@vmedcy
Copy link
Contributor Author

vmedcy commented Jan 24, 2019

@lrusinowicz

I understand you want to use newer PDL version, but I see no reason why Sequana would not work with it, if the CY8CKIT_062_BLE does. That's basically the same chip. I'm switching between these boards for testing purposes all the time. Just update PDL and be done with it.

The desire is to limit the scope of this PR to only add Cypress kits support, but don't break existing SEQUANA boards. Cypress cannot test the SEQUANA targets, due to the limited board availability and different initialization sequence in CM0+ prebuilt images; also the work on _M0 and _PSA targets have not been started.
I propose you to migrate the Sequana port over time to use the Cypress maintained TARGET_PSOC6, including the latest PDL and GeneratedSource generated by ModusToolbox. But this cannot be done all at once.

@vmedcy
Copy link
Contributor Author

vmedcy commented Jan 24, 2019

What's motivation behind this?

There is a valid reason to provide separate device support files per each BSP target (PeripheralNames.h, PeripheralPins.c, PinNames.h, system_psoc6.h/.c, linker scripts, startup assembly and prebuilt CM0+ images). Different Cypress boards have different PSoC 6 MPNs with different FLASH/SRAM layouts (CY8C6XX6, CY8C6XX7, CY8C6XXA) and different hardware dies (PSoC6ABLE2, PSoC6A2M). Each memory layout requires separate set of linker scripts. Each hardware die requires separate startup assembly and prebuilt M0 image. Having this stuff maintained at the family level and selected by extra labels in targets.json would introduce very complex directory, up to the point when the Windows MAX_PATH limit is hit: targets/TARGET_Cypress/TARGET_PSOC6/TARGET_CY8CXX7/TARGET_PSOC6ABLE2/TOOLCHAIN_GCC_ARM/cy8c6xx7_cm4_dual.ld and so on.
Another important point is the selection of the prebuilt CM0+ image: different images might consume different amount of RAM/FLASH (deepsleep, BLE, security, ..). For larger images, modification of the linker scripts and CY_CORTEX_M4_APPL_ADDR in system_psoc6.h is required at the moment (this can be reduced to the absolute minimum in the future, by json parametrization).
The proposed layout can handle this easily. Also it is super intuitive for customers who go to production with alternate MPNs and custom devboards how to create their own custom target - just copy the existing one and adjust as needed. Customers who target connectivity module (CY8CMOD_062_4343W) can copy existing TARGET_CY8CPROTO_062_4343W sub-target to only adjust PinNames.h to match their board.

Everything that can be shared across boards (CMSIS device headers and peripheral drivers) is already in psoc6pdl directory - this is where most development/maintenance happens.

@cmonr cmonr requested a review from a team January 24, 2019 20:37
@cmonr
Copy link
Contributor

cmonr commented Jan 24, 2019

@vmedcy Ooph, this is quite the PR!

While the relevant reviews take a look at this PR, a couple of things.

  1. 👍 on the commit comments, as well as on their topic deliniations!
    Even though this PR is too big for GitHub to load all of the files at once (it paginated three times for me) we should be able to use the commits to deliniate the review topics.
  2. Add Cypress PSoC 6 targets #9481 (comment)
  • Thanks for provinding the GCC test one of the targets. Since this is only one target, would you mind providing the test results for the other targets being added with this PR?
  • We also need test results for the ARM compiler (ARMc5u6) and IAR (7.8).

@cmonr
Copy link
Contributor

cmonr commented Jan 25, 2019

I thought I forgot something.

  1. The GCC test, it needs to be completely passing. I noticed there were a couple of failures and timeouts that should be corrected.

@cmonr
Copy link
Contributor

cmonr commented Jan 25, 2019

While I'm here, it looks like GitHub noticed something else.

  1. This PR will need to be rebased, to resolve this conflicting file:
    targets/TARGET_Cypress/TARGET_PSOC6/TARGET_FUTURE_SEQUANA_PSA/prebuilt/README.md

Note: When done correctly, the PR will not have any merge commits in the history. (Sometimes we find that people get merges and rebases confused)

@vmedcy
Copy link
Contributor Author

vmedcy commented Jan 25, 2019

@cmonr:

1. This PR will need to be rebased, to resolve this conflicting file:
   `targets/TARGET_Cypress/TARGET_PSOC6/TARGET_FUTURE_SEQUANA_PSA/prebuilt/README.md`

This is done, rebased against the latest master, absolutely no problem with that (force pushed the PR branch).
I also included the latest HAL updates and GeneratedSource license fixes.

The GCC test, it needs to be completely passing. I noticed there were a couple of failures and timeouts that should be corrected.

This is fully understood. The team still works on getting the full test suite passing.

We also need test results for the ARM compiler (ARMc5u6) and IAR (7.8).

Attached greentea test results on CY8CKIT_062_WIFI_BT with GCC_ARM 6.3.1, ARM Compiler 5.06 update 6, and IAR Compiler V8.20.
CY8CKIT_062_WIFI_BT-IAR-20190124.log
CY8CKIT_062_WIFI_BT-ARM-20190124.log
CY8CKIT_062_WIFI_BT-GCC_ARM-20190124.log

Test reports for other PSoC 6 targets will be attached later.

There are still several failures reported (tests-mbed_hal-sleep_manager, tests-mbed_hal-sleep, tests-mbed_drivers-lp_timeout, features-feature_ble-targets-target_cordio-tests-cordio_hci-transport):

mbedgt: test case results: 3 FAIL / 1 SKIPPED / 615 OK / 1 ERROR

Supporting IAR Compiler V7.8 requires extra work: WICED binary drivers were prebuilt with IAR 8, not compatible with IAR 7. Need to investigate possibility to support both IAR 7 and 8. I believe Mbed-os moves to IAR 8 soon, per ARMmbed/mbed-os-5-docs#885. Can you suggest if any IAR 7 related effort is justified at this point of time?

Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

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

TARGET_CYW43012/HCIDriver.cpp and TARGET_CYW4343X/HCIDriver.cpp are identical. I believe it would be better for maintenance purposes to have a single driver rather than duplicate code.

output_mode(bt_device_wake_name, 0);
output_mode(bt_power_name, 1);

//output_mode(bt_host_wake_name, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Could you comment that change ? Is there any reason to keep that code commented ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed commented code with the latest update: 8ca1720.

Regarding duplication of HCI implementation: the idea is to design this for extensibility from the beginning.
The pin assignment for CYW43012 and CYW4343X HCI drivers just happens to match for the 4 currently supported boards. In the future, as we add more boards, it may diverge and require different config.
Since CYW43012 has embedded stack mode, as we may add this feature support in future, it may also make the HCIDriver differ from 4343W.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pan-: the team put more thought on HCI driver design. The driver duplication is now addressed by 6a36183: there is a single driver for CYW4343W and CYW43012 in features/FEATURE_BLE/targets/TARGET_Cypress/TARGET_CYW43XXX. Differences in the implementation are to be handled by constructor argument and json parametrization.
Can you resolve this conversation?

@lrusinowicz
Copy link
Contributor

@vmedcy

The desire is to limit the scope of this PR to only add Cypress kits support, but don't break existing SEQUANA boards. Cypress cannot test the SEQUANA targets, due to the limited board availability and different initialization sequence in CM0+ prebuilt images; also the work on _M0 and _PSA targets have not been started.
I propose you to migrate the Sequana port over time to use the Cypress maintained TARGET_PSOC6, including the latest PDL and GeneratedSource generated by ModusToolbox. But this cannot be done all at once.

So more work for us, because you want to avoid some, good.
I don't expect you to test Sequana, but OTOH, Mbed tests for Sequana could be run on CY8CKIT_062_BLE with just proper name mocking (mbedls -mock option), assuming it's native tests do work.
You could have both PDLs at the same level and select a proper one using json file if you'd move PDLs to directories prefixed with TARGET_. This way no copy of the BSP would be needed.

You are talking about the Windows path length limit elsewhere, but you are lengthening it for us.

@pan-
Copy link
Member

pan- commented Jan 25, 2019

@vmedcy Few more questions:

'IORegistryEntryName': 'Cypress KitProg2 (CMSIS-DAP, Mass Storage)',
'USB Serial Number': '10120C5303147400',

@vmedcy
Copy link
Contributor Author

vmedcy commented Jan 25, 2019

@lrusinowicz:

You could have both PDLs at the same level and select a proper one using json file if you'd move PDLs to directories prefixed with TARGET_. This way no copy of the BSP would be needed.

Avoiding complex nested TARGET_ hierarchies is an explicit goal. I agree this adds a little maintenance overhead (same linker scripts appear in different BSP target folders), but makes things much easier for end users, especially when creating custom BSPs.

@vmedcy
Copy link
Contributor Author

vmedcy commented Jan 25, 2019

@pan-:

I want to validate the port on a CY8CKIT_062_BLE

Great!

Do I need to flash a special firmware?

Yes, CY8CKIT_062_BLE comes with KitProg2 FW, not compatible with mbedls. DAPLink firmware is under development, we flash nightly builds of KitProg3 FW during development/testing. Please suggest the convenient way for you to receive the compatible firmware (requires either special FW upload tool or Bootloader Host Tool supplied with PSoC Creator).

Do I need #9466 checked in to validate this PR ?

Yes, as well as #9509 I just submitted. To merge all 3 PRs:

git fetch origin pull/9481/head:add_cypress_kits
git fetch origin pull/9466/head:psoc6-target-hook
git fetch origin pull/9509/head:psoc6-daplink-hex
git checkout add_cypress_kits
git merge psoc6-target-hook
git merge psoc6-daplink-hex

Why BLE is not a listed feature of CY8CKIT_062_BLE ?

BLE hardware is different in CY8CKIT_062_BLE (using on-chip BLESS instead of WICED connectivity module). It is not supported by the existing port. The FW implementation will be provided in consequent PRs.

@c1728p9
Copy link
Contributor

c1728p9 commented Jan 26, 2019

I @vmedcy, thanks for the PR! I'm in the process of reviewing it, but am having trouble viewing diffs with the commit history in its current form. Some of the problems I'm running into when reviewing:

  • The commit "Add Cypress PSoC 6 targets" says that some of the code is based on the FUTURE_SEQUANA. This commit only shows new files so it isn't easy to see what was changed for the new targets. Adding a commit before this which just copies over the resued files without any modifications would allow the the diff to be shown in git.
  • The same commit, "Add Cypress PSoC 6 targets", has a large number of generated files in addition to changing files from the FUTURE_SEQUANA. If the code generation is split out into a seperate commit then the steps to generate this could be reviewed on its own.
  • The commit "PSOC6: add Cypress target to mbedtls" has license updates and maybe bug fixes in addition the mbedtls support it adds. Each commit could be reviewed in more detail if this was broken into 3 commits - one for TLS, one for license updates and one for bug fixes.

Would it be possible to update the history of this PR to make it easier to review? I made these changes to the history in my fork of mbed-os so feel free to push that directly or with modifications if it is useful to you. The end result is identical, I just broke apart the history to make it easier to review. The updated history can be found in my mbed-os repo on the branch pr9481_history. Commits of my branch:

  1. Move FUTURE_SEQUANA port to TARGET_PSOC6_FUTURE (unchanged)
  2. Reuse FUTURE_SEQUANA porting layer
    • I copied the files that I thought came from FUTURE_SEQUANA so I could see a diff on subsequent changes to these files.
  3. PSOC6: Generate Cypress PSoC 6 target code (split from commit "Add Cypress PSoC 6 targets")
    • Would it be possible to provide an overview of how to recreate this in the commit message?
  4. PSOC6: Integrate Cypress PSoC 6 target code (split from commit "Add Cypress PSoC 6 targets")
  5. PSOC6: Update Cypress PSoC 6 code (split from commit "PSOC6: add Cypress target to mbedtls")
    • Changes here look like bug fixes unrelated to mbedtls. Can you confirm?
  6. PSOC6: add Cypress target to mbedtls
  7. PSoC6: add implementation of BLE HCI driver for CYW4343X/CYW43012 (unchanged)
  8. Add WICED binary drivers for Cypress PSoC 6 targets (unchanged)

@vmedcy
Copy link
Contributor Author

vmedcy commented Jan 26, 2019

@c1728p9: thank you for reviewing.

Would it be possible to update the history of this PR to make it easier to review?

Absolutely, I admit the PR is rather hard to review in the web interface. I mishandled the previous interactive rebase (squashed my updates with wrong base commit). Thanks a lot for you work on pr9481_history, indeed it is much easier to review this PR if all Cypress HAL fixes and GeneratedSource updates are separated. I rebased per your suggestions and provided more context in the commit messages. Now the commit history should be coherent.

I also included the latest HAL updates from the PDL team. I guess you are mostly focused on this commit: 94c8081. @lrusinowicz: some Cypress HAL updates can also be applicable to FUTURE targets, kindly take a look.

@vmedcy
Copy link
Contributor Author

vmedcy commented Jan 26, 2019

@cmonr: CapSense and WICED binary drivers were recompiled with IAR 7.8; we temporary switched our internal testing to use this old version (unfortunately there is incompatibility at C++ binary level). Whenever Mbed OS decides to switch to IAR 8, Cypress is ready to provide the new binaries.

There is also a great progress on having the greentea test suite pass. Latest result is:
mbedgt: test case results: 1 FAIL / 618 OK / 1 ERROR
Failing test cases: tests-mbed_hal-sleep and features-feature_ble-targets-target_cordio-tests-cordio_hci-transport (timeout).
Will attach the complete test logs once the remaining issues are addressed by the team, I guess there is no much point to provide intermediate results.

Copy link
Contributor

@c1728p9 c1728p9 left a comment

Choose a reason for hiding this comment

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

Hi @vmedcy thanks for the quick update. The HAL changes, 94c8081, look good to me. I left a couple of cosmetic nitpicks, but no actual problems. Thanks very much for updating the history. It made this PR very straight forward to review.

@vmedcy
Copy link
Contributor Author

vmedcy commented Feb 7, 2019

@0xc0170, @yanesca:

Please revert the commit referenced above

This is done, removed 2e87bc9 from this PR.
Will create separate PR for mbedtls alternate implementation once the existing review feedback is addressed.

This PR is now ready for merge.

@yanesca
Copy link
Contributor

yanesca commented Feb 7, 2019

@vmedcy Thank you very much for your understanding!

Looks good to me!

@adbridge
Copy link
Contributor

adbridge commented Feb 7, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Feb 7, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 4
Build artifacts

@SeppoTakalo
Copy link
Contributor

@vmedcy Please note:

Seems that the NetworkInterface base class have been changed in #9568, so these binaries needs to be rebuild against latest master.
Otherwise ARM and IAR builds are broken.

Sorry for the inconvenience.

@SeppoTakalo
Copy link
Contributor

Actually, I just realised.
This is targeting 5.11.4 and the #9568 is targeting 5.12. Therefore these binaries works against 5.11. branch, but do not work against master branch.

So I think we should merge this in as it is, do do a separate build of Wiced binaries from master. We still have time before 5.12 to fix that.

@vmedcy
Copy link
Contributor Author

vmedcy commented Feb 8, 2019

@SeppoTakalo: thanks for letting us know. Since #9568 is already merged to master, is this pull request to be merged to a different branch to appear in 5.11.4, like mbed-os-5.11 or release-candidate? Is it safe to recompile libwiced_drivers.a now with the latest master and expect it to still work in 5.11.4?
@0xc0170: should I rebase this PR against master (with #9568), and ask team to recompile WICED drivers? Or this PR can be merged as-is? I see it already has "ready for merge" label - is it going to master in order to appear in 5.11.4?

@adbridge
Copy link
Contributor

adbridge commented Feb 8, 2019

@vmedcy All PRs got to master first and then we select specific ones to get cherry picked across to the release branch. So yes this would go to master first and then across to 5.11 ready for 5.11.4. So this should be fine in that release. As @SeppoTakalo said #9568 is targeting 5.12 so would not get ported across. So this can safely be merged in and go to the release as planned. You don't need to do anything else for now.

@adbridge adbridge merged commit 3252530 into ARMmbed:master Feb 8, 2019
@vmedcy
Copy link
Contributor Author

vmedcy commented Feb 8, 2019

@adbridge: thank you for explanation of release branching, and for PR approval.
@SeppoTakalo: I can confirm linkage errors of mbed-os-example-wifi with IAR/ARM in latest master due to changes in WiFiInterface vtable layout.
@deepikabhavnani: I guess you can now rebase feature-iar8 against master. We will provide libwiced_drivers.a built with IAR8 for TARGET_CY8CKIT_062_4343W, TARGET_CY8CKIT_062_WIFI_BT, TARGET_CY8CMOD_062_4343W, TARGET_CYW943012P6EVB_01.

@SeppoTakalo
Copy link
Contributor

@vmedcy Please hold before building new binaries for feature-iar8 branch (or other branches)

We can synch our changes with Multihoming branch, so that we need to rebuild once before 5.12 release. We need new binaries for master for ARM,GCC & IAR. Then we need new binaries for IAR8 and ARMC6.

There is now email discussion going between us & Cypress. Lets synch up off-line.

@vmedcy
Copy link
Contributor Author

vmedcy commented Feb 8, 2019

@SeppoTakalo: this is understood, thanks. It makes sense to reduce the total number of rebuilds (to decrease dev/test effort).

@deepikabhavnani
Copy link

@vmedcy - feature-iar8 is rebased, can add binaries after all changes for 5.12 are in sync.

@deepikabhavnani
Copy link

@vmedcy @SeppoTakalo - Any updates on binaries for IAR 8.32?

@SeppoTakalo
Copy link
Contributor

I tried to build on feature-iar8 branch, and it failed.
Probably needs some flag set on toolchain scripts to enable compiling.

See internal ticket https://jira.arm.com/browse/IOTCORE-1051

@deepikabhavnani
Copy link

yes all failing targets are explicitly disabled in feature branch. Please enable them in targets.json by removing below 2 lines (will pick default from inherited target)

    "supported_toolchains": ["GCC_ARM", "ARM"],
    "release_versions": [""],

@deepikabhavnani
Copy link

Also please add this change set of targets.json to PR adding the binaries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.