Re: [PATCH 5/6] dma-mapping: support fsl-mc bus
On 09/03/18 18:29, Nipun Gupta wrote:
-Original Message-
From: Christoph Hellwig [mailto:h...@lst.de]
Sent: Thursday, March 08, 2018 13:11
On Tue, Mar 06, 2018 at 04:41:56AM +, Nipun Gupta wrote:
Sorry for asking a trivial question - looking into dma_configure() I see that
PCI is used in the start and the end of the API.
In the end part pci_put_host_bridge_device() is called.
So are two bus callbacks something like 'dma_config_start' &
'dma_config_end'
will be required where the former one will return "dma_dev"?
I'd just use dma_configure as the callback.
This would be a decent stuff.
Yes, the PCI version should end up looking a lot like the old
pci_dma_configure() used to, before everything got pulled together.
Currently the of_dma_configure and acpi_dma_configure are only used
for PCI anyway, as no one else sets a non-NULL dma dev.
My understanding is that even the platform bus uses the of_dma_configure
and probably acpi_dma_configure too. So platform bus may also need the
callback implemented. Please correct me if my understanding is wrong.
Indeed, platform and AMBA will want an implementation to dispatch
between {of,acpi}_dma_configre() per the current logic. Host1x doesn't
seem to care about ACPI so probably wants its own just calling
of_dma_configure().
I will submit the patch with 'dma_configure' callback implemented for
the busses shortly.
Cheers - I've been trying to find some time to take a look myself, but
having something to review instead would certainly be easier :)
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 5/6] dma-mapping: support fsl-mc bus
> -Original Message- > From: Christoph Hellwig [mailto:h...@lst.de] > Sent: Thursday, March 08, 2018 13:11 > > On Tue, Mar 06, 2018 at 04:41:56AM +, Nipun Gupta wrote: > > Sorry for asking a trivial question - looking into dma_configure() I see > > that > > PCI is used in the start and the end of the API. > > In the end part pci_put_host_bridge_device() is called. > > So are two bus callbacks something like 'dma_config_start' & > 'dma_config_end' > > will be required where the former one will return "dma_dev"? > > I'd just use dma_configure as the callback. This would be a decent stuff. > > Currently the of_dma_configure and acpi_dma_configure are only used > for PCI anyway, as no one else sets a non-NULL dma dev. My understanding is that even the platform bus uses the of_dma_configure and probably acpi_dma_configure too. So platform bus may also need the callback implemented. Please correct me if my understanding is wrong. I will submit the patch with 'dma_configure' callback implemented for the busses shortly. > For fsl-mc I suspect only of_dma_configure is relevanet, so just call that > directly. > If at some point we get enough busses with either OF or ACPI we could > create a helper called from ->dma_configure for that. Totally agree with this. Thanks, Nipun ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 3/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
[ +Lorenzo ] On 09/03/18 04:50, Tomasz Figa wrote: [...] Now we need a way to do the check. Perhaps for the time being it would be enough to just check for the power-domains property in DT? AFAICS, it might be as simple as arm_smmu_probe() doing this: /* * We want to avoid touching dev->power.lock in fastpaths unless * it's really going to do something useful - pm_runtime_enabled() * can serve as an ideal proxy for that decision. */ if (dev->pm_domain) pm_runtime_enable(dev); or maybe even just gate all the calls with "if (smmu->dev.pm_domain)" directly (like pcie-mediatek does), but I'm not sure which would be conceptually cleaner. Okay, that was easier than I expected. Thanks. :) Actually, there is one more thing that might need rechecking. Are you sure that dev->pm_domain is NULL for the devices, for which we don't want runtime PM to be enabled? I think ACPI was mentioned and ACPI includes the concept of PM domains. Thanks for pointing that out - thankfully, I've confirmed that the SMMUs on my Juno don't have dev->pm_domain set when booting with ACPI, and double-checking the ACPI code I think we're OK here. Since the SMMUs are only described in the static IORT table and not in the ACPI namespace, they won't have the ACPI companion device that acpi_dev_pm_attach() looks for, and thus should always be ignored. Lorenzo, do I have that right? Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 4/5] iommu/arm-smmu: Add the device_link between masters and smmu
On 09/03/18 07:11, Vivek Gautam wrote:
On Thu, Mar 8, 2018 at 10:29 AM, Vivek Gautam
wrote:
On Wed, Mar 7, 2018 at 6:17 PM, Robin Murphy wrote:
On 02/03/18 10:10, Vivek Gautam wrote:
From: Sricharan R
Finally add the device link between the master device and
smmu, so that the smmu gets runtime enabled/disabled only when the
master needs it. This is done from add_device callback which gets
called once when the master is added to the smmu.
Signed-off-by: Sricharan R
Signed-off-by: Vivek Gautam
---
drivers/iommu/arm-smmu.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 3d6a1875431f..bb1ea82c1003 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -217,6 +217,9 @@ struct arm_smmu_device {
/* IOMMU core code handle */
struct iommu_device iommu;
+
+ /* runtime PM link to master */
+ struct device_link *link;
Just the one?
we will either have to count all the devices that are present on the
iommu bus, or
maintain a list to which all the links can be added.
But to add the list, we will have to initialize a LIST_HEAD in struct
device_link
as well.
Or, I think we don't even need to maintain a pointer to link with smmu.
In arm_smmu_remove_device(), we can find out the correct link, and delete it.
list_for_each_entry(link, &dev->links.suppliers, c_node)
if (link->supplier == smmu->dev);
device_link_del(link);
Should that be fine?
Rafael, does the above snippet looks right to you? Context: smmu->dev
is the supplier, and dev is the consumer. We want to find the link,
and delete it.
Actually, looking at the existing code, it seems like device_link_add()
will in fact look up and return any existing link between a given
supplier and consumer - is that intentional API behaviour that users may
rely on to avoid keeping track of explicit link pointers? (or
conversely, might it be reasonable to factor out a device_link_find()
function?)
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 17/37] iommu/arm-smmu-v3: Move context descriptor code
On Mon, 12 Feb 2018 18:33:32 + Jean-Philippe Brucker wrote: > In order to add support for substream ID, move the context descriptor code > into a separate library. At the moment it only manages context descriptor > 0, which is used for non-PASID translations. > > One important behavior change is the ASID allocator, which is now global > instead of per-SMMU. If we end up needing per-SMMU ASIDs after all, it > would be relatively simple to move back to per-device allocator instead > of a global one. Sharing ASIDs will require an IDR, so implement the > ASID allocator with an IDA instead of porting the bitmap, to ease the > transition. > > Signed-off-by: Jean-Philippe Brucker Hi Jean-Philippe, This would have been easier to review if split into a 'move' and additional patches actually making the changes described. Superficially it looks like there may be more going on in here than the above description suggests. I'm unsure why we are gaining the CFGI_CD_ALL and similar in this patch as there is just to much going on. Thanks, Jonathan > --- > MAINTAINERS | 2 +- > drivers/iommu/Kconfig | 11 ++ > drivers/iommu/Makefile | 1 + > drivers/iommu/arm-smmu-v3-context.c | 289 > > drivers/iommu/arm-smmu-v3.c | 265 +++-- > drivers/iommu/iommu-pasid.c | 1 + > drivers/iommu/iommu-pasid.h | 27 > 7 files changed, 451 insertions(+), 145 deletions(-) > create mode 100644 drivers/iommu/arm-smmu-v3-context.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9cb8ced8322a..93507bfe03a6 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1104,7 +1104,7 @@ R: Robin Murphy > L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers) > S: Maintained > F: drivers/iommu/arm-smmu.c > -F: drivers/iommu/arm-smmu-v3.c > +F: drivers/iommu/arm-smmu-v3* > F: drivers/iommu/io-pgtable-arm.c > F: drivers/iommu/io-pgtable-arm.h > F: drivers/iommu/io-pgtable-arm-v7s.c > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index 8add90ba9b75..4b272925ee78 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -66,6 +66,16 @@ menu "Generic PASID table support" > config IOMMU_PASID_TABLE > bool > > +config ARM_SMMU_V3_CONTEXT > + bool "ARM SMMU v3 Context Descriptor tables" > + select IOMMU_PASID_TABLE > + depends on ARM64 > + help > + Enable support for ARM SMMU v3 Context Descriptor tables, used for DMA > + and PASID support. > + > + If unsure, say N here. > + > endmenu > > config IOMMU_IOVA > @@ -344,6 +354,7 @@ config ARM_SMMU_V3 > depends on ARM64 > select IOMMU_API > select IOMMU_IO_PGTABLE_LPAE > + select ARM_SMMU_V3_CONTEXT > select GENERIC_MSI_IRQ_DOMAIN > help > Support for implementations of the ARM System MMU architecture > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile > index 338e59c93131..22758960ed02 100644 > --- a/drivers/iommu/Makefile > +++ b/drivers/iommu/Makefile > @@ -9,6 +9,7 @@ obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o > obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o > obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o > obj-$(CONFIG_IOMMU_PASID_TABLE) += iommu-pasid.o > +obj-$(CONFIG_ARM_SMMU_V3_CONTEXT) += arm-smmu-v3-context.o > obj-$(CONFIG_IOMMU_IOVA) += iova.o > obj-$(CONFIG_OF_IOMMU) += of_iommu.o > obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o > diff --git a/drivers/iommu/arm-smmu-v3-context.c > b/drivers/iommu/arm-smmu-v3-context.c > new file mode 100644 > index ..e910cb356f45 > --- /dev/null > +++ b/drivers/iommu/arm-smmu-v3-context.c > @@ -0,0 +1,289 @@ > +/* > + * Context descriptor table driver for SMMUv3 > + * > + * Copyright (C) 2018 ARM Ltd. > + * > + * SPDX-License-Identifier: GPL-2.0 > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +#include "iommu-pasid.h" > + > +#define CTXDESC_CD_DWORDS8 > +#define CTXDESC_CD_0_TCR_T0SZ_SHIFT 0 > +#define ARM64_TCR_T0SZ_SHIFT 0 > +#define ARM64_TCR_T0SZ_MASK 0x1fUL > +#define CTXDESC_CD_0_TCR_TG0_SHIFT 6 > +#define ARM64_TCR_TG0_SHIFT 14 > +#define ARM64_TCR_TG0_MASK 0x3UL > +#define CTXDESC_CD_0_TCR_IRGN0_SHIFT 8 > +#define ARM64_TCR_IRGN0_SHIFT8 > +#define ARM64_TCR_IRGN0_MASK 0x3UL > +#define CTXDESC_CD_0_TCR_ORGN0_SHIFT 10 > +#define ARM64_TCR_ORGN0_SHIFT10 > +#define ARM64_TCR_ORGN0_MASK 0x3UL > +#define CTXDESC_CD_0_TCR_SH0_SHIFT 12 > +#define ARM64_TCR_SH0_SHIFT 12 > +#define ARM64_TCR_SH0_MASK 0x3UL > +#define CTXDESC_CD_0_TCR_EPD0_SHIFT 14 > +#define ARM64_TCR_EPD0_SHIFT 7 > +#define ARM64_TCR_EPD0_MASK 0x1UL > +#define CTXDESC_CD_0_TCR_EPD1_SHIFT 30 > +#define ARM64_TCR_EPD1_SHIFT 23 > +#define ARM64_TCR_EPD1
Re: [PATCH v8 4/5] iommu/arm-smmu: Add the device_link between masters and smmu
On Wed, Mar 7, 2018 at 6:17 PM, Robin Murphy wrote:
> On 02/03/18 10:10, Vivek Gautam wrote:
>>
>> From: Sricharan R
>>
>> Finally add the device link between the master device and
>> smmu, so that the smmu gets runtime enabled/disabled only when the
>> master needs it. This is done from add_device callback which gets
>> called once when the master is added to the smmu.
>>
>> Signed-off-by: Sricharan R
>> Signed-off-by: Vivek Gautam
>> ---
>> drivers/iommu/arm-smmu.c | 21 +
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 3d6a1875431f..bb1ea82c1003 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -217,6 +217,9 @@ struct arm_smmu_device {
>> /* IOMMU core code handle */
>> struct iommu_device iommu;
>> +
>> + /* runtime PM link to master */
>> + struct device_link *link;
>
>
> Just the one?
>
>> };
>> enum arm_smmu_context_fmt {
>> @@ -1470,10 +1473,26 @@ static int arm_smmu_add_device(struct device *dev)
>> iommu_device_link(&smmu->iommu, dev);
>> + /*
>> +* Establish the link between smmu and master, so that the
>> +* smmu gets runtime enabled/disabled as per the master's
>> +* needs.
>> +*/
>> + smmu->link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
>
>
> Maybe I've misunderstood how the API works, but AFAICS the second and
> subsequent devices are all just going to overwrite (and leak) the link of
> the previous one...
Also, noticed one more thing while testing on sdm845. When we are
conditionally enabling the runtime pm, we should create the device
link too conditionally, i.e. only in the case the smmu->dev has
runtime pm_enabled we can create this device link between smmu and the
master device.
Otherwise when the master tries to do a pm_runtime_get() over itself,
the device link will ensure that pm_runtime_get() for smmu is done
first. But that will fail when we don't have pm runtime enabled over
smmu, and so the master device's pm_runtime_get() will fail too.
Will fix this in the next version.
Thanks
Vivek
>
>> + if (!smmu->link) {
>> + dev_warn(smmu->dev, "Unable to create device link between
>> %s and %s\n",
>> +dev_name(smmu->dev), dev_name(dev));
>> + ret = -ENODEV;
>> + goto out_unlink;
>> + }
>> +
>> arm_smmu_rpm_put(smmu);
>> return 0;
>> +out_unlink:
>> + iommu_device_unlink(&smmu->iommu, dev);
>> + arm_smmu_master_free_smes(fwspec);
>> out_rpm_put:
>> arm_smmu_rpm_put(smmu);
>> out_cfg_free:
>> @@ -1496,6 +1515,8 @@ static void arm_smmu_remove_device(struct device
>> *dev)
>> cfg = fwspec->iommu_priv;
>> smmu = cfg->smmu;
>> + device_link_del(smmu->link);
>
>
> ...and equivalently you end up with a double-free (or more) here of a link
> which may not have belonged to dev anyway.
>
> Robin.
>
>
>> +
>> ret = arm_smmu_rpm_get(smmu);
>> if (ret < 0)
>> return;
>>
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
