Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3

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

Hi Soctt,

please see my comments.

On 06/15/2013 06:06 AM, Scott Wood wrote:
> On 06/14/2013 02:15:57 AM, Minghuan Lian wrote:
>> Add compatible "fsl,mpic-msi-v4.3" for MPIC v4.3. MPIC v4.3 contains
>> MSIIR and MSIIR1. MSIIR supports 8 MSI registers and MSIIR1 supports
>> 16 MSI registers, but uses different IBS and SRS shift. When using
>> MSIR1, the interrupt number is not consecutive. It is hard to use
>> 'msi-available-ranges' to describe the ranges of the available
>> interrupt and the ranges are related to the application, rather than
>> the description of the hardware. this patch also removes
>> 'msi-available-ranges' property.
>>
>> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
>> ---
>>  .../devicetree/bindings/powerpc/fsl/msi-pic.txt    | 49 
>> ++++++++++------------
>>  1 file changed, 22 insertions(+), 27 deletions(-)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt 
>> b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>> index 5693877..e851e93 100644
>> --- a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>> +++ b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>> @@ -1,26 +1,23 @@
>>  * Freescale MSI interrupt controller
>>
>>  Required properties:
>> -- compatible : compatible list, contains 2 entries,
>> +- compatible : compatible list, may contains one or two entries,
>>    first is "fsl,CHIP-msi", where CHIP is the processor(mpc8610, 
>> mpc8572,
>> -  etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" depending on
>> -  the parent type.
>> +  etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" or
>> +  "fsl,mpic-msi-v4.3" depending on the parent type and version. If mpic
>> +  version is 4.3, the number of MSI registers is increased to 16, 
>> MSIIR1 is
>> +  provided to access these 16 registers, compatible "fsl,mpic-msi-v4.3"
>> +  should be used.
>
> Why "one or two"?  What does it look like in the case where there's 
> just one?
>
[Minghuan] The original doc said 'contains 2 entries', but I notcie 
pq3-mpic.dtsi and qoriq-mpic.dtsi have only one entry "fsl,mpic-msi", do 
not have "fsl,CHIP-msi".
for example:
mpc8610_hpcd.dts: compatible = "fsl,mpc8610-msi", "fsl,mpic-msi";
fsl/qoriq-mpic.dtsi:  compatible = "fsl,mpic-msi"

Maybe I should say " For some platforms, "fsl,CHIP-msi' is optional."

>>  - reg : It may contain one or two regions. The first region should 
>> contain
>>    the address and the length of the shared message interrupt 
>> register set.
>> -  The second region should contain the address of aliased MSIIR 
>> register for
>> -  platforms that have such an alias.
>> -
>> -- msi-available-ranges: use <start count> style section to define which
>> -  msi interrupt can be used in the 256 msi interrupts. This property is
>> -  optional, without this, all the 256 MSI interrupts can be used.
>> -  Each available range must begin and end on a multiple of 32 (i.e.
>> -  no splitting an individual MSI register or the associated PIC 
>> interrupt).
>> +  The second region should contain the address of aliased MSIIR or 
>> MSIIR1
>> +  register for platforms that have such an alias, if using MSIIR1, 
>> the second
>> +  region must be added because different MSI group has different 
>> MSIRR1 offset.
>
> Why are you removing msi-available-ranges?  It's not valid for MPIC 
> v4.3, but it's still valid for older MPICs.  It should move to the 
> optional section, though.
[Minghuan] Because I would like to add kernel parameter 'msiregs' 
instead of "msi-available-ranges", for all the MPICs, we will have a 
uniform way to configure
For older MPICs, we can use "msi-available-ranges" and "msiregs", but 
for MPCI4.3 we can only use "msiregs", this may easily lead to confusion.
>>  - interrupts : each one of the interrupts here is one entry per 32 
>> MSIs,
>>    and routed to the host interrupt controller. the interrupts should
>> -  be set as edge sensitive.  If msi-available-ranges is present, only
>> -  the interrupts that correspond to available ranges shall be present.
>> +  be set as edge sensitive.
>>
>>  - interrupt-parent: the phandle for the interrupt controller
>>    that services interrupts for this device. for 83xx cpu, the 
>> interrupts
>> @@ -39,20 +36,18 @@ Optional properties:
>>
>>  Example:
>>      msi@41600 {
>> -        compatible = "fsl,mpc8610-msi", "fsl,mpic-msi";
>> -        reg = <0x41600 0x80>;
>> -        msi-available-ranges = <0 0x100>;
>> -        interrupts = <
>> -            0xe0 0
>> -            0xe1 0
>> -            0xe2 0
>> -            0xe3 0
>> -            0xe4 0
>> -            0xe5 0
>> -            0xe6 0
>> -            0xe7 0>;
>> -        interrupt-parent = <&mpic>;
>> -    };
>> +    compatible = "fsl,mpic-msi";
>> +    reg = <0x41600 0x200 0x44140 4>;
>
> Why 0x200?
>
[Minghuan] The offsets of the MSIA registers are from 0x41600 to 
0x417ff, and the size is 0x200.
offset 0x41600-0x4170 are MSIIRA1-7.
0x41720 is MSISRA,
0x41750 is MSIIR.
The others are reserved.

-Scott