[PATCH 0/2] DMA: AMBA PL08x DMA driver fixes due to ARCH_DMA_ADDR_T_64BIT

[PATCH 0/2] DMA: AMBA PL08x DMA driver fixes due to ARCH_DMA_ADDR_T_64BIT

[Andre Przywara]

Rob's recent pull request (Highbank fixes for 3.12) promoted the
dma_addr_t to 64bit:
commit ac072f427a4266b28a948c67a97a8c46f49b662d
Author: Rob Herring <rob.herring@calxeda.com>
Date:   Tue Aug 13 15:55:07 2013 -0500

    ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
    
Unfortunately this breaks the AMBA PL08x driver.
Fixes are pretty easy: replacing modulo with AND and fix printk
format types.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

Andre Przywara (2):
  DMA: fix AMBA PL08x driver issue with 64bit DMA address type
  DMA: fix printk warning in AMBA PL08x DMA driver

 drivers/dma/amba-pl08x.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

-- 
1.7.12.1

[PATCH 1/2] DMA: fix AMBA PL08x driver issue with 64bit DMA address type

[Andre Przywara]

In Rob's recent pull request the patch
	ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
promotes dma_addr_t to 64bit, which breaks compilation of the
AMBA PL08x DMA driver.
GCC has no function for the 64bit/8bit modulo operation.
Looking more closely the divisor can only be 1, 2 or 4, so the full
featured '%' modulo operation is overkill and can be replaced by
simple bit masking.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 drivers/dma/amba-pl08x.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 06fe45c..29e1cf9 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -286,6 +286,11 @@ static inline struct pl08x_txd *to_pl08x_txd(struct dma_async_tx_descriptor *tx)
 	return container_of(tx, struct pl08x_txd, vd.tx);
 }
 
+static int bus_addr_offset(struct pl08x_bus_data *bus)
+{
+	return bus->addr & (bus->buswidth - 1);
+}
+
 /*
  * Mux handling.
  *
@@ -886,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				return 0;
 			}
 
-			if ((bd.srcbus.addr % bd.srcbus.buswidth) ||
-					(bd.dstbus.addr % bd.dstbus.buswidth)) {
+			if (bus_addr_offset(&bd.srcbus) ||
+					bus_addr_offset(&bd.dstbus)) {
 				dev_err(&pl08x->adev->dev,
 					"%s src & dst address must be aligned to src"
 					" & dst width if peripheral is flow controller",
@@ -908,9 +913,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		 */
 		if (bd.remainder < mbus->buswidth)
 			early_bytes = bd.remainder;
