Skip to content

Conversation

anangl
Copy link
Member

@anangl anangl commented Sep 29, 2025

Introduce "chip-family" enum property in the "jedec,mspi-nor" binding as a way of specifying what special routines need to be use for a given flash chip. This has the advantage over an additional compatible string that users can see the possibilities in the binding documentation and do not need to look into the driver code to find out those. Also any typos in the enum value will be caught by the DT compiler, while for the compatible string such typos would cause it to be silently ignored.

Addresses #93135.

@nordicjm
Copy link
Contributor

nordicjm commented Sep 29, 2025

How it works in spi-nor: https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/mtd/jedec%2Cspi-nor-common.yaml there are properties for each thing e.g. dpd, 4b addr, mx25r power mode (vendor property) - these can be used differently for many different devices and an out of tree device can also make use of them which has some or all of the properties

In this new method, there is just a chip family, this chip family selects all the options, it is possible for an out of tree device to use the exact same properties by using the same chip family but it is not possible to use only some of the properties and not others, or to use properties of 2 chip families combined, plus if the actual chip they are using has a different name completely then would seem to be very messy to have to use a chip family which they clearly are not but to use a specific feature as in above it is possible

In my mind, whilst the bottom does work for some cases, I prefer the top method for flexibility, freedom and expandability of downstream users/devices which the bottom method does not support in all cases without forking zephyr and editing it

@anangl
Copy link
Member Author

anangl commented Sep 30, 2025

The quirks mechanism is not intended to be used for things like DPD or 4-byte addressing. For those, dedicated properties are used, just like in spi-nor. Only vendor specific things should be covered by quirks, like the mentioned mx25r power mode. And I don't think that a separate property for it would be better. This is a thing specific to this chip family, so in my opinion the current approach to handling it is fine.

@anangl anangl force-pushed the flash_mspi_nor_quirk_props branch from 1a10ff4 to 5bd1927 Compare September 30, 2025 08:27
@anangl
Copy link
Member Author

anangl commented Sep 30, 2025

Rebased on #96770.

@zephyrbot zephyrbot added the area: USB Universal Serial Bus label Sep 30, 2025
tmon-nordic
tmon-nordic previously approved these changes Sep 30, 2025
Copy link
Contributor

@tmon-nordic tmon-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USB part looks OK, not sure why it is bundled here.

@tmon-nordic tmon-nordic dismissed their stale review September 30, 2025 12:31

I just noticed the USB part has been posted separately.

Introduce "chip-family" enum property in the "jedec,mspi-nor" binding
as a way of specifying what special routines need to be use for a given
flash chip. This has the advantage over an additional compatible string
that users can see the possibilities in the binding documentation and
do not need to look into the driver code to find out those. Also any
typos in the enum value will be caught by the DT compiler, while for
the compatible string such typos would cause it to be silently ignored.

Signed-off-by: Andrzej Głąbek <[email protected]>
@anangl anangl force-pushed the flash_mspi_nor_quirk_props branch from 5bd1927 to eeed45f Compare October 1, 2025 05:24
@anangl
Copy link
Member Author

anangl commented Oct 1, 2025

Rebased.

Copy link

sonarqubecloud bot commented Oct 1, 2025

@de-nordic
Copy link
Contributor

de-nordic commented Oct 1, 2025

I think we should have definitions of quirks rather than address them by devices, if we take that info directly from DTS anyway.
We could have properties like quirk-mx25uw-pre-init or something like this, which would be added as properties to a device node.

Similar approach has been proposed to PR #69257 (defunct now).

This would reduce need to change code when adding chips with the same quirks; I know that this would require careful specification in DTS, for JEDEC compatible, but also should allow to define read-only defaults for compatibles that have the full name of a chip.

@bjarki-andreasen
Copy link
Contributor

For other device types, actually pretty much all other hardware in the devicetree, a compat means:
"hardware model which matches hardware", and multiple hardware models means: "hardware models which match hardware, in order of best fitting". In this case, the common hardware model jedec,mspi-nor is not compatible with the hardware which requires the quirks identified by the "mxicy,mx25u", so this:

compatible = "mxicy,mx25u", "jedec,mspi-nor";

is invalid according to the devicetree spec. The hardware model "jedec,mspi-nor" can be made more flexible by adding properties like requires-x-at-init;, y-supported. Adding the chip-family is a way to do this, though this is really a gray area if thechip-family is not a "common quirk", but a set of quirks which match a hardware model, hence, it really should be a specific compat, which just includes the bindings from "jedec,mspi-nor", and in the driver, has a flash_jdec_mspi_nor_common.c to call into. This is how its done for ADCs, and for CAN with the shared MCAN IP.

The solution which best matches in this case would be:

compatible = "mxicy,mx25u";

in the devicetree, and the binding

compatible: mxicy,mx25u

includes: jedec,mspi-nor-common.yaml

See this example for CAN:

Is there a reason this approach can't be used?

@masz-nordic
Copy link
Contributor

Initial implementation of quirks was based on USB approach:
https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/usb/udc/udc_dwc2_vendor_quirks.h

I agree this might not be up to devicetree spec.
Extending bindings seems like the best practice - it should also be added to the documentation so there is clear guidance in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants