Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify driver vendor compatibility requirements for drivers for standalone external devices #61798

Closed
fabiobaltieri opened this issue Aug 23, 2023 · 13 comments · Fixed by #64118
Assignees
Labels
Architecture Review Discussion in the Architecture WG required RFC Request For Comments: want input from the community TSC Topics that need TSC discussion

Comments

@fabiobaltieri
Copy link
Member

Introduction

TL;DR: should we accept vendor specific drivers for devices with a vendor agnostic interface?

Problem description

Hi, this is spinning off a discussion started in #59444 (comment), the specific case is for a WiFi driver that supports multiple interfaces (SPI, SDIO, ...) that have an existing abstraction in the Zephyr code base (SPI subsystem) so that a driver could be written in a SoC agonistc way, but the proposed implementation is tied to a specific SoC vendor in this case the Cypress HAL. So while the device does not necessarily have to be paired with SoC of the same vendor and is marketed as a standalone product, the proposed implementation does not allow interoperability with any other device.

The RFC is about clarifying whether this should or should not be accepted upstream. The main issue I see is that the fact that the driver only works when paired with some specific SoC is non obvious, and certainly not expected from a system that abstracts busses and peripherals. One could imagine someone looking at the code thinking that they could design this device in with any other SOC with an SPI or SDIO interface, just to find out that the two don't work while doing the board configuration.

For this specific case the situation is particularly sad as we already have in-tree boards that could use the driver (RPI Pico W and Arduino Giga R1, STM32H7 based).

Proposed change

Clarify the requirements for upstream drivers compatibility when a device is marketed as a standalone product and uses an interface for which we already have an abstraction/subsystem.

@fabiobaltieri fabiobaltieri added the RFC Request For Comments: want input from the community label Aug 23, 2023
@fabiobaltieri
Copy link
Member Author

@fabiobaltieri fabiobaltieri added the Architecture Review Discussion in the Architecture WG required label Aug 23, 2023
@gmarull
Copy link
Member

gmarull commented Aug 23, 2023

My opinion here is that unless there's a strict limitation, we should not allow these kinds of drivers. To me, this violates some Zephyr basic design principles.

fabiobaltieri added a commit to fabiobaltieri/zephyr that referenced this issue Aug 23, 2023
Add an item to the coding style list asking to avoid using binary
literals.

Link: zephyrproject-rtos#61798
Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
@nashif nashif added the TSC Topics that need TSC discussion label Aug 23, 2023
@carlescufi
Copy link
Member

carlescufi commented Aug 23, 2023

@fabiobaltieri I think we should limit the scope of this issue to drivers for "external devices", i.e. devices that are connected via an external physical bus and that are compatible by design with any MCU or SoC. This includes:

  • Sensors
  • Bridges (like SPI to GPIO)
  • Connectivity ICs (Bluetooth, Wi-Fi)

EDIT: I see that you've already added the following:

Clarify the requirements for upstream drivers compatibility when a device is marketed as a standalone product and uses an interface for which we already have an abstraction/subsystem.

which encapsulates my thoughts perfectly. Thanks!

@carlescufi carlescufi changed the title Clarify driver vendor compatibility requirements for upstream drivers Clarify driver vendor compatibility requirements for drivers for standalone universal devices Aug 23, 2023
@carlescufi
Copy link
Member

carlescufi commented Aug 23, 2023

My take on this:

Drivers for standalone devices that are compatible with any MCU/SoC should not be restricted to a single vendor. This is aligned with the Open Source philosophy:

  1. License Must Be Technology-Neutral [1]
    No provision of the license may be predicated on any individual technology or style of interface.

The only exceptions to this would be when it makes technical sense to do so, such as:

  • The bus used to communicate with the external device is not supported by Zephyr
  • The device is closed in nature and requires some sort of binary blob to interface with

I would also allow extensions to this approach, in the following way:

  • If it is not technically possible to achieve full performance using the Zephyr APIs due to specialized accelerators in a particular SoC family, one could extend the support for an external device by providing a specialized path for that SoC family. However, the driver must still provide a regular path (via Zephyr APIs) for all other SoCs.

[1] https://opensource.org/osd/

@fabiobaltieri
Copy link
Member Author

fabiobaltieri commented Aug 23, 2023

Yeah thanks for rewording @carlescufi, this would obviously only apply to something that goes on an external bus that is somewhat standardized.

I would also add that I would not see any problem with a driver supporting multiple busses, including a vendor specific implementation (for performance reasons or whatever), as long as it's not the only one - for instance on the rpi pico this is using some odd two wire SPI implemented with the RPi PIO. That obviously has to be platform specific, but it should not be the only interface supported by an upstream driver, otherwise (as @gmarull pointed out in the original issue) the driver may as well live in the SoC HAL code base itself.

