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?
> 
>>  #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?
> 
>         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.

Both slots seems to be activated on all three SoCs. Am I looking at the 
wrong controller ? 

Thanks,

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