Re: [PATCH 01/11] iommu/arm-smmu: Introduce driver option handling

[Andreas Herrmann <herrmann.der.user-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>]

On Wed, Jan 22, 2014 at 11:51:43AM +0000, Will Deacon wrote:
> Hi Andreas,
> 
> Couple of *tiny* comments.
> 
> On Thu, Jan 16, 2014 at 12:44:13PM +0000, Andreas Herrmann wrote:
> > Introduce handling of driver options. Options are set based on DT
> > information when probing an SMMU device. The first option introduced
> > is "arm,smmu-isolate-devices". (It will be used in the bus notifier
> > block.)
> > 
> > Cc: Andreas Herrmann <herrmann.der.user-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> > Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  drivers/iommu/arm-smmu.c |   29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index e46a887..0b97d03 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -47,6 +47,9 @@
> >  
> >  #include <asm/pgalloc.h>
> >  
> > +/* Driver options */
> > +#define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
> 
> You can move this...
> 
> >  /* Maximum number of stream IDs assigned to a single device */
> >  #define MAX_MASTER_STREAMIDS		8
> >  
> > @@ -348,6 +351,7 @@ struct arm_smmu_device {
> >  #define ARM_SMMU_FEAT_TRANS_S2		(1 << 3)
> >  #define ARM_SMMU_FEAT_TRANS_NESTED	(1 << 4)
> >  	u32				features;
> 
> ... to here. Like the *_FEAT_* defines above.

Ok.

> > +	u32				options;
> >  	int				version;
> >  
> >  	u32				num_context_banks;
> > @@ -398,6 +402,29 @@ struct arm_smmu_domain {
> >  static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> >  static LIST_HEAD(arm_smmu_devices);
> >  
> > +struct arm_smmu_option_prop {
> > +	u32 opt;
> > +	const char *prop;
> > +};
> > +
> > +static struct arm_smmu_option_prop arm_smmu_options [] = {
> > +	{ ARM_SMMU_OPT_ISOLATE_DEVICES, "arm,smmu-isolate-devices" },
> > +	{ 0, NULL},
> > +};
> > +
> > +static void check_driver_options(struct arm_smmu_device *smmu)
> > +{
> > +	int i = 0;
> > +	do {
> > +		if (of_property_read_bool(smmu->dev->of_node,
> > +						arm_smmu_options[i].prop)) {
> > +			smmu->options |= arm_smmu_options[i].opt;
> > +			dev_dbg(smmu->dev, "option %s\n",
> > +				arm_smmu_options[i].prop);
> > +		}
> > +	} while (arm_smmu_options[++i].opt);
> > +}
> > +
> >  static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
> >  						struct device_node *dev_node)
> >  {
> > @@ -1783,6 +1810,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
> >  	}
> >  	smmu->dev = dev;
> >  
> > +	check_driver_options(smmu);
> 
> I think parse_driver_opts is a better name. Also, if we called this after
> arm_smmu_device_cfg_probe, we could replace the dev_dbg with a dev_notice,
> since the user probably wants to know which options ended up getting
> enabled.

Yes, that's right.
(I was undecided on adding additional output because the properties
can be checked under /proc/device-tree on a running system.)

> Is there a reason you need to probe the option so early?

There is no real requirement. Just the general consideration to be
done with option parsing as early as possible. (And I think once I had
some preliminary code in register_smmu_master that depended on it.)

I'll move the call.


Andreas