Add Hardware CRC HAL API specification #6708

ghost · 2018-04-23T15:35:33Z

Description

Define the HAL API header for the Hardware CRC module. This set of functions
allows hardware acceleration of a subset of CRC algorithms for supported
platforms by providing access to the hardware CRC module of certain platforms.

Status

NOT READY

Tasks

[X] Integrate the HAL API into the Platform API
[ ] Add Reference Implementation for STM32X
[X] Add Reference Implementation for K64F
[ ] Add Reference Implementation for Maxim 32600

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

@bulislaw @deepikabhavnani

0xc0170 · 2018-04-23T15:38:34Z

Do we need feature branch for this? it is against master at the moment

ghost · 2018-04-23T15:53:58Z

I don't think it needs a feature branch. The software implementation is already in master, not many boards support Hardware CRC so the reference implementations cover most of them. And, it's not mandatory so if it's not implemented it will fall back on the software implementation anyway.

0xc0170 · 2018-04-25T12:28:58Z

I don't think it needs a feature branch. The software implementation is already in master, not many boards support Hardware CRC so the reference implementations cover most of them. And, it's not mandatory so if it's not implemented it will fall back on the software implementation anyway.

@bulislaw Please confirm

bulislaw · 2018-04-26T08:39:03Z

drivers/MbedCRC.h

@@ -247,6 +262,7 @@ class MbedCRC
    bool _reflect_data;
    bool _reflect_remainder;
    uint32_t *_crc_table;
+    crc_mode mode_;


mode_ -> _mode

bulislaw · 2018-04-26T08:39:50Z

hal/crc_api.h

+#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+


As with other HAL APIs we need some intro (have a look at ticker/rtc/sleep)

bulislaw · 2018-04-26T08:59:37Z

hal/crc_api.h

+ * from two different contexts may lead to configurations being overwrite and
+ * results being lost.
+ *
+ * \param polynomial CRC Polynomial. Example polynomial: 0x1021 = x^12+x^5+1


Link the enum.

mprse · 2018-04-26T09:14:15Z

hal/crc_api.h

@@ -0,0 +1,117 @@
+#ifndef MBED_CRC_HAL_API_H


I think that the header file needs to be updated to be consistent with other HAL headers.
Doxygen groups, requirements, undefined behavior, potential bugs sections should be added. Example can be found here:
https://github.com/ARMmbed/mbed-os/blob/feature-hal-spec-ticker/hal/us_ticker_api.h

I have also created some example update with the requirements for testing, which can be found here:
https://github.com/mprse/mbed-os/blob/scartmell-arm_crc_tests/hal/crc_api.h

The requirements were created based on provided function descriptions, so probably they need to be extended.
Please fill free to use this example in order to update the header file.

mprse · 2018-04-26T12:55:58Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/mbed_crc_api.c

