Re: [PATCH 5/6] soc: aspeed: Add eSPI driver

Re: [PATCH 5/6] soc: aspeed: Add eSPI driver

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7094 bytes --]

Hi Wang",

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.11-rc2 next-20210104]
[cannot apply to joel-aspeed/for-next robh/for-next tip/irq/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Chia-Wei-Wang/arm-aspeed-Add-eSPI-support/20210106-140337
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62
config: ia64-randconfig-m031-20210106 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4ee780938ea027aa22e3dbd0b9041477da033ee0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Chia-Wei-Wang/arm-aspeed-Add-eSPI-support/20210106-140337
        git checkout 4ee780938ea027aa22e3dbd0b9041477da033ee0
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> error: include/uapi/linux/aspeed-espi.h: missing "WITH Linux-syscall-note" for SPDX-License-Identifier
--
>> error: include/uapi/linux/aspeed-espi.h: missing "WITH Linux-syscall-note" for SPDX-License-Identifier
   make[2]: *** [scripts/Makefile.headersinst:63: usr/include/linux/aspeed-espi.h] Error 1
   make[2]: Target '__headers' not remade because of errors.
   make[1]: *** [Makefile:1294: headers] Error 2
   make[1]: Target 'headers_install' not remade because of errors.
   make: *** [Makefile:185: __sub-make] Error 2
   make: Target 'headers_install' not remade because of errors.
--
>> error: include/uapi/linux/aspeed-espi.h: missing "WITH Linux-syscall-note" for SPDX-License-Identifier
   make[2]: *** [scripts/Makefile.headersinst:63: usr/include/linux/aspeed-espi.h] Error 1
   make[2]: Target '__headers' not remade because of errors.
   make[1]: *** [Makefile:1294: headers] Error 2
   In file included from include/linux/kernel.h:16,
                    from include/asm-generic/bug.h:20,
                    from arch/ia64/include/asm/bug.h:17,
                    from include/linux/bug.h:5,
                    from include/linux/page-flags.h:10,
                    from kernel/bounds.c:10:
   include/linux/printk.h:219:5: error: static declaration of 'printk' follows non-static declaration
     219 | int printk(const char *s, ...)
         |     ^~~~~~
   In file included from arch/ia64/include/uapi/asm/intrinsics.h:22,
                    from arch/ia64/include/asm/intrinsics.h:11,
                    from arch/ia64/include/asm/bitops.h:19,
                    from include/linux/bitops.h:32,
                    from include/linux/kernel.h:11,
                    from include/asm-generic/bug.h:20,
                    from arch/ia64/include/asm/bug.h:17,
                    from include/linux/bug.h:5,
                    from include/linux/page-flags.h:10,
                    from kernel/bounds.c:10:
   arch/ia64/include/uapi/asm/cmpxchg.h:142:14: note: previous declaration of 'printk' was here
     142 |   extern int printk(const char *fmt, ...);  \
         |              ^~~~~~
   arch/ia64/include/asm/bitops.h:309:3: note: in expansion of macro 'CMPXCHG_BUGCHECK'
     309 |   CMPXCHG_BUGCHECK(m);
         |   ^~~~~~~~~~~~~~~~
   make[2]: *** [scripts/Makefile.build:117: kernel/bounds.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1206: prepare0] Error 2
   make[1]: Target 'modules_prepare' not remade because of errors.
   make: *** [Makefile:185: __sub-make] Error 2
   make: Target 'modules_prepare' not remade because of errors.
--
   cc1: warning: ./arch/ia64/include/generated: No such file or directory [-Wmissing-include-dirs]
   cc1: warning: ./arch/ia64/include/generated/uapi: No such file or directory [-Wmissing-include-dirs]
   cc1: warning: ./include/generated/uapi: No such file or directory [-Wmissing-include-dirs]
   arch/ia64/kernel/nr-irqs.c:15:6: warning: no previous prototype for 'foo' [-Wmissing-prototypes]
      15 | void foo(void)
         |      ^~~
>> error: include/uapi/linux/aspeed-espi.h: missing "WITH Linux-syscall-note" for SPDX-License-Identifier
   make[2]: *** [scripts/Makefile.headersinst:63: usr/include/linux/aspeed-espi.h] Error 1
   make[2]: Target '__headers' not remade because of errors.
   make[1]: *** [Makefile:1294: headers] Error 2
   In file included from include/linux/kernel.h:16,
                    from include/asm-generic/bug.h:20,
                    from arch/ia64/include/asm/bug.h:17,
                    from include/linux/bug.h:5,
                    from include/linux/page-flags.h:10,
                    from kernel/bounds.c:10:
   include/linux/printk.h:219:5: error: static declaration of 'printk' follows non-static declaration
     219 | int printk(const char *s, ...)
         |     ^~~~~~
   In file included from arch/ia64/include/uapi/asm/intrinsics.h:22,
                    from arch/ia64/include/asm/intrinsics.h:11,
                    from arch/ia64/include/asm/bitops.h:19,
                    from include/linux/bitops.h:32,
                    from include/linux/kernel.h:11,
                    from include/asm-generic/bug.h:20,
                    from arch/ia64/include/asm/bug.h:17,
                    from include/linux/bug.h:5,
                    from include/linux/page-flags.h:10,
                    from kernel/bounds.c:10:
   arch/ia64/include/uapi/asm/cmpxchg.h:142:14: note: previous declaration of 'printk' was here
     142 |   extern int printk(const char *fmt, ...);  \
         |              ^~~~~~
   arch/ia64/include/asm/bitops.h:309:3: note: in expansion of macro 'CMPXCHG_BUGCHECK'
     309 |   CMPXCHG_BUGCHECK(m);
         |   ^~~~~~~~~~~~~~~~
   make[2]: *** [scripts/Makefile.build:117: kernel/bounds.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1206: prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:185: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for FRAME_POINTER
   Depends on DEBUG_KERNEL && (M68K || UML || SUPERH) || ARCH_WANT_FRAME_POINTERS
   Selected by
   - FAULT_INJECTION_STACKTRACE_FILTER && FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT && !X86_64 && !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM && !ARC && !X86

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33093 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

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@aspeedtech.com>
> ---
>  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.

> +
> +	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?

> +
> +	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@aspeedtech.com>");
> +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> +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@aspeedtech.com>
> + */
> +#ifndef _ASPEED_ESPI_H_
> +#define _ASPEED_ESPI_H_

[...]

If nothing else uses the data here, move it to the irqchip driver.

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] dt-bindings: aspeed: Add eSPI controller

[Rob Herring]

On Wed, 06 Jan 2021 13:59:34 +0800, Chia-Wei, Wang wrote:
> Add dt-bindings and the inclusion header for Aspeed eSPI controller.
> 
> Signed-off-by: Chia-Wei, Wang <chiawei_wang@aspeedtech.com>
> ---
>  .../devicetree/bindings/soc/aspeed/espi.yaml  | 252 ++++++++++++++++++
>  .../interrupt-controller/aspeed-espi-ic.h     |  15 ++
>  2 files changed, 267 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/aspeed/espi.yaml
>  create mode 100644 include/dt-bindings/interrupt-controller/aspeed-espi-ic.h
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/soc/aspeed/espi.example.dts:45.35-36 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:344: Documentation/devicetree/bindings/soc/aspeed/espi.example.dt.yaml] Error 1
make: *** [Makefile:1370: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1422809

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/6] soc: aspeed: Add eSPI driver

