lkml.org 
[lkml]   [2018]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
    From
    Date
    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@huawei.com>
    >> Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
    >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.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



    \
     
     \ /
      Last update: 2018-02-14 17:54    [W:3.001 / U:0.304 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site