From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Garry Subject: Re: [PATCH v12 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning Date: Mon, 5 Feb 2018 11:01:50 +0000 Message-ID: <8beedbb3-9098-6440-9ddf-e64b2a8e5151@huawei.com> References: <1516725385-24535-1-git-send-email-john.garry@huawei.com> <1516725385-24535-8-git-send-email-john.garry@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-acpi-owner@vger.kernel.org To: "Rafael J. Wysocki" Cc: Mika Westerberg , Lorenzo Pieralisi , "Rafael J. Wysocki" , Rob Herring , Bjorn Helgaas , Arnd Bergmann , Mark Rutland , Olof Johansson , Hanjun Guo , "dann.frazier@canonical.com" , Benjamin Herrenschmidt , Linux Kernel Mailing List , ACPI Devel Maling List , Linuxarm , Linux PCI , Corey Minyard , "devicetree@vger.kernel.org" , linux-arch List-Id: devicetree@vger.kernel.org On 04/02/2018 07:45, Rafael J. Wysocki wrote: > On Tue, Jan 23, 2018 at 5:36 PM, John Garry wrote: >> 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. > Hi Rafael, Thanks for checking. > The changelog is somewhat dry for a patch adding over 300 lines of new > code and a new file for that matter. Will add more. Indeed MFD is not even mentioned. > >> Signed-off-by: John Garry >> Signed-off-by: Zhichang Yuan >> Signed-off-by: Gabriele Paoloni >> --- >> drivers/acpi/arm64/Makefile | 1 + >> drivers/acpi/arm64/acpi_indirectio.c | 273 +++++++++++++++++++++++++++++++++++ >> drivers/acpi/internal.h | 5 + >> drivers/acpi/scan.c | 1 + >> 4 files changed, 280 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..2649f57 >> --- /dev/null >> +++ b/drivers/acpi/arm64/acpi_indirectio.c >> @@ -0,0 +1,273 @@ > > SPDX license identifier here? Will fix. I also need to fixup the other new files in the patchset. > >> +/* >> + * ACPI support for indirect-IO bus. >> + * >> + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. >> + * Author: Gabriele Paoloni >> + * Author: Zhichang Yuan >> + * Author: John Garry >> + * >> + * 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. > > And then you can skip the above. > > Also I would like to see some description of what's there in this file > to appear here. Sure. > >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> + >> +ACPI_MODULE_NAME("indirect IO"); >> + >> +#define ACPI_INDIRECTIO_NAME_LENGTH 255 >> + >> +#define INDIRECT_IO_INFO(desc) ((unsigned long)&desc) >> + >> +struct acpi_indirectio_mfd_cell { >> + struct mfd_cell_acpi_match acpi_match; >> + char name[ACPI_INDIRECTIO_NAME_LENGTH]; >> + char pnpid[ACPI_INDIRECTIO_NAME_LENGTH]; >> +}; >> + >> +struct acpi_indirectio_host_data { >> + resource_size_t io_size; >> + resource_size_t io_start; >> +}; >> + >> +struct acpi_indirectio_device_desc { > > Why don't you use a consistent naming convention and call this > acpi_indirect_io_device_desc (and analogously everywhere above and > below)? Right, I can correct the file to have consistent symbol prefixes. > >> + struct acpi_indirectio_host_data pdata; /* device relevant info data */ >> + int (*pre_setup)(struct acpi_device *adev, >> + struct acpi_indirectio_host_data *pdata); >> +}; >> + >> +static int acpi_translate_logicio_res(struct acpi_device *adev, >> + struct acpi_device *host, struct resource *resource) >> +{ >> + unsigned long sys_port; >> + struct device *dev = &adev->dev; >> + resource_size_t length = resource->end - resource->start; >> + >> + sys_port = logic_pio_trans_hwaddr(&host->fwnode, resource->start, >> + length); >> + >> + if (sys_port == -1) { > > Would if (sysp_port < 0) not work here? I don't think so because sys_port is an unsigned value. I should change this if statement to use -1UL. > >> + dev_err(dev, "translate bus-addr(0x%llx) fail!\n", >> + resource->start); > > That's not a very informative message. What are users expected to do > in response to seeing it? Right, the user generally would not be able to decipher this, so I should add some more info on what the fallout will be. > >> + return -EFAULT; >> + } >> + >> + resource->start = sys_port; >> + resource->end = sys_port + length; >> + >> + 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. >> + * >> + * @child: the device node to be updated the I/O resource; >> + * @hostdev: the device node where 'adev' is attached, which can be not >> + * the parent of 'adev'; >> + * @res: double pointer to be set to the address of the updated resources >> + * @num_res: address of the variable to contain the number of updated resources >> + * >> + * return 0 when successful, negative is for failure. >> + */ > > The above should be a proper kerneldoc comment. > >> +int acpi_indirectio_set_logicio_res(struct device *child, This should actually be static, so I think we can avoid the kerneldoc comment? >> + 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 = NULL; >> + 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, "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; >> + } >> + >> + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); >> + if (count <= 0) { >> + dev_err(&adev->dev, "failed to get ACPI resources\n"); >> + return count ? count : -EIO; >> + } >> + >> + resources = kcalloc(count, sizeof(struct resource), 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) { >> + ret = acpi_translate_logicio_res(adev, host, >> + &resources[i]); > > You don't need to break this line as far as I'm concerned. This is just to keep checkpatch happy. I could move the complete function call to a single line. And also shortening some symbols will help. > >> + if (ret) { >> + kfree(resources); >> + dev_err(child, >> + "Translate I/O range failed (%d)!\n", >> + ret); > > And this too. Again, just appeasing checkpatch > >> + return ret; >> + } >> + } >> + } >> + *res = resources; >> + *num_res = count; >> + >> + return ret; >> +} >> + >> +int > > Don't break the line here, please. Alright, will do. I thought that this was an accepted style when the arguments list is quite long. > >> +acpi_indirectio_pre_setup(struct acpi_device *adev, >> + struct acpi_indirectio_host_data *pdata) > > As a non-static function, this requires a kerneldoc comment. In fact this should be static. I should have fixed these already. > > In particular, the comment should describe who and when is expected to > call this function (and which also applies to the comment preceding > the previous function). > > Apparently, they both need to be called before the initial namespace > scan for the acpi_indirectio_attach() below to work, right? > Right, this needs to be prior to device enumeration in the ACPI namespace. >> +{ >> + struct platform_device *pdev; >> + struct mfd_cell *mfd_cells; >> + struct logic_pio_hwaddr *range; >> + struct acpi_device *child; >> + struct acpi_indirectio_mfd_cell *acpi_indirectio_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 = pdata->io_size; >> + range->hw_start = pdata->io_start; >> + >> + ret = logic_pio_register_range(range); >> + if (ret) >> + goto free_range; >> + >> + list_for_each_entry(child, &adev->children, node) >> + cell_num++; >> + >> + /* allocate the mfd cells */ >> + size = sizeof(*mfd_cells) + sizeof(*acpi_indirectio_mfd_cells); >> + mfd_cells = kcalloc(cell_num, size, GFP_KERNEL); >> + if (!mfd_cells) { >> + ret = -ENOMEM; >> + goto free_range; >> + } >> + >> + acpi_indirectio_mfd_cells = (struct acpi_indirectio_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_indirectio_mfd_cell *acpi_indirectio_mfd_cell = >> + &acpi_indirectio_mfd_cells[count]; >> + const struct mfd_cell_acpi_match *acpi_match = >> + &acpi_indirectio_mfd_cell->acpi_match; >> + char *name = &acpi_indirectio_mfd_cell[count].name[0]; >> + char *pnpid = &acpi_indirectio_mfd_cell[count].pnpid[0]; >> + struct mfd_cell_acpi_match match = { >> + .pnpid = pnpid, >> + }; >> + >> + snprintf(name, ACPI_INDIRECTIO_NAME_LENGTH, "indirect-io-%s", >> + acpi_device_hid(child)); >> + snprintf(pnpid, ACPI_INDIRECTIO_NAME_LENGTH, "%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_indirectio_set_logicio_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_res; >> + } >> + 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_res; >> + } >> + 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_res; >> + } >> + >> + return ret; >> + >> +free_mfd_res: >> + 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_host_id[] = { >> + {""}, >> +}; >> + >> +static int acpi_indirectio_attach(struct acpi_device *adev, >> + const struct acpi_device_id *id) >> +{ >> + struct acpi_indirectio_device_desc *hostdata; >> + int ret; >> + >> + hostdata = (struct acpi_indirectio_device_desc *)id->driver_data; >> + if (!hostdata || !hostdata->pre_setup) >> + return -EINVAL; >> + >> + ret = hostdata->pre_setup(adev, &hostdata->pdata); >> + >> + if (ret < 0) >> + return ret; >> + >> + return 1; >> +} >> + >> +static struct acpi_scan_handler acpi_indirect_handler = { > > acpi_indirect_io_handler? sure > >> + .ids = acpi_indirect_host_id, >> + .attach = acpi_indirectio_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 1d0a501..d6b1a95 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_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 b0fe527..8ea6f69 100644 >> --- a/drivers/acpi/scan.c >> +++ b/drivers/acpi/scan.c >> @@ -2141,6 +2141,7 @@ int __init acpi_scan_init(void) >> acpi_amba_init(); >> acpi_watchdog_init(); >> acpi_init_lpit(); >> + acpi_indirectio_scan_init(); >> >> acpi_scan_add_handler(&generic_device_handler); >> >> -- >> 1.9.1 >> Thank you, John