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

[Joel Stanley]

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.

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;
+
     /* 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;
 }