fix(PeriphDrivers): Fix MAA Operator Setter for TPU Drivers #1104

Jake-Carter · 2024-08-02T22:58:38Z

Description

Fixes #1089

Checklist Before Requesting Review

PR Title follows correct guidelines.
Description of changes and all other relevant information.
(Optional) Link any related GitHub issues using a keyword
(Optional) Provide info on any relevant functional testing/validation. For API changes or significant features, this is not optional.

Jake-Carter · 2024-08-02T23:04:41Z

@night1rider if you can re-test with this fix and confirm functionality we would very much appreciate it.

Libraries/PeriphDrivers/Source/TPU/tpu_reva.c

ozersa

Shifting not require for _keySelect function.

night1rider · 2024-08-05T16:06:27Z

@night1rider if you can re-test with this fix and confirm functionality we would very much appreciate it.

@Jake-Carter

Hello,
I applied the proposed fix in the PR, and made sure that I am calling the proper macro/enums however it seems to fail the test cases inside wolfssl now.

The fix I have been using, which is not the proper way to fix this issue, is working and the tests for wolfssl pass.

From my quick observations it does go down into the hardware and preform some sort of calculations, but the end result is wrong. I would wager that it is still not setting the correct operations to the register.

Jake-Carter · 2024-08-05T16:30:23Z

Thanks @night1rider. What size are you initializing the MAA with?

The reason I ask is that I see some of the TPU control fields are conditional based on the MAA word size (see MAA_MAWS register). Ex:

The field values that you have found to work follow the same scheme for the MAWS > 1024 that some of these other fields do. I wonder if this is also the case for the clc field but our UG is not correct.

night1rider · 2024-08-05T17:04:39Z

Thanks @night1rider. What size are you initializing the MAA with?

The reason I ask is that I see some of the TPU control fields are conditional based on the MAA word size (see MAA_MAWS register). Ex:
The field values that you have found to work follow the same scheme for the MAWS > 1024 that some of these other fields do. I wonder if this is also the case for the clc field but our UG is not correct.

@Jake-Carter

For the tests that fail for wolfssl, we are passing the values of 1024 and 2048 to MXC_TPU_MAA_Init using the datatype unsigned int

Here is a code snip of how we call it. We calculate len generically to determine what to pass for the needed operation. Also when we init we set a mutex so that the HW cannot be accessed until released (Just in case this is a concern).

/* Sets mutex and initializes hardware according to needed operation size */
int wc_MXC_MAA_init(unsigned int len)
{
    int status;
    MAX3266X_MSG("Setting Hardware Mutex and Starting MAA");
    status = wolfSSL_CryptHwMutexLock();
    if (status != 0) {
        return status;
    }
    status = MXC_TPU_MAA_Init(len);
    return wc_MXC_error(&status); /* Return Status of Init */
}

Jake-Carter · 2024-08-05T18:00:19Z

Thanks @night1rider. just updated the drivers under the assumption that the shift is necessary. Functionally this should be equivalent to the updates to the enum values you made except when the MAA is initialized with a size less than 1024.

Just pinged design to validate this check. Checking the MAA example as well - you mentioned that wasn't working and it uses a size of 16.

Let me know if the WolfSSL checks pass with the latest diff. They are very useful to validate our functionality.

--- a/Libraries/PeriphDrivers/Source/TPU/tpu_reva.c
+++ b/Libraries/PeriphDrivers/Source/TPU/tpu_reva.c
@@ -262,6 +262,7 @@ int MXC_TPU_RevA_Cipher_Config(mxc_tpu_reva_regs_t *tpu, mxc_tpu_reva_modesel_t
 int MXC_TPU_RevA_Cipher_KeySelect(mxc_tpu_reva_regs_t *tpu, mxc_tpu_reva_keysrc_t key_src)
 {
     MXC_SETFIELD(tpu->cipher_ctrl, MXC_F_TPU_REVA_CIPHER_CTRL_SRC, key_src);
+    // Note: "key_src" enum is set with "S" definitions instead of "V" definitions, so shifting is not necessary
 
     return E_SUCCESS;
 }
@@ -716,7 +717,13 @@ int MXC_TPU_RevA_MAA_Compute(mxc_tpu_reva_regs_t *tpu, mxc_tpu_maa_clcsel_t clc,
     memcpy((void *)MAA_M, (uint32_t *)mod, len);
 
     // Start MAA
-    MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC, clc);
+    uint8_t clc_val = (uint8_t)clc;
+    if (((tpu->maa_maws & MXC_F_TPU_MAA_MAWS_MSGSZ) >> MXC_F_TPU_MAA_MAWS_MSGSZ_POS) >= 1024) {
+        // The actual value of the field must be left shifted by 1 if MAWS >= 1024
+        clc_val <<= 1;
+    }
+    MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC,
+                 clc_val << MXC_F_TPU_REVA_MAA_CTRL_CLC_POS);
     tpu->maa_ctrl |= MXC_F_TPU_REVA_MAA_CTRL_STC;

