[+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} > } > }) > } > > 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 Al and Darren may add to that as they have more insights. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
