[riot-notifications] [RIOT-OS/RIOT] cpu/qn908x: Implement blocking SPI support (#15689)

benpicco notifications at github.com
Sun Jan 3 00:15:36 CET 2021


@benpicco commented on this pull request.

Looks good!
Would be nice to have a MTD config for the on-board SPI flash.

> @@ -56,6 +56,16 @@ extern "C" {
 #error "GPIO_T_ADDR(GPIO_PIN(1, x)) must be the GPIOB address"
 #endif
 
+/**
+ * @brief   Return whether the given pin is a CSHW pin.
+ */
+#define GPIO_T_IS_HWCS(pin) (((pin) & 0xff00u) == 0x8000)

This I don't understand.
Does it mean HW CS is only available on a certain port? 

> +only implements the Controller mode, the hardware is capable of operating in
+Peripheral mode as well so we use the COPI/CIPO names.
+
+### SPI configuration example (for periph_conf.h) ###
+
+The following example uses only one hardware CS (number 0) and leaves the rest
+unused. Check the user manual for the full list of CS pins available.
+
+    static const spi_conf_t spi_config[] = {
+        {
+            .dev            = SPI0,
+            .copi_pin       = GPIO_PIN(PORT_A, 4),
+            .cipo_pin       = GPIO_PIN(PORT_A, 5),
+            .clk_pin        = GPIO_PIN(PORT_A, 30),
+            .cs_pin         = {
+                GPIO_PIN(PORT_A, 3),

It's worth documenting that, to make use of this pin as CS, `SPI_HWCS(0)` should be used instead of `GPIO_PIN(PORT_A, 3)` for the driver configuration. (If I understood that correctly) 

Also, what's the advantage of using HW CS instead of software CS only? 

> @@ -43,6 +43,28 @@ static const i2c_conf_t i2c_config[] = {
 #define I2C_NUMOF           ARRAY_SIZE(i2c_config)
 /** @} */
 
+/**
+ * @name   SPI configuration
+ * @{
+ */
+static const spi_conf_t spi_config[] = {
+    {
+        .dev            = SPI0,  /* Flexcomm 2 */
+        .copi_pin       = GPIO_PIN(PORT_A, 4),
+        .cipo_pin       = GPIO_PIN(PORT_A, 5),
+        .clk_pin        = GPIO_PIN(PORT_A, 30),
+        .cs_pin         = {
+            GPIO_PIN(PORT_A, 3),  /* MX25R2035F CS# connected here. */

That should be supported by `mtd_spi_nor`, would be a good addition both as an example as well as for testing that the driver works as expected.

> +This driver uses the [OSHA SPI Signal Names](
+https://www.oshwa.org/a-resolution-to-redefine-spi-signal-names/) and while it
+only implements the Controller mode, the hardware is capable of operating in
+Peripheral mode as well so we use the COPI/CIPO names.

I'm a bit conflicted here as this makes this CPU 'special' when all the other CPUs use the MISO/MOSI names (and those names are also used by the data sheet).
I doubt we would ever rename those though as it would break all third party configurations, so if those alternative names should be used it would be in a new architecture like this.


> +        }
+    }
+    _spi_wait_txempty(spi_bus);
+}
+
+void spi_transfer_bytes(spi_t bus, spi_cs_t cs, bool cont,
+                        const void *out, void *in, size_t len)
+{
+    spi_pending_transfer_t tr;
+
+    _spi_config_transfer(&tr, cs, cont, out, in, len);
+
+    /* At least one of input or one output buffer is given */
+    assert(bus < SPI_NUMOF);
+
+    if (!GPIO_T_IS_HWCS(cs)) {

You could also do this in `_spi_config_transfer()` so it's all in one place.

> +    spi_bus->FIFOCFG = SPI_FIFOCFG_ENABLETX_MASK |
+                       SPI_FIFOCFG_ENABLERX_MASK |
+                       SPI_FIFOCFG_EMPTYTX_MASK | SPI_FIFOCFG_EMPTYRX_MASK;


How about

```suggestion
    spi_bus->FIFOCFG = SPI_FIFOCFG_ENABLETX_MASK
                     | SPI_FIFOCFG_ENABLERX_MASK
                     | SPI_FIFOCFG_EMPTYTX_MASK | SPI_FIFOCFG_EMPTYRX_MASK;
```

> +    return SPI_OK;
+}
+
+#ifdef MODULE_PERIPH_SPI_RECONFIGURE
+void spi_deinit_pins(spi_t bus)
+{
+    assert(bus < SPI_NUMOF);
+    mutex_lock(&locks[bus]);
+    const spi_conf_t *const conf = &spi_config[bus];
+
+    /* Disables the SPI block. It must be already idle. */
+    conf->dev->CFG &= ~SPI_CFG_ENABLE_MASK;
+
+    gpio_init(conf->copi_pin, GPIO_IN);
+    gpio_init(conf->cipo_pin, GPIO_IN);
+    gpio_init(conf->clk_pin, GPIO_IN);

Do we need to also disable the mux on the HWCS pins?

-- 
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/15689#pullrequestreview-560659379
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210102/81042673/attachment.htm>


More information about the notifications mailing list