RE: [PATCH v3 07/16] aspeed/smc: fix dma moving incorrect data length issue

[Jamin Lin]

Hi Cedric,
> On 4/19/24 15:41, Cédric Le Goater wrote:
> > On 4/16/24 11:18, Jamin Lin wrote:
> >> DMA length is from 1 byte to 32MB for AST2600 and AST10x0 and DMA
> >> length is from 4 bytes to 32MB for AST2500.
> >>
> >> In other words, if "R_DMA_LEN" is 0, it should move at least 1 byte
> >> data for AST2600 and AST10x0 and 4 bytes data for AST2500.
> >>> To support all ASPEED SOCs, adds dma_start_length parameter to store
> >> the start length, add helper routines function to compute the dma
> >> length and update DMA_LENGTH mask to "1FFFFFF" to fix dma moving
> >> incorrect data length issue.
> >
> > OK. There are two problems to address, the "zero" length transfer and
> > the DMA length unit, which is missing today. Newer SoC use a 1 bit /
> > byte and older ones, AST2400 and AST2500, use 1 bit / 4 bytes.
> >
> > We can introduce a AspeedSMCClass::dma_len_unit and rework the loop to :
> >
> >      do {
> >
> >        ....
> >
> >         if (s->regs[R_DMA_LEN]) {
> >              s->regs[R_DMA_LEN] -= 4 / asc->dma_len_unit;
> >          }
> >      } while (s->regs[R_DMA_LEN]);
> >
> > It should fix the current implementation.
> 
> 
> I checked what FW is doing on a QEMU ast2500-evb machine :
> 
>      U-Boot 2019.04-v00.04.12 (Sep 29 2022 - 10:40:37 +0000)
>      ...
> 
>         Loading Kernel Image ... aspeed_smc_write @0x88 size 4:
> 0x80001000
>      aspeed_smc_write @0x84 size 4: 0x20100130
>      aspeed_smc_write @0x8c size 4: 0x3c6770
>      aspeed_smc_write @0x80 size 4: 0x1
>      aspeed_smc_dma_rw read flash:@0x00100130 dram:@0x1000
> size:0x003c6774
>      aspeed_smc_read @0x8 size 4: 0x800
>      aspeed_smc_write @0x80 size 4: 0x0
>      OK
>         Loading Ramdisk to 8fef2000, end 8ffff604 ... aspeed_smc_write
> @0x88 size 4: 0x8fef2000
>      aspeed_smc_write @0x84 size 4: 0x204cdde0
>      aspeed_smc_write @0x8c size 4: 0x10d604
>      aspeed_smc_write @0x80 size 4: 0x1
>      aspeed_smc_dma_rw read flash:@0x004cdde0 dram:@0xfef2000
> size:0x0010d608
>      aspeed_smc_read @0x8 size 4: 0x800
>      aspeed_smc_write @0x80 size 4: 0x0
>      OK
>         Loading Device Tree to 8fee7000, end 8fef135e ... aspeed_smc_write
> @0x88 size 4: 0x8fee7000
>      aspeed_smc_write @0x84 size 4: 0x204c69b4
>      aspeed_smc_write @0x8c size 4: 0x7360
>      aspeed_smc_write @0x80 size 4: 0x1
>      aspeed_smc_dma_rw read flash:@0x004c69b4 dram:@0xfee7000
> size:0x00007364
>      aspeed_smc_read @0x8 size 4: 0x800
>      aspeed_smc_write @0x80 size 4: 0x0
>      OK
> 
>      Starting kernel ...
> 
> It seems that the R_DMA_LEN register is set by FW without taking into account
> the length unit ( bit / 4 bytes). Would you know why ?
> 
https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/lib/string.c#L559
This line make user input data length 4 bytes alignment.
https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/arch/arm/mach-aspeed/ast2500/utils.S#L35
This line set the value of count parameter to AST_FMC_DNA_LENGTH.
AST_FMC_DMA_LENGTH is 4 bytes alignment value.
Example: input 4
((4+3)/4) * 4 --> (7/4) *4 ---> 4
If AST_FMC_DMA_LENGTH is 0, it means it should move 4 bytes data and 
AST_FMC_DMA_LENGTH do not need to be divided by 4. 

> If I change the model to match 1 bit / 4 bytes unit of the R_DMA_LEN register.
> Linux fails to boot. I didn't dig further and this is something we need to
> understand before committing.
> 
> > I don't think this is necessary to add a Fixes tag because the problem
> > has been there for ages and no one reported it. Probably because the
> > only place DMA transfers are used is in U-Boot and transfers have a
> > non-zero length.
> >
> >> Currently, only supports dma length 4 bytes aligned.
> 
> Is this 4 bytes alignment new for the AST2700 or is this something you added
> because the mask of DMA_LENGTH is now relaxed to match all addresses ?
> 
> #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
AST2700, AST2600 and AST1030 is from 1 byte to 1FFFFFF, so I change this Micro 
to fix data lost.
https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/arch/arm/mach-aspeed/ast2700/spi.c#L186
Please see this line, it decrease dma_len 1 byte first then, set to DMA_LEN 
register because DMA_LEN is 0 which means should move 1 byte data if DMA 
enables for AST2600, AST1030 and AST2700. 
> 
> Thanks,
> 
> C.

Only AST2500 need 4 bytes alignment for DMA Length. However, according to the 
design of sapped_smc_dma_rw function,
it utilizes address_space_stl_le API and it only works data 4 bytes alignment. 
https://github.com/qemu/qemu/blob/master/hw/ssi/aspeed_smc.c#L889 
For example,
If users want to move 0x101 data_length, after 0x100 data has been moved and 
remains 1 byte data need to be moved.
Please see this line program, 
https://github.com/qemu/qemu/blob/master/hw/ssi/aspeed_smc.c#L940
```
s->regs[R_DMA_LEN] -= 4;
```
The value of s->regs[R_DMA_LEN] becomes 0xffffffffXX because it is uint32_t 
data type and this while loop run in the unexpected behavior, 
https://github.com/qemu/qemu/blob/master/hw/ssi/aspeed_smc.c#L864
That was, why I set 4bytes alignment for all models.