Enable multiple ICE40 on the same bus

[benediktibk]

At the moment the build fails if multiple ICE40 are configured on the same SPI master with CONFIG_PINCTRL=y due to duplicate instantiation of the parent pinctrl-config.

This PR introduces a new SPI API which allows reapplying the default pin state, which in turn removes the necessity to have the parent pinctrl-config in the ICE40 driver.

So far I came up with only one alternative: Macrobatics in the ICE40 driver which loops over all SPI masters and creates one pinctrl-config instance for each SPI master with at least one ICE40 instance on it. This didn't seem to be a really good option to me.

Fixes #77983.

[benediktibk]

I've added the DNM as I think it might be a good idea to implement this API not only for STM32, if it is overall an acceptable approach.

@tbursztyka Could you please take a look at this one? I assume there is some room for different opinions on how to solve this issue.

[cfriedt]

@benediktibk Looks good to me. If you're looking for additional SPI drivers to implement this on, spi_esp32_spim would be fantastic (the ice40 FPGA driver was added for icev_wireless originally)

[benediktibk]

@benediktibk Looks good to me. If you're looking for additional SPI drivers to implement this on, spi_esp32_spim would be fantastic (the ice40 FPGA driver was added for icev_wireless originally)

Done

[benediktibk]

The changes are tested only on STM32, as it is the only platform which I have available at the moment.

[cfriedt]

It the pinctrl config was always at a known offset (e.g. first) in all drivers config struct, it just occurred to me that this could be done in a generic way for all spi devices - e.g. as a static inline function in the spi.h header (would be a no-op if CONFIG_PINCTRL=n).

Thoughts?

[sylvioalves]

+1 regarding ESP32 changes.

[erwango]

Would be valuable to have SPI maintainers approval here. @tbursztyka, @teburd

[benediktibk]

Rebased to resolve merge conflicts.

[cfriedt]

For instance spi_emul doesn't even use pinctrl, so I think we can't use this approach.

It might be possible to overcome this issue fairly easily if it's just made an optional SPI driver API call (which is NULL in the api structure).

So drivers that do not have it could return -ENOTSUP from the static inline wrapper. The testsuite could just ignore that error.

That would make this solution work and scale quite well, generically.

[benediktibk]

For instance spi_emul doesn't even use pinctrl, so I think we can't use this approach.

It might be possible to overcome this issue fairly easily if it's just made an optional SPI driver API call (which is NULL in the api structure).

So drivers that do not have it could return -ENOTSUP from the static inline wrapper. The testsuite could just ignore that error.

That would make this solution work and scale quite well, generically.

Besides using ENOSYS instead of ENOTSUP, isn't this exactly what I have implemented? Or have I misunderstood you somehow?

[cfriedt]

That would make this solution work and scale quite well, generically.

Besides using ENOSYS instead of ENOTSUP, isn't this exactly what I have implemented? Or have I misunderstood you somehow?

Sorry, minor confusion.

I was still trying to think of a way to do it generically with pinctrl config at a fixed offset. Actually, if the config is just a read-only part of the API, then I think it can be done in a generic way, without functions required for every driver.

[benediktibk]

That would make this solution work and scale quite well, generically.

Besides using ENOSYS instead of ENOTSUP, isn't this exactly what I have implemented? Or have I misunderstood you somehow?

Sorry, minor confusion.

I was still trying to think of a way to do it generically with pinctrl config at a fixed offset. Actually, if the config is just a read-only part of the API, then I think it can be done in a generic way, without functions required for every driver.

Considering that we also have drivers without any pinctrl config at all we would additionally required a flag at a fixed offset which defines if the pinctrl config is available at another fixed offset. This would of course spare us the implementation of the function in every driver, but then each driver has to set at least the flag.

I'm not sure if this is in total better? Or am I missing something?

[cfriedt]

I'm not sure if this is in total better? Or am I missing something?

It's more uniform, probably easier to maintain, and scales to all SPI drivers just about immediately, without any specific customization.

The current solution would still need an additional, custom, function pointer even if the function pointer is NULL for every SPI device.

Not blocking, but more for the SPI maintainers consideration.

[tbursztyka]

@gmarull as you are pinctrl maintainer, I think we will need your input on this one.

[tbursztyka]

I have a hard time to see how this is SPI specific.
What if tomorrow the same use case appear with an i²c peripheral or a CAN one etc... would we expend each and every APIs with such a workaround? Because it looks to me as a workaround so far.

Perhaps I am missing something, I never had such use-case at hand myself.
To me, it seems that the issue is in pinctrl side. PINCTRL_DT_DEFINE() should be called only once, and by the instance that is related to it.
struct pinctrl_dev_config declared by PINCTRL _DT_DEFINE() is not static, so at this stage: any user of that cfg (like ice40 driver via FPGA_ICE40_PINCTRL_CONFIG() should be good to go.

[benediktibk]

There is actually PINCTRL_DT_DEV_CONFIG_DECLARE which would reference the one instance via extern. But then I still have got the issue, that I can declare this only once. And I really can't get my head around a Macro-Solution which declares the variable within fpga_ice40.c only once per SPI bus master.

[tbursztyka]

Perhaps auto-generated files could help: a header calling PINCTRL_DT_DEV_CONFIG_DECLARE for each valid pinctrl instance? Something like that, that would be automatically included at the end of pinctrl.h. If necessary even a .c file calling PINCTRL_DT_DEFINE() only once in a central place.

I don't know much pinctrl actually, but I am pretty sure there is room for improvements here. Let's see what @gmarull has to say.

[benediktibk]

I could also create a copy of PINCTRL_DT_DEFINE, e.g. PINCTRL _DT_DEFINE_CUSTOM_NAME, which has as an additional parameter the desired name of the variable. I would of course have to use this custom name then also for the other on the fly created variables (states, ...), but this might actually work. Although not really beautiful as well.

[cfriedt]

@tbursztyka - you're right in that it isn't necessarily a SPI-specific issue. Technically, it could be useful for other buses that require temporary pinctrl mode-change from bitbanged gpio back to the default setting.

In this case, bit-banging is required because the FPGA needs a specific handshake that is outside of the normal SPI protocol. In Linux, this is probably handled via some "quirk".

Effectively, Benedikt's change gets the device to remember it's default pinctrl assignment, so that users can tell the bus to reset back to the default pinctrl, without explicitly knowing what that default is, and that works well for multi-instance.

Also, yeah, I would agree @gmarull should take a look.

[gmarull]

I'm not sure if I follow what this PR is doing, but doesn't seem like an adequate solution by any means. Only a single pinctrl config instance is allowed per DT node. If you have multiple ICE40 instances, I assume you'll also have N DT nodes, so you should be able to have N pinctrl config instances. Maybe some code snippets will help to understand the problem better.

[benediktibk]

I'm not sure if I follow what this PR is doing, but doesn't seem like an adequate solution by any means. Only a single pinctrl config instance is allowed per DT node. If you have multiple ICE40 instances, I assume you'll also have N DT nodes, so you should be able to have N pinctrl config instances. Maybe some code snippets will help to understand the problem better.

The issue is that the ICE40 instances have to reset the pinctrl of the parent, the SPI device. Therefore there are N ICE40 nodes, but only 1 pinctrl config instance.

[gmarull]

I'm not sure if I follow what this PR is doing, but doesn't seem like an adequate solution by any means. Only a single pinctrl config instance is allowed per DT node. If you have multiple ICE40 instances, I assume you'll also have N DT nodes, so you should be able to have N pinctrl config instances. Maybe some code snippets will help to understand the problem better.

The issue is that the ICE40 instances have to reset the pinctrl of the parent, the SPI device. Therefore there are N ICE40 nodes, but only 1 pinctrl config instance.

Ok, got it. TBH, sounds a bit difficult to generalize, and extending SPI just for such cases feels like an overkill. Maybe an option can be that devices select PINCTRL_NON_STATIC, and then use PINCTRL_DT_DEV_CONFIG_DECLARE. However, I'd consider this potentially unsafe, as you'd be taking control of the pinctrl state out of the parent driver's internal state. This may not be compatible with all parent drivers...

[benediktibk]

Ok, got it. TBH, sounds a bit difficult to generalize, and extending SPI just for such cases feels like an overkill. Maybe an option can be that devices select PINCTRL_NON_STATIC, and then use PINCTRL_DT_DEV_CONFIG_DECLARE. However, I'd consider this potentially unsafe, as you'd be taking control of the pinctrl state out of the parent driver's internal state. This may not be compatible with all parent drivers...

Actually not all SPI masters even use PINCTRL, therefore I would like to get away from referencing something external which might does not even exist.

[gmarull]

Ok, got it. TBH, sounds a bit difficult to generalize, and extending SPI just for such cases feels like an overkill. Maybe an option can be that devices select PINCTRL_NON_STATIC, and then use PINCTRL_DT_DEV_CONFIG_DECLARE. However, I'd consider this potentially unsafe, as you'd be taking control of the pinctrl state out of the parent driver's internal state. This may not be compatible with all parent drivers...

Actually not all SPI masters even use PINCTRL, therefore I would like to get away from referencing something external which might does not even exist.

I guess what we need then is a mechanism to de-initialize devices, and re-initialize them again. This way you could safely take ownership of SPI resources, and give them back. This has actually been discussed a few times, and this may be one valid use case to add to the list.

[benediktibk]

Ok, got it. TBH, sounds a bit difficult to generalize, and extending SPI just for such cases feels like an overkill. Maybe an option can be that devices select PINCTRL_NON_STATIC, and then use PINCTRL_DT_DEV_CONFIG_DECLARE. However, I'd consider this potentially unsafe, as you'd be taking control of the pinctrl state out of the parent driver's internal state. This may not be compatible with all parent drivers...

Actually not all SPI masters even use PINCTRL, therefore I would like to get away from referencing something external which might does not even exist.

I guess what we need then is a mechanism to de-initialize devices, and re-initialize them again. This way you could safely take ownership of SPI resources, and give them back. This has actually been discussed a few times, and this may be one valid use case to add to the list.

I agree, deinitializing and reinitializing the SPI device would do the trick.

[benediktibk]

Another approach would be to have a Kconfig for each load modes (GPIO and SPI). This way one could solve the issue for cases, where it is possible to use the SPI load mode. But for smaller MCUs the issue would still exist.

Or the SPI driver model could be extended. At least according to the comment in fpga_ice40.c this might be a solution:

/*
 * Note: When loading a bitstream, the iCE40 has a 'quirk' in that the CS
 * polarity must be inverted during the 'leading clocks' phase and
 * 'trailing clocks' phase. While the bitstream is being transmitted, the
 * CS polarity is normal (active low). Zephyr's SPI driver model currently
 * does not handle these types of quirks (in contrast to e.g. Linux).
 *
 * The logical alternative would be to put the CS into GPIO mode, perform 3
 * separate SPI transfers (inverting CS polarity as necessary) and then
 * restore the default pinctrl settings. On some higher-end microcontrollers
 * and microprocessors, it's possible to do that without breaking the iCE40
 * timing requirements.
 *
 * However, on lower-end microcontrollers, the amount of time that elapses
 * between SPI transfers does break the iCE40 timing requirements. That
 * leaves us with the bitbanging option. Of course, on lower-end
 * microcontrollers, the amount of time required to execute something
 * like gpio_pin_configure_dt() dwarfs the 2*500 nanoseconds needed to
 * achieve the minimum 1 MHz clock rate for loading the iCE40 bistream. So
 * in order to bitbang on lower-end microcontrollers, we actually require
 * direct register access to the set and clear registers.
 *
 * With that, this driver is left with 2 possible modes of operation which
 * are:
 * - FPGA_ICE40_LOAD_MODE_SPI (for higher-end microcontrollers)
 * - FPGA_ICE40_LOAD_MODE_GPIO (for lower-end microcontrollers)
 */