diff mbox

serial-pci: Set prog interface field of pci config to 16550 compatible

Message ID 20140227011300.8CD451983B@mono.eik.bme.hu
State New
Headers show

Commit Message

BALATON Zoltan Feb. 27, 2014, 1:05 a.m. UTC 
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/char/serial-pci.c | 1 +
 1 file changed, 1 insertion(+)

Comments

BALATON Zoltan March 10, 2014, 6:40 p.m. UTC | #1 
Ping!
http://patchwork.ozlabs.org/patch/324674/

On Thu, 27 Feb 2014, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/char/serial-pci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
> index 991c99f..e662b77 100644
> --- a/hw/char/serial-pci.c
> +++ b/hw/char/serial-pci.c
> @@ -60,6 +60,7 @@ static int serial_pci_init(PCIDevice *dev)
>         return -1;
>     }
>
> +    pci->dev.config[PCI_CLASS_PROG] = 0x02; /* 16550 compatible */
>     pci->dev.config[PCI_INTERRUPT_PIN] = 0x01;
>     s->irq = pci_allocate_irq(&pci->dev);
>
> -- 
> 1.8.1.5
Michael Tokarev March 14, 2014, 4:21 p.m. UTC | #2 
10.03.2014 22:40, BALATON Zoltan wrote:
> Ping!
> http://patchwork.ozlabs.org/patch/324674/
> 
> On Thu, 27 Feb 2014, BALATON Zoltan wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/char/serial-pci.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
>> index 991c99f..e662b77 100644
>> --- a/hw/char/serial-pci.c
>> +++ b/hw/char/serial-pci.c
>> @@ -60,6 +60,7 @@ static int serial_pci_init(PCIDevice *dev)
>>         return -1;
>>     }
>>
>> +    pci->dev.config[PCI_CLASS_PROG] = 0x02; /* 16550 compatible */
>>     pci->dev.config[PCI_INTERRUPT_PIN] = 0x01;
>>     s->irq = pci_allocate_irq(&pci->dev);

Hm. While this is an one-liner indeed, I'm not sure it's the right
value.  From my pci.ids:

C 07  Communication controller
        00  Serial controller
                00  8250
                01  16450
                02  16550
                03  16650
                04  16750
                05  16850
                06  16950

so it seems correct thing to do.

What about multi_serial_pci_init() -- should it too set prog-if
like this?

Thanks,

/mjt
BALATON Zoltan March 14, 2014, 5:44 p.m. UTC | #3 
On Fri, 14 Mar 2014, Michael Tokarev wrote:
> 10.03.2014 22:40, BALATON Zoltan wrote:
>> Ping!
>> http://patchwork.ozlabs.org/patch/324674/
>>
>> On Thu, 27 Feb 2014, BALATON Zoltan wrote:
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/char/serial-pci.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
>>> index 991c99f..e662b77 100644
>>> --- a/hw/char/serial-pci.c
>>> +++ b/hw/char/serial-pci.c
>>> @@ -60,6 +60,7 @@ static int serial_pci_init(PCIDevice *dev)
>>>         return -1;
>>>     }
>>>
>>> +    pci->dev.config[PCI_CLASS_PROG] = 0x02; /* 16550 compatible */
>>>     pci->dev.config[PCI_INTERRUPT_PIN] = 0x01;
>>>     s->irq = pci_allocate_irq(&pci->dev);
>
> Hm. While this is an one-liner indeed, I'm not sure it's the right
> value.  From my pci.ids:
>
> C 07  Communication controller
>        00  Serial controller
>                00  8250
>                01  16450
>                02  16550
>                03  16650
>                04  16750
>                05  16850
>                06  16950
>
> so it seems correct thing to do.
>
> What about multi_serial_pci_init() -- should it too set prog-if
> like this?

I'm not completely sure what the correct value should be for 
multi_serial_pci_init so I've left that alone. There's also 07 02 00 that 
means multiport serial controller that might apply for that case. For the 
simple case I've sent the patch for I had a client expecting this value so 
I think it is correct for that at least but I don't know about the 
multiport case.

