Re: [PATCH for-5.2 08/19] aspeed/sdhci: Fix reset sequence

[Cédric Le Goater]

On 8/7/20 1:42 AM, Joel Stanley wrote:
> On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> BIT(0) of the ASPEED_SDHCI_INFO register is set by SW and polled until
>> the bit is cleared by HW. Add definitions for the default value of
>> this register and fix the reset sequence by clearing the RESET bit.
> 
> This is mentioned in the datasheet but I couldn't find if software
> depends on the behaviour. Were you just trying to make the model more
> accurate?

Yes. The AMI FW for the palmetto is requiring this : 


U-Boot 1.1.6 (Oct 27 2016 - 10:46:29)

DRAM:  446 MiB
Found SPI Chip Numonyx n25q256 
Flash: 32 MiB
MMC:   ast_sd: 0
Environment Read 1 time
Net:   ast_eth0
DRAM ECC disabled
Hit Esc key to stop autoboot:  0 
Image to be booted is 1
conf @ /dev/mtdblock1 Address 20050000
Found Root File System @ /dev/mtdblock2
root @ /dev/mtdblock2 Address 20120000
extlog @ /dev/mtdblock3 Address 20cd0000
www @ /dev/mtdblock4 Address 20d90000
Un-Protect Flash Bank # 1
Booting from Primary side
Booting from MODULE_PIMAGE ...
Bootargs = [root=/dev/mtdblock2 ro ip=none console=ttyS4,38400 
rootfstype=cramfs bigphysarea=8192 imagebooted=1]
## Booting image at 20ae0040 ...
   Image Name:   Linux-2.6.28.10-ami
   Image Type:   ARM Linux Kernel Image (uncompressed)
   Data Size:    1943652 Bytes = 1.9 MiB
   Load Address: 40008000
   Entry Point:  40008000
   Verifying Checksum ... OK
OK

Starting kernel ...

Uncompressing 
Linux.............................................................................................................................
 done, booting the kernel.
Linux version 2.6.28.10-ami (root@viswa-desk) (gcc version 4.5.1 (Sourcery G++ 
Lite 2010.09-50) ) #1 Thu Oct 27 10:45:59 EDT 2016
CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=00093177
CPU: VIVT data cache, VIVT instruction cache
Machine: AST2400EVB



> 
>>  #define ASPEED_SDHCI_INFO            0x00
>> -#define  ASPEED_SDHCI_INFO_RESET     0x00030000
>> +#define  ASPEED_SDHCI_INFO_SLOT1     (1 << 17)
>> +#define  ASPEED_SDHCI_INFO_SLOT0     (1 << 16)
>> +#define  ASPEED_SDHCI_INFO_RESET     (1 << 0)
>>  #define ASPEED_SDHCI_DEBOUNCE        0x04
>>  #define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
>>  #define ASPEED_SDHCI_BUS             0x08
>> @@ -67,6 +69,9 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, 
>> uint64_t val,
>>      AspeedSDHCIState *sdhci = opaque;
>>
>>      switch (addr) {
>> +    case ASPEED_SDHCI_INFO:
>> +        sdhci->regs[TO_REG(addr)] = (uint32_t)val & 
>> ~ASPEED_SDHCI_INFO_RESET;
> 
> I think bits 24 and 25 should be writable too?

ok. I will take a look.
> 
>         sdhci->regs[TO_REG(addr)] = (uint32_t)val &
> ~(ASPEED_SDHCI_INFO_RESET | ASPEED_SDHCI_INFO_SLOT10 |
> ASPEED_SDHCI_INFO_SLOT1);
> 
>> +
>>      case ASPEED_SDHCI_SDIO_140:
>>          sdhci->slots[0].capareg = (uint64_t)(uint32_t)val;
>>          break;
>> @@ -155,7 +160,8 @@ static void aspeed_sdhci_reset(DeviceState *dev)
>>      AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
>>
>>      memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
>> -    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_RESET;
>> +    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] =
>> +        ASPEED_SDHCI_INFO_SLOT1 | ASPEED_SDHCI_INFO_SLOT0;
> 
> If we want to be super strict this is true for the "sd" devices, but
> the "emmc" device in the ast2600 only sets slot0. I don't think this
> distinction is important to model though.

OK. we can add different reset arrays depending on the SoC. 

C.

>>      sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = 
>> ASPEED_SDHCI_DEBOUNCE_RESET;
>>  }
>>
>> --
>> 2.25.4
>>