Zephyr: spi sdhc CS signal not working

[lowlander]

When using the stm32 spi SDHC driver the CS signal is not managed correctly when having more than one slave on the same SPI bus (in my case a SD card and a OLED display) Depending on DT settings the SDHC CS always seems low or it is partly inverted inverted.

The PR #27442 fixes things for me, but that PR needs work and review on how to really fix things.

[lochej]

How is your CS gpio defined in your devicetree ? Because some updates have been made such that CS gpio active level is now determined from the CS gpio pin definition in your DT spi node.

spi_context.h is where you can see this change.

@lowlander Isn't it wrong to still have some CS handling in the spi sdhc disk subsys when it is supposed to be driven by the SPI driver ?

[lowlander]

How is your CS gpio defined in your devicetree ? Because some updates have been made such that CS gpio active level is now determined from the CS gpio pin definition in your DT spi node.

spi_context.h is where you can see this change.

My DT has the two CS both low active;

cs-gpios = <&gpiod 14 GPIO_ACTIVE_LOW>, <&gpiod 3 GPIO_ACTIVE_LOW>;

Which also seems correct, because my patch uses that same info, and things work. I just don't understand
why the LOW_ACTIVE flag seems to be missing (so I added it by hand, I know "bad hack" ).

The SDHC driver calls SPI functions like this;

spi_transceive(data->spi, &data->cfg, &tx, &rx), &buf[i], remain);

but the data->cfg structure has not been setup correct, it never sets the data->cfg.cs part, so the SPI driver assumes no CS is used.

@lowlander Isn't it wrong to still have some CS handling in the spi sdhc disk subsys when it is supposed to be driven by the SPI driver ?

The SPI driver seems to need its CS configuration from the one using it (the SDHC driver in this case). But I just want my SD card to work, not rewrite a complete Zephyr subsystem :-)

[henrikbrixandersen]

@lowlander Isn't it wrong to still have some CS handling in the spi sdhc disk subsys when it is supposed to be driven by the SPI driver ?

I would say the SDHC disk subsystem should leave the SPI CS handling up to the SPI driver, yes.

[erwango]

I think we miss some piece of code like follows in sdhc driver:

#if DT_INST_SPI_DEV_HAS_CS_GPIOS(0)
    cs_ctrl.gpio_dev = device_get_binding(
        DT_INST_SPI_DEV_CS_GPIOS_LABEL(0));
    if (!cs_ctrl.gpio_dev) {
        LOG_ERR("Unable to get GPIO SPI CS device");
        return -ENODEV;
    }

    cs_ctrl.gpio_pin = DT_INST_SPI_DEV_CS_GPIOS_PIN(0);
    cs_ctrl.gpio_dt_flags = DT_INST_SPI_DEV_CS_GPIOS_FLAGS(0);
    cs_ctrl.delay = 0U;

    cc1200->spi_cfg.cs = &cs_ctrl;

    LOG_DBG("SPI GPIO CS configured on %s:%u",
        DT_INST_SPI_DEV_CS_GPIOS_LABEL(0),
        DT_INST_SPI_DEV_CS_GPIOS_PIN(0));
#endif

[henrikbrixandersen]

I think we miss some piece of code like follows in sdhc driver:

@erwango From looking at the code, I think a bit more is needed. The current implementation asserts the CS GPIO line, performs a number of SPI transactions, and de-asserts the CS GPIO line again. The SDHC SPI code will need to be rewritten a bit in order to have the SPI driver handle the CS correctly.

[henrikbrixandersen]

I am working on a fix for this.

[lowlander]

I am working on a fix for this.

@henrikbrixandersen if you have something that I should test let me know.

[henrikbrixandersen]

@henrikbrixandersen if you have something that I should test let me know.

@lowlander If you can, I would be grateful if you can test my proposed fix in #27685.

[lowlander]

@henrikbrixandersen i will give it a try later today, currently my hardware is doing a long uptime test and I don't want to restart it :-)

[lowlander]

PR #27685 fixes my problem, after merging this BR can be closed.