Regards,
BALATON Zoltan
BALATON Zoltan March 14, 2014, 5:57 p.m. UTC | #4 
On Fri, 14 Mar 2014, BALATON Zoltan wrote:
> On Fri, 14 Mar 2014, Michael Tokarev wrote:
>> 10.03.2014 22:40, BALATON Zoltan wrote:
>>> Ping!
>>> http://patchwork.ozlabs.org/patch/324674/
>>> 
>>> On Thu, 27 Feb 2014, BALATON Zoltan wrote:
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> hw/char/serial-pci.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>> 
>>>> diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
>>>> index 991c99f..e662b77 100644
>>>> --- a/hw/char/serial-pci.c
>>>> +++ b/hw/char/serial-pci.c
>>>> @@ -60,6 +60,7 @@ static int serial_pci_init(PCIDevice *dev)
>>>>         return -1;
>>>>     }
>>>> 
>>>> +    pci->dev.config[PCI_CLASS_PROG] = 0x02; /* 16550 compatible */
>>>>     pci->dev.config[PCI_INTERRUPT_PIN] = 0x01;
>>>>     s->irq = pci_allocate_irq(&pci->dev);
>> 
>> Hm. While this is an one-liner indeed, I'm not sure it's the right
>> value.  From my pci.ids:
>> 
>> C 07  Communication controller
>>        00  Serial controller
>>                00  8250
>>                01  16450
>>                02  16550
>>                03  16650
>>                04  16750
>>                05  16850
>>                06  16950
>> 
>> so it seems correct thing to do.
>> 
>> What about multi_serial_pci_init() -- should it too set prog-if
>> like this?
>
> I'm not completely sure what the correct value should be for 
> multi_serial_pci_init so I've left that alone. There's also 07 02 00 that 
> means multiport serial controller that might apply for that case. For the 
> simple case I've sent the patch for I had a client expecting this value so I 
> think it is correct for that at least but I don't know about the multiport 
> case.

Also Gerd Hoffmann said he has picked this patch up so I'm adding him to 
cc too.

> Regards,
> BALATON Zoltan
Gerd Hoffmann March 17, 2014, 12:32 p.m. UTC | #5 
> >> C 07  Communication controller
> >>        00  Serial controller
> >>                00  8250
> >>                01  16450
> >>                02  16550

> >> What about multi_serial_pci_init() -- should it too set prog-if
> >> like this?
> >
> > I'm not completely sure what the correct value should be for 
> > multi_serial_pci_init so I've left that alone. There's also 07 02 00 that 
> > means multiport serial controller that might apply for that case. For the 
> > simple case I've sent the patch for I had a client expecting this value so I 
> > think it is correct for that at least but I don't know about the multiport 
> > case.
> 
> Also Gerd Hoffmann said he has picked this patch up so I'm adding him to 
> cc too.

The multiport variants are simply multiple 16550 chips mapped one after
the other.  So "02" is better than "00".

I don't know what exactly the multiport serial controller is supposed to
be.

cheers,
  Gerd
BALATON Zoltan March 20, 2014, 9:58 a.m. UTC | #6 
On Mon, 17 Mar 2014, Gerd Hoffmann wrote:
>>>> C 07  Communication controller
>>>>        00  Serial controller
>>>>                00  8250
>>>>                01  16450
>>>>                02  16550
>
>>>> What about multi_serial_pci_init() -- should it too set prog-if
>>>> like this?
>>>
>>> I'm not completely sure what the correct value should be for
>>> multi_serial_pci_init so I've left that alone. There's also 07 02 00 that
>>> means multiport serial controller that might apply for that case. For the
>>> simple case I've sent the patch for I had a client expecting this value so I
>>> think it is correct for that at least but I don't know about the multiport
>>> case.
>>
>> Also Gerd Hoffmann said he has picked this patch up so I'm adding him to
>> cc too.
>
> The multiport variants are simply multiple 16550 chips mapped one after
> the other.  So "02" is better than "00".
>
> I don't know what exactly the multiport serial controller is supposed to
> be.

So what should we do then? Should I send an updated patch which sets 
070002 in the multiport case too?

Regards,
BALATON Zoltan
Michael Tokarev March 20, 2014, 3:42 p.m. UTC | #7 
10.03.2014 22:40, BALATON Zoltan wrote:
> Ping!
> http://patchwork.ozlabs.org/patch/324674/

Thanks, applied to -trivial.

/mjt
Paolo Bonzini March 20, 2014, 3:51 p.m. UTC | #8 
Il 20/03/2014 16:42, Michael Tokarev ha scritto:
> 10.03.2014 22:40, BALATON Zoltan wrote:
>> Ping!
>> http://patchwork.ozlabs.org/patch/324674/
>
> Thanks, applied to -trivial.

No, please don't; this needs to be done for new machine types only.

Paolo
BALATON Zoltan March 20, 2014, 4:01 p.m. UTC | #9 
On Thu, 20 Mar 2014, Paolo Bonzini wrote:
> Il 20/03/2014 16:42, Michael Tokarev ha scritto:
>> 10.03.2014 22:40, BALATON Zoltan wrote:
>>> Ping!
>>> http://patchwork.ozlabs.org/patch/324674/
>> 
>> Thanks, applied to -trivial.
>
> No, please don't; this needs to be done for new machine types only.

