Re: [PATCH] mmc: sdhci-pci: Fix BYT OCP setting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Memonry Technology]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux