diff mbox

[V8,5/6] ACPI: Support the probing on the devices which apply indirect-IO

Message ID 1490887619-61732-6-git-send-email-yuanzhichang@hisilicon.com
State Not Applicable
Headers show

Commit Message

Zhichang Yuan March 30, 2017, 3:26 p.m. UTC
On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
with some special host-local I/O ports known on x86. To access the I/O
peripherals, an indirect-IO mechanism is introduced to mapped the host-local
I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
separate I/O space exists. Just as PCI MMIO, the host I/O range should be
registered before probing the downstream devices and set up the I/O mapping.
But current ACPI bus probing doesn't support these indirect-IO hosts/devices.

This patch introdueces a new ACPI handler for this device category. Through the
handler attach callback, the indirect-IO hosts I/O registration is done and
all peripherals' I/O resources are translated into logic/fake PIO before
starting the enumeration.

Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 drivers/acpi/Makefile          |   1 +
 drivers/acpi/acpi_indirectio.c | 344 +++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h        |   5 +
 drivers/acpi/scan.c            |   1 +
 4 files changed, 351 insertions(+)
 create mode 100644 drivers/acpi/acpi_indirectio.c

Comments

Rafael J. Wysocki March 30, 2017, 8:31 p.m. UTC | #1
On Thursday, March 30, 2017 11:26:58 PM zhichang.yuan wrote:
> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
> with some special host-local I/O ports known on x86. To access the I/O
> peripherals, an indirect-IO mechanism is introduced to mapped the host-local
> I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
> separate I/O space exists. Just as PCI MMIO, the host I/O range should be
> registered before probing the downstream devices and set up the I/O mapping.
> But current ACPI bus probing doesn't support these indirect-IO hosts/devices.
> 
> This patch introdueces a new ACPI handler for this device category. Through the
> handler attach callback, the indirect-IO hosts I/O registration is done and
> all peripherals' I/O resources are translated into logic/fake PIO before
> starting the enumeration.

Can you explain to me briefly what exactly this code is expected to be doing?

Thanks,
Rafael
Zhichang Yuan March 31, 2017, 6:52 a.m. UTC | #2
Hi, Rafael,

Thanks for reviewing this!

On 2017/3/31 4:31, Rafael J. Wysocki wrote:
> On Thursday, March 30, 2017 11:26:58 PM zhichang.yuan wrote:
>> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
>> with some special host-local I/O ports known on x86. To access the I/O
>> peripherals, an indirect-IO mechanism is introduced to mapped the host-local
>> I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
>> separate I/O space exists. Just as PCI MMIO, the host I/O range should be
>> registered before probing the downstream devices and set up the I/O mapping.
>> But current ACPI bus probing doesn't support these indirect-IO hosts/devices.
>>
>> This patch introdueces a new ACPI handler for this device category. Through the
>> handler attach callback, the indirect-IO hosts I/O registration is done and
>> all peripherals' I/O resources are translated into logic/fake PIO before
>> starting the enumeration.
> 
> Can you explain to me briefly what exactly this code is expected to be doing?

As you know currently for ARM architecture IO space is memory mapped and
is only used by pci devices. The port number is dynamically allocated
converting the device IO address into a PIO token: i.e.
http://lxr.free-electrons.com/source/drivers/acpi/pci_root.c#L745
This patch is meant to support a new class of IO host controller
that are not PCI based and that still require to have the IO addresses
be translated in the same PIO token space as the PCI controller

Thanks,
Zhichang

> 
> Thanks,
> Rafael
> 
> 
> .
>
Rafael J. Wysocki March 31, 2017, 11:02 p.m. UTC | #3
On Fri, Mar 31, 2017 at 8:52 AM, zhichang.yuan
<yuanzhichang@hisilicon.com> wrote:
> Hi, Rafael,
>
> Thanks for reviewing this!
>
> On 2017/3/31 4:31, Rafael J. Wysocki wrote:
>> On Thursday, March 30, 2017 11:26:58 PM zhichang.yuan wrote:
>>> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
>>> with some special host-local I/O ports known on x86. To access the I/O
>>> peripherals, an indirect-IO mechanism is introduced to mapped the host-local
>>> I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
>>> separate I/O space exists. Just as PCI MMIO, the host I/O range should be
>>> registered before probing the downstream devices and set up the I/O mapping.
>>> But current ACPI bus probing doesn't support these indirect-IO hosts/devices.
>>>
>>> This patch introdueces a new ACPI handler for this device category. Through the
>>> handler attach callback, the indirect-IO hosts I/O registration is done and
>>> all peripherals' I/O resources are translated into logic/fake PIO before
>>> starting the enumeration.
>>
>> Can you explain to me briefly what exactly this code is expected to be doing?
>
> As you know currently for ARM architecture IO space is memory mapped and
> is only used by pci devices. The port number is dynamically allocated
> converting the device IO address into a PIO token: i.e.
> http://lxr.free-electrons.com/source/drivers/acpi/pci_root.c#L745
> This patch is meant to support a new class of IO host controller
> that are not PCI based and that still require to have the IO addresses
> be translated in the same PIO token space as the PCI controller

IOW, this is ARM-specific, right?

Thanks,
Rafael
zhichang April 1, 2017, 2:16 a.m. UTC | #4
On 04/01/2017 07:02 AM, Rafael J. Wysocki wrote:
> On Fri, Mar 31, 2017 at 8:52 AM, zhichang.yuan
> <yuanzhichang@hisilicon.com> wrote:
>> Hi, Rafael,
>>
>> Thanks for reviewing this!
>>
>> On 2017/3/31 4:31, Rafael J. Wysocki wrote:
>>> On Thursday, March 30, 2017 11:26:58 PM zhichang.yuan wrote:
>>>> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
>>>> with some special host-local I/O ports known on x86. To access the I/O
>>>> peripherals, an indirect-IO mechanism is introduced to mapped the host-local
>>>> I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
>>>> separate I/O space exists. Just as PCI MMIO, the host I/O range should be
>>>> registered before probing the downstream devices and set up the I/O mapping.
>>>> But current ACPI bus probing doesn't support these indirect-IO hosts/devices.
>>>>
>>>> This patch introdueces a new ACPI handler for this device category. Through the
>>>> handler attach callback, the indirect-IO hosts I/O registration is done and
>>>> all peripherals' I/O resources are translated into logic/fake PIO before
>>>> starting the enumeration.
>>>
>>> Can you explain to me briefly what exactly this code is expected to be doing?
>>
>> As you know currently for ARM architecture IO space is memory mapped and
>> is only used by pci devices. The port number is dynamically allocated
>> converting the device IO address into a PIO token: i.e.
>> http://lxr.free-electrons.com/source/drivers/acpi/pci_root.c#L745
>> This patch is meant to support a new class of IO host controller
>> that are not PCI based and that still require to have the IO addresses
>> be translated in the same PIO token space as the PCI controller
> 
> IOW, this is ARM-specific, right?

Yes. The current host added in this patch with _HID "HISI0191" is on ARM64.
But, I think the handler driver is architecture dependent.

Thanks,
Zhichang

> 
> Thanks,
> Rafael
>
Rafael J. Wysocki April 1, 2017, 9:52 a.m. UTC | #5
On Sat, Apr 1, 2017 at 4:16 AM, zhichang.yuan <zhichang.yuan02@gmail.com> wrote:
>
>
> On 04/01/2017 07:02 AM, Rafael J. Wysocki wrote:
>> On Fri, Mar 31, 2017 at 8:52 AM, zhichang.yuan
>> <yuanzhichang@hisilicon.com> wrote:
>>> Hi, Rafael,
>>>
>>> Thanks for reviewing this!
>>>
>>> On 2017/3/31 4:31, Rafael J. Wysocki wrote:
>>>> On Thursday, March 30, 2017 11:26:58 PM zhichang.yuan wrote:
>>>>> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
>>>>> with some special host-local I/O ports known on x86. To access the I/O
>>>>> peripherals, an indirect-IO mechanism is introduced to mapped the host-local
>>>>> I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
>>>>> separate I/O space exists. Just as PCI MMIO, the host I/O range should be
>>>>> registered before probing the downstream devices and set up the I/O mapping.
>>>>> But current ACPI bus probing doesn't support these indirect-IO hosts/devices.
>>>>>
>>>>> This patch introdueces a new ACPI handler for this device category. Through the
>>>>> handler attach callback, the indirect-IO hosts I/O registration is done and
>>>>> all peripherals' I/O resources are translated into logic/fake PIO before
>>>>> starting the enumeration.
>>>>
>>>> Can you explain to me briefly what exactly this code is expected to be doing?
>>>
>>> As you know currently for ARM architecture IO space is memory mapped and
>>> is only used by pci devices. The port number is dynamically allocated
>>> converting the device IO address into a PIO token: i.e.
>>> http://lxr.free-electrons.com/source/drivers/acpi/pci_root.c#L745
>>> This patch is meant to support a new class of IO host controller
>>> that are not PCI based and that still require to have the IO addresses
>>> be translated in the same PIO token space as the PCI controller
>>
>> IOW, this is ARM-specific, right?
>
> Yes. The current host added in this patch with _HID "HISI0191" is on ARM64.

But the underlying mechanism is ARM-specific as well AFAICS.

> But, I think the handler driver is architecture dependent.

I guess you mean "independent"?  That doesn't matter.

If ARM64 is the only architecture to use it in foreseeable future
(which is the case for all I can say), it should go into acpi/arm64/
and please ask the maintainers thereof to review it.

Thanks,
Rafael
Gabriele Paoloni April 2, 2017, 2:58 p.m. UTC | #6
Hi Rafael 
Many thanks for your reply

> -----Original Message-----

> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of

> Rafael J. Wysocki

> Sent: 01 April 2017 10:52

> To: zhichang.yuan

> Cc: Rafael J. Wysocki; Yuanzhichang; Rafael J. Wysocki; Catalin

> Marinas; Will Deacon; Rob Herring; Frank Rowand; Bjorn Helgaas; Arnd

> Bergmann; linux-arm-kernel@lists.infradead.org; Mark Rutland; Brian

> Starkey; Olof Johansson; Lorenzo Pieralisi; Benjamin Herrenschmidt;

> Linux Kernel Mailing List; ACPI Devel Maling List; Linuxarm;

> devicetree@vger.kernel.org; Linux PCI; Corey Minyard; Zou Rongrong;

> John Garry; Gabriele Paoloni; kantyzc@163.com; xuwei (O)

> Subject: Re: [PATCH V8 5/6] ACPI: Support the probing on the devices

> which apply indirect-IO

> 

> On Sat, Apr 1, 2017 at 4:16 AM, zhichang.yuan

> <zhichang.yuan02@gmail.com> wrote:

> >

> >

> > On 04/01/2017 07:02 AM, Rafael J. Wysocki wrote:

> >> On Fri, Mar 31, 2017 at 8:52 AM, zhichang.yuan

> >> <yuanzhichang@hisilicon.com> wrote:

> >>> Hi, Rafael,

> >>>

> >>> Thanks for reviewing this!

> >>>

> >>> On 2017/3/31 4:31, Rafael J. Wysocki wrote:

> >>>> On Thursday, March 30, 2017 11:26:58 PM zhichang.yuan wrote:

> >>>>> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC

> devices access I/O

> >>>>> with some special host-local I/O ports known on x86. To access

> the I/O

> >>>>> peripherals, an indirect-IO mechanism is introduced to mapped the

> host-local

> >>>>> I/O to system logical/fake PIO similar the PCI MMIO on

> architectures where no

> >>>>> separate I/O space exists. Just as PCI MMIO, the host I/O range

> should be

> >>>>> registered before probing the downstream devices and set up the

> I/O mapping.

> >>>>> But current ACPI bus probing doesn't support these indirect-IO

> hosts/devices.

> >>>>>

> >>>>> This patch introdueces a new ACPI handler for this device

> category. Through the

> >>>>> handler attach callback, the indirect-IO hosts I/O registration

> is done and

> >>>>> all peripherals' I/O resources are translated into logic/fake PIO

> before

> >>>>> starting the enumeration.

> >>>>

> >>>> Can you explain to me briefly what exactly this code is expected

> to be doing?

> >>>

> >>> As you know currently for ARM architecture IO space is memory

> mapped and

> >>> is only used by pci devices. The port number is dynamically

> allocated

> >>> converting the device IO address into a PIO token: i.e.

> >>> http://lxr.free-electrons.com/source/drivers/acpi/pci_root.c#L745

> >>> This patch is meant to support a new class of IO host controller

> >>> that are not PCI based and that still require to have the IO

> addresses

> >>> be translated in the same PIO token space as the PCI controller

> >>

> >> IOW, this is ARM-specific, right?

> >

> > Yes. The current host added in this patch with _HID "HISI0191" is on

> ARM64.

> 

> But the underlying mechanism is ARM-specific as well AFAICS.

> 

> > But, I think the handler driver is architecture dependent.

> 

> I guess you mean "independent"?  That doesn't matter.

> 

> If ARM64 is the only architecture to use it in foreseeable future

> (which is the case for all I can say), it should go into acpi/arm64/

> and please ask the maintainers thereof to review it.


I guess this is the case for the foreseeable future.

So if my understanding is correct we should leave acpi_indirectio_scan_init()
call in acpi_scan_init() and move its definition under acpi/arm64/ right?

If in future other architectures needs non-pci IO controllers we may consider
to move this to acpi/...

Lorenzo what do you think? Could you have a look at the patchset?

Many thanks

Gab

> 

> Thanks,

> Rafael
dann frazier April 20, 2017, 8:57 p.m. UTC | #7
On Thu, Mar 30, 2017 at 9:26 AM, zhichang.yuan
<yuanzhichang@hisilicon.com> wrote:
> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
> with some special host-local I/O ports known on x86. To access the I/O
> peripherals, an indirect-IO mechanism is introduced to mapped the host-local
> I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
> separate I/O space exists. Just as PCI MMIO, the host I/O range should be
> registered before probing the downstream devices and set up the I/O mapping.
> But current ACPI bus probing doesn't support these indirect-IO hosts/devices.
>
> This patch introdueces a new ACPI handler for this device category. Through the
> handler attach callback, the indirect-IO hosts I/O registration is done and
> all peripherals' I/O resources are translated into logic/fake PIO before
> starting the enumeration.
>
> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> ---
>  drivers/acpi/Makefile          |   1 +
>  drivers/acpi/acpi_indirectio.c | 344 +++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/internal.h        |   5 +
>  drivers/acpi/scan.c            |   1 +
>  4 files changed, 351 insertions(+)
>  create mode 100644 drivers/acpi/acpi_indirectio.c
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index a391bbc..10e5f2b 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -57,6 +57,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
>  acpi-y                         += acpi_lpat.o
>  acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
>  acpi-$(CONFIG_ACPI_WATCHDOG)   += acpi_watchdog.o
> +acpi-$(CONFIG_INDIRECT_PIO)    += acpi_indirectio.o
>
>  # These are (potentially) separate modules
>
> diff --git a/drivers/acpi/acpi_indirectio.c b/drivers/acpi/acpi_indirectio.c
> new file mode 100644
> index 0000000..c8c80b5
> --- /dev/null
> +++ b/drivers/acpi/acpi_indirectio.c
> @@ -0,0 +1,344 @@
> +/*
> + * ACPI support for indirect-IO bus.
> + *
> + * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +#include <linux/logic_pio.h>
> +
> +#include "internal.h"
> +
> +ACPI_MODULE_NAME("indirect IO");
> +
> +#define INDIRECT_IO_INFO(desc) ((unsigned long)&desc)
> +
> +struct lpc_private_data {
> +       resource_size_t io_size;
> +       resource_size_t io_start;
> +};
> +
> +struct indirectio_device_desc {
> +       void *pdata; /* device relevant info data */
> +       int (*pre_setup)(struct acpi_device *adev, void *pdata);
> +};
> +
> +static struct lpc_private_data lpc_data = {
> +       .io_size = LPC_BUS_IO_SIZE,
> +       .io_start = LPC_MIN_BUS_RANGE,
> +};
> +
> +static inline bool acpi_logicio_supported_resource(struct acpi_resource *res)
> +{
> +       switch (res->type) {
> +       case ACPI_RESOURCE_TYPE_ADDRESS32:
> +       case ACPI_RESOURCE_TYPE_ADDRESS64:
> +               return true;
> +       }
> +       return false;
> +}
> +
> +static acpi_status acpi_count_logiciores(struct acpi_resource *res,
> +                                          void *data)
> +{
> +       int *res_cnt = data;
> +
> +       if (acpi_logicio_supported_resource(res) &&
> +               !acpi_dev_filter_resource_type(res, IORESOURCE_IO))
> +               (*res_cnt)++;
> +
> +       return AE_OK;
> +}
> +
> +static acpi_status acpi_read_one_logiciores(struct acpi_resource *res,
> +               void *data)
> +{
> +       struct acpi_resource **resource = data;
> +
> +       if (acpi_logicio_supported_resource(res) &&
> +               !acpi_dev_filter_resource_type(res, IORESOURCE_IO)) {
> +               memcpy((*resource), res, sizeof(struct acpi_resource));
> +               (*resource)->length = sizeof(struct acpi_resource);
> +               (*resource)->type = res->type;
> +               (*resource)++;
> +       }
> +
> +       return AE_OK;
> +}
> +
> +static acpi_status
> +acpi_build_logiciores_template(struct acpi_device *adev,
> +                       struct acpi_buffer *buffer)
> +{
> +       acpi_handle handle = adev->handle;
> +       struct acpi_resource *resource;
> +       acpi_status status;
> +       int res_cnt = 0;
> +
> +       status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> +                                    acpi_count_logiciores, &res_cnt);
> +       if (ACPI_FAILURE(status) || !res_cnt) {
> +               dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status);
> +               return -EINVAL;
> +       }
> +
> +       buffer->length = sizeof(struct acpi_resource) * (res_cnt + 1) + 1;
> +       buffer->pointer = kzalloc(buffer->length - 1, GFP_KERNEL);

(Seth Forshee noticed this issue, just passing it on)

Should this just allocate the full buffer->length? That would keep the
length attribute accurate (possibly avoiding an off-by-1 error later).
It's not clear what the trailing byte is needed for, but other drivers
allocate it as well (drivers/acpi/pci_link.c and
drivers/platform/x86/sony-laptop.c).

 -dann
zhichang April 21, 2017, 2:22 a.m. UTC | #8
Hi, Dann,



On 04/21/2017 04:57 AM, dann frazier wrote:
> On Thu, Mar 30, 2017 at 9:26 AM, zhichang.yuan
> <yuanzhichang@hisilicon.com> wrote:
>> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
>> with some special host-local I/O ports known on x86. To access the I/O
>> peripherals, an indirect-IO mechanism is introduced to mapped the host-local
>> I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
>> separate I/O space exists. Just as PCI MMIO, the host I/O range should be
>> registered before probing the downstream devices and set up the I/O mapping.
>> But current ACPI bus probing doesn't support these indirect-IO hosts/devices.
>>
>> This patch introdueces a new ACPI handler for this device category. Through the
>> handler attach callback, the indirect-IO hosts I/O registration is done and
>> all peripherals' I/O resources are translated into logic/fake PIO before
>> starting the enumeration.
>>
>> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>> ---
>>  drivers/acpi/Makefile          |   1 +
>>  drivers/acpi/acpi_indirectio.c | 344 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/acpi/internal.h        |   5 +
>>  drivers/acpi/scan.c            |   1 +
>>  4 files changed, 351 insertions(+)
>>  create mode 100644 drivers/acpi/acpi_indirectio.c
>>
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index a391bbc..10e5f2b 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -57,6 +57,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
>>  acpi-y                         += acpi_lpat.o
>>  acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
>>  acpi-$(CONFIG_ACPI_WATCHDOG)   += acpi_watchdog.o
>> +acpi-$(CONFIG_INDIRECT_PIO)    += acpi_indirectio.o
>>
>>  # These are (potentially) separate modules
>>
>> diff --git a/drivers/acpi/acpi_indirectio.c b/drivers/acpi/acpi_indirectio.c
>> new file mode 100644
>> index 0000000..c8c80b5
>> --- /dev/null
>> +++ b/drivers/acpi/acpi_indirectio.c
>> @@ -0,0 +1,344 @@

[snip]

>> +acpi_build_logiciores_template(struct acpi_device *adev,
>> +                       struct acpi_buffer *buffer)
>> +{
>> +       acpi_handle handle = adev->handle;
>> +       struct acpi_resource *resource;
>> +       acpi_status status;
>> +       int res_cnt = 0;
>> +
>> +       status = acpi_walk_resources(handle, METHOD_NAME__CRS,
>> +                                    acpi_count_logiciores, &res_cnt);
>> +       if (ACPI_FAILURE(status) || !res_cnt) {
>> +               dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status);
>> +               return -EINVAL;
>> +       }
>> +
>> +       buffer->length = sizeof(struct acpi_resource) * (res_cnt + 1) + 1;
>> +       buffer->pointer = kzalloc(buffer->length - 1, GFP_KERNEL);
> 
> (Seth Forshee noticed this issue, just passing it on)
> 
> Should this just allocate the full buffer->length? That would keep the
> length attribute accurate (possibly avoiding an off-by-1 error later).
> It's not clear what the trailing byte is needed for, but other drivers
> allocate it as well (drivers/acpi/pci_link.c and
> drivers/platform/x86/sony-laptop.c).

Thanks for your suggestion!

I also curious why this one appended byte is needed as it seems the later
acpi_set_current_resources() doesn't use this byte.
And I tested without setting the buffer->length as the length of resource list
directly, it seems ok.

But anyway, it looks more reasonable to allocate the memory with the
buffer->length rather than buffer->length - 1;

I was made the V9 patch-set, and I can add your suggestion there. But I also
awaiting for ARM64 ACPI maintainer's comment about this patch before really
sending V9. I wonder whether there is better way to make our indirect-IO devices
can be assigned the logic PIO before the enumeration...

Lorenzo, Hanjun, what do you think about this patch?

Thanks,
Zhichang

