From: Bing Fan <[email protected]>
In order to make pl011 work better, multiple interrupts are
required, such as TXIM, RXIM, RTIM, error interrupt(FE/PE/BE/OE);
at the same time, pl011 to GIC does not merge the interrupt
lines(each serial-interrupt corresponding to different GIC hardware
interrupt), so need to enable and request multiple gic interrupt
numbers in the driver.
Signed-off-by: Bing Fan <[email protected]>
---
drivers/tty/serial/amba-pl011.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 78682c12156a..b63164e89903 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1703,9 +1703,30 @@ static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h)
static int pl011_allocate_irq(struct uart_amba_port *uap)
{
+ int ret = -1;
+ int i = 0;
+ unsigned int virq = 0;
+ struct amba_device *amba_dev = (struct amba_device *)uap->port.dev;
+
+ if (!amba_dev)
+ return -1;
+
pl011_write(uap->im, uap, REG_IMSC);
- return request_irq(uap->port.irq, pl011_int, IRQF_SHARED, "uart-pl011", uap);
+ for (i = 0; i < AMBA_NR_IRQS; i++) {
+ virq = amba_dev->irq[i];
+ if (virq == 0)
+ break;
+
+ ret = request_irq(virq, pl011_int, IRQF_SHARED, "uart-pl011-*", uap);
+ if (ret < 0) {
+ dev_info(uap->port.dev, "%s %d request %u interrupt failed\n",
+ __func__, __LINE__, virq);
+ break;
+ }
+ }
+
+ return ret;
}
/*
--
2.17.1
On Tue, Jun 29, 2021 at 09:29:24AM +0800, Bing Fan wrote:
> From: Bing Fan <[email protected]>
>
> In order to make pl011 work better, multiple interrupts are
> required, such as TXIM, RXIM, RTIM, error interrupt(FE/PE/BE/OE);
> at the same time, pl011 to GIC does not merge the interrupt
> lines(each serial-interrupt corresponding to different GIC hardware
> interrupt), so need to enable and request multiple gic interrupt
> numbers in the driver.
>
> Signed-off-by: Bing Fan <[email protected]>
> ---
> drivers/tty/serial/amba-pl011.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 78682c12156a..b63164e89903 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1703,9 +1703,30 @@ static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h)
>
> static int pl011_allocate_irq(struct uart_amba_port *uap)
> {
> + int ret = -1;
Pick a real error value.
> + int i = 0;
Why is this initialized?
> + unsigned int virq = 0;
Why is this initialized?
> + struct amba_device *amba_dev = (struct amba_device *)uap->port.dev;
Are you sure you can just cast this like this? Did you test this?
> +
> + if (!amba_dev)
> + return -1;
Do not make up error numbers, return a specific -ERR* value.
And how can this happen?
> +
> pl011_write(uap->im, uap, REG_IMSC);
>
> - return request_irq(uap->port.irq, pl011_int, IRQF_SHARED, "uart-pl011", uap);
> + for (i = 0; i < AMBA_NR_IRQS; i++) {
> + virq = amba_dev->irq[i];
> + if (virq == 0)
> + break;
> +
> + ret = request_irq(virq, pl011_int, IRQF_SHARED, "uart-pl011-*", uap);
> + if (ret < 0) {
> + dev_info(uap->port.dev, "%s %d request %u interrupt failed\n",
> + __func__, __LINE__, virq);
dev_err() is for errors, not dev_info().
And no need for __func__ and __LINE__ for the dev_* calls.
> + break;
> + }
> + }
> +
> + return ret;
> }
>
> /*
> --
> 2.17.1
>
On Tue, Jun 29, 2021 at 07:32:36PM +0800, Bing Fan wrote:
> hello,
>
> replied as below.
>
> and new patch is at the bottom.
Please submit this properly as the documentation says to do so, I can't
take an attachment :(
> > > + struct amba_device *amba_dev = (struct amba_device *)uap->port.dev;
> > Are you sure you can just cast this like this? Did you test this?
>
>
> Yes, i have tested and applied in my project.
>
> The function pl011_probe calls pl011_setup_port with &amba_dev->dev and uap
> params;
>
> and pl011_setup_port set uap->port.dev to the address of amba_dev->dev;
>
> the two structs' relationship is:
>
> struct amba_device {
>
> struct device dev;
>
> ……
>
> };
>
> When pointer(uap->port.dev) points to amba_dev->dev address, the momery
> actully stores
>
> content of struct amba_device; so the cast assignment can be forced to
> amba_dev.
That is now how this should work, use the correct container_of() cast
instead. That will always work no matter where struct device is in the
structure. You got lucky here :)
> > > +
> > > + if (!amba_dev)
> > > + return -1;
> > Do not make up error numbers, return a specific -ERR* value.
>
> changed to "return -ENODEV"
So this changed the logic of this function, is that ok?
> >
> > And how can this happen?
>
> The function pl011_setup_port isn't called, event pl011_probe isn't called.
And how can that ever happen?
thanks,
greg k-h
On Tue, Jun 29, 2021 at 08:31:00PM +0800, Bing Fan wrote:
> hello,
>
>
> 在 6/29/2021 20:18, Greg KH 写道:
> > On Tue, Jun 29, 2021 at 07:32:36PM +0800, Bing Fan wrote:
> > > hello, replied as below. and new patch is at the bottom.
> > Please submit this properly as the documentation says to do so, I can't
> > take an attachment :(
> Ok.
> > > > > + struct amba_device *amba_dev = (struct amba_device *)uap->port.dev;
> > > > Are you sure you can just cast this like this? Did you test this?
> > > Yes, i have tested and applied in my project. The function
> > > pl011_probe calls pl011_setup_port with &amba_dev->dev and uap
> > > params; and pl011_setup_port set uap->port.dev to the address of
> > > amba_dev->dev; the two structs' relationship is: struct
> > > amba_device { struct device dev; …… }; When
> > > pointer(uap->port.dev) points to amba_dev->dev address, the momery
> > > actully stores content of struct amba_device; so the cast assignment
> > > can be forced to amba_dev.
> > That is now how this should work, use the correct container_of() cast
> > instead. That will always work no matter where struct device is in the
> > structure. You got lucky here :)
>
> changed to "struct amba_device *amba_dev = container_of(uap->port.dev, struct amba_device, dev);"
>
>
> > > > > + + if (!amba_dev) + return -1;
> > > > Do not make up error numbers, return a specific -ERR* value.
> > > changed to "return -ENODEV"
> > So this changed the logic of this function, is that ok?
>
> No, just sanity check.
If it can never happen, no need to check for it.
> > > > And how can this happen?
> > > The function pl011_setup_port isn't called, event pl011_probe isn't
> > > called.
> > And how can that ever happen?
>
> If there is no pl011 device.
How can that happen here?
thanks,
greg k-h