Re: [PATCH 14/16] gpio: Add support for banked GPIO controllers

Re: [PATCH 14/16] gpio: Add support for banked GPIO controllers

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [170914 07:00]:
> On Fri, Sep 1, 2017 at 8:57 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Some GPIO controllers are subdivided into multiple logical blocks called
> > banks (or ports). This is often caused by the design assigning separate
> > resources, such as register regions or interrupts, to each bank, or some
> > set of banks.
> >
> > This commit adds support for describing controllers that have such a
> > banked design and provides common code for dealing with them.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> This patch makes me really happy.
> 
> It pulls in a lot of weirdness to the OF core and creates a coherent
> way of handling these "banked" GPIO chips.
> 
> CC to Tony to make sure he checks that OMAP is ready to use this
> too.

Adding Grygorii to Cc as well, we'll take a look.

Probably the runtime PM will be an issue here still. We must currently
do runtime PM on per GPIO bank basis instead of per GPIO pin level as
we constantly runtime_suspend/resume the whole GPIO bank for idle modes
on the SoCs that support PM. So the usage count for the bank needs to
be either 0 or 1 and cannot be the lines used in the bank.

Regards,

Tony

Re: [PATCH 14/16] gpio: Add support for banked GPIO controllers

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170914 16:38]:
> * Linus Walleij <linus.walleij@linaro.org> [170914 07:00]:
> > On Fri, Sep 1, 2017 at 8:57 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > Some GPIO controllers are subdivided into multiple logical blocks called
> > > banks (or ports). This is often caused by the design assigning separate
> > > resources, such as register regions or interrupts, to each bank, or some
> > > set of banks.
> > >
> > > This commit adds support for describing controllers that have such a
> > > banked design and provides common code for dealing with them.
> > >
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > 
> > This patch makes me really happy.
> > 
> > It pulls in a lot of weirdness to the OF core and creates a coherent
> > way of handling these "banked" GPIO chips.
> > 
> > CC to Tony to make sure he checks that OMAP is ready to use this
> > too.
> 
> Adding Grygorii to Cc as well, we'll take a look.
> 
> Probably the runtime PM will be an issue here still. We must currently
> do runtime PM on per GPIO bank basis instead of per GPIO pin level as
> we constantly runtime_suspend/resume the whole GPIO bank for idle modes
> on the SoCs that support PM. So the usage count for the bank needs to
> be either 0 or 1 and cannot be the lines used in the bank.

And based on a quick look at this series it should not cause
problems there. For managing the banks in a generic way, I
like the idea too.

Regards,

Tony

Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked infrastructure

[Tony Lindgren]

* Thierry Reding <thierry.reding@gmail.com> [170915 08:10]:
> On Thu, Sep 14, 2017 at 03:54:56PM +0200, Linus Walleij wrote:
> > Sorry about that. Let's move ahead with this now, it is neat and
> > clean.
> > 
> > What I want (as maintainer) is a bit of fingerpointing at the drivers
> > that need to be converted to use the new banking infrastructure
> > so they don't stay with their old crappy design pattern. OMAP is
> > a clear candidate right? (Added Tony to CC...)
> 
> OMAP should be able to use this infrastructure, but it may not want to
> because the semantics would change slightly. Currently OMAP registers a
> GPIO chip for each bank, whereas this infrastructure exposes multiple
> banks via a single chip.

Oh so you don't have separate interrupts for the instances?
Thanks for clarifying that.

> There might be some userspace that relies on the existence of multiple
> chips, but Tony can probably knows that better than I.

On omaps, each bank is a separate driver instance with it's own
interrupt. Maybe really all we need to do is get rid of the "bank"
naming, I think that's left over from 15 years ago when we did not
have separate driver instances. It seems we should s/bank/ddata/
on the driver to avoid confusion.

Grygorii, any comments?

Regards,

Tony

Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked infrastructure

[Grygorii Strashko]

On 09/15/2017 11:57 AM, Tony Lindgren wrote:
> * Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [170915 08:10]:
>> On Thu, Sep 14, 2017 at 03:54:56PM +0200, Linus Walleij wrote:
>>> Sorry about that. Let's move ahead with this now, it is neat and
>>> clean.
>>>
>>> What I want (as maintainer) is a bit of fingerpointing at the drivers
>>> that need to be converted to use the new banking infrastructure
>>> so they don't stay with their old crappy design pattern. OMAP is
>>> a clear candidate right? (Added Tony to CC...)
>>
>> OMAP should be able to use this infrastructure, but it may not want to
>> because the semantics would change slightly. Currently OMAP registers a
>> GPIO chip for each bank, whereas this infrastructure exposes multiple
>> banks via a single chip.
> 
> Oh so you don't have separate interrupts for the instances?
> Thanks for clarifying that.
> 
>> There might be some userspace that relies on the existence of multiple
>> chips, but Tony can probably knows that better than I.
> 
> On omaps, each bank is a separate driver instance with it's own
> interrupt. Maybe really all we need to do is get rid of the "bank"
> naming, I think that's left over from 15 years ago when we did not
> have separate driver instances. It seems we should s/bank/ddata/
> on the driver to avoid confusion.
> 
> Grygorii, any comments?

Sry, for delayed reply - I've saw this series, but, honestly, it's very big 
change for review :(

So, can it be split? I think, patches which reorganize gpio irqchip specific fields placement 
and move them in gpio_irq_chip can be considered separately if they will not introduce
functional changes. Also, omap changes can be considered separately.
(Pay attention that new fields introduced in patch 1).  

Regarding OMAP GPIO - right now I do not see how it can be applied for OMAP :(
Each OMAP GPIO bank is standalone device which can be enabled/disabled, 
powered on/off in Linux. There are no contiguous MMIO space and each GPIO
bank have separate MMIO space.

I really, need more time to review this idea and I think that it can be done
more easily if series size will be reduced.

Few more notes:
- pay attention on commit dc749a0 "gpiolib: allow gpio irqchip to map irqs dynamically"
- good to see binding and DT examples
- not sure if I've got idea of encoding bank&pin in spec[0] :(

+	bank = (spec[0] >> gc->of_gpio_bank_mask) & gc->of_gpio_bank_shift;
+	pin = (spec[0] >> gc->of_gpio_pin_mask) & gc->of_gpio_pin_shift;

- irq "mapping" inside gpio_irq_chip is not clear :( does it static?
does it require one array item/per pin - sry, this is waste of memory?

irq->map[offset + j] = irq->parents[parent];

Potentially, this feature can be applied to Davinci GPIO driver, which
is GPIO controller divided in multiple logical banks and which also have
set of common registers for all logical banks. But, again, not sure how
effective this implementation is - need more time. As of now, we perfectly 
handle this in Davinci GPIO driver by creating only ONE gpio_chip which hides
HW details in driver and still uses standard irq DT mappings:
 <&gpio0 140 GPIO_ACTIVE_LOW>;

By the way Patch 14 adds 300 lines, while patch changes 200 lines, so
in terms of code lines this feature seems is not very efficient.
(same for Patch 15, 16)

-- 
regards,
-grygorii

Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked infrastructure

[Linus Walleij]

On Fri, Sep 15, 2017 at 6:57 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:

> On omaps, each bank is a separate driver instance with it's own
> interrupt. Maybe really all we need to do is get rid of the "bank"
> naming, I think that's left over from 15 years ago when we did not
> have separate driver instances. It seems we should s/bank/ddata/
> on the driver to avoid confusion.

OK sorry maybe OMAP is not a target for this, I just thought so
since it was one of the platforms that is patches in the patch
series.

But I'm pretty sure we have chips with banking like this: separate
interrupts but a single device. I would have to read through them
all I guess.

Yours,
Linus Walleij