Re: [Xen-devel] [PATCH 4/7] xen/arm: SMMU: Detect types of device tree binding

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

> 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.Chen@xxxxxxx>
> ---
>  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?

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
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?


>  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

> +      */
> +     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.


> +     } 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@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel