feat(PeriphDrivers): Provide user a way to manually drive SPI slave select pin

[ozersa]

Description

While using SPI manual chip drive requires, existing SPI driver drive SS pin by HW, there is no any option to disable this behaviour, This feature exactly require while porting Zephyr/MBED... On these platform i add an patch at the top of SDK layer.
It will be nice if SDK support it on default.

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.

[sihyung-maxim]

Just some minor comment revision.

It might also be useful to add a function that lets you select the slave select pin to drive if the HW CS control scheme is disabled.

[sihyung-maxim]

/**
 * @brief   Enable/Disable HW CS control feature.
 *
 * Depending on the application, the user might need to manually drive the slave select pin.
 * The SPI driver automatically drives the SS pin and this function enables/disables this
 * feature.
 *
 * @param   spi             Pointer to SPI registers (selects the SPI block used.)
 * @param   state           true or false
 */

I revised the tone and grammar. Maybe something like this? ^^

[ozersa]

Fine, thanks.

[sihyung-maxim]

@Jacob-Scheiffler @EdwinFairchild

I think I remember you two making updates to the bootloader examples (Bootloader and BLE_OTAS) and having some flash size problems for the ME17. These new SPI changes caused the project size of those two examples to exceed the flash size which throws build errors. Is there a way to provide some more space in those examples?

The library size is going to increase over time as we add fixes and features to the MSDK, so we're going to run into this issue again in the future. We may need to have a discussion about improving the memory footprint of our drivers and, as a start, enforce the proper use of integer type sizes (uint8_t/int8_t, uint16_t/int16_t, uint32_t/int32_t) in our library.

[ozersa]

Just some minor comment revision.

It might also be useful to add a function that lets you select the slave select pin to drive if the HW CS control scheme is disabled.

It is also an option but CS pin might be a standard GPIO too, so it will be more generic if CS pin configure and drive by app layer instead of driver layer.

[ozersa]

Can it be merged?
Thanks.

[Jake-Carter]

We need to resolve the build failure

[Jake-Carter]

@Jacob-Scheiffler @EdwinFairchild

I think I remember you two making updates to the bootloader examples (Bootloader and BLE_OTAS) and having some flash size problems for the ME17. These new SPI changes caused the project size of those two examples to exceed the flash size which throws build errors. Is there a way to provide some more space in those examples?

The library size is going to increase over time as we add fixes and features to the MSDK, so we're going to run into this issue again in the future. We may need to have a discussion about improving the memory footprint of our drivers and, as a start, enforce the proper use of integer type sizes (uint8_t/int8_t, uint16_t/int16_t, uint32_t/int32_t) in our library.

Our SPI drivers are already the biggest drivers in the SDK. Even with -Os they're almost 7KB.

I think we should be trying to reduce this instead of forcing projects to adapt to driver bloat.

Surely we can trim 48 bytes of code somewhere to get this build to pass.

