Re: [PATCH v9 15/15] irqchip: mbigen: Add ACPI support

[Al Stone]

On 03/27/2017 09:27 AM, Lorenzo Pieralisi wrote:
> [+Al,Darren to comment on _DSD review process]
> 
> On Mon, Mar 27, 2017 at 12:24:45PM +0000, Gabriele Paoloni wrote:
>> Hi Marc Many thanks for your comments
>>
>>> -----Original Message-----
>>> From: linuxarm-bounces@xxxxxxxxxx [mailto:linuxarm-bounces@xxxxxxxxxx]
>>> On Behalf Of Marc Zyngier
>>> Sent: 27 March 2017 09:47
>>> To: John Garry; Lorenzo Pieralisi; Guohanjun (Hanjun Guo)
>>> Cc: Rafael J. Wysocki; Yimin (Leo); Greg KH; Linuxarm; linux-
>>> kernel@xxxxxxxxxxxxxxx; Sinan Kaya; linux-acpi@xxxxxxxxxxxxxxx; Hanjun
>>> Guo; Tomasz Nowicki; Thomas Gleixner; linux-arm-
>>> kernel@xxxxxxxxxxxxxxxxxxx
>>> Subject: Re: [PATCH v9 15/15] irqchip: mbigen: Add ACPI support
>>>
>>> Hanjun, John,
>>>
>>> On 22/03/17 14:12, John Garry wrote:
>>>> On 21/03/2017 14:45, Lorenzo Pieralisi wrote:
>>>>> On Tue, Mar 07, 2017 at 08:40:10PM +0800, Hanjun Guo wrote:
>>>>>> From: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
>>>>>>
>>>>>> With the preparation of platform msi support and interrupt producer
>>>>>> in DSDT, we can add mbigen ACPI support now.
>>>>>>
>>>>>> We are using Interrupt resource type in _CRS methd to indicate
>>> number
>>>>>> of irq pins instead of num_pins in DT to avoid _DSD usage in this
>>> case.
>>>>>>
>>>>>> For mbigen,
>>>>>>     Device(MBI0) {
>>>>>>           Name(_HID, "HISI0152")
>>>>>>           Name(_UID, Zero)
>>>>>>           Name(_CRS, ResourceTemplate() {
>>>>>>                   Memory32Fixed(ReadWrite, 0xa0080000, 0x10000)
>>>>>> 		  Interrupt(ResourceProducer,...) {12,14,....}
>>>>>
>>>>> What do these interrupt numbers represent ? This looks wrong to me.
>>>>> An interrupt descriptor is there to describe the interrupts a device
>>>>> can generate; you are using it just to add a "standard" (that is
>>>>> not standard at all) way of counting the number of vectors allocated
>>>>> to this specific chip and that's just wrong.
>>>>>
>>>>
>>>> As I understand, the count of interrupts we are declaring for the
>>> mbigen
>>>> is the same as the sum of interrupts for that mbigen's children.
>>>>
>>>> So at the point we probe the mbigen, can we just deference the
>>> children
>>>> to count their interrupts, and use this as the #msis?
>>>>
>>>>> Can't you use something like Agustin did in the QCOM combiner:
>>>>>
>>>>> drivers/irqchip/qcom-irq-combiner.c
>>>>>
>>>>> to detect the MSI vector length (ie by describing the MBIgen through
>>>>> generic registers and use the bit width to compute the vector
>>>>> lenght) ? I am not sure how feasible it is given that my knowledge
>>>>> of MBIgen is pretty poor.
>>>>>
>>>>> I understand we want to avoid _DSD properties but we should not
>>>>> work around standard bindings to achieve that goal.
>>>>>
>>>>
>>>> We use "num-pins" for dt solution, but it is not so welcome here.
>>>
>>> Well, this device is already completely out of any standard description
>>> on the ACPI side. And given that it bloats both the ACPI tables and the
>>> kernel data structures, I can only suggest that you take advantage of
>>> _DSD here, as misusing the standard properties is not something that we
>>> should condone. It will also make the driver more manageable, as it
>>> will
>>> use similar properties on both firmware implementations.
>>>
>>> I feel like I need to stress the urgency here. We're at -rc4, and still
>>> with unsolved issues. None of us want to miss the next merge window.
>>>
>>
>> As follow up our guys would work on a solution whose ACPI table looks
>> like the following one:
>>
>> For mbigen,
>>     Device(MBI0) {
>>           Name(_HID, "HISI0152")
>>           Name(_UID, Zero)
>>           Name(_CRS, ResourceTemplate() {
>>                   Memory32Fixed(ReadWrite, 0xa0080000, 0x10000)
>>           })
>>
>>     Name(_DSD, Package () {
>>       ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>       Package ()
>>       {
>>         Package () {"num-pins", xxx}
>>       }
>>     })
>>     }

Please fix the indentation here; the ASL parser doesn't care, but I don't
see '}'s as clearly as it does :).  In particular, _DSD usage *must* be for
a device and it does not look like that if one doesn't carefully count all
the parentheses and braces.

>>
>> For devices,
>>    Device(COM0) {
>>           Name(_HID, "ACPIIDxx")
>>           Name(_UID, Zero)
>>           Name(_CRS, ResourceTemplate() {
>>                  Memory32Fixed(ReadWrite, 0xb0030000, 0x10000)
>> 		 Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12}
>>           })
>>     }
>>
>>
>> Marc, Lorenzo if you are ok with the above we will submit v10 based on this...
> 
> I am ok with it. I am not 100% up-to-date on what's the status on _DSD
> bindings/review/guidelines but it would be certainly a good idea to
> kickstart the process for MBIgen which basically means following this
> as far as I know (and post to the relevant mailing list):
> 
> https://github.com/ahs3/dsd/blob/master/documentation/process_rules.txt

So, let's correct this bit: consider that URL deprecated, please, and refer
instead to the text that was agreed upon and in the kernel tree:

	Documentation/acpi/DSD-properties-rules.txt

> Al and Darren may add to that as they have more insights.

Since this use of _DSD is very specific to this device, and this device
only, I don't have any real objections.  I will say that "num-pins" is
not terribly descriptive so it would be really good to either use a much
more descriptive name or add plenty of commentary in the code -- and
preferably both.  I would recommend including the ASL in the code comments,
with a description of how the property is to be used and what it means.

> I would like to send IORT patches to Catalin as soon as possible so
> as Marc pointed out the sooner we sort this out the better.
> 
> Thanks,
> Lorenzo
> 


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@xxxxxxxxxx
-----------------------------------