[Rob Herring]

On Wed, Jan 06, 2021 at 01:59:38PM +0800, Chia-Wei, Wang wrote:
> The Aspeed eSPI controller is slave device to communicate with
> the master through the Enhanced Serial Peripheral Interface (eSPI).
> All of the four eSPI channels, namely peripheral, virtual wire,
> out-of-band, and flash are supported.
> 
> Signed-off-by: Chia-Wei, Wang <chiawei_wang@aspeedtech.com>
> ---
>  drivers/soc/aspeed/Kconfig                  |  49 ++
>  drivers/soc/aspeed/Makefile                 |   5 +
>  drivers/soc/aspeed/aspeed-espi-ctrl.c       | 197 ++++++
>  drivers/soc/aspeed/aspeed-espi-flash.c      | 490 ++++++++++++++
>  drivers/soc/aspeed/aspeed-espi-oob.c        | 706 ++++++++++++++++++++
>  drivers/soc/aspeed/aspeed-espi-peripheral.c | 613 +++++++++++++++++
>  drivers/soc/aspeed/aspeed-espi-vw.c         | 211 ++++++
>  include/uapi/linux/aspeed-espi.h            | 160 +++++
>  8 files changed, 2431 insertions(+)
>  create mode 100644 drivers/soc/aspeed/aspeed-espi-ctrl.c
>  create mode 100644 drivers/soc/aspeed/aspeed-espi-flash.c
>  create mode 100644 drivers/soc/aspeed/aspeed-espi-oob.c
>  create mode 100644 drivers/soc/aspeed/aspeed-espi-peripheral.c
>  create mode 100644 drivers/soc/aspeed/aspeed-espi-vw.c

