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
