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

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 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 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: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

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