Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Arnd Bergmann]

On Tue, Feb 1, 2022 at 6:48 PM Russell King (Oracle)
<linux@xxxxxxxxxxxxxxx> wrote:
> On Tue, Feb 01, 2022 at 05:10:38PM +0000, Robin Murphy wrote:
> >
> > Hmm, my reading of it was different. AFAICS it should affect all platforms
> > with CONFIG_ARCH_SA1100 + CONFIG_SA1111 - the bus notifier from
> > sa1111_init() will initialise dmabounce for everything where
> > sa1111_configure_smc() has punched a hole in the DMA mask to handle the
> > addressing erratum. sa1111_needs_bounce() looks to be a further
> > consideration for platforms where DMA *additionally* cannot target an entire
> > bank of memory at all.
>
> Correct. The SA1111 companion can only access one SDRAM bank, whereas
> the SA1110 SoC can address up to four SDRAM banks. On platforms where
> there is only one bank of SDRAM, there is no issue. However, on the
> Assabet there is one SDRAM bank, and on the Neponset daughter board
> with the SA1111, there is a second bank. As explained in the commentry,
> the SA1111 can be hardware-configured via resistive jumpers to access
> either bank, but we only support the factory-shipped configuration,
> which is bank 0 (the lowest addressable bank.)

Ok, so this is the part that I think my patch gets right.

> The SA1111 also has an issue that one of its address lines doesn't
> behave correctly, and depending on the SDRAM columns/rows, this
> punches multiple holes in the SDRAM address space it can access,
> which is what the sa1111_dma_mask[] array is about, and we end up
> with every alternate megabyte of physical address space being
> inaccessible.
>
> The DMA mask, along with the logic in dmabounce (which truely uses the
> DMA mask as, erm, a *mask* rather than the misnamed *limit* that it
> has been) know about both of these issues.

while this part would not work if dma_alloc_flags() ends up getting
memory that is not accessible. At the minimum I need to drop the
machine_is_assabet() check and always allocate a safe buffer to
back hcd->local_mem regardless of the machine.

After reading through the dmabounce code again, my interpretation
is that the safe buffer it uses for bounces ultimately relies on
dma_alloc_coherent() allocating physical pages using GFP_DMA,
which in turn is sized to 1MB on the machines that need it.

If I'm not missing something else, using dmam_alloc_flags() in the
local_mem code should work with the same address restrictions, so
I hope I only need to update the changelog text plus the trivial change
below.

         Arnd

@@ -207,6 +207,14 @@ static int ohci_hcd_sa1111_probe(struct sa1111_dev *dev)
        }

        /*
+        * According to the "Intel StrongARM SA-1111 Microprocessor Companion
+        * Chip Specification Update" (June 2000), erratum #7, there is a
+        * significant bug in the SA1111 SDRAM shared memory controller.  If
+        * an access to a region of memory above 1MB relative to the bank base,
+        * it is important that address bit 10 _NOT_ be asserted. Depending
+        * on the configuration of the RAM, bit 10 may correspond to one
+        * of several different (processor-relative) address bits.
+        *
         * Section 4.6 of the "Intel StrongARM SA-1111 Development Module
         * User's Guide" mentions that jumpers R51 and R52 control the
         * target of SA-1111 DMA (either SDRAM bank 0 on Assabet, or
@@ -214,13 +222,14 @@ static int ohci_hcd_sa1111_probe(struct sa1111_dev *dev)
         * Assabet, so any address in bank 1 is necessarily invalid.
         *
         * As a workaround, use a bounce buffer in addressable memory
-        * as local_mem.
+        * as local_mem, relying on ZONE_DMA to provide an area that
+        * fits within the above constraints.
+        *
+        * SZ_64K is an estimate for what size this might need.
         */
-       if (machine_is_assabet()) {
-               ret = usb_hcd_setup_local_mem(hcd, 0, 0, SZ_64K);
-               if (ret)
-                       goto out_err1;
-       }
+       ret = usb_hcd_setup_local_mem(hcd, 0, 0, SZ_64K);
+       if (ret)
+               goto out_err1;

        if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, hcd_name)) {
                dev_dbg(&dev->dev, "request_mem_region failed\n");