Re: [PATCH] mmc: sdhci-pci: Remove D3 delays for Intel BYT-related host controllers

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

 



On Mon, Oct 09, 2017 at 08:01:06PM +0000, Hunter, Adrian wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:helgaas@xxxxxxxxxx]
> > Sent: Monday, October 9, 2017 4:42 PM
> > To: Hunter, Adrian <adrian.hunter@xxxxxxxxx>
> > Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>; linux-mmc <linux-
> > mmc@xxxxxxxxxxxxxxx>; linux-pci <linux-pci@xxxxxxxxxxxxxxx>; Bjorn Helgaas
> > <bhelgaas@xxxxxxxxxx>; linux-acpi <linux-acpi@xxxxxxxxxxxxxxx>; Rafael J.
> > Wysocki <rjw@xxxxxxxxxxxxx>; Alan Cox <alan@xxxxxxxxxxxxxxx>
> > Subject: Re: [PATCH] mmc: sdhci-pci: Remove D3 delays for Intel BYT-related
> > host controllers
> > 
> > [+cc Alan]
> > 
> > On Mon, Oct 09, 2017 at 11:56:43AM +0300, Adrian Hunter wrote:
> > > On 06/10/17 19:20, Bjorn Helgaas wrote:
> > > > On Fri, Oct 06, 2017 at 03:50:59PM +0300, Adrian Hunter wrote:
> > > >> The default D3 cold delay of 100 ms can cause pauses when streaming
> > > >> audio from eMMC.
> > > >>
> > > >> Intel BYT-related host controllers do not need PCI D3 delays.
> > > >> Although the delays can be set to zero via an ACPI _DSM,
> > > >> unfortunately that is not being used in all cases. So just set the delays to
> > zero in the driver.
> > > >>
> > > >> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> > > >> ---
> > > >>  drivers/mmc/host/sdhci-pci-core.c | 17 +++++++++++++----
> > > >>  1 file changed, 13 insertions(+), 4 deletions(-)
> > > >>
> > > >> diff --git a/drivers/mmc/host/sdhci-pci-core.c
> > > >> b/drivers/mmc/host/sdhci-pci-core.c
> > > >> index 5f3f7b51299f..14ef99b6e5b7 100644
> > > >> --- a/drivers/mmc/host/sdhci-pci-core.c
> > > >> +++ b/drivers/mmc/host/sdhci-pci-core.c
> > > >> @@ -592,9 +592,18 @@ static void byt_read_dsm(struct sdhci_pci_slot
> > *slot)
> > > >>  	slot->chip->rpm_retune = intel_host->d3_retune;  }
> > > >>
> > > >> -static int byt_emmc_probe_slot(struct sdhci_pci_slot *slot)
> > > >> +static void byt_common_setup(struct sdhci_pci_slot *slot)
> > > >>  {
> > > >>  	byt_read_dsm(slot);
> > > >> +
> > > >> +	/* PCI D3 delays are not needed */
> > > >> +	slot->chip->pdev->d3_delay = 0;
> > > >> +	slot->chip->pdev->d3cold_delay = 0;
> > > >
> > > > The fact that it doesn't need D3 delays sounds like a property of
> > > > the device itself, regardless of which (if any) driver claims it.
> > > >
> > > > Can this be done as a quirk instead, maybe something in
> > > > arch/x86/pci/fixup.c?  Maybe quirk_remove_d3_delay() could be used
> > > > directly, or something like pci_d3_delay_fixup()?
> > >
> > > A quirk would work, but that would mean setting the quirk for each
> > > device
> > > (28 so far) and then, when we have new ids, adding them there and to
> > > the driver, so it is more convenient just to do it in the driver.
> > > Certainly, this is the only driver we have for these Intel SDHCI PCI host
> > controllers.
> > 
> > pci_d3_delay_fixup() has code that mitigates the problem of adding new IDs.
> > Maybe that quirk should simply be moved to arch/x86/pci/fixup.c?
> 
> pci_d3_delay_fixup() is not setting zero.  quirk_remove_d3_delay() and creating
> a quirk_remove_d3cold_delay() would work.

Your concern was that a quirk would require a long list of device IDs
and we'd have to add new ones.  I share that concern, and my point was
that pci_d3_delay_fixup() checks for the platform type and sets the
delay based on that, so it doesn't have a long list of device IDs.  

Whether the appropriate delay is zero or 3 is a question for you Intel
guys to sort out.  We already have:

  pci_d3delay_fixup()     # arch/x86/pci/intel_mid_pci.c
  pci_d3_delay_fixup()    # drivers/staging/media/atomisp/platform/intel-mid/intel_mid_pcihelpers.c
  atomisp_pci_probe()     # drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c

and you're proposing to add a fourth one.  I think there's a lot of
overlap there, and it'd be nice to clean that up.

> > If we put this in the driver, won't we have the unnecessary delays on
> > systems where the driver isn't in use?
> 
> The issue is unacceptable I/O latency (streaming audio through the device
> was the example) which means you have to have a driver.

If I don't use audio, I don't need the driver.  My concern is that
even without the driver, there may be paths that use
pci_dev_d3_sleep() for this device, e.g., suspend/resume of the whole
system.  If that's the case, we should avoid the default 10ms delay
even if there's no driver loaded.

Maybe we never use pci_dev_d3_sleep() if there's no driver, but I
haven't been able to convince myself of that yet.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux