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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-11-28 at 15:28 -0600, Shawn Anastasio wrote:
> Hi Oleksii,
> 
> On 11/27/23 8:13 AM, Oleksii Kurochko wrote:
> > diff --git a/xen/arch/ppc/include/asm/Makefile
> > b/xen/arch/ppc/include/asm/Makefile
> > index ece7fa66dd..df4c1ebb08 100644
> > --- a/xen/arch/ppc/include/asm/Makefile
> > +++ b/xen/arch/ppc/include/asm/Makefile
> > @@ -1,3 +1,4 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > +generic-y += device.h
> >  generic-y += paging.h
> >  generic-y += vm_event.h
> > diff --git a/xen/arch/ppc/include/asm/device.h
> > b/xen/arch/ppc/include/asm/device.h
> > deleted file mode 100644
> > index 8253e61d51..0000000000
> > --- a/xen/arch/ppc/include/asm/device.h
> > +++ /dev/null
> > @@ -1,53 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-only */
> > -#ifndef __ASM_PPC_DEVICE_H__
> > -#define __ASM_PPC_DEVICE_H__
> > -
> > -enum device_type
> > -{
> > -    DEV_DT,
> > -    DEV_PCI,
> > -};
> > -
> > -struct device {
> > -    enum device_type type;
> > -#ifdef CONFIG_HAS_DEVICE_TREE
> > -    struct dt_device_node *of_node; /* Used by drivers imported
> > from Linux */
> > -#endif
> > -};
> > -
> > -enum device_class
> > -{
> > -    DEVICE_SERIAL,
> > -    DEVICE_IOMMU,
> > -    DEVICE_PCI_HOSTBRIDGE,
> > -    /* 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);
> > -};
> > -
> > -typedef struct device device_t;
> > -
> > -#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                                               \
> > -};
> > -
> > -#endif /* __ASM_PPC_DEVICE_H__ */
> > diff --git a/xen/arch/ppc/include/asm/irq.h
> > b/xen/arch/ppc/include/asm/irq.h
> > index 5c37d0cf25..49193fddff 100644
> > --- a/xen/arch/ppc/include/asm/irq.h
> > +++ b/xen/arch/ppc/include/asm/irq.h
> > @@ -3,7 +3,9 @@
> >  #define __ASM_PPC_IRQ_H__
> >  
> >  #include <xen/lib.h>
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> 
> I realize that you were likely following PPC's device.h which also
> checks CONFIG_HAS_DEVICE_TREE, but I'm not sure that it makes sense
> to
> check this conditional in PPC code at all -- we will always have
> HAS_DEVICE_TREE (selected by PPC) and I can't imagine a scenario
> where
> this will ever not be the case.
What about case if ACPI is used? Does ACPI is supported by PPC?

But if you are sure that CONFIG_HAS_DEVICE_TREE will be always selected
then I am OK to remove this change.

> 
> Unless Jan (or someone else) disagrees, I'd rather this conditional
> be
> dropped inside of PPC code.
I'll double check but I think I had a compilation issue if it isn't
defined.

> 
> >  #include <xen/device_tree.h>
> > +#endif
> >  #include <public/device_tree_defs.h>
> >  
> >  /* TODO */
> > @@ -25,9 +27,11 @@ static inline void arch_move_irqs(struct vcpu
> > *v)
> >      BUG_ON("unimplemented");
> >  }
> >  
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> 
> Ditto.
> 
> >  static inline int platform_get_irq(const struct dt_device_node
> > *device, int index)
> >  {
> >      BUG_ON("unimplemented");
> >  }
> > +#endif
> >  
> >  #endif /* __ASM_PPC_IRQ_H__ */

Thanks.

~ Oleksii