drivers: sdhc: add driver support for emmc host controller #61108

najumon1980 · 2023-08-03T10:13:47Z

add driver support for emmc host controller (eMMC 5.1 compliant ). Following are some of the key supported features:

Supported Mode: HS400, HS200, DDR50, SDR104, SDR50, SDR25 and SDR12
DMA support: Both SDMA and ADMA supported
Data Width: 1bit, 4bit and 8Bit
Support Interrupt driven as well as polled mode

danieldegrasse · 2023-08-11T21:46:24Z

drivers/sdhc/emmc_host.c

+}
+#endif
+
+static int emmc_reset(const struct device *dev)


I'm not familiar with this hardware, but it seems like emmc_reset is being used to reset the state of the emmc card itself. Currently the SD subsystem toggles power to the SD card at init to reset it. For emmc, would it be useful to also call sdhc_hw_reset during the SD init sequence, so the emmc card state state is reset even during a soft reset?

I believe power cycle of card is expected to be invoke via mmc stack using sdhc_set_io() as part of boot time or as part of error recovery and driver shall provide only low level interface for them.

The Zephyr mmc.h mentioned following comments as well.
"sdhc_hw_reset() Used when the SDHC has encountered an error. Resetting the SDHC controller should clear all errors on the SDHC, but does not necessarily reset I/O settings to boot (this can be done with ref sdhc_set_io)"

This is correct- sdhc_set_io is intended to reset I/O settings to boot by toggling card power. However, perhaps we need to reevalute when sdhc_rw_reset is called in the SD subsystem, since eMMC cards may not have independent power control

What I'm wondering here is if you feel adding a call to sdhc_hw_reset during the SD initialization phase would be useful for eMMC cards- since they cannot always toggle power.

I am really worried about adding it since it seems to be WA for me. My suggestion is that let the stack do such error handling scenario.

drivers/sdhc/emmc_host.c

drivers/sdhc/Kconfig.emmc

danieldegrasse · 2023-08-11T22:12:22Z

dts/bindings/sdhc/sdhc.yaml

@@ -59,3 +59,13 @@ properties:
    type: boolean
    description: |
      The host controller supports HS400 at 1.8V
+
+  mmc-dw-8bit:


This commit needs to be first in the PR, since the emmc driver depends on these properties.

Also, please use a title similar to the following:

dts: bindings: sdhc: add dts entry for sdhc bus width

This commit is before driver commit.

subsys/sd/sd.c

decsny

requesting change to compatible string

subsys/sd/sd.c

dts/bindings/sdhc/sdhc.yaml

dts/bindings/sdhc/emmc-host.yaml

jfischer-no

It is not really clear what "native driver" means in this context.

drivers/sdhc/emmc_host.c

danieldegrasse · 2023-09-21T18:11:28Z

Currently our scope limited eMMC and my suggestion is as someone start port this driver on their platform and test on SD card as well then then can enhance the features/scope.

Understood- if the scope is limited to eMMC on the alder lake platform, then I would like to see file and Kconfig names reflect this. We can leave the C function names untouched, as those aren't really user facing, but I want it to be clear that this driver may implement the SD host controller specification, but it is only designed to function with the eMMC controller on the alder lake platform.

If we want to keep Kconfig and file names generic, then I would like this driver to be generic enough to (at a minimum) support the SD and eMMC stack. Ideally we would validate this driver on additional host controllers, but I am not aware of another one that complies fully with the SDHC spec.

najumon1980 · 2023-09-22T04:43:18Z

Currently our scope limited eMMC and my suggestion is as someone start port this driver on their platform and test on SD card as well then then can enhance the features/scope.

Understood- if the scope is limited to eMMC on the alder lake platform, then I would like to see file and Kconfig names reflect this. We can leave the C function names untouched, as those aren't really user facing, but I want it to be clear that this driver may implement the SD host controller specification, but it is only designed to function with the eMMC controller on the alder lake platform.

