| Message ID | 20200928154718.2.Ic6b6031366f090393d00a53fd69e1ada31ceb29e@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] mmc: sdhci-acpi: AMDI0040: Set SDHCI_QUIRK2_PRESET_VALUE_BROKEN | expand |
On 29/09/20 12:59 am, Raul E Rangel wrote: > This change will allow platform designers better control over signal > integrity by allowing them to tune the HS200 and HS400 driver strengths. > > The driver strength was previously hard coded to A to solve boot > problems with certain platforms. This driver strength does not > universally apply to all platforms so we need a knob to adjust it. > > All older platforms currently have the SDR104 preset hard coded to A in > the firmware. This means that switching from the hard coded value in > the kernel to reading the SDR104 preset is a no-op for these platforms. > Newer platforms will have properly set presets. So this change will > support both new and old platforms. > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > > drivers/mmc/host/sdhci-acpi.c | 39 ++++++++++++++++++++++++++++++++--- > 1 file changed, 36 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c > index d335a34ad05b3..5c9a041af5b4b 100644 > --- a/drivers/mmc/host/sdhci-acpi.c > +++ b/drivers/mmc/host/sdhci-acpi.c > @@ -545,10 +545,43 @@ struct amd_sdhci_host { > > static int amd_select_drive_strength(struct mmc_card *card, > unsigned int max_dtr, int host_drv, > - int card_drv, int *drv_type) > + int card_drv, int *host_driver_strength) > { > - *drv_type = MMC_SET_DRIVER_TYPE_A; > - return MMC_SET_DRIVER_TYPE_A; > + struct sdhci_host *host = mmc_priv(card->host); > + u16 preset, preset_driver_strength; > + > + /* > + * This method is only called by mmc_select_hs200 so we only need to > + * read from the HS200 (SDR104) preset register. > + * > + * Firmware that has "invalid/default" presets return a driver strength > + * of A. This matches the previously hard coded value. > + */ > + preset = sdhci_readw(host, SDHCI_PRESET_FOR_SDR104); > + preset_driver_strength = > + (preset & SDHCI_PRESET_DRV_MASK) >> SDHCI_PRESET_DRV_SHIFT; > + > + /* > + * We want the controller driver strength to match the card's driver > + * strength so they have similar rise/fall times. > + * > + * The controller driver strength set by this method is sticky for all > + * timings after this method is called. This unfortunately means that > + * while HS400 tuning is in progress we end up with mismatched driver > + * strengths between the controller and the card. HS400 tuning requires > + * switching from HS400->DDR52->HS->HS200->HS400. So the driver mismatch > + * happens while in DDR52 and HS modes. This has not been observed to > + * cause problems. Enabling presets would fix this issue. > + */ > + *host_driver_strength = preset_driver_strength; > + > + /* > + * The resulting card driver strength is only set when switching the > + * card's timing to HS200 or HS400. The card will use the default driver > + * strength (B) for any other mode. > + */ > + return preset_driver_strength; > + > } > > static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host, bool enable) >
On Mon, 28 Sep 2020 at 23:59, Raul E Rangel <rrangel@chromium.org> wrote: > > This change will allow platform designers better control over signal > integrity by allowing them to tune the HS200 and HS400 driver strengths. > > The driver strength was previously hard coded to A to solve boot > problems with certain platforms. This driver strength does not > universally apply to all platforms so we need a knob to adjust it. > > All older platforms currently have the SDR104 preset hard coded to A in > the firmware. This means that switching from the hard coded value in > the kernel to reading the SDR104 preset is a no-op for these platforms. > Newer platforms will have properly set presets. So this change will > support both new and old platforms. > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> Applied for next, thanks! Kind regards Uffe > --- > > drivers/mmc/host/sdhci-acpi.c | 39 ++++++++++++++++++++++++++++++++--- > 1 file changed, 36 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c > index d335a34ad05b3..5c9a041af5b4b 100644 > --- a/drivers/mmc/host/sdhci-acpi.c > +++ b/drivers/mmc/host/sdhci-acpi.c > @@ -545,10 +545,43 @@ struct amd_sdhci_host { > > static int amd_select_drive_strength(struct mmc_card *card, > unsigned int max_dtr, int host_drv, > - int card_drv, int *drv_type) > + int card_drv, int *host_driver_strength) > { > - *drv_type = MMC_SET_DRIVER_TYPE_A; > - return MMC_SET_DRIVER_TYPE_A; > + struct sdhci_host *host = mmc_priv(card->host); > + u16 preset, preset_driver_strength; > + > + /* > + * This method is only called by mmc_select_hs200 so we only need to > + * read from the HS200 (SDR104) preset register. > + * > + * Firmware that has "invalid/default" presets return a driver strength > + * of A. This matches the previously hard coded value. > + */ > + preset = sdhci_readw(host, SDHCI_PRESET_FOR_SDR104); > + preset_driver_strength = > + (preset & SDHCI_PRESET_DRV_MASK) >> SDHCI_PRESET_DRV_SHIFT; > + > + /* > + * We want the controller driver strength to match the card's driver > + * strength so they have similar rise/fall times. > + * > + * The controller driver strength set by this method is sticky for all > + * timings after this method is called. This unfortunately means that > + * while HS400 tuning is in progress we end up with mismatched driver > + * strengths between the controller and the card. HS400 tuning requires > + * switching from HS400->DDR52->HS->HS200->HS400. So the driver mismatch > + * happens while in DDR52 and HS modes. This has not been observed to > + * cause problems. Enabling presets would fix this issue. > + */ > + *host_driver_strength = preset_driver_strength; > + > + /* > + * The resulting card driver strength is only set when switching the > + * card's timing to HS200 or HS400. The card will use the default driver > + * strength (B) for any other mode. > + */ > + return preset_driver_strength; > + > } > > static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host, bool enable) > -- > 2.28.0.709.gb0816b6eb0-goog >
On Mon, 5 Oct 2020 at 14:50, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Mon, 28 Sep 2020 at 23:59, Raul E Rangel <rrangel@chromium.org> wrote: > > > > This change will allow platform designers better control over signal > > integrity by allowing them to tune the HS200 and HS400 driver strengths. > > > > The driver strength was previously hard coded to A to solve boot > > problems with certain platforms. This driver strength does not > > universally apply to all platforms so we need a knob to adjust it. > > > > All older platforms currently have the SDR104 preset hard coded to A in > > the firmware. This means that switching from the hard coded value in > > the kernel to reading the SDR104 preset is a no-op for these platforms. > > Newer platforms will have properly set presets. So this change will > > support both new and old platforms. > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > Applied for next, thanks! Turned out that this doesn't build, hence I am dropping it from my next branch. Please submit a new version. [...] Kind regards Uffe
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c index d335a34ad05b3..5c9a041af5b4b 100644 --- a/drivers/mmc/host/sdhci-acpi.c +++ b/drivers/mmc/host/sdhci-acpi.c @@ -545,10 +545,43 @@ struct amd_sdhci_host { static int amd_select_drive_strength(struct mmc_card *card, unsigned int max_dtr, int host_drv, - int card_drv, int *drv_type) + int card_drv, int *host_driver_strength) { - *drv_type = MMC_SET_DRIVER_TYPE_A; - return MMC_SET_DRIVER_TYPE_A; + struct sdhci_host *host = mmc_priv(card->host); + u16 preset, preset_driver_strength; + + /* + * This method is only called by mmc_select_hs200 so we only need to + * read from the HS200 (SDR104) preset register. + * + * Firmware that has "invalid/default" presets return a driver strength + * of A. This matches the previously hard coded value. + */ + preset = sdhci_readw(host, SDHCI_PRESET_FOR_SDR104); + preset_driver_strength = + (preset & SDHCI_PRESET_DRV_MASK) >> SDHCI_PRESET_DRV_SHIFT; + + /* + * We want the controller driver strength to match the card's driver + * strength so they have similar rise/fall times. + * + * The controller driver strength set by this method is sticky for all + * timings after this method is called. This unfortunately means that + * while HS400 tuning is in progress we end up with mismatched driver + * strengths between the controller and the card. HS400 tuning requires + * switching from HS400->DDR52->HS->HS200->HS400. So the driver mismatch + * happens while in DDR52 and HS modes. This has not been observed to + * cause problems. Enabling presets would fix this issue. + */ + *host_driver_strength = preset_driver_strength; + + /* + * The resulting card driver strength is only set when switching the + * card's timing to HS200 or HS400. The card will use the default driver + * strength (B) for any other mode. + */ + return preset_driver_strength; + } static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host, bool enable)
This change will allow platform designers better control over signal integrity by allowing them to tune the HS200 and HS400 driver strengths. The driver strength was previously hard coded to A to solve boot problems with certain platforms. This driver strength does not universally apply to all platforms so we need a knob to adjust it. All older platforms currently have the SDR104 preset hard coded to A in the firmware. This means that switching from the hard coded value in the kernel to reading the SDR104 preset is a no-op for these platforms. Newer platforms will have properly set presets. So this change will support both new and old platforms. Signed-off-by: Raul E Rangel <rrangel@chromium.org> --- drivers/mmc/host/sdhci-acpi.c | 39 ++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-)