[PATCH 0/2] HISI LPC ACPI UART support

[PATCH 0/2] 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.

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

Currently an mfd-cell (platform device) is created per
device on the bus for using ACPI  firmware. As such,
we need require a platform driver for these devices.

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.

As a solution, we use the 8250 generic isa driver. For
this, we need to set the UART platform device name to
match the 8250 isa driver, so we require special setup 
for this. To support this, we introduce static MFD cells,
and support special setup handling for each device.
For the UART, this means passing the 8250 serial config
in the MFD cell platform data.

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

Differences:
RFC -> v1:
https://lkml.org/lkml/2018/4/20/278
- drop PNP support
- use static MFD cells
- add 8250 setup

John Garry (2):
  HISI LPC: Reference static MFD cells for ACPI support
  HISI LPC: Add ACPI UART support

 drivers/bus/hisi_lpc.c | 130 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 93 insertions(+), 37 deletions(-)

-- 
1.9.1

[PATCH 1/2] HISI LPC: Reference static MFD cells for ACPI support

[John Garry]

Currently for ACPI support the driver models the host as
an MFD. For a device connected to the LPC bus, we dynamically
create an MFD cell for that device, configuring the cell
name and ACPI match parameters manually. This makes supporting
named devices and also special setup handling for certain devices
awkward, as we would need to introduce some special ACPI device
handling according to device HID.

To avoid this, create reference static MFD cells for known
child devices, so when adding an MFD cell we can fix the cell
platform data as required. For this, a setup callback function
is added.

For now, only the IPMI cell is added.

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

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 2d4611e..71693d77 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -341,15 +341,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)
@@ -367,6 +358,40 @@ static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev,
 	return 0;
 }
 
+static const struct mfd_cell_acpi_match hisi_lpc_acpi_mfd_match_ipmi = {
+	.pnpid = "IPI0001",
+};
+
+struct hisi_lpc_acpi_mfd_cell {
+	struct mfd_cell mfd_cell;
+	int (*setup)(struct device *hostdev, struct mfd_cell *mfd_cell);
+} static const hisi_lpc_acpi_mfd_cells[] = {
+	/* ipmi */
+	{
+		.mfd_cell = {
+			.name = "hisi-lpc-ipmi",
+			.acpi_match = &hisi_lpc_acpi_mfd_match_ipmi,
+		},
+	},
+	{}
+};
+
+static const struct mfd_cell *hisi_lpc_acpi_mfd_get_cell(const char *hid)
+{
+	const struct hisi_lpc_acpi_mfd_cell *cell = hisi_lpc_acpi_mfd_cells;
+
+	for (; cell && cell->mfd_cell.name; cell++) {
+		const struct mfd_cell *mfd_cell = &cell->mfd_cell;
+		const struct mfd_cell_acpi_match *acpi_match;
+
+		acpi_match = mfd_cell->acpi_match;
+		if (!strcmp(acpi_match->pnpid, hid))
+			return mfd_cell;
+	}
+
+	return NULL;
+}
+
 /*
  * 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
@@ -464,7 +489,6 @@ static int hisi_lpc_acpi_set_io_res(struct device *child,
 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;
@@ -472,39 +496,24 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 	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);
+	/* allocate the mfd cells, one per child */
+	size = sizeof(*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,
-		};
-
-		/*
-		 * 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;
+		const struct mfd_cell *mfd_cell_ref;
+		const char *hid = acpi_device_hid(child);
+		struct hisi_lpc_acpi_mfd_cell *cell;
+
+		mfd_cell_ref = hisi_lpc_acpi_mfd_get_cell(hid);
+		if (!mfd_cell_ref)
+			return -ENOENT;
+
+		memcpy(mfd_cell, mfd_cell_ref, sizeof(*mfd_cell));
 
 		ret = hisi_lpc_acpi_set_io_res(&child->dev, &adev->dev,
 					       &mfd_cell->resources,
@@ -513,6 +522,14 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 			dev_warn(&child->dev, "set resource fail (%d)\n", ret);
 			return ret;
 		}
+
+		cell = container_of(mfd_cell_ref, typeof(*cell), mfd_cell);
+		if (cell->setup) {
+			ret = cell->setup(hostdev, mfd_cell);
+			if (ret)
+				return ret;
+		}
+
 		count++;
 	}
 
-- 
1.9.1

Re: [PATCH 1/2] HISI LPC: Reference static MFD cells for ACPI support

[Andy Shevchenko]

On Thu, 2018-05-03 at 23:08 +0800, John Garry wrote:
> Currently for ACPI support the driver models the host as
> an MFD. For a device connected to the LPC bus, we dynamically
> create an MFD cell for that device, configuring the cell
> name and ACPI match parameters manually. This makes supporting
> named devices and also special setup handling for certain devices
> awkward, as we would need to introduce some special ACPI device
> handling according to device HID.
> 
> To avoid this, create reference static MFD cells for known
> child devices, so when adding an MFD cell we can fix the cell
> platform data as required. For this, a setup callback function
> is added.
> 
> For now, only the IPMI cell is added.

> +static const struct mfd_cell *hisi_lpc_acpi_mfd_get_cell(const char
> *hid)
> +{
> +	const struct hisi_lpc_acpi_mfd_cell *cell =
> hisi_lpc_acpi_mfd_cells;
> +
> +	for (; cell && cell->mfd_cell.name; cell++) {
> +		const struct mfd_cell *mfd_cell = &cell->mfd_cell;
> +		const struct mfd_cell_acpi_match *acpi_match;
> +
> +		acpi_match = mfd_cell->acpi_match;
> +		if (!strcmp(acpi_match->pnpid, hid))
> +			return mfd_cell;
> +	}
> +
> +	return NULL;
> +}

I'm not sure I understand why MFD core can't do it (as seen in lines
drivers/mfd/core.c:105 and below).

> +	/* allocate the mfd cells, one per child */
> +	size = sizeof(*mfd_cells);
>  	mfd_cells = devm_kcalloc(hostdev, cell_num, size,
> GFP_KERNEL);
>  	if (!mfd_cells)
>  		return -ENOMEM;

