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

Conor.Dooley
2022-07-16T18:32:48Z

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