On Tue, 4 Jul 2017, Wei Chen wrote: > Hi Stefano, > > > -----Original Message----- > > From: Stefano Stabellini [mailto:sstabellini@kernel.org] > > Sent: 2017年7月4日 6:30 > > To: Wei Chen > > Cc: xen-devel@lists.xen.org; sstabellini@kernel.org; Steve Capper > > ; Kaly Xin ; Julien Grall > > ; nd > > Subject: Re: [Xen-devel] [PATCH 4/7] xen/arm: SMMU: Detect types of device > > tree binding > > > > On Fri, 30 Jun 2017, Wei Chen wrote: > > > The device tree provides two types of IOMMU bindings, one is legacy > > > another is generic. The legacy bindings will be depercated in favour > > ^ deprecated > > > > > of the generic bindings. But in the transitional period, we have to > > > support both of them. > > > > > > The codes to handle these two types of bindings are very differnet, > > ^ code ^ different > > > > Please use a spellchecker. > > Thanks, I will fix them. > > > > > > so we have to detect the binding types while doing SMMU probing. > > > > > > This detect code is based on Linux ARM SMMUv2 driver: > > > https://github.com/torvalds/linux/blob/master/drivers/iommu/arm-smmu.c > > > > > > Signed-off-by: Wei Chen > > > --- > > > xen/drivers/passthrough/arm/smmu.c | 23 +++++++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/xen/drivers/passthrough/arm/smmu.c > > b/xen/drivers/passthrough/arm/smmu.c > > > index 2efa52d..441c296 100644 > > > --- a/xen/drivers/passthrough/arm/smmu.c > > > +++ b/xen/drivers/passthrough/arm/smmu.c > > > @@ -143,6 +143,8 @@ typedef enum irqreturn irqreturn_t; > > > > > > #define dev_name(dev) dt_node_full_name(dev_to_dt(dev)) > > > > > > +#define pr_notice(fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__) > > > + > > > /* Alias to Xen allocation helpers */ > > > #define kfree xfree > > > #define kmalloc(size, flags) _xmalloc(size, sizeof(void *)) > > > @@ -681,6 +683,8 @@ struct arm_smmu_option_prop { > > > const char *prop; > > > }; > > > > > > +static bool using_legacy_binding, using_generic_binding; > > > > __initdata? > > > > I think these two variables are not only used in initialization. They also > have been used in ops->add_device. Althrough the add_device is only be > invoked while construct_dom0. I don't think that add_device is supposed to be limited at boot. It's best to avoid __initdata then. > > But why do these two variables need to be static? Can't they just be > > local variables in arm_smmu_device_dt_probe? > > > > Is it to enforce that all smmus on a given platform are either using the > > legacy or the generic bindings, but not a mix of the two? If so, it > > should be clearly written. Also, I am not sure we should really be > > Yes, this checking will enforce all smmus are using the same bindings. > > > checking for that. It seems to me that one smmu could be using generic > > bindings and another could be using legacy bindings. Is it specified > > anywhere that it cannot be the case? > > > > In theory, different SMMUs can use different bindings. About this concern, > I have discussed with Robin Murphy and Julien. We have three reasons: > > The first is that, we ported this checking from Linux, we are trying to keep > the code very close to the Linux driver. To ease backporting changes. > > The second is that, we think it is a good change to try to phase out the > legacy binding and request to use generic bindings everywhere if you > start to use in one SMMU. > > The other less technical reason for not supporting both at once is that anyone > who can update their DT to add or update SMMUs with the new binding has no good > excuse for not updating the whole lot. It's the likes of Seattle and ThunderX > boxes with firmware that won't get updated for which we have to preserve "mmu-masters" > support. I would like these reasons to be written in the commit message. I would also like to detect and print a clear warning when SMMUs are using mismatched bindings. > > > > > static struct arm_smmu_option_prop arm_smmu_options[] = { > > > { ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" }, > > > { 0, NULL}, > > > @@ -2289,6 +2293,25 @@ static int arm_smmu_device_dt_probe(struct > > platform_device *pdev) > > > struct rb_node *node; > > > struct of_phandle_args masterspec; > > > int num_irqs, i, err; > > > + bool legacy_binding; > > > + > > > + /* > > > + * Xen: Do the same check as Linux. Checking the SMMU device tree > > bindings > > ^ do ^ Check that > > > > > > > + * are either using generic or legacy one. > > ^ bindings > > > > > + * > > > + * The "mmu-masters" property is only existed in legacy bindings. > > ^ only exists in the legacy bindings > > > > Thanks, I will fix above typos. > > > > + */ > > > + legacy_binding = dt_find_property(dev->of_node, "mmu-masters", NULL); > > > + if (legacy_binding && !using_generic_binding) { > > > + if (!using_legacy_binding) > > > + pr_notice("deprecated \"mmu-masters\" DT property in use\n"); > > > + using_legacy_binding = true; > > > + } else if (!legacy_binding && !using_legacy_binding) { > > > + using_generic_binding = true; > > > > Please simplify this series of if/else. > > > > This code is the same as Linux SMMU driver. If we agree on enforcing all smmus > are using the same binding, I prefer to keep the code the same. Is it?! Wow... All right then, but I would still like a warning to be printed when we find out that an SMMU is using legacy bindings and others are using generic bindings. > > > > > + } else { > > > + dev_err(dev, "not probing due to mismatched DT properties\n"); > > > + return -ENODEV; > > > + } > > > > > > > > > > > smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL); > > > if (!smmu) { > > > -- > > > 2.7.4 > > > > > > > > > _______________________________________________ > > > Xen-devel mailing list > > > Xen-devel@lists.xen.org > > > https://lists.xen.org/xen-devel > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel >