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

[Cédric Le Goater]

On 8/11/20 1:20 AM, Joel Stanley wrote:
> On Mon, 10 Aug 2020 at 17:16, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> 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 ?
> 
> Yes. the "SD/SDIO Host Controller" have both slots. The "eMMC
> controller" at 0x1E750000 on the ast2600 has just the one slot.

I forgot that one.

> We have a property for the number of slots, so we could do something like 
> this:
> 
> --- a/hw/sd/aspeed_sdhci.c
> +++ b/hw/sd/aspeed_sdhci.c
> @@ -159,12 +159,15 @@ static void aspeed_sdhci_realize(DeviceState
> *dev, Error **errp)
>  static void aspeed_sdhci_reset(DeviceState *dev)
>  {
>      AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
> +    uint32_t slots = ASPEED_SDHCI_INFO_SLOT0;
> 
>      memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
> 
> +    if (sdhci->num_slots == 2)
> +        slots |= ASPEED_SDHCI_INFO_SLOT1;
> +

I think this is fine. The alternative would be an object class but it
would be a bit overkill. 

Thanks,

C. 
  
>      /* Same default value on AST2400, AST2500 and AST2600 SoCs */
> -    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] =
> -        ASPEED_SDHCI_INFO_SLOT1 | ASPEED_SDHCI_INFO_SLOT0;
> +    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = slots;
>      sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET;
>  }
>