[adding Rob, Mark, Arnd, Thierry and Hiroshi]
On Wed, Aug 13, 2014 at 01:51:37AM +0100, Mitchel Humpherys wrote:
> Generic IOMMU device tree bindings were recently added in
> ["devicetree: Add generic IOMMU device tree bindings"]. Implement the
> bindings in the ARM SMMU driver.
>
> See Documentation/devicetree/bindings/iommu/iommu.txt for the bindings
> themselves.
Could you look at moving the parsing code into a separate file please (maybe
under lib/ ?). That way, other IOMMUs can use the same binding without the
boilerplate having to be rewritten each time.
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 63c6707fad..22e25f3172 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -538,25 +538,32 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
> return 0;
> }
>
> +struct iommus_entry {
> + struct list_head list;
> + struct device_node *node;
> + u16 streamids[MAX_MASTER_STREAMIDS];
> + int num_sids;
> +};
> +
> static int register_smmu_master(struct arm_smmu_device *smmu,
> - struct device *dev,
> - struct of_phandle_args *masterspec)
> + struct iommus_entry *entry)
> {
> int i;
> struct arm_smmu_master *master;
> + struct device *dev = smmu->dev;
>
> - master = find_smmu_master(smmu, masterspec->np);
> + master = find_smmu_master(smmu, entry->node);
> if (master) {
> dev_err(dev,
> "rejecting multiple registrations for master device %s\n",
> - masterspec->np->name);
> + entry->node->name);
> return -EBUSY;
> }
>
> - if (masterspec->args_count > MAX_MASTER_STREAMIDS) {
> + if (entry->num_sids > MAX_MASTER_STREAMIDS) {
> dev_err(dev,
> "reached maximum number (%d) of stream IDs for master device %s\n",
> - MAX_MASTER_STREAMIDS, masterspec->np->name);
> + MAX_MASTER_STREAMIDS, entry->node->name);
> return -ENOSPC;
> }
>
> @@ -564,15 +571,58 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
> if (!master)
> return -ENOMEM;
>
> - master->of_node = masterspec->np;
> - master->cfg.num_streamids = masterspec->args_count;
> + master->of_node = entry->node;
> + master->cfg.num_streamids = entry->num_sids;
>
> for (i = 0; i < master->cfg.num_streamids; ++i)
> - master->cfg.streamids[i] = masterspec->args[i];
> + master->cfg.streamids[i] = entry->streamids[i];
>
> return insert_smmu_master(smmu, master);
> }
>
> +static int arm_smmu_parse_iommus_properties(struct arm_smmu_device *smmu,
> + int *num_masters)
> +{
> + struct of_phandle_args iommuspec;
> + struct device_node *dn;
> +
> + for_each_node_with_property(dn, "iommus") {
> + int arg_ind = 0;
> + struct iommus_entry *entry, *n;
> + LIST_HEAD(iommus);
> +
> + while (!of_parse_phandle_with_args(dn, "iommus", "#iommu-cells",
> + arg_ind, &iommuspec)) {
We need to check that the phandle does indeed point at one of our SMMUs
here, in case we have a system with multiple IOMMU types, all using the
generic binding.
> + int i;
> +
> + list_for_each_entry(entry, &iommus, list)
> + if (entry->node == dn)
> + break;
Oh, yuck, this is really nasty to parse...
> + if (&entry->list == &iommus) {
Where is entry initialised the first time around?
> + entry = devm_kzalloc(smmu->dev, sizeof(*entry),
> + GFP_KERNEL);
> + if (!entry)
> + return -ENOMEM;
You need to of_node_put the guys you've got back from the phandle parsing
code.
> + entry->node = dn;
> + list_add(&entry->list, &iommus);
> + }
> + entry->num_sids = iommuspec.args_count;
> + for (i = 0; i < entry->num_sids; ++i)
> + entry->streamids[i] = iommuspec.args[i];
> + arg_ind++;
Isn't this defined by #iommu-cells?
> + }
> +
> + list_for_each_entry_safe(entry, n, &iommus, list) {
Why the _safe variant? This is a local list, right?
Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
