Arm, PPC and RISC-V introduce the same things in asm/device.h, so
generic device.h was introduced.
Arm's device.h was taken as a base with the following changes:
- #ifdef ACPI related things.
- Rename #ifdef guards.
- Add SPDX tag.
- #ifdef CONFIG_HAS_DEVICE_TREE related things.
- #ifdef-ing iommu related things with CONFIG_HAS_PASSTHROUGH.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V8:
- drop #ifdef HAS_PCI around DEV_PCI in enum device_type to be consistent with DEVICE_PCI_HOSTBRIDGE
- drop the comment above enum device_type, after DEV_PCI is alaways present in enum device_type, so
there is no need anymore in comment and DEV_TYPE_MAX.
- drop #ifdef HAS_PCI around dev_is_pci() as DEV_PCI is alwayds define now.
- rename args _name, _namestr, _class to name, namestr, class.
- drop unnessary backslash after .class = class in defines ACPI_DEVICE_START() and DT_DEVICE_START()
- update the commit message
---
Changes in V7:
- keeping DEVICE_PCI_HOSTBRIDGE available for every build based on the reply:
https://lore.kernel.org/xen-devel/926a5c12-7f02-42ec-92a8-1c82d060c710@xen.org/
- add comment above enum device_type.h with explanation about DEV_TYPE_MAX.
- separate patch "[PATCH v6 9/9] xen/asm-generic: introduce generic device.h" into 3 patches.
---
Changes in V6:
- Rebase only.
---
Changes in V5:
- Removed generated file: xen/include/headers++.chk.new
- Removed pointless #ifdef CONFIG_HAS_DEVICE_TREE ... #endif for PPC as
CONFIG_HAS_DEVICE_TREE will be always used for PPC.
---
Changes in V4:
- Updated the commit message
- Switched Arm and PPC to asm-generic version of device.h
- Replaced HAS_PCI with CONFIG_HAS_PCI
- ifdef-ing iommu filed of dev_archdata struct with CONFIG_HAS_PASSTHROUGH
- ifdef-ing iommu_fwspec of device struct with CONFIG_HAS_PASSTHROUGH
- ifdef-ing DT related things with CONFIG_HAS_DEVICE_TREE
- Updated the commit message ( remove a note with question about
if device.h should be in asm-generic or not )
- Replaced DEVICE_IC with DEVICE_INTERRUPT_CONTROLLER
- Rationalized usage of CONFIG_HAS_* in device.h
- Fixed indents for ACPI_DEVICE_START and ACPI_DEVICE_END
---
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 | 149 +++++++++++++++++++++++++++++++
1 file changed, 149 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..6e56658271
--- /dev/null
+++ b/xen/include/asm-generic/device.h
@@ -0,0 +1,149 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ASM_GENERIC_DEVICE_H__
+#define __ASM_GENERIC_DEVICE_H__
+
+#include <xen/stdbool.h>
+
+enum device_type
+{
+#ifdef CONFIG_HAS_DEVICE_TREE
+ DEV_DT,
+#endif
+ DEV_PCI
+};
+
+enum device_class
+{
+ DEVICE_SERIAL,
+ DEVICE_IOMMU,
+ DEVICE_INTERRUPT_CONTROLLER,
+ DEVICE_PCI_HOSTBRIDGE,
+ /* Use for error */
+ DEVICE_UNKNOWN,
+};
+
+struct dev_archdata {
+#ifdef CONFIG_HAS_PASSTHROUGH
+ void *iommu; /* IOMMU private data */
+#endif
+};
+
+/* 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;
+#ifdef CONFIG_HAS_PASSTHROUGH
+ struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
+#endif
+};
+
+typedef struct device device_t;
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+
+#include <xen/device_tree.h>
+
+#define dev_is_dt(dev) ((dev)->type == DEV_DT)
+
+/**
+ * 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 \
+};
+
+#else /* !CONFIG_HAS_DEVICE_TREE */
+#define dev_is_dt(dev) ((void)(dev), false)
+#endif /* CONFIG_HAS_DEVICE_TREE */
+
+#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
+
+struct device_desc {
+ /* Device name */
+ const char *name;
+ /* Device class */
+ enum device_class class;
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+
+ /* 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);
+
+#endif
+};
+
+#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);
+
+#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.43.0
Hi Oleksii, On 09/02/2024 18:00, Oleksii Kurochko wrote: > diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-generic/device.h > new file mode 100644 > index 0000000000..6e56658271 > --- /dev/null > +++ b/xen/include/asm-generic/device.h > @@ -0,0 +1,149 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +#ifndef __ASM_GENERIC_DEVICE_H__ > +#define __ASM_GENERIC_DEVICE_H__ > + > +#include <xen/stdbool.h> > + > +enum device_type > +{ > +#ifdef CONFIG_HAS_DEVICE_TREE > + DEV_DT, > +#endif > + DEV_PCI > +}; > + > +enum device_class > +{ > + DEVICE_SERIAL, > + DEVICE_IOMMU, > + DEVICE_INTERRUPT_CONTROLLER, > + DEVICE_PCI_HOSTBRIDGE, > + /* Use for error */ > + DEVICE_UNKNOWN, > +}; > + > +struct dev_archdata { > +#ifdef CONFIG_HAS_PASSTHROUGH > + void *iommu; /* IOMMU private data */ > +#endif > +}; It is a bit too late to change, but I thought I would point it if someone wants to send a follow-up. It is a bit odd to have a structure dev_archdata that, if I am not mistaken, is only used ... > + > +/* 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; ... in struct device. Looking at the use, I believe this was only introduced to try to keep that SMMU code close to Linux. I would consider to fold the other structure and update dev_archdata() in drivers/passthrough/arm/smmu.c. > +#ifdef CONFIG_HAS_PASSTHROUGH > + struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */ > +#endif > +}; > + > +typedef struct device device_t; > + > +#ifdef CONFIG_HAS_DEVICE_TREE > + > +#include <xen/device_tree.h> > + > +#define dev_is_dt(dev) ((dev)->type == DEV_DT) > + > +/** > + * 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 \ > +}; > + > +#else /* !CONFIG_HAS_DEVICE_TREE */ > +#define dev_is_dt(dev) ((void)(dev), false) > +#endif /* CONFIG_HAS_DEVICE_TREE */ > + > +#define dev_is_pci(dev) ((dev)->type == DEV_PCI) > + > +struct device_desc { > + /* Device name */ > + const char *name; > + /* Device class */ > + enum device_class class; > + > +#ifdef CONFIG_HAS_DEVICE_TREE > + > + /* 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); > + > +#endif > +}; I am not sure I fully understand why "device_desc" is not protected by CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the config is disabled. Can you clarify? Cheers, -- Julien Grall
On Tue, 2024-02-13 at 18:09 +0000, Julien Grall wrote: > Hi Oleksii, > > On 09/02/2024 18:00, Oleksii Kurochko wrote: > > diff --git a/xen/include/asm-generic/device.h b/xen/include/asm- > > generic/device.h > > new file mode 100644 > > index 0000000000..6e56658271 > > --- /dev/null > > +++ b/xen/include/asm-generic/device.h > > @@ -0,0 +1,149 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +#ifndef __ASM_GENERIC_DEVICE_H__ > > +#define __ASM_GENERIC_DEVICE_H__ > > + > > +#include <xen/stdbool.h> > > + > > +enum device_type > > +{ > > +#ifdef CONFIG_HAS_DEVICE_TREE > > + DEV_DT, > > +#endif > > + DEV_PCI > > +}; > > + > > +enum device_class > > +{ > > + DEVICE_SERIAL, > > + DEVICE_IOMMU, > > + DEVICE_INTERRUPT_CONTROLLER, > > + DEVICE_PCI_HOSTBRIDGE, > > + /* Use for error */ > > + DEVICE_UNKNOWN, > > +}; > > + > > +struct dev_archdata { > > +#ifdef CONFIG_HAS_PASSTHROUGH > > + void *iommu; /* IOMMU private data */ > > +#endif > +}; > > It is a bit too late to change, but I thought I would point it if > someone wants to send a follow-up. It is a bit odd to have a > structure > dev_archdata that, if I am not mistaken, is only used ... > > > + > > +/* 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; > > ... in struct device. Looking at the use, I believe this was only > introduced to try to keep that SMMU code close to Linux. I would > consider to fold the other structure and update dev_archdata() in > drivers/passthrough/arm/smmu.c. I can do that in separate patch. struct dev_archdata was left because of drivers/passthrough/arm/smmu.c. > > > +#ifdef CONFIG_HAS_PASSTHROUGH > > + struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU > > instance data */ > > +#endif > > +}; > > + > > +typedef struct device device_t; > > + > > +#ifdef CONFIG_HAS_DEVICE_TREE > > + > > +#include <xen/device_tree.h> > > + > > +#define dev_is_dt(dev) ((dev)->type == DEV_DT) > > + > > +/** > > + * 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 \ > > +}; > > + > > +#else /* !CONFIG_HAS_DEVICE_TREE */ > > +#define dev_is_dt(dev) ((void)(dev), false) > > +#endif /* CONFIG_HAS_DEVICE_TREE */ > > + > > +#define dev_is_pci(dev) ((dev)->type == DEV_PCI) > > + > > +struct device_desc { > > + /* Device name */ > > + const char *name; > > + /* Device class */ > > + enum device_class class; > > + > > +#ifdef CONFIG_HAS_DEVICE_TREE > > + > > + /* 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); > > + > > +#endif > > +}; > I am not sure I fully understand why "device_desc" is not protected > by > CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the > config > is disabled. Can you clarify? I thought that one day struct device_desc and acpi_device_desc will be "merged", and so decided just to #ifdef only DEVICE_TREE specific fields. Another one reason it is if to protect fully struct device_desc then it would be needed more #ifdef in arm/device.c ( for example, device_init() should be all protected then ) what will require to ifdef all calls of device_init(). As an option device_init can can be defined in case when !CONFIG_HAS_DEVICE_TREE as: int __init device_init(struct dt_device_node *dev, enum device_class class, const void *data) { return -EBADF; } The similar thing will be needed for device_get_class() in Arm's device.c. Would it be better to ifdef full struct device_desc ? ~ Oleksii
Hi Oleksii, On 14/02/2024 09:32, Oleksii wrote: > On Tue, 2024-02-13 at 18:09 +0000, Julien Grall wrote: >>> +#ifdef CONFIG_HAS_PASSTHROUGH >>> + struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU >>> instance data */ >>> +#endif >>> +}; >>> + >>> +typedef struct device device_t; >>> + >>> +#ifdef CONFIG_HAS_DEVICE_TREE >>> + >>> +#include <xen/device_tree.h> >>> + >>> +#define dev_is_dt(dev) ((dev)->type == DEV_DT) >>> + >>> +/** >>> + * 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 \ >>> +}; >>> + >>> +#else /* !CONFIG_HAS_DEVICE_TREE */ >>> +#define dev_is_dt(dev) ((void)(dev), false) >>> +#endif /* CONFIG_HAS_DEVICE_TREE */ >>> + >>> +#define dev_is_pci(dev) ((dev)->type == DEV_PCI) >>> + >>> +struct device_desc { >>> + /* Device name */ >>> + const char *name; >>> + /* Device class */ >>> + enum device_class class; >>> + >>> +#ifdef CONFIG_HAS_DEVICE_TREE >>> + >>> + /* 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); >>> + >>> +#endif >>> +}; >> I am not sure I fully understand why "device_desc" is not protected >> by >> CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the >> config >> is disabled. Can you clarify? > I thought that one day struct device_desc and acpi_device_desc will be > "merged", and so decided just to #ifdef only DEVICE_TREE specific > fields. It might be possible to merge the two if we were using an union for the ACPI/DT specific part. However the majority of the parsing code needs to differ. So I am not convinced there would be any value to merge the two structures. > Another one reason it is if to protect fully struct device_desc then it > would be needed more #ifdef in arm/device.c ( for example, > device_init() should be all protected then ) what will require to ifdef > all calls of device_init(). As an option device_init can can be defined > in case when !CONFIG_HAS_DEVICE_TREE as: > int __init device_init(struct dt_device_node *dev, enum device_class > class, > const void *data) > { > return -EBADF; > } > > The similar thing will be needed for device_get_class() in Arm's device.c. I agree that in theory device_init() & co should be protected with CONFIG_HAS_DEVICE_TREE. However, it is not possible to compile Xen on Arm without the Device-Tree part today. So I don't view adding the #ifdef or any extra stub as necessary today. This may be useful in the future though. Note this is not a request to modify the patch more than... > > Would it be better to ifdef full struct device_desc ? .. moving structure within the #ifdef. Cheers, -- Julien Grall
> > On 14/02/2024 09:32, Oleksii wrote: > > On Tue, 2024-02-13 at 18:09 +0000, Julien Grall wrote: > > > > +#ifdef CONFIG_HAS_PASSTHROUGH > > > > + struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU > > > > instance data */ > > > > +#endif > > > > +}; > > > > + > > > > +typedef struct device device_t; > > > > + > > > > +#ifdef CONFIG_HAS_DEVICE_TREE > > > > + > > > > +#include <xen/device_tree.h> > > > > + > > > > +#define dev_is_dt(dev) ((dev)->type == DEV_DT) > > > > + > > > > +/** > > > > + * 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 \ > > > > +}; > > > > + > > > > +#else /* !CONFIG_HAS_DEVICE_TREE */ > > > > +#define dev_is_dt(dev) ((void)(dev), false) > > > > +#endif /* CONFIG_HAS_DEVICE_TREE */ > > > > + > > > > +#define dev_is_pci(dev) ((dev)->type == DEV_PCI) > > > > + > > > > +struct device_desc { > > > > + /* Device name */ > > > > + const char *name; > > > > + /* Device class */ > > > > + enum device_class class; > > > > + > > > > +#ifdef CONFIG_HAS_DEVICE_TREE > > > > + > > > > + /* 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); > > > > + > > > > +#endif > > > > +}; > > > I am not sure I fully understand why "device_desc" is not > > > protected > > > by > > > CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the > > > config > > > is disabled. Can you clarify? > > I thought that one day struct device_desc and acpi_device_desc will > > be > > "merged", and so decided just to #ifdef only DEVICE_TREE specific > > fields. > > It might be possible to merge the two if we were using an union for > the > ACPI/DT specific part. However the majority of the parsing code needs > to > differ. So I am not convinced there would be any value to merge the > two > structures. In this case, let's have two separate structures. This is not the current situation, and I don't have a specific example. It appears that all architectures will use Device Tree or ACPI. However, does it make sense to keep 'struct device_desc' more generic to accommodate non-DT or non-ACPI cases? I am okay with making the following change, but I am just curious if what I mentioned above makes sense at all: #ifdef CONFIG_HAS_DEVICE_TREE 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); }; #endif /* CONFIG_HAS_DEVICE_TREE */ > > > Another one reason it is if to protect fully struct device_desc > > then it > > would be needed more #ifdef in arm/device.c ( for example, > > device_init() should be all protected then ) what will require to > > ifdef > > all calls of device_init(). As an option device_init can can be > > defined > > in case when !CONFIG_HAS_DEVICE_TREE as: > > int __init device_init(struct dt_device_node *dev, enum > > device_class > > class, > > const void *data) > > { > > return -EBADF; > > } > > > > The similar thing will be needed for device_get_class() in Arm's > > device.c. > > I agree that in theory device_init() & co should be protected with > CONFIG_HAS_DEVICE_TREE. However, it is not possible to compile Xen on > Arm without the Device-Tree part today. So I don't view adding the > #ifdef or any extra stub as necessary today. > > This may be useful in the future though. Note this is not a request > to > modify the patch more than... > > > > > Would it be better to ifdef full struct device_desc ? > .. moving structure within the #ifdef. Well, I'll update the commit message of the next patch that it is not possible to compile Xen without CONFIG_HAS_DEVICE_TREE, so device_init() and Co won't be protected by CONFIG_HAS_DEVICE_TREE. ~ Oleksii
Hi Oleksii, On 15/02/2024 16:54, Oleksii wrote: >> >> On 14/02/2024 09:32, Oleksii wrote: >>> On Tue, 2024-02-13 at 18:09 +0000, Julien Grall wrote: >>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>> + struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU >>>>> instance data */ >>>>> +#endif >>>>> +}; >>>>> + >>>>> +typedef struct device device_t; >>>>> + >>>>> +#ifdef CONFIG_HAS_DEVICE_TREE >>>>> + >>>>> +#include <xen/device_tree.h> >>>>> + >>>>> +#define dev_is_dt(dev) ((dev)->type == DEV_DT) >>>>> + >>>>> +/** >>>>> + * 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 \ >>>>> +}; >>>>> + >>>>> +#else /* !CONFIG_HAS_DEVICE_TREE */ >>>>> +#define dev_is_dt(dev) ((void)(dev), false) >>>>> +#endif /* CONFIG_HAS_DEVICE_TREE */ >>>>> + >>>>> +#define dev_is_pci(dev) ((dev)->type == DEV_PCI) >>>>> + >>>>> +struct device_desc { >>>>> + /* Device name */ >>>>> + const char *name; >>>>> + /* Device class */ >>>>> + enum device_class class; >>>>> + >>>>> +#ifdef CONFIG_HAS_DEVICE_TREE >>>>> + >>>>> + /* 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); >>>>> + >>>>> +#endif >>>>> +}; >>>> I am not sure I fully understand why "device_desc" is not >>>> protected >>>> by >>>> CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the >>>> config >>>> is disabled. Can you clarify? >>> I thought that one day struct device_desc and acpi_device_desc will >>> be >>> "merged", and so decided just to #ifdef only DEVICE_TREE specific >>> fields. >> >> It might be possible to merge the two if we were using an union for >> the >> ACPI/DT specific part. However the majority of the parsing code needs >> to >> differ. So I am not convinced there would be any value to merge the >> two >> structures. > In this case, let's have two separate structures. > > This is not the current situation, and I don't have a specific example. > It appears that all architectures will use Device Tree or ACPI. > However, does it make sense to keep 'struct device_desc' more generic > to accommodate non-DT or non-ACPI cases? I am not entirely sure what else to say. As I wrote before yes it could be made generic. But right now I don't see any values. If you have any idea how to share the structure. Then feel free to make a proposal and I will review it. Cheers, -- Julien Grall
On 09.02.2024 19:00, Oleksii Kurochko wrote: > --- /dev/null > +++ b/xen/include/asm-generic/device.h > @@ -0,0 +1,149 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +#ifndef __ASM_GENERIC_DEVICE_H__ > +#define __ASM_GENERIC_DEVICE_H__ > + > +#include <xen/stdbool.h> > + > +enum device_type > +{ > +#ifdef CONFIG_HAS_DEVICE_TREE > + DEV_DT, > +#endif > + DEV_PCI > +}; > + > +enum device_class > +{ > + DEVICE_SERIAL, > + DEVICE_IOMMU, > + DEVICE_INTERRUPT_CONTROLLER, > + DEVICE_PCI_HOSTBRIDGE, > + /* Use for error */ > + DEVICE_UNKNOWN, > +}; > + > +struct dev_archdata { > +#ifdef CONFIG_HAS_PASSTHROUGH > + void *iommu; /* IOMMU private data */ > +#endif > +}; > + > +/* 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; > +#ifdef CONFIG_HAS_PASSTHROUGH > + struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */ > +#endif > +}; > + > +typedef struct device device_t; > + > +#ifdef CONFIG_HAS_DEVICE_TREE > + > +#include <xen/device_tree.h> > + > +#define dev_is_dt(dev) ((dev)->type == DEV_DT) > + > +/** > + * 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_) \ I don't think the trailing underscores are needed or helpful here (or in the ACPI counterpart), ... > +static const struct device_desc __dev_desc_##name_ __used \ > +__section(".dev.info") = { \ > + .name = namestr_, \ > + .class = class_, ... seeing this all it would have taken was to avoid the two words "name" and "class" (by e.g. using "ident" and "cls"). Nevertheless: Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On Mon, 2024-02-12 at 15:19 +0100, Jan Beulich wrote: > On 09.02.2024 19:00, Oleksii Kurochko wrote: > > --- /dev/null > > +++ b/xen/include/asm-generic/device.h > > @@ -0,0 +1,149 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +#ifndef __ASM_GENERIC_DEVICE_H__ > > +#define __ASM_GENERIC_DEVICE_H__ > > + > > +#include <xen/stdbool.h> > > + > > +enum device_type > > +{ > > +#ifdef CONFIG_HAS_DEVICE_TREE > > + DEV_DT, > > +#endif > > + DEV_PCI > > +}; > > + > > +enum device_class > > +{ > > + DEVICE_SERIAL, > > + DEVICE_IOMMU, > > + DEVICE_INTERRUPT_CONTROLLER, > > + DEVICE_PCI_HOSTBRIDGE, > > + /* Use for error */ > > + DEVICE_UNKNOWN, > > +}; > > + > > +struct dev_archdata { > > +#ifdef CONFIG_HAS_PASSTHROUGH > > + void *iommu; /* IOMMU private data */ > > +#endif > > +}; > > + > > +/* 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; > > +#ifdef CONFIG_HAS_PASSTHROUGH > > + struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU > > instance data */ > > +#endif > > +}; > > + > > +typedef struct device device_t; > > + > > +#ifdef CONFIG_HAS_DEVICE_TREE > > + > > +#include <xen/device_tree.h> > > + > > +#define dev_is_dt(dev) ((dev)->type == DEV_DT) > > + > > +/** > > + * 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_) \ > > I don't think the trailing underscores are needed or helpful here > (or in the ACPI counterpart), ... > > > +static const struct device_desc __dev_desc_##name_ __used \ > > +__section(".dev.info") = { \ > > + .name = namestr_, \ > > + .class = class_, > > ... seeing this all it would have taken was to avoid the two words > "name" and "class" (by e.g. using "ident" and "cls"). > > Nevertheless: > Acked-by: Jan Beulich <jbeulich@suse.com> Thanks. I'll apply your comments in the next patch version. ~ Oleksii
© 2016 - 2024 Red Hat, Inc.