All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brendan Higgins <brendanhiggins@google.com>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: "Marc Zyngier" <marc.zyngier@arm.com>,
	"Wolfram Sang" <wsa@the-dreams.de>,
	robh+dt@kernel.org, mark.rutland@arm.com, tglx@linutronix.de,
	jason@lakedaemon.net, "Joel Stanley" <joel@jms.id.au>,
	"Vladimir Zapolskiy" <vz@mleia.com>,
	"Kachalov Anton" <mouse@mayc.ru>,
	"Cédric Le Goater" <clg@kaod.org>,
	linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	"OpenBMC Maillist" <openbmc@lists.ozlabs.org>
Subject: Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed
Date: Wed, 29 Mar 2017 02:59:53 -0700	[thread overview]
Message-ID: <CAFd5g46TAGHWtH7p1o=AJqy8_SygpJZ4RJkJpUwDD3nKxpB5YQ@mail.gmail.com> (raw)
In-Reply-To: <1490734216.3177.140.camel@kernel.crashing.org>

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

WARNING: multiple messages have this Message-ID (diff)
From: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
To: Benjamin Herrenschmidt
	<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Cc: "Marc Zyngier" <marc.zyngier-5wv7dgnIgG8@public.gmane.org>,
	"Wolfram Sang" <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	mark.rutland-5wv7dgnIgG8@public.gmane.org,
	tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org,
	jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org,
	"Joel Stanley" <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>,
	"Vladimir Zapolskiy" <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>,
	"Kachalov Anton" <mouse-Pma6HLj0uuo@public.gmane.org>,
	"Cédric Le Goater" <clg-Bxea+6Xhats@public.gmane.org>,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	"OpenBMC Maillist"
	<openbmc-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org>
Subject: Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed
Date: Wed, 29 Mar 2017 02:59:53 -0700	[thread overview]
Message-ID: <CAFd5g46TAGHWtH7p1o=AJqy8_SygpJZ4RJkJpUwDD3nKxpB5YQ@mail.gmail.com> (raw)
In-Reply-To: <1490734216.3177.140.camel-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2017-03-29  9:59 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-28  5:12 [PATCH v6 0/5] i2c: aspeed: added driver for Aspeed I2C Brendan Higgins
2017-03-28  5:12 ` Brendan Higgins
2017-03-28  5:12 ` [PATCH v6 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller Brendan Higgins
2017-03-28  8:49   ` Benjamin Herrenschmidt
2017-03-29 10:34     ` Brendan Higgins
2017-03-29 12:11       ` Benjamin Herrenschmidt
2017-03-29 20:51         ` Brendan Higgins
2017-03-29 21:17           ` Benjamin Herrenschmidt
2017-04-03 14:16   ` Rob Herring
2017-04-03 14:16     ` Rob Herring
2017-03-28  5:12 ` [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed Brendan Higgins
2017-03-28  5:12   ` Brendan Higgins
2017-03-28  8:32   ` Marc Zyngier
2017-03-28  8:32     ` Marc Zyngier
2017-03-28  9:12     ` Benjamin Herrenschmidt
2017-03-28  9:40       ` Marc Zyngier
2017-03-28  9:40         ` Marc Zyngier
2017-03-28 20:50         ` Benjamin Herrenschmidt
2017-03-28 20:50           ` Benjamin Herrenschmidt
2017-03-29  9:59           ` Brendan Higgins [this message]
2017-03-29  9:59             ` Brendan Higgins
2017-03-29 10:55             ` Marc Zyngier
2017-03-28  8:52   ` Benjamin Herrenschmidt
2017-03-28  8:52     ` Benjamin Herrenschmidt
2017-03-29 10:58   ` Joel Stanley
2017-03-29 20:16     ` Brendan Higgins
2017-03-28  5:12 ` [PATCH v6 3/5] i2c: aspeed: added documentation for Aspeed I2C driver Brendan Higgins
2017-03-28  8:54   ` Benjamin Herrenschmidt
2017-03-28  8:54     ` Benjamin Herrenschmidt
2017-03-29 10:25     ` Brendan Higgins
2017-03-29 10:25       ` Brendan Higgins
2017-04-03 14:22     ` Rob Herring
2017-04-03 14:24   ` Rob Herring
2017-03-28  5:12 ` [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C Brendan Higgins
2017-03-28  5:12   ` Brendan Higgins
2017-03-28  8:57   ` Benjamin Herrenschmidt
2017-03-28  9:09   ` Benjamin Herrenschmidt
2017-03-29 10:23     ` Brendan Higgins
2017-03-31  0:33   ` Joel Stanley
2017-03-31  7:33   ` Benjamin Herrenschmidt
2017-03-31  7:33     ` Benjamin Herrenschmidt
2017-04-24 18:56     ` Brendan Higgins
2017-04-24 18:56       ` Brendan Higgins
2017-04-25  2:19       ` Benjamin Herrenschmidt
2017-04-25  8:32         ` Brendan Higgins
2017-04-25  8:32           ` Brendan Higgins
2017-04-25  8:50           ` Ryan Chen
2017-04-25  9:34             ` Benjamin Herrenschmidt
2017-04-25  9:47               ` Ryan Chen
2017-04-25  9:47                 ` Ryan Chen
2017-04-25 19:50                 ` Brendan Higgins
2017-04-26  0:52                   ` Ryan Chen
2017-04-26  0:52                     ` Ryan Chen
2017-03-28  5:12 ` [PATCH v6 5/5] i2c: aspeed: added slave support for Aspeed I2C driver Brendan Higgins
2017-03-31  0:01 ` [PATCH v6 0/5] i2c: aspeed: added driver for Aspeed I2C Andrew Jeffery
2017-03-31  0:01   ` Andrew Jeffery
2017-03-31  0:01   ` Andrew Jeffery

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAFd5g46TAGHWtH7p1o=AJqy8_SygpJZ4RJkJpUwDD3nKxpB5YQ@mail.gmail.com' \
    --to=brendanhiggins@google.com \
    --cc=benh@kernel.crashing.org \
    --cc=clg@kaod.org \
    --cc=devicetree@vger.kernel.org \
    --cc=jason@lakedaemon.net \
    --cc=joel@jms.id.au \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.zyngier@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=mouse@mayc.ru \
    --cc=openbmc@lists.ozlabs.org \
    --cc=robh+dt@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=vz@mleia.com \
    --cc=wsa@the-dreams.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.