If we want to keep Kconfig and file names generic, then I would like this driver to be generic enough to (at a minimum) support the SD and eMMC stack. Ideally we would validate this driver on additional host controllers, but I am not aware of another one that complies fully with the SDHC spec.

Agree and updated file name and Kconfig to intel specific

danieldegrasse · 2023-09-22T15:00:21Z

subsys/sd/sd.c

+	}
+
+	/* Set to maximum voltage support by Host controller */
+	card->card_voltage = voltage;


Sorry, just noticed this- you don't need to assign to card->card_voltage here, since you already do below

This was existing logic and I updated to maximum HC supported voltage instead of pre-defined SD_VOL_3_3_V. It seems the initial logic assume card->card_voltage will get update after reset

Not sure I follow- The issue I am highlighting is that card->card_voltage is being set twice in this function, and it only needs to be set once (on line 163). The initial logic here set card_voltage once on line 163, and your changes introduce a second assignment to it on line 144. Is there a reason we need this assignment?

removed the duplicate code

drivers/sdhc/Kconfig.intel

danieldegrasse · 2023-09-22T15:03:23Z

drivers/sdhc/Kconfig.intel

+
+config INTEL_EMMC_HOST_ADMA_DESC_SIZE
+	int "EMMC host controller ADMA Descriptor size"
+	default 32


Should this be dependent on INTEL_EMMC_HOST_ADMA?

Since we use IS_ENABLED this might cause build error during compile time

Since we use IS_ENABLED this might cause build error during compile time

It seems you're then also waisting memory, since struct emmc_data will always contain the desc_table even if ADMA is not enabled. It might be worth looking into different ways of fixing this. Some options come to mind:

Stop using IS_ENABLED

Refactor the code so the memory is allocated in a separate struct

Use something like this:

#ifdef CONFIG_INTEL_EMMC_HOST_ADMA_DESC_SIZE #define ADMA_DESC_SIZE CONFIG_INTEL_EMMC_HOST_ADMA_DESC_SIZE #else #define ADMA_DESC_SIZE 0 #endif

For the record, the current code works fine, so this could also be done as a follow-up PR (especially considering that time is running low on getting a new driver merged before the feature freeze)

updated as per your suggestion

danieldegrasse · 2023-09-22T15:05:36Z

dts/bindings/sdhc/sdhc.yaml

-
-  mmc-dw-4bit:
-    type: boolean
+  bus-width:


You introduce these properties in a prior commit, then modify them here. Please instead add the final version of this property (the bus width prop) in the initial commit where you modify the SDHC yaml

Also, is this property part of the IP, or determined by how the eMMC is connected? What I'm asking here is does the IP always support 8 bit bus width, but if only 4 data pins are routed than the user would be expected to set bus-width=4? Or do some instances of the IP only support 4 data pins?

Moved to original commit. Also regarding the bus width, Intel IP support both 4 and 8 bit and if someone design their board with only 4bit interface then they can update dts accordingly to limit driver settings.

Ok, then can't we use the bus-width property on the MMC node itself, rather than setting this at the IP level? https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/sd/zephyr%2Cmmc-disk.yaml#L12

If so, we should drop this change from the PR

agree and updated the code

najumon1980 · 2023-09-26T07:06:59Z

@danieldegrasse I updated/respond to your comments, could you please have a look ? I would like this PR to be merged before code freeze which is this Friday

drivers/sdhc/intel_emmc_host.c

najumon1980 · 2023-09-27T04:20:55Z

@danieldegrasse @jhedberg updated as per you review comment, please have a look

najumon1980 · 2023-09-27T10:52:25Z

@decsny I update the PR as per you comments, please have a look

decsny · 2023-09-28T15:35:03Z

@najumon1980

please see #59886 about bus width property, your PR conflicts conceptually with what is in tree currently

@decsny

Dismissing review on behalf of @decsny, as they are out of town and request has been addressed

danieldegrasse · 2023-09-28T18:30:33Z

