Re: [PATCH] hw/ide/microdrive: Use device_cold_reset() for self-resets

[Philippe Mathieu-Daudé]

On 13/10/22 19:40, Peter Maydell wrote:
> Currently the microdrive code uses device_legacy_reset() to reset
> itself, and has its reset method call reset on the IDE bus as the
> last thing it does.  Switch to using device_cold_reset().
>
> The only concrete microdrive device is the TYPE_DSCM1XXXX; it is not
> command-line pluggable, so it is used only by the old pxa2xx Arm
> boards 'akita', 'borzoi', 'spitz', 'terrier' and 'tosa'.

Candidates for deprecation?

> You might think that this would result in the IDE bus being
> reset automatically, but it does not, because the IDEBus type
> does not set the BusClass::reset method. Instead the controller
> must explicitly call ide_bus_reset(). We therefore leave that
> call in md_reset().
>
> Note also that because the PCMCIA card device is a direct subclass of
> TYPE_DEVICE and we don't model the PCMCIA controller-to-card
> interface as a qbus, PCMCIA cards are not on any qbus and so they
> don't get reset when the system is reset.  The reset only happens via
> the dscm1xxxx_attach() and dscm1xxxx_detach() functions during
> machine creation.
>
> Because our aim here is merely to try to get rid of calls to the
> device_legacy_reset() function, we leave these other dubious
> reset-related issues alone.  (They all stem from this code being
> absolutely ancient.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/ide/microdrive.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
> index 6df9b4cbbe1..56c5be36551 100644
> --- a/hw/ide/microdrive.c
> +++ b/hw/ide/microdrive.c
> @@ -175,7 +175,7 @@ static void md_attr_write(PCMCIACardState *card, uint32_t 
> at, uint8_t value)
>       case 0x00:        /* Configuration Option Register */
>           s->opt = value & 0xcf;
>           if (value & OPT_SRESET) {
> -            device_legacy_reset(DEVICE(s));
> +            device_cold_reset(DEVICE(s));

I agree this one is dubious.

>           }
>           md_interrupt_update(s);
>           break;
> @@ -318,7 +318,7 @@ static void md_common_write(PCMCIACardState *card, uint32_t 
> at, uint16_t value)
>       case 0xe: /* Device Control */
>           s->ctrl = value;
>           if (value & CTRL_SRST) {
> -            device_legacy_reset(DEVICE(s));
> +            device_cold_reset(DEVICE(s));

Ditto (dubious).

>           }
>           md_interrupt_update(s);
>           break;
> @@ -543,7 +543,7 @@ static int dscm1xxxx_attach(PCMCIACardState *card)
>       md->attr_base = pcc->cis[0x74] | (pcc->cis[0x76] << 8);
>       md->io_base = 0x0;
>   
> -    device_legacy_reset(DEVICE(md));
> +    device_cold_reset(DEVICE(md));

This one makes sense.

>       md_interrupt_update(md);
>   
>       return 0;
> @@ -553,7 +553,7 @@ static int dscm1xxxx_detach(PCMCIACardState *card)
>   {
>       MicroDriveState *md = MICRODRIVE(card);
>   
> -    device_legacy_reset(DEVICE(md));
> +    device_cold_reset(DEVICE(md));

Ditto (correct).

>       return 0;
>   }
>   

Hoping this help to get ride of device_legacy_reset():
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>