[PATCH] mfd: intel-lpss: use devm_ioremap_uc for mmio

[PATCH] mfd: intel-lpss: use devm_ioremap_uc for mmio

[Tuowen Zhao]

Write-combining BAR for intel-lpss-pci in MTRR causes system hangs
during boot.

This patch adds devm_ioremap_uc as a new managed wrapper to ioremap_uc
and with it forces the use of strongly uncachable mmio in intel-lpss.

This bahavior is seen on Dell XPS 13 7390 2-in-1:

[    0.001734]   5 base 4000000000 mask 6000000000 write-combining

4000000000-7fffffffff : PCI Bus 0000:00
  4000000000-400fffffff : 0000:00:02.0 (i915)
  4010000000-4010000fff : 0000:00:15.0 (intel-lpss-pci)

Link: https://bugzilla.kernel.org/show_bug.cgi?id=203485
Signed-off-by: Tuowen Zhao <ztuowen@gmail.com>
---
 drivers/mfd/intel-lpss.c |  2 +-
 include/linux/io.h       |  2 ++
 lib/devres.c             | 19 +++++++++++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
index 277f48f1cc1c..06106c9320bb 100644
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c
@@ -395,7 +395,7 @@ int intel_lpss_probe(struct device *dev,
 	if (!lpss)
 		return -ENOMEM;
 
-	lpss->priv = devm_ioremap(dev, info->mem->start + LPSS_PRIV_OFFSET,
+	lpss->priv = devm_ioremap_uc(dev, info->mem->start + LPSS_PRIV_OFFSET,
 				  LPSS_PRIV_SIZE);
 	if (!lpss->priv)
 		return -ENOMEM;
diff --git a/include/linux/io.h b/include/linux/io.h
index accac822336a..a59834bc0a11 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -64,6 +64,8 @@ static inline void devm_ioport_unmap(struct device *dev, void __iomem *addr)
 
 void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
 			   resource_size_t size);
+void __iomem *devm_ioremap_uc(struct device *dev, resource_size_t offset,
+				   resource_size_t size);
 void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
 				   resource_size_t size);
 void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
diff --git a/lib/devres.c b/lib/devres.c
index 6a0e9bd6524a..beb0a064b891 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -9,6 +9,7 @@
 enum devm_ioremap_type {
 	DEVM_IOREMAP = 0,
 	DEVM_IOREMAP_NC,
+	DEVM_IOREMAP_UC,
 	DEVM_IOREMAP_WC,
 };
 
@@ -39,6 +40,9 @@ static void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset,
 	case DEVM_IOREMAP_NC:
 		addr = ioremap_nocache(offset, size);
 		break;
+	case DEVM_IOREMAP_UC:
+		addr = ioremap_uc(offset, size);
+		break;
 	case DEVM_IOREMAP_WC:
 		addr = ioremap_wc(offset, size);
 		break;
@@ -68,6 +72,21 @@ void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
 }
 EXPORT_SYMBOL(devm_ioremap);
 