> 
>  -dann
>
Lorenzo Pieralisi April 21, 2017, 5:14 p.m. UTC | #9
On Fri, Apr 21, 2017 at 10:22:52AM +0800, zhichang.yuan wrote:
> Hi, Dann,
> 
> 
> 
> On 04/21/2017 04:57 AM, dann frazier wrote:
> > On Thu, Mar 30, 2017 at 9:26 AM, zhichang.yuan
> > <yuanzhichang@hisilicon.com> wrote:
> >> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
> >> with some special host-local I/O ports known on x86. To access the I/O
> >> peripherals, an indirect-IO mechanism is introduced to mapped the host-local
> >> I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
> >> separate I/O space exists. Just as PCI MMIO, the host I/O range should be
> >> registered before probing the downstream devices and set up the I/O mapping.
> >> But current ACPI bus probing doesn't support these indirect-IO hosts/devices.
> >>
> >> This patch introdueces a new ACPI handler for this device category. Through the
> >> handler attach callback, the indirect-IO hosts I/O registration is done and
> >> all peripherals' I/O resources are translated into logic/fake PIO before
> >> starting the enumeration.
> >>
> >> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> >> ---
> >>  drivers/acpi/Makefile          |   1 +
> >>  drivers/acpi/acpi_indirectio.c | 344 +++++++++++++++++++++++++++++++++++++++++
> >>  drivers/acpi/internal.h        |   5 +
> >>  drivers/acpi/scan.c            |   1 +
> >>  4 files changed, 351 insertions(+)
> >>  create mode 100644 drivers/acpi/acpi_indirectio.c
> >>
> >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >> index a391bbc..10e5f2b 100644
> >> --- a/drivers/acpi/Makefile
> >> +++ b/drivers/acpi/Makefile
> >> @@ -57,6 +57,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
> >>  acpi-y                         += acpi_lpat.o
> >>  acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
> >>  acpi-$(CONFIG_ACPI_WATCHDOG)   += acpi_watchdog.o
> >> +acpi-$(CONFIG_INDIRECT_PIO)    += acpi_indirectio.o
> >>
> >>  # These are (potentially) separate modules
> >>
> >> diff --git a/drivers/acpi/acpi_indirectio.c b/drivers/acpi/acpi_indirectio.c
> >> new file mode 100644
> >> index 0000000..c8c80b5
> >> --- /dev/null
> >> +++ b/drivers/acpi/acpi_indirectio.c
> >> @@ -0,0 +1,344 @@
> 
> [snip]
> 
> >> +acpi_build_logiciores_template(struct acpi_device *adev,
> >> +                       struct acpi_buffer *buffer)
> >> +{
> >> +       acpi_handle handle = adev->handle;
> >> +       struct acpi_resource *resource;
> >> +       acpi_status status;
> >> +       int res_cnt = 0;
> >> +
> >> +       status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> >> +                                    acpi_count_logiciores, &res_cnt);
> >> +       if (ACPI_FAILURE(status) || !res_cnt) {
> >> +               dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status);
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       buffer->length = sizeof(struct acpi_resource) * (res_cnt + 1) + 1;
> >> +       buffer->pointer = kzalloc(buffer->length - 1, GFP_KERNEL);
> > 
> > (Seth Forshee noticed this issue, just passing it on)
> > 
> > Should this just allocate the full buffer->length? That would keep the
> > length attribute accurate (possibly avoiding an off-by-1 error later).
> > It's not clear what the trailing byte is needed for, but other drivers
> > allocate it as well (drivers/acpi/pci_link.c and
> > drivers/platform/x86/sony-laptop.c).
> 
> Thanks for your suggestion!
> 
> I also curious why this one appended byte is needed as it seems the later
> acpi_set_current_resources() doesn't use this byte.
> And I tested without setting the buffer->length as the length of resource list
> directly, it seems ok.
> 
> But anyway, it looks more reasonable to allocate the memory with the
> buffer->length rather than buffer->length - 1;
> 
> I was made the V9 patch-set, and I can add your suggestion there. But I also
> awaiting for ARM64 ACPI maintainer's comment about this patch before really
> sending V9. I wonder whether there is better way to make our indirect-IO devices
> can be assigned the logic PIO before the enumeration...
> 
> Lorenzo, Hanjun, what do you think about this patch?

I will get to it shortly, sorry for the delay.

Thanks,
Lorenzo
diff mbox

Patch

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index a391bbc..10e5f2b 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -57,6 +57,7 @@  acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
 acpi-y				+= acpi_lpat.o
 acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
 acpi-$(CONFIG_ACPI_WATCHDOG)	+= acpi_watchdog.o
+acpi-$(CONFIG_INDIRECT_PIO)	+= acpi_indirectio.o
 
 # These are (potentially) separate modules
 
