| Message ID | 65b267137539704df7f22b37e3b0a9b372a82b33.1700221559.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce generic headers | expand |
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
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,
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
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: + */
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