| Message ID | 1594290978-8205-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | phy: renesas: rcar-gen3-usb2: fix SError happen if DEBUG_SHIRQ is enabled | expand |
Hi Yoshihiro, On 09-07-20, 19:36, Yoshihiro Shimoda wrote: > If CONFIG_DEBUG_SHIRQ was enabled, r8a77951-salvator-xs could boot > correctly. If we appended "earlycon keep_bootcon" to the kernel > command like, we could get kernel log like below. > > SError Interrupt on CPU0, code 0xbf000002 -- SError > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc3-salvator-x-00505-g6c843129e6faaf01 #785 > Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT) > pstate: 60400085 (nZCv daIf +PAN -UAO BTYPE=--) > pc : rcar_gen3_phy_usb2_irq+0x14/0x54 > lr : free_irq+0xf4/0x27c > > This means free_irq() calls the interrupt handler while PM runtime > is not getting if DEBUG_SHIRQ is enabled and rcar_gen3_phy_usb2_probe() > failed. To fix the issue, add a condition into the interrupt > handler to avoid register access if any phys are not initialized. > > Note that rcar_gen3_is_any_rphy_initialized() was introduced on v5.2. > So, if we backports this patch to v5.1 or less, we need to make > other way. Should we really check phy is initialized? I think the issue here is that you register irq first, so your handler can be invoked. Right fix for this would be to move the irq registration to later in the probe when we are ready to handle interrupts > > Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Fixes: 9f391c574efc ("phy: rcar-gen3-usb2: add runtime ID/VBUS pin detection") > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > index bfb22f8..91c732d 100644 > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > @@ -507,9 +507,13 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch) > { > struct rcar_gen3_chan *ch = _ch; > void __iomem *usb2_base = ch->base; > - u32 status = readl(usb2_base + USB2_OBINTSTA); > + u32 status; > irqreturn_t ret = IRQ_NONE; > > + if (!rcar_gen3_is_any_rphy_initialized(ch)) > + return ret; > + > + status = readl(usb2_base + USB2_OBINTSTA); > if (status & USB2_OBINT_BITS) { > dev_vdbg(ch->dev, "%s: %08x\n", __func__, status); > writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA); > -- > 2.7.4
Hi Vinod, > From: Vinod Koul, Sent: Monday, July 13, 2020 2:17 PM > > Hi Yoshihiro, > > On 09-07-20, 19:36, Yoshihiro Shimoda wrote: > > If CONFIG_DEBUG_SHIRQ was enabled, r8a77951-salvator-xs could boot > > correctly. If we appended "earlycon keep_bootcon" to the kernel > > command like, we could get kernel log like below. > > > > SError Interrupt on CPU0, code 0xbf000002 -- SError > > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc3-salvator-x-00505-g6c843129e6faaf01 #785 > > Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT) > > pstate: 60400085 (nZCv daIf +PAN -UAO BTYPE=--) > > pc : rcar_gen3_phy_usb2_irq+0x14/0x54 > > lr : free_irq+0xf4/0x27c > > > > This means free_irq() calls the interrupt handler while PM runtime > > is not getting if DEBUG_SHIRQ is enabled and rcar_gen3_phy_usb2_probe() > > failed. To fix the issue, add a condition into the interrupt > > handler to avoid register access if any phys are not initialized. > > > > Note that rcar_gen3_is_any_rphy_initialized() was introduced on v5.2. > > So, if we backports this patch to v5.1 or less, we need to make > > other way. > > Should we really check phy is initialized? I think the issue here is > that you register irq first, so your handler can be invoked. Right fix > for this would be to move the irq registration to later in the probe > when we are ready to handle interrupts Thank you for your review! I think we need to check whether phy is initialized even if the irq registration moves to rcar_gen3_phy_usb2_init() because the current driver will have multiple phy instances. However, moving the irq registration can be easy to backport than this patch, I think. So, I'll modify this patch as you suggested. Best regards, Yoshihiro Shiomda
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c index bfb22f8..91c732d 100644 --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c @@ -507,9 +507,13 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch) { struct rcar_gen3_chan *ch = _ch; void __iomem *usb2_base = ch->base; - u32 status = readl(usb2_base + USB2_OBINTSTA); + u32 status; irqreturn_t ret = IRQ_NONE; + if (!rcar_gen3_is_any_rphy_initialized(ch)) + return ret; + + status = readl(usb2_base + USB2_OBINTSTA); if (status & USB2_OBINT_BITS) { dev_vdbg(ch->dev, "%s: %08x\n", __func__, status); writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA);
If CONFIG_DEBUG_SHIRQ was enabled, r8a77951-salvator-xs could boot correctly. If we appended "earlycon keep_bootcon" to the kernel command like, we could get kernel log like below. SError Interrupt on CPU0, code 0xbf000002 -- SError CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc3-salvator-x-00505-g6c843129e6faaf01 #785 Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT) pstate: 60400085 (nZCv daIf +PAN -UAO BTYPE=--) pc : rcar_gen3_phy_usb2_irq+0x14/0x54 lr : free_irq+0xf4/0x27c This means free_irq() calls the interrupt handler while PM runtime is not getting if DEBUG_SHIRQ is enabled and rcar_gen3_phy_usb2_probe() failed. To fix the issue, add a condition into the interrupt handler to avoid register access if any phys are not initialized. Note that rcar_gen3_is_any_rphy_initialized() was introduced on v5.2. So, if we backports this patch to v5.1 or less, we need to make other way. Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> Fixes: 9f391c574efc ("phy: rcar-gen3-usb2: add runtime ID/VBUS pin detection") Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/phy/renesas/phy-rcar-gen3-usb2.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)