diff mbox series

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

Message ID 19d68b4da7fc8dbffe3308c661143584ac830f29.1609107222.git.balaton@eik.bme.hu (mailing list archive)
State New, archived
Headers show
Series Fix via-ide for fuloong2e | expand

Commit Message

BALATON Zoltan Dec. 27, 2020, 10:13 p.m. UTC 
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(-)

Comments

Mark Cave-Ayland Dec. 28, 2020, 7:30 p.m. UTC | #1 
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.
BALATON Zoltan Dec. 28, 2020, 8:23 p.m. UTC | #2 
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.
>
>
Philippe Mathieu-Daudé Dec. 29, 2020, 10:52 a.m. UTC | #3 
On 12/28/20 8:30 PM, 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?

The comment is old and Balaton didn't modified it. Usually I'd prefer
to address style change in a separate commit, ...

> 
>> -    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 */

... but since a similar comment is added here, it might be acceptable
to fix the style of the former one here too. I noted Balaton already
addressed your comment in a v3.

>>       }
>> @@ -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 mbox series

Patch

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
-    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];