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

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

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:
>> @@ -421,10 +440,29 @@ static int fsl_of_msi_probe(struct 
>> platform_device *dev)
>>          }
>>          msi->msiir_offset =
>>              features->msiir_offset + (res.start & 0xfffff);
>> +
>> +        /*
>> +         * First read the MSIIR/MSIIR1 offset from dts
>> +         * If failure use the hardcode MSIIR offset
>
> "On failure"
>
[Minghuan] OK, thanks.
>> +         */
>> +        if (of_address_to_resource(dev->dev.of_node, 1, &msiir))
>> +            msi->msiir_offset = features->msiir_offset +
>> +                        (res.start & MSIIR_OFFSET_MASK);
>> +        else
>> +            msi->msiir_offset = msiir.start & MSIIR_OFFSET_MASK;
>>      }
>>
>>      msi->feature = features->fsl_pic_ip;
>>
>> +    if (of_device_is_compatible(dev->dev.of_node, 
>> "fsl,mpic-msi-v4.3")) {
>> +        msi->srs_shift = MSIIR1_SRS_SHIFT;
>> +        msi->ibs_shift = MSIIR1_IBS_SHIFT;
>> +
>> +    } else {
>> +        msi->srs_shift = MSIIR_SRS_SHIFT;
>> +        msi->ibs_shift = MSIIR_IBS_SHIFT;
>> +    }
>
> Remove the blank line just before the "} else {".
>
[Minghuan] OK, Thanks.
>> 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.

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.

> -Scott