diff --git a/drivers/acpi/acpi_indirectio.c b/drivers/acpi/acpi_indirectio.c
new file mode 100644
index 0000000..c8c80b5
--- /dev/null
+++ b/drivers/acpi/acpi_indirectio.c
@@ -0,0 +1,344 @@ 
+/*
+ * ACPI support for indirect-IO bus.
+ *
+ * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+#include <linux/logic_pio.h>
+
+#include "internal.h"
+
+ACPI_MODULE_NAME("indirect IO");
+
+#define INDIRECT_IO_INFO(desc) ((unsigned long)&desc)
+
+struct lpc_private_data {
+	resource_size_t io_size;
+	resource_size_t io_start;
+};
+
+struct indirectio_device_desc {
+	void *pdata; /* device relevant info data */
+	int (*pre_setup)(struct acpi_device *adev, void *pdata);
+};
+
+static struct lpc_private_data lpc_data = {
+	.io_size = LPC_BUS_IO_SIZE,
+	.io_start = LPC_MIN_BUS_RANGE,
+};
+
+static inline bool acpi_logicio_supported_resource(struct acpi_resource *res)
+{
+	switch (res->type) {
+	case ACPI_RESOURCE_TYPE_ADDRESS32:
+	case ACPI_RESOURCE_TYPE_ADDRESS64:
+		return true;
+	}
+	return false;
+}
+
+static acpi_status acpi_count_logiciores(struct acpi_resource *res,
+					   void *data)
+{
+	int *res_cnt = data;
+
+	if (acpi_logicio_supported_resource(res) &&
+		!acpi_dev_filter_resource_type(res, IORESOURCE_IO))
+		(*res_cnt)++;
+
+	return AE_OK;
+}
+
+static acpi_status acpi_read_one_logiciores(struct acpi_resource *res,
+		void *data)
+{
+	struct acpi_resource **resource = data;
+
+	if (acpi_logicio_supported_resource(res) &&
+		!acpi_dev_filter_resource_type(res, IORESOURCE_IO)) {
+		memcpy((*resource), res, sizeof(struct acpi_resource));
+		(*resource)->length = sizeof(struct acpi_resource);
+		(*resource)->type = res->type;
+		(*resource)++;
+	}
+
+	return AE_OK;
+}
+
+static acpi_status
+acpi_build_logiciores_template(struct acpi_device *adev,
+			struct acpi_buffer *buffer)
+{
+	acpi_handle handle = adev->handle;
+	struct acpi_resource *resource;
+	acpi_status status;
+	int res_cnt = 0;
+
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+				     acpi_count_logiciores, &res_cnt);
+	if (ACPI_FAILURE(status) || !res_cnt) {
+		dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status);
+		return -EINVAL;
+	}
+
+	buffer->length = sizeof(struct acpi_resource) * (res_cnt + 1) + 1;
+	buffer->pointer = kzalloc(buffer->length - 1, GFP_KERNEL);
+	if (!buffer->pointer)
+		return -ENOMEM;
+
+	resource = (struct acpi_resource *)buffer->pointer;
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+				     acpi_read_one_logiciores, &resource);
+	if (ACPI_FAILURE(status)) {
+		kfree(buffer->pointer);
+		dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status);
+		return -EINVAL;
+	}
+
+	resource->type = ACPI_RESOURCE_TYPE_END_TAG;
+	resource->length = sizeof(struct acpi_resource);
+
+	return 0;
+}
+
+static int acpi_translate_logiciores(struct acpi_device *adev,
+		struct acpi_device *host, struct acpi_buffer *buffer)
+{
+	int res_cnt = (buffer->length - 1) / sizeof(struct acpi_resource) - 1;
+	struct acpi_resource *resource = buffer->pointer;
+	struct acpi_resource_address64 addr;
+	unsigned long sys_port;
+	struct device *dev = &adev->dev;
+
+	/* only one I/O resource now */
+	if (res_cnt != 1) {
+		dev_err(dev, "encode %d resources whose type is(%d)!\n",
+			res_cnt, resource->type);
+		return -EINVAL;
+	}
+
+	if (ACPI_FAILURE(acpi_resource_to_address64(resource, &addr))) {
+		dev_err(dev, "convert acpi resource(%d) as addr64 FAIL!\n",
+			resource->type);
+		return -EFAULT;
+	}
+
+	/* For indirect-IO, addr length must be fixed. (>0, 0/1, 0/1)(0,0,0) */
+	if (addr.min_address_fixed != addr.max_address_fixed) {
+		dev_warn(dev, "variable I/O resource is invalid!\n");
+		return -EINVAL;
+	}
+
+	dev_info(dev, "CRS IO: len=0x%llx [0x%llx - 0x%llx]\n",
+			addr.address.address_length, addr.address.minimum,
+			addr.address.maximum);
+	sys_port = logic_pio_trans_hwaddr(&host->fwnode, addr.address.minimum);
+	if (sys_port == -1) {
+		dev_err(dev, "translate bus-addr(0x%llx) fail!\n",
+			addr.address.minimum);
+		return -EFAULT;
+	}
+
+	switch (resource->type) {
+	case ACPI_RESOURCE_TYPE_ADDRESS32:
+	{
+		struct acpi_resource_address32 *out_res;
+
+		out_res = &resource->data.address32;
+		if (!addr.address.address_length)
+			addr.address.address_length = out_res->address.maximum -
+				out_res->address.minimum + 1;
+		out_res->address.minimum = sys_port;
+		out_res->address.maximum = sys_port +
+				addr.address.address_length - 1;
+		out_res->address.address_length = addr.address.address_length;
+
+		dev_info(dev, "_SRS 32IO: [0x%x - 0x%x] len = 0x%x\n",
+			out_res->address.minimum,
+			out_res->address.maximum,
+			out_res->address.address_length);
+
+		break;
+	}
+
+	case ACPI_RESOURCE_TYPE_ADDRESS64:
+	{
+		struct acpi_resource_address64 *out_res;
+
+		out_res = &resource->data.address64;
+		if (!addr.address.address_length)
+			addr.address.address_length = out_res->address.maximum -
+				out_res->address.minimum + 1;
+		out_res->address.minimum = sys_port;
+		out_res->address.maximum = sys_port +
+				addr.address.address_length - 1;
+		out_res->address.address_length = addr.address.address_length;
+
+		dev_info(dev, "_SRS 64IO: [0x%llx - 0x%llx] len = 0x%llx\n",
+			out_res->address.minimum,
+			out_res->address.maximum,
+			out_res->address.address_length);
+
+		break;
+	}
+
+	default:
+		return -EINVAL;
+
+	}
+
+	return 0;
+}
+
+/*
+ * update/set the current I/O resource of the designated device node.
+ * after this calling, the enumeration can be started as the I/O resource
+ * had been translated to logicial I/O from bus-local I/O.
+ *
+ * @adev: the device node to be updated the I/O resource;
+ * @host: the device node where 'adev' is attached, which can be not
+ *	the parent of 'adev';
+ *
+ * return 0 when successful, negative is for failure.
+ */
+static int acpi_set_logicio_resource(struct device *child,
+		struct device *hostdev)
+{
+	struct acpi_device *adev;
+	struct acpi_device *host;
+	struct acpi_buffer buffer;
+	acpi_status status;
+	int ret;
+
+	if (!child || !hostdev)
+		return -EINVAL;
+
+	host = to_acpi_device(hostdev);
+	adev = to_acpi_device(child);
+
+	/* check the device state */
+	if (!adev->status.present) {
+		dev_info(child, "ACPI: device is not present!\n");
+		return 0;
+	}
+	/* whether the child had been enumerated? */
+	if (acpi_device_enumerated(adev)) {
+		dev_info(child, "ACPI: had been enumerated!\n");
+		return 0;
+	}
+
+	/* read the _CRS and convert as acpi_buffer */
+	status = acpi_build_logiciores_template(adev, &buffer);
+	if (ACPI_FAILURE(status)) {
+		dev_warn(child, "Failure evaluating %s\n", METHOD_NAME__CRS);
+		return -ENODEV;
+	}
+
+	/* translate the I/O resources */
+	ret = acpi_translate_logiciores(adev, host, &buffer);
+	if (ret) {
+		kfree(buffer.pointer);
+		dev_err(child, "Translate I/O range FAIL!\n");
+		return ret;
+	}
+
+	/* set current resource... */
+	status = acpi_set_current_resources(adev->handle, &buffer);
+	kfree(buffer.pointer);
+	if (ACPI_FAILURE(status)) {
+		dev_err(child, "Error evaluating _SRS (0x%x)\n", status);
+		ret = -EIO;
+	}
+
+	return ret;
+}
+
+static int lpc_host_io_setup(struct acpi_device *adev, void *pdata)
+{
+	struct logic_pio_hwaddr *range, *tmprange;
+	struct lpc_private_data *lpc_private;
+	struct acpi_device *child;
+
+	lpc_private = (struct lpc_private_data *)pdata;
+	range = kzalloc(sizeof(*range), GFP_KERNEL);
+	if (!range)
+		return -ENOMEM;
+	range->fwnode = &adev->fwnode;
+	range->flags = PIO_INDIRECT;
+	range->size = lpc_private->io_size;
+	range->hw_start = lpc_private->io_start;
+
+	tmprange = logic_pio_register_range(range, 1);
+	if (tmprange != range) {
+		kfree(range);
+		if (IS_ERR(tmprange))
+			return -EFAULT;
+	}
+
+	/* For hisilpc, only care about the sons of host. */
+	list_for_each_entry(child, &adev->children, node) {
+		int ret;
+
+		ret = acpi_set_logicio_resource(&child->dev, &adev->dev);
+		if (ret) {
+			dev_err(&child->dev, "set resource failed..\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static const struct indirectio_device_desc lpc_host_desc = {
+	.pdata = &lpc_data,
+	.pre_setup = lpc_host_io_setup,
+};
+
+/* All the host devices which apply indirect-IO can be listed here. */
+static const struct acpi_device_id acpi_indirect_host_id[] = {
+	{"HISI0191", INDIRECT_IO_INFO(lpc_host_desc)},
+	{""},
+};
+
+static int acpi_indirectio_attach(struct acpi_device *adev,
+				const struct acpi_device_id *id)
+{
+	struct indirectio_device_desc *hostdata;
+	struct platform_device *pdev;
+	int ret;
+
+	hostdata = (struct indirectio_device_desc *)id->driver_data;
+	if (!hostdata || !hostdata->pre_setup)
+		return -EINVAL;
+
+	ret = hostdata->pre_setup(adev, hostdata->pdata);
+	if (!ret) {
+		pdev = acpi_create_platform_device(adev, NULL);
+		if (IS_ERR_OR_NULL(pdev)) {
+			dev_err(&adev->dev, "Create platform device for host FAIL!\n");
+			return -EFAULT;
+		}
+		acpi_device_set_enumerated(adev);
+		ret = 1;
+	}
+
+	return ret;
+}
+
+
+static struct acpi_scan_handler acpi_indirect_handler = {
+	.ids = acpi_indirect_host_id,
+	.attach = acpi_indirectio_attach,
+};
+
+void __init acpi_indirectio_scan_init(void)
+{
+	acpi_scan_add_handler(&acpi_indirect_handler);
+}
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index f159001..b11412d 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -31,6 +31,11 @@ 
 void acpi_platform_init(void);
 void acpi_pnp_init(void);
 void acpi_int340x_thermal_init(void);
+#ifdef CONFIG_INDIRECT_PIO
+void acpi_indirectio_scan_init(void);
+#else
+static inline void acpi_indirectio_scan_init(void) {}
+#endif
 #ifdef CONFIG_ARM_AMBA
 void acpi_amba_init(void);
 #else
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1926918..eda79ce 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2033,6 +2033,7 @@  int __init acpi_scan_init(void)
 	acpi_int340x_thermal_init();
 	acpi_amba_init();
 	acpi_watchdog_init();
+	acpi_indirectio_scan_init();
 
 	acpi_scan_add_handler(&generic_device_handler);