On Fri, 3 May 2019 at 13:45, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>
> On 3/05/19 12:59 PM, Ulf Hansson wrote:
> > On Thu, 2 May 2019 at 09:53, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> >>
> >> Some time ago, a fix was done for the sdhci-acpi driver, refer
> >> commit 6e1c7d6103fe ("mmc: sdhci-acpi: Reduce Baytrail eMMC/SD/SDIO
> >> hangs"). The same issue was not expected to affect the sdhci-pci driver,
> >> but there have been reports to the contrary, so make the same hardware
> >> setting change.
> >>
> >> This patch applies to v5.0+ but before that backports will be required.
> >>
> >> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> ---
> >> drivers/mmc/host/Kconfig | 1 +
> >> drivers/mmc/host/sdhci-pci-core.c | 89 +++++++++++++++++++++++++++++++
> >> 2 files changed, 90 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> >> index 9c01310a0d2e..d084a9d63623 100644
> >> --- a/drivers/mmc/host/Kconfig
> >> +++ b/drivers/mmc/host/Kconfig
> >> @@ -92,6 +92,7 @@ config MMC_SDHCI_PCI
> >> tristate "SDHCI support on PCI bus"
> >> depends on MMC_SDHCI && PCI
> >> select MMC_CQHCI
> >> + select IOSF_MBI if X86
> >> help
> >> This selects the PCI Secure Digital Host Controller Interface.
> >> Most controllers found today are PCI devices.
> >> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> >> index a3d7a9db76c5..64e79a19d5ad 100644
> >> --- a/drivers/mmc/host/sdhci-pci-core.c
> >> +++ b/drivers/mmc/host/sdhci-pci-core.c
> >> @@ -31,6 +31,10 @@
> >> #include <linux/mmc/sdhci-pci-data.h>
> >> #include <linux/acpi.h>
> >>
> >> +#ifdef CONFIG_X86
> >> +#include <asm/iosf_mbi.h>
> >> +#endif
> >> +
> >> #include "cqhci.h"
> >>
> >> #include "sdhci.h"
> >> @@ -451,6 +455,50 @@ static const struct sdhci_pci_fixes sdhci_intel_pch_sdio = {
> >> .probe_slot = pch_hc_probe_slot,
> >> };
> >>
> >> +#ifdef CONFIG_X86
> >> +
> >> +#define BYT_IOSF_SCCEP 0x63
> >> +#define BYT_IOSF_OCP_NETCTRL0 0x1078
> >> +#define BYT_IOSF_OCP_TIMEOUT_BASE GENMASK(10, 8)
> >> +
> >> +static void byt_ocp_setting(struct pci_dev *pdev)
> >> +{
> >> + u32 val = 0;
> >> +
> >> + if (pdev->device != PCI_DEVICE_ID_INTEL_BYT_EMMC &&
> >> + pdev->device != PCI_DEVICE_ID_INTEL_BYT_SDIO &&
> >> + pdev->device != PCI_DEVICE_ID_INTEL_BYT_SD &&
> >> + pdev->device != PCI_DEVICE_ID_INTEL_BYT_EMMC2)
> >> + return;
> >> +
> >> + if (iosf_mbi_read(BYT_IOSF_SCCEP, MBI_CR_READ, BYT_IOSF_OCP_NETCTRL0,
> >> + &val)) {
> >> + dev_err(&pdev->dev, "%s read error\n", __func__);
> >> + return;
> >> + }
> >> +
> >> + if (!(val & BYT_IOSF_OCP_TIMEOUT_BASE))
> >> + return;
> >> +
> >> + val &= ~BYT_IOSF_OCP_TIMEOUT_BASE;
> >> +
> >> + if (iosf_mbi_write(BYT_IOSF_SCCEP, MBI_CR_WRITE, BYT_IOSF_OCP_NETCTRL0,
> >> + val)) {
> >> + dev_err(&pdev->dev, "%s write error\n", __func__);
> >> + return;
> >> + }
> >> +
> >> + dev_dbg(&pdev->dev, "%s completed\n", __func__);
> >> +}
> >> +
> >> +#else
> >> +
> >> +static inline void byt_ocp_setting(struct pci_dev *pdev)
> >> +{
> >> +}
> >> +
> >> +#endif
> >> +
> >> enum {
> >> INTEL_DSM_FNS = 0,
> >> INTEL_DSM_V18_SWITCH = 3,
> >> @@ -715,6 +763,8 @@ static void byt_probe_slot(struct sdhci_pci_slot *slot)
> >>
> >> byt_read_dsm(slot);
> >>
> >> + byt_ocp_setting(slot->chip->pdev);
> >> +
> >> ops->execute_tuning = intel_execute_tuning;
> >> ops->start_signal_voltage_switch = intel_start_signal_voltage_switch;
> >>
> >> @@ -971,7 +1021,44 @@ static const struct sdhci_pci_fixes sdhci_intel_glk_emmc = {
> >> .priv_size = sizeof(struct intel_host),
> >> };
> >>
> >> +#ifdef CONFIG_PM_SLEEP
> >> +
> >> +static int byt_resume(struct sdhci_pci_chip *chip)
> >> +{
> >> + byt_ocp_setting(chip->pdev);
> >> +
> >> + return sdhci_pci_resume_host(chip);
> >> +}
> >> +
> >> +#define BYT_SPM_OPS .resume = byt_resume,
> >> +
> >> +#else
> >> +
> >> +#define BYT_SPM_OPS
> >> +
> >> +#endif
> >> +
> >> +#ifdef CONFIG_PM
> >> +
> >> +static int byt_runtime_resume(struct sdhci_pci_chip *chip)
> >> +{
> >> + byt_ocp_setting(chip->pdev);
> >> +
> >> + return sdhci_pci_runtime_resume_host(chip);
> >> +}
> >> +
> >> +#define BYT_RPM_OPS .runtime_resume = byt_runtime_resume,
> >> +
> >> +#else
> >> +
> >> +#define BYT_RPM_OPS
> >> +
> >> +#endif
> >> +
> >> +#define BYT_PM_OPS BYT_SPM_OPS BYT_RPM_OPS
> >
> > This ifdef hackary is a bit too much for me. :-)
> >
> > Could you have the callbacks being assigned always and instead rely
> > only on byt_ocp_setting() having different implementations, depending
> > on if CONFIG_X86 is set or not?
>
> The callbacks themselves are under ifdef, so that wouldn't work.
Wouldn't SET_SYSTEM_SLEEP_PM_OPS and SET_RUNTIME_PM_OPS help with that?
> Could get rid of BYT_PM_OPS and put this instead:
>
> #ifdef CONFIG_PM_SLEEP
> .resume = byt_resume,
> #endif
> #ifdef CONFIG_PM
> .runtime_resume = byt_runtime_resume,
> #endif
That's better!
[...]
Kind regards
Uffe
