analogdevicesinc / msdk

Software Development Kit for Analog Device's MAX-series microcontrollers
Apache License 2.0
60 stars 78 forks source link

MAX32665 and MAX32666 SDK TPU/MAA Bug #1089

Closed night1rider closed 1 month ago

night1rider commented 1 month ago

Hello,

I am working on using the TPU on the MAX32665 and MAX32666 to do hardware acceleration in wolfSSL. I found an issue potentially with the process of setting different math modes for the MAA.

I think it has to do with this line: #define MXC_SETFIELD(reg, mask, setting) (reg = ((reg) & ~(mask)) | ((setting) & (mask)))

I suspect it is not preforming the bit masking correct for the values give for the MAA controls. You can test this by trying your MAA example that is in the master branch and switching it to do a multiple operation. You will find it actually does a square operation.

With the current bit masking it will not use values correctly and cut off the last bit of the math control values do not get shifted left by 1 bit so this values will not work as the math operation control register in in bit position [3:1]:

        MXC_TPU_MAA_EXP      0b000
        MXC_TPU_MAA_SQ       0b001
        MXC_TPU_MAA_MUL      0b010
        MXC_TPU_MAA_SQMUL    0b011
        MXC_TPU_MAA_ADD      0b100
        MXC_TPU_MAA_SUB      0b101

To currently use the MAA I have to define control values as such:

        #define WC_MXC_TPU_MAA_EXP      0b0000
        #define WC_MXC_TPU_MAA_SQ       0b0010
        #define WC_MXC_TPU_MAA_MUL      0b0100
        #define WC_MXC_TPU_MAA_SQMUL    0b0110
        #define WC_MXC_TPU_MAA_ADD      0b1000
        #define WC_MXC_TPU_MAA_SUB      0b1010

and edit this line in the SDK: if (clc >= 0x6) { to: if (clc >= 0b1111) {

To properly use the MAA hardware. This will end up setting the correct bits to use the current bit masking controls.

chazste commented 1 month ago

I do not believe that the issue is the macro, rather the incorrect use of the macro https://github.com/analogdevicesinc/msdk/blob/8fc1336aefd6fcb17d82c1f877bfd8fa5b019d05/Libraries/PeriphDrivers/Source/TPU/tpu_reva.c#L719

Should instead be:

MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC, (clc << MXC_F_TPU_REVA_MAA_CTRL_CLC_POS));
Jake-Carter commented 1 month ago

Thank you @night1rider, I appreciate your efforts adding part support to WolfSSL.

@chazste I agree - looks like there was also the same issue for MXC_TPU_RevA_Cipher_KeySelect.

Just opened a PR with the fix.

night1rider commented 1 month ago

@chazste Hello, I tried this suggestion and made sure I use the appropriate macros/enums and values but it seems this does not fix this issue. When running wolfssl tests with my temp fix the calculations will pass, however with this fix it will preform calculations of some kind, but the expected calculations are not correct and the wolfssl tests will fail now.

night1rider commented 1 month ago

@chazste Your solution actually works, I had tested with: MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC, clc << MXC_F_TPU_REVA_MAA_CTRL_CLC); instead of MXC_SETFIELD(tpu->maa_ctrl, MXC_F_TPU_REVA_MAA_CTRL_CLC, clc << MXC_F_TPU_REVA_MAA_CTRL_CLC_POS);

Due to a typo in the initial pr fix.

Thanks!

chazste commented 1 month ago

@night1rider thank you for checking again. I'm glad that worked.