From: Hiroshi Doyu <hdoyu@xxxxxxxxxx>
Subject: Re: [PATCH v2 2/2] [RFC] ARM: IOMMU: Tegra30: iommu_ops for SMMU driver
Date: Sat, 17 Dec 2011 03:03:15 +0200 (EET)
Message-ID: <20111217.030315.2218721757650628823.hdoyu@xxxxxxxxxx>
> Hi Joerg, Thank you for your quick review.
>
> From: Joerg Roedel <joerg.roedel@xxxxxxx>
> Subject: Re: [PATCH v2 2/2] [RFC] ARM: IOMMU: Tegra30: iommu_ops for SMMU driver
> Date: Fri, 16 Dec 2011 16:39:04 +0100
> Message-ID: <20111216153904.GC29877@xxxxxxx>
>
> > On Thu, Dec 15, 2011 at 03:11:30PM +0200, Hiroshi DOYU wrote:
> > > +static int smmu_iommu_attach_dev(struct iommu_domain *domain,
> > > + struct device *dev)
> > > +{
> > > + struct smmu_as *as = domain->priv;
> > > + struct smmu_client *client, *c;
> > > + u32 map;
> > > + int err;
> > > +
> > > + client = kmalloc(sizeof(*c), GFP_KERNEL);
> > > + if (!client)
> > > + return -ENOMEM;
> > > + client->dev = dev;
> > > + client->as = as;
> > > + map = (unsigned long)dev->platform_data;
> > > + if (!map)
> > > + return -EINVAL;
> > > +
> > > + err = smmu_client_enable_hwgrp(client, map);
> > > + if (err)
> > > + goto err_hwgrp;
> > > +
> > > + spin_lock(&as->client_lock);
> > > + list_for_each_entry(c, &as->client, list) {
> > > + if (c->dev == dev) {
> > > + pr_err("%s is already attached\n", dev_name(dev));
> > > + err = -EINVAL;
> > > + goto err_client;
> > > + }
> > > + }
> > > + list_add(&client->list, &as->client);
> > > + spin_unlock(&as->client_lock);
> > > +
> > > + /*
> > > + * Reserve "page zero" for AVP vectors using a common dummy
> > > + * page.
> > > + */
> > > + if (map & HWG_AVPC) {
> > > + struct page *page;
> > > +
> > > + page = as->smmu->avp_vector_page;
> > > + __smmu_iommu_map_pfn(as, 0, page_to_pfn(page));
> > > +
> > > + pr_info("Reserve \"page zero\" for AVP vectors using a common dummy\n");
> > > + }
> > > +
> > > + pr_debug("Attached %s\n", dev_name(dev));
> > > + return 0;
> > > +err_client:
> > > + smmu_client_disable_hwgrp(client);
> > > + spin_unlock(&as->client_lock);
> > > +err_hwgrp:
> > > + kfree(client);
> > > + return err;
> > > +}
> >
> > Hmm, I have a question about that. Reading the code it looks like your
> > SMMU exists per pheripheral device
>
> A single SMMU is shared with multiple peripheral devices, a single
> SMMU has multiple ASIDs. ASID is used per group of peripheral
> devices. These peripheral groups can be configured by the above
> "hwgrp"/"map" passed via platform_data. The above "struct device"
> represents a groupd of peripheral devices, a kind of virtual device.
>
> The above "struct device" ~= a group of peripheral devices.
>
> > and the SMMU hardware supports multiple address spaces per device,
> > right?
>
> Yes, where "device" is a gropud of peripheral devices.
>
> > The domains are implemented for one address-space.
>
> Yes.
>
> > So is it right that a device can have multiple
> > address-spaces?
>
> No, at least, the following code prevents if a peripheral is already
> assigned ASID.
>
> smmu_client_enable_hwgrp():
Here, smmu_client_enable_hwgrp() == __smmu_client_set_hwgrp(),
+static int __smmu_client_set_hwgrp(struct smmu_client *c,
+ unsigned long map, int on)
+{
+ int i;
+ struct smmu_as *as = c->as;
+ u32 val, offs, mask = SMMU_ASID_ENABLE(as->asid);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+ struct smmu_device *smmu = as->smmu;
+
+ WARN_ON(!on && map);
+ if (on && !map)
+ return -EINVAL;
+ if (!on)
+ map = smmu_client_hwgrp(c);
+
+ for_each_set_bit(i, &map, BITS_PER_LONG) {
> + for_each_set_bit(i, &map, BITS_PER_LONG) {
> + offs = HWGRP_ASID_REG(i);
> + val = smmu_read(smmu, offs);
> + if (on) {
> + if (WARN_ON(val & mask))
> + goto err_hw_busy;
^^^^^^^^^^^^^^^^^^^^^^^^^
This checks if peripheral device is already enable(and ASID
assigned). If so, it returns error. A group of peripheral devices are
configured into struct device in advance.
>
>
> > If so, what kind of devices do you bind to the domains
> > then. I doesn't make sense to bind whole peripheral devices in this
> > case.
>
> Here, the above "struct device" ~= a group of any peripheral
> devices. Those groups are configurable.
>
>
> In my simple DMA API test(not posted), the above hwgrp/map is configured as below:
>
> 146 static int __init dmaapi_test_init(void)
> 147 {
> 148 int i;
> 149 struct dma_iommu_mapping *map;
> 150
> 151 map = arm_iommu_create_mapping(IOVA_START, IOVA_SIZE, 0);
> 152 BUG_ON(!map);
> 153 pr_debug("Allocate IOVA: %08x-%08x\n", map->base, map->base + IOVA_SIZE);
> 154
> 155 for (i = 0; i < ARRAY_SIZE(dmaapi_dummy_device); i++) {
> 156 int err;
> 157 struct platform_device *pdev = &dmaapi_dummy_device[i];
> 158
> 159 pdev->dev.platform_data = (void *)dummy_hwgrp_map[i];
> 160 err = platform_device_register(pdev);
> 161 BUG_ON(err);
> 162
> 163 err = arm_iommu_attach_device(&pdev->dev, map);
> 164 BUG_ON(err);
> 165 pr_debug("IOMMU API: Attached to %s\n", dev_name(&pdev->dev));
> 166 }
>
> So peripheral devices can be divided into multiple groups, and each
> each group represents struct device with hwgrp/map info in
> platform_data.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
