[v1,5/7] mfd: lpc_ich: Switch to generic pci_p2sb_bar()

Message ID 20210308122020.57071-6-andriy.shevchenko@linux.intel.com
State New
Headers show
Series
  • PCI: introduce p2sb helper
Related show

Commit Message

Andy Shevchenko March 8, 2021, 12:20 p.m.
Instead of open coding pci_p2sb_bar() functionality we are going to
use generic library for that. There one more user of it is coming.

Besides cleaning up it fixes a potential issue if, by some reason,
SPI bar is 64-bit.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/Kconfig   |  1 +
 drivers/mfd/lpc_ich.c | 20 ++++++--------------
 2 files changed, 7 insertions(+), 14 deletions(-)

Comments

Lee Jones March 10, 2021, 10:35 a.m. | #1
On Mon, 08 Mar 2021, Andy Shevchenko wrote:

> Instead of open coding pci_p2sb_bar() functionality we are going to

> use generic library for that. There one more user of it is coming.

> 

> Besides cleaning up it fixes a potential issue if, by some reason,

> SPI bar is 64-bit.


Probably worth cleaning up the English in both these sections.

 Instead of open coding pci_p2sb_bar() functionality we are going to
 use generic library. There is one more user en route.

 This is more than just a clean-up.  It also fixes a potential issue
 seen when SPI bar is 64-bit.

Also worth briefly describing what that issue is I think.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---

>  drivers/mfd/Kconfig   |  1 +

>  drivers/mfd/lpc_ich.c | 20 ++++++--------------

>  2 files changed, 7 insertions(+), 14 deletions(-)


Code looks fine:

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Andy Shevchenko March 10, 2021, 12:05 p.m. | #2
On Wed, Mar 10, 2021 at 10:35:39AM +0000, Lee Jones wrote:
> On Mon, 08 Mar 2021, Andy Shevchenko wrote:

> 

> > Instead of open coding pci_p2sb_bar() functionality we are going to

> > use generic library for that. There one more user of it is coming.

> > 

> > Besides cleaning up it fixes a potential issue if, by some reason,

> > SPI bar is 64-bit.

> 

> Probably worth cleaning up the English in both these sections.

> 

>  Instead of open coding pci_p2sb_bar() functionality we are going to

>  use generic library. There is one more user en route.

> 

>  This is more than just a clean-up.  It also fixes a potential issue

>  seen when SPI bar is 64-bit.


Thanks!

> Also worth briefly describing what that issue is I think.


Current code works if and only if the PCI BAR of the hidden device is inside 4G
address space. In case firmware decides to go above 4G, we will get a wrong
address.

Does it sound good enough?

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> > ---

> >  drivers/mfd/Kconfig   |  1 +

> >  drivers/mfd/lpc_ich.c | 20 ++++++--------------

> >  2 files changed, 7 insertions(+), 14 deletions(-)

> 

> Code looks fine:

> 

> For my own reference (apply this as-is to your sign-off block):

> 

>   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>


Thanks for reviewing this series, can you have a look at the earlier sent [1]
and [2]? First one has a regression fix.

[1]: https://lore.kernel.org/lkml/20210302135620.89958-1-andriy.shevchenko@linux.intel.com/T/#u
[2]: https://lore.kernel.org/lkml/20210301144222.31290-1-andriy.shevchenko@linux.intel.com/T/#u

-- 
With Best Regards,
Andy Shevchenko
Lee Jones March 10, 2021, 12:57 p.m. | #3
On Wed, 10 Mar 2021, Andy Shevchenko wrote:

> On Wed, Mar 10, 2021 at 10:35:39AM +0000, Lee Jones wrote:

> > On Mon, 08 Mar 2021, Andy Shevchenko wrote:

> > 

> > > Instead of open coding pci_p2sb_bar() functionality we are going to

> > > use generic library for that. There one more user of it is coming.

> > > 

> > > Besides cleaning up it fixes a potential issue if, by some reason,

> > > SPI bar is 64-bit.

> > 

> > Probably worth cleaning up the English in both these sections.

> > 

> >  Instead of open coding pci_p2sb_bar() functionality we are going to

> >  use generic library. There is one more user en route.

> > 

> >  This is more than just a clean-up.  It also fixes a potential issue

> >  seen when SPI bar is 64-bit.

> 

> Thanks!

> 

> > Also worth briefly describing what that issue is I think.

> 

> Current code works if and only if the PCI BAR of the hidden device is inside 4G

> address space. In case firmware decides to go above 4G, we will get a wrong

> address.

> 

> Does it sound good enough?


Yes, that explains it, thanks.

> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> > > ---

> > >  drivers/mfd/Kconfig   |  1 +

> > >  drivers/mfd/lpc_ich.c | 20 ++++++--------------

> > >  2 files changed, 7 insertions(+), 14 deletions(-)

> > 

> > Code looks fine:

> > 

> > For my own reference (apply this as-is to your sign-off block):

> > 

> >   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

> 

> Thanks for reviewing this series, can you have a look at the earlier sent [1]

> and [2]? First one has a regression fix.


Yes, thanks for the nudge.  These were not in my TODO list.

> [1]: https://lore.kernel.org/lkml/20210302135620.89958-1-andriy.shevchenko@linux.intel.com/T/#u

> [2]: https://lore.kernel.org/lkml/20210301144222.31290-1-andriy.shevchenko@linux.intel.com/T/#u

> 


-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Patch

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index a03de3f7a8ed..c16bec1852e5 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -553,6 +553,7 @@  config LPC_ICH
 	tristate "Intel ICH LPC"
 	depends on PCI
 	select MFD_CORE
+	select PCI_P2SB if X86
 	help
 	  The LPC bridge function of the Intel ICH provides support for
 	  many functional units. This driver provides needed support for
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 3a19ed57260e..8e9bd6813287 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -42,6 +42,7 @@ 
 #include <linux/errno.h>
 #include <linux/acpi.h>
 #include <linux/pci.h>
+#include <linux/pci-p2sb.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/lpc_ich.h>
 #include <linux/platform_data/itco_wdt.h>
@@ -69,8 +70,6 @@ 
 #define BCR			0xdc
 #define BCR_WPD			BIT(0)
 
-#define SPIBASE_APL_SZ		4096
-
 #define GPIOBASE_ICH0		0x58
 #define GPIOCTRL_ICH0		0x5C
 #define GPIOBASE_ICH6		0x48
@@ -1126,26 +1125,19 @@  static int lpc_ich_init_spi(struct pci_dev *dev)
 		break;
 
 	case INTEL_SPI_BXT: {
-		unsigned int p2sb = PCI_DEVFN(13, 0);
 		unsigned int spi = PCI_DEVFN(13, 2);
-		struct pci_bus *bus = dev->bus;
+		int ret;
 
 		/*
 		 * The P2SB is hidden by BIOS and we need to unhide it in
 		 * order to read BAR of the SPI flash device. Once that is
 		 * done we hide it again.
 		 */
-		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x0);
-		pci_bus_read_config_dword(bus, spi, PCI_BASE_ADDRESS_0,
-					  &spi_base);
-		if (spi_base != ~0) {
-			res->start = spi_base & 0xfffffff0;
-			res->end = res->start + SPIBASE_APL_SZ - 1;
-
-			lpc_ich_test_spi_write(dev, spi, info);
-		}
+		ret = pci_p2sb_bar(dev, spi, res);
+		if (ret)
+			return ret;
 
-		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
+		lpc_ich_test_spi_write(dev, spi, info);
 		break;
 	}