night1rider · 2024-08-05T21:36:25Z

@Jake-Carter
Your solution still seems to have issues however the problem is simpler than initially guessed. I provided details on how I found this solution to the issue.

Items to Reproduce:

I am using the MAX32666 FTHR dev board
Code I am using to check results of MAA other than just trusting my current code inside wolfSSL just in case.

#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include "mxc_device.h"
#include "led.h"
#include "pb.h"
#include "board.h"
#include "mxc_delay.h"
#include "trng.h"
#include "tpu.h"

#define LED 0 


/* using this to quickly switch between my temp solution and correct SDK macro */
//#define MUL_OP 0b0100
#define MUL_OP MXC_TPU_MAA_MUL


/* Generic Function to do 1*2 mulmod with variable */
/* buffer size */
void maa_test(unsigned int len)
{
    /* MAA Testing */
    char* multiplier;
    char* multiplicand;
    char* exp;
    char* mod;
    char* result;
    int retval;
    len = len/8;

    /* Init buffers */
    multiplier      = (char*)malloc(len);
    multiplicand    = (char*)malloc(len);
    exp             = (char*)malloc(len);
    mod             = (char*)malloc(len);
    result          = (char*)malloc(len);

    memset(multiplier, 0x00, len); /* zero out buffers */
    memset(multiplicand, 0x00, len);
    memset(exp, 0x00, len); 
    memset(mod, 0xFF, len); /* we just want this to be max value */
    memset(result, 0x00, len); 

    multiplier[0]   = 0x01;
    multiplicand[0] = 0x02;
    exp[0] = 0x03;

    printf("\nInitial Result: %d\n", len);
    for(int i = 0; i < len; i++){
        printf("%02X ", result[i]);
    }
    printf("\n");

    printf("\nInitial Exp: %d\n", len);
    for(int i = 0; i < len; i++){
        printf("%02X ", exp[i]);
    }
    printf("\n");

    printf("\nInitial Mod: %d\n", len);
    for(int i = 0; i < len; i++){
        printf("%02X ", mod[i]);
    }
    printf("\n");

    printf("\nInitial Multiplier: %d\n", len);
    for(int i = 0; i < len; i++){
        printf("%02X ", multiplier[i]);
    }
    printf("\n");

    printf("\nInitial Multiplicand: %d\n", len);
    for(int i = 0; i < len; i++){
        printf("%02X ", multiplicand[i]);
    }
    printf("\n");

    retval = MXC_TPU_MAA_Init(len*8);
    if (retval != E_SUCCESS) {
        printf("Failed MAA_Init().\n");
        printf("Example Failed\n");
        return retval;
    }

    retval = MXC_TPU_MAA_Compute(MUL_OP, multiplier,
                                    multiplicand, exp, mod, result, len);
    if (retval != E_SUCCESS) {
        printf("Failed MAA_Compute().\n");
        printf("Example Failed\n");
        return retval;
    }

    printf("\nCalculated Result: %d\n", len);
    for(int i = 0; i < len; i++){
        printf("%02X ", result[i]);
    }
    printf("\n\n");

    free(multiplier);
    free(multiplicand);
    free(exp);
    free(mod);
    free(result);
    return;


}

int main(void)
{
    /* preform a basic 1*2 operation */
    /* use variable buffer size based on */
    /* minimum number of bits needed */
    maa_test(1024);
    maa_test(128);

    while (1) {
        LED_On(LED);
        MXC_Delay(500000);
        LED_Off(LED);
        MXC_Delay(500000);
    }
}

As a base line I am only testing MULMOD with this for now.

Current Upstream Results:

So the results of using the current upstream:

Initial Result: 128
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Exp: 128
03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Mod: 128
FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF

Initial Multiplier: 128
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Multiplicand: 128
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Calculated Result: 128
04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 


Initial Result: 16
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 

Initial Exp: 16
03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 

Initial Mod: 16
FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 

Initial Multiplier: 16
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 

Initial Multiplicand: 16
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 

Calculated Result: 16
04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

You can see that when the MAA is given 1*2 mod (2^(16*8)) = 4, which we know cannot be the case by quick observation.

My temp fix Results:

With my temp fix as described in the initial issue:

Initial Result: 128
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Exp: 128
03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Mod: 128
FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF

Initial Multiplier: 128
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Multiplicand: 128
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Calculated Result: 128
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00


Initial Result: 16
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Exp: 16
03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Mod: 16
FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF

Initial Multiplier: 16
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Multiplicand: 16
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Calculated Result: 16
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

We can see here that with my temp fix when the MAA is given 1*2 mod (2^(16*8)) and 1*2 mod (2^(128*8)) both give the correct value of 2

Initial Proposed Solution Results:

Now testing the first proposed fixed (fixing the usage of MXC_SETFIELD)-

MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC, clc << MXC_F_TPU_REVA_MAA_CTRL_CLC);

This produces these results:

Initial Result: 16
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Exp: 16
03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Mod: 16
FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF

Initial Multiplier: 16
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Multiplicand: 16
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Calculated Result: 16
03 00 00 00 00 00 00 20 00 00 00 00 00 00 00 00

We can see when the MAA is given 1*2 mod (2^(16*8)) it produces the result of 3

Second Proposed Solution Results:

Now testing the lastest suggestion -

--- a/Libraries/PeriphDrivers/Source/TPU/tpu_reva.c
+++ b/Libraries/PeriphDrivers/Source/TPU/tpu_reva.c
@@ -262,6 +262,7 @@ int MXC_TPU_RevA_Cipher_Config(mxc_tpu_reva_regs_t *tpu, mxc_tpu_reva_modesel_t
 int MXC_TPU_RevA_Cipher_KeySelect(mxc_tpu_reva_regs_t *tpu, mxc_tpu_reva_keysrc_t key_src)
 {
     MXC_SETFIELD(tpu->cipher_ctrl, MXC_F_TPU_REVA_CIPHER_CTRL_SRC, key_src);
+    // Note: "key_src" enum is set with "S" definitions instead of "V" definitions, so shifting is not necessary
 
     return E_SUCCESS;
 }
@@ -716,7 +717,13 @@ int MXC_TPU_RevA_MAA_Compute(mxc_tpu_reva_regs_t *tpu, mxc_tpu_maa_clcsel_t clc,
     memcpy((void *)MAA_M, (uint32_t *)mod, len);
 
     // Start MAA
-    MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC, clc);
+    uint8_t clc_val = (uint8_t)clc;
+    if (((tpu->maa_maws & MXC_F_TPU_MAA_MAWS_MSGSZ) >> MXC_F_TPU_MAA_MAWS_MSGSZ_POS) >= 1024) {
+        // The actual value of the field must be left shifted by 1 if MAWS >= 1024
+        clc_val <<= 1;
+    }
+    MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC,
+                 clc_val << MXC_F_TPU_REVA_MAA_CTRL_CLC_POS);
     tpu->maa_ctrl |= MXC_F_TPU_REVA_MAA_CTRL_STC;

This will produce this result:

Initial Result: 128
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Exp: 128
03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Mod: 128
FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF

Initial Multiplier: 128
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Multiplicand: 128
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Calculated Result: 128
03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00


Initial Result: 16
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Exp: 16
03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Mod: 16
FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF

Initial Multiplier: 16
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Initial Multiplicand: 16
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Calculated Result: 16
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Here we can see that when the MAA is given 1*2 mod (2^(16*8)) it will produce 2 as expected, however when the maa is given 1*2 mod (2^(128*8)) it will produce the value of 3 which is not true.

Solution to Bug:

Now noticing this I found the actual issue:
The initial solution proposed in this commit suggested this to fix for the usage of set field:
MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC, clc << MXC_F_TPU_REVA_MAA_CTRL_CLC);

but in the second proposed solution you used this for MXC_SETFIELD:
MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC, clc_val << MXC_F_TPU_REVA_MAA_CTRL_CLC_POS);

Turns out you just need to use:
MXC_F_TPU_REVA_MAA_CTRL_CLC_POS instead of MXC_F_TPU_REVA_MAA_CTRL_CLC
So seems like a simple typo in the intial proposed fix.

I then tested this:
MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC, clc << MXC_F_TPU_REVA_MAA_CTRL_CLC_POS); byitself and it passes the wolfSSL test suite and my quick observation test posted above.

TLRD:

Use:
MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC, clc << MXC_F_TPU_REVA_MAA_CTRL_CLC_POS);

instead of:
MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC, clc << MXC_F_TPU_REVA_MAA_CTRL_CLC);

without the 1024 check (no extra bitshift) and this will pass the wolfSSL tests and the observation test I made as detailed.

This reverts commit 23c3379.

Jake-Carter · 2024-08-05T22:18:43Z

@night1rider

(applies palm to face) yep, you're absolutely right... my mistake, sorry about that typo on that first pass... CLC_POS is a left shift by one and gave the false impression.

Confirmed on the original design spec the values in the UG are correct as well. Appreciate the testing and patience

Jake-Carter · 2024-08-05T22:19:13Z

@ozersa ready for re-review

ozersa

Looks good, thanks @Jake-Carter

…1104)" This reverts commit cac32a7.

Jake-Carter added 2 commits August 2, 2024 16:57

TPU RevA: Fix incorrect usages of MXC_SETFIELD

ced870e

clang-format

4f90f39

Jake-Carter requested review from sihyung-maxim, lorne-maxim and ozersa August 2, 2024 23:03

ozersa reviewed Aug 5, 2024

View reviewed changes

Libraries/PeriphDrivers/Source/TPU/tpu_reva.c Outdated Show resolved Hide resolved

ozersa requested changes Aug 5, 2024

View reviewed changes

Revert change to key_src setter

851de15

Left shift clc value if MAA size >= 1024

23c3379

Jake-Carter force-pushed the fix/gh-1089 branch from 4eedbba to 23c3379 Compare August 5, 2024 17:44

Jake-Carter changed the title ~~fix(PeriphDrivers): Fix incorrect usages of MXC_SETFIELD in TPU RevA Drivers~~ fix(PeriphDrivers): Fix MAA Operator Setter for TPU Drivers Aug 5, 2024

Jake-Carter added 2 commits August 5, 2024 16:06

Revert "Left shift clc value if MAA size >= 1024"

5f5f7cd

This reverts commit 23c3379.

Fix CLC vs CLC_POS typo

e806278

Jake-Carter requested a review from ozersa August 5, 2024 22:19

ozersa approved these changes Aug 6, 2024

View reviewed changes

Jake-Carter merged commit cac32a7 into analogdevicesinc:main Aug 8, 2024
14 of 16 checks passed

Jake-Carter deleted the fix/gh-1089 branch August 8, 2024 00:02

sihyung-maxim pushed a commit to analogdevicesinc/hal_adi that referenced this pull request Aug 8, 2024

Update Zephyr MSDK Hal based on MSDK PR: analogdevicesinc/msdk#1104

16fa080

EricB-ADI pushed a commit that referenced this pull request Aug 21, 2024

Revert "fix(PeriphDrivers): Fix MAA Operator Setter for TPU Drivers (#…

83a4756

…1104)" This reverts commit cac32a7.

ozersa pushed a commit to analogdevicesinc/hal_adi that referenced this pull request Sep 11, 2024

Update Zephyr MSDK Hal based on MSDK PR: analogdevicesinc/msdk#1104

bc55382

ozersa pushed a commit to analogdevicesinc/hal_adi that referenced this pull request Sep 11, 2024

Update Zephyr MSDK Hal based on MSDK PR: analogdevicesinc/msdk#1104

706b6e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(PeriphDrivers): Fix MAA Operator Setter for TPU Drivers #1104

fix(PeriphDrivers): Fix MAA Operator Setter for TPU Drivers #1104

Jake-Carter commented Aug 2, 2024 •

edited

Loading

Jake-Carter commented Aug 2, 2024

ozersa left a comment

night1rider commented Aug 5, 2024 •

edited

Loading

Jake-Carter commented Aug 5, 2024

night1rider commented Aug 5, 2024

Jake-Carter commented Aug 5, 2024

night1rider commented Aug 5, 2024 •

edited

Loading

Jake-Carter commented Aug 5, 2024

Jake-Carter commented Aug 5, 2024

ozersa left a comment

fix(PeriphDrivers): Fix MAA Operator Setter for TPU Drivers #1104

fix(PeriphDrivers): Fix MAA Operator Setter for TPU Drivers #1104

Conversation

Jake-Carter commented Aug 2, 2024 • edited Loading

Description

Checklist Before Requesting Review

Jake-Carter commented Aug 2, 2024

ozersa left a comment

Choose a reason for hiding this comment

night1rider commented Aug 5, 2024 • edited Loading

Jake-Carter commented Aug 5, 2024

night1rider commented Aug 5, 2024

Jake-Carter commented Aug 5, 2024

night1rider commented Aug 5, 2024 • edited Loading

TLRD:

Jake-Carter commented Aug 5, 2024

Jake-Carter commented Aug 5, 2024

ozersa left a comment

Choose a reason for hiding this comment

Jake-Carter commented Aug 2, 2024 •

edited

Loading

night1rider commented Aug 5, 2024 •

edited

Loading

night1rider commented Aug 5, 2024 •

edited

Loading