| Message ID | 1373021097-32420-17-git-send-email-hdoyu@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/05/2013 04:44 AM, Hiroshi Doyu wrote: > This provides the info about which H/W Accelerators are supported on > Tegra SoC. This info is passed from DT. This is necessary to have the > unified SMMU driver among Tegra SoCs. Instead of using platform data, > DT passes "nvidia,swgroup" now. DT is mandatory in Tegra. > diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt > +- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA). > + Each bit represents one swgroup. The assignments may be found in header > + file <dt-bindings/memory/tegra-swgroup.h>. There needs to be a default for this field if one is not specified so that existing DTs continue to work without modification. How many cells big is this property? Is this really a bitmap of HWAs? Surely it's a bitmap of SMMU client IDs? > Example: > - smmu { > + iommu { The node name shouldn't be interpreted by anything, so there should be no need to change it at all. Certainly, it should not be changed by this patch, since this patch is all about SMMU client IDs. > + nvidia,swgroups = TEGRA30_SWGROUP_ALL; As I mentioned before, macros shouldn't include syntax/structure, just data values. > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c > @@ -265,7 +265,7 @@ struct smmu_client { > struct device *dev; > struct list_head list; > struct smmu_as *as; > - u32 hwgrp; > + u64 hwgrp; Why is that "hwgrp" not "swgrp"? Don't they represent the same thing? > @@ -307,6 +307,8 @@ struct smmu_device { > struct device *dev; > struct page *avp_vector_page; /* dummy page shared by all AS's */ > > + u64 swgroup; /* swgroup ID bitmap */ If that's a bitmap, then it represents multiple things, so "swgroups"? > @@ -382,10 +384,10 @@ static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs) > */ > #define FLUSH_SMMU_REGS(smmu) smmu_read(smmu, SMMU_CONFIG) > > -#define smmu_client_hwgrp(c) (u32)((c)->dev->platform_data) > +#define smmu_client_hwgrp(c) (c->as->smmu->swgroup) hwgrp-vs-swgrp inconsistency again. Is this series git bisectable? I worry that by the time patch 14 is applied, the SMMU starts affecting client devices, whereas none of those client devices have swgroup values defined as their client data, and hence without this patch also applied, things might blow up in interesting ways. I wonder why the code was ever using dev->platform_data in this way; the platform data for a device should have its structure/semantics defined by the driver for that device, not by an SMMU that happens to affect that device. Ick! > static int __smmu_client_set_hwgrp(struct smmu_client *c, > - unsigned long map, int on) > + u64 map, int on) > { > int i; > struct smmu_as *as = c->as; > @@ -398,12 +400,11 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c, > if (!on) > map = smmu_client_hwgrp(c); > > - for_each_set_bit(i, &map, HWGRP_COUNT) { > + for_each_set_bit(i, (unsigned long *)&map, > + sizeof(map) * BITS_PER_BYTE) { Why change the type if it just forces you to add this cast? > offs = HWGRP_ASID_REG(i); > val = smmu_read(smmu, offs); > if (on) { > - if (WARN_ON(val & mask)) > - goto err_hw_busy; Why?
Stephen Warren <swarren@wwwdotorg.org> wrote @ Thu, 18 Jul 2013 22:28:42 +0200: > On 07/05/2013 04:44 AM, Hiroshi Doyu wrote: > > This provides the info about which H/W Accelerators are supported on > > Tegra SoC. This info is passed from DT. This is necessary to have the > > unified SMMU driver among Tegra SoCs. Instead of using platform data, > > DT passes "nvidia,swgroup" now. DT is mandatory in Tegra. > > > diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt > > > +- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA). > > + Each bit represents one swgroup. The assignments may be found in header > > + file <dt-bindings/memory/tegra-swgroup.h>. > > There needs to be a default for this field if one is not specified so > that existing DTs continue to work without modification. Only enabling PPCS(AHB) can be an option because PPCS has SD/MMC where rootfs can be located ususally. > How many cells big is this property? 64 > Is this really a bitmap of HWAs? Surely it's a bitmap of SMMU client > IDs? At least this info can be used for PMC too. ..... > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c > > > @@ -265,7 +265,7 @@ struct smmu_client { > > struct device *dev; > > struct list_head list; > > struct smmu_as *as; > > - u32 hwgrp; > > + u64 hwgrp; > > Why is that "hwgrp" not "swgrp"? Don't they represent the same > thing? They are same but initial SMMU driver used the term "hwgroup". Should this be renamed with another patch or can it be left as it is? .... > > static int __smmu_client_set_hwgrp(struct smmu_client *c, > > - unsigned long map, int on) > > + u64 map, int on) > > { > > int i; > > struct smmu_as *as = c->as; > > @@ -398,12 +400,11 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c, > > if (!on) > > map = smmu_client_hwgrp(c); > > > > - for_each_set_bit(i, &map, HWGRP_COUNT) { > > + for_each_set_bit(i, (unsigned long *)&map, > > + sizeof(map) * BITS_PER_BYTE) { > > Why change the type if it just forces you to add this cast? u32 map; -> u64 map; for_each_set_bit() expects "unsigned long *" for any length of bitmap.
On 07/29/2013 05:39 AM, Hiroshi Doyu wrote: > Stephen Warren <swarren@wwwdotorg.org> wrote @ Thu, 18 Jul 2013 22:28:42 +0200: > >> On 07/05/2013 04:44 AM, Hiroshi Doyu wrote: >>> This provides the info about which H/W Accelerators are supported on >>> Tegra SoC. This info is passed from DT. This is necessary to have the >>> unified SMMU driver among Tegra SoCs. Instead of using platform data, >>> DT passes "nvidia,swgroup" now. DT is mandatory in Tegra. >> >>> diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt >> >>> +- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA). >>> + Each bit represents one swgroup. The assignments may be found in header >>> + file <dt-bindings/memory/tegra-swgroup.h>. >> >> There needs to be a default for this field if one is not specified so >> that existing DTs continue to work without modification. > > Only enabling PPCS(AHB) can be an option because PPCS has SD/MMC where > rootfs can be located ususally. There's no reason that the root filesystem has to be on SD/MMC. Either way, the DT binding shouldn't be influenced by the root fs location at all. I think more explanation of exactly what this property does and why is required. >> How many cells big is this property? > > 64 I assume that's bits, so 2 cells? To be clear: the document needs to include this information, not just this email thread. >> Is this really a bitmap of HWAs? Surely it's a bitmap of SMMU client >> IDs? > > At least this info can be used for PMC too. How and why? A complete explanation of how the SMMU and PMC are expected to interact is required. The PMC DT binding should include all information related to the PMC; the binding definitions probably shouldn't expect a PMC driver to go grovelling in an SMMU node to find information. > ..... >>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c >> >>> @@ -265,7 +265,7 @@ struct smmu_client { >>> struct device *dev; >>> struct list_head list; >>> struct smmu_as *as; >>> - u32 hwgrp; >>> + u64 hwgrp; >> >> Why is that "hwgrp" not "swgrp"? Don't they represent the same >> thing? > > They are same but initial SMMU driver used the term "hwgroup". Should > this be renamed with another patch or can it be left as it is? I thought there had already been a patch to do this rename. Was it not complete? If so, that work should probably be completed. > .... >>> static int __smmu_client_set_hwgrp(struct smmu_client *c, >>> - unsigned long map, int on) >>> + u64 map, int on) >>> { >>> int i; >>> struct smmu_as *as = c->as; >>> @@ -398,12 +400,11 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c, >>> if (!on) >>> map = smmu_client_hwgrp(c); >>> >>> - for_each_set_bit(i, &map, HWGRP_COUNT) { >>> + for_each_set_bit(i, (unsigned long *)&map, >>> + sizeof(map) * BITS_PER_BYTE) { >> >> Why change the type if it just forces you to add this cast? > > u32 map; -> u64 map; > > for_each_set_bit() expects "unsigned long *" for any length of bitmap. Shouldn't the map just be an "unsigned long map[]" then, so no casts are needed anywhere? Equally, that pointer could just be passed to the function rather than copying the data to the stack?
diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt index ce5c43e..0c14dca 100644 --- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt @@ -8,14 +8,18 @@ Required properties: - nvidia,#asids : # of ASIDs - dma-window : IOVA start address and length. - nvidia,ahb : phandle to the ahb bus connected to SMMU. +- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA). + Each bit represents one swgroup. The assignments may be found in header + file <dt-bindings/memory/tegra-swgroup.h>. Example: - smmu { + iommu { compatible = "nvidia,tegra30-smmu"; reg = <0x7000f010 0x02c 0x7000f1f0 0x010 0x7000f228 0x05c>; nvidia,#asids = <4>; /* # of ASIDs */ dma-window = <0 0x40000000>; /* IOVA start & length */ + nvidia,swgroups = TEGRA30_SWGROUP_ALL; nvidia,ahb = <&ahb>; }; diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 95c6c80..c7b33f2 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -265,7 +265,7 @@ struct smmu_client { struct device *dev; struct list_head list; struct smmu_as *as; - u32 hwgrp; + u64 hwgrp; }; /* @@ -307,6 +307,8 @@ struct smmu_device { struct device *dev; struct page *avp_vector_page; /* dummy page shared by all AS's */ + u64 swgroup; /* swgroup ID bitmap */ + /* * Register image savers for suspend/resume */ @@ -382,10 +384,10 @@ static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs) */ #define FLUSH_SMMU_REGS(smmu) smmu_read(smmu, SMMU_CONFIG) -#define smmu_client_hwgrp(c) (u32)((c)->dev->platform_data) +#define smmu_client_hwgrp(c) (c->as->smmu->swgroup) static int __smmu_client_set_hwgrp(struct smmu_client *c, - unsigned long map, int on) + u64 map, int on) { int i; struct smmu_as *as = c->as; @@ -398,12 +400,11 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c, if (!on) map = smmu_client_hwgrp(c); - for_each_set_bit(i, &map, HWGRP_COUNT) { + for_each_set_bit(i, (unsigned long *)&map, + sizeof(map) * BITS_PER_BYTE) { offs = HWGRP_ASID_REG(i); val = smmu_read(smmu, offs); if (on) { - if (WARN_ON(val & mask)) - goto err_hw_busy; val |= mask; } else { WARN_ON((val & mask) == mask); @@ -414,15 +415,6 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c, FLUSH_SMMU_REGS(smmu); c->hwgrp = map; return 0; - -err_hw_busy: - for_each_set_bit(i, &map, HWGRP_COUNT) { - offs = HWGRP_ASID_REG(i); - val = smmu_read(smmu, offs); - val &= ~mask; - smmu_write(smmu, val, offs); - } - return -EBUSY; } static int smmu_client_set_hwgrp(struct smmu_client *c, u32 map, int on) @@ -794,7 +786,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain, struct smmu_as *as = domain->priv; struct smmu_device *smmu = as->smmu; struct smmu_client *client, *c; - u32 map; + u64 map; int err; client = devm_kzalloc(smmu->dev, sizeof(*c), GFP_KERNEL); @@ -802,7 +794,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain, return -ENOMEM; client->dev = dev; client->as = as; - map = (unsigned long)dev->platform_data; + map = smmu->swgroup; if (!map) return -EINVAL; @@ -1210,6 +1202,7 @@ static int tegra_smmu_probe(struct platform_device *pdev) int i, asids, err = 0; dma_addr_t uninitialized_var(base); size_t bytes, uninitialized_var(size); + u64 swgroup; if (smmu_handle) return -EIO; @@ -1219,6 +1212,9 @@ static int tegra_smmu_probe(struct platform_device *pdev) if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids)) return -ENODEV; + if (of_property_read_u64(dev->of_node, "nvidia,swgroup", &swgroup)) + return -ENODEV; + bytes = sizeof(*smmu) + asids * (sizeof(*smmu->as) + sizeof(struct dma_iommu_mapping *)); smmu = devm_kzalloc(dev, bytes, GFP_KERNEL); @@ -1267,6 +1263,7 @@ static int tegra_smmu_probe(struct platform_device *pdev) smmu->num_as = asids; smmu->iovmm_base = base; smmu->page_count = size; + smmu->swgroup = swgroup; smmu->translation_enable_0 = ~0; smmu->translation_enable_1 = ~0;
This provides the info about which H/W Accelerators are supported on Tegra SoC. This info is passed from DT. This is necessary to have the unified SMMU driver among Tegra SoCs. Instead of using platform data, DT passes "nvidia,swgroup" now. DT is mandatory in Tegra. Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> --- .../bindings/iommu/nvidia,tegra30-smmu.txt | 6 +++- drivers/iommu/tegra-smmu.c | 31 +++++++++----------- 2 files changed, 19 insertions(+), 18 deletions(-)