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

drivers/mtd: use XFA for pointers to defined MTDs #19465

Merged
merged 9 commits into from
Oct 20, 2023

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Apr 11, 2023

Contribution description

This PR provides the support to hold pointers to defined MTDs within a XFA. The XFA allows

  • to access MTDs of different types (mtd_flashpage, mtd_sdcard, mtd_emulated, ...) by an index
  • to determine the number of MTDs defined in the system.

Testing procedure

To be defined once PR #19443 is merged because emulated MTDs will allow to test this PR on arbitrary boards.

Porting Guide

For external boards:

  • remove the MTD_NUMOF definition from board.h
  • add MTD_XFA_ADD(<mtd_dev>, <idx>); to the definition of <mtd_dev>.
  • MTD_0, MTD_1, … defines are no longer needed. Use mtd_dev_get(0), mtd_dev_get(1), … instead.

Issues/PRs references

Related to PR #19443

@github-actions github-actions bot added Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: sys Area: System Area: USB Area: Universal Serial Bus Platform: ESP Platform: This PR/issue effects ESP-based platforms Platform: native Platform: This PR/issue effects the native platform labels Apr 11, 2023
@dylad dylad added the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 11, 2023
@benpicco benpicco requested a review from kaspar030 April 11, 2023 17:04
@gschorcht
Copy link
Contributor Author

Since sys/usb/usbus/msc.c uses dynamic memory allocation anyway, the LUN descriptor array is allocated at runtime with malloc so that XFA_LEN can be used to determine the number of MTDs defined and thus the number of LUN descriptors required.

Alternatively, we could also define a XFA for the LUN descriptor array and an additional macro to add MTDs that has to be used to add MTDs to this XFA to make LUNs exposable at all.

@gschorcht gschorcht added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 12, 2023
@riot-ci
Copy link

riot-ci commented Apr 12, 2023

Murdock results

✔️ PASSED

9da0cc5 drivers/mtd_sdmmc: use XFA with MTD pointers

Success Failures Total Runtime
7937 0 7937 14m:04s

Artifacts

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Apr 12, 2023
@gschorcht gschorcht removed the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 12, 2023
@dylad
Copy link
Member

dylad commented Apr 12, 2023

I think a few boards are missing in the update, they manually declare mtd0 in their board.h

  • sensebox_samd21 (mtd_sdcard_spi)
  • remote-revb (mtd_sdcard_spi)
  • waspmote-pro (mtd_sdcard_spi)

There are also a bunch of extern mtd_dev_t *mtd0; present in the boards folder, are they still needed ?

Also I am now wondering if the module MTD_SDCARD_DEFAULT is still relevant with this patch ?

@gschorcht
Copy link
Contributor Author

I think a few boards are missing in the update, they manually declare mtd0 in their board.h

  • sensebox_samd21 (mtd_sdcard_spi)
  • remote-revb (mtd_sdcard_spi)
  • waspmote-pro (mtd_sdcard_spi)

These board only declare mtd0 and define MTD_0 for their SD card interface but they don't define the MTDs. The MTDs are defined by mtd_sdcard_default as mtd0 instead.

Also I am now wondering if the module MTD_SDCARD_DEFAULT is still relevant with this patch ?

Yes, because mtd_sdcard_default is also defining the default SD card parameters and VFS mount points. It was introduced by PR #19216 to avoid that each board with SD card interface has to define them again and again.

There are also a bunch of extern mtd_dev_t *mtd0; present in the boards folder, are they still needed ?

I din't touch these declarations for the case that they are used in use code.

@benpicco benpicco added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Apr 28, 2023
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

That's a nice addition and as far as I can see the only brekaing change is that boards now have to define the MTD device as an XFA member - but that's an easy change.

Please squash.

drivers/include/mtd_emulated.h Outdated Show resolved Hide resolved
@gschorcht gschorcht added this to the Release 2023.10 milestone Sep 15, 2023
bors bot added a commit that referenced this pull request Sep 15, 2023
19914: boards: complete SD Card MTD definition for several bords r=benpicco a=gschorcht

### Contribution description

This PR completes the MTD definition for the following boards:
- `seeedstudio-gd32`
- `sipeed-longan-nano` including `sipeed-longan-nano-tft`
- `waveshare-nrf52840-eval-kit`
- ESP32x boards that have an SPI SD Card interface and use `mtd_sdcard_default`

