On 14/02/2018 16:16, Lorenzo Pieralisi wrote:

On Wed, Feb 14, 2018 at 01:45:31AM +0800, John Garry wrote:

quoted

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.

quoted

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 <redacted>
Signed-off-by: Zhichang Yuan <redacted>
Signed-off-by: Gabriele Paoloni <redacted>
---
 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

quoted

 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