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 <[email protected]>
> > ---
> > 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