### Testing procedure

Green CI

### Issues/PRs references#19465 

Prerequisite for PR #19465 

Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

looks good to me, feel free to squash

@@ -54,7 +54,7 @@ static mtd_spi_nor_t samd51_nor_dev = {
.params = &_samd51_nor_params,
};

mtd_dev_t *mtd0 = (mtd_dev_t *)&samd51_nor_dev;
XFA(mtd_dev_xfa, 0) mtd_dev_t *mtd0 = (mtd_dev_t *)&samd51_nor_dev;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kind of ugly to have in each board - how about a macro

#define MTD_XFA_ADD(dev, idx) XFA(mtd_dev_xfa, (idx)) mtd_dev_t *mtd # (idx) = (mtd_dev_t *)&(dev);

Then this would be

Suggested change
XFA(mtd_dev_xfa, 0) mtd_dev_t *mtd0 = (mtd_dev_t *)&samd51_nor_dev;
MTD_XFA_ADD(samd51_nor_dev, 0);

Copy link
Contributor Author

@gschorcht gschorcht Sep 17, 2023

Choose a reason for hiding this comment

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

The 0 in XFA(mtd_dev_xfa, 0) is not the index but the priority. But defining the macro as

#define MTD_XFA_ADD(dev, idx) XFA(mtd_dev_xfa, 0) mtd_dev_t *mtd # (idx) = (mtd_dev_t *)&(dev);

seems to be reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benpicco I have added the macro according to your suggestion. Parameter idx is used now as priority so that the index corresponds to the position in the XFA provided that the indices are unique, start from 0 and are consecutive.

bors bot added a commit that referenced this pull request Sep 20, 2023
19914: boards: complete SD Card MTD definition for several bords r=benpicco a=gschorcht

### Contribution description

This PR completes the MTD definition for the following boards:
- `seeedstudio-gd32`
- `sipeed-longan-nano` including `sipeed-longan-nano-tft`
- `waveshare-nrf52840-eval-kit`
- ESP32x boards that have an SPI SD Card interface and use `mtd_sdcard_default`

### Testing procedure

Green CI

### Issues/PRs references#19465 

Prerequisite for PR #19465 

19915: drivers/lcd: support MCU 8080 8-bit parallel mode r=benpicco a=gschorcht

### Contribution description

LCD driver ICs usually support
- SPI serial mode,
- MCU 8080 8-bit parallel mode and
- MCU 8080 16-bit parallel mode.

This PR extends the LCD display driver API to support the MCU 8080 8-/16-bit parallel modes and implements a GPIO-driven MCU 8080 8-bit parallel mode.

The following features are already working locally and will be provided as follow-on PRs for which this PR is a prerequisite.

- GPIO-driven bit-banging implementation of the 16-bit mode of the MCU 8080 parallel interface
- Enabling the display on `stm32f723e-disco` and `stm32l496g-disco` using the feature above
- Definition of a low-level API for the parallel modes using the LCD controller of the MCU
- Using FMC for the display on `stm32f723e-disco` and `stm32l496g-disco`
- Using LCD controller for the display of `esp32-wt32-sc01-plus` (PR #19917)

### Testing procedure

The PR can be tested with PR #19917 on top of this PR.
```
BOARD=esp32s3-wt32-sc01-plus make -j8 -C tests/drivers/st77xx flash
```
The following video shows the test.

**Please note** The test is pretty slow because the display has 480 x 320 pixels and the MCU 8080 8-bit parallel interface is realized by a GPIO-driven bit-banging implementation where each GPIO of the data bus is set separately. A follow-up PR will use the ESP32-S3 LCD controller and DMA for this board. This PR just defines the extension of the driver by the parallel interface and provides the bit-banging implementation for MCUs that don't have a LCD controller on chip.

https://github.com/RIOT-OS/RIOT/assets/31932013/c1e3e3d7-05d9-4ca5-8fff-9a5eaca50fba

### Issues/PRs references

19919: drivers/st77xx: introduce rotation defines r=benpicco a=gschorcht

### Contribution description

The PR introduces counterclockwise rotations for the definition of parameter `ST77XX_PARAM_ROTATION`.

It is more intuitive and universal to use `ST77XX_ROTATION_{0,90,180,270}` instead of `ST77XX_ROTATION_{ST77XX_ROTATION_{VERT,VERT_FLIP,HORZ,HORZ_FLIP}`, especially because the orientation of the display may vary with respect to the orientation of the board.

### Testing procedure

`tests/drivers/st77xx` should still work, for example:
```
BOARD=adafruit-pybadge make -C tests/drivers/st77xx flash
```
```
BOARD=esp32s3-usb-otg make -j8 -C tests/drivers/st77xx flash
```

### Issues/PRs references


19931: boards: fix documentation for GD32V boards and doxygen 1.9.4 r=benpicco a=gschorcht

### Contribution description

This PR fixes some small problems in documentation of `sipeed-longan-nano`, `sipeed-longan-nano-tft` and `seeedstudio-gd32` for doxygen 1.9.4 that is used on `doc.riot-os.org`.

Doxygen version 1.9.4 doesn't like anymore
- single double quotes as symbol for the inches unit in the text
- line breaks in `[]()` to avoid exhausting the 100 characters per line.

See https://doc.riot-os.org/group__boards__sipeed__longan__nano.html for example.

Doxygen 1.9.1 which is part of `riot-docker` container didn't have theses problems 😟

### Testing procedure

Documentation should be fixed.

### Issues/PRs references


19935: boards/nucleo64: fix SPI Arduino mapping for most boards r=benpicco a=maribu

### Contribution description

Before, the Arduino SPI mapping for all Nucleo-64 boards was incorrect. With this, the situation improves to the following:

- [x] nucleo-f030r8
- [ ] nucleo-f070rb
    - No SPI buses provided.
- [x] nucleo-f072rb
- [x] nucleo-f091rc
- [x] nucleo-f103rb
- [ ] nucleo-f302r8
    - No SPI bus at D11, D12, D13 provided
- [x] nucleo-f303re
- [x] nucleo-f334r8
- [x] nucleo-f401re
- [x] nucleo-f410rb
- [x] nucleo-f411re
- [x] nucleo-f446re
- [x] nucleo-g070rb
- [x] nucleo-g071rb
- [x] nucleo-g431rb
- [x] nucleo-g474re
- [x] nucleo-l053r8
- [x] nucleo-l073rz
- [x] nucleo-l152re
    - No SPI bus at D11, D12, D13 provided
- [x] nucleo-l452re
- [x] nucleo-l476rg
- [x] nucleo-wl55jc

The remaining offenders still need to be fixed, but that is better done one PR at a time.

### Testing procedure

- Check if the SPI device provided in the given `boards/<BOARD_NAME>/incude/periph_conf.h` is indeed `SPI_DEV(0)`, or in `periph_conf.h` the correct SPI dev is found
    - this should be fine for all boards above, except for the unchecked ones

or:

- run #19932: The SPI test should pass now

### Issues/PRs references

Bug found in #19932 (comment)

Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Co-authored-by: Marian Buschsieweke <marian.buschsieweke@posteo.net>
bors bot added a commit that referenced this pull request Sep 20, 2023
19914: boards: complete SD Card MTD definition for several bords r=benpicco a=gschorcht

### Contribution description

This PR completes the MTD definition for the following boards:
- `seeedstudio-gd32`
- `sipeed-longan-nano` including `sipeed-longan-nano-tft`
- `waveshare-nrf52840-eval-kit`
- ESP32x boards that have an SPI SD Card interface and use `mtd_sdcard_default`

### Testing procedure

Green CI

### Issues/PRs references#19465 

Prerequisite for PR #19465 

19915: drivers/lcd: support MCU 8080 8-bit parallel mode r=benpicco a=gschorcht

### Contribution description

LCD driver ICs usually support
- SPI serial mode,
- MCU 8080 8-bit parallel mode and
- MCU 8080 16-bit parallel mode.

This PR extends the LCD display driver API to support the MCU 8080 8-/16-bit parallel modes and implements a GPIO-driven MCU 8080 8-bit parallel mode.

The following features are already working locally and will be provided as follow-on PRs for which this PR is a prerequisite.

- GPIO-driven bit-banging implementation of the 16-bit mode of the MCU 8080 parallel interface
- Enabling the display on `stm32f723e-disco` and `stm32l496g-disco` using the feature above
- Definition of a low-level API for the parallel modes using the LCD controller of the MCU
- Using FMC for the display on `stm32f723e-disco` and `stm32l496g-disco`
- Using LCD controller for the display of `esp32-wt32-sc01-plus` (PR #19917)

### Testing procedure

The PR can be tested with PR #19917 on top of this PR.
```
BOARD=esp32s3-wt32-sc01-plus make -j8 -C tests/drivers/st77xx flash
```
The following video shows the test.

**Please note** The test is pretty slow because the display has 480 x 320 pixels and the MCU 8080 8-bit parallel interface is realized by a GPIO-driven bit-banging implementation where each GPIO of the data bus is set separately. A follow-up PR will use the ESP32-S3 LCD controller and DMA for this board. This PR just defines the extension of the driver by the parallel interface and provides the bit-banging implementation for MCUs that don't have a LCD controller on chip.

https://github.com/RIOT-OS/RIOT/assets/31932013/c1e3e3d7-05d9-4ca5-8fff-9a5eaca50fba

### Issues/PRs references

Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
bors bot added a commit that referenced this pull request Sep 20, 2023
19914: boards: complete SD Card MTD definition for several bords r=benpicco a=gschorcht

### Contribution description

This PR completes the MTD definition for the following boards:
- `seeedstudio-gd32`
- `sipeed-longan-nano` including `sipeed-longan-nano-tft`
- `waveshare-nrf52840-eval-kit`
- ESP32x boards that have an SPI SD Card interface and use `mtd_sdcard_default`

### Testing procedure

Green CI

### Issues/PRs references#19465 

Prerequisite for PR #19465 

19915: drivers/lcd: support MCU 8080 8-bit parallel mode r=benpicco a=gschorcht

### Contribution description

LCD driver ICs usually support
- SPI serial mode,
- MCU 8080 8-bit parallel mode and
- MCU 8080 16-bit parallel mode.

This PR extends the LCD display driver API to support the MCU 8080 8-/16-bit parallel modes and implements a GPIO-driven MCU 8080 8-bit parallel mode.

The following features are already working locally and will be provided as follow-on PRs for which this PR is a prerequisite.

- GPIO-driven bit-banging implementation of the 16-bit mode of the MCU 8080 parallel interface
- Enabling the display on `stm32f723e-disco` and `stm32l496g-disco` using the feature above
- Definition of a low-level API for the parallel modes using the LCD controller of the MCU
- Using FMC for the display on `stm32f723e-disco` and `stm32l496g-disco`
- Using LCD controller for the display of `esp32-wt32-sc01-plus` (PR #19917)

### Testing procedure

The PR can be tested with PR #19917 on top of this PR.
```
BOARD=esp32s3-wt32-sc01-plus make -j8 -C tests/drivers/st77xx flash
```
The following video shows the test.

**Please note** The test is pretty slow because the display has 480 x 320 pixels and the MCU 8080 8-bit parallel interface is realized by a GPIO-driven bit-banging implementation where each GPIO of the data bus is set separately. A follow-up PR will use the ESP32-S3 LCD controller and DMA for this board. This PR just defines the extension of the driver by the parallel interface and provides the bit-banging implementation for MCUs that don't have a LCD controller on chip.

https://github.com/RIOT-OS/RIOT/assets/31932013/c1e3e3d7-05d9-4ca5-8fff-9a5eaca50fba

### Issues/PRs references

Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Please squash!

@gschorcht gschorcht added the CI: ready for merge train 🚃 PR is ready to be merged and awaiting the next merge train label Oct 3, 2023
@gschorcht
Copy link
Contributor Author

@benpicco Can we merge it?

@gschorcht
Copy link
Contributor Author

@benpicco Unfortunatly, we just forgot to merge it before the hard freeze 🙈

@benpicco
Copy link
Contributor

Sorry I wanted to check how this deals with external boards first, but it gives a clear

error: "MTD_NUMOF" redefined 

error and no silent failure. So I added a short porting guide to the PR description that can be added to the release notes.

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Oct 20, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 03d3874 into RIOT-OS:master Oct 20, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: sys Area: System Area: tests Area: tests and testing framework Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: ready for merge train 🚃 PR is ready to be merged and awaiting the next merge train Platform: ESP Platform: This PR/issue effects ESP-based platforms Platform: native Platform: This PR/issue effects the native platform Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants