diff mbox

[v2,16/22] iommu/tegra: smmu: Get "nvidia,swgroup" from DT

Message ID 1373021097-32420-17-git-send-email-hdoyu@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hiroshi DOYU July 5, 2013, 10:44 a.m. UTC
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(-)

Comments

Stephen Warren July 18, 2013, 8:28 p.m. UTC | #1
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?
Hiroshi DOYU July 29, 2013, 11:39 a.m. UTC | #2
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.
Stephen Warren July 29, 2013, 6 p.m. UTC | #3
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 mbox

Patch

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;