@najumon1980- just to be clear- please address my comment about the duplicate assignment to card->card_voltage as well as the bus-width DT property. Other than that I am ok to merge this PR.

add check for maximum voltage supported by hc before apply card voltage. Signed-off-by: Najumon B.A <najumon.ba@intel.com>

add host controller driver support for emmc version 5.1. The driver expose zephyr sdhc api interface for emmc host controller. Signed-off-by: Najumon B.A <najumon.ba@intel.com>

add DTS entry for enable eMMC support on Intel Alder lake platforms Signed-off-by: Najumon B.A <najumon.ba@intel.com>

najumon1980 · 2023-09-29T09:16:07Z

@najumon1980- just to be clear- please address my comment about the duplicate assignment to card->card_voltage as well as the bus-width DT property. Other than that I am ok to merge this PR.

@danieldegrasse I updated the PR and pls have a look

najumon1980 force-pushed the emmc branch 3 times, most recently from 8c9a2e2 to 58a2822 Compare August 4, 2023 01:40

najumon1980 marked this pull request as ready for review August 4, 2023 04:14

najumon1980 requested review from danieldegrasse and galak as code owners August 4, 2023 04:14

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Disk Access labels Aug 4, 2023

zephyrbot requested a review from jfischer-no August 4, 2023 04:14

zephyrbot assigned danieldegrasse Aug 4, 2023

jhedberg self-requested a review August 4, 2023 06:09

danieldegrasse reviewed Aug 11, 2023

View reviewed changes

najumon1980 force-pushed the emmc branch from 58a2822 to df4fab4 Compare August 18, 2023 14:38

najumon1980 requested review from dcpleung and nashif as code owners August 18, 2023 14:38

zephyrbot added the platform: X86 x86 and x86-64 label Aug 18, 2023

zephyrbot requested review from decsny, laurenmurphyx64 and tbursztyka August 18, 2023 14:39

najumon1980 force-pushed the emmc branch from df4fab4 to ded0240 Compare August 18, 2023 14:49

decsny previously requested changes Aug 18, 2023

View reviewed changes

subsys/sd/sd.c Show resolved Hide resolved

dts/bindings/sdhc/sdhc.yaml Outdated Show resolved Hide resolved

dts/bindings/sdhc/emmc-host.yaml Outdated Show resolved Hide resolved

najumon1980 force-pushed the emmc branch 3 times, most recently from aea3e4e to 2eab314 Compare August 22, 2023 07:25

danieldegrasse requested a review from decsny August 28, 2023 22:24

jfischer-no reviewed Aug 30, 2023

View reviewed changes

drivers/sdhc/emmc_host.c Outdated Show resolved Hide resolved

najumon1980 force-pushed the emmc branch 2 times, most recently from e582902 to 999a71e Compare September 12, 2023 09:01

najumon1980 changed the title ~~drivers: sdhc: add native driver support for emmc host controller~~ drivers: sdhc: add driver support for emmc host controller Sep 12, 2023

najumon1980 force-pushed the emmc branch from 999a71e to 518784b Compare September 12, 2023 10:36

najumon1980 force-pushed the emmc branch 3 times, most recently from 1316425 to afc7edc Compare September 19, 2023 13:22

najumon1980 force-pushed the emmc branch from f50064d to 8d36313 Compare September 22, 2023 04:42

danieldegrasse reviewed Sep 22, 2023

View reviewed changes

najumon1980 force-pushed the emmc branch from 8d36313 to 18808a4 Compare September 25, 2023 04:14

najumon1980 added this to the v3.5.0 milestone Sep 26, 2023

jhedberg reviewed Sep 26, 2023

View reviewed changes

drivers/sdhc/intel_emmc_host.c Outdated Show resolved Hide resolved

jhedberg reviewed Sep 26, 2023

View reviewed changes

drivers/sdhc/intel_emmc_host.c Show resolved Hide resolved

jhedberg reviewed Sep 26, 2023

