| Message ID | 20240621080554.18986-10-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/sd/sdcard: Accumulation of cleanups and fixes | expand |
On 21/6/24 10:05, Philippe Mathieu-Daudé wrote: > Rather than using the obscure 0x4567 magic value, > use a real random one. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/sd/sd.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index 30239b28bc..e1f13e316a 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -46,6 +46,7 @@ > #include "qemu/error-report.h" > #include "qemu/timer.h" > #include "qemu/log.h" > +#include "qemu/guest-random.h" > #include "qemu/module.h" > #include "sdmmc-internal.h" > #include "trace.h" > @@ -469,11 +470,6 @@ static void sd_set_csd(SDState *sd, uint64_t size) > sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1; > } > > -static void sd_set_rca(SDState *sd) > -{ > - sd->rca += 0x4567; > -} > - > FIELD(CSR, AKE_SEQ_ERROR, 3, 1) > FIELD(CSR, APP_CMD, 5, 1) > FIELD(CSR, FX_EVENT, 6, 1) > @@ -1055,7 +1051,7 @@ static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req) > case sd_identification_state: > case sd_standby_state: > sd->state = sd_standby_state; > - sd_set_rca(sd); > + qemu_guest_getrandom_nofail(&sd->rca, sizeof(sd->rca)); > return sd_r6; Hmm the NPCM7xx test is failing: ▶ 58/450 ERROR:../tests/qtest/npcm7xx_sdhci-test.c:101:sdwrite_read: assertion failed: (!memcmp(rmsg, msg, len)) ERROR But booting various Linux / FreeBSD guests on SD cards works, so I suppose the test (or TYPE_NPCM7XX_SDHCI model) need some rework. $ git grep 0x4567 tests/qtest/npcm7xx_sdhci-test.c:47: sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0, tests/qtest/npcm7xx_sdhci-test.c-48- SDHC_SELECT_DESELECT_CARD); In tests/qtest/libqos/sdhci-cmd.h: /* CMD Reg */ #define SDHC_SEND_RELATIVE_ADDR (3 << 8) #define SDHC_SELECT_DESELECT_CARD (7 << 8) IIUC setup_sd_card(): card address is queried ...: sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_SEND_RELATIVE_ADDR); ... but then ignored and magic 0x4567 is used instead: sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0, SDHC_SELECT_DESELECT_CARD); OK, so this test need rework. I see the sdhci_read_cmd() method but it reads the SDHC_BDATA FIFO register, not sure this is what should be used to read the RCA from R6 command response. Shengtan, Nuvoton folks, what do you think? Thanks, Phil.
On 21/6/24 11:44, Philippe Mathieu-Daudé wrote: > On 21/6/24 10:05, Philippe Mathieu-Daudé wrote: >> Rather than using the obscure 0x4567 magic value, >> use a real random one. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/sd/sd.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) > Hmm the NPCM7xx test is failing: > > ▶ 58/450 ERROR:../tests/qtest/npcm7xx_sdhci-test.c:101:sdwrite_read: > assertion failed: (!memcmp(rmsg, msg, len)) ERROR > > But booting various Linux / FreeBSD guests on SD cards works, > so I suppose the test (or TYPE_NPCM7XX_SDHCI model) need some > rework. > > $ git grep 0x4567 > tests/qtest/npcm7xx_sdhci-test.c:47: sdhci_cmd_regs(qts, > NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0, > tests/qtest/npcm7xx_sdhci-test.c-48- SDHC_SELECT_DESELECT_CARD); > > In tests/qtest/libqos/sdhci-cmd.h: > > /* CMD Reg */ > #define SDHC_SEND_RELATIVE_ADDR (3 << 8) > #define SDHC_SELECT_DESELECT_CARD (7 << 8) > > IIUC setup_sd_card(): > > card address is queried ...: > > sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, > SDHC_SEND_RELATIVE_ADDR); > > ... but then ignored and magic 0x4567 is used instead: > > sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0, > SDHC_SELECT_DESELECT_CARD); > > OK, so this test need rework. I see the sdhci_read_cmd() > method but it reads the SDHC_BDATA FIFO register, not sure > this is what should be used to read the RCA from R6 command > response. > > Shengtan, Nuvoton folks, what do you think? More than 1 week passed so I'll have a look at it myself. Regards, Phil.
On 2/7/24 15:13, Philippe Mathieu-Daudé wrote: > On 21/6/24 11:44, Philippe Mathieu-Daudé wrote: >> On 21/6/24 10:05, Philippe Mathieu-Daudé wrote: >>> Rather than using the obscure 0x4567 magic value, >>> use a real random one. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/sd/sd.c | 8 ++------ >>> 1 file changed, 2 insertions(+), 6 deletions(-) > > >> Hmm the NPCM7xx test is failing: >> >> ▶ 58/450 ERROR:../tests/qtest/npcm7xx_sdhci-test.c:101:sdwrite_read: >> assertion failed: (!memcmp(rmsg, msg, len)) ERROR >> >> But booting various Linux / FreeBSD guests on SD cards works, >> so I suppose the test (or TYPE_NPCM7XX_SDHCI model) need some >> rework. >> >> $ git grep 0x4567 >> tests/qtest/npcm7xx_sdhci-test.c:47: sdhci_cmd_regs(qts, >> NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0, >> tests/qtest/npcm7xx_sdhci-test.c-48- SDHC_SELECT_DESELECT_CARD); >> >> In tests/qtest/libqos/sdhci-cmd.h: >> >> /* CMD Reg */ >> #define SDHC_SEND_RELATIVE_ADDR (3 << 8) >> #define SDHC_SELECT_DESELECT_CARD (7 << 8) >> >> IIUC setup_sd_card(): >> >> card address is queried ...: >> >> sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, >> SDHC_SEND_RELATIVE_ADDR); >> >> ... but then ignored and magic 0x4567 is used instead: >> >> sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0, >> SDHC_SELECT_DESELECT_CARD); >> >> OK, so this test need rework. I see the sdhci_read_cmd() >> method but it reads the SDHC_BDATA FIFO register, not sure >> this is what should be used to read the RCA from R6 command >> response. Fix: https://lore.kernel.org/qemu-devel/20240702140842.54242-4-philmd@linaro.org/
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 30239b28bc..e1f13e316a 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -46,6 +46,7 @@ #include "qemu/error-report.h" #include "qemu/timer.h" #include "qemu/log.h" +#include "qemu/guest-random.h" #include "qemu/module.h" #include "sdmmc-internal.h" #include "trace.h" @@ -469,11 +470,6 @@ static void sd_set_csd(SDState *sd, uint64_t size) sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1; } -static void sd_set_rca(SDState *sd) -{ - sd->rca += 0x4567; -} - FIELD(CSR, AKE_SEQ_ERROR, 3, 1) FIELD(CSR, APP_CMD, 5, 1) FIELD(CSR, FX_EVENT, 6, 1) @@ -1055,7 +1051,7 @@ static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req) case sd_identification_state: case sd_standby_state: sd->state = sd_standby_state; - sd_set_rca(sd); + qemu_guest_getrandom_nofail(&sd->rca, sizeof(sd->rca)); return sd_r6; default:
Rather than using the obscure 0x4567 magic value, use a real random one. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/sd/sd.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)