+    }
+    case POLY_8BIT_CCITT:
+    {
+      width = kCrcBits16;


Why width is set to 16-bits if 8-bit CRC is requested?

There are only two options, 16 and 32 bit, they correspond to the hardware returning the data from the HI-LO data registers for 32 bit, or LO data register for 16 bit. Any CRC's less than 16 bits will be returned in the LO register only as a 16 bit value.

If hardware can handle only 16 or 32 bits CRC, then POLY_8BIT_CCITT, POLY_7BIT_SD polynomials should be not supported by this platform.

They are supported. The result of the CRC calculation is stored in two 16 bit data registers in the CRC module. For CRCs less than 16-bits, only one 16-bit register is used, the other register may contain junk. Setting the width to 16-bit will return only the 16-bit register, which will contain the 7/8 bit result of the calculation.

Maybe I'm missing something, but according to K64F reference manual:

The cyclic redundancy check (CRC) module generates 16/32-bit CRC code for error
detection.

It looks like currently POLY_8BIT_CCITT and POLY_7BIT_SD generates 16 bit CRC. We expect to have 8/7 bit CRC. config.crcBits is set in both cases to kCrcBits16 (and on K64F platform we don't have definitions for 7/8 bits CRC). Only different polynomials are used in each case.

Please look at the following test:

uint32_t crc = 0; const uint8_t test_data[] = "123456789"; hal_crc_compute_partial_start(POLY_8BIT_CCITT); hal_crc_compute_partial((uint8_t*) test_data, strlen((const char*)test_data)); crc = hal_crc_get_result(); printf("Result: 0x%X \n", crc); printf("Expected: 0x%X \n", 0xF4); output: Result: 0xEF6F <--- 16 bit result Expected: 0xF4 I took expected result (0xF4) from `tests-mbed_drivers-crc`.

Ah, you're correct, I'm wrong, I thought there was another option for the polynomial width. I'll remove them from the code.

deepikabhavnani · 2018-04-27T14:55:27Z

hal/crc_api.h

+ * \param data Input data stream to be written into the CRC calculation
+ * \param size Size of the data stream in bytes
+ */
+void hal_crc_compute_partial(const uint8_t *data, const size_t size);


We do not get any failures from hardware? I believe return of computation should be non-zero and error codes should be propagated

deepikabhavnani · 2018-04-27T15:00:38Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/mbed_crc_api.c

+    case POLY_16BIT_CCITT:
+    case POLY_16BIT_IBM:
+    case POLY_32BIT_ANSI:
+      return true;


K64F supports only specified polynomials or any polynomial with width 16/32?
If we have hal API to set configs, user can select any polynomial and not just above three.

It supports any 16-bit or 32-bit polynomial, but to keep the API as simple as possible across all platforms it was decided to only support the polynomials named in the enum.

deepikabhavnani · 2018-04-27T15:03:41Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/mbed_crc_api.c

+      break;
+    }
+    default:
+      MBED_ASSERT("Configuring Mbed CRC with unsupported polynomial");


Provide an option to set these configs and not ASSERT, it should be user configurable to have 16BIT_CCIIT but seed as Zero. I am sure it will work perfectly fine with HW.

As above, it was decided to only support the named subset of polynomials in the enum which define the reflection/seed settings to keep the configuration as simple as possible.

You can overload functions to set those parameters. I don;t think sticking to only specified polynomials make sense for HW. In case of software we need to add multiple tables for each polynomial which is not the case in HW.
Also with the same polynomial even SW CRC gives option to have different seed / reflection settings. How are you planning to support this constructor https://github.com/ARMmbed/mbed-os/blob/master/drivers/MbedCRC.h#L127

Also I have strong use case for this requirement, SD driver uses standard CCITT 16-bit but with seed value of 0. https://github.com/ARMmbed/sd-driver/blob/master/SDBlockDevice.cpp#L251

I don;t think sticking to only specified polynomials make sense for HW. - Unless HW supports only those few

I don;t think sticking to only specified polynomials make sense for HW

I don't either. But then, the only platforms you can reasonably set a polynomial on is K64F and STM(3 series).

How are you planning to support this constructor

By disabling the hardware mode when not using the default constructor.

If there's a use case for additional configuration, then it should be considered. @bulislaw

The CRC support in HW is rather modest. I don't really like the idea of complicating the API for K64F only for CRC polynomials that are not a very standard ones. Where the HW support actually matters is when we need to calculate the checksum repeatedly and fast eg network packets, bootloader maybe, but that should be using standard polynomials.

@bulislaw - Polynomials supported in software CRC are not the only standard ones. Please see the list on Uncyclo "https://en.wikipedia.org/wiki/Cyclic_redundancy_check" for list.

mprse · 2018-04-30T09:01:47Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/mbed_crc_api.c

+
+void hal_crc_compute_partial(const uint8_t *data, const size_t size)
+{
+  CRC_WriteData(CRC0, data, size);


I suggest to add robust checks against null pointer and 0 size and add the following information in the requirements section:

Function hal_crc_compute_partial() does nothing if pointer to buffer is undefined (NULL) or data length is equal to 0.

On different platforms data size equal to 0 might have different consequences, so we can handle such case and simply return without any action.

adbridge · 2018-04-30T11:11:02Z

@scartmell-arm Please do not edit the structure of the PR body! I have put it back to how it should be.

theotherjimmy

You defined a subset of what the standard supports: you don't include seed as a parameter, yet you also do not even use the same seed as managed boot loader mode for CRCITT32: 0.

Support the seed parameter.

deepikabhavnani · 2018-04-30T18:43:18Z

Also I do not see hardware initialization functions called in K64F implementation.

mbed-os/targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/drivers/fsl_crc.c

Line 195 in 920db8e

void CRC_Init(CRC_Type *base, const crc_config_t *config)

How are we planning to support init/deinit?

ghost · 2018-04-30T19:54:58Z

Also I do not see hardware initialization functions called in K64F implementation.

https://github.com/scartmell-arm/mbed-os/blob/53124547d246bb333025cdde1990efbae1da206a/targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/mbed_crc_api.c#L37

mprse · 2018-05-02T07:25:51Z

hal/crc_api.h

@@ -35,6 +35,19 @@ typedef enum crc_polynomial {
    POLY_32BIT_ANSI  = 0x04C11DB7, // x32+x26+x23+x22+x16+x12+x11+x10+x8+x7+x5+x4+x2+x+1
 } crc_polynomial_t;

+typedef struct crc_mbed_config {


I see that conception has changed. This change has massive impact on my test, so please provide requirements for testing.
I have also few questions, so I can start re-factoring my test.
Do we assume now that CRC platforms must support all of the listed options?
Why CRC width is not specified in crc_mbed_config (we can have different CRC width for the same polynomial)?
Does conception with predefined typical polynomials is eventually abandoned?

Do we assume now that CRC platforms must support all of the listed options?

I think yes? Some of them can be handled manually like final_xor by writing it to the module before returning the value, but I believe reflect_in/out/initial_seed should be supported as options.

Why CRC width is not specified in crc_mbed_config (we can have different CRC width for the same polynomial)?

As far as I can tell the width of the CRC polynomial can be derived from the polynomial that's passed into the module, the largest polynomial that is set defines the width, at least for all the ones I've seen. I suppose an additional check needs to be added to the K64F implementation to ensure that either bit-31 is set indicating 32-bit polynomial, or that bit-15 is the highest bit set indicating a 16-bit one.

Does conception with predefined typical polynomials is eventually abandoned?

Yeah, it seemed to be more of an indicator for which polynomials have table support.

Thanks for the answers. I have two more questions:

Do we still limit the number of supported polynomials to ones provided in crc_polynomial_t enum with additional config options (initial_xor, final_xor, reflect_in, reflect_out).

Why initial value config option is not available?

Do we still limit the number of supported polynomials to ones provided in crc_polynomial_t enum with additional config options (initial_xor, final_xor, reflect_in, reflect_out).

No, any polynomial is now supported if the hardware allows it.

Why initial value config option is not available?

This is the same as an initial_xor? Most platforms that defines that option just uses it as an initial value in the hardware.

This is the same as an initial_xor?

Looks like it is :-) Thanks.

MarceloSalazar · 2018-05-03T17:42:44Z

Where are the greentea HAL tests to verify correct integration with hw platform?
Can you point to that, please?

If this feature is going to master, it's assumed will be complete, therefore tests have to be there.
Otherwise, should be moved to feature branch.

mprse · 2018-05-04T09:27:39Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/mbed_crc_api.c

+  if (config == NULL)
+    return false;
+
+  if ((config->polynomial & 0x80008000) == 0)


It look like this check is to return false in case when CRC size is different than CRC16 or CRC32 - only these sizes can be handled by K64F hardware.
Please check the predefined polynomial values:

POLY_16BIT_CCITT = 0x1021 POLY_32BIT_ANSI = 0x04C11DB7

In both cases hal_crc_is_supported function will return false. I still suggest to add CRC size field into crc_mbed_config structure.

Hm, I somehow overlooked those, I suppose there isn't a reasonable way to handle this other than specifying the width so I'll add the config.

mprse · 2018-05-04T09:34:27Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/mbed_crc_api.c

+  platform_config.seed               = config->initial_xor;
+  platform_config.reflectIn          = config->reflect_in;
+  platform_config.reflectOut         = config->reflect_out;
+  platform_config.complementChecksum = true;


If given final_xor config value is equal to all ones (e.q. 0xFFFF_FFFF for 32-bit CRC), then platform_config.complementChecksum should be set to true.
If given final_xor config value is 0, then platform_config.complementChecksum should be set to false.
If given final_xor config value is different than both mentioned values, then platform_config.complementChecksum should be set to false, software xor operation should be performed on the hardware result and the result of the operation should be returned.

Ah okay, I assumed it wasn't supported in the hardware but that will have the same effect.

Complement checksum is different name for final XOR. Its same parameter but different hardware modules accept either true or full value. It should not be separate argument from HAL.

mprse · 2018-05-04T10:06:58Z

hal/crc_api.h

+ */
+typedef enum crc_polynomial {
+    POLY_OTHER       = 0,
+    POLY_8BIT_CCITT  = 0x07,       // x8+x2+x+1


Why x8 has been added in the comment?
0x07 == 0000 0111b => x^2 + x^1 + x^0 = x^2 + x + 1

Next definitions also have this issue.

Hrm, it's technically correct, but misleading. The top bit is excluded from the polynomial. It's defined with the x^8 here. https://en.wikipedia.org/wiki/Cyclic_redundancy_check#Polynomial_representations_of_cyclic_redundancy_checks

0xc0170 · 2018-05-04T12:47:01Z

hal/crc_api.h

+} crc_polynomial_t;
+
+typedef struct crc_mbed_config {
+    /// CRC Polynomial. Example polynomial: 0x21 = 0010_0011 = x^5+x+1


Good to have docs, but these should be done as /**< text > */ as some others in hal folder

0xc0170 · 2018-05-04T12:47:24Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/mbed_crc_api.c

@@ -0,0 +1,65 @@
+#include "crc_api.h"


missing license header on top

0xc0170 · 2018-05-04T12:47:54Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/mbed_crc_api.c

+bool hal_crc_is_supported(const crc_mbed_config_t* config)
+{
+  if (config == NULL)
+    return false;


Please review coding style (hoewever targets/ are not checked), there are others that shall be fixed

deepikabhavnani · 2018-05-04T14:58:38Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/mbed_crc_api.c

+  if (config == NULL)
+    return;
+
+  width = ((config->polynomial & 0xFFFF0000U) != 0) ? kCrcBits32 : kCrcBits16;


Why do we need this extra intelligence here? User passes the width, and it is available as part of config->width. We can simply use that.

0xc0170 · 2018-05-08T10:37:17Z

hal/crc_api.h

+ *
+ * \param polynomial CRC Polynomial. Example polynomial: 0x1021 = x^12+x^5+1
+ */
+void hal_crc_compute_partial_start(const crc_mbed_config_t* polynomial);


HAL API has first parameter this pointer (*obj), not needed for this API? This is hardware related functionality, thus will be needed to have access to their SDK or internal registers (K64F below has CRC0).

I'm not sure I understand. The K64F implementation uses CRC0 as the base pointer for the CRC module because all the CRC functions use offsets from that pointer for accessing specific CRC registers, but not all platforms require anything like that.

The point was that all HAL API have defined as first parameter "this" pointer. My question is - why these new functions do not have it

We don't need it, there is no platforms with multiple CRC engines (no real need for that) and there's no state/context that we need to be maintained.

Some HW CRC modules do store intermediate CRC state, which might be an issue when using CRC HW from different threads.

I don't think we want to support multiple hardware CRC calculations being run concurrently, a different thread shouldn't initialize/use the module until the final value has been read from it.

Driver is providing the locking, HAL specification explicitly mentions that it's up to the user to ensure access safety. HAL is designed for simplicity, the drivers add the level of complexity necessary for safe usage for the user.

bulislaw · 2018-05-09T10:54:56Z

@scartmell-arm let's try to merge it ASAP and add extra platforms after that.

@theotherjimmy @0xc0170 @deepikabhavnani we are getting close to code freeze can you guys rereview the code and accept it if your comments were resolved.

Copy edit file, mostly for American spelling and consistent tense across files.

cmonr · 2018-05-24T17:36:03Z

/morph build

mbed-ci · 2018-05-24T18:51:49Z

Build : SUCCESS

Build number : 2141
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6708/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-05-24T21:41:57Z

Test : SUCCESS

Build number : 1935
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6708/1935

mbed-ci · 2018-05-24T22:35:56Z

Exporter Build : FAILURE

Build number : 1767
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6708/

cmonr · 2018-05-24T22:41:01Z

CI nodes want to leave for the weekend a bit too soon...
/morph export-build

mbed-ci · 2018-05-25T03:43:31Z

Exporter Build : SUCCESS

Build number : 1770
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6708/

0xc0170 · 2018-05-25T09:07:44Z

This is now almost ready, but HAL #7009 should land first . Preceding PR label set

cmonr · 2018-05-25T15:37:58Z

Swapping this PR's priority with #7009. 7009 will need updates before it becomes ready to merge, whereas this is already ready to go.

ghost changed the title ~~Add Hardware CRC HAL API specification headers~~ Add Hardware CRC HAL API specification Apr 23, 2018

0xc0170 added the needs: review label Apr 23, 2018

0xc0170 requested a review from deepikabhavnani April 25, 2018 12:29

bulislaw suggested changes Apr 26, 2018

View reviewed changes

mprse reviewed Apr 26, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Apr 26, 2018

mprse reviewed Apr 26, 2018

View reviewed changes

deepikabhavnani reviewed Apr 27, 2018

View reviewed changes

mprse reviewed Apr 30, 2018

View reviewed changes

theotherjimmy suggested changes Apr 30, 2018

View reviewed changes

mprse reviewed May 2, 2018

View reviewed changes

mprse reviewed May 4, 2018

View reviewed changes

0xc0170 requested changes May 4, 2018

View reviewed changes

deepikabhavnani reviewed May 4, 2018

View reviewed changes

mprse mentioned this pull request May 7, 2018

Add HAL CRC test and header file #6831

Merged

0xc0170 reviewed May 8, 2018

View reviewed changes

bulislaw previously approved these changes May 9, 2018

View reviewed changes

Steven Cartmell and others added 8 commits May 24, 2018 17:51

HAL CRC: Update function documentation with new config parameter

eb202a2

HAL CRC: Fix inverted CRC mode selection

d53ffb7

HAL CRC: Add additional thread safety notes to the API

5e98eea

HAL CRC: Basic implementation for STM32F0

acbf41e

HAL CRC: Use HAL polynomial enum instead of STM32 enum

6b5dabe

HAL CRC: Move STM32 implementation to common folder and add targets

a92ac70

HAL CRC: Add additional STM32 targets

804d050

Copy edit crc_api.h

824466d

Copy edit file, mostly for American spelling and consistent tense across files.

cmonr added needs: CI and removed needs: work labels May 24, 2018

0xc0170 added needs: preceding PR and removed needs: CI labels May 25, 2018

adbridge added the risk: A label May 25, 2018

0xc0170 added the needs: work label May 25, 2018

cmonr added ready for merge and removed needs: preceding PR needs: work labels May 25, 2018

cmonr approved these changes May 25, 2018

View reviewed changes

cmonr merged commit 5371c10 into ARMmbed:master May 25, 2018

cmonr removed the ready for merge label May 25, 2018

bcostm mentioned this pull request Jul 27, 2018

STM32F4 & F1 add hardware CRC support #7607

Closed

cmonr removed the risk: A label Sep 2, 2018

Add Hardware CRC HAL API specification #6708

Add Hardware CRC HAL API specification #6708

Uh oh!

Conversation

ghost commented Apr 23, 2018 • edited by adbridge Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Tasks

Pull request type

Uh oh!

0xc0170 commented Apr 23, 2018

Uh oh!

ghost commented Apr 23, 2018

Uh oh!

0xc0170 commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Apr 27, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Apr 27, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adbridge commented Apr 30, 2018

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani commented Apr 30, 2018

Uh oh!

ghost commented Apr 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 23, 2018 •

edited by adbridge

Loading

ghost Apr 27, 2018 •

edited by ghost

Loading

ghost Apr 27, 2018 •

edited by ghost

Loading

mprse May 4, 2018 •

edited

Loading

MarceloSalazar commented May 3, 2018 •

edited

Loading

0xc0170 May 4, 2018 •

edited

Loading