Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 30 Sep 2019 14:05:22 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Tuowen Zhao <ztuowen@gmail.com>,
        "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc:     lee.jones@linaro.org, linux-kernel@vger.kernel.org,
        mika.westerberg@linux.intel.com, acelan.kao@canonical.com,
        bhelgaas@google.com, kai.heng.feng@canonical.com
Subject: Re: [PATCH] mfd: intel-lpss: use devm_ioremap_uc for mmio
Message-ID: <20190930110522.GT32742@smile.fi.intel.com>
References: <20190927175513.31054-1-ztuowen@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190927175513.31054-1-ztuowen@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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