arm-none-eabi-size libPeriphDriver_softfp.a                               
                                                                                                                                                   
   text    data     bss     dec     hex filename
      2       0       0       2       2 mxc_assert.o (ex libPeriphDriver_softfp.a)
    468       4      12     484     1e4 mxc_delay.o (ex libPeriphDriver_softfp.a)
     38       0       0      38      26 mxc_lock.o (ex libPeriphDriver_softfp.a)
    116       0     504     620     26c nvic_table.o (ex libPeriphDriver_softfp.a)
    696       0       0     696     2b8 pins_me17.o (ex libPeriphDriver_softfp.a)
   1372       0       0    1372     55c sys_me17.o (ex libPeriphDriver_softfp.a)
    860       0       0     860     35c adc_me17.o (ex libPeriphDriver_softfp.a)
   2368       0      25    2393     959 adc_reva.o (ex libPeriphDriver_softfp.a)
    288       0       0     288     120 aes_me17.o (ex libPeriphDriver_softfp.a)
   1028       0      16    1044     414 aes_revb.o (ex libPeriphDriver_softfp.a)
    160       0       0     160      a0 crc_me17.o (ex libPeriphDriver_softfp.a)
    356       0       4     360     168 crc_reva.o (ex libPeriphDriver_softfp.a)
    330       0       0     330     14a dma_me17.o (ex libPeriphDriver_softfp.a)
   1736       0     120    1856     740 dma_reva.o (ex libPeriphDriver_softfp.a)
    266       0       0     266     10a flc_common.o (ex libPeriphDriver_softfp.a)
    542       0       0     542     21e flc_me17.o (ex libPeriphDriver_softfp.a)
    848       4       0     852     354 flc_reva.o (ex libPeriphDriver_softfp.a)
    399       0    1025    1424     590 gpio_common.o (ex libPeriphDriver_softfp.a)
   1160       0       0    1160     488 gpio_me17.o (ex libPeriphDriver_softfp.a)
    290       0       0     290     122 gpio_reva.o (ex libPeriphDriver_softfp.a)
    652       4       0     656     290 i2c_me17.o (ex libPeriphDriver_softfp.a)
   4382       0     108    4490    118a i2c_reva.o (ex libPeriphDriver_softfp.a)
    528       4       0     532     214 i2s_me17.o (ex libPeriphDriver_softfp.a)
   2137       0      56    2193     891 i2s_reva.o (ex libPeriphDriver_softfp.a)
     30       0       0      30      1e icc_me17.o (ex libPeriphDriver_softfp.a)
    110       0       0     110      6e icc_reva.o (ex libPeriphDriver_softfp.a)
   1097       0       0    1097     449 lp_me17.o (ex libPeriphDriver_softfp.a)
    312       0       4     316     13c lpcmp_me17.o (ex libPeriphDriver_softfp.a)
     88       0       0      88      58 lpcmp_reva.o (ex libPeriphDriver_softfp.a)
    508       0       0     508     1fc owm_me17.o (ex libPeriphDriver_softfp.a)
   1216       0       8    1224     4c8 owm_reva.o (ex libPeriphDriver_softfp.a)
    532       0       0     532     214 pt_me17.o (ex libPeriphDriver_softfp.a)
    771       0       0     771     303 pt_reva.o (ex libPeriphDriver_softfp.a)
    728       0       0     728     2d8 rtc_me17.o (ex libPeriphDriver_softfp.a)
    864       0       0     864     360 rtc_reva.o (ex libPeriphDriver_softfp.a)
    164       0       0     164      a4 sema_me17.o (ex libPeriphDriver_softfp.a)
    459       8      24     491     1eb sema_reva.o (ex libPeriphDriver_softfp.a)
     96       0       0      96      60 simo_me17.o (ex libPeriphDriver_softfp.a)
    232       0       0     232      e8 simo_reva.o (ex libPeriphDriver_softfp.a)
   1198       0       0    1198     4ae spi_me17.o (ex libPeriphDriver_softfp.a)
   5603       0      80    5683    1633 spi_reva1.o (ex libPeriphDriver_softfp.a)
    138       0       0     138      8a trng_me17.o (ex libPeriphDriver_softfp.a)
    460       0      16     476     1dc trng_revb.o (ex libPeriphDriver_softfp.a)
    248       0       0     248      f8 tmr_common.o (ex libPeriphDriver_softfp.a)
   1055       0       0    1055     41f tmr_me17.o (ex libPeriphDriver_softfp.a)
   2789       0      48    2837     b15 tmr_revb.o (ex libPeriphDriver_softfp.a)
     54       0       0      54      36 uart_common.o (ex libPeriphDriver_softfp.a)
   1048       0       0    1048     418 uart_me17.o (ex libPeriphDriver_softfp.a)
   3998       0      80    4078     fee uart_revb.o (ex libPeriphDriver_softfp.a)
      0       0       0       0       0 wdt_common.o (ex libPeriphDriver_softfp.a)
    160       0       0     160      a0 wdt_me17.o (ex libPeriphDriver_softfp.a)
    248       0       0     248      f8 wdt_revb.o (ex libPeriphDriver_softfp.a)
    722       0      28     750     2ee wut_me17.o (ex libPeriphDriver_softfp.a)
    582       0       8     590     24e wut_reva.o (ex libPeriphDriver_softfp.a)

[EdwinFairchild]

Yeah there is always some issue with bootloader space on the ME17 since that baby does not have the second flash bank we partition it's single bank for bootloader and app. That bootloader is as lean as it can get, i think asserts are turned off ? among other little tweaks, but I agree maybe the real issue is the ever-growing SPI driver so maybe tackling otherwise increasing bootloader space to buy usa few more revisions lol

[Jacob-Scheiffler]

The description for the state variable should provide more detail. Please update this in all header files to something like, "Non-zero values: enable HW SS mode. Zero: disable HW SS mode."

[ozersa]

Added.

[Jake-Carter]

Yeah there is always some issue with bootloader space on the ME17 since that baby does not have the second flash bank we partition it's single bank for bootloader and app. That bootloader is as lean as it can get, i think asserts are turned off ? among other little tweaks, but I agree maybe the real issue is the ever-growing SPI driver so maybe tackling otherwise increasing bootloader space to buy usa few more revisions lol

Asserts are always added to PROJ_CFLAGS in the project's core Makefile. This is left over from the original legacy Makefiles. I'll add a better mechanism to disable them.

@EdwinFairchild @sihyung-maxim @Jacob-Scheiffler @ozersa... Please re-approve given the changes below:

I reduced the SPI_RevA object size by 830KB by removing unnecessary asserts (acaf898)

Almost every single function had something along the lines of

int spi_num;
spi_num = MXC_SPI_GET_IDX((mxc_spi_regs_t *)spi);
MXC_ASSERT(spi_num >= 0);
(void)spi_num;

