[v6,2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

Message ID 20170328051226.21677-3-brendanhiggins@google.com
State Superseded
Headers show

Commit Message

Brendan Higgins March 28, 2017, 5:12 a.m.
The Aspeed 24XX/25XX chips share a single hardware interrupt across 14
separate I2C busses. This adds a dummy irqchip which maps the single
hardware interrupt to software interrupts for each of the busses.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
Added in v6:
  - Pulled "aspeed_i2c_controller" out into a interrupt controller since that is
    what it actually does.
---
 drivers/irqchip/Makefile            |   2 +-
 drivers/irqchip/irq-aspeed-i2c-ic.c | 102 ++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+), 1 deletion(-)
 create mode 100644 drivers/irqchip/irq-aspeed-i2c-ic.c

Comments

Marc Zyngier March 28, 2017, 8:32 a.m. | #1
Hi Brendan,

On 28/03/17 06:12, Brendan Higgins wrote:
> The Aspeed 24XX/25XX chips share a single hardware interrupt across 14
> separate I2C busses. This adds a dummy irqchip which maps the single
> hardware interrupt to software interrupts for each of the busses.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> Added in v6:
>   - Pulled "aspeed_i2c_controller" out into a interrupt controller since that is
>     what it actually does.
> ---
>  drivers/irqchip/Makefile            |   2 +-
>  drivers/irqchip/irq-aspeed-i2c-ic.c | 102 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/irqchip/irq-aspeed-i2c-ic.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 152bc40b6762..c136c2bd1761 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -74,6 +74,6 @@ obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
>  obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
>  obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
> -obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
> +obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
> diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c b/drivers/irqchip/irq-aspeed-i2c-ic.c
> new file mode 100644
> index 000000000000..59c50b28dec0
> --- /dev/null
> +++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
> @@ -0,0 +1,102 @@
> +/*
> + *  Aspeed 24XX/25XX I2C Interrupt Controller.
> + *
> + *  Copyright (C) 2012-2017 ASPEED Technology Inc.
> + *  Copyright 2017 IBM Corporation
> + *  Copyright 2017 Google, Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/io.h>
> +
> +
> +#define ASPEED_I2C_IC_NUM_BUS 14
> +
> +struct aspeed_i2c_ic {
> +	void __iomem		*base;
> +	int			parent_irq;
> +	struct irq_domain	*irq_domain;
> +};
> +
> +/*
> + * The aspeed chip provides a single hardware interrupt for all of the I2C
> + * busses, so we use a dummy interrupt chip to translate this single interrupt
> + * into multiple interrupts, each associated with a single I2C bus.
> + */
> +static void aspeed_i2c_ic_irq_handler(struct irq_desc *desc)
> +{
> +	struct aspeed_i2c_ic *i2c_ic = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long bit, status;
> +	unsigned int bus_irq;
> +
> +	chained_irq_enter(chip, desc);
> +	status = readl(i2c_ic->base);
> +	for_each_set_bit(bit, &status, ASPEED_I2C_IC_NUM_BUS) {
> +		bus_irq = irq_find_mapping(i2c_ic->irq_domain, bit);
> +		generic_handle_irq(bus_irq);
> +	}
> +	chained_irq_exit(chip, desc);
> +}
> +
> +/*
> + * Set simple handler and mark IRQ as valid. Nothing interesting to do here
> + * since we are using a dummy interrupt chip.
> + */
> +static int aspeed_i2c_ic_map_irq_domain(struct irq_domain *domain,
> +					unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}

I'm a bit concerned by this. It means that you can't even mask an
interrupt. Is that really what you intend to do? Or all that the HW can
do? If you cannot mask an interrupt, you're at the mercy of a screaming
device...

Thanks,

	M.
Benjamin Herrenschmidt March 28, 2017, 8:52 a.m. | #2
On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:
> The Aspeed 24XX/25XX chips share a single hardware interrupt across
> 14
> separate I2C busses. This adds a dummy irqchip which maps the single
> hardware interrupt to software interrupts for each of the busses.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>