drivers/spi/ is the correct location for a SPI controller.

>  create mode 100644 include/uapi/linux/aspeed-espi.h

This userspace interface is not going to be accepted upstream.

I'd suggest you look at similar SPI flash capable SPI controller drivers 
upstream and model your driver after them. This looks like it needs 
major reworking.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 1/6] dt-bindings: aspeed: Add eSPI controller

[ChiaWei Wang]

Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Wednesday, January 6, 2021 11:08 PM
> To: ChiaWei Wang <chiawei_wang@aspeedtech.com>
> Subject: Re: [PATCH 1/6] dt-bindings: aspeed: Add eSPI controller
> 
> On Wed, 06 Jan 2021 13:59:34 +0800, Chia-Wei, Wang wrote:
> > Add dt-bindings and the inclusion header for Aspeed eSPI controller.
> >
> > Signed-off-by: Chia-Wei, Wang <chiawei_wang@aspeedtech.com>
> > ---
> >  .../devicetree/bindings/soc/aspeed/espi.yaml  | 252
> ++++++++++++++++++
> >  .../interrupt-controller/aspeed-espi-ic.h     |  15 ++
> >  2 files changed, 267 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/soc/aspeed/espi.yaml
> >  create mode 100644
> > include/dt-bindings/interrupt-controller/aspeed-espi-ic.h
> >
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Error:
> Documentation/devicetree/bindings/soc/aspeed/espi.example.dts:45.35-36
> syntax error FATAL ERROR: Unable to parse input tree
> make[1]: *** [scripts/Makefile.lib:344:
> Documentation/devicetree/bindings/soc/aspeed/espi.example.dt.yaml] Error 1
> make: *** [Makefile:1370: dt_binding_check] Error 2
> 
> See https://patchwork.ozlabs.org/patch/1422809
> 
> This check can fail if there are any dependencies. The base for a patch series is
> generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above error(s),
> then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit.

My 'make dt_binding_check' did not show the error.
I will update the tool as suggested to fix the error. Thanks.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 5/6] soc: aspeed: Add eSPI driver

[ChiaWei Wang]

Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Wednesday, January 6, 2021 11:32 PM
> To: ChiaWei Wang <chiawei_wang@aspeedtech.com>
> Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver
> 
> On Wed, Jan 06, 2021 at 01:59:38PM +0800, Chia-Wei, Wang wrote:
> > The Aspeed eSPI controller is slave device to communicate with the
> > master through the Enhanced Serial Peripheral Interface (eSPI).
> > All of the four eSPI channels, namely peripheral, virtual wire,
> > out-of-band, and flash are supported.
> >
> > Signed-off-by: Chia-Wei, Wang <chiawei_wang@aspeedtech.com>
> > ---
> >  drivers/soc/aspeed/Kconfig                  |  49 ++
> >  drivers/soc/aspeed/Makefile                 |   5 +
> >  drivers/soc/aspeed/aspeed-espi-ctrl.c       | 197 ++++++
> >  drivers/soc/aspeed/aspeed-espi-flash.c      | 490 ++++++++++++++
> >  drivers/soc/aspeed/aspeed-espi-oob.c        | 706
> ++++++++++++++++++++
> >  drivers/soc/aspeed/aspeed-espi-peripheral.c | 613 +++++++++++++++++
> >  drivers/soc/aspeed/aspeed-espi-vw.c         | 211 ++++++
> >  include/uapi/linux/aspeed-espi.h            | 160 +++++
> >  8 files changed, 2431 insertions(+)
> >  create mode 100644 drivers/soc/aspeed/aspeed-espi-ctrl.c
> >  create mode 100644 drivers/soc/aspeed/aspeed-espi-flash.c
> >  create mode 100644 drivers/soc/aspeed/aspeed-espi-oob.c
> >  create mode 100644 drivers/soc/aspeed/aspeed-espi-peripheral.c
> >  create mode 100644 drivers/soc/aspeed/aspeed-espi-vw.c
> 
> drivers/spi/ is the correct location for a SPI controller.
> 
> >  create mode 100644 include/uapi/linux/aspeed-espi.h
> 
> This userspace interface is not going to be accepted upstream.
> 
> I'd suggest you look at similar SPI flash capable SPI controller drivers upstream
> and model your driver after them. This looks like it needs major reworking.
> 
eSPI resues the timing and electrical specification of SPI but runs completely different protocol.
Only the flash channel is related to SPI and the other 3 channels are for EC/BMC/SIO.
Therefore, an eSPI driver might not fit into the SPI model.

