| Message ID | 1431893048-5214-19-git-send-email-parth.dixit@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 21 May 2015 at 16:49, Julien Grall <julien.grall@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.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. > > 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 >
On 24 May 2015 at 13:10, Julien Grall <julien.grall@citrix.com> wrote: > Hi Parth, > > > On 24/05/2015 08:06, Parth Dixit wrote: >> >> > +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. > > > I don't remember a such agreement. So far, only compatible and init are DT > specific. The rest (most of the fields) are device agnostic. Adding attachment of the previous discussion > If you prefix the DT callback by dt_ (or smth else), there would be > confusion and a smaller code. > > Anyway, I will let Ian and Stefano gives their opinion on it. > > Regards, > > -- > Julien Grall
On 25 May 2015 at 15:30, Julien Grall <julien.grall@citrix.com> wrote: > Hi Parth, > > On 25/05/2015 07:58, Parth Dixit wrote: >> >> On 24 May 2015 at 13:10, Julien Grall <julien.grall@citrix.com> wrote: >>> >>> On 24/05/2015 08:06, Parth Dixit wrote: >>>> >>>> >>>> > +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. >>> >>> >>> >>> I don't remember a such agreement. So far, only compatible and init are >>> DT >>> specific. The rest (most of the fields) are device agnostic. >> >> Adding attachment of the previous discussion > > > Thanks. Please a give link to the conversation (such as a mail archive) > rather than an attachment. I had to look on the archive to find the context > of this conversation... ah, sorry about that, i keep forgetting that this conversation is also available in public list and i can provide a link to it. > Also, that something useful to add in the notes of the patch (after ---). > > Regards, > > -- > Julien Grall
+shannon On 25 May 2015 at 17:08, Parth Dixit <parth.dixit@linaro.org> wrote: > On 25 May 2015 at 15:30, Julien Grall <julien.grall@citrix.com> wrote: >> Hi Parth, >> >> On 25/05/2015 07:58, Parth Dixit wrote: >>> >>> On 24 May 2015 at 13:10, Julien Grall <julien.grall@citrix.com> wrote: >>>> >>>> On 24/05/2015 08:06, Parth Dixit wrote: >>>>> >>>>> >>>>> > +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. >>>> >>>> >>>> >>>> I don't remember a such agreement. So far, only compatible and init are >>>> DT >>>> specific. The rest (most of the fields) are device agnostic. >>> >>> Adding attachment of the previous discussion >> >> >> Thanks. Please a give link to the conversation (such as a mail archive) >> rather than an attachment. I had to look on the archive to find the context >> of this conversation... > ah, sorry about that, i keep forgetting that this conversation is also > available in public list and i can provide a link to it. >> Also, that something useful to add in the notes of the patch (after ---). >> >> Regards, >> >> -- >> Julien Grall
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) +{ + 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); +}; + +/** + * acpi_device_init - Initialize a device + * @class: class of the device (serial, network...) + * @data: specific data for initializing the device + * + * Return 0 on success. + */ +int __init acpi_device_init(enum device_class class, + const void *data, int class_type); + /** * device_init - Initialize a device * @dev: device to initialize @@ -78,6 +99,15 @@ __attribute__((__section__(".dev.info"))) = { \ #define DT_DEVICE_END \ }; +#define ACPI_DEVICE_START(_name, _namestr, _class) \ +static const struct acpi_device_desc __dev_desc_##_name __used \ +__attribute__((__section__(".adev.info"))) = { \ + .name = _namestr, \ + .class = _class, \ + +#define ACPI_DEVICE_END \ +}; + #endif /* __ASM_ARM_DEVICE_H */ /*
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(+)