drivers: Add zephyr,flash-disk compatible binding

[desowin]

USB mass storage sample, when configured to use flash-based disk do not work in Zephyr 3.2 release due to removed label property. The naive fix is to change the "MX25R64" to "mx25r6435f@0" and "GD25Q16" to "gd25q16@0" in samples/subsys/usb/mass/Kconfig. During the review, the naive fix was deemed inappropriate.

The PR addresses the issue by introducing "zephyr,flash-disk" compatible property. That way, any number of flashdisks can be specified in devicetree. While this fixes the USB Mass Storage sample, the actual Mass Storage class support for multiple LUNs is out of scope of this PR.

[mbolivar-nordic]

Second patch is the wrong approach. Kconfig option should be removed entirely and DEVICE_DT_GET used instead. See devicetree release notes for v3.2 for extended discussion

[jfischer-no]

Second patch is the wrong approach. Kconfig option should be removed entirely and DEVICE_DT_GET used instead. See devicetree release notes for v3.2 for extended discussion

No, that is not the intention of this patch. See also changes in tests/subsys/fs/fat_fs_api, 418d601.

[desowin]

Second patch is the wrong approach. Kconfig option should be removed entirely and DEVICE_DT_GET used instead. See devicetree release notes for v3.2 for extended discussion

I can achieve this by modifying

zephyr/drivers/disk/flashdisk.c

Line 59 in 11e3344

flash_dev = device_get_binding(CONFIG_DISK_FLASH_DEV_NAME);

to

#if DT_NODE_HAS_STATUS(DT_NODELABEL(mx25r64), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(mx25r64));
#elif DT_NODE_HAS_STATUS(DT_NODELABEL(gd25q16), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(gd25q16));
#elif DT_NODE_HAS_STATUS(DT_NODELABEL(flashcontroller0), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(flashcontroller0));
#endif

but doing so doesn't really seem right.

@mbolivar-nordic Is above what you want? How to handle the device selection, is the ifdef chain appropriate?

[de-nordic]

#if DT_NODE_HAS_STATUS(DT_NODELABEL(mx25r64), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(mx25r64));
#elif DT_NODE_HAS_STATUS(DT_NODELABEL(gd25q16), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(gd25q16));
#elif DT_NODE_HAS_STATUS(DT_NODELABEL(flashcontroller0), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(flashcontroller0));
#endif

but doing so doesn't really seem right.

Is there an option to use chosen zephyr,flash-disk for that purpose? For example

chosen {
    zephyr,flash-disk = &mx25r64;
};

This would allow code like this

    flash_dev = DEVICE_DT_GET(DT_CHOSEN(zephyr_flash_disk));

This is flexible and easily allows to set default value in DTS or board overlay, while at the same time easily allowing to replace it in app overlay by doing /delete-property/ zephyr,flash-disk on the chosen node.

There is also an option, less nice and flexible, to use specific label, for example

zephyr_flash_disk_0: &mx25r64 : { };

and in code:

   flash_dev = DEVICE_DT_GET(DT_NODELABEL(zephyr_flash_disk_0));   

In both cases Kconfigs are no longer there and there is no need for if sequence, as flash disk selection is done in DTS.

IMHO using Kconfig to select flash disk is not best approach, because the Kconfig will require updates every time when new flash devices or boards with such devices are added; when the DTS is designated for selecting flash disk, and you add new board with new flash, all you have to do is to point to it with chosen or label, and you do not have to touch Kconfig, or the flash disk driver code.

[desowin]

Is there an option to use chosen zephyr,flash-disk for that purpose? For example

chosen {
    zephyr,flash-disk = &mx25r64;
};

This seems better than the #ifdef ladder but how to set the defaults? Should the zephyr,flash-disk (or equivalent) be specified in mass storage boards overlay? Should there be a native_posix.overlay specifying zephyr,flash-disk added to tests/subsys/fs/fat_fs_api?

Another option instead of overlays would be putting it by default in board dts, but it raises an issue what to do with targets that actually use the mx25r64 flash for some partitions? Going with the overlays in samples/test direction avoids this issue.

[de-nordic]

Should the zephyr,flash-disk (or equivalent) be specified in mass storage boards overlay? Should there be a native_posix.overlay specifying zephyr,flash-disk added to tests/subsys/fs/fat_fs_api?

Every sample and test, either generic or board specific, that uses mass storage needs to have overlay which elects the flash disk; the same way it does this with Kconfig now, just the place will be different.

Another option instead of overlays would be putting it by default in board dts, but it raises an issue what to do with targets that actually use the mx25r64 flash for some partitions? Going with the overlays in samples/test direction avoids this issue.

Not really fan of this, but it wouldn't break anything unless you enable all subsystems that use that specific flash at the same time.

[galak]

I agree with the approach of zephyr,flash-disk and removal of Kconfig DISK_FLASH_DEV_NAME and replacing device_get_binding with DEVICE_DT_GET.

[de-nordic]

This breaks ability to have RO FAT FS on non page uniform devices.

[desowin]

I'll just remove the constraints check commit then. The write on non-uniform partitions never worked before and this PR won't change it, but removing the constraint check will make the RO continue to work.

[de-nordic]

Looks OK. Thanks for commits and your patience.

[Laczen]

@desowin, Thank you for the excellent work and the perseverance.