+/**
+ * devm_ioremap_uc - Managed ioremap_uc()
+ * @dev: Generic device to remap IO address for
+ * @offset: Resource address to map
+ * @size: Size of map
+ *
+ * Managed ioremap_uc().  Map is automatically unmapped on driver detach.
+ */
+void __iomem *devm_ioremap_uc(struct device *dev, resource_size_t offset,
+			      resource_size_t size)
+{
+	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_UC);
+}
+EXPORT_SYMBOL(devm_ioremap_uc);
+
 /**
  * devm_ioremap_nocache - Managed ioremap_nocache()
  * @dev: Generic device to remap IO address for
-- 
2.23.0

Re: [PATCH] mfd: intel-lpss: use devm_ioremap_uc for mmio

[Mika Westerberg]

Hi,

In $subject, use MMIO instead of mmio.

On Fri, Sep 27, 2019 at 11:55:13AM -0600, Tuowen Zhao wrote:
> Write-combining BAR for intel-lpss-pci in MTRR causes system hangs
> during boot.
> 
> This patch adds devm_ioremap_uc as a new managed wrapper to ioremap_uc
> and with it forces the use of strongly uncachable mmio in intel-lpss.
> 
> This bahavior is seen on Dell XPS 13 7390 2-in-1:
> 
> [    0.001734]   5 base 4000000000 mask 6000000000 write-combining

I think it is worth mentioning that this is actually a BIOS bug and
fixed by some vendors via BIOS upgrade.

> 4000000000-7fffffffff : PCI Bus 0000:00
>   4000000000-400fffffff : 0000:00:02.0 (i915)
>   4010000000-4010000fff : 0000:00:15.0 (intel-lpss-pci)
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=203485
> Signed-off-by: Tuowen Zhao <ztuowen@gmail.com>
> ---
>  drivers/mfd/intel-lpss.c |  2 +-
>  include/linux/io.h       |  2 ++
>  lib/devres.c             | 19 +++++++++++++++++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> index 277f48f1cc1c..06106c9320bb 100644
> --- a/drivers/mfd/intel-lpss.c
> +++ b/drivers/mfd/intel-lpss.c
> @@ -395,7 +395,7 @@ int intel_lpss_probe(struct device *dev,
>  	if (!lpss)
>  		return -ENOMEM;
>  
> -	lpss->priv = devm_ioremap(dev, info->mem->start + LPSS_PRIV_OFFSET,
> +	lpss->priv = devm_ioremap_uc(dev, info->mem->start + LPSS_PRIV_OFFSET,
>  				  LPSS_PRIV_SIZE);

Can you add a comment here explaining why this particular driver needs
to use _uc version? Normally drivers call the plain devm_ioremap() and
expect to get non-cached memory, however ioremap() (which resolves to
ioremap_nocache()) does not touch x86 PAT configuration which is the
reason we need to call _uc variant here.

Otherwise looks good to me.

Re: [PATCH] mfd: intel-lpss: use devm_ioremap_uc for mmio

[Andy Shevchenko]

On Fri, Sep 27, 2019 at 11:55:13AM -0600, Tuowen Zhao wrote:
> Write-combining BAR for intel-lpss-pci in MTRR causes system hangs
> during boot.
> 
> This patch adds devm_ioremap_uc as a new managed wrapper to ioremap_uc
> and with it forces the use of strongly uncachable mmio in intel-lpss.
> 
> This bahavior is seen on Dell XPS 13 7390 2-in-1:
> 
> [    0.001734]   5 base 4000000000 mask 6000000000 write-combining
> 
> 4000000000-7fffffffff : PCI Bus 0000:00
>   4000000000-400fffffff : 0000:00:02.0 (i915)
>   4010000000-4010000fff : 0000:00:15.0 (intel-lpss-pci)

+Cc: Luis as author of UC flavour of ioremap.

Luis, some BIOSes in the wild have wrong MTRR setting for PCI resource window
and thus when Linux tries to allocate 64-bit MMIO address space (and in
opposite to Windows, which does this from the end of available space towards
beginning, Linux do this from the beginning towards end). Ideally we have to
push vendors to fix firmware.

This patch AFAIU overrides MTTR/PAT settings for those pages and makes it
possible to workaround firmware bug.

What do you think is the best approach here?

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=203485
> Signed-off-by: Tuowen Zhao <ztuowen@gmail.com>
> ---
>  drivers/mfd/intel-lpss.c |  2 +-
>  include/linux/io.h       |  2 ++
>  lib/devres.c             | 19 +++++++++++++++++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> index 277f48f1cc1c..06106c9320bb 100644
> --- a/drivers/mfd/intel-lpss.c
> +++ b/drivers/mfd/intel-lpss.c
> @@ -395,7 +395,7 @@ int intel_lpss_probe(struct device *dev,
>  	if (!lpss)
>  		return -ENOMEM;
>  
> -	lpss->priv = devm_ioremap(dev, info->mem->start + LPSS_PRIV_OFFSET,
> +	lpss->priv = devm_ioremap_uc(dev, info->mem->start + LPSS_PRIV_OFFSET,
>  				  LPSS_PRIV_SIZE);
>  	if (!lpss->priv)
>  		return -ENOMEM;
> diff --git a/include/linux/io.h b/include/linux/io.h
> index accac822336a..a59834bc0a11 100644
> --- a/include/linux/io.h
> +++ b/include/linux/io.h
> @@ -64,6 +64,8 @@ static inline void devm_ioport_unmap(struct device *dev, void __iomem *addr)
>  
>  void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
>  			   resource_size_t size);
> +void __iomem *devm_ioremap_uc(struct device *dev, resource_size_t offset,
> +				   resource_size_t size);
>  void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
>  				   resource_size_t size);
>  void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
> diff --git a/lib/devres.c b/lib/devres.c
> index 6a0e9bd6524a..beb0a064b891 100644
> --- a/lib/devres.c
> +++ b/lib/devres.c
> @@ -9,6 +9,7 @@
>  enum devm_ioremap_type {
>  	DEVM_IOREMAP = 0,
>  	DEVM_IOREMAP_NC,
> +	DEVM_IOREMAP_UC,
>  	DEVM_IOREMAP_WC,
>  };
>  
> @@ -39,6 +40,9 @@ static void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset,
>  	case DEVM_IOREMAP_NC:
>  		addr = ioremap_nocache(offset, size);
>  		break;
> +	case DEVM_IOREMAP_UC:
> +		addr = ioremap_uc(offset, size);
> +		break;
>  	case DEVM_IOREMAP_WC:
>  		addr = ioremap_wc(offset, size);
>  		break;
> @@ -68,6 +72,21 @@ void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
>  }
>  EXPORT_SYMBOL(devm_ioremap);
>  
> +/**
> + * devm_ioremap_uc - Managed ioremap_uc()
> + * @dev: Generic device to remap IO address for
> + * @offset: Resource address to map
> + * @size: Size of map
> + *
> + * Managed ioremap_uc().  Map is automatically unmapped on driver detach.
> + */
> +void __iomem *devm_ioremap_uc(struct device *dev, resource_size_t offset,
> +			      resource_size_t size)
> +{
> +	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_UC);
> +}
> +EXPORT_SYMBOL(devm_ioremap_uc);
> +
>  /**
>   * devm_ioremap_nocache - Managed ioremap_nocache()
>   * @dev: Generic device to remap IO address for
> -- 
> 2.23.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mfd: intel-lpss: use devm_ioremap_uc for mmio

[Andy Shevchenko]

On Mon, Sep 30, 2019 at 02:05:22PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 27, 2019 at 11:55:13AM -0600, Tuowen Zhao wrote:
> > Write-combining BAR for intel-lpss-pci in MTRR causes system hangs
> > during boot.
> > 
> > This patch adds devm_ioremap_uc as a new managed wrapper to ioremap_uc
> > and with it forces the use of strongly uncachable mmio in intel-lpss.
> > 
> > This bahavior is seen on Dell XPS 13 7390 2-in-1:
> > 
> > [    0.001734]   5 base 4000000000 mask 6000000000 write-combining
> > 
> > 4000000000-7fffffffff : PCI Bus 0000:00
> >   4000000000-400fffffff : 0000:00:02.0 (i915)
> >   4010000000-4010000fff : 0000:00:15.0 (intel-lpss-pci)
> 
> +Cc: Luis as author of UC flavour of ioremap.
> 
> Luis, some BIOSes in the wild have wrong MTRR setting for PCI resource window
> and thus when Linux tries to allocate 64-bit MMIO address space (and in
> opposite to Windows, which does this from the end of available space towards
> beginning, Linux do this from the beginning towards end). Ideally we have to
> push vendors to fix firmware.
> 
> This patch AFAIU overrides MTTR/PAT settings for those pages and makes it
> possible to workaround firmware bug.
> 
> What do you think is the best approach here?

Tuowen,

since Luis didn't respond, I think we may proceed with v2 after addressing
Mika's comments.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mfd: intel-lpss: use devm_ioremap_uc for mmio

[Luis Chamberlain]

On Mon, Sep 30, 2019 at 02:05:22PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 27, 2019 at 11:55:13AM -0600, Tuowen Zhao wrote:
> > Write-combining BAR for intel-lpss-pci in MTRR causes system hangs
> > during boot.
> > 
> > This patch adds devm_ioremap_uc as a new managed wrapper to ioremap_uc
> > and with it forces the use of strongly uncachable mmio in intel-lpss.
> > 
> > This bahavior is seen on Dell XPS 13 7390 2-in-1:
> > 
> > [    0.001734]   5 base 4000000000 mask 6000000000 write-combining
> > 
> > 4000000000-7fffffffff : PCI Bus 0000:00
> >   4000000000-400fffffff : 0000:00:02.0 (i915)
> >   4010000000-4010000fff : 0000:00:15.0 (intel-lpss-pci)
> 
> +Cc: Luis as author of UC flavour of ioremap.
> 
> Luis, some BIOSes in the wild have wrong MTRR setting for PCI resource window
> and thus when Linux tries to allocate 64-bit MMIO address space (and in
> opposite to Windows, which does this from the end of available space towards
> beginning, Linux do this from the beginning towards end). Ideally we have to
> push vendors to fix firmware.
> 
> This patch AFAIU overrides MTTR/PAT settings for those pages and makes it
> possible to workaround firmware bug.
> 
> What do you think is the best approach here?

Indeed, such cases can come up, and yes _uc can be a workaround for such
cases.

> > +EXPORT_SYMBOL(devm_ioremap_uc);

EXPORT_SYMBOL_GPL() would be my preference. But other than that, this
makes sense.

  Luis