Chiawei

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[ChiaWei Wang]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Wednesday, January 6, 2021 6:59 PM
> To: ChiaWei Wang <chiawei_wang@aspeedtech.com>
> 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@aspeedtech.com>
> > ---
> >  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@aspeedtech.com>");
> > +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> > +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@aspeedtech.com>  */ #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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

On 2021-01-07 02:59, ChiaWei Wang wrote:
> Hi Marc,
> 
>> -----Original Message-----
>> From: Marc Zyngier <maz@kernel.org>
>> Sent: Wednesday, January 6, 2021 6:59 PM
>> To: ChiaWei Wang <chiawei_wang@aspeedtech.com>
>> 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@aspeedtech.com>
>> > ---
>> >  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

That's not the place to do that.

> 
>> > +
>> > +	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.

And that's equally wrong. You need to handle interrupts individually,
as they are different signal. If you are to implement an interrupt
controller, please do it properly.

Otherwise, get rid of it and move everything into your pet driver.
There is no need to do a half-baked job.

As it is, there is no way this code can be merged.

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[ChiaWei Wang]

Hi Marc,

I will revise the patch as suggested.
Thanks for the feedback.

Chiawei

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, January 7, 2021 6:18 PM
> To: ChiaWei Wang <chiawei_wang@aspeedtech.com>
> <BMC-SW@aspeedtech.com>
> Subject: Re: [PATCH 4/6] irqchip/aspeed: Add Aspeed eSPI interrupt controller
> 
> On 2021-01-07 02:59, ChiaWei Wang wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Zyngier <maz@kernel.org>
> >> Sent: Wednesday, January 6, 2021 6:59 PM
> >> To: ChiaWei Wang <chiawei_wang@aspeedtech.com>
> >> 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@aspeedtech.com>
> >> > ---
> >> >  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
> 
> That's not the place to do that.
> 
> >
> >> > +
> >> > +	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.
> 
> And that's equally wrong. You need to handle interrupts individually, as they
> are different signal. If you are to implement an interrupt controller, please do it
> properly.
> 
> Otherwise, get rid of it and move everything into your pet driver.
> There is no need to do a half-baked job.
> 
> As it is, there is no way this code can be merged.
> 
>          M.
> --
> Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/6] soc: aspeed: Add eSPI driver

[Joel Stanley]

On Thu, 7 Jan 2021 at 02:39, ChiaWei Wang <chiawei_wang@aspeedtech.com> wrote:
>
> Hi Rob,
>
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Wednesday, January 6, 2021 11:32 PM
> > To: ChiaWei Wang <chiawei_wang@aspeedtech.com>
> > Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver
> >
> > On Wed, Jan 06, 2021 at 01:59:38PM +0800, Chia-Wei, Wang wrote:
> > > The Aspeed eSPI controller is slave device to communicate with the
> > > master through the Enhanced Serial Peripheral Interface (eSPI).
> > > All of the four eSPI channels, namely peripheral, virtual wire,
> > > out-of-band, and flash are supported.
> > >
> > > Signed-off-by: Chia-Wei, Wang <chiawei_wang@aspeedtech.com>
> > > ---
> > >  drivers/soc/aspeed/Kconfig                  |  49 ++
> > >  drivers/soc/aspeed/Makefile                 |   5 +
> > >  drivers/soc/aspeed/aspeed-espi-ctrl.c       | 197 ++++++
> > >  drivers/soc/aspeed/aspeed-espi-flash.c      | 490 ++++++++++++++
> > >  drivers/soc/aspeed/aspeed-espi-oob.c        | 706
> > ++++++++++++++++++++
> > >  drivers/soc/aspeed/aspeed-espi-peripheral.c | 613 +++++++++++++++++
> > >  drivers/soc/aspeed/aspeed-espi-vw.c         | 211 ++++++
> > >  include/uapi/linux/aspeed-espi.h            | 160 +++++
> > >  8 files changed, 2431 insertions(+)
> > >  create mode 100644 drivers/soc/aspeed/aspeed-espi-ctrl.c
> > >  create mode 100644 drivers/soc/aspeed/aspeed-espi-flash.c
> > >  create mode 100644 drivers/soc/aspeed/aspeed-espi-oob.c
> > >  create mode 100644 drivers/soc/aspeed/aspeed-espi-peripheral.c
> > >  create mode 100644 drivers/soc/aspeed/aspeed-espi-vw.c
> >
> > drivers/spi/ is the correct location for a SPI controller.
> >
> > >  create mode 100644 include/uapi/linux/aspeed-espi.h
> >
> > This userspace interface is not going to be accepted upstream.
> >
> > I'd suggest you look at similar SPI flash capable SPI controller drivers upstream
> > and model your driver after them. This looks like it needs major reworking.
> >
> eSPI resues the timing and electrical specification of SPI but runs completely different protocol.
> Only the flash channel is related to SPI and the other 3 channels are for EC/BMC/SIO.
> Therefore, an eSPI driver might not fit into the SPI model.

