| Message ID | 20160628031139.12500-2-sam@mendozajonas.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, 2016-06-28 at 13:11 +1000, Samuel Mendoza-Jonas wrote: > Update the hvc driver to use the OPAL irqchip if made available by the > running firmware. If it is not present, the driver falls back to the > existing OPAL event number. One thing that worries me a bit with the original transition to using an interrupt from the old OPAL callback is that when passed an interrupt, the HVC thread assumes interrupts work reliably and thus stops polling. However, not all platforms have a functional serial interrupt. For example rhesus doesn't. In fact we don't always know when we build the device-tree whether the serial interrupt will work or not. Now we might be saved by the OPAL heartbeat ... we do call opal_poll_events regularily there. But I'd like you to verify it by disabling the LPC interrupt for example on an openpower machine and see how the console beahves. Cheers, Ben. > Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com> > Cc: <stable@vger.kernel.org> # 4.1.x- > --- > drivers/tty/hvc/hvc_opal.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c > index b7cd0ae..8c53f5b 100644 > --- a/drivers/tty/hvc/hvc_opal.c > +++ b/drivers/tty/hvc/hvc_opal.c > @@ -170,6 +170,8 @@ static int hvc_opal_probe(struct platform_device *dev) > hv_protocol_t proto; > unsigned int termno, irq, boot = 0; > const __be32 *reg; > + u32 prop; > + int rc; > > if (of_device_is_compatible(dev->dev.of_node, "ibm,opal-console-raw")) { > proto = HV_PROTOCOL_RAW; > @@ -214,7 +216,15 @@ static int hvc_opal_probe(struct platform_device *dev) > dev->dev.of_node->full_name, > boot ? " (boot console)" : ""); > > - irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); > + rc = of_property_read_u32(dev->dev.of_node, "interrupts", &prop); > + if (rc) { > + pr_info("hvc%d: No interrupts property, using OPAL event\n", > + termno); > + irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); > + } else { > + irq = irq_of_parse_and_map(dev->dev.of_node, 0); > + } > + > if (!irq) { > pr_err("hvc_opal: Unable to map interrupt for device %s\n", > dev->dev.of_node->full_name);
On Tue, 2016-06-28 at 13:34 +1000, Benjamin Herrenschmidt wrote: > On Tue, 2016-06-28 at 13:11 +1000, Samuel Mendoza-Jonas wrote: > > Update the hvc driver to use the OPAL irqchip if made available by > > the > > running firmware. If it is not present, the driver falls back to > > the > > existing OPAL event number. > > One thing that worries me a bit with the original transition to using > an interrupt from the old OPAL callback is that when passed an > interrupt, > the HVC thread assumes interrupts work reliably and thus stops > polling. Note to Greg: This patch is fine, this is a reflexion about a change that was already done. > However, not all platforms have a functional serial interrupt. For > example rhesus doesn't. In fact we don't always know when we build > the device-tree whether the serial interrupt will work or not. > > Now we might be saved by the OPAL heartbeat ... we do call > opal_poll_events regularily there. But I'd like you to verify it > by disabling the LPC interrupt for example on an openpower machine > and see how the console beahves. > > Cheers, > Ben. > > > > Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com> > > Cc: <stable@vger.kernel.org> # 4.1.x- > > --- > > drivers/tty/hvc/hvc_opal.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/tty/hvc/hvc_opal.c > > b/drivers/tty/hvc/hvc_opal.c > > index b7cd0ae..8c53f5b 100644 > > --- a/drivers/tty/hvc/hvc_opal.c > > +++ b/drivers/tty/hvc/hvc_opal.c > > @@ -170,6 +170,8 @@ static int hvc_opal_probe(struct > > platform_device *dev) > > hv_protocol_t proto; > > unsigned int termno, irq, boot = 0; > > const __be32 *reg; > > + u32 prop; > > + int rc; > > > > if (of_device_is_compatible(dev->dev.of_node, "ibm,opal- > > console-raw")) { > > proto = HV_PROTOCOL_RAW; > > @@ -214,7 +216,15 @@ static int hvc_opal_probe(struct > > platform_device *dev) > > dev->dev.of_node->full_name, > > boot ? " (boot console)" : ""); > > > > - irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); > > + rc = of_property_read_u32(dev->dev.of_node, "interrupts", > > &prop); > > + if (rc) { > > + pr_info("hvc%d: No interrupts property, using OPAL > > event\n", > > + termno); > > + irq = > > opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); > > + } else { > > + irq = irq_of_parse_and_map(dev->dev.of_node, 0); > > + } > > + > > if (!irq) { > > pr_err("hvc_opal: Unable to map interrupt for > > device %s\n", > > dev->dev.of_node->full_name);
On Tue, 2016-28-06 at 03:11:39 UTC, Sam Mendoza-Jonas wrote: > diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c > index b7cd0ae..8c53f5b 100644 > --- a/drivers/tty/hvc/hvc_opal.c > +++ b/drivers/tty/hvc/hvc_opal.c > @@ -214,7 +216,15 @@ static int hvc_opal_probe(struct platform_device *dev) > dev->dev.of_node->full_name, > boot ? " (boot console)" : ""); > > - irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); > + rc = of_property_read_u32(dev->dev.of_node, "interrupts", &prop); > + if (rc) { > + pr_info("hvc%d: No interrupts property, using OPAL event\n", > + termno); > + irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); > + } else { > + irq = irq_of_parse_and_map(dev->dev.of_node, 0); > + } That seems a bit backward. Shouldn't we try irq_of_parse_and_map() and if that fails, then we go back to opal_event_request() ? cheers
On Tue, 2016-07-05 at 15:31 +1000, Michael Ellerman wrote: > On Tue, 2016-28-06 at 03:11:39 UTC, Sam Mendoza-Jonas wrote: > > diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c > > index b7cd0ae..8c53f5b 100644 > > --- a/drivers/tty/hvc/hvc_opal.c > > +++ b/drivers/tty/hvc/hvc_opal.c > > @@ -214,7 +216,15 @@ static int hvc_opal_probe(struct platform_device *dev) > > dev->dev.of_node->full_name, > > boot ? " (boot console)" : ""); > > > > - irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); > > + rc = of_property_read_u32(dev->dev.of_node, "interrupts", &prop); > > + if (rc) { > > + pr_info("hvc%d: No interrupts property, using OPAL event\n", > > + termno); > > + irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); > > + } else { > > + irq = irq_of_parse_and_map(dev->dev.of_node, 0); > > + } > > That seems a bit backward. > > Shouldn't we try irq_of_parse_and_map() and if that fails, then we go back to > opal_event_request() ? > > cheers I was unsure for a minute so I double checked this; of_property_read_u32() returns of_property_read_u32_array(), which returns 0 on success, unlike a bunch of other of_property helpers. So if the 'interrupts' property does exist we get 0, and try irq_of_parse_and_map(). But are you suggesting we try irq_of_parse_and_map() regardless and then fall back to opal_event_request()? Cheers, Sam
Samuel Mendoza-Jonas <sam@mendozajonas.com> writes: > On Tue, 2016-07-05 at 15:31 +1000, Michael Ellerman wrote: >> On Tue, 2016-28-06 at 03:11:39 UTC, Sam Mendoza-Jonas wrote: >> > diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c >> > index b7cd0ae..8c53f5b 100644 >> > --- a/drivers/tty/hvc/hvc_opal.c >> > +++ b/drivers/tty/hvc/hvc_opal.c >> > @@ -214,7 +216,15 @@ static int hvc_opal_probe(struct platform_device *dev) >> > dev->dev.of_node->full_name, >> > boot ? " (boot console)" : ""); >> > >> > - irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); >> > + rc = of_property_read_u32(dev->dev.of_node, "interrupts", &prop); >> > + if (rc) { >> > + pr_info("hvc%d: No interrupts property, using OPAL event\n", >> > + termno); >> > + irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); >> > + } else { >> > + irq = irq_of_parse_and_map(dev->dev.of_node, 0); >> > + } >> >> That seems a bit backward. >> >> Shouldn't we try irq_of_parse_and_map() and if that fails, then we go back to >> opal_event_request() ? > But are you suggesting we try irq_of_parse_and_map() regardless and > then fall back to opal_event_request()? Yes. cheers
diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c index b7cd0ae..8c53f5b 100644 --- a/drivers/tty/hvc/hvc_opal.c +++ b/drivers/tty/hvc/hvc_opal.c @@ -170,6 +170,8 @@ static int hvc_opal_probe(struct platform_device *dev) hv_protocol_t proto; unsigned int termno, irq, boot = 0; const __be32 *reg; + u32 prop; + int rc; if (of_device_is_compatible(dev->dev.of_node, "ibm,opal-console-raw")) { proto = HV_PROTOCOL_RAW; @@ -214,7 +216,15 @@ static int hvc_opal_probe(struct platform_device *dev) dev->dev.of_node->full_name, boot ? " (boot console)" : ""); - irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); + rc = of_property_read_u32(dev->dev.of_node, "interrupts", &prop); + if (rc) { + pr_info("hvc%d: No interrupts property, using OPAL event\n", + termno); + irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT)); + } else { + irq = irq_of_parse_and_map(dev->dev.of_node, 0); + } + if (!irq) { pr_err("hvc_opal: Unable to map interrupt for device %s\n", dev->dev.of_node->full_name);
Update the hvc driver to use the OPAL irqchip if made available by the running firmware. If it is not present, the driver falls back to the existing OPAL event number. Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com> Cc: <stable@vger.kernel.org> # 4.1.x- --- drivers/tty/hvc/hvc_opal.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)