Message-ID: <87v8rvzsfc.wl-maz@kernel.org>
Date:   Sun, 17 Jul 2022 16:10:15 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     <Conor.Dooley@...rochip.com>
Cc:     <Lewis.Hanly@...rochip.com>, <linux-riscv@...ts.infradead.org>,
        <brgl@...ev.pl>, <linux-gpio@...r.kernel.org>,
        <linus.walleij@...aro.org>, <palmer@...belt.com>,
        <linux-kernel@...r.kernel.org>, <Daire.McNamara@...rochip.com>
Subject: Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

On Sat, 16 Jul 2022 19:32:20 +0100,
<Conor.Dooley@...rochip.com> wrote:
> 
> On 16/07/2022 18:52, Marc Zyngier wrote:
> > On Sat, 16 Jul 2022 16:21:48 +0100,
> > <Lewis.Hanly@...rochip.com> wrote:
> >>
> >> Thanks Marc,
> >>
> >> On Sat, 2022-07-16 at 11:33 +0100, Marc Zyngier wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you
> >>> know the content is safe
> >>>
> >>> On Sat, 16 Jul 2022 08:11:13 +0100,
> >>> <lewis.hanly@...rochip.com> wrote:
> >>>> From: Lewis Hanly <lewis.hanly@...rochip.com>
> >>>>
> >>>> Add a driver to support the Polarfire SoC gpio controller.
> >>>>
> >>>> Signed-off-by: Lewis Hanly <lewis.hanly@...rochip.com>
> >>>
> >>> [...]
> >>>
> >>>> +static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> >>>> +                                        unsigned int child,
> >>>> +                                        unsigned int child_type,
> >>>> +                                        unsigned int *parent,
> >>>> +                                        unsigned int *parent_type)
> >>>> +{
> >>>> +     struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> >>>> +     struct irq_data *d = irq_get_irq_data(mpfs_gpio-
> >>>>> irq_number[child]);
> >>>
> >>> This looks totally wrong. It means that you have already instantiated
> >>> part of the hierarchy, and it is likely that you will get multiple
> >>> hierarchy sharing some levels, which isn't intended.
> >>
> >> Some background why I use the above.
> >> We need to support both direct and non-direct IRQ connections to the
> >> PLIC. 
> >> In direct mode the GPIO IRQ's are connected directly to the PLIC and
> >> certainly no need for the above. GPIO's can also be configured in non-
> >> direct, which means they use a shared IRQ, hence the above.
> > 
> > That's unfortunately not acceptable. You need to distinguish which one
> > is which, and separate them. Your non-direct mode certainly requires
> > special handling, and is not fit for a hierarchical mode.
> 
> Unfortunately, the configuration is not fixed on the silicon level. The
> SoC has 3 GPIOs (with 32 lines each). The interrupt configuration looks

Let's start with a bit of terminology so that we can understand each
other:
- GPIO: a single piece of wire
- GPIO block: a set of wires with a common programming interface

As I understand it, you have 3 GPIO blocks, each with 32 GPIOs, for a
total of 96 external lines. Correct?

> something like the below:
> GPIO#             width    IRQ#
> ==================================
> gpio0/2           14       [26:13]
> gpio1/2           24       [50:27]
> gpio0_non_direct  1         51
> gpio1_non_direct  1         52
> gpio2_non_direct  1         53
>
> Depending on what the bootloader/firmware does, these can be configured
> differently (done prior to linux starting). By default, 14 GPIOs from
> GPIO0 are fed into their own interrupt lines & ditto for 24 from GPIO1.
> The remaining GPIO0 & GPIO1 lines go into the corresponding non-direct
> interrupt. If they bootloader/firmware configures something different,
> a "direct" interrupt line can be switched to a GPIO2 line instead.

What does non-direct mean? Multiplexing inputs into a single output?
Can you individually mask/unmask the input lines that are in this mode
(the kernel calls this a "chained irqchip")?

How does this switch between direct and non-direct happen? Do you have
some sort of external pad to GPIO line routing? It would really help
if you could point people at an actual specification for these blocks
rather than paraphrasing things.

> 
> Something like the following (the interrupts are offset by 13 here, as
> the global interrupts feed into the PLIC at an offset):
> 
> * global int  GPIO_INTERRUPT_FAB_CR
>                 0               1
>     0       GPIO0 bit 0     GPIO2 bit 0
>     1       GPIO0 bit 1     GPIO2 bit 1
>     .
>     .
>     12      GPIO0 bit 12    GPIO2 bit 12
>     13      GPIO0 bit 13    GPIO2 bit 13
>     14      GPIO1 bit 0     GPIO2 bit 14
>     15      GPIO1 bit 1     GPIO2 bit 15
>     .
>     .
>     .
>     30      GPIO1 bit 16    GPIO2 bit 30
>     31      GPIO1 bit 17    GPIO2 bit 31
>     32          GPIO1 bit 18
>     33          GPIO1 bit 19
>     34          GPIO1 bit 20
>     35          GPIO1 bit 21
>     36          GPIO1 bit 22
>     37          GPIO1 bit 23
>     38  Or of all GPIO0 interrupts who do not have a direct connection enabled
>     39  Or of all GPIO1 interrupts who do not have a direct connection enabled
>     40  Or of all GPIO2 interrupts who do not have a direct connection enabled
> 
> Since we can tell based on the interrupt number in the device tree
> whether a line is in direct mode - can you suggest what the most 
> appropriate irq structure for the driver?

The topology must be described in DT one way or another, and I don't
really want to rely on a fixed interrupt number that will change from
one version to another.

In any case:

- direct interrupts should be handled as a hierarchy, mostly like the
  code currently does, but definitely without the probing hack.

- muxed interrupts (non-direct?) should be handled via a chained
  irqchip, using a different irqdomain, as the topology is radically
  different.

> Although for extending this driver to the "soft" IP core, it may be easier
> to just create a "microchip,gpio-direct-mode-mask" property or similar and
> use that to figure out what configuration a line is in.

My guts feeling is that this will eventually end-up biting you, as
people will want to change the direct/non-direct status of an
interrupt at boot time, without depending on the FW to do that on
their behalf.

I'm not necessarily advocating for this as this is a lot more code and
it could totally invalidate the existing binding, but this is worth
keeping in mind.

In any case, this driver needs some serious rewriting.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Powered by blists - more mailing lists