On 8/11/20 1:20 AM, Joel Stanley wrote: > On Mon, 10 Aug 2020 at 17:16, Cédric Le Goater <c...@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 <c...@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; > } >