| Message ID | 1495712248-5232-6-git-send-email-gabriele.paoloni@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | LPC: legacy ISA I/O support | expand |
Hi Gabriele,
[auto build test ERROR on linus/master]
[also build test ERROR on v4.12-rc2 next-20170525]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Gabriele-Paoloni/LPC-legacy-ISA-I-O-support/20170526-033719
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64
All errors (new ones prefixed by >>):
drivers/built-in.o: In function `acpi_scan_init':
>> (.init.text+0x6742): undefined reference to `acpi_indirectio_scan_init'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Gab, On Thu, May 25, 2017 at 12:37:26PM +0100, Gabriele Paoloni wrote: > From: Gabriele <gabriele.paoloni@huawei.com> > > On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access > I/O with some special host-local I/O ports known on x86. As their I/O > space are not memory mapped like PCI/PCIE MMIO host bridges, this patch is > meant to support a new class of I/O host controllers where the local IO > ports of the children devices are translated into the Indirect I/O address > space. > Through the handler attach callback, all the I/O translations are done > before starting the enumeration on children devices and the translated > addresses are replaced in the children resources. I do not understand why this is done through a scan handler and to be frank I do not see how this mechanism is supposed to be a generic kernel layer, possibly used by other platforms, when there is no notion in ACPI to cater for that. As far as I am concerned this patch code should live in the Hisilicon LPC driver - as things stand it is neither ACPI generic code nor ARM64 specific, it is code to make ACPI work like DT bindings without any ACPI binding at all; I still have some concerns from an ACPI standpoint below. > 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_indirect_pio.c | 301 +++++++++++++++++++++++++++++++++ > drivers/acpi/internal.h | 5 + > drivers/acpi/scan.c | 1 + > include/acpi/acpi_indirect_pio.h | 24 +++ > 5 files changed, 332 insertions(+) > create mode 100644 drivers/acpi/arm64/acpi_indirect_pio.c > create mode 100644 include/acpi/acpi_indirect_pio.h > > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > index 1017def..3944775 100644 > --- a/drivers/acpi/arm64/Makefile > +++ b/drivers/acpi/arm64/Makefile > @@ -1,2 +1,3 @@ > obj-$(CONFIG_ACPI_IORT) += iort.o > obj-$(CONFIG_ACPI_GTDT) += gtdt.o > +obj-$(CONFIG_INDIRECT_PIO) += acpi_indirect_pio.o > diff --git a/drivers/acpi/arm64/acpi_indirect_pio.c b/drivers/acpi/arm64/acpi_indirect_pio.c > new file mode 100644 > index 0000000..7813f73 > --- /dev/null > +++ b/drivers/acpi/arm64/acpi_indirect_pio.c > @@ -0,0 +1,301 @@ > +/* > + * ACPI support for indirect-PIO bus. > + * > + * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved. > + * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com> > + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/acpi.h> > +#include <linux/platform_device.h> > +#include <linux/logic_pio.h> > + > +#include <acpi/acpi_indirect_pio.h> > + > +ACPI_MODULE_NAME("indirect PIO"); > + > +#define INDIRECT_PIO_INFO(desc) ((unsigned long)&desc) > + > +static acpi_status acpi_count_logic_iores(struct acpi_resource *res, > + void *data) > +{ > + int *res_cnt = data; > + > + if (!acpi_dev_filter_resource_type(res, IORESOURCE_IO)) > + (*res_cnt)++; > + > + return AE_OK; > +} > + > +static acpi_status acpi_read_one_logicpiores(struct acpi_resource *res, > + void *data) > +{ > + struct acpi_resource **resource = data; > + > + if (!acpi_dev_filter_resource_type(res, IORESOURCE_IO)) { > + memcpy((*resource), res, sizeof(struct acpi_resource)); > + (*resource)->length = sizeof(struct acpi_resource); > + (*resource)->type = res->type; > + (*resource)++; > + } > + > + return AE_OK; > +} > + > +static acpi_status > +acpi_build_logicpiores_template(struct acpi_device *adev, > + struct acpi_buffer *buffer) > +{ > + acpi_handle handle = adev->handle; > + struct acpi_resource *resource; > + acpi_status status; > + int res_cnt = 0; > + > + status = acpi_walk_resources(handle, METHOD_NAME__CRS, > + acpi_count_logic_iores, &res_cnt); > + if (ACPI_FAILURE(status)) { > + dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status); > + return -EINVAL; > + } > + > + if (!res_cnt) { > + dev_err(&adev->dev, "no logic IO resources\n"); > + return -ENODEV; > + } > + > + buffer->length = sizeof(struct acpi_resource) * (res_cnt + 1); > + buffer->pointer = kzalloc(buffer->length, GFP_KERNEL); > + if (!buffer->pointer) > + return -ENOMEM; > + > + resource = (struct acpi_resource *)buffer->pointer; > + status = acpi_walk_resources(handle, METHOD_NAME__CRS, > + acpi_read_one_logicpiores, &resource); > + if (ACPI_FAILURE(status)) { > + kfree(buffer->pointer); > + dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status); > + return -EINVAL; > + } > + > + resource->type = ACPI_RESOURCE_TYPE_END_TAG; > + resource->length = sizeof(struct acpi_resource); > + > + return 0; > +} IIUC correctly this code is here to count resources and replace them with kernel specific IO offsets and I think that's wrong. ACPI devices _CRS,_PRS,_SRS are set-up by firmware and by no means should be updated with resources that are basically Linux kernel internals specific. The problem you are facing is there because there is no way in ACPI to specify in FW what you want to describe but that's not a reason to make it work by other means. [...] > + * update/set the current I/O resource of the designated device node. > + * after this calling, the enumeration can be started as the I/O resource > + * had been translated to logicial I/O from bus-local I/O. > + * > + * @adev: the device node to be updated the I/O resource; > + * @host: the device node where 'adev' is attached, which can be not > + * the parent of 'adev'; > + * > + * return 0 when successful, negative is for failure. > + */ > +int acpi_set_logic_pio_resource(struct device *child, > + struct device *hostdev) > +{ > + struct acpi_device *adev; > + struct acpi_device *host; > + struct acpi_buffer buffer; > + acpi_status status; > + int ret; > + > + if (!child || !hostdev) > + return -EINVAL; > + > + host = to_acpi_device(hostdev); > + adev = to_acpi_device(child); > + > + /* check the device state */ > + if (!adev->status.present) { > + dev_info(child, "ACPI: device is not present!\n"); > + return 0; > + } > + /* whether the child had been enumerated? */ > + if (acpi_device_enumerated(adev)) { > + dev_info(child, "ACPI: had been enumerated!\n"); > + return 0; > + } > + > + /* read the _CRS and convert as acpi_buffer */ > + status = acpi_build_logicpiores_template(adev, &buffer); > + if (ACPI_FAILURE(status)) { > + dev_warn(child, "Failure evaluating %s\n", METHOD_NAME__CRS); > + return -ENODEV; > + } > + > + /* translate the I/O resources */ > + ret = acpi_translate_logicpiores(adev, host, &buffer); > + if (ret) { > + kfree(buffer.pointer); > + dev_err(child, "Translate I/O range FAIL!\n"); > + return ret; > + } > + > + /* set current resource... */ > + status = acpi_set_current_resources(adev->handle, &buffer); I reckon that this is wrong. You are updating ACPI device resources with kernel specific built resources (ie IO space offsets) made by slicing up IO space into the kernel available virtual address space offset allocated to it. IIUC this is done to make sure platform devices contains the "translated" resources by the time they are probed, it is hard to follow and again, not correct from an ACPI standpoint. Furthermore I have no idea how this code is supposed to be used by _any_ other platform, ACPI just has no notion of the translation you are carrying out here (which mirrors what's done in DT without any firmware binding to support it) so even if any other platform has the same requirements I have no idea how people developing FW may write their bindings to match these given that they just don't exist. > + kfree(buffer.pointer); > + if (ACPI_FAILURE(status)) { > + dev_err(child, "Error evaluating _SRS (0x%x)\n", status); > + ret = -EIO; > + } > + > + return ret; > +} > + > +/* All the host devices which apply indirect-PIO can be listed here. */ > +static const struct acpi_device_id acpi_indirect_host_id[] = { > + {""}, How can this be used by any other platform other than Hisilicon LPC ? Imagine for a minute you have an ARM64 platform with an LPC bus in it and you have to write ACPI tables to describe it, you may not want to start by reading Linux kernel code to understand how to do it. This code is Hisilicon specific code (ie there is no ACPI binding to the best of my knowledge describing to FW developers an LPC binding) and on the ACPI side it makes assumptions that I am not sure are correct at all, see above. > +}; > + > +static int acpi_indirectpio_attach(struct acpi_device *adev, > + const struct acpi_device_id *id) > +{ > + struct indirect_pio_device_desc *hostdata; > + struct platform_device *pdev; > + int ret; > + > + hostdata = (struct indirect_pio_device_desc *)id->driver_data; > + if (!hostdata || !hostdata->pre_setup) > + return -EINVAL; > + > + ret = hostdata->pre_setup(adev, hostdata->pdata); > + if (!ret) { > + pdev = acpi_create_platform_device(adev, NULL); So, this is done through a scan handler for what reason ? Is this because you want this code to run before platform devices are created by ACPI core - so that you can update their resources in the struct indirect_pio_device_desc.pre_setup() method ? I suspect this is yet another probing order issue to solve but that's orthogonal to the comments I made above. To sum it up: (1) Creating an ARM64 ACPI standard kernel layer to parse something that is not an ACPI (let alone ARM64) standard does not make much sense to me, so either standard bindings are published for other partners to use them or this code belongs to Hisilicon specific LPC bus management (2) Even with (1), I have concerns about this patch ACPI resources handling, I really think it is wrong to update ACPI devices resources with something that is Linux kernel specific. I may understand building platform devices resources according to your LPC bus requirements but not updating ACPI device FW bindings with resources that are basically kernel internals. Thanks, Lorenzo > + if (IS_ERR_OR_NULL(pdev)) { > + dev_err(&adev->dev, "Create platform device for host FAIL!\n"); > + return -EFAULT; > + } > + acpi_device_set_enumerated(adev); > + ret = 1; > + } > + > + return ret; > +} > + > + > +static struct acpi_scan_handler acpi_indirect_handler = { > + .ids = acpi_indirect_host_id, > + .attach = acpi_indirectpio_attach, > +}; > + > +void __init acpi_indirectio_scan_init(void) > +{ > + acpi_scan_add_handler(&acpi_indirect_handler); > +} > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > index 66229ff..bf8aaf8 100644 > --- a/drivers/acpi/internal.h > +++ b/drivers/acpi/internal.h > @@ -31,6 +31,11 @@ void acpi_processor_init(void); > void acpi_platform_init(void); > void acpi_pnp_init(void); > void acpi_int340x_thermal_init(void); > +#ifdef CONFIG_INDIRECT_PIO > +void acpi_indirectio_scan_init(void); > +#else > +static inline void acpi_indirectio_scan_init(void) {} > +#endif > #ifdef CONFIG_ARM_AMBA > void acpi_amba_init(void); > #else > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index e39ec7b..37dd23c 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -2035,6 +2035,7 @@ int __init acpi_scan_init(void) > acpi_int340x_thermal_init(); > acpi_amba_init(); > acpi_watchdog_init(); > + acpi_indirectio_scan_init(); > > acpi_scan_add_handler(&generic_device_handler); > > diff --git a/include/acpi/acpi_indirect_pio.h b/include/acpi/acpi_indirect_pio.h > new file mode 100644 > index 0000000..efc5c43 > --- /dev/null > +++ b/include/acpi/acpi_indirect_pio.h > @@ -0,0 +1,24 @@ > +/* > + * ACPI support for indirect-PIO bus. > + * > + * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved. > + * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com> > + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#ifndef _ACPI_INDIRECT_PIO_H > +#define _ACPI_INDIRECT_PIO_H > + > +struct indirect_pio_device_desc { > + void *pdata; /* device relevant info data */ > + int (*pre_setup)(struct acpi_device *adev, void *pdata); > +}; > + > +int acpi_set_logic_pio_resource(struct device *child, > + struct device *hostdev); > + > +#endif > -- > 2.7.4 > >
Hi Gab, Rafael, On Wed, May 31, 2017 at 10:24:47AM +0000, Gabriele Paoloni wrote: [...] > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > > > index e39ec7b..37dd23c 100644 > > > --- a/drivers/acpi/scan.c > > > +++ b/drivers/acpi/scan.c > > > @@ -2035,6 +2035,7 @@ int __init acpi_scan_init(void) > > > acpi_int340x_thermal_init(); > > > acpi_amba_init(); > > > acpi_watchdog_init(); > > > + acpi_indirectio_scan_init(); Unfortunately this is becoming a pattern and we are ending up with a static ordering of "subsystems" init (even though for this LPC series it is just the Hisilicon driver that requires this call) and I am not sure I see any way of avoiding it. I think that's always been the case in x86, with fewer subsystems/kernel paths to care about, I wanted to flag this up though to check your opinion since I am not sure this is the right direction we are taking. I also think that relying on _DEP to build any dependency is not entirely a) usable (owing to legacy bindings and previous _DEP misuse) and b) compliant with ACPI bindings given that _DEP has to be used for operation regions only. Thoughts ? Thanks, Lorenzo > > > acpi_scan_add_handler(&generic_device_handler); > > > > > > diff --git a/include/acpi/acpi_indirect_pio.h > > b/include/acpi/acpi_indirect_pio.h > > > new file mode 100644 > > > index 0000000..efc5c43 > > > --- /dev/null > > > +++ b/include/acpi/acpi_indirect_pio.h > > > @@ -0,0 +1,24 @@ > > > +/* > > > + * ACPI support for indirect-PIO bus. > > > + * > > > + * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved. > > > + * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com> > > > + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com> > > > + * > > > + * This program is free software; you can redistribute it and/or > > modify > > > + * it under the terms of the GNU General Public License version 2 as > > > + * published by the Free Software Foundation. > > > + */ > > > + > > > +#ifndef _ACPI_INDIRECT_PIO_H > > > +#define _ACPI_INDIRECT_PIO_H > > > + > > > +struct indirect_pio_device_desc { > > > + void *pdata; /* device relevant info data */ > > > + int (*pre_setup)(struct acpi_device *adev, void *pdata); > > > +}; > > > + > > > +int acpi_set_logic_pio_resource(struct device *child, > > > + struct device *hostdev); > > > + > > > +#endif > > > -- > > > 2.7.4 > > > > > >
On Mon, Jun 12, 2017 at 04:57:00PM +0100, Lorenzo Pieralisi wrote: > I had a more in-depth look at this series and from my understanding > the problem are the following to manage the LPC bindings in ACPI. > > (1) Child devices of an LPC controller require special handling when > filling their resources (ie they need to be translated - in DT > that's guaranteed by the "isa" binding, in ACPI it has to be > done by new code) > (2) In DT systems, LPC child devices are created by the LPC bus > controller driver through an of_platform_populate() call with > the LPC controller node as the fwnode root. For ACPI to work > the same way there must be a way to prevent LPC children to > be enumerated in acpi_default_enumeration() something like > I2C does (and then the LPC driver would enumerate its children as > DT does) > > I am not sure how (1) and (2) can be managed with current ACPI bindings > and kernel code - I suspect it may be done by mirroring what's done > for I2C but I am not sure, that's why I CC'ed Mika (ie the LPC adapter > is matched as a platform device and it takes care of enumerating its > children - problem though is preventing enumeration from core ACPI code). Is there an example ASL showing how these LPC devices are currently presented in ACPI?
On Thu, Jun 15, 2017 at 06:01:02PM +0000, Gabriele Paoloni wrote: > Hi Mika > > > -----Original Message----- > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > owner@vger.kernel.org] On Behalf Of Mika Westerberg > > Sent: 13 June 2017 21:04 > > To: Gabriele Paoloni > > Cc: Lorenzo Pieralisi; rafael@kernel.org; Rafael J. Wysocki; > > catalin.marinas@arm.com; will.deacon@arm.com; robh+dt@kernel.org; > > frowand.list@gmail.com; bhelgaas@google.com; arnd@arndb.de; linux-arm- > > kernel@lists.infradead.org; mark.rutland@arm.com; > > brian.starkey@arm.com; olof@lixom.net; benh@kernel.crashing.org; linux- > > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; Linuxarm; linux- > > pci@vger.kernel.org; minyard@acm.org; John Garry; xuwei (O) > > Subject: Re: [PATCH v9 5/7] ACPI: Translate the I/O range of non-MMIO > > devices before scanning > > > > On Tue, Jun 13, 2017 at 07:01:38PM +0000, Gabriele Paoloni wrote: > > > I am not very familiar with Linux MFD however the main issue here is > > that > > > 1) for IPMI we want to re-use the standard IPMI driver without > > touching it: > > > see > > > > > > static const struct acpi_device_id acpi_ipmi_match[] = { > > > { "IPI0001", 0 }, > > > { }, > > > }; > > > > > > in "drivers/char/ipmi/ipmi_si_intf.c" (and in general any standard > > driver > > > of an LPC child) > > > > > > 2) We need a way to guarantee that all LPC children are not > > enumerated > > > by acpi_default_enumeration() (so for example if an ipmi node is > > an LPC# > > > child it should not be enumerated, otherwise it should be) > > > Currently acpi_default_enumeration() skips spi/i2c slaves by > > checking: > > > 1) if the acpi resource type is a serial bus > > > 2) if the type of serial bus descriptor is I2C or SPI > > > > > > For LPC we cannot leverage on any ACPI property to "recognize" > > that our > > > devices are LPC children; hence before I proposed for > > acpi_default_enumeration() > > > to skip devices that have already been enumerated (by calling > > > acpi_device_enumerated() ). > > > > > > So in the current scenario, how do you think that MFD can help? > > > > If you look at Documentation/acpi/enumeration.txt there is a chapter > > "MFD devices". I think it pretty much maches what you have here. An LPC > > device (MFD device) and bunch of child devices. The driver for your LPC > > device can specify _HID for each child device. Those are then mached by > > the MFD ACPI code to the corresponding ACPI nodes from which platform > > devices are created including "IPI0001". > > So I guess here in the LPC driver I would have an MFD cell for IPMI. I.e.: > > static struct mfd_cell_acpi_match hisi_lpc_ipmi_acpi_match = { > .pnpid = "IPI0001", > }; > > correct? Yes. > > > > It causes acpi_default_enumeration() to be called but it should be fine > > as we are dealing with platform device anyway. > > I do not quite understand how declaring such MFD cell above would make sure > that the LPC probe is called before the IPMI device is enumerated... In fact it may be that it is not sufficient in this case because the ACPI core might enumerate child devices before the LPC driver even gets a chance to probe so you would need to add also scan handler to the child devices and mark them already enumerated or something like that. > Could you point me to the code that does this? Check for example drivers/mfd/intel-lpss.c.
On Fri, Jun 16, 2017 at 10:33 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Thu, Jun 15, 2017 at 06:01:02PM +0000, Gabriele Paoloni wrote: >> Hi Mika >> >> > -----Original Message----- >> > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >> > owner@vger.kernel.org] On Behalf Of Mika Westerberg >> > Sent: 13 June 2017 21:04 >> > To: Gabriele Paoloni >> > Cc: Lorenzo Pieralisi; rafael@kernel.org; Rafael J. Wysocki; >> > catalin.marinas@arm.com; will.deacon@arm.com; robh+dt@kernel.org; >> > frowand.list@gmail.com; bhelgaas@google.com; arnd@arndb.de; linux-arm- >> > kernel@lists.infradead.org; mark.rutland@arm.com; >> > brian.starkey@arm.com; olof@lixom.net; benh@kernel.crashing.org; linux- >> > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; Linuxarm; linux- >> > pci@vger.kernel.org; minyard@acm.org; John Garry; xuwei (O) >> > Subject: Re: [PATCH v9 5/7] ACPI: Translate the I/O range of non-MMIO >> > devices before scanning >> > >> > On Tue, Jun 13, 2017 at 07:01:38PM +0000, Gabriele Paoloni wrote: >> > > I am not very familiar with Linux MFD however the main issue here is >> > that >> > > 1) for IPMI we want to re-use the standard IPMI driver without >> > touching it: >> > > see >> > > >> > > static const struct acpi_device_id acpi_ipmi_match[] = { >> > > { "IPI0001", 0 }, >> > > { }, >> > > }; >> > > >> > > in "drivers/char/ipmi/ipmi_si_intf.c" (and in general any standard >> > driver >> > > of an LPC child) >> > > >> > > 2) We need a way to guarantee that all LPC children are not >> > enumerated >> > > by acpi_default_enumeration() (so for example if an ipmi node is >> > an LPC# >> > > child it should not be enumerated, otherwise it should be) >> > > Currently acpi_default_enumeration() skips spi/i2c slaves by >> > checking: >> > > 1) if the acpi resource type is a serial bus >> > > 2) if the type of serial bus descriptor is I2C or SPI >> > > >> > > For LPC we cannot leverage on any ACPI property to "recognize" >> > that our >> > > devices are LPC children; hence before I proposed for >> > acpi_default_enumeration() >> > > to skip devices that have already been enumerated (by calling >> > > acpi_device_enumerated() ). >> > > >> > > So in the current scenario, how do you think that MFD can help? >> > >> > If you look at Documentation/acpi/enumeration.txt there is a chapter >> > "MFD devices". I think it pretty much maches what you have here. An LPC >> > device (MFD device) and bunch of child devices. The driver for your LPC >> > device can specify _HID for each child device. Those are then mached by >> > the MFD ACPI code to the corresponding ACPI nodes from which platform >> > devices are created including "IPI0001". >> >> So I guess here in the LPC driver I would have an MFD cell for IPMI. I.e.: >> >> static struct mfd_cell_acpi_match hisi_lpc_ipmi_acpi_match = { >> .pnpid = "IPI0001", >> }; >> >> correct? > > Yes. > >> > >> > It causes acpi_default_enumeration() to be called but it should be fine >> > as we are dealing with platform device anyway. >> >> I do not quite understand how declaring such MFD cell above would make sure >> that the LPC probe is called before the IPMI device is enumerated... > > In fact it may be that it is not sufficient in this case because the > ACPI core might enumerate child devices before the LPC driver even gets > a chance to probe so you would need to add also scan handler to the > child devices and mark them already enumerated or something like that. Or extend the special I2C/SPI handling to them. Thanks, Rafael
Hi Mika > -----Original Message----- > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Sent: 19 June 2017 11:02 > To: Gabriele Paoloni > Cc: Rafael J. Wysocki; Lorenzo Pieralisi; Rafael J. Wysocki; > catalin.marinas@arm.com; will.deacon@arm.com; robh+dt@kernel.org; > frowand.list@gmail.com; bhelgaas@google.com; arnd@arndb.de; linux-arm- > kernel@lists.infradead.org; mark.rutland@arm.com; > brian.starkey@arm.com; olof@lixom.net; benh@kernel.crashing.org; linux- > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; Linuxarm; linux- > pci@vger.kernel.org; minyard@acm.org; John Garry; xuwei (O) > Subject: Re: [PATCH v9 5/7] ACPI: Translate the I/O range of non-MMIO > devices before scanning > > On Mon, Jun 19, 2017 at 09:50:49AM +0000, Gabriele Paoloni wrote: > > Many thanks for your response and your help here. > > > > I guess that as conclusion with respect to the current v9 patchset we > can > > disregard the idea of MFD and modify the current v9 so that it > doesn't > > touch directly ACPI resources. > > Instead as I proposed before we can have the scan handler to > enumerate > > the children devices and translate its addresses filling dev- > >resources[] and > > at the same time we can modify acpi_default_enumeration to check > > acpi_device_enumerated() before continuing with device > enumeration...? > > > > Do you think it as a viable solution? > > No, I think MFD + scan handler inside the MFD driver is the way to go. > We don't want to trash ACPI core with stuff that does not belong there > IMHO. Ok Many thanks I will investigate this direction > > Also you don't need to modify acpi_default_enumeration() because you > can > mark your device enumerated in the MFD driver. So all the dirty details > will be in the MFD driver and not in ACPI core. Ok got it :) Cheers Gab
On 30/06/2017 10:05, Mika Westerberg wrote: > On Thu, Jun 29, 2017 at 05:16:15PM +0100, John Garry wrote: >> On 16/06/2017 12:24, Rafael J. Wysocki wrote: >>>>>>>>> >>>>>>>>> It causes acpi_default_enumeration() to be called but it should be fine >>>>>>>>> as we are dealing with platform device anyway. >>>>>>> >>>>>>> I do not quite understand how declaring such MFD cell above would make sure >>>>>>> that the LPC probe is called before the IPMI device is enumerated... >>>>> >>>>> In fact it may be that it is not sufficient in this case because the >>>>> ACPI core might enumerate child devices before the LPC driver even gets >>>>> a chance to probe so you would need to add also scan handler to the >>>>> child devices and mark them already enumerated or something like that. >>> Or extend the special I2C/SPI handling to them. >>> >> >> For this, is it possible to just configure the ACPI table so we spoof that >> the LPC slave (IPI0001), is an i2c/spi slave? Could we just add a resource >> of type ACPI_RESOURCE_TYPE_SERIAL_BUS, and common serial bus type i2c/spi to >> solve this? > > But is the device connected to a I2C or SPI bus? If not, then it does > not make much sense to declare it as I2C or SPI slave. Instead it should > be platform device which is the type we use when there is no explicit > bus specified in ACPI. > No, it's not a SPI nor an I2C bus. I actually would say that my idea is generally wrong, as the ACPI definition is not a real reflection of the bus/slave. However, Rafael did suggest extending special I2C/SPI handling to them. In this case, I don't see how the LPC slave can be identified like an I2C or SPI slave is. Thanks, John > . >
On Fri, Jun 30, 2017 at 11:28 AM, John Garry <john.garry@huawei.com> wrote: > On 30/06/2017 10:05, Mika Westerberg wrote: >> >> On Thu, Jun 29, 2017 at 05:16:15PM +0100, John Garry wrote: >>> >>> On 16/06/2017 12:24, Rafael J. Wysocki wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> It causes acpi_default_enumeration() to be called but it should be >>>>>>>>>> fine >>>>>>>>>> as we are dealing with platform device anyway. >>>>>>>> >>>>>>>> >>>>>>>> I do not quite understand how declaring such MFD cell above would >>>>>>>> make sure >>>>>>>> that the LPC probe is called before the IPMI device is enumerated... >>>>>> >>>>>> >>>>>> In fact it may be that it is not sufficient in this case because the >>>>>> ACPI core might enumerate child devices before the LPC driver even >>>>>> gets >>>>>> a chance to probe so you would need to add also scan handler to the >>>>>> child devices and mark them already enumerated or something like that. >>>> >>>> Or extend the special I2C/SPI handling to them. >>>> >>> >>> For this, is it possible to just configure the ACPI table so we spoof >>> that >>> the LPC slave (IPI0001), is an i2c/spi slave? Could we just add a >>> resource >>> of type ACPI_RESOURCE_TYPE_SERIAL_BUS, and common serial bus type i2c/spi >>> to >>> solve this? >> >> >> But is the device connected to a I2C or SPI bus? If not, then it does >> not make much sense to declare it as I2C or SPI slave. Instead it should >> be platform device which is the type we use when there is no explicit >> bus specified in ACPI. >> > > No, it's not a SPI nor an I2C bus. I actually would say that my idea is > generally wrong, as the ACPI definition is not a real reflection of the > bus/slave. > > However, Rafael did suggest extending special I2C/SPI handling to them. In > this case, I don't see how the LPC slave can be identified like an I2C or > SPI slave is. I meant that it can be handled similarly (ie. as an exception from the default enumeration), such that the enumeration is delayed until the proper subsystem can enumerate those devices as appropriate. Sorry for the confusion. Thanks, Rafael
diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile index 1017def..3944775 100644 --- a/drivers/acpi/arm64/Makefile +++ b/drivers/acpi/arm64/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_ACPI_IORT) += iort.o obj-$(CONFIG_ACPI_GTDT) += gtdt.o +obj-$(CONFIG_INDIRECT_PIO) += acpi_indirect_pio.o diff --git a/drivers/acpi/arm64/acpi_indirect_pio.c b/drivers/acpi/arm64/acpi_indirect_pio.c new file mode 100644 index 0000000..7813f73 --- /dev/null +++ b/drivers/acpi/arm64/acpi_indirect_pio.c @@ -0,0 +1,301 @@ +/* + * ACPI support for indirect-PIO bus. + * + * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved. + * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/acpi.h> +#include <linux/platform_device.h> +#include <linux/logic_pio.h> + +#include <acpi/acpi_indirect_pio.h> + +ACPI_MODULE_NAME("indirect PIO"); + +#define INDIRECT_PIO_INFO(desc) ((unsigned long)&desc) + +static acpi_status acpi_count_logic_iores(struct acpi_resource *res, + void *data) +{ + int *res_cnt = data; + + if (!acpi_dev_filter_resource_type(res, IORESOURCE_IO)) + (*res_cnt)++; + + return AE_OK; +} + +static acpi_status acpi_read_one_logicpiores(struct acpi_resource *res, + void *data) +{ + struct acpi_resource **resource = data; + + if (!acpi_dev_filter_resource_type(res, IORESOURCE_IO)) { + memcpy((*resource), res, sizeof(struct acpi_resource)); + (*resource)->length = sizeof(struct acpi_resource); + (*resource)->type = res->type; + (*resource)++; + } + + return AE_OK; +} + +static acpi_status +acpi_build_logicpiores_template(struct acpi_device *adev, + struct acpi_buffer *buffer) +{ + acpi_handle handle = adev->handle; + struct acpi_resource *resource; + acpi_status status; + int res_cnt = 0; + + status = acpi_walk_resources(handle, METHOD_NAME__CRS, + acpi_count_logic_iores, &res_cnt); + if (ACPI_FAILURE(status)) { + dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status); + return -EINVAL; + } + + if (!res_cnt) { + dev_err(&adev->dev, "no logic IO resources\n"); + return -ENODEV; + } + + buffer->length = sizeof(struct acpi_resource) * (res_cnt + 1); + buffer->pointer = kzalloc(buffer->length, GFP_KERNEL); + if (!buffer->pointer) + return -ENOMEM; + + resource = (struct acpi_resource *)buffer->pointer; + status = acpi_walk_resources(handle, METHOD_NAME__CRS, + acpi_read_one_logicpiores, &resource); + if (ACPI_FAILURE(status)) { + kfree(buffer->pointer); + dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status); + return -EINVAL; + } + + resource->type = ACPI_RESOURCE_TYPE_END_TAG; + resource->length = sizeof(struct acpi_resource); + + return 0; +} + +static int acpi_translate_logicpiores(struct acpi_device *adev, + struct acpi_device *host, struct acpi_buffer *buffer) +{ + struct acpi_resource *resource = buffer->pointer; + unsigned long sys_port; + struct device *dev = &adev->dev; + union acpi_resource_data *trans_data = &resource->data; + resource_size_t bus_addr; + resource_size_t max_pio; + resource_size_t length; + + switch (resource->type) { + case ACPI_RESOURCE_TYPE_ADDRESS16: + if (trans_data->address16.min_address_fixed != + trans_data->address16.max_address_fixed) { + dev_warn(dev, "variable I/O resource is invalid!\n"); + return -EINVAL; + } + bus_addr = trans_data->address16.address.minimum; + length = trans_data->address16.address.address_length; + max_pio = U16_MAX; + break; + case ACPI_RESOURCE_TYPE_ADDRESS32: + if (trans_data->address32.min_address_fixed != + trans_data->address32.max_address_fixed) { + dev_warn(dev, "variable I/O resource is invalid!\n"); + return -EINVAL; + } + bus_addr = trans_data->address32.address.minimum; + length = trans_data->address32.address.address_length; + max_pio = U32_MAX; + break; + case ACPI_RESOURCE_TYPE_ADDRESS64: + if (trans_data->address64.min_address_fixed != + trans_data->address64.max_address_fixed) { + dev_warn(dev, "variable I/O resource is invalid!\n"); + return -EINVAL; + } + bus_addr = trans_data->address64.address.minimum; + length = trans_data->address64.address.address_length; + max_pio = U64_MAX; + break; + case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64: + if (trans_data->ext_address64.min_address_fixed != + trans_data->ext_address64.max_address_fixed) { + dev_warn(dev, "variable I/O resource is invalid!\n"); + return -EINVAL; + } + bus_addr = trans_data->ext_address64.address.minimum; + length = trans_data->ext_address64.address.address_length; + max_pio = U64_MAX; + break; + case ACPI_RESOURCE_TYPE_IO: + bus_addr = trans_data->io.minimum; + length = trans_data->io.address_length; + max_pio = U16_MAX; + break; + case ACPI_RESOURCE_TYPE_FIXED_IO: + bus_addr = trans_data->fixed_io.address; + length = trans_data->fixed_io.address_length; + max_pio = U16_MAX; + break; + default: + return -EINVAL; + } + + sys_port = logic_pio_trans_hwaddr(&host->fwnode, bus_addr); + if (sys_port == -1) { + dev_err(dev, "translate bus-addr(0x%llx) fail!\n", bus_addr); + return -EFAULT; + } + + /* + * we need to check if the resource address can contain the + * translated IO token + */ + if ((sys_port + length) > max_pio) { + dev_err(dev, "sys_port exceeds the max resource address\n"); + return -ENOSPC; + } + + switch (resource->type) { + case ACPI_RESOURCE_TYPE_ADDRESS16: + trans_data->address16.address.minimum = sys_port; + trans_data->address16.address.maximum = sys_port + length; + break; + case ACPI_RESOURCE_TYPE_ADDRESS32: + trans_data->address32.address.minimum = sys_port; + trans_data->address32.address.maximum = sys_port + length; + break; + case ACPI_RESOURCE_TYPE_ADDRESS64: + trans_data->address64.address.minimum = sys_port; + trans_data->address64.address.maximum = sys_port + length; + break; + case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64: + trans_data->ext_address64.address.minimum = sys_port; + trans_data->ext_address64.address.maximum = sys_port + length; + break; + case ACPI_RESOURCE_TYPE_IO: + trans_data->io.minimum = sys_port; + trans_data->io.maximum = sys_port + length; + break; + case ACPI_RESOURCE_TYPE_FIXED_IO: + trans_data->fixed_io.address = sys_port; + break; + } + return 0; +} + +/* + * update/set the current I/O resource of the designated device node. + * after this calling, the enumeration can be started as the I/O resource + * had been translated to logicial I/O from bus-local I/O. + * + * @adev: the device node to be updated the I/O resource; + * @host: the device node where 'adev' is attached, which can be not + * the parent of 'adev'; + * + * return 0 when successful, negative is for failure. + */ +int acpi_set_logic_pio_resource(struct device *child, + struct device *hostdev) +{ + struct acpi_device *adev; + struct acpi_device *host; + struct acpi_buffer buffer; + acpi_status status; + int ret; + + if (!child || !hostdev) + return -EINVAL; + + host = to_acpi_device(hostdev); + adev = to_acpi_device(child); + + /* check the device state */ + if (!adev->status.present) { + dev_info(child, "ACPI: device is not present!\n"); + return 0; + } + /* whether the child had been enumerated? */ + if (acpi_device_enumerated(adev)) { + dev_info(child, "ACPI: had been enumerated!\n"); + return 0; + } + + /* read the _CRS and convert as acpi_buffer */ + status = acpi_build_logicpiores_template(adev, &buffer); + if (ACPI_FAILURE(status)) { + dev_warn(child, "Failure evaluating %s\n", METHOD_NAME__CRS); + return -ENODEV; + } + + /* translate the I/O resources */ + ret = acpi_translate_logicpiores(adev, host, &buffer); + if (ret) { + kfree(buffer.pointer); + dev_err(child, "Translate I/O range FAIL!\n"); + return ret; + } + + /* set current resource... */ + status = acpi_set_current_resources(adev->handle, &buffer); + kfree(buffer.pointer); + if (ACPI_FAILURE(status)) { + dev_err(child, "Error evaluating _SRS (0x%x)\n", status); + ret = -EIO; + } + + return ret; +} + +/* All the host devices which apply indirect-PIO can be listed here. */ +static const struct acpi_device_id acpi_indirect_host_id[] = { + {""}, +}; + +static int acpi_indirectpio_attach(struct acpi_device *adev, + const struct acpi_device_id *id) +{ + struct indirect_pio_device_desc *hostdata; + struct platform_device *pdev; + int ret; + + hostdata = (struct indirect_pio_device_desc *)id->driver_data; + if (!hostdata || !hostdata->pre_setup) + return -EINVAL; + + ret = hostdata->pre_setup(adev, hostdata->pdata); + if (!ret) { + pdev = acpi_create_platform_device(adev, NULL); + if (IS_ERR_OR_NULL(pdev)) { + dev_err(&adev->dev, "Create platform device for host FAIL!\n"); + return -EFAULT; + } + acpi_device_set_enumerated(adev); + ret = 1; + } + + return ret; +} + + +static struct acpi_scan_handler acpi_indirect_handler = { + .ids = acpi_indirect_host_id, + .attach = acpi_indirectpio_attach, +}; + +void __init acpi_indirectio_scan_init(void) +{ + acpi_scan_add_handler(&acpi_indirect_handler); +} diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 66229ff..bf8aaf8 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -31,6 +31,11 @@ void acpi_processor_init(void); void acpi_platform_init(void); void acpi_pnp_init(void); void acpi_int340x_thermal_init(void); +#ifdef CONFIG_INDIRECT_PIO +void acpi_indirectio_scan_init(void); +#else +static inline void acpi_indirectio_scan_init(void) {} +#endif #ifdef CONFIG_ARM_AMBA void acpi_amba_init(void); #else diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index e39ec7b..37dd23c 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -2035,6 +2035,7 @@ int __init acpi_scan_init(void) acpi_int340x_thermal_init(); acpi_amba_init(); acpi_watchdog_init(); + acpi_indirectio_scan_init(); acpi_scan_add_handler(&generic_device_handler); diff --git a/include/acpi/acpi_indirect_pio.h b/include/acpi/acpi_indirect_pio.h new file mode 100644 index 0000000..efc5c43 --- /dev/null +++ b/include/acpi/acpi_indirect_pio.h @@ -0,0 +1,24 @@ +/* + * ACPI support for indirect-PIO bus. + * + * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved. + * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef _ACPI_INDIRECT_PIO_H +#define _ACPI_INDIRECT_PIO_H + +struct indirect_pio_device_desc { + void *pdata; /* device relevant info data */ + int (*pre_setup)(struct acpi_device *adev, void *pdata); +}; + +int acpi_set_logic_pio_resource(struct device *child, + struct device *hostdev); + +#endif