lkml.org 
[lkml]   [2012]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] gpio: Emma Mobile GPIO driver V2
Date
On Sat, 19 May 2012 15:46:07 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> On Fri, May 18, 2012 at 05:25:39PM -0600, Grant Likely wrote:
> > On Thu, 17 May 2012 09:41:25 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> > > In adding irq domain support for the sh intc subsystem I hacked up some
> > > prototype code for doing an in-place update of IORESOURCE_IRQ resources
> > > hanging off platform devices that does the hwirq->virq translation and it
> > > seems to work fine, albeit hacky, and something I would rather avoid. On
> > > the other hand, there's no need for that either if we support a 1:1 hwirq
> > > to virq mapping where possible, which is fairly easy to do by just trying
> > > to fetch a virq with irq_alloc_desc_at() before falling back on the
> > > existing virq hinting logic in irq_create_mapping().
> >
> > That's exactly what the current irq_domain code does. Search for
> > 'hint' in the irqdomain code. It just cannot be relied upon in any
> > way when there is more than once irq_domain depending on which one
> > maps it first. Plus I'm planning to remove the 'hint' logic because I
> > think it just adds complexity that doesn't need to be there.
> >
> There's a subtle functionality difference that the irqdomain code fails
> to account for at present, which is the difference between the old
> dynamic IRQ API (create_irq()/create_irq_nr() -- and no, not the stupid
> signed vs unsigned thing). irq_alloc_desc_at() allocates at a fixed
> position within the bitmap, and only at that position. If we're unable to
> get the mapping we want, it's an error. irq_alloc_desc_from() simply uses
> the hwirq as a hint for where to start looking, and will quite happily
> hand you back whatever it finds. At present we have no way to tell the
> irqdomain code to allocate a specific virq, which is what we're going to
> need for converting static mappings over.
>
> I do agree that getting rid of the hinting logic is a good idea though.
> On SH we actually do our hinting the opposite, where we first populate
> the IRQ map with all of our static hardware mappings, and then scan from
> the beginning to find holes for when we need dynamic IRQs. This gives us
> a pretty tightly packed IRQ map, which helps with lookup times, radix
> tree utilization, and nr_irqs expansion for sparseirq (which we use
> unconditionally on all sh + sh/r-mobile arm platforms).
>
> > - get rid of the legacy map (I certainly won't cry to see it go)
> > - switch all legacy users to linear
>
> While this is a good direction to go, it's also important to keep in mind
> that not all legacy users will have linear maps (though perhaps out of
> the existing users it's one or the other). I've added a couple of API
> calls to help with inserting extant mappings and establishing identity
> mappings that should permit legacy users to adopt whatever mapping model
> works the best for them.

Mostly I'm thinking about simplification of the code. I think
irq_domain is a little too complex. I would like to be able to
simplify things my making the legacy map just a linear map that is
pre-populated. The majority of linear users seem to be in the 32-64
num_irqs range so I'm not concerned about wasting memory by allocating
a map for legacy users (and dropping the legacy mapping code reduced
.text by ~660 bytes in my test compile). :-)

>
> Once the semantics have been agreed upon we can begin converting the
> existing users before the _legacy proliferation gets too far along.
>
> > - Add a new api to force-associate hwirqs and linux irqs... something like:
> > irq_domain_associate(struct irq_domain *d, int irq_base,
> > int hwirq_base, int size)
> > - This function will reserve irq numbers (without allocating irq_descs)
> > for the given range.
>
> There's already an irq_reserve_irq{s,}() that can be used to inhibit
> accidental mapping. We use this in the SH INTC case to ensure that all of
> the possible hardware vectors are reserved before we permit dynamic IRQ
> allocation throughout the remaining space (we have a flat vector space
> where both static and dynamic mappings co-exist -- I plan to maintain
> this behaviour in converting to IRQ domains, which should also make it
> easy for DT/non-DT stuff to co-exist in the same vector space).

Yes, I was looking at irq_reserve_irqs() and thinking about how best
to fit it into irq_domain. I would like to be able to defer
allocation of irq_descs in the legacy or reserved use cases, but I'm
not sure the best way to do it. Baby steps I guess. Start with
making it work with reserved irq_descs and worry about lazy allocation
later.

> I've generalized irq_setup_virq() as irq_domain_associate() in order to
> allow direct insertion of existing IRQs while still allowing the domain
> to do whatever it likes with the mapping in its ->map() (ie, revmap
> insertion).
>
> > - When mapping, if an irq has been reserved, then allocate only that
> > irq_desc; otherwise do the normal irq_alloc_desc().
>
> This seems likely to duplicate state. We have no way to determine what
> caused an IRQ to be reserved in the bitmap, or to test for reserved vs
> normally allocated ones. I think we can avoid the need for this by simply
> splitting in to irq_alloc_desc_at() for static and maintaining
> irq_alloc_desc_from() for dynamic.
>
> > - In irq_request, if the irq_desc has not yet been allocated and
> > mapped, then do so.
> >
> > This does require a number of changes in the irq_desc layer though
> > which aren't particularly hard but I haven't wanted to do.
> >
> > Alternately (or perhaps as a stepping stone) irq_domain_associate()
> > could actually allocate and map irq_descs for the given range. It
> > isn't as memory efficient because unused irq_descs still get mapped,
> > but it is a whole lot simpler and easier to understand.
> >
> That's possible as well. I've taken a slightly different approach with my
> patches that leave the irqdesc behaviour alone, but I'm not sure how well
> it will mesh with the DT model.

I've looked at your patches I think it is still pretty similar to what
I'm suggesting above. Just the API needs to be quibbled over I think

g.


--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.


\
 
 \ /
  Last update: 2012-05-19 22:21    [W:0.091 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site