[v2] i.MX and SDHCI improvements

[PATCH v2 06/13] hw/sd/sd: Remove legacy sd_set_cb() in favor of GPIOs

Posted by Bernhard Beschow 2 months, 3 weeks ago

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>
---
 include/hw/sd/sdcard_legacy.h |  1 -
 hw/sd/sd.c                    | 31 ++++++++++++++-----------------
 2 files changed, 14 insertions(+), 18 deletions(-)

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)
-- 
2.48.0

Re: [PATCH v2 06/13] hw/sd/sd: Remove legacy sd_set_cb() in favor of GPIOs

Posted by Peter Maydell 2 months, 1 week ago

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

Re: [PATCH v2 06/13] hw/sd/sd: Remove legacy sd_set_cb() in favor of GPIOs

Posted by Bernhard Beschow 2 months, 1 week ago


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

Re: [PATCH v2 06/13] hw/sd/sd: Remove legacy sd_set_cb() in favor of GPIOs

Posted by Peter Maydell 2 months, 1 week ago

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

Re: [PATCH v2 06/13] hw/sd/sd: Remove legacy sd_set_cb() in favor of GPIOs

Posted by Bernhard Beschow 2 months, 1 week ago


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