Re: [PATCH v2 1/1] dt-bindings: interrupt-controller: fsl,ls-extirq: workaround wrong interrupt-map number

[Krzysztof Kozlowski]

On 04/10/2024 17:36, Frank Li wrote:
> On Fri, Oct 04, 2024 at 08:43:23AM +0200, Krzysztof Kozlowski wrote:
>> On Thu, Oct 03, 2024 at 05:43:15PM -0400, Frank Li wrote:
>>> The driver(drivers/irqchip/irq-ls-extirq.c) have not use standard DT
>>> function to parser interrupt-map. So it doesn't consider '#address-size'
>>> in parent interrupt controller, such as GIC.
>>>
>>> When dt-binding verify interrupt-map, item data matrix is spitted at
>>> incorrect position. So cause below warning:
>>>
>>> arch/arm64/boot/dts/freescale/fsl-ls1088a-qds.dtb: interrupt-controller@14:
>>> interrupt-map: [[0, 0, 1, 0, 0, 4, 1, 0], [1, 0, 1, 4, 2, 0, 1, 0], ...
>>> is too short
>>>
>>> Reduce minItems and maxItems to workaround this warning for
>>> 'fsl,ls1088a-extirq', 'fsl,ls2080a-extirq' and fsl,lx2160a-extirq.
>>> Other keep the same restriction.
>>>
>>> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
>>> ---
>>> Change from v1 to v2
>>> - remove duplicate function in commit message
>>> - only reduce miniItems for after 1088a chips
>>> - maxItems change to 9. Otherwise report too long.
>>> ---
>>>  .../interrupt-controller/fsl,ls-extirq.yaml   | 27 +++++++++++++++++--
>>>  1 file changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.yaml b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.yaml
>>> index 199b34fdbefc4..1bfced6ed620c 100644
>>> --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.yaml
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.yaml
>>> @@ -82,14 +82,37 @@ allOf:
>>>              enum:
>>>                - fsl,ls1043a-extirq
>>>                - fsl,ls1046a-extirq
>>> +    then:
>>> +      properties:
>>> +        interrupt-map:
>>> +          minItems: 12
>>> +          maxItems: 12
>>> +        interrupt-map-mask:
>>> +          items:
>>> +            - const: 0xf
>>> +            - const: 0
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>>                - fsl,ls1088a-extirq
>>>                - fsl,ls2080a-extirq
>>>                - fsl,lx2160a-extirq
>>> +# The driver(drivers/irqchip/irq-ls-extirq.c) have not use standard DT
>>> +# function function to parser interrupt-map. So it doesn't consider
>>
>> Same issue as last time, double function.
>>
>> Please run scripts/checkpatch.pl and fix reported warnings. Then please
>> run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
>> Some warnings can be ignored, especially from --strict run, but the code
>> here looks like it needs a fix. Feel free to get in touch if the warning
>> is not clear.
> 
> Thanks, I forget add --strict this time.
> 
>>
>>
>>> +# '#address-size' in parent interrupt controller, such as GIC.
>>> +#
>>> +# When dt-binding verify interrupt-map, item data matrix is spitted at
>>> +# incorrect position. Reduce minItems and maxItems to workaround this
>>> +# problem.
>>> +
>>>      then:
>>>        properties:
>>>          interrupt-map:
>>> -          minItems: 12
>>> -          maxItems: 12
>>> +          minItems: 8
>>> +          maxItems: 9
>>
>> Are you sure it works? I see 12 items in fsl-ls1088a.dtsi.
> 
> interrupt-map =
>    <0 0 &gic GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
>    <1 0 &gic GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
>    ...
>    <11 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> 
> Total 12*6 = 72 data.
> 
> Normal each row should be 6 data.
> 
> but when GIC have #address-size, <2>,  dt-schemal split at at (6+2=8).
> 
> "interrupt-map: [[0, 0, 1, 0, 0, 4, 1, 0], [1, 0, 1, 4, 2, 0, 1, 0]"
> 
> So 72/8 = 9, I just realize it can divide to whole number.  so minItems
> can be set 9 also.

I read it three times but I cannot parse it.

I cannot translate above interrupt-map to anything meaningful, so it
looks to me that not only address-cells is ignored but entire format is
different from what the spec asks.

This is not some ancient 15 year old code, but was added in 2019.

You should not add fake constraints to a valid property, because what if
other system implements this correctly?

Best regards,
Krzysztof