Arm, PPC and RISC-V use the same device.h thereby device.h
was moved to asm-generic. Arm's device.h was taken as a base with
the following changes:
 - #ifdef PCI related things.
 - #ifdef ACPI related things.
 - Rename #ifdef guards.
 - Add SPDX tag.
 - #ifdef CONFIG_HAS_DEVICE_TREE related things.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
It is still open question if device.h should be in asm-generic. Need more opinions.
---
Changes in V3:
 - ifdef device tree related things.
 - update the commit message
---
Changes in V2:
	- take ( as common ) device.h from Arm as PPC and RISC-V use it as a base.
	- #ifdef PCI related things.
	- #ifdef ACPI related things.
	- rename DEVICE_GIC to DEVIC_IC.
	- rename #ifdef guards.
	- switch Arm and PPC to generic device.h
	- add SPDX tag
	- update the commit message

---
 xen/include/asm-generic/device.h | 147 +++++++++++++++++++++++++++++++
 1 file changed, 147 insertions(+)
 create mode 100644 xen/include/asm-generic/device.h

diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-generic/device.h
new file mode 100644
index 0000000000..7ef5aa955a
--- /dev/null
+++ b/xen/include/asm-generic/device.h
@@ -0,0 +1,147 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ASM_GENERIC_DEVICE_H__
+#define __ASM_GENERIC_DEVICE_H__
+
+enum device_type
+{
+#ifdef CONFIG_HAS_DEVICE_TREE
+    DEV_DT,
+#endif
+
+#ifdef HAS_PCI
+    DEV_PCI,
+#endif
+};
+
+struct dev_archdata {
+    void *iommu;    /* IOMMU private data */
+};
+
+/* struct device - The basic device structure */
+struct device
+{
+    enum device_type type;
+#ifdef CONFIG_HAS_DEVICE_TREE
+    struct dt_device_node *of_node; /* Used by drivers imported from Linux */
+#endif
+    struct dev_archdata archdata;
+    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
+};
+
+typedef struct device device_t;
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+#include <xen/device_tree.h>
+#endif
+
+#ifdef HAS_PCI
+#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
+#endif
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
+#endif
+
+enum device_class
+{
+    DEVICE_SERIAL,
+    DEVICE_IOMMU,
+    DEVICE_IC,
+#ifdef HAS_PCI
+    DEVICE_PCI_HOSTBRIDGE,
+#endif
+    /* Use for error */
+    DEVICE_UNKNOWN,
+};
+
+struct device_desc {
+    /* Device name */
+    const char *name;
+    /* Device class */
+    enum device_class class;
+    /* List of devices supported by this driver */
+    const struct dt_device_match *dt_match;
+    /*
+     * Device initialization.
+     *
+     * -EAGAIN is used to indicate that device probing is deferred.
+     */
+    int (*init)(struct dt_device_node *dev, const void *data);
+};
+
+#ifdef CONFIG_ACPI
+
+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 acpi_device_init(enum device_class class,
+                     const void *data, int class_type);
+
+#endif /* CONFIG_ACPI */
+
+/**
+ *  device_init - Initialize a device
+ *  @dev: device to initialize
+ *  @class: class of the device (serial, network...)
+ *  @data: specific data for initializing the device
+ *
+ *  Return 0 on success.
+ */
+int device_init(struct dt_device_node *dev, enum device_class class,
+                const void *data);
+
+/**
+ * device_get_type - Get the type of the device
+ * @dev: device to match
+ *
+ * Return the device type on success or DEVICE_ANY on failure
+ */
+enum device_class device_get_class(const struct dt_device_node *dev);
+
+#define DT_DEVICE_START(_name, _namestr, _class)                    \
+static const struct device_desc __dev_desc_##_name __used           \
+__section(".dev.info") = {                                          \
+    .name = _namestr,                                               \
+    .class = _class,                                                \
+
+#define DT_DEVICE_END                                               \
+};
+
+#ifdef CONFIG_ACPI
+
+#define ACPI_DEVICE_START(_name, _namestr, _class)                    \
+static const struct acpi_device_desc __dev_desc_##_name __used           \
+__section(".adev.info") = {                       \
+    .name = _namestr,                                               \
+    .class = _class,                                                \
+
+#define ACPI_DEVICE_END                                               \
+};
+
+#endif /* CONFIG_ACPI */
+
+#endif /* __ASM_GENERIC_DEVICE_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.41.0

Hi Oleksii,

On 17/11/2023 12:24, Oleksii Kurochko wrote:
> Arm, PPC and RISC-V use the same device.h thereby device.h
> was moved to asm-generic. 

