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

[<Conor.Dooley@xxxxxxxxxxxxx>]

On 17/07/2022 16:10, Marc Zyngier wrote:
> On Sat, 16 Jul 2022 19:32:20 +0100,
> <Conor.Dooley@xxxxxxxxxxxxx> wrote:
>>
>> 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
> 
> 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?

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?

non-direct being "not directly connected to the PLIC", so yes. The
interrupts without a direct connection are muxed into a per GPIO block
interrupt.

> Can you individually mask/unmask the input lines that are in this mode
> (the kernel calls this a "chained irqchip")?

Each GPIO line has an interrupt enable bit in it's config register.

> 
> 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.

GPIO is discussed on page 85 of the TRM:
https://www.microsemi.com/document-portal/doc_download/1245725-polarfire-soc-fpga-mss-technical-reference-manual

The register map is here:
https://ww1.microchip.com/downloads/aemDocuments/documents/FPGA/ProductDocuments/SupportingCollateral/V1_4_Register_Map.zip

PFSOC_MSS_TOP_SYSREG/GPIO_INTERRUPT_FAB_CR is the register that can
switch a line between direct/non-direct mode.

GPIO_*_LO are as you might expect are the GPIO block registers


> 
>>
>> 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.

Yeah, that was my gut feeling too.

> 
> 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.

Thanks.

> 
>> 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 am inclined to not allow that in all honesty. Because this is an FPGA,
there's configuration based on the contents of the FPGA fabric that has
to be done by something before an OS can be brought up & we have to draw
a line somewhere for what's reasonable for Linux to do.

> 
> In any case, this driver needs some serious rewriting.

Thanks for your help Marc.