On 21 May 2015 at 16:49, Julien Grall <julien.gr...@citrix.com> wrote:
> 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.di...@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.
>
> I think we agreed not to use common structure as it had some dt specific
entries and there was scope of confusion.
> Regards,
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
