| Message ID | 1376406755-23703-1-git-send-email-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 08/13/2013 05:12 PM, Andre Przywara wrote: Ian, what about this one? This is a real showstopper on Midway. Thanks, Andre. > The PL011 IMSC register description is somehow fuzzy in the > documentation; by comparing it with the Linux implementation one can > see that the logic is actually reversed to Xen's implementation: > A "0" in field means interrupt disabled, a "1" enables it. > Therefore we enabled all interrupts instead of disabling them in the > beginning and later on masked the wrong interrupts. > Unclear how this worked on the Versatile Express, but this fix is > needed to get Calxeda Midway running (and works on VExpress, too). > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/drivers/char/pl011.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c > index 05d034f..8e90520 100644 > --- a/xen/drivers/char/pl011.c > +++ b/xen/drivers/char/pl011.c > @@ -87,7 +87,7 @@ static void __init pl011_init_preirq(struct serial_port *port) > unsigned int divisor; > > /* No interrupts, please. */ > - pl011_write(uart, IMSC, ALLI); > + pl011_write(uart, IMSC, 0); > > /* Definitely no DMA */ > pl011_write(uart, DMACR, 0x0); > @@ -115,7 +115,7 @@ static void __init pl011_init_preirq(struct serial_port *port) > pl011_write(uart, RSR, 0); > > /* Mask and clear the interrupts */ > - pl011_write(uart, IMSC, ALLI); > + pl011_write(uart, IMSC, 0); > pl011_write(uart, ICR, ALLI); > > /* Enable the UART for RX and TX; no flow ctrl */ > @@ -140,7 +140,7 @@ static void __init pl011_init_postirq(struct serial_port *port) > pl011_write(uart, ICR, OEI|BEI|PEI|FEI); > > /* Unmask interrupts */ > - pl011_write(uart, IMSC, RTI|DSRMI|DCDMI|CTSMI|RIMI); > + pl011_write(uart, IMSC, OEI|BEI|PEI|FEI|TXI|RXI); > } > > static void pl011_suspend(struct serial_port *port) >
On Tue, 2013-08-13 at 17:12 +0200, Andre Przywara wrote: > The PL011 IMSC register description is somehow fuzzy in the > documentation; OMG pl011 docs r1p5 issue g says: On a read this register returns the current value of the mask on the relevant interrupt. On a write of 1 to the particular bit, it sets the corresponding mask of that interrupt. A write of 0 clears the corresponding mask. Which is perfectly obvious if you read it understanding "mask an interrupt" in the normal way, but in the introduction to the section on interrupts it says: You can enable or disable the individual interrupts by changing the mask bits in the Interrupt Mask Set/Clear Register, UARTIMSC on page 3-17. Setting the appropriate mask bit HIGH enables the interrupt. So IMSC is actually the "mask" of the interrupts which are enabled. What crappy docs! > by comparing it with the Linux implementation one can > see that the logic is actually reversed to Xen's implementation: > A "0" in field means interrupt disabled, a "1" enables it. > > Therefore we enabled all interrupts instead of disabling them in the > beginning and later on masked the wrong interrupts. > Unclear how this worked on the Versatile Express, but this fix is > needed to get Calxeda Midway running (and works on VExpress, too). How on earth we got away with this I've idea! > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> Ack + applied, thanks!
On 08/21/2013 12:04 PM, Ian Campbell wrote: > On Tue, 2013-08-13 at 17:12 +0200, Andre Przywara wrote: >> The PL011 IMSC register description is somehow fuzzy in the >> documentation; > > OMG pl011 docs r1p5 issue g says: > On a read this register returns the current value of the mask on > the relevant interrupt. On a write of 1 to the particular bit, > it sets the corresponding mask of that interrupt. A write of 0 > clears the corresponding mask. > > Which is perfectly obvious if you read it understanding "mask an > interrupt" in the normal way, but in the introduction to the section on > interrupts it says: > > You can enable or disable the individual interrupts by changing > the mask bits in the Interrupt Mask Set/Clear Register, UARTIMSC > on page 3-17. Setting the appropriate mask bit HIGH enables the > interrupt. > > So IMSC is actually the "mask" of the interrupts which are enabled. > > What crappy docs! I agree, I stumbled on this by reading IMSC and wondered how it could be just 0 in the first place, then looking at the Linux code... And honestly I didn't even find the first paragraph, but was wondering why they repeat that "1 sets the mask, 0 clears it" all over the place instead of explicitly stating the meaning for the interrupt ;-) Regards, Andre. >> by comparing it with the Linux implementation one can >> see that the logic is actually reversed to Xen's implementation: >> A "0" in field means interrupt disabled, a "1" enables it. >> >> Therefore we enabled all interrupts instead of disabling them in the >> beginning and later on masked the wrong interrupts. >> Unclear how this worked on the Versatile Express, but this fix is >> needed to get Calxeda Midway running (and works on VExpress, too). > > How on earth we got away with this I've idea! > >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > > Ack + applied, thanks! > >
On Wed, 2013-08-21 at 15:58 +0200, Andre Przywara wrote: > On 08/21/2013 12:04 PM, Ian Campbell wrote: > > On Tue, 2013-08-13 at 17:12 +0200, Andre Przywara wrote: > >> The PL011 IMSC register description is somehow fuzzy in the > >> documentation; > > > > OMG pl011 docs r1p5 issue g says: > > On a read this register returns the current value of the mask on > > the relevant interrupt. On a write of 1 to the particular bit, > > it sets the corresponding mask of that interrupt. A write of 0 > > clears the corresponding mask. > > > > Which is perfectly obvious if you read it understanding "mask an > > interrupt" in the normal way, but in the introduction to the section on > > interrupts it says: > > > > You can enable or disable the individual interrupts by changing > > the mask bits in the Interrupt Mask Set/Clear Register, UARTIMSC > > on page 3-17. Setting the appropriate mask bit HIGH enables the > > interrupt. > > > > So IMSC is actually the "mask" of the interrupts which are enabled. > > > > What crappy docs! > > I agree, I stumbled on this by reading IMSC and wondered how it could be > just 0 in the first place, I wondered that too, but dismissed it as "mad hardware" without thinking carefully enough! Ian.
On Wed, 2013-08-21 at 15:58 +0200, Andre Przywara wrote: > I stumbled on this by reading IMSC and wondered how it could be > just 0 in the first place, I wondered that too, but dismissed it as "mad hardware" without thinking carefully enough! Ian.
On 08/13/2013 04:12 PM, Andre Przywara wrote: > The PL011 IMSC register description is somehow fuzzy in the > documentation; by comparing it with the Linux implementation one can > see that the logic is actually reversed to Xen's implementation: > A "0" in field means interrupt disabled, a "1" enables it. > Therefore we enabled all interrupts instead of disabling them in the > beginning and later on masked the wrong interrupts. > Unclear how this worked on the Versatile Express, but this fix is > needed to get Calxeda Midway running (and works on VExpress, too). On my Versatile Express, the keyboard is unusable with this patch. Xen receives random keys and sometimes nothing is printed on the serial port. By reverting this patch on my tree, I'm able to use correctly the serial port. > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/drivers/char/pl011.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c > index 05d034f..8e90520 100644 > --- a/xen/drivers/char/pl011.c > +++ b/xen/drivers/char/pl011.c > @@ -87,7 +87,7 @@ static void __init pl011_init_preirq(struct serial_port *port) > unsigned int divisor; > > /* No interrupts, please. */ > - pl011_write(uart, IMSC, ALLI); > + pl011_write(uart, IMSC, 0); > > /* Definitely no DMA */ > pl011_write(uart, DMACR, 0x0); > @@ -115,7 +115,7 @@ static void __init pl011_init_preirq(struct serial_port *port) > pl011_write(uart, RSR, 0); > > /* Mask and clear the interrupts */ > - pl011_write(uart, IMSC, ALLI); > + pl011_write(uart, IMSC, 0); > pl011_write(uart, ICR, ALLI); > > /* Enable the UART for RX and TX; no flow ctrl */ > @@ -140,7 +140,7 @@ static void __init pl011_init_postirq(struct serial_port *port) > pl011_write(uart, ICR, OEI|BEI|PEI|FEI); > > /* Unmask interrupts */ > - pl011_write(uart, IMSC, RTI|DSRMI|DCDMI|CTSMI|RIMI); > + pl011_write(uart, IMSC, OEI|BEI|PEI|FEI|TXI|RXI); > } > > static void pl011_suspend(struct serial_port *port) >
On Wed, 2013-08-21 at 17:11 +0100, Julien Grall wrote: > On 08/13/2013 04:12 PM, Andre Przywara wrote: > > The PL011 IMSC register description is somehow fuzzy in the > > documentation; by comparing it with the Linux implementation one can > > see that the logic is actually reversed to Xen's implementation: > > A "0" in field means interrupt disabled, a "1" enables it. > > Therefore we enabled all interrupts instead of disabling them in the > > beginning and later on masked the wrong interrupts. > > Unclear how this worked on the Versatile Express, but this fix is > > needed to get Calxeda Midway running (and works on VExpress, too). > > On my Versatile Express, the keyboard is unusable with this patch. > Xen receives random keys and sometimes nothing is printed on the serial > port. > > By reverting this patch on my tree, I'm able to use correctly the serial > port. :-/ Andre did say this patch worked on vexpress for him. I'm pretty certain Andre's patch is correct in its own right. But the fact that it worked before does seem to imply that there are other issues with the pl011 driver, it's likely that this change has just exposed a latent one. Could this be related somehow to the baud rate setting change? I wouldn't have expected so but "random keys" and nothingness could be a symptom of incorrect baud too. Does anyone have time to look into this?
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c index 05d034f..8e90520 100644 --- a/xen/drivers/char/pl011.c +++ b/xen/drivers/char/pl011.c @@ -87,7 +87,7 @@ static void __init pl011_init_preirq(struct serial_port *port) unsigned int divisor; /* No interrupts, please. */ - pl011_write(uart, IMSC, ALLI); + pl011_write(uart, IMSC, 0); /* Definitely no DMA */ pl011_write(uart, DMACR, 0x0); @@ -115,7 +115,7 @@ static void __init pl011_init_preirq(struct serial_port *port) pl011_write(uart, RSR, 0); /* Mask and clear the interrupts */ - pl011_write(uart, IMSC, ALLI); + pl011_write(uart, IMSC, 0); pl011_write(uart, ICR, ALLI); /* Enable the UART for RX and TX; no flow ctrl */ @@ -140,7 +140,7 @@ static void __init pl011_init_postirq(struct serial_port *port) pl011_write(uart, ICR, OEI|BEI|PEI|FEI); /* Unmask interrupts */ - pl011_write(uart, IMSC, RTI|DSRMI|DCDMI|CTSMI|RIMI); + pl011_write(uart, IMSC, OEI|BEI|PEI|FEI|TXI|RXI); } static void pl011_suspend(struct serial_port *port)
The PL011 IMSC register description is somehow fuzzy in the documentation; by comparing it with the Linux implementation one can see that the logic is actually reversed to Xen's implementation: A "0" in field means interrupt disabled, a "1" enables it. Therefore we enabled all interrupts instead of disabling them in the beginning and later on masked the wrong interrupts. Unclear how this worked on the Versatile Express, but this fix is needed to get Calxeda Midway running (and works on VExpress, too). Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/drivers/char/pl011.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)