Date:   Mon, 27 Mar 2017 12:24:45 +0000
From:   Gabriele Paoloni <gabriele.paoloni@...wei.com>
To:     Marc Zyngier <marc.zyngier@....com>,
        John Garry <john.garry@...wei.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        "Guohanjun (Hanjun Guo)" <guohanjun@...wei.com>
CC:     "Rafael J. Wysocki" <rafael@...nel.org>,
        "Yimin (Leo)" <yimin@...wei.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Linuxarm <linuxarm@...wei.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Sinan Kaya <okaya@...eaurora.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        Hanjun Guo <hanjun.guo@...aro.org>,
        "Tomasz Nowicki" <tn@...ihalf.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "majun (F)" <majun258@...wei.com>
Subject: RE: [PATCH v9 15/15] irqchip: mbigen: Add ACPI support

Hi Marc Many thanks for your comments

> -----Original Message-----
> From: linuxarm-bounces@...wei.com [mailto:linuxarm-bounces@...wei.com]
> 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@...r.kernel.org; Sinan Kaya; linux-acpi@...r.kernel.org; Hanjun
> Guo; Tomasz Nowicki; Thomas Gleixner; linux-arm-
> kernel@...ts.infradead.org
> 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@...aro.org>
> >>>
> >>> 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...

Many Thanks
Gab


> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...
> _______________________________________________
> linuxarm mailing list
> linuxarm@...wei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm

Powered by blists - more mailing lists