Message-ID: <61868c38b5f6989d8e2c4173e2a069ae@codeaurora.org>
Date:   Tue, 29 Nov 2016 10:18:31 -0500
From:   Agustin Vega-Frias <agustinv@...eaurora.org>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc:     linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, rjw@...ysocki.net,
        lenb@...nel.org, tglx@...utronix.de, jason@...edaemon.net,
        marc.zyngier@....com, timur@...eaurora.org, cov@...eaurora.org,
        agross@...eaurora.org, harba@...eaurora.org, jcm@...hat.com,
        msalter@...hat.com, mlangsdo@...hat.com, ahs3@...hat.com,
        astone@...hat.com, graeme.gregory@...aro.org, guohanjun@...wei.com,
        charles.garcia-tobin@....com
Subject: Re: [PATCH V7 2/3] ACPI: Add support for ResourceSource/IRQ domain
 mapping

Hi Lorenzo,

On 2016-11-29 07:11, Lorenzo Pieralisi wrote:
> Hi Agustin,
> 
> On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote:
>> Hi Rafael,
>> 
>> Can you chime in on Lorenzo's feedback and the discussion below?
>> It would be great if you can comment on the reason ACPI does things
>> in a certain way.
>> 
>> Hi Lorenzo,
>> 
>> On 2016-11-25 06:40, Lorenzo Pieralisi wrote:
>> >Hi Agustin,
>> >
>> >On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote:
>> >
>> >[...]
>> >
>> >>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>> >>>  {
>> >>>  	struct acpi_resource_irq *irq;
>> >>>  	struct acpi_resource_extended_irq *ext_irq;
>> >>> +	struct fwnode_handle *src;
>> >>>
>> >>>  	switch (ares->type) {
>> >>>  	case ACPI_RESOURCE_TYPE_IRQ:
>> >>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>> >>>  			acpi_dev_irqresource_disabled(res, 0);
>> >>>  			return false;
>> >>>  		}
>> >>> -		acpi_dev_get_irqresource(res, irq->interrupts[index],
>> >>> +		acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
>> >>>  					 irq->triggering, irq->polarity,
>> >>>  					 irq->sharable, true);
>> >>>  		break;
>> >>> @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>> >>>  			acpi_dev_irqresource_disabled(res, 0);
>> >>>  			return false;
>> >>>  		}
>> >>> -		acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
>> >>> +		src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
>> >>
>> >>Is there a reason why we need to do the domain look-up here ?
>> 
>> Because we need to pass the resource down to acpi_dev_get_irqresource
>> which does the mapping through acpi_register_irq/acpi_register_gsi.
>> 
>> >>
>> >>I would like to understand if, by reshuffling the code (and by
>> >>returning
>> >>the resource_source to the calling code - somehow), it would be
>> >>possible
>> >>to just mirror what the OF code does in of_irq_get(), namely:
>> >>
>> >>(1) parse the irq entry -> of_irq_parse_one()
>> >>(2) look the domain up -> irq_find_host()
>> >>(3) create the mapping -> irq_create_of_mapping()
>> >>
>> >>You wrote the code already, I think it is just a matter of shuffling
>> >>it around (well, minus returning the resource_source to the caller
>> >>which is phandle equivalent in DT).
>> 
>> This is one area in which DT and ACPI are fundamentally different. In 
>> DT
>> once the flattened blob is expanded the data is fixed. In ACPI the 
>> data
>> returned by a method can change. In reality most methods like CRS 
>> return
>> constants, but given that per-spec they are methods the interpreter 
>> has
>> to be involved, which makes it an expensive operation. I believe that 
>> is
>> the reason the resource parsing code in ACPI attempts all mappings
>> during
>> the bus scan. Rafael can you comment on this?
>> 
>> One way to do what you suggest would be to defer IRQ mapping by, e.g.,
>> populating res->start with the HW IRQ number and res->end with the
>> fwnode.
>> That way we can avoid having to walk the resource buffer when a 
>> mapping
>> is needed. I don't think that approach would deviate much more from
>> the spec from what the current ahead-of-time mapping does, but it 
>> would
>> require more changes in the core code. An alternative would be to do
>> that only for resources that fail to map.
>> 
>> >>
>> >>You abstracted away (2) and (3) behind acpi_register_irq(), that
>> >>on anything than does not use ACPI_GENERIC_GSI is just glue code
>> >>to acpi_register_gsi().
>> >>
>> >>Also, it is not a question on this patch but I ask it here because it
>> >>is related. On ACPI you are doing the reverse of what is done in
>> >>DT in platform_get_irq():
>> >>
>> >>- get the resources already parsed -> platform_get_resource()
>> >>- if they are disabled -> acpi_irq_get()
>> >>
>> >>and I think the ordering is tied to my question above because
>> >>you carry out the domain look up in acpi_dev_resource_interrupt()
>> >>so that if for any reason it fails the corresponding resource
>> >>is disabled so that we try to get it again through acpi_irq_get().
>> >>
>> >>I suspect you did it this way to make sure:
>> >>
>> >>a) keep the current ACPI IRQ parsing interface changes to a mininum
>> >>b) avoid changing the behaviour on x86/ia64; in particular, calling
>> >>   acpi_register_gsi() for the _same_ mapping (an IRQ that was already
>> >>   registered at device creation resource parsing) multiple times can
>> >>   trigger issues on x86/ia64
>> 
>> You are correct about my reasons. I wanted to keep ACPI core code
>> changes
>> to a minimum, and I also needed to work within the current
>> implementation
>> which uses the pre-converted IRQ resources.
>> 
>> >>
>> >>I think that's a reasonable approach but I wanted to get these
>> >>clarifications, I do not think you are far from getting this
>> >>done but since it is a significant change I think it is worth
>> >>discussing the points I raised above because I think the DT code
>> >>sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ
>> >>layer perspective (instead of having the domain look-up buried
>> >>inside the ACPI IRQ resource parsing API).
>> >
>> >I had another look and to achieve the above one way of doing that is to
>> >implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for
>> >!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI
>> >we would fall back to current solution for ACPI). Within acpi_irq_get()
>> >you can easily carry out the same steps (1->2->3) above in ACPI
>> >you have
>> >the code already there I think it is easy to change the
>> >acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and
>> >the interface would become identical to of_irq_get() that is an
>> >advantage to maintain it from an IRQ maintainership perspective I
>> >think,
>> >that's my opinion.
>> 
>> I think I get what you mean. I'll take a stab at implementing
>> acpi_irq_get()
>> in the way you suggest.
>> 
>> >
>> >There is still a nagging snag though. When platform devices are
>> >created, core ACPI code parse the resources through:
>> >
>> >acpi_dev_get_resources()
>> >
>> >and we _have_ to have way to avoid initializing IRQ resources that
>> >have a dependency (ie there is a resource_source pointer that is valid
>> >in their descriptors) that's easy to do if we think that's the right
>> >thing to do and can hardly break current code (which ignores the
>> >resource_source altogether).
>> 
>> I'd rather keep the core code as-is with regard to the ahead-of-time
>> conversion. Whether a resource source is available at the time of
>> the bus
>> scan should be transparent to the code in drivers/acpi/resource.c, and
>> we need the initialization as a disabled resource to signal the need
>> to retry anyway.
> 
> Yes, exactly that's the nub. Your current code works, I am trying to
> make it more modular and similar to the DT/irqdomain IRQ look-up path,
> which has its advantages.
> 
> There are two options IMHO:
> 
> - always disable the resource if it has a resource_source dependency 
> and defer
>   its parsing to acpi_irq_get() (where you can easily implement steps
> 1-2-3 above).
>   What I wanted to say is that, by disabling the resource if it has a
>   resource_source dependency you can't break x86/ia64 (it is ignored at
>   present - hopefully there is nothing that we are not aware of behind
>   that choice). On x86/ia64 acpi_irq_get() would be an empty stub.
>   This way you would keep the irqdomain look-up out of the ACPI 
> resource
>   parsing API, correct ?
> - keep code as-is

