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.

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

Allow for arbitrary QSPI alt sizes #11602

Merged
merged 2 commits into from Oct 18, 2019

Conversation

@kyle-cypress
Copy link

kyle-cypress commented Sep 30, 2019

Description

The QSPI spec allows alt to be any size that is a multiple of the number of data lines. For example, Micron's N25Q128A uses only a single alt cycle for all read modes (1, 2, or 4 bits depending on
how many data lines are in use).

Pull request type

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

Reviewers

@ARMmbed/team-cypress

Release notes

Reason for change

The QSPI interface permits the alt (aka mode) bits to be any multiple of the bus width. But the values in qspi_alt_size_t only support multiples of bytes. While #11295 masks the issue, this is insufficient to support the memory devices on all targets.

Analysis of effects

No functional impact on existing code. QSPI driver clients may, but are not required to, utilize the greater flexibility in alt sizes.
Allows parts which require non-byte-aligned alt bits to continue working after #11295 is fixed, which prevents QSPIFBlockDevice from functioning correctly with memory devices which require a non-empty alt value for one or more of the operations which QSPIFBlockDevice performs.

Migration path

No change is required to existing code. Compatibility macros are provided for the old qspi_alt_size_t values.
New code is recommended to explicitly specify the required number of alt bits rather than using the
QSPI_CFG_ALT_SIZE_* macros.

@kyle-cypress kyle-cypress force-pushed the kyle-cypress:pr/qspi-arbitrary-alt-size branch from 3a3d335 to baf375f Sep 30, 2019
@ciarmcom ciarmcom requested review from maclobdell and ARMmbed/mbed-os-maintainers Sep 30, 2019
@ciarmcom

This comment has been minimized.

@@ -247,7 +247,7 @@ int QSPIFBlockDevice::init()

// Configure BUS Mode to 1_1_1 for all commands other than Read
_qspi_configure_format(QSPI_CFG_BUS_SINGLE, QSPI_CFG_BUS_SINGLE, QSPI_CFG_ADDR_SIZE_24, QSPI_CFG_BUS_SINGLE,
QSPI_CFG_ALT_SIZE_8, QSPI_CFG_BUS_SINGLE, 0);
0, QSPI_CFG_BUS_SINGLE, 0);

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Oct 1, 2019

Contributor

I don't understand this change. Documentation says:

*  @param alt_size Size in bits used by alt phase(Valid values are QSPI_CFG_ALT_SIZE_8, QSPI_CFG_ALT_SIZE_16, QSPI_CFG_ALT_SIZE_24, QSPI_CFG_ALT_SIZE_32)

So why zero instead of one of the valid values?

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Oct 1, 2019

Contributor

Sorry, you changed it to:

*  @param alt_size Size in bits used by alt phase (must be a multiple of the number of bus lines indicated in alt_width)

But I still don't get it, why zero is multiple of bus lines?

This comment has been minimized.

Copy link
@kyle-cypress

kyle-cypress Oct 1, 2019

Author

Prior to this change, QSPI_CFG_ALT_SIZE_8 seems to be chosen as a somewhat arbitrary placeholder (the code has no particular knowledge that 8 alt bits are actually what is required) which then winds up being ignored because QSPIFBlockDevice never passes anything other than -1 for alt.
Setting 0 (no alt bits) seems like a more direct way of expressing what QSPIFBlockDevice is doing. However, I'm okay reverting this to 8 for now if you'd prefer given the more significant rework of alt bit handling in #11531 .

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Oct 4, 2019

Contributor

No need to revert. Explanation is enough. Zero is OK to express "No alt bits"

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 1, 2019

The QSPI spec allows alt to be any size that is a multiple of the number of data lines. For example, Micron's N25Q128A uses only a single alt cycle for all read modes (1, 2, or 4 bits depending on
how many data lines are in use).

Isn't this in bytes? As I read it, it is. It is what we support 8,16,24 and 32 bits. Where in N25Q128A datasheet I can find relevant documentation that would require this change to the driver?

Looking at your driver, it defines them the same:

/** Size in bits */
typedef enum cyhal_qspi_size {
    CYHAL_QSPI_CFG_SIZE_8                           = 8,
    CYHAL_QSPI_CFG_SIZE_16                          = 16,
    CYHAL_QSPI_CFG_SIZE_24                          = 24,
    CYHAL_QSPI_CFG_SIZE_32                          = 32,
} cyhal_qspi_size_t;

I don't think any other number makes sense.

@0xc0170 0xc0170 requested review from ARMmbed/team-silabs and removed request for ARMmbed/mbed-os-test Oct 1, 2019
Copy link
Contributor

LMESTM left a comment

hi
as this change reworks part of the driver, I would like to take the opportunity to ask for using the HAL_OSPI and HAL_QSPI definitions wherever possible. Also I'm asking a few questions for my understanding of this change ...


/* command->AlternateBytesSize needs to be shifted by OCTOSPI_CCR_ABSIZE_Pos */
// 0b00 = 1 byte, 0b01 = 2 bytes, 0b10 = 3 bytes, 0b11 = 4 bytes
st_command->AlternateBytesSize = ((rounded_size - 1) << OCTOSPI_CCR_ABSIZE_Pos) & OCTOSPI_CCR_ABSIZE_Msk;

