Re: [PATCH linux dev-4.10 1/7] drivers/fsi: Add slave interrupt polling

[Eddie James <eajames@linux.vnet.ibm.com>]

On 05/09/2017 04:38 PM, Christopher Bostic wrote:
> Scan slaves present for asserting interrupt signals in the
> si1s register and call a registered client's interrupt handler as
> appropriate.
>
> Includes initial code from Eddie James <eajames@us.ibm.com>
>
> Signed-off-by: Eddie James <eajames@us.ibm.com>
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
>   drivers/fsi/fsi-core.c   | 110 +++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/fsi/fsi-master.h |   1 +
>   include/linux/fsi.h      |   5 +++
>   3 files changed, 116 insertions(+)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index a6ed34f..73fdb69 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -15,9 +15,11 @@
>
>   #include <linux/device.h>
>   #include <linux/fsi.h>
> +#include <linux/kthread.h>
>   #include <linux/idr.h>
>   #include <linux/module.h>
>   #include <linux/slab.h>
> +#include <linux/jiffies.h>
>
>   #include "fsi-master.h"
>
> @@ -38,8 +40,11 @@
>   #define FSI_PEEK_BASE			0x410
>
>   static const int engine_page_size = 0x400;
> +static struct task_struct *master_ipoll;
> +static unsigned int fsi_ipoll_period_ms = 100;
>
>   #define FSI_SLAVE_BASE			0x800
> +#define FSI_IPOLL_PERIOD		msecs_to_jiffies(fsi_ipoll_period_ms)
>
>   /*
>    * FSI slave engine control register offsets
> @@ -47,6 +52,8 @@
>   #define FSI_SMODE		0x0	/* R/W: Mode register */
>   #define FSI_SISC		0x8	/* R/W: Interrupt condition */
>   #define FSI_SSTAT		0x14	/* R  : Slave status */
> +#define FSI_SI1M		0x18	/* R/W: IRQ mask */
> +#define FSI_SI1S		0x1C	/* R  : IRQ status */
>   #define FSI_LLMODE		0x100	/* R/W: Link layer mode register */
>
>   /*
> @@ -342,6 +349,7 @@ static int fsi_slave_scan(struct fsi_slave *slave)
>   	uint32_t engine_addr;
>   	uint32_t conf;
>   	int rc, i;
> +	uint8_t si1s_bit = 1;
>
>   	/*
>   	 * scan engines
> @@ -396,6 +404,7 @@ static int fsi_slave_scan(struct fsi_slave *slave)
>   			dev->unit = i;
>   			dev->addr = engine_addr;
>   			dev->size = slots * engine_page_size;
> +			dev->si1s_bit = si1s_bit++;
>
>   			dev_info(&slave->dev,
>   			"engine[%i]: type %x, version %x, addr %x size %x\n",
> @@ -761,6 +770,90 @@ static void fsi_master_unscan(struct fsi_master *master)
>   	device_for_each_child(&master->dev, NULL, __fsi_master_remove_slave);
>   }
>
> +static int __fsi_dev_irq(struct device *dev, void *data)
> +{
> +	uint32_t *si1s = data;
> +	struct fsi_device *fsi_dev = to_fsi_dev(dev);
> +
> +	if (!fsi_dev || !si1s) {
> +		dev_dbg(dev, "Invalid input: %p %p\n", fsi_dev, si1s);
> +		return -EINVAL;
> +	}
> +
> +	if (*si1s & (0x80000000 >> fsi_dev->si1s_bit) &&
> +					fsi_dev->irq_handler) {
> +		fsi_dev->irq_handler(0, &fsi_dev->dev);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __fsi_slave_irq(struct device *dev, void *data)
> +{
> +	return device_for_each_child(dev, data, __fsi_dev_irq);
> +}
> +
> +static void fsi_master_irq(struct fsi_master *master, int link, uint32_t si1s)
> +{
> +	device_for_each_child(&master->dev, &si1s, __fsi_slave_irq);
> +}
> +
> +static int fsi_master_ipoll(void *data)
> +{
> +	int rc;
> +	uint32_t si1s;
> +	unsigned long elapsed = 0;
> +	unsigned long previous_jiffies = jiffies;
> +	struct fsi_master *master = data;
> +
> +	while (!kthread_should_stop()) {
> +		if (!master->ipoll)
> +			goto done;
> +
> +		/* Ignore errors for now */
> +		rc = master->read(master, 0, 0, FSI_SLAVE_BASE + FSI_SI1S,
> +				&si1s, sizeof(uint32_t));
> +		if (rc)
> +			goto done;
> +
> +		if (si1s & master->ipoll)
> +			fsi_master_irq(master, 0, si1s);
> +done:
> +		elapsed = jiffies = previous_jiffies;
> +		if (elapsed < FSI_IPOLL_PERIOD) {
> +			set_current_state(TASK_UNINTERRUPTIBLE);
> +			schedule_timeout(FSI_IPOLL_PERIOD - elapsed);
> +		}
> +		previous_jiffies = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * TODO: move this to master->start_ipoll() - each master may have its
> + * own way of doing this
> + */
> +int fsi_master_start_ipoll(struct fsi_master *master)
> +{
> +	if (master_ipoll) {
> +		dev_err(&master->dev, "Already polling for irqs\n");
> +		return -EALREADY;
> +	}
> +	master_ipoll = kthread_create(fsi_master_ipoll, master,
> +					"fsi_master_ipoll");
> +	if (IS_ERR(master_ipoll)) {
> +		dev_err(&master->dev, "Coudn't create ipoll thread rc:%d\n",
> +			(int)PTR_ERR(master_ipoll));
> +		return PTR_ERR(master_ipoll);
> +	}
> +	wake_up_process(master_ipoll);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fsi_master_start_ipoll);
> +
>   static ssize_t master_rescan_store(struct device *dev,
>   		struct device_attribute *attr, const char *buf, size_t count)
>   {
> @@ -768,6 +861,7 @@ static ssize_t master_rescan_store(struct device *dev,
>
>   	fsi_master_unscan(master);
>   	fsi_master_scan(master);
> +	fsi_master_start_ipoll(master);

I have been having some problems in 4.10 with this patch in the build. 
BMC is hanging and requires AC cycle to recover, directly after scanning 
fsi. I haven't had time to debug but so far it seems to follow this 
patch. I do not think this should be merged until Chris and I have done 
more testing.

Thanks,
Eddie

>
>   	return count;
>   }
> @@ -829,6 +923,11 @@ void fsi_master_unregister(struct fsi_master *master)
>   		master->idx = -1;
>   	}
>
> +	if (master_ipoll) {
> +		kthread_stop(master_ipoll);
> +		master_ipoll = NULL;
> +	}
> +
>   	device_unregister(&master->dev);
>   }
>   EXPORT_SYMBOL_GPL(fsi_master_unregister);
> @@ -872,6 +971,17 @@ void fsi_driver_unregister(struct fsi_driver *fsi_drv)
>   }
>   EXPORT_SYMBOL_GPL(fsi_driver_unregister);
>
> +int fsi_enable_irq(struct fsi_device *dev)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL(fsi_enable_irq);
> +
> +void fsi_disable_irq(struct fsi_device *dev)
> +{
> +}
> +EXPORT_SYMBOL(fsi_disable_irq);
> +
>   struct bus_type fsi_bus_type = {
>   	.name		= "fsi",
>   	.match		= fsi_bus_match,
> diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
> index fd39924..668ab0c 100644
> --- a/drivers/fsi/fsi-master.h
> +++ b/drivers/fsi/fsi-master.h
> @@ -26,6 +26,7 @@ struct fsi_master {
>   	int		idx;
>   	int		n_links;
>   	int		flags;
> +	uint32_t	ipoll;
>   	int		(*read)(struct fsi_master *, int link, uint8_t id,
>   				uint32_t addr, void *val, size_t size);
>   	int		(*write)(struct fsi_master *, int link, uint8_t id,
> diff --git a/include/linux/fsi.h b/include/linux/fsi.h
> index 141fd38..bb5c6c6 100644
> --- a/include/linux/fsi.h
> +++ b/include/linux/fsi.h
> @@ -22,9 +22,11 @@ struct fsi_device {
>   	u8			engine_type;
>   	u8			version;
>   	u8			unit;
> +	u8			si1s_bit;
>   	struct fsi_slave	*slave;
>   	uint32_t		addr;
>   	uint32_t		size;
> +	int (*irq_handler)(int, void *);
>   };
>
>   extern int fsi_device_read(struct fsi_device *dev, uint32_t addr,
> @@ -80,4 +82,7 @@ extern int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
>
>   extern struct bus_type fsi_bus_type;
>
> +extern int fsi_enable_irq(struct fsi_device *dev);
> +extern void fsi_disable_irq(struct fsi_device *dev);
> +
>   #endif /* LINUX_FSI_H */