| Message ID | 20200806132106.747414-9-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed: mostly cleanups and some extensions | expand |
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. > sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET; > } > > -- > 2.25.4 >
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 >>
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 >>
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; }
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; > } >
diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c index 22cafce0fbdc..c51cda932463 100644 --- a/hw/sd/aspeed_sdhci.c +++ b/hw/sd/aspeed_sdhci.c @@ -16,7 +16,9 @@ #include "hw/qdev-properties.h" #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; + 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; sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET; }
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. Cc: Eddie James <eajames@linux.ibm.com> Fixes: 2bea128c3d0b ("hw/sd/aspeed_sdhci: New device") Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/sd/aspeed_sdhci.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)