Re: [PATCH v3 02/14] xen/asm-generic: introduce generic device.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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