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

[Mark Cave-Ayland]

On 29/02/2020 23:02, 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 furhter flags if needed in
> the future. This patch changes the "secondary" field to "flags" and
> define the flags for CMD646 and via-ide and change CMD646 and its
> users accordingly.
> 
> Signed-off-by: BALATON Zoltan <address@hidden>
> ---
>  hw/alpha/dp264.c     |  2 +-
>  hw/ide/cmd646.c      | 12 ++++++------
>  hw/sparc64/sun4u.c   |  9 ++-------
>  include/hw/ide.h     |  4 ++--
>  include/hw/ide/pci.h |  7 ++++++-
>  5 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index 8d71a30617..e4075feaaf 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -102,7 +102,7 @@ static void clipper_init(MachineState *machine)
>          DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>          ide_drive_get(hd, ARRAY_SIZE(hd));
>  
> -        pci_cmd646_ide_init(pci_bus, hd, 0);
> +        pci_cmd646_ide_init(pci_bus, hd, -1, false);
>      }
>  
>      /* Load PALcode.  Given that this is not "real" cpu palcode,
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index 335c060673..0be650791f 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
> -    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 */
>      }
> @@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>      }
>  }
>  
> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> -                         int secondary_ide_enabled)
> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
> +                         bool secondary_ide_enabled)
>  {
>      PCIDevice *dev;
>  
> -    dev = pci_create(bus, -1, "cmd646-ide");
> -    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
> +    dev = pci_create(bus, devfn, "cmd646-ide");
> +    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
>      qdev_init_nofail(&dev->qdev);
>  
>      pci_ide_create_devs(dev, hd_table);
>  }

Note that legacy init functions such as pci_cmd646_ide_init() should be removed 
where
possible, and in fact I posted a patch last week to completely remove it. This 
is
because using qdev directly allows each board to wire up the device as required,
rather than pushing it down into a set of init functions with different 
defaults.

Given that you're working in this area, I'd highly recommend doing the same for
via_ide_init() too.

>  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 b7ac42f7a5..b64899300c 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -50,8 +50,7 @@
>  #include "hw/sparc/sparc64.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/sysbus.h"
> -#include "hw/ide.h"
> -#include "hw/ide/pci.h"
> +#include "hw/ide/internal.h"
>  #include "hw/loader.h"
>  #include "hw/fw-path-provider.h"
>  #include "elf.h"
> @@ -664,11 +663,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>      }
>  
>      ide_drive_get(hd, ARRAY_SIZE(hd));
> -
> -    pci_dev = pci_create(pci_busA, PCI_DEVFN(3, 0), "cmd646-ide");
> -    qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
> -    qdev_init_nofail(&pci_dev->qdev);
> -    pci_ide_create_devs(pci_dev, hd);
> +    pci_cmd646_ide_init(pci_busA, hd, PCI_DEVFN(3, 0), true);
>  
>      /* Map NVRAM into I/O (ebus) space */
>      nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index 28d8a06439..d88c5ee695 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -12,8 +12,8 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int 
> iobase2, int isairq,
>                          DriveInfo *hd0, DriveInfo *hd1);
>  
>  /* ide-pci.c */
> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> -                         int secondary_ide_enabled);
> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
> +                         bool secondary_ide_enabled);
>  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
> devfn);
>  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index a9f2c33e68..21075edf16 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -40,6 +40,11 @@ typedef struct BMDMAState {
>  #define TYPE_PCI_IDE "pci-ide"
>  #define PCI_IDE(obj) OBJECT_CHECK(PCIIDEState, (obj), TYPE_PCI_IDE)
>  
> +enum {
> +    PCI_IDE_SECONDARY, /* used only for cmd646 */
> +    PCI_IDE_LEGACY_IRQ
> +};
> +
>  typedef struct PCIIDEState {
>      /*< private >*/
>      PCIDevice parent_obj;
> @@ -47,7 +52,7 @@ typedef 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];

ATB,

Mark.