Add support of rcar mmc driver #60285

xakep-amatop · 2023-07-12T07:20:50Z

Add basic functionality of Renesas SD/MMC driver. It can be used for both gen3 and gen4 R-car boards, but tested only with H3ULCB, Salvator XS M3 and Spider boards. This driver working with SDHC subsystem.

The driver supports regularal reading/writing throught SD/MMC controller buffer, DMA mode w/o interrupts.
Notes:

the driver doesn't support SPI mode;
SCC tuning and DMA mode based on IRQs are enabled by default;
an address of a data buffer has to be aligned to 128 bytes if it is not, driver will use non-DMA mode automatically;
Renesas MMC DMAC doesn't support 64-bit DMA addresses, so for case when we have 64-bit xref data address we use non-DMA mode;
SD/MMC controller supports block size between 512 and 1 with a lot of restrictions, more details you can find in code;
support of HS400 mode isn't implemented inside driver.

danieldegrasse

Driver itself generally looks good. Just a few comments/questions

Note- I'm not familiar with the hardware itself- I'd appreciate a review from anyone who is (@lorc)?

danieldegrasse · 2023-07-13T15:12:49Z

drivers/regulator/regulator_fixed.c

A few comments here:

personally, I'd prefer this be in a second PR. That way the SD changes can be looked at independently of the regulator ones (and the regulator changes will pull in the appropriate reviewers)

If we need these functions to be available to provide a common interface to regulators, could we implement them within regulator_common.c?

ping @gmarull

personally, I'd prefer this be in a second PR. That way the SD changes can be looked at independently of the regulator ones (and the regulator changes will pull in the appropriate reviewers)

actually, GitHub CI will be red here due to lack of regulator changes, some of reviewers really worried about it

If we need these functions to be available to provide a common interface to regulators, could we implement them within regulator_common.c?

I'm not sure about that. I think it's better to change this file. At least from now, it works in a similar way to the fixed regulator inside the Linux kernel. But we should consult with the maintainer of this driver, @gmarull, for the final decision.

Based on your other comments in this PR, would the preferred regulator for this SD device be the driver being added in #58411?

Yep, this pull request doesn't contain all the necessary changes for supporting SD cards. The driver is full, but we still need the following:

pin control driver changes for controlling power on pins (drivers: pinctrl: pfc_rcar: add support of voltage control to pfc driver #60444);

GPIO driver changes (Zephyr add regulator gpio #58411) for controlling power of the SD card;

also, some changes are needed for the board(s) DTS files (changes for our project repository can be found here: Sd card support xen-troops/zephyr#45).

The eMMC device on R-Car has only fixed regulators, and we can't disable power using these regulators, they are always on and fixed. If we talk about SD cards, they have both fixed and gpio regulators. One of them can disable the SD card power supply, and the other is used for controlling IO lines power.

Please copy this patch to own PR.

ok, I'll do it

danieldegrasse · 2023-07-13T15:15:19Z

subsys/sd/sd.c

Please update the comment here, and correctly record the signal voltage below:

zephyr/subsys/sd/sd.c

Line 148 in 18636af

card->card_voltage = SD_VOL_3_3_V;

danieldegrasse · 2023-07-13T15:18:45Z

drivers/sdhc/Kconfig.rcar

This should be default y, with depends on DT_HAS_RENESAS_RCAR_MMC_ENABLED, see here:

zephyr/drivers/sdhc/Kconfig.mcux_sdif

Line 7 in 18636af

depends on DT_HAS_NXP_LPC_SDIF_ENABLED

drivers/sdhc/rcar_mmc.c

danieldegrasse · 2023-07-13T15:39:34Z

drivers/sdhc/rcar_mmc.c

The SD subsystem should insure all data buffers passed to the driver are aligned to CONFIG_SDHC_BUFFER_ALIGNMENT, see here: https://github.com/zephyrproject-rtos/zephyr/blob/18636af6d6658cf66e5f055901ffe2515116d329/subsys/sd/sd_ops.c

So it should be possible to always use the DMA function when dev_data->dma_support is true unless I'm missing something specific to this controller?

Thanks for pointing that out. I need some time to investigate it thoroughly.

danieldegrasse · 2023-07-13T15:43:08Z

drivers/sdhc/rcar_mmc.c

I'm confused what the regulator driver is used for here. It seems these fixed regulators cannot be enabled or disabled, and are just being used to determine the voltage ranges supported by the SDHC? Or is there a method to enable and disable these regulators I'm missing?

It is a common API used to determine the supported voltage of the regulator. However, this driver is not aware of the specific regulator used underneath. For example, it could be a GPIO regulator for SD cards.
Unfortunately, the GPIO regulator driver is still under review. Once it is merged, I will add overlays to samples and make additional changes to the boards dts/dtsi files.

danieldegrasse · 2023-07-13T15:44:18Z

dts/arm64/renesas/rcar_gen3_ca57.dtsi

I'm slightly confused about these regulator nodes:

Should always-on be present, or boot-on? These regulators are not always enabled, and they are managed via the SDHC driver.

Based on the changes to the fixed regulator driver and the lack of an enable pin, are these regulator nodes just being used to determine the voltage the SD host controller supports? If so, we should just make this a property of the host controller binding.

Should always-on be present, or boot-on?

Yes, it means that you cannot disable this regulator and it's enabled after boot of board.

These regulators are not always enabled, and they are managed via the SDHC driver.

The common regulator API examines the always-on flag, and in cases where we request turning off or on, it simply ignores the request. You can refer to the source code at https://elixir.bootlin.com/zephyr/latest/source/drivers/regulator/regulator_common.c#L89 for more details.

Based on the changes to the fixed regulator driver and the lack of an enable pin, are these regulator nodes just being used to determine the voltage the SD host controller supports?

In the SD/MMC driver, we utilize a common regulator API for voltage control (get/set) and enabling/disabling regulators. While we cannot control power for this particular device, there might be other devices, such as SD cards, for which power control is possible. GPIO regulators will be used for SD cards for R-Car Gen3/Gen4 boards. The pull request can be found here: #58411.

Ok- I'm not familiar with the Rcar boards, but given that #58411 is merged, can this PR be updated to use that driver for either of these boards?

I believe it would be better to address these changes in a separate pull request. Adding new modifications to this one will only extend the review time.

The driver introduced in #58411 can be used for SD cards, I'll add support of them in a separate pull after merging this one.

+1 to rebasing on the current code state. We don't want to introduce code that is known to ignore existing features.

Ah, I got this. This is for eMMC, when regulators are indeed fixed. It it strange that you didn't mentioned in the commit message, that you are adding DTS definitions for eMMC.

Nothing seems strange to me. Feel free to walk through Zephyr commits in the 'dts' directory and look at the commit descriptions to see how other people have done it.

danieldegrasse · 2023-07-13T15:46:45Z

boards/arm64/rcar_salvator_xs_m3/rcar_salvator_xs_m3.dts

Why declare the disk here, if we are only going to enable it in overlays? Couldn't the disk be defined and enabled within the overlay itself?

I see no reason to declare the compatible property in every instance. It should always be the same for all the MMC/SD nodes.

Certainly, I can move the complete definition of the disk nodes to overlays. If you want, I'll do it.

I would personally prefer we declare the disk in overlays, but I don't feel too strongly about it. Feel free to leave this as is

danieldegrasse · 2023-07-13T15:49:34Z

subsys/fs/littlefs_fs.c

Commit message needs to be updated here, this is an update to the LFS subsystem. It should also be in a separate PR (although I assume the change here is to deal with 64 bit pointers)

done, #60452

xakep-amatop · 2023-08-02T10:09:27Z

Hi, @danieldegrasse @lorc @aaillet

I want to become a maintainer of this driver. Is it possible or not? Do I need to get additional approvals from someone else?

aaillet · 2023-08-02T10:25:26Z

Hi, @danieldegrasse @lorc @aaillet

I want to become a maintainer of this driver. Is it possible or not? Do I need to get additional approvals from someone else?

From my POV it's okay, add yourself as such in CODEOWNER file.
This file is supposed not to be used anymore but you will also become a collaborator after merge of #61066 so it's okay.
You can get more information about official "promotion" mechanism here : https://docs.zephyrproject.org/latest/project/project_roles.html

xakep-amatop · 2024-03-05T08:34:23Z

fixed conflicts

danieldegrasse · 2024-03-05T18:34:51Z

fixed conflicts

Can you take a look at the failing testcases?

xakep-amatop · 2024-03-06T10:20:06Z

fixed conflicts

Can you take a look at the failing testcases?

done

xakep-amatop · 2024-03-14T07:09:39Z

@danieldegrasse @lorc please take another look at this pull request.

Add basic functionality of Renesas SD/MMC driver. It can be used for both gen3 and gen4 R-car boards, but tested only with H3ULCB, Salvator XS M3 and Spider boards. This driver working with SDHC subsystem. The driver supports regularal reading/writing throught SD/MMC controller buffer, DMA mode w/o interrupts and timing tuning. Add gpio5 and sd0 nodes to h3ulcb and salvator xs which are needed for working with SD cards. The GPIO node is needed for switching voltage on SD card through gpio regulator driver. Notes: * the driver doesn't support SPI mode; * SCC tuning and DMA mode based on IRQs are enabled by default; * an address of a data buffer has to be aligned to 128 bytes if it is not, driver will use non-DMA mode automatically; * Renesas MMC DMAC doesn't support 64-bit DMA addresses, so for case when we have 64-bit xref data address we use non-DMA mode; * SD/MMC controller supports block size between 512 and 1 with a lot of restrictions, more details you can find in code; * support of HS400 mode isn't implemented inside driver. Signed-off-by: Mykola Kvach <[email protected]>

Add pin control group for UHS modes for H3ULCB, Salvator XS M3 boards to appropriate dts files. Both 'uhs' and 'default' pins states have the same properties for eMMC, e.g. 1.8V. Signed-off-by: Mykola Kvach <[email protected]>

Add MMC related cfgs/overlays of arm64 gen3 rcar boards to perf and susys/sd/mmc tests. Signed-off-by: Mykola Kvach <[email protected]>

Add support of R-Car H3ULCB and Salvator XS M3 boards to littlefs sample. Signed-off-by: Mykola Kvach <[email protected]>

xakep-amatop · 2024-03-14T11:50:06Z

fixed conflicts

danieldegrasse · 2024-03-20T16:00:40Z

@jfischer-no can you please revisit this pull request?

xakep-amatop · 2024-03-30T16:57:24Z

@danieldegrasse would you please review it again?

danieldegrasse · 2024-04-01T14:54:12Z

Problematic or unnecessary comment style not addressed. A large number of doxygen-style comments for no reason. Not only is this unnecessary and unused, but it bloats the code, requires maintenance, and causes unnecessary diff lines.
Also increases the driver's carbon footprint.

@jfischer-no is there a specific reason you don't want to use doxygen-style comments in the code for this PR? From what I can tell, this does not result in the functions being added to the Zephyr project API documentation, and other drivers in tree do use this style. If there is a code or style guideline I'm missing that would mandate this, please let me know. Otherwise, I do not want to continue to block the movement of this PR based on this issue.

xakep-amatop · 2024-04-15T13:37:40Z

@danieldegrasse ping

danieldegrasse · 2024-04-22T15:03:34Z

@jfischer-no is there a specific reason you don't want to use doxygen-style comments in the code for this PR? From what I can tell, this does not result in the functions being added to the Zephyr project API documentation, and other drivers in tree do use this style. If there is a code or style guideline I'm missing that would mandate this, please let me know. Otherwise, I do not want to continue to block the movement of this PR based on this issue.

@jfischer-no Please revisit your block (or add a comment clarifying your reasoning above) before Thursday (April 22nd, 2024). Otherwise, following the criteria given by the project I will have to dismiss your review to in order to move this PR forwards.

(To be clear, the reason for dismissal would be The reviewer did not revisit the pull request after 2 week and multiple pings by the author. While @xakep-amatop has not pinged you recently, I have done so multiple times on their behalf.)

Dismissing review due to no response from reviewer

xakep-amatop force-pushed the add-support-of-rcar-mmc-driver branch 2 times, most recently from 18d519a to f549562 Compare July 12, 2023 11:20

xakep-amatop marked this pull request as ready for review July 12, 2023 11:51

xakep-amatop requested review from carlocaione, danieldegrasse, galak, gmarull and nashif as code owners July 12, 2023 11:51

zephyrbot requested review from Laczen, SgrrZhf, aaillet, aasinclair, de-nordic, jfischer-no, lorc, npitre, pmarzin and povergoing July 12, 2023 11:51

danieldegrasse reviewed Jul 13, 2023

View reviewed changes

xakep-amatop force-pushed the add-support-of-rcar-mmc-driver branch from f549562 to 919b264 Compare July 17, 2023 22:38

xakep-amatop requested a review from danieldegrasse July 17, 2023 22:47

MaureenHelm removed the dev-review To be discussed in dev-review meeting label Feb 14, 2024

xakep-amatop requested a review from jfischer-no February 20, 2024 11:35

xakep-amatop dismissed stale reviews from lorc and danieldegrasse via 9faa8d5 March 5, 2024 08:32

xakep-amatop force-pushed the add-support-of-rcar-mmc-driver branch from b8f99e4 to 9faa8d5 Compare March 5, 2024 08:32

xakep-amatop force-pushed the add-support-of-rcar-mmc-driver branch from 9faa8d5 to e1d7fae Compare March 6, 2024 07:58

xakep-amatop requested review from danieldegrasse and lorc March 6, 2024 10:20

xakep-amatop added 4 commits March 14, 2024 13:40

tests: add h3ulcb and salvator xs m3configs to perf disk tests

038552b

Add MMC related cfgs/overlays of arm64 gen3 rcar boards to perf and susys/sd/mmc tests. Signed-off-by: Mykola Kvach <[email protected]>

samples: littlefs: add support for H3ULCB and Salvator XS M3

80dc96d

Add support of R-Car H3ULCB and Salvator XS M3 boards to littlefs sample. Signed-off-by: Mykola Kvach <[email protected]>

lorc previously approved these changes Mar 14, 2024

View reviewed changes

xakep-amatop dismissed lorc’s stale review via 80dc96d March 14, 2024 11:49

xakep-amatop force-pushed the add-support-of-rcar-mmc-driver branch from e1d7fae to 80dc96d Compare March 14, 2024 11:49

xakep-amatop requested a review from lorc March 14, 2024 11:50

lorc approved these changes Mar 14, 2024

View reviewed changes

danieldegrasse approved these changes May 1, 2024

View reviewed changes

nashif merged commit 27fa6d9 into zephyrproject-rtos:main May 1, 2024

Add support of rcar mmc driver #60285

Add support of rcar mmc driver #60285

Uh oh!

Conversation

xakep-amatop commented Jul 12, 2023

Uh oh!

danieldegrasse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xakep-amatop commented Aug 2, 2023

Uh oh!

aaillet commented Aug 2, 2023

Uh oh!

xakep-amatop commented Mar 5, 2024

Uh oh!

danieldegrasse commented Mar 5, 2024

Uh oh!

xakep-amatop commented Mar 6, 2024

Uh oh!

xakep-amatop commented Mar 14, 2024

Uh oh!

xakep-amatop commented Mar 14, 2024

Uh oh!

danieldegrasse commented Mar 20, 2024

Uh oh!

danieldegrasse commented Apr 1, 2024 •

edited

Loading