[PATCH 1/1] of: add dma-mask binding

[PATCH 1/1] of: add dma-mask binding

[Jean-Christophe PLAGNIOL-VILLARD]

This will allow each device to specify its dma-mask
The microblaze architecture hook is keep temporary if no dma-mask is specified
int the DT

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 drivers/of/platform.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index cae9477..bb22194 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -121,6 +121,26 @@ void of_device_make_bus_id(struct device *dev)
 	dev_set_name(dev, "%s.%d", node->name, magic - 1);
 }
 
+static u64* of_get_dma_mask(struct device_node *np)
+{
+	const __be32 *prop;
+	int len;
+	u64 *dma_mask;
+
+	prop = of_get_property(np, "dma-mask", &len);
+
+	if (!prop)
+		return NULL;
+
+	dma_mask = kzalloc(sizeof(u64), GFP_KERNEL);
+	if (!dma_mask)
+		return NULL;
+
+	*dma_mask = of_read_number(prop, len / 4);
+
+	return dma_mask;
+}
+
 /**
  * of_device_alloc - Allocate and initialize an of_device
  * @np: device node to assign to device
@@ -161,10 +181,14 @@ struct platform_device *of_device_alloc(struct device_node *np,
 		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
 	}
 
+	dev->dev.dma_mask = of_get_dma_mask(np);
 	dev->dev.of_node = of_node_get(np);
+
 #if defined(CONFIG_MICROBLAZE)
-	dev->dev.dma_mask = &dev->archdata.dma_mask;
+	if (!dev->dev.dma_mask)
+		dev->dev.dma_mask = &dev->archdata.dma_mask;
 #endif
+
 	dev->dev.parent = parent;
 
 	if (bus_id)
-- 
1.7.7

[PATCH 1/1] of: add coherent dma-mask binding

[Jean-Christophe PLAGNIOL-VILLARD]

This will allow each device to specify its coherent-dma-mask
The default behavior the set it to DMA_BIT_MASK(32) if none specified
is keeped.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 drivers/of/platform.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index bb22194..d2a2ea02 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -141,6 +141,19 @@ static u64* of_get_dma_mask(struct device_node *np)
 	return dma_mask;
 }
 
+static u64 of_get_coherent_dma_mask(struct device_node *np)
+{
+	const __be32 *prop;
+	int len;
+
+	prop = of_get_property(np, "coherent-dma-mask", &len);
+
+	if (!prop)
+		return DMA_BIT_MASK(32);
+
+	return of_read_number(prop, len / 4);
+}
+
 /**
  * of_device_alloc - Allocate and initialize an of_device
  * @np: device node to assign to device
@@ -228,7 +241,7 @@ struct platform_device *of_platform_device_create_pdata(
 #if defined(CONFIG_MICROBLAZE)
 	dev->archdata.dma_mask = 0xffffffffUL;
 #endif
-	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	dev->dev.coherent_dma_mask = of_get_coherent_dma_mask(np);
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
 
-- 
1.7.7

[PATCH 1/1] of: add dma-mask binding

[Rob Herring]

On 03/07/2012 05:26 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> This will allow each device to specify its dma-mask
> The microblaze architecture hook is keep temporary if no dma-mask is specified
> int the DT
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  drivers/of/platform.c |   26 +++++++++++++++++++++++++-
>  1 files changed, 25 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index cae9477..bb22194 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -121,6 +121,26 @@ void of_device_make_bus_id(struct device *dev)
>  	dev_set_name(dev, "%s.%d", node->name, magic - 1);
>  }
>  
> +static u64* of_get_dma_mask(struct device_node *np)
> +{
> +	const __be32 *prop;
> +	int len;
> +	u64 *dma_mask;
> +
> +	prop = of_get_property(np, "dma-mask", &len);

This would need some documentation. There is already "dma-ranges"
defined for OF which nay do what's needed.

> +
> +	if (!prop)
> +		return NULL;
> +
> +	dma_mask = kzalloc(sizeof(u64), GFP_KERNEL);

This seems kind of wasteful for 1 u64.

> +	if (!dma_mask)
> +		return NULL;
> +
> +	*dma_mask = of_read_number(prop, len / 4);
> +
> +	return dma_mask;
> +}
> +
>  /**
>   * of_device_alloc - Allocate and initialize an of_device
>   * @np: device node to assign to device
> @@ -161,10 +181,14 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
>  	}
>  
> +	dev->dev.dma_mask = of_get_dma_mask(np);
>  	dev->dev.of_node = of_node_get(np);
> +
>  #if defined(CONFIG_MICROBLAZE)
> -	dev->dev.dma_mask = &dev->archdata.dma_mask;

And doing it this way for didn't get a warm reception either:

http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-April/005285.html

Do you really need this to be something other than ~0UL and if so does
it need to be per device or system wide?

You can solve this with bus notifiers in your at91 code. Here's an
example which I did (but no longer need):

static u64 highbank_dma_mask = DMA_BIT_MASK(32);

static int highbank_platform_notifier(struct notifier_block *nb,
				  unsigned long event, void *__dev)
{
	struct device *dev = __dev;

	if (event != BUS_NOTIFY_ADD_DEVICE)
		return NOTIFY_DONE;

	if (of_device_is_compatible(dev->of_node, "calxeda,hb-nfc"))
		dev->dma_mask = &highbank_dma_mask;
	else
		return NOTIFY_DONE;

	return NOTIFY_OK;
}

static struct notifier_block highbank_platform_nb = {
	.notifier_call = highbank_platform_notifier,
};

int highbank_devices_init(void)
{
	return bus_register_notifier(&platform_bus_type, &highbank_platform_nb);
}

Rob

> +	if (!dev->dev.dma_mask)
> +		dev->dev.dma_mask = &dev->archdata.dma_mask;
>  #endif
> +
>  	dev->dev.parent = parent;
>  
>  	if (bus_id)

[PATCH 1/1] of: add dma-mask binding

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:59 Wed 07 Mar     , Rob Herring wrote:
> On 03/07/2012 05:26 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > This will allow each device to specify its dma-mask
> > The microblaze architecture hook is keep temporary if no dma-mask is specified
> > int the DT
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> >  drivers/of/platform.c |   26 +++++++++++++++++++++++++-
> >  1 files changed, 25 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index cae9477..bb22194 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -121,6 +121,26 @@ void of_device_make_bus_id(struct device *dev)
> >  	dev_set_name(dev, "%s.%d", node->name, magic - 1);
> >  }
> >  
> > +static u64* of_get_dma_mask(struct device_node *np)
> > +{
> > +	const __be32 *prop;
> > +	int len;
> > +	u64 *dma_mask;
> > +
> > +	prop = of_get_property(np, "dma-mask", &len);
> 
> This would need some documentation. There is already "dma-ranges"
> defined for OF which nay do what's needed.
what is dma-ranges I see no doc about it
but I don't think it could be used for the dma mask
> 
> > +
> > +	if (!prop)
> > +		return NULL;
> > +
> > +	dma_mask = kzalloc(sizeof(u64), GFP_KERNEL);
> 
> This seems kind of wasteful for 1 u64.
no choice dma_mask must be a pointer
> 
> > +	if (!dma_mask)
> > +		return NULL;
> > +
> > +	*dma_mask = of_read_number(prop, len / 4);
> > +
> > +	return dma_mask;
> > +}
> > +
> >  /**
> >   * of_device_alloc - Allocate and initialize an of_device
> >   * @np: device node to assign to device
> > @@ -161,10 +181,14 @@ struct platform_device *of_device_alloc(struct device_node *np,
> >  		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
> >  	}
> >  
> > +	dev->dev.dma_mask = of_get_dma_mask(np);
> >  	dev->dev.of_node = of_node_get(np);
> > +
> >  #if defined(CONFIG_MICROBLAZE)
> > -	dev->dev.dma_mask = &dev->archdata.dma_mask;
> 
> And doing it this way for didn't get a warm reception either:
> 
> http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-April/005285.html
> 
> Do you really need this to be something other than ~0UL and if so does
> it need to be per device or system wide?
> 
> You can solve this with bus notifiers in your at91 code. Here's an
> example which I did (but no longer need):
> 
> static u64 highbank_dma_mask = DMA_BIT_MASK(32);
IIRC on at91 all of them that need it have a DMA_BIT_MASK(32)


> 
> static int highbank_platform_notifier(struct notifier_block *nb,
> 				  unsigned long event, void *__dev)
> {
> 	struct device *dev = __dev;
> 
> 	if (event != BUS_NOTIFY_ADD_DEVICE)
> 		return NOTIFY_DONE;
> 
> 	if (of_device_is_compatible(dev->of_node, "calxeda,hb-nfc"))
> 		dev->dma_mask = &highbank_dma_mask;
execpt as example today I need it on 2 node (OHCI & EHCI)

and if more device need I don't want to end up with more fixup

I really prefer to set it via DT

Best Regards,
J.

[PATCH 1/1] of: add dma-mask binding

[Rob Herring]

On 03/07/2012 11:34 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 10:59 Wed 07 Mar     , Rob Herring wrote:
>> On 03/07/2012 05:26 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> This will allow each device to specify its dma-mask
>>> The microblaze architecture hook is keep temporary if no dma-mask is specified
>>> int the DT
>>>
>>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>> ---
>>>  drivers/of/platform.c |   26 +++++++++++++++++++++++++-
>>>  1 files changed, 25 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index cae9477..bb22194 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -121,6 +121,26 @@ void of_device_make_bus_id(struct device *dev)
>>>  	dev_set_name(dev, "%s.%d", node->name, magic - 1);
>>>  }
>>>  
>>> +static u64* of_get_dma_mask(struct device_node *np)
>>> +{
>>> +	const __be32 *prop;
>>> +	int len;
>>> +	u64 *dma_mask;
>>> +
>>> +	prop = of_get_property(np, "dma-mask", &len);
>>
>> This would need some documentation. There is already "dma-ranges"
>> defined for OF which nay do what's needed.
> what is dma-ranges I see no doc about it
> but I don't think it could be used for the dma mask

See the ePAPR spec. It's a PowerPC doc, but parts still apply.

https://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.1.pdf

There may also be something in OpenFirmware specs.

>>
>>> +
>>> +	if (!prop)
>>> +		return NULL;
>>> +
>>> +	dma_mask = kzalloc(sizeof(u64), GFP_KERNEL);
>>
>> This seems kind of wasteful for 1 u64.
> no choice dma_mask must be a pointer
>>
>>> +	if (!dma_mask)
>>> +		return NULL;
>>> +
>>> +	*dma_mask = of_read_number(prop, len / 4);
>>> +
>>> +	return dma_mask;
>>> +}
>>> +
>>>  /**
>>>   * of_device_alloc - Allocate and initialize an of_device
>>>   * @np: device node to assign to device
>>> @@ -161,10 +181,14 @@ struct platform_device *of_device_alloc(struct device_node *np,
>>>  		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
>>>  	}
>>>  
>>> +	dev->dev.dma_mask = of_get_dma_mask(np);
>>>  	dev->dev.of_node = of_node_get(np);
>>> +
>>>  #if defined(CONFIG_MICROBLAZE)
>>> -	dev->dev.dma_mask = &dev->archdata.dma_mask;
>>
>> And doing it this way for didn't get a warm reception either:
>>
>> http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-April/005285.html
>>
>> Do you really need this to be something other than ~0UL and if so does
>> it need to be per device or system wide?
>>
>> You can solve this with bus notifiers in your at91 code. Here's an
>> example which I did (but no longer need):
>>
>> static u64 highbank_dma_mask = DMA_BIT_MASK(32);
> IIRC on at91 all of them that need it have a DMA_BIT_MASK(32)
> 
> 
>>
>> static int highbank_platform_notifier(struct notifier_block *nb,
>> 				  unsigned long event, void *__dev)
>> {
>> 	struct device *dev = __dev;
>>
>> 	if (event != BUS_NOTIFY_ADD_DEVICE)
>> 		return NOTIFY_DONE;
>>
>> 	if (of_device_is_compatible(dev->of_node, "calxeda,hb-nfc"))
>> 		dev->dma_mask = &highbank_dma_mask;
> execpt as example today I need it on 2 node (OHCI & EHCI)

I don't think there is any reason you can't point to a single value if a
system wide setting fits. If not, using a per device value here is
easily doable.

> 
> and if more device need I don't want to end up with more fixup
> 
> I really prefer to set it via DT

You don't have any h/w restrictions here and most ARM platforms don't,
so really this doesn't need to be solved with DT for the default case.

Rob

[PATCH 1/1] of: add dma-mask binding

[Grant Likely]

[cc'ing Russell and Ben because my dma-mask-foo is weak]

On Wed, 7 Mar 2012 18:34:37 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> On 10:59 Wed 07 Mar     , Rob Herring wrote:
> > On 03/07/2012 05:26 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > This will allow each device to specify its dma-mask
> > > The microblaze architecture hook is keep temporary if no dma-mask is specified
> > > int the DT
> > > 
> > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > > ---
> > >  drivers/of/platform.c |   26 +++++++++++++++++++++++++-
> > >  1 files changed, 25 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > index cae9477..bb22194 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -121,6 +121,26 @@ void of_device_make_bus_id(struct device *dev)
> > >  	dev_set_name(dev, "%s.%d", node->name, magic - 1);
> > >  }
> > >  
> > > +static u64* of_get_dma_mask(struct device_node *np)
> > > +{
> > > +	const __be32 *prop;
> > > +	int len;
> > > +	u64 *dma_mask;
> > > +
> > > +	prop = of_get_property(np, "dma-mask", &len);
> > 
> > This would need some documentation. There is already "dma-ranges"
> > defined for OF which nay do what's needed.
> what is dma-ranges I see no doc about it
> but I don't think it could be used for the dma mask

As Rob says, it's documented in ePAPR.

> > 
> > > +
> > > +	if (!prop)
> > > +		return NULL;
> > > +
> > > +	dma_mask = kzalloc(sizeof(u64), GFP_KERNEL);
> > 
> > This seems kind of wasteful for 1 u64.
> no choice dma_mask must be a pointer

We could merely add an empty dma_mask u64 to the containing struct
platform_device and then assign the pointer to dev.dma_mask if it is
needed.  That would eliminate the malloc.  Or we could by default
use dev->dev.dma_mask = &dev->dev.coherent_dma_mask;  Drivers or
notifier code could override whatever we setup here as the default.

dma-ranges really is the property that should be used for this, but
dma-ranges can describe a different set of permutations than dma_mask.
The former can have any number of valid DMA windows, but dma_mask
is based on per-address-line values.  (The whole dma_mask approach
appears insufficient to me, but I don't understand all the background
details as to why it is implemented that way).

I'd like to avoid defining a new mechanism for describing the valid
dma layout if at all possible.  This could be done with a new function
to fetch and parse each of the dma-ranges tuples and extract the
intersect of the ranges.  It will never be a perfect translation,
particularly because RAM doesn't start at 0 on a lot of platforms.

So, let's call the base of ram X, and the top of ram X+S.  Then let's
assume that dma_mask is intended to filter out high address (instead
of broken DMA low addresses) and that the DMA limitations are based
on attached address lines, not arbitrary start-end addresses, so if
we let number of address lines be 'A', then dma_mask = 2^A-1
(ideally).  Individual bits could also be blanked out, but what that
really means in a practical sense is that those address lines must be
0.  There isn't a way to constrain address bits that must be set to 1.

Okay, so if we assume that linux will never choose a dma address below
X (because there is no RAM there; except in the iommu case which can
put dma addresses where it needs to regardless), then we only need to
worry about the upper range for a device.  We could derive a dma_mask
by calling of_translate_address on the base address of each dma-ranges
tuple and finding the one that starts at or below the base of memory.
Then the default dma_mask can perhaps be calculated thusly (using 'Y'
for top of the chosen dma-range tuple):

	dma_mask = (2 ^ fls64(Y)) - 1;

Alternately, we could simply decide to do as you suggest and define a
dma-mask property because that approach is sufficient for a lot of
hardware and is therefore is a reasonable binding.

Thoughts?

> > 
> > > +	if (!dma_mask)
> > > +		return NULL;
> > > +
> > > +	*dma_mask = of_read_number(prop, len / 4);
> > > +
> > > +	return dma_mask;
> > > +}
> > > +
> > >  /**
> > >   * of_device_alloc - Allocate and initialize an of_device
> > >   * @np: device node to assign to device
> > > @@ -161,10 +181,14 @@ struct platform_device *of_device_alloc(struct device_node *np,
> > >  		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
> > >  	}
> > >  
> > > +	dev->dev.dma_mask = of_get_dma_mask(np);
> > >  	dev->dev.of_node = of_node_get(np);
> > > +
> > >  #if defined(CONFIG_MICROBLAZE)
> > > -	dev->dev.dma_mask = &dev->archdata.dma_mask;
> > 
> > And doing it this way for didn't get a warm reception either:
> > 
> > http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-April/005285.html
> > 
> > Do you really need this to be something other than ~0UL and if so does
> > it need to be per device or system wide?
> > 
> > You can solve this with bus notifiers in your at91 code. Here's an
> > example which I did (but no longer need):
> > 
> > static u64 highbank_dma_mask = DMA_BIT_MASK(32);
> IIRC on at91 all of them that need it have a DMA_BIT_MASK(32)
> 
> 
> > 
> > static int highbank_platform_notifier(struct notifier_block *nb,
> > 				  unsigned long event, void *__dev)
> > {
> > 	struct device *dev = __dev;
> > 
> > 	if (event != BUS_NOTIFY_ADD_DEVICE)
> > 		return NOTIFY_DONE;
> > 
> > 	if (of_device_is_compatible(dev->of_node, "calxeda,hb-nfc"))
> > 		dev->dma_mask = &highbank_dma_mask;
> execpt as example today I need it on 2 node (OHCI & EHCI)
> 
> and if more device need I don't want to end up with more fixup
> 
> I really prefer to set it via DT
> 
> Best Regards,
> J.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
email sent from notmuch.vim plugin

[PATCH 1/1] of: add dma-mask binding

[Benjamin Herrenschmidt]

On Thu, 2012-03-08 at 17:59 -0700, Grant Likely wrote:

> > > This would need some documentation. There is already "dma-ranges"
> > > defined for OF which nay do what's needed.
> > what is dma-ranges I see no doc about it
> > but I don't think it could be used for the dma mask
> 
> As Rob says, it's documented in ePAPR.

dma-ranges is generally defined for bridges to indicate the dma window
to system memory, it's not been by actual devices so far. It provides
the opposite of "ranges", ie a translation from children to parent
address space.

So it's not quite the same thing.

Now the fact that we are limited to power-of-2 masks is a Linux
implementation detail. We might want to make the device-tree a bit more
flexible and use something like dma-limit instead. However hardware
limits tend to be in term of number of bits anyways so it's not a big
issue.

I don't see a need to keep a "ranges" type of semantic. If we ever do
that we can do a new binding for it or add a dma-base...

> We could merely add an empty dma_mask u64 to the containing struct
> platform_device and then assign the pointer to dev.dma_mask if it is
> needed.  That would eliminate the malloc.

Agreed.

>   Or we could by default
> use dev->dev.dma_mask = &dev->dev.coherent_dma_mask;  Drivers or
> notifier code could override whatever we setup here as the default.
> 
> dma-ranges really is the property that should be used for this, but
> dma-ranges can describe a different set of permutations than dma_mask.

I don't think so. As I said, dma-ranges is about a bridge/bus exposing
DMA windows & translations. It has nothing to do with the device
intrinsic DMA'ing capabilities.
 
Cheers,
Ben.