And since you have structures already, I'm not sure why you need another
allocation for them. Only what you would need is to apply resources and
call devm_mfd_add_devices() per each found device.

> +		cell = container_of(mfd_cell_ref, typeof(*cell),
> mfd_cell);

Why we can't iterate over inherited type of objects directly?

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

Re: [PATCH 1/2] HISI LPC: Reference static MFD cells for ACPI support

[Lee Jones]

On Thu, 03 May 2018, Andy Shevchenko wrote:

> On Thu, 2018-05-03 at 23:08 +0800, John Garry wrote:
> > Currently for ACPI support the driver models the host as
> > an MFD. For a device connected to the LPC bus, we dynamically
> > create an MFD cell for that device, configuring the cell
> > name and ACPI match parameters manually. This makes supporting
> > named devices and also special setup handling for certain devices
> > awkward, as we would need to introduce some special ACPI device
> > handling according to device HID.
> > 
> > To avoid this, create reference static MFD cells for known
> > child devices, so when adding an MFD cell we can fix the cell
> > platform data as required. For this, a setup callback function
> > is added.
> > 
> > For now, only the IPMI cell is added.
> 
> > +static const struct mfd_cell *hisi_lpc_acpi_mfd_get_cell(const char
> > *hid)
> > +{
> > +	const struct hisi_lpc_acpi_mfd_cell *cell =
> > hisi_lpc_acpi_mfd_cells;
> > +
> > +	for (; cell && cell->mfd_cell.name; cell++) {
> > +		const struct mfd_cell *mfd_cell = &cell->mfd_cell;
> > +		const struct mfd_cell_acpi_match *acpi_match;
> > +
> > +		acpi_match = mfd_cell->acpi_match;
> > +		if (!strcmp(acpi_match->pnpid, hid))
> > +			return mfd_cell;
> > +	}
> > +
> > +	return NULL;
> > +}
> 
> I'm not sure I understand why MFD core can't do it (as seen in lines
> drivers/mfd/core.c:105 and below).

You shouldn't be using the MFD API outside of MFD anyway.  Either it
is an MFD driver, or it isn't.  If it is, please move it. If it's not,
please don't use the API.

My current suspicion is that the driver needs splitting and only part
of it ends up in MFD.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] HISI LPC: Reference static MFD cells for ACPI support

[John Garry]

On 04/05/2018 10:02, Lee Jones wrote:
> On Thu, 03 May 2018, Andy Shevchenko wrote:
>
>> On Thu, 2018-05-03 at 23:08 +0800, John Garry wrote:
>>> Currently for ACPI support the driver models the host as
>>> an MFD. For a device connected to the LPC bus, we dynamically
>>> create an MFD cell for that device, configuring the cell
>>> name and ACPI match parameters manually. This makes supporting
>>> named devices and also special setup handling for certain devices
>>> awkward, as we would need to introduce some special ACPI device
>>> handling according to device HID.
>>>
>>> To avoid this, create reference static MFD cells for known
>>> child devices, so when adding an MFD cell we can fix the cell
>>> platform data as required. For this, a setup callback function
>>> is added.
>>>
>>> For now, only the IPMI cell is added.
>>
>>> +static const struct mfd_cell *hisi_lpc_acpi_mfd_get_cell(const char
>>> *hid)
>>> +{
>>> +	const struct hisi_lpc_acpi_mfd_cell *cell =
>>> hisi_lpc_acpi_mfd_cells;
>>> +
>>> +	for (; cell && cell->mfd_cell.name; cell++) {
>>> +		const struct mfd_cell *mfd_cell = &cell->mfd_cell;
>>> +		const struct mfd_cell_acpi_match *acpi_match;
>>> +
>>> +		acpi_match = mfd_cell->acpi_match;
>>> +		if (!strcmp(acpi_match->pnpid, hid))
>>> +			return mfd_cell;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>
>> I'm not sure I understand why MFD core can't do it (as seen in lines
>> drivers/mfd/core.c:105 and below).
>

Hi Lee,

> You shouldn't be using the MFD API outside of MFD anyway.  Either it
> is an MFD driver, or it isn't.  If it is, please move it. If it's not,
> please don't use the API.

We're modelling as an MFD, but it's not an MFD in the classic sense. 
We're just using the MFD API for convenience (and to avoid code 
duplication), as the MFD API does what we require for this driver.

>
> My current suspicion is that the driver needs splitting and only part
> of it ends up in MFD.
>

How would you propose splitting the driver? By adding a lib function 
specific for this driver for the ACPI probe?

Cheers,
John

Re: [PATCH 1/2] HISI LPC: Reference static MFD cells for ACPI support

[Lee Jones]

On Fri, 04 May 2018, John Garry wrote:

> On 04/05/2018 10:02, Lee Jones wrote:
> > On Thu, 03 May 2018, Andy Shevchenko wrote:
> > 
> > > On Thu, 2018-05-03 at 23:08 +0800, John Garry wrote:
> > > > Currently for ACPI support the driver models the host as
> > > > an MFD. For a device connected to the LPC bus, we dynamically
> > > > create an MFD cell for that device, configuring the cell
> > > > name and ACPI match parameters manually. This makes supporting
> > > > named devices and also special setup handling for certain devices
> > > > awkward, as we would need to introduce some special ACPI device
> > > > handling according to device HID.
> > > > 
> > > > To avoid this, create reference static MFD cells for known
> > > > child devices, so when adding an MFD cell we can fix the cell
> > > > platform data as required. For this, a setup callback function
> > > > is added.
> > > > 
> > > > For now, only the IPMI cell is added.
> > > 
> > > > +static const struct mfd_cell *hisi_lpc_acpi_mfd_get_cell(const char
> > > > *hid)
> > > > +{
> > > > +	const struct hisi_lpc_acpi_mfd_cell *cell =
> > > > hisi_lpc_acpi_mfd_cells;
> > > > +
> > > > +	for (; cell && cell->mfd_cell.name; cell++) {
> > > > +		const struct mfd_cell *mfd_cell = &cell->mfd_cell;
> > > > +		const struct mfd_cell_acpi_match *acpi_match;
> > > > +
> > > > +		acpi_match = mfd_cell->acpi_match;
> > > > +		if (!strcmp(acpi_match->pnpid, hid))
> > > > +			return mfd_cell;
> > > > +	}
> > > > +
> > > > +	return NULL;
> > > > +}
> > > 
> > > I'm not sure I understand why MFD core can't do it (as seen in lines
> > > drivers/mfd/core.c:105 and below).
> > 
> 
> Hi Lee,
> 
> > You shouldn't be using the MFD API outside of MFD anyway.  Either it
> > is an MFD driver, or it isn't.  If it is, please move it. If it's not,
> > please don't use the API.
> 
> We're modelling as an MFD, but it's not an MFD in the classic sense. We're
> just using the MFD API for convenience (and to avoid code duplication), as
> the MFD API does what we require for this driver.

I know what you're doing, and it's wrong. ;)

