Messages in this thread | | | From | Fabien DESSENNE <> | Subject | Re: [PATCH] irqchip: stm32: add a second level init to request hwspinlock | Date | Fri, 8 Mar 2019 16:01:17 +0000 |
| |
Hi Marc,
Thanks for your detailed answer.
I will study how to move this driver to a device one.
BR
Fabien
On 08/03/2019 4:30 PM, Marc Zyngier wrote: > On Fri, 08 Mar 2019 14:03:55 +0000, > Fabien DESSENNE <fabien.dessenne@st.com> wrote: > > Fabien, > >> Hi Marc, >> >> Thank you for your feedback. Let me try to explain this patch, and the >> reason of its unusual implementation choices. >> >> >> Regarding the driver init mode: >> As an important requirement, I want to keep this irq driver declared >> with IRQCHIP_DECLARE(), so it is initialized early from >> start_kernel()/init_IRQ(). >> Most of the other irq drivers are implemented this way and I imagine >> that this ensures the availability of the irq drivers, before the other >> platform drivers get probed. > Let me get this straight: > > - Either you don't have dependencies on anything, and you need to > enable your irqchip early -> you use IRQCHIP_DECLARE. > > - Or you have dependencies on other subsystems -> You *do not* use > IRQCHIP_DECLARE, and use the expected dependency system (deferred > probing) > > There is no intermediate state. The other irqchip controllers that use > IRQCHIP_DECLARE *do NOT* have dependencies on external subsystems. > >> Regarding the second init: >> With the usage of the hwspinlock framework (used to protect against >> coprocessor concurrent access to registers) we have a problem as the >> hwspinlock driver is not available when the irq driver is being initialized. >> In order to solve this, I added a second initialization where we get a >> reference to hwspinlock. >> You pointed that we are not supposed to use of_node_clear_flag (which >> allows to get a second init call) : >> I spent some time to find any information about it, but could not find >> any reason to not use it. >> Please, let me know if I missed something here. > Yes, you missed the fact that each time someone tries to add some > driver probing via an initcall, we push back. This is an internal > kernel mechanism that is not to be used by random, non architectural > drivers such as this interrupt controller. > > Furthermore, you're playing with stuff that is outside of the exported > API of the DT framework. Clearing node flags is not something I really > want to see, as you're messing with a state machine that isn't under > your control. > >> Regarding the inits sequence and dependencies: >> - The second init is guaranteed to be called after the first one, since >> start_kernel()->init_IRQ() is called before platform drivers init. > There is no such requirements that holds for secondary interrupt > controllers. > >> - During the second init, the dependency with the hwspinlock driver is >> implemented correctly : it makes use of defered probe when needed. > Then do the right thing all the way: move your favourite toy irqchip > to being a proper device driver, use the right abstraction, and stop > piling ugly hacks on top of each other. Other irqchip drivers do that > just fine (all GPIO irqchips, for example), and most drivers are > already able to defer their probe routine. > >> I understand that this patch is 'surprising' but I hope that my >> explanations justify its implementation. > Surprising is not the word I'd have used. The explanations do not > justify anything, as all you're saying is "it is the way it is because > it is the way it is". So please fix your irqchip driver properly, in a > way that is maintainable in the long term, using the abstractions that > are available. If such abstractions are not good enough, please > explain what you need and we'll work something out. > > Thanks, > > M. > | |