On 24/08/20 8:05 pm, Raul Rangel wrote: > On Thu, Aug 20, 2020 at 2:56 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote: >> >> On 20/08/20 1:56 am, Raul Rangel wrote: >>> I noticed some odd things with the MMC/SDHCI code while debugging an >>> HS400 tuning issue. >>> >>> 1) Is it expected that we never enable SDHCI_CTRL_PRESET_VAL_ENABLE >>> for an eMMC running at HS200 or HS400? >> >> Seems like an oversight. eMMC transfer modes are not supported by the SDHCI >> specification, and many drivers use SDHCI_QUIRK2_PRESET_VALUE_BROKEN, so it >> looks like it has never been noticeable. >> > > Thanks for the confirmation. I'll get a patch sent to fix it. > >>> The flow for enabling HS400 is: Legacy -> HS200 -> Perform Tuning -> >>> HS -> HS400. >>> Looking at [sdhci_set_ios](https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/third_party/kernel/v5.4/drivers/mmc/host/sdhci.c;l=2019), >>> it looks like it's responsible for enabling presets. >>> >>> if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) && >>> ((ios->timing == MMC_TIMING_UHS_SDR12) || >>> (ios->timing == MMC_TIMING_UHS_SDR25) || >>> (ios->timing == MMC_TIMING_UHS_SDR50) || >>> (ios->timing == MMC_TIMING_UHS_SDR104) || >>> (ios->timing == MMC_TIMING_UHS_DDR50) || >>> (ios->timing == MMC_TIMING_MMC_DDR52))) { >>> u16 preset; >>> >>> sdhci_enable_preset_value(host, true); >>> preset = sdhci_get_preset_value(host); >>> ios->drv_type = (preset & SDHCI_PRESET_DRV_MASK) >>> >> SDHCI_PRESET_DRV_SHIFT; >>> } >>> >>> MMC_TIMING_MMC_HS200 and MMC_TIMING_MMC_HS400 are missing from the >>> conditions, so we never enable presets. This means that by default >>> (only 2 controllers provide select_drive_strength) we use drive >>> strength B for both the card and the controller. >>> >>> int mmc_select_drive_strength(struct mmc_card *card, unsigned int max_dtr, >>> int card_drv_type, int *drv_type) >>> { >>> struct mmc_host *host = card->host; >>> int host_drv_type = SD_DRIVER_TYPE_B; >>> >>> *drv_type = 0; >>> >>> if (!host->ops->select_drive_strength) >>> return 0; >>> ... >>> } >>> >>> Here is a trace log showing HS400 initialization: https://0paste.com/79874 >>> >>> 2) When performing HS400 tuning we end up enabling presets. >>> The re-tuning sequence is: HS400->DDR52->HS->HS200->Perform Tuning->HS->HS400 >>> >>> So when we transition to DDR52 the code above enables presets. You can >>> see this happening in this trace log: https://0paste.com/79875. Look >>> at timestamp 1926.173800. >>> >>> This means that the card and controller have the potential to have >>> mismatching drive strengths. This can be seen at timestamp >>> 1926.175813.The HS200 preset on my controller is configured to A, but >>> the card would be set up as B (if the controller didn't override >>> select_drive_strength). >>> >>> Should we be enabling presets for HS200/HS400 (and potentially >>> MMC_HS), or should we remove MMC_DDR52 from the condition above? >> >> The only things that matter are: >> 1. don't break other drivers >> 2. do make it work for your driver >> >> So we can't universally enable presets for HS200 and HS400, nor remove >> MMC_DDR52, but we can do something to make it work for you. >> > > Makes sense. I'll see what direction I want to take. > >>> >>> It looks like 0dafa60eb2506 ("mmc: sdhci: also get preset value and >>> driver type for MMC_DDR52") was the CL that added MMC_DDR52 to the >>> condition. >>> >>> 3) How do we ensure that card drive strength and controller drive >>> strength stay in sync when presets are enabled? >> >> Is that your requirement? Which driver is it? > > We want to provide a knob for our OEMs so they can choose the best > drive strength for their design. Right now for the AMD controller it's > hard coded to A: > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/third_party/kernel/v5.4/drivers/mmc/host/sdhci-acpi.c;l=542 > > I was hoping to use the SDHCI presets to not introduce a non-standard > convention of passing in the drive strength. I wasn't aware that the > SDHCI driver doesn't use presets for eMMC. So maybe the path of least > resistance is to add an ACPI property that is read by > amd_select_drive_strength. Like `mmc_of_parse` does with > `fixed-emmc-driver-type`. This ensures that both the card and > controller drive strengths are set. > > It looks like `host->fixed_drv_type` is only for the card. It doesn't > set the same drive strength on the controller. Is this intentional? You would need to ask the author of the patch, but it seems reasonable to assume it is intentional. > > static void mmc_select_driver_type(struct mmc_card *card) > { > int card_drv_type, drive_strength, drv_type = 0; > int fixed_drv_type = card->host->fixed_drv_type; > > card_drv_type = card->ext_csd.raw_driver_strength | > mmc_driver_type_mask(0); > > if (fixed_drv_type >= 0) <--- Never updates drv_type > drive_strength = card_drv_type & > mmc_driver_type_mask(fixed_drv_type) > ? fixed_drv_type : 0; > else > drive_strength = mmc_select_drive_strength(card, > > card->ext_csd.hs200_max_dtr, > > card_drv_type, &drv_type); > > card->drive_strength = drive_strength; > > if (drv_type) > mmc_set_driver_type(card->host, drv_type); > } > >> >>> Right now mmc_select_driver_type is only called from >>> `mmc_select_hs400es` and `mmc_select_hs200`. `mmc_select_driver_type >>> doesn't currently take the timing into account when making a decision. >>> Only two devices currently provide the `select_drive_strength` >>> override, so we are setting the card to drive strength B for most >>> controllers. >>> >>> Should we modify mmc_select_drive_strength to take in the target >>> timing so it can return the correct card drive strength. We could then >>> add an sdhci_select_drive_strength that queries the preset registers >>> (if enabled) and returns the target drive strength. >>> >>> 4) Should we be calling `mmc_select_driver_type` from >>> `mmc_hs400_to_hs200` and `mmc_hs200_to_hs400` during the transitions? >> >> The same driver strength continues to be used for HS200 and HS400 i.e. >> card->driver_strength >> >>> Or do we not care? >>> >>> Sorry for the long email... >>> Once I get some guidance, I can send some patches. >> >> Generally people first want to know what problem you are trying to solve. > I was just thinking we need to keep the drive strengths in sync > between controller and card. AFAIK there is no general requirement for that.