I agree, the naming is confusing but eSPI doesn't belong in drivers/spi.

As it is a bus that is common to more than just the Aspeed BMC, we may
want to implement it as a new bus type that has devices hanging off
it, similar to FSI.

Cheers,

Joel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 5/6] soc: aspeed: Add eSPI driver

[Ryan Chen]

> -----Original Message-----
> From: Joel Stanley <joel@jms.id.au>
> Sent: Friday, January 8, 2021 10:59 AM
> To: ChiaWei Wang <chiawei_wang@aspeedtech.com>; Jeremy Kerr
> <jk@codeconstruct.com.au>
> Cc: Rob Herring <robh@kernel.org>; andrew@aj.id.au; tglx@linutronix.de;
> maz@kernel.org; p.zabel@pengutronix.de; linux-aspeed@lists.ozlabs.org;
> openbmc@lists.ozlabs.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; BMC-SW
> <BMC-SW@aspeedtech.com>
> Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver
> 
> On Thu, 7 Jan 2021 at 02:39, ChiaWei Wang
> <chiawei_wang@aspeedtech.com> wrote:
> >
> > Hi Rob,
> >
> > > -----Original Message-----
> > > From: Rob Herring <robh@kernel.org>
> > > Sent: Wednesday, January 6, 2021 11:32 PM
> > > To: ChiaWei Wang <chiawei_wang@aspeedtech.com>
> > > Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver
> > >
> > > On Wed, Jan 06, 2021 at 01:59:38PM +0800, Chia-Wei, Wang wrote:
> > > > The Aspeed eSPI controller is slave device to communicate with the
> > > > master through the Enhanced Serial Peripheral Interface (eSPI).
> > > > All of the four eSPI channels, namely peripheral, virtual wire,
> > > > out-of-band, and flash are supported.
> > > >
> > > > Signed-off-by: Chia-Wei, Wang <chiawei_wang@aspeedtech.com>
> > > > ---
> > > >  drivers/soc/aspeed/Kconfig                  |  49 ++
> > > >  drivers/soc/aspeed/Makefile                 |   5 +
> > > >  drivers/soc/aspeed/aspeed-espi-ctrl.c       | 197 ++++++
> > > >  drivers/soc/aspeed/aspeed-espi-flash.c      | 490 ++++++++++++++
> > > >  drivers/soc/aspeed/aspeed-espi-oob.c        | 706
> > > ++++++++++++++++++++
> > > >  drivers/soc/aspeed/aspeed-espi-peripheral.c | 613
> +++++++++++++++++
> > > >  drivers/soc/aspeed/aspeed-espi-vw.c         | 211 ++++++
> > > >  include/uapi/linux/aspeed-espi.h            | 160 +++++
> > > >  8 files changed, 2431 insertions(+)  create mode 100644
> > > > drivers/soc/aspeed/aspeed-espi-ctrl.c
> > > >  create mode 100644 drivers/soc/aspeed/aspeed-espi-flash.c
> > > >  create mode 100644 drivers/soc/aspeed/aspeed-espi-oob.c
> > > >  create mode 100644 drivers/soc/aspeed/aspeed-espi-peripheral.c
> > > >  create mode 100644 drivers/soc/aspeed/aspeed-espi-vw.c
> > >
> > > drivers/spi/ is the correct location for a SPI controller.
> > >
> > > >  create mode 100644 include/uapi/linux/aspeed-espi.h
> > >
> > > This userspace interface is not going to be accepted upstream.
> > >
> > > I'd suggest you look at similar SPI flash capable SPI controller
> > > drivers upstream and model your driver after them. This looks like it needs
> major reworking.
> > >
> > eSPI resues the timing and electrical specification of SPI but runs completely
> different protocol.
> > Only the flash channel is related to SPI and the other 3 channels are for
> EC/BMC/SIO.
> > Therefore, an eSPI driver might not fit into the SPI model.
> 
> I agree, the naming is confusing but eSPI doesn't belong in drivers/spi.
> 
> As it is a bus that is common to more than just the Aspeed BMC, we may want
> to implement it as a new bus type that has devices hanging off it, similar to
> FSI.
> 
The ASPEED eSPI controller driver is slave side device, not master side. I think it will be stay soc/aspeed first. 
Because is most SoC Chip related. 

Cheers,

Ryan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel