> -----Original Message----- > From: Stefano Stabellini [mailto:sstabell...@kernel.org] > Sent: 2017年7月6日 2:08 > To: Wei Chen <wei.c...@arm.com> > Cc: Stefano Stabellini <sstabell...@kernel.org>; Kaly Xin <kaly....@arm.com>; > Julien Grall <julien.gr...@arm.com>; Steve Capper <steve.cap...@arm.com>; nd > <n...@arm.com>; xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH 4/7] xen/arm: SMMU: Detect types of device > tree binding > > On Tue, 4 Jul 2017, Wei Chen wrote: > > Hi Stefano, > > > > > -----Original Message----- > > > From: Stefano Stabellini [mailto:sstabell...@kernel.org] > > > Sent: 2017年7月4日 6:30 > > > To: Wei Chen <wei.c...@arm.com> > > > Cc: xen-devel@lists.xen.org; sstabell...@kernel.org; Steve Capper > > > <steve.cap...@arm.com>; Kaly Xin <kaly....@arm.com>; Julien Grall > > > <julien.gr...@arm.com>; nd <n...@arm.com> > > > 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 <wei.c...@arm.com> > > > > --- > > > > 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. >
Yes. So let keep these two variables without __initdata here. > > > > 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. > Ok. > > > > > > > > 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. > Ok, I will add this warning message. > > > > > > > > + } 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 > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel