| Message ID | 1408372091-12689-13-git-send-email-daniel.thompson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On 08/18/2014 10:28 AM, Daniel Thompson wrote: > Speculatively register a FIQ resource with KGDB. KGDB will only > accept it if the kgdb/fiq feature is enabled (both with compile time and > runtime switches) and the interrupt controller supports FIQ. > > By providing this information to KGDB the serial driver offers > "permission" for KGDB to route the UART interrupt signal from the > drivers own handler to KGDBs FIQ handler (which will eventually use the > UART's polled I/O callbacks to interact with the user). This permission > also implies the amba-pl011 driver has already unmasked RX interrupts > (otherwise the FIQ handler will never trigger). > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Jiri Slaby <jslaby@suse.cz> > Cc: linux-serial@vger.kernel.org > --- > drivers/tty/serial/amba-pl011.c | 94 ++++++++++++++++++++++++----------------- > 1 file changed, 55 insertions(+), 39 deletions(-) > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 8572f2a..ec8ddc7 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -58,6 +58,7 @@ > #include <linux/pinctrl/consumer.h> > #include <linux/sizes.h> > #include <linux/io.h> > +#include <linux/kgdb.h> > > #define UART_NR 14 > > @@ -1416,8 +1417,61 @@ static void pl011_break_ctl(struct uart_port *port, int break_state) > spin_unlock_irqrestore(&uap->port.lock, flags); > } > Is pl011_hwinit() just being relocated in source to avoid the forward declaration? If so, this is usually split into its own commit. > +static int pl011_hwinit(struct uart_port *port) > +{ > + struct uart_amba_port *uap = (struct uart_amba_port *)port; > + int retval; > + > + /* Optionaly enable pins to be muxed in and configured */ > + pinctrl_pm_select_default_state(port->dev); > + > + /* > + * Try to enable the clock producer. > + */ > + retval = clk_prepare_enable(uap->clk); > + if (retval) > + return retval; > + > + uap->port.uartclk = clk_get_rate(uap->clk); > + > + /* Clear pending error and receive interrupts */ > + writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS | > + UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR); > + > + /* > + * Save interrupts enable mask, and enable RX interrupts in case if > + * the interrupt is used for NMI entry. > + */ > + uap->im = readw(uap->port.membase + UART011_IMSC); > + writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC); > + > + if (dev_get_platdata(uap->port.dev)) { > + struct amba_pl011_data *plat; > + > + plat = dev_get_platdata(uap->port.dev); > + if (plat->init) > + plat->init(); > + } > + return 0; > +} > + > #ifdef CONFIG_CONSOLE_POLL > > +static int pl011_poll_init(struct uart_port *port) > +{ > + struct uart_amba_port *uap = (struct uart_amba_port *)port; Please use container_of() in new code. > + int retval; > + > + retval = pl011_hwinit(port); > + > +#ifdef CONFIG_KGDB_FIQ > + if (retval == 0) > + kgdb_register_fiq(uap->port.irq); The uap->port dereference is unnecessary since the port parameter is the same thing. kgdb_register_fiq(port->irq); Regards, Peter Hurley > +#endif > + > + return retval; > +} > + > static void pl011_quiesce_irqs(struct uart_port *port) > { > struct uart_amba_port *uap = (struct uart_amba_port *)port; > @@ -1471,44 +1525,6 @@ static void pl011_put_poll_char(struct uart_port *port, > > #endif /* CONFIG_CONSOLE_POLL */ > > -static int pl011_hwinit(struct uart_port *port) > -{ > - struct uart_amba_port *uap = (struct uart_amba_port *)port; > - int retval; > - > - /* Optionaly enable pins to be muxed in and configured */ > - pinctrl_pm_select_default_state(port->dev); > - > - /* > - * Try to enable the clock producer. > - */ > - retval = clk_prepare_enable(uap->clk); > - if (retval) > - return retval; > - > - uap->port.uartclk = clk_get_rate(uap->clk); > - > - /* Clear pending error and receive interrupts */ > - writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS | > - UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR); > - > - /* > - * Save interrupts enable mask, and enable RX interrupts in case if > - * the interrupt is used for NMI entry. > - */ > - uap->im = readw(uap->port.membase + UART011_IMSC); > - writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC); > - > - if (dev_get_platdata(uap->port.dev)) { > - struct amba_pl011_data *plat; > - > - plat = dev_get_platdata(uap->port.dev); > - if (plat->init) > - plat->init(); > - } > - return 0; > -} > - > static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h) > { > writew(lcr_h, uap->port.membase + uap->lcrh_rx); > @@ -1888,7 +1904,7 @@ static struct uart_ops amba_pl011_pops = { > .config_port = pl011_config_port, > .verify_port = pl011_verify_port, > #ifdef CONFIG_CONSOLE_POLL > - .poll_init = pl011_hwinit, > + .poll_init = pl011_poll_init, > .poll_get_char = pl011_get_poll_char, > .poll_put_char = pl011_put_poll_char, > #endif >
On 18/08/14 19:30, Peter Hurley wrote: > Hi Daniel, > > On 08/18/2014 10:28 AM, Daniel Thompson wrote: >> Speculatively register a FIQ resource with KGDB. KGDB will only >> accept it if the kgdb/fiq feature is enabled (both with compile time and >> runtime switches) and the interrupt controller supports FIQ. >> >> By providing this information to KGDB the serial driver offers >> "permission" for KGDB to route the UART interrupt signal from the >> drivers own handler to KGDBs FIQ handler (which will eventually use the >> UART's polled I/O callbacks to interact with the user). This permission >> also implies the amba-pl011 driver has already unmasked RX interrupts >> (otherwise the FIQ handler will never trigger). >> >> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> >> Cc: Russell King <linux@arm.linux.org.uk> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Jiri Slaby <jslaby@suse.cz> >> Cc: linux-serial@vger.kernel.org >> --- >> drivers/tty/serial/amba-pl011.c | 94 ++++++++++++++++++++++++----------------- >> 1 file changed, 55 insertions(+), 39 deletions(-)# Thanks for the review. >> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c >> index 8572f2a..ec8ddc7 100644 >> --- a/drivers/tty/serial/amba-pl011.c >> +++ b/drivers/tty/serial/amba-pl011.c >> @@ -58,6 +58,7 @@ >> #include <linux/pinctrl/consumer.h> >> #include <linux/sizes.h> >> #include <linux/io.h> >> +#include <linux/kgdb.h> >> >> #define UART_NR 14 >> >> @@ -1416,8 +1417,61 @@ static void pl011_break_ctl(struct uart_port *port, int break_state) >> spin_unlock_irqrestore(&uap->port.lock, flags); >> } >> > > Is pl011_hwinit() just being relocated in source to avoid the forward > declaration? If so, this is usually split into its own commit. Ok. I'll do this. >> +static int pl011_hwinit(struct uart_port *port) >> +{ >> + struct uart_amba_port *uap = (struct uart_amba_port *)port; >> + int retval; >> + >> + /* Optionaly enable pins to be muxed in and configured */ >> + pinctrl_pm_select_default_state(port->dev); >> + >> + /* >> + * Try to enable the clock producer. >> + */ >> + retval = clk_prepare_enable(uap->clk); >> + if (retval) >> + return retval; >> + >> + uap->port.uartclk = clk_get_rate(uap->clk); >> + >> + /* Clear pending error and receive interrupts */ >> + writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS | >> + UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR); >> + >> + /* >> + * Save interrupts enable mask, and enable RX interrupts in case if >> + * the interrupt is used for NMI entry. >> + */ >> + uap->im = readw(uap->port.membase + UART011_IMSC); >> + writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC); >> + >> + if (dev_get_platdata(uap->port.dev)) { >> + struct amba_pl011_data *plat; >> + >> + plat = dev_get_platdata(uap->port.dev); >> + if (plat->init) >> + plat->init(); >> + } >> + return 0; >> +} >> + >> #ifdef CONFIG_CONSOLE_POLL >> >> +static int pl011_poll_init(struct uart_port *port) >> +{ >> + struct uart_amba_port *uap = (struct uart_amba_port *)port; > > Please use container_of() in new code. Ok. Personally I dislike a file that mixes casts and conatiner_of but I guess I can make both of us happy by switching the whole driver to container_of. Separate patch again? >> + int retval; >> + >> + retval = pl011_hwinit(port); >> + >> +#ifdef CONFIG_KGDB_FIQ >> + if (retval == 0) >> + kgdb_register_fiq(uap->port.irq); > > The uap->port dereference is unnecessary since the port parameter > is the same thing. > > kgdb_register_fiq(port->irq); Ok.
On 08/19/2014 05:08 AM, Daniel Thompson wrote: > On 18/08/14 19:30, Peter Hurley wrote: [cut] >>> +static int pl011_poll_init(struct uart_port *port) >>> +{ >>> + struct uart_amba_port *uap = (struct uart_amba_port *)port; >> >> Please use container_of() in new code. > > Ok. > > Personally I dislike a file that mixes casts and conatiner_of but I > guess I can make both of us happy by switching the whole driver to > container_of. Separate patch again? The change below makes the uap local unnecessary, so you can skip the container_of() change, if you'd prefer. >>> + int retval; >>> + >>> + retval = pl011_hwinit(port); >>> + >>> +#ifdef CONFIG_KGDB_FIQ >>> + if (retval == 0) >>> + kgdb_register_fiq(uap->port.irq); >> >> The uap->port dereference is unnecessary since the port parameter >> is the same thing. >> >> kgdb_register_fiq(port->irq); > > Ok. Thanks, Peter Hurley
On 19/08/14 12:58, Peter Hurley wrote: > On 08/19/2014 05:08 AM, Daniel Thompson wrote: >> On 18/08/14 19:30, Peter Hurley wrote: > > [cut] > >>>> +static int pl011_poll_init(struct uart_port *port) >>>> +{ >>>> + struct uart_amba_port *uap = (struct uart_amba_port *)port; >>> >>> Please use container_of() in new code. >> >> Ok. >> >> Personally I dislike a file that mixes casts and conatiner_of but I >> guess I can make both of us happy by switching the whole driver to >> container_of. Separate patch again? > > The change below makes the uap local unnecessary, so you can skip the > container_of() change, if you'd prefer. I realized that myself although not until after I'd replaced all the casts with container_of()... So I'll keep the patch for now but can drop it if anyone takes against it. Thanks Daniel. > >>>> + int retval; >>>> + >>>> + retval = pl011_hwinit(port); >>>> + >>>> +#ifdef CONFIG_KGDB_FIQ >>>> + if (retval == 0) >>>> + kgdb_register_fiq(uap->port.irq); >>> >>> The uap->port dereference is unnecessary since the port parameter >>> is the same thing. >>> >>> kgdb_register_fiq(port->irq); >> >> Ok. > > Thanks, > Peter Hurley >
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 8572f2a..ec8ddc7 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -58,6 +58,7 @@ #include <linux/pinctrl/consumer.h> #include <linux/sizes.h> #include <linux/io.h> +#include <linux/kgdb.h> #define UART_NR 14 @@ -1416,8 +1417,61 @@ static void pl011_break_ctl(struct uart_port *port, int break_state) spin_unlock_irqrestore(&uap->port.lock, flags); } +static int pl011_hwinit(struct uart_port *port) +{ + struct uart_amba_port *uap = (struct uart_amba_port *)port; + int retval; + + /* Optionaly enable pins to be muxed in and configured */ + pinctrl_pm_select_default_state(port->dev); + + /* + * Try to enable the clock producer. + */ + retval = clk_prepare_enable(uap->clk); + if (retval) + return retval; + + uap->port.uartclk = clk_get_rate(uap->clk); + + /* Clear pending error and receive interrupts */ + writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS | + UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR); + + /* + * Save interrupts enable mask, and enable RX interrupts in case if + * the interrupt is used for NMI entry. + */ + uap->im = readw(uap->port.membase + UART011_IMSC); + writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC); + + if (dev_get_platdata(uap->port.dev)) { + struct amba_pl011_data *plat; + + plat = dev_get_platdata(uap->port.dev); + if (plat->init) + plat->init(); + } + return 0; +} + #ifdef CONFIG_CONSOLE_POLL +static int pl011_poll_init(struct uart_port *port) +{ + struct uart_amba_port *uap = (struct uart_amba_port *)port; + int retval; + + retval = pl011_hwinit(port); + +#ifdef CONFIG_KGDB_FIQ + if (retval == 0) + kgdb_register_fiq(uap->port.irq); +#endif + + return retval; +} + static void pl011_quiesce_irqs(struct uart_port *port) { struct uart_amba_port *uap = (struct uart_amba_port *)port; @@ -1471,44 +1525,6 @@ static void pl011_put_poll_char(struct uart_port *port, #endif /* CONFIG_CONSOLE_POLL */ -static int pl011_hwinit(struct uart_port *port) -{ - struct uart_amba_port *uap = (struct uart_amba_port *)port; - int retval; - - /* Optionaly enable pins to be muxed in and configured */ - pinctrl_pm_select_default_state(port->dev); - - /* - * Try to enable the clock producer. - */ - retval = clk_prepare_enable(uap->clk); - if (retval) - return retval; - - uap->port.uartclk = clk_get_rate(uap->clk); - - /* Clear pending error and receive interrupts */ - writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS | - UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR); - - /* - * Save interrupts enable mask, and enable RX interrupts in case if - * the interrupt is used for NMI entry. - */ - uap->im = readw(uap->port.membase + UART011_IMSC); - writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC); - - if (dev_get_platdata(uap->port.dev)) { - struct amba_pl011_data *plat; - - plat = dev_get_platdata(uap->port.dev); - if (plat->init) - plat->init(); - } - return 0; -} - static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h) { writew(lcr_h, uap->port.membase + uap->lcrh_rx); @@ -1888,7 +1904,7 @@ static struct uart_ops amba_pl011_pops = { .config_port = pl011_config_port, .verify_port = pl011_verify_port, #ifdef CONFIG_CONSOLE_POLL - .poll_init = pl011_hwinit, + .poll_init = pl011_poll_init, .poll_get_char = pl011_get_poll_char, .poll_put_char = pl011_put_poll_char, #endif
Speculatively register a FIQ resource with KGDB. KGDB will only accept it if the kgdb/fiq feature is enabled (both with compile time and runtime switches) and the interrupt controller supports FIQ. By providing this information to KGDB the serial driver offers "permission" for KGDB to route the UART interrupt signal from the drivers own handler to KGDBs FIQ handler (which will eventually use the UART's polled I/O callbacks to interact with the user). This permission also implies the amba-pl011 driver has already unmasked RX interrupts (otherwise the FIQ handler will never trigger). Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jiri Slaby <jslaby@suse.cz> Cc: linux-serial@vger.kernel.org --- drivers/tty/serial/amba-pl011.c | 94 ++++++++++++++++++++++++----------------- 1 file changed, 55 insertions(+), 39 deletions(-)