I do think as I said earlier that is' a tiny bit overkill, I do worry
about the overhead of the added layer of indirections on a 400Mhz ARMv9
(AST2400) core but otherwise:

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> ---
> Added in v6:
>   - Pulled "aspeed_i2c_controller" out into a interrupt controller
> since that is
>     what it actually does.
> ---
>  drivers/irqchip/Makefile            |   2 +-
>  drivers/irqchip/irq-aspeed-i2c-ic.c | 102
> ++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/irqchip/irq-aspeed-i2c-ic.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 152bc40b6762..c136c2bd1761 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -74,6 +74,6 @@ obj-$(CONFIG_MVEBU_ODMI)		+= irq-
> mvebu-odmi.o
>  obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
>  obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
> -obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
> +obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-
> aspeed-i2c-ic.o
>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-
> combiner.o
> diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c
> b/drivers/irqchip/irq-aspeed-i2c-ic.c
> new file mode 100644
> index 000000000000..59c50b28dec0
> --- /dev/null
> +++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
> @@ -0,0 +1,102 @@
> +/*
> + *  Aspeed 24XX/25XX I2C Interrupt Controller.
> + *
> + *  Copyright (C) 2012-2017 ASPEED Technology Inc.
> + *  Copyright 2017 IBM Corporation
> + *  Copyright 2017 Google, Inc.
> + *
> + *  This program is free software; you can redistribute it and/or
> modify
> + *  it under the terms of the GNU General Public License version 2
> as
> + *  published by the Free Software Foundation.
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/io.h>
> +
> +
> +#define ASPEED_I2C_IC_NUM_BUS 14
> +
> +struct aspeed_i2c_ic {
> +	void __iomem		*base;
> +	int			parent_irq;
> +	struct irq_domain	*irq_domain;
> +};
> +
> +/*
> + * The aspeed chip provides a single hardware interrupt for all of
> the I2C
> + * busses, so we use a dummy interrupt chip to translate this single
> interrupt
> + * into multiple interrupts, each associated with a single I2C bus.
> + */
> +static void aspeed_i2c_ic_irq_handler(struct irq_desc *desc)
> +{
> +	struct aspeed_i2c_ic *i2c_ic =
> irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long bit, status;
> +	unsigned int bus_irq;
> +
> +	chained_irq_enter(chip, desc);
> +	status = readl(i2c_ic->base);
> +	for_each_set_bit(bit, &status, ASPEED_I2C_IC_NUM_BUS) {
> +		bus_irq = irq_find_mapping(i2c_ic->irq_domain, bit);
> +		generic_handle_irq(bus_irq);
> +	}
> +	chained_irq_exit(chip, desc);
> +}
> +
> +/*
> + * Set simple handler and mark IRQ as valid. Nothing interesting to
> do here
> + * since we are using a dummy interrupt chip.
> + */
> +static int aspeed_i2c_ic_map_irq_domain(struct irq_domain *domain,
> +					unsigned int irq,
> irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &dummy_irq_chip,
> handle_simple_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops aspeed_i2c_ic_irq_domain_ops = {
> +	.map = aspeed_i2c_ic_map_irq_domain,
> +};
> +
> +static int __init aspeed_i2c_ic_of_init(struct device_node *node,
> +					struct device_node *parent)
> +{
> +	struct aspeed_i2c_ic *i2c_ic;
> +
> +	i2c_ic = kzalloc(sizeof(*i2c_ic), GFP_KERNEL);
> +	if (!i2c_ic)
> +		return -ENOMEM;
> +
> +	i2c_ic->base = of_iomap(node, 0);
> +	if (IS_ERR(i2c_ic->base))
> +		return PTR_ERR(i2c_ic->base);
> +
> +	i2c_ic->parent_irq = irq_of_parse_and_map(node, 0);
> +	if (i2c_ic->parent_irq < 0)
> +		return i2c_ic->parent_irq;
> +
> +	i2c_ic->irq_domain = irq_domain_add_linear(
> +			node, ASPEED_I2C_IC_NUM_BUS,
> +			&aspeed_i2c_ic_irq_domain_ops, NULL);
> +	if (!i2c_ic->irq_domain)
> +		return -ENOMEM;
> +
> +	i2c_ic->irq_domain->name = "ast-i2c-domain";
> +
> +	irq_set_chained_handler_and_data(i2c_ic->parent_irq,
> +					 aspeed_i2c_ic_irq_handler,
> i2c_ic);
> +
> +	pr_info("i2c controller registered, irq %d\n", i2c_ic-
> >parent_irq);
> +
> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(ast2400_i2c_ic, "aspeed,ast2400-i2c-ic",
> aspeed_i2c_ic_of_init);
> +IRQCHIP_DECLARE(ast2500_i2c_ic, "aspeed,ast2500-i2c-ic",
> aspeed_i2c_ic_of_init);
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt March 28, 2017, 9:12 a.m. | #3
On Tue, 2017-03-28 at 09:32 +0100, Marc Zyngier wrote:
> I'm a bit concerned by this. It means that you can't even mask an
> interrupt. Is that really what you intend to do? Or all that the HW can
> do? If you cannot mask an interrupt, you're at the mercy of a screaming
> device...

This is not really an interrupt controller. It's a "summary" register
that reflects the state of the 14 i2c controller interrupts.

This approach does have the advantage of providing separate counters in
/proc/interrupts which is rather nice, but it does have overhead. On
those shittly little ARMv9 400Mhz cores it can be significant.

I would personally have some kind of trick to register a single
interrupt handler that calls directly the handlers of the respective
i2c busses via a simple indirection for speed, maybe adding my custom
sysfs or debugfs statistics. But that's just me trying to suck the last
cycle out of the bloody thing ;-)

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier March 28, 2017, 9:40 a.m. | #4
On 28/03/17 10:12, Benjamin Herrenschmidt wrote:
> On Tue, 2017-03-28 at 09:32 +0100, Marc Zyngier wrote:
>> I'm a bit concerned by this. It means that you can't even mask an
>> interrupt. Is that really what you intend to do? Or all that the HW can
>> do? If you cannot mask an interrupt, you're at the mercy of a screaming
>> device...
> 
> This is not really an interrupt controller. It's a "summary" register
> that reflects the state of the 14 i2c controller interrupts.
> 
> This approach does have the advantage of providing separate counters in
> /proc/interrupts which is rather nice, but it does have overhead. On
> those shittly little ARMv9 400Mhz cores it can be significant.

<pedantic>
s/ARMv9/ARM9/, as we're still on variations of the ARMv8 architecture ;-)
</pedantic>

A 400MHz ARM9 (which is either ARMv4 or ARMv5) is not too bad (hey, we
still have a couple of Versatile-ABs here...). Caches are pretty small
though.

> I would personally have some kind of trick to register a single
> interrupt handler that calls directly the handlers of the respective
> i2c busses via a simple indirection for speed, maybe adding my custom
> sysfs or debugfs statistics. But that's just me trying to suck the last
> cycle out of the bloody thing ;-)

