Hi Al,
Thanks for your feedback, reply inline.
On 03/28/2017 02:56 AM, Al Stone wrote:
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
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.
I will fix it :)
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.
Thanks for the confirmation.
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.
"num-pins" is used for mbigen DT support:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/interrupt-controller/hisilicon,mbigen-v2.txt
I think we can use the name unchanged and adding more comments
as you suggested, will cc you for this updated patch.
Thanks
Hanjun
--
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
