On Mon, 28 Dec 2020, Mark Cave-Ayland wrote:
> 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?

It didn't (nether when I ran it nor in patchew) but I can fix it if I send 
a new version.

Regards,
BALATON Zoltan

>> -    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.
>
>