diff mbox

[01/17] of: add dma-mask binding

Message ID 1352710357-3265-2-git-send-email-wenyou.yang@atmel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wenyou Yang Nov. 12, 2012, 8:52 a.m. UTC 
From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

This will allow each device to specify its dma-mask for this we use the
coherent_dma_mask as pointer. By default the dma-mask will be set to
DMA_BIT_MASK(32).
The microblaze architecture hook is drop

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: grant.likely@secretlab.ca
Cc: rob.herring@calxeda.com
Cc: devicetree-discuss@lists.ozlabs.org
---
 drivers/of/platform.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Comments

Rob Herring Nov. 14, 2012, 3:55 a.m. UTC | #1 
On 11/12/2012 02:52 AM, Wenyou Yang wrote:
> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> 
> This will allow each device to specify its dma-mask for this we use the
> coherent_dma_mask as pointer. By default the dma-mask will be set to
> DMA_BIT_MASK(32).

Do you really have a use case this is not DMA_BIT_MASK(32)?

> The microblaze architecture hook is drop
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: grant.likely@secretlab.ca
> Cc: rob.herring@calxeda.com
> Cc: devicetree-discuss@lists.ozlabs.org
> ---
>  drivers/of/platform.c |   23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b80891b..31ed405 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -130,6 +130,21 @@ void of_device_make_bus_id(struct device *dev)
>  	dev_set_name(dev, "%s.%d", node->name, magic - 1);
>  }
>  
> +static void of_get_dma_mask(struct device *dev, struct device_node *np)
> +{
> +	const __be32 *prop;
> +	int len;
> +
> +	prop = of_get_property(np, "dma-mask", &len);

dma-ranges may work for this purpose.

> +
> +	dev->dma_mask = &dev->coherent_dma_mask;

I don't really know, but I suspect this is wrong.

> +
> +	if (!prop)
> +		dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +	else
> +		dev->coherent_dma_mask = of_read_number(prop, len / 4);
> +}
> +
>  /**
>   * of_device_alloc - Allocate and initialize an of_device
>   * @np: device node to assign to device
> @@ -171,10 +186,8 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
>  	}
>  
> +	of_get_dma_mask(&dev->dev, np);
>  	dev->dev.of_node = of_node_get(np);
> -#if defined(CONFIG_MICROBLAZE)
> -	dev->dev.dma_mask = &dev->archdata.dma_mask;
> -#endif
>  	dev->dev.parent = parent;
>  
>  	if (bus_id)
> @@ -211,10 +224,6 @@ struct platform_device *of_platform_device_create_pdata(
>  	if (!dev)
>  		return NULL;
>  
> -#if defined(CONFIG_MICROBLAZE)
> -	dev->archdata.dma_mask = 0xffffffffUL;
> -#endif
> -	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>  	dev->dev.bus = &platform_bus_type;
>  	dev->dev.platform_data = platform_data;
>  
>
Jean-Christophe PLAGNIOL-VILLARD Nov. 14, 2012, 6 a.m. UTC | #2 
On 21:55 Tue 13 Nov     , Rob Herring wrote:
> On 11/12/2012 02:52 AM, Wenyou Yang wrote:
> > From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > 
> > This will allow each device to specify its dma-mask for this we use the
> > coherent_dma_mask as pointer. By default the dma-mask will be set to
> > DMA_BIT_MASK(32).
> 
> Do you really have a use case this is not DMA_BIT_MASK(32)?
yes as exmample on 64bit platfrom it will be 64 on x86 it's also 24, on power
pc 40
> 
> > The microblaze architecture hook is drop
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > Cc: grant.likely@secretlab.ca
> > Cc: rob.herring@calxeda.com
> > Cc: devicetree-discuss@lists.ozlabs.org
> > ---
> >  drivers/of/platform.c |   23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index b80891b..31ed405 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -130,6 +130,21 @@ void of_device_make_bus_id(struct device *dev)
> >  	dev_set_name(dev, "%s.%d", node->name, magic - 1);
> >  }
> >  
> > +static void of_get_dma_mask(struct device *dev, struct device_node *np)
> > +{
> > +	const __be32 *prop;
> > +	int len;
> > +
> > +	prop = of_get_property(np, "dma-mask", &len);
> 
> dma-ranges may work for this purpose.
> 
> > +
> > +	dev->dma_mask = &dev->coherent_dma_mask;
> 
> I don't really know, but I suspect this is wrong.
no this is correct was suggest by Russell
we already do so on other ARM or SH as example

the dma expect a pointer for dma_mask but the value is the same as coherent
> 
> > +
> > +	if (!prop)
> > +		dev->coherent_dma_mask = DMA_BIT_MASK(32);
> > +	else
> > +		dev->coherent_dma_mask = of_read_number(prop, len / 4);
> > +}
> > +
> >  /**
> >   * of_device_alloc - Allocate and initialize an of_device
> >   * @np: device node to assign to device
> > @@ -171,10 +186,8 @@ struct platform_device *of_device_alloc(struct device_node *np,
> >  		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
> >  	}
> >  
> > +	of_get_dma_mask(&dev->dev, np);
> >  	dev->dev.of_node = of_node_get(np);
> > -#if defined(CONFIG_MICROBLAZE)
> > -	dev->dev.dma_mask = &dev->archdata.dma_mask;
> > -#endif
> >  	dev->dev.parent = parent;
> >  
> >  	if (bus_id)
> > @@ -211,10 +224,6 @@ struct platform_device *of_platform_device_create_pdata(
> >  	if (!dev)
> >  		return NULL;
> >  
> > -#if defined(CONFIG_MICROBLAZE)
> > -	dev->archdata.dma_mask = 0xffffffffUL;
> > -#endif
> > -	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> >  	dev->dev.bus = &platform_bus_type;
> >  	dev->dev.platform_data = platform_data;
> >  
> >
Rob Herring Nov. 14, 2012, 8:56 p.m. UTC | #3 
On 11/14/2012 12:00 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 21:55 Tue 13 Nov     , Rob Herring wrote:
>> On 11/12/2012 02:52 AM, Wenyou Yang wrote:
>>> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>>
>>> This will allow each device to specify its dma-mask for this we use the
>>> coherent_dma_mask as pointer. By default the dma-mask will be set to
>>> DMA_BIT_MASK(32).
>>
>> Do you really have a use case this is not DMA_BIT_MASK(32)?
> yes as exmample on 64bit platfrom it will be 64 on x86 it's also 24, on power
> pc 40
>>
>>> The microblaze architecture hook is drop
>>>
>>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>> Cc: grant.likely@secretlab.ca
>>> Cc: rob.herring@calxeda.com
>>> Cc: devicetree-discuss@lists.ozlabs.org
>>> ---
>>>  drivers/of/platform.c |   23 ++++++++++++++++-------
>>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index b80891b..31ed405 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -130,6 +130,21 @@ void of_device_make_bus_id(struct device *dev)
>>>  	dev_set_name(dev, "%s.%d", node->name, magic - 1);
>>>  }
>>>  
>>> +static void of_get_dma_mask(struct device *dev, struct device_node *np)
>>> +{
>>> +	const __be32 *prop;
>>> +	int len;
>>> +
>>> +	prop = of_get_property(np, "dma-mask", &len);
>>
>> dma-ranges may work for this purpose.
>>
>>> +
>>> +	dev->dma_mask = &dev->coherent_dma_mask;
>>
>> I don't really know, but I suspect this is wrong.
> no this is correct was suggest by Russell
> we already do so on other ARM or SH as example
> 
> the dma expect a pointer for dma_mask but the value is the same as coherent

Okay. Then perhaps this part should be a separate patch as that is
useful on its own for 32-bit machines with no DMA address restrictions
(most modern ARM h/w).

My comment on using dma-ranges still stands though. The form is
<parent-address child-address size>. Normally, parent and child would be
the same. I think the mask would be "parent address + size - 1". The
simple case is <0 0 0> for all of 32-bit memory. I'm using 0 for full
32-bit size here as #size-cells is typically 1 and I can't imagine that
0 size would be useful.

Rob

>>
>>> +
>>> +	if (!prop)
>>> +		dev->coherent_dma_mask = DMA_BIT_MASK(32);
>>> +	else
>>> +		dev->coherent_dma_mask = of_read_number(prop, len / 4);
>>> +}
>>> +
>>>  /**
>>>   * of_device_alloc - Allocate and initialize an of_device
>>>   * @np: device node to assign to device
>>> @@ -171,10 +186,8 @@ struct platform_device *of_device_alloc(struct device_node *np,
>>>  		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
>>>  	}
>>>  
>>> +	of_get_dma_mask(&dev->dev, np);
>>>  	dev->dev.of_node = of_node_get(np);
>>> -#if defined(CONFIG_MICROBLAZE)
>>> -	dev->dev.dma_mask = &dev->archdata.dma_mask;
>>> -#endif
>>>  	dev->dev.parent = parent;
>>>  
>>>  	if (bus_id)
>>> @@ -211,10 +224,6 @@ struct platform_device *of_platform_device_create_pdata(
>>>  	if (!dev)
>>>  		return NULL;
>>>  
>>> -#if defined(CONFIG_MICROBLAZE)
>>> -	dev->archdata.dma_mask = 0xffffffffUL;
>>> -#endif
>>> -	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>>>  	dev->dev.bus = &platform_bus_type;
>>>  	dev->dev.platform_data = platform_data;
>>>  
>>>
Russell King - ARM Linux Nov. 14, 2012, 9:05 p.m. UTC | #4 
On Wed, Nov 14, 2012 at 02:56:14PM -0600, Rob Herring wrote:
> On 11/14/2012 12:00 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 21:55 Tue 13 Nov     , Rob Herring wrote:
> >> On 11/12/2012 02:52 AM, Wenyou Yang wrote:
> >>> +
> >>> +	dev->dma_mask = &dev->coherent_dma_mask;
> >>
> >> I don't really know, but I suspect this is wrong.
> > no this is correct was suggest by Russell
> > we already do so on other ARM or SH as example
> > 
> > the dma expect a pointer for dma_mask but the value is the same as coherent
> 
> Okay. Then perhaps this part should be a separate patch as that is
> useful on its own for 32-bit machines with no DMA address restrictions
> (most modern ARM h/w).

I'm not sure that I did make the exact suggestion being alluded to above
(I think I may have made the suggestion that dev->dma_mask should be
pointed at a dma_mask, and it might be a good idea that it should be
part of struct device.)

I've always shy'd away from making it the same thing as the coherent
DMA mask, because there are drivers around which modify the value
pointed to by dev->dma_mask via standard DMA API calls.  (Eg, when they
know that the device is only N-bit capable.)

With that set to the same as dev->coherent_dma_mask, it ends up
modifying the coherent DMA mask too which may or may not be entirely
a good thing; I suspect ultimately that depends on the driver.

My thoughts on this though is that the whole thing is a mess.  I'm
not sure why we ended up with a separate coherent_dma_mask from the
main dma_mask, and why we ended up with dma_mask being a pointer to
something allocated elsewhere... it all seems like it's unnecessarily
complicated and was designed to cause confusion...
diff mbox

Patch

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b80891b..31ed405 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -130,6 +130,21 @@  void of_device_make_bus_id(struct device *dev)
 	dev_set_name(dev, "%s.%d", node->name, magic - 1);
 }
 
+static void of_get_dma_mask(struct device *dev, struct device_node *np)
+{
+	const __be32 *prop;
+	int len;
+
+	prop = of_get_property(np, "dma-mask", &len);
+
+	dev->dma_mask = &dev->coherent_dma_mask;
+
+	if (!prop)
+		dev->coherent_dma_mask = DMA_BIT_MASK(32);
+	else
+		dev->coherent_dma_mask = of_read_number(prop, len / 4);
+}
+
 /**
  * of_device_alloc - Allocate and initialize an of_device
  * @np: device node to assign to device
@@ -171,10 +186,8 @@  struct platform_device *of_device_alloc(struct device_node *np,
 		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
 	}
 
+	of_get_dma_mask(&dev->dev, np);
 	dev->dev.of_node = of_node_get(np);
-#if defined(CONFIG_MICROBLAZE)
-	dev->dev.dma_mask = &dev->archdata.dma_mask;
-#endif
 	dev->dev.parent = parent;
 
 	if (bus_id)
@@ -211,10 +224,6 @@  struct platform_device *of_platform_device_create_pdata(
 	if (!dev)
 		return NULL;
 
-#if defined(CONFIG_MICROBLAZE)
-	dev->archdata.dma_mask = 0xffffffffUL;
-#endif
-	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;