[PATCH v2 0/3] HISI LPC ACPI UART support

[PATCH v2 0/3] HISI LPC ACPI UART support

[John Garry]

This patchset adds ACPI FW support for the UART on
the LPC bus on the Huawei D03 development board.

It also drops MFD API usage. It's not right to use MFD
APIs outside drivers/mfd. As the alternate solution, we
use platform device APIs directly.

The UART is 8250-compatible, and has the following
profile:
- IO space iotype
- no interrupt, so polling mode required
- 16550 type

Currently no platform driver exists for the UART. Indeed,
for PNP-compatible devices - like this UART - it would be
better to create a PNP device so that we may use the
existing PNP driver. Thus, we should use the 8250 PNP
driver.

However this host driver does not support PNP devices.
An RFC was sent for PNP support in [1]. However it was
deemed impractical to follow this path.

So to provide this UART support we use the 8250 generic
isa driver. For this, we need to set the UART platform
device name to match the 8250 isa driver. This means
passing the 8250 serial config in the child pdev platform
data.

1. https://lkml.org/lkml/2018/4/20/278

Differences:
v1 -> v2:
- drop MFD API usage and use platform device APIs
  directly for ACPI support

RFC -> v1:
- drop PNP support
- use static MFD cells
- add 8250 setup

John Garry (3):
  HISI LPC: Stop using MFD APIs
  HISI LPC: Re-Add ACPI child enumeration support
  HISI LPC: Add ACPI UART support

 drivers/bus/Kconfig    |   1 -
 drivers/bus/hisi_lpc.c | 159 ++++++++++++++++++++++++++++++-------------------
 2 files changed, 97 insertions(+), 63 deletions(-)

-- 
1.9.1

[PATCH v2 1/3] HISI LPC: Stop using MFD APIs

[John Garry]

The MFD APIs should only be used by drivers in
drivers/mfd. It is not worth splitting the driver to have
separate parts in drivers/bus and drivers/mfd, so just
drop MFD API usage.

As a solution, we will use the platform device APIs directly
to achieve the same as we had when using MFD APIs.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/Kconfig    |  1 -
 drivers/bus/hisi_lpc.c | 72 ++++++--------------------------------------------
 2 files changed, 8 insertions(+), 65 deletions(-)

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 6dc177b..d1c0b60 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -33,7 +33,6 @@ config HISILICON_LPC
 	bool "Support for ISA I/O space on HiSilicon Hip06/7"
 	depends on ARM64 && (ARCH_HISI || COMPILE_TEST)
 	select INDIRECT_PIO
-	select MFD_CORE if ACPI
 	help
 	  Driver to enable I/O access to devices attached to the Low Pin
 	  Count bus on the HiSilicon Hip06/7 SoC.
diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 2d4611e..252a6a4 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -11,7 +11,6 @@
 #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>
@@ -341,15 +340,6 @@ static void hisi_lpc_comm_outs(void *hostdata, unsigned long pio,
 };
 
 #ifdef CONFIG_ACPI
-#define MFD_CHILD_NAME_PREFIX DRV_NAME"-"
-#define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + sizeof(MFD_CHILD_NAME_PREFIX) - 1)
-
-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)
@@ -368,7 +358,7 @@ static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev,
 }
 
 /*
- * hisi_lpc_acpi_set_io_res - set the resources for a child's MFD
+ * hisi_lpc_acpi_set_io_res - set the resources for a child
  * @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
@@ -458,69 +448,23 @@ static int hisi_lpc_acpi_set_io_res(struct device *child,
  *
  * 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;
+	int ret;
 
-	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,
-		};
-
-		/*
-		 * For any instances of this host controller (Hip06 and Hip07
-		 * are the only chipsets), we would not have multiple slaves
-		 * with the same HID. And in any system we would have just one
-		 * controller active. So don't worrry about MFD name clashes.
-		 */
-		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);
+		const struct resource *res;
+		int num_res;
+
+		ret = hisi_lpc_acpi_set_io_res(&child->dev, &adev->dev, &res,
+					       &num_res);
 		if (ret) {
-			dev_warn(&child->dev, "set resource fail (%d)\n", ret);
-			return ret;
+			dev_warn(hostdev, "set resource fail (%d)\n", 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;
-- 
1.9.1

[PATCH v2 2/3] HISI LPC: Re-Add ACPI child enumeration support

[John Garry]

Since we no longer use the MFD APIs to enumerate the
child devices on the bus, use the platform driver APIs
directly.

In this patch we iterate of the children devices for the
host, and create a platform device directly per child.

For the iterating, we match the child ACPI HID against a
known list of supported child devices and their respective
ACPIs HID, to find the device name and any other
supplementary data.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/hisi_lpc.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 252a6a4..eb5b3fc 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -442,12 +442,28 @@ static int hisi_lpc_acpi_set_io_res(struct device *child,
 	return 0;
 }
 
+static int hisi_lpc_acpi_remove_subdev(struct device *dev, void *unused)
+{
+	platform_device_unregister(to_platform_device(dev));
+	return 0;
+}
+
+struct hisi_lpc_acpi_cell {
+	const char *hid;
+	const char *name;
+	void *pdata;
+	size_t pdata_size;
+};
+
 /*
  * hisi_lpc_acpi_probe - probe children for ACPI FW
  * @hostdev: LPC host device pointer
  *
  * Returns 0 when successful, and a negative value for failure.
  *
+ * Create a platform device per child, fixing up the resources
+ * from bus addresses to Logical PIO addresses.
+ *
  */
 static int hisi_lpc_acpi_probe(struct device *hostdev)
 {
@@ -457,17 +473,75 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 
 	/* Only consider the children of the host */
 	list_for_each_entry(child, &adev->children, node) {
+		const char *hid = acpi_device_hid(child);
+		const struct hisi_lpc_acpi_cell *cell;
+		struct platform_device *pdev;
 		const struct resource *res;
+		bool found = false;
 		int num_res;
 
 		ret = hisi_lpc_acpi_set_io_res(&child->dev, &adev->dev, &res,
 					       &num_res);
 		if (ret) {
 			dev_warn(hostdev, "set resource fail (%d)\n", ret);
+			goto fail;
+		}
+
+		cell = (struct hisi_lpc_acpi_cell []){
+			/* ipmi */
+			{
+				.hid = "IPI0001",
+				.name = "hisi-lpc-ipmi",
+			},
+			{}
+		};
+
+		for (; cell && cell->name; cell++) {
+			if (!strcmp(cell->hid, hid)) {
+				found = true;
+				break;
+			}
 		}
+
+		if (!found) {
+			dev_warn(hostdev,
+				 "could not find cell for child device (%s)\n",
+				 hid);
+			ret = -ENODEV;
+			goto fail;
+		}
+
+		pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);
+		if (!pdev) {
+			ret = -ENOMEM;
+			goto fail;
+		}
+
+		pdev->dev.parent = hostdev;
+		ACPI_COMPANION_SET(&pdev->dev, child);
+
+		ret = platform_device_add_resources(pdev, res, num_res);
+		if (ret)
+			goto fail;
+
+		ret = platform_device_add_data(pdev, cell->pdata,
+					       cell->pdata_size);
+		if (ret)
+			goto fail;
+
+		ret = platform_device_add(pdev);
+		if (ret)
+			goto fail;
+
+		acpi_device_set_enumerated(child);
 	}
 
 	return 0;
+
+fail:
+	device_for_each_child(hostdev, NULL,
+			      hisi_lpc_acpi_remove_subdev);
+	return ret;
 }
 
 static const struct acpi_device_id hisi_lpc_acpi_match[] = {
-- 
1.9.1

[PATCH v2 3/3] HISI LPC: Add ACPI UART support

[John Garry]

On the Huawei D03 development board the system UART is
the UART connected on the LPC bus.

The profile for the device driver required for this HW
is as follows:
 - platform driver
 - 16550
 - ACPI support
 - polling mode support
 - IO space support

In principle we should use the PNP driver (8250_dw.c) for
8250-devices with ACPI FW. However since this driver does
not support PNP devices, and modifying the PNP core code
to support it is not worth the effort, use the generic
8250 isa driver.

For this, we setup the pdev platform data for the serial
8250 port.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/hisi_lpc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index eb5b3fc..d5f8545 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -16,6 +16,7 @@
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/pci.h>
+#include <linux/serial_8250.h>
 #include <linux/slab.h>
 
 #define DRV_NAME "hisi-lpc"
@@ -493,6 +494,22 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 				.hid = "IPI0001",
 				.name = "hisi-lpc-ipmi",
 			},
+			/* 8250-compatible uart */
+			{
+				.hid = "HISI1031",
+				.name = "serial8250",
+				.pdata = (struct plat_serial8250_port []) {
+					{
+						.iobase = res->start,
+						.uartclk = 1843200,
+						.iotype = UPIO_PORT,
+						.flags = UPF_BOOT_AUTOCONF,
+					},
+					{}
+				},
+				.pdata_size = 2 *
+					sizeof(struct plat_serial8250_port),
+			},
 			{}
 		};
 
