Re: [PATCH v2 1/2] ide: Make room for flags in PCIIDEState and add one for legacy mode

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 27/12/2020 22:13, BALATON Zoltan wrote:

> We'll need a flag for implementing some device specific behaviour in
> via-ide but we already have a currently CMD646 specific field that can
> be repurposed for this and leave room for further flags if needed in
> the future. This patch changes the "secondary" field to "flags" and
> change CMD646 and its users accordingly and define a new flag for
> forcing legacy mode that will be used by via-ide for now.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Fixed typo in commit message
> 
>   hw/ide/cmd646.c      | 4 ++--
>   hw/sparc64/sun4u.c   | 2 +-
>   include/hw/ide/pci.h | 7 ++++++-
>   3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index c254631485..7a96016116 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>       pci_conf[PCI_CLASS_PROG] = 0x8f;
>   
>       pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0

Doesn't the existing comment above cause checkpatch to fail?

> -    if (d->secondary) {
> +    if (d->flags & BIT(PCI_IDE_SECONDARY)) {
>           /* XXX: if not enabled, really disable the seconday IDE controller */
>           pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
>       }
> @@ -314,7 +314,7 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>   }
>   
>   static Property cmd646_ide_properties[] = {
> -    DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
> +    DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, false),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 0fa13a7330..c46baa9f48 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -674,7 +674,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>       }
>   
>       pci_dev = pci_new(PCI_DEVFN(3, 0), "cmd646-ide");
> -    qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
> +    qdev_prop_set_bit(&pci_dev->qdev, "secondary", true);
>       pci_realize_and_unref(pci_dev, pci_busA, &error_fatal);
>       pci_ide_create_devs(pci_dev);
>   
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index d8384e1c42..75d1a32f6d 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -42,6 +42,11 @@ typedef struct BMDMAState {
>   #define TYPE_PCI_IDE "pci-ide"
>   OBJECT_DECLARE_SIMPLE_TYPE(PCIIDEState, PCI_IDE)
>   
> +enum {
> +    PCI_IDE_SECONDARY, /* used only for cmd646 */
> +    PCI_IDE_LEGACY_MODE
> +};
> +
>   struct PCIIDEState {
>       /*< private >*/
>       PCIDevice parent_obj;
> @@ -49,7 +54,7 @@ struct PCIIDEState {
>   
>       IDEBus bus[2];
>       BMDMAState bmdma[2];
> -    uint32_t secondary; /* used only for cmd646 */
> +    uint32_t flags;
>       MemoryRegion bmdma_bar;
>       MemoryRegion cmd_bar[2];
>       MemoryRegion data_bar[2];

Other than that I think this looks okay.


ATB,

Mark.