I plan to work on V8 today to address the point you raised w.r.t. making
it look more like DT for maintainability. I will leave other things 
as-is
with an understanding that we might revisit based on Rafael's feedback

Thanks,
Agustin

> 
> Your point on _CRS being _current_ resource setting is perfectly valid
> so platform_get_resource() in platform_get_irq() must always take
> precedence over acpi_irq_get() (which should just apply to disabled
> resources), I am not sure that doing it the other way around is safe.
> 
>> Rafael, do you have any other suggestions/feedback on how to go about
>> doing this?
> 
> Yes, comments very appreciated, these changes are not trivial and need
> agreement.
> 
> Thanks,
> Lorenzo
> 
>> 
>> Thanks,
>> Agustin
>> 
>> >
>> >It is an important difference with DT probing, where the IRQ
>> >resources are only created if the domain reference (ie interrupt
>> >controller phandle) is satisfied at of_device_alloc() time
>> >(see of_device_alloc()).
>> >
>> >Thoughts ? Please let me know, the code to implement what I say
>> >is already in these patches, it is just a matter of reshuffling it.
>> >
>> >Thanks !
>> >Lorenzo
>> 
>> --
>> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
>> Technologies, Inc.
>> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
>> Linux Foundation Collaborative Project.

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Powered by blists - more mailing lists