This comment has been minimized.

Copy link
@LMESTM

LMESTM Oct 1, 2019

Contributor

Please rework this part to use a switch case and assign st_command->AlternateBytesSize only with definitions of HAL_OSPI_ALTERNATE_BYTES_8_BITS, HAL_OSPI_ALTERNATE_BYTES_16_BITS, etc.
The actual values assignment (0, 01, 02 ) and its position in the register might change from a target to another ...

This comment has been minimized.

Copy link
@kyle-cypress

kyle-cypress Oct 3, 2019

Author

Updated to introduce get_alt_bytes_size for this purpose

st_command->AlternateBytesSize = (command->alt.size << QUADSPI_CCR_ABSIZE_Pos) & QUADSPI_CCR_ABSIZE_Msk;
st_command->AlternateBytesSize = command->alt.size;
// 0b00 = 1 byte, 0b01 = 2 bytes, 0b10 = 3 bytes, 0b11 = 4 bytes
st_command->AlternateBytesSize = ((rounded_size - 1) << QUADSPI_CCR_ABSIZE_Pos) & QUADSPI_CCR_ABSIZE_Msk;

This comment has been minimized.

Copy link
@LMESTM

LMESTM Oct 1, 2019

Contributor

Same remark as above, please use switch case and assign st_command->AlternateBytesSize only with definitions of HAL_QSPI_ALTERNATE_BYTES_8_BITS, HAL_QSPI_ALTERNATE_BYTES_16_BITS, etc ...

I know this was not done like this before but tests were all okay, so we lived with it. Now that we need to rework to cope with a new condition, better to clean it up.

@@ -230,15 +255,19 @@ void qspi_prepare_command(const qspi_command_t *command, QSPI_CommandTypeDef *st
st_command->AddressSize = (command->address.size << QUADSPI_CCR_ADSIZE_Pos) & QUADSPI_CCR_ADSIZE_Msk;

This comment has been minimized.

Copy link
@LMESTM

LMESTM Oct 1, 2019

Contributor

As part of rework, better need to also assign AddressSize with switch case and assignment from one of definitions: HAL_OSPI_ADDRESS_8_BITS, HAL_OSPI_ADDRESS_16_BITS, etc...

This comment has been minimized.

Copy link
@kyle-cypress

kyle-cypress Oct 3, 2019

Author

I don't think this change belongs in this pull request, which right now doesn't do anything related to AddressSize. I'd rather keep this PR focused on the driver interface change, with the minimum target updates required to keep things working.

}

// Round up to nearest byte - unused parts of byte act as dummy cycles
uint32_t rounded_size = ((command->alt.size - 1) >> 3) + 1;

This comment has been minimized.

Copy link
@LMESTM

LMESTM Oct 1, 2019

Contributor

Could you add some comments to explain what is being done here ?
command->alt.size is a number of bits right ? before the change it was one of 8 / 16 / 24 / 32 bits, right ?
what are the possible values now ? Any from 1 to 32 and we're rounding this value to what ? nearest upper byte or nearest byte ?

from what I see
1 bit is being rounded up to 1 byte
7 bits is being rounded up to 1 byte
18 bits is being rounded to 3 bytes
31 bits to 4 bytes
so I guess that would be the nearest up byte ...

This comment has been minimized.

Copy link
@kyle-cypress

kyle-cypress Oct 3, 2019

Author

Yes, rounded_size is up to the nearest byte. I renamed the variable to make this more clear.

st_command->DummyCycles -= integrated_dummy_cycles;

// Align alt value to the end of the most significant byte
st_command->AlternateBytes = command->alt.value << leftover_bits;

This comment has been minimized.

Copy link
@LMESTM

LMESTM Oct 1, 2019

Contributor

Isn't there a check or a mask that the AlternateBytes value width actually fits into the AlternateBytesSize ?
e.g. make sure that if 3 bits size were selected, then AlternateBytes isn't bigger than 3 bits ?

This comment has been minimized.

Copy link
@kyle-cypress

kyle-cypress Oct 3, 2019

Author

There isn't an existing check that I can see, and this PR does not change that. I'm also not sure how one would write such a check. You could approximate by checking the position of the last non-zero bit, but that doesn't give you any guarantee because there could be trailing zeros as part of the required mode bits. Unfortunately I think this just has to trust that the caller set consistent values.
(additionally, as above, any change for this should not be in this PR).

@0xc0170 0xc0170 added needs: work and removed needs: review labels Oct 1, 2019
@0xc0170 0xc0170 removed the request for review from ARMmbed/mbed-os-core Oct 1, 2019
@kyle-cypress

This comment has been minimized.

Copy link
Author

kyle-cypress commented Oct 1, 2019

The QSPI spec allows alt to be any size that is a multiple of the number of data lines. For example, Micron's N25Q128A uses only a single alt cycle for all read modes (1, 2, or 4 bits depending on
how many data lines are in use).

Isn't this in bytes? As I read it, it is. It is what we support 8,16,24 and 32 bits. Where in N25Q128A datasheet I can find relevant documentation that would require this change to the driver?

In table 22 page 39 of the datasheet, all of the "number of mode bits" fields are defined to be either 0 or 1.
Per JESD216, table 3 (page 26), " This field should be counted in clocks not number of bits received by the serial flash.". So a value of "1" means 1, 2, or 4 mode bits depending on the number of data lines (a value of 0 means mode bits are not supported).

Looking at your driver, it defines them the same:

/** Size in bits */
typedef enum cyhal_qspi_size {
    CYHAL_QSPI_CFG_SIZE_8                           = 8,
    CYHAL_QSPI_CFG_SIZE_16                          = 16,
    CYHAL_QSPI_CFG_SIZE_24                          = 24,
    CYHAL_QSPI_CFG_SIZE_32                          = 32,
} cyhal_qspi_size_t;

I don't think any other number makes sense.

A number of hardware implementations, including the Cypress QSPI interface, only support byte multiples of mode bits. This changeset attempts to support the full flexibility allowed in the standard by rounding the mode bits up to the nearest byte and using the extra bits as part of the dummy cycles (which should be safe because during the dummy cycles the flash device assumes that the data lines are tri-stated and therefore don't contain any meaningful data). Naturally this only works if there are enough dummy cycles to absorb the extra mode bits; otherwise this is flagged as an error (it means that the flash part and the MCU's QSPI interface are incompatible).

@maclobdell

This comment has been minimized.

Copy link
Contributor

maclobdell commented Oct 8, 2019

@LMESTM, @ARMmbed/mbed-os-hal - does this look good to you? Can you finish your review?

@LMESTM
LMESTM approved these changes Oct 11, 2019
Copy link
Contributor

LMESTM left a comment

LGTM for target_stm related changes - thanks !

Copy link
Member

0xc0170 left a comment

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 15, 2019

Changes look good to me, one last question: we are removing QSPI_CFG_ALT_SIZE_8, etc - the values 8 to 32 we defined. This is a breaking change, can we at least keep these for backward compatibility?

@kyle-cypress

This comment has been minimized.

Copy link
Author

kyle-cypress commented Oct 15, 2019

Changes look good to me, one last question: we are removing QSPI_CFG_ALT_SIZE_8, etc - the values 8 to 32 we defined. This is a breaking change, can we at least keep these for backward compatibility?

They don't make sense as an enum anymore, but I can add them back in as #defines to 8, 16, 24 32 for BWC.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 15, 2019

@kyle-cypress yes, but we shall not break preferably. Make a note there that use a number as argument is recommended and these are there for backward compatibility.

@kyle-cypress

This comment has been minimized.

Copy link
Author

kyle-cypress commented Oct 15, 2019

@kyle-cypress yes, but we shall not break preferably. Make a note there that use a number as argument is recommended and these are there for backward compatibility.

Pushed 1a12c0f to address

@kyle-cypress

This comment has been minimized.

Copy link
Author

kyle-cypress commented Oct 15, 2019

@kyle-cypress Please add Release notes section, following the template :

https://os.mbed.com/docs/mbed-os/v5.14/contributing/workflow.html#pull-request-template-functional-changes

Updated description to add release notes. Based on our discussion this morning and the addition of backwards compatibility macros, should this be changed from "functionality change" to "fix"?

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 16, 2019

I would leave the type as it is rather, it fixes the issue but introducing new functionality.

👍 for making it backward compatible

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Oct 16, 2019
Kyle Kearney added 2 commits Oct 3, 2019
- Use a switch statement rather than shifting and masking to compute
  the AlternateBytes value.
- Rename rounded_size to alt_bytes to clarify its purpose.
@kyle-cypress kyle-cypress force-pushed the kyle-cypress:pr/qspi-arbitrary-alt-size branch from 1a12c0f to bb872ee Oct 16, 2019
@kyle-cypress

This comment has been minimized.

Copy link
Author

kyle-cypress commented Oct 16, 2019

Rebased to resolve conflict

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 17, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 17, 2019

Test run: SUCCESS

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

Copy link
Member

0xc0170 left a comment

Something is not correct after the latest rebase or? I can see here only macros addition and STM target change. Please review @kyle-cypress

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Oct 17, 2019
@kyle-cypress

This comment has been minimized.

Copy link
Author

kyle-cypress commented Oct 17, 2019

It looks like #11297 has been merged into master. As mentioned in #11297 (comment), that PR was based on top of this PR (but seems to have missed the "dependent PR" tag getting added when it was split apart). So when #11297 was merged it pulled in the changes from baf375f and 9b32c0f. Leaving this PR with just the two subsequent changes that you mention.

@0xc0170 0xc0170 self-requested a review Oct 18, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 18, 2019

It should be in 5.14.2 as the referenced one above is as well. Thus changed release version here.

@0xc0170 0xc0170 merged commit d851a63 into ARMmbed:master Oct 18, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8692 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.