I read "was moved" as the patch should also contain some deleted lines. 
But below, I only see the file introduced. Did you intend to also remove 
the version in arm/include/asm?

> Arm's device.h was taken as a base with
> the following changes:
>   - #ifdef PCI related things.
>   - #ifdef ACPI related things.
>   - Rename #ifdef guards.
>   - Add SPDX tag.
>   - #ifdef CONFIG_HAS_DEVICE_TREE related things.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> It is still open question if device.h should be in asm-generic. Need more opinions.

I still think it should. But I guess you want others to answer? If so it 
would be worth to point out from who you seek opinions.

> ---
> Changes in V3:
>   - ifdef device tree related things.
>   - update the commit message
> ---
> Changes in V2:
> 	- take ( as common ) device.h from Arm as PPC and RISC-V use it as a base.
> 	- #ifdef PCI related things.
> 	- #ifdef ACPI related things.
> 	- rename DEVICE_GIC to DEVIC_IC.
> 	- rename #ifdef guards.
> 	- switch Arm and PPC to generic device.h
> 	- add SPDX tag
> 	- update the commit message
> 
> ---
>   xen/include/asm-generic/device.h | 147 +++++++++++++++++++++++++++++++
>   1 file changed, 147 insertions(+)
>   create mode 100644 xen/include/asm-generic/device.h
> 
> diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-generic/device.h
> new file mode 100644
> index 0000000000..7ef5aa955a
> --- /dev/null
> +++ b/xen/include/asm-generic/device.h
> @@ -0,0 +1,147 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __ASM_GENERIC_DEVICE_H__
> +#define __ASM_GENERIC_DEVICE_H__
> +
> +enum device_type
> +{
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +    DEV_DT,
> +#endif
> +
> +#ifdef HAS_PCI
> +    DEV_PCI,
> +#endif
> +};
> +
> +struct dev_archdata {
> +    void *iommu;    /* IOMMU private data */
> +};
> +
> +/* struct device - The basic device structure */
> +struct device
> +{
> +    enum device_type type;
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +    struct dt_device_node *of_node; /* Used by drivers imported from Linux */
> +#endif
> +    struct dev_archdata archdata;
> +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
> +};
> +
> +typedef struct device device_t;
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +#include <xen/device_tree.h>
> +#endif

NIT: Could we try to rationalize the number of #ifdef CONFIG_HAS_*? For 
example ...

> +
> +#ifdef HAS_PCI
> +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> +#endif
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> +#endif

... this is another definition for Device-Tree only. It could be easily 
moved above the definitnion of dev_is_pci(). The others would be the 
DT_DEVICE_*() helpers.

