Re: [Xen-devel] [PATCH V8 3/3] arm/acpi: Add Server Base System Architecture UART support

[Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>]

On 06/09/2016 05:19 AM, Julien Grall wrote:
> Hello Shanker,
>
> On 08/06/16 14:28, Shanker Donthineni wrote:
> > The ARM Server Base System Architecture describes a generic UART
> > interface. It doesn't support clock control registers, modem
> > control, DMA and hardware flow control features. So, extend the
> > driver probe() to handle SBSA interface and skip the accessing
> > PL011 registers that are not described in SBSA document
> > (ARM-DEN-0029 Version 3.0, 6 APPENDIX B: GENERIC UART).
> >
> > Signed-off-by: Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>
> > Reviewed-by: Julien Grall <julien.grall@xxxxxxx>
> > ---
> > Changes sicne v7:
> >     Moved comment 'To compatible with SBSA v2.x document, all 
> > accesses should be 32-bit' from #2
> > Changes since v3:
> >    Moved non-SBSA related changes to patches 1/3 and 2/3.
> >
> > changes since v2:
> >    Edited commit text to include SBSA document version.
> >    Remove setting baudrate code completely as per Julien's suggestion.
> >    Support both the SBSA interface types ACPI_DBG2_SBSA & 
> > ACPI_DBG2_SBSA_32.
> >    Replace MIS references with combination of RIS & IMSC.
> >
> > Changes since v1:
> >    Don't access UART registers that are not part of SBSA document.
> >    Move setting baudrate function to a separate function.
> >
> >   xen/drivers/char/pl011.c | 55 
> > +++++++++++++++++++++++++++++++++++-------------
> >   1 file changed, 40 insertions(+), 15 deletions(-)
> >
> > diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> > index a2f929b..d70ec99 100644
> > --- a/xen/drivers/char/pl011.c
> > +++ b/xen/drivers/char/pl011.c
> > @@ -41,6 +41,7 @@ static struct pl011 {
> >       /* struct timer timer; */
> >       /* unsigned int timeout_ms; */
> >       /* bool_t probing, intr_works; */
> > +    bool sbsa;  /* ARM SBSA generic interface */
> >   } pl011_com = {0};
> >
> >   /* These parity settings can be ORed directly into the LCR. */
> > @@ -50,6 +51,7 @@ static struct pl011 {
> >   #define PARITY_MARK  (PEN|SPS)
> >   #define PARITY_SPACE (PEN|EPS|SPS)
> >
> > +/* To compatible with SBSA v2.x document, all accesses should be 
> > 32-bit */
> The verb is missing. Also please add a full stop at the end of the 
> comment.
> >   #define pl011_read(uart, off) readl((uart)->regs + (off))
> >   #define pl011_write(uart, off,val)      writel((val), (uart)->regs 
> > + (off))
> [...]
Sorry, I didn't understand what is [...]?
> > @@ -313,11 +323,15 @@ static int __init pl011_acpi_uart_init(const 
> > void *data)
> >           return -EINVAL;
> >       }
> >
> > +    if ( (spcr->interface_type == ACPI_DBG2_SBSA) ||
> > +         (spcr->interface_type == ACPI_DBG2_SBSA_32) )
> > +        sbsa = true;
> I thought I already mentioned this on a previous version:
>
> sbsa = (spcr->interface_type == ACPI_DBG2_SBSA || ...);
You want me change to

sbsa = (spcr->interface_type == ACPI_DBG2_SBSA || spcr->interface_type 
== ACPI_DBG2_SBSA_32)
right?
> Regards,
--
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel