| Message ID | 1414485440-29673-1-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote: > static int sdhci_acpi_enable_dma(struct sdhci_host *host) > { > - return 0; > + struct sdhci_acpi_host *c = sdhci_priv(host); > + struct device *dev = &c->pdev->dev; > + int err = -1; > + > + if (c->dma_setup) > + return 0; > + > + if (host->flags & SDHCI_USE_64_BIT_DMA) { > + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { > + host->flags &= ~SDHCI_USE_64_BIT_DMA; > + } else { > + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); > + if (err) > + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); > + } > + } > + > + if (err) > + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); > + > + c->dma_setup = !err; > + > + return err; > } > I don't think it's worth a dev_warn() message (maybe dev_info), there is nothing wrong in this case, but you probably have to clear the SDHCI_USE_64_BIT_DMA to ensure that the driver won't try to to use the 64-bit DMA if the mask is not set. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/10/14 11:43, Arnd Bergmann wrote: > On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote: >> static int sdhci_acpi_enable_dma(struct sdhci_host *host) >> { >> - return 0; >> + struct sdhci_acpi_host *c = sdhci_priv(host); >> + struct device *dev = &c->pdev->dev; >> + int err = -1; >> + >> + if (c->dma_setup) >> + return 0; >> + >> + if (host->flags & SDHCI_USE_64_BIT_DMA) { >> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { >> + host->flags &= ~SDHCI_USE_64_BIT_DMA; >> + } else { >> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); >> + if (err) >> + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); >> + } >> + } >> + >> + if (err) >> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); >> + >> + c->dma_setup = !err; >> + >> + return err; >> } >> > > I don't think it's worth a dev_warn() message (maybe dev_info), there is nothing It is worth a dev_warn because 32-bit DMA can allocate memory for bounce buffers which jeopardizes memory reclaim. > wrong in this case, but you probably have to clear the SDHCI_USE_64_BIT_DMA to > ensure that the driver won't try to to use the 64-bit DMA if the mask is not set. 64-bit DMA will work with a 32-bit DMA mask so there is no need. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote: > On 28/10/14 11:43, Arnd Bergmann wrote: > > On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote: > >> static int sdhci_acpi_enable_dma(struct sdhci_host *host) > >> { > >> - return 0; > >> + struct sdhci_acpi_host *c = sdhci_priv(host); > >> + struct device *dev = &c->pdev->dev; > >> + int err = -1; > >> + > >> + if (c->dma_setup) > >> + return 0; > >> + > >> + if (host->flags & SDHCI_USE_64_BIT_DMA) { > >> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { > >> + host->flags &= ~SDHCI_USE_64_BIT_DMA; > >> + } else { > >> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); > >> + if (err) > >> + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); > >> + } > >> + } > >> + > >> + if (err) > >> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); > >> + > >> + c->dma_setup = !err; > >> + > >> + return err; > >> } > >> > > > > I don't think it's worth a dev_warn() message (maybe dev_info), there is nothing > > It is worth a dev_warn because 32-bit DMA can allocate memory for bounce > buffers which jeopardizes memory reclaim. Then you should also warn if SDHCI_USE_64_BIT_DMA isn't or if SDHCI_QUIRK2_BROKEN_64_BIT_DMA is set I guess. > > wrong in this case, but you probably have to clear the SDHCI_USE_64_BIT_DMA to > > ensure that the driver won't try to to use the 64-bit DMA if the mask is not set. > > 64-bit DMA will work with a 32-bit DMA mask so there is no need. Wouldn't that at least be (slightly) less efficient? You end up having to do two expensive MMIO register accesses each time you transfer an address. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/10/14 12:18, Arnd Bergmann wrote: > On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote: >> On 28/10/14 11:43, Arnd Bergmann wrote: >>> On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote: >>>> static int sdhci_acpi_enable_dma(struct sdhci_host *host) >>>> { >>>> - return 0; >>>> + struct sdhci_acpi_host *c = sdhci_priv(host); >>>> + struct device *dev = &c->pdev->dev; >>>> + int err = -1; >>>> + >>>> + if (c->dma_setup) >>>> + return 0; >>>> + >>>> + if (host->flags & SDHCI_USE_64_BIT_DMA) { >>>> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { >>>> + host->flags &= ~SDHCI_USE_64_BIT_DMA; >>>> + } else { >>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); >>>> + if (err) >>>> + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); >>>> + } >>>> + } >>>> + >>>> + if (err) >>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); >>>> + >>>> + c->dma_setup = !err; >>>> + >>>> + return err; >>>> } >>>> >>> >>> I don't think it's worth a dev_warn() message (maybe dev_info), there is nothing >> >> It is worth a dev_warn because 32-bit DMA can allocate memory for bounce >> buffers which jeopardizes memory reclaim. > > Then you should also warn if SDHCI_USE_64_BIT_DMA isn't or if > SDHCI_QUIRK2_BROKEN_64_BIT_DMA is set I guess. The warning is for when the controller supports 64-bit, not when it doesn't. > >>> wrong in this case, but you probably have to clear the SDHCI_USE_64_BIT_DMA to >>> ensure that the driver won't try to to use the 64-bit DMA if the mask is not set. >> >> 64-bit DMA will work with a 32-bit DMA mask so there is no need. > > Wouldn't that at least be (slightly) less efficient? You end up having to do > two expensive MMIO register accesses each time you transfer an address. I doubt it is measureable and I don't want to optimize a degenerate case anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote: > On 28/10/14 12:18, Arnd Bergmann wrote: > > On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote: > >> On 28/10/14 11:43, Arnd Bergmann wrote: > >>> On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote: > >>>> static int sdhci_acpi_enable_dma(struct sdhci_host *host) > >>>> { > >>>> - return 0; > >>>> + struct sdhci_acpi_host *c = sdhci_priv(host); > >>>> + struct device *dev = &c->pdev->dev; > >>>> + int err = -1; > >>>> + > >>>> + if (c->dma_setup) > >>>> + return 0; > >>>> + > >>>> + if (host->flags & SDHCI_USE_64_BIT_DMA) { > >>>> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { > >>>> + host->flags &= ~SDHCI_USE_64_BIT_DMA; > >>>> + } else { > >>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); > >>>> + if (err) > >>>> + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); > >>>> + } > >>>> + } > >>>> + > >>>> + if (err) > >>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); > >>>> + > >>>> + c->dma_setup = !err; > >>>> + > >>>> + return err; > >>>> } > >>>> > >>> > >>> I don't think it's worth a dev_warn() message (maybe dev_info), there is nothing > >> > >> It is worth a dev_warn because 32-bit DMA can allocate memory for bounce > >> buffers which jeopardizes memory reclaim. > > > > Then you should also warn if SDHCI_USE_64_BIT_DMA isn't or if > > SDHCI_QUIRK2_BROKEN_64_BIT_DMA is set I guess. > > The warning is for when the controller supports 64-bit, not when it doesn't. But why warn about a feature of the controller being present? You just said it's a problem for memory reclaim if 64-bit DMA is not supported. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/10/2014 3:54 p.m., Arnd Bergmann wrote: > On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote: >> On 28/10/14 12:18, Arnd Bergmann wrote: >>> On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote: >>>> On 28/10/14 11:43, Arnd Bergmann wrote: >>>>> On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote: >>>>>> static int sdhci_acpi_enable_dma(struct sdhci_host *host) >>>>>> { >>>>>> - return 0; >>>>>> + struct sdhci_acpi_host *c = sdhci_priv(host); >>>>>> + struct device *dev = &c->pdev->dev; >>>>>> + int err = -1; >>>>>> + >>>>>> + if (c->dma_setup) >>>>>> + return 0; >>>>>> + >>>>>> + if (host->flags & SDHCI_USE_64_BIT_DMA) { >>>>>> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { >>>>>> + host->flags &= ~SDHCI_USE_64_BIT_DMA; >>>>>> + } else { >>>>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); >>>>>> + if (err) >>>>>> + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + if (err) >>>>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); >>>>>> + >>>>>> + c->dma_setup = !err; >>>>>> + >>>>>> + return err; >>>>>> } >>>>>> >>>>> >>>>> I don't think it's worth a dev_warn() message (maybe dev_info), there is nothing >>>> >>>> It is worth a dev_warn because 32-bit DMA can allocate memory for bounce >>>> buffers which jeopardizes memory reclaim. >>> >>> Then you should also warn if SDHCI_USE_64_BIT_DMA isn't or if >>> SDHCI_QUIRK2_BROKEN_64_BIT_DMA is set I guess. >> >> The warning is for when the controller supports 64-bit, not when it doesn't. > > But why warn about a feature of the controller being present? You just > said it's a problem for memory reclaim if 64-bit DMA is not supported. The warning is for when the controller supports 64-bit but it can't get a 64-bit DMA mask, and might therefore need to bounce things. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 28 October 2014 16:14:46 Adrian Hunter wrote: > On 28/10/2014 3:54 p.m., Arnd Bergmann wrote: > > On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote: > >> On 28/10/14 12:18, Arnd Bergmann wrote: > >>> On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote: > >>>> On 28/10/14 11:43, Arnd Bergmann wrote: > >>>>> On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote: > >>>>>> static int sdhci_acpi_enable_dma(struct sdhci_host *host) > >>>>>> { > >>>>>> - return 0; > >>>>>> + struct sdhci_acpi_host *c = sdhci_priv(host); > >>>>>> + struct device *dev = &c->pdev->dev; > >>>>>> + int err = -1; > >>>>>> + > >>>>>> + if (c->dma_setup) > >>>>>> + return 0; > >>>>>> + > >>>>>> + if (host->flags & SDHCI_USE_64_BIT_DMA) { > >>>>>> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { > >>>>>> + host->flags &= ~SDHCI_USE_64_BIT_DMA; > >>>>>> + } else { > >>>>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); > >>>>>> + if (err) > >>>>>> + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); > >>>>>> + } > >>>>>> + } > >>>>>> + > >>>>>> + if (err) > >>>>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); > >>>>>> + > >>>>>> + c->dma_setup = !err; > >>>>>> + > >>>>>> + return err; > >>>>>> } > >>>>>> > >>>>> > >>>>> I don't think it's worth a dev_warn() message (maybe dev_info), there is nothing > >>>> > >>>> It is worth a dev_warn because 32-bit DMA can allocate memory for bounce > >>>> buffers which jeopardizes memory reclaim. > >>> > >>> Then you should also warn if SDHCI_USE_64_BIT_DMA isn't or if > >>> SDHCI_QUIRK2_BROKEN_64_BIT_DMA is set I guess. > >> > >> The warning is for when the controller supports 64-bit, not when it doesn't. > > > > But why warn about a feature of the controller being present? You just > > said it's a problem for memory reclaim if 64-bit DMA is not supported. > > The warning is for when the controller supports 64-bit but it can't > get a 64-bit DMA mask, and might therefore need to bounce things. What does "can't get a 64-bit DMA mask" mean? This is just a different way to say it doesn't support 64-bit for some reason. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/10/2014 5:08 p.m., Arnd Bergmann wrote: > On Tuesday 28 October 2014 16:14:46 Adrian Hunter wrote: >> On 28/10/2014 3:54 p.m., Arnd Bergmann wrote: >>> On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote: >>>> On 28/10/14 12:18, Arnd Bergmann wrote: >>>>> On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote: >>>>>> On 28/10/14 11:43, Arnd Bergmann wrote: >>>>>>> On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote: >>>>>>>> static int sdhci_acpi_enable_dma(struct sdhci_host *host) >>>>>>>> { >>>>>>>> - return 0; >>>>>>>> + struct sdhci_acpi_host *c = sdhci_priv(host); >>>>>>>> + struct device *dev = &c->pdev->dev; >>>>>>>> + int err = -1; >>>>>>>> + >>>>>>>> + if (c->dma_setup) >>>>>>>> + return 0; >>>>>>>> + >>>>>>>> + if (host->flags & SDHCI_USE_64_BIT_DMA) { >>>>>>>> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { >>>>>>>> + host->flags &= ~SDHCI_USE_64_BIT_DMA; >>>>>>>> + } else { >>>>>>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); >>>>>>>> + if (err) >>>>>>>> + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); >>>>>>>> + } >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (err) >>>>>>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); >>>>>>>> + >>>>>>>> + c->dma_setup = !err; >>>>>>>> + >>>>>>>> + return err; >>>>>>>> } >>>>>>>> >>>>>>> >>>>>>> I don't think it's worth a dev_warn() message (maybe dev_info), there is nothing >>>>>> >>>>>> It is worth a dev_warn because 32-bit DMA can allocate memory for bounce >>>>>> buffers which jeopardizes memory reclaim. >>>>> >>>>> Then you should also warn if SDHCI_USE_64_BIT_DMA isn't or if >>>>> SDHCI_QUIRK2_BROKEN_64_BIT_DMA is set I guess. >>>> >>>> The warning is for when the controller supports 64-bit, not when it doesn't. >>> >>> But why warn about a feature of the controller being present? You just >>> said it's a problem for memory reclaim if 64-bit DMA is not supported. >> >> The warning is for when the controller supports 64-bit but it can't >> get a 64-bit DMA mask, and might therefore need to bounce things. > > What does "can't get a 64-bit DMA mask" mean? This is just a different > way to say it doesn't support 64-bit for some reason. The host controller advertises whether it is capable of 64-bit DMA. If it is 64-bit capable but the driver cannot get a 64-bit DMA mask it issues a warning. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 28 October 2014 17:14:25 Adrian Hunter wrote: > On 28/10/2014 5:08 p.m., Arnd Bergmann wrote: > > On Tuesday 28 October 2014 16:14:46 Adrian Hunter wrote: > >> On 28/10/2014 3:54 p.m., Arnd Bergmann wrote: > >>> On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote: > >>>> On 28/10/14 12:18, Arnd Bergmann wrote: > >>>>> On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote: > >>>>>> On 28/10/14 11:43, Arnd Bergmann wrote: > >>>>>>> On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote: > >>>>>>>> static int sdhci_acpi_enable_dma(struct sdhci_host *host) > >>>>>>>> { > >>>>>>>> - return 0; > >>>>>>>> + struct sdhci_acpi_host *c = sdhci_priv(host); > >>>>>>>> + struct device *dev = &c->pdev->dev; > >>>>>>>> + int err = -1; > >>>>>>>> + > >>>>>>>> + if (c->dma_setup) > >>>>>>>> + return 0; > >>>>>>>> + > >>>>>>>> + if (host->flags & SDHCI_USE_64_BIT_DMA) { > >>>>>>>> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { > >>>>>>>> + host->flags &= ~SDHCI_USE_64_BIT_DMA; > >>>>>>>> + } else { > >>>>>>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); > >>>>>>>> + if (err) > >>>>>>>> + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); > >>>>>>>> + } > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> + if (err) > >>>>>>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); > >>>>>>>> + > >>>>>>>> + c->dma_setup = !err; > >>>>>>>> + > >>>>>>>> + return err; > >>>>>>>> } > >>>>>>>> > >>>>>>> > >>>>>>> I don't think it's worth a dev_warn() message (maybe dev_info), there is nothing > >>>>>> > >>>>>> It is worth a dev_warn because 32-bit DMA can allocate memory for bounce > >>>>>> buffers which jeopardizes memory reclaim. > >>>>> > >>>>> Then you should also warn if SDHCI_USE_64_BIT_DMA isn't or if > >>>>> SDHCI_QUIRK2_BROKEN_64_BIT_DMA is set I guess. > >>>> > >>>> The warning is for when the controller supports 64-bit, not when it doesn't. > >>> > >>> But why warn about a feature of the controller being present? You just > >>> said it's a problem for memory reclaim if 64-bit DMA is not supported. > >> > >> The warning is for when the controller supports 64-bit but it can't > >> get a 64-bit DMA mask, and might therefore need to bounce things. > > > > What does "can't get a 64-bit DMA mask" mean? This is just a different > > way to say it doesn't support 64-bit for some reason. > > The host controller advertises whether it is capable of 64-bit DMA. If > it is 64-bit capable but the driver cannot get a 64-bit DMA mask it issues > a warning. But the host controller doesn't know if it's 64-bit capable, nor should it know. All the host controller knows is that it has registers to perform 64-bit DMA, but the platform code (ACPI in your case) is the only thing that knows how many of those address lines are connected to an upstream bus. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/10/14 17:38, Arnd Bergmann wrote: > On Tuesday 28 October 2014 17:14:25 Adrian Hunter wrote: >> On 28/10/2014 5:08 p.m., Arnd Bergmann wrote: >>> On Tuesday 28 October 2014 16:14:46 Adrian Hunter wrote: >>>> On 28/10/2014 3:54 p.m., Arnd Bergmann wrote: >>>>> On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote: >>>>>> On 28/10/14 12:18, Arnd Bergmann wrote: >>>>>>> On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote: >>>>>>>> On 28/10/14 11:43, Arnd Bergmann wrote: >>>>>>>>> On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote: >>>>>>>>>> static int sdhci_acpi_enable_dma(struct sdhci_host *host) >>>>>>>>>> { >>>>>>>>>> - return 0; >>>>>>>>>> + struct sdhci_acpi_host *c = sdhci_priv(host); >>>>>>>>>> + struct device *dev = &c->pdev->dev; >>>>>>>>>> + int err = -1; >>>>>>>>>> + >>>>>>>>>> + if (c->dma_setup) >>>>>>>>>> + return 0; >>>>>>>>>> + >>>>>>>>>> + if (host->flags & SDHCI_USE_64_BIT_DMA) { >>>>>>>>>> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { >>>>>>>>>> + host->flags &= ~SDHCI_USE_64_BIT_DMA; >>>>>>>>>> + } else { >>>>>>>>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); >>>>>>>>>> + if (err) >>>>>>>>>> + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); >>>>>>>>>> + } >>>>>>>>>> + } >>>>>>>>>> + >>>>>>>>>> + if (err) >>>>>>>>>> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); >>>>>>>>>> + >>>>>>>>>> + c->dma_setup = !err; >>>>>>>>>> + >>>>>>>>>> + return err; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>> >>>>>>>>> I don't think it's worth a dev_warn() message (maybe dev_info), there is nothing >>>>>>>> >>>>>>>> It is worth a dev_warn because 32-bit DMA can allocate memory for bounce >>>>>>>> buffers which jeopardizes memory reclaim. >>>>>>> >>>>>>> Then you should also warn if SDHCI_USE_64_BIT_DMA isn't or if >>>>>>> SDHCI_QUIRK2_BROKEN_64_BIT_DMA is set I guess. >>>>>> >>>>>> The warning is for when the controller supports 64-bit, not when it doesn't. >>>>> >>>>> But why warn about a feature of the controller being present? You just >>>>> said it's a problem for memory reclaim if 64-bit DMA is not supported. >>>> >>>> The warning is for when the controller supports 64-bit but it can't >>>> get a 64-bit DMA mask, and might therefore need to bounce things. >>> >>> What does "can't get a 64-bit DMA mask" mean? This is just a different >>> way to say it doesn't support 64-bit for some reason. >> >> The host controller advertises whether it is capable of 64-bit DMA. If >> it is 64-bit capable but the driver cannot get a 64-bit DMA mask it issues >> a warning. > > But the host controller doesn't know if it's 64-bit capable, nor should > it know. All the host controller knows is that it has registers to perform > 64-bit DMA, but the platform code (ACPI in your case) is the only thing that > knows how many of those address lines are connected to an upstream bus. No, the SDHCI spec. says: 64-bit System Bus Support Setting 1 to this bit indicates that the Host Controller supports 64-bit address descriptor mode and is connected to 64-bit address system bus. So the expectation is that it is on a 64-bit bus. The warning is reasonable. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c index 9cccc0e..3109763 100644 --- a/drivers/mmc/host/sdhci-acpi.c +++ b/drivers/mmc/host/sdhci-acpi.c @@ -76,6 +76,7 @@ struct sdhci_acpi_host { const struct sdhci_acpi_slot *slot; struct platform_device *pdev; bool use_runtime_pm; + bool dma_setup; }; static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag) @@ -85,7 +86,29 @@ static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag) static int sdhci_acpi_enable_dma(struct sdhci_host *host) { - return 0; + struct sdhci_acpi_host *c = sdhci_priv(host); + struct device *dev = &c->pdev->dev; + int err = -1; + + if (c->dma_setup) + return 0; + + if (host->flags & SDHCI_USE_64_BIT_DMA) { + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { + host->flags &= ~SDHCI_USE_64_BIT_DMA; + } else { + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); + if (err) + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); + } + } + + if (err) + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); + + c->dma_setup = !err; + + return err; } static void sdhci_acpi_int_hw_reset(struct sdhci_host *host) @@ -305,21 +328,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev) goto err_free; } - if (!dev->dma_mask) { - u64 dma_mask; - - if (sdhci_readl(host, SDHCI_CAPABILITIES) & SDHCI_CAN_64BIT) { - /* 64-bit DMA is not supported at present */ - dma_mask = DMA_BIT_MASK(32); - } else { - dma_mask = DMA_BIT_MASK(32); - } - - err = dma_coerce_mask_and_coherent(dev, dma_mask); - if (err) - goto err_free; - } - if (c->slot) { if (c->slot->probe_slot) { err = c->slot->probe_slot(pdev, hid, uid);
Set the DMA mask during the first call to ->enable_dma() to make use of the SDHCI_USE_64_BIT_DMA flag. This patch is dependent on commit 8a2f38ddfeb526c30b3ec209468172a30a38d996 ("ACPI / platform: provide default DMA mask") which provides the dev->dma_mask pointer without which dma_set_mask_and_coherent() will always fail. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/mmc/host/sdhci-acpi.c | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-)