| Message ID | 20180727210916.66642-5-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | serial: sh-sci: Clean up previous RZ/A2 support | expand |
Hi Chris, On Fri, Jul 27, 2018 at 11:09 PM Chris Brandt <chris.brandt@renesas.com> wrote: > Some SCIF versions mux error and break interrupts together and then provide > a separate interrupt ID for just TEI/DRI. > > Allow all 6 types of interrupts to be specified via platform data (or DT) > and for any signals that are muxed together (have the same interrupt > number) simply register one handler. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Thanks for your patch! > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -2842,17 +2832,17 @@ static int sci_init_single(struct platform_device *dev, > > /* The SCI generates several interrupts. They can be muxed together or > * connected to different interrupt lines. In the muxed case only one > - * interrupt resource is specified. In the non-muxed case three or four > - * interrupt resources are specified, as the BRI interrupt is optional. > + * interrupt resource is specified as there is only one interrupt ID. > + * In the non-muxed case, up to 6 interrupt signals might be generated > + * from the SCI, however those signals might have their own individual > + * interrupt ID numbers, or muxed together with another interrupt. > */ > if (sci_port->irqs[0] < 0) > return -ENXIO; > > - if (sci_port->irqs[1] < 0) { > - sci_port->irqs[1] = sci_port->irqs[0]; > - sci_port->irqs[2] = sci_port->irqs[0]; > - sci_port->irqs[3] = sci_port->irqs[0]; > - } > + if (sci_port->irqs[1] < 0) > + for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++) Shouldn't the "- 1" be dropped? > + sci_port->irqs[i] = sci_port->irqs[0]; > > sci_port->params = sci_probe_regmap(p); > if (unlikely(sci_port->params == NULL)) With the above fixed: Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
Hi Geert, On Monday, July 30, 2018, Geert Uytterhoeven wrote: > > if (sci_port->irqs[0] < 0) > > return -ENXIO; > > > > - if (sci_port->irqs[1] < 0) { > > - sci_port->irqs[1] = sci_port->irqs[0]; > > - sci_port->irqs[2] = sci_port->irqs[0]; > > - sci_port->irqs[3] = sci_port->irqs[0]; > > - } > > + if (sci_port->irqs[1] < 0) > > + for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++) > > Shouldn't the "- 1" be dropped? In reality, the last entry of the array is 'SCIx_NR_IRQS', so it's not really used anywhere. The original array was: enum { SCIx_ERI_IRQ, (the only IRQ specified in DT) SCIx_RXI_IRQ, << copy from irqs[0] SCIx_TXI_IRQ, << copy from irqs[0] SCIx_BRI_IRQ, << copy from irqs[0] SCIx_NR_IRQS, (didn’t' touch) SCIx_MUX_IRQ = SCIx_NR_IRQS, /* special case */ }; So the new for loop used "- "1 in order to create the same outcome. But as far as I can tell irqs[SCIx_NR_IRQS] is never used anywhere, it doesn't really matter. > With the above fixed: > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> I have no problem taking the "- 1" out. But...here's the funny part: It was you that suggested the "- 1" ;) On Thursday, July 26, 2018, Geert Uytterhoeven wrote: > > @@ -2809,6 +2845,8 @@ static int sci_init_single(struct platform_device > *dev, > > sci_port->irqs[1] = sci_port->irqs[0]; > > sci_port->irqs[2] = sci_port->irqs[0]; > > sci_port->irqs[3] = sci_port->irqs[0]; > > + sci_port->irqs[4] = sci_port->irqs[0]; > > + sci_port->irqs[5] = sci_port->irqs[0]; > > You may want to start using a loop from 1 to ARRAY_SIZE(sci_port->irqs) - 1 > instead. Cheers Chris
Hi Chris, On Mon, Jul 30, 2018 at 2:33 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Monday, July 30, 2018, Geert Uytterhoeven wrote: > > > if (sci_port->irqs[0] < 0) > > > return -ENXIO; > > > > > > - if (sci_port->irqs[1] < 0) { > > > - sci_port->irqs[1] = sci_port->irqs[0]; > > > - sci_port->irqs[2] = sci_port->irqs[0]; > > > - sci_port->irqs[3] = sci_port->irqs[0]; > > > - } > > > + if (sci_port->irqs[1] < 0) > > > + for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++) > > > > Shouldn't the "- 1" be dropped? > > In reality, the last entry of the array is 'SCIx_NR_IRQS', so it's not > really used anywhere. > > The original array was: > enum { > SCIx_ERI_IRQ, (the only IRQ specified in DT) =0 > SCIx_RXI_IRQ, << copy from irqs[0] = 1 > SCIx_TXI_IRQ, << copy from irqs[0] = 2 > SCIx_BRI_IRQ, << copy from irqs[0] = 3 > SCIx_NR_IRQS, (didn’t' touch) = 4 > > SCIx_MUX_IRQ = SCIx_NR_IRQS, /* special case */ > }; That's not the array, but the enum. The array is in struct sci_port: int irqs[SCIx_NR_IRQS]; It has four entries, at indices 0..3. > So the new for loop used "- "1 in order to create the same outcome. > > But as far as I can tell irqs[SCIx_NR_IRQS] is never used anywhere, it > doesn't really matter. irqs[SCIx_NR_IRQS] does not exist! sci_irq_desc[SCIx_NR_IRQS] aka sci_irq_desc[SCIx_MUX_IRQ] does exit, but that's a different array. > > With the above fixed: > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > I have no problem taking the "- 1" out. > > But...here's the funny part: It was you that suggested the "- 1" ;) > > On Thursday, July 26, 2018, Geert Uytterhoeven wrote: > > > @@ -2809,6 +2845,8 @@ static int sci_init_single(struct platform_device > > *dev, > > > sci_port->irqs[1] = sci_port->irqs[0]; > > > sci_port->irqs[2] = sci_port->irqs[0]; > > > sci_port->irqs[3] = sci_port->irqs[0]; > > > + sci_port->irqs[4] = sci_port->irqs[0]; > > > + sci_port->irqs[5] = sci_port->irqs[0]; > > > > You may want to start using a loop from 1 to ARRAY_SIZE(sci_port->irqs) - 1 > > instead. Your loop is: for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++) It loops over 1..ARRAY_SIZE(sci_port->irqs) - 2. Note the "<" and the "- 1". Gr{oetje,eeting}s, Geert
Hi Geert, On Monday, July 30, 2018, Geert Uytterhoeven wrote: > That's not the array, but the enum. > > The array is in struct sci_port: > > int irqs[SCIx_NR_IRQS]; > > It has four entries, at indices 0..3. > irqs[SCIx_NR_IRQS] does not exist! > > sci_irq_desc[SCIx_NR_IRQS] aka sci_irq_desc[SCIx_MUX_IRQ] does exit, > but that's a different array. > Your loop is: > > for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++) > > It loops over 1..ARRAY_SIZE(sci_port->irqs) - 2. > Note the "<" and the "- 1". Ahhhh, you're right! Sorry about that. Thanks, Chris
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index cb4f7d3e7192..a3519e8e4d13 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -65,7 +65,8 @@ enum { SCIx_RXI_IRQ, SCIx_TXI_IRQ, SCIx_BRI_IRQ, - SCIx_TEIDRI_IRQ, + SCIx_DRI_IRQ, + SCIx_TEI_IRQ, SCIx_NR_IRQS, SCIx_MUX_IRQ = SCIx_NR_IRQS, /* special case */ @@ -77,9 +78,6 @@ enum { ((port)->irqs[SCIx_ERI_IRQ] && \ ((port)->irqs[SCIx_RXI_IRQ] < 0)) -#define SCIx_TEIDRI_IRQ_EXISTS(port) \ - ((port)->irqs[SCIx_TEIDRI_IRQ] > 0) - enum SCI_CLKS { SCI_FCK, /* Functional Clock */ SCI_SCK, /* Optional External Clock */ @@ -1685,14 +1683,23 @@ static irqreturn_t sci_tx_interrupt(int irq, void *ptr) return IRQ_HANDLED; } -static irqreturn_t sci_br_interrupt(int irq, void *ptr); +static irqreturn_t sci_br_interrupt(int irq, void *ptr) +{ + struct uart_port *port = ptr; + + /* Handle BREAKs */ + sci_handle_breaks(port); + sci_clear_SCxSR(port, SCxSR_BREAK_CLEAR(port)); + + return IRQ_HANDLED; +} static irqreturn_t sci_er_interrupt(int irq, void *ptr) { struct uart_port *port = ptr; struct sci_port *s = to_sci_port(port); - if (SCIx_TEIDRI_IRQ_EXISTS(s)) { + if (s->irqs[SCIx_ERI_IRQ] == s->irqs[SCIx_BRI_IRQ]) { /* Break and Error interrupts are muxed */ unsigned short ssr_status = serial_port_in(port, SCxSR); @@ -1727,17 +1734,6 @@ static irqreturn_t sci_er_interrupt(int irq, void *ptr) return IRQ_HANDLED; } -static irqreturn_t sci_br_interrupt(int irq, void *ptr) -{ - struct uart_port *port = ptr; - - /* Handle BREAKs */ - sci_handle_breaks(port); - sci_clear_SCxSR(port, SCxSR_BREAK_CLEAR(port)); - - return IRQ_HANDLED; -} - static irqreturn_t sci_mpxed_interrupt(int irq, void *ptr) { unsigned short ssr_status, scr_status, err_enabled, orer_status = 0; @@ -1811,6 +1807,16 @@ static const struct sci_irq_desc { .handler = sci_br_interrupt, }, + [SCIx_DRI_IRQ] = { + .desc = "rx ready", + .handler = sci_rx_interrupt, + }, + + [SCIx_TEI_IRQ] = { + .desc = "tx end", + .handler = sci_tx_interrupt, + }, + /* * Special muxed handler. */ @@ -1823,12 +1829,19 @@ static const struct sci_irq_desc { static int sci_request_irq(struct sci_port *port) { struct uart_port *up = &port->port; - int i, j, ret = 0; + int i, j, w, ret = 0; for (i = j = 0; i < SCIx_NR_IRQS; i++, j++) { const struct sci_irq_desc *desc; int irq; + /* Check if already registered (muxed) */ + for (w = 0; w < i; w++) + if (port->irqs[w] == port->irqs[i]) + w = i + 1; + if (w > i) + continue; + if (SCIx_IRQ_IS_MUXED(port)) { i = SCIx_MUX_IRQ; irq = up->irq; @@ -1845,31 +1858,8 @@ static int sci_request_irq(struct sci_port *port) desc = sci_irq_desc + i; port->irqstr[j] = NULL; - if (SCIx_TEIDRI_IRQ_EXISTS(port)) { - /* - * ERI and BRI are muxed, just register ERI and - * ignore BRI. - * TEI and DRI are muxed, but only DRI - * is enabled, so use RXI handler - */ - if (i == SCIx_ERI_IRQ) - port->irqstr[j] = kasprintf(GFP_KERNEL, - "%s:err + break", - dev_name(up->dev)); - if (i == SCIx_BRI_IRQ) - continue; - if (i == SCIx_TEIDRI_IRQ) { - port->irqstr[j] = kasprintf(GFP_KERNEL, - "%s:tx end + rx ready", - dev_name(up->dev)); - desc = sci_irq_desc + SCIx_RXI_IRQ; - } - } - - if (!port->irqstr[j]) - port->irqstr[j] = kasprintf(GFP_KERNEL, "%s:%s", - dev_name(up->dev), - desc->desc); + port->irqstr[j] = kasprintf(GFP_KERNEL, "%s:%s", + dev_name(up->dev), desc->desc); if (!port->irqstr[j]) { ret = -ENOMEM; goto out_nomem; @@ -2842,17 +2832,17 @@ static int sci_init_single(struct platform_device *dev, /* The SCI generates several interrupts. They can be muxed together or * connected to different interrupt lines. In the muxed case only one - * interrupt resource is specified. In the non-muxed case three or four - * interrupt resources are specified, as the BRI interrupt is optional. + * interrupt resource is specified as there is only one interrupt ID. + * In the non-muxed case, up to 6 interrupt signals might be generated + * from the SCI, however those signals might have their own individual + * interrupt ID numbers, or muxed together with another interrupt. */ if (sci_port->irqs[0] < 0) return -ENXIO; - if (sci_port->irqs[1] < 0) { - sci_port->irqs[1] = sci_port->irqs[0]; - sci_port->irqs[2] = sci_port->irqs[0]; - sci_port->irqs[3] = sci_port->irqs[0]; - } + if (sci_port->irqs[1] < 0) + for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++) + sci_port->irqs[i] = sci_port->irqs[0]; sci_port->params = sci_probe_regmap(p); if (unlikely(sci_port->params == NULL))
Some SCIF versions mux error and break interrupts together and then provide a separate interrupt ID for just TEI/DRI. Allow all 6 types of interrupts to be specified via platform data (or DT) and for any signals that are muxed together (have the same interrupt number) simply register one handler. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- drivers/tty/serial/sh-sci.c | 90 ++++++++++++++++++++------------------------- 1 file changed, 40 insertions(+), 50 deletions(-)