I'd hope the irqdomain itself to be pretty light (the revmap should help
here), but of course you're going to do more work. Counters also come at
a cost. It'd be interesting to see if Brendan has any overhead data
about this.

Cheers,

	M.
Benjamin Herrenschmidt March 28, 2017, 8:50 p.m. | #5
On Tue, 2017-03-28 at 10:40 +0100, Marc Zyngier wrote:
> On 28/03/17 10:12, Benjamin Herrenschmidt wrote:
> > On Tue, 2017-03-28 at 09:32 +0100, Marc Zyngier wrote:
> > > I'm a bit concerned by this. It means that you can't even mask an
> > > interrupt. Is that really what you intend to do? Or all that the HW can
> > > do? If you cannot mask an interrupt, you're at the mercy of a screaming
> > > device...
> > 
> > This is not really an interrupt controller. It's a "summary" register
> > that reflects the state of the 14 i2c controller interrupts.
> > 
> > This approach does have the advantage of providing separate counters in
> > /proc/interrupts which is rather nice, but it does have overhead. On
> > those shittly little ARMv9 400Mhz cores it can be significant.
> 
> <pedantic>
> s/ARMv9/ARM9/, as we're still on variations of the ARMv8 architecture ;-)
> </pedantic>

It was a typo, I meant ARM9/ARMv5 :-)

