Revert "[ARM] pxa: remove now unnecessary dma_needs_bounce()"

Revert "[ARM] pxa: remove now unnecessary dma_needs_bounce()"

[Barry Song]

Hi Eric/Fujita/Russell,
As i checked the "remove now unnecessary dma_needs_bounce()" patches
of Eric, I found you were all involved. Due to compiling error of
IXP4xx platforms, the patch was reverted by Russell in
http://ftp.arm.linux.org.uk/git/gitweb.cgi?p=linux-2.6-arm.git;a=commit;h=0485e18bc4112a3b548baa314c24bfbece4d156b

But I think Eric's patch made much sense. dma_needs_bounce() is very
ambiguous in the whole arm arch, if devices use dma_mask right,
map_single() will decide whether it needs DMA bounce by the following
logic:
        if (dev->dma_mask) {
                unsigned long mask = *dev->dma_mask;
                unsigned long limit;

                limit = (mask + 1) & ~mask;
                if (limit && size > limit) {
                        dev_err(dev, "DMA mapping too big (requested %#x "
                                "mask %#Lx)\n", size, *dev->dma_mask);
                        return ~0;
                }

                /*
                 * Figure out if we need to bounce from the DMA mask.
                 */
                needs_bounce = (dma_addr | (dma_addr + size - 1)) & ~mask;
        }

Basically, we don't need dma_needs_bounce() for all chips at all. For
saa1111, it is needed only because a bug in the SoC.
For  assabet and pfs168 machines, it gave a workaound by self-defined
a dma_needs_bounce function.
return ((machine_is_assabet() || machine_is_pfs168()) &&
                (addr >= 0xc8000000 || (addr + size) >= 0xc8000000));

I think the right workaround for saa1111 should be setting dma_mask of
related devices to 0xc8000000 for the assabet and pfs168 machines but
not add a unnecessary APIs,  which will cause other SoCs need to
finish the function if they use dma bounce too.

Eric's path make much sense, we should delete the unnecessary
dma_needs_bounce() from the whole arm arch at all. If all related
people agree, we need another patch to fix it.

Thanks
Barry

Revert "[ARM] pxa: remove now unnecessary dma_needs_bounce()"

[Eric Miao]

Hrm... checked again. The IXP4xx build regression seems to have been fixed
by the below commits:

88a5810 ARM: fix IXP4xx build failure
710224f arm: fix "arm: fix pci_set_consistent_dma_mask for dmabounce devices"

Yet the simple introduction of dma_set_coherent_mask() doesn't look
like to be a correct fix to this particular issue.

Barry,

You may want to follow the lead more and help come up with a proper fix.

- eric
On Mon, Mar 21, 2011 at 2:12 PM, Barry Song <21cnbao@gmail.com> wrote:
> Hi Eric/Fujita/Russell,
> As i checked the "remove now unnecessary dma_needs_bounce()" patches
> of Eric, I found you were all involved. Due to compiling error of
> IXP4xx platforms, the patch was reverted by Russell in
> http://ftp.arm.linux.org.uk/git/gitweb.cgi?p=linux-2.6-arm.git;a=commit;h=0485e18bc4112a3b548baa314c24bfbece4d156b
>
> But I think Eric's patch made much sense. dma_needs_bounce() is very
> ambiguous in the whole arm arch, if devices use dma_mask right,
> map_single() will decide whether it needs DMA bounce by the following
> logic:
> ? ? ? ?if (dev->dma_mask) {
> ? ? ? ? ? ? ? ?unsigned long mask = *dev->dma_mask;
> ? ? ? ? ? ? ? ?unsigned long limit;
>
> ? ? ? ? ? ? ? ?limit = (mask + 1) & ~mask;
> ? ? ? ? ? ? ? ?if (limit && size > limit) {
> ? ? ? ? ? ? ? ? ? ? ? ?dev_err(dev, "DMA mapping too big (requested %#x "
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"mask %#Lx)\n", size, *dev->dma_mask);
> ? ? ? ? ? ? ? ? ? ? ? ?return ~0;
> ? ? ? ? ? ? ? ?}
>
> ? ? ? ? ? ? ? ?/*
> ? ? ? ? ? ? ? ? * Figure out if we need to bounce from the DMA mask.
> ? ? ? ? ? ? ? ? */
> ? ? ? ? ? ? ? ?needs_bounce = (dma_addr | (dma_addr + size - 1)) & ~mask;
> ? ? ? ?}
>
> Basically, we don't need dma_needs_bounce() for all chips at all. For
> saa1111, it is needed only because a bug in the SoC.
> For ?assabet and pfs168 machines, it gave a workaound by self-defined
> a dma_needs_bounce function.
> return ((machine_is_assabet() || machine_is_pfs168()) &&
> ? ? ? ? ? ? ? ?(addr >= 0xc8000000 || (addr + size) >= 0xc8000000));
>
> I think the right workaround for saa1111 should be setting dma_mask of
> related devices to 0xc8000000 for the assabet and pfs168 machines but
> not add a unnecessary APIs, ?which will cause other SoCs need to
> finish the function if they use dma bounce too.
>
> Eric's path make much sense, we should delete the unnecessary
> dma_needs_bounce() from the whole arm arch at all. If all related
> people agree, we need another patch to fix it.
>
> Thanks
> Barry
>

Revert "[ARM] pxa: remove now unnecessary dma_needs_bounce()"

[Russell King - ARM Linux]

On Mon, Mar 21, 2011 at 03:21:24PM +0800, Eric Miao wrote:
> Hrm... checked again. The IXP4xx build regression seems to have been fixed
> by the below commits:
> 
> 88a5810 ARM: fix IXP4xx build failure
> 710224f arm: fix "arm: fix pci_set_consistent_dma_mask for dmabounce devices"

Annoyingly, no one reported what the actual error messages with IXP4xx
were, so we can only guess at this point.

Eric's patch (4fa5518) did this:

+#ifdef CONFIG_SA1111
 extern int dma_needs_bounce(struct device*, dma_addr_t, size_t);
+#else
+static inline int dma_needs_bounce(struct device *dev, dma_addr_t addr,
+                                  size_t size)
+{
+       return 0;
+}
+#endif

which assumes that SA1111 is the _only_ user of the dma_needs_bounce()
function.  This is incorrect as IXP4xx also has an implementation.

So, reapplying the original patch will immediately reintroduce the
IXP4xx build failure as we end up with two functions called
dma_needs_bounce().

Revert "[ARM] pxa: remove now unnecessary dma_needs_bounce()"

[Eric Miao]

On Mon, Mar 21, 2011 at 10:54 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Mar 21, 2011 at 03:21:24PM +0800, Eric Miao wrote:
>> Hrm... checked again. The IXP4xx build regression seems to have been fixed
>> by the below commits:
>>
>> 88a5810 ARM: fix IXP4xx build failure
>> 710224f arm: fix "arm: fix pci_set_consistent_dma_mask for dmabounce devices"
>
> Annoyingly, no one reported what the actual error messages with IXP4xx
> were, so we can only guess at this point.
>
> Eric's patch (4fa5518) did this:
>
> +#ifdef CONFIG_SA1111
> ?extern int dma_needs_bounce(struct device*, dma_addr_t, size_t);
> +#else
> +static inline int dma_needs_bounce(struct device *dev, dma_addr_t addr,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?size_t size)
> +{
> + ? ? ? return 0;
> +}
> +#endif
>
> which assumes that SA1111 is the _only_ user of the dma_needs_bounce()
> function. ?This is incorrect as IXP4xx also has an implementation.
>
> So, reapplying the original patch will immediately reintroduce the
> IXP4xx build failure as we end up with two functions called
> dma_needs_bounce().
>

Yeah, I remember I had once removed that dma_needs_bounce() from
ixp4xx. And simply re-revert that patch won't work. And it looks like
things changed a bit since then, so it does need further look-into for
a proper fix now.

Revert "[ARM] pxa: remove now unnecessary dma_needs_bounce()"

[Russell King - ARM Linux]

On Sun, Mar 20, 2011 at 11:12:29PM -0700, Barry Song wrote:
> Hi Eric/Fujita/Russell,
> As i checked the "remove now unnecessary dma_needs_bounce()" patches
> of Eric, I found you were all involved. Due to compiling error of
> IXP4xx platforms, the patch was reverted by Russell in
> http://ftp.arm.linux.org.uk/git/gitweb.cgi?p=linux-2.6-arm.git;a=commit;h=0485e18bc4112a3b548baa314c24bfbece4d156b
> 
> But I think Eric's patch made much sense. dma_needs_bounce() is very
> ambiguous in the whole arm arch, if devices use dma_mask right,
> map_single() will decide whether it needs DMA bounce by the following
> logic:
>         if (dev->dma_mask) {
>                 unsigned long mask = *dev->dma_mask;
>                 unsigned long limit;
> 
>                 limit = (mask + 1) & ~mask;
>                 if (limit && size > limit) {
>                         dev_err(dev, "DMA mapping too big (requested %#x "
>                                 "mask %#Lx)\n", size, *dev->dma_mask);
>                         return ~0;
>                 }
> 
>                 /*
>                  * Figure out if we need to bounce from the DMA mask.
>                  */
>                 needs_bounce = (dma_addr | (dma_addr + size - 1)) & ~mask;
>         }
> 
> Basically, we don't need dma_needs_bounce() for all chips at all. For
> saa1111, it is needed only because a bug in the SoC.
> For  assabet and pfs168 machines, it gave a workaound by self-defined
> a dma_needs_bounce function.
> return ((machine_is_assabet() || machine_is_pfs168()) &&
>                 (addr >= 0xc8000000 || (addr + size) >= 0xc8000000));

No, you don't understand what's a bug and what's a design issue.

The SA1111 (not SAA which would be a Phillips device) is designed to only
access one SDRAM chip.  From memory, the SA1110 can have up to four SDRAM
chips connected to it - at 0xc0000000, 0xc8000000, 0xd0000000, 0xd8000000
phys.

Depending on how the platform is wired up, the SA1111 can only access
memory from exactly _one_ of those banks.  It can _never_ access all of
those banks of memory.

However, as an entirely separate issue, the SA1111 has a bug in that it
incorrectly drives one of the SDRAM address inputs during part of the
access cycle, which if logic one causes the SDRAM to become confused.
This means that we end up with situations where odd MB of SDRAM are
unavailable for DMA, whereas even MB of SDRAM are perfectly fine.

So, not only do we need to ensure that we bounce any memory which is not
in the right bank, but also any memory which conflicts with the buggy
behaviour.

The buggy behaviour is dealt with setting an appropriate DMA mask,
which clears the problematical bit.  This would be legal if the DMA mask
was a mask, but it isn't - it's misnamed as its actually a DMA _limit_.

This is where the problem comes - a DMA limit can't incorporate the
information necessary to indicate which bank of SDRAM is accessible to
the SA1111 as it can't indicate that only the second bank should be
used for DMA.

This is exactly where dma_needs_bounce() comes in to provide this kind
of platform knowledge into the DMA bounce code - and I think trying to
remove the function is a big mistake.

Revert "[ARM] pxa: remove now unnecessary dma_needs_bounce()"

[Barry Song]

2011/3/21 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sun, Mar 20, 2011 at 11:12:29PM -0700, Barry Song wrote:
>> Hi Eric/Fujita/Russell,
>> As i checked the "remove now unnecessary dma_needs_bounce()" patches
>> of Eric, I found you were all involved. Due to compiling error of
>> IXP4xx platforms, the patch was reverted by Russell in
>> http://ftp.arm.linux.org.uk/git/gitweb.cgi?p=linux-2.6-arm.git;a=commit;h=0485e18bc4112a3b548baa314c24bfbece4d156b
>>
>> But I think Eric's patch made much sense. dma_needs_bounce() is very
>> ambiguous in the whole arm arch, if devices use dma_mask right,
>> map_single() will decide whether it needs DMA bounce by the following
>> logic:
>> ? ? ? ? if (dev->dma_mask) {
>> ? ? ? ? ? ? ? ? unsigned long mask = *dev->dma_mask;
>> ? ? ? ? ? ? ? ? unsigned long limit;
>>
>> ? ? ? ? ? ? ? ? limit = (mask + 1) & ~mask;
>> ? ? ? ? ? ? ? ? if (limit && size > limit) {
>> ? ? ? ? ? ? ? ? ? ? ? ? dev_err(dev, "DMA mapping too big (requested %#x "
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "mask %#Lx)\n", size, *dev->dma_mask);
>> ? ? ? ? ? ? ? ? ? ? ? ? return ~0;
>> ? ? ? ? ? ? ? ? }
>>
>> ? ? ? ? ? ? ? ? /*
>> ? ? ? ? ? ? ? ? ?* Figure out if we need to bounce from the DMA mask.
>> ? ? ? ? ? ? ? ? ?*/
>> ? ? ? ? ? ? ? ? needs_bounce = (dma_addr | (dma_addr + size - 1)) & ~mask;
>> ? ? ? ? }
>>
>> Basically, we don't need dma_needs_bounce() for all chips at all. For
>> saa1111, it is needed only because a bug in the SoC.
>> For ?assabet and pfs168 machines, it gave a workaound by self-defined
>> a dma_needs_bounce function.
>> return ((machine_is_assabet() || machine_is_pfs168()) &&
>> ? ? ? ? ? ? ? ? (addr >= 0xc8000000 || (addr + size) >= 0xc8000000));
>
> No, you don't understand what's a bug and what's a design issue.
>
> The SA1111 (not SAA which would be a Phillips device) is designed to only
> access one SDRAM chip. ?From memory, the SA1110 can have up to four SDRAM
> chips connected to it - at 0xc0000000, 0xc8000000, 0xd0000000, 0xd8000000
> phys.
>
> Depending on how the platform is wired up, the SA1111 can only access
> memory from exactly _one_ of those banks. ?It can _never_ access all of
> those banks of memory.
>
> However, as an entirely separate issue, the SA1111 has a bug in that it
> incorrectly drives one of the SDRAM address inputs during part of the
> access cycle, which if logic one causes the SDRAM to become confused.
> This means that we end up with situations where odd MB of SDRAM are
> unavailable for DMA, whereas even MB of SDRAM are perfectly fine.
>
> So, not only do we need to ensure that we bounce any memory which is not
> in the right bank, but also any memory which conflicts with the buggy
> behaviour.
>
> The buggy behaviour is dealt with setting an appropriate DMA mask,
> which clears the problematical bit. ?This would be legal if the DMA mask
> was a mask, but it isn't - it's misnamed as its actually a DMA _limit_.
>
> This is where the problem comes - a DMA limit can't incorporate the
> information necessary to indicate which bank of SDRAM is accessible to
> the SA1111 as it can't indicate that only the second bank should be
> used for DMA.
>
> This is exactly where dma_needs_bounce() comes in to provide this kind
> of platform knowledge into the DMA bounce code - and I think trying to
> remove the function is a big mistake.
>
Now i understand sa1111 issue better. Even though sa1111 really need
dma_needs_bounce() to handle what dma_mask can't provide, for example,
tell which bank can be used for DMA, for most chips, memory area which
can be used for DMA is lower address, dma_limit is just dma_mask. So
is it possible to make dma_needs_bounce() optional for SoC to
implement? Chips  without issues like sa1111 don't need to implement
this function. dma_mask will help to decide whether dma bounce will be
done.

For sa1111, why don't PCB design just connect one sdram to bank0 and
make software use bank0 as DMA bank if bank0 is not slower than
bank1/2/3? If bank0 is same with bank1/2/3 in hardware timing, there
is no reason to make a strange design which uses bank1/2/3 as DMA
area.

Revert "[ARM] pxa: remove now unnecessary dma_needs_bounce()"

[Russell King - ARM Linux]

On Mon, Mar 21, 2011 at 07:00:57PM -0700, Barry Song wrote:
> For sa1111, why don't PCB design just connect one sdram to bank0 and
> make software use bank0 as DMA bank if bank0 is not slower than
> bank1/2/3? If bank0 is same with bank1/2/3 in hardware timing, there
> is no reason to make a strange design which uses bank1/2/3 as DMA
> area.

Who knows why board and SoC designers do silly things.  If we could stop
that, the kernel would be *much* simpler.

Revert "[ARM] pxa: remove now unnecessary dma_needs_bounce()"

[Barry Song]

2011/3/22 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Mon, Mar 21, 2011 at 07:00:57PM -0700, Barry Song wrote:
>> For sa1111, why don't PCB design just connect one sdram to bank0 and
>> make software use bank0 as DMA bank if bank0 is not slower than
>> bank1/2/3? If bank0 is same with bank1/2/3 in hardware timing, there
>> is no reason to make a strange design which uses bank1/2/3 as DMA
>> area.
>
> Who knows why board and SoC designers do silly things. ?If we could stop
> that, the kernel would be *much* simpler.

How about doing something like this to permit chips without strange
issues don't implement dma_needs_bounce() function?  if you are ok, i
can send a full patch:

diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig
index ea5ee4d..f4ef5ac 100644
--- a/arch/arm/common/Kconfig
+++ b/arch/arm/common/Kconfig
@@ -23,6 +23,10 @@ config PL330
 config SA1111
        bool
        select DMABOUNCE if !ARCH_PXA
+       select HAVE_DMA_NEEDS_BOUNCE if !ARCH_PXA
+
+config HAVE_DMA_NEEDS_BOUNCE
+       bool

 config DMABOUNCE
        bool
diff --git a/arch/arm/include/asm/dma-mapping.h
b/arch/arm/include/asm/dma-mapping.h
index 4fff837..baef463 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -277,6 +277,7 @@ extern int dmabounce_register_dev(struct device *,
unsigned long,
  */
 extern void dmabounce_unregister_dev(struct device *);

+#ifdef CONFIG_HAVE_DMA_NEEDS_BOUNCE
 /**
  * dma_needs_bounce
  *
@@ -294,6 +295,12 @@ extern void dmabounce_unregister_dev(struct device *);
  *
  */
 extern int dma_needs_bounce(struct device*, dma_addr_t, size_t);
+#else
+static inline int dma_needs_bounce(struct device*, dma_addr_t, size_t)
+{
+       return 0;
+}
+#endif

 /*
  * The DMA API, implemented by dmabounce.c.  See below for descriptions.

>