> > My current suspicion is that the driver needs splitting and only part
> > of it ends up in MFD.
> 
> How would you propose splitting the driver? By adding a lib function
> specific for this driver for the ACPI probe?

Look at:

  drivers/platform/chrome/cros_ec_lpc.c

and

  drivers/mfd/cros_ec.c

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] HISI LPC: Reference static MFD cells for ACPI support

[John Garry]

On 04/05/2018 11:03, Lee Jones wrote:
> On Fri, 04 May 2018, John Garry wrote:
>
>> On 04/05/2018 10:02, Lee Jones wrote:
>>> On Thu, 03 May 2018, Andy Shevchenko wrote:
>>>
>>>> On Thu, 2018-05-03 at 23:08 +0800, John Garry wrote:
>>>>> Currently for ACPI support the driver models the host as
>>>>> an MFD. For a device connected to the LPC bus, we dynamically
>>>>> create an MFD cell for that device, configuring the cell
>>>>> name and ACPI match parameters manually. This makes supporting
>>>>> named devices and also special setup handling for certain devices
>>>>> awkward, as we would need to introduce some special ACPI device
>>>>> handling according to device HID.
>>>>>
>>>>> To avoid this, create reference static MFD cells for known
>>>>> child devices, so when adding an MFD cell we can fix the cell
>>>>> platform data as required. For this, a setup callback function
>>>>> is added.
>>>>>
>>>>> For now, only the IPMI cell is added.
>>>>
>>>>> +static const struct mfd_cell *hisi_lpc_acpi_mfd_get_cell(const char
>>>>> *hid)
>>>>> +{
>>>>> +	const struct hisi_lpc_acpi_mfd_cell *cell =
>>>>> hisi_lpc_acpi_mfd_cells;
>>>>> +
>>>>> +	for (; cell && cell->mfd_cell.name; cell++) {
>>>>> +		const struct mfd_cell *mfd_cell = &cell->mfd_cell;
>>>>> +		const struct mfd_cell_acpi_match *acpi_match;
>>>>> +
>>>>> +		acpi_match = mfd_cell->acpi_match;
>>>>> +		if (!strcmp(acpi_match->pnpid, hid))
>>>>> +			return mfd_cell;
>>>>> +	}
>>>>> +
>>>>> +	return NULL;
>>>>> +}
>>>>
>>>> I'm not sure I understand why MFD core can't do it (as seen in lines
>>>> drivers/mfd/core.c:105 and below).
>>>
>>
>> Hi Lee,
>>
>>> You shouldn't be using the MFD API outside of MFD anyway.  Either it
>>> is an MFD driver, or it isn't.  If it is, please move it. If it's not,
>>> please don't use the API.
>>
>> We're modelling as an MFD, but it's not an MFD in the classic sense. We're
>> just using the MFD API for convenience (and to avoid code duplication), as
>> the MFD API does what we require for this driver.
>
> I know what you're doing, and it's wrong. ;)
>
>>> My current suspicion is that the driver needs splitting and only part
>>> of it ends up in MFD.
>>
>> How would you propose splitting the driver? By adding a lib function
>> specific for this driver for the ACPI probe?
>
> Look at:
>
>   drivers/platform/chrome/cros_ec_lpc.c
>
> and
>
>   drivers/mfd/cros_ec.c
>

Right, I see, something similar to what I suggested.

I don't really see a point in splitting the driver across drivers/mfd 
and drivers/bus, and introducing dependencies. This is more especially 
considering this is a legacy host controller with no potential future 
developments, and not worth the effort.

If you feel strongly enough about not using the MFD API outside 
drivers/mfd, then I'll look at other solutions, like using platform 
device APIs directly.

Cheers,
John

Re: [PATCH 1/2] HISI LPC: Reference static MFD cells for ACPI support

[John Garry]

On 03/05/2018 17:54, Andy Shevchenko wrote:
> On Thu, 2018-05-03 at 23:08 +0800, John Garry wrote:
>> Currently for ACPI support the driver models the host as
>> an MFD. For a device connected to the LPC bus, we dynamically
>> create an MFD cell for that device, configuring the cell
>> name and ACPI match parameters manually. This makes supporting
>> named devices and also special setup handling for certain devices
>> awkward, as we would need to introduce some special ACPI device
>> handling according to device HID.
>>
>> To avoid this, create reference static MFD cells for known
>> child devices, so when adding an MFD cell we can fix the cell
>> platform data as required. For this, a setup callback function
>> is added.

Hi Andy,

>>
>> For now, only the IPMI cell is added.
>
>> +static const struct mfd_cell *hisi_lpc_acpi_mfd_get_cell(const char
>> *hid)
>> +{
>> +	const struct hisi_lpc_acpi_mfd_cell *cell =
>> hisi_lpc_acpi_mfd_cells;
>> +
>> +	for (; cell && cell->mfd_cell.name; cell++) {
>> +		const struct mfd_cell *mfd_cell = &cell->mfd_cell;
>> +		const struct mfd_cell_acpi_match *acpi_match;
>> +
>> +		acpi_match = mfd_cell->acpi_match;
>> +		if (!strcmp(acpi_match->pnpid, hid))
>> +			return mfd_cell;
>> +	}
>> +
>> +	return NULL;
>> +}
>
> I'm not sure I understand why MFD core can't do it (as seen in lines
> drivers/mfd/core.c:105 and below).
>

Right, I think MFD core does the same.

>> +	/* allocate the mfd cells, one per child */
>> +	size = sizeof(*mfd_cells);
>>  	mfd_cells = devm_kcalloc(hostdev, cell_num, size,
>> GFP_KERNEL);
>>  	if (!mfd_cells)
>>  		return -ENOMEM;
>
> And since you have structures already, I'm not sure why you need another
> allocation for them. Only what you would need is to apply resources and
> call devm_mfd_add_devices() per each found device.

I was creating a copy as the originals are in the hisi_lpc_acpi_mfd_cell 
struct type, while we would require an array of mfd_cells.

>
>> +		cell = container_of(mfd_cell_ref, typeof(*cell),
>> mfd_cell);
>
> Why we can't iterate over inherited type of objects directly?
>

We could do.

Thanks,
John

[PATCH 2/2] 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
would be as follows:
- platform driver
- supports 16550
- supports ACPI
- supports polling mode
- supports IO space

In principle we should use the PNP driver (8250_dw.c) for
8250-devices with ACPI FW. However since this host 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 need to setup the MFD cell platform data for
a serial 8250 Port.

In addition to this change, we also make the following
changes:
- set child ACPI device as enumerated. This fixes a conflict
  in serdev setup for the UART
- use platform device ID auto for creating the uart child
  platform device to avoid conflict with platform devices
  created for serial8250_isa_devs from 8250_core.c

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

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 71693d77..b0ee2fa 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -17,6 +17,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"
@@ -362,6 +363,35 @@ static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev,
 	.pnpid = "IPI0001",
 };
 