The 2 SOC families we are talking about (Aspeed 24xx and 25xx) are
based on a ARM926EJ at 400Mhz and an ARM1176JZFS at 800Mhz
respectively, so cycles do count :-)
 
> A 400MHz ARM9 (which is either ARMv4 or ARMv5) is not too bad (hey, we
> still have a couple of Versatile-ABs here...). Caches are pretty small
> though.

16K/16K, no L2 :) 

> > I would personally have some kind of trick to register a single
> > interrupt handler that calls directly the handlers of the respective
> > i2c busses via a simple indirection for speed, maybe adding my custom
> > sysfs or debugfs statistics. But that's just me trying to suck the last
> > cycle out of the bloody thing ;-)
> 
> I'd hope the irqdomain itself to be pretty light (the revmap should help
> here), but of course you're going to do more work. Counters also come at
> a cost. It'd be interesting to see if Brendan has any overhead data
> about this.

Thankfully, the HW supports buffered sends/receive or even DMA. The
current patch doesn't yet support these but they would be a good way to
alleviate the cost of the interrupts if it becomes a problem.

Cheers,
Ben.

> Cheers,
> 
> 	M.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brendan Higgins March 29, 2017, 9:59 a.m. | #6
The main reason I took this approach is just because I thought it was
cleaner from the perspective of the busses which are totally
independent (except for the fact that they share a single hardware
interrupt).

I did not make any measurements, so I doubt that I have anything to
add that you don't already know. I saw other usages of chained
interrupts that do the same thing (scan a "status" register and use
them to make software interrupts) and I thought that is basically what
the dummy irq chip code is for. The only thing I thought I was doing
that was novel was actually breaking out the dummy irqchip into its
own driver; it is not my idea, but I do think makes it a lot cleaner.
Nevertheless, it should be cheap in terms of number of instructions;
the most expensive part looks like looking up the mapping. In any
case, I think the low hanging fruit here is supporting buffering or
DMA, like Ben suggested.

To address the comment on being over engineered: outside of the init
function (which would exist regardless of how we do this, if not here
then in the I2C driver); the code is actually pretty small and
generic.

All that being said, it would not be very hard to do this without
using the dummy irqchip code and it would definitely be smaller in
terms of indirection and space used, but I think the code would
actually be more complicated to read. We would be going back to having
an I2C controller along with the I2C busses; where all the I2C
controller does is read the IRQ register and then call the appropriate
bus irq handler, which looks a lot like a dummy irqchip.

Cheers
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier March 29, 2017, 10:55 a.m. | #7
On 29/03/17 10:59, Brendan Higgins wrote:
> The main reason I took this approach is just because I thought it was
> cleaner from the perspective of the busses which are totally
> independent (except for the fact that they share a single hardware
> interrupt).
> 
> I did not make any measurements, so I doubt that I have anything to
> add that you don't already know. I saw other usages of chained
> interrupts that do the same thing (scan a "status" register and use
> them to make software interrupts) and I thought that is basically what
> the dummy irq chip code is for. The only thing I thought I was doing
> that was novel was actually breaking out the dummy irqchip into its
> own driver; it is not my idea, but I do think makes it a lot cleaner.
> Nevertheless, it should be cheap in terms of number of instructions;
> the most expensive part looks like looking up the mapping. In any
> case, I think the low hanging fruit here is supporting buffering or
> DMA, like Ben suggested.
> 
> To address the comment on being over engineered: outside of the init
> function (which would exist regardless of how we do this, if not here
> then in the I2C driver); the code is actually pretty small and
> generic.
> 
> All that being said, it would not be very hard to do this without
> using the dummy irqchip code and it would definitely be smaller in
> terms of indirection and space used, but I think the code would
> actually be more complicated to read. We would be going back to having
> an I2C controller along with the I2C busses; where all the I2C
> controller does is read the IRQ register and then call the appropriate
> bus irq handler, which looks a lot like a dummy irqchip.

As long as you're happy with the performance and the restrictions that
come attached to the HW, I'm happy to take the irqchip patches.

Thanks,

	M.
