Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Mika Westerberg]

On Wed, May 13, 2020 at 03:13:28PM +0100, Richard Hughes wrote:
> On Wed, 13 May 2020 at 10:11, Mika Westerberg
> <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> > > I can fix up all those, but out of interest how did you "know" the
> > > right three digit identifier to use?
> > I work for Intel ;-)
> 
> Hah, okay, thanks :)
> 
> > > I'm really wondering if drivers/mfd/lpc_ich.c is the right place for
> > > this kind of "just expose one byte of PCI config space" functionality.
> > Ideally there is one driver per device.
> 
> My idea in https://github.com/hughsie/spi_lpc was to not actually
> register a pci_driver.

OK, I see the original code iterated over PCI devices finding anything
that matches the IDs in the table.

This may be problematic if there is driver bound to the device and
accessing the hardware simultaneusly. Although this is just read side
and I don't think these registers have any side effects when you read
them, so should not be an issue.

> 
> > If this is touching the 00:1f.5 PCI device (SPI-NOR controller) then the
> > right place is the intel-spi-pci.c as that's the driver for this
> > controller.
> 
> So Cannon Lake, Cannon Point and Ice Lake would go into
> drivers/mtd/spi-nor/controllers/intel-spi-pci.c and the systems like
> Sunrise Point using an ISA bridge would use drivers/mfd/lpc_ich.c?

Yes, something like that.

> > We can put this there so that it does not enable the SPI-NOR
> > functionality itself and the mark the SPI-NOR functionality only as
> > being dangerous or something like that.
> 
> I think getting the distros to enable SPI_INTEL_SPI_PCI might be a
> tough sell. Could we perhaps remove the DANGEROUS label as it's not
> writeable without a module option?

I would like to keep it (the label) there but I think we can label
SPI_INTEL_SPI with that instead and then make the -pci.c to work
standalone if CONFIG_SPI_INTEL_SPI is not enabled.

config SPI_INTEL_SPI
        tristate "Intel PCH/PCU SPI flash core driver (DANGEROUS)"
	depends on SPI_INTEL_SPI_PCI || SPI_INTEL_SPI_PLATFORM
	...

config SPI_INTEL_SPI_PCI
        tristate "Intel PCH/PCU SPI flash PCI driver"
	depends on PCI
	...

Then distros could enable only CONFIG_SPI_INTEL_SPI_PCI which would only
expose the security bits. Of course I'm open to any other ideas :)