Re: [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3

[Lian Minghuan-b31939 <B31939@freescale.com>]

Hi Soctt,

please see my comments inline.

On 06/18/2013 08:15 AM, Scott Wood wrote:
> On 06/16/2013 10:00:01 PM, Lian Minghuan-b31939 wrote:
>> Hi Scott,
>>
>> please see my comments inline.
>>
>> On 06/15/2013 06:09 AM, Scott Wood wrote:
>>> On 06/14/2013 02:15:56 AM, Minghuan Lian wrote:
>>>> diff --git a/arch/powerpc/sysdev/fsl_msi.h 
>>>> b/arch/powerpc/sysdev/fsl_msi.h
>>>> index 8225f86..43a9d99 100644
>>>> --- a/arch/powerpc/sysdev/fsl_msi.h
>>>> +++ b/arch/powerpc/sysdev/fsl_msi.h
>>>> @@ -16,7 +16,7 @@
>>>>  #include <linux/of.h>
>>>>  #include <asm/msi_bitmap.h>
>>>>
>>>> -#define NR_MSI_REG        8
>>>> +#define NR_MSI_REG        16
>>>>  #define IRQS_PER_MSI_REG    32
>>>>  #define NR_MSI_IRQS    (NR_MSI_REG * IRQS_PER_MSI_REG)
>>>
>>> I don't see where you update all_avail in fsl_of_msi_probe.
>>>
>>> We should also be bounds-checking the contents of msi-available-ranges.
>>> Currently it looks like we just silently overrun the bitmap if we 
>>> get bad
>>> input from the device tree.
>>>
>> [Minghuan] all_avail definition: static const u32 all_avail[] = { 0, 
>> NR_MSI_IRQS };
>> When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to 
>> 16*32, and all_avail also is updated.
>
> That's my point.  It shouldn't change for older hardware.
[Minghaun] the older hardware has 8 registers, mipcv4.3 has 16 
registers. If we do not use 16*32 bitmap to indicate 8*32 irqs.(this way 
just only wastes some memory and has no other harm)
we have two choice I think.
1. Use a variable assigned value 8 or 16 based on compatible, then 
dynamically create bitmap
2. Add a new file for mpic v4.3.
What do you think?
>
>> Before calling fsl_msi_setup_hwirq(), the code has checked 
>> 'msi-available-ranges',  only the interrupts lied in 
>> 'msi-available-ranges' will be initialized by call 
>> fsl_msi_setup_hwirq() , and the corresponding bitmap will be freed. I 
>> moved msi_bitmap_free_hwirqs() to fsl_msi_setup_hwirq(), because the 
>> code would generate different bitmap when using MSIIR or MSIIR1.
>
> And what happens if msi-available-ranges is bad, and refers to 
> non-existent MSIs past the end of the bitmap?
[Minghuan] If msi-available-ranges is bad,  the below code will get error.
virt_msir = irq_of_parse_and_map(dev->dev.of_node, irq_index);
And the related error will be printed out and fsl_msi_setup_hwirq() will 
return error directly. There is no chance to set non-existent MSIs past 
the end of the bitmap.

>
> -Scott