RE: [PATCH 4/6] irqchip/aspeed: Add Aspeed eSPI interrupt controller

[ChiaWei Wang]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@xxxxxxxxxx>
> Sent: Wednesday, January 6, 2021 6:59 PM
> To: ChiaWei Wang <chiawei_wang@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH 4/6] irqchip/aspeed: Add Aspeed eSPI interrupt controller
> 
> On 2021-01-06 05:59, Chia-Wei, Wang wrote:
> > The eSPI interrupt controller acts as a SW IRQ number decoder to
> > correctly control/dispatch interrupts of the eSPI peripheral, virtual
> > wire, out-of-band, and flash channels.
> >
> > Signed-off-by: Chia-Wei, Wang <chiawei_wang@xxxxxxxxxxxxxx>
> > ---
> >  drivers/irqchip/Makefile             |   2 +-
> >  drivers/irqchip/irq-aspeed-espi-ic.c | 251 ++++++++++++++++++++++++
> >  include/soc/aspeed/espi.h            | 279
> +++++++++++++++++++++++++++
> >  3 files changed, 531 insertions(+), 1 deletion(-)  create mode 100644
> > drivers/irqchip/irq-aspeed-espi-ic.c
> >  create mode 100644 include/soc/aspeed/espi.h
> >
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index
> > 0ac93bfaec61..56da4a3123f8 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -86,7 +86,7 @@ obj-$(CONFIG_MVEBU_PIC)			+=
> irq-mvebu-pic.o
> >  obj-$(CONFIG_MVEBU_SEI)			+= irq-mvebu-sei.o
> >  obj-$(CONFIG_LS_EXTIRQ)			+= irq-ls-extirq.o
> >  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
> > -obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
> > irq-aspeed-scu-ic.o
> > +obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
> > irq-aspeed-scu-ic.o irq-aspeed-espi-ic.o
> >  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
> >  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
> >  obj-$(CONFIG_IRQ_UNIPHIER_AIDET)	+= irq-uniphier-aidet.o
> > diff --git a/drivers/irqchip/irq-aspeed-espi-ic.c
> > b/drivers/irqchip/irq-aspeed-espi-ic.c
> > new file mode 100644
> > index 000000000000..8a5cc8fe3f0c
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-aspeed-espi-ic.c
> > @@ -0,0 +1,251 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2020 Aspeed Technology Inc.
> > + */
> > +#include <linux/bitops.h>
> > +#include <linux/module.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h>
> > +#include <linux/interrupt.h> #include <linux/mfd/syscon.h> #include
> > +<linux/regmap.h> #include <linux/of.h> #include <linux/of_platform.h>
> > +
> > +#include <soc/aspeed/espi.h>
> > +#include <dt-bindings/interrupt-controller/aspeed-espi-ic.h>
> > +
> > +#define DEVICE_NAME	"aspeed-espi-ic"
> > +#define IRQCHIP_NAME	"eSPI-IC"
> > +
> > +#define ESPI_IC_IRQ_NUM	7
> > +
> > +struct aspeed_espi_ic {
> > +	struct regmap *map;
> > +	int irq;
> > +	int gpio_irq;
> > +	struct irq_domain *irq_domain;
> > +};
> > +
> > +static void aspeed_espi_ic_gpio_isr(struct irq_desc *desc) {
> > +	unsigned int irq;
> > +	struct aspeed_espi_ic *espi_ic = irq_desc_get_handler_data(desc);
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +
> > +	chained_irq_enter(chip, desc);
> > +
> > +	irq = irq_find_mapping(espi_ic->irq_domain,
> > +				   ASPEED_ESPI_IC_CTRL_RESET);
> > +	generic_handle_irq(irq);
> > +
> > +	irq = irq_find_mapping(espi_ic->irq_domain,
> > +				   ASPEED_ESPI_IC_CHAN_RESET);
> > +	generic_handle_irq(irq);
> 
> So for each mux interrupt, you generate two endpoints interrupt, without even
> checking whether they are pending? That's no good.

As the eSPI IC driver is chained to Aspeed GPIO IC, the pending is checked in the gpio-aspeed.c

> > +
> > +	chained_irq_exit(chip, desc);
> > +}
> > +
> > +static void aspeed_espi_ic_isr(struct irq_desc *desc) {
> > +	unsigned int sts;
> > +	unsigned int irq;
> > +	struct aspeed_espi_ic *espi_ic = irq_desc_get_handler_data(desc);
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +
> > +	chained_irq_enter(chip, desc);
> > +
> > +	regmap_read(espi_ic->map, ESPI_INT_STS, &sts);
> > +
> > +	if (sts & ESPI_INT_STS_PERIF_BITS) {
> > +		irq = irq_find_mapping(espi_ic->irq_domain,
> > +				       ASPEED_ESPI_IC_PERIF_EVENT);
> > +		generic_handle_irq(irq);
> > +	}
> > +
> > +	if (sts & ESPI_INT_STS_VW_BITS) {
> > +		irq = irq_find_mapping(espi_ic->irq_domain,
> > +				       ASPEED_ESPI_IC_VW_EVENT);
> > +		generic_handle_irq(irq);
> > +	}
> > +
> > +	if (sts & ESPI_INT_STS_OOB_BITS) {
> > +		irq = irq_find_mapping(espi_ic->irq_domain,
> > +				       ASPEED_ESPI_IC_OOB_EVENT);
> > +		generic_handle_irq(irq);
> > +	}
> > +
> > +	if (sts & ESPI_INT_STS_FLASH_BITS) {
> > +		irq = irq_find_mapping(espi_ic->irq_domain,
> > +				       ASPEED_ESPI_IC_FLASH_EVENT);
> > +		generic_handle_irq(irq);
> > +	}
> > +
> > +	if (sts & ESPI_INT_STS_HW_RST_DEASSERT) {
> > +		irq = irq_find_mapping(espi_ic->irq_domain,
> > +				       ASPEED_ESPI_IC_CTRL_EVENT);
> > +		generic_handle_irq(irq);
> > +	}
> 
> This is horrible. Why can't you just use fls() in a loop?

The bits in the interrupt status register for a eSPI channel are not sequentially arranged.
Using fls() may invoke an eSPI channel ISR multiple times.
So I collected the bitmap for each channel, respectively, and call the ISR at once.

> 
> > +
> > +	chained_irq_exit(chip, desc);
> > +}
> > +
> > +static void aspeed_espi_ic_irq_disable(struct irq_data *data) {
> > +	struct aspeed_espi_ic *espi_ic = irq_data_get_irq_chip_data(data);
> > +
> > +	switch (data->hwirq) {
> > +	case ASPEED_ESPI_IC_CTRL_EVENT:
> > +		regmap_update_bits(espi_ic->map, ESPI_INT_EN,
> > +				   ESPI_INT_EN_HW_RST_DEASSERT,
> > +				   0);
> > +		break;
> > +	case ASPEED_ESPI_IC_PERIF_EVENT:
> > +		regmap_update_bits(espi_ic->map, ESPI_INT_EN,
> > +				   ESPI_INT_EN_PERIF_BITS, 0);
> > +		break;
> > +	case ASPEED_ESPI_IC_VW_EVENT:
> > +		regmap_update_bits(espi_ic->map, ESPI_INT_EN,
> > +				   ESPI_INT_EN_VW_BITS, 0);
> > +		break;
> > +	case ASPEED_ESPI_IC_OOB_EVENT:
> > +		regmap_update_bits(espi_ic->map, ESPI_INT_EN,
> > +				   ESPI_INT_EN_OOB_BITS, 0);
> > +		break;
> > +	case ASPEED_ESPI_IC_FLASH_EVENT:
> > +		regmap_update_bits(espi_ic->map, ESPI_INT_EN,
> > +				   ESPI_INT_EN_FLASH_BITS, 0);
> > +		break;
> > +	}
> 
> Most of these are masking multiple events at once, which makes me think that
> it really doesn't belong here...
> 
> > +}
> > +
> > +static void aspeed_espi_ic_irq_enable(struct irq_data *data) {
> > +	struct aspeed_espi_ic *espi_ic = irq_data_get_irq_chip_data(data);
> > +
> > +	switch (data->hwirq) {
> > +	case ASPEED_ESPI_IC_CTRL_EVENT:
> > +		regmap_update_bits(espi_ic->map, ESPI_INT_EN,
> > +				   ESPI_INT_EN_HW_RST_DEASSERT,
> > +				   ESPI_INT_EN_HW_RST_DEASSERT);
> > +		break;
> > +	case ASPEED_ESPI_IC_PERIF_EVENT:
> > +		regmap_update_bits(espi_ic->map, ESPI_INT_EN,
> > +				   ESPI_INT_EN_PERIF_BITS,
> > +				   ESPI_INT_EN_PERIF_BITS);
> > +		break;
> > +	case ASPEED_ESPI_IC_VW_EVENT:
> > +		regmap_update_bits(espi_ic->map, ESPI_INT_EN,
> > +				   ESPI_INT_EN_VW_BITS,
> > +				   ESPI_INT_EN_VW_BITS);
> > +		break;
> > +	case ASPEED_ESPI_IC_OOB_EVENT:
> > +		regmap_update_bits(espi_ic->map, ESPI_INT_EN,
> > +				   ESPI_INT_EN_OOB_BITS,
> > +				   ESPI_INT_EN_OOB_BITS);
> > +		break;
> > +	case ASPEED_ESPI_IC_FLASH_EVENT:
> > +		regmap_update_bits(espi_ic->map, ESPI_INT_EN,
> > +				   ESPI_INT_EN_FLASH_BITS,
> > +				   ESPI_INT_EN_FLASH_BITS);
> > +		break;
> > +	}
> > +}
> > +
> > +static struct irq_chip aspeed_espi_ic_chip = {
> > +	.name = IRQCHIP_NAME,
> > +	.irq_enable = aspeed_espi_ic_irq_enable,
> > +	.irq_disable = aspeed_espi_ic_irq_disable, };
> > +
> > +static int aspeed_espi_ic_map(struct irq_domain *domain, unsigned int
> > irq,
> > +			     irq_hw_number_t hwirq)
> > +{
> > +	irq_set_chip_and_handler(irq, &aspeed_espi_ic_chip,
> > handle_simple_irq);
> > +	irq_set_chip_data(irq, domain->host_data);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops aspeed_espi_ic_domain_ops = {
> > +	.map = aspeed_espi_ic_map,
> > +};
> > +
> > +static int aspeed_espi_ic_probe(struct platform_device *pdev) {
> > +	struct device *dev;
> > +	struct aspeed_espi_ic *espi_ic;
> > +
> > +	dev = &pdev->dev;
> > +
> > +	espi_ic = devm_kzalloc(dev, sizeof(*espi_ic), GFP_KERNEL);
> > +	if (!espi_ic)
> > +		return -ENOMEM;
> > +
> > +	espi_ic->map = syscon_node_to_regmap(dev->parent->of_node);
> > +	if (IS_ERR(espi_ic->map)) {
> > +		dev_err(dev, "cannot get regmap\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	espi_ic->irq = platform_get_irq(pdev, 0);
> > +	if (espi_ic->irq < 0)
> > +		return espi_ic->irq;
> > +
> > +	espi_ic->gpio_irq = platform_get_irq(pdev, 1);
> > +	if (espi_ic->gpio_irq < 0)
> > +		return espi_ic->gpio_irq;
> > +
> > +	espi_ic->irq_domain = irq_domain_add_linear(dev->of_node,
> > ESPI_IC_IRQ_NUM,
> > +						    &aspeed_espi_ic_domain_ops,
> > +						    espi_ic);
> > +	if (!espi_ic->irq_domain) {
> > +		dev_err(dev, "cannot to add irq domain\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	irq_set_chained_handler_and_data(espi_ic->irq,
> > +					 aspeed_espi_ic_isr,
> > +					 espi_ic);
> > +
> > +	irq_set_chained_handler_and_data(espi_ic->gpio_irq,
> > +					 aspeed_espi_ic_gpio_isr,
> > +					 espi_ic);
> > +
> > +	dev_set_drvdata(dev, espi_ic);
> > +
> > +	dev_info(dev, "eSPI IRQ controller initialized\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int aspeed_espi_ic_remove(struct platform_device *pdev) {
> > +	struct aspeed_espi_ic *espi_ic = platform_get_drvdata(pdev);
> > +
> > +	irq_domain_remove(espi_ic->irq_domain);
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id aspeed_espi_ic_of_matches[] = {
> > +	{ .compatible = "aspeed,ast2600-espi-ic" },
> > +	{ },
> > +};
> > +
> > +static struct platform_driver aspeed_espi_ic_driver = {
> > +	.driver = {
> > +		.name = DEVICE_NAME,
> > +		.of_match_table = aspeed_espi_ic_of_matches,
> > +	},
> > +	.probe = aspeed_espi_ic_probe,
> > +	.remove = aspeed_espi_ic_remove,
> > +};
> > +
> > +module_platform_driver(aspeed_espi_ic_driver);
> > +
> > +MODULE_AUTHOR("Chia-Wei Wang <chiawei_wang@xxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Aspeed eSPI interrupt controller");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/soc/aspeed/espi.h b/include/soc/aspeed/espi.h new
> > file mode 100644 index 000000000000..c9a4f51737ee
> > --- /dev/null
> > +++ b/include/soc/aspeed/espi.h
> > @@ -0,0 +1,279 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2020 Aspeed Technology Inc.
> > + * Author: Chia-Wei Wang <chiawei_wang@xxxxxxxxxxxxxx>  */ #ifndef
> > +_ASPEED_ESPI_H_ #define _ASPEED_ESPI_H_
> 
> [...]
> 
> If nothing else uses the data here, move it to the irqchip driver.

The header will be used by other eSPI driver files. 

Chiawei.