@carlescufi
Copy link
Member

I would also add that I would see any problem with a driver supporting multiple busses, including a vendor specific implementation (for performance reasons or whatever),

Did you read my thoughts ? :D

@carlescufi carlescufi changed the title Clarify driver vendor compatibility requirements for drivers for standalone universal devices Clarify driver vendor compatibility requirements for drivers for standalone external devices Aug 23, 2023
@gmarull
Copy link
Member

gmarull commented Aug 23, 2023

The only exceptions to this would be when it makes technical sense to do so, such as:

  • The bus used to communicate with the external device is not supported by Zephyr

I'd not consider this a valid point, a new API can be proposed for the bus.

@fabiobaltieri
Copy link
Member Author

fabiobaltieri commented Aug 23, 2023

I'd not consider this a valid point, a new API can be proposed for the bus.

In fairness that's what they are doing for the whole rpi pico PIO story.

@carlescufi
Copy link
Member

The only exceptions to this would be when it makes technical sense to do so, such as:

  • The bus used to communicate with the external device is not supported by Zephyr

I'd not consider this a valid point, a new API can be proposed for the bus.

I disagree. It may be a very specialized bus or even one that we have no interest of supporting in-tree.

@gmarull
Copy link
Member

gmarull commented Aug 23, 2023

The only exceptions to this would be when it makes technical sense to do so, such as:

  • The bus used to communicate with the external device is not supported by Zephyr

I'd not consider this a valid point, a new API can be proposed for the bus.

I disagree. It may be a very specialized bus or even one that we have no interest of supporting in-tree.

If it's a vendor-specific bus, then yes. I'm talking about vendor-agnostic cases.

@gmarull
Copy link
Member

gmarull commented Aug 23, 2023

I'd not consider this a valid point, a new API can be proposed for the bus.

In fairness that's what they are doing for the whole rpi pico PIO story.

If it's done for performance reasons, then I think it makes sense, as soon as the driver is designed in a way that allows to (1) use a generic layer or (2) is designed to be easily extensible.

@nashif nashif removed the Architecture Review Discussion in the Architecture WG required label Aug 23, 2023
carlescufi pushed a commit that referenced this issue Aug 24, 2023
Add an item to the coding style list asking to avoid using binary
literals.

Link: #61798
Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
coreboot-org-bot pushed a commit to coreboot/zephyr-cros that referenced this issue Aug 24, 2023
Add an item to the coding style list asking to avoid using binary
literals.

(cherry picked from commit bef540b)

Original-Link: zephyrproject-rtos/zephyr#61798
Original-Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
GitOrigin-RevId: bef540b
Change-Id: I998a564023f614fbd05eaf767c5c5321e01912b0
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4810116
Tested-by: Fabio Baltieri <fabiobaltieri@google.com>
Tested-by: ChromeOS Prod (Robot) <chromeos-ci-prod@chromeos-bot.iam.gserviceaccount.com>
Reviewed-by: Fabio Baltieri <fabiobaltieri@google.com>
Commit-Queue: Fabio Baltieri <fabiobaltieri@google.com>
piotrnarajowski pushed a commit to piotrnarajowski/zephyr that referenced this issue Aug 31, 2023
Add an item to the coding style list asking to avoid using binary
literals.

Link: zephyrproject-rtos#61798
Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
@carlescufi carlescufi added the Architecture Review Discussion in the Architecture WG required label Sep 5, 2023
@carlescufi
Copy link
Member

Arch WG:

  • Suggested wording: "Support for generic off-die devices over standardized buses/transports that can be natively supported in Zephyr must be implemented using common Zephyr APIs"
  • @bjarki-trackunit agrees with the overall statement
  • @bjarki-trackunit states that this should be the case even for SiPs/chiplets, etc.

Exception text:
"If it is not technically possible to achieve full performance using the Zephyr APIs due to specialized accelerators in a particular SoC family, one could extend the support for an external device by providing a specialized path for that SoC family. However, the driver must still provide a regular path (via Zephyr APIs) for all other SoCs. Every exception must be approved by the Architecture WG in order to be validated and potentially to be learned/improved from."

@npal-cy
Copy link
Contributor

npal-cy commented Oct 11, 2023

Reworked host agnostic version of AIROC (cyw43xx) wi-fi driver: #63721

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Architecture Review Discussion in the Architecture WG required RFC Request For Comments: want input from the community TSC Topics that need TSC discussion
Projects
Status: Done
Status: Done
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants