Date:   Wed, 14 Feb 2018 16:52:59 +0000
From:   John Garry <john.garry@...wei.com>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>
CC:     <mika.westerberg@...ux.intel.com>, <rafael@...nel.org>,
        <rjw@...ysocki.net>, <hanjun.guo@...aro.org>, <robh+dt@...nel.org>,
        <bhelgaas@...gle.com>, <arnd@...db.de>, <mark.rutland@....com>,
        <olof@...om.net>, <dann.frazier@...onical.com>,
        <andy.shevchenko@...il.com>, <robh@...nel.org>, <joe@...ches.com>,
        <benh@...nel.crashing.org>, <linux-pci@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
        <linuxarm@...wei.com>, <minyard@....org>,
        <devicetree@...r.kernel.org>, <linux-arch@...r.kernel.org>,
        <rdunlap@...radead.org>
Subject: Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices
 before scanning

On 14/02/2018 16:16, Lorenzo Pieralisi wrote:
> On Wed, Feb 14, 2018 at 01:45:31AM +0800, John Garry wrote:
>> On some platforms (such as arm64-based hip06/hip07), access to legacy
>> ISA/LPC devices through access IO space is required, similar to x86
>> platforms. As the I/O for these devices are not memory mapped like
>> PCI/PCIE MMIO host bridges, they require special low-level device
>> operations through some host to generate IO accesses, i.e. a non-
>> transparent bridge.
>>
>> Through the logical PIO framework, hosts are able to register address
>> ranges in the logical PIO space for IO accesses. For hosts which require
>> a LLDD to generate the IO accesses, through the logical PIO framework
>> the host also registers accessors as a backend to generate the physical
>> bus transactions for IO space accesses (called indirect IO).
>>
>> When describing the indirect IO child device in APCI tables, the IO
>> resource is the host-specific address for the child (generally a
>> bus address).
>> An example is as follows:
>>   Device (LPC0) {
>>     Name (_HID, "HISI0191")  // HiSi LPC
>>     Name (_CRS, ResourceTemplate () {
>>       Memory32Fixed (ReadWrite, 0xa01b0000, 0x1000)
>>     })
>>   }
>>
>>   Device (LPC0.IPMI) {
>>     Name (_HID, "IPI0001")
>>     Name (LORS, ResourceTemplate() {
>>       QWordIO (
>>         ResourceConsumer,
>> 	MinNotFixed,     // _MIF
>> 	MaxNotFixed,     // _MAF
>> 	PosDecode,
>> 	EntireRange,
>> 	0x0,             // _GRA
>> 	0xe4,            // _MIN
>> 	0x3fff,          // _MAX
>> 	0x0,             // _TRA
>> 	0x04,            // _LEN
>> 	, ,
>> 	BTIO
>>       )
>>     })
>>
>> Since the IO resource for the child is a host-specific address,
>> special translation are required to retrieve the logical PIO address
>> for that child.
>

Hi Lorenzo,

> The problem I have with this patchset and with pretending that the ACPI
> bits are generic is that the rules used to translate resources (I am
> referring to LPC0.IPMI above) are documented _nowhere_ which means that
> making this series generic code is just wishful thinking - there are no
> bindings backing it, it will never ever be used on a platform different
> from the one you are pushing this code for and I stated this already.
>

Right, it is working on the presumption that this is how all "indirectio 
IO" hosts and children should/would be described in DSDT.

> Reworded differently - this is a Hisilicon driver it is not generic ACPI
> code; I can't see how it can be used on a multitude of platforms unless
> you specify FW level bindings.
>
>> To overcome the problem of associating this logical PIO address
>> with the child device, a scan handler is added to scan the ACPI
>> namespace for known indirect IO hosts. This scan handler creates an
>> MFD per child with the translated logical PIO address as it's IO
>> resource, as a substitute for the normal platform device which ACPI
>> would create during device enumeration.
>>
>> Signed-off-by: John Garry <john.garry@...wei.com>
>> Signed-off-by: Zhichang Yuan <yuanzhichang@...ilicon.com>
>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@...wei.com>
>> ---
>>  drivers/acpi/arm64/Makefile          |   1 +
>>  drivers/acpi/arm64/acpi_indirectio.c | 250 +++++++++++++++++++++++++++++++++++
>
> See above (and I do not understand what arm64 has to do with it).

Nothing apart from only being used by arm64 platforms today, which is 
circumstantial.

>
> I understand you need to find a place to add the:
>
> acpi_indirect_io_scan_init()
>
> to be called from core ACPI code because ACPI can't handle probe
> dependencies in any other way but other than that this patch is
> a Hisilicon ACPI driver - there is nothing generic in it (or at
> least there are no standard bindings to make it so).
>
> Whether a callback from ACPI core code (acpi_scan_init()) to a driver
> specific hook is sane or not that's the question and the only reason
> why you want to add this in drivers/acpi/arm64 rather than, say,
> drivers/bus (as you do for the DT driver).
>
> I do not know Rafael's opinion on the above, I would like to help
> you make forward progress but please understand my concerns, mostly
> on FW side.
>

I did mention an alternative in my "ping" in v12 patch 7/9 (Feb 1), but 
no response to this specific note so I kept on the same path.

Here's what I then wrote:
"I think another solution - which you may prefer - is to avoid adding
this scan handler (and all this other scan code) and add a check like
acpi_is_serial_bus_slave() [which checks the device parent versus a list 
of known indirectIO hosts] to not enumerate these children, and do it 
from the LLDD host probe instead (https://lkml.org/lkml/2017/6/16/250)"

Please consider this.

> Thanks,
> Lorenzo
>
>>  drivers/acpi/internal.h              |   5 +
>>  drivers/acpi/scan.c                  |   1 +
>>  4 files changed, 257 insertions(+)
>>  create mode 100644 drivers/acpi/arm64/acpi_indirectio.c
>>

Cheers,
John

Powered by blists - more mailing lists