[U-Boot] [PATCH 1/3] board_f: Add mach specific DMA address check function.

[U-Boot] [PATCH 1/3] board_f: Add mach specific DMA address check function.

[Christoph Muellner]

From: Christoph Müllner <christoph.muellner@theobroma-systems.com>

Some machines have limited DMA engines, which cannot deal
with arbitrary addresses. This patch introduces a function
to model these restrictions on a machine level.

Signed-off-by: Christoph Müllner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---

 common/board_f.c | 5 +++++
 include/init.h   | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/common/board_f.c b/common/board_f.c
index 7ef20f2042..fed3c24373 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -1053,3 +1053,8 @@ void board_init_f_r(void)
 	hang();
 }
 #endif /* CONFIG_X86 */
+
+__weak int mach_addr_is_dmaable(unsigned long addr)
+{
+	return 1;
+}
diff --git a/include/init.h b/include/init.h
index afc953d51e..18210a1489 100644
--- a/include/init.h
+++ b/include/init.h
@@ -125,6 +125,8 @@ int misc_init_f(void);
 int embedded_dtb_select(void);
 #endif
 
+int mach_addr_is_dmaable(unsigned long addr);
+
 /* common/init/board_init.c */
 extern ulong monitor_flash_len;
 
-- 
2.11.0

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

[Christoph Muellner]

Currently addr_aligned() performs an alignment and a length check
to validate the DMA address. However, some machines have stricter
restrictions of DMA-able addresses.

This patch adds a call to mach_addr_is_dmaable() to honor this
machine specific restrictions.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---

 common/bouncebuf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/common/bouncebuf.c b/common/bouncebuf.c
index a7098e2caf..26ddf30ea2 100644
--- a/common/bouncebuf.c
+++ b/common/bouncebuf.c
@@ -26,6 +26,12 @@ static int addr_aligned(struct bounce_buffer *state)
 		return 0;
 	}
 
+	/* Check if valid DMA address. */
+	if (!mach_addr_is_dmaable((ulong)state->user_buffer)) {
+		debug("Buffer address is not DMA-able\n");
+		return 0;
+	}
+
 	/* Aligned */
 	return 1;
 }
-- 
2.11.0

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

[Marek Vasut]

On 5/7/19 11:05 AM, Christoph Muellner wrote:
> Currently addr_aligned() performs an alignment and a length check
> to validate the DMA address. However, some machines have stricter
> restrictions of DMA-able addresses.
> 
> This patch adds a call to mach_addr_is_dmaable() to honor this
> machine specific restrictions.
> 
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> ---
> 
>  common/bouncebuf.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
> index a7098e2caf..26ddf30ea2 100644
> --- a/common/bouncebuf.c
> +++ b/common/bouncebuf.c
> @@ -26,6 +26,12 @@ static int addr_aligned(struct bounce_buffer *state)
>  		return 0;
>  	}
>  
> +	/* Check if valid DMA address. */
> +	if (!mach_addr_is_dmaable((ulong)state->user_buffer)) {

Is the cast necessary ?

> +		debug("Buffer address is not DMA-able\n");
> +		return 0;
> +	}
> +
>  	/* Aligned */
>  	return 1;
>  }
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

[Christoph Müllner]

On 07.05.19 15:05, Marek Vasut wrote:
> On 5/7/19 11:05 AM, Christoph Muellner wrote:
>> Currently addr_aligned() performs an alignment and a length check
>> to validate the DMA address. However, some machines have stricter
>> restrictions of DMA-able addresses.
>>
>> This patch adds a call to mach_addr_is_dmaable() to honor this
>> machine specific restrictions.
>>
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> ---
>>
>>  common/bouncebuf.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
>> index a7098e2caf..26ddf30ea2 100644
>> --- a/common/bouncebuf.c
>> +++ b/common/bouncebuf.c
>> @@ -26,6 +26,12 @@ static int addr_aligned(struct bounce_buffer *state)
>>  		return 0;
>>  	}
>>  
>> +	/* Check if valid DMA address. */
>> +	if (!mach_addr_is_dmaable((ulong)state->user_buffer)) {
> 
> Is the cast necessary ?

Of course not.
Will be fixed in v2.

Thanks!

> 
>> +		debug("Buffer address is not DMA-able\n");
>> +		return 0;
>> +	}
>> +
>>  	/* Aligned */
>>  	return 1;
>>  }
>>
> 
> 

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

[Christoph Müllner]

On 5/7/19 3:48 PM, Christoph Müllner wrote:
> 
> 
> On 07.05.19 15:05, Marek Vasut wrote:
>> On 5/7/19 11:05 AM, Christoph Muellner wrote:
>>> Currently addr_aligned() performs an alignment and a length check
>>> to validate the DMA address. However, some machines have stricter
>>> restrictions of DMA-able addresses.
>>>
>>> This patch adds a call to mach_addr_is_dmaable() to honor this
>>> machine specific restrictions.
>>>
>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>> ---
>>>
>>>  common/bouncebuf.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
>>> index a7098e2caf..26ddf30ea2 100644
>>> --- a/common/bouncebuf.c
>>> +++ b/common/bouncebuf.c
>>> @@ -26,6 +26,12 @@ static int addr_aligned(struct bounce_buffer *state)
>>>  		return 0;
>>>  	}
>>>  
>>> +	/* Check if valid DMA address. */
>>> +	if (!mach_addr_is_dmaable((ulong)state->user_buffer)) {
>>
>> Is the cast necessary ?
> 
> Of course not.
> Will be fixed in v2.
> 
> Thanks!

I take that back.
The cast is needed.
(And also done several times in this file)

Otherwise you will get:

common/bouncebuf.c: In function ‘addr_aligned’:
common/bouncebuf.c:35:33: warning: passing argument 1 of ‘mach_addr_is_dmaable’ makes integer from pointer without a cast [-Wint-conversion]
  if (!mach_addr_is_dmaable(state->user_buffer)) {
                            ~~~~~^~~~~~~~~~~~~
In file included from include/common.h:64,
                 from common/bouncebuf.c:8:
include/init.h:128:40: note: expected ‘long unsigned int’ but argument is of type ‘void *’
 int mach_addr_is_dmaable(unsigned long addr);
                          ~~~~~~~~~~~~~~^~~~





> 
>>
>>> +		debug("Buffer address is not DMA-able\n");
>>> +		return 0;
>>> +	}
>>> +
>>>  	/* Aligned */
>>>  	return 1;
>>>  }
>>>
>>
>>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
> 

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

[Marek Vasut]

On 5/7/19 3:52 PM, Christoph Müllner wrote:
> 
> 
> On 5/7/19 3:48 PM, Christoph Müllner wrote:
>>
>>
>> On 07.05.19 15:05, Marek Vasut wrote:
>>> On 5/7/19 11:05 AM, Christoph Muellner wrote:
>>>> Currently addr_aligned() performs an alignment and a length check
>>>> to validate the DMA address. However, some machines have stricter
>>>> restrictions of DMA-able addresses.
>>>>
>>>> This patch adds a call to mach_addr_is_dmaable() to honor this
>>>> machine specific restrictions.
>>>>
>>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>>> ---
>>>>
>>>>  common/bouncebuf.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
>>>> index a7098e2caf..26ddf30ea2 100644
>>>> --- a/common/bouncebuf.c
>>>> +++ b/common/bouncebuf.c
>>>> @@ -26,6 +26,12 @@ static int addr_aligned(struct bounce_buffer *state)
>>>>  		return 0;
>>>>  	}
>>>>  
>>>> +	/* Check if valid DMA address. */
>>>> +	if (!mach_addr_is_dmaable((ulong)state->user_buffer)) {
>>>
>>> Is the cast necessary ?
>>
>> Of course not.
>> Will be fixed in v2.
>>
>> Thanks!
> 
> I take that back.
> The cast is needed.
> (And also done several times in this file)
> 
> Otherwise you will get:
> 
> common/bouncebuf.c: In function ‘addr_aligned’:
> common/bouncebuf.c:35:33: warning: passing argument 1 of ‘mach_addr_is_dmaable’ makes integer from pointer without a cast [-Wint-conversion]
>   if (!mach_addr_is_dmaable(state->user_buffer)) {
>                             ~~~~~^~~~~~~~~~~~~
> In file included from include/common.h:64,
>                  from common/bouncebuf.c:8:
> include/init.h:128:40: note: expected ‘long unsigned int’ but argument is of type ‘void *’
>  int mach_addr_is_dmaable(unsigned long addr);
>                           ~~~~~~~~~~~~~~^~~~

Shouldn't the function use void __iomem * in the first place ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

[Christoph Müllner]

On 07.05.19 15:53, Marek Vasut wrote:
> On 5/7/19 3:52 PM, Christoph Müllner wrote:
>>
>>
>> On 5/7/19 3:48 PM, Christoph Müllner wrote:
>>>
>>>
>>> On 07.05.19 15:05, Marek Vasut wrote:
>>>> On 5/7/19 11:05 AM, Christoph Muellner wrote:
>>>>> Currently addr_aligned() performs an alignment and a length check
>>>>> to validate the DMA address. However, some machines have stricter
>>>>> restrictions of DMA-able addresses.
>>>>>
>>>>> This patch adds a call to mach_addr_is_dmaable() to honor this
>>>>> machine specific restrictions.
>>>>>
>>>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>>>> ---
>>>>>
>>>>>  common/bouncebuf.c | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
>>>>> index a7098e2caf..26ddf30ea2 100644
>>>>> --- a/common/bouncebuf.c
>>>>> +++ b/common/bouncebuf.c
>>>>> @@ -26,6 +26,12 @@ static int addr_aligned(struct bounce_buffer *state)
>>>>>  		return 0;
>>>>>  	}
>>>>>  
>>>>> +	/* Check if valid DMA address. */
>>>>> +	if (!mach_addr_is_dmaable((ulong)state->user_buffer)) {
>>>>
>>>> Is the cast necessary ?
>>>
>>> Of course not.
>>> Will be fixed in v2.
>>>
>>> Thanks!
>>
>> I take that back.
>> The cast is needed.
>> (And also done several times in this file)
>>
>> Otherwise you will get:
>>
>> common/bouncebuf.c: In function ‘addr_aligned’:
>> common/bouncebuf.c:35:33: warning: passing argument 1 of ‘mach_addr_is_dmaable’ makes integer from pointer without a cast [-Wint-conversion]
>>   if (!mach_addr_is_dmaable(state->user_buffer)) {
>>                             ~~~~~^~~~~~~~~~~~~
>> In file included from include/common.h:64,
>>                  from common/bouncebuf.c:8:
>> include/init.h:128:40: note: expected ‘long unsigned int’ but argument is of type ‘void *’
>>  int mach_addr_is_dmaable(unsigned long addr);
>>                           ~~~~~~~~~~~~~~^~~~
> 
> Shouldn't the function use void __iomem * in the first place ?

IMHO Matter of taste.
If you prefer it this way, then I can change.
Doing grep "unsigned long addr" gave me enough confidence to use that.
But I leave this up to you.

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

[Marek Vasut]

On 5/7/19 4:01 PM, Christoph Müllner wrote:
> 
> 
> On 07.05.19 15:53, Marek Vasut wrote:
>> On 5/7/19 3:52 PM, Christoph Müllner wrote:
>>>
>>>
>>> On 5/7/19 3:48 PM, Christoph Müllner wrote:
>>>>
>>>>
>>>> On 07.05.19 15:05, Marek Vasut wrote:
>>>>> On 5/7/19 11:05 AM, Christoph Muellner wrote:
>>>>>> Currently addr_aligned() performs an alignment and a length check
>>>>>> to validate the DMA address. However, some machines have stricter
>>>>>> restrictions of DMA-able addresses.
>>>>>>
>>>>>> This patch adds a call to mach_addr_is_dmaable() to honor this
>>>>>> machine specific restrictions.
>>>>>>
>>>>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>>>>> ---
>>>>>>
>>>>>>  common/bouncebuf.c | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
>>>>>> index a7098e2caf..26ddf30ea2 100644
>>>>>> --- a/common/bouncebuf.c
>>>>>> +++ b/common/bouncebuf.c
>>>>>> @@ -26,6 +26,12 @@ static int addr_aligned(struct bounce_buffer *state)
>>>>>>  		return 0;
>>>>>>  	}
>>>>>>  
>>>>>> +	/* Check if valid DMA address. */
>>>>>> +	if (!mach_addr_is_dmaable((ulong)state->user_buffer)) {
>>>>>
>>>>> Is the cast necessary ?
>>>>
>>>> Of course not.
>>>> Will be fixed in v2.
>>>>
>>>> Thanks!
>>>
>>> I take that back.
>>> The cast is needed.
>>> (And also done several times in this file)
>>>
>>> Otherwise you will get:
>>>
>>> common/bouncebuf.c: In function ‘addr_aligned’:
>>> common/bouncebuf.c:35:33: warning: passing argument 1 of ‘mach_addr_is_dmaable’ makes integer from pointer without a cast [-Wint-conversion]
>>>   if (!mach_addr_is_dmaable(state->user_buffer)) {
>>>                             ~~~~~^~~~~~~~~~~~~
>>> In file included from include/common.h:64,
>>>                  from common/bouncebuf.c:8:
>>> include/init.h:128:40: note: expected ‘long unsigned int’ but argument is of type ‘void *’
>>>  int mach_addr_is_dmaable(unsigned long addr);
>>>                           ~~~~~~~~~~~~~~^~~~
>>
>> Shouldn't the function use void __iomem * in the first place ?
> 
> IMHO Matter of taste.
> If you prefer it this way, then I can change.
> Doing grep "unsigned long addr" gave me enough confidence to use that.
> But I leave this up to you.

At least that's what Linux uses for mapped addresses.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

[Christoph Müllner]

On 07.05.19 17:04, Marek Vasut wrote:
> On 5/7/19 4:01 PM, Christoph Müllner wrote:
>>
>>
>> On 07.05.19 15:53, Marek Vasut wrote:
>>> On 5/7/19 3:52 PM, Christoph Müllner wrote:
>>>>
>>>>
>>>> On 5/7/19 3:48 PM, Christoph Müllner wrote:
>>>>>
>>>>>
>>>>> On 07.05.19 15:05, Marek Vasut wrote:
>>>>>> On 5/7/19 11:05 AM, Christoph Muellner wrote:
>>>>>>> Currently addr_aligned() performs an alignment and a length check
>>>>>>> to validate the DMA address. However, some machines have stricter
>>>>>>> restrictions of DMA-able addresses.
>>>>>>>
>>>>>>> This patch adds a call to mach_addr_is_dmaable() to honor this
>>>>>>> machine specific restrictions.
>>>>>>>
>>>>>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>>>>>> ---
>>>>>>>
>>>>>>>  common/bouncebuf.c | 6 ++++++
>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>
>>>>>>> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
>>>>>>> index a7098e2caf..26ddf30ea2 100644
>>>>>>> --- a/common/bouncebuf.c
>>>>>>> +++ b/common/bouncebuf.c
>>>>>>> @@ -26,6 +26,12 @@ static int addr_aligned(struct bounce_buffer *state)
>>>>>>>  		return 0;
>>>>>>>  	}
>>>>>>>  
>>>>>>> +	/* Check if valid DMA address. */
>>>>>>> +	if (!mach_addr_is_dmaable((ulong)state->user_buffer)) {
>>>>>>
>>>>>> Is the cast necessary ?
>>>>>
>>>>> Of course not.
>>>>> Will be fixed in v2.
>>>>>
>>>>> Thanks!
>>>>
>>>> I take that back.
>>>> The cast is needed.
>>>> (And also done several times in this file)
>>>>
>>>> Otherwise you will get:
>>>>
>>>> common/bouncebuf.c: In function ‘addr_aligned’:
>>>> common/bouncebuf.c:35:33: warning: passing argument 1 of ‘mach_addr_is_dmaable’ makes integer from pointer without a cast [-Wint-conversion]
>>>>   if (!mach_addr_is_dmaable(state->user_buffer)) {
>>>>                             ~~~~~^~~~~~~~~~~~~
>>>> In file included from include/common.h:64,
>>>>                  from common/bouncebuf.c:8:
>>>> include/init.h:128:40: note: expected ‘long unsigned int’ but argument is of type ‘void *’
>>>>  int mach_addr_is_dmaable(unsigned long addr);
>>>>                           ~~~~~~~~~~~~~~^~~~
>>>
>>> Shouldn't the function use void __iomem * in the first place ?
>>
>> IMHO Matter of taste.
>> If you prefer it this way, then I can change.
>> Doing grep "unsigned long addr" gave me enough confidence to use that.
>> But I leave this up to you.
> 
> At least that's what Linux uses for mapped addresses.

In Linux two versions of that function are needed: one for phys_addr_t and one for void *.
The linear mapping in U-Boot allows to combine both.
Therefore I see it as a matter of taste.

I will change this to "void __iomem *" and cast to uintptr_t in the rk3399
implementation of that function in a v2 series.

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

[Marek Vasut]

On 5/7/19 5:22 PM, Christoph Müllner wrote:
> 
> 
> On 07.05.19 17:04, Marek Vasut wrote:
>> On 5/7/19 4:01 PM, Christoph Müllner wrote:
>>>
>>>
>>> On 07.05.19 15:53, Marek Vasut wrote:
>>>> On 5/7/19 3:52 PM, Christoph Müllner wrote:
>>>>>
>>>>>
>>>>> On 5/7/19 3:48 PM, Christoph Müllner wrote:
>>>>>>
>>>>>>
>>>>>> On 07.05.19 15:05, Marek Vasut wrote:
>>>>>>> On 5/7/19 11:05 AM, Christoph Muellner wrote:
>>>>>>>> Currently addr_aligned() performs an alignment and a length check
>>>>>>>> to validate the DMA address. However, some machines have stricter
>>>>>>>> restrictions of DMA-able addresses.
>>>>>>>>
>>>>>>>> This patch adds a call to mach_addr_is_dmaable() to honor this
>>>>>>>> machine specific restrictions.
>>>>>>>>
>>>>>>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>  common/bouncebuf.c | 6 ++++++
>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
>>>>>>>> index a7098e2caf..26ddf30ea2 100644
>>>>>>>> --- a/common/bouncebuf.c
>>>>>>>> +++ b/common/bouncebuf.c
>>>>>>>> @@ -26,6 +26,12 @@ static int addr_aligned(struct bounce_buffer *state)
>>>>>>>>  		return 0;
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>> +	/* Check if valid DMA address. */
>>>>>>>> +	if (!mach_addr_is_dmaable((ulong)state->user_buffer)) {
>>>>>>>
>>>>>>> Is the cast necessary ?
>>>>>>
>>>>>> Of course not.
>>>>>> Will be fixed in v2.
>>>>>>
>>>>>> Thanks!
>>>>>
>>>>> I take that back.
>>>>> The cast is needed.
>>>>> (And also done several times in this file)
>>>>>
>>>>> Otherwise you will get:
>>>>>
>>>>> common/bouncebuf.c: In function ‘addr_aligned’:
>>>>> common/bouncebuf.c:35:33: warning: passing argument 1 of ‘mach_addr_is_dmaable’ makes integer from pointer without a cast [-Wint-conversion]
>>>>>   if (!mach_addr_is_dmaable(state->user_buffer)) {
>>>>>                             ~~~~~^~~~~~~~~~~~~
>>>>> In file included from include/common.h:64,
>>>>>                  from common/bouncebuf.c:8:
>>>>> include/init.h:128:40: note: expected ‘long unsigned int’ but argument is of type ‘void *’
>>>>>  int mach_addr_is_dmaable(unsigned long addr);
>>>>>                           ~~~~~~~~~~~~~~^~~~
>>>>
>>>> Shouldn't the function use void __iomem * in the first place ?
>>>
>>> IMHO Matter of taste.
>>> If you prefer it this way, then I can change.
>>> Doing grep "unsigned long addr" gave me enough confidence to use that.
>>> But I leave this up to you.
>>
>> At least that's what Linux uses for mapped addresses.
> 
> In Linux two versions of that function are needed: one for phys_addr_t and one for void *.
> The linear mapping in U-Boot allows to combine both.
> Therefore I see it as a matter of taste.
> 
> I will change this to "void __iomem *" and cast to uintptr_t in the rk3399
> implementation of that function in a v2 series.

Wait for more feedback please. Maybe someone has a more compelling
argument for one or the other.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

[Christoph Müllner]

On 07.05.19 17:56, Marek Vasut wrote:
> On 5/7/19 5:22 PM, Christoph Müllner wrote:
>>
>>
>> On 07.05.19 17:04, Marek Vasut wrote:
>>> On 5/7/19 4:01 PM, Christoph Müllner wrote:
>>>>
>>>>
>>>> On 07.05.19 15:53, Marek Vasut wrote:
>>>>> On 5/7/19 3:52 PM, Christoph Müllner wrote:
>>>>>>
>>>>>>
>>>>>> On 5/7/19 3:48 PM, Christoph Müllner wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 07.05.19 15:05, Marek Vasut wrote:
>>>>>>>> On 5/7/19 11:05 AM, Christoph Muellner wrote:
>>>>>>>>> Currently addr_aligned() performs an alignment and a length check
>>>>>>>>> to validate the DMA address. However, some machines have stricter
>>>>>>>>> restrictions of DMA-able addresses.
>>>>>>>>>
>>>>>>>>> This patch adds a call to mach_addr_is_dmaable() to honor this
>>>>>>>>> machine specific restrictions.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>  common/bouncebuf.c | 6 ++++++
>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
>>>>>>>>> index a7098e2caf..26ddf30ea2 100644
>>>>>>>>> --- a/common/bouncebuf.c
>>>>>>>>> +++ b/common/bouncebuf.c
>>>>>>>>> @@ -26,6 +26,12 @@ static int addr_aligned(struct bounce_buffer *state)
>>>>>>>>>  		return 0;
>>>>>>>>>  	}
>>>>>>>>>  
>>>>>>>>> +	/* Check if valid DMA address. */
>>>>>>>>> +	if (!mach_addr_is_dmaable((ulong)state->user_buffer)) {
>>>>>>>>
>>>>>>>> Is the cast necessary ?
>>>>>>>
>>>>>>> Of course not.
>>>>>>> Will be fixed in v2.
>>>>>>>
>>>>>>> Thanks!
>>>>>>
>>>>>> I take that back.
>>>>>> The cast is needed.
>>>>>> (And also done several times in this file)
>>>>>>
>>>>>> Otherwise you will get:
>>>>>>
>>>>>> common/bouncebuf.c: In function ‘addr_aligned’:
>>>>>> common/bouncebuf.c:35:33: warning: passing argument 1 of ‘mach_addr_is_dmaable’ makes integer from pointer without a cast [-Wint-conversion]
>>>>>>   if (!mach_addr_is_dmaable(state->user_buffer)) {
>>>>>>                             ~~~~~^~~~~~~~~~~~~
>>>>>> In file included from include/common.h:64,
>>>>>>                  from common/bouncebuf.c:8:
>>>>>> include/init.h:128:40: note: expected ‘long unsigned int’ but argument is of type ‘void *’
>>>>>>  int mach_addr_is_dmaable(unsigned long addr);
>>>>>>                           ~~~~~~~~~~~~~~^~~~
>>>>>
>>>>> Shouldn't the function use void __iomem * in the first place ?
>>>>
>>>> IMHO Matter of taste.
>>>> If you prefer it this way, then I can change.
>>>> Doing grep "unsigned long addr" gave me enough confidence to use that.
>>>> But I leave this up to you.
>>>
>>> At least that's what Linux uses for mapped addresses.
>>
>> In Linux two versions of that function are needed: one for phys_addr_t and one for void *.
>> The linear mapping in U-Boot allows to combine both.
>> Therefore I see it as a matter of taste.
>>
>> I will change this to "void __iomem *" and cast to uintptr_t in the rk3399
>> implementation of that function in a v2 series.
> 
> Wait for more feedback please. Maybe someone has a more compelling
> argument for one or the other.

Too late, but I have no fear from a v3.

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

[Simon Glass]

Hi Christoph,

On Tue, 7 May 2019 at 03:05, Christoph Muellner
<christoph.muellner@theobroma-systems.com> wrote:
>
> Currently addr_aligned() performs an alignment and a length check
> to validate the DMA address. However, some machines have stricter
> restrictions of DMA-able addresses.
>
> This patch adds a call to mach_addr_is_dmaable() to honor this
> machine specific restrictions.
>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> ---
>
>  common/bouncebuf.c | 6 ++++++
>  1 file changed, 6 insertions(+)

I feel like this should be handled with DM. Can we add a new method to
the DMA uclass to check an address? If not provided by the DMA driver,
we can assume the address is OK.

Regards,
Simon

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().【请注意，邮件由u-boot-bounces@lists.denx.de代发】 addr_aligned().

[Kever Yang]

Hi Simon,


On 05/19/2019 12:08 AM, Simon Glass wrote:
> Hi Christoph,
>
> On Tue, 7 May 2019 at 03:05, Christoph Muellner
> <christoph.muellner@theobroma-systems.com> wrote:
>> Currently addr_aligned() performs an alignment and a length check
>> to validate the DMA address. However, some machines have stricter
>> restrictions of DMA-able addresses.
>>
>> This patch adds a call to mach_addr_is_dmaable() to honor this
>> machine specific restrictions.
>>
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> ---
>>
>>  common/bouncebuf.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
> I feel like this should be handled with DM. Can we add a new method to
> the DMA uclass to check an address? If not provided by the DMA driver,
> we can assume the address is OK.

The DMA in MMC controller which is not stand alone, do not using the
driver of DMA uclass, so I'm afraid this is not able to using DMA uclass
for this address check.

Thanks,
- Kever
>
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().【请注意，邮件由u-boot-bounces@lists.denx.de代发】 addr_aligned().

[Heiko Stübner]

Hi Simon,

Am Dienstag, 4. Juni 2019, 05:23:14 CEST schrieb Kever Yang:
> On 05/19/2019 12:08 AM, Simon Glass wrote:
> > On Tue, 7 May 2019 at 03:05, Christoph Muellner
> > <christoph.muellner@theobroma-systems.com> wrote:
> >> Currently addr_aligned() performs an alignment and a length check
> >> to validate the DMA address. However, some machines have stricter
> >> restrictions of DMA-able addresses.
> >>
> >> This patch adds a call to mach_addr_is_dmaable() to honor this
> >> machine specific restrictions.
> >>
> >> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> >> ---
> >>
> >>  common/bouncebuf.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> > I feel like this should be handled with DM. Can we add a new method to
> > the DMA uclass to check an address? If not provided by the DMA driver,
> > we can assume the address is OK.
> 
> The DMA in MMC controller which is not stand alone, do not using the
> driver of DMA uclass, so I'm afraid this is not able to using DMA uclass
> for this address check.

were you able to consider Kever's response?

The issue bites us for example when the mmc-driver with its internal dma
does transfer atf loadables to the socs sram. There is no system dma
controller involved but so far we're experiencing this on _all_ Rockchip
socs that need multiple parts of the ATF be written to different memory
locations. (the sram code is obviously needed for suspend/resume).


Thanks
Heiko

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().【请注意，邮件由u-boot-bounces@lists.denx.de代发】 addr_aligned().

[Simon Glass]

Hi,

On Thu, 4 Jul 2019 at 14:43, Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi Simon,
>
> Am Dienstag, 4. Juni 2019, 05:23:14 CEST schrieb Kever Yang:
> > On 05/19/2019 12:08 AM, Simon Glass wrote:
> > > On Tue, 7 May 2019 at 03:05, Christoph Muellner
> > > <christoph.muellner@theobroma-systems.com> wrote:
> > >> Currently addr_aligned() performs an alignment and a length check
> > >> to validate the DMA address. However, some machines have stricter
> > >> restrictions of DMA-able addresses.
> > >>
> > >> This patch adds a call to mach_addr_is_dmaable() to honor this
> > >> machine specific restrictions.
> > >>
> > >> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> > >> ---
> > >>
> > >>  common/bouncebuf.c | 6 ++++++
> > >>  1 file changed, 6 insertions(+)
> > > I feel like this should be handled with DM. Can we add a new method to
> > > the DMA uclass to check an address? If not provided by the DMA driver,
> > > we can assume the address is OK.
> >
> > The DMA in MMC controller which is not stand alone, do not using the
> > driver of DMA uclass, so I'm afraid this is not able to using DMA uclass
> > for this address check.
>
> were you able to consider Kever's response?
>
> The issue bites us for example when the mmc-driver with its internal dma
> does transfer atf loadables to the socs sram. There is no system dma
> controller involved but so far we're experiencing this on _all_ Rockchip
> socs that need multiple parts of the ATF be written to different memory
> locations. (the sram code is obviously needed for suspend/resume).

I don't really understand the thing about the internal MMC DMA controller.

Is it not possible to call a DMA driver to validate an address, or to
allocate an address? It isn't necessary to use the driver for a
transfer.

But if this is related to DMA, then it seems to me that the DMA uclass
should support the functionality, perhaps with a new method?

Regards,
Simon

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().【请注意，邮件由u-boot-bounces@lists.denx.de代发】 addr_aligned().

[Heiko Stuebner]

Hi Simon,

Am Samstag, 6. Juli 2019, 19:16:29 CEST schrieb Simon Glass:
> On Thu, 4 Jul 2019 at 14:43, Heiko Stübner <heiko@sntech.de> wrote:
> > Am Dienstag, 4. Juni 2019, 05:23:14 CEST schrieb Kever Yang:
> > > On 05/19/2019 12:08 AM, Simon Glass wrote:
> > > > On Tue, 7 May 2019 at 03:05, Christoph Muellner
> > > > <christoph.muellner@theobroma-systems.com> wrote:
> > > >> Currently addr_aligned() performs an alignment and a length check
> > > >> to validate the DMA address. However, some machines have stricter
> > > >> restrictions of DMA-able addresses.
> > > >>
> > > >> This patch adds a call to mach_addr_is_dmaable() to honor this
> > > >> machine specific restrictions.
> > > >>
> > > >> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> > > >> ---
> > > >>
> > > >>  common/bouncebuf.c | 6 ++++++
> > > >>  1 file changed, 6 insertions(+)
> > > > I feel like this should be handled with DM. Can we add a new method to
> > > > the DMA uclass to check an address? If not provided by the DMA driver,
> > > > we can assume the address is OK.
> > >
> > > The DMA in MMC controller which is not stand alone, do not using the
> > > driver of DMA uclass, so I'm afraid this is not able to using DMA uclass
> > > for this address check.
> >
> > were you able to consider Kever's response?
> >
> > The issue bites us for example when the mmc-driver with its internal dma
> > does transfer atf loadables to the socs sram. There is no system dma
> > controller involved but so far we're experiencing this on _all_ Rockchip
> > socs that need multiple parts of the ATF be written to different memory
> > locations. (the sram code is obviously needed for suspend/resume).
> 
> I don't really understand the thing about the internal MMC DMA controller.

The dw_mmc IP block can either be connected to a system-level
dma controller (like a pl330 on for example the rk3188)
or have its own completely separate dma-controller _inside_ the
dw-mmc ip-block itself (I think all newer Rockchip socs for example).

> Is it not possible to call a DMA driver to validate an address, or to
> allocate an address? It isn't necessary to use the driver for a
> transfer.

For the internal-dma case, there really is no separate dma-driver
involved, all is contained in dw-mmc.

On rk3288 on the kernel-side we also "just" blocked off the offending
memory area due to all other approaches being cumbersome,
with multiple IP blocks all bringing their own dma-controllers
circumventing the system-level ones.

> But if this is related to DMA, then it seems to me that the DMA uclass
> should support the functionality, perhaps with a new method?

So something like
	int dma_address_is_allowed(void *address);
which then calls into a system-specific function (or valus from Kconfig)?
Or do you have something different in mind?


Thanks
Heiko

[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().【请注意，邮件由u-boot-bounces@lists.denx.de代发】 addr_aligned().

[Heiko Stuebner]

Hi Simon,

just pulling up this thread again, as it would be great if we could
resolve how to proceed here.

The important points regarding your questions are below.


Am Samstag, 6. Juli 2019, 20:04:52 CEST schrieb Heiko Stuebner:
> Am Samstag, 6. Juli 2019, 19:16:29 CEST schrieb Simon Glass:
> > On Thu, 4 Jul 2019 at 14:43, Heiko Stübner <heiko@sntech.de> wrote:
> > > Am Dienstag, 4. Juni 2019, 05:23:14 CEST schrieb Kever Yang:
> > > > On 05/19/2019 12:08 AM, Simon Glass wrote:
> > > > > On Tue, 7 May 2019 at 03:05, Christoph Muellner
> > > > > <christoph.muellner@theobroma-systems.com> wrote:
> > > > >> Currently addr_aligned() performs an alignment and a length check
> > > > >> to validate the DMA address. However, some machines have stricter
> > > > >> restrictions of DMA-able addresses.
> > > > >>
> > > > >> This patch adds a call to mach_addr_is_dmaable() to honor this
> > > > >> machine specific restrictions.
> > > > >>
> > > > >> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> > > > >> ---
> > > > >>
> > > > >>  common/bouncebuf.c | 6 ++++++
> > > > >>  1 file changed, 6 insertions(+)
> > > > > I feel like this should be handled with DM. Can we add a new method to
> > > > > the DMA uclass to check an address? If not provided by the DMA driver,
> > > > > we can assume the address is OK.
> > > >
> > > > The DMA in MMC controller which is not stand alone, do not using the
> > > > driver of DMA uclass, so I'm afraid this is not able to using DMA uclass
> > > > for this address check.
> > >
> > > were you able to consider Kever's response?
> > >
> > > The issue bites us for example when the mmc-driver with its internal dma
> > > does transfer atf loadables to the socs sram. There is no system dma
> > > controller involved but so far we're experiencing this on _all_ Rockchip
> > > socs that need multiple parts of the ATF be written to different memory
> > > locations. (the sram code is obviously needed for suspend/resume).
> > 
> > I don't really understand the thing about the internal MMC DMA controller.
>
> The dw_mmc IP block can either be connected to a system-level
> dma controller (like a pl330 on for example the rk3188)
> or have its own completely separate dma-controller _inside_ the
> dw-mmc ip-block itself (I think all newer Rockchip socs for example).
> 
> > Is it not possible to call a DMA driver to validate an address, or to
> > allocate an address? It isn't necessary to use the driver for a
> > transfer.
> 
> For the internal-dma case, there really is no separate dma-driver
> involved, all is contained in dw-mmc.
> 
> On rk3288 on the kernel-side we also "just" blocked off the offending
> memory area due to all other approaches being cumbersome,
> with multiple IP blocks all bringing their own dma-controllers
> circumventing the system-level ones.
> 
> > But if this is related to DMA, then it seems to me that the DMA uclass
> > should support the functionality, perhaps with a new method?
> 
> So something like
> 	int dma_address_is_allowed(void *address);
> which then calls into a system-specific function (or valus from Kconfig)?
> Or do you have something different in mind?


Thanks
Heiko

[U-Boot] [PATCH 3/3] rk3399: Add restriction for DMA-able addresses.

[Christoph Muellner]

Patches on the U-Boot mailing list from Rockchip engineers
indicate, that the RK3399's DMA engines are not able to use
addresses in high-memory (above 0xf8000000).

This patch models this restriction in an RK3399 specific
mach_addr_is_dmaable() function.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---

 arch/arm/mach-rockchip/rk3399/rk3399.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/mach-rockchip/rk3399/rk3399.c b/arch/arm/mach-rockchip/rk3399/rk3399.c
index a7ccd4f3ed..15b03726eb 100644
--- a/arch/arm/mach-rockchip/rk3399/rk3399.c
+++ b/arch/arm/mach-rockchip/rk3399/rk3399.c
@@ -109,3 +109,14 @@ void board_debug_uart_init(void)
 #endif
 }
 #endif
+
+int mach_addr_is_dmaable(unsigned long addr)
+{
+	/*
+	 * The RK3399 cannot cope with high-memory DMA target/sources.
+	 */
+	if (addr < 0xf8000000UL)
+		return 1;
+
+	return 0;
+}
-- 
2.11.0

[U-Boot] [PATCH 3/3] rk3399: Add restriction for DMA-able addresses.

[Kever Yang]

Hi Christoph,


On 05/07/2019 05:05 PM, Christoph Muellner wrote:
> Patches on the U-Boot mailing list from Rockchip engineers
> indicate, that the RK3399's DMA engines are not able to use
> addresses in high-memory (above 0xf8000000).
>
> This patch models this restriction in an RK3399 specific
> mach_addr_is_dmaable() function.
>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> ---
>
>  arch/arm/mach-rockchip/rk3399/rk3399.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm/mach-rockchip/rk3399/rk3399.c b/arch/arm/mach-rockchip/rk3399/rk3399.c
> index a7ccd4f3ed..15b03726eb 100644
> --- a/arch/arm/mach-rockchip/rk3399/rk3399.c
> +++ b/arch/arm/mach-rockchip/rk3399/rk3399.c
> @@ -109,3 +109,14 @@ void board_debug_uart_init(void)
>  #endif
>  }
>  #endif
> +
> +int mach_addr_is_dmaable(unsigned long addr)
> +{
> +	/*
> +	 * The RK3399 cannot cope with high-memory DMA target/sources.
> +	 */
> +	if (addr < 0xf8000000UL)
You'd better use SDRAM_MAX_SIZE instead of hard code, and this can
re-used on all Rockchip platform.

Thanks,
- Kever
> +		return 1;
> +
> +	return 0;
> +}

[U-Boot] [PATCH 1/3] board_f: Add mach specific DMA address check function.

[Kever Yang]

Hi Christoph,


On 05/07/2019 05:05 PM, Christoph Muellner wrote:
> From: Christoph Müllner <christoph.muellner@theobroma-systems.com>
>
> Some machines have limited DMA engines, which cannot deal
> with arbitrary addresses. This patch introduces a function
> to model these restrictions on a machine level.
>
> Signed-off-by: Christoph Müllner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>

Do you need all these two signed-off ?

Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>
>  common/board_f.c | 5 +++++
>  include/init.h   | 2 ++
>  2 files changed, 7 insertions(+)
>
> diff --git a/common/board_f.c b/common/board_f.c
> index 7ef20f2042..fed3c24373 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -1053,3 +1053,8 @@ void board_init_f_r(void)
>  	hang();
>  }
>  #endif /* CONFIG_X86 */
> +
> +__weak int mach_addr_is_dmaable(unsigned long addr)
> +{
> +	return 1;
> +}
> diff --git a/include/init.h b/include/init.h
> index afc953d51e..18210a1489 100644
> --- a/include/init.h
> +++ b/include/init.h
> @@ -125,6 +125,8 @@ int misc_init_f(void);
>  int embedded_dtb_select(void);
>  #endif
>  
> +int mach_addr_is_dmaable(unsigned long addr);
> +
>  /* common/init/board_init.c */
>  extern ulong monitor_flash_len;
>