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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxxx>
> ---
> 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