Joel Stanley March 29, 2017, 10:58 a.m. | #8
On Tue, Mar 28, 2017 at 3:42 PM, Brendan Higgins
<brendanhiggins@google.com> wrote:
> +static int __init aspeed_i2c_ic_of_init(struct device_node *node,
> +                                       struct device_node *parent)
> +{
> +       struct aspeed_i2c_ic *i2c_ic;
> +
> +       i2c_ic = kzalloc(sizeof(*i2c_ic), GFP_KERNEL);
> +       if (!i2c_ic)
> +               return -ENOMEM;
> +
> +       i2c_ic->base = of_iomap(node, 0);
> +       if (IS_ERR(i2c_ic->base))
> +               return PTR_ERR(i2c_ic->base);
> +
> +       i2c_ic->parent_irq = irq_of_parse_and_map(node, 0);
> +       if (i2c_ic->parent_irq < 0)
> +               return i2c_ic->parent_irq;
> +
> +       i2c_ic->irq_domain = irq_domain_add_linear(
> +                       node, ASPEED_I2C_IC_NUM_BUS,
> +                       &aspeed_i2c_ic_irq_domain_ops, NULL);
> +       if (!i2c_ic->irq_domain)
> +               return -ENOMEM;
> +
> +       i2c_ic->irq_domain->name = "ast-i2c-domain";

Nit: Make this aspeed-i2c-domain to make this consistent with the
other Aspeed drivers in the kernel tree.

Could this irq code be embedded in the i2c driver? We took a similar
approach for the Aspeed GPIO driver, which has a similar IRQ structure
of one hardware IRQ that tells the driver to check status registers
for the precise irq source. The upside being all of the i2c code is in
the same place in the kernel tree.

Cheers,

Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brendan Higgins March 29, 2017, 8:16 p.m. | #9
> Nit: Make this aspeed-i2c-domain to make this consistent with the
> other Aspeed drivers in the kernel tree.
>
> Could this irq code be embedded in the i2c driver? We took a similar
> approach for the Aspeed GPIO driver, which has a similar IRQ structure
> of one hardware IRQ that tells the driver to check status registers
> for the precise irq source. The upside being all of the i2c code is in
> the same place in the kernel tree.

In the previous version of the patch, this code was embedded in the
I2C driver as the "struct aspeed_i2c_controller;" I really did not
change anything about it other than rename some stuff and change the
init method to match what irqchip code wants. I pulled it out into a
separate driver because I was asked to by Vladimir; nevertheless, it
does turn the I2C driver into a normal platforms driver, which is
nice. Another benefit: if we put our dummy irqchip code in with the
other irqchips, it makes it easier for the irqchip people to recognize
when we are reusing the same patterns; for example, I would not at all
be surprised if there are other dummy irqchips which have the same
exact map(...) operation (I looked and did not see anything), but it
is quite possibly something that other people want to do. If we put
this stuff in drivers/irqchip, it is more likely that the irqchip
people would recognize this as a common use case. I do not think
either of these reasons I provided are particularly compelling, but I
do not think the reasons to move it out are particularly compelling
either (unless we decide we do not want make our own irq_domain).

Cheers
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 152bc40b6762..c136c2bd1761 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -74,6 +74,6 @@  obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
 obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
 obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
-obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
+obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c b/drivers/irqchip/irq-aspeed-i2c-ic.c
new file mode 100644
index 000000000000..59c50b28dec0
--- /dev/null
+++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
@@ -0,0 +1,102 @@ 
+/*
+ *  Aspeed 24XX/25XX I2C Interrupt Controller.
+ *
+ *  Copyright (C) 2012-2017 ASPEED Technology Inc.
+ *  Copyright 2017 IBM Corporation
+ *  Copyright 2017 Google, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ */
+
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+
+
+#define ASPEED_I2C_IC_NUM_BUS 14
+
+struct aspeed_i2c_ic {
+	void __iomem		*base;
+	int			parent_irq;
+	struct irq_domain	*irq_domain;
+};
+
+/*
+ * The aspeed chip provides a single hardware interrupt for all of the I2C
+ * busses, so we use a dummy interrupt chip to translate this single interrupt
+ * into multiple interrupts, each associated with a single I2C bus.
+ */
+static void aspeed_i2c_ic_irq_handler(struct irq_desc *desc)
+{
+	struct aspeed_i2c_ic *i2c_ic = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long bit, status;
+	unsigned int bus_irq;
+
+	chained_irq_enter(chip, desc);
+	status = readl(i2c_ic->base);
+	for_each_set_bit(bit, &status, ASPEED_I2C_IC_NUM_BUS) {
+		bus_irq = irq_find_mapping(i2c_ic->irq_domain, bit);
+		generic_handle_irq(bus_irq);
+	}
+	chained_irq_exit(chip, desc);
+}
+
+/*
+ * Set simple handler and mark IRQ as valid. Nothing interesting to do here
+ * since we are using a dummy interrupt chip.
+ */
+static int aspeed_i2c_ic_map_irq_domain(struct irq_domain *domain,
+					unsigned int irq, irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops aspeed_i2c_ic_irq_domain_ops = {
+	.map = aspeed_i2c_ic_map_irq_domain,
+};
+
+static int __init aspeed_i2c_ic_of_init(struct device_node *node,
+					struct device_node *parent)
+{
+	struct aspeed_i2c_ic *i2c_ic;
+
+	i2c_ic = kzalloc(sizeof(*i2c_ic), GFP_KERNEL);
+	if (!i2c_ic)
+		return -ENOMEM;
+
+	i2c_ic->base = of_iomap(node, 0);
+	if (IS_ERR(i2c_ic->base))
+		return PTR_ERR(i2c_ic->base);
+
+	i2c_ic->parent_irq = irq_of_parse_and_map(node, 0);
+	if (i2c_ic->parent_irq < 0)
+		return i2c_ic->parent_irq;
+
+	i2c_ic->irq_domain = irq_domain_add_linear(
+			node, ASPEED_I2C_IC_NUM_BUS,
+			&aspeed_i2c_ic_irq_domain_ops, NULL);
+	if (!i2c_ic->irq_domain)
+		return -ENOMEM;
+
+	i2c_ic->irq_domain->name = "ast-i2c-domain";
+
+	irq_set_chained_handler_and_data(i2c_ic->parent_irq,
+					 aspeed_i2c_ic_irq_handler, i2c_ic);
+
+	pr_info("i2c controller registered, irq %d\n", i2c_ic->parent_irq);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(ast2400_i2c_ic, "aspeed,ast2400-i2c-ic", aspeed_i2c_ic_of_init);
+IRQCHIP_DECLARE(ast2500_i2c_ic, "aspeed,ast2500-i2c-ic", aspeed_i2c_ic_of_init);