Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[<Conor.Dooley@xxxxxxxxxxxxx>]

On 16/07/2022 18:52, Marc Zyngier wrote:
> On Sat, 16 Jul 2022 16:21:48 +0100,
> <Lewis.Hanly@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> wrote:
>>>> From: Lewis Hanly <lewis.hanly@xxxxxxxxxxxxx>
>>>>
>>>> Add a driver to support the Polarfire SoC gpio controller.
>>>>
>>>> Signed-off-by: Lewis Hanly <lewis.hanly@xxxxxxxxxxxxx>
>>>
>>> [...]
>>>
>>>> +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
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.

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?

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.

Thanks,
Conor.