[RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[John Garry]

There is a requirement for supporting an 8250-compatible UART with
the following profile/features:
- platform device
- polling mode (i.e. no interrupt support)
- ACPI FW
- IO port iotype
- 16550-compatible

For OF, we have 8250_of.c, and for PNP device we have 8250_pnp.c
drivers. However there does not seem to any driver satisfying
the above requirements. So this RFC is to find opinion on
modifying the Synopsys DW 8250_dw.c driver to support these
generic features.

With patch 2/2, we're relaxing the interrupt support requirement.
As I mentioned in the commit log, the 8250_of.c driver seems to
ignore the dt bindings, and I do the same.

John Garry (2):
  serial: 8250_dw: add IO space support
  serial: 8250_dw: support polling mode

 drivers/tty/serial/8250/8250_dw.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

-- 
1.9.1

[RFC PATCH 1/2] serial: 8250_dw: add IO space support

[John Garry]

The current 8250_dw driver only supports MEM iotype. It is desired
to have a platform device-based 8250 UART driver for ACPI FW
with IO port iotype, so update the driver to support this.

Note: a solution needs to be found for autconfig
using MEM accessors only.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/tty/serial/8250/8250_dw.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index cd1b94a..28fbc8f 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -405,7 +405,7 @@ static void dw8250_setup_port(struct uart_port *p)
 static int dw8250_probe(struct platform_device *pdev)
 {
 	struct uart_8250_port uart = {};
-	struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct resource *regs;
 	int irq = platform_get_irq(pdev, 0);
 	struct uart_port *p = &uart.port;
 	struct device *dev = &pdev->dev;
@@ -413,6 +413,9 @@ static int dw8250_probe(struct platform_device *pdev)
 	int err;
 	u32 val;
 
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0) ?:
+	       platform_get_resource(pdev, IORESOURCE_IO, 0);
+
 	if (!regs) {
 		dev_err(dev, "no registers defined\n");
 		return -EINVAL;
@@ -425,22 +428,28 @@ static int dw8250_probe(struct platform_device *pdev)
 	}
 
 	spin_lock_init(&p->lock);
-	p->mapbase	= regs->start;
 	p->irq		= irq;
 	p->handle_irq	= dw8250_handle_irq;
 	p->pm		= dw8250_do_pm;
 	p->type		= PORT_8250;
 	p->flags	= UPF_SHARE_IRQ | UPF_FIXED_PORT;
 	p->dev		= dev;
-	p->iotype	= UPIO_MEM;
-	p->serial_in	= dw8250_serial_in;
-	p->serial_out	= dw8250_serial_out;
 	p->set_ldisc	= dw8250_set_ldisc;
 	p->set_termios	= dw8250_set_termios;
 
-	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
-	if (!p->membase)
-		return -ENOMEM;
+	if ((regs->flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM) {
+		p->mapbase	= regs->start;
+		p->membase = devm_ioremap(dev, regs->start,
+					  resource_size(regs));
+		if (!p->membase)
+			return -ENOMEM;
+		p->iotype = UPIO_MEM;
+		p->serial_in	= dw8250_serial_in;
+		p->serial_out	= dw8250_serial_out;
+	} else {
+		p->iobase = regs->start;
+		p->iotype = UPIO_PORT;
+	}
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
-- 
1.9.1

[RFC PATCH 2/2] serial: 8250_dw: support polling mode

[John Garry]

It would be useful to make this driver support some
8250-compatible devices which have no interrupt line.

For these, we allow for no interrupt, and will fallback on
polling mode.

Note: the 8250 dt bindings state that "interrupts"
is a required property:
"interrupts : should contain uart interrupt."

But the 8250_of.c driver can live without it. So
this patch is going this way also.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/tty/serial/8250/8250_dw.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 28fbc8f..3154988 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -422,9 +422,10 @@ static int dw8250_probe(struct platform_device *pdev)
 	}
 
 	if (irq < 0) {
-		if (irq != -EPROBE_DEFER)
-			dev_err(dev, "cannot get irq\n");
-		return irq;
+		if (irq == -EPROBE_DEFER)
+			return irq;
+		dev_warn(dev, "cannot get irq, using polling mode\n");
+		irq = 0;
 	}
 
 	spin_lock_init(&p->lock);
-- 
1.9.1

Re: [RFC PATCH 2/2] serial: 8250_dw: support polling mode

[Andy Shevchenko]

On Fri, 2018-02-23 at 02:42 +0800, John Garry wrote:
> It would be useful to make this driver support some
> 8250-compatible devices which have no interrupt line.
> 
> For these, we allow for no interrupt, and will fallback on
> polling mode.
> 
> Note: the 8250 dt bindings state that "interrupts"
> is a required property:
> "interrupts : should contain uart interrupt."
> 
> But the 8250_of.c driver can live without it. So
> this patch is going this way also.

It should be documented in the binding.

>  	if (irq < 0) {
> -		if (irq != -EPROBE_DEFER)
> -			dev_err(dev, "cannot get irq\n");
> -		return irq;
> +		if (irq == -EPROBE_DEFER)
> +			return irq;
> +		dev_warn(dev, "cannot get irq, using polling
> mode\n");
> +		irq = 0;

NO_IRQ _is_ 0. You need to do something like

if (irq < 0) }
 ... leave existing code ...
}

if (!irq)
 dev_warn(, "Use polling mode\n");

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

Re: [RFC PATCH 2/2] serial: 8250_dw: support polling mode

[John Garry]

On 23/02/2018 17:35, Andy Shevchenko wrote:
> On Fri, 2018-02-23 at 02:42 +0800, John Garry wrote:
>> It would be useful to make this driver support some
>> 8250-compatible devices which have no interrupt line.
>>
>> For these, we allow for no interrupt, and will fallback on
>> polling mode.
>>
>> Note: the 8250 dt bindings state that "interrupts"
>> is a required property:
>> "interrupts : should contain uart interrupt."
>>
>> But the 8250_of.c driver can live without it. So
>> this patch is going this way also.
>
> It should be documented in the binding.

Agreed. I find the wording a bit ambigious in bindings/serial/8250.txt 
and snps-dw-apb-uart.txt:

Required properties:
[...]
- interrupts : should contain uart interrupt.

It uses the word "should", and not "must". Maybe "should" means "must"...

>
>>  	if (irq < 0) {
>> -		if (irq != -EPROBE_DEFER)
>> -			dev_err(dev, "cannot get irq\n");
>> -		return irq;
>> +		if (irq == -EPROBE_DEFER)
>> +			return irq;
>> +		dev_warn(dev, "cannot get irq, using polling
>> mode\n");
>> +		irq = 0;
>
> NO_IRQ _is_ 0. You need to do something like
>
> if (irq < 0) }
>  ... leave existing code ...
> }
>
> if (!irq)
>  dev_warn(, "Use polling mode\n");

OK

>

Thanks,
John

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[Andy Shevchenko]

On Fri, 2018-02-23 at 02:42 +0800, John Garry wrote:
> There is a requirement

Where?

>  for supporting an 8250-compatible UART with
> the following profile/features:
> - platform device
> - polling mode (i.e. no interrupt support)

> - ACPI FW

Elaborate this one, please.

> - IO port iotype
> - 16550-compatible
> 
> For OF, we have 8250_of.c, and for PNP device we have 8250_pnp.c
> drivers. However there does not seem to any driver satisfying
> the above requirements. So this RFC is to find opinion on
> modifying the Synopsys DW 8250_dw.c driver to support these
> generic features.

Synopsys 8250 is a particular case of platform drivers. It doesn't
satisfy "8250-compatible UART" requirement.

> With patch 2/2, we're relaxing the interrupt support requirement.
> As I mentioned in the commit log, the 8250_of.c driver seems to
> ignore the dt bindings, and I do the same.


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

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[John Garry]

On 23/02/2018 10:30, Andy Shevchenko wrote:
> On Fri, 2018-02-23 at 02:42 +0800, John Garry wrote:
>> There is a requirement
>

Hi Andy,

> Where?
>

We require it for a development board for our hip06 platform. FWIW, 
here's a link to a board overview (does not mention UART particulars):
http://open-estuary.org/d03/

It uses the BMC virtual uart (16550A) over the LPC as the system UART. 
You know about the Hisi LPC support from here:
https://lkml.org/lkml/2018/2/19/460

>>  for supporting an 8250-compatible UART with
>> the following profile/features:
>> - platform device
>> - polling mode (i.e. no interrupt support)
>
>> - ACPI FW
>
> Elaborate this one, please.

So we need to define our own HID here, and cannot use PNP compatible CID 
(like PNP0501) as we cannot use the 8250 PNP driver.

This is related to the Hisi LPC ACPI support, where we would create an 
MFD (i.e. platform device) for the UART.

>
>> - IO port iotype
>> - 16550-compatible
>>
>> For OF, we have 8250_of.c, and for PNP device we have 8250_pnp.c
>> drivers. However there does not seem to any driver satisfying
>> the above requirements. So this RFC is to find opinion on
>> modifying the Synopsys DW 8250_dw.c driver to support these
>> generic features.
>
> Synopsys 8250 is a particular case of platform drivers. It doesn't
> satisfy "8250-compatible UART" requirement.
>

Right, but I wanted to try to use the generic parts of the driver to 
support this UART to save writing yet another driver.

>> With patch 2/2, we're relaxing the interrupt support requirement.
>> As I mentioned in the commit log, the 8250_of.c driver seems to
>> ignore the dt bindings, and I do the same.
>
>

Thanks,
John

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[Andy Shevchenko]

On Fri, 2018-02-23 at 11:02 +0000, John Garry wrote:
> On 23/02/2018 10:30, Andy Shevchenko wrote:
> > On Fri, 2018-02-23 at 02:42 +0800, John Garry wrote:
> > > There is a requirement
> 
> > Where?
> 
> We require it for a development board for our hip06 platform.

Okay, and this particular platform uses Synopsys IP?

> > >  for supporting an 8250-compatible UART with
> > > the following profile/features:
> > > - platform device
> > > - polling mode (i.e. no interrupt support)
> > > - ACPI FW
> > 
> > Elaborate this one, please.
> 
> So we need to define our own HID here, and cannot use PNP compatible
> CID 
> (like PNP0501) as we cannot use the 8250 PNP driver.

Why not? What are the impediments?

> This is related to the Hisi LPC ACPI support, where we would create
> an 
> MFD (i.e. platform device) for the UART.

Why you can't do properly in ACPI?

> > > - IO port iotype
> > > - 16550-compatible
> > > 
> > > For OF, we have 8250_of.c, and for PNP device we have 8250_pnp.c
> > > drivers. However there does not seem to any driver satisfying
> > > the above requirements. So this RFC is to find opinion on
> > > modifying the Synopsys DW 8250_dw.c driver to support these
> > > generic features.
> > 
> > Synopsys 8250 is a particular case of platform drivers. It doesn't
> > satisfy "8250-compatible UART" requirement.

> Right, but I wanted to try to use the generic parts of the driver to 
> support this UART to save writing yet another driver.

It's still odd. Why this one, why not 8250_foo_bar to touch instead?

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

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[John Garry]

On 23/02/2018 17:31, Andy Shevchenko wrote:
> On Fri, 2018-02-23 at 11:02 +0000, John Garry wrote:
>> On 23/02/2018 10:30, Andy Shevchenko wrote:
>>> On Fri, 2018-02-23 at 02:42 +0800, John Garry wrote:
>>>> There is a requirement
>>
>>> Where?
>>

Hi Andy,

>> We require it for a development board for our hip06 platform.
>
> Okay, and this particular platform uses Synopsys IP?

As I see this uart is really a virtual 8250, so HW details like apb 
clocks and the like are hidden, so may not be relevant.

However I will check with the BMC team to know the specific details.

>
>>>>  for supporting an 8250-compatible UART with
>>>> the following profile/features:
>>>> - platform device
>>>> - polling mode (i.e. no interrupt support)
>>>> - ACPI FW
>>>
>>> Elaborate this one, please.
>>
>> So we need to define our own HID here, and cannot use PNP compatible
>> CID
>> (like PNP0501) as we cannot use the 8250 PNP driver.
>
> Why not? What are the impediments?
>

To support the host controller for this device, we will create an MFD, 
i.e. platform device, per slave device.

>> This is related to the Hisi LPC ACPI support, where we would create
>> an
>> MFD (i.e. platform device) for the UART.
>
> Why you can't do properly in ACPI?
>
>>>> - IO port iotype
>>>> - 16550-compatible
>>>>
>>>> For OF, we have 8250_of.c, and for PNP device we have 8250_pnp.c
>>>> drivers. However there does not seem to any driver satisfying
>>>> the above requirements. So this RFC is to find opinion on
>>>> modifying the Synopsys DW 8250_dw.c driver to support these
>>>> generic features.
>>>
>>> Synopsys 8250 is a particular case of platform drivers. It doesn't
>>> satisfy "8250-compatible UART" requirement.
>
>> Right, but I wanted to try to use the generic parts of the driver to
>> support this UART to save writing yet another driver.
>
> It's still odd. Why this one, why not 8250_foo_bar to touch instead?
>

Agreed, it's odd. I choose as 8250_dw.c as it has ACPI support already. 
And I recognise the hw from popularity. No stronger reasons than that.

Thanks,
John

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[Andy Shevchenko]

On Mon, 2018-02-26 at 09:33 +0000, John Garry wrote:
> On 23/02/2018 17:31, Andy Shevchenko wrote:
> > On Fri, 2018-02-23 at 11:02 +0000, John Garry wrote:
> > > On 23/02/2018 10:30, Andy Shevchenko wrote:
> > > > On Fri, 2018-02-23 at 02:42 +0800, John Garry wrote:

> > > We require it for a development board for our hip06 platform.
> > 
> > Okay, and this particular platform uses Synopsys IP?
> 
> As I see this uart is really a virtual 8250, so HW details like apb 
> clocks and the like are hidden, so may not be relevant.

Good to know.

> However I will check with the BMC team to know the specific details.

> > > > >  for supporting an 8250-compatible UART with
> > > > > the following profile/features:
> > > > > - platform device
> > > > > - polling mode (i.e. no interrupt support)
> > > > > - ACPI FW
> > > > 
> > > > Elaborate this one, please.
> > > 
> > > So we need to define our own HID here, and cannot use PNP
> > > compatible
> > > CID
> > > (like PNP0501) as we cannot use the 8250 PNP driver.
> > 
> > Why not? What are the impediments?
> > 
> 
> To support the host controller for this device, we will create an
> MFD, 
> i.e. platform device, per slave device.

There is no answer here...

> > > This is related to the Hisi LPC ACPI support, where we would
> > > create
> > > an
> > > MFD (i.e. platform device) for the UART.
> > 
> > Why you can't do properly in ACPI?

No answer here either.

Sorry, but with this level of communication it's no go for the series.

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

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[John Garry]

Hi Andy,

>>>> We require it for a development board for our hip06 platform.
>>>
>>> Okay, and this particular platform uses Synopsys IP?
>>
>> As I see this uart is really a virtual 8250, so HW details like apb
>> clocks and the like are hidden, so may not be relevant.
>
> Good to know.
>
>> However I will check with the BMC team to know the specific details.
>
>>>>>>  for supporting an 8250-compatible UART with
>>>>>> the following profile/features:
>>>>>> - platform device
>>>>>> - polling mode (i.e. no interrupt support)
>>>>>> - ACPI FW
>>>>>
>>>>> Elaborate this one, please.
>>>>
>>>> So we need to define our own HID here, and cannot use PNP
>>>> compatible
>>>> CID
>>>> (like PNP0501) as we cannot use the 8250 PNP driver.
>>>
>>> Why not? What are the impediments?
>>>
>>
>> To support the host controller for this device, we will create an
>> MFD,
>> i.e. platform device, per slave device.
>
> There is no answer here...
>
>>>> This is related to the Hisi LPC ACPI support, where we would
>>>> create
>>>> an
>>>> MFD (i.e. platform device) for the UART.
>>>
>>> Why you can't do properly in ACPI?
>
> No answer here either.
>
> Sorry, but with this level of communication it's no go for the series.
>

Sorry if my answers did not tell you want you want to know.

My point was that the 8250_pnp driver would be used for a pnp_device, 
but we are creating a platform device for this UART slave so would 
require a platform device driver, that which 8250_dw.c is. But I will 
check on pnp device support.

Much appreciated,
John

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[Andy Shevchenko]

On Mon, 2018-02-26 at 10:45 +0000, John Garry wrote:

> > > > > > >  for supporting an 8250-compatible UART with
> > > > > > > the following profile/features:
> > > > > > > - platform device
> > > > > > > - polling mode (i.e. no interrupt support)
> > > > > > > - ACPI FW
> > > > > > 
> > > > > > Elaborate this one, please.
> > > > > 
> > > > > So we need to define our own HID here, and cannot use PNP
> > > > > compatible
> > > > > CID
> > > > > (like PNP0501) as we cannot use the 8250 PNP driver.
> > > > 
> > > > Why not? What are the impediments?
> > > > 
> > > 
> > > To support the host controller for this device, we will create an
> > > MFD,
> > > i.e. platform device, per slave device.
> > 
> > There is no answer here...
> > 
> > > > > This is related to the Hisi LPC ACPI support, where we would
> > > > > create
> > > > > an
> > > > > MFD (i.e. platform device) for the UART.
> > > > 
> > > > Why you can't do properly in ACPI?
> > 
> > No answer here either.
> > 
> > Sorry, but with this level of communication it's no go for the
> > series.
> > 
> 
> Sorry if my answers did not tell you want you want to know.
> 
> My point was that the 8250_pnp driver would be used for a pnp_device, 
> but we are creating a platform device for this UART slave so would 
> require a platform device driver, that which 8250_dw.c is. But I will 
> check on pnp device support.

Perhaps it's not visible, though below is a description of the drivers
we have:

8250_dw - OF/ACPI driver for Synopsys DW (+ DW DMA)
8250_lpss - PCI driver for Synopsys DW (+ DW DMA)
8250_of -  generic 8250 compatible driver for OF
8250_pci - generic 8250 compatible driver for PCI
8250_pnp - generic 8250 compatible driver for ACPI

8250_* (except core parts) - custom glue drivers per some IPs

By description you gave your driver fits 8250_pnp if ACPI tables crafted
properly.

Share the ACPI excerpt and we can discuss further how to improve them.

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

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[John Garry]

>>>>> Why you can't do properly in ACPI?
>>>
>>> No answer here either.
>>>
>>> Sorry, but with this level of communication it's no go for the
>>> series.
>>>
>>
>> Sorry if my answers did not tell you want you want to know.
>>
>> My point was that the 8250_pnp driver would be used for a pnp_device,
>> but we are creating a platform device for this UART slave so would
>> require a platform device driver, that which 8250_dw.c is. But I will
>> check on pnp device support.
>

Hi Andy,

> Perhaps it's not visible, though below is a description of the drivers
> we have:
>
> 8250_dw - OF/ACPI driver for Synopsys DW (+ DW DMA)
> 8250_lpss - PCI driver for Synopsys DW (+ DW DMA)
> 8250_of -  generic 8250 compatible driver for OF
> 8250_pci - generic 8250 compatible driver for PCI
> 8250_pnp - generic 8250 compatible driver for ACPI
>
> 8250_* (except core parts) - custom glue drivers per some IPs
>
> By description you gave your driver fits 8250_pnp if ACPI tables crafted
> properly.
>
> Share the ACPI excerpt and we can discuss further how to improve them.
>

For a bit of background, MFD support was discussed here initially:
https://lkml.org/lkml/2017/6/13/796

Here is the ACPI table:
Scope(_SB) {
   Device (LPC0) {
     Name (_HID, "HISI0191")  // HiSi LPC
     Name (_CRS, ResourceTemplate () {
       Memory32Fixed (ReadWrite, 0xa01b0000, 0x1000)
     })
   }

   Device (LPC0.CON0) {
     Name (_HID, "HISI1031")
     // Name (_CID, "PNP0501") // cannot support PNP
     Name (LORS, ResourceTemplate() {
       QWordIO (
         ResourceConsumer,
     MinNotFixed,     // _MIF
     MaxNotFixed,     // _MAF
     PosDecode,
     EntireRange,
     0x0,             // _GRA
     0x2F8,           // _MIN
     0x3fff,          // _MAX
     0x0,             // _TRA
     0x08,            // _LEN
     , ,
     IO02


The latest framework changes and host driver patchset are here:
https://lkml.org/lkml/2018/2/19/465

Here is how we probe the children:
static int hisi_lpc_acpi_set_io_res(struct device *child,
                     struct device *hostdev,
                     const struct resource **res,
                     int *num_res)
{

     [ ... In this part we just get the child resources ]


     /* 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;
}

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;
}

As you know, this is not accepted upstream yet, but I really hope I'm 
close. Hence the RFC tag for the UART patchset.

Please let me know if you require more details.

Thanks,
John

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[Andy Shevchenko]

On Mon, 2018-02-26 at 11:56 +0000, John Garry wrote:
> > > > > > Why you can't do properly in ACPI?
> > > > 
> > > > No answer here either.
> > > > 
> > > > Sorry, but with this level of communication it's no go for the
> > > > series.
> > > > 
> > > 
> > > Sorry if my answers did not tell you want you want to know.
> > > 
> > > My point was that the 8250_pnp driver would be used for a
> > > pnp_device,
> > > but we are creating a platform device for this UART slave so would
> > > require a platform device driver, that which 8250_dw.c is. But I
> > > will
> > > check on pnp device support.
> 
> Hi Andy,
> 
> > Perhaps it's not visible, though below is a description of the
> > drivers
> > we have:
> > 
> > 8250_dw - OF/ACPI driver for Synopsys DW (+ DW DMA)
> > 8250_lpss - PCI driver for Synopsys DW (+ DW DMA)
> > 8250_of -  generic 8250 compatible driver for OF
> > 8250_pci - generic 8250 compatible driver for PCI
> > 8250_pnp - generic 8250 compatible driver for ACPI
> > 
> > 8250_* (except core parts) - custom glue drivers per some IPs
> > 
> > By description you gave your driver fits 8250_pnp if ACPI tables
> > crafted
> > properly.
> > 
> > Share the ACPI excerpt and we can discuss further how to improve
> > them.
> > 
> 
> For a bit of background, MFD support was discussed here initially:
> https://lkml.org/lkml/2017/6/13/796
> 
> Here is the ACPI table:
> Scope(_SB) {
>    Device (LPC0) {
>      Name (_HID, "HISI0191")  // HiSi LPC
>      Name (_CRS, ResourceTemplate () {
>        Memory32Fixed (ReadWrite, 0xa01b0000, 0x1000)
>      })
>    }
> 
>    Device (LPC0.CON0) {
>      Name (_HID, "HISI1031")
>      // Name (_CID, "PNP0501") // cannot support PNP
>      Name (LORS, ResourceTemplate() {
>        QWordIO (
>          ResourceConsumer,
>      MinNotFixed,     // _MIF
>      MaxNotFixed,     // _MAF
>      PosDecode,
>      EntireRange,
>      0x0,             // _GRA
>      0x2F8,           // _MIN

>      0x3fff,          // _MAX

Shouldn't be 0x2ff ?

>      0x0,             // _TRA
>      0x08,            // _LEN
>      , ,
>      IO02
> 
> 
> The latest framework changes and host driver patchset are here:
> https://lkml.org/lkml/2018/2/19/465
> 

It still doesn't explain impediments you have.

Just few approaches comes to my mind:
 - move UART outside of parent device
 - register PNP driver manually for that cell instead of MFD
 - use serial8250 platform driver (I totally forgot that we have a
generic platform driver, so, it might be what you need to use at the
end)

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

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[Andy Shevchenko]

On Mon, 2018-02-26 at 14:21 +0200, Andy Shevchenko wrote:
> On Mon, 2018-02-26 at 11:56 +0000, John Garry wrote:


> >    Device (LPC0.CON0) {
> >      Name (_HID, "HISI1031")

> >      // Name (_CID, "PNP0501") // cannot support PNP


One more question. What is the problem with this CID? Do you have a race
condition in enumeration?

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

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[John Garry]

On 26/02/2018 12:27, Andy Shevchenko wrote:
> On Mon, 2018-02-26 at 14:21 +0200, Andy Shevchenko wrote:
>> On Mon, 2018-02-26 at 11:56 +0000, John Garry wrote:
>
>
>>>    Device (LPC0.CON0) {
>>>      Name (_HID, "HISI1031")
>
>>>      // Name (_CID, "PNP0501") // cannot support PNP
>
>
> One more question. What is the problem with this CID? Do you have a race
> condition in enumeration?
>

Hi Andy,

Not sure if race condition exactly. I tried enabling this CID and a pnp 
device is created in pnpacpi_add_device_handler(), while we have already 
marked the corresponding acpi_device to skip enumeration in ACPI scan 
handler (by flagging it as a serial bus slave).

Thanks,
John

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[Andy Shevchenko]

On Mon, 2018-02-26 at 13:15 +0000, John Garry wrote:
> On 26/02/2018 12:27, Andy Shevchenko wrote:
> > On Mon, 2018-02-26 at 14:21 +0200, Andy Shevchenko wrote:
> > > On Mon, 2018-02-26 at 11:56 +0000, John Garry wrote:
> > 
> > 
> > > >    Device (LPC0.CON0) {
> > > >      Name (_HID, "HISI1031")
> > > >      // Name (_CID, "PNP0501") // cannot support PNP
> > 
> > 
> > One more question. What is the problem with this CID? Do you have a
> > race
> > condition in enumeration?
> > 
> 
> Hi Andy,
> 
> Not sure if race condition exactly. I tried enabling this CID and a
> pnp 
> device is created in pnpacpi_add_device_handler(), while we have
> already 
> marked the corresponding acpi_device to skip enumeration in ACPI scan 
> handler (by flagging it as a serial bus slave).

Is that code already in upstream?

If no, please, Cc next version to me and possible Mika.

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

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[John Garry]

On 26/02/2018 15:02, Andy Shevchenko wrote:
> On Mon, 2018-02-26 at 13:15 +0000, John Garry wrote:
>> On 26/02/2018 12:27, Andy Shevchenko wrote:
>>> On Mon, 2018-02-26 at 14:21 +0200, Andy Shevchenko wrote:
>>>> On Mon, 2018-02-26 at 11:56 +0000, John Garry wrote:
>>>
>>>
>>>>>    Device (LPC0.CON0) {
>>>>>      Name (_HID, "HISI1031")
>>>>>      // Name (_CID, "PNP0501") // cannot support PNP
>>>
>>>
>>> One more question. What is the problem with this CID? Do you have a
>>> race
>>> condition in enumeration?
>>>
>>
>> Hi Andy,
>>
>> Not sure if race condition exactly. I tried enabling this CID and a
>> pnp
>> device is created in pnpacpi_add_device_handler(), while we have
>> already
>> marked the corresponding acpi_device to skip enumeration in ACPI scan
>> handler (by flagging it as a serial bus slave).
>
> Is that code already in upstream?

No, not yet.

>
> If no, please, Cc next version to me and possible Mika.

Of course. I should be sending it later today.

>
All the best,
John

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[John Garry]

On 26/02/2018 15:07, John Garry wrote:
> On 26/02/2018 15:02, Andy Shevchenko wrote:
>> On Mon, 2018-02-26 at 13:15 +0000, John Garry wrote:
>>> On 26/02/2018 12:27, Andy Shevchenko wrote:
>>>> On Mon, 2018-02-26 at 14:21 +0200, Andy Shevchenko wrote:
>>>>> On Mon, 2018-02-26 at 11:56 +0000, John Garry wrote:
>>>>
>>>>
>>>>>>    Device (LPC0.CON0) {
>>>>>>      Name (_HID, "HISI1031")
>>>>>>      // Name (_CID, "PNP0501") // cannot support PNP
>>>>
>>>>
>>>> One more question. What is the problem with this CID? Do you have a
>>>> race
>>>> condition in enumeration?
>>>>
>>>
>>> Hi Andy,
>>>
>>> Not sure if race condition exactly. I tried enabling this CID and a
>>> pnp
>>> device is created in pnpacpi_add_device_handler(), while we have
>>> already
>>> marked the corresponding acpi_device to skip enumeration in ACPI scan
>>> handler (by flagging it as a serial bus slave).
>>
>> Is that code already in upstream?
>
> No, not yet.
>
>>
>> If no, please, Cc next version to me and possible Mika.
>
> Of course. I should be sending it later today.
>

Hi Andy,

A while ago we discussed on this thread the possibility of adding 
generic 8250 IO space platform driver support for ACPI FW.

In this discussion I mentioned that we require specifically platform 
device support, and not PNP device support, as this is how we enumerate 
the devices in the host controller driver. I think you're familiar with 
this driver - here is the thread posting for reference: 
https://lkml.org/lkml/2018/3/6/230

I would say that there were 2 main takeaway points:
a. for 8250-compatible UART, we should use a PNP driver for ACPI FW
b. you prefered us to change the host driver to use an ACPI handler approach

For b., I was not keen as we already did try the handler in the ACPI 
core code, but this was not so welcome. Reasoning here: 
https://lkml.org/lkml/2018/2/14/532

I did also say that I would prefer not to change approach after a very 
long upstream effort, with no clear end in sight.

However do you have an idea on creating a PNP device in a.? That is, 
enumerate (create) a 8250 PNP device.

If you look at the least driver here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bus/hisi_lpc.c

We could have this working with a change in the ACPI probe code, like in 
this code snippet:

list_for_each_entry(child, &adev->children, node) {
	struct resource_entry *rentry;
	LIST_HEAD(resource_list);
         int rc;

         if (!acpi_is_pnp_device(child))
             continue;

         acpi_dev_get_resources(child, &resource_list, NULL, NULL);

         list_for_each_entry(rentry, &resource_list, node) {
             struct resource *res = rentry->res;

             if (res->flags | IORESOURCE_IO)
                 hisi_lpc_acpi_xlat_io_res(child, adev, res); /* bad */
         }
         rc = pnpacpi_add_device(child);
         if (rc)
             return rc;
}

Obviously this is not sound as we should not modify the child 
acpi_device resources.

Alternatively, as another approach, I could copy the relevant code 
pnpacpi_add_device() verbatim into this above code, and xlat the 
resource of the PNP device, but it's not good to copy the code like.

Any other ideas?

All the best,
John

>>
> All the best,
> John

Re: [RFC PATCH 0/2] serial: 8250_dw: IO space + polling mode support

[John Garry]

On 26/02/2018 12:21, Andy Shevchenko wrote:
> On Mon, 2018-02-26 at 11:56 +0000, John Garry wrote:
>>>>>>> Why you can't do properly in ACPI?
>>>>>
>>>>> No answer here either.
>>>>>
>>>>> Sorry, but with this level of communication it's no go for the
>>>>> series.
>>>>>
>>>>
>>>> Sorry if my answers did not tell you want you want to know.
>>>>
>>>> My point was that the 8250_pnp driver would be used for a
>>>> pnp_device,
>>>> but we are creating a platform device for this UART slave so would
>>>> require a platform device driver, that which 8250_dw.c is. But I
>>>> will
>>>> check on pnp device support.
>>
>> Hi Andy,
>>
>>> Perhaps it's not visible, though below is a description of the
>>> drivers
>>> we have:
>>>
>>> 8250_dw - OF/ACPI driver for Synopsys DW (+ DW DMA)
>>> 8250_lpss - PCI driver for Synopsys DW (+ DW DMA)
>>> 8250_of -  generic 8250 compatible driver for OF
>>> 8250_pci - generic 8250 compatible driver for PCI
>>> 8250_pnp - generic 8250 compatible driver for ACPI
>>>
>>> 8250_* (except core parts) - custom glue drivers per some IPs
>>>
>>> By description you gave your driver fits 8250_pnp if ACPI tables
>>> crafted
>>> properly.
>>>
>>> Share the ACPI excerpt and we can discuss further how to improve
>>> them.
>>>

Hi Andy,

>>
>> For a bit of background, MFD support was discussed here initially:
>> https://lkml.org/lkml/2017/6/13/796
>>
>> Here is the ACPI table:
>> Scope(_SB) {
>>    Device (LPC0) {
>>      Name (_HID, "HISI0191")  // HiSi LPC
>>      Name (_CRS, ResourceTemplate () {
>>        Memory32Fixed (ReadWrite, 0xa01b0000, 0x1000)
>>      })
>>    }
>>
>>    Device (LPC0.CON0) {
>>      Name (_HID, "HISI1031")
>>      // Name (_CID, "PNP0501") // cannot support PNP
>>      Name (LORS, ResourceTemplate() {
>>        QWordIO (
>>          ResourceConsumer,
>>      MinNotFixed,     // _MIF
>>      MaxNotFixed,     // _MAF
>>      PosDecode,
>>      EntireRange,
>>      0x0,             // _GRA
>>      0x2F8,           // _MIN
>
>>      0x3fff,          // _MAX
>
> Shouldn't be 0x2ff ?

Yes, something like this. I have asked for it to be fixed.

>
>>      0x0,             // _TRA
>>      0x08,            // _LEN
>>      , ,
>>      IO02
>>
>>
>> The latest framework changes and host driver patchset are here:
>> https://lkml.org/lkml/2018/2/19/465
>>
>
> It still doesn't explain impediments you have.
>
> Just few approaches comes to my mind:
>  - move UART outside of parent device

In this case the UART would not be a child and would not be enumerated 
to have the correct iobase.

>  - register PNP driver manually for that cell instead of MFD

Maybe it could work.

>  - use serial8250 platform driver (I totally forgot that we have a
> generic platform driver, so, it might be what you need to use at the
> end)

Let me check this also. I am not fimilar.

>

Thanks for the support!
John