-		else if ((mbus->addr) % (mbus->buswidth)) {
-			early_bytes = mbus->buswidth - (mbus->addr) %
-				(mbus->buswidth);
+		else if (bus_addr_offset(mbus)) {
+			early_bytes = mbus->buswidth - bus_addr_offset(mbus);
 			if ((bd.remainder - early_bytes) < mbus->buswidth)
 				early_bytes = bd.remainder;
 		}
@@ -928,7 +932,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			 * Master now aligned
 			 * - if slave is not then we must set its width down
 			 */
-			if (sbus->addr % sbus->buswidth) {
+			if (bus_addr_offset(sbus)) {
 				dev_dbg(&pl08x->adev->dev,
 					"%s set down bus width to one byte\n",
 					__func__);
-- 
1.7.12.1

[PATCH 1/2] DMA: fix AMBA PL08x driver issue with 64bit DMA address type

[Russell King - ARM Linux]

NAK.

On Wed, Aug 14, 2013 at 02:52:08PM +0200, Andre Przywara wrote:
> In Rob's recent pull request the patch
> 	ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
> promotes dma_addr_t to 64bit, which breaks compilation of the
> AMBA PL08x DMA driver.
> GCC has no function for the 64bit/8bit modulo operation.
> Looking more closely the divisor can only be 1, 2 or 4, so the full
> featured '%' modulo operation is overkill and can be replaced by
> simple bit masking.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  drivers/dma/amba-pl08x.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 06fe45c..29e1cf9 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -286,6 +286,11 @@ static inline struct pl08x_txd *to_pl08x_txd(struct dma_async_tx_descriptor *tx)
>  	return container_of(tx, struct pl08x_txd, vd.tx);
>  }
>  
> +static int bus_addr_offset(struct pl08x_bus_data *bus)
> +{
> +	return bus->addr & (bus->buswidth - 1);
> +}
> +
>  /*
>   * Mux handling.
>   *
> @@ -886,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  				return 0;
>  			}
>  
> -			if ((bd.srcbus.addr % bd.srcbus.buswidth) ||
> -					(bd.dstbus.addr % bd.dstbus.buswidth)) {
> +			if (bus_addr_offset(&bd.srcbus) ||
> +					bus_addr_offset(&bd.dstbus)) {
>  				dev_err(&pl08x->adev->dev,
>  					"%s src & dst address must be aligned to src"
>  					" & dst width if peripheral is flow controller",
> @@ -908,9 +913,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  		 */
>  		if (bd.remainder < mbus->buswidth)
>  			early_bytes = bd.remainder;
> -		else if ((mbus->addr) % (mbus->buswidth)) {
> -			early_bytes = mbus->buswidth - (mbus->addr) %
> -				(mbus->buswidth);
> +		else if (bus_addr_offset(mbus)) {
> +			early_bytes = mbus->buswidth - bus_addr_offset(mbus);
>  			if ((bd.remainder - early_bytes) < mbus->buswidth)
>  				early_bytes = bd.remainder;
>  		}
> @@ -928,7 +932,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  			 * Master now aligned
>  			 * - if slave is not then we must set its width down
>  			 */
> -			if (sbus->addr % sbus->buswidth) {
> +			if (bus_addr_offset(sbus)) {
>  				dev_dbg(&pl08x->adev->dev,
>  					"%s set down bus width to one byte\n",
>  					__func__);
> -- 
> 1.7.12.1
> 

[PATCH 1/2] DMA: fix AMBA PL08x driver issue with 64bit DMA address type

[Rob Herring]

On Wed, Aug 14, 2013 at 8:00 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> NAK.

This patch has nothing to do with dma masks or your dma mask series.
The code deals with bus alignment and cleans up the code to do
alignment operations in a sane way compared to modulo operator. The
only thing 64-bit dma_addr_t did was expose crap code.

Perhaps bus_addr_offset needs a better name to indicate it is dealing
with bus alignment rather than bus address offset.

Rob

> On Wed, Aug 14, 2013 at 02:52:08PM +0200, Andre Przywara wrote:
>> In Rob's recent pull request the patch
>>       ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
>> promotes dma_addr_t to 64bit, which breaks compilation of the
>> AMBA PL08x DMA driver.
>> GCC has no function for the 64bit/8bit modulo operation.
>> Looking more closely the divisor can only be 1, 2 or 4, so the full
>> featured '%' modulo operation is overkill and can be replaced by
>> simple bit masking.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>  drivers/dma/amba-pl08x.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
>> index 06fe45c..29e1cf9 100644
>> --- a/drivers/dma/amba-pl08x.c
>> +++ b/drivers/dma/amba-pl08x.c
>> @@ -286,6 +286,11 @@ static inline struct pl08x_txd *to_pl08x_txd(struct dma_async_tx_descriptor *tx)
>>       return container_of(tx, struct pl08x_txd, vd.tx);
>>  }
>>
>> +static int bus_addr_offset(struct pl08x_bus_data *bus)
>> +{
>> +     return bus->addr & (bus->buswidth - 1);
>> +}
>> +
>>  /*
>>   * Mux handling.
>>   *
>> @@ -886,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>>                               return 0;
>>                       }
>>
>> -                     if ((bd.srcbus.addr % bd.srcbus.buswidth) ||
>> -                                     (bd.dstbus.addr % bd.dstbus.buswidth)) {
>> +                     if (bus_addr_offset(&bd.srcbus) ||
>> +                                     bus_addr_offset(&bd.dstbus)) {
>>                               dev_err(&pl08x->adev->dev,
>>                                       "%s src & dst address must be aligned to src"
>>                                       " & dst width if peripheral is flow controller",
>> @@ -908,9 +913,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>>                */
>>               if (bd.remainder < mbus->buswidth)
>>                       early_bytes = bd.remainder;
>> -             else if ((mbus->addr) % (mbus->buswidth)) {
>> -                     early_bytes = mbus->buswidth - (mbus->addr) %
>> -                             (mbus->buswidth);
>> +             else if (bus_addr_offset(mbus)) {
>> +                     early_bytes = mbus->buswidth - bus_addr_offset(mbus);
>>                       if ((bd.remainder - early_bytes) < mbus->buswidth)
>>                               early_bytes = bd.remainder;
>>               }
>> @@ -928,7 +932,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>>                        * Master now aligned
>>                        * - if slave is not then we must set its width down
>>                        */
>> -                     if (sbus->addr % sbus->buswidth) {
>> +                     if (bus_addr_offset(sbus)) {
>>                               dev_dbg(&pl08x->adev->dev,
>>                                       "%s set down bus width to one byte\n",
>>                                       __func__);
>> --
>> 1.7.12.1
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] DMA: fix AMBA PL08x driver issue with 64bit DMA address type

[Andre Przywara]

On 08/14/2013 09:13 PM, Rob Herring wrote:
> On Wed, Aug 14, 2013 at 8:00 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> NAK.
>
> This patch has nothing to do with dma masks or your dma mask series.
> The code deals with bus alignment and cleans up the code to do
> alignment operations in a sane way compared to modulo operator. The
> only thing 64-bit dma_addr_t did was expose crap code.

I agree. Actually I'd see the DMA mask thing just as an opportunity to 
fix this code, not as the reason. I guess there are gazillions of 
drivers in the ARM world which have problems with any address related 
variable being bigger than 32bit, and those should all be fixed eventually.

> Perhaps bus_addr_offset needs a better name to indicate it is dealing
> with bus alignment rather than bus address offset.

Actually my first patch version called this function 
unaligned_bus_addr(), but this was rather odd with the one non-boolean 
usage of it - where it actually wants to know the offset.

Regards,
Andre.

>> On Wed, Aug 14, 2013 at 02:52:08PM +0200, Andre Przywara wrote:
>>> In Rob's recent pull request the patch
>>>        ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
>>> promotes dma_addr_t to 64bit, which breaks compilation of the
>>> AMBA PL08x DMA driver.
>>> GCC has no function for the 64bit/8bit modulo operation.
>>> Looking more closely the divisor can only be 1, 2 or 4, so the full
>>> featured '%' modulo operation is overkill and can be replaced by
>>> simple bit masking.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>> ---
>>>   drivers/dma/amba-pl08x.c | 16 ++++++++++------
>>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
>>> index 06fe45c..29e1cf9 100644
>>> --- a/drivers/dma/amba-pl08x.c
>>> +++ b/drivers/dma/amba-pl08x.c
>>> @@ -286,6 +286,11 @@ static inline struct pl08x_txd *to_pl08x_txd(struct dma_async_tx_descriptor *tx)
>>>        return container_of(tx, struct pl08x_txd, vd.tx);
>>>   }
>>>
>>> +static int bus_addr_offset(struct pl08x_bus_data *bus)
>>> +{
>>> +     return bus->addr & (bus->buswidth - 1);
>>> +}
>>> +
>>>   /*
>>>    * Mux handling.
>>>    *
>>> @@ -886,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>>>                                return 0;
>>>                        }
>>>
>>> -                     if ((bd.srcbus.addr % bd.srcbus.buswidth) ||
>>> -                                     (bd.dstbus.addr % bd.dstbus.buswidth)) {
>>> +                     if (bus_addr_offset(&bd.srcbus) ||
>>> +                                     bus_addr_offset(&bd.dstbus)) {
>>>                                dev_err(&pl08x->adev->dev,
>>>                                        "%s src & dst address must be aligned to src"
>>>                                        " & dst width if peripheral is flow controller",
>>> @@ -908,9 +913,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>>>                 */
>>>                if (bd.remainder < mbus->buswidth)
>>>                        early_bytes = bd.remainder;
>>> -             else if ((mbus->addr) % (mbus->buswidth)) {
>>> -                     early_bytes = mbus->buswidth - (mbus->addr) %
>>> -                             (mbus->buswidth);
>>> +             else if (bus_addr_offset(mbus)) {
>>> +                     early_bytes = mbus->buswidth - bus_addr_offset(mbus);
>>>                        if ((bd.remainder - early_bytes) < mbus->buswidth)
>>>                                early_bytes = bd.remainder;
>>>                }
>>> @@ -928,7 +932,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>>>                         * Master now aligned
>>>                         * - if slave is not then we must set its width down
>>>                         */
>>> -                     if (sbus->addr % sbus->buswidth) {
>>> +                     if (bus_addr_offset(sbus)) {
>>>                                dev_dbg(&pl08x->adev->dev,
>>>                                        "%s set down bus width to one byte\n",
>>>                                        __func__);
>>> --
>>> 1.7.12.1
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] DMA: fix AMBA PL08x driver issue with 64bit DMA address type

[Russell King - ARM Linux]

On Thu, Aug 15, 2013 at 11:07:26PM +0200, Andre Przywara wrote:
> On 08/14/2013 09:13 PM, Rob Herring wrote:
>> On Wed, Aug 14, 2013 at 8:00 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> NAK.
>>
>> This patch has nothing to do with dma masks or your dma mask series.
>> The code deals with bus alignment and cleans up the code to do
>> alignment operations in a sane way compared to modulo operator. The
>> only thing 64-bit dma_addr_t did was expose crap code.
>
> I agree. Actually I'd see the DMA mask thing just as an opportunity to  
> fix this code, not as the reason. I guess there are gazillions of  
> drivers in the ARM world which have problems with any address related  
> variable being bigger than 32bit, and those should all be fixed 
> eventually.

No they shouldn't.  If the DMA address registers are 32-bit, then the
DMA engine can *only* handle 32-bit addresses.

That's partly the point of my patch set: if the DMA registers are 32-bit,
then the device driver should set a 32-bit DMA mask to tell the rest of
the kernel that this driver only supports 32-bit addresses.

If DMA bus addresses of 0-4G translates to memory in the range (say) of
4G-8G physical, and you have memory in the region of 4G-8G, then that
should work.  At the moment, it doesn't - and that's what those patches
address.

The fact that the PL08x driver was ending up with DMA addresses in the
4G-8G range is a bug.  DMA addresses after mapping are supposed to be
the _exact_ value you program into the hardware.  If that value is not,
then the DMA API is buggy for that platform.

That all said, yes this patch is mostly fine, except:

>>>> +static int bus_addr_offset(struct pl08x_bus_data *bus)
>>>> +{
>>>> +     return bus->addr & (bus->buswidth - 1);
>>>> +}

that this is a poor name for this function - and we don't need it because
we have a generic form of this - IS_ALIGNED().  Is there any reason to
re-code what's already provided?

[PATCH v2] DMA: fix AMBA PL08x compilation issue with 64bit DMA address type

[Andre Przywara]

When dma_addr_t is 64 bits long, compilation of the AMBA PL08x DMA
driver breaks due to a missing 64bit%8bit modulo operation.
Looking more closely the divisor in these operations can only be
1, 2 or 4, so the full featured '%' modulo operation is overkill and
can be replaced with simple bit masking.

Change from v1:
Replace open-coded function with existing IS_ALIGNED macro and use a
macro around that to avoid a line becoming too long.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 drivers/dma/amba-pl08x.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index bce5fbf..bff41d4 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -133,6 +133,8 @@ struct pl08x_bus_data {
 	u8 buswidth;
 };
 
+#define IS_BUS_ALIGNED(bus) IS_ALIGNED((bus)->addr, (bus)->buswidth)
+
 /**
  * struct pl08x_phy_chan - holder for the physical channels
  * @id: physical index to this channel
@@ -889,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				return 0;
 			}
 
-			if ((bd.srcbus.addr % bd.srcbus.buswidth) ||
-					(bd.dstbus.addr % bd.dstbus.buswidth)) {
+			if (!IS_BUS_ALIGNED(&bd.srcbus) ||
+				!IS_BUS_ALIGNED(&bd.dstbus)) {
 				dev_err(&pl08x->adev->dev,
 					"%s src & dst address must be aligned to src"
 					" & dst width if peripheral is flow controller",
@@ -911,9 +913,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		 */
 		if (bd.remainder < mbus->buswidth)
 			early_bytes = bd.remainder;
-		else if ((mbus->addr) % (mbus->buswidth)) {
-			early_bytes = mbus->buswidth - (mbus->addr) %
-				(mbus->buswidth);
+		else if (!IS_BUS_ALIGNED(mbus)) {
+			early_bytes = mbus->buswidth -
+				(mbus->addr & (mbus->buswidth - 1));
 			if ((bd.remainder - early_bytes) < mbus->buswidth)
 				early_bytes = bd.remainder;
 		}
@@ -931,7 +933,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			 * Master now aligned
 			 * - if slave is not then we must set its width down
 			 */
-			if (sbus->addr % sbus->buswidth) {
+			if (!IS_BUS_ALIGNED(sbus)) {
 				dev_dbg(&pl08x->adev->dev,
 					"%s set down bus width to one byte\n",
 					__func__);
-- 
1.7.12.1

[PATCH v2] DMA: fix AMBA PL08x compilation issue with 64bit DMA address type

[Rob Herring]

On 08/19/2013 05:19 AM, Andre Przywara wrote:
> When dma_addr_t is 64 bits long, compilation of the AMBA PL08x DMA
> driver breaks due to a missing 64bit%8bit modulo operation.
> Looking more closely the divisor in these operations can only be
> 1, 2 or 4, so the full featured '%' modulo operation is overkill and
> can be replaced with simple bit masking.
> 
> Change from v1:
> Replace open-coded function with existing IS_ALIGNED macro and use a
> macro around that to avoid a line becoming too long.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

Vinod,

Can I have your ack and take this thru arm-soc tree. I'm dependent on
this to select 64-bit dma_addr_t for highbank.

Rob

> ---
>  drivers/dma/amba-pl08x.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index bce5fbf..bff41d4 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -133,6 +133,8 @@ struct pl08x_bus_data {
>  	u8 buswidth;
>  };
>  
> +#define IS_BUS_ALIGNED(bus) IS_ALIGNED((bus)->addr, (bus)->buswidth)
> +
>  /**
>   * struct pl08x_phy_chan - holder for the physical channels
>   * @id: physical index to this channel
> @@ -889,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  				return 0;
>  			}
>  
> -			if ((bd.srcbus.addr % bd.srcbus.buswidth) ||
> -					(bd.dstbus.addr % bd.dstbus.buswidth)) {
> +			if (!IS_BUS_ALIGNED(&bd.srcbus) ||
> +				!IS_BUS_ALIGNED(&bd.dstbus)) {
>  				dev_err(&pl08x->adev->dev,
>  					"%s src & dst address must be aligned to src"
>  					" & dst width if peripheral is flow controller",
> @@ -911,9 +913,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  		 */
>  		if (bd.remainder < mbus->buswidth)
>  			early_bytes = bd.remainder;
> -		else if ((mbus->addr) % (mbus->buswidth)) {
> -			early_bytes = mbus->buswidth - (mbus->addr) %
> -				(mbus->buswidth);
> +		else if (!IS_BUS_ALIGNED(mbus)) {
> +			early_bytes = mbus->buswidth -
> +				(mbus->addr & (mbus->buswidth - 1));
>  			if ((bd.remainder - early_bytes) < mbus->buswidth)
>  				early_bytes = bd.remainder;
>  		}
> @@ -931,7 +933,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  			 * Master now aligned
>  			 * - if slave is not then we must set its width down
>  			 */
> -			if (sbus->addr % sbus->buswidth) {
> +			if (!IS_BUS_ALIGNED(sbus)) {
>  				dev_dbg(&pl08x->adev->dev,
>  					"%s set down bus width to one byte\n",
>  					__func__);
> 

[PATCH v2] DMA: fix AMBA PL08x compilation issue with 64bit DMA address type

[Vinod Koul]

On Mon, Aug 19, 2013 at 12:19:28PM +0200, Andre Przywara wrote:
> When dma_addr_t is 64 bits long, compilation of the AMBA PL08x DMA
> driver breaks due to a missing 64bit%8bit modulo operation.
> Looking more closely the divisor in these operations can only be
> 1, 2 or 4, so the full featured '%' modulo operation is overkill and
> can be replaced with simple bit masking.
> 
> Change from v1:
> Replace open-coded function with existing IS_ALIGNED macro and use a
> macro around that to avoid a line becoming too long.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Acked-by: Vinod Koul <vinod.koul@intel.com>

~Vinod
> ---
>  drivers/dma/amba-pl08x.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index bce5fbf..bff41d4 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -133,6 +133,8 @@ struct pl08x_bus_data {
>  	u8 buswidth;
>  };
>  
> +#define IS_BUS_ALIGNED(bus) IS_ALIGNED((bus)->addr, (bus)->buswidth)
> +
>  /**
>   * struct pl08x_phy_chan - holder for the physical channels
>   * @id: physical index to this channel
> @@ -889,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  				return 0;
>  			}
>  
> -			if ((bd.srcbus.addr % bd.srcbus.buswidth) ||
> -					(bd.dstbus.addr % bd.dstbus.buswidth)) {
> +			if (!IS_BUS_ALIGNED(&bd.srcbus) ||
> +				!IS_BUS_ALIGNED(&bd.dstbus)) {
>  				dev_err(&pl08x->adev->dev,
>  					"%s src & dst address must be aligned to src"
>  					" & dst width if peripheral is flow controller",
> @@ -911,9 +913,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  		 */
>  		if (bd.remainder < mbus->buswidth)
>  			early_bytes = bd.remainder;
> -		else if ((mbus->addr) % (mbus->buswidth)) {
> -			early_bytes = mbus->buswidth - (mbus->addr) %
> -				(mbus->buswidth);
> +		else if (!IS_BUS_ALIGNED(mbus)) {
> +			early_bytes = mbus->buswidth -
> +				(mbus->addr & (mbus->buswidth - 1));
>  			if ((bd.remainder - early_bytes) < mbus->buswidth)
>  				early_bytes = bd.remainder;
>  		}
> @@ -931,7 +933,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  			 * Master now aligned
>  			 * - if slave is not then we must set its width down
>  			 */
> -			if (sbus->addr % sbus->buswidth) {
> +			if (!IS_BUS_ALIGNED(sbus)) {
>  				dev_dbg(&pl08x->adev->dev,
>  					"%s set down bus width to one byte\n",
>  					__func__);
> -- 
> 1.7.12.1
> 

-- 

[PATCH 1/2] DMA: fix AMBA PL08x driver issue with 64bit DMA address type

[Linus Walleij]

On Wed, Aug 14, 2013 at 2:52 PM, Andre Przywara
<andre.przywara@linaro.org> wrote:

> In Rob's recent pull request the patch
>         ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
> promotes dma_addr_t to 64bit, which breaks compilation of the
> AMBA PL08x DMA driver.
> GCC has no function for the 64bit/8bit modulo operation.
> Looking more closely the divisor can only be 1, 2 or 4, so the full
> featured '%' modulo operation is overkill and can be replaced by
> simple bit masking.
>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

Can you give some context? Which platform have you tested
this on after the change, or is this only compile-tested?

Of the current defconfigs selecting this driver, none is 64bit,
so makes me a bit curious if there is really some PL08x
on the Highbank or similar?

Yours,
Linus Walleij

[PATCH 1/2] DMA: fix AMBA PL08x driver issue with 64bit DMA address type

[Andre Przywara]

On 08/21/2013 11:01 PM, Linus Walleij wrote:
> On Wed, Aug 14, 2013 at 2:52 PM, Andre Przywara
> <andre.przywara@linaro.org> wrote:
>
>> In Rob's recent pull request the patch
>>          ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
>> promotes dma_addr_t to 64bit, which breaks compilation of the
>> AMBA PL08x DMA driver.
>> GCC has no function for the 64bit/8bit modulo operation.
>> Looking more closely the divisor can only be 1, 2 or 4, so the full
>> featured '%' modulo operation is overkill and can be replaced by
>> simple bit masking.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>
> Can you give some context? Which platform have you tested
> this on after the change, or is this only compile-tested?

Only compile tested (sorry, forgot to mention that).
Actually I don't have this hardware anywhere, I just happened to had 
this in my config - probably just by accident as a leftover from some 
experiments.
So if you have access to any of those platforms, please give it a test.

> Of the current defconfigs selecting this driver, none is 64bit,

But it would be selected by allyesconfig or randconfig, right? And with 
more and more platforms moving into single zImage, we will spot more of 
those, as one "64-bit platform" will promote the types for every other.

> so makes me a bit curious if there is really some PL08x
> on the Highbank or similar?

Sorry, no hidden DMA controller on Highbank ;-)

Regards,
Andre.

[PATCH 1/2] DMA: fix AMBA PL08x driver issue with 64bit DMA address type

[Matt Sealey]

On Wed, Aug 21, 2013 at 4:01 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Aug 14, 2013 at 2:52 PM, Andre Przywara
> <andre.przywara@linaro.org> wrote:
>
>> In Rob's recent pull request the patch
>>         ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
>> promotes dma_addr_t to 64bit, which breaks compilation of the
>> AMBA PL08x DMA driver.
>> GCC has no function for the 64bit/8bit modulo operation.
>> Looking more closely the divisor can only be 1, 2 or 4, so the full
>> featured '%' modulo operation is overkill and can be replaced by
>> simple bit masking.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>
> Can you give some context? Which platform have you tested
> this on after the change, or is this only compile-tested?
>
> Of the current defconfigs selecting this driver, none is 64bit,
> so makes me a bit curious if there is really some PL08x
> on the Highbank or similar?

I thought the whole PL08x thing was just a side-effect, no?

The issues here I see that Rob is trying to fix:

* If you want to put a 64-bit address into a DMA controller somewhere
- be it a register (64-bit) or descriptor (64-bit), dma_addr_t is how
the kernel handles it. If dma_addr_t is only 32-bit, how can you
specify the address there? How can you do some mapping and return a
pointer into 64-bit address space into a 32-bit value? How can you
even allocate memory (dma_alloc_coherent, for example) for the range
you want?

The objections here are obvious - as below:

1) In the case where you have an LPAE platform which enables Rob's
patch, and you use dma_addr_t and the write descriptors in memory that
are 32-bit or registers that are 32-bit using specific knowledge of
the size of dma_addr_t or just assume it's a 32-bit wide value (why
would anyone do that?) then these drivers ARE buggy and should be
fixed. But that could be all of them if they're not setting their
masks correctly.. considering you can mix DMA controllers in a system,
some with 32-bit address spaces and some with >32-bit address spaces,
making dma_addr_t 64-bit might fix one and break all the others
depending on driver quality (i.e. masks..)

2) dma_addr_t being 64-bit everywhere couldn't be that much of a
problem, though, with properly written drivers. The main culprits will
be (if they're broken):

- If they're writing values into descriptors and just using writel()
then surely there'll be a compiler warning first, which may be ignored
by the developer, and then end up writing the wrong 32-bit value to
memory or just the lower 32-bits giving a perfectly 'random' DMA
failure where it silently walks over something 4GB below where it
needed to be or somewhere else (so you'd see memory trashing, random
bus faults..)

- Passing dma_addr_t in function calls to assembly code, with an
assumption of the register usage in the following case where it's
called from C (rarer but, could happen):

void write_some_dma_thing_in_assembly(int a, dma_addr_t b, char c);

 -> r0, r1, r2 on 32-bit dma_addr_t
-> r0, r2+r3, stack on 64-bit dma_addr_t

If the assembly code assumes the dma_addr_t is 32-bit and therefore
the address (per EABI) is in r1 and not r2+r3 it will get garbage from
elsewhere in the 64-bit dma_addr_t case, and plug it into the target
descriptor or register. Same memory trashing and random bus faults as
above.

They're going to explode and there's not going to be ANY warning until
there's an explosion..

On the second point, I wonder if sparse or checkpatch can figure out
somehow if a 64-bit value is being passed as the second argument to a
function where the first argument is smaller, and print a nitpick
warning about how one might want to be careful about doing this
considering possible ABI differences depending on possible
configuration differences? Does it already?

~~

So, here's the question; how do you determine what size dma_addr_t is
considering that it may be possible that the actual size for the DMA
controller may be either, but you have a minimum of 64-bit because at
least ONE DMA controller needs that can address the full range? If any
drivers are accidentally setting >32-bit masks on non-LPAE then
there're bad addresses coming back anyway and these should have been
caught already, right?

So nothing will magically get trashed in the beginning, only during
rafts of patches to "update the dma mask to full 64-bit" where it
actually depends on the runtime CPU and config options in play anyway?

I recall something around late-to-discontinued time on Windows
XP/Server 2003 where they quit certifying 32-bit drivers that would
bail or do weird things given a 64-bit pointer. The driver core of
Windows managed to move up to 64-bit dma addresses (even on "32-bit"
hardware) internally forcing the driver developers with it. The main
reason: every x86 chip worth a damn that they supported had PAE so it
could have had 8GB of RAM, on Server 2003 Datacenter Edition I think
the upper limit was 32GB. It was perfectly possible to have a "32-bit"
system with DMA going right up to the end of the extended physical
memory address..

Did I miss something here?

-- Matt

[PATCH 2/2] DMA: fix printk warning in AMBA PL08x DMA driver

[Andre Przywara]

In Rob's recent pull request the patch
	ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
promotes dma_addr_t to 64bit, so printk generates a warning about
an incorrect type. Fix this by casting it to u64 and using %llx.
Fixing long lines on the way.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 drivers/dma/amba-pl08x.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 29e1cf9..ae19618 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -850,10 +850,13 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 		pl08x_choose_master_bus(&bd, &mbus, &sbus, cctl);
 
-		dev_vdbg(&pl08x->adev->dev, "src=0x%08x%s/%u dst=0x%08x%s/%u len=%zu\n",
-			bd.srcbus.addr, cctl & PL080_CONTROL_SRC_INCR ? "+" : "",
+		dev_vdbg(&pl08x->adev->dev,
+			"src=0x%08llx%s/%u dst=0x%08llx%s/%u len=%zu\n",
+			(u64)bd.srcbus.addr,
+			cctl & PL080_CONTROL_SRC_INCR ? "+" : "",
 			bd.srcbus.buswidth,
-			bd.dstbus.addr, cctl & PL080_CONTROL_DST_INCR ? "+" : "",
+			(u64)bd.dstbus.addr,
+			cctl & PL080_CONTROL_DST_INCR ? "+" : "",
 			bd.dstbus.buswidth,
 			bd.remainder);
 		dev_vdbg(&pl08x->adev->dev, "mbus=%s sbus=%s\n",
-- 
1.7.12.1

[PATCH 2/2] DMA: fix printk warning in AMBA PL08x DMA driver

[Russell King - ARM Linux]

NAK.

On Wed, Aug 14, 2013 at 02:52:09PM +0200, Andre Przywara wrote:
> In Rob's recent pull request the patch
> 	ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
> promotes dma_addr_t to 64bit, so printk generates a warning about
> an incorrect type. Fix this by casting it to u64 and using %llx.
> Fixing long lines on the way.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  drivers/dma/amba-pl08x.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 29e1cf9..ae19618 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -850,10 +850,13 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  
>  		pl08x_choose_master_bus(&bd, &mbus, &sbus, cctl);
>  
> -		dev_vdbg(&pl08x->adev->dev, "src=0x%08x%s/%u dst=0x%08x%s/%u len=%zu\n",
> -			bd.srcbus.addr, cctl & PL080_CONTROL_SRC_INCR ? "+" : "",
> +		dev_vdbg(&pl08x->adev->dev,
> +			"src=0x%08llx%s/%u dst=0x%08llx%s/%u len=%zu\n",
> +			(u64)bd.srcbus.addr,
> +			cctl & PL080_CONTROL_SRC_INCR ? "+" : "",
>  			bd.srcbus.buswidth,
> -			bd.dstbus.addr, cctl & PL080_CONTROL_DST_INCR ? "+" : "",
> +			(u64)bd.dstbus.addr,
> +			cctl & PL080_CONTROL_DST_INCR ? "+" : "",
>  			bd.dstbus.buswidth,
>  			bd.remainder);
>  		dev_vdbg(&pl08x->adev->dev, "mbus=%s sbus=%s\n",
> -- 
> 1.7.12.1
> 

[PATCH 0/2] DMA: AMBA PL08x DMA driver fixes due to ARCH_DMA_ADDR_T_64BIT

[Russell King - ARM Linux]

On Wed, Aug 14, 2013 at 02:52:07PM +0200, Andre Przywara wrote:
> Rob's recent pull request (Highbank fixes for 3.12) promoted the
> dma_addr_t to 64bit:
> commit ac072f427a4266b28a948c67a97a8c46f49b662d
> Author: Rob Herring <rob.herring@calxeda.com>
> Date:   Tue Aug 13 15:55:07 2013 -0500
> 
>     ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
>     
> Unfortunately this breaks the AMBA PL08x driver.
> Fixes are pretty easy: replacing modulo with AND and fix printk
> format types.

I thought I effectively said no to that patch, and told Rob to go and look
at my patch series.

Your patch series is wrong to fix this.  We're just piling more bad fixes
on top of a bug in the generic parts of the kernel.  Please stop this
madness.

[PATCH 0/2] DMA: AMBA PL08x DMA driver fixes due to ARCH_DMA_ADDR_T_64BIT

[Rob Herring]

On Wed, Aug 14, 2013 at 8:00 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Aug 14, 2013 at 02:52:07PM +0200, Andre Przywara wrote:
>> Rob's recent pull request (Highbank fixes for 3.12) promoted the
>> dma_addr_t to 64bit:
>> commit ac072f427a4266b28a948c67a97a8c46f49b662d
>> Author: Rob Herring <rob.herring@calxeda.com>
>> Date:   Tue Aug 13 15:55:07 2013 -0500
>>
>>     ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
>>
>> Unfortunately this breaks the AMBA PL08x driver.
>> Fixes are pretty easy: replacing modulo with AND and fix printk
>> format types.
>
> I thought I effectively said no to that patch, and told Rob to go and look
> at my patch series.

I thought you only objected to doing this unconditionally for all
LPAE, so I made it highbank specific like exynos is.

Rob