Re: [PATCH v4 12/38] platform/x86: intel_scu_ipc: Split out SCU IPC functionality from the SCU driver

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Wed, Jan 22, 2020 at 02:40:26PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 21, 2020 at 07:00:48PM +0300, Mika Westerberg wrote:
> > The SCU IPC functionality is usable outside of Intel MID devices. For
> > example modern Intel CPUs include the same thing but now it is called
> > PMC (Power Management Controller) instead of SCU. To make the IPC
> > available for those split the driver into library part (intel_scu_ipc.c)
> > and the SCU PCI driver part (intel_scu_pcidrv.c) which then calls the
> > former before it goes and creates rest of the SCU devices.
> > 
> > We also split the Kconfig symbols so that INTEL_SCU_IPC enables the SCU
> > IPC library and INTEL_SCU_PCI the SCU driver and convert the users
> > accordingly. While there remove default y from the INTEL_SCU_PCI symbol
> > as it is already selected by X86_INTEL_MID.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  arch/x86/Kconfig                        |  2 +-
> >  arch/x86/include/asm/intel_scu_ipc.h    | 16 +++++
> >  drivers/mfd/Kconfig                     |  4 +-
> >  drivers/platform/x86/Kconfig            | 26 ++++---
> >  drivers/platform/x86/Makefile           |  1 +
> >  drivers/platform/x86/intel_scu_ipc.c    | 90 ++++++++++++-------------
> >  drivers/platform/x86/intel_scu_pcidrv.c | 54 +++++++++++++++
> >  7 files changed, 135 insertions(+), 58 deletions(-)
> >  create mode 100644 drivers/platform/x86/intel_scu_pcidrv.c
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 5e8949953660..aca17d1583c4 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -594,7 +594,7 @@ config X86_INTEL_MID
> >  	select I2C
> >  	select DW_APB_TIMER
> >  	select APB_TIMER
> > -	select INTEL_SCU_IPC
> > +	select INTEL_SCU_PCI
> >  	select MFD_INTEL_MSIC
> >  	---help---
> >  	  Select to build a kernel capable of supporting Intel MID (Mobile
> > diff --git a/arch/x86/include/asm/intel_scu_ipc.h b/arch/x86/include/asm/intel_scu_ipc.h
> > index 2a1442ba6e78..252ea0046f16 100644
> > --- a/arch/x86/include/asm/intel_scu_ipc.h
> > +++ b/arch/x86/include/asm/intel_scu_ipc.h
> > @@ -2,6 +2,7 @@
> >  #ifndef _ASM_X86_INTEL_SCU_IPC_H_
> >  #define  _ASM_X86_INTEL_SCU_IPC_H_
> >  
> > +#include <linux/ioport.h>
> >  #include <linux/notifier.h>
> >  
> >  #define IPCMSG_INDIRECT_READ	0x02
> > @@ -19,6 +20,21 @@
> >  	#define IPC_CMD_VRTC_SETTIME      1 /* Set time */
> >  	#define IPC_CMD_VRTC_SETALARM     2 /* Set alarm */
> >  
> > +struct device;
> > +
> > +/**
> > + * struct intel_scu_ipc_pdata - Platform data for SCU IPC
> > + * @mem: Base address of SCU IPC MMIO registers
> > + * @irq: The IRQ number used for SCU (optional)
> > + */
> > +struct intel_scu_ipc_pdata {
> > +	struct resource mem;
> > +	int irq;
> > +};
> > +
> > +int intel_scu_ipc_register(struct device *dev,
> > +			   const struct intel_scu_ipc_pdata *pdata);
> > +
> >  /* Read single register */
> >  int intel_scu_ipc_ioread8(u16 addr, u8 *data);
> >  
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 420900852166..59515142438e 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -593,7 +593,7 @@ config INTEL_SOC_PMIC_MRFLD
> >  	tristate "Support for Intel Merrifield Basin Cove PMIC"
> >  	depends on GPIOLIB
> >  	depends on ACPI
> > -	depends on INTEL_SCU_IPC
> > +	depends on INTEL_SCU
> >  	select MFD_CORE
> >  	select REGMAP_IRQ
> >  	help
> > @@ -625,7 +625,7 @@ config MFD_INTEL_LPSS_PCI
> >  
> >  config MFD_INTEL_MSIC
> >  	bool "Intel MSIC"
> > -	depends on INTEL_SCU_IPC
> > +	depends on INTEL_SCU
> >  	select MFD_CORE
> >  	help
> >  	  Select this option to enable access to Intel MSIC (Avatele
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index dd4326736d11..50deff627b46 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -986,25 +986,33 @@ config INTEL_VBTN
> >  	  be called intel_vbtn.
> >  
> >  config INTEL_SCU_IPC
> > -	bool "Intel SCU IPC Support"
> > +	bool
> > +
> > +config INTEL_SCU
> > +	bool
> > +	select INTEL_SCU_IPC
> > +
> > +config INTEL_SCU_PCI
> > +	bool "Intel SCU PCI driver"
> >  	depends on X86_INTEL_MID
> > -	default y
> > -	---help---
> > -	  IPC is used to bridge the communications between kernel and SCU on
> > -	  some embedded Intel x86 platforms. This is not needed for PC-type
> > -	  machines.
> > +	select INTEL_SCU
> > +	help
> > +	  This driver is used to bridge the communications between kernel
> > +	  and SCU on some embedded Intel x86 platforms. It also creates
> > +	  devices that are connected to the SoC through the SCU. This is
> > +	  not needed for PC-type machines.
> >  
> >  config INTEL_SCU_IPC_UTIL
> >  	tristate "Intel SCU IPC utility driver"
> > -	depends on INTEL_SCU_IPC
> > -	---help---
> > +	depends on INTEL_SCU
> > +	help
> >  	  The IPC Util driver provides an interface with the SCU enabling
> >  	  low level access for debug work and updating the firmware. Say
> >  	  N unless you will be doing this on an Intel MID platform.
> >  
> >  config INTEL_MID_POWER_BUTTON
> >  	tristate "power button driver for Intel MID platforms"
> > -	depends on INTEL_SCU_IPC && INPUT
> > +	depends on INTEL_SCU && INPUT
> >  	help
> >  	  This driver handles the power button on the Intel MID platforms.
> >  
> > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > index 42d85a00be4e..c7a42feaa521 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -68,6 +68,7 @@ intel_cht_int33fe-objs		:= intel_cht_int33fe_common.o \
> >  obj-$(CONFIG_INTEL_INT0002_VGPIO) += intel_int0002_vgpio.o
> >  obj-$(CONFIG_INTEL_HID_EVENT)	+= intel-hid.o
> >  obj-$(CONFIG_INTEL_VBTN)	+= intel-vbtn.o
> > +obj-$(CONFIG_INTEL_SCU_PCI)	+= intel_scu_pcidrv.o
> >  obj-$(CONFIG_INTEL_SCU_IPC)	+= intel_scu_ipc.o
> >  obj-$(CONFIG_INTEL_SCU_IPC_UTIL) += intel_scu_ipcutil.o
> >  obj-$(CONFIG_INTEL_MFLD_THERMAL) += intel_mid_thermal.o
> > diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> > index 93a810fa6c8a..a5ca1aae9547 100644
> > --- a/drivers/platform/x86/intel_scu_ipc.c
> > +++ b/drivers/platform/x86/intel_scu_ipc.c
> > @@ -18,11 +18,8 @@
> >  #include <linux/errno.h>
> >  #include <linux/init.h>
> >  #include <linux/interrupt.h>
> > -#include <linux/pci.h>
> > -#include <linux/pm.h>
> > -#include <linux/sfi.h>
> > +#include <linux/io.h>
> >  
> > -#include <asm/intel-mid.h>
> >  #include <asm/intel_scu_ipc.h>
> >  
> >  /* IPC defines the following message types */
> > @@ -56,9 +53,10 @@
> >  
> >  struct intel_scu_ipc_dev {
> >  	struct device *dev;
> > +	struct resource mem;
> > +	int irq;
> >  	void __iomem *ipc_base;
> >  	struct completion cmd_complete;
> > -	u8 irq_mode;
> >  };
> >  
> >  static struct intel_scu_ipc_dev  ipcdev; /* Only one for now */
> > @@ -166,7 +164,7 @@ static inline int ipc_wait_for_interrupt(struct intel_scu_ipc_dev *scu)
> >  
> >  static int intel_scu_ipc_check_status(struct intel_scu_ipc_dev *scu)
> >  {
> > -	return scu->irq_mode ? ipc_wait_for_interrupt(scu) : busy_loop(scu);
> > +	return scu->irq > 0 ? ipc_wait_for_interrupt(scu) : busy_loop(scu);
> >  }
> >  
> >  /* Read/Write power control(PMIC in Langwell, MSIC in PenWell) registers */
> > @@ -402,60 +400,60 @@ static irqreturn_t ioc(int irq, void *dev_id)
> >  }
> >  
> >  /**
> > - *	ipc_probe	-	probe an Intel SCU IPC
> > - *	@pdev: the PCI device matching
> > - *	@id: entry in the match table
> > + * intel_scu_ipc_register() - Register SCU IPC mechanism
> > + * @dev: SCU device
> > + * @pdata: Platform specific data
> >   *
> > - *	Enable and install an intel SCU IPC. This appears in the PCI space
> > - *	but uses some hard coded addresses as well.
> > + * Call this function to initialize SCU IPC mechanism for @dev.
> >   */
> > -static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +int intel_scu_ipc_register(struct device *dev,
> > +			   const struct intel_scu_ipc_pdata *pdata)
> >  {
> 
> How do you "know" that dev really is a SCU device?

Well this expects the caller to know that.

> If you are "registering" something, shouldn't you have set it up
> properly?  You are saying that any random struct device has these
> attributes.  Now that's fine as you "know" this, but usually you would
> create a child device that really is a SCU functionality, i.e. a class
> device.
> 
> So how about making the SCU stuff a class as that is what is then made a
> child of the struct device as it is how it talks to this class of
> devices, right?

That's a good idea. I'll look into this.

> As it is, this "register" really feels odd to me in the scheme of how
> the whole driver model works.
> 
> Not to mention I didn't see any reference counting increment happening
> here, as you are passing a pointer that is then saved off, right?

Right.

> >  	int err;
> >  	struct intel_scu_ipc_dev *scu = &ipcdev;
> >  
> > -	if (scu->dev)		/* We support only one SCU */
> > -		return -EBUSY;
> > +	mutex_lock(&ipclock);
> > +	/* We support only one IPC */
> > +	if (scu->dev) {
> > +		err = -EBUSY;
> > +		goto err_unlock;
> > +	}
> >  
> > -	err = pcim_enable_device(pdev);
> > -	if (err)
> > -		return err;
> > +	if (!request_mem_region(pdata->mem.start, resource_size(&pdata->mem),
> > +				"intel_scu_ipc")) {
> > +		err = -EBUSY;
> > +		goto err_unlock;
> > +	}
> >  
> > -	err = pcim_iomap_regions(pdev, 1 << 0, pci_name(pdev));
> > -	if (err)
> > -		return err;
> > +	scu->ipc_base = ioremap(pdata->mem.start, resource_size(&pdata->mem));
> > +	if (!scu->ipc_base) {
> > +		err = -ENOMEM;
> > +		goto err_release;
> > +	}
> >  
> > +	scu->mem = pdata->mem;
> > +	scu->irq = pdata->irq;
> >  	init_completion(&scu->cmd_complete);
> >  
> > -	scu->ipc_base = pcim_iomap_table(pdev)[0];
> > -
> > -	err = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_scu_ipc",
> > -			       scu);
> > -	if (err)
> > -		return err;
> > +	if (scu->irq > 0) {
> > +		err = request_irq(scu->irq, ioc, 0, "intel_scu_ipc", scu);
> > +		if (err)
> > +			goto err_unmap;
> > +	}
> >  
> >  	/* Assign device at last */
> > -	scu->dev = &pdev->dev;
> > -
> > -	intel_scu_devices_create();
> > +	scu->dev = dev;
> 
> Look ma, a reference count mess!
> 
> Ick.
> 
> Please make this a class.

I will, thanks!