View reviewed changes

drivers/sdhc/intel_emmc_host.c Outdated Show resolved Hide resolved

najumon1980 force-pushed the emmc branch from 18808a4 to 7fdcec5 Compare September 27, 2023 04:19

najumon1980 force-pushed the emmc branch from 7fdcec5 to a91d4bf Compare September 28, 2023 09:51

jhedberg previously approved these changes Sep 28, 2023

View reviewed changes

najumon1980 added 3 commits September 29, 2023 07:50

sd: add check for maximum supported voltage by host controller

8b8b714

add check for maximum voltage supported by hc before apply card voltage. Signed-off-by: Najumon B.A <najumon.ba@intel.com>

drivers: sdhc: add driver support for emmc host controller

c5b5202

add host controller driver support for emmc version 5.1. The driver expose zephyr sdhc api interface for emmc host controller. Signed-off-by: Najumon B.A <najumon.ba@intel.com>

boards: x86: add eMMC support for Intel Alder lake platform

b1d0b0d

add DTS entry for enable eMMC support on Intel Alder lake platforms Signed-off-by: Najumon B.A <najumon.ba@intel.com>

najumon1980 dismissed jhedberg’s stale review via b1d0b0d September 29, 2023 03:05

najumon1980 force-pushed the emmc branch from a91d4bf to b1d0b0d Compare September 29, 2023 03:05

jhedberg approved these changes Sep 29, 2023

View reviewed changes

danieldegrasse approved these changes Sep 29, 2023

View reviewed changes

carlescufi merged commit dfec79c into zephyrproject-rtos:main Sep 29, 2023
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: sdhc: add driver support for emmc host controller #61108

drivers: sdhc: add driver support for emmc host controller #61108

najumon1980 commented Aug 3, 2023 •

edited

danieldegrasse Aug 11, 2023

najumon1980 Aug 16, 2023

danieldegrasse Aug 18, 2023

najumon1980 Aug 21, 2023

danieldegrasse Aug 11, 2023

najumon1980 Aug 21, 2023

decsny left a comment

jfischer-no left a comment

danieldegrasse commented Sep 21, 2023

najumon1980 commented Sep 22, 2023

danieldegrasse Sep 22, 2023

najumon1980 Sep 25, 2023 •

edited

danieldegrasse Sep 28, 2023

najumon1980 Sep 29, 2023

danieldegrasse Sep 22, 2023

najumon1980 Sep 25, 2023

jhedberg Sep 27, 2023 •

edited

najumon1980 Sep 28, 2023

danieldegrasse Sep 22, 2023

najumon1980 Sep 25, 2023

danieldegrasse Sep 28, 2023 •

edited

najumon1980 Sep 29, 2023

najumon1980 commented Sep 26, 2023

najumon1980 commented Sep 27, 2023

najumon1980 commented Sep 27, 2023

decsny commented Sep 28, 2023

danieldegrasse commented Sep 28, 2023

najumon1980 commented Sep 29, 2023

drivers: sdhc: add driver support for emmc host controller #61108

drivers: sdhc: add driver support for emmc host controller #61108

Conversation

najumon1980 commented Aug 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decsny left a comment

Choose a reason for hiding this comment

jfischer-no left a comment

Choose a reason for hiding this comment

danieldegrasse commented Sep 21, 2023

najumon1980 commented Sep 22, 2023

Choose a reason for hiding this comment

najumon1980 Sep 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhedberg Sep 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldegrasse Sep 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

najumon1980 commented Sep 26, 2023

najumon1980 commented Sep 27, 2023

najumon1980 commented Sep 27, 2023

decsny commented Sep 28, 2023

danieldegrasse commented Sep 28, 2023

najumon1980 commented Sep 29, 2023

najumon1980 commented Aug 3, 2023 •

edited

najumon1980 Sep 25, 2023 •

edited

jhedberg Sep 27, 2023 •

edited

danieldegrasse Sep 28, 2023 •

edited