| Message ID | 20250111183711.2338-7-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i.MX and SDHCI improvements | expand |
On Sat, 11 Jan 2025 at 18:37, Bernhard Beschow <shentey@gmail.com> wrote: > > Commit ce5dd27534b0 "hw/sd: Remove omap2_mmc device" removed the last user of > sd_set_cb(). Rework this functionality into GPIOs. > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Signed-off-by: Bernhard Beschow <shentey@gmail.com> What is this for? We have a non-legacy API for "the SD controller needs to know when the SD card is inserted or the readonly status changes", which is that the controller implements the SDBasClass set_inserted and set_readonly methods. (See the pl011 for an example.) I would prefer it if we used that consistently, rather than having two mechanisms, one using GPIO lines and one using class methods. I think we should delete the sd_set_cb() API and handling code entirely. thanks -- PMM
Am 27. Januar 2025 13:24:46 UTC schrieb Peter Maydell <peter.maydell@linaro.org>: >On Sat, 11 Jan 2025 at 18:37, Bernhard Beschow <shentey@gmail.com> wrote: >> >> Commit ce5dd27534b0 "hw/sd: Remove omap2_mmc device" removed the last user of >> sd_set_cb(). Rework this functionality into GPIOs. >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> > >What is this for? We have a non-legacy API for "the SD controller >needs to know when the SD card is inserted or the readonly >status changes", which is that the controller implements the >SDBasClass set_inserted and set_readonly methods. (See the pl011 >for an example.) > >I would prefer it if we used that consistently, rather than having >two mechanisms, one using GPIO lines and one using class methods. >I think we should delete the sd_set_cb() API and handling code >entirely. According to the Linux MMC controller DT schema, there are actually two ways to implement cd and wp lines [1]. When implementing the imx8mp-evk board, I thought I would need to model the GPIO style [2], hence I implemented it plus the active low part on the SD card. Later I noticed that the card gets detected anyway without the GPIO wiring, so I'm fine if the code gets removed instead. Best regards, Bernhard [1] <https://github.com/torvalds/linux/blob/v6.13/Documentation/devicetree/bindings/mmc/mmc-controller.yaml#L60> [2] <https://github.com/torvalds/linux/blob/v6.13/arch/arm64/boot/dts/freescale/imx8mp-evk.dts#L776> > >thanks >-- PMM
On Mon, 27 Jan 2025 at 23:11, Bernhard Beschow <shentey@gmail.com> wrote: > > > > Am 27. Januar 2025 13:24:46 UTC schrieb Peter Maydell <peter.maydell@linaro.org>: > >On Sat, 11 Jan 2025 at 18:37, Bernhard Beschow <shentey@gmail.com> wrote: > >> > >> Commit ce5dd27534b0 "hw/sd: Remove omap2_mmc device" removed the last user of > >> sd_set_cb(). Rework this functionality into GPIOs. > >> > >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> > > > >What is this for? We have a non-legacy API for "the SD controller > >needs to know when the SD card is inserted or the readonly > >status changes", which is that the controller implements the > >SDBasClass set_inserted and set_readonly methods. (See the pl011 > >for an example.) > > > >I would prefer it if we used that consistently, rather than having > >two mechanisms, one using GPIO lines and one using class methods. > >I think we should delete the sd_set_cb() API and handling code > >entirely. > > According to the Linux MMC controller DT schema, there are actually two ways to implement cd and wp lines [1]. When implementing the imx8mp-evk board, I thought I would need to model the GPIO style [2], hence I implemented it plus the active low part on the SD card. Later I noticed that the card gets detected anyway without the GPIO wiring, so I'm fine if the code gets removed instead. Even if you did need to implement that GPIO wiring, I think the right way to do it is for the SD controller to implement a subclass of SDBusClass so it can have its own implementations of the set_inserted and set_readonly methods, and then it is the SD controller object that has the GPIO lines, not the SD card itself. (I have a series almost ready to send out which QOMifies the omap_mmc device and then deletes the "legacy" SD API, including sd_set_cb().) thanks -- PMM
Am 28. Januar 2025 10:34:23 UTC schrieb Peter Maydell <peter.maydell@linaro.org>: >On Mon, 27 Jan 2025 at 23:11, Bernhard Beschow <shentey@gmail.com> wrote: >> >> >> >> Am 27. Januar 2025 13:24:46 UTC schrieb Peter Maydell <peter.maydell@linaro.org>: >> >On Sat, 11 Jan 2025 at 18:37, Bernhard Beschow <shentey@gmail.com> wrote: >> >> >> >> Commit ce5dd27534b0 "hw/sd: Remove omap2_mmc device" removed the last user of >> >> sd_set_cb(). Rework this functionality into GPIOs. >> >> >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> > >> >What is this for? We have a non-legacy API for "the SD controller >> >needs to know when the SD card is inserted or the readonly >> >status changes", which is that the controller implements the >> >SDBasClass set_inserted and set_readonly methods. (See the pl011 >> >for an example.) >> > >> >I would prefer it if we used that consistently, rather than having >> >two mechanisms, one using GPIO lines and one using class methods. >> >I think we should delete the sd_set_cb() API and handling code >> >entirely. >> >> According to the Linux MMC controller DT schema, there are actually two ways to implement cd and wp lines [1]. When implementing the imx8mp-evk board, I thought I would need to model the GPIO style [2], hence I implemented it plus the active low part on the SD card. Later I noticed that the card gets detected anyway without the GPIO wiring, so I'm fine if the code gets removed instead. > >Even if you did need to implement that GPIO wiring, I think the >right way to do it is for the SD controller to implement a >subclass of SDBusClass so it can have its own implementations >of the set_inserted and set_readonly methods, and then it >is the SD controller object that has the GPIO lines, not >the SD card itself. I agree. > >(I have a series almost ready to send out which QOMifies >the omap_mmc device and then deletes the "legacy" SD API, >including sd_set_cb().) Okay, let's just remove this legacy API. Best regards, Bernhard > >thanks >-- PMM
diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h index 0dc3889555..a121232560 100644 --- a/include/hw/sd/sdcard_legacy.h +++ b/include/hw/sd/sdcard_legacy.h @@ -36,7 +36,6 @@ SDState *sd_init(BlockBackend *blk, bool is_spi); int sd_do_command(SDState *card, SDRequest *request, uint8_t *response); void sd_write_byte(SDState *card, uint8_t value); uint8_t sd_read_byte(SDState *card); -void sd_set_cb(SDState *card, qemu_irq readonly, qemu_irq insert); /* sd_enable should not be used -- it is only used on the nseries boards, * where it is part of a broken implementation of the MMC card slot switch diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 0330d432fd..aa8d86e1af 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -820,6 +820,16 @@ static inline uint64_t sd_addr_to_wpnum(uint64_t addr) return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT); } +static bool sd_get_inserted(SDState *sd) +{ + return sd->blk && blk_is_inserted(sd->blk); +} + +static bool sd_get_readonly(SDState *sd) +{ + return sd->wp_switch; +} + static void sd_reset(DeviceState *dev) { SDState *sd = SDMMC_COMMON(dev); @@ -865,16 +875,9 @@ static void sd_reset(DeviceState *dev) sd->dat_lines = 0xf; sd->cmd_line = true; sd->multi_blk_cnt = 0; -} -static bool sd_get_inserted(SDState *sd) -{ - return sd->blk && blk_is_inserted(sd->blk); -} - -static bool sd_get_readonly(SDState *sd) -{ - return sd->wp_switch; + qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd)); + qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd)); } static void sd_cardchange(void *opaque, bool load, Error **errp) @@ -1034,14 +1037,6 @@ SDState *sd_init(BlockBackend *blk, bool is_spi) return sd; } -void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert) -{ - sd->readonly_cb = readonly; - sd->inserted_cb = insert; - qemu_set_irq(readonly, sd->blk ? !blk_is_writable(sd->blk) : 0); - qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0); -} - static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len) { trace_sdcard_read_block(addr, len); @@ -2727,6 +2722,8 @@ static void sd_instance_init(Object *obj) sd->last_cmd_name = "UNSET"; sd->enable = true; sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd); + qdev_init_gpio_out_named(DEVICE(sd), &sd->inserted_cb, "cd", 1); + qdev_init_gpio_out_named(DEVICE(sd), &sd->readonly_cb, "wp", 1); } static void sd_instance_finalize(Object *obj)