Message-ID: <e12155805a9d92d3f5edc81395a76f48@codeaurora.org>
Date:   Mon, 28 Nov 2016 17:40:24 -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 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.

Rafael, do you have any other suggestions/feedback on how to go about
doing this?

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.

Powered by blists - more mailing lists