On 24/08/2023 13:32, Binbin Zhou wrote:
> Hi Krzysztof:
>
> Thanks for your detailed reply.
>
> On Tue, Aug 22, 2023 at 4:30 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>
>> On 22/08/2023 10:13, Binbin Zhou wrote:
>>> Hi Krzysztof:
>>>
>>> Thanks for your detailed reply.
>>>
>>> On Tue, Aug 22, 2023 at 1:44 PM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>>>
>>>> On 21/08/2023 08:13, Binbin Zhou wrote:
>>>>> Since commit f4dee5d8e1fa ("dt-bindings: interrupt-controller: Add
>>>>> Loongson-2K1000 LIOINTC"), the loongson liointc supports configuring
>>>>> routes for 64-bit interrupt sources.
>>>>>
>>>>> For liointc-2.0, we need to define two liointc nodes in dts, one for
>>>>> "0-31" interrupt sources and the other for "32-63" interrupt sources.
>>>>> This applies to mips Loongson-2K1000.
>>>>>
>>>>> Unfortunately, there are some warnings about "loongson,liointc-2.0":
>>>>> 1. "interrupt-names" should be "required", the driver gets the parent
>>>>> interrupts through it.
>>>>
>>>> No, why? Parent? This does not make sense.
>>>
>>> This was noted in the v1 patch discussion. The liointc driver now gets
>>> the parent interrupt via of_irq_get_byname(), so I think the
>>> "interrupt-names" should be "required".
>>
>> of_irq_get_byname() does not give you parent interrupt, but the
>> interrupt. Why do you need parent interrupt and what is it?
>>
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-loongson-liointc.c?h=v6.5-rc6#n345
>>>
>>> static const char *const parent_names[] = {"int0", "int1", "int2", "int3"};
>>>
>>> for (i = 0; i < LIOINTC_NUM_PARENT; i++) {
>>> parent_irq[i] = of_irq_get_byname(node, parent_names[i]);
>>> if (parent_irq[i] > 0)
>>> have_parent = TRUE;
>>> }
>>> if (!have_parent)
>>> return -ENODEV;
>>
>> How requiring parents interrupt is related to other changes in this
>> file? One logical change, one patch.
>
> Yes, that was my mistake, whether or not the interrupt-names need to
> be "required" is another issue. It does not cause a check warning.
> I'll think about it some more.
>>
>> Anyway why did you do it and take it by names? Names here are basically
>> useless if they match indices, so just get interrupt by indices.
>
> There is a match between interrupts, interrupt names and interrupt maps:
>
> interrupt->interrupt name->interrupt map
> 2->int0->int_map[0]
> 3->int1->int_map[1]
> 4->int2->int_map[2]
> 5->int3->int_map[3]
>
> As part of the 2k1000 liointc1 node:
>
> liointc1: interrupt-controller@1fe11440 {
> ....
> interrupt-parent = <&cpuintc>;
> interrupts = <3>;
> interrupt-names = "int1";
>
> loongson,parent_int_map = <0x00000000>, /* int0 */
How did you sneak this property? The version - v2 - which was reviewed
by Rob:
https://lore.kernel.org/all/20190905144316.12527-7-jiaxun.yang@xxxxxxxxxxx/
did not have it.
Now v3 suddenly appears with Rob's review and this property:
https://lore.kernel.org/all/20200112081416.722218-4-jiaxun.yang@xxxxxxxxxxx/
Please help me understand this property appeared there and how did you
get it reviewed?
> <0xffffffff>, /* int1 */
> <0x00000000>, /* int2 */
> <0x00000000>; /* int3 */
So now you will keep bringing more hacks for a hacky property. No, this
cannot go on.
Best regards,
Krzysztof