• As a check to ensure the function has a valid MXC_SPIN this is useless. The functions accept mxc_spi_regs_t, so the compiler will complain about undefined symbols at compile time.

• As a check to ensure we don't access memory out of bounds of the state array it's more logical but still essentially useless. The only way the user can cause the assertion to return -1 is to give it an undefined MXC_SPIN. See point above.

[Jake-Carter]

Approved pending Jacob's comment:

[ozersa]

Yeah there is always some issue with bootloader space on the ME17 since that baby does not have the second flash bank we partition it's single bank for bootloader and app. That bootloader is as lean as it can get, i think asserts are turned off ? among other little tweaks, but I agree maybe the real issue is the ever-growing SPI driver so maybe tackling otherwise increasing bootloader space to buy usa few more revisions lol

Asserts are always added to PROJ_CFLAGS in the project's core Makefile. This is left over from the original legacy Makefiles. I'll add a better mechanism to disable them.

@EdwinFairchild @sihyung-maxim @Jacob-Scheiffler @ozersa... Please re-approve given the changes below:

I reduced the SPI_RevA object size by 830KB by removing unnecessary asserts (acaf898)

Almost every single function had something along the lines of

int spi_num;
spi_num = MXC_SPI_GET_IDX((mxc_spi_regs_t *)spi);
MXC_ASSERT(spi_num >= 0);
(void)spi_num;

• As a check to ensure the function has a valid MXC_SPIN this is useless. The functions accept mxc_spi_regs_t, so the compiler will complain about undefined symbols at compile time.
• As a check to ensure we don't access memory out of bounds of the state array it's more logical but still essentially useless. The only way the user can cause the assertion to return -1 is to give it an undefined MXC_SPIN. See point above.

Looks good, I like clean code.
Thanks.

[sihyung-maxim]

Yeah there is always some issue with bootloader space on the ME17 since that baby does not have the second flash bank we partition it's single bank for bootloader and app. That bootloader is as lean as it can get, i think asserts are turned off ? among other little tweaks, but I agree maybe the real issue is the ever-growing SPI driver so maybe tackling otherwise increasing bootloader space to buy usa few more revisions lol

Asserts are always added to PROJ_CFLAGS in the project's core Makefile. This is left over from the original legacy Makefiles. I'll add a better mechanism to disable them.

@EdwinFairchild @sihyung-maxim @Jacob-Scheiffler @ozersa... Please re-approve given the changes below:

I reduced the SPI_RevA object size by 830KB by removing unnecessary asserts (acaf898)

Almost every single function had something along the lines of

int spi_num;
spi_num = MXC_SPI_GET_IDX((mxc_spi_regs_t *)spi);
MXC_ASSERT(spi_num >= 0);
(void)spi_num;

• As a check to ensure the function has a valid MXC_SPIN this is useless. The functions accept mxc_spi_regs_t, so the compiler will complain about undefined symbols at compile time.
• As a check to ensure we don't access memory out of bounds of the state array it's more logical but still essentially useless. The only way the user can cause the assertion to return -1 is to give it an undefined MXC_SPIN. See point above.

I thought the assert was for cases when the user defines its own SPI instance instead of using the predefined instances in the CMSIS part header file. Although rare for this to happen, there have been previous projects in the past that defined their own instances, and it's meant for scenarios like this.

#define SLAVE_SPI_BASE ((uint32_t)0x12345678UL) // I just purposefully added incorrect SPI base address as an example.
#define SLAVE_SPI ((mxc_spi_regs_t *)SLAVE_SPI_BASE)

Edit: And also, the asserts should only be for functions that have a void return type. For all other functions that return a signed integer type, it should be replaced with an error-checking if statement that returns the appropriate error value.

[Jake-Carter]

I thought the assert was for cases when the user defines its own SPI instance instead of using the predefined instances in the CMSIS part header file.

Hmm okay, I didn't consider this case... 830KB is still too much overhead to handle it though. I'll add some lightweight checks back in

[ozersa]

@Jake-Carter one proposal, how about to manage optimization related change on a different PR then after it to be merged rebase this one.
This will be very useful to keep changes individual and able to track history incase of need.

[Jake-Carter]

@Jake-Carter one proposal, how about to manage optimization related change on a different PR then after it to be merged rebase this one. This will be very useful to keep changes individual and able to track history incase of need.

I think it would be acceptable to merge in the commits above because it gets the bootloader builds to pass. But I'm open to rebasing, I agree it's cleaner.

Up to you - it will take us a little more time to apply universally. We can merge this now, or I can revert my changes and we can rebase in a few days.

[Jake-Carter]

Alternatively, I can do a separate PR just for the SPI RevA file and we can rebase on that. We could merge that in a few minutes.

[Jake-Carter]

@ozersa My changes have been reverted in favor of the separate PR

[Jake-Carter]

Rebased this branch, @Jacob-Scheiffler ready to merge pending your approval