Re: [Xen-devel] [PATCH 6/7] drivers/passthrough/arm: Refactor code for arm smmu drivers

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 09, 2018 at 10:51:01AM +0000, Julien Grall wrote:
> Hi,
> 
> On 02/09/2018 10:43 AM, Roger Pau Monné wrote:
> > > +    unsigned int type;
> > > +};
> > > +
> > > +#define resource_size(res) ((res)->size)
> > > +
> > > +#define platform_device device
> > > +
> > > +#define IORESOURCE_MEM 0
> > > +#define IORESOURCE_IRQ 1
> > > +
> > > +/* Stub out DMA domain related functions */
> > > +#define iommu_get_dma_cookie(dom) 0
> > > +#define iommu_put_dma_cookie(dom)
> > > +
> > > +#define VA_BITS          0 /* Only used for configuring stage-1 input 
> > > size */
> > > +
> > > +#define MODULE_DEVICE_TABLE(type, name)
> > > +#define module_param_named(name, value, type, perm)
> > > +#define MODULE_PARM_DESC(_parm, desc)
> > > +
> > > +#define dma_set_mask_and_coherent(d, b)  0
> > > +#define of_dma_is_coherent(n)    0
> > > +
> > > +static void __iomem *devm_ioremap_resource(struct device *dev,
> > > +                                    struct resource *res)
> > 
> > Aligment, please use spaces.
> > 
> > Also, is __iomem needed here at all?
> 
> On Arm, we tend to add keep __iomem on pointer dealing with MMIO.

I understand that you keep it when directly importing code from Linux,
but this is Xen code, so unless this is done merely for consistency it
seems quite pointless (__iomem is defined to nothing AFAICT).

> 
> [...]
> 
> > > +
> > > +#endif /* __ARM_SMMU_H__ */
> > > +
> > > diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> > > b/xen/drivers/passthrough/arm/smmu-v3.c
> > > index f43485fe6e..f0a61521fb 100644
> > > --- a/xen/drivers/passthrough/arm/smmu-v3.c
> > > +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> > > @@ -49,28 +49,7 @@
> > >   #include <asm/io.h>
> > >   #include <asm/platform.h>
> > > -/* Alias to Xen device tree helpers */
> > > -#define device_node dt_device_node
> > > -#define of_phandle_args dt_phandle_args
> > > -#define of_device_id dt_device_match
> > > -#define of_match_node dt_match_node
> > > -#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, 
> > > pname, out))
> > > -#define of_property_read_bool dt_property_read_bool
> > > -#define of_parse_phandle_with_args dt_parse_phandle_with_args
> > > -
> > > -/* Xen: Helpers to get device MMIO and IRQs */
> > > -struct resource {
> > > - u64 addr;
> > > - u64 size;
> > > - unsigned int type;
> > > -};
> > > -
> > > -#define resource_size(res) ((res)->size)
> > > -
> > > -#define platform_device device
> > > -
> > > -#define IORESOURCE_MEM 0
> > > -#define IORESOURCE_IRQ 1
> > 
> > You introduce the above code in patch 5, and remove it in patch 6, is
> > this really needed?
> > 
> > Ie: why not simply introduce this code directly in this patch?
> 
> See https://lists.xen.org/archives/html/xen-devel/2018-01/msg02066.html

Hm, OK, I'm not sure I follow that.

AFAICT the above code is added in patch 5 so that the driver can be
hooked up into the build system. Could we just hold off hooking the
driver to the build system until patch 6, in order to avoid such
addition and removal of code?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel