[riot-notifications] [RIOT-OS/RIOT] TI CC3200 SimpleLink RIOT support (#11866)

benpicco notifications at github.com
Fri Jul 19 13:02:20 CEST 2019


benpicco commented on this pull request.

I think you can add proper Chip Select handling easily.
Just what exactly does the `MCSPI_CH0CONF_FORCE` option do? 

> +
+/**
+ * @brief Initialize CPU and reset device periphiria
+ *
+ */
+void cpu_init(void)
+{
+	/* initializes the Cortex-M core */
+	cortexm_init();
+
+	/* trigger static peripheral initialization */
+	periph_init();
+}
+
+/**
+ * @brief init periphiria clock and perform a softreset

peripheral 

> +	cortexm_init();
+
+	/* trigger static peripheral initialization */
+	periph_init();
+}
+
+/**
+ * @brief init periphiria clock and perform a softreset
+ *
+ * @param reg pointer to register
+ */
+void reset_periph_clk(cc3200_periph_regs_t *reg)
+{
+	volatile unsigned long ulDelay;
+
+	/* enable & reset periphiria */

peripherals

> +			void *in, size_t len)
+{
+	/* TODO: handle cs and cont for */
+	(void)cont;
+
+	/* disable or enable cs */
+	if (cs == GPIO_UNDEF) {
+		spi(bus)->ch0_conf &= ~MCSPI_CH0CONF_FORCE;
+	} else {
+		spi(bus)->ch0_conf |= MCSPI_CH0CONF_FORCE;
+	}
+	ROM_SPITransfer((unsigned long)spi(bus), (unsigned char *)out,
+			(unsigned char *)in, len, 0);
+
+	/* if cs was enabled disable it again */
+	if (cs != GPIO_UNDEF) {

Change it to
`if ((!cont) && (cs != GPIO_UNDEF))`

to handle `cont`, then add 
`gpio_set((gpio_t)cs)` for the actual GPIO handling.

I'm not sure if it's necessary to modify `ch0_conf` after the transfer is done.

> +{
+	spi_transfer_bytes(bus, cs, cont, &out, 0, 1);
+	return SPI_OK;
+}
+
+void spi_transfer_bytes(spi_t bus, spi_cs_t cs, bool cont, const void *out,
+			void *in, size_t len)
+{
+	/* TODO: handle cs and cont for */
+	(void)cont;
+
+	/* disable or enable cs */
+	if (cs == GPIO_UNDEF) {
+		spi(bus)->ch0_conf &= ~MCSPI_CH0CONF_FORCE;
+	} else {
+		spi(bus)->ch0_conf |= MCSPI_CH0CONF_FORCE;

You should add a 
`gpio_clear((gpio_t)cs)`
here.

> +		spi(bus)->ch0_conf &= ~MCSPI_CH0CONF_FORCE;
+	} else {
+		spi(bus)->ch0_conf |= MCSPI_CH0CONF_FORCE;
+	}
+	ROM_SPITransfer((unsigned long)spi(bus), (unsigned char *)out,
+			(unsigned char *)in, len, 0);
+
+	/* if cs was enabled disable it again */
+	if (cs != GPIO_UNDEF) {
+		spi(bus)->ch0_conf &= ~MCSPI_CH0CONF_FORCE;
+	}
+}
+
+uint8_t spi_transfer_reg(spi_t bus, spi_cs_t cs, uint8_t reg, uint8_t out)
+{
+	(void)cs;

You can use the cs gpio handling from `spi_transfer_bytes` here.

> +	if (bus >= SPI_NUMOF) {
+		return -1;
+	}
+	ROM_SPITransfer(spi_config[bus].base_addr, &reg, 0, 1, 0);
+
+	if (ROM_SPITransfer(spi_config[bus].base_addr, (unsigned char *)&out, 0,
+			    1, 0)) {
+		return -1;
+	}
+	return 1; /* success transfer */
+}
+
+void spi_transfer_regs(spi_t bus, spi_cs_t cs, uint8_t reg, const void *out,
+		       void *in, size_t len)
+{
+	(void)cs;

dito

> +#define SPI_CS_ENABLE 0x00000001
+#define SPI_CS_DISABLE 0x00000002
+
+/**
+ * @brief   Allocate one lock per SPI device
+ */
+static mutex_t locks[SPI_NUMOF];
+
+/**
+ * @brief init pins for a bus
+ *
+ * @param bus
+ */
+void spi_init_pins(spi_t bus)
+{
+	/* check if sck and mosi pins are zero indicating no pin setup */

Is this case even possible?
If no SPI configuration exists, `spi_init()` would not be called.

> @@ -0,0 +1,8 @@
+include $(RIOTCPU)/cortexm_common/Makefile.features
+
+
+# Additional MCU features
+FEATURES_PROVIDED += periph_cpuid
+FEATURES_PROVIDED += periph_hwrng
+FEATURES_PROVIDED += puf_sram
+

You can add
`FEATURES_PROVIDED += periph_gpio periph_gpio_irq`

> @@ -0,0 +1,12 @@
+FEATURES_PROVIDED += cpp
+FEATURES_PROVIDED += periph_uart
+FEATURES_PROVIDED += periph_gpio
+FEATURES_PROVIDED += periph_random
+FEATURES_PROVIDED += periph_timer
+FEATURES_PROVIDED += periph_cpuid
+FEATURES_PROVIDED += periph_spi
+
+FEATURES_MCU_GROUP = cortex_m4_2

This is not used anymore, you can remove it.

> + */
+
+#include "board.h"
+#include "cpu.h"
+
+#include "periph/gpio.h"
+#include "vendor/hw_gpio.h"
+#include "vendor/hw_hib3p3.h"
+#include "vendor/hw_memmap.h"
+#include "vendor/hw_types.h"
+#include "vendor/rom.h"
+
+
+static void board_reset(void);
+
+#ifdef HAVE_GPIO_T

I can't think of any case where this would not be defined. Just remove the #ifdef 

> +
+	/* reset board */
+	board_reset();
+
+#ifdef HAVE_GPIO_T
+	/* initialize the boards LEDs */
+	led_init();
+#endif
+}
+
+/**
+ * @brief Get the sys reset cause object
+ *
+ * @return uint32_t restart reason code
+ */
+static uint32_t get_sys_reset_cause(void)

I think this belongs to the CPU, not the board.

> +			HWREG(HIB3P3_BASE + HIB3P3_O_MEM_HIB_WAKE_STATUS);
+		/* FIXME: wait for some reason test if this is needed*/
+		ROM_UtilsDelay(PRCM_OP_DELAY);
+		if (hiber_status & 0x1) {
+			return PRCM_HIB_EXIT;
+		}
+	}
+	return reset_cause;
+}
+
+/**
+ * @brief board_reset resets board periphiria
+ * based upon PRCMCC3200MCUInit from TIs prcm.c
+ *
+ */
+static void board_reset(void)

How much of this is board specific?
Common init code should go to cpu init, so it doesn't get copy & pasted for every new board.

> +		}
+	}
+	return reset_cause;
+}
+
+/**
+ * @brief board_reset resets board periphiria
+ * based upon PRCMCC3200MCUInit from TIs prcm.c
+ *
+ */
+static void board_reset(void)
+{
+	cc3200_reg_t tmp;
+
+	/* DIG DCDC LPDS ECO Enable */
+	HWREG(0x4402F064) |= 0x800000;

please replace those magic numbers by a `#define`d constant

> +/**
+ *
+ */
+#define SPI_NUMOF (sizeof(spi_config) / sizeof(spi_conf_t))
+/** @} */
+
+/**
+ * @name UART configuration
+ * @{
+ */
+
+/**
+ * @name UART configuration
+ * @{
+ */
+#define UART_NUMOF 1

Why not also enable the second one when you have already made a configuration for it?

> +static inline cc3200_spi_t *spi(spi_t bus)
+{
+	return (cc3200_spi_t *)spi_config[bus].base_addr;
+}
+
+/**
+ * @brief reset spi to default state
+ *
+ * @param bus spi bus id
+ */
+void spi_reset(spi_t bus)
+{
+	volatile cc3200_spi_t *dev = spi(bus);
+
+	/* disable chip select in software controlled mode */
+	dev->ch0_conf &= ~MCSPI_CH0CONF_FORCE;

Does this disable automatic chip select or does it directly control the state of the cs pin?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11866#pullrequestreview-264131056
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190719/d69c64a7/attachment-0001.htm>


More information about the notifications mailing list