-- 
1.9.1

Re: [PATCH v2 0/3] HISI LPC ACPI UART support

[Andy Shevchenko]

On Tue, 2018-05-08 at 18:27 +0800, John Garry wrote:
> This patchset adds ACPI FW support for the UART on
> the LPC bus on the Huawei D03 development board.
> 
> It also drops MFD API usage. It's not right to use MFD
> APIs outside drivers/mfd. As the alternate solution, we
> use platform device APIs directly.
> 
> The UART is 8250-compatible, and has the following
> profile:
> - IO space iotype
> - no interrupt, so polling mode required
> - 16550 type
> 
> Currently no platform driver exists for the UART. Indeed,
> for PNP-compatible devices - like this UART - it would be
> better to create a PNP device so that we may use the
> existing PNP driver. Thus, we should use the 8250 PNP
> driver.
> 
> However this host driver does not support PNP devices.
> An RFC was sent for PNP support in [1]. However it was
> deemed impractical to follow this path.
> 
> So to provide this UART support we use the 8250 generic
> isa driver. For this, we need to set the UART platform
> device name to match the 8250 isa driver. This means
> passing the 8250 serial config in the child pdev platform
> data.
> 
> 1. https://lkml.org/lkml/2018/4/20/278
> 

I'm fine with this least invasive approach. It seems it has minimum
duplication of code, which is anyway unavoidable when we are speaking of
instantiating platform devices.

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Differences:
> v1 -> v2:
> - drop MFD API usage and use platform device APIs
>   directly for ACPI support
> 
> RFC -> v1:
> - drop PNP support
> - use static MFD cells
> - add 8250 setup
> 
> John Garry (3):
>   HISI LPC: Stop using MFD APIs
>   HISI LPC: Re-Add ACPI child enumeration support
>   HISI LPC: Add ACPI UART support
> 
>  drivers/bus/Kconfig    |   1 -
>  drivers/bus/hisi_lpc.c | 159 ++++++++++++++++++++++++++++++--------
> -----------
>  2 files changed, 97 insertions(+), 63 deletions(-)
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 0/3] HISI LPC ACPI UART support

[John Garry]

On 08/05/2018 12:17, Andy Shevchenko wrote:
> On Tue, 2018-05-08 at 18:27 +0800, John Garry wrote:
>> This patchset adds ACPI FW support for the UART on
>> the LPC bus on the Huawei D03 development board.
>>
>> It also drops MFD API usage. It's not right to use MFD
>> APIs outside drivers/mfd. As the alternate solution, we
>> use platform device APIs directly.
>>
>> The UART is 8250-compatible, and has the following
>> profile:
>> - IO space iotype
>> - no interrupt, so polling mode required
>> - 16550 type
>>
>> Currently no platform driver exists for the UART. Indeed,
>> for PNP-compatible devices - like this UART - it would be
>> better to create a PNP device so that we may use the
>> existing PNP driver. Thus, we should use the 8250 PNP
>> driver.
>>
>> However this host driver does not support PNP devices.
>> An RFC was sent for PNP support in [1]. However it was
>> deemed impractical to follow this path.
>>
>> So to provide this UART support we use the 8250 generic
>> isa driver. For this, we need to set the UART platform
>> device name to match the 8250 isa driver. This means
>> passing the 8250 serial config in the child pdev platform
>> data.
>>
>> 1. https://lkml.org/lkml/2018/4/20/278
>>
>
> I'm fine with this least invasive approach. It seems it has minimum
> duplication of code, which is anyway unavoidable when we are speaking of
> instantiating platform devices.
>
> FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks Andy!

