Re: [PATCH v2 18/41] arm: Introduce a generic way to use a device from acpi

[Julien Grall <julien.grall@citrix.com>]

Hi Parth,

On 17/05/15 21:03, Parth Dixit wrote:
> add generic way to use device from acpi similar to
> the way it is supported in device tree.
> 
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> ---
>  xen/arch/arm/device.c        | 19 +++++++++++++++++++
>  xen/arch/arm/xen.lds.S       |  7 +++++++
>  xen/include/asm-arm/device.h | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> index 0b53f6a..5494de0 100644
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -22,6 +22,7 @@
>  #include <xen/lib.h>
>  
>  extern const struct device_desc _sdevice[], _edevice[];
> +extern const struct acpi_device_desc _asdevice[], _aedevice[];
>  
>  int __init device_init(struct dt_device_node *dev, enum device_class class,
>                         const void *data)
> @@ -50,6 +51,24 @@ int __init device_init(struct dt_device_node *dev, enum device_class class,
>      return -EBADF;
>  }
>  
> +int __init acpi_device_init(enum device_class class, const void *data, int class_type)

Please explain what means class_type and how this will fit with every
ACPI device tables.

AFAICT, it does only works for SPCR table used for UART device. For the
GIC you've hardcoded the value and I can't find any version number in
the table.

You may need to introduce another way to find the device such as a
callback taking the table in parameter.

> +{
> +    const struct acpi_device_desc *desc;
> +
> +    for ( desc = _asdevice; desc != _aedevice; desc++ )
> +    {
> +        if ( ( desc->class != class ) && ( desc->class_type != class_type ) )
> +            continue;
> +
> +
> +        ASSERT(desc->init != NULL);
> +
> +        return desc->init(data);
> +    }
> +
> +    return -EBADF;
> +}
> +
>  enum device_class device_get_class(const struct dt_device_node *dev)
>  {
>      const struct device_desc *desc;
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 0488f37..60802f6 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -100,6 +100,13 @@ SECTIONS
>        _edevice = .;
>    } :text
>  
> +  . = ALIGN(8);
> +  .adev.info : {
> +      _asdevice = .;
> +      *(.adev.info)
> +      _aedevice = .;
> +  } :text
> +
>    . = ALIGN(PAGE_SIZE);             /* Init code and data */
>    __init_begin = .;
>    .init.text : {
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index a72f7c9..09fcfc3 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -50,6 +50,27 @@ struct device_desc {
>      int (*init)(struct dt_device_node *dev, const void *data);
>  };
>  
> +struct acpi_device_desc {
> +    /* Device name */
> +    const char *name;
> +    /* Device class */
> +    enum device_class class;
> +    /* type of device supported by the driver */
> +    const int class_type;
> +    /* Device initialization */
> +    int (*init)(const void *data);
> +};

Given that the number of device will be minimal in Xen, I would prefer
to merge this structure into device_desc by adding the ACPI fields.

It would avoid to duplicate everything for only 2 fields changes.

>From the drivers point of view it would look like

DEVICE_START(....)
	.dt_init = ...
#ifdef CONFIG_ACPI
	.acpi_init = ...
#endif
DEVICE_END

Or something like

DEVICE_START(...)
	DT_INIT(...)
	ACPI_INIT(...)
DEVICE_END

And ACPI_INIT will be a no-op when CONFIG_ACPI is not enabled.

Regards,

-- 
Julien Grall