+static const struct mfd_cell_acpi_match hisi_lpc_acpi_mfd_match_uart = {
+	.pnpid = "HISI1031",
+};
+
+static int hisi_lpc_acpi_uart_setup(struct device *hostdev,
+				    struct mfd_cell *mfd_cell)
+{
+	const struct resource * const resource = mfd_cell->resources;
+	struct plat_serial8250_port ref_port[] = {
+		{
+			.iobase = resource->start,
+			.uartclk = 1843200,
+			.iotype = UPIO_PORT,
+			.flags = UPF_BOOT_AUTOCONF,
+		},
+		{}
+	};
+
+	mfd_cell->platform_data = devm_kmemdup(hostdev, &ref_port,
+					       sizeof(ref_port), GFP_KERNEL);
+	if (!mfd_cell->platform_data)
+		return -ENOMEM;
+
+	mfd_cell->pdata_size = sizeof(ref_port);
+
+	return 0;
+}
+
+
 struct hisi_lpc_acpi_mfd_cell {
 	struct mfd_cell mfd_cell;
 	int (*setup)(struct device *hostdev, struct mfd_cell *mfd_cell);
@@ -373,6 +403,14 @@ struct hisi_lpc_acpi_mfd_cell {
 			.acpi_match = &hisi_lpc_acpi_mfd_match_ipmi,
 		},
 	},
+	/* 8250-compatible uart */
+	{
+		.mfd_cell = {
+			.name = "serial8250",
+			.acpi_match = &hisi_lpc_acpi_mfd_match_uart,
+		},
+		.setup = hisi_lpc_acpi_uart_setup,
+	},
 	{}
 };
 
@@ -530,10 +568,11 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 				return ret;
 		}
 
+		acpi_device_set_enumerated(child);
 		count++;
 	}
 
-	ret = mfd_add_devices(hostdev, PLATFORM_DEVID_NONE,
+	ret = mfd_add_devices(hostdev, PLATFORM_DEVID_AUTO,
 			      mfd_cells, cell_num, NULL, 0, NULL);
 	if (ret) {
 		dev_err(hostdev, "failed to add mfd cells (%d)\n", ret);
-- 
1.9.1

Re: [PATCH 2/2] HISI LPC: Add ACPI UART support

[Andy Shevchenko]

On Thu, 2018-05-03 at 23:08 +0800, John Garry wrote:
> 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
> would be as follows:
> - platform driver
> - supports 16550
> - supports ACPI
> - supports polling mode
> - supports IO space
> 
> In principle we should use the PNP driver (8250_dw.c) for
> 8250-devices with ACPI FW. However since this host 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 need to setup the MFD cell platform data for
> a serial 8250 Port.
> 
> In addition to this change, we also make the following
> changes:
> - set child ACPI device as enumerated. This fixes a conflict
>   in serdev setup for the UART
> - use platform device ID auto for creating the uart child
>   platform device to avoid conflict with platform devices
>   created for serial8250_isa_devs from 8250_core.c

> +	mfd_cell->platform_data = devm_kmemdup(hostdev, &ref_port,
> +					       sizeof(ref_port),
> GFP_KERNEL);

Isn't this done by MFD core?


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

Re: [PATCH 2/2] HISI LPC: Add ACPI UART support

[John Garry]

On 03/05/2018 17:56, Andy Shevchenko wrote:
> On Thu, 2018-05-03 at 23:08 +0800, John Garry wrote:
>> 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
>> would be as follows:
>> - platform driver
>> - supports 16550
>> - supports ACPI
>> - supports polling mode
>> - supports IO space
>>
>> In principle we should use the PNP driver (8250_dw.c) for
>> 8250-devices with ACPI FW. However since this host 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 need to setup the MFD cell platform data for
>> a serial 8250 Port.
>>
>> In addition to this change, we also make the following
>> changes:
>> - set child ACPI device as enumerated. This fixes a conflict
>>   in serdev setup for the UART
>> - use platform device ID auto for creating the uart child
>>   platform device to avoid conflict with platform devices
>>   created for serial8250_isa_devs from 8250_core.c
>

Hi Andy,

>> +	mfd_cell->platform_data = devm_kmemdup(hostdev, &ref_port,
>> +					       sizeof(ref_port),
>> GFP_KERNEL);
>
> Isn't this done by MFD core?

It does, but what we are copying from is on the stack and could be 
overwritten by the point we call the mfd_add_devices(). But this could 
be improved.

BTW, As seen in other discussion, future of MFD usage is in doubt, so 
all this may be dropped.

Thanks very much,
John

>
>