Why? I tried running a Windows XP image that was set up before this patch 
and it did not complain about changed hardware. Or is there another reason 
I'm missing?

Regards,
BALATON Zoltan
Paolo Bonzini March 20, 2014, 4:32 p.m. UTC | #10 
Il 20/03/2014 17:01, BALATON Zoltan ha scritto:
> On Thu, 20 Mar 2014, Paolo Bonzini wrote:
>> Il 20/03/2014 16:42, Michael Tokarev ha scritto:
>>> 10.03.2014 22:40, BALATON Zoltan wrote:
>>>> Ping!
>>>> http://patchwork.ozlabs.org/patch/324674/
>>>
>>> Thanks, applied to -trivial.
>>
>> No, please don't; this needs to be done for new machine types only.
>
> Why? I tried running a Windows XP image that was set up before this
> patch and it did not complain about changed hardware. Or is there
> another reason I'm missing?

Classes (or anything guest-visible in fact) must never ever be changed 
for the same machine type.  It's a rule.

Paolo
BALATON Zoltan March 20, 2014, 4:58 p.m. UTC | #11 
On Thu, 20 Mar 2014, Paolo Bonzini wrote:
> Il 20/03/2014 17:01, BALATON Zoltan ha scritto:
>> On Thu, 20 Mar 2014, Paolo Bonzini wrote:
>>> Il 20/03/2014 16:42, Michael Tokarev ha scritto:
>>>> 10.03.2014 22:40, BALATON Zoltan wrote:
>>>>> Ping!
>>>>> http://patchwork.ozlabs.org/patch/324674/
>>>> 
>>>> Thanks, applied to -trivial.
>>> 
>>> No, please don't; this needs to be done for new machine types only.
>> 
>> Why? I tried running a Windows XP image that was set up before this
>> patch and it did not complain about changed hardware. Or is there
>> another reason I'm missing?
>
> Classes (or anything guest-visible in fact) must never ever be changed for 
> the same machine type.  It's a rule.

The class did not change only the prog interface field after it (which is 
guest visible but I think most guests just ignore it). So then what's the 
correct way to do it? Do I need to change the patch or can it be merged as 
it is the next time a new machine type is added?

Regards,
BALATON Zoltan
Paolo Bonzini March 20, 2014, 5:35 p.m. UTC | #12 
Il 20/03/2014 17:58, BALATON Zoltan ha scritto:
>>
>> Classes (or anything guest-visible in fact) must never ever be changed
>> for the same machine type.  It's a rule.
>
> The class did not change only the prog interface field after it (which
> is guest visible but I think most guests just ignore it). So then what's
> the correct way to do it? Do I need to change the patch or can it be
> merged as it is the next time a new machine type is added?

You can wait for the 2.1 machine type to be added.

Paolo
Michael Tokarev March 21, 2014, 8:19 a.m. UTC | #13 
20.03.2014 19:51, Paolo Bonzini wrote:
> Il 20/03/2014 16:42, Michael Tokarev ha scritto:
>> 10.03.2014 22:40, BALATON Zoltan wrote:
>>> Ping!
>>> http://patchwork.ozlabs.org/patch/324674/
>>
>> Thanks, applied to -trivial.
> 
> No, please don't; this needs to be done for new machine types only.

I thought about this too, but decided this is too minor a difference
to warrant special per-machine-type handling.  Oh well.

/mjt
Gerd Hoffmann March 21, 2014, 8:43 a.m. UTC | #14 
On Do, 2014-03-20 at 10:58 +0100, BALATON Zoltan wrote:
> So what should we do then? Should I send an updated patch which sets 
> 070002 in the multiport case too?

Yes, that would be great.

cheers,
  Gerd
BALATON Zoltan March 21, 2014, 9:10 a.m. UTC | #15 
On Fri, 21 Mar 2014, Gerd Hoffmann wrote:
> On Do, 2014-03-20 at 10:58 +0100, BALATON Zoltan wrote:
>> So what should we do then? Should I send an updated patch which sets
>> 070002 in the multiport case too?
>
> Yes, that would be great.

OK, I'll resubmit it after pc-2.1 is added also with the multiport case.

Thank you,
BALATON Zoltan
diff mbox

Patch

diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index 991c99f..e662b77 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -60,6 +60,7 @@  static int serial_pci_init(PCIDevice *dev)
         return -1;
     }
 
+    pci->dev.config[PCI_CLASS_PROG] = 0x02; /* 16550 compatible */
     pci->dev.config[PCI_INTERRUPT_PIN] = 0x01;
     s->irq = pci_allocate_irq(&pci->dev);