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 <[email protected]>
> > Signed-off-by: Andreas Herrmann <[email protected]>
> > ---
> > 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
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
