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

[Jamin Lin]

Hi Cedric,

Sorry reply you late.
> On 4/30/24 10:51, Jamin Lin wrote:
> > 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 ?
> >>
After I discussed with designers, my explanation as below.
AST2500 datasheet description:
FMC8C: DMA Length Register
31:25      Reserved(0)
24:2  RW  DMA_LENGTH
          From 4bytes to 32MB
          0: 4bytes
          0x7FFFFF: 32MB
1:0       Reserved

If users set this register 0, SPI DMA controller would move 4 bytes data.
If users set this register bits[24:2] 0x7FFFFF, SPI DMC controller would move 
32MB data because this register includes reserved bits [1:0].
Ex: bits 24:2 --> 0x7fffff
   bits 1:0 --> 0 Reserved  
bits[24:0] is (0x1FFFFFC + start length 4) --> 2000000 --> 32MB  
           111 1111 1111 1111 1111 1111  00
LENGTH[24:2] 7   F    F   F   F    F    Reserved [1:0]

That was why AST2500 DMA length should 4 bytes alignment and you do not see 
handling length units because it includes reserved bits 1 and 0.
If user want to move 3 bytes data, our firmware set this register 4 ensure it 
4bytes alignment and the value of register as following
bits[2:0] = "b100" Finally, SPI DMA controller move 8 bytes data(4bytes input 
count + start length 4bytes) 

And that was why it set DMA_LENGTH 0x01FFFFFC
https://github.com/qemu/qemu/blob/master/hw/ssi/aspeed_smc.c#L187
#define DMA_LENGTH(val)         ((val) & 0x01FFFFFC)

I want to change it to 0x01FFFFFF to support all ASPEED SOCs because AST2600, 
AST2700 and AST1030 use this register bit 0 and 1 whose unit is 1byte rather 
than 4 bytes.

> > https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/l
> > ib/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/a
> > rch/arm/mach-aspeed/ast2500/utils.S#L35
> 
> yes. I don't see any 1bit / 4bytes conversion when setting the DMA_LEN
> register. Am I mistaking ? That's not what the specs says.
> 
> > 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.
> 
> ok. For that, I think you could replace aspeed_smc_dma_len() with
> 
>     return QEMU_ALIGN_UP(s->regs[R_DMA_LEN] + asc->dma_start_length,
> 4);
> 
Thanks for your suggestion and will fix it.

> Thanks,
> 
> C.
> 
> 
> 
> >
> >> 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/a
> > rch/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.
> >
> >