Hi xuwei,

Unless there are any more comments, can you please pick up this series 
(with Andy's review tags) for merging through the arm soc tree?

All the best,
John

>
>> Differences:
>> v1 -> v2:
>> - drop MFD API usage and use platform device APIs
>>   directly for ACPI support
>>
>> RFC -> v1:
>> - drop PNP support
>> - use static MFD cells
>> - add 8250 setup
>>
>> John Garry (3):
>>   HISI LPC: Stop using MFD APIs
>>   HISI LPC: Re-Add ACPI child enumeration support
>>   HISI LPC: Add ACPI UART support
>>
>>  drivers/bus/Kconfig    |   1 -
>>  drivers/bus/hisi_lpc.c | 159 ++++++++++++++++++++++++++++++--------
>> -----------
>>  2 files changed, 97 insertions(+), 63 deletions(-)
>>
>

Re: [PATCH v2 0/3] HISI LPC ACPI UART support

[Wei Xu]

Hi John,

On 2018/5/9 15:48, John Garry wrote:
> On 08/05/2018 12:17, Andy Shevchenko wrote:
>> On Tue, 2018-05-08 at 18:27 +0800, John Garry wrote:
>>> This patchset adds ACPI FW support for the UART on
>>> the LPC bus on the Huawei D03 development board.
>>>
>>> It also drops MFD API usage. It's not right to use MFD
>>> APIs outside drivers/mfd. As the alternate solution, we
>>> use platform device APIs directly.
>>>
>>> The UART is 8250-compatible, and has the following
>>> profile:
>>> - IO space iotype
>>> - no interrupt, so polling mode required
>>> - 16550 type
>>>
>>> Currently no platform driver exists for the UART. Indeed,
>>> for PNP-compatible devices - like this UART - it would be
>>> better to create a PNP device so that we may use the
>>> existing PNP driver. Thus, we should use the 8250 PNP
>>> driver.
>>>
>>> However this host driver does not support PNP devices.
>>> An RFC was sent for PNP support in [1]. However it was
>>> deemed impractical to follow this path.
>>>
>>> So to provide this UART support we use the 8250 generic
>>> isa driver. For this, we need to set the UART platform
>>> device name to match the 8250 isa driver. This means
>>> passing the 8250 serial config in the child pdev platform
>>> data.
>>>
>>> 1. https://lkml.org/lkml/2018/4/20/278
>>>
>>
>> I'm fine with this least invasive approach. It seems it has minimum
>> duplication of code, which is anyway unavoidable when we are speaking of
>> instantiating platform devices.
>>
>> FWIW,
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Thanks Andy!
> 
> Hi xuwei,
> 
> Unless there are any more comments, can you please pick up this series (with Andy's review tags) for merging through the arm soc tree?

Thanks!
Series applied into the hisilicon driver tree.

BR,
Wei

> 
> All the best,
> John
> 
>>
>>> Differences:
>>> v1 -> v2:
>>> - drop MFD API usage and use platform device APIs
>>>   directly for ACPI support
>>>
>>> RFC -> v1:
>>> - drop PNP support
>>> - use static MFD cells
>>> - add 8250 setup
>>>
>>> John Garry (3):
>>>   HISI LPC: Stop using MFD APIs
>>>   HISI LPC: Re-Add ACPI child enumeration support
>>>   HISI LPC: Add ACPI UART support
>>>
>>>  drivers/bus/Kconfig    |   1 -
>>>  drivers/bus/hisi_lpc.c | 159 ++++++++++++++++++++++++++++++--------
>>> -----------
>>>  2 files changed, 97 insertions(+), 63 deletions(-)
>>>
>>
> 
> 
> 
> .
>