Message ID | 1518543933-22456-8-git-send-email-john.garry@huawei.com |
---|---|
State | Not Applicable |
Headers | show |
Series | LPC: legacy ISA I/O support | expand |
On Tue, Feb 13, 2018 at 6:45 PM, John Garry <john.garry@huawei.com> 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. > > 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> Just a few minor nits below. > --- > drivers/acpi/arm64/Makefile | 1 + > drivers/acpi/arm64/acpi_indirectio.c | 250 +++++++++++++++++++++++++++++++++++ > drivers/acpi/internal.h | 5 + > drivers/acpi/scan.c | 1 + > 4 files changed, 257 insertions(+) > create mode 100644 drivers/acpi/arm64/acpi_indirectio.c > > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > index 1017def..f4a7f46 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_indirectio.o > diff --git a/drivers/acpi/arm64/acpi_indirectio.c b/drivers/acpi/arm64/acpi_indirectio.c > new file mode 100644 > index 0000000..51a1b92 > --- /dev/null > +++ b/drivers/acpi/arm64/acpi_indirectio.c > @@ -0,0 +1,250 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. > + * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com> > + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com> > + * Author: John Garry <john.garry@huawei.com> > + * > + * This file implements functunality to scan the ACPI namespace and config > + * devices under "indirect IO" hosts. An "indirect IO" host allows child > + * devices to use logical IO accesses when the host, itself, does not provide > + * a transparent bridge. The device setup creates a per-child MFD with a > + * logical port IO resource. > + */ > + > +#include <linux/acpi.h> > +#include <linux/logic_pio.h> > +#include <linux/mfd/core.h> > +#include <linux/platform_device.h> > + > +ACPI_MODULE_NAME("indirect IO"); > + > +#define ACPI_INDIRECT_IO_NAME_LEN 255 > + > +struct acpi_indirect_io_mfd_cell { > + struct mfd_cell_acpi_match acpi_match; > + char name[ACPI_INDIRECT_IO_NAME_LEN]; > + char pnpid[ACPI_INDIRECT_IO_NAME_LEN]; > +}; > + > +static int acpi_indirect_io_xlat_res(struct acpi_device *adev, > + struct acpi_device *host, > + struct resource *res) > +{ > + unsigned long sys_port; > + resource_size_t len = res->end - res->start; > + > + sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len); > + if (sys_port == -1UL) > + return -EFAULT; > + > + res->start = sys_port; > + res->end = sys_port + len; > + > + return 0; > +} > + > +/* > + * acpi_indirect_io_set_res - set the resources for a child device > + * (MFD) of an "indirect IO" host. The above should fit into a single line. I'd make it something like "acpi_indirect_io_set_res - set "indirect IO" host child (MFD) resources" and it already is explained in the comment below. > + * @child: the device node to be updated the I/O resource > + * @hostdev: the device node associated with the "indirect IO" host > + * @res: double pointer to be set to the address of translated resources > + * @num_res: pointer to variable to hold the number of translated resources > + * > + * Returns 0 when successful, and a negative value for failure. > + * > + * For a given "indirect IO" host, each child device will have associated > + * host-relevative address resource. This function will return the translated host-relative > + * logical PIO addresses for each child devices resources. > + */ > +static int acpi_indirect_io_set_res(struct device *child, > + struct device *hostdev, > + const struct resource **res, > + int *num_res) > +{ > + struct acpi_device *adev; > + struct acpi_device *host; > + struct resource_entry *rentry; > + LIST_HEAD(resource_list); > + struct resource *resources; > + int count; > + int i; > + int ret = -EIO; > + > + 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, "device is not present\n"); dev_dbg()? > + return 0; > + } > + /* whether the child had been enumerated? */ > + if (acpi_device_enumerated(adev)) { > + dev_info(child, "had been enumerated\n"); Again, dev_dbg()? > + return 0; > + } > + > + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); > + if (count <= 0) { > + dev_err(child, "failed to get resources\n"); I'd use dev_dbg() here too (the message may not even be meaningful to a user). > + return count ? count : -EIO; > + } > + > + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); > + if (!resources) { And you don't print anything here, I wonder why? > + acpi_dev_free_resource_list(&resource_list); > + return -ENOMEM; > + } > + count = 0; > + list_for_each_entry(rentry, &resource_list, node) > + resources[count++] = *rentry->res; > + > + acpi_dev_free_resource_list(&resource_list); > + > + /* translate the I/O resources */ > + for (i = 0; i < count; i++) { > + if (!(resources[i].flags & IORESOURCE_IO)) > + continue; > + ret = acpi_indirect_io_xlat_res(adev, host, &resources[i]); > + if (ret) { > + kfree(resources); > + dev_err(child, "translate IO range failed(%d)\n", ret); > + return ret; > + } > + } > + *res = resources; > + *num_res = count; > + > + return ret; > +} > + > +/* > + * acpi_indirect_io_setup - scan handler for "indirect IO" host. > + * @adev: "indirect IO" host ACPI device pointer One extra empty comment line here, please. > + * Returns 0 when successful, and a negative value for failure. > + * > + * Setup an "indirect IO" host by scanning all child devices, and > + * create a per-device MFD with logical PIO translated IO resources. > + */ > +static int acpi_indirect_io_setup(struct acpi_device *adev) > +{ > + struct platform_device *pdev; > + struct mfd_cell *mfd_cells; > + struct logic_pio_hwaddr *range; > + struct acpi_device *child; > + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cells; > + int size, ret, count = 0, cell_num = 0; > + > + range = kzalloc(sizeof(*range), GFP_KERNEL); > + if (!range) > + return -ENOMEM; > + range->fwnode = &adev->fwnode; > + range->flags = PIO_INDIRECT; > + range->size = PIO_INDIRECT_SIZE; > + > + ret = logic_pio_register_range(range); > + if (ret) > + goto free_range; > + > + list_for_each_entry(child, &adev->children, node) > + cell_num++; > + > + /* allocate the mfd cell and companion acpi info, one per child */ > + size = sizeof(*mfd_cells) + sizeof(*acpi_indirect_io_mfd_cells); > + mfd_cells = kcalloc(cell_num, size, GFP_KERNEL); > + if (!mfd_cells) { > + ret = -ENOMEM; > + goto free_range; > + } > + > + acpi_indirect_io_mfd_cells = (struct acpi_indirect_io_mfd_cell *) > + &mfd_cells[cell_num]; > + /* Only consider the children of the host */ > + list_for_each_entry(child, &adev->children, node) { > + struct mfd_cell *mfd_cell = &mfd_cells[count]; > + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cell = > + &acpi_indirect_io_mfd_cells[count]; > + const struct mfd_cell_acpi_match *acpi_match = > + &acpi_indirect_io_mfd_cell->acpi_match; > + char *name = &acpi_indirect_io_mfd_cell[count].name[0]; > + char *pnpid = &acpi_indirect_io_mfd_cell[count].pnpid[0]; > + struct mfd_cell_acpi_match match = { > + .pnpid = pnpid, > + }; > + > + snprintf(name, ACPI_INDIRECT_IO_NAME_LEN, "indirect-io-%s", > + acpi_device_hid(child)); > + snprintf(pnpid, ACPI_INDIRECT_IO_NAME_LEN, "%s", > + acpi_device_hid(child)); > + > + memcpy((void *)acpi_match, (void *)&match, sizeof(*acpi_match)); > + mfd_cell->name = name; > + mfd_cell->acpi_match = acpi_match; > + > + ret = acpi_indirect_io_set_res(&child->dev, &adev->dev, > + &mfd_cell->resources, > + &mfd_cell->num_resources); > + if (ret) { > + dev_err(&child->dev, "set resource failed (%d)\n", ret); Again, please consider using dev_dbg() here and below (for the same reason as above). > + goto free_mfd_resources; > + } > + count++; > + } > + > + pdev = acpi_create_platform_device(adev, NULL); > + if (IS_ERR_OR_NULL(pdev)) { > + dev_err(&adev->dev, "create platform device for host failed\n"); > + ret = PTR_ERR(pdev); > + goto free_mfd_resources; > + } > + acpi_device_set_enumerated(adev); > + > + ret = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, > + mfd_cells, cell_num, NULL, 0, NULL); > + if (ret) { > + dev_err(&pdev->dev, "failed to add mfd cells (%d)\n", ret); > + goto free_mfd_resources; > + } > + > + return ret; return 0; You know that ret must be 0 here anyway. > + > +free_mfd_resources: > + while (cell_num--) > + kfree(mfd_cells[cell_num].resources); > + kfree(mfd_cells); > +free_range: > + kfree(range); > + > + return ret; > +} > + > +/* All the host devices which apply indirect-IO can be listed here. */ > +static const struct acpi_device_id acpi_indirect_io_host_id[] = { > + {} > +}; > + > +static int acpi_indirect_io_attach(struct acpi_device *adev, > + const struct acpi_device_id *id) > +{ > + int ret = acpi_indirect_io_setup(adev); > + > + if (ret < 0) > + return ret; > + > + return 1; The above can be written as return ret < 0 ? ret : 1; to save a few lines of code (you are using this pattern above, so why not here?). > +} > + > +static struct acpi_scan_handler acpi_indirect_io_handler = { > + .ids = acpi_indirect_io_host_id, > + .attach = acpi_indirect_io_attach, > +}; > + > +void __init acpi_indirect_io_scan_init(void) > +{ > + acpi_scan_add_handler(&acpi_indirect_io_handler); > +} > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > index 1d0a501..680f3cf 100644 > --- a/drivers/acpi/internal.h > +++ b/drivers/acpi/internal.h > @@ -31,6 +31,11 @@ > void acpi_platform_init(void); > void acpi_pnp_init(void); > void acpi_int340x_thermal_init(void); > +#ifdef CONFIG_INDIRECT_PIO > +void acpi_indirect_io_scan_init(void); > +#else > +static inline void acpi_indirect_io_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 8e63d93..204da8a 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -2155,6 +2155,7 @@ int __init acpi_scan_init(void) > acpi_amba_init(); > acpi_watchdog_init(); > acpi_init_lpit(); > + acpi_indirect_io_scan_init(); > > acpi_scan_add_handler(&generic_device_handler); > > -- But generally Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> for the generic ACPI changes.
>> 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> > Hi Rafael, Thanks for checking again. > Just a few minor nits below. > >> --- >> drivers/acpi/arm64/Makefile | 1 + >> drivers/acpi/arm64/acpi_indirectio.c | 250 +++++++++++++++++++++++++++++++++++ >> drivers/acpi/internal.h | 5 + >> drivers/acpi/scan.c | 1 + >> 4 files changed, 257 insertions(+) >> create mode 100644 drivers/acpi/arm64/acpi_indirectio.c >> >> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile >> index 1017def..f4a7f46 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_indirectio.o >> diff --git a/drivers/acpi/arm64/acpi_indirectio.c b/drivers/acpi/arm64/acpi_indirectio.c >> new file mode 100644 >> index 0000000..51a1b92 >> --- /dev/null >> +++ b/drivers/acpi/arm64/acpi_indirectio.c >> @@ -0,0 +1,250 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. >> + * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com> >> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com> >> + * Author: John Garry <john.garry@huawei.com> >> + * >> + * This file implements functunality to scan the ACPI namespace and config >> + * devices under "indirect IO" hosts. An "indirect IO" host allows child >> + * devices to use logical IO accesses when the host, itself, does not provide >> + * a transparent bridge. The device setup creates a per-child MFD with a >> + * logical port IO resource. >> + */ >> + >> +#include <linux/acpi.h> >> +#include <linux/logic_pio.h> >> +#include <linux/mfd/core.h> >> +#include <linux/platform_device.h> >> + >> +ACPI_MODULE_NAME("indirect IO"); >> + >> +#define ACPI_INDIRECT_IO_NAME_LEN 255 >> + >> +struct acpi_indirect_io_mfd_cell { >> + struct mfd_cell_acpi_match acpi_match; >> + char name[ACPI_INDIRECT_IO_NAME_LEN]; >> + char pnpid[ACPI_INDIRECT_IO_NAME_LEN]; >> +}; >> + >> +static int acpi_indirect_io_xlat_res(struct acpi_device *adev, >> + struct acpi_device *host, >> + struct resource *res) >> +{ >> + unsigned long sys_port; >> + resource_size_t len = res->end - res->start; >> + >> + sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len); >> + if (sys_port == -1UL) >> + return -EFAULT; >> + >> + res->start = sys_port; >> + res->end = sys_port + len; >> + >> + return 0; >> +} >> + >> +/* >> + * acpi_indirect_io_set_res - set the resources for a child device >> + * (MFD) of an "indirect IO" host. > > The above should fit into a single line. > > I'd make it something like "acpi_indirect_io_set_res - set "indirect > IO" host child (MFD) resources" and it already is explained in the > comment below. Fine > >> + * @child: the device node to be updated the I/O resource >> + * @hostdev: the device node associated with the "indirect IO" host >> + * @res: double pointer to be set to the address of translated resources >> + * @num_res: pointer to variable to hold the number of translated resources >> + * >> + * Returns 0 when successful, and a negative value for failure. >> + * >> + * For a given "indirect IO" host, each child device will have associated >> + * host-relevative address resource. This function will return the translated > > host-relative right > >> + * logical PIO addresses for each child devices resources. >> + */ >> +static int acpi_indirect_io_set_res(struct device *child, >> + struct device *hostdev, >> + const struct resource **res, >> + int *num_res) >> +{ >> + struct acpi_device *adev; >> + struct acpi_device *host; >> + struct resource_entry *rentry; >> + LIST_HEAD(resource_list); >> + struct resource *resources; >> + int count; >> + int i; >> + int ret = -EIO; >> + >> + 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, "device is not present\n"); > > dev_dbg()? > >> + return 0; >> + } >> + /* whether the child had been enumerated? */ >> + if (acpi_device_enumerated(adev)) { >> + dev_info(child, "had been enumerated\n"); > > Again, dev_dbg()? sure, I think that these can be downgraded > >> + return 0; >> + } >> + >> + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); >> + if (count <= 0) { >> + dev_err(child, "failed to get resources\n"); > > I'd use dev_dbg() here too (the message may not even be meaningful to a user). I think that it could be ok - having no resources is not really an "error". > >> + return count ? count : -EIO; >> + } >> + >> + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); >> + if (!resources) { > > And you don't print anything here, I wonder why? As I see, we generally don't print out-of-memory failure as we expect the alloc code to do it. But I agree it's useful as we know the point of failure in the driver and can add diagnostic info like count value, as maybe we're trying to allocate a huge amount of memory. > >> + acpi_dev_free_resource_list(&resource_list); >> + return -ENOMEM; >> + } >> + count = 0; >> + list_for_each_entry(rentry, &resource_list, node) >> + resources[count++] = *rentry->res; >> + >> + acpi_dev_free_resource_list(&resource_list); >> + >> + /* translate the I/O resources */ >> + for (i = 0; i < count; i++) { >> + if (!(resources[i].flags & IORESOURCE_IO)) >> + continue; >> + ret = acpi_indirect_io_xlat_res(adev, host, &resources[i]); >> + if (ret) { >> + kfree(resources); >> + dev_err(child, "translate IO range failed(%d)\n", ret); >> + return ret; >> + } >> + } >> + *res = resources; >> + *num_res = count; >> + >> + return ret; >> +} >> + >> +/* >> + * acpi_indirect_io_setup - scan handler for "indirect IO" host. >> + * @adev: "indirect IO" host ACPI device pointer > > One extra empty comment line here, please. ok > >> + * Returns 0 when successful, and a negative value for failure. >> + * >> + * Setup an "indirect IO" host by scanning all child devices, and >> + * create a per-device MFD with logical PIO translated IO resources. >> + */ >> +static int acpi_indirect_io_setup(struct acpi_device *adev) >> +{ >> + struct platform_device *pdev; >> + struct mfd_cell *mfd_cells; >> + struct logic_pio_hwaddr *range; >> + struct acpi_device *child; >> + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cells; >> + int size, ret, count = 0, cell_num = 0; >> + >> + range = kzalloc(sizeof(*range), GFP_KERNEL); >> + if (!range) >> + return -ENOMEM; >> + range->fwnode = &adev->fwnode; >> + range->flags = PIO_INDIRECT; >> + range->size = PIO_INDIRECT_SIZE; >> + >> + ret = logic_pio_register_range(range); >> + if (ret) >> + goto free_range; >> + >> + list_for_each_entry(child, &adev->children, node) >> + cell_num++; >> + >> + /* allocate the mfd cell and companion acpi info, one per child */ >> + size = sizeof(*mfd_cells) + sizeof(*acpi_indirect_io_mfd_cells); >> + mfd_cells = kcalloc(cell_num, size, GFP_KERNEL); >> + if (!mfd_cells) { >> + ret = -ENOMEM; >> + goto free_range; >> + } >> + >> + acpi_indirect_io_mfd_cells = (struct acpi_indirect_io_mfd_cell *) >> + &mfd_cells[cell_num]; >> + /* Only consider the children of the host */ >> + list_for_each_entry(child, &adev->children, node) { >> + struct mfd_cell *mfd_cell = &mfd_cells[count]; >> + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cell = >> + &acpi_indirect_io_mfd_cells[count]; >> + const struct mfd_cell_acpi_match *acpi_match = >> + &acpi_indirect_io_mfd_cell->acpi_match; >> + char *name = &acpi_indirect_io_mfd_cell[count].name[0]; >> + char *pnpid = &acpi_indirect_io_mfd_cell[count].pnpid[0]; >> + struct mfd_cell_acpi_match match = { >> + .pnpid = pnpid, >> + }; >> + >> + snprintf(name, ACPI_INDIRECT_IO_NAME_LEN, "indirect-io-%s", >> + acpi_device_hid(child)); >> + snprintf(pnpid, ACPI_INDIRECT_IO_NAME_LEN, "%s", >> + acpi_device_hid(child)); >> + >> + memcpy((void *)acpi_match, (void *)&match, sizeof(*acpi_match)); >> + mfd_cell->name = name; >> + mfd_cell->acpi_match = acpi_match; >> + >> + ret = acpi_indirect_io_set_res(&child->dev, &adev->dev, >> + &mfd_cell->resources, >> + &mfd_cell->num_resources); >> + if (ret) { >> + dev_err(&child->dev, "set resource failed (%d)\n", ret); > > Again, please consider using dev_dbg() here and below (for the same > reason as above). > well if this happens the device is not enumerated, so I think a warn may be more appropiate >> + goto free_mfd_resources; >> + } >> + count++; >> + } >> + >> + pdev = acpi_create_platform_device(adev, NULL); >> + if (IS_ERR_OR_NULL(pdev)) { >> + dev_err(&adev->dev, "create platform device for host failed\n"); as above >> + ret = PTR_ERR(pdev); >> + goto free_mfd_resources; >> + } >> + acpi_device_set_enumerated(adev); >> + >> + ret = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, >> + mfd_cells, cell_num, NULL, 0, NULL); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to add mfd cells (%d)\n", ret); >> + goto free_mfd_resources; >> + } >> + >> + return ret; > > return 0; > > You know that ret must be 0 here anyway. yes, I prefer this > >> + >> +free_mfd_resources: >> + while (cell_num--) >> + kfree(mfd_cells[cell_num].resources); >> + kfree(mfd_cells); >> +free_range: >> + kfree(range); >> + >> + return ret; >> +} >> + >> +/* All the host devices which apply indirect-IO can be listed here. */ >> +static const struct acpi_device_id acpi_indirect_io_host_id[] = { >> + {} >> +}; >> + >> +static int acpi_indirect_io_attach(struct acpi_device *adev, >> + const struct acpi_device_id *id) >> +{ >> + int ret = acpi_indirect_io_setup(adev); >> + >> + if (ret < 0) >> + return ret; >> + >> + return 1; > > The above can be written as > > return ret < 0 ? ret : 1; > > to save a few lines of code (you are using this pattern above, so why > not here?). I can change it. > >> +} >> + >> +static struct acpi_scan_handler acpi_indirect_io_handler = { >> + .ids = acpi_indirect_io_host_id, >> + .attach = acpi_indirect_io_attach, >> +}; >> + >> +void __init acpi_indirect_io_scan_init(void) >> +{ >> + acpi_scan_add_handler(&acpi_indirect_io_handler); >> +} >> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h >> index 1d0a501..680f3cf 100644 >> --- a/drivers/acpi/internal.h >> +++ b/drivers/acpi/internal.h >> @@ -31,6 +31,11 @@ >> void acpi_platform_init(void); >> void acpi_pnp_init(void); >> void acpi_int340x_thermal_init(void); >> +#ifdef CONFIG_INDIRECT_PIO >> +void acpi_indirect_io_scan_init(void); >> +#else >> +static inline void acpi_indirect_io_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 8e63d93..204da8a 100644 >> --- a/drivers/acpi/scan.c >> +++ b/drivers/acpi/scan.c >> @@ -2155,6 +2155,7 @@ int __init acpi_scan_init(void) >> acpi_amba_init(); >> acpi_watchdog_init(); >> acpi_init_lpit(); >> + acpi_indirect_io_scan_init(); >> >> acpi_scan_add_handler(&generic_device_handler); >> >> -- > > But generally > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > for the generic ACPI changes. > Thanks, John > . >
On Tue, Feb 13, 2018 at 7:45 PM, John Garry <john.garry@huawei.com> 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. > > 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. > + unsigned long sys_port; > + sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len); > + if (sys_port == -1UL) Wouldn't it be better to compare with ULONG_MAX? > + return -EFAULT; > +/* Shouldn't be a kernel-doc? > + * acpi_indirect_io_set_res - set the resources for a child device > + * (MFD) of an "indirect IO" host. In that case this would be one line w/o period at the end. > + * @child: the device node to be updated the I/O resource > + * @hostdev: the device node associated with the "indirect IO" host > + * @res: double pointer to be set to the address of translated resources > + * @num_res: pointer to variable to hold the number of translated resources > + * > + * Returns 0 when successful, and a negative value for failure. > + * > + * For a given "indirect IO" host, each child device will have associated > + * host-relevative address resource. This function will return the translated > + * logical PIO addresses for each child devices resources. > + */ > +static int acpi_indirect_io_set_res(struct device *child, > + struct device *hostdev, > + const struct resource **res, > + int *num_res) > +{ > + struct acpi_device *adev; > + struct acpi_device *host; > + struct resource_entry *rentry; > + LIST_HEAD(resource_list); > + struct resource *resources; > + int count; > + int i; > + int ret = -EIO; > + > + if (!child || !hostdev) > + return -EINVAL; > + > + host = to_acpi_device(hostdev); > + adev = to_acpi_device(child); > + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); > + if (count <= 0) { > + dev_err(child, "failed to get resources\n"); > + return count ? count : -EIO; > + } > + > + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); > + if (!resources) { > + acpi_dev_free_resource_list(&resource_list); > + return -ENOMEM; > + } > + count = 0; > + list_for_each_entry(rentry, &resource_list, node) > + resources[count++] = *rentry->res; > + > + acpi_dev_free_resource_list(&resource_list); It has similarities with acpi_create_platform_device(). I guess we can utilize existing code. > + /* translate the I/O resources */ > + for (i = 0; i < count; i++) { > + if (!(resources[i].flags & IORESOURCE_IO)) > + continue; > + ret = acpi_indirect_io_xlat_res(adev, host, &resources[i]); > + if (ret) { > + kfree(resources); > + dev_err(child, "translate IO range failed(%d)\n", ret); > + return ret; > + } > + } > + *res = resources; > + *num_res = count; > + > + return ret; Perhaps, ret = ... if (ret) break; } if (ret) { kfree(resources); dev_err(child, "translate IO range failed(%d)\n", ret); return ret; } *res = resources; *num_res = count; return 0; ? > +} > + > +/* > + * acpi_indirect_io_setup - scan handler for "indirect IO" host. > + * @adev: "indirect IO" host ACPI device pointer > + * Returns 0 when successful, and a negative value for failure. > + * > + * Setup an "indirect IO" host by scanning all child devices, and > + * create a per-device MFD with logical PIO translated IO resources. > + */ > +static int acpi_indirect_io_setup(struct acpi_device *adev) > +{ > + struct platform_device *pdev; > + struct mfd_cell *mfd_cells; > + struct logic_pio_hwaddr *range; > + struct acpi_device *child; > + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cells; > + int size, ret, count = 0, cell_num = 0; > + > + range = kzalloc(sizeof(*range), GFP_KERNEL); > + if (!range) > + return -ENOMEM; > + range->fwnode = &adev->fwnode; > + range->flags = PIO_INDIRECT; > + range->size = PIO_INDIRECT_SIZE; > + > + ret = logic_pio_register_range(range); > + if (ret) > + goto free_range; > + > + list_for_each_entry(child, &adev->children, node) > + cell_num++; > + > + /* allocate the mfd cell and companion acpi info, one per child */ > + size = sizeof(*mfd_cells) + sizeof(*acpi_indirect_io_mfd_cells); > + mfd_cells = kcalloc(cell_num, size, GFP_KERNEL); > + if (!mfd_cells) { > + ret = -ENOMEM; > + goto free_range; > + } > + > + acpi_indirect_io_mfd_cells = (struct acpi_indirect_io_mfd_cell *) > + &mfd_cells[cell_num]; > + /* Only consider the children of the host */ > + list_for_each_entry(child, &adev->children, node) { > + struct mfd_cell *mfd_cell = &mfd_cells[count]; > + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cell = > + &acpi_indirect_io_mfd_cells[count]; > + const struct mfd_cell_acpi_match *acpi_match = > + &acpi_indirect_io_mfd_cell->acpi_match; > + char *name = &acpi_indirect_io_mfd_cell[count].name[0]; > + char *pnpid = &acpi_indirect_io_mfd_cell[count].pnpid[0]; Plain x is equivalent to &x[0]. > + struct mfd_cell_acpi_match match = { > + .pnpid = pnpid, > + }; > + > + snprintf(name, ACPI_INDIRECT_IO_NAME_LEN, "indirect-io-%s", > + acpi_device_hid(child)); > + snprintf(pnpid, ACPI_INDIRECT_IO_NAME_LEN, "%s", > + acpi_device_hid(child)) > + memcpy((void *)acpi_match, (void *)&match, sizeof(*acpi_match)); Casting to void * is pointless. In both cases. > + mfd_cell->name = name; > + mfd_cell->acpi_match = acpi_match; > + > + ret = acpi_indirect_io_set_res(&child->dev, &adev->dev, > + &mfd_cell->resources, > + &mfd_cell->num_resources); > + if (ret) { > + dev_err(&child->dev, "set resource failed (%d)\n", ret); > + goto free_mfd_resources; > + } > + count++; > + } > + > + pdev = acpi_create_platform_device(adev, NULL); > + if (IS_ERR_OR_NULL(pdev)) { > + dev_err(&adev->dev, "create platform device for host failed\n"); > + ret = PTR_ERR(pdev); So, NULL case will return 0. Is it expected? > + goto free_mfd_resources; > + } > + acpi_device_set_enumerated(adev); > + > + ret = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, > + mfd_cells, cell_num, NULL, 0, NULL); > + if (ret) { > + dev_err(&pdev->dev, "failed to add mfd cells (%d)\n", ret); > + goto free_mfd_resources; > + } > + > + return ret; > + > +free_mfd_resources: > + while (cell_num--) > + kfree(mfd_cells[cell_num].resources); > + kfree(mfd_cells); > +free_range: > + kfree(range); > + > + return ret; > +} One question, what a scope of use of this function? Is it ->probe() time? If it's so, can we use devm_* variants?
On 14/02/2018 13:53, Andy Shevchenko wrote: > On Tue, Feb 13, 2018 at 7:45 PM, John Garry <john.garry@huawei.com> 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. >> >> 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. > Hi Andy, >> + unsigned long sys_port; > >> + sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len); >> + if (sys_port == -1UL) > > Wouldn't it be better to compare with ULONG_MAX? Could do, being the same thing. Maybe people prefer -1UL as it saves having to figure out what ULONG_MAX is :) > >> + return -EFAULT; > > >> +/* > > Shouldn't be a kernel-doc? Right, I'll make it /** > >> + * acpi_indirect_io_set_res - set the resources for a child device >> + * (MFD) of an "indirect IO" host. > > In that case this would be one line w/o period at the end. > >> + * @child: the device node to be updated the I/O resource >> + * @hostdev: the device node associated with the "indirect IO" host >> + * @res: double pointer to be set to the address of translated resources >> + * @num_res: pointer to variable to hold the number of translated resources >> + * >> + * Returns 0 when successful, and a negative value for failure. >> + * >> + * For a given "indirect IO" host, each child device will have associated >> + * host-relevative address resource. This function will return the translated >> + * logical PIO addresses for each child devices resources. >> + */ >> +static int acpi_indirect_io_set_res(struct device *child, >> + struct device *hostdev, >> + const struct resource **res, >> + int *num_res) >> +{ >> + struct acpi_device *adev; >> + struct acpi_device *host; >> + struct resource_entry *rentry; >> + LIST_HEAD(resource_list); >> + struct resource *resources; >> + int count; >> + int i; >> + int ret = -EIO; >> + >> + if (!child || !hostdev) >> + return -EINVAL; >> + >> + host = to_acpi_device(hostdev); >> + adev = to_acpi_device(child); > *** >> + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); >> + if (count <= 0) { >> + dev_err(child, "failed to get resources\n"); >> + return count ? count : -EIO; >> + } >> + >> + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); >> + if (!resources) { >> + acpi_dev_free_resource_list(&resource_list); >> + return -ENOMEM; >> + } >> + count = 0; >> + list_for_each_entry(rentry, &resource_list, node) >> + resources[count++] = *rentry->res; >> + >> + acpi_dev_free_resource_list(&resource_list); > > It has similarities with acpi_create_platform_device(). > I guess we can utilize existing code. > For sure, this particular segment is effectively same as part of acpi_create_platform_device(): struct platform_device *acpi_create_platform_device(struct acpi_device *adev, struct property_entry *properties) { struct platform_device *pdev = NULL; struct platform_device_info pdevinfo; struct resource_entry *rentry; struct list_head resource_list; struct resource *resources = NULL; int count; /* If the ACPI node already has a physical device attached, skip it. */ if (adev->physical_node_count) return NULL; if (!acpi_match_device_ids(adev, forbidden_id_list)) return ERR_PTR(-EINVAL); ***> INIT_LIST_HEAD(&resource_list); count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); if (count < 0) { return NULL; } else if (count > 0) { resources = kzalloc(count * sizeof(struct resource), GFP_KERNEL); if (!resources) { dev_err(&adev->dev, "No memory for resources\n"); acpi_dev_free_resource_list(&resource_list); return ERR_PTR(-ENOMEM); } count = 0; list_for_each_entry(rentry, &resource_list, node) acpi_platform_fill_resource(adev, rentry->res, &resources[count++]); acpi_dev_free_resource_list(&resource_list); } <**** memset(&pdevinfo, 0, sizeof(pdevinfo)); /* * If the ACPI node has a parent and that parent has a physical So is your idea to refactor this common segment into a helper function? >> + /* translate the I/O resources */ >> + for (i = 0; i < count; i++) { >> + if (!(resources[i].flags & IORESOURCE_IO)) >> + continue; > >> + ret = acpi_indirect_io_xlat_res(adev, host, &resources[i]); >> + if (ret) { >> + kfree(resources); >> + dev_err(child, "translate IO range failed(%d)\n", ret); >> + return ret; >> + } >> + } >> + *res = resources; >> + *num_res = count; >> + >> + return ret; > > Perhaps, > > ret = ... > if (ret) > break; > } > > if (ret) { > kfree(resources); > dev_err(child, "translate IO range failed(%d)\n", ret); > return ret; > } > > *res = resources; > *num_res = count; > return 0; seems fine > > ? > >> +} >> + >> +/* >> + * acpi_indirect_io_setup - scan handler for "indirect IO" host. >> + * @adev: "indirect IO" host ACPI device pointer >> + * Returns 0 when successful, and a negative value for failure. >> + * >> + * Setup an "indirect IO" host by scanning all child devices, and >> + * create a per-device MFD with logical PIO translated IO resources. >> + */ >> +static int acpi_indirect_io_setup(struct acpi_device *adev) >> +{ >> + struct platform_device *pdev; >> + struct mfd_cell *mfd_cells; >> + struct logic_pio_hwaddr *range; >> + struct acpi_device *child; >> + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cells; >> + int size, ret, count = 0, cell_num = 0; >> + >> + range = kzalloc(sizeof(*range), GFP_KERNEL); >> + if (!range) >> + return -ENOMEM; >> + range->fwnode = &adev->fwnode; >> + range->flags = PIO_INDIRECT; >> + range->size = PIO_INDIRECT_SIZE; >> + >> + ret = logic_pio_register_range(range); >> + if (ret) >> + goto free_range; >> + >> + list_for_each_entry(child, &adev->children, node) >> + cell_num++; >> + >> + /* allocate the mfd cell and companion acpi info, one per child */ >> + size = sizeof(*mfd_cells) + sizeof(*acpi_indirect_io_mfd_cells); >> + mfd_cells = kcalloc(cell_num, size, GFP_KERNEL); >> + if (!mfd_cells) { >> + ret = -ENOMEM; >> + goto free_range; >> + } >> + >> + acpi_indirect_io_mfd_cells = (struct acpi_indirect_io_mfd_cell *) >> + &mfd_cells[cell_num]; >> + /* Only consider the children of the host */ >> + list_for_each_entry(child, &adev->children, node) { >> + struct mfd_cell *mfd_cell = &mfd_cells[count]; >> + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cell = >> + &acpi_indirect_io_mfd_cells[count]; >> + const struct mfd_cell_acpi_match *acpi_match = >> + &acpi_indirect_io_mfd_cell->acpi_match; > >> + char *name = &acpi_indirect_io_mfd_cell[count].name[0]; >> + char *pnpid = &acpi_indirect_io_mfd_cell[count].pnpid[0]; > > Plain x is equivalent to &x[0]. Right, but I thought for arrays that we should use address of x rather than x itself, no? > >> + struct mfd_cell_acpi_match match = { >> + .pnpid = pnpid, >> + }; >> + >> + snprintf(name, ACPI_INDIRECT_IO_NAME_LEN, "indirect-io-%s", >> + acpi_device_hid(child)); >> + snprintf(pnpid, ACPI_INDIRECT_IO_NAME_LEN, "%s", >> + acpi_device_hid(child)) > >> + memcpy((void *)acpi_match, (void *)&match, sizeof(*acpi_match)); > > Casting to void * is pointless. In both cases. I rechecked this. The casting to void * was there to mask another issue which I've now fixed. > >> + mfd_cell->name = name; >> + mfd_cell->acpi_match = acpi_match; >> + >> + ret = acpi_indirect_io_set_res(&child->dev, &adev->dev, >> + &mfd_cell->resources, >> + &mfd_cell->num_resources); >> + if (ret) { >> + dev_err(&child->dev, "set resource failed (%d)\n", ret); >> + goto free_mfd_resources; >> + } >> + count++; >> + } >> + >> + pdev = acpi_create_platform_device(adev, NULL); >> + if (IS_ERR_OR_NULL(pdev)) { >> + dev_err(&adev->dev, "create platform device for host failed\n"); > >> + ret = PTR_ERR(pdev); > > So, NULL case will return 0. Is it expected? > Should error in that case also, so I'll change. >> + goto free_mfd_resources; >> + } >> + acpi_device_set_enumerated(adev); >> + >> + ret = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, >> + mfd_cells, cell_num, NULL, 0, NULL); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to add mfd cells (%d)\n", ret); >> + goto free_mfd_resources; >> + } >> + >> + return ret; >> + >> +free_mfd_resources: >> + while (cell_num--) >> + kfree(mfd_cells[cell_num].resources); >> + kfree(mfd_cells); >> +free_range: >> + kfree(range); >> + >> + return ret; >> +} > > One question, what a scope of use of this function? Is it ->probe() time? > If it's so, can we use devm_* variants? It is called from a scan handler, so prior to device probing. > Thanks, John
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. 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. 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). 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. 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 > > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > index 1017def..f4a7f46 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_indirectio.o > diff --git a/drivers/acpi/arm64/acpi_indirectio.c b/drivers/acpi/arm64/acpi_indirectio.c > new file mode 100644 > index 0000000..51a1b92 > --- /dev/null > +++ b/drivers/acpi/arm64/acpi_indirectio.c > @@ -0,0 +1,250 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. > + * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com> > + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com> > + * Author: John Garry <john.garry@huawei.com> > + * > + * This file implements functunality to scan the ACPI namespace and config > + * devices under "indirect IO" hosts. An "indirect IO" host allows child > + * devices to use logical IO accesses when the host, itself, does not provide > + * a transparent bridge. The device setup creates a per-child MFD with a > + * logical port IO resource. > + */ > + > +#include <linux/acpi.h> > +#include <linux/logic_pio.h> > +#include <linux/mfd/core.h> > +#include <linux/platform_device.h> > + > +ACPI_MODULE_NAME("indirect IO"); > + > +#define ACPI_INDIRECT_IO_NAME_LEN 255 > + > +struct acpi_indirect_io_mfd_cell { > + struct mfd_cell_acpi_match acpi_match; > + char name[ACPI_INDIRECT_IO_NAME_LEN]; > + char pnpid[ACPI_INDIRECT_IO_NAME_LEN]; > +}; > + > +static int acpi_indirect_io_xlat_res(struct acpi_device *adev, > + struct acpi_device *host, > + struct resource *res) > +{ > + unsigned long sys_port; > + resource_size_t len = res->end - res->start; > + > + sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len); > + if (sys_port == -1UL) > + return -EFAULT; > + > + res->start = sys_port; > + res->end = sys_port + len; > + > + return 0; > +} > + > +/* > + * acpi_indirect_io_set_res - set the resources for a child device > + * (MFD) of an "indirect IO" host. > + * @child: the device node to be updated the I/O resource > + * @hostdev: the device node associated with the "indirect IO" host > + * @res: double pointer to be set to the address of translated resources > + * @num_res: pointer to variable to hold the number of translated resources > + * > + * Returns 0 when successful, and a negative value for failure. > + * > + * For a given "indirect IO" host, each child device will have associated > + * host-relevative address resource. This function will return the translated > + * logical PIO addresses for each child devices resources. > + */ > +static int acpi_indirect_io_set_res(struct device *child, > + struct device *hostdev, > + const struct resource **res, > + int *num_res) > +{ > + struct acpi_device *adev; > + struct acpi_device *host; > + struct resource_entry *rentry; > + LIST_HEAD(resource_list); > + struct resource *resources; > + int count; > + int i; > + int ret = -EIO; > + > + 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, "device is not present\n"); > + return 0; > + } > + /* whether the child had been enumerated? */ > + if (acpi_device_enumerated(adev)) { > + dev_info(child, "had been enumerated\n"); > + return 0; > + } > + > + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); > + if (count <= 0) { > + dev_err(child, "failed to get resources\n"); > + return count ? count : -EIO; > + } > + > + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); > + if (!resources) { > + acpi_dev_free_resource_list(&resource_list); > + return -ENOMEM; > + } > + count = 0; > + list_for_each_entry(rentry, &resource_list, node) > + resources[count++] = *rentry->res; > + > + acpi_dev_free_resource_list(&resource_list); > + > + /* translate the I/O resources */ > + for (i = 0; i < count; i++) { > + if (!(resources[i].flags & IORESOURCE_IO)) > + continue; > + ret = acpi_indirect_io_xlat_res(adev, host, &resources[i]); > + if (ret) { > + kfree(resources); > + dev_err(child, "translate IO range failed(%d)\n", ret); > + return ret; > + } > + } > + *res = resources; > + *num_res = count; > + > + return ret; > +} > + > +/* > + * acpi_indirect_io_setup - scan handler for "indirect IO" host. > + * @adev: "indirect IO" host ACPI device pointer > + * Returns 0 when successful, and a negative value for failure. > + * > + * Setup an "indirect IO" host by scanning all child devices, and > + * create a per-device MFD with logical PIO translated IO resources. > + */ > +static int acpi_indirect_io_setup(struct acpi_device *adev) > +{ > + struct platform_device *pdev; > + struct mfd_cell *mfd_cells; > + struct logic_pio_hwaddr *range; > + struct acpi_device *child; > + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cells; > + int size, ret, count = 0, cell_num = 0; > + > + range = kzalloc(sizeof(*range), GFP_KERNEL); > + if (!range) > + return -ENOMEM; > + range->fwnode = &adev->fwnode; > + range->flags = PIO_INDIRECT; > + range->size = PIO_INDIRECT_SIZE; > + > + ret = logic_pio_register_range(range); > + if (ret) > + goto free_range; > + > + list_for_each_entry(child, &adev->children, node) > + cell_num++; > + > + /* allocate the mfd cell and companion acpi info, one per child */ > + size = sizeof(*mfd_cells) + sizeof(*acpi_indirect_io_mfd_cells); > + mfd_cells = kcalloc(cell_num, size, GFP_KERNEL); > + if (!mfd_cells) { > + ret = -ENOMEM; > + goto free_range; > + } > + > + acpi_indirect_io_mfd_cells = (struct acpi_indirect_io_mfd_cell *) > + &mfd_cells[cell_num]; > + /* Only consider the children of the host */ > + list_for_each_entry(child, &adev->children, node) { > + struct mfd_cell *mfd_cell = &mfd_cells[count]; > + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cell = > + &acpi_indirect_io_mfd_cells[count]; > + const struct mfd_cell_acpi_match *acpi_match = > + &acpi_indirect_io_mfd_cell->acpi_match; > + char *name = &acpi_indirect_io_mfd_cell[count].name[0]; > + char *pnpid = &acpi_indirect_io_mfd_cell[count].pnpid[0]; > + struct mfd_cell_acpi_match match = { > + .pnpid = pnpid, > + }; > + > + snprintf(name, ACPI_INDIRECT_IO_NAME_LEN, "indirect-io-%s", > + acpi_device_hid(child)); > + snprintf(pnpid, ACPI_INDIRECT_IO_NAME_LEN, "%s", > + acpi_device_hid(child)); > + > + memcpy((void *)acpi_match, (void *)&match, sizeof(*acpi_match)); > + mfd_cell->name = name; > + mfd_cell->acpi_match = acpi_match; > + > + ret = acpi_indirect_io_set_res(&child->dev, &adev->dev, > + &mfd_cell->resources, > + &mfd_cell->num_resources); > + if (ret) { > + dev_err(&child->dev, "set resource failed (%d)\n", ret); > + goto free_mfd_resources; > + } > + count++; > + } > + > + pdev = acpi_create_platform_device(adev, NULL); > + if (IS_ERR_OR_NULL(pdev)) { > + dev_err(&adev->dev, "create platform device for host failed\n"); > + ret = PTR_ERR(pdev); > + goto free_mfd_resources; > + } > + acpi_device_set_enumerated(adev); > + > + ret = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, > + mfd_cells, cell_num, NULL, 0, NULL); > + if (ret) { > + dev_err(&pdev->dev, "failed to add mfd cells (%d)\n", ret); > + goto free_mfd_resources; > + } > + > + return ret; > + > +free_mfd_resources: > + while (cell_num--) > + kfree(mfd_cells[cell_num].resources); > + kfree(mfd_cells); > +free_range: > + kfree(range); > + > + return ret; > +} > + > +/* All the host devices which apply indirect-IO can be listed here. */ > +static const struct acpi_device_id acpi_indirect_io_host_id[] = { > + {} > +}; > + > +static int acpi_indirect_io_attach(struct acpi_device *adev, > + const struct acpi_device_id *id) > +{ > + int ret = acpi_indirect_io_setup(adev); > + > + if (ret < 0) > + return ret; > + > + return 1; > +} > + > +static struct acpi_scan_handler acpi_indirect_io_handler = { > + .ids = acpi_indirect_io_host_id, > + .attach = acpi_indirect_io_attach, > +}; > + > +void __init acpi_indirect_io_scan_init(void) > +{ > + acpi_scan_add_handler(&acpi_indirect_io_handler); > +} > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > index 1d0a501..680f3cf 100644 > --- a/drivers/acpi/internal.h > +++ b/drivers/acpi/internal.h > @@ -31,6 +31,11 @@ > void acpi_platform_init(void); > void acpi_pnp_init(void); > void acpi_int340x_thermal_init(void); > +#ifdef CONFIG_INDIRECT_PIO > +void acpi_indirect_io_scan_init(void); > +#else > +static inline void acpi_indirect_io_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 8e63d93..204da8a 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -2155,6 +2155,7 @@ int __init acpi_scan_init(void) > acpi_amba_init(); > acpi_watchdog_init(); > acpi_init_lpit(); > + acpi_indirect_io_scan_init(); > > acpi_scan_add_handler(&generic_device_handler); > > -- > 1.9.1 >
On Wed, Feb 14, 2018 at 5:33 PM, John Garry <john.garry@huawei.com> wrote: > On 14/02/2018 13:53, Andy Shevchenko wrote: >> On Tue, Feb 13, 2018 at 7:45 PM, John Garry <john.garry@huawei.com> wrote: >>> + sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, >>> len); >>> + if (sys_port == -1UL) >> Wouldn't it be better to compare with ULONG_MAX? > Could do, being the same thing. Maybe people prefer -1UL as it saves having > to figure out what ULONG_MAX is :) -1UL looks confusing. Another approach is to use ~0UL if that is preferable. >>> + list_for_each_entry(rentry, &resource_list, node) >>> + resources[count++] = *rentry->res; >> It has similarities with acpi_create_platform_device(). >> I guess we can utilize existing code. > For sure, this particular segment is effectively same as part of > acpi_create_platform_device(): Not the same, acpi_create_platform_device() does a bit more than copying the resources. If it indeed makes no hurt... > list_for_each_entry(rentry, &resource_list, node) > acpi_platform_fill_resource(adev, rentry->res, > &resources[count++]); > So is your idea to refactor this common segment into a helper function? ...I would go with helper. >>> + char *name = &acpi_indirect_io_mfd_cell[count].name[0]; >>> + char *pnpid = &acpi_indirect_io_mfd_cell[count].pnpid[0]; >> >> >> Plain x is equivalent to &x[0]. > Right, but I thought for arrays that we should use address of x rather than > x itself, no? x is addressed by it's beginning.
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
> 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)" > Hi Rafael, Lorenzo, I can avoid adding the scan handler in acpi_indirectio.c by skipping the child enumeration, like with this change in scan.c: +static const struct acpi_device_id indirect_io_hosts[] = { + {"HISI0191", 0}, /* HiSilicon LPC host */ + {}, +}; + +static bool acpi_is_indirect_io_slave(struct acpi_device *device) +{ + struct acpi_device *parent = dev->parent; + + if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) + return false; + + return true; +} + static bool acpi_is_serial_bus_slave(struct acpi_device *device) { struct list_head resource_list; bool is_serial_bus_slave = false; + if (acpi_is_indirect_io_slave(device)) + return true; + /* Macs use device properties in lieu of _CRS resources */ This means I can move all this scan code into the LLDD. What do you think? Please let me know. John
On Thu, Feb 15, 2018 at 12:19 PM, John Garry <john.garry@huawei.com> wrote: >> 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)" >> > > Hi Rafael, Lorenzo, > > I can avoid adding the scan handler in acpi_indirectio.c by skipping the > child enumeration, like with this change in scan.c: > > +static const struct acpi_device_id indirect_io_hosts[] = { > + {"HISI0191", 0}, /* HiSilicon LPC host */ > + {}, > +}; > + > +static bool acpi_is_indirect_io_slave(struct acpi_device *device) > +{ Why don't you put the table definition here? > + struct acpi_device *parent = dev->parent; > + > + if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) > + return false; > + > + return true; return parent && !acpi_match_device_ids(parent, indirect_io_hosts); > +} > + > static bool acpi_is_serial_bus_slave(struct acpi_device *device) > { > struct list_head resource_list; > bool is_serial_bus_slave = false; > > + if (acpi_is_indirect_io_slave(device)) > + return true; > + > /* Macs use device properties in lieu of _CRS resources */ > > > This means I can move all this scan code into the LLDD. > > What do you think? Please let me know. If Lorenzo agrees, that will be fine by me modulo the above remarks.
On Thu, Feb 15, 2018 at 1:19 PM, John Garry <john.garry@huawei.com> wrote: > +static const struct acpi_device_id indirect_io_hosts[] = { > + {"HISI0191", 0}, /* HiSilicon LPC host */ > + {}, Just a nit. It seems lately this happens more often than usual, I mean a comma in the terminator line. If we remove it. we terminate not only at runtime, but at compile time, which is slightly better. > +};
On Thu, Feb 15, 2018 at 12:47:25PM +0100, Rafael J. Wysocki wrote: > On Thu, Feb 15, 2018 at 12:19 PM, John Garry <john.garry@huawei.com> wrote: > >> 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)" > >> > > > > Hi Rafael, Lorenzo, > > > > I can avoid adding the scan handler in acpi_indirectio.c by skipping the > > child enumeration, like with this change in scan.c: > > > > +static const struct acpi_device_id indirect_io_hosts[] = { > > + {"HISI0191", 0}, /* HiSilicon LPC host */ > > + {}, > > +}; > > + > > +static bool acpi_is_indirect_io_slave(struct acpi_device *device) > > +{ > > Why don't you put the table definition here? > > > + struct acpi_device *parent = dev->parent; > > + > > + if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) > > + return false; > > + > > + return true; > > return parent && !acpi_match_device_ids(parent, indirect_io_hosts); > > > +} > > + > > static bool acpi_is_serial_bus_slave(struct acpi_device *device) > > { > > struct list_head resource_list; > > bool is_serial_bus_slave = false; > > > > + if (acpi_is_indirect_io_slave(device)) > > + return true; > > + > > /* Macs use device properties in lieu of _CRS resources */ > > > > > > This means I can move all this scan code into the LLDD. > > > > What do you think? Please let me know. > > If Lorenzo agrees, that will be fine by me modulo the above remarks. I agree and I thank you for accepting this in core ACPI code, I think that's much cleaner than a driver specific scan hook. It is a shame we do not have a generic identifier for such bus in ACPI but that won't happen overnight anyway (if ever, I think a binding for LPC in the ACPI specs is hard to justify). Thank you, Lorenzo
On 15/02/2018 12:22, Andy Shevchenko wrote: > On Thu, Feb 15, 2018 at 1:19 PM, John Garry <john.garry@huawei.com> wrote: > Hi Andy, >> +static const struct acpi_device_id indirect_io_hosts[] = { >> + {"HISI0191", 0}, /* HiSilicon LPC host */ >> + {}, > > Just a nit. > I noticed that I have this in the LLDD also. I think that this may be a force of habit. > It seems lately this happens more often than usual, I mean a comma in > the terminator line. > > If we remove it. we terminate not only at runtime, but at compile > time, which is slightly better. I grepped for "{}," in the drivers folder and it gives many results (I do accept that some are not sentinels): >grep -R "{}," * | tail watchdog/gef_wdt.c: {}, watchdog/asm9260_wdt.c: {}, watchdog/bcm2835_wdt.c: {}, watchdog/digicolor_wdt.c: {}, watchdog/bcm7038_wdt.c: {}, watchdog/ath79_wdt.c: {}, watchdog/riowd.c: {}, watchdog/sbsa_gwdt.c: {}, watchdog/sbsa_gwdt.c: {}, watchdog/bcm_kona_wdt.c: {}, Much appreciated, John
On Thu, Feb 15, 2018 at 2:52 PM, John Garry <john.garry@huawei.com> wrote: > On 15/02/2018 12:22, Andy Shevchenko wrote: >> On Thu, Feb 15, 2018 at 1:19 PM, John Garry <john.garry@huawei.com> wrote: >>> +static const struct acpi_device_id indirect_io_hosts[] = { >>> + {"HISI0191", 0}, /* HiSilicon LPC host */ >>> + {}, >> >> >> Just a nit. >> > > I noticed that I have this in the LLDD also. I think that this may be a > force of habit. >> It seems lately this happens more often than usual, I mean a comma in >> the terminator line. >> >> If we remove it. we terminate not only at runtime, but at compile >> time, which is slightly better. > > > I grepped for "{}," in the drivers folder and it gives many results (I do > accept that some are not sentinels): Yes, in old code it might be, but why to cargo-culting bad (okay, less worth) practices? >>grep -R "{}," * | tail Btw, `git grep ...` is much faster. > watchdog/gef_wdt.c: {}, > watchdog/asm9260_wdt.c: {}, > watchdog/bcm2835_wdt.c: {}, > watchdog/digicolor_wdt.c: {}, > watchdog/bcm7038_wdt.c: {}, > watchdog/ath79_wdt.c: {}, > watchdog/riowd.c: {}, > watchdog/sbsa_gwdt.c: {}, > watchdog/sbsa_gwdt.c: {}, > watchdog/bcm_kona_wdt.c: {},
On 15/02/2018 11:47, Rafael J. Wysocki wrote: > On Thu, Feb 15, 2018 at 12:19 PM, John Garry <john.garry@huawei.com> wrote: >>> 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)" >>> >> >> Hi Rafael, Lorenzo, >> >> I can avoid adding the scan handler in acpi_indirectio.c by skipping the >> child enumeration, like with this change in scan.c: >> Hi Rafael, >> +static const struct acpi_device_id indirect_io_hosts[] = { >> + {"HISI0191", 0}, /* HiSilicon LPC host */ >> + {}, >> +}; >> + >> +static bool acpi_is_indirect_io_slave(struct acpi_device *device) >> +{ > > Why don't you put the table definition here? > I can do. >> + struct acpi_device *parent = dev->parent; >> + >> + if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) >> + return false; >> + >> + return true; > > return parent && !acpi_match_device_ids(parent, indirect_io_hosts); Fine, a bit more concise > >> +} >> + >> static bool acpi_is_serial_bus_slave(struct acpi_device *device) >> { >> struct list_head resource_list; >> bool is_serial_bus_slave = false; >> >> + if (acpi_is_indirect_io_slave(device)) >> + return true; >> + >> /* Macs use device properties in lieu of _CRS resources */ >> >> >> This means I can move all this scan code into the LLDD. >> >> What do you think? Please let me know. > > If Lorenzo agrees, that will be fine by me modulo the above remarks. > > . I see Lorenzo also finds this ok, so I'll go with that. Thanks to all, John >
On 14/02/2018 16:16, Andy Shevchenko wrote: > Another approach is to use ~0UL if that is preferable. > >>>> >>> + list_for_each_entry(rentry, &resource_list, node) >>>> >>> + resources[count++] = *rentry->res; >>> >> It has similarities with acpi_create_platform_device(). >>> >> I guess we can utilize existing code. >> > For sure, this particular segment is effectively same as part of >> > acpi_create_platform_device(): > Not the same, acpi_create_platform_device() does a bit more than > copying the resources. If it indeed makes no hurt... > >> > list_for_each_entry(rentry, &resource_list, node) >> > acpi_platform_fill_resource(adev, rentry->res, >> > &resources[count++]); >> > So is your idea to refactor this common segment into a helper function? > ...I would go with helper. > Hi Andy, Since the plan now is that this code is no longer going to be added to drivers/acpi, but instead pushed to the LLDD, I am pondering whether we should still factor out of this common code. Opinion? Thanks, John >>>> >>> + char *name = &acpi_indirect_io_mfd_cell[count].name[0];
On Thu, Feb 15, 2018 at 7:07 PM, John Garry <john.garry@huawei.com> wrote: > On 14/02/2018 16:16, Andy Shevchenko wrote: >>>>> >>> + list_for_each_entry(rentry, &resource_list, node) >>>>> >>> + resources[count++] = *rentry->res; >>>> >>>> >> It has similarities with acpi_create_platform_device(). >>>> >> I guess we can utilize existing code. >>> >>> > For sure, this particular segment is effectively same as part of >>> > acpi_create_platform_device(): >> >> Not the same, acpi_create_platform_device() does a bit more than >> copying the resources. If it indeed makes no hurt... >> >>> > list_for_each_entry(rentry, &resource_list, node) >>> > acpi_platform_fill_resource(adev, rentry->res, >>> > &resources[count++]); >>> > So is your idea to refactor this common segment into a helper function? >> >> ...I would go with helper. >> > > Hi Andy, > > Since the plan now is that this code is no longer going to be added to > drivers/acpi, but instead pushed to the LLDD, I am pondering whether we > should still factor out of this common code. Opinion? I would still go with a common helper. Though, as first step, we can make it lazy, i.e. put a comment in your code, like a todo notice (w/o TODO word :-) ) to consider a common helper.
On 16/02/2018 14:42, Andy Shevchenko wrote: > On Thu, Feb 15, 2018 at 7:07 PM, John Garry <john.garry@huawei.com> wrote: >> On 14/02/2018 16:16, Andy Shevchenko wrote: > >>>>>>>>> + list_for_each_entry(rentry, &resource_list, node) >>>>>>>>> + resources[count++] = *rentry->res; >>>>> >>>>>>> It has similarities with acpi_create_platform_device(). >>>>>>> I guess we can utilize existing code. >>>> >>>>> For sure, this particular segment is effectively same as part of >>>>> acpi_create_platform_device(): >>> >>> Not the same, acpi_create_platform_device() does a bit more than >>> copying the resources. If it indeed makes no hurt... >>> >>>>> list_for_each_entry(rentry, &resource_list, node) >>>>> acpi_platform_fill_resource(adev, rentry->res, >>>>> &resources[count++]); >>>>> So is your idea to refactor this common segment into a helper function? >>> >>> ...I would go with helper. >>> >> >> Hi Andy, >> >> Since the plan now is that this code is no longer going to be added to >> drivers/acpi, but instead pushed to the LLDD, I am pondering whether we >> should still factor out of this common code. Opinion? > > I would still go with a common helper. Though, as first step, we can > make it lazy, i.e. put a comment in your code, like a todo notice (w/o > TODO word :-) ) to consider a common helper. Fine, I was also thinking that I don't want to do this now as it could make merging the patchset more complex. For now, the ACPI change I plan creates no dependencies. Cheers, John >
diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile index 1017def..f4a7f46 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_indirectio.o diff --git a/drivers/acpi/arm64/acpi_indirectio.c b/drivers/acpi/arm64/acpi_indirectio.c new file mode 100644 index 0000000..51a1b92 --- /dev/null +++ b/drivers/acpi/arm64/acpi_indirectio.c @@ -0,0 +1,250 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. + * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com> + * Author: John Garry <john.garry@huawei.com> + * + * This file implements functunality to scan the ACPI namespace and config + * devices under "indirect IO" hosts. An "indirect IO" host allows child + * devices to use logical IO accesses when the host, itself, does not provide + * a transparent bridge. The device setup creates a per-child MFD with a + * logical port IO resource. + */ + +#include <linux/acpi.h> +#include <linux/logic_pio.h> +#include <linux/mfd/core.h> +#include <linux/platform_device.h> + +ACPI_MODULE_NAME("indirect IO"); + +#define ACPI_INDIRECT_IO_NAME_LEN 255 + +struct acpi_indirect_io_mfd_cell { + struct mfd_cell_acpi_match acpi_match; + char name[ACPI_INDIRECT_IO_NAME_LEN]; + char pnpid[ACPI_INDIRECT_IO_NAME_LEN]; +}; + +static int acpi_indirect_io_xlat_res(struct acpi_device *adev, + struct acpi_device *host, + struct resource *res) +{ + unsigned long sys_port; + resource_size_t len = res->end - res->start; + + sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len); + if (sys_port == -1UL) + return -EFAULT; + + res->start = sys_port; + res->end = sys_port + len; + + return 0; +} + +/* + * acpi_indirect_io_set_res - set the resources for a child device + * (MFD) of an "indirect IO" host. + * @child: the device node to be updated the I/O resource + * @hostdev: the device node associated with the "indirect IO" host + * @res: double pointer to be set to the address of translated resources + * @num_res: pointer to variable to hold the number of translated resources + * + * Returns 0 when successful, and a negative value for failure. + * + * For a given "indirect IO" host, each child device will have associated + * host-relevative address resource. This function will return the translated + * logical PIO addresses for each child devices resources. + */ +static int acpi_indirect_io_set_res(struct device *child, + struct device *hostdev, + const struct resource **res, + int *num_res) +{ + struct acpi_device *adev; + struct acpi_device *host; + struct resource_entry *rentry; + LIST_HEAD(resource_list); + struct resource *resources; + int count; + int i; + int ret = -EIO; + + 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, "device is not present\n"); + return 0; + } + /* whether the child had been enumerated? */ + if (acpi_device_enumerated(adev)) { + dev_info(child, "had been enumerated\n"); + return 0; + } + + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); + if (count <= 0) { + dev_err(child, "failed to get resources\n"); + return count ? count : -EIO; + } + + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); + if (!resources) { + acpi_dev_free_resource_list(&resource_list); + return -ENOMEM; + } + count = 0; + list_for_each_entry(rentry, &resource_list, node) + resources[count++] = *rentry->res; + + acpi_dev_free_resource_list(&resource_list); + + /* translate the I/O resources */ + for (i = 0; i < count; i++) { + if (!(resources[i].flags & IORESOURCE_IO)) + continue; + ret = acpi_indirect_io_xlat_res(adev, host, &resources[i]); + if (ret) { + kfree(resources); + dev_err(child, "translate IO range failed(%d)\n", ret); + return ret; + } + } + *res = resources; + *num_res = count; + + return ret; +} + +/* + * acpi_indirect_io_setup - scan handler for "indirect IO" host. + * @adev: "indirect IO" host ACPI device pointer + * Returns 0 when successful, and a negative value for failure. + * + * Setup an "indirect IO" host by scanning all child devices, and + * create a per-device MFD with logical PIO translated IO resources. + */ +static int acpi_indirect_io_setup(struct acpi_device *adev) +{ + struct platform_device *pdev; + struct mfd_cell *mfd_cells; + struct logic_pio_hwaddr *range; + struct acpi_device *child; + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cells; + int size, ret, count = 0, cell_num = 0; + + range = kzalloc(sizeof(*range), GFP_KERNEL); + if (!range) + return -ENOMEM; + range->fwnode = &adev->fwnode; + range->flags = PIO_INDIRECT; + range->size = PIO_INDIRECT_SIZE; + + ret = logic_pio_register_range(range); + if (ret) + goto free_range; + + list_for_each_entry(child, &adev->children, node) + cell_num++; + + /* allocate the mfd cell and companion acpi info, one per child */ + size = sizeof(*mfd_cells) + sizeof(*acpi_indirect_io_mfd_cells); + mfd_cells = kcalloc(cell_num, size, GFP_KERNEL); + if (!mfd_cells) { + ret = -ENOMEM; + goto free_range; + } + + acpi_indirect_io_mfd_cells = (struct acpi_indirect_io_mfd_cell *) + &mfd_cells[cell_num]; + /* Only consider the children of the host */ + list_for_each_entry(child, &adev->children, node) { + struct mfd_cell *mfd_cell = &mfd_cells[count]; + struct acpi_indirect_io_mfd_cell *acpi_indirect_io_mfd_cell = + &acpi_indirect_io_mfd_cells[count]; + const struct mfd_cell_acpi_match *acpi_match = + &acpi_indirect_io_mfd_cell->acpi_match; + char *name = &acpi_indirect_io_mfd_cell[count].name[0]; + char *pnpid = &acpi_indirect_io_mfd_cell[count].pnpid[0]; + struct mfd_cell_acpi_match match = { + .pnpid = pnpid, + }; + + snprintf(name, ACPI_INDIRECT_IO_NAME_LEN, "indirect-io-%s", + acpi_device_hid(child)); + snprintf(pnpid, ACPI_INDIRECT_IO_NAME_LEN, "%s", + acpi_device_hid(child)); + + memcpy((void *)acpi_match, (void *)&match, sizeof(*acpi_match)); + mfd_cell->name = name; + mfd_cell->acpi_match = acpi_match; + + ret = acpi_indirect_io_set_res(&child->dev, &adev->dev, + &mfd_cell->resources, + &mfd_cell->num_resources); + if (ret) { + dev_err(&child->dev, "set resource failed (%d)\n", ret); + goto free_mfd_resources; + } + count++; + } + + pdev = acpi_create_platform_device(adev, NULL); + if (IS_ERR_OR_NULL(pdev)) { + dev_err(&adev->dev, "create platform device for host failed\n"); + ret = PTR_ERR(pdev); + goto free_mfd_resources; + } + acpi_device_set_enumerated(adev); + + ret = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, + mfd_cells, cell_num, NULL, 0, NULL); + if (ret) { + dev_err(&pdev->dev, "failed to add mfd cells (%d)\n", ret); + goto free_mfd_resources; + } + + return ret; + +free_mfd_resources: + while (cell_num--) + kfree(mfd_cells[cell_num].resources); + kfree(mfd_cells); +free_range: + kfree(range); + + return ret; +} + +/* All the host devices which apply indirect-IO can be listed here. */ +static const struct acpi_device_id acpi_indirect_io_host_id[] = { + {} +}; + +static int acpi_indirect_io_attach(struct acpi_device *adev, + const struct acpi_device_id *id) +{ + int ret = acpi_indirect_io_setup(adev); + + if (ret < 0) + return ret; + + return 1; +} + +static struct acpi_scan_handler acpi_indirect_io_handler = { + .ids = acpi_indirect_io_host_id, + .attach = acpi_indirect_io_attach, +}; + +void __init acpi_indirect_io_scan_init(void) +{ + acpi_scan_add_handler(&acpi_indirect_io_handler); +} diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 1d0a501..680f3cf 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -31,6 +31,11 @@ void acpi_platform_init(void); void acpi_pnp_init(void); void acpi_int340x_thermal_init(void); +#ifdef CONFIG_INDIRECT_PIO +void acpi_indirect_io_scan_init(void); +#else +static inline void acpi_indirect_io_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 8e63d93..204da8a 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -2155,6 +2155,7 @@ int __init acpi_scan_init(void) acpi_amba_init(); acpi_watchdog_init(); acpi_init_lpit(); + acpi_indirect_io_scan_init(); acpi_scan_add_handler(&generic_device_handler);