Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cypress: Remove qspi_frequency() call. #12038

Merged
merged 1 commit into from
Dec 6, 2019
Merged

Cypress: Remove qspi_frequency() call. #12038

merged 1 commit into from
Dec 6, 2019

Conversation

yarbcy
Copy link
Contributor

@yarbcy yarbcy commented Dec 5, 2019

Summary of changes

Cypress: Remove qspi_frequency() call. cy_qspi_frequency is not implemented.
This change is made because for compatibility reason with upcoming cy_hal changes.

Impact of changes

Cypress

Migration actions required

Documentation

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

CY8CKIT_062_WIFI_BT.txt

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@yarbcy
Cypress: Remove qspi_frequency() call.
91e97a1 
cy_qspi_frequency is not implemented.
This change is made because for compatibility
reason with upcoming cy_hal changes.
@ciarmcom ciarmcom requested review from maclobdell and a team December 5, 2019 16:00
@ciarmcom
Copy link
Member

ciarmcom commented Dec 5, 2019

@yarbcy, thank you for your changes.
@maclobdell @ARMmbed/mbed-os-maintainers please review.

0xc0170
0xc0170 reviewed Dec 6, 2019
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

frequency is not implemented? How to change bus freq?

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 6, 2019

This change is made because for compatibility reason with upcoming cy_hal changes.

will freq be implemented?

@yarbcy
Copy link
Contributor Author

yarbcy commented Dec 6, 2019

This change is made because for compatibility reason with upcoming cy_hal changes.

will freq be implemented?
No.

@yarbcy
Copy link
Contributor Author

yarbcy commented Dec 6, 2019

frequency is not implemented? How to change bus freq?

QSPI frequency depends on CLK_FH2, which (in current implementation of BSPs) rely on divided by 2 FLL, which is configured to 100 MHz :

image

In order to change QSPI bus frequency, user need to change source clock for HF2 and its divider, but this will not give precise clock adjusting unless you change FLL or PLL parameters, which can influence other (non-QSPI related) clocks.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 6, 2019

Unfortunate to have such dependency in the clocks.

In this case, returning QSPI_STATUS_OK is misleading.

@return QSPI_STATUS_OK if frequency was set

Rather return an error, so a user knows t his is not supported. Ideally there would be not supported or similar error code, as it's not there yet, lets return an error.

0xc0170
0xc0170 requested changes Dec 6, 2019
View reviewed changes
targets/TARGET_Cypress/TARGET_PSOC6/cy_qspi_api.c
@@ -38,7 +38,8 @@ qspi_status_t qspi_free(qspi_t *obj)

qspi_status_t qspi_frequency(qspi_t *obj, int hz)
{
return CY_RSLT_SUCCESS == cyhal_qspi_set_frequency(&(obj->hal_qspi), (uint32_t)hz) ? QSPI_STATUS_OK : QSPI_STATUS_ERROR;
/* Return OK since this API is not implemented in cy_hal */
return QSPI_STATUS_OK;
Copy link
Contributor

Choose a reason for hiding this comment

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

QSPI_STATUS_ERROR here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TESTS/mbed_hal/qspi/main.cpp line 261 will fail:
ret = qspi_frequency(&qspi.handle, frequency);

What should we do in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, this function should always return TRUE as it should be implemented 🙄

Lets leave it as it is, this should get fixed once we have capabilities

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 6, 2019

To summarize, a user need to recreate bsp for this target to change QSPI frequency, as it has other dependencies to the system ?

@yarbcy
Copy link
Contributor Author

yarbcy commented Dec 6, 2019

To summarize, a user need to recreate bsp for this target to change QSPI frequency, as it has other dependencies to the system ?

Cypress has targets\TARGET_Cypress\TARGET_PSOC6\psoc6csp\hal\src\cyhal_system.c APIs to work with clocks, where user can adjust FLL/PLL and HF dividers parameters in order to change QSPI frequency. Changing FLL/PLL may require reinitialization of drivers, that are rely on them.

@mbed-ci
Copy link

mbed-ci commented Dec 6, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit 30a54cc into ARMmbed:master Dec 6, 2019
@yarbcy yarbcy deleted the pr/qspi-fix branch December 6, 2019 19:53
@adbridge
Copy link
Contributor

adbridge commented Apr 2, 2020

@Mergifyio backport mbed-os-5.15

@mergify
Copy link

mergify bot commented Apr 2, 2020

Command backport mbed-os-5.15: failure

No backport have been created

  • Backport to branch mbed-os-5.15 failed

1 similar comment
@mergify
Copy link

mergify bot commented Apr 2, 2020

Command backport mbed-os-5.15: failure

No backport have been created

  • Backport to branch mbed-os-5.15 failed

kyle-cypress pushed a commit to kyle-cypress/mbed-os that referenced this pull request Apr 2, 2020
@yarbcy
Backport ARMmbed#12038: Remove qspi_frequency() call.
63583ff 
cy_qspi_frequency is not implemented.
This change is made because for compatibility
reason with upcoming cy_hal changes.
@kyle-cypress kyle-cypress mentioned this pull request Apr 2, 2020
Merged
artokin pushed a commit to artokin/mbed-os that referenced this pull request Apr 16, 2020
Merge remote-tracking branch 'upstream/mbed-os-5.15' into feature-wisun
190eb9a 
* upstream/mbed-os-5.15: (45 commits)
  Revert "Backport ARMmbed#12603: Add CYSBSYSKIT_01"
  Update STM32 EMAC driver based on review
  Update STM32 EMAC driver - limit RX frame length
  WHD: Remove an assert from get_rssi()
  crypto: Use updated ECC curve macros
  crypto: Update the service for Mbed Crypto 3.x
  crypto: Upgrade to Mbed Crypto 3.1.0
  tls: Upgrade to Mbed TLS 2.20.0
  Backport ARMmbed#12701: Custom BT Firmware for CYW9P62S1_43012EVB_01
  Backport ARMmbed#12603: Add CYSBSYSKIT_01
  Backport ARMmbed#12492: Update psoc6cm0p to version 1.1.1.
  Backport ARMmbed#12422: Cypress Asset Update
  Backport ARMmbed#12421: Cypress target reorganization
  Backport ARMmbed#12394: Fix Cypress 1M SDIO + other minor bugs
  Backport ARMmbed#12097: Cypress: Fix IAR Warnings
  Backport ARMmbed#12052: Fix for ARM issue 11859.
  Backport ARMmbed#12038: Remove qspi_frequency() call.
  Backport ARMmbed#12019: rework cypress lptimer hal
  Cellular: ALT1250 PPP cellular driver for mbed-os 5.15
  RZ_A1H and GR_LYCHEE: Enable bootloader support (Mbed OS 5.15)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@maclobdell maclobdell Awaiting requested review from maclobdell

Assignees
No one assigned
Labels
Backport Candidate devices: cypress release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yarbcy @ciarmcom @0xc0170 @mbed-ci @adbridge