> +
> +enum device_class
> +{
> +    DEVICE_SERIAL,
> +    DEVICE_IOMMU,
> +    DEVICE_IC,

I guess you mean INTERRUPT_CONTROLLER? If so, can this be spelt out? (I 
don't think shorthand version is worth it here)

Cheers,

-- 
Julien Grall

On Fri, 2023-11-24 at 11:01 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 17/11/2023 12:24, Oleksii Kurochko wrote:
> > Arm, PPC and RISC-V use the same device.h thereby device.h
> > was moved to asm-generic. 
> 
> I read "was moved" as the patch should also contain some deleted
> lines. 
> But below, I only see the file introduced. Did you intend to also
> remove 
> the version in arm/include/asm?
Yes, I just messed up with the patch version. Here is the version of
the patch which remove Arm's device.h:
https://gitlab.com/xen-project/people/olkur/xen/-/commit/ce845abfb57d27b0c08984f5433085c767550495
> 
> > Arm's device.h was taken as a base with
> > the following changes:
> >   - #ifdef PCI related things.
> >   - #ifdef ACPI related things.
> >   - Rename #ifdef guards.
> >   - Add SPDX tag.
> >   - #ifdef CONFIG_HAS_DEVICE_TREE related things.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > It is still open question if device.h should be in asm-generic.
> > Need more opinions.
> 
> I still think it should. But I guess you want others to answer? If so
> it 
> would be worth to point out from who you seek opinions.
> 
> > ---
> > Changes in V3:
> >   - ifdef device tree related things.
> >   - update the commit message
> > ---
> > Changes in V2:
> > 	- take ( as common ) device.h from Arm as PPC and RISC-V
> > use it as a base.
> > 	- #ifdef PCI related things.
> > 	- #ifdef ACPI related things.
> > 	- rename DEVICE_GIC to DEVIC_IC.
> > 	- rename #ifdef guards.
> > 	- switch Arm and PPC to generic device.h
> > 	- add SPDX tag
> > 	- update the commit message
> > 
> > ---
> >   xen/include/asm-generic/device.h | 147
> > +++++++++++++++++++++++++++++++
> >   1 file changed, 147 insertions(+)
> >   create mode 100644 xen/include/asm-generic/device.h
> > 
> > diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-
> > generic/device.h
> > new file mode 100644
> > index 0000000000..7ef5aa955a
> > --- /dev/null
> > +++ b/xen/include/asm-generic/device.h
> > @@ -0,0 +1,147 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_DEVICE_H__
> > +#define __ASM_GENERIC_DEVICE_H__
> > +
> > +enum device_type
> > +{
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    DEV_DT,
> > +#endif
> > +
> > +#ifdef HAS_PCI
> > +    DEV_PCI,
> > +#endif
> > +};
> > +
> > +struct dev_archdata {
> > +    void *iommu;    /* IOMMU private data */
> > +};
> > +
> > +/* struct device - The basic device structure */
> > +struct device
> > +{
> > +    enum device_type type;
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    struct dt_device_node *of_node; /* Used by drivers imported
> > from Linux */
> > +#endif
> > +    struct dev_archdata archdata;
> > +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU
> > instance data */
> > +};
> > +
> > +typedef struct device device_t;
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +#include <xen/device_tree.h>
> > +#endif
> 
> NIT: Could we try to rationalize the number of #ifdef CONFIG_HAS_*?
> For 
> example ...
> 
> > +
> > +#ifdef HAS_PCI
> > +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> > +#endif
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> > +#endif
> 
> ... this is another definition for Device-Tree only. It could be
> easily 
> moved above the definitnion of dev_is_pci(). The others would be the 
> DT_DEVICE_*() helpers.
Yes, sure. I can combine CONFIG_HAS_DEVICE_TREE related things better.

> 
> > +
> > +enum device_class
> > +{
> > +    DEVICE_SERIAL,
> > +    DEVICE_IOMMU,
> > +    DEVICE_IC,
> 
> I guess you mean INTERRUPT_CONTROLLER? If so, can this be spelt out?
> (I 
> don't think shorthand version is worth it here)
Sure, I'll change to INTERRUPT_CONTROLLER.

~ Oleksii

On 17.11.2023 13:24, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/include/asm-generic/device.h
> @@ -0,0 +1,147 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __ASM_GENERIC_DEVICE_H__
> +#define __ASM_GENERIC_DEVICE_H__
> +
> +enum device_type
> +{
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +    DEV_DT,
> +#endif
> +
> +#ifdef HAS_PCI

CONFIG_HAS_PCI?

> +    DEV_PCI,
> +#endif
> +};
> +
> +struct dev_archdata {
> +    void *iommu;    /* IOMMU private data */

#ifdef CONFIG_HAS_PASSTHROUGH around this field?

> +};
> +
> +/* struct device - The basic device structure */
> +struct device
> +{
> +    enum device_type type;
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +    struct dt_device_node *of_node; /* Used by drivers imported from Linux */
> +#endif
> +    struct dev_archdata archdata;
> +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */

Same here then?

> +};
> +
> +typedef struct device device_t;
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +#include <xen/device_tree.h>
> +#endif
> +
> +#ifdef HAS_PCI
> +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> +#endif
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> +#endif
> +
> +enum device_class
> +{
> +    DEVICE_SERIAL,
> +    DEVICE_IOMMU,
> +    DEVICE_IC,

What is IC here? (And thus: Is this generic enough to live here?)

> +#ifdef HAS_PCI
> +    DEVICE_PCI_HOSTBRIDGE,
> +#endif
> +    /* Use for error */
> +    DEVICE_UNKNOWN,
> +};
> +
> +struct device_desc {
> +    /* Device name */
> +    const char *name;
> +    /* Device class */
> +    enum device_class class;
> +    /* List of devices supported by this driver */
> +    const struct dt_device_match *dt_match;

This and ...

> +    /*
> +     * Device initialization.
> +     *
> +     * -EAGAIN is used to indicate that device probing is deferred.
> +     */
> +    int (*init)(struct dt_device_node *dev, const void *data);

... this look to be DT-specific.

> +};
> +
> +#ifdef CONFIG_ACPI
> +
> +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 acpi_device_init(enum device_class class,
> +                     const void *data, int class_type);
> +
> +#endif /* CONFIG_ACPI */
> +
> +/**
> + *  device_init - Initialize a device
> + *  @dev: device to initialize
> + *  @class: class of the device (serial, network...)
> + *  @data: specific data for initializing the device
> + *
> + *  Return 0 on success.
> + */
> +int device_init(struct dt_device_node *dev, enum device_class class,
> +                const void *data);

As is this, simply from its first parameter's type.

> +/**
> + * device_get_type - Get the type of the device
> + * @dev: device to match
> + *
> + * Return the device type on success or DEVICE_ANY on failure
> + */
> +enum device_class device_get_class(const struct dt_device_node *dev);
> +
> +#define DT_DEVICE_START(_name, _namestr, _class)                    \
> +static const struct device_desc __dev_desc_##_name __used           \
> +__section(".dev.info") = {                                          \
> +    .name = _namestr,                                               \
> +    .class = _class,                                                \
> +
> +#define DT_DEVICE_END                                               \
> +};

And all of these.

Jan

On Tue, 2023-11-21 at 16:43 +0100, Jan Beulich wrote:
> On 17.11.2023 13:24, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/device.h
> > @@ -0,0 +1,147 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_DEVICE_H__
> > +#define __ASM_GENERIC_DEVICE_H__
> > +
> > +enum device_type
> > +{
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    DEV_DT,
> > +#endif
> > +
> > +#ifdef HAS_PCI
> 
> CONFIG_HAS_PCI?
Should be CONFIG_HAS_PCI. Thanks.

> 
> > +    DEV_PCI,
> > +#endif
> > +};
> > +
> > +struct dev_archdata {
> > +    void *iommu;    /* IOMMU private data */
> 
> #ifdef CONFIG_HAS_PASSTHROUGH around this field?
It makes sense to #ifdef iommu and iommu_fwspec fields. I'll add it.

> 
> > +};
> > +
> > +/* struct device - The basic device structure */
> > +struct device
> > +{
> > +    enum device_type type;
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    struct dt_device_node *of_node; /* Used by drivers imported
> > from Linux */
> > +#endif
> > +    struct dev_archdata archdata;
> > +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU
> > instance data */
> 
> Same here then?
> 
> > +};
> > +
> > +typedef struct device device_t;
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +#include <xen/device_tree.h>
> > +#endif
> > +
> > +#ifdef HAS_PCI
> > +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> > +#endif
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> > +#endif
> > +
> > +enum device_class
> > +{
> > +    DEVICE_SERIAL,
> > +    DEVICE_IOMMU,
> > +    DEVICE_IC,
> 
> What is IC here? (And thus: Is this generic enough to live here?)
It is Interrupt Controller. I think yes, it should live here and is
expected to use by Arm, RISC-V and PPC.

> 
> > +#ifdef HAS_PCI
> > +    DEVICE_PCI_HOSTBRIDGE,
> > +#endif
> > +    /* Use for error */
> > +    DEVICE_UNKNOWN,
> > +};
> > +
> > +struct device_desc {
> > +    /* Device name */
> > +    const char *name;
> > +    /* Device class */
> > +    enum device_class class;
> > +    /* List of devices supported by this driver */
> > +    const struct dt_device_match *dt_match;
> 
> This and ...
> 
> > +    /*
> > +     * Device initialization.
> > +     *
> > +     * -EAGAIN is used to indicate that device probing is
> > deferred.
> > +     */
> > +    int (*init)(struct dt_device_node *dev, const void *data);
> 
> ... this look to be DT-specific.
They are. I'll ifdef it.

> 
> > +};
> > +
> > +#ifdef CONFIG_ACPI
> > +
> > +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 acpi_device_init(enum device_class class,
> > +                     const void *data, int class_type);
> > +
> > +#endif /* CONFIG_ACPI */
> > +
> > +/**
> > + *  device_init - Initialize a device
> > + *  @dev: device to initialize
> > + *  @class: class of the device (serial, network...)
> > + *  @data: specific data for initializing the device
> > + *
> > + *  Return 0 on success.
> > + */
> > +int device_init(struct dt_device_node *dev, enum device_class
> > class,
> > +                const void *data);
> 
> As is this, simply from its first parameter's type.
I missed to take changes related to ifdef-ing DT related things to this
version of device.h. So I'll update that.

> 
> > +/**
> > + * device_get_type - Get the type of the device
> > + * @dev: device to match
> > + *
> > + * Return the device type on success or DEVICE_ANY on failure
> > + */
> > +enum device_class device_get_class(const struct dt_device_node
> > *dev);
> > +
> > +#define DT_DEVICE_START(_name, _namestr,
> > _class)                    \
> > +static const struct device_desc __dev_desc_##_name
> > __used           \
> > +__section(".dev.info") =
> > {                                          \
> > +    .name =
> > _namestr,                                               \
> > +    .class =
> > _class,                                                \
> > +
> > +#define
> > DT_DEVICE_END                                               \
> > +};
> 
> And all of these.

Probably it also make sense to swtich Arm and PPC to asm-generic
device.h file.

~ Oleksii