Message ID | 1519663249-9850-9-git-send-email-john.garry@huawei.com |
---|---|
State | Superseded |
Headers | show |
Series | None | expand |
On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote: > Based on the previous patches, this patch supports the > LPC host on hip06/hip07 for ACPI FW. > > It is the responsibility of the LPC host driver to > enumerate the child devices, as the ACPI scan code will > not enumerate children of "indirect IO" hosts. > > The ACPI table for the LPC host controller and the child > devices is in the following format: > 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 resources of the child devices need to be > translated from LPC bus addresses to logical PIO addresses, > and we shouldn't modify the resources of the devices > generated in the FW scan, a per-child MFD is created as > a substitute. The MFD IO resources will be the translated > bus addresses of the ACPI child. Ok, this needs to be thought about a bit more. I guess I understand what's is the problem with PNP IDs in the driver. You probe your LPC device quite late. One option is to move from classical probe to a event-driven model, i.e. via registering a notifier (see acpi_lpss.c), preparing necessary stuff at earlier stages and then register devices by their enumeration and appearance. Though, if I would be you I would seek a opinion from Rafael and Mika (maybe others as well). See also some comments below. > +#ifdef CONFIG_ACPI > +#define MFD_CHILD_NAME_PREFIX DRV_NAME"-" > +#define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + > sizeof(MFD_CHILD_NAME_PREFIX)) ..._PREFIX) - 1) ? > +static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev, > + struct acpi_device *host, > + struct resource *res) > +{ > + unsigned long sys_port; > + resource_size_t len = res->end - res->start; resource_size() > + return 0; > +} > +static int hisi_lpc_acpi_set_io_res(struct device *child, > + struct device *hostdev, > + const struct resource **res, > + int *num_res) > +{ > + /* > + * The following code segment to retrieve the resources is > common to > + * acpi_create_platform_device(), so consider a common helper > function > + * in future. > + */ > + count = acpi_dev_get_resources(adev, &resource_list, NULL, > NULL); > + if (count <= 0) { > + dev_dbg(child, "failed to get resources\n"); > + return count ? count : -EIO; count == 0 --> return 0; Is it by design? (I didn't check acpi_create_platform_device() though) > + } > + > + resources = devm_kcalloc(hostdev, count, sizeof(*resources), > + GFP_KERNEL); > + if (!resources) { > + dev_warn(hostdev, "could not allocate memory for %d > resources\n", > + count); > + 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); > + > + return 0; > +} > + > +/* > + * hisi_lpc_acpi_probe - probe children for ACPI FW > + * @hostdev: LPC host device pointer > + * > + * Returns 0 when successful, and a negative value for failure. > + * > + * Scan all child devices and create a per-device MFD with > + * logical PIO translated IO resources. > + */ > +static int hisi_lpc_acpi_probe(struct device *hostdev) > +{ > + struct acpi_device *adev = ACPI_COMPANION(hostdev); > + struct hisi_lpc_mfd_cell *hisi_lpc_mfd_cells; > + struct mfd_cell *mfd_cells; > + struct acpi_device *child; > + int size, ret, count = 0, cell_num = 0; > + > + 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(*hisi_lpc_mfd_cells); > + mfd_cells = devm_kcalloc(hostdev, cell_num, size, > GFP_KERNEL); > + if (!mfd_cells) > + return -ENOMEM; > + > + hisi_lpc_mfd_cells = (struct hisi_lpc_mfd_cell *) > + &mfd_cells[cell_num]; One line, please. Just noticed that calloc() memory layout is not the same how you are using it. > + /* Only consider the children of the host */ > + list_for_each_entry(child, &adev->children, node) { > + struct mfd_cell *mfd_cell = &mfd_cells[count]; > + struct hisi_lpc_mfd_cell *hisi_lpc_mfd_cell = > + &hisi_lpc_mfd_cells[count]; > + struct mfd_cell_acpi_match *acpi_match = > + &hisi_lpc_mfd_cell- > >acpi_match; > + char *name = hisi_lpc_mfd_cell[count].name; > + char *pnpid = hisi_lpc_mfd_cell[count].pnpid; > + struct mfd_cell_acpi_match match = { > + .pnpid = pnpid, > + }; > + > + snprintf(name, MFD_CHILD_NAME_LEN, > MFD_CHILD_NAME_PREFIX"%s", > + acpi_device_hid(child)); No possibility of identical devices? > + snprintf(pnpid, ACPI_ID_LEN, "%s", > acpi_device_hid(child)); > + > + memcpy(acpi_match, &match, sizeof(*acpi_match)); > + mfd_cell->name = name; > + mfd_cell->acpi_match = acpi_match; > + > + ret = hisi_lpc_acpi_set_io_res(&child->dev, &adev- > >dev, > + &mfd_cell->resources, > + &mfd_cell- > >num_resources); > + if (ret) { > + dev_warn(&child->dev, "set resource > fail(%d)\n", ret); > + return ret; > + } > + count++; > + } > + > + ret = mfd_add_devices(hostdev, > PLATFORM_DEVID_NONE, You mean it's not possible to have more than one identical device? > + mfd_cells, cell_num, NULL, 0, NULL); > + if (ret) { > + dev_err(hostdev, "failed to add mfd cells (%d)\n", > ret); > + return ret; > + } -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy
On 01/03/2018 19:50, Andy Shevchenko wrote: > On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote: >> Based on the previous patches, this patch supports the >> LPC host on hip06/hip07 for ACPI FW. >> >> It is the responsibility of the LPC host driver to >> enumerate the child devices, as the ACPI scan code will >> not enumerate children of "indirect IO" hosts. >> >> The ACPI table for the LPC host controller and the child >> devices is in the following format: >> 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 resources of the child devices need to be >> translated from LPC bus addresses to logical PIO addresses, >> and we shouldn't modify the resources of the devices >> generated in the FW scan, a per-child MFD is created as >> a substitute. The MFD IO resources will be the translated >> bus addresses of the ACPI child. > Hi Andy, > Ok, this needs to be thought about a bit more. > > I guess I understand what's is the problem with PNP IDs in the driver. > > You probe your LPC device quite late. > One option is to move from classical probe to a event-driven model, i.e. > via registering a notifier (see acpi_lpss.c), preparing necessary stuff > at earlier stages and then register devices by their enumeration and > appearance. > > Though, if I would be you I would seek a opinion from Rafael and Mika > (maybe others as well). I would like to give a bit more background on this HW. This HW is now for us a legacy device. It will be used in no more chipsets. It is only used in hip06 and hip07 chipsets on our D03 and D05 boards, respectively. On these boards we have the following LPC slave devices only: D03: UART, IPMI D05: IPMI Supporting IPMI for D05 is required. Supporting legacy D03 and the UART is a "nice-to-have". But it is definitely ok for us to not support this device. Our previous ACPI support solution did use a scan handler for this host, but there was some sensible pushback on this - please check this if not familiar: https://lkml.org/lkml/2018/2/14/532 Overall it does not make sense to try to move this back to drivers/acpi and receive more pushback from that direction, and only delay indefinitely upstreaming this driver (which is now running at ~27 months since v1) to just support a PNP-compatible device which we don't care too much about. > > See also some comments below. > >> +#ifdef CONFIG_ACPI >> +#define MFD_CHILD_NAME_PREFIX DRV_NAME"-" > >> +#define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + >> sizeof(MFD_CHILD_NAME_PREFIX)) > > ..._PREFIX) - 1) I didn't think so. But now I have noticed that ACPI_ID_LEN is 9 (which of course it needs to be for pnpid name), and not 8. I was thinking that the sizeof(MFD_CHILD_NAME_PREFIX) was providing the extra byte for the NULL terminator I required. > > ? > >> +static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev, >> + struct acpi_device *host, >> + struct resource *res) >> +{ >> + unsigned long sys_port; > >> + resource_size_t len = res->end - res->start; > > resource_size() should be ok > >> + return 0; >> +} > > >> +static int hisi_lpc_acpi_set_io_res(struct device *child, >> + struct device *hostdev, >> + const struct resource **res, >> + int *num_res) >> +{ > >> + /* >> + * The following code segment to retrieve the resources is >> common to >> + * acpi_create_platform_device(), so consider a common helper >> function >> + * in future. >> + */ >> + count = acpi_dev_get_resources(adev, &resource_list, NULL, >> NULL); >> + if (count <= 0) { >> + dev_dbg(child, "failed to get resources\n"); > >> + return count ? count : -EIO; > > count == 0 --> return 0; > The idea is that having no IO resources is a failure for a slave device on this bus. So then, if count == 0, we should error. > Is it by design? (I didn't check acpi_create_platform_device() though) > >> + } >> + >> + resources = devm_kcalloc(hostdev, count, sizeof(*resources), >> + GFP_KERNEL); >> + if (!resources) { >> + dev_warn(hostdev, "could not allocate memory for %d >> resources\n", >> + count); >> + 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); > >> + >> + return 0; >> +} > >> + >> +/* >> + * hisi_lpc_acpi_probe - probe children for ACPI FW >> + * @hostdev: LPC host device pointer >> + * >> + * Returns 0 when successful, and a negative value for failure. >> + * >> + * Scan all child devices and create a per-device MFD with >> + * logical PIO translated IO resources. >> + */ >> +static int hisi_lpc_acpi_probe(struct device *hostdev) >> +{ >> + struct acpi_device *adev = ACPI_COMPANION(hostdev); >> + struct hisi_lpc_mfd_cell *hisi_lpc_mfd_cells; >> + struct mfd_cell *mfd_cells; >> + struct acpi_device *child; >> + int size, ret, count = 0, cell_num = 0; >> + >> + 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(*hisi_lpc_mfd_cells); >> + mfd_cells = devm_kcalloc(hostdev, cell_num, size, >> GFP_KERNEL); >> + if (!mfd_cells) >> + return -ENOMEM; >> + > >> + hisi_lpc_mfd_cells = (struct hisi_lpc_mfd_cell *) >> + &mfd_cells[cell_num]; > > One line, please. Should be possible. I just want to keep checkpatch happy. > > Just noticed that calloc() memory layout is not the same how you are > using it. Right, because we need to group the MFD cells together. > >> + /* Only consider the children of the host */ >> + list_for_each_entry(child, &adev->children, node) { >> + struct mfd_cell *mfd_cell = &mfd_cells[count]; >> + struct hisi_lpc_mfd_cell *hisi_lpc_mfd_cell = >> + &hisi_lpc_mfd_cells[count]; >> + struct mfd_cell_acpi_match *acpi_match = >> + &hisi_lpc_mfd_cell- >>> acpi_match; >> + char *name = hisi_lpc_mfd_cell[count].name; >> + char *pnpid = hisi_lpc_mfd_cell[count].pnpid; >> + struct mfd_cell_acpi_match match = { >> + .pnpid = pnpid, >> + }; >> + > >> + snprintf(name, MFD_CHILD_NAME_LEN, >> MFD_CHILD_NAME_PREFIX"%s", >> + acpi_device_hid(child)); > > No possibility of identical devices? As explained above, actually no in reality. So I should comment on this. > >> + snprintf(pnpid, ACPI_ID_LEN, "%s", >> acpi_device_hid(child)); >> + >> + memcpy(acpi_match, &match, sizeof(*acpi_match)); >> + mfd_cell->name = name; >> + mfd_cell->acpi_match = acpi_match; >> + >> + ret = hisi_lpc_acpi_set_io_rsses(&child->dev, &adev- >>> dev, >> + &mfd_cell->resources, >> + &mfd_cell- >>> num_resources); >> + if (ret) { >> + dev_warn(&child->dev, "set resource >> fail(%d)\n", ret); >> + return ret; >> + } >> + count++; >> + } >> + >> + ret = mfd_add_devices(hostdev, > >> PLATFORM_DEVID_NONE, > > You mean it's not possible to have more than one identical device? Again, as above, this would not happen. I could make the code more generic, but I feel that there is little gain. > >> + mfd_cells, cell_num, NULL, 0, NULL); >> + if (ret) { >> + dev_err(hostdev, "failed to add mfd cells (%d)\n", >> ret); >> + return ret; >> + } > Thanks, John >
On Fri, 2018-03-02 at 10:19 +0000, John Garry wrote: > On 01/03/2018 19:50, Andy Shevchenko wrote: > > On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote: > > Ok, this needs to be thought about a bit more. > > > > I guess I understand what's is the problem with PNP IDs in the > > driver. > > > > You probe your LPC device quite late. > > One option is to move from classical probe to a event-driven model, > > i.e. > > via registering a notifier (see acpi_lpss.c), preparing necessary > > stuff > > at earlier stages and then register devices by their enumeration and > > appearance. > > > > Though, if I would be you I would seek a opinion from Rafael and > > Mika > > (maybe others as well). > > I would like to give a bit more background on this HW. This HW is now > for us a legacy device. It will be used in no more chipsets. It is > only > used in hip06 and hip07 chipsets on our D03 and D05 boards, > respectively. On these boards we have the following LPC slave devices > only: > D03: UART, IPMI > D05: IPMI > > Supporting IPMI for D05 is required. Supporting legacy D03 and the > UART > is a "nice-to-have". But it is definitely ok for us to not support > this > device. I see. But it raises another question to the whole series, why do we introduce a generic indirect IO for only two devices ever? Can't it be done in the (MFD) driver itself? Possible another option, is to introduce a specific regmap for that and use it in the drivers (though it might be not so trivial with existing ones). > Our previous ACPI support solution did use a scan handler for this > host, > but there was some sensible pushback on this - please check this if > not > familiar: https://lkml.org/lkml/2018/2/14/532 > Overall it does not make sense to try to move this back to > drivers/acpi > and receive more pushback from that direction, Ah, I think the ARM people just worried mostly about arm64 in the pathnames, though your case is very similar to our LPSS for only few SoCs. I would rather think if you move it directly to drivers/acpi it would be fine. > and only delay > indefinitely upstreaming this driver (which is now running at ~27 > months > since v1) to just support a PNP-compatible device which we don't care > too much about. Ouch! As I suggested before, you would better to get an input from maintainers. My personal opinion that the handler approach is cleaner, though it's still non-generic stuff. I think that this is what Mika referred to in his mail. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy
diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c index c2f3c14..0c7c1ec 100644 --- a/drivers/bus/hisi_lpc.c +++ b/drivers/bus/hisi_lpc.c @@ -12,6 +12,7 @@ #include <linux/delay.h> #include <linux/io.h> #include <linux/logic_pio.h> +#include <linux/mfd/core.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_address.h> @@ -353,6 +354,199 @@ static void hisi_lpc_comm_out(void *hostdata, unsigned long pio, .outs = hisi_lpc_comm_outs, }; +#ifdef CONFIG_ACPI +#define MFD_CHILD_NAME_PREFIX DRV_NAME"-" +#define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + sizeof(MFD_CHILD_NAME_PREFIX)) + +struct hisi_lpc_mfd_cell { + struct mfd_cell_acpi_match acpi_match; + char name[MFD_CHILD_NAME_LEN]; + char pnpid[ACPI_ID_LEN]; +}; + +static int hisi_lpc_acpi_xlat_io_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 == ~0UL) + return -EFAULT; + + res->start = sys_port; + res->end = sys_port + len; + + return 0; +} + +/* + * hisi_lpc_acpi_set_io_res - set the resources for a child's MFD + * @child: the device node to be updated the I/O resource + * @hostdev: the device node associated with host controller + * @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 host controller, each child device will have an associated + * host-relative address resource. This function will return the translated + * logical PIO addresses for each child devices resources. + */ +static int hisi_lpc_acpi_set_io_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; + + if (!child || !hostdev) + return -EINVAL; + + host = to_acpi_device(hostdev); + adev = to_acpi_device(child); + + /* check the device state */ + if (!adev->status.present) { + dev_dbg(child, "device is not present\n"); + return -EIO; + } + /* whether the child had been enumerated? */ + if (acpi_device_enumerated(adev)) { + dev_dbg(child, "has been enumerated\n"); + return -EIO; + } + + /* + * The following code segment to retrieve the resources is common to + * acpi_create_platform_device(), so consider a common helper function + * in future. + */ + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); + if (count <= 0) { + dev_dbg(child, "failed to get resources\n"); + return count ? count : -EIO; + } + + resources = devm_kcalloc(hostdev, count, sizeof(*resources), + GFP_KERNEL); + if (!resources) { + dev_warn(hostdev, "could not allocate memory for %d resources\n", + count); + 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++) { + int ret; + + if (!(resources[i].flags & IORESOURCE_IO)) + continue; + ret = hisi_lpc_acpi_xlat_io_res(adev, host, &resources[i]); + if (ret) { + dev_err(child, "translate IO range failed(%d)\n", ret); + return ret; + } + } + *res = resources; + *num_res = count; + + return 0; +} + +/* + * hisi_lpc_acpi_probe - probe children for ACPI FW + * @hostdev: LPC host device pointer + * + * Returns 0 when successful, and a negative value for failure. + * + * Scan all child devices and create a per-device MFD with + * logical PIO translated IO resources. + */ +static int hisi_lpc_acpi_probe(struct device *hostdev) +{ + struct acpi_device *adev = ACPI_COMPANION(hostdev); + struct hisi_lpc_mfd_cell *hisi_lpc_mfd_cells; + struct mfd_cell *mfd_cells; + struct acpi_device *child; + int size, ret, count = 0, cell_num = 0; + + 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(*hisi_lpc_mfd_cells); + mfd_cells = devm_kcalloc(hostdev, cell_num, size, GFP_KERNEL); + if (!mfd_cells) + return -ENOMEM; + + hisi_lpc_mfd_cells = (struct hisi_lpc_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 hisi_lpc_mfd_cell *hisi_lpc_mfd_cell = + &hisi_lpc_mfd_cells[count]; + struct mfd_cell_acpi_match *acpi_match = + &hisi_lpc_mfd_cell->acpi_match; + char *name = hisi_lpc_mfd_cell[count].name; + char *pnpid = hisi_lpc_mfd_cell[count].pnpid; + struct mfd_cell_acpi_match match = { + .pnpid = pnpid, + }; + + snprintf(name, MFD_CHILD_NAME_LEN, MFD_CHILD_NAME_PREFIX"%s", + acpi_device_hid(child)); + snprintf(pnpid, ACPI_ID_LEN, "%s", acpi_device_hid(child)); + + memcpy(acpi_match, &match, sizeof(*acpi_match)); + mfd_cell->name = name; + mfd_cell->acpi_match = acpi_match; + + ret = hisi_lpc_acpi_set_io_res(&child->dev, &adev->dev, + &mfd_cell->resources, + &mfd_cell->num_resources); + if (ret) { + dev_warn(&child->dev, "set resource fail(%d)\n", ret); + return ret; + } + count++; + } + + ret = mfd_add_devices(hostdev, PLATFORM_DEVID_NONE, + mfd_cells, cell_num, NULL, 0, NULL); + if (ret) { + dev_err(hostdev, "failed to add mfd cells (%d)\n", ret); + return ret; + } + + return 0; +} + +static const struct acpi_device_id hisi_lpc_acpi_match[] = { + {"HISI0191"}, + {} +}; +#else +static int hisi_lpc_acpi_probe(struct device *dev) +{ + return -ENODEV; +} +#endif // CONFIG_ACPI + /* * hisi_lpc_probe - the probe callback function for hisi lpc host, * will finish all the initialization. @@ -397,6 +591,8 @@ static int hisi_lpc_probe(struct platform_device *pdev) /* register the LPC host PIO resources */ if (!acpi_device) ret = of_platform_populate(dev->of_node, NULL, NULL, dev); + else + ret = hisi_lpc_acpi_probe(dev); if (ret) return ret; @@ -420,6 +616,7 @@ static int hisi_lpc_probe(struct platform_device *pdev) .driver = { .name = DRV_NAME, .of_match_table = hisi_lpc_of_match, + .acpi_match_table = ACPI_PTR(hisi_lpc_acpi_match), }, .probe = hisi_lpc_probe, };